allow updating text fields on assessed outcomes via api

test plan:
* create a learning outcome
* assess the learning outcome for a student
* before, this would have meant that no changes could be made
 to the outcome through the update api, but now changes
 should be allowed as long as they only affect text fields:

 - title
 - display_name
 - description
 - ratings[][description]

* any other changes should cause an error message

closes #CNVS-36951

Change-Id: I043af8fb0bfc427970a0723db9741a5fa93c5617
Reviewed-on: https://gerrit.instructure.com/112594
Tested-by: Jenkins
Reviewed-by: Michael Brewer-Davis <mbd@instructure.com>
Reviewed-by: Matt Berns <mberns@instructure.com>
QA-Review: Deepeeca Soundarrajan <dsoundarrajan@instructure.com>
Product-Review: McCall Smith <mcsmith@instructure.com>

app/controllers/outcomes_api_controller.rb

@@ -221,17 +221,8 @@ class OutcomesApiController < ApplicationController
   def update
     return unless authorized_action(@outcome, @current_user, :update)
 
-    if @outcome.assessed?
-      return render(
-        :json => assessed_outcome_error_message,
-        :status => :bad_request
-      )
-    end
-
-    @outcome.update_attributes(params.permit(*DIRECT_PARAMS))
     update_outcome_criterion(@outcome) if params[:mastery_points] || params[:ratings]
-
-    if @outcome.save
+    if @outcome.update_attributes(params.permit(*DIRECT_PARAMS))
       render :json => outcome_json(@outcome, @current_user, session)
     else
       render :json => @outcome.errors, :status => :bad_request
@@ -258,11 +249,6 @@ class OutcomesApiController < ApplicationController
     outcome.rubric_criterion = criterion
   end
 
-  def assessed_outcome_error_message
-    message = t("This outcome has been used to assess a student and can no longer be updated")
-    { errors: message }
-  end
-
   # Direct params are those that have a direct correlation to attrs in the model
   DIRECT_PARAMS = %w[title display_name description vendor_guid calculation_method calculation_int].freeze
 end

app/models/learning_outcome.rb

@@ -53,6 +53,7 @@ class LearningOutcome < ActiveRecord::Base
   validates :short_description, :workflow_state, presence: true
   sanitize_field :description, CanvasSanitize::SANITIZE
   validate :validate_calculation_int
+  validate :validate_text_only_changes_when_assessed
 
   set_policy do
     # managing a contextual outcome requires manage_outcomes on the outcome's context
@@ -102,6 +103,33 @@ class LearningOutcome < ActiveRecord::Base
     end
   end
 
+  def validate_text_only_changes_when_assessed
+    if persisted? && assessed?
+      if criterion_non_text_fields_changed? || (self.changes.keys - %w{data description short_description display_name}).any?
+        self.errors.add(:base, t("This outcome has been used to assess a student. Only text fields can be updated"))
+      end
+    end
+  end
+
+  def criterion_non_text_fields_changed?
+    return false unless self.data_changed?
+    old_criterion = (self.data_was && self.data_was[:rubric_criterion]) || {}
+    return true if self.rubric_criterion.symbolize_keys.except(:description, :ratings) != old_criterion.symbolize_keys.except(:description, :ratings)
+
+    new_ratings = self.rubric_criterion[:ratings] || []
+    old_ratings = old_criterion[:ratings] || []
+    return true if new_ratings.count != old_ratings.count
+
+    non_description_changed = false
+    new_ratings.each_with_index do |new_rating, idx|
+      if new_rating.symbolize_keys.except(:description) != old_ratings[idx].symbolize_keys.except(:description)
+        non_description_changed = true
+        break
+      end
+    end
+    return non_description_changed
+  end
+
   def self.valid_calculation_method?(method)
     CALCULATION_METHODS.keys.include?(method)
   end

gems/plugins/account_reports/spec_canvas/outcome_reports_spec.rb

@@ -178,8 +178,6 @@ describe "Outcome Reports" do
       outcome_group = sub_account.root_outcome_group
       @course1.account = sub_account
       @course1.save!
-      @outcome.context_id = sub_account.id
-      @outcome.save!
       outcome_group.add_outcome(@outcome)
 
       parsed = read_report(@type, {order: [0, 1], account: sub_account})

spec/apis/v1/outcomes_api_spec.rb

@@ -740,48 +740,65 @@ describe "Outcomes API", type: :request do
             expect(@outcome.calculation_int).to eq(62)
           end
 
-          %w[null not_null].each do |context_type|
-            context "rejecting parameters with #{context_type} data" do
-              def setup_for_context_type(ct)
-                case ct
-                when "null"
-                  @outcome.data = nil
-                when "not_null"
-                  @outcome.rubric_criterion = {
-                    description: "Thoughtful description",
-                    mastery_points: 5,
-                    ratings: [
-                      {
-                        description: "Strong work",
-                        points: 5
-                      },
-                      {
-                        description: "Weak sauce",
-                        points: 1
-                      }
-                    ],
-                  }
-                end
-                @outcome.save!
-              end
+          it "should allow updating text-only fields even when assessed" do
+            new_title = "some new title"
+            new_display_name = "some display name"
+            new_desc = "some new description or something"
+            json = api_call(:put, "/api/v1/outcomes/#{@outcome.id}",
+              { :controller => 'outcomes_api', :action => 'update',
+                :id => @outcome.id.to_s, :format => 'json' },
+              { :title => new_title, :description => new_desc, :display_name => new_display_name },
+              {}, { :expected_status => 200 })
+            @outcome.reload
+            expect(@outcome.title).to eq new_title
+            expect(@outcome.display_name).to eq new_display_name
+            expect(@outcome.description).to eq new_desc
+          end
 
-              before :each do
-                setup_for_context_type(context_type)
-              end
+          context "updating rubric criterion when assessed" do
+            before :each do
+              @outcome2 = @course.created_learning_outcomes.create!(:title => 'outcome')
+              @course.root_outcome_group.add_outcome(@outcome2)
+              @outcome2.rubric_criterion = {
+                mastery_points: 5,
+                ratings: [{ description: "Strong work", points: 5}, { description: "Weak sauce", points: 1}],
+              }
+              @outcome2.save!
+              assess_outcome(@outcome2)
+            end
 
-              it "rejects all parameters changed after being used for assessing" do
-                json = update_outcome_api.call(update_hash)
-                expect(json["errors"]).not_to be_nil
-                expect(json["errors"]).to match(/outcome.*assess/)
-              end
+            it "should allow updating rating descriptions even when assessed" do
+              new_ratings = [{ description: "some new desc1", points: 5 },
+                { description: "some new desc2", points: 1 }]
+              json = api_call(:put, "/api/v1/outcomes/#{@outcome2.id}",
+                { :controller => 'outcomes_api', :action => 'update',
+                  :id => @outcome2.id.to_s, :format => 'json' },
+                { :ratings => new_ratings },
+                {}, { :expected_status => 200 })
+              @outcome2.reload
+              expect(@outcome2.rubric_criterion[:ratings]).to eq new_ratings
+            end
 
-              %w[title display_name description vendor_guid calculation_method calculation_int mastery_points ratings].each do |attr|
-                it "rejects a change to #{attr} after being used to assess a student" do
-                  json = update_outcome_api.call({ attr => "Test value" })
-                  expect(json["errors"]).not_to be_nil
-                  expect(json["errors"]).to match(/outcome.*assess/)
-                end
-              end
+            it "should not allow updating rating points" do
+              new_ratings = [{ description: "some new desc1", points: 5 },
+                { description: "some new desc2", points: 3 }]
+              json = api_call(:put, "/api/v1/outcomes/#{@outcome2.id}",
+                { :controller => 'outcomes_api', :action => 'update',
+                  :id => @outcome2.id.to_s, :format => 'json' },
+                { :ratings => new_ratings },
+                {}, { :expected_status => 400 })
+              @outcome2.reload
+              expect(@outcome2.rubric_criterion[:ratings]).to_not eq new_ratings
+            end
+
+            it "should not allow updating mastery points" do
+              json = api_call(:put, "/api/v1/outcomes/#{@outcome2.id}",
+                { :controller => 'outcomes_api', :action => 'update',
+                  :id => @outcome2.id.to_s, :format => 'json' },
+                { :mastery_points => 7 },
+                {}, { :expected_status => 400 })
+              @outcome2.reload
+              expect(@outcome2.rubric_criterion[:mastery_points]).to_not eq 7
             end
           end
         end