From 52ec55970b71ae71e7d84277d88ae88c9c0e386f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Aug 2021 01:24:40 +0200 Subject: [PATCH 01/17] Use buttons to send notifications and newsletters As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST requests to the server has a few issues. --- .../admin/admin_notifications/show.html.erb | 19 ++++++++++++------- app/views/admin/newsletters/show.html.erb | 15 +++++++++------ spec/system/admin/admin_notifications_spec.rb | 2 +- spec/system/admin/emails/newsletters_spec.rb | 4 ++-- spec/system/emails_spec.rb | 2 +- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/app/views/admin/admin_notifications/show.html.erb b/app/views/admin/admin_notifications/show.html.erb index ad2445edf..e78ecb875 100644 --- a/app/views/admin/admin_notifications/show.html.erb +++ b/app/views/admin/admin_notifications/show.html.erb @@ -64,12 +64,17 @@ <% if @admin_notification.draft? && @admin_notification.valid_segment_recipient? %>
- <%= link_to t("admin.admin_notifications.show.send"), - deliver_admin_admin_notification_path(@admin_notification), - "data-alert": t("admin.admin_notifications.show.send_alert", - n: @admin_notification.list_of_recipients_count), - method: :post, - id: "js-send-admin_notification-alert", - class: "button success expanded" %> + <%= render Admin::ActionComponent.new( + :deliver, + @admin_notification, + text: t("admin.admin_notifications.show.send"), + method: :post, + "data-alert": t( + "admin.admin_notifications.show.send_alert", + n: @admin_notification.list_of_recipients_count + ), + id: "js-send-admin_notification-alert", + class: "button success expanded", + ) %>
<% end %> diff --git a/app/views/admin/newsletters/show.html.erb b/app/views/admin/newsletters/show.html.erb index 3065b8bff..a9def3756 100644 --- a/app/views/admin/newsletters/show.html.erb +++ b/app/views/admin/newsletters/show.html.erb @@ -56,10 +56,13 @@ <% if @newsletter.draft? && @newsletter.valid_segment_recipient? %> - <%= link_to t("admin.newsletters.show.send"), - deliver_admin_newsletter_path(@newsletter), - "data-alert": t("admin.newsletters.show.send_alert", n: recipients_count), - method: :post, - id: "js-send-newsletter-alert", - class: "button success" %> + <%= render Admin::ActionComponent.new( + :deliver, + @newsletter, + text: t("admin.newsletters.show.send"), + method: :post, + "data-alert": t("admin.newsletters.show.send_alert", n: recipients_count), + id: "js-send-newsletter-alert", + class: "button success" + ) %> <% end %> diff --git a/spec/system/admin/admin_notifications_spec.rb b/spec/system/admin/admin_notifications_spec.rb index 52b881f9c..0f15ce649 100644 --- a/spec/system/admin/admin_notifications_spec.rb +++ b/spec/system/admin/admin_notifications_spec.rb @@ -190,7 +190,7 @@ describe "Admin Notifications", :admin do visit admin_admin_notification_path(notification) - accept_confirm { click_link "Send notification" } + accept_confirm { click_button "Send notification" } expect(page).to have_content "Notification sent successfully" diff --git a/spec/system/admin/emails/newsletters_spec.rb b/spec/system/admin/emails/newsletters_spec.rb index ed5dd7d08..5d8101c0f 100644 --- a/spec/system/admin/emails/newsletters_spec.rb +++ b/spec/system/admin/emails/newsletters_spec.rb @@ -134,7 +134,7 @@ describe "Admin newsletter emails", :admin do newsletter = create(:newsletter) visit admin_newsletter_path(newsletter) - accept_confirm { click_link "Send" } + accept_confirm { click_button "Send" } expect(page).to have_content "Newsletter sent successfully" end @@ -153,7 +153,7 @@ describe "Admin newsletter emails", :admin do newsletter = create(:newsletter, segment_recipient: "administrators") visit admin_newsletter_path(newsletter) - accept_confirm { click_link "Send" } + accept_confirm { click_button "Send" } expect(page).to have_content "Newsletter sent successfully" diff --git a/spec/system/emails_spec.rb b/spec/system/emails_spec.rb index 5d488053f..592aceb8d 100644 --- a/spec/system/emails_spec.rb +++ b/spec/system/emails_spec.rb @@ -482,7 +482,7 @@ describe "Emails" do expect(page).to have_content "Newsletter created successfully" - accept_confirm { click_link "Send" } + accept_confirm { click_button "Send" } expect(page).to have_content "Newsletter sent successfully" From cb3bea8eec14d286e33c592362342a07a04ae089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 04:16:28 +0100 Subject: [PATCH 02/17] Simplify code to ask for send newsletter confirmation Using the standard `confirm` parameter, we can remove all the custom code we added to do the same thing. Since the code is similar, we're doing the same when asking for confirmation to send notifications. --- app/assets/javascripts/application.js | 4 ---- .../javascripts/send_admin_notification_alert.js | 10 ---------- app/assets/javascripts/send_newsletter_alert.js | 10 ---------- app/views/admin/admin_notifications/show.html.erb | 3 +-- app/views/admin/newsletters/show.html.erb | 3 +-- 5 files changed, 2 insertions(+), 28 deletions(-) delete mode 100644 app/assets/javascripts/send_admin_notification_alert.js delete mode 100644 app/assets/javascripts/send_newsletter_alert.js diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 449ab33dc..86a0f8243 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -110,11 +110,9 @@ //= require sortable //= require table_sortable //= require investment_report_alert -//= require send_newsletter_alert //= require managers //= require i18n //= require globalize -//= require send_admin_notification_alert //= require settings //= require cookies //= require columns_selector @@ -166,10 +164,8 @@ var initialize_modules = function() { App.Sortable.initialize(); App.TableSortable.initialize(); App.InvestmentReportAlert.initialize(); - App.SendNewsletterAlert.initialize(); App.Managers.initialize(); App.Globalize.initialize(); - App.SendAdminNotificationAlert.initialize(); App.Settings.initialize(); if ($("#js-columns-selector").length) { App.ColumnsSelector.initialize(); diff --git a/app/assets/javascripts/send_admin_notification_alert.js b/app/assets/javascripts/send_admin_notification_alert.js deleted file mode 100644 index 6dc7b36f2..000000000 --- a/app/assets/javascripts/send_admin_notification_alert.js +++ /dev/null @@ -1,10 +0,0 @@ -(function() { - "use strict"; - App.SendAdminNotificationAlert = { - initialize: function() { - $("#js-send-admin_notification-alert").on("click", function() { - return confirm(this.dataset.alert); - }); - } - }; -}).call(this); diff --git a/app/assets/javascripts/send_newsletter_alert.js b/app/assets/javascripts/send_newsletter_alert.js deleted file mode 100644 index fa5a605af..000000000 --- a/app/assets/javascripts/send_newsletter_alert.js +++ /dev/null @@ -1,10 +0,0 @@ -(function() { - "use strict"; - App.SendNewsletterAlert = { - initialize: function() { - $("#js-send-newsletter-alert").on("click", function() { - return confirm(this.dataset.alert); - }); - } - }; -}).call(this); diff --git a/app/views/admin/admin_notifications/show.html.erb b/app/views/admin/admin_notifications/show.html.erb index e78ecb875..bf30a0aa3 100644 --- a/app/views/admin/admin_notifications/show.html.erb +++ b/app/views/admin/admin_notifications/show.html.erb @@ -69,11 +69,10 @@ @admin_notification, text: t("admin.admin_notifications.show.send"), method: :post, - "data-alert": t( + confirm: t( "admin.admin_notifications.show.send_alert", n: @admin_notification.list_of_recipients_count ), - id: "js-send-admin_notification-alert", class: "button success expanded", ) %> diff --git a/app/views/admin/newsletters/show.html.erb b/app/views/admin/newsletters/show.html.erb index a9def3756..b8a575dd1 100644 --- a/app/views/admin/newsletters/show.html.erb +++ b/app/views/admin/newsletters/show.html.erb @@ -61,8 +61,7 @@ @newsletter, text: t("admin.newsletters.show.send"), method: :post, - "data-alert": t("admin.newsletters.show.send_alert", n: recipients_count), - id: "js-send-newsletter-alert", + confirm: t("admin.newsletters.show.send_alert", n: recipients_count), class: "button success" ) %> <% end %> From 20d3725709f99993d74ac992d3cdc040ed78d51c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 21 Mar 2024 02:09:15 +0100 Subject: [PATCH 03/17] Add missing test to delete a draft version We weren't testing this action anywhere. --- spec/system/admin/legislation/draft_versions_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index d36daf6e2..f5957d2fd 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -74,6 +74,15 @@ describe "Admin legislation draft versions", :admin do end end + scenario "Delete" do + version = create(:legislation_draft_version, body: "Version 1") + + visit edit_admin_legislation_process_draft_version_path(version.process, version) + click_link "Delete" + + expect(page).to have_content "Draft deleted successfully" + end + context "Changing content with the markdown editor" do let(:prompt) { "You've edited the text without saving it. Do you confirm to leave the page?" } let(:version) { create(:legislation_draft_version, body: "Version 1") } From ecad046a99d8a34c0a218f2bd75062cefa3b213e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 04:53:15 +0100 Subject: [PATCH 04/17] Use buttons to destroy drafts and questions As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, DELETE) requests to the server has a few issues. --- .../admin/legislation/draft_versions/edit.html.erb | 10 ++++++---- app/views/admin/legislation/questions/edit.html.erb | 9 ++++++--- config/initializers/routes_hierarchy.rb | 6 +++--- config/locales/en/admin.yml | 2 -- config/locales/es/admin.yml | 2 -- config/routes/admin.rb | 4 ++++ spec/system/admin/legislation/draft_versions_spec.rb | 2 +- spec/system/admin/legislation/questions_spec.rb | 2 +- 8 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/views/admin/legislation/draft_versions/edit.html.erb b/app/views/admin/legislation/draft_versions/edit.html.erb index a49b9dbd2..a9fadfdc8 100644 --- a/app/views/admin/legislation/draft_versions/edit.html.erb +++ b/app/views/admin/legislation/draft_versions/edit.html.erb @@ -15,10 +15,12 @@

<%= @draft_version.title %>

- <%= link_to t("admin.legislation.draft_versions.index.delete"), - admin_legislation_process_draft_version_path(@process, @draft_version), - method: :delete, - class: "button hollow alert" %> + <%= render Admin::ActionComponent.new( + :destroy, + @draft_version, + method: :delete, + class: "button hollow alert" + ) %>
diff --git a/app/views/admin/legislation/questions/edit.html.erb b/app/views/admin/legislation/questions/edit.html.erb index 3151efa90..c98ce4eaf 100644 --- a/app/views/admin/legislation/questions/edit.html.erb +++ b/app/views/admin/legislation/questions/edit.html.erb @@ -15,9 +15,12 @@

<%= t("admin.legislation.questions.edit.title", question_title: @question.title) %>

- <%= link_to t("admin.legislation.questions.index.delete"), admin_legislation_process_question_path(@process, @question), - method: :delete, - class: "button hollow alert" %> + <%= render Admin::ActionComponent.new( + :destroy, + @question, + method: :delete, + class: "button hollow alert" + ) %>
diff --git a/config/initializers/routes_hierarchy.rb b/config/initializers/routes_hierarchy.rb index e66c8386c..298805088 100644 --- a/config/initializers/routes_hierarchy.rb +++ b/config/initializers/routes_hierarchy.rb @@ -24,9 +24,9 @@ module ActionDispatch::Routing::UrlFor end def namespaced_polymorphic_path(namespace, resource, options = {}) - if %w[Budget::Group Budget::Heading Poll::Booth Poll::BoothAssignment Poll::Officer - Poll::Question Poll::Question::Answer Poll::Question::Answer::Video Poll::Shift - SDG::LocalTarget].include?(resource.class.name) + if %w[Budget::Group Budget::Heading Legislation::DraftVersion Legislation::Question + Poll::Booth Poll::BoothAssignment Poll::Officer Poll::Question Poll::Question::Answer + Poll::Question::Answer::Video Poll::Shift SDG::LocalTarget].include?(resource.class.name) resolve = resolve_for(resource) resolve_options = resolve.pop diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 95f85c628..a7724c856 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -664,7 +664,6 @@ en: index: title: Draft versions create: Create version - delete: Delete preview: Preview new: back: Back @@ -697,7 +696,6 @@ en: back: Back title: Questions associated to this process create: Create question - delete: Delete new: back: Back title: Create new question diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index e7b7bb600..b1c80ee13 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -664,7 +664,6 @@ es: index: title: Versiones del borrador create: Crear versión - delete: Borrar preview: Previsualizar new: back: Volver @@ -697,7 +696,6 @@ es: back: Volver title: Preguntas asociadas a este proceso create: Crear pregunta - delete: Borrar new: back: Volver title: Crear nueva pregunta diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 7d97f99e3..5ffbf5eba 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -346,3 +346,7 @@ end resolve "Poll::Question::Answer::Video" do |video, options| [:answer, :video, options.merge(answer_id: video.answer, id: video)] end + +resolve "Legislation::DraftVersion" do |version, options| + [version.process, :draft_version, options.merge(id: version)] +end diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index f5957d2fd..2e5f8034e 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -78,7 +78,7 @@ describe "Admin legislation draft versions", :admin do version = create(:legislation_draft_version, body: "Version 1") visit edit_admin_legislation_process_draft_version_path(version.process, version) - click_link "Delete" + click_button "Delete" expect(page).to have_content "Draft deleted successfully" end diff --git a/spec/system/admin/legislation/questions_spec.rb b/spec/system/admin/legislation/questions_spec.rb index ed39d2f55..20c8fb1a7 100644 --- a/spec/system/admin/legislation/questions_spec.rb +++ b/spec/system/admin/legislation/questions_spec.rb @@ -75,7 +75,7 @@ describe "Admin legislation questions", :admin do visit edit_admin_legislation_process_question_path(process, question) - click_link "Delete" + click_button "Delete" expect(page).to have_content "Questions" expect(page).to have_content "Question 1" From 58767383699bd0ce734725f72fcdba2ef490c582 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 05:00:03 +0100 Subject: [PATCH 05/17] Ask confirmation to delete drafts and questions This is similar to what we do in almost every other page of the admin section. --- app/views/admin/legislation/draft_versions/edit.html.erb | 1 + app/views/admin/legislation/questions/edit.html.erb | 1 + spec/system/admin/legislation/draft_versions_spec.rb | 5 ++++- spec/system/admin/legislation/questions_spec.rb | 4 +++- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/views/admin/legislation/draft_versions/edit.html.erb b/app/views/admin/legislation/draft_versions/edit.html.erb index a9fadfdc8..efdde4e27 100644 --- a/app/views/admin/legislation/draft_versions/edit.html.erb +++ b/app/views/admin/legislation/draft_versions/edit.html.erb @@ -19,6 +19,7 @@ :destroy, @draft_version, method: :delete, + confirm: true, class: "button hollow alert" ) %> diff --git a/app/views/admin/legislation/questions/edit.html.erb b/app/views/admin/legislation/questions/edit.html.erb index c98ce4eaf..ec51522ab 100644 --- a/app/views/admin/legislation/questions/edit.html.erb +++ b/app/views/admin/legislation/questions/edit.html.erb @@ -19,6 +19,7 @@ :destroy, @question, method: :delete, + confirm: true, class: "button hollow alert" ) %> diff --git a/spec/system/admin/legislation/draft_versions_spec.rb b/spec/system/admin/legislation/draft_versions_spec.rb index 2e5f8034e..642a91f58 100644 --- a/spec/system/admin/legislation/draft_versions_spec.rb +++ b/spec/system/admin/legislation/draft_versions_spec.rb @@ -78,7 +78,10 @@ describe "Admin legislation draft versions", :admin do version = create(:legislation_draft_version, body: "Version 1") visit edit_admin_legislation_process_draft_version_path(version.process, version) - click_button "Delete" + + accept_confirm("Are you sure? This action will delete \"Version 1\" and can't be undone.") do + click_button "Delete" + end expect(page).to have_content "Draft deleted successfully" end diff --git a/spec/system/admin/legislation/questions_spec.rb b/spec/system/admin/legislation/questions_spec.rb index 20c8fb1a7..45b39d088 100644 --- a/spec/system/admin/legislation/questions_spec.rb +++ b/spec/system/admin/legislation/questions_spec.rb @@ -75,7 +75,9 @@ describe "Admin legislation questions", :admin do visit edit_admin_legislation_process_question_path(process, question) - click_button "Delete" + accept_confirm("Are you sure? This action will delete \"Question 2\" and can't be undone.") do + click_button "Delete" + end expect(page).to have_content "Questions" expect(page).to have_content "Question 1" From 53d85d6431765a7d2218b9a3b30e81358ee69fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 05:06:24 +0100 Subject: [PATCH 06/17] Use a button to destroy officials As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, DELETE) requests to the server has a few issues. --- app/assets/stylesheets/admin/action.scss | 4 ++++ app/views/admin/officials/edit.html.erb | 16 ++++++++++++---- spec/system/admin/officials_spec.rb | 2 +- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/admin/action.scss b/app/assets/stylesheets/admin/action.scss index cca144bbb..abdba6794 100644 --- a/app/assets/stylesheets/admin/action.scss +++ b/app/assets/stylesheets/admin/action.scss @@ -3,4 +3,8 @@ &[disabled] { @include button-disabled; } + + &.delete { + cursor: pointer; + } } diff --git a/app/views/admin/officials/edit.html.erb b/app/views/admin/officials/edit.html.erb index 40e428e49..c03c77c65 100644 --- a/app/views/admin/officials/edit.html.erb +++ b/app/views/admin/officials/edit.html.erb @@ -14,9 +14,17 @@ <%= f.text_field :official_position %> <%= f.select :official_level, official_level_options %> <%= f.submit class: "button" %> - <% if @user.official? %> -
- <%= link_to t("admin.officials.edit.destroy"), admin_official_path(@user), method: :delete, class: "delete" %> - <% end %> + <% end %> + + <% if @user.official? %> +
+ <%= render Admin::ActionComponent.new( + :destroy, + @user, + text: t("admin.officials.edit.destroy"), + path: admin_official_path(@user), + method: :delete, + class: "delete" + ) %> <% end %> diff --git a/spec/system/admin/officials_spec.rb b/spec/system/admin/officials_spec.rb index c71dfe928..cf94a541d 100644 --- a/spec/system/admin/officials_spec.rb +++ b/spec/system/admin/officials_spec.rb @@ -63,7 +63,7 @@ describe "Admin officials", :admin do scenario "Destroy" do visit edit_admin_official_path(official) - click_link 'Remove "Official" status' + click_button 'Remove "Official" status' expect(page).to have_content "Details saved: the user is no longer an official" expect(page).to have_current_path(admin_officials_path, ignore_query: true) From d050c04bb0dca066d59b9ffc8f82e83113743909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 05:05:29 +0100 Subject: [PATCH 07/17] Use a button to destroy poll question answer images As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, DELETE) requests to the server has a few issues. Note that the AJAX response stopped working after replacing the link with a button. Not sure about the reason, but, since this is one of the very few places where we use AJAX calls to delete content, the easiest solution is to stop using AJAX and be consistent with what we do in the rest of the admin section. --- .../poll/questions/answers/images_controller.rb | 5 ++--- .../poll/questions/answers/images/destroy.js.erb | 3 --- .../poll/questions/answers/images/index.html.erb | 14 ++++++++------ config/locales/en/responders.yml | 1 + config/locales/es/responders.yml | 1 + .../poll/questions/answers/images/images_spec.rb | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) delete mode 100644 app/views/admin/poll/questions/answers/images/destroy.js.erb diff --git a/app/controllers/admin/poll/questions/answers/images_controller.rb b/app/controllers/admin/poll/questions/answers/images_controller.rb index 48b843d72..768f8a532 100644 --- a/app/controllers/admin/poll/questions/answers/images_controller.rb +++ b/app/controllers/admin/poll/questions/answers/images_controller.rb @@ -25,9 +25,8 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr def destroy @image.destroy! - respond_to do |format| - format.js { render layout: false } - end + redirect_to admin_answer_images_path(@image.imageable), + notice: t("flash.actions.destroy.poll_question_answer_image") end private diff --git a/app/views/admin/poll/questions/answers/images/destroy.js.erb b/app/views/admin/poll/questions/answers/images/destroy.js.erb deleted file mode 100644 index 137451dda..000000000 --- a/app/views/admin/poll/questions/answers/images/destroy.js.erb +++ /dev/null @@ -1,3 +0,0 @@ -$(".delete").on("ajax:success", function () { - $(this).closest("div").fadeOut(); -}); diff --git a/app/views/admin/poll/questions/answers/images/index.html.erb b/app/views/admin/poll/questions/answers/images/index.html.erb index e3f2b3e92..957c56d76 100644 --- a/app/views/admin/poll/questions/answers/images/index.html.erb +++ b/app/views/admin/poll/questions/answers/images/index.html.erb @@ -26,12 +26,14 @@ <%= render_image(image, :large, true) if image.present? %> <% if can?(:destroy, image) %> - <%= link_to t("images.remove_image"), - admin_image_path(image), - class: "delete float-right", - method: :delete, - remote: true, - data: { confirm: t("admin.actions.confirm_action", action: t("images.remove_image"), name: image.title) } %> + <%= render Admin::ActionComponent.new( + :destroy, + image, + text: t("images.remove_image"), + method: :delete, + confirm: t("admin.actions.confirm_action", action: t("images.remove_image"), name: image.title), + class: "delete float-right" + ) %> <% end %> <% end %> diff --git a/config/locales/en/responders.yml b/config/locales/en/responders.yml index 31e9350b0..b4d2799ce 100644 --- a/config/locales/en/responders.yml +++ b/config/locales/en/responders.yml @@ -34,6 +34,7 @@ en: destroy: budget_investment: "Investment project deleted successfully." topic: "Topic deleted successfully." + poll_question_answer_image: "Image deleted successfully." poll_question_answer_video: "Answer video deleted successfully." valuator_group: "Valuator group deleted successfully" vote: "Vote deleted successfully" diff --git a/config/locales/es/responders.yml b/config/locales/es/responders.yml index f35f42a96..8928540e5 100644 --- a/config/locales/es/responders.yml +++ b/config/locales/es/responders.yml @@ -34,6 +34,7 @@ es: destroy: budget_investment: "Proyecto de gasto eliminado." topic: "Tema eliminado." + poll_question_answer_image: "Imagen eliminada correctamente." poll_question_answer_video: "Vídeo de respuesta eliminado." valuator_group: "Grupo de evaluadores eliminado correctamente" vote: "Voto eliminado correctamente" diff --git a/spec/system/admin/poll/questions/answers/images/images_spec.rb b/spec/system/admin/poll/questions/answers/images/images_spec.rb index cfbc12293..a6e35acc8 100644 --- a/spec/system/admin/poll/questions/answers/images/images_spec.rb +++ b/spec/system/admin/poll/questions/answers/images/images_spec.rb @@ -73,7 +73,7 @@ describe "Images", :admin do expect(page).to have_content image.title accept_confirm "Are you sure? Remove image \"#{image.title}\"" do - click_link "Remove image" + click_button "Remove image" end expect(page).not_to have_css "img[title='#{image.title}']" @@ -88,7 +88,7 @@ describe "Images", :admin do expect(page).to have_css "img[title='#{image.title}']" expect(page).to have_content image.title - expect(page).not_to have_link "Remove image" + expect(page).not_to have_content "Remove image" expect(page).to have_content "Once the poll has started it will not be possible to create, edit or" end end From 5a7021396ea70751f172d823aa7a98090d2e9c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 14:05:17 +0100 Subject: [PATCH 08/17] Use a button to destroy content blocks from the edit page We were already using button to destroy content blocks from the content blocks index. As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, DELETE) requests to the server has a few issues. --- .../site_customization/content_blocks/edit.html.erb | 12 ++++++++---- .../admin/site_customization/content_blocks_spec.rb | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/views/admin/site_customization/content_blocks/edit.html.erb b/app/views/admin/site_customization/content_blocks/edit.html.erb index 0a66bd5d5..5fa818c7d 100644 --- a/app/views/admin/site_customization/content_blocks/edit.html.erb +++ b/app/views/admin/site_customization/content_blocks/edit.html.erb @@ -4,10 +4,14 @@ <%= back_link_to admin_site_customization_content_blocks_path %> -<%= link_to t("admin.site_customization.content_blocks.index.delete"), - (@is_heading_content_block ? admin_site_customization_delete_heading_content_block_path(@content_block.id) : admin_site_customization_content_block_path(@content_block)), - method: :delete, - class: "delete float-right" %> +<%= render Admin::ActionComponent.new( + :destroy, + @content_block, + text: t("admin.site_customization.content_blocks.index.delete"), + path: (@is_heading_content_block ? admin_site_customization_delete_heading_content_block_path(@content_block.id) : admin_site_customization_content_block_path(@content_block)), + method: :delete, + class: "delete float-right" +) %>

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

diff --git a/spec/system/admin/site_customization/content_blocks_spec.rb b/spec/system/admin/site_customization/content_blocks_spec.rb index b9e49774a..42c125769 100644 --- a/spec/system/admin/site_customization/content_blocks_spec.rb +++ b/spec/system/admin/site_customization/content_blocks_spec.rb @@ -103,7 +103,7 @@ describe "Admin custom content blocks", :admin do block = create(:site_customization_content_block) visit edit_admin_site_customization_content_block_path(block) - click_link "Delete block" + click_button "Delete block" expect(page).not_to have_content("#{block.name} (#{block.locale})") expect(page).not_to have_content(block.body) From 6a2ee921de6a363d52f1ebdd1dc89e5db28235a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 14:06:51 +0100 Subject: [PATCH 09/17] Ask confirmation to delete content blocks from the edit page We were already doing that when deleting content blocks from the index page, and we also ask for confirmation in almost every page in the admin section. --- .../admin/site_customization/content_blocks/edit.html.erb | 1 + spec/system/admin/site_customization/content_blocks_spec.rb | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/views/admin/site_customization/content_blocks/edit.html.erb b/app/views/admin/site_customization/content_blocks/edit.html.erb index 5fa818c7d..bdc15f1f8 100644 --- a/app/views/admin/site_customization/content_blocks/edit.html.erb +++ b/app/views/admin/site_customization/content_blocks/edit.html.erb @@ -9,6 +9,7 @@ @content_block, text: t("admin.site_customization.content_blocks.index.delete"), path: (@is_heading_content_block ? admin_site_customization_delete_heading_content_block_path(@content_block.id) : admin_site_customization_content_block_path(@content_block)), + confirm: true, method: :delete, class: "delete float-right" ) %> diff --git a/spec/system/admin/site_customization/content_blocks_spec.rb b/spec/system/admin/site_customization/content_blocks_spec.rb index 42c125769..abbefc138 100644 --- a/spec/system/admin/site_customization/content_blocks_spec.rb +++ b/spec/system/admin/site_customization/content_blocks_spec.rb @@ -103,7 +103,9 @@ describe "Admin custom content blocks", :admin do block = create(:site_customization_content_block) visit edit_admin_site_customization_content_block_path(block) - click_button "Delete block" + accept_confirm("Are you sure? This action will delete \"#{block.name}\" and can't be undone.") do + click_button "Delete block" + end expect(page).not_to have_content("#{block.name} (#{block.locale})") expect(page).not_to have_content(block.body) From 62aad851bf6c94e256b5d2824ec4f2a74c638b0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 14:07:39 +0100 Subject: [PATCH 10/17] Use icons as links to edit content blocks Just like we do with the rest of the tables in the admin section. --- .../content_blocks/index.html.erb | 8 ++++---- .../site_customization/content_blocks_spec.rb | 16 +++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/views/admin/site_customization/content_blocks/index.html.erb b/app/views/admin/site_customization/content_blocks/index.html.erb index 03fd96248..05cda15a7 100644 --- a/app/views/admin/site_customization/content_blocks/index.html.erb +++ b/app/views/admin/site_customization/content_blocks/index.html.erb @@ -34,21 +34,21 @@ <% @content_blocks.each do |content_block| %> - <%= link_to "#{content_block.name} (#{content_block.locale})", edit_admin_site_customization_content_block_path(content_block) %> + <%= "#{content_block.name} (#{content_block.locale})" %> <%= raw content_block.body %> - <%= render Admin::TableActionsComponent.new(content_block, actions: [:destroy]) %> + <%= render Admin::TableActionsComponent.new(content_block) %> <% end %> <% @headings_content_blocks.each do |content_block| %> - <%= link_to "#{content_block.name} (#{content_block.locale})", admin_site_customization_edit_heading_content_block_path(content_block) %> + <%= "#{content_block.name} (#{content_block.locale})" %> <%= raw content_block.body %> <%= render Admin::TableActionsComponent.new( content_block, - actions: [:destroy], + edit_path: admin_site_customization_edit_heading_content_block_path(content_block), destroy_path: admin_site_customization_delete_heading_content_block_path(content_block) ) %> diff --git a/spec/system/admin/site_customization/content_blocks_spec.rb b/spec/system/admin/site_customization/content_blocks_spec.rb index abbefc138..012fc28ee 100644 --- a/spec/system/admin/site_customization/content_blocks_spec.rb +++ b/spec/system/admin/site_customization/content_blocks_spec.rb @@ -2,13 +2,19 @@ require "rails_helper" describe "Admin custom content blocks", :admin do scenario "Index" do - block = create(:site_customization_content_block) - heading_block = create(:heading_content_block) + block = create(:site_customization_content_block, name: "top_links") + heading_block = create(:heading_content_block, heading: create(:budget_heading, name: "Reforestation")) visit admin_site_customization_content_blocks_path - expect(page).to have_content(block.name) + within "tr", text: "top_links" do + expect(page).to have_link "Edit" + end + + within "tr", text: "Reforestation" do + expect(page).to have_link "Edit" + end + expect(page).to have_content(block.body) - expect(page).to have_content(heading_block.heading.name) expect(page).to have_content(heading_block.body) end @@ -73,7 +79,7 @@ describe "Admin custom content blocks", :admin do click_link "Custom content blocks" end - click_link "top_links (en)" + within("tr", text: "top_links (en)") { click_link "Edit" } fill_in "site_customization_content_block_body", with: "Some other custom content" click_button "Update Custom content block" From fc4940ccb699fe2b581b2c4a3e0dd2876e8fce20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 31 Mar 2024 18:45:50 +0200 Subject: [PATCH 11/17] Move edit page and new page views to components This way we can simplify setting the title and styling the link in the header. We're also fixing the unnecessary padding introduced by the `column` classes, which caused the header not to be aligned with the rest of the elements surrounding it. We're still keeping it the margin used in the `row` classes so it's aligned with the rest of the form; ideally, we would remove the `row` classes in the rest of the form and in the whole admin section, but this isn't something we can tackle right now. Note that, in the CSS, the `margin-left: auto` property needs to be included after `@include regular-button` because that mixin overwrites the `margin-left` property. Since we're modifying this code, we're making it compatible with RTL text, using `$global-left` instead of `left`. --- app/assets/stylesheets/admin.scss | 17 +++++++++++++++-- .../pages/edit_component.html.erb | 8 ++++++++ .../site_customization/pages/edit_component.rb | 12 ++++++++++++ .../pages/new_component.html.erb | 6 ++++++ .../site_customization/pages/new_component.rb | 12 ++++++++++++ .../site_customization/pages/edit.html.erb | 15 +-------------- .../admin/site_customization/pages/new.html.erb | 13 +------------ 7 files changed, 55 insertions(+), 28 deletions(-) create mode 100644 app/components/admin/site_customization/pages/edit_component.html.erb create mode 100644 app/components/admin/site_customization/pages/edit_component.rb create mode 100644 app/components/admin/site_customization/pages/new_component.html.erb create mode 100644 app/components/admin/site_customization/pages/new_component.rb diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index b19590a97..ed3f0dda5 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -31,6 +31,15 @@ $table-header: #ecf1f6; .admin { @include admin-layout; + main { + &.admin-site-customization-pages-new, + &.admin-site-customization-pages-edit { + > header { + @include grid-row; + } + } + } + h2 { font-weight: 100; margin-bottom: $line-height; @@ -503,8 +512,12 @@ code { a, button { - @include regular-button; - margin-left: auto; + margin-#{$global-left}: auto; + + &:not(.delete) { + @include regular-button; + margin-#{$global-left}: auto; + } } } diff --git a/app/components/admin/site_customization/pages/edit_component.html.erb b/app/components/admin/site_customization/pages/edit_component.html.erb new file mode 100644 index 000000000..327bbcbc7 --- /dev/null +++ b/app/components/admin/site_customization/pages/edit_component.html.erb @@ -0,0 +1,8 @@ +<% provide :main_class, "admin-site-customization-pages-edit" %> +<%= back_link_to admin_site_customization_pages_path %> + +<%= header do %> + <%= link_to t("admin.site_customization.pages.index.delete"), admin_site_customization_page_path(page), method: :delete, class: "delete" %> +<% end %> + +<%= render "form" %> diff --git a/app/components/admin/site_customization/pages/edit_component.rb b/app/components/admin/site_customization/pages/edit_component.rb new file mode 100644 index 000000000..0e1cf20f4 --- /dev/null +++ b/app/components/admin/site_customization/pages/edit_component.rb @@ -0,0 +1,12 @@ +class Admin::SiteCustomization::Pages::EditComponent < ApplicationComponent + include Header + attr_reader :page + + def initialize(page) + @page = page + end + + def title + t("admin.site_customization.pages.edit.title", page_title: page.title) + end +end diff --git a/app/components/admin/site_customization/pages/new_component.html.erb b/app/components/admin/site_customization/pages/new_component.html.erb new file mode 100644 index 000000000..aee470c74 --- /dev/null +++ b/app/components/admin/site_customization/pages/new_component.html.erb @@ -0,0 +1,6 @@ +<% provide :main_class, "admin-site-customization-pages-new" %> +<%= back_link_to admin_site_customization_pages_path %> + +<%= header %> + +<%= render "form" %> diff --git a/app/components/admin/site_customization/pages/new_component.rb b/app/components/admin/site_customization/pages/new_component.rb new file mode 100644 index 000000000..74c90de79 --- /dev/null +++ b/app/components/admin/site_customization/pages/new_component.rb @@ -0,0 +1,12 @@ +class Admin::SiteCustomization::Pages::NewComponent < ApplicationComponent + include Header + attr_reader :page + + def initialize(page) + @page = page + end + + def title + t("admin.site_customization.pages.new.title") + end +end diff --git a/app/views/admin/site_customization/pages/edit.html.erb b/app/views/admin/site_customization/pages/edit.html.erb index 1965b636f..9484633df 100644 --- a/app/views/admin/site_customization/pages/edit.html.erb +++ b/app/views/admin/site_customization/pages/edit.html.erb @@ -1,14 +1 @@ -<% provide :title do %> - <%= t("admin.header.title") %> - <%= t("admin.menu.site_customization.pages") %> - <%= @page.title %> -<% end %> - -<%= back_link_to admin_site_customization_pages_path %> -
-
-

<%= t("admin.site_customization.pages.edit.title", page_title: @page.title) %>

- - <%= link_to t("admin.site_customization.pages.index.delete"), admin_site_customization_page_path(@page), method: :delete, class: "delete float-right" %> -
-
- -<%= render "form" %> +<%= render Admin::SiteCustomization::Pages::EditComponent.new(@page) %> diff --git a/app/views/admin/site_customization/pages/new.html.erb b/app/views/admin/site_customization/pages/new.html.erb index dd6fdddd6..bb45bfb51 100644 --- a/app/views/admin/site_customization/pages/new.html.erb +++ b/app/views/admin/site_customization/pages/new.html.erb @@ -1,12 +1 @@ -<% provide :title do %> - <%= t("admin.header.title") %> - <%= t("admin.menu.site_customization.pages") %> - <%= t("admin.site_customization.pages.new.title") %> -<% end %> - -<%= back_link_to admin_site_customization_pages_path %> -
-
-

<%= t("admin.site_customization.pages.new.title") %>

-
-
- -<%= render "form" %> +<%= render Admin::SiteCustomization::Pages::NewComponent.new(@page) %> From ccf5c81ea977b5d057e61da803449696430f5c98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 14:09:01 +0100 Subject: [PATCH 12/17] Use a button to destroy pages from the edit page We were already using buttons to destroy pages from the pages index. As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, DELETE) requests to the server has a few issues. --- app/assets/stylesheets/admin.scss | 6 +++++- .../site_customization/pages/edit_component.html.erb | 8 +++++++- spec/system/admin/site_customization/pages_spec.rb | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index ed3f0dda5..70258353f 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -511,9 +511,13 @@ code { flex-wrap: wrap; a, - button { + button, + form { margin-#{$global-left}: auto; + } + a, + button { &:not(.delete) { @include regular-button; margin-#{$global-left}: auto; diff --git a/app/components/admin/site_customization/pages/edit_component.html.erb b/app/components/admin/site_customization/pages/edit_component.html.erb index 327bbcbc7..6e7650ad3 100644 --- a/app/components/admin/site_customization/pages/edit_component.html.erb +++ b/app/components/admin/site_customization/pages/edit_component.html.erb @@ -2,7 +2,13 @@ <%= back_link_to admin_site_customization_pages_path %> <%= header do %> - <%= link_to t("admin.site_customization.pages.index.delete"), admin_site_customization_page_path(page), method: :delete, class: "delete" %> + <%= render Admin::ActionComponent.new( + :destroy, + @page, + text: t("admin.site_customization.pages.index.delete"), + method: :delete, + class: "delete" + ) %> <% end %> <%= render "form" %> diff --git a/spec/system/admin/site_customization/pages_spec.rb b/spec/system/admin/site_customization/pages_spec.rb index 15c029389..6d1f5d841 100644 --- a/spec/system/admin/site_customization/pages_spec.rb +++ b/spec/system/admin/site_customization/pages_spec.rb @@ -93,7 +93,7 @@ describe "Admin custom pages", :admin do custom_page = create(:site_customization_page, title: "An example custom page") visit edit_admin_site_customization_page_path(custom_page) - click_link "Delete page" + click_button "Delete page" expect(page).not_to have_content "An example custom page" expect(page).not_to have_content "example-page" From df17bd1354820c484ffc2e0ece2b045d4d0dec61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 31 Mar 2024 18:34:09 +0200 Subject: [PATCH 13/17] Ask confirmation to delete pages from the edit page We were already doing that when deleting pages from the index page, and we also ask for confirmation in almost every page in the admin section. --- .../admin/site_customization/pages/edit_component.html.erb | 1 + spec/system/admin/site_customization/pages_spec.rb | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/components/admin/site_customization/pages/edit_component.html.erb b/app/components/admin/site_customization/pages/edit_component.html.erb index 6e7650ad3..9b4850cec 100644 --- a/app/components/admin/site_customization/pages/edit_component.html.erb +++ b/app/components/admin/site_customization/pages/edit_component.html.erb @@ -7,6 +7,7 @@ @page, text: t("admin.site_customization.pages.index.delete"), method: :delete, + confirm: true, class: "delete" ) %> <% end %> diff --git a/spec/system/admin/site_customization/pages_spec.rb b/spec/system/admin/site_customization/pages_spec.rb index 6d1f5d841..6192d5632 100644 --- a/spec/system/admin/site_customization/pages_spec.rb +++ b/spec/system/admin/site_customization/pages_spec.rb @@ -93,7 +93,9 @@ describe "Admin custom pages", :admin do custom_page = create(:site_customization_page, title: "An example custom page") visit edit_admin_site_customization_page_path(custom_page) - click_button "Delete page" + accept_confirm "Are you sure? This action will delete \"An example custom page\" and can't be undone." do + click_button "Delete page" + end expect(page).not_to have_content "An example custom page" expect(page).not_to have_content "example-page" From e884bc28e1f013d3b1b34039438437dcf030f3f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 14:15:05 +0100 Subject: [PATCH 14/17] Use a button to moderate proposal notifications As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST (or, in this case, PUT) requests to the server has a few issues. --- .../_proposal_notification_digest.html.erb | 13 ++++++++----- spec/system/admin/system_emails_spec.rb | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb b/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb index fa02ac0a5..24627ee0a 100644 --- a/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb +++ b/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb @@ -30,11 +30,14 @@
- <%= link_to t("admin.system_emails.preview_pending.moderate_pending"), - admin_system_email_moderate_pending_path(system_email_id: "proposal_notification_digest", - id: preview.id), - method: :put, - class: "button hollow float-right" %> + <%= render Admin::ActionComponent.new( + :moderate_pending, + "proposal_notification_digest", + text: t("admin.system_emails.preview_pending.moderate_pending"), + path: admin_system_email_moderate_pending_path(system_email_id: "proposal_notification_digest", id: preview.id), + method: :put, + class: "button hollow float-right" + ) %>
<% end %> diff --git a/spec/system/admin/system_emails_spec.rb b/spec/system/admin/system_emails_spec.rb index c2cf3e26c..baefc2e2f 100644 --- a/spec/system/admin/system_emails_spec.rb +++ b/spec/system/admin/system_emails_spec.rb @@ -328,7 +328,7 @@ describe "System Emails" do visit admin_system_email_preview_pending_path("proposal_notification_digest") within("#proposal_notification_#{proposal_notification1.id}") do - click_link "Moderate notification send" + click_button "Moderate notification send" end expect(page).not_to have_content("Proposal A Title") From b33eec101e40ba5e943ec11478dc97ade2868430 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 31 Mar 2024 19:31:36 +0200 Subject: [PATCH 15/17] Extract components to render custom image table actions This way it'll be easier to refactor them. We're also giving a proper title to the images index page. --- .../images/index_component.html.erb | 22 +++++++++++++ .../images/index_component.rb | 14 ++++++++ .../images/table_actions_component.html.erb | 12 +++++++ .../images/table_actions_component.rb | 7 ++++ .../site_customization/images/index.html.erb | 32 +------------------ 5 files changed, 56 insertions(+), 31 deletions(-) create mode 100644 app/components/admin/site_customization/images/index_component.html.erb create mode 100644 app/components/admin/site_customization/images/index_component.rb create mode 100644 app/components/admin/site_customization/images/table_actions_component.html.erb create mode 100644 app/components/admin/site_customization/images/table_actions_component.rb diff --git a/app/components/admin/site_customization/images/index_component.html.erb b/app/components/admin/site_customization/images/index_component.html.erb new file mode 100644 index 000000000..a6357caed --- /dev/null +++ b/app/components/admin/site_customization/images/index_component.html.erb @@ -0,0 +1,22 @@ +<%= header %> + + + + + + + + + + <% images.each do |image| %> + + + + + <% end %> + +
<%= t("admin.site_customization.images.index.image") %><%= t("admin.actions.actions") %>
+ <%= image.name %> (<%= image.required_width %>x<%= image.required_height %>) + + <%= render Admin::SiteCustomization::Images::TableActionsComponent.new(image) %> +
diff --git a/app/components/admin/site_customization/images/index_component.rb b/app/components/admin/site_customization/images/index_component.rb new file mode 100644 index 000000000..14648cc9b --- /dev/null +++ b/app/components/admin/site_customization/images/index_component.rb @@ -0,0 +1,14 @@ +class Admin::SiteCustomization::Images::IndexComponent < ApplicationComponent + include Header + attr_reader :images + + def initialize(images) + @images = images + end + + private + + def title + t("admin.site_customization.images.index.title") + end +end diff --git a/app/components/admin/site_customization/images/table_actions_component.html.erb b/app/components/admin/site_customization/images/table_actions_component.html.erb new file mode 100644 index 000000000..dbb1e106a --- /dev/null +++ b/app/components/admin/site_customization/images/table_actions_component.html.erb @@ -0,0 +1,12 @@ +
+ <%= form_for([:admin, image], html: { id: "edit_#{dom_id(image)}" }) do |f| %> +
+ <%= image_tag image.image if image.persisted_attachment? %> + <%= f.file_field :image, label: false %> +
+
+ <%= f.submit(t("admin.site_customization.images.index.update"), class: "button hollow") %> + <%= link_to t("admin.site_customization.images.index.delete"), admin_site_customization_image_path(image), method: :delete, class: "button hollow alert" if image.persisted_attachment? %> +
+ <% end %> +
diff --git a/app/components/admin/site_customization/images/table_actions_component.rb b/app/components/admin/site_customization/images/table_actions_component.rb new file mode 100644 index 000000000..ee4b3a20d --- /dev/null +++ b/app/components/admin/site_customization/images/table_actions_component.rb @@ -0,0 +1,7 @@ +class Admin::SiteCustomization::Images::TableActionsComponent < ApplicationComponent + attr_reader :image + + def initialize(image) + @image = image + end +end diff --git a/app/views/admin/site_customization/images/index.html.erb b/app/views/admin/site_customization/images/index.html.erb index 354c3626c..e34996508 100644 --- a/app/views/admin/site_customization/images/index.html.erb +++ b/app/views/admin/site_customization/images/index.html.erb @@ -1,31 +1 @@ -

<%= t("admin.site_customization.images.index.title") %>

- - - - - - - - - - <% @images.each do |image| %> - - - - - <% end %> - -
<%= t("admin.site_customization.images.index.image") %><%= t("admin.actions.actions") %>
- <%= image.name %> (<%= image.required_width %>x<%= image.required_height %>) - - <%= form_for([:admin, image], html: { id: "edit_#{dom_id(image)}" }) do |f| %> -
- <%= image_tag image.image if image.persisted_attachment? %> - <%= f.file_field :image, label: false %> -
-
- <%= f.submit(t("admin.site_customization.images.index.update"), class: "button hollow") %> - <%= link_to t("admin.site_customization.images.index.delete"), admin_site_customization_image_path(image), method: :delete, class: "button hollow alert" if image.persisted_attachment? %> -
- <% end %> -
+<%= render Admin::SiteCustomization::Images::IndexComponent.new(@images) %> From 54977116e7f49560c6236c0f8995d3bdb5483818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 04:16:34 +0100 Subject: [PATCH 16/17] Use a button to delete site customization images Note that we used to have the link to delete images inside the same
tag as the button to update the image. However, using a button means we're adding a new tag for the action to delete the image. This isn't valid HTML and, in some browsers, might result in the button sending the request to the wrong URL. As explained in commit 5311daadf, to avoid this, we'd need to replace `button_to` with `button_tag` in the action in order to generate a button without a form. Then, we could add either a `form` or a `formaction` attribute to the button. However, I thik it's easier to move the delete button outside the update button tag. On the minus side, since the buttons no longer share a parent, they're harder to style. So we're using a mix of nested flex layouts with one of the nested elements using a container unit as width. Since we're at it, we're also improving the styles on small and medium screens by making sure the "Update" button wraps before the "Delete" button does (using a container query), by giving enough width to the column containing this actions on small screens as well (removing `small-12` and giving it two-thirds of the width on all screen sizes) and by having a gap between elements. Note that, at the time of writing, container queries are only supported by about 91%-93% of the browsers, meaning that some administrators will see all from controls displayed vertically, one on top of the other, on all screen sizes. We think this is acceptable, and the page remains fully functional in this case. --- .../site_customization/images/index.scss | 11 +++++++ .../images/table_actions.scss | 29 +++++++++++++++++++ .../images/index_component.html.erb | 5 ++-- .../images/table_actions_component.html.erb | 17 +++++++---- config/locales/en/admin.yml | 1 - config/locales/es/admin.yml | 1 - .../admin/site_customization/images_spec.rb | 2 +- 7 files changed, 56 insertions(+), 10 deletions(-) create mode 100644 app/assets/stylesheets/admin/site_customization/images/index.scss create mode 100644 app/assets/stylesheets/admin/site_customization/images/table_actions.scss diff --git a/app/assets/stylesheets/admin/site_customization/images/index.scss b/app/assets/stylesheets/admin/site_customization/images/index.scss new file mode 100644 index 000000000..00e6c5a08 --- /dev/null +++ b/app/assets/stylesheets/admin/site_customization/images/index.scss @@ -0,0 +1,11 @@ +.admin-site-customization-images-index { + tbody tr td { + &:first-child { + width: calc(100% / 3); + } + + &:last-child { + width: calc(100% * 2 / 3); + } + } +} diff --git a/app/assets/stylesheets/admin/site_customization/images/table_actions.scss b/app/assets/stylesheets/admin/site_customization/images/table_actions.scss new file mode 100644 index 000000000..adecdc7b7 --- /dev/null +++ b/app/assets/stylesheets/admin/site_customization/images/table_actions.scss @@ -0,0 +1,29 @@ +.site-customization-images-table-actions { + align-items: flex-start; + container-type: inline-size; + display: flex; + flex-wrap: wrap; + gap: calc(#{$line-height} / 2); + + > :first-child { + align-items: flex-start; + display: flex; + flex-direction: column; + + @container (min-width: 30rem) { + flex-direction: row; + } + + > div { + @include grid-column-gutter; + min-width: 5rem; + padding-#{$global-left}: 0 !important; + width: 50cqi; + } + } + + .destroy-link { + @include hollow-button; + color: $alert-color; + } +} diff --git a/app/components/admin/site_customization/images/index_component.html.erb b/app/components/admin/site_customization/images/index_component.html.erb index a6357caed..3ff4a6eae 100644 --- a/app/components/admin/site_customization/images/index_component.html.erb +++ b/app/components/admin/site_customization/images/index_component.html.erb @@ -1,3 +1,4 @@ +<% provide :main_class, "admin-site-customization-images-index" %> <%= header %> @@ -10,10 +11,10 @@ <% images.each do |image| %> - - diff --git a/app/components/admin/site_customization/images/table_actions_component.html.erb b/app/components/admin/site_customization/images/table_actions_component.html.erb index dbb1e106a..913a0cc45 100644 --- a/app/components/admin/site_customization/images/table_actions_component.html.erb +++ b/app/components/admin/site_customization/images/table_actions_component.html.erb @@ -1,12 +1,19 @@
<%= form_for([:admin, image], html: { id: "edit_#{dom_id(image)}" }) do |f| %> -
+
<%= image_tag image.image if image.persisted_attachment? %> <%= f.file_field :image, label: false %>
-
- <%= f.submit(t("admin.site_customization.images.index.update"), class: "button hollow") %> - <%= link_to t("admin.site_customization.images.index.delete"), admin_site_customization_image_path(image), method: :delete, class: "button hollow alert" if image.persisted_attachment? %> -
+ + <%= f.submit(t("admin.site_customization.images.index.update"), class: "button hollow") %> + <% end %> + + <% if image.persisted_attachment? %> + <%= render Admin::ActionComponent.new( + :destroy, + image, + path: admin_site_customization_image_path(image), + method: :delete + ) %> <% end %>
diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index a7724c856..c1b8d651e 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1607,7 +1607,6 @@ en: index: title: Custom images update: Update - delete: Delete image: Image update: notice: Image updated successfully diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index b1c80ee13..e0b9ce808 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1607,7 +1607,6 @@ es: index: title: Personalizar imágenes update: Actualizar - delete: Borrar image: Imagen update: notice: Imagen actualizada correctamente diff --git a/spec/system/admin/site_customization/images_spec.rb b/spec/system/admin/site_customization/images_spec.rb index c7552c1e4..11cf9fc0e 100644 --- a/spec/system/admin/site_customization/images_spec.rb +++ b/spec/system/admin/site_customization/images_spec.rb @@ -112,7 +112,7 @@ describe "Admin custom images", :admin do expect(page).to have_css("img[src*='social_media_icon.png']") within("tr#image_social_media_icon") do - click_link "Delete" + click_button "Delete" end expect(page).not_to have_css("img[src*='social_media_icon.png']") From 251a5fb6e9352f234f63106a45c1186148556ec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 6 Mar 2024 15:07:14 +0100 Subject: [PATCH 17/17] Default to delete method for the destroy action This is consistent with what Rails does. --- app/components/admin/action_component.rb | 9 +++-- .../admin/budgets/actions_component.rb | 1 - .../groups_and_headings_component.html.erb | 2 +- .../images/table_actions_component.html.erb | 3 +- .../pages/edit_component.html.erb | 1 - .../admin/table_actions_component.rb | 1 - .../legislation/draft_versions/edit.html.erb | 1 - .../admin/legislation/questions/edit.html.erb | 1 - app/views/admin/officials/edit.html.erb | 1 - .../questions/answers/images/index.html.erb | 1 - .../content_blocks/edit.html.erb | 1 - .../components/admin/action_component_spec.rb | 34 ++++++++++++++++++- 12 files changed, 42 insertions(+), 14 deletions(-) diff --git a/app/components/admin/action_component.rb b/app/components/admin/action_component.rb index 33e00e01d..fc592aff3 100644 --- a/app/components/admin/action_component.rb +++ b/app/components/admin/action_component.rb @@ -11,7 +11,11 @@ class Admin::ActionComponent < ApplicationComponent private def button? - options[:method] && options[:method] != :get + method && method != :get + end + + def method + options[:method] || (:delete if action == :destroy) end def text @@ -30,6 +34,7 @@ class Admin::ActionComponent < ApplicationComponent def html_options { + method: method, class: html_class, id: (dom_id(record, action) if record.respond_to?(:to_key)), "aria-describedby": describedby, @@ -38,7 +43,7 @@ class Admin::ActionComponent < ApplicationComponent confirm: confirmation_text, disable_with: (text if button?) } - }.merge(options.except(:"aria-describedby", :"aria-label", :class, :confirm, :path, :text)) + }.merge(options.except(:"aria-describedby", :"aria-label", :class, :confirm, :method, :path, :text)) end def html_class diff --git a/app/components/admin/budgets/actions_component.rb b/app/components/admin/budgets/actions_component.rb index 336878065..ddbd77bfc 100644 --- a/app/components/admin/budgets/actions_component.rb +++ b/app/components/admin/budgets/actions_component.rb @@ -39,7 +39,6 @@ class Admin::Budgets::ActionsComponent < ApplicationComponent def destroy_action action(:destroy, text: t("admin.budgets.edit.delete"), - method: :delete, confirm: t("admin.budgets.actions.confirm.destroy"), disabled: budget.investments.any? || budget.poll) end diff --git a/app/components/admin/budgets/groups_and_headings_component.html.erb b/app/components/admin/budgets/groups_and_headings_component.html.erb index e9ba73fc9..f3c935af0 100644 --- a/app/components/admin/budgets/groups_and_headings_component.html.erb +++ b/app/components/admin/budgets/groups_and_headings_component.html.erb @@ -4,7 +4,7 @@
<%= action(:edit, group, "aria-label": true) %> - <%= action(:destroy, group, method: :delete, confirm: true, "aria-label": true) %> + <%= action(:destroy, group, confirm: true, "aria-label": true) %> <%= action(:new, group, text: t("admin.budgets.show.add_heading"), diff --git a/app/components/admin/site_customization/images/table_actions_component.html.erb b/app/components/admin/site_customization/images/table_actions_component.html.erb index 913a0cc45..3957d23ad 100644 --- a/app/components/admin/site_customization/images/table_actions_component.html.erb +++ b/app/components/admin/site_customization/images/table_actions_component.html.erb @@ -12,8 +12,7 @@ <%= render Admin::ActionComponent.new( :destroy, image, - path: admin_site_customization_image_path(image), - method: :delete + path: admin_site_customization_image_path(image) ) %> <% end %>
diff --git a/app/components/admin/site_customization/pages/edit_component.html.erb b/app/components/admin/site_customization/pages/edit_component.html.erb index 9b4850cec..2cc012275 100644 --- a/app/components/admin/site_customization/pages/edit_component.html.erb +++ b/app/components/admin/site_customization/pages/edit_component.html.erb @@ -6,7 +6,6 @@ :destroy, @page, text: t("admin.site_customization.pages.index.delete"), - method: :delete, confirm: true, class: "delete" ) %> diff --git a/app/components/admin/table_actions_component.rb b/app/components/admin/table_actions_component.rb index 52d8c12be..14b7798f8 100644 --- a/app/components/admin/table_actions_component.rb +++ b/app/components/admin/table_actions_component.rb @@ -38,7 +38,6 @@ class Admin::TableActionsComponent < ApplicationComponent def destroy_options { - method: :delete, confirm: options[:destroy_confirmation] || true }.merge(options[:destroy_options] || {}) end diff --git a/app/views/admin/legislation/draft_versions/edit.html.erb b/app/views/admin/legislation/draft_versions/edit.html.erb index efdde4e27..1d9d84e3b 100644 --- a/app/views/admin/legislation/draft_versions/edit.html.erb +++ b/app/views/admin/legislation/draft_versions/edit.html.erb @@ -18,7 +18,6 @@ <%= render Admin::ActionComponent.new( :destroy, @draft_version, - method: :delete, confirm: true, class: "button hollow alert" ) %> diff --git a/app/views/admin/legislation/questions/edit.html.erb b/app/views/admin/legislation/questions/edit.html.erb index ec51522ab..1cd06b140 100644 --- a/app/views/admin/legislation/questions/edit.html.erb +++ b/app/views/admin/legislation/questions/edit.html.erb @@ -18,7 +18,6 @@ <%= render Admin::ActionComponent.new( :destroy, @question, - method: :delete, confirm: true, class: "button hollow alert" ) %> diff --git a/app/views/admin/officials/edit.html.erb b/app/views/admin/officials/edit.html.erb index c03c77c65..a74af769d 100644 --- a/app/views/admin/officials/edit.html.erb +++ b/app/views/admin/officials/edit.html.erb @@ -23,7 +23,6 @@ @user, text: t("admin.officials.edit.destroy"), path: admin_official_path(@user), - method: :delete, class: "delete" ) %> <% end %> diff --git a/app/views/admin/poll/questions/answers/images/index.html.erb b/app/views/admin/poll/questions/answers/images/index.html.erb index 957c56d76..27484ce98 100644 --- a/app/views/admin/poll/questions/answers/images/index.html.erb +++ b/app/views/admin/poll/questions/answers/images/index.html.erb @@ -30,7 +30,6 @@ :destroy, image, text: t("images.remove_image"), - method: :delete, confirm: t("admin.actions.confirm_action", action: t("images.remove_image"), name: image.title), class: "delete float-right" ) %> diff --git a/app/views/admin/site_customization/content_blocks/edit.html.erb b/app/views/admin/site_customization/content_blocks/edit.html.erb index bdc15f1f8..4439ad5fa 100644 --- a/app/views/admin/site_customization/content_blocks/edit.html.erb +++ b/app/views/admin/site_customization/content_blocks/edit.html.erb @@ -10,7 +10,6 @@ text: t("admin.site_customization.content_blocks.index.delete"), path: (@is_heading_content_block ? admin_site_customization_delete_heading_content_block_path(@content_block.id) : admin_site_customization_content_block_path(@content_block)), confirm: true, - method: :delete, class: "delete float-right" ) %> diff --git a/spec/components/admin/action_component_spec.rb b/spec/components/admin/action_component_spec.rb index f401e582e..f18ca4823 100644 --- a/spec/components/admin/action_component_spec.rb +++ b/spec/components/admin/action_component_spec.rb @@ -1,6 +1,38 @@ require "rails_helper" describe Admin::ActionComponent do + describe "method" do + it "is not included by default for most actions" do + render_inline Admin::ActionComponent.new(:create, double, path: "/") + + expect(page).to have_link count: 1 + expect(page).not_to have_button + expect(page).not_to have_css "[data-method]" + end + + it "is included in the link when the method is get" do + render_inline Admin::ActionComponent.new(:create, double, path: "/", method: :get) + + expect(page).to have_link count: 1 + expect(page).to have_css "a[data-method='get']" + expect(page).not_to have_button + end + + it "defaults to :delete for the destroy action" do + render_inline Admin::ActionComponent.new(:destroy, double, path: "/") + + expect(page).to have_css "input[name='_method']", visible: :all, count: 1 + expect(page).to have_css "input[name='_method'][value='delete']", visible: :hidden + end + + it "can be overriden for the destroy action" do + render_inline Admin::ActionComponent.new(:destroy, double, path: "/", method: :put) + + expect(page).to have_css "input[name='_method']", visible: :all, count: 1 + expect(page).to have_css "input[name='_method'][value='put']", visible: :hidden + end + end + describe "HTML class" do it "includes an HTML class for the action by default" do render_inline Admin::ActionComponent.new(:edit, double, path: "/") @@ -159,7 +191,7 @@ describe Admin::ActionComponent do render_inline Admin::ActionComponent.new(:destroy, record, path: "/", confirm: true) - expect(page).to have_link count: 1 + expect(page).to have_button count: 1 expect(page).to have_css "[data-confirm='#{text}']" end end
+ <%= image.name %> (<%= image.required_width %>x<%= image.required_height %>) + <%= render Admin::SiteCustomization::Images::TableActionsComponent.new(image) %>