From 3cebce9a292678f617d93aa5b126076f8b996f34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 01:50:15 +0200 Subject: [PATCH 01/28] Extract link to toggle selection to a component --- .../toggle_selection_component.html.erb | 1 + .../proposals/toggle_selection_component.rb | 26 +++++++++++++++++++ app/helpers/proposals_helper.rb | 19 -------------- .../legislation/proposals/_proposals.html.erb | 2 +- .../proposals/_select_proposal.html.erb | 1 - .../proposals/toggle_selection.js.erb | 4 ++- .../admin/proposals/_select_proposal.html.erb | 1 - app/views/admin/proposals/index.html.erb | 2 +- .../admin/proposals/toggle_selection.js.erb | 4 ++- 9 files changed, 35 insertions(+), 25 deletions(-) create mode 100644 app/components/admin/proposals/toggle_selection_component.html.erb create mode 100644 app/components/admin/proposals/toggle_selection_component.rb delete mode 100644 app/views/admin/legislation/proposals/_select_proposal.html.erb delete mode 100644 app/views/admin/proposals/_select_proposal.html.erb diff --git a/app/components/admin/proposals/toggle_selection_component.html.erb b/app/components/admin/proposals/toggle_selection_component.html.erb new file mode 100644 index 000000000..a63c1b4a9 --- /dev/null +++ b/app/components/admin/proposals/toggle_selection_component.html.erb @@ -0,0 +1 @@ +<%= link_to_toggle_proposal_selection %> diff --git a/app/components/admin/proposals/toggle_selection_component.rb b/app/components/admin/proposals/toggle_selection_component.rb new file mode 100644 index 000000000..c7d3cd117 --- /dev/null +++ b/app/components/admin/proposals/toggle_selection_component.rb @@ -0,0 +1,26 @@ +class Admin::Proposals::ToggleSelectionComponent < ApplicationComponent + attr_reader :proposal + + def initialize(proposal) + @proposal = proposal + end + + def link_to_toggle_proposal_selection + if proposal.selected? + button_text = t("admin.proposals.index.selected") + html_class = "button expanded" + else + button_text = t("admin.proposals.index.select") + html_class = "button hollow expanded" + end + + case proposal.class.to_s + when "Proposal" + path = toggle_selection_admin_proposal_path(proposal) + when "Legislation::Proposal" + path = toggle_selection_admin_legislation_process_proposal_path(proposal.process, proposal) + end + + link_to button_text, path, remote: true, method: :patch, class: html_class + end +end diff --git a/app/helpers/proposals_helper.rb b/app/helpers/proposals_helper.rb index f2b82e8dc..a497c7037 100644 --- a/app/helpers/proposals_helper.rb +++ b/app/helpers/proposals_helper.rb @@ -64,25 +64,6 @@ module ProposalsHelper proposals_current_view == "default" ? "minimal" : "default" end - def link_to_toggle_proposal_selection(proposal) - if proposal.selected? - button_text = t("admin.proposals.index.selected") - html_class = "button expanded" - else - button_text = t("admin.proposals.index.select") - html_class = "button hollow expanded" - end - - case proposal.class.to_s - when "Proposal" - path = toggle_selection_admin_proposal_path(proposal) - when "Legislation::Proposal" - path = toggle_selection_admin_legislation_process_proposal_path(proposal.process, proposal) - end - - link_to button_text, path, remote: true, method: :patch, class: html_class - end - def css_for_proposal_info_row(proposal) if proposal.image.present? if params[:selected].present? diff --git a/app/views/admin/legislation/proposals/_proposals.html.erb b/app/views/admin/legislation/proposals/_proposals.html.erb index 264f92cda..c63bdf977 100644 --- a/app/views/admin/legislation/proposals/_proposals.html.erb +++ b/app/views/admin/legislation/proposals/_proposals.html.erb @@ -19,7 +19,7 @@ <%= proposal.id %> <%= proposal.title %> <%= proposal.votes_score %> - <%= render "select_proposal", proposal: proposal %> + <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> <% end %> diff --git a/app/views/admin/legislation/proposals/_select_proposal.html.erb b/app/views/admin/legislation/proposals/_select_proposal.html.erb deleted file mode 100644 index d5d2069ab..000000000 --- a/app/views/admin/legislation/proposals/_select_proposal.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= link_to_toggle_proposal_selection(proposal) %> diff --git a/app/views/admin/legislation/proposals/toggle_selection.js.erb b/app/views/admin/legislation/proposals/toggle_selection.js.erb index d38292e9d..8b0507552 100644 --- a/app/views/admin/legislation/proposals/toggle_selection.js.erb +++ b/app/views/admin/legislation/proposals/toggle_selection.js.erb @@ -1 +1,3 @@ -$("#<%= dom_id(@proposal) %> .select").html("<%= j render("select_proposal", proposal: @proposal) %>"); +$("#<%= dom_id(@proposal) %> .select").html( + "<%= j render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %>" +); diff --git a/app/views/admin/proposals/_select_proposal.html.erb b/app/views/admin/proposals/_select_proposal.html.erb deleted file mode 100644 index d5d2069ab..000000000 --- a/app/views/admin/proposals/_select_proposal.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= link_to_toggle_proposal_selection(proposal) %> diff --git a/app/views/admin/proposals/index.html.erb b/app/views/admin/proposals/index.html.erb index 5cad75632..23f5388f4 100644 --- a/app/views/admin/proposals/index.html.erb +++ b/app/views/admin/proposals/index.html.erb @@ -27,7 +27,7 @@ <%= link_to proposal.title, admin_proposal_path(proposal) %> <%= proposal.author.username %> <%= proposal.milestones.count %> - <%= render "select_proposal", proposal: proposal %> + <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> <% end %> diff --git a/app/views/admin/proposals/toggle_selection.js.erb b/app/views/admin/proposals/toggle_selection.js.erb index 0f7f171ba..bb832e071 100644 --- a/app/views/admin/proposals/toggle_selection.js.erb +++ b/app/views/admin/proposals/toggle_selection.js.erb @@ -1 +1,3 @@ -$("#<%= dom_id(@proposal) %> .js-select").html("<%= j render("select_proposal", proposal: @proposal) %>"); +$("#<%= dom_id(@proposal) %> .js-select").html( + "<%= j render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %>" +); From 3febe3f0ccde9c4f78f7731b3b5661f60e52d90c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 02:08:25 +0200 Subject: [PATCH 02/28] Extract methods in toggle selection component --- .../toggle_selection_component.html.erb | 2 +- .../proposals/toggle_selection_component.rb | 40 ++++++++++++------- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/app/components/admin/proposals/toggle_selection_component.html.erb b/app/components/admin/proposals/toggle_selection_component.html.erb index a63c1b4a9..99bf73538 100644 --- a/app/components/admin/proposals/toggle_selection_component.html.erb +++ b/app/components/admin/proposals/toggle_selection_component.html.erb @@ -1 +1 @@ -<%= link_to_toggle_proposal_selection %> +<%= link_to text, path, options %> diff --git a/app/components/admin/proposals/toggle_selection_component.rb b/app/components/admin/proposals/toggle_selection_component.rb index c7d3cd117..4bf670851 100644 --- a/app/components/admin/proposals/toggle_selection_component.rb +++ b/app/components/admin/proposals/toggle_selection_component.rb @@ -5,22 +5,34 @@ class Admin::Proposals::ToggleSelectionComponent < ApplicationComponent @proposal = proposal end - def link_to_toggle_proposal_selection - if proposal.selected? - button_text = t("admin.proposals.index.selected") - html_class = "button expanded" - else - button_text = t("admin.proposals.index.select") - html_class = "button hollow expanded" + private + + def text + if proposal.selected? + t("admin.proposals.index.selected") + else + t("admin.proposals.index.select") + end end - case proposal.class.to_s - when "Proposal" - path = toggle_selection_admin_proposal_path(proposal) - when "Legislation::Proposal" - path = toggle_selection_admin_legislation_process_proposal_path(proposal.process, proposal) + def path + case proposal.class.to_s + when "Proposal" + toggle_selection_admin_proposal_path(proposal) + when "Legislation::Proposal" + toggle_selection_admin_legislation_process_proposal_path(proposal.process, proposal) + end end - link_to button_text, path, remote: true, method: :patch, class: html_class - end + def options + { remote: true, method: :patch, class: html_class } + end + + def html_class + if proposal.selected? + "button expanded" + else + "button hollow expanded" + end + end end From 43b6ab00e3b4f12576f7c15b04db12d934a9c145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 02:26:20 +0200 Subject: [PATCH 03/28] Simplify method to get the toggle selection path --- .../admin/proposals/toggle_selection_component.rb | 7 +------ config/initializers/routes_hierarchy.rb | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/app/components/admin/proposals/toggle_selection_component.rb b/app/components/admin/proposals/toggle_selection_component.rb index 4bf670851..c046e64db 100644 --- a/app/components/admin/proposals/toggle_selection_component.rb +++ b/app/components/admin/proposals/toggle_selection_component.rb @@ -16,12 +16,7 @@ class Admin::Proposals::ToggleSelectionComponent < ApplicationComponent end def path - case proposal.class.to_s - when "Proposal" - toggle_selection_admin_proposal_path(proposal) - when "Legislation::Proposal" - toggle_selection_admin_legislation_process_proposal_path(proposal.process, proposal) - end + admin_polymorphic_path(proposal, action: :toggle_selection) end def options diff --git a/config/initializers/routes_hierarchy.rb b/config/initializers/routes_hierarchy.rb index fc9eac7f0..9f15c4587 100644 --- a/config/initializers/routes_hierarchy.rb +++ b/config/initializers/routes_hierarchy.rb @@ -24,7 +24,7 @@ module ActionDispatch::Routing::UrlFor end def namespaced_polymorphic_path(namespace, resource, options = {}) - if %w[Budget::Group Budget::Heading Legislation::DraftVersion Legislation::Question + if %w[Budget::Group Budget::Heading Legislation::DraftVersion Legislation::Proposal Legislation::Question Poll::Booth Poll::BoothAssignment Poll::Officer Poll::Question Poll::Question::Option Poll::Question::Option::Video Poll::Shift SDG::LocalTarget].include?(resource.class.name) resolve = resolve_for(resource) From b127bd2f51e614a1570ffce25fc6cef120167561 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 02:33:05 +0200 Subject: [PATCH 04/28] Use a button to toggle proposal selection As mentioned in commit 5311daadf, using buttons for non-GET requests has several advantages over using links. --- .../proposals/toggle_selection_component.html.erb | 2 +- spec/system/admin/legislation/proposals_spec.rb | 2 +- spec/system/admin/proposals_spec.rb | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/components/admin/proposals/toggle_selection_component.html.erb b/app/components/admin/proposals/toggle_selection_component.html.erb index 99bf73538..e9e6cb5d5 100644 --- a/app/components/admin/proposals/toggle_selection_component.html.erb +++ b/app/components/admin/proposals/toggle_selection_component.html.erb @@ -1 +1 @@ -<%= link_to text, path, options %> +<%= button_to text, path, options %> diff --git a/spec/system/admin/legislation/proposals_spec.rb b/spec/system/admin/legislation/proposals_spec.rb index 8653594f2..5ad426356 100644 --- a/spec/system/admin/legislation/proposals_spec.rb +++ b/spec/system/admin/legislation/proposals_spec.rb @@ -19,7 +19,7 @@ describe "Admin collaborative legislation", :admin do proposal = create(:legislation_proposal, cached_votes_score: 10) visit admin_legislation_process_proposals_path(proposal.legislation_process_id) - click_link "Select" + click_button "Select" within "#legislation_proposal_#{proposal.id}" do expect(page).to have_content("Selected") diff --git a/spec/system/admin/proposals_spec.rb b/spec/system/admin/proposals_spec.rb index 473470c67..fe062220f 100644 --- a/spec/system/admin/proposals_spec.rb +++ b/spec/system/admin/proposals_spec.rb @@ -26,13 +26,13 @@ describe "Admin proposals", :admin do visit admin_proposals_path - within("#proposal_#{proposal.id}") { click_link "Select" } + within("#proposal_#{proposal.id}") { click_button "Select" } - within("#proposal_#{proposal.id}") { expect(page).to have_link "Selected" } + within("#proposal_#{proposal.id}") { expect(page).to have_button "Selected" } refresh - within("#proposal_#{proposal.id}") { expect(page).to have_link "Selected" } + within("#proposal_#{proposal.id}") { expect(page).to have_button "Selected" } end scenario "Unselect a proposal" do @@ -40,13 +40,13 @@ describe "Admin proposals", :admin do visit admin_proposals_path - within("#proposal_#{proposal.id}") { click_link "Selected" } + within("#proposal_#{proposal.id}") { click_button "Selected" } - within("#proposal_#{proposal.id}") { expect(page).to have_link "Select" } + within("#proposal_#{proposal.id}") { expect(page).to have_button "Select" } refresh - within("#proposal_#{proposal.id}") { expect(page).to have_link "Select" } + within("#proposal_#{proposal.id}") { expect(page).to have_button "Select" } end end From fec44c146cd4eef37b9f713a688f1d8ebe405b41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 02:58:10 +0200 Subject: [PATCH 05/28] Use a switch to toggle proposal selection The button to select/deselect a proposal wasn't very intuitive; for example, it wasn't obvious that pressing a button saying "selected" would deselect the proposal. So we're using a switch control, like we do to enable/disable features since commit fabe97e50. --- .../toggle_selection_component.html.erb | 2 +- .../proposals/toggle_selection_component.rb | 24 +++++++------------ config/locales/en/admin.yml | 2 +- config/locales/es/admin.yml | 2 +- .../toggle_selection_component_spec.rb | 21 ++++++++++++++++ .../admin/legislation/proposals_spec.rb | 8 +++---- spec/system/admin/proposals_spec.rb | 24 ++++++++++++------- 7 files changed, 53 insertions(+), 30 deletions(-) create mode 100644 spec/components/admin/proposals/toggle_selection_component_spec.rb diff --git a/app/components/admin/proposals/toggle_selection_component.html.erb b/app/components/admin/proposals/toggle_selection_component.html.erb index e9e6cb5d5..bdb2aaa99 100644 --- a/app/components/admin/proposals/toggle_selection_component.html.erb +++ b/app/components/admin/proposals/toggle_selection_component.html.erb @@ -1 +1 @@ -<%= button_to text, path, options %> +<%= render Admin::ToggleSwitchComponent.new(action, proposal, pressed: selected?, **options) %> diff --git a/app/components/admin/proposals/toggle_selection_component.rb b/app/components/admin/proposals/toggle_selection_component.rb index c046e64db..97a48803b 100644 --- a/app/components/admin/proposals/toggle_selection_component.rb +++ b/app/components/admin/proposals/toggle_selection_component.rb @@ -7,27 +7,21 @@ class Admin::Proposals::ToggleSelectionComponent < ApplicationComponent private - def text - if proposal.selected? - t("admin.proposals.index.selected") - else - t("admin.proposals.index.select") - end + def action + :toggle_selection end - def path - admin_polymorphic_path(proposal, action: :toggle_selection) + def selected? + proposal.selected? end def options - { remote: true, method: :patch, class: html_class } + { + "aria-label": label + } end - def html_class - if proposal.selected? - "button expanded" - else - "button hollow expanded" - end + def label + t("admin.actions.label", action: t("admin.actions.select"), name: proposal.title) end end diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 3b70a864d..589127371 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -16,6 +16,7 @@ en: unmark_featured: Unmark featured edit: Edit configure: Configure + select: Select 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: @@ -1294,7 +1295,6 @@ en: title: Proposals id: ID author: Author - select: Select selected: Selected milestones: Milestones no_proposals: There are no proposals. diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 513ae0306..510753097 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -16,6 +16,7 @@ es: unmark_featured: Quitar destacado edit: Editar configure: Configurar + select: Seleccionar 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: @@ -1295,7 +1296,6 @@ es: id: ID author: Autor milestones: Hitos - select: Seleccionar selected: Seleccionada no_proposals: No hay propuestas. show: diff --git a/spec/components/admin/proposals/toggle_selection_component_spec.rb b/spec/components/admin/proposals/toggle_selection_component_spec.rb new file mode 100644 index 000000000..2350c92f8 --- /dev/null +++ b/spec/components/admin/proposals/toggle_selection_component_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + +describe Admin::Proposals::ToggleSelectionComponent, :admin do + describe "aria-pressed attribute" do + it "is true for selected proposals" do + proposal = create(:proposal, :selected) + + render_inline Admin::Proposals::ToggleSelectionComponent.new(proposal) + + expect(page).to have_css "button[aria-pressed='true']" + end + + it "is false for not selected proposals" do + proposal = create(:proposal) + + render_inline Admin::Proposals::ToggleSelectionComponent.new(proposal) + + expect(page).to have_css "button[aria-pressed='false']" + end + end +end diff --git a/spec/system/admin/legislation/proposals_spec.rb b/spec/system/admin/legislation/proposals_spec.rb index 5ad426356..0b2a1d6eb 100644 --- a/spec/system/admin/legislation/proposals_spec.rb +++ b/spec/system/admin/legislation/proposals_spec.rb @@ -11,18 +11,18 @@ describe "Admin collaborative legislation", :admin do expect(page).to have_content(proposal.title) expect(page).to have_content(proposal.id) expect(page).to have_content(proposal.cached_votes_score) - expect(page).to have_content("Select") + expect(page).to have_content("No") end end scenario "Selecting legislation proposals" do - proposal = create(:legislation_proposal, cached_votes_score: 10) + proposal = create(:legislation_proposal, title: "Add more accessibility tests") visit admin_legislation_process_proposals_path(proposal.legislation_process_id) - click_button "Select" + click_button "Select Add more accessibility tests" within "#legislation_proposal_#{proposal.id}" do - expect(page).to have_content("Selected") + expect(page).to have_content "Yes" end end diff --git a/spec/system/admin/proposals_spec.rb b/spec/system/admin/proposals_spec.rb index fe062220f..d0377a70b 100644 --- a/spec/system/admin/proposals_spec.rb +++ b/spec/system/admin/proposals_spec.rb @@ -22,31 +22,39 @@ describe "Admin proposals", :admin do end scenario "Select a proposal" do - proposal = create(:proposal) + proposal = create(:proposal, title: "Forbid door-to-door sales") visit admin_proposals_path - within("#proposal_#{proposal.id}") { click_button "Select" } + within("#proposal_#{proposal.id}") do + expect(page).to have_content "No" - within("#proposal_#{proposal.id}") { expect(page).to have_button "Selected" } + click_button "Select Forbid door-to-door sales" + + expect(page).to have_content "Yes" + end refresh - within("#proposal_#{proposal.id}") { expect(page).to have_button "Selected" } + within("#proposal_#{proposal.id}") { expect(page).to have_content "Yes" } end scenario "Unselect a proposal" do - proposal = create(:proposal, :selected) + proposal = create(:proposal, :selected, title: "Allow door-to-door sales") visit admin_proposals_path - within("#proposal_#{proposal.id}") { click_button "Selected" } + within("#proposal_#{proposal.id}") do + expect(page).to have_content "Yes" - within("#proposal_#{proposal.id}") { expect(page).to have_button "Select" } + click_button "Select Allow door-to-door sales" + + expect(page).to have_content "No" + end refresh - within("#proposal_#{proposal.id}") { expect(page).to have_button "Select" } + within("#proposal_#{proposal.id}") { expect(page).to have_content "No" } end end From 4a2fc50c763ea8d947be1a2db5a74b769cdb717d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 12:29:20 +0200 Subject: [PATCH 06/28] Use separate actions to select/deselect proposals This is consistent to what we usually do. Also, we're applying the same criteria mentioned in commit 72704d776: > We're also making these actions idempotent, so sending many requests > to the same action will get the same result, which wasn't the case > with the `toggle` action. Although it's a low probability case, the > `toggle` action could result in [selecting a proposal] when trying to > [deselect] it if someone else has [deselected it] it between the time > the page loaded and the time the admin clicked on the "[Selected]" > button. --- .../admin/proposals/toggle_selection_component.rb | 6 +++++- .../admin/legislation/proposals_controller.rb | 13 ++++++++++--- app/controllers/admin/proposals_controller.rb | 13 ++++++++++--- config/routes/admin.rb | 11 +++++++++-- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/app/components/admin/proposals/toggle_selection_component.rb b/app/components/admin/proposals/toggle_selection_component.rb index 97a48803b..dfcb9ea8e 100644 --- a/app/components/admin/proposals/toggle_selection_component.rb +++ b/app/components/admin/proposals/toggle_selection_component.rb @@ -8,7 +8,11 @@ class Admin::Proposals::ToggleSelectionComponent < ApplicationComponent private def action - :toggle_selection + if selected? + :deselect + else + :select + end end def selected? diff --git a/app/controllers/admin/legislation/proposals_controller.rb b/app/controllers/admin/legislation/proposals_controller.rb index c2711aa99..aee02f7b0 100644 --- a/app/controllers/admin/legislation/proposals_controller.rb +++ b/app/controllers/admin/legislation/proposals_controller.rb @@ -8,8 +8,15 @@ class Admin::Legislation::ProposalsController < Admin::Legislation::BaseControll @proposals = @proposals.send("sort_by_#{@current_order}").page(params[:page]) end - def toggle_selection - @proposal.toggle :selected - @proposal.save! + def select + @proposal.update!(selected: true) + + render :toggle_selection + end + + def deselect + @proposal.update!(selected: false) + + render :toggle_selection end end diff --git a/app/controllers/admin/proposals_controller.rb b/app/controllers/admin/proposals_controller.rb index 18b2866ec..b2046e6b5 100644 --- a/app/controllers/admin/proposals_controller.rb +++ b/app/controllers/admin/proposals_controller.rb @@ -19,9 +19,16 @@ class Admin::ProposalsController < Admin::BaseController end end - def toggle_selection - @proposal.toggle :selected - @proposal.save! + def select + @proposal.update!(selected: true) + + render :toggle_selection + end + + def deselect + @proposal.update!(selected: false) + + render :toggle_selection end private diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 6f3f3d561..442882bb5 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -32,7 +32,11 @@ namespace :admin do resources :debates, only: [:index, :show] resources :proposals, only: [:index, :show, :update] do - member { patch :toggle_selection } + member do + patch :select + patch :deselect + end + resources :milestones, controller: "proposal_milestones" resources :progress_bars, except: :show, controller: "proposal_progress_bars" end @@ -232,7 +236,10 @@ namespace :admin do resources :processes do resources :questions resources :proposals do - member { patch :toggle_selection } + member do + patch :select + patch :deselect + end end resources :draft_versions resources :milestones From 2acaa14705a915048918b22f1f741f755d2b16fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 12:47:49 +0200 Subject: [PATCH 07/28] Make it possible to select proposals without JavaScript This way, when JavaScript hasn't loaded (for whatever reason), administrators can still use this functionality. --- .../admin/legislation/proposals_controller.rb | 10 ++- app/controllers/admin/proposals_controller.rb | 10 ++- .../legislation/proposals_controller_spec.rb | 61 +++++++++++++++++++ .../admin/proposals_controller_spec.rb | 61 +++++++++++++++++++ spec/factories/legislations.rb | 4 ++ 5 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 spec/controllers/admin/legislation/proposals_controller_spec.rb create mode 100644 spec/controllers/admin/proposals_controller_spec.rb diff --git a/app/controllers/admin/legislation/proposals_controller.rb b/app/controllers/admin/legislation/proposals_controller.rb index aee02f7b0..8022e8628 100644 --- a/app/controllers/admin/legislation/proposals_controller.rb +++ b/app/controllers/admin/legislation/proposals_controller.rb @@ -11,12 +11,18 @@ class Admin::Legislation::ProposalsController < Admin::Legislation::BaseControll def select @proposal.update!(selected: true) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.proposal") } + format.js { render :toggle_selection } + end end def deselect @proposal.update!(selected: false) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.proposal") } + format.js { render :toggle_selection } + end end end diff --git a/app/controllers/admin/proposals_controller.rb b/app/controllers/admin/proposals_controller.rb index b2046e6b5..5e9e52b49 100644 --- a/app/controllers/admin/proposals_controller.rb +++ b/app/controllers/admin/proposals_controller.rb @@ -22,13 +22,19 @@ class Admin::ProposalsController < Admin::BaseController def select @proposal.update!(selected: true) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.proposal") } + format.js { render :toggle_selection } + end end def deselect @proposal.update!(selected: false) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.proposal") } + format.js { render :toggle_selection } + end end private diff --git a/spec/controllers/admin/legislation/proposals_controller_spec.rb b/spec/controllers/admin/legislation/proposals_controller_spec.rb new file mode 100644 index 000000000..6cd64562c --- /dev/null +++ b/spec/controllers/admin/legislation/proposals_controller_spec.rb @@ -0,0 +1,61 @@ +require "rails_helper" + +describe Admin::Legislation::ProposalsController, :admin do + describe "PATCH select" do + let(:proposal) { create(:legislation_proposal) } + + it "selects the proposal" do + expect do + patch :select, xhr: true, params: { id: proposal.id, process_id: proposal.process.id } + end.to change { proposal.reload.selected? }.from(false).to(true) + + expect(response).to be_successful + end + + it "does not modify already selected proposals" do + proposal.update!(selected: true) + + expect do + patch :select, xhr: true, params: { id: proposal.id, process_id: proposal.process.id } + end.not_to change { proposal.reload.selected? } + end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_proposals_path + + patch :select, params: { id: proposal.id, process_id: proposal.process.id } + + expect(response).to redirect_to admin_proposals_path + expect(flash[:notice]).to eq "Proposal updated successfully." + end + end + + describe "PATCH deselect" do + let(:proposal) { create(:legislation_proposal, :selected) } + + it "deselects the proposal" do + expect do + patch :deselect, xhr: true, params: { id: proposal.id, process_id: proposal.process.id } + end.to change { proposal.reload.selected? }.from(true).to(false) + + expect(response).to be_successful + end + + it "does not modify non-selected proposals" do + proposal.update!(selected: false) + + expect do + patch :deselect, xhr: true, params: { id: proposal.id, process_id: proposal.process.id } + end.not_to change { proposal.reload.selected? } + end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_proposals_path + + patch :deselect, params: { id: proposal.id, process_id: proposal.process.id } + + expect(response).to redirect_to admin_proposals_path + expect(flash[:notice]).to eq "Proposal updated successfully." + end + end +end diff --git a/spec/controllers/admin/proposals_controller_spec.rb b/spec/controllers/admin/proposals_controller_spec.rb new file mode 100644 index 000000000..b23369adb --- /dev/null +++ b/spec/controllers/admin/proposals_controller_spec.rb @@ -0,0 +1,61 @@ +require "rails_helper" + +describe Admin::ProposalsController, :admin do + describe "PATCH select" do + let(:proposal) { create(:proposal) } + + it "selects the proposal" do + expect do + patch :select, xhr: true, params: { id: proposal.id } + end.to change { proposal.reload.selected? }.from(false).to(true) + + expect(response).to be_successful + end + + it "does not modify already selected proposals" do + proposal.update!(selected: true) + + expect do + patch :select, xhr: true, params: { id: proposal.id } + end.not_to change { proposal.reload.selected? } + end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_proposals_path + + patch :select, params: { id: proposal.id } + + expect(response).to redirect_to admin_proposals_path + expect(flash[:notice]).to eq "Proposal updated successfully." + end + end + + describe "PATCH deselect" do + let(:proposal) { create(:proposal, :selected) } + + it "deselects the proposal" do + expect do + patch :deselect, xhr: true, params: { id: proposal.id } + end.to change { proposal.reload.selected? }.from(true).to(false) + + expect(response).to be_successful + end + + it "does not modify non-selected proposals" do + proposal.update!(selected: false) + + expect do + patch :deselect, xhr: true, params: { id: proposal.id } + end.not_to change { proposal.reload.selected? } + end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_proposals_path + + patch :deselect, params: { id: proposal.id } + + expect(response).to redirect_to admin_proposals_path + expect(flash[:notice]).to eq "Proposal updated successfully." + end + end +end diff --git a/spec/factories/legislations.rb b/spec/factories/legislations.rb index 4ae250317..5c27fc3f5 100644 --- a/spec/factories/legislations.rb +++ b/spec/factories/legislations.rb @@ -177,5 +177,9 @@ FactoryBot.define do trait :hidden do hidden_at { Time.current } end + + trait :selected do + selected { true } + end end end From 02b6302f25f9c1d4eb2b79d4c33b2a6620e56c2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 13:15:08 +0200 Subject: [PATCH 08/28] Focus the proposal selection switch after pressing it Since this button is replaced by a new element in an AJAX call, nothing was focused after pressing it. So we're reusing the code we used to enable/disable budget phases, which already dealt with this issue. --- .../admin/budgets_wizard/phases/toggle_enabled.js.erb | 8 +++----- app/views/admin/legislation/proposals/_proposals.html.erb | 2 +- .../admin/legislation/proposals/toggle_selection.js.erb | 6 +++--- app/views/admin/proposals/index.html.erb | 2 +- app/views/admin/proposals/toggle_selection.js.erb | 6 +++--- app/views/admin/shared/_toggle_switch.js.erb | 5 +++++ 6 files changed, 16 insertions(+), 13 deletions(-) create mode 100644 app/views/admin/shared/_toggle_switch.js.erb diff --git a/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb b/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb index d0688ab25..764515939 100644 --- a/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb +++ b/app/views/admin/budgets_wizard/phases/toggle_enabled.js.erb @@ -1,5 +1,3 @@ -var replacement = $("<%= j render Admin::BudgetPhases::ToggleEnabledComponent.new(@phase) %>"); -var form = $("#<%= dom_id(@phase) %> .toggle-switch"); - -form.replaceWith(replacement); -replacement.find("[type='submit']").focus(); +<%= render "admin/shared/toggle_switch", + record: @phase, + new_content: render(Admin::BudgetPhases::ToggleEnabledComponent.new(@phase)) %> diff --git a/app/views/admin/legislation/proposals/_proposals.html.erb b/app/views/admin/legislation/proposals/_proposals.html.erb index c63bdf977..4694b1732 100644 --- a/app/views/admin/legislation/proposals/_proposals.html.erb +++ b/app/views/admin/legislation/proposals/_proposals.html.erb @@ -19,7 +19,7 @@ <%= proposal.id %> <%= proposal.title %> <%= proposal.votes_score %> - <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> + <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> <% end %> diff --git a/app/views/admin/legislation/proposals/toggle_selection.js.erb b/app/views/admin/legislation/proposals/toggle_selection.js.erb index 8b0507552..258c2901c 100644 --- a/app/views/admin/legislation/proposals/toggle_selection.js.erb +++ b/app/views/admin/legislation/proposals/toggle_selection.js.erb @@ -1,3 +1,3 @@ -$("#<%= dom_id(@proposal) %> .select").html( - "<%= j render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %>" -); +<%= render "admin/shared/toggle_switch", + record: @proposal, + new_content: render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %> diff --git a/app/views/admin/proposals/index.html.erb b/app/views/admin/proposals/index.html.erb index 23f5388f4..27964da5b 100644 --- a/app/views/admin/proposals/index.html.erb +++ b/app/views/admin/proposals/index.html.erb @@ -27,7 +27,7 @@ <%= link_to proposal.title, admin_proposal_path(proposal) %> <%= proposal.author.username %> <%= proposal.milestones.count %> - <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> + <%= render Admin::Proposals::ToggleSelectionComponent.new(proposal) %> <% end %> diff --git a/app/views/admin/proposals/toggle_selection.js.erb b/app/views/admin/proposals/toggle_selection.js.erb index bb832e071..258c2901c 100644 --- a/app/views/admin/proposals/toggle_selection.js.erb +++ b/app/views/admin/proposals/toggle_selection.js.erb @@ -1,3 +1,3 @@ -$("#<%= dom_id(@proposal) %> .js-select").html( - "<%= j render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %>" -); +<%= render "admin/shared/toggle_switch", + record: @proposal, + new_content: render(Admin::Proposals::ToggleSelectionComponent.new(@proposal)) %> diff --git a/app/views/admin/shared/_toggle_switch.js.erb b/app/views/admin/shared/_toggle_switch.js.erb new file mode 100644 index 000000000..f8c172155 --- /dev/null +++ b/app/views/admin/shared/_toggle_switch.js.erb @@ -0,0 +1,5 @@ +var new_toggle_switch = $("<%= j new_content %>"); +var current_toggle_switch = $("#<%= dom_id(record) %> .toggle-switch"); + +current_toggle_switch.replaceWith(new_toggle_switch); +new_toggle_switch.find("[type='submit']").focus(); From bb4280916814f5b26109c2b9da0ec32d9c6593a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 7 Oct 2024 13:08:53 +0200 Subject: [PATCH 09/28] Move investment partial to a component This way we'll be able to simplify it a little bit. Note that the original partial didn't include the whole row and only the cells. Since, most of the time, we include the whole row in partials, we're slightly modifying the component. --- .../budget_investments/row_component.html.erb | 111 ++++++++++++++++++ .../admin/budget_investments/row_component.rb | 14 +++ .../budget_investments/_investments.html.erb | 4 +- .../_select_investment.html.erb | 109 ----------------- .../toggle_selection.js.erb | 4 +- .../budget_investments/row_component_spec.rb | 9 ++ .../select_investment_spec.rb | 13 -- 7 files changed, 138 insertions(+), 126 deletions(-) create mode 100644 app/components/admin/budget_investments/row_component.html.erb create mode 100644 app/components/admin/budget_investments/row_component.rb delete mode 100644 app/views/admin/budget_investments/_select_investment.html.erb create mode 100644 spec/components/admin/budget_investments/row_component_spec.rb delete mode 100644 spec/views/admin/budget_investments/select_investment_spec.rb diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb new file mode 100644 index 000000000..e615a67c3 --- /dev/null +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -0,0 +1,111 @@ + + + <%= investment.id %> + + + + <%= link_to investment.title, + admin_budget_budget_investment_path(budget_id: budget.id, + id: investment.id, + params: Budget::Investment.filter_params(params).to_h), + target: "_blank" %> + + + + <%= investment.total_votes %> + + + + <% if investment.administrator.present? %> + "> + <%= investment.administrator.description_or_name %> + + <% else %> + <%= t("admin.budget_investments.index.no_admin_assigned") %> + <% end %> + + + + <%= investment.author.name %> + + + + <% valuators = [investment.assigned_valuation_groups, investment.assigned_valuators].compact %> + <% no_valuators_assigned = t("admin.budget_investments.index.no_valuators_assigned") %> + <%= valuators.present? ? valuators.join(", ") : no_valuators_assigned %> + + + + <%= investment.heading.name %> + + + + <%= t("admin.budget_investments.index.feasibility.#{investment.feasibility}") %> + + + <% if budget.show_money? %> + + <%= investment.formatted_price %> + + <% end %> + + + <%= investment.valuation_finished? ? t("shared.yes") : t("shared.no") %> + + + + <% 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" %> + <% end %> + <% else %> + <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %> + <% end %> + + + + <% if investment.selected? %> + <%= link_to_if can?(:toggle_selection, investment), + t("admin.budget_investments.index.selected"), + toggle_selection_admin_budget_budget_investment_path( + budget, + investment, + filter: params[:filter], + sort_by: params[:sort_by], + min_total_supports: params[:min_total_supports], + max_total_supports: params[:max_total_supports], + advanced_filters: params[:advanced_filters], + page: params[:page] + ), + method: :patch, + remote: true, + class: "button small expanded" %> + <% elsif investment.feasible? && investment.valuation_finished? %> + <% if can?(:toggle_selection, investment) %> + <%= link_to t("admin.budget_investments.index.select"), + toggle_selection_admin_budget_budget_investment_path( + budget, + investment, + filter: params[:filter], + sort_by: params[:sort_by], + min_total_supports: params[:min_total_supports], + max_total_supports: params[:max_total_supports], + advanced_filters: params[:advanced_filters], + page: params[:page] + ), + method: :patch, + remote: true, + class: "button small hollow expanded" %> + <% end %> + <% end %> + + + <% if params[:advanced_filters]&.include?("selected") %> + + <%= investment.incompatible? ? t("shared.yes") : t("shared.no") %> + + <% end %> + diff --git a/app/components/admin/budget_investments/row_component.rb b/app/components/admin/budget_investments/row_component.rb new file mode 100644 index 000000000..96a429cf2 --- /dev/null +++ b/app/components/admin/budget_investments/row_component.rb @@ -0,0 +1,14 @@ +class Admin::BudgetInvestments::RowComponent < ApplicationComponent + attr_reader :investment + use_helpers :can? + + def initialize(investment) + @investment = investment + end + + private + + def budget + investment.budget + end +end diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index f378fd63a..05559b935 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -49,9 +49,7 @@ <% @investments.each do |investment| %> - - <%= render "/admin/budget_investments/select_investment", investment: investment %> - + <%= render Admin::BudgetInvestments::RowComponent.new(investment) %> <% end %> diff --git a/app/views/admin/budget_investments/_select_investment.html.erb b/app/views/admin/budget_investments/_select_investment.html.erb deleted file mode 100644 index 9753b35f9..000000000 --- a/app/views/admin/budget_investments/_select_investment.html.erb +++ /dev/null @@ -1,109 +0,0 @@ - - <%= investment.id %> - - - - <%= link_to investment.title, - admin_budget_budget_investment_path(budget_id: @budget.id, - id: investment.id, - params: Budget::Investment.filter_params(params).to_h), - target: "_blank" %> - - - - <%= investment.total_votes %> - - - - <% if investment.administrator.present? %> - "> - <%= investment.administrator.description_or_name %> - - <% else %> - <%= t("admin.budget_investments.index.no_admin_assigned") %> - <% end %> - - - - <%= investment.author.name %> - - - - <% valuators = [investment.assigned_valuation_groups, investment.assigned_valuators].compact %> - <% no_valuators_assigned = t("admin.budget_investments.index.no_valuators_assigned") %> - <%= valuators.present? ? valuators.join(", ") : no_valuators_assigned %> - - - - <%= investment.heading.name %> - - - - <%= t("admin.budget_investments.index.feasibility.#{investment.feasibility}") %> - - -<% if @budget.show_money? %> - - <%= investment.formatted_price %> - -<% end %> - - - <%= investment.valuation_finished? ? t("shared.yes") : t("shared.no") %> - - - - <% 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 %> - - - - <% if investment.selected? %> - <%= link_to_if can?(:toggle_selection, investment), - t("admin.budget_investments.index.selected"), - toggle_selection_admin_budget_budget_investment_path( - @budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), - method: :patch, - remote: true, - class: "button small expanded" %> - <% elsif investment.feasible? && investment.valuation_finished? %> - <% if can?(:toggle_selection, investment) %> - <%= link_to t("admin.budget_investments.index.select"), - toggle_selection_admin_budget_budget_investment_path( - @budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), - method: :patch, - remote: true, - class: "button small hollow expanded" %> - <% end %> - <% end %> - - -<% if params[:advanced_filters]&.include?("selected") %> - - <%= investment.incompatible? ? t("shared.yes") : t("shared.no") %> - -<% end %> diff --git a/app/views/admin/budget_investments/toggle_selection.js.erb b/app/views/admin/budget_investments/toggle_selection.js.erb index ecf457e8c..75fc98a36 100644 --- a/app/views/admin/budget_investments/toggle_selection.js.erb +++ b/app/views/admin/budget_investments/toggle_selection.js.erb @@ -1 +1,3 @@ -$("#<%= dom_id(@investment) %>").html("<%= j render("select_investment", investment: @investment) %>").trigger("inserted"); +$("#<%= dom_id(@investment) %>").replaceWith( + "<%= j render Admin::BudgetInvestments::RowComponent.new(@investment) %>" +).trigger("inserted"); diff --git a/spec/components/admin/budget_investments/row_component_spec.rb b/spec/components/admin/budget_investments/row_component_spec.rb new file mode 100644 index 000000000..4f7dfab7c --- /dev/null +++ b/spec/components/admin/budget_investments/row_component_spec.rb @@ -0,0 +1,9 @@ +require "rails_helper" + +describe Admin::BudgetInvestments::RowComponent, :admin do + it "uses a JSON request to update visible to valuators" do + render_inline Admin::BudgetInvestments::RowComponent.new(create(:budget_investment)) + + expect(page).to have_css "form[action$='json'] input[name$='[visible_to_valuators]']" + end +end diff --git a/spec/views/admin/budget_investments/select_investment_spec.rb b/spec/views/admin/budget_investments/select_investment_spec.rb deleted file mode 100644 index e04c6dd8f..000000000 --- a/spec/views/admin/budget_investments/select_investment_spec.rb +++ /dev/null @@ -1,13 +0,0 @@ -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 9330cdb3040c2f3072bea59a6e2690354d5acf61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 7 Oct 2024 13:23:14 +0200 Subject: [PATCH 10/28] Extract methods in investment row component This way we simplify the code a little bit. --- .../budget_investments/row_component.html.erb | 18 +++---------- .../admin/budget_investments/row_component.rb | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index e615a67c3..0ddd47c38 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -4,11 +4,7 @@ - <%= link_to investment.title, - admin_budget_budget_investment_path(budget_id: budget.id, - id: investment.id, - params: Budget::Investment.filter_params(params).to_h), - target: "_blank" %> + <%= link_to investment.title, investment_path, target: "_blank" %> @@ -16,13 +12,7 @@ - <% if investment.administrator.present? %> - "> - <%= investment.administrator.description_or_name %> - - <% else %> - <%= t("admin.budget_investments.index.no_admin_assigned") %> - <% end %> + <%= administrator_info %> @@ -30,9 +20,7 @@ - <% valuators = [investment.assigned_valuation_groups, investment.assigned_valuators].compact %> - <% no_valuators_assigned = t("admin.budget_investments.index.no_valuators_assigned") %> - <%= valuators.present? ? valuators.join(", ") : no_valuators_assigned %> + <%= valuators_info %> diff --git a/app/components/admin/budget_investments/row_component.rb b/app/components/admin/budget_investments/row_component.rb index 96a429cf2..71a001d9c 100644 --- a/app/components/admin/budget_investments/row_component.rb +++ b/app/components/admin/budget_investments/row_component.rb @@ -11,4 +11,29 @@ class Admin::BudgetInvestments::RowComponent < ApplicationComponent def budget investment.budget end + + def investment_path + admin_budget_budget_investment_path(budget_id: budget.id, + id: investment.id, + params: Budget::Investment.filter_params(params).to_h) + end + + def administrator_info + if investment.administrator.present? + tag.span(investment.administrator.description_or_name, + title: t("admin.budget_investments.index.assigned_admin")) + else + t("admin.budget_investments.index.no_admin_assigned") + end + end + + def valuators_info + valuators = [investment.assigned_valuation_groups, investment.assigned_valuators].compact + + if valuators.present? + valuators.join(", ") + else + t("admin.budget_investments.index.no_valuators_assigned") + end + end end From e4df6426c2493bf62e339fcabd8ebb6d26e8ba39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 7 Oct 2024 14:25:44 +0200 Subject: [PATCH 11/28] Remove unused JavaScript view in investments admin This code isn't used since commit c9f31b8e1. Since we no longer depend on the content of the `#investments` element being in a separate partial, we're also moving this element to the partial itself and adding an HTML class to it, like we usually do. We're also removing the code that loads all the investments in the `toggle_selection` action, which wasn't needed since commit 3278b3572, when we stopped rendering all the investments in this action. --- .../admin/budget_investments_controller.rb | 6 +- .../budget_investments/_investments.html.erb | 114 +++++++++--------- .../admin/budget_investments/index.html.erb | 4 +- .../admin/budget_investments/index.js.erb | 1 - 4 files changed, 61 insertions(+), 64 deletions(-) delete mode 100644 app/views/admin/budget_investments/index.js.erb diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index c4d68ae43..cb760d43b 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -6,19 +6,18 @@ class Admin::BudgetInvestmentsController < Admin::BaseController feature_flag :budgets has_orders %w[oldest], only: [:show, :edit] - has_filters %w[all], only: [:index, :toggle_selection] + has_filters %w[all], only: :index before_action :load_budget before_action :load_investment, only: [:show, :edit, :update, :toggle_selection] before_action :load_ballot, only: [:show, :index] before_action :parse_valuation_filters - before_action :load_investments, only: [:index, :toggle_selection] + before_action :load_investments, only: :index def index load_tags respond_to do |format| format.html - format.js format.csv do send_data Budget::Investment::Exporter.new(@investments).to_csv, filename: "budget_investments.csv" @@ -65,7 +64,6 @@ class Admin::BudgetInvestmentsController < Admin::BaseController authorize! :toggle_selection, @investment @investment.toggle :selected @investment.save! - load_investments end private diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/views/admin/budget_investments/_investments.html.erb index 05559b935..073abb90d 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/views/admin/budget_investments/_investments.html.erb @@ -1,62 +1,64 @@ -<%= link_to t("admin.budget_investments.index.download_current_selection"), - admin_budget_budget_investments_path(csv_params), - class: "float-right small clear" %> +
+ <%= link_to t("admin.budget_investments.index.download_current_selection"), + admin_budget_budget_investments_path(csv_params), + class: "float-right small clear" %> -<% if params[:advanced_filters].include?("winners") %> - <%= render Admin::Budgets::CalculateWinnersButtonComponent.new(@budget, from_investments: true) %> -<% end %> + <% if params[:advanced_filters].include?("winners") %> + <%= render Admin::Budgets::CalculateWinnersButtonComponent.new(@budget, from_investments: true) %> + <% end %> -<% if @investments.any? %> -

<%= page_entries_info @investments %>

- <%= render "admin/shared/columns_selector", - cookie: "investments-columns", - default: %w[id title supports admin valuator geozone feasibility price valuation_finished visible_to_valuators selected incompatible] %> -
+ <% if @investments.any? %> +

<%= page_entries_info @investments %>

+ <%= render "admin/shared/columns_selector", + cookie: "investments-columns", + default: %w[id title supports admin valuator geozone feasibility price valuation_finished visible_to_valuators selected incompatible] %> +
- <%= render "filters_description", i18n_namespace: "admin.budget_investments.index" %> + <%= render "filters_description", i18n_namespace: "admin.budget_investments.index" %> - - - - - - - - - - - - <% if @budget.show_money? %> - +
<%= link_to_investments_sorted_by :id %><%= link_to_investments_sorted_by :title %><%= link_to_investments_sorted_by :supports %><%= t("admin.budget_investments.index.list.admin") %> - <%= t("admin.budget_investments.index.list.author") %> - - <%= t("admin.budget_investments.index.list.valuation_group") %> / - <%= t("admin.budget_investments.index.list.valuator") %> - <%= t("admin.budget_investments.index.list.geozone") %><%= t("admin.budget_investments.index.list.feasibility") %><%= t("admin.budget_investments.index.list.price") %>
+ + + + + + + + + + + <% if @budget.show_money? %> + + <% end %> + + + + <% if params[:advanced_filters]&.include?("selected") %> + + <% end %> + + + + + <% @investments.each do |investment| %> + <%= render Admin::BudgetInvestments::RowComponent.new(investment) %> <% end %> - - - - <% if params[:advanced_filters]&.include?("selected") %> - - <% end %> - - + +
<%= link_to_investments_sorted_by :id %><%= link_to_investments_sorted_by :title %><%= link_to_investments_sorted_by :supports %><%= t("admin.budget_investments.index.list.admin") %> + <%= t("admin.budget_investments.index.list.author") %> + + <%= t("admin.budget_investments.index.list.valuation_group") %> / + <%= t("admin.budget_investments.index.list.valuator") %> + <%= t("admin.budget_investments.index.list.geozone") %><%= t("admin.budget_investments.index.list.feasibility") %><%= t("admin.budget_investments.index.list.price") %> + <%= t("admin.budget_investments.index.list.valuation_finished") %> + + <%= t("admin.budget_investments.index.list.visible_to_valuators") %> + <%= t("admin.budget_investments.index.list.selected") %><%= t("admin.budget_investments.index.list.incompatible") %>
- <%= t("admin.budget_investments.index.list.valuation_finished") %> - - <%= t("admin.budget_investments.index.list.visible_to_valuators") %> - <%= t("admin.budget_investments.index.list.selected") %><%= t("admin.budget_investments.index.list.incompatible") %>
- - <% @investments.each do |investment| %> - <%= render Admin::BudgetInvestments::RowComponent.new(investment) %> - <% end %> - - - - <%= paginate @investments %> -<% else %> -
- <%= t("admin.budget_investments.index.no_budget_investments") %> -
-<% end %> + <%= paginate @investments %> + <% else %> +
+ <%= t("admin.budget_investments.index.no_budget_investments") %> +
+ <% end %> +
diff --git a/app/views/admin/budget_investments/index.html.erb b/app/views/admin/budget_investments/index.html.erb index 4878421cd..bfd50f9e8 100644 --- a/app/views/admin/budget_investments/index.html.erb +++ b/app/views/admin/budget_investments/index.html.erb @@ -12,6 +12,4 @@ <%= render "/shared/filter_subnav", i18n_namespace: "admin.budget_investments.index" %> -
- <%= render "investments" %> -
+<%= render "investments" %> diff --git a/app/views/admin/budget_investments/index.js.erb b/app/views/admin/budget_investments/index.js.erb deleted file mode 100644 index c1569cc59..000000000 --- a/app/views/admin/budget_investments/index.js.erb +++ /dev/null @@ -1 +0,0 @@ -$("#investments").html("<%= j render("admin/budget_investments/investments") %>"); From 607dabbc8abd88721a28d375e7825481a89bd0c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 7 Oct 2024 13:45:23 +0200 Subject: [PATCH 12/28] Move admin investments partial to a component This way it'll be easier to organize the code related to it. --- .../investments_component.html.erb} | 12 ++++---- .../investments_component.rb | 30 +++++++++++++++++++ app/helpers/budget_investments_helper.rb | 12 -------- app/helpers/budgets_helper.rb | 7 ----- .../admin/budget_investments/index.html.erb | 2 +- 5 files changed, 37 insertions(+), 26 deletions(-) rename app/{views/admin/budget_investments/_investments.html.erb => components/admin/budget_investments/investments_component.html.erb} (91%) create mode 100644 app/components/admin/budget_investments/investments_component.rb diff --git a/app/views/admin/budget_investments/_investments.html.erb b/app/components/admin/budget_investments/investments_component.html.erb similarity index 91% rename from app/views/admin/budget_investments/_investments.html.erb rename to app/components/admin/budget_investments/investments_component.html.erb index 073abb90d..f5e0f4a59 100644 --- a/app/views/admin/budget_investments/_investments.html.erb +++ b/app/components/admin/budget_investments/investments_component.html.erb @@ -4,11 +4,11 @@ class: "float-right small clear" %> <% if params[:advanced_filters].include?("winners") %> - <%= render Admin::Budgets::CalculateWinnersButtonComponent.new(@budget, from_investments: true) %> + <%= render Admin::Budgets::CalculateWinnersButtonComponent.new(budget, from_investments: true) %> <% end %> - <% if @investments.any? %> -

<%= page_entries_info @investments %>

+ <% if investments.any? %> +

<%= page_entries_info investments %>

<%= render "admin/shared/columns_selector", cookie: "investments-columns", default: %w[id title supports admin valuator geozone feasibility price valuation_finished visible_to_valuators selected incompatible] %> @@ -32,7 +32,7 @@ <%= t("admin.budget_investments.index.list.geozone") %> <%= t("admin.budget_investments.index.list.feasibility") %> - <% if @budget.show_money? %> + <% if budget.show_money? %> <%= t("admin.budget_investments.index.list.price") %> <% end %> @@ -49,13 +49,13 @@ - <% @investments.each do |investment| %> + <% investments.each do |investment| %> <%= render Admin::BudgetInvestments::RowComponent.new(investment) %> <% end %> - <%= paginate @investments %> + <%= paginate investments %> <% else %>
<%= t("admin.budget_investments.index.no_budget_investments") %> diff --git a/app/components/admin/budget_investments/investments_component.rb b/app/components/admin/budget_investments/investments_component.rb new file mode 100644 index 000000000..74f7f439b --- /dev/null +++ b/app/components/admin/budget_investments/investments_component.rb @@ -0,0 +1,30 @@ +class Admin::BudgetInvestments::InvestmentsComponent < ApplicationComponent + attr_reader :budget, :investments + use_helpers :set_direction, :set_sorting_icon + + def initialize(budget, investments) + @budget = budget + @investments = investments + end + + private + + def csv_params + csv_params = params.clone.merge(format: :csv) + csv_params = csv_params.to_unsafe_h.transform_keys(&:to_sym) + csv_params.delete(:page) + csv_params + end + + def link_to_investments_sorted_by(column) + direction = set_direction(params[:direction]) + icon = set_sorting_icon(direction, column) + + translation = t("admin.budget_investments.index.list.#{column}") + + link_to( + safe_join([translation, tag.span(class: "icon-sortable #{icon}")]), + admin_budget_budget_investments_path(sort_by: column, direction: direction) + ) + end +end diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index dc0920630..f3a49cde6 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -3,18 +3,6 @@ module BudgetInvestmentsHelper params.map { |af| t("admin.budget_investments.index.filters.#{af}") }.join(", ") end - def link_to_investments_sorted_by(column) - direction = set_direction(params[:direction]) - icon = set_sorting_icon(direction, column) - - translation = t("admin.budget_investments.index.list.#{column}") - - link_to( - safe_join([translation, tag.span(class: "icon-sortable #{icon}")]), - admin_budget_budget_investments_path(sort_by: column, direction: direction) - ) - end - def set_sorting_icon(direction, sort_by) if sort_by.to_s == params[:sort_by] if direction == "desc" diff --git a/app/helpers/budgets_helper.rb b/app/helpers/budgets_helper.rb index ffcfc548c..7ad8d9a52 100644 --- a/app/helpers/budgets_helper.rb +++ b/app/helpers/budgets_helper.rb @@ -1,11 +1,4 @@ module BudgetsHelper - def csv_params - csv_params = params.clone.merge(format: :csv) - csv_params = csv_params.to_unsafe_h.transform_keys(&:to_sym) - csv_params.delete(:page) - csv_params - end - def namespaced_budget_investment_path(investment, options = {}) case namespace when "management" diff --git a/app/views/admin/budget_investments/index.html.erb b/app/views/admin/budget_investments/index.html.erb index bfd50f9e8..7a8e46741 100644 --- a/app/views/admin/budget_investments/index.html.erb +++ b/app/views/admin/budget_investments/index.html.erb @@ -12,4 +12,4 @@ <%= render "/shared/filter_subnav", i18n_namespace: "admin.budget_investments.index" %> -<%= render "investments" %> +<%= render Admin::BudgetInvestments::InvestmentsComponent.new(@budget, @investments) %> From 73166e164b2acd6f9523ab95b313a8b7517b680c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 7 Oct 2024 14:00:35 +0200 Subject: [PATCH 13/28] Simplify HTML for an investment row Since we define the `data-field` element, we can style each element individually with CSS. I'm not sure whether these styles make sense, though. For instance, why is "Supports" aligned to the center, since it's a number? For now, we're leaving it as it was. --- .../admin/budget_investments/investments.scss | 15 +++++++++++++ .../budget_investments/row_component.html.erb | 22 +++++++++---------- 2 files changed, 26 insertions(+), 11 deletions(-) create mode 100644 app/assets/stylesheets/admin/budget_investments/investments.scss diff --git a/app/assets/stylesheets/admin/budget_investments/investments.scss b/app/assets/stylesheets/admin/budget_investments/investments.scss new file mode 100644 index 000000000..479c69fe9 --- /dev/null +++ b/app/assets/stylesheets/admin/budget_investments/investments.scss @@ -0,0 +1,15 @@ +.admin .admin-budget-investments { + td { + &[data-field=supports], + &[data-field=valuation_finished], + &[data-field=visible_to_valuators], + &[data-field=selected], + &[data-field=incompatible] { + text-align: center; + } + + &:not([data-field=id], [data-field=title], [data-field=supports]) { + font-size: $small-font-size; + } + } +} diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index 0ddd47c38..b707b56bc 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -7,41 +7,41 @@ <%= link_to investment.title, investment_path, target: "_blank" %> - + <%= investment.total_votes %> - + <%= administrator_info %> - + <%= investment.author.name %> - + <%= valuators_info %> - + <%= investment.heading.name %> - + <%= t("admin.budget_investments.index.feasibility.#{investment.feasibility}") %> <% if budget.show_money? %> - + <%= investment.formatted_price %> <% end %> - + <%= investment.valuation_finished? ? t("shared.yes") : t("shared.no") %> - + <% if can?(:admin_update, investment) %> <%= form_for [:admin, budget, investment], remote: true, format: :json do |f| %> <%= f.check_box :visible_to_valuators, @@ -54,7 +54,7 @@ <% end %> - + <% if investment.selected? %> <%= link_to_if can?(:toggle_selection, investment), t("admin.budget_investments.index.selected"), @@ -92,7 +92,7 @@ <% if params[:advanced_filters]&.include?("selected") %> - + <%= investment.incompatible? ? t("shared.yes") : t("shared.no") %> <% end %> From b9c3e759302a91d6e206576a24d2ff1b8f7c2909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 14:35:22 +0200 Subject: [PATCH 14/28] Fix duplicate HTML IDs in investments selection Since this cell is shown once per row, there were multiple rows with the same HTML ID on the page. --- .../admin/budget_investments/row_component.html.erb | 2 +- spec/system/admin/budget_investments_spec.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index b707b56bc..cc37536f9 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -54,7 +54,7 @@ <% end %> - + <% if investment.selected? %> <%= link_to_if can?(:toggle_selection, investment), t("admin.budget_investments.index.selected"), diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 506562d17..1a5a16a89 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1451,22 +1451,22 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) - within("#budget_investment_#{unfeasible_bi.id} #selection") do + within("#budget_investment_#{unfeasible_bi.id} [data-field=selected]") do expect(page).not_to have_content("Select") expect(page).not_to have_content("Selected") end - within("#budget_investment_#{feasible_bi.id} #selection") do + within("#budget_investment_#{feasible_bi.id} [data-field=selected]") do expect(page).not_to have_content("Select") expect(page).not_to have_content("Selected") end - within("#budget_investment_#{feasible_vf_bi.id} #selection") do + within("#budget_investment_#{feasible_vf_bi.id} [data-field=selected]") do expect(page).not_to have_content("Select") expect(page).not_to have_content("Selected") end - within("#budget_investment_#{selected_bi.id} #selection") do + within("#budget_investment_#{selected_bi.id} [data-field=selected]") do expect(page).not_to contain_exactly("Select") expect(page).to have_content("Selected") end From fde24870bc1cd2d0a22232fb93167c0251d6b06c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:25:36 +0200 Subject: [PATCH 15/28] Extract component for investment toggle selection This way it'll be easier to refactor it. --- .../budget_investments/row_component.html.erb | 35 +--------- .../toggle_selection_component.html.erb | 34 ++++++++++ .../toggle_selection_component.rb | 14 ++++ .../toggle_selection_component_spec.rb | 64 ++++++++++++++++++ spec/system/admin/budget_investments_spec.rb | 65 ------------------- 5 files changed, 113 insertions(+), 99 deletions(-) create mode 100644 app/components/admin/budget_investments/toggle_selection_component.html.erb create mode 100644 app/components/admin/budget_investments/toggle_selection_component.rb create mode 100644 spec/components/admin/budget_investments/toggle_selection_component_spec.rb diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index cc37536f9..49129a7c7 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -55,40 +55,7 @@ - <% if investment.selected? %> - <%= link_to_if can?(:toggle_selection, investment), - t("admin.budget_investments.index.selected"), - toggle_selection_admin_budget_budget_investment_path( - budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), - method: :patch, - remote: true, - class: "button small expanded" %> - <% elsif investment.feasible? && investment.valuation_finished? %> - <% if can?(:toggle_selection, investment) %> - <%= link_to t("admin.budget_investments.index.select"), - toggle_selection_admin_budget_budget_investment_path( - budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), - method: :patch, - remote: true, - class: "button small hollow expanded" %> - <% end %> - <% end %> + <%= render Admin::BudgetInvestments::ToggleSelectionComponent.new(investment) %> <% if params[:advanced_filters]&.include?("selected") %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb new file mode 100644 index 000000000..82b4a9165 --- /dev/null +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -0,0 +1,34 @@ +<% if investment.selected? %> + <%= link_to_if can?(:toggle_selection, investment), + t("admin.budget_investments.index.selected"), + toggle_selection_admin_budget_budget_investment_path( + budget, + investment, + filter: params[:filter], + sort_by: params[:sort_by], + min_total_supports: params[:min_total_supports], + max_total_supports: params[:max_total_supports], + advanced_filters: params[:advanced_filters], + page: params[:page] + ), + method: :patch, + remote: true, + class: "button small expanded" %> +<% elsif investment.feasible? && investment.valuation_finished? %> + <% if can?(:toggle_selection, investment) %> + <%= link_to t("admin.budget_investments.index.select"), + toggle_selection_admin_budget_budget_investment_path( + budget, + investment, + filter: params[:filter], + sort_by: params[:sort_by], + min_total_supports: params[:min_total_supports], + max_total_supports: params[:max_total_supports], + advanced_filters: params[:advanced_filters], + page: params[:page] + ), + method: :patch, + remote: true, + class: "button small hollow expanded" %> + <% end %> +<% end %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb new file mode 100644 index 000000000..312be4ba9 --- /dev/null +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -0,0 +1,14 @@ +class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent + attr_reader :investment + use_helpers :can? + + def initialize(investment) + @investment = investment + end + + private + + def budget + investment.budget + end +end diff --git a/spec/components/admin/budget_investments/toggle_selection_component_spec.rb b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb new file mode 100644 index 000000000..dd5157242 --- /dev/null +++ b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb @@ -0,0 +1,64 @@ +require "rails_helper" + +describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do + context "open budget" do + let(:budget) { create(:budget) } + + it "is not rendered for not-yet-evaluated investments" do + unfeasible_investment = create(:budget_investment, :unfeasible, budget: budget) + feasible_investment = create(:budget_investment, :feasible, budget: budget) + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(unfeasible_investment) + expect(page).not_to be_rendered + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(feasible_investment) + expect(page).not_to be_rendered + end + + it "renders a link to select unselected evaluated investments" do + valuation_finished_investment = create(:budget_investment, :feasible, :finished, budget: budget) + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(valuation_finished_investment) + + expect(page).to have_link "Select" + expect(page).not_to have_link "Selected" + end + + it "renders a link to deselect selected investments" do + selected_investment = create(:budget_investment, :selected, budget: budget) + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) + + expect(page).to have_link "Selected" + expect(page).not_to have_link "Select" + end + end + + context "finished budget" do + let(:budget) { create(:budget, :finished) } + + it "is not rendered for unselected investments" do + unfeasible_investment = create(:budget_investment, :unfeasible, budget: budget) + feasible_investment = create(:budget_investment, :feasible, budget: budget) + valuation_finished_investment = create(:budget_investment, :feasible, :finished, budget: budget) + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(unfeasible_investment) + expect(page).not_to be_rendered + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(feasible_investment) + expect(page).not_to be_rendered + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(valuation_finished_investment) + expect(page).not_to be_rendered + end + + it "renders plain text for selected investments" do + selected_investment = create(:budget_investment, :selected, budget: budget) + + render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) + + expect(page).to have_content "Selected" + expect(page).not_to have_link "Selected" + end + end +end diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 1a5a16a89..69f0023bf 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -43,21 +43,6 @@ describe "Admin budget investments", :admin do expect(page).not_to have_content("€") end - scenario "If budget is finished do not show 'Selected' button" do - finished_budget = create(:budget, :finished) - budget_investment = create(:budget_investment, budget: finished_budget, cached_votes_up: 77) - - visit admin_budget_budget_investments_path(budget_id: finished_budget.id) - - within("#budget_investment_#{budget_investment.id}") do - expect(page).to have_content(budget_investment.title) - expect(page).to have_content(budget_investment.heading.name) - expect(page).to have_content(budget_investment.id) - expect(page).to have_content(budget_investment.total_votes) - expect(page).not_to have_link("Selected") - end - end - scenario "Display admin and valuator assignments" do olga = create(:user, username: "Olga") miriam = create(:user, username: "Miriam") @@ -1422,56 +1407,6 @@ describe "Admin budget investments", :admin do expect(page).not_to have_content(feasible_vf_bi.title) end - scenario "Showing the selection buttons" do - visit admin_budget_budget_investments_path(budget) - - within("#budget_investment_#{unfeasible_bi.id}") do - expect(page).not_to have_link("Select") - expect(page).not_to have_link("Selected") - end - - within("#budget_investment_#{feasible_bi.id}") do - expect(page).not_to have_link("Select") - expect(page).not_to have_link("Selected") - end - - within("#budget_investment_#{feasible_vf_bi.id}") do - expect(page).to have_link("Select") - expect(page).not_to have_link("Selected") - end - - within("#budget_investment_#{selected_bi.id}") do - expect(page).not_to have_link("Select") - expect(page).to have_link("Selected") - end - end - - scenario "Show only selected text when budget is finished" do - budget.update!(phase: "finished") - - visit admin_budget_budget_investments_path(budget) - - within("#budget_investment_#{unfeasible_bi.id} [data-field=selected]") do - expect(page).not_to have_content("Select") - expect(page).not_to have_content("Selected") - end - - within("#budget_investment_#{feasible_bi.id} [data-field=selected]") do - expect(page).not_to have_content("Select") - expect(page).not_to have_content("Selected") - end - - within("#budget_investment_#{feasible_vf_bi.id} [data-field=selected]") do - expect(page).not_to have_content("Select") - expect(page).not_to have_content("Selected") - end - - within("#budget_investment_#{selected_bi.id} [data-field=selected]") do - expect(page).not_to contain_exactly("Select") - expect(page).to have_content("Selected") - end - end - scenario "Selecting an investment" do visit admin_budget_budget_investments_path(budget) From c78494c100c985a14815c52e2b79916bab574801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:29:07 +0200 Subject: [PATCH 16/28] Extract method to get toggle selection path This way we simplify the view code. Since now we only use the `budget` method in one place, we're removing it. --- .../toggle_selection_component.html.erb | 22 ++----------------- .../toggle_selection_component.rb | 13 +++++++++-- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 82b4a9165..97d671e67 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,32 +1,14 @@ <% if investment.selected? %> <%= link_to_if can?(:toggle_selection, investment), t("admin.budget_investments.index.selected"), - toggle_selection_admin_budget_budget_investment_path( - budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), + path, method: :patch, remote: true, class: "button small expanded" %> <% elsif investment.feasible? && investment.valuation_finished? %> <% if can?(:toggle_selection, investment) %> <%= link_to t("admin.budget_investments.index.select"), - toggle_selection_admin_budget_budget_investment_path( - budget, - investment, - filter: params[:filter], - sort_by: params[:sort_by], - min_total_supports: params[:min_total_supports], - max_total_supports: params[:max_total_supports], - advanced_filters: params[:advanced_filters], - page: params[:page] - ), + path, method: :patch, remote: true, class: "button small hollow expanded" %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb index 312be4ba9..9dd870d86 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.rb +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -8,7 +8,16 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent private - def budget - investment.budget + def path + toggle_selection_admin_budget_budget_investment_path( + investment.budget, + investment, + filter: params[:filter], + sort_by: params[:sort_by], + min_total_supports: params[:min_total_supports], + max_total_supports: params[:max_total_supports], + advanced_filters: params[:advanced_filters], + page: params[:page] + ) end end From 95f36ed52fb0c4385852ba7578b4fe9e19a8465d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:44:08 +0200 Subject: [PATCH 17/28] Simplify code to toggle investment selection This way it'll be easier to change the link/button used to toggle the selection. Note that the conditions in the view seem to be different because we no longer include the `selected?` condition when rendering the link/button. However, an investment can only be selected if it's feasible and its valuation is finished, so writing something like this would have been redundant: ```ruby can?(:toggle_selection, investment) && (selected? || investment.feasible? && investment.valuation_finished?) ``` The reason why the previous code was using the `selected?` condition was to check whether to render the link/button to select or to deselect an investment. We're now doing that in the Ruby part of the component. --- .../toggle_selection_component.html.erb | 19 ++++------------- .../toggle_selection_component.rb | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 97d671e67..9d5d3c87e 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,16 +1,5 @@ -<% if investment.selected? %> - <%= link_to_if can?(:toggle_selection, investment), - t("admin.budget_investments.index.selected"), - path, - method: :patch, - remote: true, - class: "button small expanded" %> -<% elsif investment.feasible? && investment.valuation_finished? %> - <% if can?(:toggle_selection, investment) %> - <%= link_to t("admin.budget_investments.index.select"), - path, - method: :patch, - remote: true, - class: "button small hollow expanded" %> - <% end %> +<% if can?(:toggle_selection, investment) && investment.feasible? && investment.valuation_finished? %> + <%= link_to text, path, method: :patch, remote: true, class: html_class %> +<% elsif selected? %> + <%= selected_text %> <% end %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb index 9dd870d86..64d662fd5 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.rb +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -1,6 +1,7 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent attr_reader :investment use_helpers :can? + delegate :selected?, to: :investment def initialize(investment) @investment = investment @@ -8,6 +9,18 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent private + def text + if selected? + selected_text + else + t("admin.budget_investments.index.select") + end + end + + def selected_text + t("admin.budget_investments.index.selected") + end + def path toggle_selection_admin_budget_budget_investment_path( investment.budget, @@ -20,4 +33,12 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent page: params[:page] ) end + + def html_class + if selected? + "button small expanded" + else + "button small hollow expanded" + end + end end From cf0d8258edbd6324ac6400ba4c14f24da6bd4b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:49:16 +0200 Subject: [PATCH 18/28] Use abilities to allow toggling investment selection We were checking it in the view, meaning that it was possible to toggle the selection by sending a custom request even when the investment wasn't feasible. --- .../toggle_selection_component.html.erb | 2 +- app/models/abilities/administrator.rb | 7 +++++-- .../admin/budget_investments_controller_spec.rb | 15 +++++++++++++++ spec/models/abilities/administrator_spec.rb | 4 ++++ 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 9d5d3c87e..8653ae80e 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,4 +1,4 @@ -<% if can?(:toggle_selection, investment) && investment.feasible? && investment.valuation_finished? %> +<% if can?(:toggle_selection, investment) %> <%= link_to text, path, method: :patch, remote: true, class: html_class %> <% elsif selected? %> <%= selected_text %> diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 9422a4cf5..82f6e48d7 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -71,10 +71,13 @@ module Abilities can [:read, :create, :update, :destroy], Budget::Group can [:read, :create, :update, :destroy], Budget::Heading - can [:hide, :admin_update, :toggle_selection], Budget::Investment + can [:hide, :admin_update], Budget::Investment can [:valuate, :comment_valuation], Budget::Investment - cannot [:admin_update, :toggle_selection, :valuate, :comment_valuation], + cannot [:admin_update, :valuate, :comment_valuation], Budget::Investment, budget: { phase: "finished" } + can :toggle_selection, Budget::Investment do |investment| + investment.feasible? && investment.valuation_finished? && !investment.budget.finished? + end can :create, Budget::ValuatorAssignment diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb index 6fd1cc4e7..7f7873192 100644 --- a/spec/controllers/admin/budget_investments_controller_spec.rb +++ b/spec/controllers/admin/budget_investments_controller_spec.rb @@ -37,4 +37,19 @@ describe Admin::BudgetInvestmentsController, :admin do expect(response).not_to be_redirect end end + + describe "PATCH toggle selection" do + it "uses the toggle_selection authorization rules" do + investment = create(:budget_investment) + + patch :toggle_selection, xhr: true, params: { + id: investment, + budget_id: investment.budget, + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action " \ + "'toggle_selection' on Investment." + expect(investment).not_to be_selected + end + end end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index f24a6f4b4..eab07520f 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -115,6 +115,10 @@ describe Abilities::Administrator do it { should_not be_able_to(:admin_update, finished_investment) } it { should_not be_able_to(:valuate, finished_investment) } it { should_not be_able_to(:comment_valuation, finished_investment) } + + it { should be_able_to(:toggle_selection, create(:budget_investment, :feasible, :finished)) } + it { should_not be_able_to(:toggle_selection, create(:budget_investment, :feasible, :open)) } + it { should_not be_able_to(:toggle_selection, create(:budget_investment, :unfeasible, :finished)) } it { should_not be_able_to(:toggle_selection, finished_investment) } it { should be_able_to(:destroy, proposal_image) } From 9e5566cee48cf2cee201228f17225baf44c23767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:54:04 +0200 Subject: [PATCH 19/28] Use a button to toggle investment selection As mentioned in commit 5311daadf, using buttons for non-GET requests has several advantages over using links. --- .../toggle_selection_component.html.erb | 2 +- .../toggle_selection_component_spec.rb | 14 +++++------ spec/system/admin/budget_investments_spec.rb | 24 +++++++++---------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 8653ae80e..ffe677cc3 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,5 +1,5 @@ <% if can?(:toggle_selection, investment) %> - <%= link_to text, path, method: :patch, remote: true, class: html_class %> + <%= button_to text, path, method: :patch, remote: true, class: html_class %> <% elsif selected? %> <%= selected_text %> <% end %> diff --git a/spec/components/admin/budget_investments/toggle_selection_component_spec.rb b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb index dd5157242..954bee04b 100644 --- a/spec/components/admin/budget_investments/toggle_selection_component_spec.rb +++ b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb @@ -15,22 +15,22 @@ describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do expect(page).not_to be_rendered end - it "renders a link to select unselected evaluated investments" do + it "renders a button to select unselected evaluated investments" do valuation_finished_investment = create(:budget_investment, :feasible, :finished, budget: budget) render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(valuation_finished_investment) - expect(page).to have_link "Select" - expect(page).not_to have_link "Selected" + expect(page).to have_button "Select" + expect(page).not_to have_button "Selected" end - it "renders a link to deselect selected investments" do + it "renders a button to deselect selected investments" do selected_investment = create(:budget_investment, :selected, budget: budget) render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) - expect(page).to have_link "Selected" - expect(page).not_to have_link "Select" + expect(page).to have_button "Selected" + expect(page).not_to have_button "Select" end end @@ -58,7 +58,7 @@ describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) expect(page).to have_content "Selected" - expect(page).not_to have_link "Selected" + expect(page).not_to have_button "Selected" end end end diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 69f0023bf..cfa25f076 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1411,8 +1411,8 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) within("#budget_investment_#{feasible_vf_bi.id}") do - click_link("Select") - expect(page).to have_link("Selected") + click_button "Select" + expect(page).to have_button "Selected" end click_link "Advanced filters" @@ -1421,8 +1421,8 @@ describe "Admin budget investments", :admin do click_button("Filter") within("#budget_investment_#{feasible_vf_bi.id}") do - expect(page).not_to have_link("Select") - expect(page).to have_link("Selected") + expect(page).not_to have_button "Select" + expect(page).to have_button "Selected" end end @@ -1436,9 +1436,9 @@ describe "Admin budget investments", :admin do expect(page).to have_content("There are 2 investments") within("#budget_investment_#{selected_bi.id}") do - click_link("Selected") + click_button "Selected" - expect(page).to have_link("Select") + expect(page).to have_button "Select" end click_button("Filter") @@ -1448,8 +1448,8 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) within("#budget_investment_#{selected_bi.id}") do - expect(page).to have_link("Select") - expect(page).not_to have_link("Selected") + expect(page).to have_button "Select" + expect(page).not_to have_button "Selected" end end @@ -1462,9 +1462,9 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) within("#budget_investment_#{selected_bi.id}") do - click_link("Selected") + click_button "Selected" - expect(page).to have_link "Select" + expect(page).to have_button "Select" end click_link("Next") @@ -1802,9 +1802,9 @@ describe "Admin budget investments", :admin do within("#js-columns-selector-wrapper") { uncheck "Title" } within("#budget_investment_#{investment.id}") do - click_link "Selected" + click_button "Selected" - expect(page).to have_link "Select" + expect(page).to have_button "Select" expect(page).not_to have_content "Don't display me, please!" end end From f72daff71fcb0749b473458cc3278d68b4029591 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 14:22:18 +0200 Subject: [PATCH 20/28] Simplify JavaScript to toggle investment selection We don't need to replace the whole row, since the changes only affect the button. Therefore, we don't need to depend on an `inserted` event to decide which columns to render in that row. --- app/assets/javascripts/columns_selector.js | 3 --- app/views/admin/budget_investments/toggle_selection.js.erb | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/columns_selector.js b/app/assets/javascripts/columns_selector.js index 888dd2c76..40a562510 100644 --- a/app/assets/javascripts/columns_selector.js +++ b/app/assets/javascripts/columns_selector.js @@ -80,9 +80,6 @@ App.ColumnsSelector.toggleColumn(event); } }); - $(".column-selectable").on("inserted", function() { - App.ColumnsSelector.initColumns(); - }); }, destroy: function() { $("#js-columns-selector-wrapper").children(":not(#column_selector_item_template)").remove(); diff --git a/app/views/admin/budget_investments/toggle_selection.js.erb b/app/views/admin/budget_investments/toggle_selection.js.erb index 75fc98a36..6a99f9783 100644 --- a/app/views/admin/budget_investments/toggle_selection.js.erb +++ b/app/views/admin/budget_investments/toggle_selection.js.erb @@ -1,3 +1,3 @@ -$("#<%= dom_id(@investment) %>").replaceWith( - "<%= j render Admin::BudgetInvestments::RowComponent.new(@investment) %>" -).trigger("inserted"); +$("#<%= dom_id(@investment) %> [data-field='selected']").html( + "<%= j render(Admin::BudgetInvestments::ToggleSelectionComponent.new(@investment)) %>" +); From 1b46afc95cb75ce58b23cfdc6b5e0f0f76ab6832 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 16:06:31 +0200 Subject: [PATCH 21/28] Check different content in select investments test Since we were checking something that was already there, if the form were submitted using an AJAX request (which is the default with Turbo, although we don't use it yet), there would be a chance that the request didn't finish before the test does, leading to potential failures in the next test. This way we're also checking that the "Filter" button actually does something when selecting the "Selected" filter. --- spec/system/admin/budget_investments_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index cfa25f076..2df4ca0cf 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1410,6 +1410,8 @@ describe "Admin budget investments", :admin do scenario "Selecting an investment" do visit admin_budget_budget_investments_path(budget) + expect(page).to have_content "Unfeasible project" + within("#budget_investment_#{feasible_vf_bi.id}") do click_button "Select" expect(page).to have_button "Selected" @@ -1420,6 +1422,8 @@ describe "Admin budget investments", :admin do within("#advanced_filters") { check("Selected") } click_button("Filter") + expect(page).not_to have_content "Unfeasible project" + within("#budget_investment_#{feasible_vf_bi.id}") do expect(page).not_to have_button "Select" expect(page).to have_button "Selected" From 463112c2eaca29280499476b2591cb1dad67e9b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 00:50:11 +0200 Subject: [PATCH 22/28] Use a switch to toggle investment selection Just like it happened with proposals, the button to select/deselect an investment wasn't very intuitive; for example, it wasn't obvious that pressing a button saying "selected" would deselect the investment. So we're using a switch control, like we do to enable/disable features since commit fabe97e50. Note that we're making the text of the switch smaller than in other places because the text in the investments table it is also smaller (we're using `font-size: inherit` for that purpose). That made the button look weird because we were using rems instead of ems for the width of the button, so we're adjusting that as well. Also note we're changing the width of the switch to `6em` instead of `6.25em` (which would be 100px if 1em is 16px). We're doing so because we used 100 for the minimum width because it's a round number, so now we're using another round number. --- .../admin/budget_investments/investments.scss | 4 ++ app/assets/stylesheets/mixins/buttons.scss | 2 +- .../toggle_selection_component.html.erb | 2 +- .../toggle_selection_component.rb | 27 ++++----- .../toggle_selection.js.erb | 6 +- config/locales/en/admin.yml | 1 - config/locales/es/admin.yml | 1 - .../toggle_selection_component_spec.rb | 12 ++-- spec/system/admin/budget_investments_spec.rb | 60 ++++++++++++------- 9 files changed, 68 insertions(+), 47 deletions(-) diff --git a/app/assets/stylesheets/admin/budget_investments/investments.scss b/app/assets/stylesheets/admin/budget_investments/investments.scss index 479c69fe9..5d931e6ff 100644 --- a/app/assets/stylesheets/admin/budget_investments/investments.scss +++ b/app/assets/stylesheets/admin/budget_investments/investments.scss @@ -12,4 +12,8 @@ font-size: $small-font-size; } } + + .toggle-switch [aria-pressed] { + font-size: inherit; + } } diff --git a/app/assets/stylesheets/mixins/buttons.scss b/app/assets/stylesheets/mixins/buttons.scss index ee09362ac..72691b9f1 100644 --- a/app/assets/stylesheets/mixins/buttons.scss +++ b/app/assets/stylesheets/mixins/buttons.scss @@ -85,7 +85,7 @@ @include regular-button; border-radius: $line-height; font-weight: bold; - min-width: rem-calc(100); + min-width: 6em; position: relative; &::after { diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index ffe677cc3..8981bf633 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,5 +1,5 @@ <% if can?(:toggle_selection, investment) %> - <%= button_to text, path, method: :patch, remote: true, class: html_class %> + <%= render Admin::ToggleSwitchComponent.new(action, investment, pressed: selected?, **options) %> <% elsif selected? %> <%= selected_text %> <% end %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb index 64d662fd5..5e5fa43e4 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.rb +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -9,18 +9,14 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent private - def text - if selected? - selected_text - else - t("admin.budget_investments.index.select") - end - end - def selected_text t("admin.budget_investments.index.selected") end + def action + :toggle_selection + end + def path toggle_selection_admin_budget_budget_investment_path( investment.budget, @@ -34,11 +30,14 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent ) end - def html_class - if selected? - "button small expanded" - else - "button small hollow expanded" - end + def options + { + "aria-label": label, + path: path + } + end + + def label + t("admin.actions.label", action: t("admin.actions.select"), name: investment.title) end end diff --git a/app/views/admin/budget_investments/toggle_selection.js.erb b/app/views/admin/budget_investments/toggle_selection.js.erb index 6a99f9783..98d258bad 100644 --- a/app/views/admin/budget_investments/toggle_selection.js.erb +++ b/app/views/admin/budget_investments/toggle_selection.js.erb @@ -1,3 +1,3 @@ -$("#<%= dom_id(@investment) %> [data-field='selected']").html( - "<%= j render(Admin::BudgetInvestments::ToggleSelectionComponent.new(@investment)) %>" -); +<%= render "admin/shared/toggle_switch", + record: @investment, + new_content: render(Admin::BudgetInvestments::ToggleSelectionComponent.new(@investment)) %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 589127371..433d2d10f 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -269,7 +269,6 @@ en: unfeasible: "Unfeasible" undecided: "Undecided" selected: "Selected" - select: "Select" list: id: ID title: Title diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 510753097..0e5f3bd0e 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -269,7 +269,6 @@ es: unfeasible: "Inviable" undecided: "Sin decidir" selected: "Seleccionado" - select: "Seleccionar" list: id: ID title: Título diff --git a/spec/components/admin/budget_investments/toggle_selection_component_spec.rb b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb index 954bee04b..aff7da09f 100644 --- a/spec/components/admin/budget_investments/toggle_selection_component_spec.rb +++ b/spec/components/admin/budget_investments/toggle_selection_component_spec.rb @@ -20,8 +20,9 @@ describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(valuation_finished_investment) - expect(page).to have_button "Select" - expect(page).not_to have_button "Selected" + expect(page).to have_button count: 1 + expect(page).to have_button exact_text: "No" + expect(page).to have_css "button[aria-pressed='false']" end it "renders a button to deselect selected investments" do @@ -29,8 +30,9 @@ describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) - expect(page).to have_button "Selected" - expect(page).not_to have_button "Select" + expect(page).to have_button count: 1 + expect(page).to have_button exact_text: "Yes" + expect(page).to have_css "button[aria-pressed='true']" end end @@ -58,7 +60,7 @@ describe Admin::BudgetInvestments::ToggleSelectionComponent, :admin do render_inline Admin::BudgetInvestments::ToggleSelectionComponent.new(selected_investment) expect(page).to have_content "Selected" - expect(page).not_to have_button "Selected" + expect(page).not_to have_button end end end diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 2df4ca0cf..90fda619c 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1412,9 +1412,14 @@ describe "Admin budget investments", :admin do expect(page).to have_content "Unfeasible project" - within("#budget_investment_#{feasible_vf_bi.id}") do - click_button "Select" - expect(page).to have_button "Selected" + within("tr", text: "Feasible, VF project") do + within("[data-field=selected]") do + expect(page).to have_content "No" + + click_button "Select Feasible, VF project" + + expect(page).to have_content "Yes" + end end click_link "Advanced filters" @@ -1424,9 +1429,10 @@ describe "Admin budget investments", :admin do expect(page).not_to have_content "Unfeasible project" - within("#budget_investment_#{feasible_vf_bi.id}") do - expect(page).not_to have_button "Select" - expect(page).to have_button "Selected" + within("tr", text: "Feasible, VF project") do + within("[data-field=selected]") do + expect(page).to have_content "Yes" + end end end @@ -1439,21 +1445,26 @@ describe "Admin budget investments", :admin do expect(page).to have_content("There are 2 investments") - within("#budget_investment_#{selected_bi.id}") do - click_button "Selected" + within("tr", text: "Selected project") do + within("[data-field=selected]") do + expect(page).to have_content "Yes" - expect(page).to have_button "Select" + click_button "Select Selected project" + + expect(page).to have_content "No" + end end - click_button("Filter") - expect(page).not_to have_content(selected_bi.title) - expect(page).to have_content("There is 1 investment") + click_button "Filter" + expect(page).not_to have_content "Selected project" + expect(page).to have_content "There is 1 investment" visit admin_budget_budget_investments_path(budget) - within("#budget_investment_#{selected_bi.id}") do - expect(page).to have_button "Select" - expect(page).not_to have_button "Selected" + within("tr", text: "Selected project") do + within("[data-field=selected]") do + expect(page).to have_content "No" + end end end @@ -1465,10 +1476,12 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) - within("#budget_investment_#{selected_bi.id}") do - click_button "Selected" + within("tr", text: "Selected project") do + within("[data-field=selected]") do + click_button "Select Selected project" - expect(page).to have_button "Select" + expect(page).to have_content "No" + end end click_link("Next") @@ -1806,11 +1819,16 @@ describe "Admin budget investments", :admin do within("#js-columns-selector-wrapper") { uncheck "Title" } within("#budget_investment_#{investment.id}") do - click_button "Selected" + within("[data-field=selected]") do + expect(page).to have_content "Yes" - expect(page).to have_button "Select" - expect(page).not_to have_content "Don't display me, please!" + click_button "Select Don't display me, please!" + + expect(page).to have_content "No" + end end + + expect(page).not_to have_content "Don't display me, please!" end scenario "When restoring the page from browser history renders columns selectors only once" do From 54a48d63e1b00cdcf9a9973e47809a9ec35aa1e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 9 Oct 2024 01:56:32 +0200 Subject: [PATCH 23/28] Use separate actions to select/deselect investments This is consistent to what we usually do. Also, we're applying the same criteria mentioned in commit 72704d776: > We're also making these actions idempotent, so sending many requests > to the same action will get the same result, which wasn't the case > with the `toggle` action. Although it's a low probability case, the > `toggle` action could result in [selecting an investment] when trying > to [deselect] it if someone else has [deselected it] it between the > time the page loaded and the time the admin clicked on the > "[Selected]" button. --- .../toggle_selection_component.html.erb | 2 +- .../toggle_selection_component.rb | 16 +++-- .../admin/budget_investments_controller.rb | 18 ++++-- app/models/abilities/administrator.rb | 2 +- config/routes/admin.rb | 5 +- .../budget_investments_controller_spec.rb | 61 ++++++++++++++++--- spec/models/abilities/administrator_spec.rb | 8 +-- 7 files changed, 87 insertions(+), 25 deletions(-) diff --git a/app/components/admin/budget_investments/toggle_selection_component.html.erb b/app/components/admin/budget_investments/toggle_selection_component.html.erb index 8981bf633..a417c6043 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.html.erb +++ b/app/components/admin/budget_investments/toggle_selection_component.html.erb @@ -1,4 +1,4 @@ -<% if can?(:toggle_selection, investment) %> +<% if can?(action, investment) %> <%= render Admin::ToggleSwitchComponent.new(action, investment, pressed: selected?, **options) %> <% elsif selected? %> <%= selected_text %> diff --git a/app/components/admin/budget_investments/toggle_selection_component.rb b/app/components/admin/budget_investments/toggle_selection_component.rb index 5e5fa43e4..faa54f518 100644 --- a/app/components/admin/budget_investments/toggle_selection_component.rb +++ b/app/components/admin/budget_investments/toggle_selection_component.rb @@ -14,20 +14,26 @@ class Admin::BudgetInvestments::ToggleSelectionComponent < ApplicationComponent end def action - :toggle_selection + if selected? + :deselect + else + :select + end end def path - toggle_selection_admin_budget_budget_investment_path( - investment.budget, - investment, + url_for({ + controller: "admin/budget_investments", + action: action, + budget_id: investment.budget, + id: investment, filter: params[:filter], sort_by: params[:sort_by], min_total_supports: params[:min_total_supports], max_total_supports: params[:max_total_supports], advanced_filters: params[:advanced_filters], page: params[:page] - ) + }) end def options diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index cb760d43b..a8e2b140e 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -9,7 +9,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController has_filters %w[all], only: :index before_action :load_budget - before_action :load_investment, only: [:show, :edit, :update, :toggle_selection] + before_action :load_investment, except: [:index] before_action :load_ballot, only: [:show, :index] before_action :parse_valuation_filters before_action :load_investments, only: :index @@ -60,10 +60,18 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end end - def toggle_selection - authorize! :toggle_selection, @investment - @investment.toggle :selected - @investment.save! + def select + authorize! :select, @investment + @investment.update!(selected: true) + + render :toggle_selection + end + + def deselect + authorize! :deselect, @investment + @investment.update!(selected: false) + + render :toggle_selection end private diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 82f6e48d7..5c9fb6dee 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -75,7 +75,7 @@ module Abilities can [:valuate, :comment_valuation], Budget::Investment cannot [:admin_update, :valuate, :comment_valuation], Budget::Investment, budget: { phase: "finished" } - can :toggle_selection, Budget::Investment do |investment| + can [:select, :deselect], Budget::Investment do |investment| investment.feasible? && investment.valuation_finished? && !investment.budget.finished? end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 442882bb5..b0c577675 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -66,7 +66,10 @@ namespace :admin do end resources :budget_investments, only: [:index, :show, :edit, :update] do - member { patch :toggle_selection } + member do + patch :select + patch :deselect + end resources :audits, only: :show, controller: "budget_investment_audits" resources :milestones, controller: "budget_investment_milestones" diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb index 7f7873192..8cee63ce0 100644 --- a/spec/controllers/admin/budget_investments_controller_spec.rb +++ b/spec/controllers/admin/budget_investments_controller_spec.rb @@ -38,18 +38,63 @@ describe Admin::BudgetInvestmentsController, :admin do end end - describe "PATCH toggle selection" do - it "uses the toggle_selection authorization rules" do - investment = create(:budget_investment) + describe "PATCH select" do + let(:investment) { create(:budget_investment, :feasible, :finished) } - patch :toggle_selection, xhr: true, params: { - id: investment, - budget_id: investment.budget, - } + it "selects the investment" do + expect do + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } + end.to change { investment.reload.selected? }.from(false).to(true) + + expect(response).to be_successful + end + + it "does not modify already selected investments" do + investment.update!(selected: true) + + expect do + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } + end.not_to change { investment.reload.selected? } + end + + it "uses the select/deselect authorization rules" do + investment.update!(valuation_finished: false) + + patch :select, xhr: true, params: { id: investment, budget_id: investment.budget } expect(flash[:alert]).to eq "You do not have permission to carry out the action " \ - "'toggle_selection' on Investment." + "'select' on Investment." expect(investment).not_to be_selected end end + + describe "PATCH deselect" do + let(:investment) { create(:budget_investment, :feasible, :finished, :selected) } + + it "deselects the investment" do + expect do + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + end.to change { investment.reload.selected? }.from(true).to(false) + + expect(response).to be_successful + end + + it "does not modify non-selected investments" do + investment.update!(selected: false) + + expect do + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + end.not_to change { investment.reload.selected? } + end + + it "uses the select/deselect authorization rules" do + investment.update!(valuation_finished: false) + + patch :deselect, xhr: true, params: { id: investment, budget_id: investment.budget } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action " \ + "'deselect' on Investment." + expect(investment).to be_selected + end + end end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index eab07520f..f75aac370 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -116,10 +116,10 @@ describe Abilities::Administrator do it { should_not be_able_to(:valuate, finished_investment) } it { should_not be_able_to(:comment_valuation, finished_investment) } - it { should be_able_to(:toggle_selection, create(:budget_investment, :feasible, :finished)) } - it { should_not be_able_to(:toggle_selection, create(:budget_investment, :feasible, :open)) } - it { should_not be_able_to(:toggle_selection, create(:budget_investment, :unfeasible, :finished)) } - it { should_not be_able_to(:toggle_selection, finished_investment) } + it { should be_able_to([:select, :deselect], create(:budget_investment, :feasible, :finished)) } + it { should_not be_able_to([:select, :deselect], create(:budget_investment, :feasible, :open)) } + it { should_not be_able_to([:select, :deselect], create(:budget_investment, :unfeasible, :finished)) } + it { should_not be_able_to([:select, :deselect], finished_investment) } it { should be_able_to(:destroy, proposal_image) } it { should be_able_to(:destroy, proposal_document) } From 173b1bb07ca34523f18c66a348d4ba40b0040404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 9 Oct 2024 13:03:27 +0200 Subject: [PATCH 24/28] Make it possible to select investments without JavaScript --- .../admin/budget_investments_controller.rb | 10 ++++++++-- .../budget_investments_controller_spec.rb | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index a8e2b140e..eec3583ac 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -64,14 +64,20 @@ class Admin::BudgetInvestmentsController < Admin::BaseController authorize! :select, @investment @investment.update!(selected: true) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.budget_investment") } + format.js { render :toggle_selection } + end end def deselect authorize! :deselect, @investment @investment.update!(selected: false) - render :toggle_selection + respond_to do |format| + format.html { redirect_to request.referer, notice: t("flash.actions.update.budget_investment") } + format.js { render :toggle_selection } + end end private diff --git a/spec/controllers/admin/budget_investments_controller_spec.rb b/spec/controllers/admin/budget_investments_controller_spec.rb index 8cee63ce0..54760c292 100644 --- a/spec/controllers/admin/budget_investments_controller_spec.rb +++ b/spec/controllers/admin/budget_investments_controller_spec.rb @@ -66,6 +66,15 @@ describe Admin::BudgetInvestmentsController, :admin do "'select' on Investment." expect(investment).not_to be_selected end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_budget_budget_investments_path(investment.budget) + + patch :select, 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 deselect" do @@ -96,5 +105,14 @@ describe Admin::BudgetInvestmentsController, :admin do "'deselect' on Investment." expect(investment).to be_selected end + + it "redirects admins without JavaScript to the same page" do + request.env["HTTP_REFERER"] = admin_budget_budget_investments_path(investment.budget) + + patch :deselect, 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 end From 958c13061f73d7e169db3dd90fab03a9a2034786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 21 Aug 2021 01:01:50 +0200 Subject: [PATCH 25/28] Fix duplicate HTML visible to valuator IDs --- .../budget_investments/row_component.html.erb | 2 +- spec/system/admin/budget_investments_spec.rb | 22 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index 49129a7c7..e7360e9eb 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -47,7 +47,7 @@ <%= f.check_box :visible_to_valuators, label: false, class: "js-submit-on-change", - id: "budget_investment_visible_to_valuators" %> + id: "budget_investment_visible_to_valuators_#{investment.id}" %> <% end %> <% else %> <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %> diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 90fda619c..12d4604ce 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1512,7 +1512,7 @@ describe "Admin budget investments", :admin do click_button "Filter" within("#budget_investment_#{investment1.id}") do - check "budget_investment_visible_to_valuators" + check "budget_investment[visible_to_valuators]" end visit admin_budget_budget_investments_path(budget) @@ -1521,7 +1521,7 @@ describe "Admin budget investments", :admin do click_button "Filter" within("#budget_investment_#{investment1.id}") do - expect(find("#budget_investment_visible_to_valuators")).to be_checked + expect(page).to have_field "budget_investment[visible_to_valuators]", checked: true end end @@ -1561,7 +1561,7 @@ describe "Admin budget investments", :admin do click_button "Filter" within("#budget_investment_#{investment1.id}") do - uncheck "budget_investment_visible_to_valuators" + uncheck "budget_investment[visible_to_valuators]" end visit admin_budget_budget_investments_path(budget) @@ -1571,7 +1571,7 @@ describe "Admin budget investments", :admin do click_button "Filter" within("#budget_investment_#{investment1.id}") do - expect(find("#budget_investment_visible_to_valuators")).not_to be_checked + expect(page).to have_field "budget_investment[visible_to_valuators]", checked: false end end @@ -1585,14 +1585,14 @@ describe "Admin budget investments", :admin do 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" + 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" + expect(page).not_to have_field "budget_investment[visible_to_valuators]" end end end @@ -1605,20 +1605,16 @@ describe "Admin budget investments", :admin do visit admin_budget_budget_investments_path(budget) - expect(page).to have_css("#budget_investment_visible_to_valuators") - click_link "Advanced filters" check "Under valuation" click_button "Filter" within("#budget_investment_#{investment1.id}") do - valuating_checkbox = find("#budget_investment_visible_to_valuators") - expect(valuating_checkbox).to be_checked + expect(page).to have_field "budget_investment[visible_to_valuators]", checked: true end within("#budget_investment_#{investment2.id}") do - valuating_checkbox = find("#budget_investment_visible_to_valuators") - expect(valuating_checkbox).not_to be_checked + expect(page).to have_field "budget_investment[visible_to_valuators]", checked: false end end @@ -1629,7 +1625,7 @@ 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" + check "budget_investment[visible_to_valuators]" end visit edit_admin_budget_budget_investment_path(budget, investment1) From 00d7299e9eebe529722752b4b0d18bacb4b5a82c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2024 18:04:44 +0200 Subject: [PATCH 26/28] Extract component for visible to valuators toggling --- .../budget_investments/row_component.html.erb | 11 +---------- .../admin/budget_investments/row_component.rb | 1 - .../toggle_visible_to_valuators_component.html.erb | 10 ++++++++++ .../toggle_visible_to_valuators_component.rb | 14 ++++++++++++++ .../admin/budget_investments/row_component_spec.rb | 9 --------- .../toggle_visible_to_valuators_component_spec.rb | 9 +++++++++ 6 files changed, 34 insertions(+), 20 deletions(-) create mode 100644 app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb create mode 100644 app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb delete mode 100644 spec/components/admin/budget_investments/row_component_spec.rb create mode 100644 spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb diff --git a/app/components/admin/budget_investments/row_component.html.erb b/app/components/admin/budget_investments/row_component.html.erb index e7360e9eb..3c191a726 100644 --- a/app/components/admin/budget_investments/row_component.html.erb +++ b/app/components/admin/budget_investments/row_component.html.erb @@ -42,16 +42,7 @@ - <% 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 %> - <% else %> - <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %> - <% end %> + <%= render Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent.new(investment) %> diff --git a/app/components/admin/budget_investments/row_component.rb b/app/components/admin/budget_investments/row_component.rb index 71a001d9c..c48156735 100644 --- a/app/components/admin/budget_investments/row_component.rb +++ b/app/components/admin/budget_investments/row_component.rb @@ -1,6 +1,5 @@ class Admin::BudgetInvestments::RowComponent < ApplicationComponent attr_reader :investment - use_helpers :can? def initialize(investment) @investment = investment 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 new file mode 100644 index 000000000..fda45a374 --- /dev/null +++ b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.html.erb @@ -0,0 +1,10 @@ +<% 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 %> +<% else %> + <%= investment.visible_to_valuators? ? t("shared.yes") : t("shared.no") %> +<% 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 new file mode 100644 index 000000000..c1cdb0e31 --- /dev/null +++ b/app/components/admin/budget_investments/toggle_visible_to_valuators_component.rb @@ -0,0 +1,14 @@ +class Admin::BudgetInvestments::ToggleVisibleToValuatorsComponent < ApplicationComponent + attr_reader :investment + use_helpers :can? + + def initialize(investment) + @investment = investment + end + + private + + def budget + investment.budget + end +end diff --git a/spec/components/admin/budget_investments/row_component_spec.rb b/spec/components/admin/budget_investments/row_component_spec.rb deleted file mode 100644 index 4f7dfab7c..000000000 --- a/spec/components/admin/budget_investments/row_component_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -require "rails_helper" - -describe Admin::BudgetInvestments::RowComponent, :admin do - it "uses a JSON request to update visible to valuators" do - render_inline Admin::BudgetInvestments::RowComponent.new(create(:budget_investment)) - - expect(page).to have_css "form[action$='json'] input[name$='[visible_to_valuators]']" - end -end 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 new file mode 100644 index 000000000..6bbe4a6d8 --- /dev/null +++ b/spec/components/admin/budget_investments/toggle_visible_to_valuators_component_spec.rb @@ -0,0 +1,9 @@ +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)) + + expect(page).to have_css "form[action$='json'] input[name$='[visible_to_valuators]']" + end +end From 76b0971b4a221d79a93fab3ed6415c1da433c398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 8 Oct 2024 15:40:18 +0200 Subject: [PATCH 27/28] Simplify mark as visible to valuators tests We were performing 3 requests in order to refresh the page and check the content was still there. We can use `refresh` instead. We're also reusing the `investment1` variable in every test, instead of redifining it in one of them. --- spec/system/admin/budget_investments_spec.rb | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 12d4604ce..36f45b17f 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1515,10 +1515,7 @@ describe "Admin budget investments", :admin do check "budget_investment[visible_to_valuators]" end - visit admin_budget_budget_investments_path(budget) - click_link "Advanced filters" - check "Under valuation" - click_button "Filter" + refresh within("#budget_investment_#{investment1.id}") do expect(page).to have_field "budget_investment[visible_to_valuators]", checked: true @@ -1564,11 +1561,7 @@ describe "Admin budget investments", :admin do uncheck "budget_investment[visible_to_valuators]" end - visit admin_budget_budget_investments_path(budget) - - click_link "Advanced filters" - check "Under valuation" - click_button "Filter" + refresh within("#budget_investment_#{investment1.id}") do expect(page).to have_field "budget_investment[visible_to_valuators]", checked: false @@ -1598,10 +1591,10 @@ describe "Admin budget investments", :admin do end scenario "Showing the valuating checkbox" do - investment1 = create(:budget_investment, :with_administrator, :with_valuator, :visible_to_valuators, - budget: budget) - investment2 = create(:budget_investment, :with_administrator, :with_valuator, :invisible_to_valuators, - budget: budget) + investment1.valuators << valuator + investment2.valuators << valuator + investment1.update!(administrator: admin, visible_to_valuators: true) + investment2.update!(administrator: admin, visible_to_valuators: false) visit admin_budget_budget_investments_path(budget) From fc5103881d78580c671ef9536b7c7160282befe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 8 Oct 2024 16:04:18 +0200 Subject: [PATCH 28/28] 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)