Browse Source

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
pull/2803/head^2
kosiakkatrina 1 month ago committed by GitHub
parent
commit
b0500a297d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 1
      app/controllers/merge_requests_controller.rb
  2. 4
      app/helpers/merge_requests_helper.rb
  3. 7
      app/models/merge_request_organisation.rb
  4. 8
      app/views/merge_requests/_notification_banners.html.erb
  5. 2
      app/views/merge_requests/show.html.erb
  6. 4
      config/locales/en.yml
  7. 34
      spec/requests/merge_requests_controller_spec.rb

1
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) 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_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 else
@merge_request.errors.add(:merge_date, :invalid) @merge_request.errors.add(:merge_date, :invalid)
end end

4
app/helpers/merge_requests_helper.rb

@ -276,4 +276,8 @@ module MergeRequestsHelper
def any_organisations_share_logs?(organisations, type) 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? } organisations.any? { |organisation| organisation.send("#{type}_logs").filter_by_managing_organisation(organisations.where.not(id: organisation.id)).exists? }
end end
def begin_merge_disabled?(merge_request)
merge_request.status != "ready_to_merge" || merge_request.merge_date.future?
end
end end

7
app/models/merge_request_organisation.rb

@ -29,5 +29,12 @@ private
if merging_organisation_id.blank? || !Organisation.where(id: merging_organisation_id).exists? 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")) merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_not_selected"))
end 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
end end

8
app/views/merge_requests/_notification_banners.html.erb

@ -19,3 +19,11 @@
No changes have been made. Try beginning the merge again. No changes have been made. Try beginning the merge again.
<% end %> <% end %>
<% end %> <% end %>
<% if @merge_request.merge_date&.future? %>
<%= govuk_notification_banner(title_text: "Important") do %>
<p class="govuk-notification-banner__heading govuk-!-width-full" style="max-width: fit-content">
This merge is happening in the future. Wait until the merge date to begin this merge.
</p>
<% end %>
<% end %>

2
app/views/merge_requests/show.html.erb

@ -12,7 +12,7 @@
</h1> </h1>
<% unless @merge_request.status == "request_merged" || @merge_request.status == "processing" %> <% unless @merge_request.status == "request_merged" || @merge_request.status == "processing" %>
<div class="govuk-button-group"> <div class="govuk-button-group">
<%= 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 %> <%= govuk_button_link_to "Delete merge request", delete_confirmation_merge_request_path(@merge_request), warning: true %>
</div> </div>
<% end %> <% end %>

4
config/locales/en.yml

@ -183,8 +183,11 @@ en:
merge_date: merge_date:
blank: "Enter a merge date." blank: "Enter a merge date."
invalid: "Enter a valid 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: existing_absorbing_organisation:
blank: "You must answer absorbing organisation already active?" 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: notification:
attributes: attributes:
title: title:
@ -370,6 +373,7 @@ en:
during_deactivated_period: "The location is already deactivated during this date, please enter a different date." during_deactivated_period: "The location is already deactivated during this date, please enter a different date."
merge_request: merge_request:
organisation_part_of_another_merge: "This organisation is part of another merge - select a different one." 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." organisation_not_selected: "Select an organisation from the search list."
soft_validations: soft_validations:

34
spec/requests/merge_requests_controller_spec.rb

@ -84,6 +84,22 @@ RSpec.describe MergeRequestsController, type: :request do
end end
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 context "when the user selects an organisation that is a part of another merge" do
let(:another_organisation) { create(:organisation) } let(:another_organisation) { create(:organisation) }
let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } } 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)) }.from(nil).to(Time.zone.local(2022, 4, 10))
end end
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 end
describe "from merging_organisations page" do describe "from merging_organisations page" do

Loading…
Cancel
Save