From 349dbb74d7b8d1f5ac6c58ec5cd3fbd35a0287e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 23 Aug 2021 01:00:05 +0200 Subject: [PATCH] Move phases and actions out of the budget form Having links in the middle of a form distracts users from the task of filling in the form, and following a link before submitting the form will mean whatever has been filled in is lost. And the budgets form is already very long and hard to fill in. Having the phases table in the middle of it made it even harder. And, since we're planning to add the option to manage groups and headings from the same page, it's better to have a dedicated page for the form. --- .../stylesheets/admin/budgets/actions.scss | 25 +++--------- .../stylesheets/admin/budgets/links.scss | 40 +++++++++++++++++++ .../stylesheets/admin/budgets/show.scss | 12 ++++++ .../admin/budgets/actions_component.html.erb | 7 ---- .../calculate_winners_button_component.rb | 6 +-- .../admin/budgets/form_component.html.erb | 15 +++---- .../admin/budgets/links_component.html.erb | 11 +++++ .../admin/budgets/links_component.rb | 13 ++++++ .../admin/budgets/show_component.html.erb | 18 +++++++++ .../admin/budgets/show_component.rb | 14 +++++++ .../budgets/table_actions_component.html.erb | 2 +- .../admin/budget_phases_controller.rb | 2 +- app/controllers/admin/budgets_controller.rb | 5 +-- app/views/admin/budgets/edit.html.erb | 4 +- app/views/admin/budgets/show.html.erb | 1 + config/locales/en/admin.yml | 2 +- config/locales/es/admin.yml | 2 +- .../budgets/table_actions_component_spec.rb | 2 +- spec/system/admin/budget_investments_spec.rb | 5 ++- spec/system/admin/budget_phases_spec.rb | 4 +- spec/system/admin/budgets_spec.rb | 39 ++++++++++-------- .../admin/budgets_wizard/phases_spec.rb | 2 +- 22 files changed, 158 insertions(+), 73 deletions(-) create mode 100644 app/assets/stylesheets/admin/budgets/links.scss create mode 100644 app/assets/stylesheets/admin/budgets/show.scss create mode 100644 app/components/admin/budgets/links_component.html.erb create mode 100644 app/components/admin/budgets/links_component.rb create mode 100644 app/components/admin/budgets/show_component.html.erb create mode 100644 app/components/admin/budgets/show_component.rb create mode 100644 app/views/admin/budgets/show.html.erb diff --git a/app/assets/stylesheets/admin/budgets/actions.scss b/app/assets/stylesheets/admin/budgets/actions.scss index faa967c0e..8bebcf5b3 100644 --- a/app/assets/stylesheets/admin/budgets/actions.scss +++ b/app/assets/stylesheets/admin/budgets/actions.scss @@ -9,27 +9,12 @@ margin-top: $gap; } - + * { - margin-top: $line-height; - } - - a, button { - margin-bottom: 0; - } - - .preview-link { - @include has-fa-icon(eye, regular); - @include hollow-button; - margin-left: $gap; - margin-top: $gap; - - &::before { - margin-#{$global-right}: $font-icon-margin; - } - } - - .delete { cursor: pointer; } + + .calculate-winners-link { + @include hollow-button; + margin-bottom: 0; + } } diff --git a/app/assets/stylesheets/admin/budgets/links.scss b/app/assets/stylesheets/admin/budgets/links.scss new file mode 100644 index 000000000..28ba12b42 --- /dev/null +++ b/app/assets/stylesheets/admin/budgets/links.scss @@ -0,0 +1,40 @@ +.admin .budgets-links { + $gap: $line-height; + @include flex-with-gap($gap); + align-items: baseline; + flex-wrap: wrap; + margin-top: -$gap; + + > * { + margin-top: $gap; + } + + + * { + margin-top: $line-height; + } + + .preview-link { + @include has-fa-icon(eye, regular); + @include hollow-button; + } + + .edit-link { + @include has-fa-icon(edit, regular); + @include hollow-button; + } + + .results-link { + @include hollow-button; + } + + .edit-link, + .preview-link, + .results-link { + margin-left: $gap; + margin-top: $gap; + + &::before { + margin-#{$global-right}: $font-icon-margin; + } + } +} diff --git a/app/assets/stylesheets/admin/budgets/show.scss b/app/assets/stylesheets/admin/budgets/show.scss new file mode 100644 index 000000000..1ae4e8413 --- /dev/null +++ b/app/assets/stylesheets/admin/budgets/show.scss @@ -0,0 +1,12 @@ +.admin-budgets-show { + + h3 { + margin-top: $line-height * 1.5; + + @each $size, $headers in $header-styles { + @include breakpoint($size) { + font-size: (rem-calc(map-get(map-get($headers, h2), font-size)) + rem-calc(map-get(map-get($headers, h3), font-size))) / 2; + } + } + } +} diff --git a/app/components/admin/budgets/actions_component.html.erb b/app/components/admin/budgets/actions_component.html.erb index b57d22a67..dab442b38 100644 --- a/app/components/admin/budgets/actions_component.html.erb +++ b/app/components/admin/budgets/actions_component.html.erb @@ -1,12 +1,5 @@
<%= render Admin::Budgets::CalculateWinnersButtonComponent.new(budget) %> - <% if budget.has_winning_investments? %> - <%= link_to t("budgets.show.see_results"), - budget_results_path(budget), - class: "button hollow" %> - <% end %> - - <%= link_to t("admin.budgets.actions.preview"), budget_path(budget), class: "preview-link", target: "_blank" %> <%= action(:destroy, text: t("admin.budgets.edit.delete"), diff --git a/app/components/admin/budgets/calculate_winners_button_component.rb b/app/components/admin/budgets/calculate_winners_button_component.rb index 92eb9a8e6..2aa1ff012 100644 --- a/app/components/admin/budgets/calculate_winners_button_component.rb +++ b/app/components/admin/budgets/calculate_winners_button_component.rb @@ -21,10 +21,6 @@ class Admin::Budgets::CalculateWinnersButtonComponent < ApplicationComponent end def html_class - if from_investments - "button hollow float-right clear" - else - "button hollow" - end + "button hollow float-right clear" if from_investments end end diff --git a/app/components/admin/budgets/form_component.html.erb b/app/components/admin/budgets/form_component.html.erb index fd2026da0..5f9331958 100644 --- a/app/components/admin/budgets/form_component.html.erb +++ b/app/components/admin/budgets/form_component.html.erb @@ -39,6 +39,12 @@
+ <% unless wizard? %> +
+ <%= f.select :phase, phases_select_options %> +
+ <% end %> + <% if feature?(:allow_images) %>
<%= render "/images/nested_image", f: f %> @@ -64,15 +70,6 @@ <% unless wizard? %> -
- <%= t("admin.budgets.edit.info.phases_settings") %> -
- <%= f.select :phase, phases_select_options %> -
- - <%= render Admin::BudgetPhases::PhasesComponent.new(budget) %> -
- <%= render "admin/shared/show_results_fields", form: f %> <% end %> diff --git a/app/components/admin/budgets/links_component.html.erb b/app/components/admin/budgets/links_component.html.erb new file mode 100644 index 000000000..bc6799b80 --- /dev/null +++ b/app/components/admin/budgets/links_component.html.erb @@ -0,0 +1,11 @@ +
+ <% if budget.has_winning_investments? %> + <%= action(:results, + text: t("budgets.show.see_results"), + path: budget_results_path(budget)) %> + <% end %> + + <%= action(:preview, text: t("admin.budgets.actions.preview"), path: budget_path(budget), target: "_blank") %> + + <%= action(:edit, text: t("admin.budgets.actions.edit")) %> +
diff --git a/app/components/admin/budgets/links_component.rb b/app/components/admin/budgets/links_component.rb new file mode 100644 index 000000000..ffec46c93 --- /dev/null +++ b/app/components/admin/budgets/links_component.rb @@ -0,0 +1,13 @@ +class Admin::Budgets::LinksComponent < ApplicationComponent + attr_reader :budget + + def initialize(budget) + @budget = budget + end + + private + + def action(action_name, **options) + render Admin::ActionComponent.new(action_name, budget, **options) + end +end diff --git a/app/components/admin/budgets/show_component.html.erb b/app/components/admin/budgets/show_component.html.erb new file mode 100644 index 000000000..39cda6458 --- /dev/null +++ b/app/components/admin/budgets/show_component.html.erb @@ -0,0 +1,18 @@ +
+ <%= back_link_to admin_budgets_path %> + + <%= header %> + + <%= render Admin::Budgets::DraftingComponent.new(budget) %> + <%= render Admin::Budgets::LinksComponent.new(budget) %> + +
+

<%= t("admin.budgets.edit.phases_caption") %>

+ <%= render Admin::BudgetPhases::PhasesComponent.new(budget) %> +
+ +
+

<%= t("admin.budgets.edit.actions") %>

+ <%= render Admin::Budgets::ActionsComponent.new(budget) %> +
+
diff --git a/app/components/admin/budgets/show_component.rb b/app/components/admin/budgets/show_component.rb new file mode 100644 index 000000000..f8d6e98b1 --- /dev/null +++ b/app/components/admin/budgets/show_component.rb @@ -0,0 +1,14 @@ +class Admin::Budgets::ShowComponent < ApplicationComponent + include Header + attr_reader :budget + + def initialize(budget) + @budget = budget + end + + private + + def title + budget.name + end +end diff --git a/app/components/admin/budgets/table_actions_component.html.erb b/app/components/admin/budgets/table_actions_component.html.erb index 1fa38d32f..be0abfa07 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) do |actions| %> +<%= render Admin::TableActionsComponent.new(budget, edit_path: admin_budget_path(budget)) 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/controllers/admin/budget_phases_controller.rb b/app/controllers/admin/budget_phases_controller.rb index ac2a763e1..ad2bd8146 100644 --- a/app/controllers/admin/budget_phases_controller.rb +++ b/app/controllers/admin/budget_phases_controller.rb @@ -4,6 +4,6 @@ class Admin::BudgetPhasesController < Admin::BaseController private def phases_index - edit_admin_budget_path(@phase.budget) + admin_budget_path(@phase.budget) end end diff --git a/app/controllers/admin/budgets_controller.rb b/app/controllers/admin/budgets_controller.rb index f0840123c..96380e78e 100644 --- a/app/controllers/admin/budgets_controller.rb +++ b/app/controllers/admin/budgets_controller.rb @@ -15,7 +15,6 @@ class Admin::BudgetsController < Admin::BaseController end def show - render :edit end def edit @@ -23,7 +22,7 @@ class Admin::BudgetsController < Admin::BaseController def publish @budget.publish! - redirect_to edit_admin_budget_path(@budget), notice: t("admin.budgets.publish.notice") + redirect_to admin_budget_path(@budget), notice: t("admin.budgets.publish.notice") end def calculate_winners @@ -38,7 +37,7 @@ class Admin::BudgetsController < Admin::BaseController def update if @budget.update(budget_params) - redirect_to admin_budgets_path, notice: t("admin.budgets.update.notice") + redirect_to admin_budget_path(@budget), notice: t("admin.budgets.update.notice") else render :edit end diff --git a/app/views/admin/budgets/edit.html.erb b/app/views/admin/budgets/edit.html.erb index 07247f80d..15849aa95 100644 --- a/app/views/admin/budgets/edit.html.erb +++ b/app/views/admin/budgets/edit.html.erb @@ -1,9 +1,7 @@ -<%= back_link_to admin_budgets_path %> +<%= back_link_to admin_budget_path(@budget) %>

<%= t("admin.budgets.edit.title") %>

- <%= render Admin::Budgets::DraftingComponent.new(@budget) %>
-<%= render Admin::Budgets::ActionsComponent.new(@budget) %> <%= render Admin::Budgets::FormComponent.new(@budget) %> diff --git a/app/views/admin/budgets/show.html.erb b/app/views/admin/budgets/show.html.erb new file mode 100644 index 000000000..2f3198f04 --- /dev/null +++ b/app/views/admin/budgets/show.html.erb @@ -0,0 +1 @@ +<%= render Admin::Budgets::ShowComponent.new(@budget) %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 0749e67c6..7dc2270f5 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -69,6 +69,7 @@ en: no_activity: There are no moderators activity. budgets: actions: + edit: "Edit budget" preview: "Preview" index: title: Participatory budgets @@ -127,7 +128,6 @@ en: main_call_to_action_description: "This link will appear on main banner of this participatory budget and encourages your user to perform a specific action like creating a proposal, voting for existing ones, or learn more about the process." info: budget_settings: "General participatory budget settings" - phases_settings: "Phases settings" staff_settings: "Administrators and Valuators assigned to the budget" destroy: success_notice: Budget deleted successfully diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 5ffce5ea8..bc574ed5b 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -69,6 +69,7 @@ es: no_activity: No hay actividad de moderadores. budgets: actions: + edit: "Editar presupuesto" preview: "Previsualizar" index: title: Presupuestos participativos @@ -127,7 +128,6 @@ es: main_call_to_action_description: "Este enlace aparecerá en la cabecera de este presupuesto participativo y permite al usuario ejecutar una acción específica como crear una nueva propuesta, votar las existentes, o leer más sobre el funcionamiento de los presupuestos participativos." info: budget_settings: "Configuración genérica del presupuesto participativo" - phases_settings: "Configuración de las fases" staff_settings: "Administradores y Evaluadores asigandos al presupuesto" destroy: success_notice: Presupuesto eliminado correctamente diff --git a/spec/components/admin/budgets/table_actions_component_spec.rb b/spec/components/admin/budgets/table_actions_component_spec.rb index 6a2206de2..86391bff5 100644 --- a/spec/components/admin/budgets/table_actions_component_spec.rb +++ b/spec/components/admin/budgets/table_actions_component_spec.rb @@ -10,7 +10,7 @@ describe Admin::Budgets::TableActionsComponent, controller: Admin::BaseControlle expect(page).to have_link count: 4 expect(page).to have_link "Investment projects", href: /investments/ expect(page).to have_link "Heading groups", href: /groups/ - expect(page).to have_link "Edit", href: /edit/ + 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 diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 23753c0a7..f9e9c67c8 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -498,10 +498,11 @@ describe "Admin budget investments", :admin do expect(page).to have_button "Calculate Winner Investments" - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) expect(page).to have_button "Calculate Winner Investments" + click_link "Edit budget" select "Accepting projects", from: "Active phase" click_button "Update Budget" @@ -518,7 +519,7 @@ describe "Admin budget investments", :admin do '"Reviewing Ballots" or "Finished budget" in order '\ "to calculate winners projects" - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) expect(page).not_to have_button "Calculate Winner Investments" end diff --git a/spec/system/admin/budget_phases_spec.rb b/spec/system/admin/budget_phases_spec.rb index 5893a8aea..0d07bcc2f 100644 --- a/spec/system/admin/budget_phases_spec.rb +++ b/spec/system/admin/budget_phases_spec.rb @@ -13,7 +13,7 @@ describe "Admin budget phases" do uncheck "budget_phase_enabled" click_button "Save changes" - expect(page).to have_current_path(edit_admin_budget_path(budget)) + expect(page).to have_current_path(admin_budget_path(budget)) expect(page).to have_content "Changes saved" expect(budget.current_phase.starts_at.to_date).to eq((Date.current + 1.day).to_date) @@ -23,7 +23,7 @@ describe "Admin budget phases" do end scenario "Show default phase name or custom if present" do - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) within_table "Phases" do expect(page).to have_content "Accepting projects" diff --git a/spec/system/admin/budgets_spec.rb b/spec/system/admin/budgets_spec.rb index d9dd1d25e..6262c5406 100644 --- a/spec/system/admin/budgets_spec.rb +++ b/spec/system/admin/budgets_spec.rb @@ -114,7 +114,7 @@ describe "Admin budgets", :admin do let(:budget) { create(:budget, :drafting) } scenario "Can preview budget before it is published" do - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) within_window(window_opened_by { click_link "Preview" }) do expect(page).to have_current_path budget_path(budget) @@ -122,7 +122,7 @@ describe "Admin budgets", :admin do end scenario "Can preview a budget after it is published" do - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) accept_confirm { click_button "Publish budget" } @@ -141,7 +141,7 @@ describe "Admin budgets", :admin do let(:heading) { create(:budget_heading, budget: budget) } scenario "Destroy a budget without investments" do - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) click_button "Delete budget" expect(page).to have_content("Budget deleted successfully") @@ -152,7 +152,7 @@ describe "Admin budgets", :admin do budget.administrators << Administrator.first budget.valuators << create(:valuator) - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) click_button "Delete budget" expect(page).to have_content "Budget deleted successfully" @@ -162,7 +162,7 @@ describe "Admin budgets", :admin do scenario "Try to destroy a budget with investments" do create(:budget_investment, heading: heading) - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) click_button "Delete budget" expect(page).to have_content("You cannot delete a budget that has associated investments") @@ -172,7 +172,7 @@ describe "Admin budgets", :admin do scenario "Try to destroy a budget with polls" do create(:poll, budget: budget) - visit edit_admin_budget_path(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") @@ -188,9 +188,7 @@ describe "Admin budgets", :admin do budget.update!(phase: "selecting") budget.phases.valuating.update!(enabled: false) - visit edit_admin_budget_path(budget) - - expect(page).to have_select "Active phase", selected: "Selecting projects" + visit admin_budget_path(budget) expect(page).to have_table "Phases", with_cols: [ [ @@ -230,6 +228,10 @@ describe "Admin budgets", :admin do expect(page).to have_link "Edit" end end + + click_link "Edit budget" + + expect(page).to have_select "Active phase", selected: "Selecting projects" end end @@ -289,13 +291,16 @@ describe "Admin budgets", :admin do context "Update" do scenario "Update budget" do - visit edit_admin_budget_path(create(:budget)) + budget = create(:budget) + + visit edit_admin_budget_path(budget) fill_in "Name", with: "More trees on the streets" click_button "Update Budget" + expect(page).to have_content "Participatory budget updated successfully" expect(page).to have_content("More trees on the streets") - expect(page).to have_current_path(admin_budgets_path) + expect(page).to have_current_path(admin_budget_path(budget)) end scenario "Deselect all selected staff" do @@ -333,7 +338,8 @@ describe "Admin budgets", :admin do ballot_lines_count: 2) selected = create(:budget_investment, :selected, heading: heading, price: 2, ballot_lines_count: 1) - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) + expect(page).not_to have_content "See results" click_button "Calculate Winner Investments" @@ -343,15 +349,16 @@ describe "Admin budgets", :admin do expect(page).not_to have_content unselected.title expect(page).not_to have_content selected.title - visit edit_admin_budget_path(budget) - expect(page).to have_content "See results" + visit admin_budget_path(budget) + + expect(page).to have_link "See results" end scenario "For a finished Budget" do budget = create(:budget, :finished) allow_any_instance_of(Budget).to receive(:has_winning_investments?).and_return(true) - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) expect(page).to have_content "Calculate Winner Investments" expect(page).to have_content "See results" @@ -361,7 +368,7 @@ describe "Admin budgets", :admin do budget = create(:budget, :finished) create(:budget_investment, :winner, budget: budget) - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) expect(page).to have_content "Recalculate Winner Investments" expect(page).to have_content "See results" diff --git a/spec/system/admin/budgets_wizard/phases_spec.rb b/spec/system/admin/budgets_wizard/phases_spec.rb index 72edc9e6c..3b692ad32 100644 --- a/spec/system/admin/budgets_wizard/phases_spec.rb +++ b/spec/system/admin/budgets_wizard/phases_spec.rb @@ -30,7 +30,7 @@ describe "Budgets wizard, phases step", :admin do expect(page).to have_content "Phases configured successfully" - visit edit_admin_budget_path(budget) + visit admin_budget_path(budget) within "tr", text: "Information" do expect(page).to have_css ".budget-phase-disabled", visible: :all