From 4a5d4b3c0b975b1fc08f22f347e18153de362218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 8 Sep 2020 16:28:39 +0200 Subject: [PATCH 1/2] Group investment search form methods together We had helper methods all over the place which were only used in one view. Since we're going to change some of them to use the budget as a parameter, they don't belong in those helpers anymore. Note the method `budget_heading_select_options` is used in more places, so we're keeping it where it was. --- .../admin_budget_investments_helper.rb | 21 +++++++++++++++++++ app/helpers/admin_helper.rb | 4 ---- app/helpers/budgets_helper.rb | 4 ---- app/helpers/valuation_helper.rb | 13 ------------ 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/app/helpers/admin_budget_investments_helper.rb b/app/helpers/admin_budget_investments_helper.rb index e58a709f2..08c600243 100644 --- a/app/helpers/admin_budget_investments_helper.rb +++ b/app/helpers/admin_budget_investments_helper.rb @@ -12,4 +12,25 @@ module AdminBudgetInvestmentsHelper def init_advanced_menu params[:advanced_filters] = [] unless params[:advanced_filters] end + + def admin_select_options + Administrator.with_user.map { |v| [v.description_or_name, v.id] }.sort_by { |a| a[0] } + end + + def valuator_or_group_select_options + valuator_group_select_options + valuator_select_options + end + + def valuator_select_options + Valuator.order("description ASC").order("users.email ASC").includes(:user). + map { |v| [v.description_or_email, "valuator_#{v.id}"] } + end + + def valuator_group_select_options + ValuatorGroup.order("name ASC").map { |g| [g.name, "group_#{g.id}"] } + end + + def investment_tags_select_options(budget, context) + budget.investments.tags_on(context).order(:name).pluck(:name) + end end diff --git a/app/helpers/admin_helper.rb b/app/helpers/admin_helper.rb index 5731035c3..119bbe125 100644 --- a/app/helpers/admin_helper.rb +++ b/app/helpers/admin_helper.rb @@ -84,10 +84,6 @@ module AdminHelper options end - def admin_select_options - Administrator.with_user.map { |v| [v.description_or_name, v.id] }.sort_by { |a| a[0] } - end - def admin_submit_action(resource) resource.persisted? ? "edit" : "new" end diff --git a/app/helpers/budgets_helper.rb b/app/helpers/budgets_helper.rb index 122327830..d56d6de61 100644 --- a/app/helpers/budgets_helper.rb +++ b/app/helpers/budgets_helper.rb @@ -59,10 +59,6 @@ module BudgetsHelper Budget::Ballot.find_by(user: current_user, budget: @budget) end - def investment_tags_select_options(budget, context) - budget.investments.tags_on(context).order(:name).pluck(:name) - end - def unfeasible_or_unselected_filter ["unselected", "unfeasible"].include?(@current_filter) end diff --git a/app/helpers/valuation_helper.rb b/app/helpers/valuation_helper.rb index 9dab279ef..41217fdf5 100644 --- a/app/helpers/valuation_helper.rb +++ b/app/helpers/valuation_helper.rb @@ -1,17 +1,4 @@ module ValuationHelper - def valuator_or_group_select_options - valuator_group_select_options + valuator_select_options - end - - def valuator_select_options - Valuator.order("description ASC").order("users.email ASC").includes(:user). - map { |v| [v.description_or_email, "valuator_#{v.id}"] } - end - - def valuator_group_select_options - ValuatorGroup.order("name ASC").map { |g| [g.name, "group_#{g.id}"] } - end - def explanation_field(field) simple_format_no_tags_no_sanitize(sanitize_and_auto_link(field)) if field.present? end From 5332ae609ec503166ea2b474cbc6e8cba3fd9c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 8 Sep 2020 19:11:38 +0200 Subject: [PATCH 2/2] Filter investments only by assigned staff In commit 74083df1 we added the possibility to assign administrators and valuators to budgets, so they would only manage the budgets they're assigned to. However, when filtering projects, we were still showing all administrators and valuators as options to filter investments. It makes more sense to only show the valuators and administrators assigned to the current budget. Note this change only affects the view, and so malicious users could technically send any other administrator or valuator ID. In this case, they would get empty results since those administrators/valuators wouldn't have any investments assigned, so taking this case into account is not necessary. --- .../admin_budget_investments_helper.rb | 12 +++---- .../budget_investments/_search_form.html.erb | 4 +-- .../admin_budget_investments_helper_spec.rb | 35 +++++++++++++++++++ spec/system/admin/budget_investments_spec.rb | 6 +++- 4 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 spec/helpers/admin_budget_investments_helper_spec.rb diff --git a/app/helpers/admin_budget_investments_helper.rb b/app/helpers/admin_budget_investments_helper.rb index 08c600243..f34fbcb65 100644 --- a/app/helpers/admin_budget_investments_helper.rb +++ b/app/helpers/admin_budget_investments_helper.rb @@ -13,16 +13,16 @@ module AdminBudgetInvestmentsHelper params[:advanced_filters] = [] unless params[:advanced_filters] end - def admin_select_options - Administrator.with_user.map { |v| [v.description_or_name, v.id] }.sort_by { |a| a[0] } + def admin_select_options(budget) + budget.administrators.with_user.map { |v| [v.description_or_name, v.id] }.sort_by { |a| a[0] } end - def valuator_or_group_select_options - valuator_group_select_options + valuator_select_options + def valuator_or_group_select_options(budget) + valuator_group_select_options + valuator_select_options(budget) end - def valuator_select_options - Valuator.order("description ASC").order("users.email ASC").includes(:user). + def valuator_select_options(budget) + budget.valuators.order("description ASC").order("users.email ASC").includes(:user). map { |v| [v.description_or_email, "valuator_#{v.id}"] } end diff --git a/app/views/admin/budget_investments/_search_form.html.erb b/app/views/admin/budget_investments/_search_form.html.erb index 890700340..8b78a8f73 100644 --- a/app/views/admin/budget_investments/_search_form.html.erb +++ b/app/views/admin/budget_investments/_search_form.html.erb @@ -34,12 +34,12 @@
<%= select_tag :administrator_id, - options_for_select(admin_select_options, params[:administrator_id]), + options_for_select(admin_select_options(@budget), params[:administrator_id]), { prompt: t("admin.budget_investments.index.administrator_filter_all") } %>
<%= select_tag :valuator_or_group_id, - options_for_select(valuator_or_group_select_options, params[:valuator_or_group_id]), + options_for_select(valuator_or_group_select_options(@budget), params[:valuator_or_group_id]), { prompt: t("admin.budget_investments.index.valuator_filter_all") } %>
diff --git a/spec/helpers/admin_budget_investments_helper_spec.rb b/spec/helpers/admin_budget_investments_helper_spec.rb new file mode 100644 index 000000000..2c8a0294d --- /dev/null +++ b/spec/helpers/admin_budget_investments_helper_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +describe AdminBudgetInvestmentsHelper do + describe "#admin_select_options" do + it "includes administrators assigned to the budget" do + admin = create(:administrator, user: create(:user, username: "Winston")) + budget = create(:budget, administrators: [admin]) + + expect(admin_select_options(budget)).to eq([["Winston", admin.id]]) + end + + it "does not include other administrators" do + create(:administrator, user: create(:user, username: "Winston")) + budget = create(:budget, administrators: []) + + expect(admin_select_options(budget)).to be_empty + end + end + + describe "#valuator_select_options" do + it "includes valuators assigned to the budget" do + valuator = create(:valuator, description: "Kodogo") + budget = create(:budget, valuators: [valuator]) + + expect(valuator_select_options(budget)).to eq([["Kodogo", "valuator_#{valuator.id}"]]) + end + + it "does not include other valuators" do + create(:valuator, description: "Kodogo") + budget = create(:budget, valuators: []) + + expect(valuator_select_options(budget)).to be_empty + end + end +end diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 8b2f1f72b..ec8d4c9ec 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -160,7 +160,8 @@ describe "Admin budget investments" do user = create(:user, username: "Admin 1") user2 = create(:user, username: "Admin 2") administrator = create(:administrator, user: user) - create(:administrator, user: user2, description: "Alias") + administrator2 = create(:administrator, user: user2, description: "Alias") + budget.administrators = [administrator, administrator2] create(:budget_investment, title: "Realocate visitors", budget: budget, administrator: administrator) create(:budget_investment, title: "Destroy the city", budget: budget) @@ -201,6 +202,7 @@ describe "Admin budget investments" do scenario "Filtering by valuator", :js do user = create(:user) valuator = create(:valuator, user: user, description: "Valuator 1") + budget.valuators = [valuator] create(:budget_investment, title: "Realocate visitors", budget: budget, valuators: [valuator]) create(:budget_investment, title: "Destroy the city", budget: budget) @@ -645,6 +647,7 @@ describe "Admin budget investments" do scenario "Combination of checkbox with text search", :js do user = create(:user, username: "Admin 1") administrator = create(:administrator, user: user) + budget.administrators = [administrator] create(:budget_investment, budget: budget, title: "Educate the children", administrator: administrator) @@ -716,6 +719,7 @@ describe "Admin budget investments" do scenario "Combination of checkbox with text search and checkbox", :js do user = create(:user, username: "Admin 1") administrator = create(:administrator, user: user) + budget.administrators = [administrator] create(:budget_investment, :feasible, :finished, budget: budget, title: "Educate the children", administrator: administrator)