don't require url or domain for canvas lti-extensioned tools

When tools have one of the Canvas LTI extensions set, they shouldn't be required to have a url or domain. In that case they also shouldn't show up in the list of tools that can be used to add items to a module. test plan: - try adding a tool by hand with no url or domain - make sure canvas doesn't let you - try adding a tool using xml (examples at lti-examples.heroku.com) that doesn't have url or domain, but does have a canvas lti extension (any extension other than resource_selection) - make sure canvas lets you - this non-url non-domain tool shouldn't show up in the add modules list - try adding a tool using xml (examples at lti-examples.heroku.com) that doesn't have url or domain, but does have a canvas lti extension (specifically resource_selection) - make sure canvas lets you - this non-url non-domain tool should show up in the add modules list Change-Id: Ie575c52da610f41b1afe09fbd838b0fd0da39ff2 Reviewed-on: https://gerrit.instructure.com/10251 Tested-by: Jenkins <[email protected]> Reviewed-by: Bracken Mosbacker <[email protected]>
rthbound · May 25, 2012 · ab2dceb · ab2dceb
1 parent 579bab1
commit ab2dceb
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 9 deletions.
diff --git a/app/models/context_external_tool.rb b/app/models/context_external_tool.rb
@@ -33,10 +33,13 @@ class ContextExternalTool < ActiveRecord::Base
   end
 
   def url_or_domain_is_set
+    setting_types = [:user_navigation, :course_navigation, :account_navigation, :resource_selection, :editor_button]
+    # both url and domain should not be set
     if url.present? && domain.present?
       errors.add(:url, t('url_or_domain_not_both', "Either the url or domain should be set, not both."))
       errors.add(:domain, t('url_or_domain_not_both', "Either the url or domain should be set, not both."))
-    elsif url.blank? && domain.blank?
+    # url or domain (or url on canvas lti extension) is required
+    elsif url.blank? && domain.blank? && setting_types.all?{|k| !settings[k] || settings[k]['url'].blank? }
       errors.add(:url, t('url_or_domain_required', "Either the url or domain should be set."))
       errors.add(:domain, t('url_or_domain_required', "Either the url or domain should be set."))
     end

diff --git a/public/javascripts/select_content_dialog.js b/public/javascripts/select_content_dialog.js
@@ -257,14 +257,16 @@ $(document).ready(function() {
           $select.find(".tools").empty();
           for(var idx in data) {
             var tool = data[idx];
-            var $tool = $tool_template.clone(true);
-            $tool.toggleClass('resource_selection', !!tool.resource_selection_settings);
-            $tool.fillTemplateData({
-              data: tool,
-              dataValues: ['id', 'url', 'domain', 'name']
-            });
-            $tool.data('tool', tool);
-            $select.find(".tools").append($tool.show());
+            if(tool.url || tool.domain || tool.resource_selection_settings) {
+              var $tool = $tool_template.clone(true);
+              $tool.toggleClass('resource_selection', !!tool.resource_selection_settings);
+              $tool.fillTemplateData({
+                data: tool,
+                dataValues: ['id', 'url', 'domain', 'name']
+              });
+              $tool.data('tool', tool);
+              $select.find(".tools").append($tool.show());
+            }
           }
         }, function(data) {
           $select.find(".message").text(I18n.t('errors.loading_failed', "Loading Failed"));

diff --git a/spec/controllers/external_tools_controller_spec.rb b/spec/controllers/external_tools_controller_spec.rb
@@ -112,6 +112,33 @@
       assigns[:tool].shared_secret.should == "secret"
     end
 
+    it "should fail on basic xml with no url or domain set" do
+      rescue_action_in_public!
+      course_with_teacher_logged_in(:active_all => true)
+      xml = <<-XML
+<?xml version="1.0" encoding="UTF-8"?>
+<cartridge_basiclti_link xmlns="http://www.imsglobal.org/xsd/imslticc_v1p0"
+    xmlns:blti = "http://www.imsglobal.org/xsd/imsbasiclti_v1p0"
+    xmlns:lticm ="http://www.imsglobal.org/xsd/imslticm_v1p0"
+    xmlns:lticp ="http://www.imsglobal.org/xsd/imslticp_v1p0"
+    xmlns:xsi = "http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation = "http://www.imsglobal.org/xsd/imslticc_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticc_v1p0.xsd
+    http://www.imsglobal.org/xsd/imsbasiclti_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imsbasiclti_v1p0.xsd
+    http://www.imsglobal.org/xsd/imslticm_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticm_v1p0.xsd
+    http://www.imsglobal.org/xsd/imslticp_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticp_v1p0.xsd">
+    <blti:title>Other Name</blti:title>
+    <blti:description>Description</blti:description>
+    <blti:extensions platform="canvas.instructure.com">
+      <lticm:property name="privacy_level">public</lticm:property>
+    </blti:extensions>
+    <cartridge_bundle identifierref="BLTI001_Bundle"/>
+    <cartridge_icon identifierref="BLTI001_Icon"/>
+</cartridge_basiclti_link>  
+      XML
+      post 'create', :course_id => @course.id, :external_tool => {:name => "tool name", :consumer_key => "key", :shared_secret => "secret", :config_type => "by_xml", :config_xml => xml}
+      response.should_not be_success
+    end
+
     it "should handle advanced xml configurations" do
       course_with_teacher_logged_in(:active_all => true)
       xml = <<-XML
@@ -154,6 +181,48 @@
       assigns[:tool].has_editor_button.should be_true
     end
 
+    it "should handle advanced xml configurations with no url or domain set" do
+      course_with_teacher_logged_in(:active_all => true)
+      xml = <<-XML
+<?xml version="1.0" encoding="UTF-8"?>
+<cartridge_basiclti_link xmlns="http://www.imsglobal.org/xsd/imslticc_v1p0"
+    xmlns:blti = "http://www.imsglobal.org/xsd/imsbasiclti_v1p0"
+    xmlns:lticm ="http://www.imsglobal.org/xsd/imslticm_v1p0"
+    xmlns:lticp ="http://www.imsglobal.org/xsd/imslticp_v1p0"
+    xmlns:xsi = "http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation = "http://www.imsglobal.org/xsd/imslticc_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticc_v1p0.xsd
+    http://www.imsglobal.org/xsd/imsbasiclti_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imsbasiclti_v1p0.xsd
+    http://www.imsglobal.org/xsd/imslticm_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticm_v1p0.xsd
+    http://www.imsglobal.org/xsd/imslticp_v1p0 http://www.imsglobal.org/xsd/lti/ltiv1p0/imslticp_v1p0.xsd">
+    <blti:title>Other Name</blti:title>
+    <blti:description>Description</blti:description>
+    <blti:extensions platform="canvas.instructure.com">
+      <lticm:property name="privacy_level">public</lticm:property>
+      <lticm:options name="editor_button">
+        <lticm:property name="url">http://example.com/editor</lticm:property>
+        <lticm:property name="icon_url">http://example.com/icon.png</lticm:property>
+        <lticm:property name="text">Editor Button</lticm:property>
+        <lticm:property name="selection_width">500</lticm:property>
+        <lticm:property name="selection_height">300</lticm:property>
+      </lticm:options>
+    </blti:extensions>
+    <cartridge_bundle identifierref="BLTI001_Bundle"/>
+    <cartridge_icon identifierref="BLTI001_Icon"/>
+</cartridge_basiclti_link>  
+      XML
+      post 'create', :course_id => @course.id, :external_tool => {:name => "tool name", :consumer_key => "key", :shared_secret => "secret", :config_type => "by_xml", :config_xml => xml}
+      response.should be_success
+      assigns[:tool].should_not be_nil
+      # User-entered name overrides name provided in xml
+      assigns[:tool].name.should == "tool name"
+      assigns[:tool].description.should == "Description"
+      assigns[:tool].url.should be_nil
+      assigns[:tool].domain.should be_nil
+      assigns[:tool].consumer_key.should == "key"
+      assigns[:tool].shared_secret.should == "secret"
+      assigns[:tool].has_editor_button.should be_true
+    end
+
     it "should fail gracefully on invalid xml configurations" do
       course_with_teacher_logged_in(:active_all => true)
       xml = "bob"

diff --git a/spec/models/context_external_tool_spec.rb b/spec/models/context_external_tool_spec.rb
@@ -29,6 +29,40 @@
     @account.parent_account.should eql(@root_account)
     @account.root_account.should eql(@root_account)
   end
+  describe "url or domain validation" do
+    it "should validate with a domain setting" do
+      @tool = @course.context_external_tools.create(:name => "a", :domain => "google.com", :consumer_key => '12345', :shared_secret => 'secret')
+      @tool.should_not be_new_record
+      @tool.errors.should be_empty
+    end
+
+    it "should validate with a url setting" do
+      @tool = @course.context_external_tools.create(:name => "a", :url => "http://google.com", :consumer_key => '12345', :shared_secret => 'secret')
+      @tool.should_not be_new_record
+      @tool.errors.should be_empty
+    end
+
+    it "should validate with a canvas lti extension url setting" do
+      @tool = @course.context_external_tools.new(:name => "a", :consumer_key => '12345', :shared_secret => 'secret')
+      @tool.settings[:editor_button] = {
+        "icon_url"=>"http://www.example.com/favicon.ico", 
+        "text"=>"Example",
+        "url"=>"http://www.example.com", 
+        "selection_height"=>400, 
+        "selection_width"=>600
+      }
+      @tool.save
+      @tool.should_not be_new_record
+      @tool.errors.should be_empty
+    end
+
+    it "should not validate with no domain or url setting" do
+      @tool = @course.context_external_tools.create(:name => "a", :consumer_key => '12345', :shared_secret => 'secret')
+      @tool.should be_new_record
+      @tool.errors['url'].should == "Either the url or domain should be set."
+      @tool.errors['domain'].should == "Either the url or domain should be set."
+    end
+  end
   describe "find_external_tool" do
     it "should match on the same domain" do
       @tool = @course.context_external_tools.create!(:name => "a", :domain => "google.com", :consumer_key => '12345', :shared_secret => 'secret')

diff --git a/spec/selenium/external_tools_spec.rb b/spec/selenium/external_tools_spec.rb
@@ -73,6 +73,31 @@
     @tag.url.should == "http://www.example.com"
   end
 
+  it "should not list external tools that don't have a url, domain, or resource_selection configured" do
+    @module = @course.context_modules.create!(:name => "module")
+
+    @tool1 = @course.context_external_tools.create!(:name => "First Tool", :url => "http://www.example.com", :consumer_key => "key", :shared_secret => "secret")
+    @tool2 = @course.context_external_tools.new(:name => "Another Tool", :consumer_key => "key", :shared_secret => "secret")
+    @tool2.settings[:editor_button] = {:url => "http://www.example.com", :icon_url => "http://www.example.com", :selection_width => 100, :selection_height => 100}.with_indifferent_access
+    @tool2.save!
+    @tool3 = @course.context_external_tools.new(:name => "Third Tool", :consumer_key => "key", :shared_secret => "secret")
+    @tool3.settings[:resource_selection] = {:url => "http://www.example.com", :icon_url => "http://www.example.com", :selection_width => 100, :selection_height => 100}.with_indifferent_access
+    @tool3.save!
+
+    get "/courses/#{@course.id}/modules"
+
+    keep_trying_until { driver.execute_script("return window.modules.refreshed == true") }
+
+    driver.find_element(:css, "#context_module_#{@module.id} .add_module_item_link").click
+    driver.find_element(:css, "#add_module_item_select option[value='context_external_tool']").click
+
+    keep_trying_until { driver.find_elements(:css, "#context_external_tools_select .tool .name").length > 0 }
+    names = driver.find_elements(:css, "#context_external_tools_select .tool .name").map(&:text)
+    names.should be_include(@tool1.name)
+    names.should_not be_include(@tool2.name)
+    names.should be_include(@tool3.name)
+  end
+
   it "should allow adding an existing external tool to a course module, and should pick the correct tool" do
     @module = @course.context_modules.create!(:name => "module")
     @tool1 = @course.context_external_tools.create!(:name => "a", :url => "http://www.google.com", :consumer_key => '12345', :shared_secret => 'secret')