fix assessment content id prepending

test plan:
* create a course
* import a quiz into it (doesn't matter how)

* import a qti/cc file, making sure not to check
 the "overwrite content box"
* re-import the same file, this time, checking the
 "overwrite content box"

* should have overwritten the assessment content

closes #CNVS-15844

Change-Id: I13532e2c4cbd332d2a4d84a2339dcc529df1b167
Reviewed-on: https://gerrit.instructure.com/41826
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Clare Strong <clare@instructure.com>

app/models/content_migration.rb

@@ -380,11 +380,7 @@ class ContentMigration < ActiveRecord::Base
   def check_quiz_id_prepender
     return unless self.context.respond_to?(:assessment_questions)
     if !migration_settings[:id_prepender] && (!migration_settings[:overwrite_questions] || !migration_settings[:overwrite_quizzes])
-      # only prepend an id if the course already has some migrated questions/quizzes
+      migration_settings[:id_prepender] = self.id
-      if self.context.assessment_questions.where('assessment_questions.migration_id IS NOT NULL').exists? ||
-         (self.context.respond_to?(:quizzes) && self.context.quizzes.where('quizzes.migration_id IS NOT NULL').exists?)
-        migration_settings[:id_prepender] = self.id
-      end
     end
   end
 

app/models/importers/account_content_importer.rb

@@ -5,6 +5,8 @@ module Importers
     Importers.register_content_importer(self)
 
     def self.import_content(account, data, params, migration)
+      Importers::ContentImporterHelper.add_assessment_id_prepend(account, data, migration)
+
       Importers::AssessmentQuestionImporter.process_migration(data, migration)
       Importers::LearningOutcomeImporter.process_migration(data, migration)
 

app/models/importers/content_importer_helper.rb

@@ -0,0 +1,37 @@
+module Importers
+  class ContentImporterHelper
+
+    # do the id prepending right before import so we can only make an id unique if it's necessary
+    def self.add_assessment_id_prepend(context, data, migration)
+      id_prepender = migration.migration_settings[:id_prepender]
+      if id_prepender && !migration.migration_settings[:overwrite_quizzes]
+        existing_ids = existing_migration_ids(context)
+        if data[:assessment_question_banks]
+          Canvas::Migration::MigratorHelper.prepend_id_to_assessment_question_banks(data[:assessment_question_banks], id_prepender, existing_ids)
+        end
+
+        if data[:assessment_questions] && data[:assessment_questions][:assessment_questions]
+          Canvas::Migration::MigratorHelper.prepend_id_to_questions(data[:assessment_questions][:assessment_questions], id_prepender, existing_ids)
+        end
+
+        if data[:assessments] && data[:assessments][:assessments]
+          Canvas::Migration::MigratorHelper.prepend_id_to_assessments(data[:assessments][:assessments], id_prepender, existing_ids)
+          if data[:modules]
+            Canvas::Migration::MigratorHelper.prepend_id_to_linked_assessment_module_items(data[:modules], id_prepender, existing_ids)
+          end
+        end
+      end
+    end
+
+    def self.existing_migration_ids(context)
+      existing_ids = {}
+      existing_ids[:assessments] = context.quizzes.active.
+          where("quizzes.migration_id IS NOT NULL").pluck(:migration_id) if context.respond_to?(:quizzes)
+      existing_ids[:assessment_questions] = context.assessment_questions.active.
+          where("assessment_questions.migration_id IS NOT NULL").pluck(:migration_id) if context.respond_to?(:assessment_questions)
+      existing_ids[:assessment_question_banks] = context.assessment_question_banks.active.
+          where("assessment_question_banks.migration_id IS NOT NULL").pluck(:migration_id) if context.respond_to?(:assessment_question_banks)
+      existing_ids
+    end
+  end
+end

app/models/importers/course_content_importer.rb

@@ -67,6 +67,9 @@ module Importers
     def self.import_content(course, data, params, migration)
       params ||= {:copy=>{}}
       logger.debug "starting import"
+
+      Importers::ContentImporterHelper.add_assessment_id_prepend(course, data, migration)
+
       course.full_migration_hash = data
       course.external_url_hash = {}
       course.migration_results = []

lib/canvas/migration/migrator_helper.rb

@@ -138,8 +138,6 @@ module MigratorHelper
     @course = @course.with_indifferent_access
     Importers::AssessmentQuestionImporter.preprocess_migration_data(@course)
 
-    add_assessment_id_prepend
-
     file_name ||= File.join(@base_export_dir, FULL_COURSE_JSON_FILENAME)
     file_name = File.expand_path(file_name)
     @course[:full_export_file_path] = file_name
@@ -153,68 +151,72 @@ module MigratorHelper
   def id_prepender
     @settings[:id_prepender]
   end
-  
+
-  def prepend_id(id, prepend_value=nil)
+  def self.prepend_id(id, prepend_value)
-    prepend_value ||= id_prepender
     prepend_value ? "#{prepend_value}_#{id}" : id
   end
-  
+
-  def add_assessment_id_prepend
+  def self.should_prepend?(type, id, existing_ids)
-    if id_prepender && !@settings[:overwrite_quizzes]
+    existing_ids.nil? || existing_ids[type].to_a.include?(id)
-      if @course[:assessment_question_banks]
-        prepend_id_to_assessment_question_banks(@course[:assessment_question_banks])
-      end
-      if @course[:assessment_questions] && @course[:assessment_questions][:assessment_questions]
-        prepend_id_to_questions(@course[:assessment_questions][:assessment_questions])
-      end
-      if @course[:assessments] && @course[:assessments][:assessments]
-        prepend_id_to_assessments(@course[:assessments][:assessments])
-        prepend_id_to_linked_assessment_module_items(@course[:modules]) if @course[:modules]
-      end
-    end
   end
 
-  def prepend_id_to_assessment_question_banks(banks, prepend_value=nil)
+  def self.prepend_id_to_assessment_question_banks(banks, prepend_value, existing_ids=nil)
     banks.each do |b|
-      b[:migration_id] = prepend_id(b[:migration_id], prepend_value)
+      if should_prepend?(:assessment_question_banks, b[:migration_id], existing_ids)
-    end
+        b[:migration_id] = prepend_id(b[:migration_id], prepend_value)
-  end
-
-  def prepend_id_to_questions(questions, prepend_value=nil)
-    questions.each do |q|
-      [:migration_id, :question_bank_id, :question_bank_migration_id, :assessment_question_migration_id].each do |key|
-        q[key] = prepend_id(q[key], prepend_value) if q[key].present?
       end
     end
   end
-  
+
-  def prepend_id_to_assessments(assessments, prepend_value=nil)
+  # still used by standard/quiz_converter
+  def self.prepend_id_to_questions(questions, prepend_value, existing_ids=nil)
+    key_types = {:migration_id => :assessment_questions, :question_bank_id => :assessment_question_banks,
+     :question_bank_migration_id => :assessment_question_banks, :assessment_question_migration_id => :assessment_questions}
+
+    questions.each do |q|
+      key_types.each do |key, type|
+        q[key] = prepend_id(q[key], prepend_value) if q[key].present? && should_prepend?(type, q[key], existing_ids)
+      end
+    end
+  end
+
+  def self.prepend_id_to_assessments(assessments, prepend_value, existing_ids=nil)
     assessments.each do |a|
-      a[:migration_id] = prepend_id(a[:migration_id], prepend_value)
+      if a[:migration_id].present? && should_prepend?(:assessments, a[:migration_id], existing_ids)
-      if h = a[:assignment]
+        a[:migration_id] = prepend_id(a[:migration_id], prepend_value)
-        h[:migration_id] = prepend_id(h[:migration_id], prepend_value)
+        if h = a[:assignment]
+          h[:migration_id] = prepend_id(h[:migration_id], prepend_value)
+        end
       end
 
       a[:questions].each do |q|
         if q[:question_type] == "question_reference"
-          q[:migration_id] = prepend_id(q[:migration_id], prepend_value)
+          if should_prepend?(:assessment_questions, q[:migration_id], existing_ids)
+            q[:migration_id] = prepend_id(q[:migration_id], prepend_value)
+          end
         elsif q[:question_type] == "question_group"
-          q[:question_bank_migration_id] = prepend_id(q[:question_bank_migration_id], prepend_value) if q[:question_bank_migration_id].present?
+          if q[:question_bank_migration_id].present? && should_prepend?(:assessment_question_banks, q[:question_bank_migration_id], existing_ids)
+            q[:question_bank_migration_id] = prepend_id(q[:question_bank_migration_id], prepend_value)
+          end
           q[:questions].each do |gq|
-            gq[:migration_id] = prepend_id(gq[:migration_id], prepend_value)
+            if should_prepend?(:assessment_questions, gq[:migration_id], existing_ids)
+              gq[:migration_id] = prepend_id(gq[:migration_id], prepend_value)
+            end
           end
         end
       end
     end
   end
 
-  def prepend_id_to_linked_assessment_module_items(modules, prepend_value=nil)
+  def self.prepend_id_to_linked_assessment_module_items(modules, prepend_value, existing_ids=nil)
     modules.each do |m|
       next unless m[:items]
       m[:items].each do |i|
         if i[:linked_resource_type] =~ /assessment|quiz/i
-          i[:item_migration_id] = prepend_id(i[:item_migration_id], prepend_value)
+          if should_prepend?(:assessments, i[:linked_resource_id], existing_ids)
-          i[:linked_resource_id] = prepend_id(i[:linked_resource_id], prepend_value)
+            i[:item_migration_id] = prepend_id(i[:item_migration_id], prepend_value)
+            i[:linked_resource_id] = prepend_id(i[:linked_resource_id], prepend_value)
+          end
         end
       end
     end

lib/cc/importer/standard/quiz_converter.rb

@@ -74,7 +74,7 @@ module CC::Importer::Standard
       begin
         manifest_file = File.join(out_folder, Qti::Converter::MANIFEST_FILE)
         questions = Qti.convert_questions(manifest_file, :flavor => Qti::Flavors::COMMON_CARTRIDGE)
-        prepend_id_to_questions(questions, resource_id)
+        ::Canvas::Migration::MigratorHelper.prepend_id_to_questions(questions, resource_id)
 
         #try to replace relative urls
         questions.each do |question|
@@ -98,7 +98,7 @@ module CC::Importer::Standard
       begin
         manifest_file = File.join(out_folder, Qti::Converter::MANIFEST_FILE)
         quizzes = Qti.convert_assessments(manifest_file, :flavor => Qti::Flavors::COMMON_CARTRIDGE)
-        prepend_id_to_assessments(quizzes, resource_id)
+        ::Canvas::Migration::MigratorHelper.prepend_id_to_assessments(quizzes, resource_id)
         if quiz = quizzes.first
           quiz[:migration_id] = resource_id
         end

spec/lib/cc/importer/common_cartridge_1_3_spec.rb

@@ -142,7 +142,7 @@ describe "Standard Common Cartridge importing" do
       if Qti.qti_enabled?
         archive_file_path = File.join(File.dirname(__FILE__) + "/../../../fixtures/migration/cc_inline_qti.zip")
         unzipped_file_path = File.join(File.dirname(archive_file_path), "cc_#{File.basename(archive_file_path, '.zip')}", 'oi')
-        @export_folder = File.join(File.dirname(archive_file_path), "cc_inline_qti")
+        @export_folder = File.join(File.dirname(archive_file_path), "cc_cc_inline_qti")
         @converter = CC::Importer::Standard::Converter.new(:export_archive_path=>archive_file_path, :course_name=>'oi', :base_download_dir=>unzipped_file_path)
         @converter.export
         @course_data = @converter.course.with_indifferent_access

spec/models/content_migration_spec.rb

@@ -252,7 +252,9 @@ describe ContentMigration do
       cm = ContentMigration.new(:context => account, :user => @user)
       cm.migration_type = 'qti_converter'
       cm.migration_settings['import_immediately'] = true
-      cm.migration_settings['overwrite_quizzes'] = true
+
+      # having this set used to always prepend the id, and it would get set it there were any other imported quizzes/questions
+      cm.migration_settings['id_prepender'] = 'thisusedtobreakstuff'
       cm.save!
 
       package_path = File.join(File.dirname(__FILE__) + "/../fixtures/migration/quiz_qti.zip")
@@ -268,6 +270,9 @@ describe ContentMigration do
       cm.queue_migration
       run_jobs
 
+      cm.migration_settings['overwrite_quizzes'] = true
+      cm.migration_settings['id_prepender'] = 'somethingelse'
+      cm.save!
       # run again
       cm.queue_migration
       run_jobs

vendor/plugins/qti_exporter/lib/qti/converter.rb

@@ -124,7 +124,7 @@ class Converter < Canvas::Migration::Migrator
       manifest_file = File.join(@dest_dir_2_1, MANIFEST_FILE)
       Qti.convert_files(manifest_file).each do |attachment|
         mig_id = Digest::MD5.hexdigest(attachment)
-        mig_id = prepend_id(mig_id) if id_prepender
+        mig_id = ::Canvas::Migration::MigratorHelper.prepend_id(mig_id, id_prepender)
         @course[:file_map][mig_id] = {
           :migration_id => mig_id,
           :path_name => attachment,

vendor/plugins/qti_exporter/spec_canvas/lib/qti/qti_1_2_zip_spec.rb

@@ -1,6 +1,6 @@
 require File.expand_path(File.dirname(__FILE__) + '/../../qti_helper')
 if Qti.migration_executable
-  describe "QTI 1.2 zip with id prepender value" do
+  describe "QTI 1.2 zip" do
     before(:all) do
       @archive_file_path = File.join(BASE_FIXTURE_DIR, 'qti', 'plain_qti.zip')
       unzipped_file_path = File.join(File.dirname(@archive_file_path), "qti_#{File.basename(@archive_file_path, '.zip')}", 'oi')
@@ -9,15 +9,16 @@ if Qti.migration_executable
       @course = Course.create!(:name => 'tester')
       @migration = ContentMigration.create(:context => @course)
       
-      @converter = Qti::Converter.new(:export_archive_path=>@archive_file_path, :base_download_dir=>unzipped_file_path, :id_prepender=>'prepend_test', :content_migration => @migration)
+      @converter = Qti::Converter.new(:export_archive_path=>@archive_file_path, :base_download_dir=>unzipped_file_path, :content_migration => @migration)
       @converter.export
       @course_data = @converter.course.with_indifferent_access
       @course_data['all_files_export'] ||= {}
       @course_data['all_files_export']['file_path'] = @course_data['all_files_zip']
 
+      @migration.set_default_settings
       @migration.migration_settings[:migration_ids_to_import] = {:copy=>{}}
       @migration.migration_settings[:files_import_root_path] = @course_data[:files_import_root_path]
-      Importers::CourseContentImporter.import_content(@course, @course_data, nil, @migration)
+      Importers::CourseContentImporter.import_content(@course, @course_data, nil, @migration)      
     end
 
     after(:all) do
@@ -58,30 +59,30 @@ if Qti.migration_executable
       @course.attachments.count.should == 4
 
       dir = Canvas::Migration::MigratorHelper::QUIZ_FILE_DIRECTORY
-      @course.attachments.find_by_migration_id("prepend_test_f3e5ead7f6e1b25a46a4145100566821").full_display_path.should == "course files/#{dir}/#{@migration.id}/exam1/my_files/org1/images/image.png"
+      @course.attachments.find_by_migration_id("f3e5ead7f6e1b25a46a4145100566821").full_display_path.should == "course files/#{dir}/#{@migration.id}/exam1/my_files/org1/images/image.png"
-      @course.attachments.find_by_migration_id("prepend_test_c16566de1661613ef9e5517ec69c25a1").full_display_path.should == "course files/#{dir}/#{@migration.id}/contact info.png"
+      @course.attachments.find_by_migration_id("c16566de1661613ef9e5517ec69c25a1").full_display_path.should == "course files/#{dir}/#{@migration.id}/contact info.png"
-      @course.attachments.find_by_migration_id("prepend_test_4d348a246af616c7d9a7d403367c1a30").full_display_path.should == "course files/#{dir}/#{@migration.id}/exam1/my_files/org0/images/image.png"
+      @course.attachments.find_by_migration_id("4d348a246af616c7d9a7d403367c1a30").full_display_path.should == "course files/#{dir}/#{@migration.id}/exam1/my_files/org0/images/image.png"
-      @course.attachments.find_by_migration_id("prepend_test_d2b5ca33bd970f64a6301fa75ae2eb22").full_display_path.should == "course files/#{dir}/#{@migration.id}/image.png"
+      @course.attachments.find_by_migration_id("d2b5ca33bd970f64a6301fa75ae2eb22").full_display_path.should == "course files/#{dir}/#{@migration.id}/image.png"
     end
 
     it "should use expected file links in questions" do
-      aq = @course.assessment_questions.find_by_migration_id("prepend_test_QUE_1003")
+      aq = @course.assessment_questions.find_by_migration_id("QUE_1003")
-      c_att = @course.attachments.find_by_migration_id("prepend_test_4d348a246af616c7d9a7d403367c1a30")
+      c_att = @course.attachments.find_by_migration_id("4d348a246af616c7d9a7d403367c1a30")
       att = aq.attachments.find_by_migration_id(CC::CCHelper.create_key(c_att))
       aq.question_data["question_text"].should =~ %r{files/#{att.id}/download}
       
-      aq = @course.assessment_questions.find_by_migration_id("prepend_test_QUE_1007")
+      aq = @course.assessment_questions.find_by_migration_id("QUE_1007")
-      c_att = @course.attachments.find_by_migration_id("prepend_test_f3e5ead7f6e1b25a46a4145100566821")
+      c_att = @course.attachments.find_by_migration_id("f3e5ead7f6e1b25a46a4145100566821")
       att = aq.attachments.find_by_migration_id(CC::CCHelper.create_key(c_att))
       aq.question_data["question_text"].should =~ %r{files/#{att.id}/download}
       
-      aq = @course.assessment_questions.find_by_migration_id("prepend_test_QUE_1014")
+      aq = @course.assessment_questions.find_by_migration_id("QUE_1014")
-      c_att = @course.attachments.find_by_migration_id("prepend_test_d2b5ca33bd970f64a6301fa75ae2eb22")
+      c_att = @course.attachments.find_by_migration_id("d2b5ca33bd970f64a6301fa75ae2eb22")
       att = aq.attachments.find_by_migration_id(CC::CCHelper.create_key(c_att))
       aq.question_data["question_text"].should =~ %r{files/#{att.id}/download}
       
-      aq = @course.assessment_questions.find_by_migration_id("prepend_test_QUE_1053")
+      aq = @course.assessment_questions.find_by_migration_id("QUE_1053")
-      c_att = @course.attachments.find_by_migration_id("prepend_test_c16566de1661613ef9e5517ec69c25a1")
+      c_att = @course.attachments.find_by_migration_id("c16566de1661613ef9e5517ec69c25a1")
       att = aq.attachments.find_by_migration_id(CC::CCHelper.create_key(c_att))
       aq.question_data["question_text"].should =~ %r{files/#{att.id}/download}
     end
@@ -94,18 +95,19 @@ if Qti.migration_executable
     it "should use new attachments for imports with same file names" do
       # run a second migration and check that there are different attachments on the questions
       migration = ContentMigration.create(:context => @course)
-      converter = Qti::Converter.new(:export_archive_path=>@archive_file_path, :id_prepender=>'test2', :content_migration => migration)
+      converter = Qti::Converter.new(:export_archive_path=>@archive_file_path, :content_migration => migration, :id_prepender => 'test2')
       converter.export
       course_data = converter.course.with_indifferent_access
       course_data['all_files_export'] ||= {}
       course_data['all_files_export']['file_path'] = course_data['all_files_zip']
       migration.migration_settings[:migration_ids_to_import] = {:copy=>{}}
       migration.migration_settings[:files_import_root_path] = course_data[:files_import_root_path]
+      migration.migration_settings[:id_prepender] = 'test2'
       Importers::CourseContentImporter.import_content(@course, course_data, nil, migration)
       
       # Check the first import
-      aq = @course.assessment_questions.find_by_migration_id("prepend_test_QUE_1003")
+      aq = @course.assessment_questions.find_by_migration_id("QUE_1003")
-      c_att = @course.attachments.find_by_migration_id("prepend_test_4d348a246af616c7d9a7d403367c1a30")
+      c_att = @course.attachments.find_by_migration_id("4d348a246af616c7d9a7d403367c1a30")
       att = aq.attachments.find_by_migration_id(CC::CCHelper.create_key(c_att))
       aq.question_data["question_text"].should =~ %r{files/#{att.id}/download}
       
@@ -121,18 +123,18 @@ if Qti.migration_executable
 
   QTI_EXPORT_ASSESSMENT = {
           :assessments=>
-                  [{:migration_id=>"prepend_test_A1001",
+                  [{:migration_id=>"A1001",
                     :questions=>
-                            [{:migration_id=>"prepend_test_QUE_1003", :question_type=>"question_reference"},
+                            [{:migration_id=>"QUE_1003", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1007", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1007", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1014", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1014", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1018", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1018", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1022", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1022", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1031", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1031", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1037", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1037", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1043", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1043", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1049", :question_type=>"question_reference"},
+                             {:migration_id=>"QUE_1049", :question_type=>"question_reference"},
-                             {:migration_id=>"prepend_test_QUE_1053", :question_type=>"question_reference"}],
+                             {:migration_id=>"QUE_1053", :question_type=>"question_reference"}],
                     :question_count=>10,
                     :quiz_type=>nil,
                     :quiz_name=>"Quiz",