From 593527f0fab3e918dae5229c2649a62bc98caa31 Mon Sep 17 00:00:00 2001
From: Kat <katrina@kosiak.co.uk>
Date: Tue, 27 Jun 2023 13:56:25 +0100
Subject: [PATCH] Do not allow removing managing agents as data providers and
 fix remove_stock_owner

---
 .../organisation_relationships_controller.rb  | 12 +++++++---
 .../organisation_relationship_policy.rb       |  4 ++++
 ...anisation_relationships_controller_spec.rb | 24 +++++++++++++------
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/app/controllers/organisation_relationships_controller.rb b/app/controllers/organisation_relationships_controller.rb
index 9573a5dfb..443d4a505 100644
--- a/app/controllers/organisation_relationships_controller.rb
+++ b/app/controllers/organisation_relationships_controller.rb
@@ -65,8 +65,8 @@ class OrganisationRelationshipsController < ApplicationController
 
   def remove_stock_owner
     organisation_relationship = OrganisationRelationship.find_by!(
-      parent_organisation: organisation,
-      child_organisation: @target_organisation,
+      parent_organisation: @target_organisation,
+      child_organisation: organisation,
     )
     authorize organisation_relationship
   end
@@ -80,7 +80,13 @@ class OrganisationRelationshipsController < ApplicationController
     redirect_to stock_owners_organisation_path
   end
 
-  def remove_managing_agent; end
+  def remove_managing_agent
+    organisation_relationship = OrganisationRelationship.find_by!(
+      parent_organisation: organisation,
+      child_organisation: @target_organisation,
+    )
+    authorize organisation_relationship
+  end
 
   def delete_managing_agent
     OrganisationRelationship.find_by!(
diff --git a/app/policies/organisation_relationship_policy.rb b/app/policies/organisation_relationship_policy.rb
index 79540f20d..4ed5b7222 100644
--- a/app/policies/organisation_relationship_policy.rb
+++ b/app/policies/organisation_relationship_policy.rb
@@ -17,4 +17,8 @@ class OrganisationRelationshipPolicy
   def create_managing_agent?
     return true unless user.data_provider?
   end
+
+  def remove_managing_agent?
+    return true unless user.data_provider?
+  end
 end
diff --git a/spec/requests/organisation_relationships_controller_spec.rb b/spec/requests/organisation_relationships_controller_spec.rb
index be366a7c7..a5e5f8a08 100644
--- a/spec/requests/organisation_relationships_controller_spec.rb
+++ b/spec/requests/organisation_relationships_controller_spec.rb
@@ -318,21 +318,17 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
       end
 
       context "when directly removing a stock owner" do
-        let(:managing_agent) { FactoryBot.create(:organisation) }
-        let(:request) { get "/organisations/#{organisation.id}/stock-owners/remove?target_organisation_id=#{managing_agent.id}", headers: }
+        let(:stock_owner) { FactoryBot.create(:organisation) }
+        let(:request) { get "/organisations/#{organisation.id}/stock-owners/remove?target_organisation_id=#{stock_owner.id}", headers: }
 
         before do
-          FactoryBot.create(:organisation_relationship, parent_organisation: organisation, child_organisation: managing_agent)
+          FactoryBot.create(:organisation_relationship, parent_organisation: stock_owner, child_organisation: organisation)
         end
 
         it "returns 401 from users page" do
           request
           expect(response).to have_http_status(:unauthorized)
         end
-
-        it "does not remove the organisation relationship" do
-          expect { request }.not_to change(OrganisationRelationship, :count)
-        end
       end
 
       context "when directly adding a managing agent" do
@@ -356,6 +352,20 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
         end
       end
 
+      context "when directly removing a managing agent" do
+        let(:managing_agent) { FactoryBot.create(:organisation) }
+        let(:request) { get "/organisations/#{organisation.id}/managing-agents/remove?target_organisation_id=#{managing_agent.id}", headers: }
+
+        before do
+          FactoryBot.create(:organisation_relationship, parent_organisation: organisation, child_organisation: managing_agent)
+        end
+
+        it "returns 401 from users page" do
+          request
+          expect(response).to have_http_status(:unauthorized)
+        end
+      end
+
       context "when accessing the managing agents tab" do
         context "with an organisation that the user belongs to" do
           let!(:managing_agent) { FactoryBot.create(:organisation) }