8000 Fix quiz questions from banks · csseditor/canvas-lms@bfb266c · GitHub
Skip to content

Commit bfb266c

Browse files
amirehDerek DeVries
amireh
authored and
Derek DeVries
committed
Fix quiz questions from banks
This patch makes it so that whenever we generate a submission that pulls questions from a QuestionBank, we are assured that we're dealing with QuizQuestion objects. Retroactively, a data fixer is installed that will process every submission on hot points like the API readers if they happen to have bad data. There are a lot of comments in the additions which should give you more context. Huge refactors: - Quiz is no longer responsible for "preparing" its question data, nor generating questions for submissions. That's now delegated to the QuizQuestionBuilder Closes CNVS-17412 TEST PLAN ---- ---- - create a crazy quiz with all sorts of things: a) single questions, b) question groups with manual questions c) question groups tied to question banks d) question groups that have questions from banks and others manually created e) multiple attempts so we can verify versioning is not affected - MAKE SURE that you can get some questions duplicated (even if it takes you a couple of attempts), you can achieve this by creating 1 group that is linked to a bank, and another group that has 1 or more questions from that same bank (and possibly other banks) - keep trying the quiz until you get a duplicate - take the quiz both as a student and a teacher, and do it multiple times + verify all the answers to the questions are as you entered + ON DUPLICATES: provide different answers, and make sure that all answers were recorded + go over all the submission-related pages and verify everything is still rendering + use the API to read the quiz, its questions, and the submissions and verify that all endpoints are functional Change-Id: I322e2786d33eda03fb18dde0fbb5cd11a16f2449 Reviewed-on: https://gerrit.instructure.com/45456 Tested-by: Jenkins <jenkins@instructure.com> Reviewed-by: Ryan Taylor <rtaylor@instructure.com> QA-Review: Trevor deHaan <tdehaan@instructure.com> Product-Review: Matt Fairbourn <mfairbourn@instructure.com> Reviewed-by: Ahmad Amireh <ahmad@instructure.com> Product-Review: Derek DeVries <ddevries@instructure.com> Reviewed-by: Derek DeVries <ddevries@instructure.com>
1 parent 5e87afa commit bfb266c

19 files changed

-277Lines changed: 1191 additions & 277 deletions

‎app/controllers/filters/quiz_submissions.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ def require_quiz_submission
4242
raise ActiveRecord::RecordNotFound.new('Quiz Submission not found')
4343
end
4444

45+
@quiz_submission.ensure_question_reference_integrity!
4546
@quiz_submission
4647
end
4748

‎app/controllers/quizzes/quizzes_controller.rb‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,6 +700,10 @@ def get_submission
700700
submission = @quiz.quiz_submissions.where(temporary_user_code: user_code).first
701701
end
702702

703+
if submission
704+
submission.ensure_question_reference_integrity!
705+
end
706+
703707
submission
704708
end
705709

‎app/models/assessment_question.rb‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -193,18 +193,28 @@ def editable_by?(question)
193193
false
194194
elsif self.assessment_question_bank && self.assessment_question_bank.title != AssessmentQuestionBank.default_unfiled_title
195195
false
196+
elsif question.is_a?(Quizzes::QuizQuestion) && question.generated?
197+
false
196198
elsif self.new_record? || (quiz_questions.count <= 1 && question.assessment_question_id == self.id)
197199
true
198200
else
199201
false
200202
end
201203
end
202204

203-
def create_quiz_question
204-
qq = quiz_questions.new
205-
qq.migration_id = self.migration_id
206-
qq.write_attribute(:question_data, question_data)
207-
qq
205+
def create_quiz_question(quiz_id)
206+
quiz_questions.new.tap do |qq|
207+
qq.write_attribute(:question_data, question_data)
208+
qq.quiz_id = quiz_id
209+
qq.workflow_state = 'generated'
210+
qq.save_without_callbacks
211+
end
212+
end
213+
214+
def find_or_create_quiz_question(quiz_id, exclude_ids=[])
215+
finder = quiz_questions.where({ quiz_id: quiz_id })
216+
finder = finder.where('id NOT IN (?)', exclude_ids) if exclude_ids.any?
217+
finder.first || create_quiz_question(quiz_id)
208218
end
209219

210220
def self.scrub(text)

‎app/models/assessment_question_bank.rb‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,13 @@ def bookmarked_for?(user)
121121
user && self.assessment_question_bank_users.where(user_id: user).exists?
122122
end
123123

124-
def select_for_submission(count, exclude_ids=[])
124+
def select_for_submission(quiz_id, count, exclude_ids=[], exclude_qq_ids=[])
125125
ids = self.assessment_questions.active.pluck(:id)
126126
ids = (ids - exclude_ids).shuffle[0...count]
127-
ids.empty? ? [] : AssessmentQuestion.where(id: ids).shuffle
127+
questions = ids.empty? ? [] : AssessmentQuestion.where(id: ids).shuffle
128+
questions.map do |aq|
129+
aq.find_or_create_quiz_question(quiz_id, exclude_qq_ids)
130+
end
128131
end
129132

130133
alias_method :destroy!, :destroy

‎app/models/quizzes/quiz.rb‎

Lines changed: 26 additions & 173 deletions
10BC8
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,6 @@ class Quizzes::Quiz < ActiveRecord::Base
4242
attr_readonly :context_id, :context_type
4343
attr_accessor :notify_of_update
4444

45-
# @property [Fixnum] submission_question_index
46-
# @private
47-
#
48-
# A counter used in generating question names for students based on the
49-
# position of the question within the quiz_data set.
50-
#
51-
# See #generate_submission
52-
# See #generate_submission_question
53-
attr_readonly :submission_question_index
54-
5545
has_many :quiz_questions, :dependent => :destroy, :order => 'position', class_name: 'Quizzes::QuizQuestion'
5646
has_many :quiz_submissions, :dependent => :destroy, :class_name => 'Quizzes::QuizSubmission'
5747
has_many :quiz_groups, :dependent => :destroy, :order => 'position', class_name: 'Quizzes::QuizGroup'
@@ -545,7 +535,7 @@ def question_count(force_check=false)
545535
if q[:pick_count]
546536
question_count += q[:actual_pick_count] || q[:pick_count]
547537
else
548-
question_count += 1 unless q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
538+
question_count += 1 unless q[:question_type] == Quizzes::QuizQuestion::Q_TEXT_ONLY
549539
end
550540
end
551541
question_count || 0
@@ -559,36 +549,22 @@ def available_question_count
559549
# the version found by gathering relationships on the Quiz data models,
560550
# but the version being held in Quizzes::Quiz.quiz_data. Caches the result
561551
# in @stored_questions.
562-
def stored_questions(hashes=nil)
563-
res = []
564-
return @stored_questions if @stored_questions && !hashes
565-
questions = hashes || self.quiz_data || []
566-
questions.each do |val|
567-
568-
if val[:answers]
569-
val[:answers] = prepare_answers(val)
570-
val[:matches] = prepare_matches(val) if val[:matches]
571-
elsif val[:questions] # It's a Quizzes::QuizGroup
572-
if val[:assessment_question_bank_id]
573-
# It points to a question bank
574-
# question/answer/match shuffling happens when a submission is generated
575-
else #normal Quizzes::QuizGroup
576-
questions = []
577-
val[:questions].each do |question|
578-
if question[:answers]
579-
question[:answers] = prepare_answers(question)
580-
question[:matches] = prepare_matches(question) if question[:matches]
581-
end
582-
questions << question
583-
end
584-
questions = questions.sort_by { |q| rand }
585-
val[:questions] = questions
586-
end
552+
def stored_questions(preview=false)
553+
return @stored_questions if @stored_questions && !preview
554+
555+
@stored_questions = begin
556+
data_set = if preview
557+
self.generate_quiz_data(:persist => false)
558+
else
559+
self.quiz_data || []
587560
end
588-
res << val
561+
562+
builder = Quizzes::QuizQuestionBuilder.new({
563+
shuffle_answers: self.shuffle_answers
564+
})
565+
566+
builder.shuffle_quiz_data!(data_set)
589567
end
590-
@stored_questions = res
591-
res
592568
end
593569

594570
def single_attempt?
@@ -599,121 +575,6 @@ def unlimited_attempts?
599575
self.allowed_attempts == -1
600576
end
601577

602-
def generate_submission_question(q)
603-
@submission_question_index = 0 if @submission_question_index.nil?
604-
605-
unless q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
606-
@submission_question_index += 1
607-
end
608-
609-
self.class.decorate_question_for_submission(q, @submission_question_index)
610-
end
611-
612-
# TODO: could this stop mutating the question object and instead return the
613-
# decorated version?
614-
#
615-
# this currently has too many side-effects: on quiz_data, @stored_questions,
616-
# and (what we really want) a submissions's quiz_data...
617-
#
618-
# anyway if ur wondering why any of these fields are being modified in a spec
619-
# when you are just innocently calling Quiz#generate_submission, you know why
620-
def self.decorate_question_for_submission(q, position)
621-
q[:name] = t '#quizzes.quiz.question_name_counter', "Question %{question_number}", :question_number => position
622-
623-
if q[:question_type] == Quizzes::QuizQuestion::TEXT_ONLY
624-
q[:name] = t '#quizzes.quiz.default_text_only_question_name', "Spacer"
625-
elsif q[:question_type] == 'fill_in_multiple_blanks_question'
626-
text = q[:question_text]
627-
variables = q[:answers].map { |a| a[:blank_id] }.uniq
628-
variables.each do |variable|
629-
variable_id = ::AssessmentQuestion.variable_id(variable)
630-
re = Regexp.new("\\[#{variable}\\]")
631-
text = text.sub(re, "<input class='question_input' type='text' autocomplete='off' style='width: 120px;' name='question_#{q[:id]}_#{variable_id}' value='{{question_#{q[:id]}_#{variable_id}}}' />")
632-
end
633-
q[:original_question_text] = q[:question_text]
634-
q[:question_text] = text
635-
elsif q[:question_type] == 'multiple_dropdowns_question'
636-
text = q[:question_text]
637-
variables = q[:answers].map { |a| a[:blank_id] }.uniq
638-
variables.each do |variable|
639-
variable_id = ::AssessmentQuestion.variable_id(variable)
640-
variable_answers = q[:answers].select { |a| a[:blank_id] == variable }
641-
options = variable_answers.map { |a| "<option value='#{a[:id]}'>#{CGI::escapeHTML(a[:text])}</option>" }
642-
select = "<select class='question_input' name='question_#{q[:id]}_#{variable_id}'><option value=''>#{ERB::Util.h(t('#quizzes.quiz.default_question_input', "[ Select ]"))}</option>#{options}</select>"
643-
re = Regexp.new("\\[#{variable}\\]")
644-
text = text.sub(re, select)
645-
end
646-
q[:original_question_text] = q[:question_text]
647-
q[:question_text] = text
648-
# on equation questions, pick one of the formulas, plug it in
649-
# and you should be able to treat it like a numerical_answer
650-
# question for all intents and purposes
651-
elsif q[:question_type] == 'calculated_question'
652-
text = q[:question_text]
653-
q[:answers] = [q[:answers].sort_by { |a| rand }.first].compact
654-
if q[:answers].first
655-
q[:answers].first[:variables].each do |variable|
656-
re = Regexp.new("\\[#{variable[:name]}\\]")
657-
text = text.gsub(re, variable[:value].to_s)
658-
end
659-
end
660-
q[:question_text] = text
661-
end
662-
q[:question_name] = q[:name]
663-
q
664-
end
665-
666-
def build_user_questions(preview)
667-
user_questions = []
668-
@submission_question_index = 0
669-
@stored_questions = nil
670-
submission_questions = self.stored_questions
671-
if preview
672-
submission_questions = self.stored_questions(generate_quiz_data(:persist => false))
673-
end
674-
675-
exclude_ids = submission_questions.map { |q| q[:assessment_question_id] }.compact
676-
submission_questions.each do |q|
677-
# pulling from question group
678-
if q[:pick_count]
679-
680-
# pulling from question bank
681-
if q[:assessment_question_bank_id]
682-
bank = ::AssessmentQuestionBank.where(id: q[:assessment_question_bank_id]).first if q[:assessment_question_bank_id].present?
683-
if bank
684-
questions = bank.select_for_submission(q[:pick_count], exclude_ids)
685-
exclude_ids.concat(questions.map {|q| q.id })
686-
687-
questions = questions.map { |aq| aq.data }
688-
questions.each do |question|
689-
if question[:answers]
690-
question[:answers] = prepare_answers(question)
691-
question[:matches] = prepare_matches(question) if question[:matches]
692-
end
693-
question[:points_possible] = q[:question_points]
694-
question[:published_at] = q[:published_at]
695-
user_questions << generate_submission_question(question)
696-
end
697-
end
698-
699-
# a group with questions
700-
else
701-
questions = q[:questions].shuffle.slice(0, q[:pick_count])
702-
questions.each do |question|
703-
question[:points_possible] = q[:question_points]
704-
user_questions << generate_submission_question(question)
705-
end
706-
end
707-
708-
# just a question
709-
else
710-
user_questions << generate_submission_question(q)
711-
end
712-
end
713-
714-
user_questions
715-
end
716-
717578
def build_submission_end_at(submission)
718579
course = context
719580
user = submission.user
@@ -759,12 +620,21 @@ def generate_submission(user, preview=false)
759620
submission = nil
760621

761622
transaction do
623+
builder = Quizzes::QuizQuestionBuilder.new({
624+
shuffle_answers: self.shuffle_answers
625+
})
626+
762627
submission = Quizzes::SubmissionManager.new(self).find_or_create_submission(user, preview)
763628
submission.retake
764629
submission.attempt = (submission.attempt + 1) rescue 1
765630
submission.score = nil
766631
submission.fudge_points = nil
767-
submission.quiz_data = build_user_questions(preview)
632+
633+
submission.quiz_data = begin
634+
@stored_questions = nil
635+
builder.build_submission_questions(self.id, self.stored_questions(preview))
636+
end
637+
768638
submission.quiz_version = self.version_number
769639
submission.started_at = ::Time.now
770640
submission.score_before_regrade = nil
@@ -773,6 +643,7 @@ def generate_submission(user, preview=false)
773643
submission.submission_data = {}
774644
submission.workflow_state = 'preview' if preview
775645
submission.was_preview = preview
646+
submission.question_references_fixed = true
776647

777648
if preview || submission.untaken?
778649
submission.save!
@@ -797,24 +668,6 @@ def generate_submission_for_participant(quiz_participant)
797668
generate_submission quiz_participant.send(identity), false
798669
end
799670

800-
def prepare_answers(question)
801-
if answers = question[:answers]
802-
if shuffle_answers && Quizzes::Quiz.shuffleable_question_type?(question[:question_type])
803-
answers.sort_by { |a| rand }
804-
else
805-
answers
806-
end
807-
end
808-
end
809-
810-
def prepare_matches(question)
811-
if matches = question[:matches]
812-
# question matches should always be shuffled, regardless of the
813-
# shuffle_answers option
814-
matches.sort_by { |m| rand }
815-
end
816-
end
817-
818671
# Takes the PRE-SAVED version of the quiz and uses it to generate a
819672
# SAVED version. That is, gathers the relationship entities from
820673
# the database and uses them to populate a static version that will

‎app/models/quizzes/quiz_question.rb‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,17 @@
1616
# with this program. If not, see <http://www.gnu.org/licenses/>.
1717
#
1818

19+
# A little note about QuizQuestions: they could be auto-generated by an
20+
# AssessmentQuestion that belongs to a Bank. This happens when we're pulling
21+
# out a certain question for the first time out of a bank (during submission
22+
# generation, usually) in which case the callbacks that are normally necessary
23+
# to do the wiring between the QQ and its source AQ are skipped entirely - keep
24+
# this in mind if you're adding any new callbacks to the class.
25+
#
26+
# Also, generated QuizQuestions may not edit their source AssessmentQuestions.
27+
#
28+
# @see AssessmentQuestion#create_quiz_question()
29+
# @see AssessmentQuestionBank#select_for_submission()
1930
class Quizzes::QuizQuestion < ActiveRecord::Base
2031
self.table_name = 'quiz_questions'
2132

@@ -29,12 +40,16 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
2940

3041
EXPORTABLE_ATTRIBUTES = [:id, :quiz_id, :quiz_group_id, :assessment_question_id, :question_data, :assessment_question_version, :position, :created_at, :updated_at, :workflow_state]
3142
EXPORTABLE_ASSOCIATIONS = [:quiz, :assessment_question, :quiz_group]
32-
TEXT_ONLY = 'text_only_question'
43+
44+
Q_TEXT_ONLY = 'text_only_question'
45+
Q_FILL_IN_MULTIPLE_BLANKS = 'fill_in_multiple_blanks_question'
46+
Q_MULTIPLE_DROPDOWNS = 'multiple_dropdowns_question'
47+
Q_CALCULATED = 'calculated_question'
3348

3449
before_save :validate_blank_questions
3550
before_save :infer_defaults
36-
before_save :create_assessment_question
37-
before_destroy :delete_assessment_question
51+
before_save :create_assessment_question, unless: :generated?
52+
before_destroy :delete_assessment_question, unless: :generated?
3853
before_destroy :update_quiz
3954
validates_presence_of :quiz_id
4055
serialize :question_data
@@ -43,9 +58,12 @@ class Quizzes::QuizQuestion < ActiveRecord::Base
4358
workflow do
4459
state :active
4560
state :deleted
61+
# generated from an AssessmentQuestion inside a question bank
62+
state :generated
4663
end
4764

4865
scope :active, -> { where("workflow_state='active' OR workflow_state IS NULL") }
66+
scope :generated, -> { where(workflow_state: 'generated') }
4967

5068
def infer_defaults
5169
if !self.position && self.quiz

0 commit comments

Comments
 (0)