fix problems with lti2 and assignments

fixes PLAT-1371

test plan:

create an LTI 2 assignment
it should launch

create and LTI 2 assignment that is a module item
it should launch

Change-Id: I9cb77477a5d8eec7742c90e5a3c501fc9b6cdce6
Reviewed-on: https://gerrit.instructure.com/76300
Tested-by: Jenkins
Reviewed-by: Matthew Wheeler <mwheeler@instructure.com>
QA-Review: Benjamin Christian Nelson <bcnelson@instructure.com>
Product-Review: Nathan Mills <nathanm@instructure.com>

Author: rivernate

app/controllers/application_controller.rb

@@ -1327,6 +1327,7 @@ def content_tag_redirect(context, tag, error_redirect_symbol, tag_type=nil)
     elsif tag.content_type == 'Rubric'
       redirect_to named_context_url(context, :context_rubric_url, tag.content_id, url_params)
     elsif tag.content_type == 'Lti::MessageHandler'
+      url_params[:module_item_id] = params[:module_item_id] if params[:module_item_id]
       url_params[:resource_link_fragment] = "ContentTag:#{tag.id}"
       redirect_to named_context_url(context, :context_basic_lti_launch_request_url, tag.content_id, url_params)
     elsif tag.content_type == 'ExternalUrl'

app/controllers/lti/message_controller.rb

@@ -100,9 +100,11 @@ def basic_lti_launch_request
           @lti_launch.link_text = resource_handler.name
           @lti_launch.launch_type = message.launch_presentation_document_target
 
-          module_sequence(message_handler)
-          tool_setting_ids = prep_tool_settings(message_handler.parameters, tool_proxy, message.resource_link_id)
-          message.add_custom_params(custom_params(message_handler.parameters, tool_setting_ids.merge(tool: tool_proxy)))
+          tag = find_tag
+          module_sequence(tag) if tag
+          custom_param_opts = prep_tool_settings(message_handler.parameters, tool_proxy, message.resource_link_id)
+          custom_param_opts[:content_tag] = tag if tag
+          message.add_custom_params(custom_params(message_handler.parameters, custom_param_opts.merge(tool: tool_proxy)))
           message.add_custom_params(ToolSetting.custom_settings(tool_proxy.id, @context, message.resource_link_id))
           @lti_launch.params = message.signed_post_params(tool_proxy.shared_secret)
 
@@ -127,21 +129,19 @@ def registration_return
 
     private
 
-    def module_sequence(message_handler)
+    def module_sequence(tag)
       env_hash = {}
-      if params[:module_item_id]
-        @tag = ContextModuleItem.find_tag_with_preferred([message_handler], params[:module_item_id])
-        @lti_launch.launch_type = 'window' if @tag.new_tab
-        @tag.context_module_action(@current_user, :read)
-        sequence_asset = @tag.try(:content)
-        if sequence_asset
-          env_hash[:SEQUENCE] = {
-              :ASSET_ID => sequence_asset.id,
-              :COURSE_ID => @context.id,
-          }
-          js_hash = {:LTI => env_hash}
-          js_env(js_hash)
-        end
+      tag = @context.context_module_tags.not_deleted.find(params[:module_item_id])
+      @lti_launch.launch_type = 'window' if tag.new_tab
+      tag.context_module_action(@current_user, :read)
+      sequence_asset = tag.try(:content)
+      if sequence_asset
+        env_hash[:SEQUENCE] = {
+            :ASSET_ID => sequence_asset.id,
+            :COURSE_ID => @context.id,
+        }
+        js_hash = {:LTI => env_hash}
+        js_env(js_hash)
       end
     end
 
@@ -172,7 +172,6 @@ def create_variable_expander(opts = {})
       default_opts = {
           current_user: @current_user,
           current_pseudonym: @current_pseudonym,
-          content_tag: @tag,
           assignment: nil
       }
       VariableExpander.new(@domain_root_account, @context, self, default_opts.merge(opts))
@@ -212,5 +211,9 @@ def prep_tool_settings(parameters, tool_proxy, resource_link_id)
       end
     end
 
+    def find_tag
+      @context.context_module_tags.not_deleted.where(id: params[:module_item_id]).first if params[:module_item_id]
+    end
+
   end
 end

app/models/assignment.rb

@@ -82,10 +82,10 @@ class Assignment < ActiveRecord::Base
     # only accept the url, content_type, content_id, and new_tab params, the other accessible
     # params don't apply to an content tag being used as an external_tool_tag
     content = case attrs['content_type']
-                when 'Lti::MessageHandler'
-                  Lti::MessageHandler.find(attrs['content_id'].to_i)
-                when 'ContextExternalTool'
-                  ContextExternalTool.find(attrs['content_id'].to_i)
+              when 'Lti::MessageHandler', 'lti/message_handler'
+                Lti::MessageHandler.find(attrs['content_id'].to_i)
+              when 'ContextExternalTool', 'context_external_tool'
+                ContextExternalTool.find(attrs['content_id'].to_i)
               end
     attrs[:content] = content if content
     attrs.slice!(:url, :new_tab, :content)

spec/apis/lti/lti_app_api_spec.rb

@@ -17,12 +17,11 @@
 #
 
 require File.expand_path(File.dirname(__FILE__) + '/../api_spec_helper')
-require File.expand_path(File.dirname(__FILE__) + '/../../lti_spec_helper.rb')
 
 module Lti
-  describe LtiAppsController, type: :request do
+  describe LtiAppsController, :include_lti_spec_helpers, type: :request do
 
-    let (:account) { Account.create }
+    let(:account) { Account.create }
     describe '#launch_definitions' do
 
       before do

spec/apis/lti/tool_proxy_api_spec.rb

@@ -17,12 +17,11 @@
 #
 
 require File.expand_path(File.dirname(__FILE__) + '/../api_spec_helper')
-require File.expand_path(File.dirname(__FILE__) + '/../../lti_spec_helper.rb')
 require 'webmock'
 WebMock.allow_net_connect!
 
 module Lti
-  describe ToolProxyController, type: :request do
+  describe ToolProxyController, :include_lti_spec_helpers, type: :request do
 
     let(:account) { Account.create }
 

spec/apis/v1/assignments_api_spec.rb

@@ -20,7 +20,7 @@
 require File.expand_path(File.dirname(__FILE__) + '/../locked_spec')
 require File.expand_path(File.dirname(__FILE__) + '/../../sharding_spec_helper')
 
-describe AssignmentsApiController, type: :request do
+describe AssignmentsApiController, :include_lti_spec_helpers, type: :request do
   include Api
   include Api::V1::Assignment
   include Api::V1::Submission
@@ -2556,6 +2556,49 @@ def visibility_api_request(assignment)
       @assignment = @course.assignments.create!(:title => "some assignment")
     end
 
+    it 'updates the external tool content_id' do
+      mh = create_message_handler(create_resource_handler(create_tool_proxy))
+      tool_tag = ContentTag.new(url: 'http://www.example.com', new_tab: false, tag_type: 'context_module')
+      tool_tag.context = @assignment
+      tool_tag.save!
+      params = {
+        "submission_types" => ["external_tool"],
+        "external_tool_tag_attributes" => {
+          "url" => "https://testvmserver.test.com/canvas/test/",
+          "content_type" => "lti/message_handler",
+          "content_id" => mh.id,
+          "new_tab" => "0"
+        }
+      }
+      assignment = update_from_params(@assignment, params, @user)
+      tag = assignment.external_tool_tag
+      expect(tag.content_id).to eq mh.id
+      expect(tag.content_type).to eq "Lti::MessageHandler"
+    end
+
+    it 'sets the context external tool type' do
+      tool = ContextExternalTool.new( name: 'test tool', consumer_key:'test',
+        shared_secret: 'shh', url: 'http://www.example.com')
+      tool.context = @course
+      tool.save!
+      tool_tag = ContentTag.new(url: 'http://www.example.com', new_tab: false, tag_type: 'context_module')
+      tool_tag.context = @assignment
+      tool_tag.save!
+      params = {
+        "submission_types" => ["external_tool"],
+        "external_tool_tag_attributes" => {
+          "url" => "https://testvmserver.test.com/canvas/test/",
+          "content_type" => "context_external_tool",
+          "content_id" => tool.id,
+          "new_tab" => "0"
+        }
+      }
+      assignment = update_from_params(@assignment, params, @user)
+      tag = assignment.external_tool_tag
+      expect(tag.content_id).to eq tool.id
+      expect(tag.content_type).to eq "ContextExternalTool"
+    end
+
     it "does not update integration_data when lacking permission" do
       json = %{{"key": "value"}}
       params = {"integration_data" => json}

spec/controllers/lti/message_controller_spec.rb

@@ -222,6 +222,17 @@ module Lti
           expect(params[:oauth_signature]).not_to be_empty
         end
 
+        it 'launches gracefully if it can not find the content_tag for the given module_item_id' do
+          course = Course.create!
+          tag = course.context_module_tags.create!(context: account, tag_type: 'context_module')
+          tag.context_module = ContextModule.create!(context: course)
+          tag.save!
+          tag.delete
+          get 'basic_lti_launch_request', course_id: course.id, message_handler_id: message_handler.id,
+              module_item_id: tag.id, params: {tool_launch_context: 'my_custom_context' }
+          expect(response.code).to eq "200"
+        end
+
         it 'sets the active tab' do
           get 'basic_lti_launch_request', account_id: account.id, message_handler_id: message_handler.id
           expect(response.code).to eq "200"
@@ -257,17 +268,18 @@ module Lti
         end
 
         it 'adds module item substitutions' do
+          course = Course.create!
           parameters = %w( Canvas.module.id Canvas.moduleItem.id ).map do |key|
             IMS::LTI::Models::Parameter.new(name: key.underscore, variable: key )
           end
           message_handler.parameters = parameters.as_json
           message_handler.save
 
-          tag = message_handler.context_module_tags.create!(context: account, tag_type: 'context_module')
-          tag.context_module =  ContextModule.create!(context: Course.create!)
+          tag = message_handler.context_module_tags.create!(context: course, tag_type: 'context_module')
+          tag.context_module = ContextModule.create!(context: course)
           tag.save!
 
-          get 'basic_lti_launch_request', account_id: account.id, message_handler_id: message_handler.id,
+          get 'basic_lti_launch_request', course_id: course.id, message_handler_id: message_handler.id,
               module_item_id: tag.id, params: {tool_launch_context: 'my_custom_context' }
           expect(response.code).to eq "200"
 
@@ -277,10 +289,11 @@ module Lti
         end
 
         it 'sets the launch to window' do
-          tag = message_handler.context_module_tags.create!(context: account, tag_type: 'context_module', new_tab: true)
-          tag.context_module =  ContextModule.create!(context: Course.create!)
+          course = Course.create!
+          tag = message_handler.context_module_tags.create!(context: course, tag_type: 'context_module', new_tab: true)
+          tag.context_module = ContextModule.create!(context: course)
           tag.save!
-          get 'basic_lti_launch_request', account_id: account.id, message_handler_id: message_handler.id,
+          get 'basic_lti_launch_request', course_id: course.id, message_handler_id: message_handler.id,
               module_item_id: tag.id, params: {tool_launch_context: 'my_custom_context' }
           expect(response.code).to eq "200"
           expect(assigns[:lti_launch].launch_type).to eq 'window'

spec/lti_spec_helper.rb

@@ -15,43 +15,56 @@
 # You should have received a copy of the GNU Affero General Public License along
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
+module LtiSpecHelper
+  def create_tool_proxy(opts = {})
 
-def create_tool_proxy(opts = {})
-  default_opts = {
-    context: account,
-    shared_secret: 'shared_secret',
-    guid: SecureRandom.uuid,
-    product_version: '1.0beta',
-    lti_version: 'LTI-2p0',
-    product_family: find_or_create_product_family,
-    workflow_state: 'active',
-    raw_data: 'some raw data',
-    name:  (0...8).map { (65 + rand(26)).chr }.join,
-  }
-  Lti::ToolProxy.create!(default_opts.merge(opts))
-end
+    default_opts = {
+      shared_secret: 'shared_secret',
+      guid: SecureRandom.uuid,
+      product_version: '1.0beta',
+      lti_version: 'LTI-2p0',
+      product_family: find_or_create_product_family,
+      workflow_state: 'active',
+      raw_data: 'some raw data',
+      name: (0...8).map { (65 + rand(26)).chr }.join,
+    }
+    combined_opts = default_opts.merge(opts)
+    combined_opts[:context] = Account.create!(name: 'Test Account') unless combined_opts.has_key?(:context)
+    combined_opts[:product_family] = find_or_create_product_family(combined_opts[:context]) unless combined_opts.has_key?(:product_family)
+    Lti::ToolProxy.create!(combined_opts)
+  end
 
-def find_or_create_product_family(opts = {})
-  default_opts = {vendor_code: '123', product_code: 'abc', vendor_name: 'acme', root_account_id: account.id}
-  Lti::ProductFamily.where(default_opts.merge(opts)).first_or_create
-end
+  def find_or_create_product_family(opts = {})
+    default_opts = {vendor_code: '123', product_code: 'abc', vendor_name: 'acme'}
+    default_opts[:root_account_id] = Account.create!(name: 'Test Account') unless opts.has_key?(:root_account_id)
+    Lti::ProductFamily.where(default_opts.merge(opts)).first_or_create
+  end
 
-def create_resource_handler(tool_proxy, opts = {})
-  default_opts = {resource_type_code: 'code', name: (0...8).map { (65 + rand(26)).chr }.join, tool_proxy: tool_proxy}
-  Lti::ResourceHandler.create(default_opts.merge(opts))
-end
+  def create_resource_handler(tool_proxy, opts = {})
+    default_opts = {resource_type_code: 'code', name: (0...8).map { (65 + rand(26)).chr }.join, tool_proxy: tool_proxy}
+    Lti::ResourceHandler.create(default_opts.merge(opts))
+  end
 
-def create_message_handler(resource_handler, opts = {})
-  default_ops = {message_type: 'basic-lti-launch-request', launch_path: 'https://samplelaunch/blti', resource_handler: resource_handler}
-  Lti::MessageHandler.create(default_ops.merge(opts))
-end
+  def create_message_handler(resource_handler, opts = {})
+    default_ops = {
+      message_type: 'basic-lti-launch-request',
+      launch_path: 'https://samplelaunch/blti',
+      resource_handler: resource_handler
+    }
+    Lti::MessageHandler.create(default_ops.merge(opts))
+  end
 
-def new_valid_external_tool(context, resource_selection = false)
-  tool = context.context_external_tools.new(:name => (0...8).map { (65 + rand(26)).chr }.join,
-                                            :consumer_key => "key",
-                                            :shared_secret => "secret")
-  tool.url = "http://www.example.com/basic_lti"
-  tool.resource_selection = {:url => "http://example.com/selection_test", :selection_width => 400, :selection_height => 400} if resource_selection
-  tool.save!
-  tool
+  def new_valid_external_tool(context, resource_selection = false)
+    tool = context.context_external_tools.new(:name => (0...8).map { (65 + rand(26)).chr }.join,
+                                              :consumer_key => "key",
+                                              :shared_secret => "secret")
+    tool.url = "http://www.example.com/basic_lti"
+    tool.resource_selection = {
+      :url => "http://example.com/selection_test",
+      :selection_width => 400,
+      :selection_height => 400
+    } if resource_selection
+    tool.save!
+    tool
+  end
 end

spec/models/lti/app_collator_spec.rb

@@ -16,10 +16,9 @@
 #
 
 require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
-require File.expand_path(File.dirname(__FILE__) + '/../../lti_spec_helper.rb')
 
 module Lti
-  describe AppCollator do
+  describe AppCollator, :include_lti_spec_helpers do
 
     subject { described_class.new(account, mock_reregistration_url_builder)}
     let(:account) { Account.create }
@@ -46,7 +45,7 @@ module Lti
     describe "#app_definitions" do
 
       it 'returns tool_proxy app definitions' do
-        tool_proxy = create_tool_proxy
+        tool_proxy = create_tool_proxy(context: account)
         tool_proxy.bindings.create(context: account)
         tools_collection = subject.bookmarked_collection.paginate(per_page: 100).to_a
         definitions = subject.app_definitions(tools_collection)
@@ -137,7 +136,7 @@ module Lti
       it 'has_update set to false for tool proxies without an update_payload' do
         account.root_account.enable_feature!(:lti2_rereg)
 
-        tool_proxy = create_tool_proxy
+        tool_proxy = create_tool_proxy(context: account)
         tool_proxy.bindings.create(context: account)
 
         tools_collection = subject.bookmarked_collection.paginate(per_page: 100).to_a
@@ -162,7 +161,7 @@ module Lti
       it 'has_update set to true for tool proxies with an update_payload' do
         account.root_account.enable_feature!(:lti2_rereg)
 
-        tool_proxy = create_tool_proxy
+        tool_proxy = create_tool_proxy(context: account)
         tool_proxy.bindings.create(context: account)
         tool_proxy.update_payload = {one: 2}
         tool_proxy.save!

spec/models/lti/app_launch_collator_spec.rb

@@ -17,12 +17,13 @@
 #
 
 require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
-require File.expand_path(File.dirname(__FILE__) + '/../../lti_spec_helper.rb')
 
 module Lti
-  describe AppLaunchCollator do
-    let (:account) { Account.create }
-    let (:resource_handler) { ResourceHandler.create(resource_type_code: 'code', name: 'resource name', tool_proxy: tool_proxy) }
+  describe AppLaunchCollator, :include_lti_spec_helpers do
+    let(:account) { Account.create }
+    let(:resource_handler) do
+      ResourceHandler.create(resource_type_code: 'code', name: 'resource name', tool_proxy: tool_proxy)
+    end
 
     describe "#launch_definitions" do