From 0b0cbcfe5a6c92bf43ea76d43adb9908cbfe61cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 5 Oct 2023 20:46:01 +0200 Subject: [PATCH 1/7] Fix typos in HTML target attributes In some places, we were using `blank` instead of `_blank`. Most browsers treat `blank` like `_blank`, though, so most people didn't experience any difference. In another place, we were incorrectly passing the `target` option inside an `options:` hash, resulting in invalid HTML. --- .../admin/site_customization/pages/index_component.html.erb | 2 +- app/components/budgets/investments/form_component.html.erb | 4 ++-- app/components/debates/form_component.html.erb | 4 ++-- app/components/layout/footer_component.html.erb | 4 ++-- app/components/proposals/form_component.html.erb | 4 ++-- app/views/legislation/proposals/_form.html.erb | 4 ++-- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/components/admin/site_customization/pages/index_component.html.erb b/app/components/admin/site_customization/pages/index_component.html.erb index 68db19b5f..c5f6ebac0 100644 --- a/app/components/admin/site_customization/pages/index_component.html.erb +++ b/app/components/admin/site_customization/pages/index_component.html.erb @@ -34,7 +34,7 @@ <%= actions.action(:show, text: t("admin.site_customization.pages.index.see_page"), path: page.url, - options: { target: "_blank" }) %> + target: "_blank") %> <% end %> <% end %> diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 228aec322..7e0c52280 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -92,8 +92,8 @@ <%= f.check_box :terms_of_service, title: t("form.accept_terms_title"), label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "blank")) %> + policy: link_to(t("form.policy"), "/privacy", target: "_blank"), + conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> <% end %> diff --git a/app/components/debates/form_component.html.erb b/app/components/debates/form_component.html.erb index 851269d1b..e0f14ef06 100644 --- a/app/components/debates/form_component.html.erb +++ b/app/components/debates/form_component.html.erb @@ -44,8 +44,8 @@ <%= f.check_box :terms_of_service, title: t("form.accept_terms_title"), label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "blank")) %> + policy: link_to(t("form.policy"), "/privacy", target: "_blank"), + conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> <% end %> diff --git a/app/components/layout/footer_component.html.erb b/app/components/layout/footer_component.html.erb index 52f78b18d..dc9a559fd 100644 --- a/app/components/layout/footer_component.html.erb +++ b/app/components/layout/footer_component.html.erb @@ -7,8 +7,8 @@

<%= sanitize(t("layouts.footer.description", - open_source: link_to(t("layouts.footer.open_source"), t("layouts.footer.open_source_url"), target: "blank", rel: "nofollow"), - consul: link_to(t("layouts.footer.consul"), t("layouts.footer.consul_url"), target: "blank", rel: "nofollow"))) %> + open_source: link_to(t("layouts.footer.open_source"), t("layouts.footer.open_source_url"), target: "_blank", rel: "nofollow"), + consul: link_to(t("layouts.footer.consul"), t("layouts.footer.consul_url"), target: "_blank", rel: "nofollow"))) %>

diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index fddca56bb..7884aa270 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -99,8 +99,8 @@ <%= f.check_box :terms_of_service, title: t("form.accept_terms_title"), label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "blank")) %> + policy: link_to(t("form.policy"), "/privacy", target: "_blank"), + conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> <% end %> diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index b1cf5d8b4..03e905aee 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -65,8 +65,8 @@ <%= f.check_box :terms_of_service, title: t("form.accept_terms_title"), label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "blank")) %> + policy: link_to(t("form.policy"), "/privacy", target: "_blank"), + conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> <% end %> From 95958b2080cad072cb58bab8bf94395cda3cfd56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 15 Oct 2023 14:47:53 +0200 Subject: [PATCH 2/7] Remove title attribute in fields to accept terms Not sure why we introduced these titles in commit 9dce52a69. The title attributes contained the same text as the label, which IMHO made them useless. --- app/components/budgets/investments/form_component.html.erb | 1 - app/components/debates/form_component.html.erb | 1 - app/components/proposals/form_component.html.erb | 1 - app/views/legislation/proposals/_form.html.erb | 1 - app/views/organizations/registrations/new.html.erb | 1 - app/views/users/registrations/new.html.erb | 1 - app/views/verification/residence/new.html.erb | 1 - config/locales/en/devise_views.yml | 1 - config/locales/en/general.yml | 1 - config/locales/en/verification.yml | 1 - config/locales/es/devise_views.yml | 1 - config/locales/es/general.yml | 1 - config/locales/es/verification.yml | 1 - 13 files changed, 13 deletions(-) diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 7e0c52280..25a886bd8 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -90,7 +90,6 @@ <% unless current_user.manager? || investment.persisted? %>
<%= f.check_box :terms_of_service, - title: t("form.accept_terms_title"), label: t("form.accept_terms", policy: link_to(t("form.policy"), "/privacy", target: "_blank"), conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> diff --git a/app/components/debates/form_component.html.erb b/app/components/debates/form_component.html.erb index e0f14ef06..5df529206 100644 --- a/app/components/debates/form_component.html.erb +++ b/app/components/debates/form_component.html.erb @@ -42,7 +42,6 @@ <% if debate.new_record? %>
<%= f.check_box :terms_of_service, - title: t("form.accept_terms_title"), label: t("form.accept_terms", policy: link_to(t("form.policy"), "/privacy", target: "_blank"), conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index 7884aa270..b77eba8a7 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -97,7 +97,6 @@ <% if proposal.new_record? %>
<%= f.check_box :terms_of_service, - title: t("form.accept_terms_title"), label: t("form.accept_terms", policy: link_to(t("form.policy"), "/privacy", target: "_blank"), conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index 03e905aee..179d1bb4b 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -63,7 +63,6 @@
<% if @proposal.new_record? %> <%= f.check_box :terms_of_service, - title: t("form.accept_terms_title"), label: t("form.accept_terms", policy: link_to(t("form.policy"), "/privacy", target: "_blank"), conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> diff --git a/app/views/organizations/registrations/new.html.erb b/app/views/organizations/registrations/new.html.erb index b36fba601..a850766e2 100644 --- a/app/views/organizations/registrations/new.html.erb +++ b/app/views/organizations/registrations/new.html.erb @@ -25,7 +25,6 @@ label: t("devise_views.organizations.registrations.new.password_confirmation_label") %> <%= f.check_box :terms_of_service, - title: t("devise_views.users.registrations.new.terms_title"), label: t("devise_views.users.registrations.new.terms", terms: link_to(t("devise_views.users.registrations.new.terms_link"), "/conditions", diff --git a/app/views/users/registrations/new.html.erb b/app/views/users/registrations/new.html.erb index 0afca1696..cdb376fb7 100644 --- a/app/views/users/registrations/new.html.erb +++ b/app/views/users/registrations/new.html.erb @@ -35,7 +35,6 @@ <% end %> <%= f.check_box :terms_of_service, - title: t("devise_views.users.registrations.new.terms_title"), label: t("devise_views.users.registrations.new.terms", terms: link_to(t("devise_views.users.registrations.new.terms_link"), "/conditions", diff --git a/app/views/verification/residence/new.html.erb b/app/views/verification/residence/new.html.erb index b528598f9..1dbcee3e3 100644 --- a/app/views/verification/residence/new.html.erb +++ b/app/views/verification/residence/new.html.erb @@ -69,7 +69,6 @@
<%= f.check_box :terms_of_service, - title: t("verification.residence.new.accept_terms_text_title"), label: t("verification.residence.new.accept_terms_text", terms_url: link_to(t("verification.residence.new.terms"), page_path("census_terms"), diff --git a/config/locales/en/devise_views.yml b/config/locales/en/devise_views.yml index 7e184e028..85e95184f 100644 --- a/config/locales/en/devise_views.yml +++ b/config/locales/en/devise_views.yml @@ -103,7 +103,6 @@ en: submit: Register terms: By registering you accept the %{terms} terms_link: terms and conditions of use - terms_title: By registering you accept the terms and conditions of use title: Register username_is_available: Username available username_is_not_available: Username already in use diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 0be47b5a9..4f58a19c3 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -143,7 +143,6 @@ en: poll_ended: "Cannot be changed if voting has already ended" form: accept_terms: I agree to the %{policy} and the %{conditions} - accept_terms_title: I agree to the Privacy Policy and the Terms and conditions of use conditions: Terms and conditions of use debate: Debate direct_message: private message diff --git a/config/locales/en/verification.yml b/config/locales/en/verification.yml index 7b6ac56ee..7e6d33f79 100644 --- a/config/locales/en/verification.yml +++ b/config/locales/en/verification.yml @@ -46,7 +46,6 @@ en: success: Residence verified new: accept_terms_text: I accept %{terms_url} of the Census - accept_terms_text_title: I accept the terms and conditions of access of the Census document_number: Document number document_number_help_title: Help document_number_help_text: "DNI: 12345678A
Passport: AAA000001
Residence card: X1234567P" diff --git a/config/locales/es/devise_views.yml b/config/locales/es/devise_views.yml index da16e7421..5e3d5a40e 100644 --- a/config/locales/es/devise_views.yml +++ b/config/locales/es/devise_views.yml @@ -103,7 +103,6 @@ es: submit: Registrarse terms: Al registrarte aceptas las %{terms} terms_link: condiciones de uso - terms_title: Al registrarte aceptas las condiciones de uso title: Registrarse username_is_available: Nombre de usuario disponible username_is_not_available: Nombre de usuario ya existente diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index e40c5b43d..dd2380dbd 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -143,7 +143,6 @@ es: poll_ended: "No puede ser cambiada si la votación ha acabado" form: accept_terms: Acepto la %{policy} y las %{conditions} - accept_terms_title: Acepto la Política de privacidad y las Condiciones de uso conditions: Condiciones de uso debate: el debate direct_message: el mensaje privado diff --git a/config/locales/es/verification.yml b/config/locales/es/verification.yml index 4f52a3d5a..75d909f11 100644 --- a/config/locales/es/verification.yml +++ b/config/locales/es/verification.yml @@ -46,7 +46,6 @@ es: success: Residencia verificada new: accept_terms_text: Acepto %{terms_url} al Padrón - accept_terms_text_title: Acepto los términos de acceso al Padrón document_number: Número de documento document_number_help_title: Ayuda document_number_help_text: "DNI: 12345678A
Pasaporte: AAA000001
Tarjeta de residencia: X1234567P" From 46f43236bb17922052f4d31b328e34d9ec73c342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 15 Oct 2023 15:08:47 +0200 Subject: [PATCH 3/7] Extract component to render the "I agree" checkbox IMHO the best solution would be to completely remove this checkbox on forms that require registration. Other than the fact that people have already agreed with these terms when registering (although I guess these terms might have changed since then) and that approximately 0% of the population will read the conditions every time they agree with them, there's the fact that these links are inside a label and people might accidentally click on them while trying to click on the label in order to check the checkbox. I guess the idea is that these conditions might have changed since the moment people registered. In a fair world, checking "I agree" would have no legal meaning because it's unreasonable to expect that people will read these conditions every time they fill in a form, so whatever we're trying to do here would be pointless. But, since I'm not sure about the legal implications of removing this field in a world where you have to inform people that websites requiring identification use cookies, for now the field will stay where it is. --- .../budgets/investments/form_component.html.erb | 5 +---- app/components/debates/form_component.html.erb | 5 +---- app/components/proposals/form_component.html.erb | 5 +---- ...with_terms_of_service_field_component.html.erb | 1 + ...agree_with_terms_of_service_field_component.rb | 15 +++++++++++++++ app/views/legislation/proposals/_form.html.erb | 5 +---- 6 files changed, 20 insertions(+), 16 deletions(-) create mode 100644 app/components/shared/agree_with_terms_of_service_field_component.html.erb create mode 100644 app/components/shared/agree_with_terms_of_service_field_component.rb diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 25a886bd8..f5aa72e31 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -89,10 +89,7 @@
<% unless current_user.manager? || investment.persisted? %>
- <%= f.check_box :terms_of_service, - label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "_blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> + <%= render Shared::AgreeWithTermsOfServiceFieldComponent.new(f) %>
<% end %> diff --git a/app/components/debates/form_component.html.erb b/app/components/debates/form_component.html.erb index 5df529206..1816d437c 100644 --- a/app/components/debates/form_component.html.erb +++ b/app/components/debates/form_component.html.erb @@ -41,10 +41,7 @@
<% if debate.new_record? %>
- <%= f.check_box :terms_of_service, - label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "_blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> + <%= render Shared::AgreeWithTermsOfServiceFieldComponent.new(f) %>
<% end %> diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index b77eba8a7..c4467fe7d 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -96,10 +96,7 @@
<% if proposal.new_record? %>
- <%= f.check_box :terms_of_service, - label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "_blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> + <%= render Shared::AgreeWithTermsOfServiceFieldComponent.new(f) %>
<% end %> diff --git a/app/components/shared/agree_with_terms_of_service_field_component.html.erb b/app/components/shared/agree_with_terms_of_service_field_component.html.erb new file mode 100644 index 000000000..dcfcb0e32 --- /dev/null +++ b/app/components/shared/agree_with_terms_of_service_field_component.html.erb @@ -0,0 +1 @@ +<%= form.check_box :terms_of_service, label: label %> diff --git a/app/components/shared/agree_with_terms_of_service_field_component.rb b/app/components/shared/agree_with_terms_of_service_field_component.rb new file mode 100644 index 000000000..808dbe812 --- /dev/null +++ b/app/components/shared/agree_with_terms_of_service_field_component.rb @@ -0,0 +1,15 @@ +class Shared::AgreeWithTermsOfServiceFieldComponent < ApplicationComponent + attr_reader :form + + def initialize(form) + @form = form + end + + private + + def label + t("form.accept_terms", + policy: link_to(t("form.policy"), "/privacy", target: "_blank"), + conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) + end +end diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index 179d1bb4b..0bea7d205 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -62,10 +62,7 @@
<% if @proposal.new_record? %> - <%= f.check_box :terms_of_service, - label: t("form.accept_terms", - policy: link_to(t("form.policy"), "/privacy", target: "_blank"), - conditions: link_to(t("form.conditions"), "/conditions", target: "_blank")) %> + <%= render Shared::AgreeWithTermsOfServiceFieldComponent.new(f) %> <% end %>
From cdf859621e96b1a7a5a8268d2fc4dee5828b436f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 15 Oct 2023 16:04:02 +0200 Subject: [PATCH 4/7] Allow links in forms to open in new tabs We used to open these links in new tabs, but accidentally stopped doing so in commit 75a28fafc. While, in general, automatically opening a link in a new tab/window is a bad idea, the exception comes when people are filling in a form and there are links to pages that contain information which will help them fill in a form. There are mainly two advantages of this approach. First, it makes less likely for people to accidentally lose the information they were filling in. And, second, having both the form and a help page open at the same time can make it easier to fill in the form. However, opening these links in new tabs also has disadvantages, like taking control away from people or making it harder to navigate through pages when using a mobile phone. So this is a compromise solution. --- lib/consul_form_builder.rb | 12 +++++++++-- ...h_terms_of_service_field_component_spec.rb | 21 +++++++++++++++++++ spec/lib/consul_form_builder_spec.rb | 11 ++++++++++ spec/system/verification/residence_spec.rb | 5 +++-- 4 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 spec/components/shared/agree_with_terms_of_service_field_component_spec.rb diff --git a/lib/consul_form_builder.rb b/lib/consul_form_builder.rb index e9868185e..620a0886c 100644 --- a/lib/consul_form_builder.rb +++ b/lib/consul_form_builder.rb @@ -23,7 +23,7 @@ class ConsulFormBuilder < FoundationRailsHelper::FormBuilder if options[:label] == false super else - label = tag.span sanitize(label_text(attribute, options[:label])), class: "checkbox" + label = tag.span sanitized_label_text(attribute, options[:label]), class: "checkbox" super(attribute, options.merge( label: label, @@ -51,7 +51,7 @@ class ConsulFormBuilder < FoundationRailsHelper::FormBuilder if text == false super else - super(attribute, sanitize(label_text(attribute, text)), options) + super(attribute, sanitized_label_text(attribute, text), options) end end @@ -68,6 +68,14 @@ class ConsulFormBuilder < FoundationRailsHelper::FormBuilder end end + def sanitized_label_text(attribute, text) + sanitize(label_text(attribute, text), attributes: allowed_attributes) + end + + def allowed_attributes + self.class.sanitized_allowed_attributes + ["target"] + end + def label_options_for(options) label_options = options[:label_options] || {} diff --git a/spec/components/shared/agree_with_terms_of_service_field_component_spec.rb b/spec/components/shared/agree_with_terms_of_service_field_component_spec.rb new file mode 100644 index 000000000..6b7c8bc7a --- /dev/null +++ b/spec/components/shared/agree_with_terms_of_service_field_component_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + +describe Shared::AgreeWithTermsOfServiceFieldComponent do + before do + dummy_model = Class.new do + include ActiveModel::Model + attr_accessor :terms_of_service + end + + stub_const("DummyModel", dummy_model) + end + + let(:form) { ConsulFormBuilder.new(:dummy, DummyModel.new, ApplicationController.new.view_context, {}) } + let(:component) { Shared::AgreeWithTermsOfServiceFieldComponent.new(form) } + + it "contains links that open in a new window" do + render_inline component + + expect(page).to have_css "a[target=_blank]", count: 2 + end +end diff --git a/spec/lib/consul_form_builder_spec.rb b/spec/lib/consul_form_builder_spec.rb index d8d9adf5f..db57847d2 100644 --- a/spec/lib/consul_form_builder_spec.rb +++ b/spec/lib/consul_form_builder_spec.rb @@ -13,6 +13,17 @@ describe ConsulFormBuilder do let(:builder) { ConsulFormBuilder.new(:dummy, DummyModel.new, ApplicationController.new.view_context, {}) } + describe "label" do + it "accepts links that open in a new window in its content" do + render builder.text_field(:title, label: 'My title') + + expect(page).to have_link count: 1 + expect(page).to have_link "title" + expect(page).to have_link "New tab" + expect(page).to have_css "a[target=_blank]" + end + end + describe "hints" do it "does not generate hints by default" do render builder.text_field(:title) diff --git a/spec/system/verification/residence_spec.rb b/spec/system/verification/residence_spec.rb index c54e44fee..3603d4ba9 100644 --- a/spec/system/verification/residence_spec.rb +++ b/spec/system/verification/residence_spec.rb @@ -164,8 +164,9 @@ describe "Residence" do login_as(create(:user)) visit new_residence_path - click_link "the terms and conditions of access" - expect(page).to have_content "Terms and conditions of access of the Census" + within_window(window_opened_by { click_link "the terms and conditions of access" }) do + expect(page).to have_content "Terms and conditions of access of the Census" + end end end From cc25dbe437f8ad9e533113786259b271b31c1d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 15 Oct 2023 16:32:30 +0200 Subject: [PATCH 5/7] Simplify help links in debates/proposals forms --- app/components/debates/new_component.html.erb | 4 +--- app/components/proposals/new_component.html.erb | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/components/debates/new_component.html.erb b/app/components/debates/new_component.html.erb index 91510dd95..26866561a 100644 --- a/app/components/debates/new_component.html.erb +++ b/app/components/debates/new_component.html.erb @@ -2,9 +2,7 @@ <%= back_link_to debates_path, t("debates.index.section_header.title") %> <%= header do %> - <%= link_to help_path(anchor: "debates"), title: t("shared.target_blank"), target: "_blank" do %> - <%= t("debates.new.more_info") %> - <% end %> + <%= link_to t("debates.new.more_info"), help_path(anchor: "debates"), title: t("shared.target_blank"), target: "_blank" %> <% end %>