From b0500a297d6aa054a97f385237e9f42f3a381115 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:08:39 +0000 Subject: [PATCH] CLDC-3728 Add more merge request validations (#2814) * Add more_than_year_from_today merge date validation * Add a part of another merge validation * Disable merging if merge day is in the future --- app/controllers/merge_requests_controller.rb | 1 + app/helpers/merge_requests_helper.rb | 4 +++ app/models/merge_request_organisation.rb | 7 ++++ .../_notification_banners.html.erb | 8 +++++ app/views/merge_requests/show.html.erb | 2 +- config/locales/en.yml | 4 +++ .../merge_requests_controller_spec.rb | 34 +++++++++++++++++++ 7 files changed, 59 insertions(+), 1 deletion(-) diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index a21d42bbb..a6e2c08e5 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -143,6 +143,7 @@ private if [day, month, year].none?(&:blank?) && Date.valid_date?(year.to_i, month.to_i, day.to_i) merge_request_params["merge_date"] = Time.zone.local(year.to_i, month.to_i, day.to_i) + @merge_request.errors.add(:merge_date, :more_than_year_from_today) if Time.zone.local(year.to_i, month.to_i, day.to_i) - 1.year > Time.zone.today else @merge_request.errors.add(:merge_date, :invalid) end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 6283ef42e..28c693935 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -276,4 +276,8 @@ module MergeRequestsHelper def any_organisations_share_logs?(organisations, type) organisations.any? { |organisation| organisation.send("#{type}_logs").filter_by_managing_organisation(organisations.where.not(id: organisation.id)).exists? } end + + def begin_merge_disabled?(merge_request) + merge_request.status != "ready_to_merge" || merge_request.merge_date.future? + end end diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index 6dda8b35e..5bfbe14d7 100644 --- a/app/models/merge_request_organisation.rb +++ b/app/models/merge_request_organisation.rb @@ -29,5 +29,12 @@ private 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 + + existing_merges = MergeRequestOrganisation.with_merging_organisation(merging_organisation) + if existing_merges.count.positive? + existing_merge_request = existing_merges.first.merge_request + 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_incomplete_merge", organisation: merging_organisation.name, absorbing_organisation: existing_merge_request.absorbing_organisation&.name, merge_date: existing_merge_request.merge_date&.to_fs(:govuk_date))) + end end end diff --git a/app/views/merge_requests/_notification_banners.html.erb b/app/views/merge_requests/_notification_banners.html.erb index 38c05dbcd..9e6a085ca 100644 --- a/app/views/merge_requests/_notification_banners.html.erb +++ b/app/views/merge_requests/_notification_banners.html.erb @@ -19,3 +19,11 @@ No changes have been made. Try beginning the merge again. <% end %> <% end %> + +<% if @merge_request.merge_date&.future? %> + <%= govuk_notification_banner(title_text: "Important") do %> +

+ This merge is happening in the future. Wait until the merge date to begin this merge. +

+ <% end %> +<% end %> diff --git a/app/views/merge_requests/show.html.erb b/app/views/merge_requests/show.html.erb index 0fbde7621..040cd7704 100644 --- a/app/views/merge_requests/show.html.erb +++ b/app/views/merge_requests/show.html.erb @@ -12,7 +12,7 @@ <% unless @merge_request.status == "request_merged" || @merge_request.status == "processing" %>
- <%= govuk_button_link_to "Begin merge", merge_start_confirmation_merge_request_path(@merge_request), disabled: @merge_request.status != "ready_to_merge" %> + <%= govuk_button_link_to "Begin merge", merge_start_confirmation_merge_request_path(@merge_request), disabled: begin_merge_disabled?(@merge_request) %> <%= govuk_button_link_to "Delete merge request", delete_confirmation_merge_request_path(@merge_request), warning: true %>
<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index f8bb8255b..698618717 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -183,8 +183,11 @@ en: merge_date: blank: "Enter a merge date." invalid: "Enter a valid merge date." + more_than_year_from_today: "The merge date must not be later than a year from today’s date." existing_absorbing_organisation: blank: "You must answer absorbing organisation already active?" + merging_organisation_id: + part_of_another_merge: "Another merge request records %{organisation} as merging into %{absorbing_organisation} on %{merge_date}. Select another organisation or remove this organisation from the other merge request." notification: attributes: title: @@ -370,6 +373,7 @@ en: during_deactivated_period: "The location is already deactivated during this date, please enter a different date." merge_request: organisation_part_of_another_merge: "This organisation is part of another merge - select a different one." + organisation_part_of_another_incomplete_merge: "Another merge request records %{organisation} as merging into %{absorbing_organisation} on %{merge_date}. Select another organisation or remove this organisation from the other merge request." organisation_not_selected: "Select an organisation from the search list." soft_validations: diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index dc1dd817d..a73db8067 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -84,6 +84,22 @@ RSpec.describe MergeRequestsController, type: :request do end end + context "when the user updates merge request with organisation that is already part of another merge" do + let(:another_organisation) { create(:organisation) } + let(:other_merge_request) { create(:merge_request, merge_date: Time.zone.local(2022, 5, 4)) } + let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } } + + before do + MergeRequestOrganisation.create!(merge_request_id: other_merge_request.id, merging_organisation_id: another_organisation.id) + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: + end + + it "displays the page with an error message" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("Another merge request records #{another_organisation.name} as merging into #{other_merge_request.absorbing_organisation&.name} on 4 May 2022. Select another organisation or remove this organisation from the other merge request.") + 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, new_merging_org_ids: [] } } } @@ -396,6 +412,24 @@ RSpec.describe MergeRequestsController, type: :request do }.from(nil).to(Time.zone.local(2022, 4, 10)) end end + + context "when merge date set to a date more than 1 year in the future" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } + let(:params) do + { merge_request: { page: "merge_date", "merge_date(3i)": (Time.zone.now.day + 1).to_s, "merge_date(2i)": Time.zone.now.month.to_s, "merge_date(1i)": (Time.zone.now.year + 1).to_s } } + end + + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "displays the page with an error message" do + request + + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("The merge date must not be later than a year from today’s date.") + end + end end describe "from merging_organisations page" do