Update merging organisations question

app/controllers/merge_requests_controller.rb

@@ -2,10 +2,10 @@ class MergeRequestsController < ApplicationController
   before_action :find_resource, exclude: %i[create new]
   before_action :authenticate_user!
   before_action :authenticate_scope!
-  before_action :set_organisations_answer_options, only: %i[organisations absorbing_organisation update_organisations remove_merging_organisation update]
+  before_action :set_organisations_answer_options, only: %i[merging_organisations absorbing_organisation update_merging_organisations remove_merging_organisation update]
 
   def absorbing_organisation; end
-  def organisations; end
+  def merging_organisations; end
   def confirm_telephone_number; end
   def new_organisation_name; end
   def new_organisation_address; end
@@ -32,18 +32,18 @@ class MergeRequestsController < ApplicationController
     end
   end
 
-  def update_organisations
+  def update_merging_organisations
     merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params)
     if merge_request_organisation.save
-      render :organisations
+      render :merging_organisations
     else
-      render :organisations, status: :unprocessable_entity
+      render :merging_organisations, status: :unprocessable_entity
     end
   end
 
   def remove_merging_organisation
     MergeRequestOrganisation.find_by(merge_request_organisation_params)&.destroy!
-    render :organisations
+    render :merging_organisations
   end
 
 private
@@ -55,8 +55,8 @@ private
   def next_page_path
     case page
     when "absorbing_organisation"
-      organisations_merge_request_path(@merge_request)
+      merging_organisations_merge_request_path(@merge_request)
-    when "organisations"
+    when "merging_organisations"
       if create_new_organisation?
         new_organisation_name_merge_request_path(@merge_request)
       else
@@ -96,7 +96,6 @@ private
   def merge_request_params
     merge_params = params.fetch(:merge_request, {}).permit(
       :requesting_organisation_id,
-      :other_merging_organisations,
       :status,
       :absorbing_organisation_id,
       :telephone_number_correct,

app/helpers/merge_request_helper.rb

@@ -0,0 +1,5 @@
+module MergeRequestHelper
+  def ordered_merging_organisations(merge_request)
+    merge_request.merge_request_organisations.order(created_at: :desc).map(&:merging_organisation)
+  end
+end

app/models/merge_request_organisation.rb

@@ -26,11 +26,6 @@ private
       merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge"))
     end
 
-    if MergeRequest.merged.where.not(id: merge_request_id).where(requesting_organisation: merging_organisation).count.positive?
-      errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge"))
-      merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge"))
-    end
-
     if merging_organisation_id.blank? || !Organisation.where(id: merging_organisation_id).exists?
       merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_not_selected"))
     end

app/views/merge_requests/merging_organisations.html.erb

@@ -0,0 +1,45 @@
+<% content_for :before_content do %>
+  <% title = "Tell us if your organisation is merging" %>
+  <% content_for :title, title %>
+  <%= govuk_back_link href: merge_request_organisation_path(id: @merge_request.requesting_organisation_id) %>
+<% end %>
+
+<%= form_with model: @merge_request, url: merging_organisations_merge_request_path, method: :patch do |f| %>
+  <%= f.govuk_error_summary %>
+  <h2 class="govuk-heading-l">Which organisations are merging into <%= @merge_request.absorbing_organisation&.name %>?</h2>
+
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds-from-desktop">
+      <p class="govuk-hint">Add all organisations that are merging.</p>
+
+      <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
+        label: { text: "Select an organisation", class: "govuk-label--m" },
+        field: :merging_organisation,
+        question: Form::Question.new("", { "answer_options" => @answer_options.reject { |id, _org_name| id != "" && id == @merge_request.absorbing_organisation_id } }, nil),
+        f:,
+      } %>
+      <%= f.govuk_submit "Add organisation", secondary: true, classes: "govuk-button--secondary" %>
+      <%= govuk_table do |table| %>
+        <% ordered_merging_organisations(@merge_request).each do |merging_organisation| %>
+          <%= table.with_body do |body| %>
+            <%= body.with_row do |row| %>
+              <% row.with_cell(text: merging_organisation.name) %>
+              <% row.with_cell(html_attributes: {
+                scope: "row",
+                class: "govuk-!-text-align-right",
+              }) do %>
+                <%= govuk_link_to("Remove", merging_organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %>
+              <% end %>
+            <% end %>
+          <% end %>
+        <% end %>
+      <% end %>
+    <% end %>
+    <%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %>
+      <% if @merge_request.merging_organisations.count.positive? %>
+        <%= f.hidden_field :page, value: "merging_organisations" %>
+        <%= f.govuk_submit "Continue" %>
+      <% end %>
+    <% end %>
+  </div>
+</div>

app/views/merge_requests/organisations.html.erb

@@ -1,52 +0,0 @@
-<% content_for :before_content do %>
-  <% title = "Tell us if your organisation is merging" %>
-  <% content_for :title, title %>
-  <%= govuk_back_link href: merge_request_organisation_path(id: @merge_request.requesting_organisation_id) %>
-<% end %>
-
-<%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %>
-  <%= f.govuk_error_summary %>
-  <h2 class="govuk-heading-l">Which organisations are merging?</h2>
-
-  <div class="govuk-grid-row">
-    <div class="govuk-grid-column-two-thirds-from-desktop">
-    <p class="govuk-body">
-      Add all organisations to be merged - we have already added your own.
-    </p>
-
-    <p class="govuk-body">Start typing to search</p>
-    <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
-      field: :merging_organisation,
-      question: Form::Question.new("", { "answer_options" => @answer_options }, nil),
-      f:,
-    } %>
-    <%= f.govuk_submit "Add organisation", classes: "govuk-button--secondary" %>
-    <%= govuk_table do |table| %>
-      <% @merge_request.merging_organisations.order(:name).each do |merging_organisation| %>
-        <%= table.with_body do |body| %>
-          <%= body.with_row do |row| %>
-            <% row.with_cell(text: merging_organisation.name) %>
-            <% row.with_cell(html_attributes: {
-              scope: "row",
-              class: "govuk-!-text-align-right",
-            }) do %>
-              <% if @merge_request.requesting_organisation != merging_organisation %>
-                <%= govuk_link_to("Remove", organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %>
-              <% end %>
-            <% end %>
-          <% end %>
-        <% end %>
-      <% end %>
-    <% end %>
-  <% end %>
-  <%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %>
-    <%= govuk_details(summary_text: "I cannot find an organisation on the list") do %>
-      <%= f.govuk_text_area :other_merging_organisations, label: { text: "Other organisations" }, hint: { text: "List other organisations that are part of the merge but not registered on CORE." }, rows: 9 %>
-    <% end %>
-    <% if @merge_request.merging_organisations.count > 1 %>
-      <%= f.hidden_field :page, value: "organisations" %>
-      <%= f.govuk_submit "Continue" %>
-    <% end %>
-  <% end %>
-  </div>
-</div>

app/views/organisation_relationships/_related_organisation_select_question.html.erb

@@ -1,3 +1,3 @@
 <% answers = question.answer_options.map { |key, value| OpenStruct.new(id: key, name: value) } %>
-<%= f.govuk_collection_select field, answers, :id, :name, label: { hidden: true }, "data-controller": "accessible-autocomplete" do %>
+<%= f.govuk_collection_select field, answers, :id, :name, label:, "data-controller": "accessible-autocomplete" do %>
 <% end %>

app/views/organisation_relationships/add_managing_agent.html.erb

@@ -19,6 +19,7 @@
   <% end %>
   <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
     field: :child_organisation_id,
+    label: { hidden: true },
     question: Form::Question.new("", { "answer_options" => answer_options }, nil),
     f:,
   } %>

app/views/organisation_relationships/add_stock_owner.html.erb

@@ -19,6 +19,7 @@
   <% end %>
   <%= render partial: "organisation_relationships/related_organisation_select_question", locals: {
     field: :parent_organisation_id,
+    label: { hidden: true },
     question: Form::Question.new("", { "answer_options" => answer_options }, nil),
     f:,
   } %>

config/routes.rb

@@ -195,9 +195,9 @@ Rails.application.routes.draw do
 
   resources :merge_requests, path: "/merge-request" do
     member do
-      get "organisations"
+      get "merging-organisations"
-      patch "organisations", to: "merge_requests#update_organisations"
+      patch "merging-organisations", to: "merge_requests#update_merging_organisations"
-      get "organisations/remove", to: "merge_requests#remove_merging_organisation"
+      get "merging-organisations/remove", to: "merge_requests#remove_merging_organisation"
       get "absorbing-organisation"
       get "confirm-telephone-number"
       get "new-organisation-name"

db/migrate/20240813072041_remove_other_merging_org_field.rb

@@ -0,0 +1,5 @@
+class RemoveOtherMergingOrgField < ActiveRecord::Migration[7.0]
+  def change
+    remove_column :merge_requests, :other_merging_organisations, :string
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_08_09_154241) do
+ActiveRecord::Schema[7.0].define(version: 2024_08_13_072041) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -417,7 +417,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_09_154241) do
 
   create_table "merge_requests", force: :cascade do |t|
     t.integer "requesting_organisation_id"
-    t.text "other_merging_organisations"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "status"

spec/requests/merge_requests_controller_spec.rb

@@ -16,10 +16,9 @@ RSpec.describe MergeRequestsController, type: :request do
       sign_in support_user
     end
 
-    describe "#organisations" do
+    context "when creating a new merge request" do
       let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id, status: "incomplete" } } }
 
-      context "when creating a new merge request" do
       before do
         post "/merge-request", headers:, params:
       end
@@ -27,7 +26,7 @@ RSpec.describe MergeRequestsController, type: :request do
       it "creates merge request with requesting organisation" do
         follow_redirect!
         expect(page).to have_content("Which organisation is absorbing the others?")
-          expect(page).to have_content(support_user.organisation.name)
+        expect(MergeRequest.first.requesting_organisation_id).to eq(support_user.organisation_id)
       end
 
       context "when passing a different requesting organisation id" do
@@ -42,30 +41,31 @@ RSpec.describe MergeRequestsController, type: :request do
       end
     end
 
-      context "when viewing existing merge request" do
+    describe "#merging-organisations" do
+      context "when viewing merging organisations page" do
         before do
-          get "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          merge_request.update!(absorbing_organisation_id: organisation.id)
+          get "/merge-request/#{merge_request.id}/merging-organisations", headers:
         end
 
-        it "shows merge request with requesting organisation" do
+        it "shows the correct content" do
-          expect(page).to have_content("Which organisations are merging?")
+          expect(page).to have_content("Which organisations are merging into MHCLG?")
-          expect(page).to have_content(organisation.name)
         end
       end
     end
 
-    describe "#update_organisations" do
+    describe "#update_merging_organisations" do
       let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
 
-      context "when updating a merge request with a new organisation" do
+      context "when updating a merge request with a new merging organisation" do
         before do
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "updates the merge request" do
           merge_request.reload
           expect(merge_request.merging_organisations.count).to eq(1)
-          expect(page).to have_content("Test Org")
+          expect(page).to have_content("MHCLG")
           expect(page).to have_content("Other Test Org")
           expect(page).to have_link("Remove")
         end
@@ -76,23 +76,7 @@ RSpec.describe MergeRequestsController, type: :request do
 
         before do
           MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
-        end
-
-        it "does not update the merge request" do
-          merge_request.reload
-          expect(merge_request.merging_organisations.count).to eq(0)
-          expect(response).to have_http_status(:unprocessable_entity)
-          expect(page).to have_content(I18n.t("validations.merge_request.organisation_part_of_another_merge"))
-        end
-      end
-
-      context "when the user selects an organisation that has another non submitted merge" do
-        let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
-
-        before do
-          MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete")
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
         end
 
         it "updates the merge request" do
@@ -109,7 +93,7 @@ RSpec.describe MergeRequestsController, type: :request do
         before do
           existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
           MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id)
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "does not update the merge request" do
@@ -120,17 +104,17 @@ RSpec.describe MergeRequestsController, type: :request do
         end
       end
 
-      context "when the user selects an organisation that is a part of another unsubmitted merge" do
+      context "when the user selects an organisation that is a part of another incomplete merge" do
         let(:another_organisation) { create(:organisation) }
         let(:params) { { merge_request: { merging_organisation: another_organisation.id } } }
 
         before do
           existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete")
           MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id)
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
-        it "does not update the merge request" do
+        it "updates the merge request" do
           merge_request.reload
           expect(merge_request.merging_organisations.count).to eq(1)
           expect(page).not_to have_content(I18n.t("validations.merge_request.organisation_part_of_another_merge"))
@@ -143,25 +127,11 @@ RSpec.describe MergeRequestsController, type: :request do
 
         before do
           merge_request.merging_organisations << another_organisation
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
-        end
-
-        it "does not update the merge request" do
-          merge_request.reload
-          expect(merge_request.merging_organisations.count).to eq(1)
-        end
-      end
-
-      context "when the user selects an organisation that is requesting this merge" do
-        let(:params) { { merge_request: { merging_organisation: merge_request.requesting_organisation_id } } }
-
-        before do
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
         end
 
         it "does not update the merge request" do
           merge_request.reload
-          expect(page).not_to have_content(I18n.t("validations.merge_request.organisation_part_of_another_merge"))
           expect(merge_request.merging_organisations.count).to eq(1)
         end
       end
@@ -170,7 +140,7 @@ RSpec.describe MergeRequestsController, type: :request do
         let(:params) { { merge_request: { merging_organisation: nil } } }
 
         before do
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "does not update the merge request" do
@@ -185,7 +155,7 @@ RSpec.describe MergeRequestsController, type: :request do
         let(:params) { { merge_request: { merging_organisation: "clearly_not_an_id" } } }
 
         before do
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "does not update the merge request" do
@@ -203,7 +173,7 @@ RSpec.describe MergeRequestsController, type: :request do
       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:
+          get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params:
         end
 
         it "updates the merge request" do
@@ -214,7 +184,7 @@ RSpec.describe MergeRequestsController, type: :request do
 
       context "when removing an organisation that is not part of a merge from merge request" do
         before do
-          get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params:
+          get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params:
         end
 
         it "does not throw an error" do
@@ -306,7 +276,7 @@ RSpec.describe MergeRequestsController, type: :request do
           it "redirects to merging organisations path" do
             request
 
-            expect(response).to redirect_to(organisations_merge_request_path(merge_request))
+            expect(response).to redirect_to(merging_organisations_merge_request_path(merge_request))
           end
 
           it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do
@@ -319,30 +289,6 @@ RSpec.describe MergeRequestsController, type: :request do
         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)
-          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(confirm_telephone_number_merge_request_path(merge_request))
-          end
-        end
-      end
-
       describe "from confirm_telephone_number page" do
         context "when confirming the number" do
           let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true, new_telephone_number: "123") }
@@ -694,7 +640,7 @@ RSpec.describe MergeRequestsController, type: :request do
 
       context "when viewing existing merge request" do
         before do
-          get "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          get "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "does not allow viewing a merge request" do
@@ -708,7 +654,7 @@ RSpec.describe MergeRequestsController, type: :request do
 
       context "when updating a merge request with a new organisation" do
         before do
-          patch "/merge-request/#{merge_request.id}/organisations", headers:, params:
+          patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
         it "does not allow updaing a merge request" do
@@ -723,7 +669,7 @@ RSpec.describe MergeRequestsController, type: :request do
       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:
+          get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params:
         end
 
         it "does not allow removing an organisation" do