DRY Up LTI Tool links

We can now use context_external_tools_helper to update lti links inside erbs Fixes PLAT-1242 Test Plan: Add an LTI tool using [this xml](https://gist.github.com/defektive/dbd182cb04500e236bde) or use [this url](https://gist.githubusercontent.com/defektive/dbd182cb04500e236bde/raw/4a7939ccb4eca7d604f35f6e593f6e05898682f5/canvas_icon_class-lti_test.xml) check the following cog menus for functioning LTI links - assignments - quizzes - modules - discussions Check the following right side menus for functioning LTI links - Course Home - Course Settings Change-Id: I75f6cf7c5b3e73b25f8bd6c20404e7379bc46ce6 Reviewed-on: https://gerrit.instructure.com/63620 Tested-by: Jenkins Reviewed-by: Nathan Mills <[email protected]> QA-Review: August Thornton <[email protected]> Product-Review: Brad Horrocks <[email protected]>
nikolas · Sep 29, 2015 · 18e5369 · 18e5369
1 parent ab3c67a
commit 18e5369
Show file tree

Hide file tree

Showing 18 changed files with 277 additions and 91 deletions.
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -155,14 +155,21 @@ def external_tools_display_hashes(type, context=@context, custom_settings=[])
       :root_account => @domain_root_account, :current_user => @current_user})
 
     tools.map do |tool|
-      external_tool_display_hash(tool, type, context, custom_settings)
+      external_tool_display_hash(tool, type, {}, context, custom_settings)
     end
   end
 
-  def external_tool_display_hash(tool, type, context=@context, custom_settings=[])
+  def external_tool_display_hash(tool, type, url_params={}, context=@context, custom_settings=[])
+
+    url_params = {
+      id: tool.id,
+      launch_type: type
+    }.merge(url_params)
+
     hash = {
       :title => tool.label_for(type, I18n.locale),
-      :base_url =>  polymorphic_url([context, :external_tool], id: tool.id, launch_type: type)
+      :base_url =>  polymorphic_url([context, :external_tool], url_params),
+      :is_new => tool.integration_type == 'lor'
     }
 
     extension_settings = [:icon_url, :canvas_icon_class] | custom_settings

diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb
@@ -281,6 +281,7 @@
 #
 class CoursesController < ApplicationController
   include SearchHelper
+  include ContextExternalToolsHelper
 
   before_filter :require_user, :only => [:index]
   before_filter :require_context, :only => [:roster, :locks, :create_file, :ping]

diff --git a/app/helpers/context_external_tools_helper.rb b/app/helpers/context_external_tools_helper.rb
@@ -0,0 +1,61 @@
+module ContextExternalToolsHelper
+  def external_tools_menu_items(tools, options={})
+    markup = tools.map do |tool|
+      external_tool_menu_item_tag(tool, options)
+    end
+    raw(markup.join(''))
+  end
+
+  def external_tool_menu_item_tag(tool, options={})
+    defaults = {
+      show_icon: true,
+      in_list: false,
+      url_params: {}
+    }
+
+    options = defaults.merge(options)
+    url_params = options.delete(:url_params)
+
+    if tool.respond_to?(:extension_setting)
+      tool = external_tool_display_hash(tool, options[:settings_key], url_params)
+    elsif !url_params.empty?
+      # url_params were supplied, but we aren't hitting the url helper
+      # we need to make sure the tool url includes the url_params
+      parsed = URI.parse(tool[:base_url])
+      parsed.query = Rack::Utils.parse_nested_query(parsed.query).merge(url_params).to_query
+      tool[:base_url] = parsed.to_s
+    end
+
+    link_attrs =  {
+      href: tool[:base_url]
+    }
+
+    link_attrs[:class] = options[:link_class] if options[:link_class]
+    link = content_tag(:a, link_attrs) do
+      concat(render(partial: 'external_tools/helpers/icon', locals: {tool: tool})) if options[:show_icon]
+      concat(tool[:title])
+      concat(external_tool_new_badge_tag) if tool[:is_new]
+    end
+
+    if options[:in_list]
+      li_attrs = {
+        role: "presentation",
+        class: options[:settings_key]
+      }
+      link = content_tag(:li, li_attrs) { link }
+    end
+
+    raw(link)
+  end
+
+  def external_tool_new_badge_tag
+      # <span class="badge new_badge pull-right"><%= t('links.new_badge', 'New') %></span>
+      tag_attrs = {
+        class: "badge new_badge pull-right"
+      }
+
+      content_tag(:span, tag_attrs) do
+        t('New')
+      end
+  end
+end
diff --git a/app/views/assignments/show.html.erb b/app/views/assignments/show.html.erb
@@ -54,14 +54,7 @@
             <span class="screenreader-only"><%= t('manage', 'Manage') %></span>
           </a>
           <ul id="toolbar-1" class="al-options" role="menu" tabindex="0" aria-hidden="true" aria-expanded="false">
-          <% @assignment_menu_tools.each do |tool_hash| %>
-            <li role="presentation">
-              <a class="menu_tool_link" href="<%= tool_hash[:base_url] %>&assignments[]=<%= @assignment.id %>" tabindex="-1" role="menuitem">
-                <%= render(partial: 'external_tools/icon', locals: {tool: tool_hash, settings_key: :assignment_menu}) %>
-                <%= tool_hash[:title] %>
-              </a>
-            </li>
-          <% end %>
+            <%= external_tools_menu_items(@assignment_menu_tools, {link_class: "menu_tool_link", settings_key: :assignment_menu, in_list: true, url_params: {:assignments => [@assignment.id]}}) %>
           </ul>
         <% end %>
       </div>

diff --git a/app/views/context_modules/_context_module_next.html.erb b/app/views/context_modules/_context_module_next.html.erb
@@ -148,16 +148,7 @@
               class="delete_module_link icon-trash"
               title="<%= t('links.title.delete_module', %{Delete this module}) %>"><%= t('links.text.delete_module', %{Delete}) %></a>
           </li>
-          <% @menu_tools[:module_menu].each do |tool| %>
-            <li>
-              <% tool_path = course_external_tool_path(@context, tool, launch_type: 'module_menu',
-                                                  :modules => [context_module ? context_module.id : "{{ id }}"]) %>
-              <a class="menu_tool_link" href="<%= tool_path %>">
-                <%= render(partial: 'external_tools/icon', locals: {tool: tool, settings_key: :module_menu}) %>
-                <%= tool.label_for(:module_menu, I18n.locale) %>
-              </a>
-            </li>
-          <% end %>
+          <%= external_tools_menu_items(@menu_tools[:module_menu], {link_class: "menu_tool_link", settings_key: :module_menu, in_list: true, url_params: {:modules => [context_module ? context_module.id : "{{ id }}"]}}) %>
         </ul>
       <% end %>
       <span style="display: none;">

diff --git a/app/views/context_modules/_module_item_next.html.erb b/app/views/context_modules/_module_item_next.html.erb
@@ -160,22 +160,16 @@
             <% menu_type_to_class.each do |menu_type, content_class| %>
               <% if !tag || tag.content.is_a?(content_class) %>
                 <%
-                   launch_options = {launch_type: menu_type}
+                   launch_options = {}
                    if menu_type == :file_menu
                      launch_options[:files] = [tag ? tag.content_id : "{{ content_id }}"]
                    else
                      launch_options[:module_items] = [tag ? tag.id : "{{ id }}"]
                    end
                 %>
-                <% @menu_tools[menu_type].each do |tool| %>
-                  <li role="presentation" class="<%= menu_type %>">
-                    <% tool_path = course_external_tool_path(@context, tool, launch_options) %>
-                    <a class="menu_tool_link" href="<%= tool_path %>">
-                      <%= render(partial: 'external_tools/icon', locals: {tool: tool, settings_key: menu_type}) %>
-                      <%= tool.label_for(menu_type, I18n.locale) %>
-                    </a>
-                  </li>
-                <% end %>
+
+                <%= external_tools_menu_items(@menu_tools[menu_type], {link_class: "menu_tool_link", settings_key: menu_type, in_list: true, url_params: launch_options}) %>
+
               <% end %>
             <% end %>
           </ul>

diff --git a/app/views/courses/_settings_sidebar.html.erb b/app/views/courses/_settings_sidebar.html.erb
@@ -1,16 +1,5 @@
 <div class="rs-margin-lr<% unless use_new_styles? %> rs-margin-top<% end %>">
-  <% @course_settings_sub_navigation_tools.each do |tool| %>
-    <a class='btn button-sidebar-wide course-settings-sub-navigation-lti'
-      href="<%= course_external_tool_path(@context, tool, launch_type: 'course_settings_sub_navigation') %>">
-
-      <%= render(partial: 'external_tools/icon', locals: {tool: tool, settings_key: :course_settings_sub_navigation}) %>
-
-      <%= tool.label_for(:course_settings_sub_navigation, I18n.locale) %>
-      <% if tool.integration_type == 'lor' %>
-        <span class="badge new_badge pull-right"><%= t('links.new_badge', 'New') %></span>
-      <% end %>
-    </a>
-  <% end %>
+  <%= external_tools_menu_items(@course_settings_sub_navigation_tools, {link_class: "btn button-sidebar-wide course-settings-sub-navigation-lti", settings_key: :course_settings_sub_navigation}) %>
   <% if can_do(@context, @current_user, :use_student_view) %>
     <%= link_to course_student_view_path(@context), :method => :post, :class => 'btn button-sidebar-wide student_view_button' do %>
       <i class="icon-student-view"></i>

diff --git a/app/views/courses/show.html.erb b/app/views/courses/show.html.erb
@@ -48,13 +48,7 @@
 
     <% if @can_manage_content || @course_home_view != 'feed' || @course_home_sub_navigation_tools.present? %>
       <div class="course-options">
-        <% @course_home_sub_navigation_tools.each do |tool| %>
-          <a class="btn button-sidebar-wide course-home-sub-navigation-lti"
-             href="<%= course_external_tool_path(@context, tool, launch_type: 'course_home_sub_navigation') %>">
-            <%= render(partial: 'external_tools/icon', locals: {tool: tool, settings_key: :course_home_sub_navigation}) %>
-            <%= tool.label_for(:course_home_sub_navigation, I18n.locale) %>
-          </a>
-        <% end %>
+        <%= external_tools_menu_items(@course_home_sub_navigation_tools, {link_class: "btn button-sidebar-wide course-home-sub-navigation-lti", settings_key: :course_home_sub_navigation}) %>
         <% if @can_manage_content %>
           <a class="btn button-sidebar-wide element_toggler choose_home_page_link" aria-controls="edit_course_home_content_form" href="<%= context_url(@context, :context_details_url) %>">
             <i class="icon-target"></i>
@@ -83,12 +77,8 @@
     <% else %>
       <% if @can_manage_content || @course_home_sub_navigation_tools.present? %>
         <div class="secondary-button-group rs-margin-lr rs-margin-top rs-margin-bottom">
+          <%= external_tools_menu_items(@course_home_sub_navigation_tools, {link_class: "btn button-sidebar-wide course-home-sub-navigation-lti", settings_key: :course_home_sub_navigation}) %>
         <% @course_home_sub_navigation_tools.each do |tool| %>
-          <a class="btn button-sidebar-wide course-home-sub-navigation-lti"
-             href="<%= course_external_tool_path(@context, tool, launch_type: 'course_home_sub_navigation') %>">
-            <%= render(partial: 'external_tools/icon', locals: {tool: tool, settings_key: :course_home_sub_navigation}) %>
-            <%= tool.label_for(:course_home_sub_navigation, I18n.locale) %>
-          </a>
         <% end %>
         <% if @can_manage_content %>
             <% js_bundle :course_wizard %>

diff --git a/app/views/discussion_topics/show.html.erb b/app/views/discussion_topics/show.html.erb
@@ -141,14 +141,7 @@
                 <% end %>
 
                 <% if can_do(@context, @current_user, :manage_content) %>
-                  <% (@discussion_topic_menu_tools || []).each do |tool_hash| %>
-                    <li>
-                      <a class="menu_tool_link" href="<%= tool_hash[:base_url] %>&discussion_topics[]=<%= @topic.id %>" tabindex="-1" role="menuitem">
-                        <%= render(partial: 'external_tools/icon', locals: {tool: tool_hash, settings_key: :discussion_topic_menu}) %>
-                        <%= tool_hash[:title] %>
-                      </a>
-                    </li>
-                  <% end %>
+                  <%= external_tools_menu_items((@discussion_topic_menu_tools || []), {link_class: "menu_tool_link", settings_key: :discussion_topic_menu, in_list: true, url_params: {:discussion_topics => [@topic.id]}}) %>
                 <% end %>
               </ul>
             </div>

diff --git a/app/views/external_tools/_icon.html.erb b/app/views/external_tools/_icon.html.erb
diff --git a/app/views/external_tools/helpers/_icon.html.erb b/app/views/external_tools/helpers/_icon.html.erb
@@ -0,0 +1,5 @@
+<% if tool[:canvas_icon_class] %>
+  <i class="<%= tool[:canvas_icon_class] %>"></i>
+<% elsif tool[:icon_url] %>
+  <img class="icon" alt="" src="<%= tool[:icon_url] %>" />
+<% end %>
diff --git a/app/views/quizzes/quizzes/show.html.erb b/app/views/quizzes/quizzes/show.html.erb
@@ -217,15 +217,7 @@
                 <i class="icon-trash"><span class="screenreader-only"><%= admin_delete_title %></span></i> <%= admin_delete_title %>
               </a>
             </li>
-
-            <% (@quiz_menu_tools || []).each do |tool_hash| %>
-              <li role="presentation">
-                <a class="menu_tool_link" href="<%= tool_hash[:base_url] %>&quizzes[]=<%= @quiz.id %>" tabindex="-1" role="menuitem">
-                  <%= render(partial: 'external_tools/icon', locals: {tool: tool_hash, settings_key: :quiz_menu}) %>
-                  <%= tool_hash[:title] %>
-                </a>
-              </li>
-            <% end %>
+            <%= external_tools_menu_items((@quiz_menu_tools || []), {link_class: "menu_tool_link", settings_key: :quiz_menu, in_list: true, url_params: {:quizzes => [@quiz.id]}}) %>
           </ul>
         </div>
 

diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
@@ -131,7 +131,6 @@
       settings_mock = mock()
       settings_mock.stubs(:settings).returns({})
       Canvas::Plugin.stubs(:find).returns(settings_mock)
-
     end
 
     it "uses @real_current_user first" do
@@ -601,7 +600,7 @@
       controller.stubs(:polymorphic_url).returns("http://example.com")
       external_tools = controller.external_tools_display_hashes(:account_navigation, @course)
 
-      expect(external_tools).to eq([{:title=>"bob", :base_url=>"http://example.com", :icon_url=>"http://example.com", :canvas_icon_class => 'icon-commons'}])
+      expect(external_tools).to eq([{:title=>"bob", :base_url=>"http://example.com", :is_new => false, :icon_url=>"http://example.com", :canvas_icon_class => 'icon-commons'}])
     end
   end
 
@@ -643,7 +642,13 @@ def tool_settings(setting, include_class=false)
 
     it 'returns a hash' do
       hash = controller.external_tool_display_hash(@tool, :account_navigation)
-      left_over_keys = hash.keys - [:base_url, :title, :icon_url, :canvas_icon_class]
+      left_over_keys = hash.keys - [:is_new, :base_url, :title, :icon_url, :canvas_icon_class]
+      expect(left_over_keys).to eq []
+    end
+
+    it 'returns a hash' do
+      hash = controller.external_tool_display_hash(@tool, :account_navigation)
+      left_over_keys = hash.keys - [:is_new, :base_url, :title, :icon_url, :canvas_icon_class]
       expect(left_over_keys).to eq []
     end