From 0a6345c60b0b691c75a69e9b9226f5d96115f4ed Mon Sep 17 00:00:00 2001 From: Harald Reingruber Date: Fri, 27 May 2022 17:06:25 +0200 Subject: [PATCH] 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. --- .../admin/ordergroups_controller.rb | 2 +- app/controllers/admin/users_controller.rb | 4 +- .../foodcoop/ordergroups_controller.rb | 2 +- app/controllers/foodcoop/users_controller.rb | 2 +- app/models/ordergroup.rb | 23 ++++ app/models/user.rb | 23 ++++ .../admin/ordergroups/_ordergroups.html.haml | 8 +- app/views/admin/users/_users.html.haml | 8 +- .../ordergroups/_ordergroups.html.haml | 6 +- app/views/foodcoop/users/_users.html.haml | 10 +- db/seeds/small.en.seeds.rb | 9 +- spec/factories/user.rb | 4 +- spec/models/ordergroup_spec.rb | 84 +++++++++++++ spec/models/user_spec.rb | 118 ++++++++++++++++++ 14 files changed, 278 insertions(+), 25 deletions(-) diff --git a/app/controllers/admin/ordergroups_controller.rb b/app/controllers/admin/ordergroups_controller.rb index 279cccdf..d9dabe1e 100644 --- a/app/controllers/admin/ordergroups_controller.rb +++ b/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' diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 97ff4177..18bbbc1d 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 diff --git a/app/controllers/foodcoop/ordergroups_controller.rb b/app/controllers/foodcoop/ordergroups_controller.rb index 8aeb977e..6940a376 100644 --- a/app/controllers/foodcoop/ordergroups_controller.rb +++ b/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]}%") diff --git a/app/controllers/foodcoop/users_controller.rb b/app/controllers/foodcoop/users_controller.rb index a3a8bd45..196f1be8 100644 --- a/app/controllers/foodcoop/users_controller.rb +++ b/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? diff --git a/app/models/ordergroup.rb b/app/models/ordergroup.rb index e7ad7f11..c29ec762 100644 --- a/app/models/ordergroup.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 82d80ed5..05a67547 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/admin/ordergroups/_ordergroups.html.haml b/app/views/admin/ordergroups/_ordergroups.html.haml index b317b4ce..ab61c81e 100644 --- a/app/views/admin/ordergroups/_ordergroups.html.haml +++ b/app/views/admin/ordergroups/_ordergroups.html.haml @@ -4,10 +4,11 @@ %table.table.table-striped %thead %tr - %th= heading_helper Ordergroup, :name - %th= heading_helper Ordergroup, :user_tokens + %th= sort_link_helper heading_helper(Ordergroup, :name), "name" + %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)}, diff --git a/app/views/admin/users/_users.html.haml b/app/views/admin/users/_users.html.haml index 1a7a0cea..20795b03 100644 --- a/app/views/admin/users/_users.html.haml +++ b/app/views/admin/users/_users.html.haml @@ -5,11 +5,11 @@ %thead %tr - if FoodsoftConfig[:use_nick] - %th= heading_helper User, :nick - %th= heading_helper User, :name - %th= heading_helper User, :email + %th= sort_link_helper heading_helper(User, :nick), "nick" + %th= sort_link_helper heading_helper(User, :name), "name" + %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 diff --git a/app/views/foodcoop/ordergroups/_ordergroups.html.haml b/app/views/foodcoop/ordergroups/_ordergroups.html.haml index 5e226991..36b73184 100644 --- a/app/views/foodcoop/ordergroups/_ordergroups.html.haml +++ b/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= heading_helper Ordergroup, :last_order + %th= sort_link_helper heading_helper(Ordergroup, :last_user_activity), "last_user_activity" + %th= sort_link_helper heading_helper(Ordergroup, :last_order), "last_order" %tbody - for ordergroup in @ordergroups diff --git a/app/views/foodcoop/users/_users.html.haml b/app/views/foodcoop/users/_users.html.haml index 9ee7ce48..da3f2ad5 100644 --- a/app/views/foodcoop/users/_users.html.haml +++ b/app/views/foodcoop/users/_users.html.haml @@ -5,11 +5,11 @@ %thead %tr - if FoodsoftConfig[:use_nick] - %th= heading_helper User, :nick - %th= heading_helper User, :name - %th= heading_helper User, :email - %th= heading_helper User, :phone - %th= heading_helper User, :ordergroup + %th= sort_link_helper heading_helper(User, :nick), "nick" + %th= sort_link_helper heading_helper(User, :name), "name" + %th= sort_link_helper heading_helper(User, :email), "email" + %th= sort_link_helper heading_helper(User, :phone), "phone" + %th= sort_link_helper heading_helper(User, :ordergroup), "ordergroup" %th= heading_helper User, :workgroup, count: 3 %tbody - for user in @users diff --git a/db/seeds/small.en.seeds.rb b/db/seeds/small.en.seeds.rb index 40c338ac..52f0b9db 100644 --- a/db/seeds/small.en.seeds.rb +++ b/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: 2, starts: 2.days.ago, ends: 4.days.from_now) +seed_order(supplier_id: 3, starts: 3.days.ago, ends: 5.days.from_now) +seed_order(supplier_id: 2, starts: 4.days.ago, ends: 4.days.from_now) ## GroupOrders & such diff --git a/spec/factories/user.rb b/spec/factories/user.rb index eb12196f..a6067e6b 100644 --- a/spec/factories/user.rb +++ b/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 diff --git a/spec/models/ordergroup_spec.rb b/spec/models/ordergroup_spec.rb index 2306b255..0c142a33 100644 --- a/spec/models/ordergroup_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ead923e4..59a797de 100644 --- a/spec/models/user_spec.rb +++ b/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