From 54a48d63e1b00cdcf9a9973e47809a9ec35aa1e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 9 Oct 2024 01:56:32 +0200 Subject: [PATCH] Use separate actions to select/deselect investments This is consistent to what we usually do. Also, we're applying the same criteria mentioned in commit 72704d776: > We're also making these actions idempotent, so sending many requests > to the same action will get the same result, which wasn't the case > with the `toggle` action. Although it's a low probability case, the > `toggle` action could result in [selecting an investment] when trying > to [deselect] it if someone else has [deselected it] it between the > time the page loaded and the time the admin clicked on the > "[Selected]" button. --- .../toggle_selection_component.html.erb | 2 +- .../toggle_selection_component.rb | 16 +++-- .../admin/budget_investments_controller.rb | 18 ++++-- app/models/abilities/administrator.rb | 2 +- config/routes/admin.rb | 5 +- .../budget_investments_controller_spec.rb | 61 ++++++++++++++++--- spec/models/abilities/administrator_spec.rb | 8 +-- 7 files changed, 87 insertions(+), 25 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 8981bf633..a417c6043 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,4 +1,4 @@ -<% if can?(:toggle_selection, investment) %> +<% if can?(action, investment) %> <%= render Admin::ToggleSwitchComponent.new(action, investment, pressed: selected?, **options) %> <% elsif selected? %> <%= selected_text %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb index 5e5fa43e4..faa54f518 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.rb +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -14,20 +14,26 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent end def action - :toggle_selection + if selected? + :deselect + else + :select + end end def path - toggle_selection_admin_budget_budget_investment_path( - investment.budget, - investment, + url_for({ + controller: "admin/budget_investments", + action: action, + budget_id: investment.budget, + id: investment, filter: params[:filter], sort_by: params[:sort_by], min_total_supports: params[:min_total_supports], max_total_supports: params[:max_total_supports], advanced_filters: params[:advanced_filters], page: params[:page] - ) + }) end def options diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index cb760d43b..a8e2b140e 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -9,7 +9,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController has_filters %w[all], only: :index before_action :load_budget - before_action :load_investment, only: [:show, :edit, :update, :toggle_selection] + before_action :load_investment, except: [:index] before_action :load_ballot, only: [:show, :index] before_action :parse_valuation_filters before_action :load_investments, only: :index @@ -60,10 +60,18 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end end - def toggle_selection - authorize! :toggle_selection, @investment - @investment.toggle :selected - @investment.save! + def select + authorize! :select, @investment + @investment.update!(selected: true) + + render :toggle_selection + end + + def deselect + authorize! :deselect, @investment + @investment.update!(selected: false) + + render :toggle_selection end private diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 82f6e48d7..5c9fb6dee 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -75,7 +75,7 @@ module Abilities can [:valuate, :comment_valuation], Budget::Investment cannot [:admin_update, :valuate, :comment_valuation], Budget::Investment, budget: { phase: "finished" } - can :toggle_selection, Budget::Investment do |investment| + can [:select, :deselect], Budget::Investment do |investment| investment.feasible? && investment.valuation_finished? && !investment.budget.finished? end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 442882bb5..b0c577675 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -66,7 +66,10 @@ namespace :admin do end resources :budget_investments, only: [:index, :show, :edit, :update] do - member { patch :toggle_selection } + member do + patch :select + patch :deselect + end resources :audits, only: :show, controller: "budget_investment_audits" resources :milestones, controller: "budget_investment_milestones" diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb index 7f7873192..8cee63ce0 100644 --- a/spec/controllers/admin/budget_investments_controller_spec.rb +++ b/spec/controllers/admin/budget_investments_controller_spec.rb @@ -38,18 +38,63 @@ describe Admin::BudgetInvestmentsController, :admin do end end - describe "PATCH toggle selection" do - it "uses the toggle_selection authorization rules" do - investment = create(:budget_investment) + describe "PATCH select" do + let(:investment) { create(:budget_investment, :feasible, :finished) } - patch :toggle_selection, xhr: true, params: { - id: investment, - budget_id: investment.budget, - } + it "selects the investment" do + expect do + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } + end.to change { investment.reload.selected? }.from(false).to(true) + + expect(response).to be_successful + end + + it "does not modify already selected investments" do + investment.update!(selected: true) + + expect do + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } + end.not_to change { investment.reload.selected? } + end + + it "uses the select/deselect authorization rules" do + investment.update!(valuation_finished: false) + + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } expect(flash[:alert]).to eq "You do not have permission to carry out the action " \ - "'toggle_selection' on Investment." + "'select' on Investment." expect(investment).not_to be_selected end end + + describe "PATCH deselect" do + let(:investment) { create(:budget_investment, :feasible, :finished, :selected) } + + it "deselects the investment" do + expect do + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + end.to change { investment.reload.selected? }.from(true).to(false) + + expect(response).to be_successful + end + + it "does not modify non-selected investments" do + investment.update!(selected: false) + + expect do + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + end.not_to change { investment.reload.selected? } + end + + it "uses the select/deselect authorization rules" do + investment.update!(valuation_finished: false) + + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action " \ + "'deselect' on Investment." + expect(investment).to be_selected + end + end end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index eab07520f..f75aac370 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -116,10 +116,10 @@ describe Abilities::Administrator do it { should_not be_able_to(:valuate, finished_investment) } it { should_not be_able_to(:comment_valuation, finished_investment) } - it { should be_able_to(:toggle_selection, create(:budget_investment, :feasible, :finished)) } - it { should_not be_able_to(:toggle_selection, create(:budget_investment, :feasible, :open)) } - it { should_not be_able_to(:toggle_selection, create(:budget_investment, :unfeasible, :finished)) } - it { should_not be_able_to(:toggle_selection, finished_investment) } + it { should be_able_to([:select, :deselect], create(:budget_investment, :feasible, :finished)) } + it { should_not be_able_to([:select, :deselect], create(:budget_investment, :feasible, :open)) } + it { should_not be_able_to([:select, :deselect], create(:budget_investment, :unfeasible, :finished)) } + it { should_not be_able_to([:select, :deselect], finished_investment) } it { should be_able_to(:destroy, proposal_image) } it { should be_able_to(:destroy, proposal_document) }