From ea3319ae6f4e6ddeaf58360a613f3e5eaff06488 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Sun, 30 Dec 2018 22:01:01 +0100 Subject: [PATCH 01/25] Add sorting links to table headers with proper styling [#2931] --- app/assets/stylesheets/admin.scss | 4 ++++ app/views/admin/budget_investments/_investments.html.erb | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index f08d14fda..e51f9f4e2 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -215,6 +215,10 @@ $sidebar-active: #f4fcd0; thead { color: #fff; + + a { + color: inherit; + } } th { diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index 81b46f9d3..d56cc65f8 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -35,9 +35,9 @@ - - - + + +
<%= t("admin.budget_investments.index.list.id") %><%= t("admin.budget_investments.index.list.title") %><%= t("admin.budget_investments.index.list.supports") %><%= link_to t("admin.budget_investments.index.list.id"), admin_budget_budget_investments_path(sort_by: "id") %><%= link_to t("admin.budget_investments.index.list.title"), admin_budget_budget_investments_path(sort_by: "title") %><%= link_to t("admin.budget_investments.index.list.supports"), admin_budget_budget_investments_path(sort_by: "supports") %> <%= t("admin.budget_investments.index.list.admin") %> <%= t("admin.budget_investments.index.list.valuation_group") %> From 22059379f598241bc9e3bf3b856f96d694fe9063 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Sun, 30 Dec 2018 23:18:11 +0100 Subject: [PATCH 02/25] Refactor by creating a helper method for generating sorting links [#2931] --- app/helpers/budget_investments_helper.rb | 5 +++++ app/views/admin/budget_investments/_investments.html.erb | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index e3f17601a..d0a01b6af 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -9,6 +9,11 @@ module BudgetInvestmentsHelper params.map { |af| t("admin.budget_investments.index.filters.#{af}") }.join(', ') end + def link_to_investments_sorted_by(column) + sorting_option = budget_investments_sorting_options.select { |so| so[1] == column.downcase }.flatten + link_to t(sorting_option[0]), admin_budget_budget_investments_path(sort_by: sorting_option[1]) + end + def investments_minimal_view_path budget_investments_path(id: @heading.group.to_param, heading_id: @heading.to_param, diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index d56cc65f8..9daef3ac3 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -35,9 +35,9 @@ - - - + + +
<%= link_to t("admin.budget_investments.index.list.id"), admin_budget_budget_investments_path(sort_by: "id") %><%= link_to t("admin.budget_investments.index.list.title"), admin_budget_budget_investments_path(sort_by: "title") %><%= link_to t("admin.budget_investments.index.list.supports"), admin_budget_budget_investments_path(sort_by: "supports") %><%= link_to_investments_sorted_by t("admin.budget_investments.index.list.id") %><%= link_to_investments_sorted_by t("admin.budget_investments.index.list.title") %><%= link_to_investments_sorted_by t("admin.budget_investments.index.list.supports") %> <%= t("admin.budget_investments.index.list.admin") %> <%= t("admin.budget_investments.index.list.valuation_group") %> From 5dd468092a8de9f969d318ee3916b01b2f287a01 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Sun, 30 Dec 2018 23:41:09 +0100 Subject: [PATCH 03/25] Rewrite the method to not use budget_investments_sorting_options [#2931] --- app/helpers/budget_investments_helper.rb | 13 +++++-------- .../admin/budget_investments/_investments.html.erb | 9 --------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index d0a01b6af..6725fbf11 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -1,17 +1,14 @@ module BudgetInvestmentsHelper - def budget_investments_sorting_options - Budget::Investment::SORTING_OPTIONS.map do |so| - [t("admin.budget_investments.index.sort_by.#{so}"), so] - end - end - def budget_investments_advanced_filters(params) params.map { |af| t("admin.budget_investments.index.filters.#{af}") }.join(', ') end def link_to_investments_sorted_by(column) - sorting_option = budget_investments_sorting_options.select { |so| so[1] == column.downcase }.flatten - link_to t(sorting_option[0]), admin_budget_budget_investments_path(sort_by: sorting_option[1]) + sorting_option = column.downcase + link_to( + t("admin.budget_investments.index.sort_by.#{sorting_option}"), + admin_budget_budget_investments_path(sort_by: sorting_option) + ) end def investments_minimal_view_path diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index 9daef3ac3..f7aea9c33 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -1,12 +1,3 @@ -<%= form_tag(admin_budget_budget_investments_path(budget: @budget), method: :get) do %> -
- <%= select_tag :sort_by, options_for_select(budget_investments_sorting_options, params[:sort_by]), - { prompt: t("admin.budget_investments.index.sort_by.placeholder"), - label: false, - class: "js-submit-on-change" } %> -
-<% end %> - <%= link_to t("admin.budget_investments.index.download_current_selection"), admin_budget_budget_investments_path(csv_params), class: "float-right small clear" %> From 2877229904ed28846189f6fa3ff39ee6b5765275 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Sun, 30 Dec 2018 23:41:25 +0100 Subject: [PATCH 04/25] Move the link styling to th [#2931] --- app/assets/stylesheets/admin.scss | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index e51f9f4e2..30cfa01ec 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -215,10 +215,6 @@ $sidebar-active: #f4fcd0; thead { color: #fff; - - a { - color: inherit; - } } th { @@ -228,6 +224,10 @@ $sidebar-active: #f4fcd0; label { color: #fff; } + + a { + color: inherit; + } } .break { From 9786b0bf7dc14986b262fe69f0154eefb9bdf927 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 01:11:36 +0100 Subject: [PATCH 05/25] Rewrite sorting to support direction param [#2931] --- app/assets/stylesheets/admin.scss | 1 + .../admin/budget_investments_controller.rb | 3 ++- app/helpers/budget_investments_helper.rb | 10 ++++++++-- app/models/budget/investment.rb | 11 +++++++---- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index 30cfa01ec..bfa6aa445 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -227,6 +227,7 @@ $sidebar-active: #f4fcd0; a { color: inherit; + white-space: nowrap; } } diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 60ab11d7c..7bdd7254d 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -77,7 +77,8 @@ class Admin::BudgetInvestmentsController < Admin::BaseController def load_investments @investments = Budget::Investment.scoped_filter(params, @current_filter) - .order_filter(params[:sort_by]) + .order_filter(params[:sort_by], params[:direction]) + @investments = @investments.page(params[:page]) unless request.format.csv? end diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 6725fbf11..617fcc2fd 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -5,9 +5,15 @@ module BudgetInvestmentsHelper def link_to_investments_sorted_by(column) sorting_option = column.downcase + direction = sorting_option && params[:direction] == "asc" ? "desc" : "asc" + icon = direction == "asc" ? "icon-arrow-top" : "icon-arrow-down" + icon = sorting_option == params[:sort_by] ? icon : "" + + translation = t("admin.budget_investments.index.sort_by.#{sorting_option}") + link_to( - t("admin.budget_investments.index.sort_by.#{sorting_option}"), - admin_budget_budget_investments_path(sort_by: sorting_option) + "#{translation} ".html_safe, + admin_budget_budget_investments_path(sort_by: sorting_option, direction: direction) ) end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 1b528a9cb..16a9dff0b 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -1,6 +1,6 @@ class Budget class Investment < ActiveRecord::Base - SORTING_OPTIONS = %w(id title supports).freeze + SORTING_OPTIONS = [{"id": "id"}, {"title": "title"}, {"supports": "cached_votes_up"}].freeze include Rails.application.routes.url_helpers include Measurable @@ -139,9 +139,12 @@ class Budget results.where("budget_investments.id IN (?)", ids) end - def self.order_filter(sorting_param) - if sorting_param.present? && SORTING_OPTIONS.include?(sorting_param) - send("sort_by_#{sorting_param}") + def self.order_filter(sorting_param, direction) + sorting_key = sorting_param.to_sym + available_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce + if sorting_param.present? && available_option.present? then + %w[asc desc].include?(direction) ? direction : "desc" + order("#{available_option[sorting_key]} #{direction}") else order(cached_votes_up: :desc).order(id: :desc) end From 2523775d9fabe8c763594f0162b6271682b53de0 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 18:17:32 +0100 Subject: [PATCH 06/25] Make the conditional more readable temporarily --- app/controllers/admin/budget_investments_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 7bdd7254d..96d8e35e5 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -76,6 +76,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end def load_investments + params[:direction].present? ? params[:direction] : params[:direction] = "asc" @investments = Budget::Investment.scoped_filter(params, @current_filter) .order_filter(params[:sort_by], params[:direction]) From 12484ed4fdf5a041811e2475cee124b4640771d6 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 18:18:20 +0100 Subject: [PATCH 07/25] Change SORTING_OPTIONS to has --- app/models/budget/investment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 16a9dff0b..fa2756ba5 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -1,6 +1,6 @@ class Budget class Investment < ActiveRecord::Base - SORTING_OPTIONS = [{"id": "id"}, {"title": "title"}, {"supports": "cached_votes_up"}].freeze + SORTING_OPTIONS = [{id: "id"}, {title: "title"}, {supports: "cached_votes_up"}].freeze include Rails.application.routes.url_helpers include Measurable From 35cebe0eefcb1727981ad8411c6b65574ac84a98 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 18:19:06 +0100 Subject: [PATCH 08/25] Use symbols in sorting and set direction properly --- app/models/budget/investment.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index fa2756ba5..8bb47d507 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -140,10 +140,11 @@ class Budget end def self.order_filter(sorting_param, direction) - sorting_key = sorting_param.to_sym + sorting_key = sorting_param.to_sym if sorting_param available_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce + if sorting_param.present? && available_option.present? then - %w[asc desc].include?(direction) ? direction : "desc" + direction = %w[asc desc].include?(direction) ? direction : "asc" order("#{available_option[sorting_key]} #{direction}") else order(cached_votes_up: :desc).order(id: :desc) From 747b8295b1f0104a920dbafa5820027ac72710bd Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 18:20:35 +0100 Subject: [PATCH 09/25] Refactor link_to_investments_sorted method --- app/helpers/budget_investments_helper.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 617fcc2fd..1b3ca39f2 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -5,8 +5,9 @@ module BudgetInvestmentsHelper def link_to_investments_sorted_by(column) sorting_option = column.downcase - direction = sorting_option && params[:direction] == "asc" ? "desc" : "asc" - icon = direction == "asc" ? "icon-arrow-top" : "icon-arrow-down" + direction = params[:direction] ? params[:direction] : "desc" + + icon = direction == "desc" ? "icon-arrow-down" : "icon-arrow-top" icon = sorting_option == params[:sort_by] ? icon : "" translation = t("admin.budget_investments.index.sort_by.#{sorting_option}") From 0affaaee7e194d901b0ebc1bbb032444e12ec01e Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 18:52:02 +0100 Subject: [PATCH 10/25] Refactor sorting specs to work with direction --- .../features/admin/budget_investments_spec.rb | 76 +++++++++++++++---- 1 file changed, 63 insertions(+), 13 deletions(-) diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 7e5a6be03..fb3b2e5c9 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -603,7 +603,7 @@ feature 'Admin budget investments' do create(:budget_investment, title: 'C Third Investment', cached_votes_up: 10, budget: budget) end - scenario "Default sorting" do + scenario "Default" do create(:budget_investment, title: 'D Fourth Investment', cached_votes_up: 50, budget: budget) visit admin_budget_budget_investments_path(budget) @@ -613,26 +613,76 @@ feature 'Admin budget investments' do expect('A Second Investment').to appear_before('C Third Investment') end - scenario 'Sort by ID' do - visit admin_budget_budget_investments_path(budget, sort_by: 'id') + context 'Ascending' do + scenario 'Sort by ID' do + visit admin_budget_budget_investments_path(budget, sort_by: 'id', direction: 'asc') - expect('C Third Investment').to appear_before('A Second Investment') - expect('A Second Investment').to appear_before('B First Investment') + expect('B First Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('C Third Investment') + end + + scenario 'Sort by title' do + visit admin_budget_budget_investments_path(budget, sort_by: 'title', direction: 'asc') + + expect('A Second Investment').to appear_before('B First Investment') + expect('B First Investment').to appear_before('C Third Investment') + end + + scenario 'Sort by supports' do + visit admin_budget_budget_investments_path(budget, sort_by: 'supports', direction: 'asc') + + expect('C Third Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('B First Investment') + end end - scenario 'Sort by title' do - visit admin_budget_budget_investments_path(budget, sort_by: 'title') + context 'Descending' do + scenario 'Sort by ID' do + visit admin_budget_budget_investments_path(budget, sort_by: 'id', direction: 'desc') - expect('A Second Investment').to appear_before('B First Investment') - expect('B First Investment').to appear_before('C Third Investment') + expect('C Third Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('B First Investment') + end + + scenario 'Sort by title' do + visit admin_budget_budget_investments_path(budget, sort_by: 'title', direction: 'desc') + + expect('C Third Investment').to appear_before('B First Investment') + expect('B First Investment').to appear_before('A Second Investment') + end + + scenario 'Sort by supports' do + visit admin_budget_budget_investments_path(budget, sort_by: 'supports', direction: 'desc') + + expect('B First Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('C Third Investment') + end end - scenario 'Sort by supports' do - visit admin_budget_budget_investments_path(budget, sort_by: 'supports') + context 'With no direction provided sorts ascending' do + scenario 'Sort by ID' do + visit admin_budget_budget_investments_path(budget, sort_by: 'id') - expect('B First Investment').to appear_before('A Second Investment') - expect('A Second Investment').to appear_before('C Third Investment') + expect('B First Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('C Third Investment') + end + + scenario 'Sort by title' do + visit admin_budget_budget_investments_path(budget, sort_by: 'title') + + expect('A Second Investment').to appear_before('B First Investment') + expect('B First Investment').to appear_before('C Third Investment') + end + + scenario 'Sort by supports' do + visit admin_budget_budget_investments_path(budget, sort_by: 'supports') + + expect('C Third Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('B First Investment') + end end + + end context 'Show' do From 39cc997ef4fbf212c3420683e9e3f5e4a6d80a5e Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 19:53:56 +0100 Subject: [PATCH 11/25] Add check for arrow icons --- .../features/admin/budget_investments_spec.rb | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index fb3b2e5c9..76a63e350 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -619,6 +619,9 @@ feature 'Admin budget investments' do expect('B First Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('C Third Investment') + within('th', text: 'ID') do + expect(page).to have_css(".icon-arrow-top") + end end scenario 'Sort by title' do @@ -626,6 +629,9 @@ feature 'Admin budget investments' do expect('A Second Investment').to appear_before('B First Investment') expect('B First Investment').to appear_before('C Third Investment') + within('th', text: 'Title') do + expect(page).to have_css(".icon-arrow-top") + end end scenario 'Sort by supports' do @@ -633,6 +639,9 @@ feature 'Admin budget investments' do expect('C Third Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('B First Investment') + within('th', text: 'Supports') do + expect(page).to have_css(".icon-arrow-top") + end end end @@ -642,6 +651,9 @@ feature 'Admin budget investments' do expect('C Third Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('B First Investment') + within('th', text: 'ID') do + expect(page).to have_css(".icon-arrow-down") + end end scenario 'Sort by title' do @@ -649,6 +661,9 @@ feature 'Admin budget investments' do expect('C Third Investment').to appear_before('B First Investment') expect('B First Investment').to appear_before('A Second Investment') + within('th', text: 'Title') do + expect(page).to have_css(".icon-arrow-down") + end end scenario 'Sort by supports' do @@ -656,6 +671,9 @@ feature 'Admin budget investments' do expect('B First Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('C Third Investment') + within('th', text: 'Supports') do + expect(page).to have_css(".icon-arrow-down") + end end end @@ -665,6 +683,9 @@ feature 'Admin budget investments' do expect('B First Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('C Third Investment') + within('th', text: 'ID') do + expect(page).to have_css(".icon-arrow-top") + end end scenario 'Sort by title' do @@ -672,6 +693,9 @@ feature 'Admin budget investments' do expect('A Second Investment').to appear_before('B First Investment') expect('B First Investment').to appear_before('C Third Investment') + within('th', text: 'Title') do + expect(page).to have_css(".icon-arrow-top") + end end scenario 'Sort by supports' do @@ -679,10 +703,23 @@ feature 'Admin budget investments' do expect('C Third Investment').to appear_before('A Second Investment') expect('A Second Investment').to appear_before('B First Investment') + within('th', text: 'Supports') do + expect(page).to have_css(".icon-arrow-top") + end end end + context 'With incorrect direction provided sorts ascending' do + scenario 'Sort by ID' do + visit admin_budget_budget_investments_path(budget, sort_by: 'id', direction: 'incorrect') + expect('B First Investment').to appear_before('A Second Investment') + expect('A Second Investment').to appear_before('C Third Investment') + within('th', text: 'ID') do + expect(page).to have_css(".icon-arrow-top") + end + end + end end context 'Show' do From 101292d30318ee9c0c0d29d1f750883a5467ce9c Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 19:54:39 +0100 Subject: [PATCH 12/25] Rename allowed sort option variable --- app/models/budget/investment.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 8bb47d507..265d2ba86 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -141,11 +141,11 @@ class Budget def self.order_filter(sorting_param, direction) sorting_key = sorting_param.to_sym if sorting_param - available_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce + allowed_sort_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce - if sorting_param.present? && available_option.present? then + if sorting_param.present? && allowed_sort_option.present? then direction = %w[asc desc].include?(direction) ? direction : "asc" - order("#{available_option[sorting_key]} #{direction}") + order("#{allowed_sort_option[sorting_key]} #{direction}") else order(cached_votes_up: :desc).order(id: :desc) end From cb761c56c22a6a8faf3633a589c66a80d5ff0482 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 20:34:14 +0100 Subject: [PATCH 13/25] Fix issue with app crashing when sort_by param was incorrect --- app/controllers/admin/budget_investments_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 96d8e35e5..259945c8e 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -80,6 +80,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController @investments = Budget::Investment.scoped_filter(params, @current_filter) .order_filter(params[:sort_by], params[:direction]) + @investments = Budget::Investment.by_budget(@budget).all @investments = @investments.page(params[:page]) unless request.format.csv? end From 37b22264326ac41f595d4974226a74d8c3030989 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 20:36:51 +0100 Subject: [PATCH 14/25] Make the link_to helper more readable --- app/helpers/budget_investments_helper.rb | 21 +++++++++++++++------ app/models/budget/investment.rb | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 1b3ca39f2..75255e7a8 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -4,17 +4,26 @@ module BudgetInvestmentsHelper end def link_to_investments_sorted_by(column) - sorting_option = column.downcase - direction = params[:direction] ? params[:direction] : "desc" + sort_by = column.downcase + allowed_directions = %w[asc desc].freeze + default_direction = "desc" + current_direction = params[:direction] - icon = direction == "desc" ? "icon-arrow-down" : "icon-arrow-top" - icon = sorting_option == params[:sort_by] ? icon : "" + if allowed_directions.include?(current_direction) + #select opposite direction + direction = allowed_directions.reject { |dir| dir == current_direction }.first + else + direction = default_direction + end - translation = t("admin.budget_investments.index.sort_by.#{sorting_option}") + icon = direction == default_direction ? "icon-arrow-top" : "icon-arrow-down" + icon = sort_by == params[:sort_by] ? icon : "" + + translation = t("admin.budget_investments.index.sort_by.#{sort_by}") link_to( "#{translation} ".html_safe, - admin_budget_budget_investments_path(sort_by: sorting_option, direction: direction) + admin_budget_budget_investments_path(sort_by: sort_by, direction: direction) ) end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 265d2ba86..0c90369be 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -143,7 +143,7 @@ class Budget sorting_key = sorting_param.to_sym if sorting_param allowed_sort_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce - if sorting_param.present? && allowed_sort_option.present? then + if allowed_sort_option.present? then direction = %w[asc desc].include?(direction) ? direction : "asc" order("#{allowed_sort_option[sorting_key]} #{direction}") else From 3ab70ff0d8da59e5e039aca73043e93c8d3e418a Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Tue, 1 Jan 2019 21:29:29 +0100 Subject: [PATCH 15/25] Move validating params to model --- .../admin/budget_investments_controller.rb | 5 +---- app/models/budget/investment.rb | 15 +++++++-------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 259945c8e..71c92aee9 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -76,11 +76,9 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end def load_investments - params[:direction].present? ? params[:direction] : params[:direction] = "asc" @investments = Budget::Investment.scoped_filter(params, @current_filter) - .order_filter(params[:sort_by], params[:direction]) + .order_filter(params) - @investments = Budget::Investment.by_budget(@budget).all @investments = @investments.page(params[:page]) unless request.format.csv? end @@ -136,5 +134,4 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end end end - end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 0c90369be..e10070592 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -139,16 +139,15 @@ class Budget results.where("budget_investments.id IN (?)", ids) end - def self.order_filter(sorting_param, direction) - sorting_key = sorting_param.to_sym if sorting_param - allowed_sort_option = SORTING_OPTIONS.select { |sp| sp[sorting_key]}.reduce + def self.order_filter(params) + sorting_key = params[:sort_by].to_sym if params[:sort_by] + allowed_sort_option = SORTING_OPTIONS.select { |so| so[sorting_key]}.reduce - if allowed_sort_option.present? then - direction = %w[asc desc].include?(direction) ? direction : "asc" - order("#{allowed_sort_option[sorting_key]} #{direction}") - else - order(cached_votes_up: :desc).order(id: :desc) + if allowed_sort_option.present? + direction = %w[asc desc].include?(params[:direction]) ? params[:direction] : "asc" + return order("#{allowed_sort_option[sorting_key]} #{direction}") end + order(cached_votes_up: :desc).order(id: :desc) end def self.limit_results(budget, params, results) From e264490bca577f0872d5bed1dc5295ceeee3b823 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Wed, 2 Jan 2019 20:55:57 +0100 Subject: [PATCH 16/25] Styling and refactor sorting methods and helpers --- app/helpers/budget_investments_helper.rb | 11 ++--------- app/models/budget/investment.rb | 2 +- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 75255e7a8..e98bd2b70 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -5,18 +5,11 @@ module BudgetInvestmentsHelper def link_to_investments_sorted_by(column) sort_by = column.downcase - allowed_directions = %w[asc desc].freeze default_direction = "desc" - current_direction = params[:direction] - if allowed_directions.include?(current_direction) - #select opposite direction - direction = allowed_directions.reject { |dir| dir == current_direction }.first - else - direction = default_direction - end + direction = params[:direction] == default_direction ? default_direction : "asc" - icon = direction == default_direction ? "icon-arrow-top" : "icon-arrow-down" + icon = direction == default_direction ? "icon-arrow-down" : "icon-arrow-top" icon = sort_by == params[:sort_by] ? icon : "" translation = t("admin.budget_investments.index.sort_by.#{sort_by}") diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index e10070592..607c6cb95 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -144,7 +144,7 @@ class Budget allowed_sort_option = SORTING_OPTIONS.select { |so| so[sorting_key]}.reduce if allowed_sort_option.present? - direction = %w[asc desc].include?(params[:direction]) ? params[:direction] : "asc" + direction = params[:direction] == "desc" ? "desc" : "asc" return order("#{allowed_sort_option[sorting_key]} #{direction}") end order(cached_votes_up: :desc).order(id: :desc) From e88acb8905e04e8beb60888c36fe896fde4d5664 Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Wed, 2 Jan 2019 21:08:00 +0100 Subject: [PATCH 17/25] Make params handling case insensitive --- app/helpers/budget_investments_helper.rb | 3 ++- app/models/budget/investment.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index e98bd2b70..e26b4099b 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -6,8 +6,9 @@ module BudgetInvestmentsHelper def link_to_investments_sorted_by(column) sort_by = column.downcase default_direction = "desc" + current_direction = params[:direction].downcase if params[:direction] - direction = params[:direction] == default_direction ? default_direction : "asc" + direction = current_direction == default_direction ? default_direction : "asc" icon = direction == default_direction ? "icon-arrow-down" : "icon-arrow-top" icon = sort_by == params[:sort_by] ? icon : "" diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 607c6cb95..4017fc2ce 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -140,7 +140,7 @@ class Budget end def self.order_filter(params) - sorting_key = params[:sort_by].to_sym if params[:sort_by] + sorting_key = params[:sort_by].downcase.to_sym if params[:sort_by] allowed_sort_option = SORTING_OPTIONS.select { |so| so[sorting_key]}.reduce if allowed_sort_option.present? From 0fe9ea08c36ca917c3d7006837055a0266800d3a Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Wed, 2 Jan 2019 21:33:20 +0100 Subject: [PATCH 18/25] Fix the direction assignment --- app/helpers/budget_investments_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index e26b4099b..739d1b283 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -8,9 +8,9 @@ module BudgetInvestmentsHelper default_direction = "desc" current_direction = params[:direction].downcase if params[:direction] - direction = current_direction == default_direction ? default_direction : "asc" + direction = current_direction == default_direction ? "asc" : default_direction - icon = direction == default_direction ? "icon-arrow-down" : "icon-arrow-top" + icon = direction == default_direction ? "icon-arrow-top" : "icon-arrow-down" icon = sort_by == params[:sort_by] ? icon : "" translation = t("admin.budget_investments.index.sort_by.#{sort_by}") From 3fd7b3d216180a5a55dec5cf17a75087c44a28bc Mon Sep 17 00:00:00 2001 From: Anna Anks Nowak Date: Wed, 2 Jan 2019 22:43:17 +0100 Subject: [PATCH 19/25] Extract setting icon and direction to methods --- app/helpers/budget_investments_helper.rb | 16 +++++-- .../helpers/budget_investments_helper_spec.rb | 47 +++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 spec/helpers/budget_investments_helper_spec.rb diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 739d1b283..e81931c43 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -5,13 +5,10 @@ module BudgetInvestmentsHelper def link_to_investments_sorted_by(column) sort_by = column.downcase - default_direction = "desc" current_direction = params[:direction].downcase if params[:direction] - direction = current_direction == default_direction ? "asc" : default_direction - - icon = direction == default_direction ? "icon-arrow-top" : "icon-arrow-down" - icon = sort_by == params[:sort_by] ? icon : "" + direction = set_direction(current_direction) + icon = set_sorting_icon(direction, sort_by) translation = t("admin.budget_investments.index.sort_by.#{sort_by}") @@ -21,6 +18,15 @@ module BudgetInvestmentsHelper ) end + def set_sorting_icon(direction, sort_by) + icon = direction == "desc" ? "icon-arrow-top" : "icon-arrow-down" + icon = sort_by == params[:sort_by] ? icon : "" + end + + def set_direction(current_direction) + current_direction == "desc" ? "asc" : "desc" + end + def investments_minimal_view_path budget_investments_path(id: @heading.group.to_param, heading_id: @heading.to_param, diff --git a/spec/helpers/budget_investments_helper_spec.rb b/spec/helpers/budget_investments_helper_spec.rb new file mode 100644 index 000000000..4af4adb0e --- /dev/null +++ b/spec/helpers/budget_investments_helper_spec.rb @@ -0,0 +1,47 @@ +require 'rails_helper' + +RSpec.describe BudgetInvestmentsHelper, type: :helper do + + describe "#set_direction" do + + it "returns ASC if current_direction is DESC" do + expect(set_direction("desc")).to eq "asc" + end + + it "returns DESC if current_direction is ASC" do + expect(set_direction("asc")).to eq "desc" + end + + it "returns DESC if current_direction is nil" do + expect(set_direction(nil)).to eq "desc" + end + end + + describe "#set_sorting_icon" do + let(:sort_by) { "title" } + let(:params) { { sort_by: sort_by } } + + it "returns arrow down if current direction is ASC" do + expect(set_sorting_icon("asc", sort_by)).to eq "icon-arrow-down" + end + + it "returns arrow top if current direction is DESC" do + expect(set_sorting_icon("desc", sort_by)).to eq "icon-arrow-top" + end + + it "returns arrow down if sort_by present, but no direction" do + expect(set_sorting_icon(nil, sort_by)).to eq "icon-arrow-down" + end + + it "returns no icon if sort_by and direction is missing" do + params[:sort_by] = nil + expect(set_sorting_icon(nil, sort_by)).to eq "" + end + + it "returns no icon if sort_by is incorrect" do + params[:sort_by] = "random" + expect(set_sorting_icon("asc", sort_by)).to eq "" + end + end + +end From ee2f970e03f91c8e53b31e4af38202ac54bc2931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:05:04 +0100 Subject: [PATCH 20/25] Fix sorting links for non-English languages We were assuming the translation and the column name were related. --- app/helpers/budget_investments_helper.rb | 9 ++++----- app/views/admin/budget_investments/_investments.html.erb | 6 +++--- config/locales/en/admin.yml | 5 ----- config/locales/es/admin.yml | 5 ----- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index e81931c43..810856057 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -4,23 +4,22 @@ module BudgetInvestmentsHelper end def link_to_investments_sorted_by(column) - sort_by = column.downcase current_direction = params[:direction].downcase if params[:direction] direction = set_direction(current_direction) - icon = set_sorting_icon(direction, sort_by) + icon = set_sorting_icon(direction, column) - translation = t("admin.budget_investments.index.sort_by.#{sort_by}") + translation = t("admin.budget_investments.index.list.#{column}") link_to( "#{translation} ".html_safe, - admin_budget_budget_investments_path(sort_by: sort_by, direction: direction) + admin_budget_budget_investments_path(sort_by: column, direction: direction) ) end def set_sorting_icon(direction, sort_by) icon = direction == "desc" ? "icon-arrow-top" : "icon-arrow-down" - icon = sort_by == params[:sort_by] ? icon : "" + icon = sort_by.to_s == params[:sort_by] ? icon : "" end def set_direction(current_direction) diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index f7aea9c33..1bae41ee1 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -26,9 +26,9 @@ - - - + + +
<%= link_to_investments_sorted_by t("admin.budget_investments.index.list.id") %><%= link_to_investments_sorted_by t("admin.budget_investments.index.list.title") %><%= link_to_investments_sorted_by t("admin.budget_investments.index.list.supports") %><%= link_to_investments_sorted_by :id %><%= link_to_investments_sorted_by :title %><%= link_to_investments_sorted_by :supports %> <%= t("admin.budget_investments.index.list.admin") %> <%= t("admin.budget_investments.index.list.valuation_group") %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index edff8acf7..c7f6fb984 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -176,11 +176,6 @@ en: tags_filter_all: All tags advanced_filters: Advanced filters placeholder: Search projects - sort_by: - placeholder: Sort by - id: ID - title: Title - supports: Supports filters: all: All without_admin: Without assigned admin diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index c099bce58..ad829b724 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -176,11 +176,6 @@ es: tags_filter_all: Todas las etiquetas advanced_filters: Filtros avanzados placeholder: Buscar proyectos - sort_by: - placeholder: Ordenar por - id: ID - title: Título - supports: Apoyos filters: all: Todos without_admin: Sin administrador From 814579d58f5c110e1f649c6b4997df58cf79315e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:08:17 +0100 Subject: [PATCH 21/25] Simplify interpolation It's OK ot use single quotes inside a string with double quotes in order to avoid escape characters. --- app/helpers/budget_investments_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 810856057..419efe6b4 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -12,7 +12,7 @@ module BudgetInvestmentsHelper translation = t("admin.budget_investments.index.list.#{column}") link_to( - "#{translation} ".html_safe, + "#{translation} ".html_safe, admin_budget_budget_investments_path(sort_by: column, direction: direction) ) end From eb6bba52e3bf9e2f945803a8b9d48923df0e0fd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:09:34 +0100 Subject: [PATCH 22/25] Simplify code The string inside `params[:direction]` should already use lowercase characters. --- app/helpers/budget_investments_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index 419efe6b4..ee5aad93c 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -4,9 +4,7 @@ module BudgetInvestmentsHelper end def link_to_investments_sorted_by(column) - current_direction = params[:direction].downcase if params[:direction] - - direction = set_direction(current_direction) + direction = set_direction(params[:direction]) icon = set_sorting_icon(direction, column) translation = t("admin.budget_investments.index.list.#{column}") From 68fb295db5a1779c50dd24df2d72b1ae5982067b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:09:58 +0100 Subject: [PATCH 23/25] Use `if` blocks instead of two ternary operators Using a simple ternary operator is usually fine; however, code combining two ternary operator is a bit hard to follow. --- app/helpers/budget_investments_helper.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index ee5aad93c..3918c9b0d 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -16,8 +16,15 @@ module BudgetInvestmentsHelper end def set_sorting_icon(direction, sort_by) - icon = direction == "desc" ? "icon-arrow-top" : "icon-arrow-down" - icon = sort_by.to_s == params[:sort_by] ? icon : "" + if sort_by.to_s == params[:sort_by] + if direction == "desc" + "icon-arrow-top" + else + "icon-arrow-down" + end + else + "" + end end def set_direction(current_direction) From d5d800f75c57ca5df214b95cbbd0fdf0170d7512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:16:22 +0100 Subject: [PATCH 24/25] Simplify SORTING_OPTIONS usage Using a hash instead of an array of hashes makes accessing its keys and values much easier. --- app/models/budget/investment.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 4017fc2ce..c7357ee52 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -1,6 +1,6 @@ class Budget class Investment < ActiveRecord::Base - SORTING_OPTIONS = [{id: "id"}, {title: "title"}, {supports: "cached_votes_up"}].freeze + SORTING_OPTIONS = {id: "id", title: "title", supports: "cached_votes_up"}.freeze include Rails.application.routes.url_helpers include Measurable @@ -140,12 +140,12 @@ class Budget end def self.order_filter(params) - sorting_key = params[:sort_by].downcase.to_sym if params[:sort_by] - allowed_sort_option = SORTING_OPTIONS.select { |so| so[sorting_key]}.reduce + sorting_key = params[:sort_by]&.downcase&.to_sym + allowed_sort_option = SORTING_OPTIONS[sorting_key] if allowed_sort_option.present? direction = params[:direction] == "desc" ? "desc" : "asc" - return order("#{allowed_sort_option[sorting_key]} #{direction}") + return order("#{allowed_sort_option} #{direction}") end order(cached_votes_up: :desc).order(id: :desc) end From f8e9566699e5929e04a1b25add2b2f646d1ee6f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Jan 2019 15:20:26 +0100 Subject: [PATCH 25/25] Simplify return statement Just the way is usually done in the rest of the code. --- app/models/budget/investment.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index c7357ee52..30203bd04 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -145,9 +145,10 @@ class Budget if allowed_sort_option.present? direction = params[:direction] == "desc" ? "desc" : "asc" - return order("#{allowed_sort_option} #{direction}") + order("#{allowed_sort_option} #{direction}") + else + order(cached_votes_up: :desc).order(id: :desc) end - order(cached_votes_up: :desc).order(id: :desc) end def self.limit_results(budget, params, results)