Merge pull request #2441 from kristinalim/fix-scoping_issue_in_sorting_of_tables

Fix front-end sorting in "Customers" index and "Bulk Order Management" page

app/assets/javascripts/admin/customers/controllers/customers_controller.js.coffee

@@ -1,4 +1,4 @@
-angular.module("admin.customers").controller "customersCtrl", ($scope, $q, $filter, Customers, TagRuleResource, CurrentShop, RequestMonitor, Columns, pendingChanges, shops, availableCountries) ->
+angular.module("admin.customers").controller "customersCtrl", ($scope, $q, $filter, Customers, TagRuleResource, CurrentShop, RequestMonitor, Columns, SortOptions, pendingChanges, shops, availableCountries) ->
   $scope.shops = shops
   $scope.availableCountries = availableCountries
   $scope.RequestMonitor = RequestMonitor
@@ -6,6 +6,7 @@ angular.module("admin.customers").controller "customersCtrl", ($scope, $q, $filt
   $scope.customerLimit = 20
   $scope.customers = Customers.all
   $scope.columns = Columns.columns
+  $scope.sorting = SortOptions
 
   $scope.confirmRefresh = (event) ->
     event.preventDefault() unless pendingChanges.unsavedCount() == 0 || confirm(t("unsaved_changes_warning"))

app/assets/javascripts/admin/index_utils/controllers/columns_controller.js.coffee

@@ -1,4 +1,2 @@
 angular.module("admin.indexUtils").controller "ColumnsCtrl", ($scope, Columns) ->
   $scope.columns = Columns.columns
-  $scope.predicate = ""
-  $scope.reverse = false

app/assets/javascripts/admin/index_utils/services/sort_options.js.coffee

@@ -0,0 +1,8 @@
+angular.module("admin.indexUtils").factory 'SortOptions', ->
+  new class SortOptions
+    predicate: ""
+    reverse: true
+
+    toggle: (predicate) ->
+      @reverse = (@predicate == predicate) && !@reverse
+      @predicate = predicate

app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee

@@ -1,4 +1,4 @@
-angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, $http, $q, StatusMessage, Columns, Dereferencer, Orders, LineItems, Enterprises, OrderCycles, VariantUnitManager, RequestMonitor) ->
+angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, $http, $q, StatusMessage, Columns, SortOptions, Dereferencer, Orders, LineItems, Enterprises, OrderCycles, VariantUnitManager, RequestMonitor) ->
   $scope.initialized = false
   $scope.RequestMonitor = RequestMonitor
   $scope.filteredLineItems = []
@@ -10,6 +10,7 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout,
   $scope.selectedUnitsVariant = {}
   $scope.sharedResource = false
   $scope.columns = Columns.columns
+  $scope.sorting = SortOptions
 
   $scope.confirmRefresh = ->
     LineItems.allSaved() || confirm(t("unsaved_changes_warning"))

app/views/admin/customers/index.html.haml

@@ -61,38 +61,39 @@
             -# %th.bulk
               -# %input{ :type => "checkbox", :name => 'toggle_bulk', 'ng-click' => 'toggleAllCheckboxes()', 'ng-checked' => "allBoxesChecked()" }
             %th.email{ 'ng-show' => 'columns.email.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'customer.email'; reverse = !reverse" }=t('admin.email')
+              %a{ :href => '', 'ng-click' => "sorting.toggle('email')" }=t('admin.email')
             %th.name{ 'ng-show' => 'columns.name.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'customer.name'; reverse = !reverse" }=t('admin.name')
+              %a{ :href => '', 'ng-click' => "sorting.toggle('name')" }=t('admin.name')
             %th.code{ 'ng-show' => 'columns.code.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'customer.code'; reverse = !reverse" }=t('admin.customers.index.code')
+              %a{ :href => '', 'ng-click' => "sorting.toggle('code')" }=t('admin.customers.index.code')
             %th.tags{ 'ng-show' => 'columns.tags.visible' }=t('admin.tags')
             %th.bill_address{ 'ng-show' => 'columns.bill_address.visible' }=t('admin.customers.index.bill_address')
             %th.ship_address{ 'ng-show' => 'columns.ship_address.visible' }=t('admin.customers.index.ship_address')
             %th.actions
               Ask? 
               %input{ :type => 'checkbox', 'ng-model' => "confirmDelete" }
-        %tr.customer{ 'ng-repeat' => "customer in filteredCustomers = ( customers | filter:quickSearch | orderBy:predicate:reverse ) | limitTo:customerLimit track by customer.id", 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'", :id => "c_{{customer.id}}" }
-          -# %td.bulk
-            -# %input{ :type => "checkbox", :name => 'bulk', 'ng-model' => 'customer.checked' }
-          %td.email{ 'ng-show' => 'columns.email.visible'}
-            %span{ 'ng-bind' => '::customer.email' }
-            %span.guest-label{ 'ng-show' => 'customer.user_id == null' }= t('.guest_label')
-          %td.name{ 'ng-show' => 'columns.name.visible'}
-            %input{ type: 'text', name: 'name', ng: { model: 'customer.name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'}
-          %td.code{ 'ng-show' => 'columns.code.visible' }
-            %input{ type: 'text', name: 'code', ng: {model: 'customer.code', change: 'checkForDuplicateCodes()'}, "obj-for-update" => "customer", "attr-for-update" => "code" }
-            %i.icon-warning-sign{ ng: {if: 'duplicate'} }
-              = t('.duplicate_code')
-          %td.tags{ 'ng-show' => 'columns.tags.visible' }
-            .tag_watcher{ 'obj-for-update' => "customer", "attr-for-update" => "tag_list"}
-              %tags_with_translation{ object: 'customer', 'find-tags' => 'findTags(query)' }
-          %td.bill_address{ 'ng-show' => 'columns.bill_address.visible' }
-            %a{ id: 'bill-address-link', href: 'javascript:void(0)', "ng-bind" => "customer.bill_address ? customer.bill_address.address1 : '#{t('admin.customers.index.edit')}' | limitTo: 15", 'edit-address-dialog' => true }
-          %td.ship_address{ 'ng-show' => 'columns.ship_address.visible' }
-            %a{ id: 'ship-address-link', href: 'javascript:void(0)', "ng-bind" => "customer.ship_address ? customer.ship_address.address1 : '#{t('admin.customers.index.edit')}' | limitTo: 15", 'edit-address-dialog' => true }
-          %td.actions
-            %a{ 'ng-click' => "deleteCustomer(customer)", :class => "delete-customer icon-trash no-text" }
+        %tbody
+          %tr.customer{ 'ng-repeat' => "customer in filteredCustomers = ( customers | filter:quickSearch | orderBy: sorting.predicate:sorting.reverse ) | limitTo:customerLimit track by customer.id", 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'", :id => "c_{{customer.id}}" }
+            -# %td.bulk
+              -# %input{ :type => "checkbox", :name => 'bulk', 'ng-model' => 'customer.checked' }
+            %td.email{ 'ng-show' => 'columns.email.visible'}
+              %span{ 'ng-bind' => '::customer.email' }
+              %span.guest-label{ 'ng-show' => 'customer.user_id == null' }= t('.guest_label')
+            %td.name{ 'ng-show' => 'columns.name.visible'}
+              %input{ type: 'text', name: 'name', ng: { model: 'customer.name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'}
+            %td.code{ 'ng-show' => 'columns.code.visible' }
+              %input{ type: 'text', name: 'code', ng: {model: 'customer.code', change: 'checkForDuplicateCodes()'}, "obj-for-update" => "customer", "attr-for-update" => "code" }
+              %i.icon-warning-sign{ ng: {if: 'duplicate'} }
+                = t('.duplicate_code')
+            %td.tags{ 'ng-show' => 'columns.tags.visible' }
+              .tag_watcher{ 'obj-for-update' => "customer", "attr-for-update" => "tag_list"}
+                %tags_with_translation{ object: 'customer', 'find-tags' => 'findTags(query)' }
+            %td.bill_address{ 'ng-show' => 'columns.bill_address.visible' }
+              %a{ id: 'bill-address-link', href: 'javascript:void(0)', "ng-bind" => "customer.bill_address ? customer.bill_address.address1 : '#{t('admin.customers.index.edit')}' | limitTo: 15", 'edit-address-dialog' => true }
+            %td.ship_address{ 'ng-show' => 'columns.ship_address.visible' }
+              %a{ id: 'ship-address-link', href: 'javascript:void(0)', "ng-bind" => "customer.ship_address ? customer.ship_address.address1 : '#{t('admin.customers.index.edit')}' | limitTo: 15", 'edit-address-dialog' => true }
+            %td.actions
+              %a{ 'ng-click' => "deleteCustomer(customer)", :class => "delete-customer icon-trash no-text" }
 
       -# %show-more.text-center{ data: "filteredCustomers", limit: "customerLimit", increment: "20" }
       %div.text-center{ ng: { show: "filteredCustomers.length > customerLimit" } }

app/views/spree/admin/orders/bulk_management.html.haml

@@ -118,31 +118,31 @@
             %th.bulk
               %input{ :type => "checkbox", :name => 'toggle_bulk', 'ng-click' => 'toggleAllCheckboxes()', 'ng-checked' => "allBoxesChecked()" }
             %th.order_no{ 'ng-show' => 'columns.order_no.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.number'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.number')" }
                 = t("admin.orders.bulk_management.order_no")
             %th.full_name{ 'ng-show' => 'columns.full_name.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.full_name'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.full_name')" }
                 = t("admin.name")
             %th.email{ 'ng-show' => 'columns.email.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.email'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.email')" }
                 = t("admin.email")
             %th.phone{ 'ng-show' => 'columns.phone.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.phone'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.phone')" }
                 = t("admin.phone")
             %th.date{ 'ng-show' => 'columns.order_date.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.completed_at'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.completed_at')" }
                 = t("admin.orders.bulk_management.order_date")
             %th.producer{ 'ng-show' => 'columns.producer.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'supplier.name'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('supplier.name')" }
                 = t("admin.producer")
             %th.order_cycle{ 'ng-show' => 'columns.order_cycle.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.order_cycle.name'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.order_cycle.name')" }
                 = t("admin.order_cycle")
             %th.hub{ 'ng-show' => 'columns.hub.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'order.distributor.name'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('order.distributor.name')" }
                 = t("admin.shop")
             %th.variant{ 'ng-show' => 'columns.variant.visible' }
-              %a{ :href => '', 'ng-click' => "predicate = 'units_variant.full_name'; reverse = !reverse" }
+              %a{ :href => '', 'ng-click' => "sorting.toggle('units_variant.full_name')" }
                 = t("admin.orders.bulk_management.product_unit")
             %th.quantity{ 'ng-show' => 'columns.quantity.visible' }
               = t("admin.quantity")
@@ -157,7 +157,7 @@
               = t("admin.orders.bulk_management.ask")
               %input{ :type => 'checkbox', 'ng-model' => "confirmDelete" }
 
-        %tr.line_item{ 'ng-repeat' => "line_item in filteredLineItems = ( lineItems | filter:quickSearch | selectFilter:supplierFilter:distributorFilter:orderCycleFilter | variantFilter:selectedUnitsProduct:selectedUnitsVariant:sharedResource | orderBy:predicate:reverse )", 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'", :id => "li_{{line_item.id}}" }
+        %tr.line_item{ 'ng-repeat' => "line_item in filteredLineItems = ( lineItems | filter:quickSearch | selectFilter:supplierFilter:distributorFilter:orderCycleFilter | variantFilter:selectedUnitsProduct:selectedUnitsVariant:sharedResource | orderBy:sorting.predicate:sorting.reverse )", 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'", :id => "li_{{line_item.id}}" }
           %td.bulk
             %input{ :type => "checkbox", :name => 'bulk', 'ng-model' => 'line_item.checked', 'ignore-dirty' => true }
           %td.order_no{ 'ng-show' => 'columns.order_no.visible' } {{ line_item.order.number }}

spec/features/admin/bulk_order_management_spec.rb

@@ -82,6 +82,40 @@ feature %q{
         expect(page).to have_selector "td.max", text: li2.max_quantity.to_s, :visible => true
       end
     end
+
+    describe "sorting of line items" do
+      let!(:o1) { create(:order_with_distributor, state: 'complete', completed_at: Time.zone.now) }
+      let!(:o2) { create(:order_with_distributor, state: 'complete', completed_at: Time.zone.now) }
+      let!(:li1) { create(:line_item, order: o1) }
+      let!(:li2) { create(:line_item, order: o2) }
+
+      before do
+        visit spree.admin_bulk_order_management_path
+      end
+
+      it "sorts by customer name when the customer name header is clicked" do
+        customer_names = [o1.name, o2.name].sort
+
+        within "#listing_orders thead" do
+          click_on "Name"
+        end
+
+        expect(page).to have_selector("#listing_orders .line_item:nth-child(1) .full_name", text: customer_names[0])
+        expect(page).to have_selector("#listing_orders .line_item:nth-child(2) .full_name", text: customer_names[1])
+      end
+
+      it "sorts by customer name in reverse when the customer name header is clicked twice" do
+        customer_names = [o1.name, o2.name].sort.reverse
+
+        within "#listing_orders thead" do
+          click_on "Name"
+          click_on "Name"
+        end
+
+        expect(page).to have_selector("#listing_orders .line_item:nth-child(1) .full_name", text: customer_names[1])
+        expect(page).to have_selector("#listing_orders .line_item:nth-child(2) .full_name", text: customer_names[0])
+      end
+    end
   end
 
   context "altering line item properties" do

spec/features/admin/customers_spec.rb

@@ -48,6 +48,21 @@ feature 'Customers' do
         expect(page).to have_selector "tr#c_#{customer2.id}"
         fill_in "quick_search", with: ""
 
+        # Sorting when the header of a sortable column is clicked
+        customer_emails = [customer1.email, customer2.email].sort
+        within "#customers thead" do
+          click_on "Email"
+        end
+        expect(page).to have_selector("#customers .customer:nth-child(1) .email", text: customer_emails[0])
+        expect(page).to have_selector("#customers .customer:nth-child(2) .email", text: customer_emails[1])
+
+        # Then sorting in reverse when the header is clicked again
+        within "#customers thead" do
+          click_on "Email"
+        end
+        expect(page).to have_selector("#customers .customer:nth-child(1) .email", text: customer_emails[1])
+        expect(page).to have_selector("#customers .customer:nth-child(2) .email", text: customer_emails[0])
+
         # Toggling columns
         expect(page).to have_selector "th.email"
         expect(page).to have_content customer1.email

spec/javascripts/unit/admin/index_utils/controllers/columns_controller_spec.js.coffee

@@ -13,5 +13,3 @@ describe "ColumnsCtrl", ->
 
   it "initialises data", ->
     expect(scope.columns).toEqual Columns.columns
-    expect(scope.predicate).toEqual ""
-    expect(scope.reverse).toEqual false

spec/javascripts/unit/admin/index_utils/services/sort_options_spec.js.coffee

@@ -0,0 +1,44 @@
+describe "SortOptions service", ->
+  SortOptions = null
+
+  beforeEach ->
+    module 'admin.indexUtils'
+    inject (_SortOptions_) ->
+      SortOptions = _SortOptions_
+
+  describe "initialising predicate", ->
+    it "sets predicate to blank", ->
+      expect(SortOptions.predicate).toEqual ""
+
+  describe "initialising reverse", ->
+    it "sets reverse to true", ->
+      expect(SortOptions.reverse).toBe true
+
+  describe "sorting by a column", ->
+    describe "when selecting Column A once", ->
+      it "sorts by Column A", ->
+        SortOptions.toggle("column.a")
+        expect(SortOptions.predicate).toEqual "column.a"
+        expect(SortOptions.reverse).toBe false
+
+    describe "when selecting Column A twice", ->
+      it "sorts by Column A in reverse order", ->
+        SortOptions.toggle("column.a")
+        SortOptions.toggle("column.a")
+        expect(SortOptions.predicate).toEqual "column.a"
+        expect(SortOptions.reverse).toBe true
+
+    describe "when selecting Column A once then selecting Column B once", ->
+      it "sorts by Column B", ->
+        SortOptions.toggle("column.a")
+        SortOptions.toggle("column.b")
+        expect(SortOptions.predicate).toEqual "column.b"
+        expect(SortOptions.reverse).toBe false
+
+    describe "when selecting Column A twice then selecting Column B once", ->
+      it "sorts by Column B in reverse order", ->
+        SortOptions.toggle("column.a")
+        SortOptions.toggle("column.a")
+        SortOptions.toggle("column.b")
+        expect(SortOptions.predicate).toEqual "column.b"
+        expect(SortOptions.reverse).toBe false