From fc5103881d78580c671ef9536b7c7160282befe4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Tue, 8 Oct 2024 16:04:18 +0200
Subject: [PATCH] Use a switch to toggle visibility to valuators

Using a checkbox wasn't very intuitive because checkboxes are
checked/unchecked when clicked on even if there's an error in the
request. Usually, when checkboxes appear on a form, they don't send any
information to the server unless we click a button to send the form.

So we're using a switch instead of a checkbox, like we did to
enable/disable phases in commit 46d8bc4f0.

Note that, since we've got two switches that match the default
`dom_id(record) .toggle-switch` selector, we need to find a way to
differentiate them. We're adding the `form_class` option for that.

Also note that we're now using a separate action and removing the
JavaScript in the `update` action which assumed that AJAX requests to
this action were always related to updating the `visible_to_valuators`
attribute.
---
 .../toggle_selection_component.rb             |  1 +
 ...le_visible_to_valuators_component.html.erb |  9 +--
 .../toggle_visible_to_valuators_component.rb  | 28 +++++++-
 .../admin/toggle_switch_component.html.erb    |  2 +-
 .../admin/toggle_switch_component.rb          |  6 +-
 .../admin/budget_investments_controller.rb    | 48 ++++++++-----
 .../toggle_selection.js.erb                   |  1 +
 .../toggle_visible_to_valuators.js.erb        |  4 ++
 app/views/admin/shared/_toggle_switch.js.erb  |  2 +-
 config/locales/en/admin.yml                   |  1 +
 config/locales/es/admin.yml                   |  1 +
 config/routes/admin.rb                        |  2 +
 ...gle_visible_to_valuators_component_spec.rb | 18 ++++-
 .../budget_investments_controller_spec.rb     | 63 ++++++++++++++---
 spec/system/admin/budget_investments_spec.rb  | 68 +++++++++++++------
 15 files changed, 191 insertions(+), 63 deletions(-)
 create mode 100644 app/views/admin/budget_investments/toggle_visible_to_valuators.js.erb

diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb
index faa54f518..4e35f425d 100644
--- a/app/components/admin/budget_investments/toggle_selection_component.rb
+++ b/app/components/admin/budget_investments/toggle_selection_component.rb
@@ -39,6 +39,7 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent
     def options
       {
         "aria-label": label,
+        form_class: "toggle-selection",
         path: path
       }
     end
diff --git a/app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb
index fda45a374..6233b0355 100644
--- a/app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb
+++ b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb
@@ -1,10 +1,5 @@
 <% if can?(:admin_update, investment) %>
-  <%= form_for [:admin, 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_#{investment.id}" %>
-  <% end %>
+  <%= render Admin::ToggleSwitchComponent.new(action, investment, pressed: visible_to_valuators?, **options) %>
 <% else %>
-  <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %>
+  <%= text %>
 <% end %>
diff --git a/app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb
index c1cdb0e31..2ecdfd8ef 100644
--- a/app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb
+++ b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb
@@ -1,6 +1,7 @@
 class Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent < ApplicationComponent
   attr_reader :investment
   use_helpers :can?
+  delegate :visible_to_valuators?, to: :investment
 
   def initialize(investment)
     @investment = investment
@@ -8,7 +9,30 @@ class Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent < ApplicationC
 
   private
 
-    def budget
-      investment.budget
+    def action
+      if visible_to_valuators?
+        :hide_from_valuators
+      else
+        :show_to_valuators
+      end
+    end
+
+    def text
+      if visible_to_valuators?
+        t("shared.yes")
+      else
+        t("shared.no")
+      end
+    end
+
+    def options
+      {
+        "aria-label": label,
+        form_class: "visible-to-valuators"
+      }
+    end
+
+    def label
+      t("admin.actions.show_to_valuators", name: investment.title)
     end
 end
diff --git a/app/components/admin/toggle_switch_component.html.erb b/app/components/admin/toggle_switch_component.html.erb
index 35b566c48..20639357f 100644
--- a/app/components/admin/toggle_switch_component.html.erb
+++ b/app/components/admin/toggle_switch_component.html.erb
@@ -1 +1 @@
-<%= render Admin::ActionComponent.new(action, record, **default_options.merge(options)) %>
+<%= render Admin::ActionComponent.new(action, record, **html_options) %>
diff --git a/app/components/admin/toggle_switch_component.rb b/app/components/admin/toggle_switch_component.rb
index ceee74d5b..c61c2dfe0 100644
--- a/app/components/admin/toggle_switch_component.rb
+++ b/app/components/admin/toggle_switch_component.rb
@@ -25,7 +25,11 @@ class Admin::ToggleSwitchComponent < ApplicationComponent
         method: :patch,
         remote: true,
         "aria-pressed": pressed?,
-        form_class: "toggle-switch"
+        form_class: "toggle-switch #{options[:form_class]}".strip
       }
     end
+
+    def html_options
+      default_options.merge(options.except(:form_class))
+    end
 end
diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb
index eec3583ac..6b031dd3b 100644
--- a/app/controllers/admin/budget_investments_controller.rb
+++ b/app/controllers/admin/budget_investments_controller.rb
@@ -39,24 +39,36 @@ class Admin::BudgetInvestmentsController < Admin::BaseController
   def update
     authorize! :admin_update, @investment
 
-    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
+    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
+  def show_to_valuators
+    authorize! :admin_update, @investment
+    @investment.update!(visible_to_valuators: true)
+
+    respond_to do |format|
+      format.html { redirect_to request.referer, notice: t("flash.actions.update.budget_investment") }
+      format.js { render :toggle_visible_to_valuators }
+    end
+  end
+
+  def hide_from_valuators
+    authorize! :admin_update, @investment
+    @investment.update!(visible_to_valuators: false)
+
+    respond_to do |format|
+      format.html { redirect_to request.referer, notice: t("flash.actions.update.budget_investment") }
+      format.js { render :toggle_visible_to_valuators }
     end
   end
 
@@ -108,7 +120,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController
 
     def allowed_params
       attributes = [:external_url, :heading_id, :administrator_id, :tag_list,
-                    :valuation_tag_list, :incompatible, :visible_to_valuators, :selected,
+                    :valuation_tag_list, :incompatible, :selected,
                     :milestone_tag_list, valuator_ids: [], valuator_group_ids: []]
       [*attributes, translation_params(Budget::Investment)]
     end
diff --git a/app/views/admin/budget_investments/toggle_selection.js.erb b/app/views/admin/budget_investments/toggle_selection.js.erb
index 98d258bad..bac042d52 100644
--- a/app/views/admin/budget_investments/toggle_selection.js.erb
+++ b/app/views/admin/budget_investments/toggle_selection.js.erb
@@ -1,3 +1,4 @@
 <%= render "admin/shared/toggle_switch",
            record: @investment,
+           form_class: "toggle-selection",
            new_content: render(Admin::BudgetInvestments::ToggleSelectionComponent.new(@investment)) %>
diff --git a/app/views/admin/budget_investments/toggle_visible_to_valuators.js.erb b/app/views/admin/budget_investments/toggle_visible_to_valuators.js.erb
new file mode 100644
index 000000000..207826f07
--- /dev/null
+++ b/app/views/admin/budget_investments/toggle_visible_to_valuators.js.erb
@@ -0,0 +1,4 @@
+<%= render "admin/shared/toggle_switch",
+           record: @investment,
+           form_class: "visible-to-valuators",
+           new_content: render(Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent.new(@investment)) %>
diff --git a/app/views/admin/shared/_toggle_switch.js.erb b/app/views/admin/shared/_toggle_switch.js.erb
index f8c172155..3be8b17dc 100644
--- a/app/views/admin/shared/_toggle_switch.js.erb
+++ b/app/views/admin/shared/_toggle_switch.js.erb
@@ -1,5 +1,5 @@
 var new_toggle_switch = $("<%= j new_content %>");
-var current_toggle_switch = $("#<%= dom_id(record) %> .toggle-switch");
+var current_toggle_switch = $("#<%= dom_id(record) %> .<%= local_assigns[:form_class] || "toggle-switch" %>");
 
 current_toggle_switch.replaceWith(new_toggle_switch);
 new_toggle_switch.find("[type='submit']").focus();
diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml
index 433d2d10f..d07775899 100644
--- a/config/locales/en/admin.yml
+++ b/config/locales/en/admin.yml
@@ -17,6 +17,7 @@ en:
       edit: Edit
       configure: Configure
       select: Select
+      show_to_valuators: "Show %{name} to valuators"
     officing_booth:
       title: "You are officing the booth located at %{booth}. If this is not correct, do not continue and call the help phone number. Thank you."
     banners:
diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml
index 0e5f3bd0e..d2b103454 100644
--- a/config/locales/es/admin.yml
+++ b/config/locales/es/admin.yml
@@ -17,6 +17,7 @@ es:
       edit: Editar
       configure: Configurar
       select: Seleccionar
+      show_to_valuators: "Mostrar %{name} a evaluadores"
     officing_booth:
       title: "Estás ahora mismo en la mesa ubicada en %{booth}. Si esto no es correcto no sigas adelante y llama al teléfono de incidencias. Gracias."
     banners:
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index b0c577675..9709300d3 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -69,6 +69,8 @@ namespace :admin do
       member do
         patch :select
         patch :deselect
+        patch :show_to_valuators
+        patch :hide_from_valuators
       end
 
       resources :audits, only: :show, controller: "budget_investment_audits"
diff --git a/spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb b/spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb
index 6bbe4a6d8..cf73b05f2 100644
--- a/spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb
+++ b/spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb
@@ -1,9 +1,21 @@
 require "rails_helper"
 
 describe Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent, :admin do
-  it "uses a JSON request to update visible to valuators" do
-    render_inline Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent.new(create(:budget_investment))
+  describe "aria-pressed attribute" do
+    it "is true for investments visible to valuators" do
+      investment = create(:budget_investment, :visible_to_valuators)
 
-    expect(page).to have_css "form[action$='json'] input[name$='[visible_to_valuators]']"
+      render_inline Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent.new(investment)
+
+      expect(page).to have_css "[aria-pressed=true]"
+    end
+
+    it "is true for investments invisible to valuators" do
+      investment = create(:budget_investment, :invisible_to_valuators)
+
+      render_inline Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent.new(investment)
+
+      expect(page).to have_css "[aria-pressed=false]"
+    end
   end
 end
diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb
index 54760c292..53122d970 100644
--- a/spec/controllers/admin/budget_investments_controller_spec.rb
+++ b/spec/controllers/admin/budget_investments_controller_spec.rb
@@ -23,18 +23,61 @@ describe Admin::BudgetInvestmentsController, :admin do
     end
   end
 
-  describe "PATCH update" do
-    it "does not redirect on AJAX requests" do
-      investment = create(:budget_investment)
+  describe "PATCH show_to_valuators" do
+    let(:investment) { create(:budget_investment, :invisible_to_valuators) }
 
-      patch :update, params: {
-        id: investment,
-        budget_id: investment.budget,
-        format: :json,
-        budget_investment: { visible_to_valuators: true }
-      }
+    it "marks the investment as visible to valuators" do
+      expect do
+        patch :show_to_valuators, xhr: true, params: { id: investment, budget_id: investment.budget }
+      end.to change { investment.reload.visible_to_valuators? }.from(false).to(true)
 
-      expect(response).not_to be_redirect
+      expect(response).to be_successful
+    end
+
+    it "does not modify investments visible to valuators" do
+      investment.update!(visible_to_valuators: true)
+
+      expect do
+        patch :show_to_valuators, xhr: true, params: { id: investment, budget_id: investment.budget }
+      end.not_to change { investment.reload.visible_to_valuators? }
+    end
+
+    it "redirects admins without JavaScript to the same page" do
+      request.env["HTTP_REFERER"] = admin_budget_budget_investments_path(investment.budget)
+
+      patch :show_to_valuators, params: { id: investment, budget_id: investment.budget }
+
+      expect(response).to redirect_to admin_budget_budget_investments_path(investment.budget)
+      expect(flash[:notice]).to eq "Investment project updated successfully."
+    end
+  end
+
+  describe "PATCH hide_from_valuators" do
+    let(:investment) { create(:budget_investment, :visible_to_valuators) }
+
+    it "marks the investment as visible to valuators" do
+      expect do
+        patch :hide_from_valuators, xhr: true, params: { id: investment, budget_id: investment.budget }
+      end.to change { investment.reload.visible_to_valuators? }.from(true).to(false)
+
+      expect(response).to be_successful
+    end
+
+    it "does not modify investments visible to valuators" do
+      investment.update!(visible_to_valuators: false)
+
+      expect do
+        patch :hide_from_valuators, xhr: true, params: { id: investment, budget_id: investment.budget }
+      end.not_to change { investment.reload.visible_to_valuators? }
+    end
+
+    it "redirects admins without JavaScript to the same page" do
+      request.env["HTTP_REFERER"] = admin_budget_budget_investments_path(investment.budget)
+
+      patch :hide_from_valuators, params: { id: investment, budget_id: investment.budget }
+
+      expect(response).to redirect_to admin_budget_budget_investments_path(investment.budget)
+      expect(flash[:notice]).to eq "Investment project updated successfully."
     end
   end
 
diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb
index 36f45b17f..bd695df79 100644
--- a/spec/system/admin/budget_investments_spec.rb
+++ b/spec/system/admin/budget_investments_spec.rb
@@ -1497,8 +1497,8 @@ describe "Admin budget investments", :admin do
 
     let(:heading) { create(:budget_heading, budget: budget) }
 
-    let(:investment1) { create(:budget_investment, heading: heading) }
-    let(:investment2) { create(:budget_investment, heading: heading) }
+    let(:investment1) { create(:budget_investment, heading: heading, title: "Investment 1") }
+    let(:investment2) { create(:budget_investment, heading: heading, title: "Investment 2") }
 
     scenario "Mark as visible to valuator" do
       investment1.valuators << valuator
@@ -1511,14 +1511,22 @@ describe "Admin budget investments", :admin do
       check "Under valuation"
       click_button "Filter"
 
-      within("#budget_investment_#{investment1.id}") do
-        check "budget_investment[visible_to_valuators]"
+      within("tr", text: "Investment 1") do
+        within("[data-field=visible_to_valuators]") do
+          expect(page).to have_content "No"
+
+          click_button "Show Investment 1 to valuators"
+
+          expect(page).to have_content "Yes"
+        end
       end
 
       refresh
 
-      within("#budget_investment_#{investment1.id}") do
-        expect(page).to have_field "budget_investment[visible_to_valuators]", checked: true
+      within("tr", text: "Investment 1") do
+        within("[data-field=visible_to_valuators]") do
+          expect(page).to have_content "Yes"
+        end
       end
     end
 
@@ -1557,14 +1565,22 @@ describe "Admin budget investments", :admin do
       check "Under valuation"
       click_button "Filter"
 
-      within("#budget_investment_#{investment1.id}") do
-        uncheck "budget_investment[visible_to_valuators]"
+      within("tr", text: "Investment 1") do
+        within("[data-field=visible_to_valuators]") do
+          expect(page).to have_content "Yes"
+
+          click_button "Show Investment 1 to valuators"
+
+          expect(page).to have_content "No"
+        end
       end
 
       refresh
 
-      within("#budget_investment_#{investment1.id}") do
-        expect(page).to have_field "budget_investment[visible_to_valuators]", checked: false
+      within("tr", text: "Investment 1") do
+        within("[data-field=visible_to_valuators]") do
+          expect(page).to have_content "No"
+        end
       end
     end
 
@@ -1576,16 +1592,16 @@ describe "Admin budget investments", :admin do
       visit admin_budget_budget_investments_path(budget)
 
       within "tr", text: "Visible" do
-        within "td[data-field=visible_to_valuators]" do
+        within "[data-field=visible_to_valuators]" do
           expect(page).to have_text "Yes"
-          expect(page).not_to have_field "budget_investment[visible_to_valuators]"
+          expect(page).not_to have_button
         end
       end
 
       within "tr", text: "Invisible" do
-        within "td[data-field=visible_to_valuators]" do
+        within "[data-field=visible_to_valuators]" do
           expect(page).to have_text "No"
-          expect(page).not_to have_field "budget_investment[visible_to_valuators]"
+          expect(page).not_to have_button
         end
       end
     end
@@ -1602,12 +1618,18 @@ describe "Admin budget investments", :admin do
       check "Under valuation"
       click_button "Filter"
 
-      within("#budget_investment_#{investment1.id}") do
-        expect(page).to have_field "budget_investment[visible_to_valuators]", checked: true
+      within "tr", text: "Investment 1" do
+        within "[data-field=visible_to_valuators]" do
+          expect(page).to have_content "Yes"
+          expect(page).to have_css "button[aria-pressed='true']"
+        end
       end
 
-      within("#budget_investment_#{investment2.id}") do
-        expect(page).to have_field "budget_investment[visible_to_valuators]", checked: false
+      within "tr", text: "Investment 2" do
+        within "[data-field=visible_to_valuators]" do
+          expect(page).to have_content "No"
+          expect(page).to have_css "button[aria-pressed='false']"
+        end
       end
     end
 
@@ -1617,8 +1639,14 @@ describe "Admin budget investments", :admin do
 
       visit admin_budget_budget_investments_path(budget)
 
-      within("#budget_investment_#{investment1.id}") do
-        check "budget_investment[visible_to_valuators]"
+      within "tr", text: "Investment 1" do
+        within "[data-field=visible_to_valuators]" do
+          expect(page).to have_content "No"
+
+          click_button "Show Investment 1 to valuators"
+
+          expect(page).to have_content "Yes"
+        end
       end
 
       visit edit_admin_budget_budget_investment_path(budget, investment1)