From 37e7eeb6e162c7e733dd116b39d6178f6bc5cf41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 23 Oct 2020 13:43:25 +0200 Subject: [PATCH 1/2] Don't redirect when toggling visible to valuators After upgrading to Turbolinks 5, redirects are followed on AJAX requests, so we were accidentally redirecting the user after they mark an investment as visible to valuators. There was already a system spec failing due to this issue ("Admin budget investments Mark as visible to valuators Keeps the valuation tags"); however, it only failed in some cases, so we're adding additional tests. Ideally we would write a system test to check what happens when users click on the checkbox. However, from the user's point of view, nothing happens when they do so, and so testing it is hard. There's a usability issue here (no feedback is provided to the user indicating the investment is actually updated when they click on the checkbox and so they might look for a button to send the form), which also results in a feature which is difficult to test. So we're writing two tests instead: one checking the controller does not redirect when using a JSON request, and one checking the form submits a JSON request. I've chosen JSON over AJAX because usually requests to the update action come from the edit form, and we might change the edit form to send an AJAX request (and, in this case, Turbolinks would handle the redirect as mentioned above). Another option would be to send an AJAX request to a different action, like it's done for the toggle selection action. I don't have a strong preference for either option, so I'm leaving it the way it was. At some point we should change the user interface, though; right now in the same row there are two actions doing basically the same thing (toggling valuator visibility and toggling selection) but with very different user interfaces (one is a checkbox and the other one a link changing its style depending on the state), resulting in a confusing interface. --- .../admin/budget_investments_controller.rb | 29 ++++++++++++------- .../_select_investment.html.erb | 2 +- .../budget_investments_controller_spec.rb | 19 ++++++++++++ spec/spec_helper.rb | 1 + .../select_investment_spec.rb | 13 +++++++++ 5 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 spec/controllers/admin/budget_investments_controller_spec.rb create mode 100644 spec/views/admin/budget_investments/select_investment_spec.rb diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 8f0fa3dca..a55c04a01 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -39,16 +39,25 @@ class Admin::BudgetInvestmentsController < Admin::BaseController def update authorize! :admin_update, @investment - if @investment.update(budget_investment_params) - redirect_to admin_budget_budget_investment_path(@budget, - @investment, - Budget::Investment.filter_params(params).to_h), - notice: t("flash.actions.update.budget_investment") - else - load_staff - load_valuator_groups - load_tags - render :edit + + respond_to do |format| + format.html do + if @investment.update(budget_investment_params) + redirect_to admin_budget_budget_investment_path(@budget, + @investment, + Budget::Investment.filter_params(params).to_h), + notice: t("flash.actions.update.budget_investment") + else + load_staff + load_valuator_groups + load_tags + render :edit + end + end + + format.json do + @investment.update!(budget_investment_params) + end end end diff --git a/app/views/admin/budget_investments/_select_investment.html.erb b/app/views/admin/budget_investments/_select_investment.html.erb index 698aab1dc..a7593a0c5 100644 --- a/app/views/admin/budget_investments/_select_investment.html.erb +++ b/app/views/admin/budget_investments/_select_investment.html.erb @@ -51,7 +51,7 @@ - <%= form_for [:admin, investment.budget, investment], remote: true do |f| %> + <%= form_for [:admin, investment.budget, investment], remote: true, format: :json do |f| %> <%= f.check_box :visible_to_valuators, label: false, class: "js-submit-on-change", diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb new file mode 100644 index 000000000..3d0c1fddc --- /dev/null +++ b/spec/controllers/admin/budget_investments_controller_spec.rb @@ -0,0 +1,19 @@ +require "rails_helper" + +describe Admin::BudgetInvestmentsController do + describe "PATCH update" do + it "does not redirect on AJAX requests" do + investment = create(:budget_investment) + sign_in(create(:administrator).user) + + patch :update, params: { + id: investment, + budget_id: investment.budget, + format: :json, + budget_investment: { visible_to_valuators: true } + } + + expect(response).not_to be_redirect + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5a775065f..1996c2bb1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -14,6 +14,7 @@ RSpec.configure do |config| config.run_all_when_everything_filtered = true config.include RequestSpecHelper, type: :request config.include Devise::Test::ControllerHelpers, type: :controller + config.include Devise::Test::ControllerHelpers, type: :view config.include FactoryBot::Syntax::Methods config.include(EmailSpec::Helpers) config.include(EmailSpec::Matchers) diff --git a/spec/views/admin/budget_investments/select_investment_spec.rb b/spec/views/admin/budget_investments/select_investment_spec.rb new file mode 100644 index 000000000..e04c6dd8f --- /dev/null +++ b/spec/views/admin/budget_investments/select_investment_spec.rb @@ -0,0 +1,13 @@ +require "rails_helper" + +describe "investment row" do + it "uses a JSON request to update visible to valuators" do + investment = create(:budget_investment) + @budget = investment.budget + sign_in(create(:administrator).user) + + render "admin/budget_investments/select_investment", investment: investment + + expect(rendered).to have_css "form[action$='json'] input[name$='[visible_to_valuators]']" + end +end From 01cfed8882b464815a1523c4322e32be57ef1e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 23 Oct 2020 14:03:55 +0200 Subject: [PATCH 2/2] Hide checkbox when investments cannot be updated We were allowing users to check/uncheck the "Visible to valuators" checkbox even when the budget is finished and so the investments cannot be edited. So users were still able to check/uncheck this attribute, but the server was silently rejecting these changes. We've considered removing the column in this case but decided to keep it since users can already control which columns they'd like to display. --- .../_select_investment.html.erb | 14 +++++++----- spec/system/admin/budget_investments_spec.rb | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/views/admin/budget_investments/_select_investment.html.erb b/app/views/admin/budget_investments/_select_investment.html.erb index a7593a0c5..320abc579 100644 --- a/app/views/admin/budget_investments/_select_investment.html.erb +++ b/app/views/admin/budget_investments/_select_investment.html.erb @@ -51,11 +51,15 @@ - <%= form_for [:admin, investment.budget, investment], remote: true, format: :json do |f| %> - <%= f.check_box :visible_to_valuators, - label: false, - class: "js-submit-on-change", - id: "budget_investment_visible_to_valuators" %> + <% if can?(:admin_update, investment) %> + <%= form_for [:admin, investment.budget, investment], remote: true, format: :json do |f| %> + <%= f.check_box :visible_to_valuators, + label: false, + class: "js-submit-on-change", + id: "budget_investment_visible_to_valuators" %> + <% end %> + <% else %> + <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %> <% end %> diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index ec8d4c9ec..165b15aea 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1651,6 +1651,28 @@ describe "Admin budget investments" do end end + scenario "Cannot mark/unmark visible to valuators on finished budgets" do + budget.update!(phase: "finished") + create(:budget_investment, budget: budget, title: "Visible", visible_to_valuators: true) + create(:budget_investment, budget: budget, title: "Invisible", visible_to_valuators: false) + + visit admin_budget_budget_investments_path(budget) + + within "tr", text: "Visible" do + within "td[data-field=visible_to_valuators]" do + expect(page).to have_text "Yes" + expect(page).not_to have_field "budget_investment_visible_to_valuators" + end + end + + within "tr", text: "Invisible" do + within "td[data-field=visible_to_valuators]" do + expect(page).to have_text "No" + expect(page).not_to have_field "budget_investment_visible_to_valuators" + end + end + end + scenario "Showing the valuating checkbox" do investment1 = create(:budget_investment, :with_administrator, :with_valuator, :visible_to_valuators, budget: budget)