CLDC-3585 Update absorbing organisation question (#2564)

* Update user permissions and absorbing org question order

* Update absorbing organisation question and routing

app/controllers/merge_requests_controller.rb

@@ -1,21 +1,11 @@
 class MergeRequestsController < ApplicationController
-  before_action :find_resource, only: %i[
+  before_action :find_resource, exclude: %i[create new]
-    update
-    organisations
-    update_organisations
-    remove_merging_organisation
-    absorbing_organisation
-    confirm_telephone_number
-    new_organisation_name
-    new_organisation_address
-    new_organisation_telephone_number
-    new_organisation_type
-    merge_date
-  ]
   before_action :authenticate_user!
-  before_action :authenticate_scope!, except: [:create]
+  before_action :authenticate_scope!
+  before_action :set_organisations_answer_options, only: %i[organisations absorbing_organisation update_organisations remove_merging_organisation update]
 
   def absorbing_organisation; end
+  def organisations; end
   def confirm_telephone_number; end
   def new_organisation_name; end
   def new_organisation_address; end
@@ -26,17 +16,12 @@ 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
 
-  def organisations
-    @answer_options = organisations_answer_options
-  end
-
   def update
     validate_response
 
@@ -49,7 +34,6 @@ class MergeRequestsController < ApplicationController
 
   def update_organisations
     merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params)
-    @answer_options = organisations_answer_options
     if merge_request_organisation.save
       render :organisations
     else
@@ -59,7 +43,6 @@ class MergeRequestsController < ApplicationController
 
   def remove_merging_organisation
     MergeRequestOrganisation.find_by(merge_request_organisation_params)&.destroy!
-    @answer_options = organisations_answer_options
     render :organisations
   end
 
@@ -72,13 +55,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"
@@ -98,13 +81,16 @@ private
     params.dig(:merge_request, :absorbing_organisation_id) == "other"
   end
 
-  def organisations_answer_options
+  def set_organisations_answer_options
     answer_options = { "" => "Select an option" }
 
-    Organisation.all.pluck(:id, :name).each do |organisation|
+    if current_user.support?
-      answer_options[organisation[0]] = organisation[1]
+      Organisation.all.pluck(:id, :name).each do |organisation|
+        answer_options[organisation[0]] = organisation[1]
+      end
     end
-    answer_options
+
+    @answer_options = answer_options
   end
 
   def merge_request_params
@@ -122,9 +108,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?
@@ -168,11 +152,13 @@ private
   end
 
   def find_resource
+    return if params[:id].blank?
+
     @merge_request = MergeRequest.find(params[:id])
   end
 
   def authenticate_scope!
-    if current_user.organisation != @merge_request.requesting_organisation && !current_user.support?
+    unless current_user.support?
       render_not_found
     end
   end

app/views/merge_requests/absorbing_organisation.html.erb

@@ -1,32 +1,28 @@
 <% content_for :before_content do %>
   <% title = "Tell us if your organisation is merging" %>
   <% content_for :title, title %>
-  <%= govuk_back_link href: organisations_merge_request_path(id: @merge_request) %>
+  <%= govuk_back_link href: organisations_path(anchor: "merge-requests") %>
 <% end %>
 
 <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %>
   <%= f.govuk_error_summary %>
 
-  <h2 class="govuk-heading-l">Which organisation is absorbing the others?</h2>
+  <h1 class="govuk-heading-l">Which organisation is absorbing the others?</h1>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds-from-desktop">
-      <p class="govuk-body">Select the organisation that the other organisations are merging into.</p>
+      <p class="govuk-hint">If organisations are merging into a new organisation, <%= govuk_link_to "create the new organisation", new_organisation_path %> first and then select it here.</p>
 
-      <%= f.govuk_radio_buttons_fieldset(
+      <%= f.govuk_select(:absorbing_organisation_id,
-        :absorbing_organisation_id,
+        label: { text: "Select organisation name", class: "govuk-label--m" },
-        hint: { text: "For example, if Skype and Yammer merged into Microsoft, you would select Microsoft." },
+        "data-controller": "accessible-autocomplete") do %>
-        legend: nil,
+          <% @answer_options.map { |id, name| OpenStruct.new(id:, name:) }.each do |answer| %>
-      ) do %>
+            <option value="<%= answer.id %>"
-        <% @merge_request.merging_organisations.order(:name).each do |org| %>
+              data-synonyms="<%= answer_option_synonyms(answer.resource) %>"
-          <%= f.govuk_radio_button(
+              data-append="<%= answer_option_append(answer.resource) %>"
-            :absorbing_organisation_id,
+              data-hint="<%= answer_option_hint(answer.resource) %>"
-            org.id,
+              <%= @merge_request.absorbing_organisation_id == answer.id ? "selected" : "" %>><%= answer.name || answer.resource %></option>
-            label: { text: org.name },
+          <% end %>
-          ) %>
         <% end %>
-        <%= f.govuk_radio_divider %>
-        <%= f.govuk_radio_button :absorbing_organisation_id, "other", checked: @merge_request.new_absorbing_organisation?, label: { text: "These organisations are merging into a new one" } %>
-      <% end %>
 
       <%= f.hidden_field :page, value: "absorbing_organisation" %>
 

app/views/organisations/index.html.erb

@@ -15,7 +15,7 @@
       <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "organisations" } %>
     <% end %>
     <% c.with_tab(label: "Merge requests") do %>
-      <%= govuk_button_link_to "Create new merge request", "/organisations/#{current_user.organisation_id}/merge-request", html: { method: :get } %>
+      <%= govuk_button_to "Create new merge request", merge_requests_path, html: { method: :post } %>
       <%= render partial: "merge_request_list", locals: { merge_requests: @merge_requests, title: "Merge requests", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %>
     <% 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
@@ -265,33 +256,22 @@ RSpec.describe MergeRequestsController, type: :request do
       end
     end
 
-    describe "#update" do
+    describe "#absorbing_organisation" do
-      before { sign_in user }
+      let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) }
 
-      describe "#other_merging_organisations" do
+      before { get "/merge-request/#{merge_request.id}/absorbing-organisation", headers: }
-        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
+      it "asks for the absorbing organisation" do
-            expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations)
+        expect(page).to have_content("Which organisation is absorbing the others?")
-          end
+        expect(page).to have_content("Select organisation name")
-
+      end
-          it "redirects telephone number path" do
-            request
 
-            expect(response).to redirect_to(absorbing_organisation_merge_request_path(merge_request))
+      it "has the correct back button" do
-          end
+        expect(page).to have_link("Back", href: organisations_path(anchor: "merge-requests"))
-        end
       end
+    end
 
+    describe "#update" do
       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 +293,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 +340,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 +354,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 +379,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 +674,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

spec/requests/organisations_controller_spec.rb

@@ -1022,7 +1022,7 @@ RSpec.describe OrganisationsController, type: :request do
         end
 
         it "has a create new merge request button" do
-          expect(page).to have_link("Create new merge request")
+          expect(page).to have_button("Create new merge request")
         end
 
         it "displays 'No merge requests' when @merge_requests is empty" do