ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix a potential race condition bug for Q.N importing 

closes QUIZ-4898

test plan:
- a regression run on old Canvas CC/Qti imports
- try to import a Q.N import (without running Q.N) in Canvas
- from rails console, workflow_state should be `imported` for the migration
- from Canvas Api, we should be able to see values for the following key
    audit_info/migration_settings/imported_assets/lti_assignment_quiz_set

Change-Id: Ib4c13e59023578431afa502a3590bbc5e081eaf1
Reviewed-on: https://gerrit.instructure.com/156348
Reviewed-by: Steve Kacsmark <skacsmark@instructure.com>
Tested-by: Jenkins
QA-Review: David Tan <dtan@instructure.com>
Product-Review: Kevin Dougherty <jdougherty@instructure.com>
This commit is contained in:
Han Yan 2018-07-06 09:39:03 -05:00
parent d28dae33ac
commit 753c155b67
4 changed files with 61 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/models/importers/course_content_importer.rb
View File

@ -277,7 +277,7 @@ module Importers
imported_asset_hash = {}
migration.imported_migration_items_hash.each{|k, assets| imported_asset_hash[k] = assets.values.map(&:id).join(',') if assets.present?}
migration.migration_settings[:imported_assets] = imported_asset_hash
migration.workflow_state = :imported
migration.workflow_state = :imported unless post_processing?(migration)
migration.save
ActiveRecord::Base.skip_touch_context(false)
if course.changed?
@ -292,6 +292,10 @@ module Importers
migration.imported_migration_items
end
def self.post_processing?(migration)
migration.quizzes_next_migration?
end
def self.import_syllabus_from_migration(course, syllabus_body, migration)
if migration.for_master_course_import?
course.updating_master_template_id = migration.master_course_subscription.master_template_id

5
app/models/quizzes_next/importers/course_content_importer.rb
View File

@ -29,6 +29,7 @@ module QuizzesNext::Importers
import_content(context, @data, params, @migration)
migration_lti!
mark_completion!
end
private
@ -55,6 +56,10 @@ module QuizzesNext::Importers
imported_asset_hash = @migration.migration_settings[:imported_assets] || {}
imported_asset_hash[:lti_assignment_quiz_set] = lti_assignment_quiz_set
@migration.migration_settings[:imported_assets] = imported_asset_hash
end
def mark_completion!
@migration.workflow_state = :imported
@migration.save!
end
end

54
spec/models/importers/course_content_importer_spec.rb
View File

@ -202,33 +202,44 @@ describe Course do
expect(file.folder.full_name).to eq("course files/Course Content/Orientation/WebCT specific and old stuff")
end
def setup_import(import_course, filename, params, copy_options={})
json = File.open(File.join(IMPORT_JSON_DIR, filename)).read
data = JSON.parse(json).with_indifferent_access
def build_migration(import_course, params, copy_options={})
migration = ContentMigration.create!(:context => import_course)
migration.migration_settings[:migration_ids_to_import] = params
migration.migration_settings[:copy_options] = copy_options
migration.save!
migration
end
Importers::CourseContentImporter.import_content(import_course, data, params, migration)
def setup_import(import_course, filename, migration)
json = File.open(File.join(IMPORT_JSON_DIR, filename)).read
data = JSON.parse(json).with_indifferent_access
Importers::CourseContentImporter.import_content(
import_course,
data,
migration.migration_settings[:migration_ids_to_import],
migration
)
end
it "should not duplicate assessment questions in question banks" do
params = {:copy => {"everything" => true}}
setup_import(@course, 'assessments.json', params)
migration = build_migration(@course, params)
setup_import(@course, 'assessments.json', migration)
aqb1 = @course.assessment_question_banks.where(migration_id: "i05dab0b3d55dae214bd0c4787bd6d20f").first
expect(aqb1.assessment_questions.count).to eq 3
aqb2 = @course.assessment_question_banks.where(migration_id: "iaac763df0de1199ef143b2ab8f237e76").first
expect(aqb2.assessment_questions.count).to eq 2
expect(migration.workflow_state).to eq('imported')
end
it "should not create assessment question banks if they are not selected" do
params = {"copy" => {"assessment_question_banks" => {"i05dab0b3d55dae214bd0c4787bd6d20f" => true},
"quizzes" => {"i7ed12d5eade40d9ee8ecb5300b8e02b2" => true,
"ife86eb19e30869506ee219b17a6a1d4e" => true}}}
setup_import(@course, 'assessments.json', params)
migration = build_migration(@course, params)
setup_import(@course, 'assessments.json', migration)
expect(@course.assessment_question_banks.count).to eq 1
aqb1 = @course.assessment_question_banks.where(migration_id: "i05dab0b3d55dae214bd0c4787bd6d20f").first
@ -241,24 +252,29 @@ describe Course do
quiz2 = @course.quizzes.where(migration_id: "ife86eb19e30869506ee219b17a6a1d4e").first
quiz2.quiz_questions.each{|qq| expect(qq.assessment_question).to be_nil } # since the bank wasn't brought in
expect(migration.workflow_state).to eq('imported')
end
it "should lock announcements if 'lock_all_annoucements' setting is true" do
@course.update_attribute(:lock_all_announcements, true)
params = {"copy" => {"announcements" => {"4488523052421" => true}}}
setup_import(@course, 'announcements.json', params, all_course_settings: true)
migration = build_migration(@course, params, all_course_settings: true)
setup_import(@course, 'announcements.json', migration)
ann = @course.announcements.first
expect(ann).to be_locked
expect(migration.workflow_state).to eq('imported')
end
it "should not lock announcements if 'lock_all_annoucements' setting is false" do
@course.update_attribute(:lock_all_announcements, false)
params = {"copy" => {"announcements" => {"4488523052421" => true}}}
setup_import(@course, 'announcements.json', params, all_course_settings: true)
migration = build_migration(@course, params, all_course_settings: true)
setup_import(@course, 'announcements.json', migration)
ann = @course.announcements.first
expect(ann).to_not be_locked
expect(migration.workflow_state).to eq('imported')
end
it "runs DueDateCacher only twice" do
@ -269,7 +285,25 @@ describe Course do
expect(due_date_cacher).to receive(:recompute).twice
params = {:copy => {"everything" => true}}
setup_import(@course, 'assessments.json', params)
migration = build_migration(@course, params)
setup_import(@course, 'assessments.json', migration)
expect(migration.workflow_state).to eq('imported')
end
context "when it is a Quizzes.Next migration" do
let(:migration) do
params = {:copy => {"everything" => true}}
build_migration(@course, params)
end
before do
allow(migration).to receive(:quizzes_next_migration?).and_return(true)
end
it "shouldn't set workflow_state to imported" do
setup_import(@course, 'assessments.json', migration)
expect(migration.workflow_state).not_to eq('imported')
end
end
end

7
spec/models/quizzes_next/importers/course_content_importer_spec.rb
View File

@ -70,8 +70,15 @@ describe QuizzesNext::Importers::CourseContentImporter do
expect(quiz1).to receive(:destroy)
expect(quiz2).to receive(:destroy)
expect(quiz2).not_to receive(:destroy)
original_setup_assets_imported = importer.method(:setup_assets_imported)
expect(importer).to receive(:setup_assets_imported) do |lti_assignment_quiz_set|
expect(migration.workflow_state).not_to eq('imported')
original_setup_assets_imported.call(lti_assignment_quiz_set)
end
importer.import_content(double)
expect(migration.workflow_state).to eq('imported')
expect(migration.migration_settings[:imported_assets][:lti_assignment_quiz_set]).
to eq([[112_233, 123_456], [888_777, 22_345]])
end