Avoid removing other proposals map locations

It was possible to remove a map location from a different proposal (even
one created by a different author) by modifying the hidden `id`
parameter in the form.

So we're making sure the map location we destroy is the one associated
to the proposal we're updating.

Since we're now using the `@proposal` instance variable in the
`destroy_map_location_association` method, we're calling that method
after loading the resource with cancancan.

app/controllers/proposals_controller.rb

@@ -10,7 +10,6 @@ class ProposalsController < ApplicationController
   before_action :load_categories, only: [:index, :map, :summary]
   before_action :load_geozones, only: [:edit, :map, :summary]
   before_action :authenticate_user!, except: [:index, :show, :map, :summary]
-  before_action :destroy_map_location_association, only: :update
   before_action :set_view, only: :index
   before_action :proposals_recommendations, only: :index, if: :current_user
 
@@ -22,6 +21,8 @@ class ProposalsController < ApplicationController
   has_orders %w[most_voted newest oldest], only: :show
 
   load_and_authorize_resource
+  before_action :destroy_map_location_association, only: :update
+
   helper_method :resource_model, :resource_name
   respond_to :html, :js
 
@@ -166,9 +167,10 @@ class ProposalsController < ApplicationController
     end
 
     def destroy_map_location_association
-      map_location = params[:proposal][:map_location_attributes]
-      if map_location && (map_location[:longitude] && map_location[:latitude]).blank? && !map_location[:id].blank?
-        MapLocation.destroy(map_location[:id])
+      map_location = proposal_params[:map_location_attributes]
+
+      if map_location.blank? || map_location.values.all?(&:blank?)
+        @proposal.map_location = nil
       end
     end
 

app/views/map_locations/_form_fields.html.erb

@@ -4,9 +4,6 @@
 <%= render_map(map_location, parent_class, editable = true, remove_marker_label) %>
 
 <%= form.fields_for :map_location, map_location do |m_l_fields| %>
-  <%= m_l_fields.hidden_field :id,
-                              value: map_location.id,
-                              id: map_location_input_id(parent_class, "id") %>
   <%= m_l_fields.hidden_field :latitude,
                               value: map_location.latitude,
                               id: map_location_input_id(parent_class, "latitude") %>

spec/controllers/proposals_controller_spec.rb

@@ -8,4 +8,27 @@ describe ProposalsController do
       expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled)
     end
   end
+
+  describe "PATCH update" do
+    before { InvisibleCaptcha.timestamp_enabled = false }
+    after { InvisibleCaptcha.timestamp_enabled = true }
+
+    it "does not delete other proposal's map location" do
+      proposal = create(:proposal)
+      other_proposal = create(:proposal, :with_map_location)
+
+      sign_in(proposal.author)
+
+      patch :update, params: {
+        proposal: {
+          map_location_attributes: { id: other_proposal.map_location.id },
+          responsible_name: "Skinny Fingers"
+        },
+        id: proposal.id
+      }
+
+      expect(proposal.reload.responsible_name).to eq "Skinny Fingers"
+      expect(other_proposal.reload.map_location).not_to be nil
+    end
+  end
 end

spec/factories/proposals.rb

@@ -69,6 +69,10 @@ FactoryBot.define do
       published_at { Time.current }
     end
 
+    trait :with_map_location do
+      map_location
+    end
+
     trait :with_milestone_tags do
       after(:create) { |proposal| proposal.milestone_tags << create(:tag, :milestone) }
     end