From 46d8bc4f0ee1cde932f2f4499d7e5a15c10016bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 23 Aug 2021 02:23:09 +0200 Subject: [PATCH] Use a switch to enable/disable budget phases In the past it would have been confusing to add a way to directly enable/disable a phase in the phases table because it was in the middle of the form. So we would have had next to each other controls that don't do anything until the form is sent and controls which modify the database immediately. That's why we couldn't add the checkboxes we used when using the wizard. Now the phases aren't on the same page as the budget form, so we can edit them independently. We're using a switch, so it's consistent with the way we enable/disable features. We could have used checkboxes, but with checkboxes, users expect they aren't changing anything until they click on a button to send the form, so we'd have to add a button, and it might be missed since we're going to add "buttons" for headings and groups to this page which won't send a form but will be links. Since we're changing the element with JavaScript after an AJAX call, we need a way to find the button we're changing. The easiest way is adding an ID attribute to all admin actions buttons/links. --- .../admin/budget_phases/phases.scss | 26 ++----------- app/components/admin/action_component.rb | 1 + .../admin/budget_phases/phases_component.rb | 25 ++---------- .../toggle_enabled_component.html.erb | 1 + .../budget_phases/toggle_enabled_component.rb | 27 +++++++++++++ .../phases/index_component.html.erb | 6 +-- .../admin/budgets_wizard/phases_controller.rb | 14 ------- .../concerns/admin/budget_phases_actions.rb | 11 +++++- .../admin/budget_phases/toggle_enabled.js.erb | 1 + .../phases/toggle_enabled.js.erb | 4 ++ config/locales/en/admin.yml | 2 - config/locales/es/admin.yml | 2 - config/routes/admin.rb | 6 ++- .../components/admin/action_component_spec.rb | 25 ++++++++++++ .../toggle_enabled_component_spec.rb | 28 +++++++++++++ .../admin/budgets_wizard/phases_spec.rb | 39 ++++++++++++------- .../admin/budgets_wizard/wizard_spec.rb | 8 +--- 17 files changed, 137 insertions(+), 89 deletions(-) create mode 100644 app/components/admin/budget_phases/toggle_enabled_component.html.erb create mode 100644 app/components/admin/budget_phases/toggle_enabled_component.rb create mode 100644 app/views/admin/budget_phases/toggle_enabled.js.erb create mode 100644 app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb create mode 100644 spec/components/admin/budget_phases/toggle_enabled_component_spec.rb diff --git a/app/assets/stylesheets/admin/budget_phases/phases.scss b/app/assets/stylesheets/admin/budget_phases/phases.scss index ca0269f23..0efe9037f 100644 --- a/app/assets/stylesheets/admin/budget_phases/phases.scss +++ b/app/assets/stylesheets/admin/budget_phases/phases.scss @@ -1,27 +1,7 @@ .budget-phases-table { - .budget-phase-enabled { - @include has-fa-icon(check, solid); - color: $check; - } - - .budget-phase-disabled { - @include has-fa-icon(times, solid); - color: $black; - } - - .budget-phase-enabled, - .budget-phase-disabled { - display: block; - font-size: 1px; - left: $off-screen-left; - line-height: 1; - position: relative; - - &::before { - font-size: $base-font-size; - left: -1 * $off-screen-left; - position: relative; - } + [aria-pressed] { + @include switch; + margin-bottom: 0; } } diff --git a/app/components/admin/action_component.rb b/app/components/admin/action_component.rb index 2481a07b0..debc27c64 100644 --- a/app/components/admin/action_component.rb +++ b/app/components/admin/action_component.rb @@ -25,6 +25,7 @@ class Admin::ActionComponent < ApplicationComponent def html_options { class: html_class, + id: (dom_id(record, action) if record.respond_to?(:to_key)), "aria-label": label, data: { confirm: confirmation_text, diff --git a/app/components/admin/budget_phases/phases_component.rb b/app/components/admin/budget_phases/phases_component.rb index 383f46c53..5dfd9c6f5 100644 --- a/app/components/admin/budget_phases/phases_component.rb +++ b/app/components/admin/budget_phases/phases_component.rb @@ -1,9 +1,8 @@ class Admin::BudgetPhases::PhasesComponent < ApplicationComponent - attr_reader :budget, :form + attr_reader :budget - def initialize(budget, form: nil) + def initialize(budget) @budget = budget - @form = form end private @@ -17,25 +16,7 @@ class Admin::BudgetPhases::PhasesComponent < ApplicationComponent end def enabled_cell(phase) - if form - form.fields_for :phases, phase do |phase_fields| - phase_fields.check_box :enabled, - label: false, - aria: { - label: t("admin.budgets.edit.enable_phase", phase: phase.name) - } - end - else - enabled_text(phase) - end - end - - def enabled_text(phase) - if phase.enabled? - tag.span t("shared.yes"), class: "budget-phase-enabled" - else - tag.span t("shared.no"), class: "budget-phase-disabled" - end + render Admin::BudgetPhases::ToggleEnabledComponent.new(phase) end def edit_path(phase) diff --git a/app/components/admin/budget_phases/toggle_enabled_component.html.erb b/app/components/admin/budget_phases/toggle_enabled_component.html.erb new file mode 100644 index 000000000..d230e97e5 --- /dev/null +++ b/app/components/admin/budget_phases/toggle_enabled_component.html.erb @@ -0,0 +1 @@ +<%= render Admin::ActionComponent.new(:toggle_enabled, phase, options) %> diff --git a/app/components/admin/budget_phases/toggle_enabled_component.rb b/app/components/admin/budget_phases/toggle_enabled_component.rb new file mode 100644 index 000000000..0236cdae5 --- /dev/null +++ b/app/components/admin/budget_phases/toggle_enabled_component.rb @@ -0,0 +1,27 @@ +class Admin::BudgetPhases::ToggleEnabledComponent < ApplicationComponent + attr_reader :phase + + def initialize(phase) + @phase = phase + end + + private + + def options + { + text: text, + method: :patch, + remote: true, + "aria-label": t("admin.budgets.edit.enable_phase", phase: phase.name), + "aria-pressed": phase.enabled? + } + end + + def text + if phase.enabled? + t("shared.yes") + else + t("shared.no") + end + end +end diff --git a/app/components/admin/budgets_wizard/phases/index_component.html.erb b/app/components/admin/budgets_wizard/phases/index_component.html.erb index e0cf8c9b7..fe5da4590 100644 --- a/app/components/admin/budgets_wizard/phases/index_component.html.erb +++ b/app/components/admin/budgets_wizard/phases/index_component.html.erb @@ -5,7 +5,5 @@ <%= render Admin::Budgets::HelpComponent.new("phases") %> <%= render Admin::BudgetsWizard::CreationTimelineComponent.new("phases") %> -<%= form_for budget, url: update_all_admin_budgets_wizard_budget_budget_phases_path(budget) do |f| %> - <%= render Admin::BudgetPhases::PhasesComponent.new(budget, form: f) %> - <%= f.submit t("admin.budgets_wizard.phases.continue"), class: "button success" %> -<% end %> +<%= render Admin::BudgetPhases::PhasesComponent.new(budget) %> +<%= link_to t("admin.budgets_wizard.phases.continue"), admin_budgets_path, class: "button success" %> diff --git a/app/controllers/admin/budgets_wizard/phases_controller.rb b/app/controllers/admin/budgets_wizard/phases_controller.rb index 59903c3b5..113893aa5 100644 --- a/app/controllers/admin/budgets_wizard/phases_controller.rb +++ b/app/controllers/admin/budgets_wizard/phases_controller.rb @@ -4,23 +4,9 @@ class Admin::BudgetsWizard::PhasesController < Admin::BudgetsWizard::BaseControl def index end - def update_all - @budget.update!(phases_params) - - redirect_to admin_budgets_path, notice: t("admin.budgets_wizard.phases.update_all.notice") - end - private def phases_index admin_budgets_wizard_budget_budget_phases_path(@phase.budget, url_params) end - - def phases_params - params.require(:budget).permit(allowed_phases_params) - end - - def allowed_phases_params - { phases_attributes: [:id, :enabled] } - end end diff --git a/app/controllers/concerns/admin/budget_phases_actions.rb b/app/controllers/concerns/admin/budget_phases_actions.rb index 6d0434416..5d0ce53e2 100644 --- a/app/controllers/concerns/admin/budget_phases_actions.rb +++ b/app/controllers/concerns/admin/budget_phases_actions.rb @@ -6,7 +6,7 @@ module Admin::BudgetPhasesActions include ImageAttributes before_action :load_budget - before_action :load_phase, only: [:edit, :update] + before_action :load_phase, only: [:edit, :update, :toggle_enabled] end def edit @@ -20,6 +20,15 @@ module Admin::BudgetPhasesActions end end + def toggle_enabled + @phase.update!(enabled: !@phase.enabled) + + respond_to do |format| + format.html { redirect_to phases_index, notice: t("flash.actions.save_changes.notice") } + format.js + end + end + private def load_budget diff --git a/app/views/admin/budget_phases/toggle_enabled.js.erb b/app/views/admin/budget_phases/toggle_enabled.js.erb new file mode 100644 index 000000000..b90a979bd --- /dev/null +++ b/app/views/admin/budget_phases/toggle_enabled.js.erb @@ -0,0 +1 @@ +<%= render template: "admin/budgets_wizard/phases/toggle_enabled" %> diff --git a/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb b/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb new file mode 100644 index 000000000..d4424bf4c --- /dev/null +++ b/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb @@ -0,0 +1,4 @@ +var replacement = $("<%= j render Admin::BudgetPhases::ToggleEnabledComponent.new(@phase) %>"); +var form = $("#" + replacement.find("[type='submit']").attr("id")).closest("form"); + +form.html(replacement.html()).find("[type='submit']").focus(); diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 7dc2270f5..33f391383 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -340,8 +340,6 @@ en: back: "Go back to headings" single: back: "Go back to edit heading" - update_all: - notice: "Phases configured successfully" milestones: index: table_id: "ID" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index bc574ed5b..9d30c4b78 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -340,8 +340,6 @@ es: back: "Volver a partidas" single: back: "Volver a editar partida" - update_all: - notice: "Fases configuradas con éxito" milestones: index: table_id: "ID" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 4a358a0c7..27776f745 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -69,7 +69,9 @@ namespace :admin do resources :progress_bars, except: :show, controller: "budget_investment_progress_bars" end - resources :budget_phases, only: [:edit, :update] + resources :budget_phases, only: [:edit, :update] do + member { patch :toggle_enabled } + end end namespace :budgets_wizard do @@ -79,7 +81,7 @@ namespace :admin do end resources :phases, as: "budget_phases", only: [:index, :edit, :update] do - collection { patch :update_all } + member { patch :toggle_enabled } end end end diff --git a/spec/components/admin/action_component_spec.rb b/spec/components/admin/action_component_spec.rb index e32e8a6f6..5712b4479 100644 --- a/spec/components/admin/action_component_spec.rb +++ b/spec/components/admin/action_component_spec.rb @@ -16,6 +16,31 @@ describe Admin::ActionComponent do end end + describe "HTML id" do + it "is not rendered for non-ActiveModel records" do + render_inline Admin::ActionComponent.new(:edit, double, path: "/") + + expect(page).not_to have_css "[id]" + end + + it "includes an id based on the model and the action by default" do + record = double(model_name: double(param_key: "computer"), to_key: [1]) + + render_inline Admin::ActionComponent.new(:edit, record, path: "/") + + expect(page).to have_css "a.edit-link#edit_computer_1" + end + + it "can be overwritten" do + record = double(model_name: double(param_key: "computer"), to_key: [1]) + + render_inline Admin::ActionComponent.new(:edit, record, path: "/", id: "my_id") + + expect(page).to have_css "a.edit-link#my_id" + expect(page).not_to have_css "#edit_computer_1" + end + end + describe "aria-label attribute" do it "is not rendered by default" do record = double(human_name: "Stay home") diff --git a/spec/components/admin/budget_phases/toggle_enabled_component_spec.rb b/spec/components/admin/budget_phases/toggle_enabled_component_spec.rb new file mode 100644 index 000000000..6786e1eb8 --- /dev/null +++ b/spec/components/admin/budget_phases/toggle_enabled_component_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +describe Admin::BudgetPhases::ToggleEnabledComponent, controller: Admin::BaseController do + let(:phase) { create(:budget).phases.informing } + let(:component) { Admin::BudgetPhases::ToggleEnabledComponent.new(phase) } + + it "is pressed when the phase is enabled" do + phase.update!(enabled: true) + + render_inline component + + expect(page).to have_button count: 1 + expect(page).to have_button exact_text: "Yes" + expect(page).to have_button "Enable Information phase" + expect(page).to have_css "button[aria-pressed='true']" + end + + it "is not pressed when the phase is disabled" do + phase.update!(enabled: false) + + render_inline component + + expect(page).to have_button count: 1 + expect(page).to have_button exact_text: "No" + expect(page).to have_button "Enable Information phase" + expect(page).to have_css "button[aria-pressed='false']" + end +end diff --git a/spec/system/admin/budgets_wizard/phases_spec.rb b/spec/system/admin/budgets_wizard/phases_spec.rb index 3b692ad32..2f053689d 100644 --- a/spec/system/admin/budgets_wizard/phases_spec.rb +++ b/spec/system/admin/budgets_wizard/phases_spec.rb @@ -23,29 +23,42 @@ describe "Budgets wizard, phases step", :admin do scenario "Enable and disable phases" do visit admin_budgets_wizard_budget_budget_phases_path(budget) - uncheck "Enable Information phase" - uncheck "Enable Reviewing voting phase" - - click_button "Finish" - - expect(page).to have_content "Phases configured successfully" - - visit admin_budget_path(budget) - within "tr", text: "Information" do - expect(page).to have_css ".budget-phase-disabled", visible: :all + expect(page).to have_content "Yes" + + click_button "Enable Information phase" + + expect(page).to have_content "No" + expect(page).not_to have_content "Yes" end within "tr", text: "Reviewing voting" do - expect(page).to have_css ".budget-phase-disabled", visible: :all + expect(page).to have_content "Yes" + + click_button "Enable Reviewing voting phase" + + expect(page).to have_content "No" + expect(page).not_to have_content "Yes" + end + + click_link "Finish" + + within("tr", text: budget.name) { click_link "Edit" } + + within "tr", text: "Information" do + expect(page).to have_content "No" + end + + within "tr", text: "Reviewing voting" do + expect(page).to have_content "No" end within "tr", text: "Accepting projects" do - expect(page).to have_css ".budget-phase-enabled", visible: :all + expect(page).to have_content "Yes" end within "tr", text: "Voting projects" do - expect(page).to have_css ".budget-phase-enabled", visible: :all + expect(page).to have_content "Yes" end end end diff --git a/spec/system/admin/budgets_wizard/wizard_spec.rb b/spec/system/admin/budgets_wizard/wizard_spec.rb index b388feac0..e2495aef4 100644 --- a/spec/system/admin/budgets_wizard/wizard_spec.rb +++ b/spec/system/admin/budgets_wizard/wizard_spec.rb @@ -22,9 +22,7 @@ describe "Budgets creation wizard", :admin do expect(page).to have_css ".budget-phases-table" - click_button "Finish" - - expect(page).to have_content "Phases configured successfully" + click_link "Finish" within "tr", text: "Single heading budget" do click_link "Heading groups" @@ -119,9 +117,7 @@ describe "Budgets creation wizard", :admin do expect(page).not_to have_content "Voting projects" end - click_button "Finish" - - expect(page).to have_content "Phases configured successfully" + click_link "Finish" within "tr", text: "Multiple headings budget" do click_link "Heading groups"