fix pagination for tools with the same name

fixes PLAT-816

test-plan:
*create a bunch of external_tools and lti2 tools with the same name
*hit the api here: http://{host}/api/v1/{context}/{context_id}/lti_apps
*it should paginate correctly and return all results across pages

*regression test the external tool selection for modules and assignments

Change-Id: I715d2bf38b796c70ea3f547b0b5c2d85f44f85ea
Reviewed-on: https://gerrit.instructure.com/46828
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Eric Berry <ericb@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Nathan Mills <nathanm@instructure.com>

app/models/context_external_tool.rb

@@ -480,7 +480,7 @@ class ContextExternalTool < ActiveRecord::Base
     scope = ContextExternalTool.shard(context.shard).polymorphic_where(context: contexts).active
     scope = scope.placements(*placements)
     scope = scope.selectable if Canvas::Plugin.value_to_boolean(options[:selectable])
-    scope.order(ContextExternalTool.best_unicode_collation_key('name'))
+    scope.order("#{ContextExternalTool.best_unicode_collation_key('context_external_tools.name')}, context_external_tools.id")
   end
 
   def self.find_external_tool_by_id(id, context)

lib/lti/app_collator.rb

@@ -28,7 +28,7 @@ module Lti
       tool_proxy_collection = BookmarkedCollection.wrap(ToolProxyNameBookmarker, tool_proxy_scope)
       BookmarkedCollection.merge(
         ['external_tools', external_tools_collection],
-        ['message)handlers', tool_proxy_collection]
+        ['message_handlers', tool_proxy_collection]
       )
     end
 
@@ -69,4 +69,4 @@ module Lti
 
 
   end
-end
+end

lib/lti/external_tool_name_bookmarker.rb

@@ -17,22 +17,28 @@
 module Lti
   module ExternalToolNameBookmarker
     def self.bookmark_for(external_tool)
-      external_tool.name
+      [external_tool.name.downcase, external_tool.id]
     end
 
     def self.validate(bookmark)
-      bookmark.is_a?(String)
+      bookmark.is_a?(Array) && bookmark.size == 2 &&
+        bookmark[0].is_a?(String) &&
+        bookmark[1].is_a?(Fixnum)
     end
 
     def self.restrict_scope(scope, pager)
+      name_collation_key = BookmarkedCollection.best_unicode_collation_key('context_external_tools.name')
+      placeholder_collation_key = BookmarkedCollection.best_unicode_collation_key('?')
       if pager.current_bookmark
-        name = pager.current_bookmark
+        bookmark = pager.current_bookmark
         comparison = (pager.include_bookmark ? ">=" : ">")
         scope = scope.where(
-          "name #{comparison} ?",
-          name)
+          " (#{name_collation_key} = #{placeholder_collation_key} AND context_external_tools.id #{comparison} ?) "\
+          "OR #{name_collation_key} #{comparison} #{placeholder_collation_key}",
+          bookmark[0], bookmark[1], bookmark[0])
       end
-      scope.order(:name)
+      scope
     end
+
   end
 end

lib/lti/message_handler_name_bookmarker.rb

@@ -17,22 +17,27 @@
 module Lti
   module MessageHandlerNameBookmarker
     def self.bookmark_for(message_handler)
-      message_handler.resource_handler.name
+      [(message_handler.resource_handler.name || '').downcase, message_handler.id]
     end
 
     def self.validate(bookmark)
-      bookmark.is_a?(String)
+      bookmark.is_a?(Array) && bookmark.size == 2 &&
+        bookmark[0].is_a?(String) &&
+        bookmark[1].is_a?(Fixnum)
     end
 
     def self.restrict_scope(scope, pager)
+      name_collation_key = BookmarkedCollection.best_unicode_collation_key('lti_resource_handlers.name')
+      placeholder_collation_key = BookmarkedCollection.best_unicode_collation_key('?')
       if pager.current_bookmark
-        name = pager.current_bookmark
+        bookmark = pager.current_bookmark
         comparison = (pager.include_bookmark ? ">=" : ">")
         scope = scope.where(
-          "name #{comparison} ?",
-          name)
+          " (#{name_collation_key} = #{placeholder_collation_key} AND lti_message_handlers.id #{comparison} ?) "\
+          "OR #{name_collation_key} #{comparison} #{placeholder_collation_key}",
+          bookmark[0], bookmark[1], bookmark[0])
       end
-      scope.order('lti_resource_handlers.name')
+      scope.order("#{name_collation_key}, lti_message_handlers.id")
     end
   end
 end

lib/lti/tool_proxy_name_bookmarker.rb

@@ -17,22 +17,27 @@
 module Lti
   module ToolProxyNameBookmarker
     def self.bookmark_for(tool_proxy)
-      tool_proxy.name || ''
+      [(tool_proxy.name || '').downcase, tool_proxy.id]
     end
 
     def self.validate(bookmark)
-      bookmark.is_a?(String)
+      bookmark.is_a?(Array) && bookmark.size == 2 &&
+        bookmark[0].is_a?(String) &&
+        bookmark[1].is_a?(Fixnum)
     end
 
     def self.restrict_scope(scope, pager)
+      name_collation_key = BookmarkedCollection.best_unicode_collation_key('lti_tool_proxies.name')
+      placeholder_collation_key = BookmarkedCollection.best_unicode_collation_key('?')
       if pager.current_bookmark
-        name = pager.current_bookmark
+        bookmark = pager.current_bookmark
         comparison = (pager.include_bookmark ? ">=" : ">")
         scope = scope.where(
-          "name #{comparison} ?",
-          name)
+          " (#{name_collation_key} = #{placeholder_collation_key} AND lti_tool_proxies.id #{comparison} ?) "\
+          "OR #{name_collation_key} #{comparison} #{placeholder_collation_key}",
+          bookmark[0], bookmark[1], bookmark[0])
       end
-      scope.order(:name)
+      scope.order("#{name_collation_key}, lti_tool_proxies.id")
     end
   end
 end

spec/models/lti/app_collator_spec.rb

@@ -27,7 +27,7 @@ module Lti
     context 'pagination' do
       it 'paginates correctly' do
         3.times do |_|
-          tp = create_tool_proxy(account: account)
+          tp = create_tool_proxy(account: account, name: 'aaa')
           tp.bindings.create(context: account)
         end
         3.times { |_| new_valid_external_tool(account) }
@@ -99,4 +99,4 @@ module Lti
     end
 
   end
-end
+end