Quiz question integrity after regrade

The problem was that quiz questions were going through some
transformations when generated for a submission, but those
transformations were not applied when we re-generated the questions from
a regrade. This patch makes those xforms happen in both phases.

Closes CNVS-15727

TEST PLAN
---- ----

  - create a quiz with two questions, the second being like MChoice
  - modify the title of the second question
  - take the quiz by a student
  - edit the quiz, and the question, and choose a different answer (and
    make sure it's not what you picked, so you get affected)
    - choose the option to only give full credit to ones who chose the
      *new* answer (again, so the student sub gets affected)
  - reload the student view of the submission and verify:
    - you see the "This question has been regraded." banner
    - you see Question 1 and Question 2 as titles
    - you see the same thing (for titles) from the teacher's perspective

Change-Id: I43a9edc03a381efa0d1c2ccfc7705d0ba912184b
Reviewed-on: https://gerrit.instructure.com/41906
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Derek DeVries <ddevries@instructure.com>
QA-Review: Trevor deHaan <tdehaan@instructure.com>
Product-Review: Derek DeVries <ddevries@instructure.com>

app/middleware/load_account.rb

@@ -15,8 +15,8 @@ class LoadAccount
   def self.default_domain_root_account; Account.default; end
 
   def clear_caches
-    Account.clear_special_account_cache!(LoadAccount.force_special_account_reload)
-    LoadAccount.clear_shard_cache
+    Account.clear_special_account_cache!(::LoadAccount.force_special_account_reload)
+    ::LoadAccount.clear_shard_cache
   end
 
   def self.clear_shard_cache

app/models/quizzes/quiz.rb

@@ -42,6 +42,16 @@ class Quizzes::Quiz < ActiveRecord::Base
   attr_readonly :context_id, :context_type
   attr_accessor :notify_of_update
 
+  # @property [Fixnum] submission_question_index
+  # @private
+  #
+  # A counter used in generating question names for students based on the
+  # position of the question within the quiz_data set.
+  #
+  # See #generate_submission
+  # See #generate_submission_question
+  attr_readonly :submission_question_index
+
   has_many :quiz_questions, :dependent => :destroy, :order => 'position', class_name: 'Quizzes::QuizQuestion'
   has_many :quiz_submissions, :dependent => :destroy, :class_name => 'Quizzes::QuizSubmission'
   has_many :quiz_groups, :dependent => :destroy, :order => 'position', class_name: 'Quizzes::QuizGroup'
@@ -553,7 +563,7 @@ class Quizzes::Quiz < ActiveRecord::Base
       if q[:pick_count]
         question_count += q[:actual_pick_count] || q[:pick_count]
       else
-        question_count += 1 unless q[:question_type] == "text_only_question"
+        question_count += 1 unless q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
       end
     end
     question_count || 0
@@ -608,11 +618,28 @@ class Quizzes::Quiz < ActiveRecord::Base
   end
 
   def generate_submission_question(q)
-    @idx ||= 1
-    q[:name] = t '#quizzes.quiz.question_name_counter', "Question %{question_number}", :question_number => @idx
-    if q[:question_type] == 'text_only_question'
+    @submission_question_index = 0 if @submission_question_index.nil?
+
+    unless q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
+      @submission_question_index += 1
+    end
+
+    self.class.decorate_question_for_submission(q, @submission_question_index)
+  end
+
+  # TODO: could this stop mutating the question object and instead return the
+  # decorated version?
+  #
+  # this currently has too many side-effects: on quiz_data, @stored_questions,
+  # and (what we really want) a submissions's quiz_data...
+  #
+  # anyway if ur wondering why any of these fields are being modified in a spec
+  # when you are just innocently calling Quiz#generate_submission, you know why
+  def self.decorate_question_for_submission(q, position)
+    q[:name] = t '#quizzes.quiz.question_name_counter', "Question %{question_number}", :question_number => position
+
+    if q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
       q[:name] = t '#quizzes.quiz.default_text_only_question_name', "Spacer"
-      @idx -= 1
     elsif q[:question_type] == 'fill_in_multiple_blanks_question'
       text = q[:question_text]
       variables = q[:answers].map { |a| a[:blank_id] }.uniq
@@ -651,7 +678,6 @@ class Quizzes::Quiz < ActiveRecord::Base
       q[:question_text] = text
     end
     q[:question_name] = q[:name]
-    @idx += 1
     q
   end
 
@@ -662,7 +688,7 @@ class Quizzes::Quiz < ActiveRecord::Base
     submission.retake
     submission.attempt = (submission.attempt + 1) rescue 1
     user_questions = []
-    @idx = 1
+    @submission_question_index = 0
     @stored_questions = nil
     @submission_questions = self.stored_questions
     if preview

app/models/quizzes/quiz_question.rb

@@ -29,6 +29,7 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
 
   EXPORTABLE_ATTRIBUTES = [:id, :quiz_id, :quiz_group_id, :assessment_question_id, :question_data, :assessment_question_version, :position, :created_at, :updated_at, :workflow_state]
   EXPORTABLE_ASSOCIATIONS = [:quiz, :assessment_question, :quiz_group]
+  TEXT_ONLY = 'text_only_question'
 
   before_save :infer_defaults
   before_save :create_assessment_question
@@ -61,25 +62,42 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
     Quizzes::Quiz.mark_quiz_edited(self.quiz_id)
   end
 
-  def question_data=(data)
+  # @param [Hash] data
+  # @param [String] data[:regrade_option]
+  #  If present, the question will be regraded.
+  #  You must also pass in data[:regrade_user] if you pass this option.
+  #
+  #  See Quizzes::QuizRegrader::Answer::REGRADE_OPTIONS for a rundown of the
+  #  possible values for this parameter.
+  #
+  # @param [User] data[:regrade_user]
+  #  The user/teacher who's performing the regrade (e.g, updating the question).
+  #  Note that this is NOT an id, but an actual instance of a User model.
+  def question_data=(in_data)
+    data = if in_data.is_a?(String)
+      ActiveSupport::JSON.decode(in_data)
+    elsif in_data.is_a?(Hash)
+      in_data.with_indifferent_access
+    else
+      in_data
+    end
+
     if data[:regrade_option].present?
       update_question_regrade(data[:regrade_option], data[:regrade_user])
     end
 
-    if data.is_a?(String)
-      data = ActiveSupport::JSON.decode(data) rescue nil
-    elsif data.class == Hash
-      data = data.with_indifferent_access
-    end
     return if data == self.question_data
+
     data = AssessmentQuestion.parse_question(data, self.assessment_question)
     data[:name] = data[:question_name]
+
     write_attribute(:question_data, data.to_hash)
   end
 
   def question_data
     if data = read_attribute(:question_data)
       if data.class == Hash
+        # TODO: a reader shouldn't write ?????????????????????
         data = write_attribute(:question_data, data.with_indifferent_access)
       end
     end

app/models/quizzes/quiz_regrader/submission.rb

@@ -68,13 +68,21 @@ class Quizzes::QuizRegrader::Submission
   REGRADE_KEEP_FIELDS = (%w{id position name question_name published_at}).to_set
 
   def regraded_question_data
+    pos = 0
+
     submission.quiz_data.map do |question|
       id = question[:id]
+
+      pos += 1 unless question[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
+
       if submitted_answer_ids.include?(id)
-        question.keep_if {|k, v| REGRADE_KEEP_FIELDS.include?(k) }
+        question.keep_if {|k, v| REGRADE_KEEP_FIELDS.include?(k.to_s) }
 
         quiz_question = question_regrades[id].quiz_question
-        data  = quiz_question.question_data
+        data  = Quizzes::Quiz.decorate_question_for_submission(
+          quiz_question.question_data,
+          pos
+        )
         group = quiz_question.quiz_group
 
         if group && group.pick_count

spec/integration/quiz_regrading_spec.rb

@@ -68,7 +68,8 @@ describe "QuizRegrading" do
     @mcq_qqr = @regrade.quiz_question_regrades.create!(quiz_question_id: @multiple_choice_question.id, regrade_option: 'no_regrade')
     @ttf_qqr = @regrade.quiz_question_regrades.create!(quiz_question_id: @true_false_question.id, regrade_option: 'no_regrade')
     @quiz.generate_quiz_data
-    @quiz.workflow_state = 'available'; @quiz.without_versioning { @quiz.save! }
+    @quiz.workflow_state = 'available'
+    @quiz.without_versioning { @quiz.save! }
     @submission = @quiz.generate_submission(@student)
     reset_submission_data!
     @submission.save!
@@ -101,4 +102,29 @@ describe "QuizRegrading" do
     @submission.reload.score.should == 3
   end
 
+  it 'does not expose the question names' do
+    set_regrade_option!('current_correct_only')
+
+    data = @true_false_question.question_data
+    data[:question_name] = 'foo'
+    @true_false_question.question_data = data.to_hash
+    @true_false_question.save!
+
+    data = @multiple_choice_question.question_data
+    data[:question_name] = 'bar'
+    @multiple_choice_question.question_data = data.to_hash
+    @multiple_choice_question.save!
+
+    @quiz.generate_quiz_data
+    @quiz.save!
+
+    Quizzes::QuizRegrader::Regrader.regrade!(quiz: @quiz)
+
+    @submission.reload
+    @quiz.quiz_data[0][:question_name].should == 'foo'
+    @quiz.quiz_data[1][:question_name].should == 'bar'
+    @submission.quiz_data[0][:question_name].should == 'Question 1'
+    @submission.quiz_data[1][:question_name].should == 'Question 2'
+  end
+
 end

spec/models/quizzes/quiz_regrader/submission_spec.rb

@@ -20,7 +20,9 @@ describe Quizzes::QuizRegrader::Submission do
   end
 
   let(:quiz_data) do
-    question_regrades.map {|id, q| q.quiz_question.question_data.dup.merge('question_name' => "Question #{id}") }
+    question_regrades.map do |id, q|
+      q.quiz_question.question_data.dup.merge(question_name: "Question #{id}")
+    end
   end
 
   let(:submission_data) do
@@ -79,11 +81,14 @@ describe Quizzes::QuizRegrader::Submission do
 
   describe "#rescored_submission" do
     before do
-      params = question_regrades.map do |key, qr|
-        {:id => key, :points_possible => question_group.question_points, 'question_name' => "Question #{key}" }
+      regraded_quiz_data = question_regrades.map do |key, qr|
+        Quizzes::Quiz.decorate_question_for_submission({
+          id: key,
+          points_possible: question_group.question_points
+        }, key)
       end
 
-      submission.expects(:quiz_data=).with(params)
+      submission.expects(:quiz_data=).with(regraded_quiz_data)
 
       @regrade_submission = Quizzes::QuizRegrader::Submission.new(
         :submission        => submission,
@@ -97,12 +102,17 @@ describe Quizzes::QuizRegrader::Submission do
     end
 
     it "doesn't change question names" do
-      @regrade_submission.rescored_submission.quiz_data.each_with_index do |q, i|
-        q['question_name'].should == "Question #{i + 1}"
+      @regrade_submission.stubs(submitted_answer_ids: quiz_data.map { |q| q[:id] })
+
+      question_names = @regrade_submission.rescored_submission.quiz_data.map do |q|
+        q[:question_name]
       end
+
+      question_names.sort.should == [
+        'Question 1',
+        'Question 2',
+        'Question 3'
+      ]
     end
-
   end
-
-
 end