Check version when finding questions from a pool

Fixes CNVS-18016

Test plan:
 - create a question bank
 - create a quiz with a question group and associate it with the
 question bank
 - take the quiz, see your questions
 - change the questions in the question bank
 - take the quiz, see your changes

Change-Id: I2fd65eb1a5daaba3bda1a96ee6dcb2f2b54b2533
Reviewed-on: https://gerrit.instructure.com/48596
Tested-by: Jenkins
Reviewed-by: Ryan Taylor <rtaylor@instructure.com>
QA-Review: Amber Taniuchi <amber@instructure.com>
Product-Review: Hilary Scharton <hilary@instructure.com>

app/models/assessment_question.rb

@@ -212,9 +212,14 @@ class AssessmentQuestion < ActiveRecord::Base
   end
 
   def find_or_create_quiz_question(quiz_id, exclude_ids=[])
-    finder = quiz_questions.where({ quiz_id: quiz_id })
-    finder = finder.where('id NOT IN (?)', exclude_ids) if exclude_ids.any?
-    finder.first || create_quiz_question(quiz_id)
+    query = quiz_questions.where(quiz_id: quiz_id)
+    query = query.where('id NOT IN (?)', exclude_ids) if exclude_ids.present?
+
+    if qq = query.first
+      qq.update_assessment_question! self
+    else
+      create_quiz_question(quiz_id)
+    end
   end
 
   def self.scrub(text)
@@ -233,28 +238,23 @@ class AssessmentQuestion < ActiveRecord::Base
 
   def self.parse_question(qdata, assessment_question=nil)
     qdata = qdata.to_hash.with_indifferent_access
-    previous_data = assessment_question.question_data rescue {}
-    previous_data ||= {}
+    qdata[:question_name] ||= qdata[:name]
 
-    question = Quizzes::QuizQuestion::QuestionData.generate(
-      id: qdata[:id] || previous_data[:id],
-      regrade_option: qdata[:regrade_option] || previous_data[:regrade_option],
-      points_possible: qdata[:points_possible] || previous_data[:points_possible],
-      correct_comments: qdata[:correct_comments] || previous_data[:correct_comments],
-      incorrect_comments: qdata[:incorrect_comments] || previous_data[:incorrect_comments],
-      neutral_comments: qdata[:neutral_comments] || previous_data[:neutral_comments],
-      question_type: qdata[:question_type] || previous_data[:question_type],
-      question_name: qdata[:question_name] || qdata[:name] || previous_data[:question_name],
-      question_text: qdata[:question_text] || previous_data[:question_text],
-      answers: qdata[:answers] || previous_data[:answers],
-      formulas: qdata[:formulas] || previous_data[:formulas],
-      variables: qdata[:variables] || previous_data[:variables],
-      answer_tolerance: qdata[:answer_tolerance] || previous_data[:answer_tolerance],
-      formula_decimal_places: qdata[:formula_decimal_places] || previous_data[:formula_decimal_places],
-      matching_answer_incorrect_matches: qdata[:matching_answer_incorrect_matches] || previous_data[:matching_answer_incorrect_matches],
-      matches: qdata[:matches] || previous_data[:matches]
+    previous_data = if assessment_question.present?
+                      assessment_question.question_data || {}
+                    else
+                      {}
+                    end.with_indifferent_access
+
+    data = previous_data.merge(qdata.delete_if {|k, v| !v}).slice(
+      :id, :regrade_option, :points_possible, :correct_comments, :incorrect_comments,
+      :neutral_comments, :question_type, :question_name, :question_text, :answers,
+      :formulas, :variables, :answer_tolerance, :formula_decimal_places,
+      :matching_answer_incorrect_matches, :matches
     )
 
+    question = Quizzes::QuizQuestion::QuestionData.generate(data)
+
     question[:assessment_question_id] = assessment_question.id rescue nil
     question
   end

app/models/quizzes/quiz_question.rb

@@ -132,6 +132,11 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
     data
   end
 
+  def assessment_question= aq
+    self.assessment_question_version = aq.version_number
+    super aq
+  end
+
   def delete_assessment_question
     if self.assessment_question && self.assessment_question.editable_by?(self)
       self.assessment_question.destroy
@@ -140,15 +145,28 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
 
   def create_assessment_question
     return if self.question_data && self.question_data.is_type?(:text_only)
-    self.assessment_question ||= AssessmentQuestion.new
-    if self.assessment_question.editable_by?(self)
-      self.assessment_question.question_data = self.question_data
-      self.assessment_question.initial_context = self.quiz.context if self.quiz && self.quiz.context
-      self.assessment_question.save if self.assessment_question.new_record?
-      self.assessment_question_id = self.assessment_question.id
-      self.assessment_question_version = self.assessment_question.version_number rescue nil
+
+    aq = self.assessment_question || AssessmentQuestion.new
+
+    if aq.editable_by?(self)
+      aq.question_data = self.question_data
+      aq.initial_context = self.quiz.context if self.quiz && self.quiz.context
+      aq.save! if aq.new_record?
     end
-    true
+
+    self.assessment_question = aq
+
+    return true
+  end
+
+  def update_assessment_question! aq
+    if assessment_question_version.blank? || assessment_question_version < aq.version_number
+      self.assessment_question = aq
+      self.question_data = aq.question_data
+      save!
+    end
+
+    return self
   end
 
   def validate_blank_questions

app/models/quizzes/quiz_question_builder.rb

@@ -231,4 +231,4 @@ class Quizzes::QuizQuestionBuilder
     @picked[:aq].concat(questions.map { |q| q[:assessment_question_id] }).uniq!
     @picked[:qq].concat(questions.map { |q| q[:id] })
   end
-end
+end

spec/models/assessment_question_spec.rb

@@ -19,7 +19,6 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe AssessmentQuestion do
-
   before :once do
     course
     @bank = @course.assessment_question_banks.create!(:title=>'Test Bank')
@@ -202,5 +201,37 @@ describe AssessmentQuestion do
     expect(question.question_data[:name]).to eq "new name"
     expect(data.object_id).to eq question.question_data.object_id
   end
-  
+
+  describe '#find_or_create_quiz_question' do
+    let(:assessment_question){assessment_question_model(bank: AssessmentQuestionBank.create!(context: Course.create!))}
+    let(:quiz){quiz_model}
+
+    it 'should create a quiz_question when one does not exist' do
+      expect do
+        assessment_question.find_or_create_quiz_question(quiz.id)
+      end.to change{AssessmentQuestion.count}.by(1)
+    end
+
+    it 'should find an existing quiz_question' do
+      qq = assessment_question.find_or_create_quiz_question(quiz.id)
+
+      expect do
+        qq2 = assessment_question.find_or_create_quiz_question(quiz.id)
+        expect(qq2.id).to eql(qq.id)
+      end.to_not change{AssessmentQuestion.count}
+    end
+
+    it 'should find and update an out of date quiz_question' do
+      qq = assessment_question.find_or_create_quiz_question(quiz.id)
+
+      assessment_question.name = 'changed'
+      assessment_question.with_versioning(&:save!)
+
+      expect(qq.assessment_question_version).to_not eql(assessment_question.version_number)
+
+      qq2 = assessment_question.find_or_create_quiz_question(quiz.id)
+      expect(qq.assessment_question_version).to_not eql(qq2.assessment_question_version)
+      expect(qq2.assessment_question_version).to eql(assessment_question.version_number)
+    end
+  end
 end