From 6bbfb555868249c4c4b1f77c689163e1ffd54da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Nov 2019 14:43:54 +0100 Subject: [PATCH 1/3] Fix admin permissions for finished budgets Although we weren't showing links in the views to execute certain actions, forms could be still sent using a PUT/PATCH pull request to the controller actions. --- .../admin/budget_investments_controller.rb | 2 + app/models/abilities/administrator.rb | 4 +- .../_select_investment.html.erb | 48 ++++++++++--------- .../admin/budget_investments/show.html.erb | 43 ++++++++++------- .../budget_investments/show.html.erb | 20 ++++---- spec/models/abilities/administrator_spec.rb | 6 ++- 6 files changed, 73 insertions(+), 50 deletions(-) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index db731040bbc..8f0fa3dca02 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -31,6 +31,7 @@ def show end def edit + authorize! :admin_update, @investment load_staff load_valuator_groups load_tags @@ -52,6 +53,7 @@ def update end def toggle_selection + authorize! :toggle_selection, @investment @investment.toggle :selected @investment.save! load_investments diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index ef51690e076..b997e56a478 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -64,7 +64,9 @@ def initialize(user) can [:read, :create, :update, :destroy], Budget::Heading can [:hide, :admin_update, :toggle_selection], Budget::Investment can [:valuate, :comment_valuation], Budget::Investment - cannot [:comment_valuation], Budget::Investment, budget: { phase: "finished" } + cannot [:admin_update, :toggle_selection, :valuate, :comment_valuation], + Budget::Investment, budget: { phase: "finished" } + can :create, Budget::ValuatorAssignment can :read_admin_stats, Budget, &:balloting_or_later? diff --git a/app/views/admin/budget_investments/_select_investment.html.erb b/app/views/admin/budget_investments/_select_investment.html.erb index b293894b26c..698aab1dc65 100644 --- a/app/views/admin/budget_investments/_select_investment.html.erb +++ b/app/views/admin/budget_investments/_select_investment.html.erb @@ -61,30 +61,34 @@ <% if investment.selected? %> - <%= link_to_unless investment.budget.finished?, - t("admin.budget_investments.index.selected"), - toggle_selection_admin_budget_budget_investment_path(@budget, - 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]), - method: :patch, - remote: true, - class: "button small expanded" %> + <%= link_to_if can?(:toggle_selection, investment), + t("admin.budget_investments.index.selected"), + toggle_selection_admin_budget_budget_investment_path( + @budget, + 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] + ), + method: :patch, + remote: true, + class: "button small expanded" %> <% elsif investment.feasible? && investment.valuation_finished? %> - <% unless investment.budget.finished? %> + <% if can?(:toggle_selection, investment) %> <%= link_to t("admin.budget_investments.index.select"), - toggle_selection_admin_budget_budget_investment_path(@budget, - 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]), + toggle_selection_admin_budget_budget_investment_path( + @budget, + 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] + ), method: :patch, remote: true, class: "button small hollow expanded" %> diff --git a/app/views/admin/budget_investments/show.html.erb b/app/views/admin/budget_investments/show.html.erb index 5c9f85203cc..375c5e978d3 100644 --- a/app/views/admin/budget_investments/show.html.erb +++ b/app/views/admin/budget_investments/show.html.erb @@ -6,15 +6,17 @@ <%= render "written_by_author" %>

<%= t("admin.budget_investments.show.preview") %>

-
- <%= link_to t("admin.budget_investments.show.edit"), - edit_admin_budget_budget_investment_path( - @budget, - @investment, - Budget::Investment.filter_params(params).to_h - ), - class: "button hollow" unless @budget.finished? %> -
+<% if can?(:admin_update, @investment) %> +
+ <%= link_to t("admin.budget_investments.show.edit"), + edit_admin_budget_budget_investment_path( + @budget, + @investment, + Budget::Investment.filter_params(params).to_h + ), + class: "button hollow" %> +
+<% end %>
<%= render "/budgets/investments/investment_detail", investment: @investment, preview: true %> @@ -50,19 +52,26 @@ <% end %>

-

- <%= link_to t("admin.budget_investments.show.edit_classification"), - edit_admin_budget_budget_investment_path(@budget, @investment, - { anchor: "classification" }.merge(Budget::Investment.filter_params(params).to_h)) unless @budget.finished? %> -

+<% if can?(:admin_update, @investment) %> +

+ <%= link_to t("admin.budget_investments.show.edit_classification"), + edit_admin_budget_budget_investment_path( + @budget, + @investment, + { anchor: "classification" }.merge(Budget::Investment.filter_params(params).to_h) + ) %> +

+<% end %>

<%= t("admin.budget_investments.show.dossier") %>

<%= render "valuation/budget_investments/dossier" %> -

- <%= link_to t("admin.budget_investments.show.edit_dossier"), edit_valuation_budget_budget_investment_path(@budget, @investment) unless @budget.finished? %> -

+<% if can?(:valuate, @investment) %> +

+ <%= link_to t("admin.budget_investments.show.edit_dossier"), edit_valuation_budget_budget_investment_path(@budget, @investment) %> +

+<% end %> <%= render "valuation/budget_investments/valuation_comments" %> diff --git a/app/views/valuation/budget_investments/show.html.erb b/app/views/valuation/budget_investments/show.html.erb index 7d987421999..1ac2f68a9e0 100644 --- a/app/views/valuation/budget_investments/show.html.erb +++ b/app/views/valuation/budget_investments/show.html.erb @@ -3,15 +3,17 @@

<%= t("admin.budget_investments.show.preview") %>

-
- <%= link_to t("admin.budget_investments.show.edit"), - edit_valuation_budget_budget_investment_path( - @budget, - @investment, - Budget::Investment.filter_params(params) - ), - class: "button hollow" unless @budget.finished? %> -
+<% if can?(:valuate, @investment) %> +
+ <%= link_to t("admin.budget_investments.show.edit"), + edit_valuation_budget_budget_investment_path( + @budget, + @investment, + Budget::Investment.filter_params(params) + ), + class: "button hollow" %> +
+<% end %>
<%= render "/budgets/investments/investment_detail", investment: @investment, preview: true %> diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 4576c167347..c904dcb91ce 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -15,6 +15,7 @@ let(:comment) { create(:comment) } let(:proposal) { create(:proposal, author: user) } let(:budget_investment) { create(:budget_investment) } + let(:finished_investment) { create(:budget_investment, budget: create(:budget, :finished)) } let(:legislation_question) { create(:legislation_question) } let(:poll_question) { create(:poll_question) } @@ -77,7 +78,10 @@ it { should be_able_to(:hide, Budget::Investment) } it { should be_able_to(:valuate, create(:budget_investment, budget: create(:budget, :valuating))) } - it { should be_able_to(:valuate, create(:budget_investment, budget: create(:budget, :finished))) } + it { should_not be_able_to(:admin_update, finished_investment) } + it { should_not be_able_to(:valuate, finished_investment) } + it { should_not be_able_to(:comment_valuation, finished_investment) } + it { should_not be_able_to(:toggle_selection, finished_investment) } it { should be_able_to(:destroy, proposal_image) } it { should be_able_to(:destroy, proposal_document) } From 33c2b28063a7164beee57b449355d07e4b2e26ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Nov 2019 15:21:10 +0100 Subject: [PATCH 2/3] Remove duplicate test This test is exactly the same as the "visible for admins" scenario. --- .../valuation/budget_investments_spec.rb | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/spec/features/valuation/budget_investments_spec.rb b/spec/features/valuation/budget_investments_spec.rb index 11656f9ef63..ce4d014b135 100644 --- a/spec/features/valuation/budget_investments_spec.rb +++ b/spec/features/valuation/budget_investments_spec.rb @@ -283,23 +283,6 @@ visit valuation_budget_budget_investment_path(budget, investment) end.to raise_error "Not Found" end - - scenario "preview is visible" do - logout - login_as create(:administrator).user - - visit valuation_budget_budget_investment_path(budget, investment) - - expect(page).to have_content("Investment preview") - expect(page).to have_content(investment.title) - expect(page).to have_content(investment.description) - expect(page).to have_content(investment.author.name) - expect(page).to have_content(investment.heading.name) - expect(page).to have_content("1234") - expect(page).to have_content("Unfeasible") - expect(page).to have_content("It is impossible") - expect(page).to have_content("Ana (ana@admins.org)") - end end describe "Valuate" do From 0383ed96f576fdceebd0e03d630af5ca9020abf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Nov 2019 15:31:51 +0100 Subject: [PATCH 3/3] Fix typo related to investment previews --- spec/features/admin/budget_investments_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index f239e188d8e..5cd9a41398f 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -1017,7 +1017,7 @@ expect(page).to have_content("Ana (ana@admins.org)") end - scenario "Not show related content or hide links on preview" do + scenario "Does not show related content or hide links on preview" do budget_investment = create(:budget_investment, :unfeasible, price: 1234,