Make columns of user and ordergroup lists sortable

This commit implements the sort functionality for the user lists (by name, email, last_activity) and ordergroup lists (by name).
It is a first attempt addressing issue #560.

app/controllers/admin/ordergroups_controller.rb

@@ -2,7 +2,7 @@ class Admin::OrdergroupsController < Admin::BaseController
   inherit_resources
 
   def index
-    @ordergroups = Ordergroup.undeleted.order('name ASC')
+    @ordergroups = Ordergroup.undeleted.sort_by_param(params["sort"])
 
     if request.format.csv?
       send_data OrdergroupsCsv.new(@ordergroups).to_csv, filename: 'ordergroups.csv', type: 'text/csv'

app/controllers/admin/users_controller.rb

@@ -3,6 +3,8 @@ class Admin::UsersController < Admin::BaseController
 
   def index
     @users = params[:show_deleted] ? User.deleted : User.undeleted
+    @users = @users.sort_by_param(params["sort"])
+
     @users = @users.includes(:mail_delivery_status)
 
     if request.format.csv?
@@ -12,7 +14,7 @@ class Admin::UsersController < Admin::BaseController
     # if somebody uses the search field:
     @users = @users.natural_search(params[:user_name]) unless params[:user_name].blank?
 
-    @users = @users.natural_order.page(params[:page]).per(@per_page)
+    @users = @users.page(params[:page]).per(@per_page)
   end
 
   def destroy

app/controllers/foodcoop/ordergroups_controller.rb

@@ -1,6 +1,6 @@
 class Foodcoop::OrdergroupsController < ApplicationController
   def index
-    @ordergroups = Ordergroup.undeleted.order('name')
+    @ordergroups = Ordergroup.undeleted.sort_by_param(params["sort"])
 
     unless params[:name].blank? # Search by name
       @ordergroups = @ordergroups.where('name LIKE ?', "%#{params[:name]}%")

app/controllers/foodcoop/users_controller.rb

@@ -1,6 +1,6 @@
 class Foodcoop::UsersController < ApplicationController
   def index
-    @users = User.undeleted.natural_order
+    @users = User.undeleted.sort_by_param(params["sort"])
 
     # if somebody uses the search field:
     @users = @users.natural_search(params[:user_name]) unless params[:user_name].blank?

app/models/ordergroup.rb

@@ -148,6 +148,29 @@ class Ordergroup < Group
     financial_transactions.last.try(:created_on) || created_on
   end
 
+  def self.sort_by_param(param)
+    param ||= "name"
+
+    sort_param_map = {
+      "name" => "name",
+      "name_reverse" => "name DESC",
+      "members_count" => "count(users.id)",
+      "members_count_reverse" => "count(users.id) DESC",
+      "last_user_activity" => "max(users.last_activity)",
+      "last_user_activity_reverse" => "max(users.last_activity) DESC",
+      "last_order" => "max(orders.starts)",
+      "last_order_reverse" => "max(orders.starts) DESC"
+    }
+
+    result = self
+    result = result.left_joins(:users).group("groups.id") if param.starts_with?("members_count", "last_user_activity")
+    result = result.left_joins(:orders).group("groups.id") if param.starts_with?("last_order")
+
+    # Never pass user input data to Arel.sql() because of SQL Injection vulnerabilities.
+    # This case here is okay, as param is mapped to the actual order string.
+    result.order(Arel.sql(sort_param_map[param]))
+  end
+
   private
 
   # Make sure, that a user can only be in one ordergroup

app/models/user.rb

@@ -250,4 +250,27 @@ class User < ApplicationRecord
     #   this should not be part of the model anyway
     { :id => id, :name => "#{display} (#{ordergroup.try(:name)})" }
   end
+
+  def self.sort_by_param(param)
+    param ||= "name"
+
+    sort_param_map = {
+      "nick" => "nick",
+      "nick_reverse" => "nick DESC",
+      "name" => "first_name, last_name",
+      "name_reverse" => "first_name DESC, last_name DESC",
+      "email" => "users.email",
+      "email_reverse" => "users.email DESC",
+      "phone" => "phone",
+      "phone_reverse" => "phone DESC",
+      "last_activity" => "last_activity",
+      "last_activity_reverse" => "last_activity DESC",
+      "ordergroup" => "IFNULL(groups.type, '') <> 'Ordergroup', groups.name",
+      "ordergroup_reverse" => "IFNULL(groups.type, '') <> 'Ordergroup', groups.name DESC"
+    }
+
+    # Never pass user input data to Arel.sql() because of SQL Injection vulnerabilities.
+    # This case here is okay, as param is mapped to the actual order string.
+    self.eager_load(:groups).order(Arel.sql(sort_param_map[param])) # eager_load is like left_join but without duplicates
+  end
 end

app/views/admin/ordergroups/_ordergroups.html.haml

@@ -4,10 +4,11 @@
 %table.table.table-striped
   %thead
     %tr
-      %th= heading_helper Ordergroup, :name
+      %th= sort_link_helper heading_helper(Ordergroup, :name), "name"
-      %th= heading_helper Ordergroup, :user_tokens
+      %th= sort_link_helper heading_helper(Ordergroup, :user_tokens), "members_count"
       %th= heading_helper Ordergroup, :contact_address
-      %th= heading_helper Ordergroup, :last_user_activity
+      %th= sort_link_helper heading_helper(Ordergroup, :last_user_activity), "last_user_activity"
+      %th= sort_link_helper heading_helper(Ordergroup, :last_order), "last_order"
       %th= t 'ui.actions'
   %tbody
     - for ordergroup in @ordergroups
@@ -17,6 +18,7 @@
           %abbr{title: ordergroup_members_title(ordergroup)}= ordergroup.users.size
         %td= link_to_gmaps ordergroup.contact_address
         %td= format_date ordergroup.last_user_activity
+        %td= format_date ordergroup.last_order.try(:starts)
         %td
           = link_to t('ui.edit'), edit_admin_ordergroup_path(ordergroup), class: 'btn btn-mini'
           = link_to t('ui.delete'), [:admin, ordergroup], :data => {:confirm => t('ui.confirm_delete', name: ordergroup.name)},

app/views/admin/users/_users.html.haml

@@ -5,11 +5,11 @@
   %thead
     %tr
       - if FoodsoftConfig[:use_nick]
-        %th= heading_helper User, :nick
+        %th= sort_link_helper heading_helper(User, :nick), "nick"
-      %th= heading_helper User, :name
+      %th= sort_link_helper heading_helper(User, :name), "name"
-      %th= heading_helper User, :email
+      %th= sort_link_helper heading_helper(User, :email), "email"
       %th= t 'admin.access_to'
-      %th= heading_helper User, :last_activity
+      %th= sort_link_helper heading_helper(User, :last_activity), "last_activity"
       %th(colspan="2")= t 'ui.actions'
   %tbody
     - for user in @users

app/views/foodcoop/ordergroups/_ordergroups.html.haml

@@ -5,11 +5,11 @@
 %table.table.table-striped
   %thead
     %tr
-      %th= heading_helper Ordergroup, :name
+      %th= sort_link_helper heading_helper(Ordergroup, :name), "name"
       %th= heading_helper Ordergroup, :user_tokens
       %th= heading_helper Ordergroup, :break
-      %th= heading_helper Ordergroup, :last_user_activity
+      %th= sort_link_helper heading_helper(Ordergroup, :last_user_activity), "last_user_activity"
-      %th= heading_helper Ordergroup, :last_order
+      %th= sort_link_helper heading_helper(Ordergroup, :last_order), "last_order"
 
   %tbody
     - for ordergroup in @ordergroups

app/views/foodcoop/users/_users.html.haml

@@ -5,11 +5,11 @@
   %thead
     %tr
       - if FoodsoftConfig[:use_nick]
-        %th= heading_helper User, :nick
+        %th= sort_link_helper heading_helper(User, :nick), "nick"
-      %th= heading_helper User, :name
+      %th= sort_link_helper heading_helper(User, :name), "name"
-      %th= heading_helper User, :email
+      %th= sort_link_helper heading_helper(User, :email), "email"
-      %th= heading_helper User, :phone
+      %th= sort_link_helper heading_helper(User, :phone), "phone"
-      %th= heading_helper User, :ordergroup
+      %th= sort_link_helper heading_helper(User, :ordergroup), "ordergroup"
       %th= heading_helper User, :workgroup, count: 3
   %tbody
     - for user in @users

db/seeds/small.en.seeds.rb

@@ -142,11 +142,12 @@ Article.create!(:name => "Coconut grated", :supplier_id => 4, :article_category_
 
 ## Members & groups
 
-User.create!(:id => 1, :nick => "admin", :password => "secret", :first_name => "Anton", :last_name => "Administrator", :email => "admin@foo.test", :created_on => 'Wed, 15 Jan 2014 16:15:33 UTC +00:00')
+User.create!(:id => 1, :nick => "admin", :password => "secret", :first_name => "Anton", :last_name => "Administrator", :email => "admin@foo.test", :phone => "+4421486548", :created_on => 'Wed, 15 Jan 2014 16:15:33 UTC +00:00')
 User.create!(:id => 2, :nick => "john", :password => "secret", :first_name => "John", :last_name => "Doe", :email => "john@doe.test", :created_on => 'Sun, 19 Jan 2014 17:38:22 UTC +00:00')
-User.create!(:id => 3, :nick => "peter", :password => "secret", :first_name => "Peter", :last_name => "Peters", :email => "peter@peters.test", :created_on => 'Sat, 25 Jan 2014 20:20:36 UTC +00:00')
+User.create!(:id => 3, :nick => "peter", :password => "secret", :first_name => "Peter", :last_name => "Peters", :email => "peter@peters.test", :phone => "+4711235486811", :created_on => 'Sat, 25 Jan 2014 20:20:36 UTC +00:00')
 User.create!(:id => 4, :nick => "jan", :password => "secret", :first_name => "Jan", :last_name => "Lou", :email => "jan@lou.test", :created_on => 'Mon, 27 Jan 2014 16:22:14 UTC +00:00')
 User.create!(:id => 5, :nick => "mary", :password => "secret", :first_name => "Mary", :last_name => "Lou", :email => "marie@lou.test", :created_on => 'Mon, 03 Feb 2014 11:47:17 UTC +00:00')
+User.find_by_nick("mary").update(last_activity: 5.days.ago)
 
 Workgroup.create!(:id => 1, :name => "Administrators", :description => "System administrators.", :account_balance => 0.0, :created_on => 'Wed, 15 Jan 2014 16:15:33 UTC +00:00', :role_admin => true, :role_suppliers => true, :role_article_meta => true, :role_finance => true, :role_orders => true, :next_weekly_tasks_number => 8, :ignore_apple_restriction => false)
 Workgroup.create!(:id => 2, :name => "Finances", :account_balance => 0.0, :created_on => 'Sun, 19 Jan 2014 17:40:03 UTC +00:00', :role_admin => false, :role_suppliers => false, :role_article_meta => false, :role_finance => true, :role_orders => false, :next_weekly_tasks_number => 8, :ignore_apple_restriction => false)
@@ -172,8 +173,8 @@ Membership.create!(:group_id => 4, :user_id => 5)
 ## Orders & OrderArticles
 
 seed_order(supplier_id: 1, starts: 2.days.ago, ends: 5.days.from_now)
-seed_order(supplier_id: 3, starts: 2.days.ago, ends: 5.days.from_now)
+seed_order(supplier_id: 3, starts: 3.days.ago, ends: 5.days.from_now)
-seed_order(supplier_id: 2, starts: 2.days.ago, ends: 4.days.from_now)
+seed_order(supplier_id: 2, starts: 4.days.ago, ends: 4.days.from_now)
 
 ## GroupOrders & such
 

spec/factories/user.rb

@@ -34,10 +34,10 @@ FactoryBot.define do
     sequence(:name) { |n| "Group ##{n}" }
 
     factory :workgroup do
-      type { '' }
+      type { 'Workgroup' }
     end
 
-    factory :ordergroup do
+    factory :ordergroup, class: "Ordergroup" do
       type { 'Ordergroup' }
       sequence(:name) { |n| "Order group ##{n}" }
       # workaround to avoid needing to save the ordergroup

spec/models/ordergroup_spec.rb

@@ -22,6 +22,90 @@ describe Ordergroup do
     expect(Ordergroup.active).not_to be_empty
   end
 
+  describe 'sort correctly' do
+    it 'by name' do
+      group_b = create :ordergroup, name: 'bbb'
+      group_a = create :ordergroup, name: 'aaa'
+      group_c = create :ordergroup, name: 'ccc'
+
+      expect(Ordergroup.sort_by_param('name')).to eq([group_a, group_b, group_c])
+    end
+
+    it 'reverse by name' do
+      group_b = create :ordergroup, name: 'bbb'
+      group_a = create :ordergroup, name: 'aaa'
+      group_c = create :ordergroup, name: 'ccc'
+
+      expect(Ordergroup.sort_by_param('name_reverse')).to eq([group_c, group_b, group_a])
+    end
+
+    it 'by members_count' do
+      users_b = [create(:user)]
+      users_a = []
+      users_c = [create(:user), create(:user), create(:user)]
+      group_b = create :ordergroup, name: 'bbb', user_ids: users_b.map(&:id)
+      group_a = create :ordergroup, name: 'aaa', user_ids: users_a.map(&:id)
+      group_c = create :ordergroup, name: 'ccc', user_ids: users_c.map(&:id)
+
+      expect(Ordergroup.sort_by_param('members_count')).to eq([group_a, group_b, group_c])
+    end
+
+    it 'reverse by members_count' do
+      users_b = [create(:user)]
+      users_a = []
+      users_c = [create(:user), create(:user), create(:user)]
+      group_b = create :ordergroup, name: 'bbb', user_ids: users_b.map(&:id)
+      group_a = create :ordergroup, name: 'aaa', user_ids: users_a.map(&:id)
+      group_c = create :ordergroup, name: 'ccc', user_ids: users_c.map(&:id)
+
+      expect(Ordergroup.sort_by_param('members_count_reverse')).to eq([group_c, group_b, group_a])
+    end
+
+    it 'by last_user_activity' do
+      user_b = create :user, last_activity: 3.days.ago
+      user_a = create :user, last_activity: 5.days.ago
+      user_c = create :user, last_activity: Time.now
+      group_b = create :ordergroup, name: 'bbb', user_ids: [user_b.id]
+      group_a = create :ordergroup, name: 'aaa', user_ids: [user_a.id]
+      group_c = create :ordergroup, name: 'ccc', user_ids: [user_c.id]
+
+      expect(Ordergroup.sort_by_param('last_user_activity')).to eq([group_a, group_b, group_c])
+    end
+
+    it 'reverse by last_user_activity' do
+      user_b = create :user, last_activity: 3.days.ago
+      user_a = create :user, last_activity: 5.days.ago
+      user_c = create :user, last_activity: Time.now
+      group_b = create :ordergroup, name: 'bbb', user_ids: [user_b.id]
+      group_a = create :ordergroup, name: 'aaa', user_ids: [user_a.id]
+      group_c = create :ordergroup, name: 'ccc', user_ids: [user_c.id]
+
+      expect(Ordergroup.sort_by_param('last_user_activity_reverse')).to eq([group_c, group_b, group_a])
+    end
+
+    it 'by last_order' do
+      group_b = create :ordergroup, name: 'bbb'
+      group_a = create :ordergroup, name: 'aaa'
+      group_c = create :ordergroup, name: 'ccc'
+      group_b.group_orders.create! order: create(:order, starts: 6.days.ago)
+      group_a.group_orders.create! order: create(:order, starts: 4.months.ago)
+      group_c.group_orders.create! order: create(:order, starts: Time.now)
+
+      expect(Ordergroup.sort_by_param('last_order')).to eq([group_a, group_b, group_c])
+    end
+
+    it 'reverse by last_order' do
+      group_b = create :ordergroup, name: 'bbb'
+      group_a = create :ordergroup, name: 'aaa'
+      group_c = create :ordergroup, name: 'ccc'
+      group_b.group_orders.create! order: create(:order, starts: 6.days.ago)
+      group_a.group_orders.create! order: create(:order, starts: 4.months.ago)
+      group_c.group_orders.create! order: create(:order, starts: Time.now)
+
+      expect(Ordergroup.sort_by_param('last_order_reverse')).to eq([group_c, group_b, group_a])
+    end
+  end
+
   context 'with financial transactions' do
     before do
       og = user.ordergroup

spec/models/user_spec.rb

@@ -78,4 +78,122 @@ describe User do
 
     it 'default admin role' do expect(user.role_admin?).to be_truthy end
   end
+
+  describe 'sort correctly' do
+    it 'by nick' do
+      user_b = create :user, nick: 'bbb'
+      user_a = create :user, nick: 'aaa'
+      user_c = create :user, nick: 'ccc'
+
+      expect(User.sort_by_param('nick')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by nick' do
+      user_b = create :user, nick: 'bbb'
+      user_a = create :user, nick: 'aaa'
+      user_c = create :user, nick: 'ccc'
+
+      expect(User.sort_by_param('nick_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'by name' do
+      user_b = create :user, first_name: 'aaa', last_name: 'bbb'
+      user_a = create :user, first_name: 'aaa', last_name: 'aaa'
+      user_c = create :user, first_name: 'ccc', last_name: 'aaa'
+
+      expect(User.sort_by_param('name')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by name' do
+      user_b = create :user, first_name: 'aaa', last_name: 'bbb'
+      user_a = create :user, first_name: 'aaa', last_name: 'aaa'
+      user_c = create :user, first_name: 'ccc', last_name: 'aaa'
+
+      expect(User.sort_by_param('name_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'by email' do
+      user_b = create :user, email: 'bbb@dummy.com'
+      user_a = create :user, email: 'aaa@dummy.com'
+      user_c = create :user, email: 'ccc@dummy.com'
+
+      expect(User.sort_by_param('email')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by email' do
+      user_b = create :user, email: 'bbb@dummy.com'
+      user_a = create :user, email: 'aaa@dummy.com'
+      user_c = create :user, email: 'ccc@dummy.com'
+
+      expect(User.sort_by_param('email_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'by phone' do
+      user_b = create :user, phone: 'bbb'
+      user_a = create :user, phone: 'aaa'
+      user_c = create :user, phone: 'ccc'
+
+      expect(User.sort_by_param('phone')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by phone' do
+      user_b = create :user, phone: 'bbb'
+      user_a = create :user, phone: 'aaa'
+      user_c = create :user, phone: 'ccc'
+
+      expect(User.sort_by_param('phone_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'by last_activity' do
+      user_b = create :user, last_activity: 3.days.ago
+      user_a = create :user, last_activity: 5.days.ago
+      user_c = create :user, last_activity: Time.now
+
+      expect(User.sort_by_param('last_activity')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by last_activity' do
+      user_b = create :user, last_activity: 3.days.ago
+      user_a = create :user, last_activity: 5.days.ago
+      user_c = create :user, last_activity: Time.now
+
+      expect(User.sort_by_param('last_activity_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'by ordergroup' do
+      user_b = create :user, groups: [create(:workgroup, name: 'a'), create(:ordergroup, name: 'bb')]
+      user_a = create :user, groups: [create(:workgroup, name: 'b'), create(:ordergroup, name: 'aa')]
+      user_c = create :user, groups: [create(:workgroup, name: 'c'), create(:ordergroup, name: 'cc')]
+
+      expect(User.sort_by_param('ordergroup')).to eq([user_a, user_b, user_c])
+    end
+
+    it 'reverse by ordergroup' do
+      user_b = create :user, groups: [create(:workgroup, name: 'a'), create(:ordergroup, name: 'bb')]
+      user_a = create :user, groups: [create(:workgroup, name: 'b'), create(:ordergroup, name: 'aa')]
+      user_c = create :user, groups: [create(:workgroup, name: 'c'), create(:ordergroup, name: 'cc')]
+
+      expect(User.sort_by_param('ordergroup_reverse')).to eq([user_c, user_b, user_a])
+    end
+
+    it 'and users are only listed once' do
+      create :user
+
+      expect(User.sort_by_param('ordergroup').size).to eq(1)
+    end
+
+    it 'and users belonging to a workgroup are only listed once' do
+      create :admin
+
+      expect(User.sort_by_param('ordergroup').size).to eq(1)
+    end
+
+    it 'and users belonging to 2 ordergroups are only listed once' do
+      user = create :user
+      create :ordergroup, user_ids: [user.id]
+      create :ordergroup, user_ids: [user.id]
+
+      expect(User.sort_by_param('ordergroup').size).to eq(1)
+    end
+  end
 end