Fixes multiple assignment bug

app/controllers/sites_controller.rb

@@ -42,7 +42,7 @@ def show
   def new
     do_new_resource
     get_project_if_exists
-    do_set_attributes
+    do_set_attributes(site_params(create: true))
     do_authorize_instance
 
     # initialize lat/lng to Brisbane-ish
@@ -65,7 +65,7 @@ def edit
   # POST /projects/:project_id/sites
   def create
     do_new_resource
-    do_set_attributes(site_params)
+    do_set_attributes(site_params(for_create: true))
     get_project_if_exists
     do_authorize_instance
 
@@ -96,7 +96,7 @@ def update
     @original_site_name = @site.name
 
     respond_to do |format|
-      if @site.update(site_params)
+      if @site.update(site_params(for_create: false))
         format.html { redirect_to [@project, @site], notice: 'Site was successfully updated.' }
         format.json { respond_show }
       else
@@ -225,16 +225,23 @@ def nav_menu
 
   def get_project
     @project = Project.find(params[:project_id])
-    # avoid the same project assigned more than once to a site
-    @site.projects << @project if defined?(@site) && [email protected]?(@project)
   end
 
   def get_project_if_exists
+    # none of this matters for the shallow routes
     return unless params.key?(:project_id)
 
-    @project = Project.find(params[:project_id])
-    # avoid the same project assigned more than once to a site
-    @site.projects << @project if defined?(@site) && defined?(@project) && [email protected]?(@project)
+    project_id = params[:project_id].to_i
+
+    # for show/edit/update, check that the site belongs to the project
+    if @site.present? && [email protected]_record? && [email protected]_ids.include?(project_id)
+
+      # this site does not belong to the project in the route parameter
+      raise ActiveRecord::RecordNotFound
+    end
+
+    # for index just load it to use for the permissions check
+    @project = Project.find(project_id)
   end
 
   def list_permissions
@@ -245,13 +252,27 @@ def list_permissions
     end
   end
 
-  def site_params
-    params.require(:site).permit(
+  def site_params(for_create:)
+    result = params.require(:site).permit(
       :name, :latitude, :longitude, :description, :image, :notes, :tzinfo_tz, :region_id, project_ids: []
     )
-  end
 
-  def site_show_params
-    params.permit(:id, :project_id, site: {})
+    # normalize the project_ids between the route parameter and the body
+    # route param does not exist for shallow routes
+    if params.key?(:project_id)
+      # is it also in the body?
+      if result.key?(:project_ids)
+        # if so it should be a subset of the supplied project_ids
+        unless result[:project_ids].include?(params[:project_id].to_i)
+          raise CustomErrors::BadRequestError, '`project_ids` must include the project id in the route parameter'
+        end
+      elsif for_create
+        # otherwise fill the body with the route parameter
+        # but not for updates - we don't want to change the project unless explicitly requested
+        result[:project_ids] = [params[:project_id].to_i]
+      end
+    end
+
+    result
   end
 end

app/models/site.rb

@@ -100,6 +100,11 @@ class Site < ApplicationRecord
     content_type: %r{^image/(jpg|jpeg|pjpeg|png|x-png|gif)$},
     message: 'file type %<value>s is not allowed (only jpeg/png/gif images)'
 
+  # AT 2024: soft deprecating sites existing in more than one project
+  # Causes many issues and is officially replaced by the project-region-site relationship
+  # Later work will remove the projects_sites join table
+  validate :only_one_site_per_project, on: :create
+
   # commonly used queries
   #scope :specified_sites, lambda { |site_ids| where('id in (:ids)', { :ids => site_ids } ) }
   #scope :sites_in_project, lambda { |project_ids| where(Project.specified_projects, { :ids => project_ids } ) }
@@ -222,6 +227,12 @@ def self.add_location_jitter(value, min, max, seed)
     modified_value
   end
 
+  def only_one_site_per_project
+    return if project_ids.count == 1
+
+    errors.add(:project_ids, 'Site must belong to exactly one project')
+  end
+
   # Define filter api settings
   def self.filter_settings
     {

spec/factories/site_factory.rb

@@ -44,6 +44,7 @@
     sequence(:description) { |n| "site description #{n}" }
 
     creator
+    projects { [create(:project)] }
 
     trait :with_lat_long do
       # Random.rand returns "a random integer greater than or equal to zero and less than the argument"
@@ -65,7 +66,7 @@
         raise 'Creator was blank' if evaluator.creator.blank?
 
         create_list(:audio_recording_with_audio_events_and_bookmarks, evaluator.audio_recording_count,
-                    site: site, creator: evaluator.creator, uploader: evaluator.creator)
+          site:, creator: evaluator.creator, uploader: evaluator.creator)
       end
     end
 

spec/models/site_spec.rb

@@ -193,8 +193,8 @@
   it { is_expected.to belong_to(:deleter).with_foreign_key(:deleter_id).optional }
 
   it 'errors on checking orphaned site if site is orphaned' do
-    # depends on factory not automatically associating a site with any projects
     site = create(:site)
+    site.projects = []
     expect {
       Access::Core.check_orphan_site!(site)
     }.to raise_error(CustomErrors::OrphanedSiteError)

spec/requests/sites_spec.rb

@@ -12,7 +12,7 @@
         }
       }
       post "/projects/#{project.id}/sites", params: body, headers: api_request_headers(owner_token, send_body: true),
-as: :json
+        as: :json
       expect(response).to have_http_status(:success)
       expect(api_result).to include(data: hash_including({
         timezone_information: hash_including({
@@ -35,7 +35,8 @@
       expect_error(400, 'Site testy test site () is not in any projects.')
     end
 
-    example 'can create an a site that belongs to multiple projects' do
+    # AT 2024: soft-deprecating the many to many project-site relationship
+    example 'canNOT create an a site that belongs to multiple projects' do
       second_project = create(:project)
       body = {
         site: {
@@ -46,14 +47,9 @@
 
       post '/sites', params: body, headers: api_request_headers(owner_token, send_body: true), as: :json
 
-      aggregate_failures do
-        expect(response).to have_http_status(:success)
-        expect(api_result).to include({
-          data: hash_including({
-            project_ids: match_array([project.id, second_project.id])
-          })
-        })
-      end
+      expect_error(:unprocessable_entity, 'Record could not be saved', {
+        project_ids: ['Site must belong to exactly one project']
+      })
     end
 
     example 'update a site so that it belongs to multiple projects' do
@@ -118,4 +114,167 @@
       )])
     end
   end
+
+  describe 'accidental assignment bug' do
+    # https://github.com/QutEcoacoustics/baw-server/issues/679
+    create_entire_hierarchy
+
+    let(:site) { create(:site) }
+    let(:another_site) { create(:site) }
+
+    def assert_isolation
+      site.reload
+      another_site.reload
+
+      aggregate_failures do
+        expect(another_site.project_ids).to match([another_site.projects.first[:id]])
+        expect(another_site.project_ids).not_to include(*site.projects.map(&:id))
+
+        expect(site.projects.map(&:site_ids)).not_to include(another_site.id)
+        expect(another_site.projects.map(&:site_ids)).not_to include(site.id)
+
+        expect(site.project_ids).to match(site.projects.map(&:id))
+        expect(site.project_ids).not_to include(*another_site.projects.map(&:id))
+      end
+    end
+
+    before do
+      Permission.new(
+        user: owner_user, project: site.projects.first, level: :owner,
+        creator: admin_user
+      ).save!
+      Permission.new(
+        user: owner_user, project: another_site.projects.first, level: :owner, creator: admin_user
+      ).save!
+
+      assert_isolation
+    end
+
+    after do
+      assert_isolation
+    end
+
+    it 'does not assign a site to the wrong project' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}", **api_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'does not assign a site to the wrong project (anonymous)' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}", **api_headers(anonymous_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'does not assign a site to the wrong project (filter)' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}/filter", **api_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested page.')
+    end
+
+    it 'does not assign a site to the wrong project (update)' do
+      body = {
+        site: {
+          name: 'new name'
+        }
+      }
+
+      patch "/projects/#{site.projects.first.id}/sites/#{another_site.id}", params: body,
+        **api_with_body_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'we can still create sites' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'we can still create sites (but we cant mix ids)' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [another_site.projects.first.id]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_error(:bad_request,
+        'The request was not valid: `project_ids` must include the project id in the route parameter')
+    end
+
+    it 'we can still create sites (with just a route parameter)' do
+      body = {
+        site: {
+          name: 'new name'
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'we can still create sites (with just a body parameter)' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id]
+        }
+      }
+
+      post '/sites', params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'rejects multiple project ids' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id, another_site.project_ids.first]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_error(:unprocessable_entity, 'Record could not be saved', {
+        project_ids: ['Site must belong to exactly one project']
+      })
+    end
+
+    it 'fails if project id is invalid' do
+      get "/projects/999999/sites/#{another_site.id}", **api_headers(owner_token)
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    # lastly some sanity checks
+
+    it 'can list sites' do
+      get "/projects/#{site.projects.first.id}/sites", **api_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'can show a site' do
+      get "/projects/#{site.projects.first.id}/sites/#{site.id}", **api_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'can filter sites' do
+      get "/projects/#{site.projects.first.id}/sites/filter", **api_headers(owner_token)
+
+      expect_success
+    end
+  end
 end

spec/requests/stats_spec.rb

@@ -23,7 +23,7 @@
         users_online: an_instance_of(Integer),
         users_total: User.count,
         online_window_start: an_instance_of(String),
-        projects_total: 2,
+        projects_total: 3,
         regions_total: 1,
         sites_total: 3,
         annotations_total: 2,

spec/support/creation_helper.rb

@@ -381,8 +381,8 @@ def create_region(creator, project)
       end
 
       def create_site(creator, project, region: nil, name: nil)
-        site = FactoryBot.create(:site, :with_lat_long, creator:)
-        site.projects << project
+        site = FactoryBot.build(:site, :with_lat_long, creator:, projects: [project])
+
         site.region = region unless region.nil?
         site.name = name unless name.nil?
         site.save!