From 65a6ddab517bd0c37ff563d3e3148c5a3427513e Mon Sep 17 00:00:00 2001 From: Peipei Zhou Date: Thu, 22 Aug 2024 12:47:52 +0200 Subject: [PATCH] use safe_constantize to ignore unmappable assets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes CLAB-445 flag=none Test plan: - enable Smart Search feature flag - go to course -> settings -> import course content - choose 'Copy a Canvas course' and check 'Import existing quizzes as New Quizzes' checkbox - click 'Import' - the course copy should be success Change-Id: I4c80af9f2d162b2d01e4118cecd94890c10ee6ff Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/355649 Tested-by: Service Cloud Jenkins Reviewed-by: Tamas Barna Reviewed-by: Ferenc Marcsó QA-Review: Tamas Barna Product-Review: Peipei Zhou --- app/models/content_migration.rb | 4 +-- lib/smart_search.rb | 2 +- spec/models/content_migration_spec.rb | 41 +++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/app/models/content_migration.rb b/app/models/content_migration.rb index 459d7165dd2..4310f084f29 100644 --- a/app/models/content_migration.rb +++ b/app/models/content_migration.rb @@ -1393,8 +1393,8 @@ class ContentMigration < ActiveRecord::Base global_ids = master_template.present? || use_global_identifiers? migration_settings[:imported_assets].each do |asset_type, dest_ids| - klass = asset_type.constantize - next unless klass.column_names.include? "migration_id" + klass = asset_type.safe_constantize + next unless klass&.column_names&.include? "migration_id" dest_ids = dest_ids.split(",").map(&:to_i) mig_id_to_dest_id = context.shard.activate do diff --git a/lib/smart_search.rb b/lib/smart_search.rb index 827f592cfbd..6533dc97b21 100644 --- a/lib/smart_search.rb +++ b/lib/smart_search.rb @@ -232,7 +232,7 @@ module SmartSearch SmartSearch.smart_search_available?(content_migration.context) content_migration.imported_asset_id_map&.each do |class_name, id_mapping| - klass = class_name.constantize + klass = class_name.safe_constantize next unless klass.respond_to?(:embedding_class) fk = klass.embedding_foreign_key # i.e. :wiki_page_id diff --git a/spec/models/content_migration_spec.rb b/spec/models/content_migration_spec.rb index 859a90dad8f..ab74834476c 100644 --- a/spec/models/content_migration_spec.rb +++ b/spec/models/content_migration_spec.rb @@ -2239,4 +2239,45 @@ describe ContentMigration do expect(@course.attachments.last.file_state).not_to eq("deleted") end end + + context "imported_asset_id_map" do + before :once do + # not actually doing a course copy here, just simulating a finished one + @src = course_factory + @dst = course_factory + @old_assignment = @src.assignments.create! title: "foo" + @new_assignment = @dst.assignments.create! title: "foo", migration_id: CC::CCHelper.create_key(@old_assignment, global: true) + + @old_wp = @src.wiki_pages.create! title: "bar" + @new_wp = @dst.wiki_pages.create!( + title: "bar", + migration_id: CC::CCHelper.create_key(@old_wp, global: true) + ) + + @cm = @dst.content_migrations.build(migration_type: "course_copy_importer", user: @teacher) + @cm.migration_settings[:imported_assets] = { + "Assignment" => @new_assignment.id.to_s, + "WikiPage" => @new_wp.id.to_s, + } + @cm.workflow_state = "imported" + @cm.source_course = @src + @cm.save! + end + + it "returns a hash mapping the source asset IDs to the destination asset IDs" do + expect(@cm.imported_asset_id_map).to eq({ + "Assignment" => { @old_assignment.id => @new_assignment.id }, + "WikiPage" => { @old_wp.id => @new_wp.id } + }) + end + + it "ignores assets that can't be mapped" do + @cm.migration_settings[:imported_assets]["lti_assignment_quiz_set"] = [] + @cm.save! + expect(@cm.imported_asset_id_map).to eq({ + "Assignment" => { @old_assignment.id => @new_assignment.id }, + "WikiPage" => { @old_wp.id => @new_wp.id } + }) + end + end end