Add cancel button to merging orgs and update merging orgs selection

app/controllers/merge_requests_controller.rb

@@ -1,11 +1,10 @@
 class MergeRequestsController < ApplicationController
   before_action :find_resource, exclude: %i[create new]
-  before_action :authenticate_user!
+  # before_action :authenticate_user!
   before_action :authenticate_scope!
   before_action :set_organisations_answer_options, only: %i[merging_organisations absorbing_organisation update_merging_organisations remove_merging_organisation update]
 
   def absorbing_organisation; end
-  def merging_organisations; end
   def merge_date; end
   def helpdesk_ticket; end
 
@@ -22,6 +21,12 @@ class MergeRequestsController < ApplicationController
     validate_response
 
     if @merge_request.errors.blank? && @merge_request.update(merge_request_params)
+      if page == "merging_organisations"
+        new_merging_org_ids = params["merge_request"]["new_merging_org_ids"].split(" ")
+        new_merging_org_ids.each do |org_id|
+          MergeRequestOrganisation.create!(merge_request: @merge_request, merging_organisation_id: org_id)
+        end
+      end
       redirect_to next_page_path
     else
       render previous_template, status: :unprocessable_entity
@@ -29,8 +34,10 @@ class MergeRequestsController < ApplicationController
   end
 
   def update_merging_organisations
+    @new_merging_org_ids = params["merge_request"]["new_merging_org_ids"].split(" ")
     merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params)
-    if merge_request_organisation.save
+    if merge_request_organisation.valid?
+      @new_merging_org_ids.push(merge_request_organisation_params[:merging_organisation_id])
       render :merging_organisations
     else
       render :merging_organisations, status: :unprocessable_entity
@@ -38,6 +45,9 @@ class MergeRequestsController < ApplicationController
   end
 
   def remove_merging_organisation
+    @new_merging_org_ids = params["merge_request"]["new_merging_org_ids"] || []
+    org_id_to_remove = merge_request_organisation_params[:merging_organisation_id]
+    @new_merging_org_ids.delete(org_id_to_remove)
     MergeRequestOrganisation.find_by(merge_request_organisation_params)&.destroy!
     render :merging_organisations
   end
@@ -47,6 +57,10 @@ class MergeRequestsController < ApplicationController
     redirect_to organisations_path(anchor: "merge-requests")
   end
 
+  def merging_organisations
+    @new_merging_org_ids = []
+  end
+
 private
 
   def page

app/helpers/merge_requests_helper.rb

@@ -28,8 +28,8 @@ module MergeRequestsHelper
     ]
   end
 
-  def ordered_merging_organisations(merge_request)
+  def ordered_merging_organisations(merge_request, new_merging_org_ids)
-    merge_request.merge_request_organisations.order(created_at: :desc).map(&:merging_organisation)
+    Organisation.where(id: new_merging_org_ids) + merge_request.merge_request_organisations.order(created_at: :desc).map(&:merging_organisation)
   end
 
   def submit_merge_request_button_text(referrer)

app/views/merge_requests/merging_organisations.html.erb

@@ -18,9 +18,10 @@
         question: Form::Question.new("", { "answer_options" => @answer_options.reject { |id, _org_name| id != "" && id == @merge_request.absorbing_organisation_id } }, nil),
         f:,
       } %>
+      <%= f.hidden_field :new_merging_org_ids, value: @new_merging_org_ids %>
       <%= f.govuk_submit "Add organisation", secondary: true, classes: "govuk-button--secondary" %>
       <%= govuk_table do |table| %>
-        <% ordered_merging_organisations(@merge_request).each do |merging_organisation| %>
+        <% ordered_merging_organisations(@merge_request, @new_merging_org_ids).each do |merging_organisation| %>
           <%= table.with_body do |body| %>
             <%= body.with_row do |row| %>
               <% row.with_cell(text: merging_organisation.name) %>
@@ -28,7 +29,7 @@
                 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 })) %>
+                <%= govuk_link_to("Remove", merging_organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id, new_merging_org_ids: @new_merging_org_ids })) %>
               <% end %>
             <% end %>
           <% end %>
@@ -36,13 +37,14 @@
       <% 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.hidden_field :page, value: "merging_organisations" %>
+      <%= f.hidden_field :new_merging_org_ids, value: @new_merging_org_ids %>
-        <div class="govuk-button-group">
+      <div class="govuk-button-group">
+        <% if @merge_request.merging_organisations.count.positive? || @new_merging_org_ids.count.positive? %>
           <%= f.govuk_submit submit_merge_request_button_text(request.query_parameters["referrer"]) %>
-          <%= govuk_link_to(secondary_merge_request_link_text(request.query_parameters["referrer"]), merge_request_path(@merge_request)) %>
+        <% end %>
-        </div>
+        <%= govuk_link_to secondary_merge_request_link_text(request.query_parameters["referrer"]), merge_request_path(@merge_request) %>
-      <% end %>
+      </div>
     <% end %>
   </div>
 </div>

spec/requests/merge_requests_controller_spec.rb

@@ -55,16 +55,15 @@ RSpec.describe MergeRequestsController, type: :request do
     end
 
     describe "#update_merging_organisations" do
-      let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
+      let(:params) { { merge_request: { merging_organisation: other_organisation.id, new_merging_org_ids: [] } } }
 
       context "when updating a merge request with a new merging organisation" do
         before do
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
-        it "updates the merge request" do
+        it "adds merging organisation to the page" do
           merge_request.reload
-          expect(merge_request.merging_organisations.count).to eq(1)
           expect(page).to have_content("MHCLG")
           expect(page).to have_content("Other Test Org")
           expect(page).to have_link("Remove")
@@ -72,23 +71,22 @@ RSpec.describe MergeRequestsController, type: :request do
       end
 
       context "when the user selects an organisation that requested another merge" do
-        let(:params) { { merge_request: { merging_organisation: other_organisation.id } } }
+        let(:params) { { merge_request: { merging_organisation: other_organisation.id, new_merging_org_ids: [] } } }
 
         before do
           MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
-        it "updates the merge request" do
+        it "does not error" 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"))
         end
       end
 
       context "when the user selects an organisation that is a part of another merge" do
         let(:another_organisation) { create(:organisation) }
-        let(:params) { { merge_request: { merging_organisation: another_organisation.id } } }
+        let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } }
 
         before do
           existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
@@ -106,7 +104,7 @@ RSpec.describe MergeRequestsController, type: :request 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 } } }
+        let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } }
 
         before do
           existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete")
@@ -114,16 +112,15 @@ RSpec.describe MergeRequestsController, type: :request do
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
-        it "updates the merge request" do
+        it "does not error" 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"))
         end
       end
 
       context "when the user selects an organisation that is a part of current merge" do
         let(:another_organisation) { create(:organisation) }
-        let(:params) { { merge_request: { merging_organisation: another_organisation.id } } }
+        let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } }
 
         before do
           merge_request.merging_organisations << another_organisation
@@ -137,7 +134,7 @@ RSpec.describe MergeRequestsController, type: :request do
       end
 
       context "when the user does not select an organisation" do
-        let(:params) { { merge_request: { merging_organisation: nil } } }
+        let(:params) { { merge_request: { merging_organisation: nil, new_merging_org_ids: [] } } }
 
         before do
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
@@ -152,7 +149,7 @@ RSpec.describe MergeRequestsController, type: :request do
       end
 
       context "when the user selects non existent id" do
-        let(:params) { { merge_request: { merging_organisation: "clearly_not_an_id" } } }
+        let(:params) { { merge_request: { merging_organisation: "clearly_not_an_id", new_merging_org_ids: [] } } }
 
         before do
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
@@ -344,6 +341,28 @@ RSpec.describe MergeRequestsController, type: :request do
           end
         end
       end
+
+      describe "from merging_organisations page" do
+        context "when the user updates merge request with valid merging organisation ID" do
+          let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) }
+          let(:another_organisation) { create(:organisation) }
+          let(:params) do
+            { merge_request: { page: "merging_organisations", new_merging_org_ids: another_organisation.id } }
+          end
+
+          let(:request) do
+            patch "/merge-request/#{merge_request.id}", headers:, params:
+          end
+
+          it "updates the merge request" do
+            request
+
+            merge_request.reload
+            expect(merge_request.merging_organisations.count).to eq(1)
+            expect(merge_request.merging_organisations.first.id).to eq(another_organisation.id)
+          end
+        end
+      end
     end
   end