Update user permissions and absorbing org question order

app/controllers/merge_requests_controller.rb

@@ -13,7 +13,7 @@ class MergeRequestsController < ApplicationController
     merge_date
   ]
   before_action :authenticate_user!
-  before_action :authenticate_scope!, except: [:create]
+  before_action :authenticate_scope!
 
   def absorbing_organisation; end
   def confirm_telephone_number; end
@@ -26,9 +26,8 @@ class MergeRequestsController < ApplicationController
   def create
     ActiveRecord::Base.transaction do
       @merge_request = MergeRequest.create!(merge_request_params.merge(status: :incomplete))
-      MergeRequestOrganisation.create!({ merge_request: @merge_request, merging_organisation: @merge_request.requesting_organisation })
     end
-    redirect_to organisations_merge_request_path(@merge_request)
+    redirect_to absorbing_organisation_merge_request_path(@merge_request)
   rescue ActiveRecord::RecordInvalid
     render_not_found
   end
@@ -72,13 +71,13 @@ private
   def next_page_path
     case page
     when "absorbing_organisation"
+      organisations_merge_request_path(@merge_request)
+    when "organisations"
       if create_new_organisation?
         new_organisation_name_merge_request_path(@merge_request)
       else
         confirm_telephone_number_merge_request_path(@merge_request)
       end
-    when "organisations"
-      absorbing_organisation_merge_request_path(@merge_request)
     when "confirm_telephone_number"
       merge_date_merge_request_path(@merge_request)
     when "new_organisation_name"
@@ -122,9 +121,7 @@ private
       :new_organisation_telephone_number,
     )
 
-    if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?)
+    merge_params[:requesting_organisation_id] = current_user.organisation.id
-      merge_params[:requesting_organisation_id] = current_user.organisation.id
-    end
 
     if merge_params[:absorbing_organisation_id].present?
       if create_new_organisation?
@@ -172,7 +169,7 @@ private
   end
 
   def authenticate_scope!
-    if current_user.organisation != @merge_request.requesting_organisation && !current_user.support?
+    unless current_user.support?
       render_not_found
     end
   end

spec/requests/merge_requests_controller_spec.rb

@@ -10,11 +10,14 @@ RSpec.describe MergeRequestsController, type: :request do
   let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) }
   let(:other_merge_request) { MergeRequest.create!(requesting_organisation: other_organisation) }
 
-  context "when user is signed in with a data coordinator user" do
+  context "when user is signed in with a support user" do
-    before { sign_in user }
+    before do
+      allow(support_user).to receive(:need_two_factor_authentication?).and_return(false)
+      sign_in support_user
+    end
 
     describe "#organisations" do
-      let(:params) { { merge_request: { requesting_organisation_id: "9", status: "incomplete" } } }
+      let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id, status: "incomplete" } } }
 
       context "when creating a new merge request" do
         before do
@@ -23,9 +26,8 @@ RSpec.describe MergeRequestsController, type: :request do
 
         it "creates merge request with requesting organisation" do
           follow_redirect!
-          expect(page).to have_content("Which organisations are merging?")
+          expect(page).to have_content("Which organisation is absorbing the others?")
-          expect(page).to have_content(organisation.name)
+          expect(page).to have_content(support_user.organisation.name)
-          expect(page).not_to have_link("Remove")
         end
 
         context "when passing a different requesting organisation id" do
@@ -34,9 +36,8 @@ RSpec.describe MergeRequestsController, type: :request do
           it "creates merge request with current user organisation" do
             follow_redirect!
             expect(MergeRequest.count).to eq(1)
-            expect(MergeRequest.first.requesting_organisation_id).to eq(organisation.id)
+            expect(MergeRequest.first.requesting_organisation_id).to eq(support_user.organisation_id)
-            expect(MergeRequest.first.merging_organisations.count).to eq(1)
+            expect(MergeRequest.first.merging_organisations.count).to eq(0)
-            expect(MergeRequest.first.merging_organisations.first.id).to eq(organisation.id)
           end
         end
       end
@@ -51,16 +52,6 @@ RSpec.describe MergeRequestsController, type: :request do
           expect(page).to have_content(organisation.name)
         end
       end
-
-      context "when viewing existing merge request of a different (unauthorised) organisation" do
-        before do
-          get "/merge-request/#{other_merge_request.id}/organisations", headers:, params:
-        end
-
-        it "shows merge request with requesting organisation" do
-          expect(response).to have_http_status(:not_found)
-        end
-      end
     end
 
     describe "#update_organisations" do
@@ -266,32 +257,6 @@ RSpec.describe MergeRequestsController, type: :request do
     end
 
     describe "#update" do
-      before { sign_in user }
-
-      describe "#other_merging_organisations" do
-        let(:other_merging_organisations) { "A list of other merging organisations" }
-        let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } }
-        let(:request) do
-          patch "/merge-request/#{merge_request.id}", headers:, params:
-        end
-
-        context "when adding other merging organisations" do
-          before do
-            MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id)
-          end
-
-          it "updates the merge request" do
-            expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations)
-          end
-
-          it "redirects telephone number path" do
-            request
-
-            expect(response).to redirect_to(absorbing_organisation_merge_request_path(merge_request))
-          end
-        end
-      end
-
       describe "from absorbing_organisation page" do
         context "when not answering the question" do
           let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) }
@@ -313,38 +278,46 @@ RSpec.describe MergeRequestsController, type: :request do
           end
         end
 
-        context "when absorbing_organisation_id set to other" do
+        context "when absorbing_organisation_id set to id" do
-          let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) }
+          let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) }
           let(:params) do
-            { merge_request: { absorbing_organisation_id: "other", page: "absorbing_organisation" } }
+            { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } }
           end
+
           let(:request) do
             patch "/merge-request/#{merge_request.id}", headers:, params:
           end
 
-          it "redirects to new org path" do
+          it "redirects to merging organisations path" do
             request
 
-            expect(response).to redirect_to(new_organisation_name_merge_request_path(merge_request))
+            expect(response).to redirect_to(organisations_merge_request_path(merge_request))
           end
 
-          it "resets absorbing_organisation and sets new_absorbing_organisation to true" do
+          it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do
             expect { request }.to change {
               merge_request.reload.absorbing_organisation
-            }.from(other_organisation).to(nil).and change {
+            }.from(nil).to(other_organisation).and change {
               merge_request.reload.new_absorbing_organisation
-            }.from(nil).to(true)
+            }.from(true).to(false)
           end
         end
+      end
 
-        context "when absorbing_organisation_id set to id" do
+      describe "#other_merging_organisations" do
-          let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) }
+        let(:other_merging_organisations) { "A list of other merging organisations" }
-          let(:params) do
+        let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } }
-            { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } }
+        let(:request) do
+          patch "/merge-request/#{merge_request.id}", headers:, params:
+        end
+
+        context "when adding other merging organisations" do
+          before do
+            MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id)
           end
 
-          let(:request) do
+          it "updates the merge request" do
-            patch "/merge-request/#{merge_request.id}", headers:, params:
+            expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations)
           end
 
           it "redirects telephone number path" do
@@ -352,14 +325,6 @@ RSpec.describe MergeRequestsController, type: :request do
 
             expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request))
           end
-
-          it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do
-            expect { request }.to change {
-              merge_request.reload.absorbing_organisation
-            }.from(nil).to(other_organisation).and change {
-              merge_request.reload.new_absorbing_organisation
-            }.from(true).to(false)
-          end
         end
       end
 
@@ -374,7 +339,7 @@ RSpec.describe MergeRequestsController, type: :request do
             patch "/merge-request/#{merge_request.id}", headers:, params:
           end
 
-          it "redirects telephone number path" do
+          it "redirects merge date path" do
             request
 
             expect(response).to redirect_to(merge_date_merge_request_path(merge_request))
@@ -399,7 +364,7 @@ RSpec.describe MergeRequestsController, type: :request do
             patch "/merge-request/#{merge_request.id}", headers:, params:
           end
 
-          it "redirects telephone number path" do
+          it "redirects merge date path" do
             request
 
             expect(response).to redirect_to(merge_date_merge_request_path(merge_request))
@@ -694,23 +659,99 @@ RSpec.describe MergeRequestsController, type: :request do
     end
   end
 
-  context "when user is signed in as a support user" do
+  context "when user is signed in with a data coordinator user" do
     before do
-      allow(support_user).to receive(:need_two_factor_authentication?).and_return(false)
+      sign_in user
-      sign_in support_user
     end
 
     describe "#organisations" do
       let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } }
 
-      before do
+      context "when creating a new merge request" do
-        post "/merge-request", headers:, params:
+        before do
+          post "/merge-request", headers:, params:
+        end
+
+        it "does not allow creating a new merge request" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+
+      context "when viewing existing merge request" do
+        before do
+          get "/merge-request/#{merge_request.id}/organisations", headers:, params:
+        end
+
+        it "does not allow viewing a merge request" do
+          expect(response).to have_http_status(:not_found)
+        end
       end
+    end
 
-      it "creates merge request with requesting organisation" do
+    describe "#update_organisations" do
-        follow_redirect!
+      let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
-        expect(MergeRequest.count).to eq(1)
+
-        expect(MergeRequest.first.requesting_organisation_id).to eq(other_organisation.id)
+      context "when updating a merge request with a new organisation" do
+        before do
+          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+        end
+
+        it "does not allow updaing a merge request" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+    end
+
+    describe "#remove_organisation" do
+      let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
+
+      context "when removing an organisation from merge request" do
+        before do
+          MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id)
+          get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params:
+        end
+
+        it "does not allow removing an organisation" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+    end
+
+    describe "#update" do
+      describe "from absorbing_organisation page" do
+        context "when absorbing_organisation_id set to id" do
+          let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) }
+          let(:params) do
+            { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } }
+          end
+
+          before do
+            patch "/merge-request/#{merge_request.id}", headers:, params:
+          end
+
+          it "does not allow updating absorbing organisation" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
+
+      describe "#other_merging_organisations" do
+        let(:other_merging_organisations) { "A list of other merging organisations" }
+        let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } }
+        let(:request) do
+          patch "/merge-request/#{merge_request.id}", headers:, params:
+        end
+
+        context "when adding other merging organisations" do
+          before do
+            MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id)
+            request
+          end
+
+          it "does not allow updating merging organisations" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
       end
     end
   end