diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 93b730e20..95be8d875 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -5,7 +5,6 @@ class MergeRequestsController < ApplicationController 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 diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 6546e39b2..8318b533a 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -28,8 +28,8 @@ module MergeRequestsHelper ] end - def ordered_merging_organisations(merge_request) - merge_request.merge_request_organisations.order(created_at: :desc).map(&:merging_organisation) + def ordered_merging_organisations(merge_request, new_merging_org_ids) + 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) @@ -53,4 +53,19 @@ module MergeRequestsHelper def accessed_from_check_answers?(referrer) %w[check_answers].include?(referrer) end + + def merge_request_back_link(merge_request, page, referrer) + return merge_request_path(merge_request) if accessed_from_check_answers?(referrer) + + case page + when "absorbing_organisation" + organisations_path(anchor: "merge-requests") + when "merging_organisations" + absorbing_organisation_merge_request_path(merge_request) + when "merge_date" + merging_organisations_merge_request_path(merge_request) + when "helpdesk_ticket" + merge_date_merge_request_path(merge_request) + end + end end diff --git a/app/views/merge_requests/absorbing_organisation.html.erb b/app/views/merge_requests/absorbing_organisation.html.erb index 33c246bb7..40faa88f5 100644 --- a/app/views/merge_requests/absorbing_organisation.html.erb +++ b/app/views/merge_requests/absorbing_organisation.html.erb @@ -1,7 +1,7 @@ <% content_for :before_content do %> <% title = "Tell us if your organisation is merging" %> <% content_for :title, title %> - <%= govuk_back_link href: organisations_path(anchor: "merge-requests") %> + <%= govuk_back_link href: merge_request_back_link(@merge_request, "absorbing_organisation", request.query_parameters["referrer"]) %> <% end %> <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> diff --git a/app/views/merge_requests/helpdesk_ticket.html.erb b/app/views/merge_requests/helpdesk_ticket.html.erb index accaf4aca..18e06121b 100644 --- a/app/views/merge_requests/helpdesk_ticket.html.erb +++ b/app/views/merge_requests/helpdesk_ticket.html.erb @@ -1,7 +1,7 @@ <% content_for :before_content do %> <% title = "Which helpdesk ticket reported this merge?" %> <% content_for :title, title %> - <%= govuk_back_link href: merge_date_merge_request_path(@merge_request) %> + <%= govuk_back_link href: merge_request_back_link(@merge_request, "helpdesk_ticket", request.query_parameters["referrer"]) %> <% end %> <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> diff --git a/app/views/merge_requests/merge_date.html.erb b/app/views/merge_requests/merge_date.html.erb index 90cb22159..84b93e44f 100644 --- a/app/views/merge_requests/merge_date.html.erb +++ b/app/views/merge_requests/merge_date.html.erb @@ -1,7 +1,7 @@ <% content_for :before_content do %> <% title = "Tell us if your organisation is merging" %> <% content_for :title, title %> - <%= govuk_back_link href: merging_organisations_merge_request_path(@merge_request) %> + <%= govuk_back_link href: merge_request_back_link(@merge_request, "merge_date", request.query_parameters["referrer"]) %> <% end %>
diff --git a/app/views/merge_requests/merging_organisations.html.erb b/app/views/merge_requests/merging_organisations.html.erb index fb4133889..2b58cd49e 100644 --- a/app/views/merge_requests/merging_organisations.html.erb +++ b/app/views/merge_requests/merging_organisations.html.erb @@ -1,7 +1,7 @@ <% 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) %> + <%= govuk_back_link href: merge_request_back_link(@merge_request, "merging_organisations", request.query_parameters["referrer"]) %> <% end %> <%= form_with model: @merge_request, url: merging_organisations_merge_request_path(referrer: request.query_parameters["referrer"]), method: :patch do |f| %> @@ -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 %> +
+ <% 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 %> + <% end %> + <%= govuk_link_to secondary_merge_request_link_text(request.query_parameters["referrer"]), merge_request_path(@merge_request) %> +
<% end %>
diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 2144b409b..865b75ba1 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/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