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] 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