From 4997e5446a47280a336dfa814f478464163192df Mon Sep 17 00:00:00 2001 From: Ahmad Amireh Date: Mon, 29 Sep 2014 12:04:16 +0300 Subject: [PATCH] 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 Reviewed-by: Derek DeVries QA-Review: Trevor deHaan Product-Review: Derek DeVries --- app/middleware/load_account.rb | 4 +- app/models/quizzes/quiz.rb | 40 +++++++++++++++---- app/models/quizzes/quiz_question.rb | 30 +++++++++++--- .../quizzes/quiz_regrader/submission.rb | 12 +++++- spec/integration/quiz_regrading_spec.rb | 28 ++++++++++++- .../quizzes/quiz_regrader/submission_spec.rb | 28 ++++++++----- 6 files changed, 115 insertions(+), 27 deletions(-) diff --git a/app/middleware/load_account.rb b/app/middleware/load_account.rb index a4084c186ee..fc3b3e02b3b 100644 --- a/app/middleware/load_account.rb +++ b/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 diff --git a/app/models/quizzes/quiz.rb b/app/models/quizzes/quiz.rb index 38a65161afb..e5ce549debb 100644 --- a/app/models/quizzes/quiz.rb +++ b/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 diff --git a/app/models/quizzes/quiz_question.rb b/app/models/quizzes/quiz_question.rb index 88afdb7cfb2..0858c7f468f 100644 --- a/app/models/quizzes/quiz_question.rb +++ b/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 diff --git a/app/models/quizzes/quiz_regrader/submission.rb b/app/models/quizzes/quiz_regrader/submission.rb index 23f364eefb4..18cfc15c47a 100644 --- a/app/models/quizzes/quiz_regrader/submission.rb +++ b/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 diff --git a/spec/integration/quiz_regrading_spec.rb b/spec/integration/quiz_regrading_spec.rb index 2fb819bff06..451a880c228 100644 --- a/spec/integration/quiz_regrading_spec.rb +++ b/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 diff --git a/spec/models/quizzes/quiz_regrader/submission_spec.rb b/spec/models/quizzes/quiz_regrader/submission_spec.rb index 9fda95b6841..e7a8a80c40b 100644 --- a/spec/models/quizzes/quiz_regrader/submission_spec.rb +++ b/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