From 51a0bce58cd791dc46c027fa5ef91311b68e09c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 30 Aug 2021 18:36:24 +0200 Subject: [PATCH] Add information about budget actions Both the calculate winners and delete actions benefit from some kind of hint. The "calculate winners" hint informs administrators that results won't be publicly available unless the "show results" option is enabled. The delete action was redirecting with an error message when the budget couldn't be deleted; IMHO it's better to disable it and inform administrators why it's disabled. Alternatively we could remove the button completely; however, users might be looking for a way to delete a budget and wouldn't find any hint about it. We're now removing the "Delete" action from the budgets index table, since most of the time it isn't possible to delete a budget and so the action takes up space and we get little gain in return. We could keep the "Delete" icon just for budgets which can be deleted; however, the alignment of the table rows would suffer, making it harder to find the intended action. --- .../stylesheets/admin/budgets/actions.scss | 56 ++++++++++++++++--- app/components/admin/action_component.rb | 11 +++- .../admin/budgets/actions_component.html.erb | 16 +++--- .../admin/budgets/actions_component.rb | 50 ++++++++++++++++- ...alculate_winners_button_component.html.erb | 1 + .../budgets/table_actions_component.html.erb | 2 +- .../admin/budgets/table_actions_component.rb | 8 +++ app/controllers/admin/budgets_controller.rb | 4 +- config/locales/en/admin.yml | 5 ++ config/locales/es/admin.yml | 5 ++ .../components/admin/action_component_spec.rb | 25 +++++++++ .../budgets/table_actions_component_spec.rb | 5 +- .../admin/budgets_controller_spec.rb | 37 ++++++++++++ spec/system/admin/budgets_spec.rb | 38 ++----------- 14 files changed, 208 insertions(+), 55 deletions(-) diff --git a/app/assets/stylesheets/admin/budgets/actions.scss b/app/assets/stylesheets/admin/budgets/actions.scss index 8bebcf5b3..03b82fd6b 100644 --- a/app/assets/stylesheets/admin/budgets/actions.scss +++ b/app/assets/stylesheets/admin/budgets/actions.scss @@ -1,20 +1,62 @@ .admin .budgets-actions { - $gap: $line-height; - @include flex-with-gap($gap); - align-items: baseline; - flex-wrap: wrap; - margin-top: -$gap; - > * { - margin-top: $gap; + $gap: $line-height; + $vertical-gap: $line-height / 2; + @include flex-with-gap($gap); + align-items: center; + flex-wrap: wrap; + margin-top: -$vertical-gap; + + > * { + margin-top: $vertical-gap; + } + + dt { + flex-basis: 12em; + } + + &:only-child dt { + flex-basis: auto; + } + + dd { + flex-basis: 20em; + flex-grow: 1; + max-width: $global-width * 3 / 4; + } + + + * { + border-top: $admin-border; + clip-path: inset(0 0 0 $gap); + margin-top: $line-height * 1.5; + padding-top: $line-height * 1.5; + } } button { cursor: pointer; + width: 100%; } .calculate-winners-link { @include hollow-button; + } + + .destroy-link { + @include regular-button($alert-color); + + &:hover, + &:focus { + background-color: $color-alert; + } + + &[disabled] { + opacity: 0.4; + } + } + + .calculate-winners-link, + .destroy-link { margin-bottom: 0; } } diff --git a/app/components/admin/action_component.rb b/app/components/admin/action_component.rb index 77783528c..2235bb682 100644 --- a/app/components/admin/action_component.rb +++ b/app/components/admin/action_component.rb @@ -32,18 +32,27 @@ class Admin::ActionComponent < ApplicationComponent { class: html_class, id: (dom_id(record, action) if record.respond_to?(:to_key)), + "aria-describedby": describedby, "aria-label": label, data: { confirm: confirmation_text, disable_with: (text if button?) } - }.merge(options.except(:"aria-label", :class, :confirm, :path, :text)) + }.merge(options.except(:"aria-describedby", :"aria-label", :class, :confirm, :path, :text)) end def html_class "admin-action #{options[:class] || "#{action.to_s.gsub("_", "-")}-link"}".strip end + def describedby + if options[:"aria-describedby"] == true + "#{dom_id(record, action)}_descriptor" + else + options[:"aria-describedby"] + end + end + def label if options[:"aria-label"] == true t("admin.actions.label", action: text, name: record_name) diff --git a/app/components/admin/budgets/actions_component.html.erb b/app/components/admin/budgets/actions_component.html.erb index dab442b38..1068e9ac3 100644 --- a/app/components/admin/budgets/actions_component.html.erb +++ b/app/components/admin/budgets/actions_component.html.erb @@ -1,8 +1,8 @@ -
- <%= render Admin::Budgets::CalculateWinnersButtonComponent.new(budget) %> - - <%= action(:destroy, - text: t("admin.budgets.edit.delete"), - method: :delete, - class: "delete") %> -
+
+ <% actions.each do |action_name, button| %> +
+
<%= button[:html] %>
+
<%= sanitize(button[:hint]) %>
+
+ <% end %> +
diff --git a/app/components/admin/budgets/actions_component.rb b/app/components/admin/budgets/actions_component.rb index b16604f17..0728f4a93 100644 --- a/app/components/admin/budgets/actions_component.rb +++ b/app/components/admin/budgets/actions_component.rb @@ -8,6 +8,54 @@ class Admin::Budgets::ActionsComponent < ApplicationComponent private def action(action_name, **options) - render Admin::ActionComponent.new(action_name, budget, **options) + render Admin::ActionComponent.new( + action_name, + budget, + "aria-describedby": true, + **options + ) + end + + def actions + @actions ||= { + calculate_winners: { + hint: winners_hint, + html: winners_action + }, + destroy: { + hint: destroy_hint, + html: destroy_action + } + }.select { |_, button| button[:html].present? } + end + + def winners_action + render Admin::Budgets::CalculateWinnersButtonComponent.new(budget) + end + + def winners_hint + t("admin.budgets.actions.descriptions.calculate_winners", phase: t("budgets.phase.finished")) + end + + def destroy_action + action(:destroy, + text: t("admin.budgets.edit.delete"), + method: :delete, + confirm: t("admin.budgets.actions.confirm.destroy"), + disabled: budget.investments.any? || budget.poll) + end + + def destroy_hint + if budget.investments.any? + t("admin.budgets.destroy.unable_notice") + elsif budget.poll + t("admin.budgets.destroy.unable_notice_polls") + else + t("admin.budgets.actions.descriptions.destroy") + end + end + + def descriptor_id(action_name) + "#{dom_id(budget, action_name)}_descriptor" end end diff --git a/app/components/admin/budgets/calculate_winners_button_component.html.erb b/app/components/admin/budgets/calculate_winners_button_component.html.erb index 49448c00e..62b26541c 100644 --- a/app/components/admin/budgets/calculate_winners_button_component.html.erb +++ b/app/components/admin/budgets/calculate_winners_button_component.html.erb @@ -2,6 +2,7 @@ <%= render Admin::ActionComponent.new( :calculate_winners, budget, + "aria-describedby": !from_investments || nil, text: text, method: :put, class: html_class ) %> <% elsif from_investments %> diff --git a/app/components/admin/budgets/table_actions_component.html.erb b/app/components/admin/budgets/table_actions_component.html.erb index e17d988a4..8a250ecef 100644 --- a/app/components/admin/budgets/table_actions_component.html.erb +++ b/app/components/admin/budgets/table_actions_component.html.erb @@ -1,4 +1,4 @@ -<%= render Admin::TableActionsComponent.new(budget, edit_path: admin_budget_path(budget)) do |actions| %> +<%= render actions_component do |actions| %> <%= actions.action(:investments, text: t("admin.budgets.index.budget_investments"), path: admin_budget_budget_investments_path(budget_id: budget.id)) %> diff --git a/app/components/admin/budgets/table_actions_component.rb b/app/components/admin/budgets/table_actions_component.rb index 0e0dfde97..92277c152 100644 --- a/app/components/admin/budgets/table_actions_component.rb +++ b/app/components/admin/budgets/table_actions_component.rb @@ -17,4 +17,12 @@ class Admin::Budgets::TableActionsComponent < ApplicationComponent ends_at: balloting_phase.ends_at }) end + + def actions_component + Admin::TableActionsComponent.new( + budget, + edit_path: admin_budget_path(budget), + actions: [:edit] + ) + end end diff --git a/app/controllers/admin/budgets_controller.rb b/app/controllers/admin/budgets_controller.rb index 240051ecd..ab2050d46 100644 --- a/app/controllers/admin/budgets_controller.rb +++ b/app/controllers/admin/budgets_controller.rb @@ -43,9 +43,9 @@ class Admin::BudgetsController < Admin::BaseController def destroy if @budget.investments.any? - redirect_to admin_budgets_path, alert: t("admin.budgets.destroy.unable_notice") + redirect_to admin_budget_path(@budget), alert: t("admin.budgets.destroy.unable_notice") elsif @budget.poll.present? - redirect_to admin_budgets_path, alert: t("admin.budgets.destroy.unable_notice_polls") + redirect_to admin_budget_path(@budget), alert: t("admin.budgets.destroy.unable_notice_polls") else @budget.destroy! redirect_to admin_budgets_path, notice: t("admin.budgets.destroy.success_notice") diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 5a1a5e52d..c834727f9 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -69,6 +69,11 @@ en: no_activity: There are no moderators activity. budgets: actions: + confirm: + destroy: "Are you sure? This will delete the budget and all its associated groups and headings. This action cannot be undone." + descriptions: + calculate_winners: 'After calculating the results, only administrators will be able to access them. They will be public when the project reaches the "%{phase}" phase if the option "Show results" is enabled when editing the budget.' + destroy: "This will delete the budget and all its associated groups and headers. This action cannot be undone." edit: "Edit budget" preview: "Preview" preview_results: "Preview results" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index f4af55f64..d9122a158 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -69,6 +69,11 @@ es: no_activity: No hay actividad de moderadores. budgets: actions: + confirm: + destroy: "¿Seguro? Esta acción borrará el presupuesto y todos sus grupos y partidas. Esta acción no se puede deshacer." + descriptions: + calculate_winners: 'Tras calcular los resultados, solamente los administradores tendrán acceso a los mismos. Se harán públicos cuando el presupuesto llegue a la fase "%{phase}" si se habilita la opción "Mostrar resultados" al editar el presupuesto.' + destroy: "Esta acción borrará el presupuesto y todos sus grupos y partidas. Esta acción no se puede deshacer." edit: "Editar presupuesto" preview: "Previsualizar" preview_results: "Previsualizar resultados" diff --git a/spec/components/admin/action_component_spec.rb b/spec/components/admin/action_component_spec.rb index 5712b4479..068692471 100644 --- a/spec/components/admin/action_component_spec.rb +++ b/spec/components/admin/action_component_spec.rb @@ -41,6 +41,31 @@ describe Admin::ActionComponent do end end + describe "aria-describedby attribute" do + it "is not rendered by default" do + render_inline Admin::ActionComponent.new(:edit, double, path: "/") + + expect(page).to have_link count: 1 + expect(page).not_to have_css "[aria-describedby]" + end + + it "renders with the given value" do + render_inline Admin::ActionComponent.new(:edit, double, path: "/", "aria-describedby": "my_descriptor") + + expect(page).to have_link count: 1 + expect(page).to have_css "[aria-describedby='my_descriptor']" + end + + it "renders a default value when aria-describedby is true" do + record = double(model_name: double(param_key: "book"), to_key: [23]) + + render_inline Admin::ActionComponent.new(:edit, record, path: "/", "aria-describedby": true) + + expect(page).to have_link count: 1 + expect(page).to have_css "[aria-describedby='edit_book_23_descriptor']" + 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/budgets/table_actions_component_spec.rb b/spec/components/admin/budgets/table_actions_component_spec.rb index ac91530b5..7f320da9e 100644 --- a/spec/components/admin/budgets/table_actions_component_spec.rb +++ b/spec/components/admin/budgets/table_actions_component_spec.rb @@ -4,7 +4,7 @@ describe Admin::Budgets::TableActionsComponent, controller: Admin::BaseControlle let(:budget) { create(:budget) } let(:component) { Admin::Budgets::TableActionsComponent.new(budget) } - it "renders actions to edit and delete budget, manage investments and manage ballots" do + it "renders actions to edit budget, manage investments and manage ballots" do render_inline component expect(page).to have_link count: 3 @@ -12,8 +12,7 @@ describe Admin::Budgets::TableActionsComponent, controller: Admin::BaseControlle expect(page).to have_link "Edit", href: /#{budget.id}\Z/ expect(page).to have_link "Preview", href: /budgets/ - expect(page).to have_button count: 2 - expect(page).to have_css "form[action*='budgets']", exact_text: "Delete" + expect(page).to have_button count: 1 expect(page).to have_button "Ballots" end diff --git a/spec/controllers/admin/budgets_controller_spec.rb b/spec/controllers/admin/budgets_controller_spec.rb index cb31b0acf..7c881975b 100644 --- a/spec/controllers/admin/budgets_controller_spec.rb +++ b/spec/controllers/admin/budgets_controller_spec.rb @@ -22,4 +22,41 @@ describe Admin::BudgetsController, :admin do end.to raise_error ActiveRecord::RecordNotFound end end + + describe "DELETE destroy" do + let(:budget) { create(:budget) } + + it "allows destroying budgets without investments but with administrators and valuators" do + budget.administrators << Administrator.first + budget.valuators << create(:valuator) + + delete :destroy, params: { id: budget } + + expect(response).to redirect_to admin_budgets_path + expect(flash[:notice]).to eq "Budget deleted successfully" + expect(Budget.count).to eq 0 + expect(BudgetAdministrator.count).to eq 0 + expect(BudgetValuator.count).to eq 0 + end + + it "does not destroy budgets with investments" do + create(:budget_investment, budget: budget) + + delete :destroy, params: { id: budget } + + expect(response).to redirect_to admin_budget_path(budget) + expect(flash[:alert]).to eq "You cannot delete a budget that has associated investments" + expect(Budget.all).to eq [budget] + end + + it "does not destroy budgets with a poll" do + create(:poll, budget: budget) + + delete :destroy, params: { id: budget } + + expect(response).to redirect_to admin_budget_path(budget) + expect(flash[:alert]).to eq "You cannot delete a budget that has an associated poll" + expect(Budget.all).to eq [budget] + end + end end diff --git a/spec/system/admin/budgets_spec.rb b/spec/system/admin/budgets_spec.rb index fd37d0262..db39f419f 100644 --- a/spec/system/admin/budgets_spec.rb +++ b/spec/system/admin/budgets_spec.rb @@ -92,22 +92,6 @@ describe "Admin budgets", :admin do end end end - - scenario "Delete budget from index" do - create(:budget, name: "To be deleted") - - visit admin_budgets_path - - within "tr", text: "To be deleted" do - message = "Are you sure? This action will delete \"To be deleted\" and can't be undone." - - accept_confirm(message) { click_button "Delete" } - end - - expect(page).to have_content("Budget deleted successfully") - expect(page).to have_content("There are no budgets.") - expect(page).not_to have_content "To be deleted" - end end context "Publish" do @@ -141,18 +125,10 @@ describe "Admin budgets", :admin do scenario "Destroy a budget without investments" do visit admin_budget_path(budget) - click_button "Delete budget" - expect(page).to have_content("Budget deleted successfully") - expect(page).to have_content("There are no budgets.") - end + message = "Are you sure? This will delete the budget and all its associated groups and headings. This action cannot be undone." - scenario "Destroy a budget without investments but with administrators and valuators" do - budget.administrators << Administrator.first - budget.valuators << create(:valuator) - - visit admin_budget_path(budget) - click_button "Delete budget" + accept_confirm(message) { click_button "Delete budget" } expect(page).to have_content "Budget deleted successfully" expect(page).to have_content "There are no budgets." @@ -162,20 +138,18 @@ describe "Admin budgets", :admin do create(:budget_investment, heading: heading) visit admin_budget_path(budget) - click_button "Delete budget" - expect(page).to have_content("You cannot delete a budget that has associated investments") - expect(page).to have_content("There is 1 budget") + expect(page).to have_button "Delete budget", disabled: true + expect(page).to have_content "You cannot delete a budget that has associated investments" end scenario "Try to destroy a budget with polls" do create(:poll, budget: budget) visit admin_budget_path(budget) - click_button "Delete budget" - expect(page).to have_content("You cannot delete a budget that has an associated poll") - expect(page).to have_content("There is 1 budget") + expect(page).to have_button "Delete budget", disabled: true + expect(page).to have_content "You cannot delete a budget that has an associated poll" end end