From d0b57868af3b9d3fe40b074de4d753267b09e391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 15 Nov 2025 05:37:59 +0100 Subject: [PATCH 01/14] Move settings map inside the form That's what we usually do, and it makes sense since clicking on the map changes the content of hidden fields in the form. --- .../settings/map_form_component.html.erb | 29 +++++++++---------- spec/system/admin/settings_spec.rb | 3 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/components/admin/settings/map_form_component.html.erb b/app/components/admin/settings/map_form_component.html.erb index 7a0f00a95..c9738230f 100644 --- a/app/components/admin/settings/map_form_component.html.erb +++ b/app/components/admin/settings/map_form_component.html.erb @@ -1,21 +1,20 @@
-
" - data-map-center-longitude="<%= Setting["map.longitude"] %>" - data-map-zoom="<%= Setting["map.zoom"] %>" - data-map-tiles-provider="<%= Rails.application.secrets.map_tiles_provider %>" - data-map-tiles-provider-attribution="<%= Rails.application.secrets.map_tiles_provider_attribution %>" - data-marker-editable="true" - data-marker-latitude="<%= Setting["map.latitude"] %>" - data-marker-longitude="<%= Setting["map.longitude"] %>" - data-latitude-input-selector="#latitude" - data-longitude-input-selector="#longitude" - data-zoom-input-selector="#zoom"> -
- <%= form_tag admin_update_map_path, method: :put, id: "map-form" do |f| %> +
" + data-map-center-longitude="<%= Setting["map.longitude"] %>" + data-map-zoom="<%= Setting["map.zoom"] %>" + data-map-tiles-provider="<%= Rails.application.secrets.map_tiles_provider %>" + data-map-tiles-provider-attribution="<%= Rails.application.secrets.map_tiles_provider_attribution %>" + data-marker-editable="true" + data-marker-latitude="<%= Setting["map.latitude"] %>" + data-marker-longitude="<%= Setting["map.longitude"] %>" + data-latitude-input-selector="#latitude" + data-longitude-input-selector="#longitude" + data-zoom-input-selector="#zoom"> +
<%= hidden_field_tag :latitude, Setting["map.latitude"] %> <%= hidden_field_tag :longitude, Setting["map.longitude"] %> diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index 83c4369b3..1647ac690 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -92,8 +92,9 @@ describe "Admin settings", :admin do visit admin_settings_path click_link "Map configuration" - find("#admin-map").click + within "#map-form" do + find("#admin-map").click click_button "Update" end From 0ec7c65b9bbd374263812a701f4b73e2c6113338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 16 Nov 2025 23:45:56 +0100 Subject: [PATCH 02/14] Don't pass unused parameter to map location fields partial We don't use this parameter since commit c34aa5412. --- app/components/budgets/investments/form_component.html.erb | 3 +-- app/components/proposals/form_component.html.erb | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index f5aa72e31..05826600d 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -50,8 +50,7 @@ form: f, map_location: investment.map_location || MapLocation.new, label: t("budgets.investments.form.map_location"), - help: t("budgets.investments.form.map_location_instructions"), - i18n_namespace: "budgets.investments" %> + help: t("budgets.investments.form.map_location_instructions") %>
<% end %> diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index c4467fe7d..c115de81c 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -58,8 +58,7 @@ form: f, map_location: proposal.map_location || MapLocation.new, label: t("proposals.form.map_location"), - help: t("proposals.form.map_location_instructions"), - i18n_namespace: "proposals" %> + help: t("proposals.form.map_location_instructions") %>
<% end %> From 2d85bd5351ac7dee88c976a1a9995561fe83cf2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 16 Nov 2025 23:53:26 +0100 Subject: [PATCH 03/14] Remove duplication rendering map location fields We're going to move the partial to a component, and this makes it easier. --- .../investments/form_component.html.erb | 14 +++++--------- .../proposals/form_component.html.erb | 14 +++++--------- app/views/map_locations/_form_fields.html.erb | 18 +++++++++++------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 05826600d..240bf2117 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -44,15 +44,11 @@ <%= render Documents::NestedComponent.new(f) %> <% end %> - <% if feature?(:map) %> -
- <%= render "map_locations/form_fields", - form: f, - map_location: investment.map_location || MapLocation.new, - label: t("budgets.investments.form.map_location"), - help: t("budgets.investments.form.map_location_instructions") %> -
- <% end %> + <%= render "map_locations/form_fields", + form: f, + map_location: investment.map_location || MapLocation.new, + label: t("budgets.investments.form.map_location"), + help: t("budgets.investments.form.map_location_instructions") %>
<%= f.text_field :location %> diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index c115de81c..b3db02571 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -52,15 +52,11 @@
<% end %> - <% if feature?(:map) %> -
- <%= render "map_locations/form_fields", - form: f, - map_location: proposal.map_location || MapLocation.new, - label: t("proposals.form.map_location"), - help: t("proposals.form.map_location_instructions") %> -
- <% end %> + <%= render "map_locations/form_fields", + form: f, + map_location: proposal.map_location || MapLocation.new, + label: t("proposals.form.map_location"), + help: t("proposals.form.map_location_instructions") %>
<%= f.label :tag_list, t("proposals.form.tags_label") %> diff --git a/app/views/map_locations/_form_fields.html.erb b/app/views/map_locations/_form_fields.html.erb index 403368b1b..59efd6489 100644 --- a/app/views/map_locations/_form_fields.html.erb +++ b/app/views/map_locations/_form_fields.html.erb @@ -1,10 +1,14 @@ -<%= form.label :map_location, label %> -

<%= help %>

+<% if feature?(:map) %> +
+ <%= form.label :map_location, label %> +

<%= help %>

-<%= form.fields_for :map_location, map_location do |m_l_fields| %> - <%= render_map(map_location, form: m_l_fields) %> + <%= form.fields_for :map_location, map_location do |m_l_fields| %> + <%= render_map(map_location, form: m_l_fields) %> - <%= m_l_fields.hidden_field :latitude %> - <%= m_l_fields.hidden_field :longitude %> - <%= m_l_fields.hidden_field :zoom %> + <%= m_l_fields.hidden_field :latitude %> + <%= m_l_fields.hidden_field :longitude %> + <%= m_l_fields.hidden_field :zoom %> + <% end %> +
<% end %> From a6908f201755c2594214baff05c5cf8aa5384b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 00:31:44 +0100 Subject: [PATCH 04/14] Group similar map tests together We're about to change some of these tests, and we usually group similar system tests in order to make the test suite a bit faster. --- spec/shared/system/mappable.rb | 54 +++++++----------------------- spec/system/admin/settings_spec.rb | 40 +++------------------- 2 files changed, 16 insertions(+), 78 deletions(-) diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index d2fca3089..ef96ffdd9 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -17,7 +17,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, describe "At #{mappable_new_path}" do before { set_arguments(arguments, mappable, mappable_path_arguments) } - scenario "Should not show marker by default on create #{mappable_factory_name}" do + scenario "Should show marker and create #{mappable_factory_name} with map" do do_login_for user, management: management visit send(mappable_new_path, arguments) @@ -26,27 +26,14 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, within ".map-location" do expect(page).not_to have_css(".map-icon") end - end - scenario "Should show marker on create #{mappable_factory_name} when click on map" do - do_login_for user, management: management - visit send(mappable_new_path, arguments) - - send("fill_in_#{mappable_factory_name}") find("#new_map_location").click within ".map-location" do expect(page).to have_css ".map-icon" expect(page).not_to have_css ".map-icon[aria-label]" end - end - scenario "Should create #{mappable_factory_name} with map" do - do_login_for user, management: management - visit send(mappable_new_path, arguments) - - send("fill_in_#{mappable_factory_name}") - find("#new_map_location").click send("submit_#{mappable_factory_name}_form") within ".map-location" do @@ -180,8 +167,9 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, end describe "At #{mappable_edit_path}", if: mappable_edit_path.present? do - scenario "Should edit map on #{mappable_factory_name} and contain default values" do + scenario "Should edit mappable on #{mappable_factory_name} without changing the map" do mappable.map_location.update!(latitude: 51.48, longitude: 0.0) + do_login_for mappable.author, management: management visit send(mappable_edit_path, id: mappable.id) @@ -191,6 +179,14 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, with: "51.48" expect(page).to have_field "#{mappable_factory_name}_map_location_attributes_longitude", type: :hidden, with: "0.0" + + fill_in "#{mappable_factory_name.camelize} title", with: "New title" + click_button "Save changes" + + expect(page).not_to have_button "Save changes" + expect(page).to have_css ".map-location" + expect(page).to have_css ".map-location[data-marker-latitude='51.48']" + expect(page).to have_css ".map-location[data-marker-longitude='0.0']" end scenario "Should edit default values from map on #{mappable_factory_name} edit page" do @@ -219,22 +215,6 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).not_to have_css ".map-location[data-marker-longitude='#{original_longitude}']" end - scenario "Should edit mappable on #{mappable_factory_name} without change map" do - original_longitude = map_location.longitude - original_latitude = map_location.latitude - - do_login_for mappable.author, management: management - - visit send(mappable_edit_path, id: mappable.id) - fill_in "#{mappable_factory_name.camelize} title", with: "New title" - click_button "Save changes" - - expect(page).not_to have_button "Save changes" - expect(page).to have_css ".map-location" - expect(page).to have_css ".map-location[data-marker-latitude='#{original_latitude}']" - expect(page).to have_css ".map-location[data-marker-longitude='#{original_longitude}']" - end - scenario "Can not display map on #{mappable_factory_name} edit when remove map marker" do do_login_for mappable.author, management: management @@ -244,6 +224,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).not_to have_button "Save changes" expect(page).not_to have_css ".map-location" + expect(page).not_to have_content "Map location can't be blank" end scenario "Can not display map on #{mappable_factory_name} edit when feature.map is disabled" do @@ -257,17 +238,6 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).not_to have_button "Save changes" expect(page).not_to have_css ".map-location" end - - scenario "No need to skip map on update" do - do_login_for mappable.author, management: management - - visit send(mappable_edit_path, id: mappable.id) - click_button "Remove map marker" - click_button "Save changes" - - expect(page).not_to have_button "Save changes" - expect(page).not_to have_content "Map location can't be blank" - end end describe "At #{mappable_show_path}" do diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index 1647ac690..549a54b59 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -21,22 +21,16 @@ describe "Admin settings", :admin do end describe "Map settings initialization" do - before do + scenario "Map is only initialized when the map settings tab content is shown" do Setting["feature.map"] = true - end - scenario "When `Map settings` tab content is hidden map should not be initialized" do visit admin_settings_path - expect(page).not_to have_css("#admin-map.leaflet-container", visible: :all) - end - - scenario "When `Map settings` tab content is shown map should be initialized" do - visit admin_settings_path + expect(page).not_to have_css "#admin-map.leaflet-container", visible: :all click_link "Map configuration" - expect(page).to have_css("#admin-map.leaflet-container") + expect(page).to have_css "#admin-map.leaflet-container" end end @@ -53,7 +47,7 @@ describe "Admin settings", :admin do expect(page).not_to have_css("#admin-map") end - scenario "Should be able when map feature activated" do + scenario "Should update marker" do Setting["feature.map"] = true visit admin_settings_path @@ -63,35 +57,9 @@ describe "Admin settings", :admin do expect(page).not_to have_content "To show the map to users you must enable " \ '"Proposals and budget investments geolocation" ' \ 'on "Features" tab.' - end - - scenario "Should show successful notice" do - Setting["feature.map"] = true - - visit admin_settings_path - click_link "Map configuration" - - within "#map-form" do - click_button "Update" - end - - expect(page).to have_content "Map configuration updated successfully" - end - - scenario "Should display marker by default" do - Setting["feature.map"] = true - - visit admin_settings_path expect(find("#latitude", visible: :hidden).value).to eq "51.48" expect(find("#longitude", visible: :hidden).value).to eq "0.0" - end - - scenario "Should update marker" do - Setting["feature.map"] = true - - visit admin_settings_path - click_link "Map configuration" within "#map-form" do find("#admin-map").click From 86a12b23ad0a4c99f947cbeb7956a22fdd77038f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 01:59:17 +0100 Subject: [PATCH 05/14] Test admin map settings from the user's point of view People using these settings don't know about the hidden fields, but they do know about the fields that are actually displayed on the page. So we check that these fields are updated when the marker is updated. --- spec/system/admin/settings_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index 549a54b59..adde6dfb2 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -58,16 +58,17 @@ describe "Admin settings", :admin do '"Proposals and budget investments geolocation" ' \ 'on "Features" tab.' - expect(find("#latitude", visible: :hidden).value).to eq "51.48" - expect(find("#longitude", visible: :hidden).value).to eq "0.0" + expect(page).to have_field "Latitude", with: "51.48" + expect(page).to have_field "Longitude", with: "0.0" within "#map-form" do find("#admin-map").click click_button "Update" end - expect(find("#latitude", visible: :hidden).value).not_to eq "51.48" expect(page).to have_content "Map configuration updated successfully" + expect(page).to have_field "Latitude" + expect(page).not_to have_field "Latitude", with: "51.48" end end From 8b3ac5ac971cc188d6a685141aa34450d2cd8d51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 00:18:19 +0100 Subject: [PATCH 06/14] Use a legend instead of a label in map location fields The label was invalid HTML since it wasn't referencing any existing element. --- app/views/map_locations/_form_fields.html.erb | 6 +-- spec/shared/system/mappable.rb | 45 ++++++++++--------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/app/views/map_locations/_form_fields.html.erb b/app/views/map_locations/_form_fields.html.erb index 59efd6489..0b0c519c8 100644 --- a/app/views/map_locations/_form_fields.html.erb +++ b/app/views/map_locations/_form_fields.html.erb @@ -1,6 +1,6 @@ <% if feature?(:map) %> -
- <%= form.label :map_location, label %> +
+ <%= label %>

<%= help %>

<%= form.fields_for :map_location, map_location do |m_l_fields| %> @@ -10,5 +10,5 @@ <%= m_l_fields.hidden_field :longitude %> <%= m_l_fields.hidden_field :zoom %> <% end %> -
+ <% end %> diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index ef96ffdd9..b0d33be04 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -23,13 +23,11 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, send("fill_in_#{mappable_factory_name}") - within ".map-location" do - expect(page).not_to have_css(".map-icon") - end + within_fieldset "Map location" do + expect(page).not_to have_css ".map-icon" - find("#new_map_location").click + find("#new_map_location").click - within ".map-location" do expect(page).to have_css ".map-icon" expect(page).not_to have_css ".map-icon[aria-label]" end @@ -83,7 +81,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, go_back - within ".map-location" do + within_fieldset "Map location" do expect(page).to have_css(".leaflet-map-pane", count: 1) end end @@ -92,15 +90,16 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, do_login_for user, management: management visit send(mappable_new_path, arguments) - within ".map-location" do - expect(page).not_to have_css(".map-icon") + within_fieldset "Map location" do + expect(page).not_to have_css ".map-icon" end expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq(10) map_zoom_in - find("#new_map_location").click - within ".map-location" do + within_fieldset "Map location" do + find("#new_map_location").click + expect(page).to have_css(".map-icon") end @@ -116,9 +115,9 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, go_back - within ".map-location" do - expect(page).to have_css(".map-icon") - expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq(11) + within_fieldset "Map location" do + expect(page).to have_css ".map-icon" + expect(page.execute_script("return App.Map.maps[0].getZoom();")).to eq 11 end end @@ -126,14 +125,15 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, do_login_for user, management: management visit send(mappable_new_path, arguments) - within ".map-location" do - expect(page).not_to have_css(".map-icon") + within_fieldset "Map location" do + expect(page).not_to have_css ".map-icon" end place_map_at(-68.592487, -62.391357) - find("#new_map_location").click - within ".map-location" do + within_fieldset "Map location" do + find("#new_map_location").click + expect(page).to have_css(".map-icon") end @@ -149,8 +149,8 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, go_back - within ".map-location" do - expect(page).to have_css(".map-icon") + within_fieldset "Map location" do + expect(page).to have_css ".map-icon" end end end @@ -196,7 +196,10 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, do_login_for mappable.author, management: management visit send(mappable_edit_path, id: mappable.id) - find(".map-location").click(x: 30, y: 30) + + within_fieldset "Map location" do + find(".map-location").click(x: 30, y: 30) + end new_latitude = find_field( "#{mappable_factory_name}_map_location_attributes_latitude", type: :hidden @@ -219,7 +222,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, do_login_for mappable.author, management: management visit send(mappable_edit_path, id: mappable.id) - click_button "Remove map marker" + within_fieldset("Map location") { click_button "Remove map marker" } click_button "Save changes" expect(page).not_to have_button "Save changes" From 67e00654bd56ef91b635b41b275e40c594c9b8e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 02:11:14 +0100 Subject: [PATCH 07/14] Extract methods to get map location in form components This way changing them will be easier. --- app/components/budgets/investments/form_component.html.erb | 2 +- app/components/budgets/investments/form_component.rb | 4 ++++ app/components/proposals/form_component.html.erb | 2 +- app/components/proposals/form_component.rb | 4 ++++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index 240bf2117..ce157d9db 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -46,7 +46,7 @@ <%= render "map_locations/form_fields", form: f, - map_location: investment.map_location || MapLocation.new, + map_location: map_location, label: t("budgets.investments.form.map_location"), help: t("budgets.investments.form.map_location_instructions") %> diff --git a/app/components/budgets/investments/form_component.rb b/app/components/budgets/investments/form_component.rb index 0ee340ed3..eb6c0c4ce 100644 --- a/app/components/budgets/investments/form_component.rb +++ b/app/components/budgets/investments/form_component.rb @@ -19,4 +19,8 @@ class Budgets::Investments::FormComponent < ApplicationComponent def categories Tag.category.order(:name) end + + def map_location + investment.map_location || MapLocation.new + end end diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index b3db02571..9aec4e6eb 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -54,7 +54,7 @@ <%= render "map_locations/form_fields", form: f, - map_location: proposal.map_location || MapLocation.new, + map_location: map_location, label: t("proposals.form.map_location"), help: t("proposals.form.map_location_instructions") %> diff --git a/app/components/proposals/form_component.rb b/app/components/proposals/form_component.rb index 17ec5713c..b4a796229 100644 --- a/app/components/proposals/form_component.rb +++ b/app/components/proposals/form_component.rb @@ -15,4 +15,8 @@ class Proposals::FormComponent < ApplicationComponent def categories Tag.category.order(:name) end + + def map_location + proposal.map_location || MapLocation.new + end end From 29e5adc233a09b0430151a399eb343eab60a8cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 16 Nov 2025 23:57:46 +0100 Subject: [PATCH 08/14] Move map location fields partial to a component This way it'll be easier to test it and refactor it. --- .../investments/form_component.html.erb | 11 ++++---- .../form_fields_component.html.erb | 12 ++++++++ .../map_locations/form_fields_component.rb | 15 ++++++++++ .../proposals/form_component.html.erb | 11 ++++---- app/views/map_locations/_form_fields.html.erb | 14 ---------- .../form_fields_component_spec.rb | 28 +++++++++++++++++++ 6 files changed, 67 insertions(+), 24 deletions(-) create mode 100644 app/components/map_locations/form_fields_component.html.erb create mode 100644 app/components/map_locations/form_fields_component.rb delete mode 100644 app/views/map_locations/_form_fields.html.erb create mode 100644 spec/components/map_locations/form_fields_component_spec.rb diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index ce157d9db..a7e6e2d21 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -44,11 +44,12 @@ <%= render Documents::NestedComponent.new(f) %> <% end %> - <%= render "map_locations/form_fields", - form: f, - map_location: map_location, - label: t("budgets.investments.form.map_location"), - help: t("budgets.investments.form.map_location_instructions") %> + <%= render MapLocations::FormFieldsComponent.new( + f, + map_location: map_location, + label: t("budgets.investments.form.map_location"), + help: t("budgets.investments.form.map_location_instructions") + ) %>
<%= f.text_field :location %> diff --git a/app/components/map_locations/form_fields_component.html.erb b/app/components/map_locations/form_fields_component.html.erb new file mode 100644 index 000000000..9d8d02e10 --- /dev/null +++ b/app/components/map_locations/form_fields_component.html.erb @@ -0,0 +1,12 @@ +
+ <%= label %> +

<%= help %>

+ + <%= form.fields_for :map_location, map_location do |m_l_fields| %> + <%= render_map(map_location, form: m_l_fields) %> + + <%= m_l_fields.hidden_field :latitude %> + <%= m_l_fields.hidden_field :longitude %> + <%= m_l_fields.hidden_field :zoom %> + <% end %> +
diff --git a/app/components/map_locations/form_fields_component.rb b/app/components/map_locations/form_fields_component.rb new file mode 100644 index 000000000..d4235b3e5 --- /dev/null +++ b/app/components/map_locations/form_fields_component.rb @@ -0,0 +1,15 @@ +class MapLocations::FormFieldsComponent < ApplicationComponent + attr_reader :form, :map_location, :label, :help + use_helpers :render_map + + def initialize(form, map_location:, label:, help:) + @form = form + @map_location = map_location + @label = label + @help = help + end + + def render? + feature?(:map) + end +end diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index 9aec4e6eb..6e7fed36c 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -52,11 +52,12 @@
<% end %> - <%= render "map_locations/form_fields", - form: f, - map_location: map_location, - label: t("proposals.form.map_location"), - help: t("proposals.form.map_location_instructions") %> + <%= render MapLocations::FormFieldsComponent.new( + f, + map_location: proposal.map_location || MapLocation.new, + label: t("proposals.form.map_location"), + help: t("proposals.form.map_location_instructions") + ) %>
<%= f.label :tag_list, t("proposals.form.tags_label") %> diff --git a/app/views/map_locations/_form_fields.html.erb b/app/views/map_locations/_form_fields.html.erb deleted file mode 100644 index 0b0c519c8..000000000 --- a/app/views/map_locations/_form_fields.html.erb +++ /dev/null @@ -1,14 +0,0 @@ -<% if feature?(:map) %> -
- <%= label %> -

<%= help %>

- - <%= form.fields_for :map_location, map_location do |m_l_fields| %> - <%= render_map(map_location, form: m_l_fields) %> - - <%= m_l_fields.hidden_field :latitude %> - <%= m_l_fields.hidden_field :longitude %> - <%= m_l_fields.hidden_field :zoom %> - <% end %> -
-<% end %> diff --git a/spec/components/map_locations/form_fields_component_spec.rb b/spec/components/map_locations/form_fields_component_spec.rb new file mode 100644 index 000000000..d81508877 --- /dev/null +++ b/spec/components/map_locations/form_fields_component_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +describe MapLocations::FormFieldsComponent do + let(:proposal) { Proposal.new } + let(:map_location) { MapLocation.new } + let(:form) { ConsulFormBuilder.new(:proposal, proposal, ApplicationController.new.view_context, {}) } + let(:label) { "Map location" } + let(:help) { "Add a marker" } + let(:component) do + MapLocations::FormFieldsComponent.new(form, map_location: map_location, label: label, help: help) + end + + it "is rendered when the map feature is enabled" do + Setting["feature.map"] = true + + render_inline component + + expect(page).to be_rendered + end + + it "is not rendered when the map feature is not enabled" do + Setting["feature.map"] = false + + render_inline component + + expect(page).not_to be_rendered + end +end From b9adef481a9ff34df4a61c845997f4a1033f4b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 00:43:09 +0100 Subject: [PATCH 09/14] Simplify variable name in map location fields The `m_l` prefix isn't really necessary when we're talking about map locations, and sometimes when searching the project I think `m_l_` stands for "machine learning" and get confused. --- .../map_locations/form_fields_component.html.erb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/components/map_locations/form_fields_component.html.erb b/app/components/map_locations/form_fields_component.html.erb index 9d8d02e10..eb0643a77 100644 --- a/app/components/map_locations/form_fields_component.html.erb +++ b/app/components/map_locations/form_fields_component.html.erb @@ -2,11 +2,11 @@ <%= label %>

<%= help %>

- <%= form.fields_for :map_location, map_location do |m_l_fields| %> - <%= render_map(map_location, form: m_l_fields) %> + <%= form.fields_for :map_location, map_location do |fields| %> + <%= render_map(map_location, form: fields) %> - <%= m_l_fields.hidden_field :latitude %> - <%= m_l_fields.hidden_field :longitude %> - <%= m_l_fields.hidden_field :zoom %> + <%= fields.hidden_field :latitude %> + <%= fields.hidden_field :longitude %> + <%= fields.hidden_field :zoom %> <% end %> From 8a575ae83c66111166c5f55015541e67f302621d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 02:21:52 +0100 Subject: [PATCH 10/14] Remove duplicate map location translations We were using the same texts twice. For the remove marker label text, however, we were using the text defined in proposals for both proposals and investments. Ideally the translation keys for these texts would go in another namespace, since they no longer refer to just proposals. However, renaming the translation keys would mean losing the existing translations in every language we manage through Crowdin. So we aren't doing so. --- .../budgets/investments/form_component.html.erb | 7 +------ .../map_locations/form_fields_component.rb | 16 ++++++++++++---- app/components/proposals/form_component.html.erb | 7 +------ config/locales/en/budgets.yml | 2 -- config/locales/es/budgets.yml | 2 -- .../map_locations/form_fields_component_spec.rb | 6 +----- 6 files changed, 15 insertions(+), 25 deletions(-) diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index a7e6e2d21..e560a8112 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -44,12 +44,7 @@ <%= render Documents::NestedComponent.new(f) %> <% end %> - <%= render MapLocations::FormFieldsComponent.new( - f, - map_location: map_location, - label: t("budgets.investments.form.map_location"), - help: t("budgets.investments.form.map_location_instructions") - ) %> + <%= render MapLocations::FormFieldsComponent.new(f, map_location: map_location) %>
<%= f.text_field :location %> diff --git a/app/components/map_locations/form_fields_component.rb b/app/components/map_locations/form_fields_component.rb index d4235b3e5..585eff0b6 100644 --- a/app/components/map_locations/form_fields_component.rb +++ b/app/components/map_locations/form_fields_component.rb @@ -1,15 +1,23 @@ class MapLocations::FormFieldsComponent < ApplicationComponent - attr_reader :form, :map_location, :label, :help + attr_reader :form, :map_location use_helpers :render_map - def initialize(form, map_location:, label:, help:) + def initialize(form, map_location:) @form = form @map_location = map_location - @label = label - @help = help end def render? feature?(:map) end + + private + + def label + t("proposals.form.map_location") + end + + def help + t("proposals.form.map_location_instructions") + end end diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index 6e7fed36c..d3d6a4f1a 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -52,12 +52,7 @@
<% end %> - <%= render MapLocations::FormFieldsComponent.new( - f, - map_location: proposal.map_location || MapLocation.new, - label: t("proposals.form.map_location"), - help: t("proposals.form.map_location_instructions") - ) %> + <%= render MapLocations::FormFieldsComponent.new(f, map_location: map_location) %>
<%= f.label :tag_list, t("proposals.form.tags_label") %> diff --git a/config/locales/en/budgets.yml b/config/locales/en/budgets.yml index 384b55030..b6320ce64 100644 --- a/config/locales/en/budgets.yml +++ b/config/locales/en/budgets.yml @@ -80,8 +80,6 @@ en: tags_instructions: "Tag this proposal. You can choose from proposed categories or add your own" tags_label: Tags tags_placeholder: "Enter the tags you would like to use, separated by commas (',')" - map_location: "Map location" - map_location_instructions: "Navigate the map to the location and place the marker." index: title: Participatory budgeting unfeasible: Unfeasible investment projects diff --git a/config/locales/es/budgets.yml b/config/locales/es/budgets.yml index 53c5a4b35..489752b81 100644 --- a/config/locales/es/budgets.yml +++ b/config/locales/es/budgets.yml @@ -80,8 +80,6 @@ es: tags_instructions: "Etiqueta este proyecto. Puedes elegir entre las categorías propuestas o introducir las que desees" tags_label: Etiquetas tags_placeholder: "Escribe las etiquetas que desees separadas por coma (',')" - map_location: "Ubicación en el mapa" - map_location_instructions: "Navega por el mapa hasta la ubicación y coloca el marcador." index: title: Presupuestos participativos unfeasible: Proyectos de gasto no viables diff --git a/spec/components/map_locations/form_fields_component_spec.rb b/spec/components/map_locations/form_fields_component_spec.rb index d81508877..34c17d277 100644 --- a/spec/components/map_locations/form_fields_component_spec.rb +++ b/spec/components/map_locations/form_fields_component_spec.rb @@ -4,11 +4,7 @@ describe MapLocations::FormFieldsComponent do let(:proposal) { Proposal.new } let(:map_location) { MapLocation.new } let(:form) { ConsulFormBuilder.new(:proposal, proposal, ApplicationController.new.view_context, {}) } - let(:label) { "Map location" } - let(:help) { "Add a marker" } - let(:component) do - MapLocations::FormFieldsComponent.new(form, map_location: map_location, label: label, help: help) - end + let(:component) { MapLocations::FormFieldsComponent.new(form, map_location: map_location) } it "is rendered when the map feature is enabled" do Setting["feature.map"] = true From 1693aa5d9cfda3079889f40f3b01a5eefaf2143f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 00:44:51 +0100 Subject: [PATCH 11/14] Use render_map to render the admin settings map This way we remove duplication. Note that to check whether to render the button to remove a marker, we're checking whether the map location belongs to a mappable. This means we're changing the code that renders the map in the "new proposal" and "new investment" forms so the map location belongs to a proposal or investment. We're association the map location to a new record because writing something like: ``` def map_location proposal.map_location || MapLocation.new(proposal: proposal) end ``` Would change the `proposal` object because of the way Rails treats non-persisted `has_one` associations. Although probably safe in this case, changing an object when rendering a view could have side effects. Also note that we're changing the HTML ID of the map element from `admin-map` to `new_map_location` (the latter is returned by the `dom_id` method). We were only using this ID in tests since commit 289426c1c, so changing it doesn't really affect us. --- .../settings/map_form_component.html.erb | 24 +++----------- .../admin/settings/map_form_component.rb | 11 +++++++ .../budgets/investments/form_component.rb | 2 +- app/components/proposals/form_component.rb | 2 +- .../shared/map_location_component.html.erb | 2 +- .../shared/map_location_component.rb | 4 +++ app/controllers/admin/settings_controller.rb | 6 ++-- app/models/map_location.rb | 6 +++- .../admin/settings/map_form_component_spec.rb | 10 ++++++ .../investments/form_component_spec.rb | 13 ++++++++ .../proposals/form_component_spec.rb | 17 ++++++++++ .../shared/map_location_component_spec.rb | 31 +++++++++++++++++++ spec/system/admin/settings_spec.rb | 10 +++--- 13 files changed, 107 insertions(+), 31 deletions(-) create mode 100644 spec/components/admin/settings/map_form_component_spec.rb create mode 100644 spec/components/proposals/form_component_spec.rb create mode 100644 spec/components/shared/map_location_component_spec.rb diff --git a/app/components/admin/settings/map_form_component.html.erb b/app/components/admin/settings/map_form_component.html.erb index c9738230f..e4a2e7c48 100644 --- a/app/components/admin/settings/map_form_component.html.erb +++ b/app/components/admin/settings/map_form_component.html.erb @@ -1,24 +1,10 @@
- <%= form_tag admin_update_map_path, method: :put, id: "map-form" do |f| %> -
" - data-map-center-longitude="<%= Setting["map.longitude"] %>" - data-map-zoom="<%= Setting["map.zoom"] %>" - data-map-tiles-provider="<%= Rails.application.secrets.map_tiles_provider %>" - data-map-tiles-provider-attribution="<%= Rails.application.secrets.map_tiles_provider_attribution %>" - data-marker-editable="true" - data-marker-latitude="<%= Setting["map.latitude"] %>" - data-marker-longitude="<%= Setting["map.longitude"] %>" - data-latitude-input-selector="#latitude" - data-longitude-input-selector="#longitude" - data-zoom-input-selector="#zoom"> -
- - <%= hidden_field_tag :latitude, Setting["map.latitude"] %> - <%= hidden_field_tag :longitude, Setting["map.longitude"] %> - <%= hidden_field_tag :zoom, Setting["map.zoom"] %> + <%= form_for map_location, url: admin_update_map_path, method: :put, html: { id: "map-form" } do |f| %> + <%= render_map(map_location, form: f) %> + <%= f.hidden_field :latitude %> + <%= f.hidden_field :longitude %> + <%= f.hidden_field :zoom %> <%= hidden_field_tag :tab, tab if tab %>
diff --git a/app/components/admin/settings/map_form_component.rb b/app/components/admin/settings/map_form_component.rb index 81209a7ee..5bd8016d7 100644 --- a/app/components/admin/settings/map_form_component.rb +++ b/app/components/admin/settings/map_form_component.rb @@ -1,7 +1,18 @@ class Admin::Settings::MapFormComponent < ApplicationComponent attr_reader :tab + use_helpers :render_map def initialize(tab: nil) @tab = tab end + + private + + def map_location + MapLocation.new( + latitude: Setting["map.latitude"], + longitude: Setting["map.longitude"], + zoom: Setting["map.zoom"] + ) + end end diff --git a/app/components/budgets/investments/form_component.rb b/app/components/budgets/investments/form_component.rb index eb6c0c4ce..5e6ff31f1 100644 --- a/app/components/budgets/investments/form_component.rb +++ b/app/components/budgets/investments/form_component.rb @@ -21,6 +21,6 @@ class Budgets::Investments::FormComponent < ApplicationComponent end def map_location - investment.map_location || MapLocation.new + investment.map_location || MapLocation.new(investment: Budget::Investment.new) end end diff --git a/app/components/proposals/form_component.rb b/app/components/proposals/form_component.rb index b4a796229..5c3e71ae4 100644 --- a/app/components/proposals/form_component.rb +++ b/app/components/proposals/form_component.rb @@ -17,6 +17,6 @@ class Proposals::FormComponent < ApplicationComponent end def map_location - proposal.map_location || MapLocation.new + proposal.map_location || MapLocation.new(proposal: Proposal.new) end end diff --git a/app/components/shared/map_location_component.html.erb b/app/components/shared/map_location_component.html.erb index 378bf1dda..f5907fef2 100644 --- a/app/components/shared/map_location_component.html.erb +++ b/app/components/shared/map_location_component.html.erb @@ -1,5 +1,5 @@ <%= tag.div(id: dom_id(map_location), class: "map-location map", data: data) %> -<% if editable? %> +<% if show_remove_marker_button? %> <%= remove_marker %> <% end %> diff --git a/app/components/shared/map_location_component.rb b/app/components/shared/map_location_component.rb index fb399cc09..05c06dac7 100644 --- a/app/components/shared/map_location_component.rb +++ b/app/components/shared/map_location_component.rb @@ -18,6 +18,10 @@ class Shared::MapLocationComponent < ApplicationComponent form.present? end + def show_remove_marker_button? + editable? && map_location.mappable.present? + end + def latitude map_location.latitude.presence || Setting["map.latitude"] end diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index 2ee26ca2e..4ea0dbbc5 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/app/controllers/admin/settings_controller.rb @@ -13,9 +13,9 @@ class Admin::SettingsController < Admin::BaseController end def update_map - Setting["map.latitude"] = params[:latitude].to_f - Setting["map.longitude"] = params[:longitude].to_f - Setting["map.zoom"] = params[:zoom].to_i + Setting["map.latitude"] = params[:map_location][:latitude].to_f + Setting["map.longitude"] = params[:map_location][:longitude].to_f + Setting["map.zoom"] = params[:map_location][:zoom].to_i redirect_to request_referer, notice: t("admin.settings.index.map.flash.update") end diff --git a/app/models/map_location.rb b/app/models/map_location.rb index 34825782b..e44bfd0a4 100644 --- a/app/models/map_location.rb +++ b/app/models/map_location.rb @@ -8,8 +8,12 @@ class MapLocation < ApplicationRecord latitude.present? && longitude.present? && zoom.present? end + def mappable + proposal || investment + end + def title - (proposal || investment)&.title + mappable&.title end def json_data diff --git a/spec/components/admin/settings/map_form_component_spec.rb b/spec/components/admin/settings/map_form_component_spec.rb new file mode 100644 index 000000000..cd274efff --- /dev/null +++ b/spec/components/admin/settings/map_form_component_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +describe Admin::Settings::MapFormComponent do + it "does not render a button to remove the marker" do + render_inline Admin::Settings::MapFormComponent.new + + expect(page).to have_css ".map-location" + expect(page).not_to have_button "Remove map marker" + end +end diff --git a/spec/components/budgets/investments/form_component_spec.rb b/spec/components/budgets/investments/form_component_spec.rb index 22459cf09..dc3eb0e4b 100644 --- a/spec/components/budgets/investments/form_component_spec.rb +++ b/spec/components/budgets/investments/form_component_spec.rb @@ -33,4 +33,17 @@ describe Budgets::Investments::FormComponent do expect(page).not_to have_field "I agree to the Privacy Policy and the Terms and conditions of use" end end + + describe "map" do + it "renders a button to remove the map marker" do + Setting["feature.map"] = true + + render_inline Budgets::Investments::FormComponent.new( + budget.investments.new, + url: budget_investments_path(budget) + ) + + expect(page).to have_button "Remove map marker" + end + end end diff --git a/spec/components/proposals/form_component_spec.rb b/spec/components/proposals/form_component_spec.rb new file mode 100644 index 000000000..5c3272c75 --- /dev/null +++ b/spec/components/proposals/form_component_spec.rb @@ -0,0 +1,17 @@ +require "rails_helper" + +describe Proposals::FormComponent do + include Rails.application.routes.url_helpers + + before { sign_in(create(:user)) } + + describe "map" do + it "renders a button to remove the map marker" do + Setting["feature.map"] = true + + render_inline Proposals::FormComponent.new(Proposal.new, url: proposals_path) + + expect(page).to have_button "Remove map marker" + end + end +end diff --git a/spec/components/shared/map_location_component_spec.rb b/spec/components/shared/map_location_component_spec.rb new file mode 100644 index 000000000..efd71028b --- /dev/null +++ b/spec/components/shared/map_location_component_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" + +describe Shared::MapLocationComponent do + describe "remove marker button" do + it "is not rendered when there's no form" do + map_location = build(:map_location, proposal: Proposal.new) + + render_inline Shared::MapLocationComponent.new(map_location) + + expect(page).not_to have_button "Remove map marker" + end + + it "is not rendered when there's no mappable" do + map_location = build(:map_location) + form = ConsulFormBuilder.new(:map_location, map_location, ApplicationController.new.view_context, {}) + + render_inline Shared::MapLocationComponent.new(map_location, form: form) + + expect(page).not_to have_button "Remove map marker" + end + + it "is rendered when there's a form and a mappable" do + map_location = build(:map_location, proposal: Proposal.new) + form = ConsulFormBuilder.new(:map_location, map_location, ApplicationController.new.view_context, {}) + + render_inline Shared::MapLocationComponent.new(map_location, form: form) + + expect(page).to have_button "Remove map marker" + end + end +end diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index adde6dfb2..68f6c3ba5 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -26,11 +26,11 @@ describe "Admin settings", :admin do visit admin_settings_path - expect(page).not_to have_css "#admin-map.leaflet-container", visible: :all + expect(page).not_to have_css ".map-location.leaflet-container", visible: :all click_link "Map configuration" - expect(page).to have_css "#admin-map.leaflet-container" + expect(page).to have_css ".map-location.leaflet-container" end end @@ -44,7 +44,7 @@ describe "Admin settings", :admin do expect(page).to have_content "To show the map to users you must enable " \ '"Proposals and budget investments geolocation" ' \ 'on "Features" tab.' - expect(page).not_to have_css("#admin-map") + expect(page).not_to have_css ".map-location" end scenario "Should update marker" do @@ -53,7 +53,7 @@ describe "Admin settings", :admin do visit admin_settings_path click_link "Map configuration" - expect(page).to have_css("#admin-map") + expect(page).to have_css ".map-location" expect(page).not_to have_content "To show the map to users you must enable " \ '"Proposals and budget investments geolocation" ' \ 'on "Features" tab.' @@ -62,7 +62,7 @@ describe "Admin settings", :admin do expect(page).to have_field "Longitude", with: "0.0" within "#map-form" do - find("#admin-map").click + find(".map-location").click click_button "Update" end From 99696cb302535933babc8bbcc45343e08845bc77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 01:08:22 +0100 Subject: [PATCH 12/14] Add aria-label to markers in admin map settings We forgot to do so in commit b896fc4bb. Back then, we said: > Note that we aren't providing a proper aria-label for markers on the > map we use in the form to create a proposal or an investment. Adding > one isn't trivial given the current code, and keyboard users can't add > a marker in the first place. We'll have to revisit this issue when we > add keyboard support for this. However, in the admin section, the marker is already there, so it should have a label. In this case, we're using the coordinates as label because it's the most relevant text for the marker in the context of a form. We could also use "Default map location" instead, but that information is already present on the page. Axe was reporting the same accessibility error we mentioned in commit b896fc4bb in this situation. --- .../shared/map_location_component.rb | 10 ++++- config/locales/en/general.yml | 1 + config/locales/es/general.yml | 1 + .../shared/map_location_component_spec.rb | 44 +++++++++++++++++++ spec/system/admin/settings_spec.rb | 3 ++ 5 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/components/shared/map_location_component.rb b/app/components/shared/map_location_component.rb index 05c06dac7..bd59e52db 100644 --- a/app/components/shared/map_location_component.rb +++ b/app/components/shared/map_location_component.rb @@ -62,12 +62,20 @@ class Shared::MapLocationComponent < ApplicationComponent marker_investments_coordinates: investments_coordinates, marker_latitude: map_location.latitude.presence, marker_longitude: map_location.longitude.presence, - marker_title: map_location.title.presence, + marker_title: map_location.title.presence || marker_coordinates_text, marker_clustering: feature?("map.feature.marker_clustering"), geozones: geozones_data }.merge(input_selectors) end + def marker_coordinates_text + return unless map_location.available? + + t("proposals.form.map_marker_coordinates", + latitude: map_location.latitude, + longitude: map_location.longitude) + end + def input_selectors if form { diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 269452fb7..b54745690 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -328,6 +328,7 @@ en: tags_placeholder: "Enter the tags you would like to use, separated by commas (',')" map_location: "Map location" map_location_instructions: "Navigate the map to the location and place the marker." + map_marker_coordinates: "Latitude: %{latitude}. Longitude: %{longitude}" map_remove_marker: "Remove map marker" index: featured_proposals: Featured diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 9bcb5566a..514378598 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -328,6 +328,7 @@ es: tags_placeholder: "Escribe las etiquetas que desees separadas por coma (',')" map_location: "Ubicación en el mapa" map_location_instructions: "Navega por el mapa hasta la ubicación y coloca el marcador." + map_marker_coordinates: "Latitud: %{latitude}. Longitud: %{longitude}" map_remove_marker: "Eliminar el marcador" index: featured_proposals: Destacadas diff --git a/spec/components/shared/map_location_component_spec.rb b/spec/components/shared/map_location_component_spec.rb index efd71028b..be8b045ba 100644 --- a/spec/components/shared/map_location_component_spec.rb +++ b/spec/components/shared/map_location_component_spec.rb @@ -28,4 +28,48 @@ describe Shared::MapLocationComponent do expect(page).to have_button "Remove map marker" end end + + describe "marker title" do + it "uses the mappable title when there's a mappable with a title" do + map_location = build( + :map_location, + proposal: Proposal.new(title: "Meet me here"), + latitude: "25.25", + longitude: "13.14" + ) + + render_inline Shared::MapLocationComponent.new(map_location) + + expect(page).to have_css "[data-marker-title='Meet me here']" + end + + it "uses the coordinates when there's no mappable" do + map_location = build(:map_location, latitude: "25.25", longitude: "13.14") + + render_inline Shared::MapLocationComponent.new(map_location) + + expect(page).to have_css "[data-marker-title='Latitude: 25.25. Longitude: 13.14']" + end + + it "uses the coordinates when the mappable has an empty title" do + map_location = build( + :map_location, + proposal: Proposal.new(title: ""), + latitude: "25.25", + longitude: "13.14" + ) + + render_inline Shared::MapLocationComponent.new(map_location) + + expect(page).to have_css "[data-marker-title='Latitude: 25.25. Longitude: 13.14']" + end + + it "is not present when the map location isn't available" do + map_location = build(:map_location, latitude: "25.25", longitude: nil) + + render_inline Shared::MapLocationComponent.new(map_location) + + expect(page).not_to have_css "[data-marker-title]" + end + end end diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index 68f6c3ba5..5917591e6 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -60,6 +60,7 @@ describe "Admin settings", :admin do expect(page).to have_field "Latitude", with: "51.48" expect(page).to have_field "Longitude", with: "0.0" + expect(page).to have_css ".map-icon[aria-label='Latitude: 51.48. Longitude: 0.0']" within "#map-form" do find(".map-location").click @@ -68,7 +69,9 @@ describe "Admin settings", :admin do expect(page).to have_content "Map configuration updated successfully" expect(page).to have_field "Latitude" + expect(page).to have_css ".map-icon" expect(page).not_to have_field "Latitude", with: "51.48" + expect(page).not_to have_css ".map-icon[aria-label='Latitude: 51.48. Longitude: 0.0']" end end From 288f62cdd2041af28ba17904987d26877979c153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 04:24:08 +0100 Subject: [PATCH 13/14] Use coordinates as marker labels when there's only one mappable When editing/showing a proposal or an investment, the most relevant information regarding the marker are the coordinates. The title of the proposal or investment is redundant because we already know the marker is about that proposal/investment. There's one problem with this approach, though: when editing a proposal or an investment, the aria-label of the marker isn't updated automatically when we move the marker to a different place. This behaviour will only affect people who use both a screen reader and a mouse, since keyboard users can't change the position of the marker in the first place. We'll deal with this issue when we make it possible to change the position of a marker using the keyboard. --- app/components/shared/map_location_component.rb | 2 +- .../shared/map_location_component_spec.rb | 17 ++--------------- spec/shared/system/mappable.rb | 5 ++++- 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/app/components/shared/map_location_component.rb b/app/components/shared/map_location_component.rb index bd59e52db..11bcbccba 100644 --- a/app/components/shared/map_location_component.rb +++ b/app/components/shared/map_location_component.rb @@ -62,7 +62,7 @@ class Shared::MapLocationComponent < ApplicationComponent marker_investments_coordinates: investments_coordinates, marker_latitude: map_location.latitude.presence, marker_longitude: map_location.longitude.presence, - marker_title: map_location.title.presence || marker_coordinates_text, + marker_title: marker_coordinates_text, marker_clustering: feature?("map.feature.marker_clustering"), geozones: geozones_data }.merge(input_selectors) diff --git a/spec/components/shared/map_location_component_spec.rb b/spec/components/shared/map_location_component_spec.rb index be8b045ba..f16a85e31 100644 --- a/spec/components/shared/map_location_component_spec.rb +++ b/spec/components/shared/map_location_component_spec.rb @@ -30,7 +30,7 @@ describe Shared::MapLocationComponent do end describe "marker title" do - it "uses the mappable title when there's a mappable with a title" do + it "uses the coordinates when there's a mappable" do map_location = build( :map_location, proposal: Proposal.new(title: "Meet me here"), @@ -40,7 +40,7 @@ describe Shared::MapLocationComponent do render_inline Shared::MapLocationComponent.new(map_location) - expect(page).to have_css "[data-marker-title='Meet me here']" + expect(page).to have_css "[data-marker-title='Latitude: 25.25. Longitude: 13.14']" end it "uses the coordinates when there's no mappable" do @@ -51,19 +51,6 @@ describe Shared::MapLocationComponent do expect(page).to have_css "[data-marker-title='Latitude: 25.25. Longitude: 13.14']" end - it "uses the coordinates when the mappable has an empty title" do - map_location = build( - :map_location, - proposal: Proposal.new(title: ""), - latitude: "25.25", - longitude: "13.14" - ) - - render_inline Shared::MapLocationComponent.new(map_location) - - expect(page).to have_css "[data-marker-title='Latitude: 25.25. Longitude: 13.14']" - end - it "is not present when the map location isn't available" do map_location = build(:map_location, latitude: "25.25", longitude: nil) diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index b0d33be04..1b130a46f 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -175,6 +175,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, visit send(mappable_edit_path, id: mappable.id) expect(page).to have_content "Navigate the map to the location and place the marker." + expect(page).to have_css ".map-icon[aria-label='Latitude: 51.48. Longitude: 0.0']" expect(page).to have_field "#{mappable_factory_name}_map_location_attributes_latitude", type: :hidden, with: "51.48" expect(page).to have_field "#{mappable_factory_name}_map_location_attributes_longitude", type: :hidden, @@ -255,8 +256,10 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, do_login_for user, management: management if management visit send(mappable_show_path, arguments) + label = "Latitude: #{map_location.latitude}. Longitude: #{map_location.longitude}" + within ".map-location" do - expect(page).to have_css ".map-icon[aria-label='#{mappable.title}']" + expect(page).to have_css ".map-icon[aria-label='#{label}']" end end From 5a432da4986b490fe4f984edeea297627da4a3fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 17 Nov 2025 03:19:31 +0100 Subject: [PATCH 14/14] Update old usages of investments JSON data We aren't using these properties since commit 3fa3c90db. An old test was failing when checking for Axe accessibility issues because of this. --- app/assets/javascripts/map.js | 1 - app/models/map_location.rb | 9 --------- spec/system/budgets/budgets_spec.rb | 8 ++++---- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index ff4ce0dd0..0684b654f 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -193,7 +193,6 @@ if (App.Map.validCoordinates(coordinates)) { marker = createMarker(coordinates.lat, coordinates.long, coordinates.title); - marker.options.id = coordinates.investment_id; marker.bindPopup(App.Map.getPopupContent(coordinates)); } }); diff --git a/app/models/map_location.rb b/app/models/map_location.rb index e44bfd0a4..2573a08fb 100644 --- a/app/models/map_location.rb +++ b/app/models/map_location.rb @@ -16,15 +16,6 @@ class MapLocation < ApplicationRecord mappable&.title end - def json_data - { - investment_id: investment_id, - proposal_id: proposal_id, - lat: latitude, - long: longitude - } - end - def self.from_heading(heading) new( zoom: Budget::Heading::OSM_DISTRICT_LEVEL_ZOOM, diff --git a/spec/system/budgets/budgets_spec.rb b/spec/system/budgets/budgets_spec.rb index b21d9a4aa..e272a931b 100644 --- a/spec/system/budgets/budgets_spec.rb +++ b/spec/system/budgets/budgets_spec.rb @@ -305,9 +305,8 @@ describe "Budgets" do { lat: map_location[:latitude], long: map_location[:longitude], - investment_title: investment.title, - investment_id: investment.id, - budget_id: budget.id + title: investment.title, + link: "/budgets/#{budget.id}/investments/#{investment.id}" } end @@ -316,7 +315,8 @@ describe "Budgets" do visit budgets_path within ".map-location" do - expect(page).to have_css(".map-icon", count: 1, visible: :all) + expect(page).to have_css ".map-icon", count: 1, visible: :all + expect(page).to have_css ".map-icon[aria-label='#{investment.title}']", visible: :all end end