Correct sorting for numerically named groups

fixes CNVS-14327

Followed the tutorial at: https://gollum.instructure.com/sorting. Used
the database level solution

Groups sort first by group category name and then by group name both on
the server side and the client side.

Updated app/coffeescripts/util/natcompare.coffee because when the tests in
public/javascripts/compiled/ember/student_groups/tests/controllers/student_groups_controller.spec.js
one fails because window.I18n.locale is not defined or is null. Hence I
added a default of 'en-US'.

Added spec for server side sorting check.

Test Plan:
* Create a group set and add groups named 1-9, 10, 110, 28 and whatever
  other numbers you want
* Notice as a teacher that the groups are naturally ordered meaning that
  10 and 110 don't come directly after 1 for example

* Next, login as a student and verify that groups are naturally ordered

* Then on the account level, enable the new student groups option
* As a student go back to the groups page and verify that the ordering
  is natural on the new students group page

* Groups get pulled down from the server in groups of 50. I would
  suggest creating two group sets with the names 1 and 2. Add a couple
  groups to group set 1, then add 50+ groups to group set 2, and then
  add a couple more groups to group set 1 but that would come after the
  groups in group set 1. Then check to make sure that when the page
  loads all of group set 1's groups appear at the top of the page in the
  correct order.

Change-Id: Ie066abd3cef237fd8c8c0f73231cea0177756e21
Reviewed-on: https://gerrit.instructure.com/43565
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Braden Anderson <braden@instructure.com>
QA-Review: Steven Shepherd <sshepherd@instructure.com>
Product-Review: Janelle Seegmiller <jseegmiller@instructure.com>

app/coffeescripts/collections/GroupCollection.coffee

@@ -20,11 +20,12 @@ define [
   'compiled/collections/PaginatedCollection'
   'compiled/collections/GroupUserCollection'
   'compiled/models/Group'
-], (PaginatedCollection, GroupUserCollection, Group) ->
+  'compiled/util/natcompare'
+], (PaginatedCollection, GroupUserCollection, Group, natcompare) ->
 
   class GroupCollection extends PaginatedCollection
     model: Group
-    comparator: (group) -> group.get('name').toLowerCase()
+    comparator: natcompare.byGet('name')
 
     @optionProperty 'category'
     @optionProperty 'loadAll'

app/coffeescripts/ember/student_groups/controllers/student_groups_controller.coffee

@@ -1,7 +1,8 @@
 define [
   'i18n!student_groups'
   'ember'
-], (I18n,Ember) ->
+  'compiled/util/natcompare'
+], (I18n,Ember, natcompare) ->
 
   StudentGroupsController = Ember.ObjectController.extend
     groups: []
@@ -18,16 +19,24 @@ define [
     groupsUrl: "/api/v1/courses/#{ENV.course_id}/groups?include[]=users&include[]=group_category"
     studentCanOrganizeGroupsForCourse: ENV.STUDENT_CAN_ORGANIZE_GROUPS_FOR_COURSE
     sortedGroups: (->
-      groups = @get('groups') || []
+      groups = @get('groups')?.toArray() || []
       text = @get('filterText').toLowerCase()
       groups = groups.filter (group) ->
+
         text.length == 0 or
           group.name.toLowerCase().indexOf(text) >= 0 or
           group.users.find (user) ->
             (user.display_name and user.display_name.toLowerCase().indexOf(text) >= 0) or
-            (user.name and user.name.toLowerCase().indexOf(text) >= 0)
+              (user.name and user.name.toLowerCase().indexOf(text) >= 0)
 
-      groups.sortBy('group_category_id')
+      groups.sort (group1, group2) ->
+        group1CategoryName = group1.group_category.name
+        group2CategoryName = group2.group_category.name
+        
+        if group1CategoryName != group2CategoryName
+          natcompare.strings group1CategoryName, group2CategoryName
+        else
+          natcompare.strings group1.name, group2.name
     ).property('groups.[]', 'filterText')
 
     removeFromCategory: (categoryId) ->

app/coffeescripts/ember/student_groups/tests/controllers/student_groups_controller.spec.coffee

@@ -19,14 +19,28 @@ define [
       groups = null
       run =>
         groups = [
+           id: 5
+           name: "9"
+           group_category_id: 1
+           group_category: { name: "1" }
+           users: [{id: 1, name: "steve"}, {id: 2, name: "cliff"}, {id: 3, name: "walt"}]
+         ,
            id: 1
-           name: "one"
+           name: "11"
            group_category_id: 2
+           group_category: { name: "2" }
+           users: [{id: 1, name: "steve"}, {id: 2, name: "cliff"}, {id: 3, name: "walt"}, {id: 5, name: "bobby"}]
+         ,
+           id: 3
+           name: "1"
+           group_category_id: 2
+           group_category: { name: "2" }
            users: [{id: 1, name: "steve"}, {id: 2, name: "cliff"}, {id: 3, name: "walt"}, {id: 4, name: "pinkman"}]
          ,
            id: 2
-           name: "two"
+           name: "2"
            group_category_id: 1
+           group_category: { name: "2" }
            users: [{id: 1, name: "steve"}, {id: 2, name: "cliff"}, {id: 3, name: "walt"}]
          ]
         @sgc.set('groups', groups)
@@ -37,15 +51,18 @@ define [
       Ember.run App, 'destroy'
 
 
-  test 'Groups are sorted group category id', ->
+  test 'Groups are sorted by localeCompare', ->
     sorted = @sgc.get('sortedGroups')
-    equal sorted[0].name, "two"
+    equal sorted[0].name, "9"
+    equal sorted[1].name, "1"
+    equal sorted[2].name, "2"
+    equal sorted[3].name, "11"
 
   test 'filterText will filter groups by user name', ->
     @sgc.set('filterText', "pink")
     sorted = @sgc.get('sortedGroups')
     equal sorted.length, 1
-    equal sorted[0].name, "one"
+    equal sorted[0].name, "1"
 
 
   test 'is member of category should be true if the current user is', ->

app/coffeescripts/util/natcompare.coffee

@@ -1,6 +1,6 @@
 define [], ->
   strings: (x, y) ->
-    x.localeCompare(y, window.I18n.locale, { sensitivity: 'accent', ignorePunctuation: true, numeric: true})
+    x.localeCompare(y, window.I18n.locale || 'en-US', { sensitivity: 'accent', ignorePunctuation: true, numeric: true})
 
   by: (f) ->
     return (x, y) =>

app/controllers/groups_controller.rb

@@ -225,8 +225,9 @@ class GroupsController < ApplicationController
   # @returns [Group]
   def context_index
     return unless authorized_action(@context, @current_user, :read_roster)
-
-    @groups      = all_groups = @context.groups.active.by_name
+    @groups      = all_groups = @context.groups.active
+                                  .order(GroupCategory::Bookmarker.order_by, Group::Bookmarker.order_by)
+                                  .includes(:group_category)
     @categories  = @context.group_categories.order("role <> 'student_organized'", GroupCategory.best_unicode_collation_key('name'))
     @user_groups = @current_user.group_memberships_for(@context) if @current_user
 
@@ -282,8 +283,8 @@ class GroupsController < ApplicationController
 
       format.json do
         path = send("api_v1_#{@context.class.to_s.downcase}_user_groups_url")
-        paginated_groups = Api.paginate(all_groups, self, path)
-        render :json => paginated_groups.map { |g| group_json(g, @current_user, session, :include => Array(params[:include])) }
+        @paginated_groups = Api.paginate(all_groups, self, path)
+        render :json => @paginated_groups.map { |g| group_json(g, @current_user, session, :include => Array(params[:include])) }
       end
     end
   end

app/models/group_category.rb

@@ -80,8 +80,10 @@ class GroupCategory < ActiveRecord::Base
     end
   end
 
+  Bookmarker = BookmarkedCollection::SimpleBookmarker.new(GroupCategory, :name, :id)
+  
+  scope :by_name, -> { order(Bookmarker.order_by) }
   scope :active, -> { where(:deleted_at => nil) }
-
   scope :other_than, lambda { |cat| where("group_categories.id<>?", cat.id || 0) }
 
   class << self

spec/controllers/groups_controller_spec.rb

@@ -52,6 +52,49 @@ describe GroupsController do
       expect(assigns[:groups] - [g1,g2,g3]).to be_empty
       expect(assigns[:categories].length).to eql(2)
     end
+
+    it "should return groups in sorted by group category name, then group name for student view" do
+      skip "requires pg_collkey on the server" if Group.connection.select_value("SELECT COUNT(*) FROM pg_proc WHERE proname='collkey'").to_i == 0
+      user_session(@student)
+      category1 = @course.group_categories.create(:name => "1")
+      category2 = @course.group_categories.create(:name => "2")
+      category3 = @course.group_categories.create(:name => "11")
+      groups = []
+      groups << @course.groups.create(:name => "11", :group_category => category1)
+      groups << @course.groups.create(:name => "2", :group_category => category1)
+      groups << @course.groups.create(:name => "1", :group_category => category1)
+      groups << @course.groups.create(:name => "22", :group_category => category2)
+      groups << @course.groups.create(:name => "2", :group_category => category2)
+      groups << @course.groups.create(:name => "3", :group_category => category2)
+      groups << @course.groups.create(:name => "4", :group_category => category3)
+      groups << @course.groups.create(:name => "44", :group_category => category3)
+      groups << @course.groups.create(:name => "4.5", :group_category => category3)
+      groups.each {|g| g.add_user @student, 'accepted' }
+      get 'index', :course_id => @course.id, :per_page => 50, :format => 'json'
+      expect(response).to be_success
+      expect(assigns[:paginated_groups]).not_to be_empty
+      expect(assigns[:paginated_groups].length).to eql(9)
+      #Check group category ordering
+      expect(assigns[:paginated_groups][0].group_category.name).to eql("1")
+      expect(assigns[:paginated_groups][1].group_category.name).to eql("1")
+      expect(assigns[:paginated_groups][2].group_category.name).to eql("1")
+      expect(assigns[:paginated_groups][3].group_category.name).to eql("2")
+      expect(assigns[:paginated_groups][4].group_category.name).to eql("2")
+      expect(assigns[:paginated_groups][5].group_category.name).to eql("2")
+      expect(assigns[:paginated_groups][6].group_category.name).to eql("11")
+      expect(assigns[:paginated_groups][7].group_category.name).to eql("11")
+      expect(assigns[:paginated_groups][8].group_category.name).to eql("11")
+      #Check group name ordering
+      expect(assigns[:paginated_groups][0].name).to eql("1")
+      expect(assigns[:paginated_groups][1].name).to eql("2")
+      expect(assigns[:paginated_groups][2].name).to eql("11")
+      expect(assigns[:paginated_groups][3].name).to eql("2")
+      expect(assigns[:paginated_groups][4].name).to eql("3")
+      expect(assigns[:paginated_groups][5].name).to eql("22")
+      expect(assigns[:paginated_groups][6].name).to eql("4")
+      expect(assigns[:paginated_groups][7].name).to eql("4.5")
+      expect(assigns[:paginated_groups][8].name).to eql("44")
+    end
   end
 
   describe "GET index" do