Copy Custom Params for Tool Module Items

Previously, we had added support for migrating custom parameters for
tools that were in a course or assignment context. The next step logical
step after this was to add support for copying over custom params for
tools that were module items. This means that teachers will no longer
have to manually re-add LTI 1.3 tools to their modules after doing a
course copy.

closes INTEROP-6881

flag = none

test-plan:
* Install the LTI 1.3 Test Tool. You'll want to make sure it has the
  module item placement, so you can add it as a module item. In addition,
  you'll want to give it no custom parameters.
* Create a new course and add the test tool to a module as a module
  item. When you're adding it, make sure to define some custom
  parameters, something like `{ "context_id": "$Context.id" }`  Also add an assignment and a URL, just to be thorough.
* Create a new empty course and perform a course copy from the original
  course.
* Launch the tool from the module. You should see in the custom
  parameters section that the custom parameters you defined in the
  original course!
* Export the original course into an IMS Common Cartridge file. Then
  import that file in a new course. You should still be able to launch the
  tool from the module and have the custom params get copied over.

Change-Id: I37265ea8a28f15e6ebc711e18df6aad1c6ef883a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/268683
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Mysti Lilla <mysti@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Ryan Hawkins <ryan.hawkins@instructure.com>

app/models/context_module.rb

@@ -689,7 +689,8 @@ class ContextModule < ActiveRecord::Base
         # of the added item fails, no orphaned resource link is created
         added_item.associated_asset = context.lti_resource_links.new(
           custom: Lti::DeepLinkingUtil.validate_custom_params(params[:custom_params]),
-          context_external_tool: content
+          context_external_tool: content,
+          lookup_uuid: params[:lti_resource_link_lookup_uuid]
         )
       end
       added_item.save

app/models/importers/context_module_importer.rb

@@ -300,7 +300,8 @@ module Importers
             :type => 'context_external_tool',
             :indent => hash[:indent].to_i,
             :url => external_tool_url,
-            :id => external_tool_id
+            :id => external_tool_id,
+            :lti_resource_link_lookup_uuid => hash[:lti_resource_link_lookup_uuid]
           }, existing_item, :position => context_module.migration_position)
         end
       elsif resource_class == Quizzes::Quiz

app/models/importers/lti_resource_link_importer.rb

@@ -41,11 +41,26 @@ module Importers
         next if updated
 
         create_or_update_resource_link_for_a_course_context(lti_resource_link, migration)
+        update_resource_link_for_context_module_context(lti_resource_link, migration)
       end
 
       true
     end
 
+    # Some resource links are tied to ContextModule's items, which are stored as ContentTags.
+    # These resource links will already have been created by the context_module importer with the
+    # appropriate lookup_uuid, they just need to be updated to use the custom params of the old
+    # matching Lti::ResourceLink.
+    def self.update_resource_link_for_context_module_context(lti_resource_link, migration)
+      existing_resource_link = migration.context.lti_resource_links.find_by(lookup_uuid: lti_resource_link[:lookup_uuid])
+      # If this isn't a resource link that already exists, it won't be for a context module.
+      return unless existing_resource_link
+
+      existing_resource_link.custom =
+        Lti::DeepLinkingUtil.validate_custom_params(lti_resource_link[:custom])
+      existing_resource_link.save
+    end
+
     def self.create_or_update_resource_link_for_a_course_context(lti_resource_link, migration)
       custom_params = Lti::DeepLinkingUtil.validate_custom_params(lti_resource_link['custom'])
       destination_course = migration.context

lib/cc/importer/canvas/module_converter.rb

@@ -50,6 +50,7 @@ module CC::Importer::Canvas
           item[:linked_resource_type] = get_node_val(item_node, 'content_type')
           item[:linked_resource_id] = get_node_val(item_node, 'identifierref')
           item[:linked_resource_global_id] = get_node_val(item_node, 'global_identifierref')
+          item[:lti_resource_link_lookup_uuid] = get_node_val(item_node, 'lti_resource_link_lookup_uuid')
 
           mod[:items] << item
         end

lib/cc/module_meta.rb

@@ -95,6 +95,9 @@ module CC
                     if ct.content && ct.content.context != @course
                       item_node.global_identifierref ct.content.id
                     end
+                    if ct.associated_asset.class == Lti::ResourceLink
+                      item_node.lti_resource_link_lookup_uuid ct.associated_asset.lookup_uuid
+                    end
                   end
                   item_node.url ct.url if ct.content_type == 'ExternalUrl'
                   item_node.position ct.position

spec/lib/cc/importer/canvas_cartridge_converter_spec.rb

@@ -462,109 +462,171 @@ describe "Canvas Cartridge importing" do
     expect(rubric2_2.title).to eq rubric2.title
   end
 
-  it "should import modules" do
-    mod1 = @copy_from.context_modules.create!(:name => "some module", :unlock_at => 1.week.from_now, :require_sequential_progress => true)
-    mod2 = @copy_from.context_modules.create!(:name => "next module")
-    mod3 = @copy_from.context_modules.create!(:name => "url module")
-    mod4 = @copy_from.context_modules.create!(:name => "attachment module")
-    mod2.prerequisites = [{:type=>"context_module", :name=>mod1.name, :id=>mod1.id}]
-    mod2.require_sequential_progress = true
-    mod2.save!
+  context 'importing modules' do
+    it 'can import all info from a basic module structure' do
+      mod1 = @copy_from.context_modules.create!(:name => "some module", :unlock_at => 1.week.from_now, :require_sequential_progress => true)
+      mod2 = @copy_from.context_modules.create!(:name => "next module")
+      mod3 = @copy_from.context_modules.create!(:name => "url module")
+      mod4 = @copy_from.context_modules.create!(:name => "attachment module")
+      mod2.prerequisites = [{:type=>"context_module", :name=>mod1.name, :id=>mod1.id}]
+      mod2.require_sequential_progress = true
+      mod2.save!
 
-    asmnt1 = @copy_from.assignments.create!(:title => "some assignment")
-    tag = mod1.add_item({:id => asmnt1.id, :type => 'assignment', :indent => 1})
-    c_reqs = []
-    c_reqs << {:type => 'min_score', :min_score => 5, :id => tag.id}
-    page = @copy_from.wiki_pages.create!(:title => "some page")
-    tag = mod1.add_item({:id => page.id, :type => 'wiki_page'})
-    c_reqs << {:type => 'must_view', :id => tag.id}
-    mod1.completion_requirements = c_reqs
-    mod1.save!
+      asmnt1 = @copy_from.assignments.create!(:title => "some assignment")
+      tag = mod1.add_item({:id => asmnt1.id, :type => 'assignment', :indent => 1})
+      c_reqs = []
+      c_reqs << {:type => 'min_score', :min_score => 5, :id => tag.id}
+      page = @copy_from.wiki_pages.create!(:title => "some page")
+      tag = mod1.add_item({:id => page.id, :type => 'wiki_page'})
+      c_reqs << {:type => 'must_view', :id => tag.id}
+      mod1.completion_requirements = c_reqs
+      mod1.save!
 
-    #Add assignment/page to @copy_to so the module can reference them on import
-    asmnt2 = @copy_to.assignments.create(:title => "some assignment")
-    asmnt2.migration_id = CC::CCHelper.create_key(asmnt1)
-    asmnt2.save!
-    page2 = @copy_to.wiki_pages.create(:title => "some page")
-    page2.migration_id = CC::CCHelper.create_key(page)
-    page2.save!
+      #Add assignment/page to @copy_to so the module can reference them on import
+      asmnt2 = @copy_to.assignments.create(:title => "some assignment")
+      asmnt2.migration_id = CC::CCHelper.create_key(asmnt1)
+      asmnt2.save!
+      page2 = @copy_to.wiki_pages.create(:title => "some page")
+      page2.migration_id = CC::CCHelper.create_key(page)
+      page2.save!
 
-    mod3.add_item({ :title => 'Example 1', :type => 'external_url', :url => 'http://a.example.com/' })
-    mod3.add_item({ :title => 'Example 2', :type => 'external_url', :url => 'http://b.example.com/' })
-    ct = mod3.add_item({ :title => 'Example 3', :type => 'external_url', :url => 'http://b.example.com/with%20space' })
-    ContentTag.where(:id => ct).update_all(:url => "http://b.example.com/with space")
+      mod3.add_item({ :title => 'Example 1', :type => 'external_url', :url => 'http://a.example.com/' })
+      mod3.add_item({ :title => 'Example 2', :type => 'external_url', :url => 'http://b.example.com/' })
+      ct = mod3.add_item({ :title => 'Example 3', :type => 'external_url', :url => 'http://b.example.com/with%20space' })
+      ContentTag.where(:id => ct).update_all(:url => "http://b.example.com/with space")
 
-    # attachments are migrated with just their filename as display_name,
-    # if a content tag has a different title the display_name should not update
-    att = Attachment.create!(:filename => 'boring.txt', :display_name => "Super exciting!", :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_from), :context => @copy_from)
-    expect(att.display_name).to eq "Super exciting!"
-    # create @copy_to attachment with normal display_name
-    att_2 = Attachment.create!(:filename => 'boring.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_to), :context => @copy_to)
-    att_2.migration_id = CC::CCHelper.create_key(att)
-    att_2.save
-    att_tag = mod4.add_item({:title => "A different title just because", :type => "attachment", :id => att.id})
+      # attachments are migrated with just their filename as display_name,
+      # if a content tag has a different title the display_name should not update
+      att = Attachment.create!(:filename => 'boring.txt', :display_name => "Super exciting!", :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_from), :context => @copy_from)
+      expect(att.display_name).to eq "Super exciting!"
+      # create @copy_to attachment with normal display_name
+      att_2 = Attachment.create!(:filename => 'boring.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_to), :context => @copy_to)
+      att_2.migration_id = CC::CCHelper.create_key(att)
+      att_2.save
+      att_tag = mod4.add_item({:title => "A different title just because", :type => "attachment", :id => att.id})
 
-    # create @copy_to module link with different name than attachment
-    att_3 = Attachment.create!(:filename => 'filename.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_from), :context => @copy_from)
-    att_3.migration_id = CC::CCHelper.create_key(att_3)
-    att_3.save
-    mod4.add_item({:title => "test answers", :type => "attachment", :id => att_3.id})
+      # create @copy_to module link with different name than attachment
+      att_3 = Attachment.create!(:filename => 'filename.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_from), :context => @copy_from)
+      att_3.migration_id = CC::CCHelper.create_key(att_3)
+      att_3.save
+      mod4.add_item({:title => "test answers", :type => "attachment", :id => att_3.id})
 
-    att_3_2 = Attachment.create!(:filename => 'filename.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_to), :context => @copy_to)
-    att_3_2.migration_id = CC::CCHelper.create_key(att_3)
-    att_3_2.save
+      att_3_2 = Attachment.create!(:filename => 'filename.txt', :uploaded_data => StringIO.new('even more boring'), :folder => Folder.unfiled_folder(@copy_to), :context => @copy_to)
+      att_3_2.migration_id = CC::CCHelper.create_key(att_3)
+      att_3_2.save
 
-    #export to xml
-    builder = Builder::XmlMarkup.new(:indent=>2)
-    @resource.create_module_meta(builder)
-    #convert to json
-    doc = Nokogiri::XML(builder.target!)
-    hash = @converter.convert_modules(doc)
-    #import json into new course
-    hash[0] = hash[0].with_indifferent_access
-    hash[1] = hash[1].with_indifferent_access
-    hash[2] = hash[2].with_indifferent_access
-    hash[3] = hash[3].with_indifferent_access
-    Importers::ContextModuleImporter.process_migration({'modules'=>hash}, @migration)
-    @copy_to.save!
+      #export to xml
+      builder = Builder::XmlMarkup.new(:indent=>2)
+      @resource.create_module_meta(builder)
+      #convert to json
+      doc = Nokogiri::XML(builder.target!)
+      hash = @converter.convert_modules(doc)
+      #import json into new course
+      hash[0] = hash[0].with_indifferent_access
+      hash[1] = hash[1].with_indifferent_access
+      hash[2] = hash[2].with_indifferent_access
+      hash[3] = hash[3].with_indifferent_access
+      Importers::ContextModuleImporter.process_migration({'modules'=>hash}, @migration)
+      @copy_to.save!
 
-    mod1_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod1)).first
-    expect(mod1_2.name).to eq mod1.name
-    expect(mod1_2.unlock_at.to_i).to eq mod1.unlock_at.to_i
-    expect(mod1_2.require_sequential_progress).to eq mod1.require_sequential_progress
-    expect(mod1_2.content_tags.count).to eq mod1.content_tags.count
-    tag = mod1_2.content_tags.first
-    expect(tag.content_id).to eq asmnt2.id
-    expect(tag.content_type).to eq 'Assignment'
-    expect(tag.indent).to eq 1
-    cr1 = mod1_2.completion_requirements.find {|cr| cr[:id] == tag.id}
-    expect(cr1[:type]).to eq 'min_score'
-    expect(cr1[:min_score]).to eq 5
-    tag = mod1_2.content_tags.last
-    expect(tag.content_id).to eq page2.id
-    expect(tag.content_type).to eq 'WikiPage'
-    cr2 = mod1_2.completion_requirements.find {|cr| cr[:id] == tag.id}
-    expect(cr2[:type]).to eq 'must_view'
+      mod1_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod1)).first
+      expect(mod1_2.name).to eq mod1.name
+      expect(mod1_2.unlock_at.to_i).to eq mod1.unlock_at.to_i
+      expect(mod1_2.require_sequential_progress).to eq mod1.require_sequential_progress
+      expect(mod1_2.content_tags.count).to eq mod1.content_tags.count
+      tag = mod1_2.content_tags.first
+      expect(tag.content_id).to eq asmnt2.id
+      expect(tag.content_type).to eq 'Assignment'
+      expect(tag.indent).to eq 1
+      cr1 = mod1_2.completion_requirements.find {|cr| cr[:id] == tag.id}
+      expect(cr1[:type]).to eq 'min_score'
+      expect(cr1[:min_score]).to eq 5
+      tag = mod1_2.content_tags.last
+      expect(tag.content_id).to eq page2.id
+      expect(tag.content_type).to eq 'WikiPage'
+      cr2 = mod1_2.completion_requirements.find {|cr| cr[:id] == tag.id}
+      expect(cr2[:type]).to eq 'must_view'
 
-    mod2_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod2)).first
-    expect(mod2_2.prerequisites.length).to eq 1
-    expect(mod2_2.prerequisites.first).to eq({:type=>"context_module", :name=>mod1_2.name, :id=>mod1_2.id})
+      mod2_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod2)).first
+      expect(mod2_2.prerequisites.length).to eq 1
+      expect(mod2_2.prerequisites.first).to eq({:type=>"context_module", :name=>mod1_2.name, :id=>mod1_2.id})
 
-    mod3_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod3)).first
-    expect(mod3_2.content_tags.length).to eq 2
-    expect(mod3_2.content_tags[0].url).to eq "http://a.example.com/"
-    expect(mod3_2.content_tags[1].url).to eq "http://b.example.com/"
-    expect(@migration.old_warnings_format.first.first).to eq %{Import Error: Module Item - "Example 3"}
+      mod3_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod3)).first
+      expect(mod3_2.content_tags.length).to eq 2
+      expect(mod3_2.content_tags[0].url).to eq "http://a.example.com/"
+      expect(mod3_2.content_tags[1].url).to eq "http://b.example.com/"
+      expect(@migration.old_warnings_format.first.first).to eq %{Import Error: Module Item - "Example 3"}
 
-    mod4_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod4)).first
-    expect(mod4_2.content_tags.first.title).to eq att_tag.title
-    att_2.reload
-    expect(att_2.display_name).to eq 'boring.txt'
+      mod4_2 = @copy_to.context_modules.where(migration_id: CC::CCHelper.create_key(mod4)).first
+      expect(mod4_2.content_tags.first.title).to eq att_tag.title
+      att_2.reload
+      expect(att_2.display_name).to eq 'boring.txt'
 
-    expect(mod4_2.content_tags.count).to eq 2
-    tag = mod4_2.content_tags.last
-    expect(tag.content_type).to eq "Attachment"
-    expect(tag.content_id).to eq att_3_2.id
+      expect(mod4_2.content_tags.count).to eq 2
+      tag = mod4_2.content_tags.last
+      expect(tag.content_type).to eq "Attachment"
+      expect(tag.content_id).to eq att_3_2.id
+    end
+
+    context 'and the module items are ContextExternalTools' do
+      let(:modules_doc) do
+        xml_builder = Builder::XmlMarkup.new(indent: 1)
+        @resource.create_module_meta(xml_builder)
+        Nokogiri::XML(xml_builder.target!)
+      end
+      let(:converted_modules) do
+        @converter.convert_modules(modules_doc)
+      end
+
+      context 'that use LTI 1.3' do
+        let(:context_module) { @copy_from.context_modules.create!(name: 'test module') }
+        let(:tool) { external_tool_model(context: @copy_from, opts: { use_1_3: true, developer_key: developer_key })}
+        let(:developer_key) { DeveloperKey.create!(account: @copy_from.root_account) }
+        let(:content_tag) do
+          context_module.add_item({ name: 'Test Tool', content: tool, url: tool.url,
+                                  type: 'context_external_tool', custom_params: custom_params,
+                                   id: tool.id })
+        end
+        let(:resource_link) { content_tag.associated_asset }
+        let(:custom_params) { { foo: 'bar' } }
+        # I hate defining this hash manually, rather than testing writing to the file, but I don't
+        # actually need to test the resource link converter, as it has its own set of tests already.
+        let(:external_tools) do
+          builder = Builder::XmlMarkup.new(indent: 2)
+          @resource.create_blti_link(tool, builder)
+          doc = Nokogiri::XML(builder.target!)
+          lti_converter = CC::Importer::BLTIConverter.new
+          tool_data_hash = lti_converter.convert_blti_link(doc)
+          tool_data_hash['migration_id'] = CC::CCHelper.create_key(tool)
+          [ tool_data_hash ]
+        end
+        let(:data) do
+          {
+            modules: converted_modules,
+            lti_resource_links: [
+              {
+                custom: custom_params,
+                lookup_uuid: content_tag.associated_asset.lookup_uuid,
+                context_id: @copy_from.id,
+                context_type: 'Course'
+              }
+            ],
+            external_tools: external_tools
+          }.with_indifferent_access
+        end
+
+        it 'can copy over the UUID and custom params to the new Lti::ResourceLink' do
+          content_tag
+          Importers::CourseContentImporter.import_content(@copy_to,
+                                                          data,
+                                                          nil, @migration)
+          copied_module = @copy_to.context_modules.find_by(migration_id: CC::CCHelper.create_key(context_module))
+          copied_content_tag = copied_module.content_tags.find_by(migration_id: CC::CCHelper.create_key(content_tag))
+          expect(copied_content_tag.associated_asset.lookup_uuid).to eq resource_link.lookup_uuid
+          expect(copied_content_tag.associated_asset.custom).to eq resource_link.custom
+        end
+      end
+    end
   end
 
   it "should translate attachment links on import" do