diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 11ed2210c..a6f5113cb 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -14,17 +14,14 @@ class MergeRequestsController < ApplicationController end def update_organisations - if @merge_request.merging_organisation_ids - @merge_request.merging_organisation_ids << params[:merge_request][:merging_organisation].to_i - else - @merge_request.merging_organisation_ids = [params[:merge_request][:merging_organisation]] - end + merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params) @answer_options = organisations_answer_options - @merging_organisations_list = [@merge_request.requesting_organisation] + @merge_request.merging_organisations - if @merge_request.save + if merge_request_organisation.save + @merge_request.reload @merging_organisations_list = [@merge_request.requesting_organisation] + @merge_request.merging_organisations - render "organisations" + render :organisations else + @merging_organisations_list = [@merge_request.requesting_organisation] + @merge_request.merging_organisations render :organisations, status: :unprocessable_entity end end @@ -47,6 +44,10 @@ private merge_params end + def merge_request_organisation_params + { merge_request: @merge_request, merging_organisation: Organisation.find(params[:merge_request][:merging_organisation]) } + end + def find_resource @merge_request = MergeRequest.find(params[:merge_request_id]) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5b26a6682..2d3234070 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -2,18 +2,4 @@ class MergeRequest < ApplicationRecord belongs_to :requesting_organisation, class_name: "Organisation" has_many :merge_request_organisations has_many :merging_organisations, through: :merge_request_organisations, source: :merging_organisation - - validate :validate_merging_organisations - -private - - def validate_merging_organisations - if MergeRequest.where(requesting_organisation_id: merging_organisation_ids).count.positive? - errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) - end - - if merging_organisation_ids&.any? { |org_id| MergeRequest.where("merging_organisation_ids @> ARRAY[?]", org_id).exists? } - errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) - end - end end diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index da457702e..4c9ccd186 100644 --- a/app/models/merge_request_organisation.rb +++ b/app/models/merge_request_organisation.rb @@ -3,6 +3,25 @@ class MergeRequestOrganisation < ApplicationRecord belongs_to :merging_organisation, class_name: "Organisation" validates :merge_request_id, presence: { message: I18n.t("validations.organisation.stock_owner.blank") } validates :merging_organisation_id, presence: { message: I18n.t("validations.organisation.managing_agent.blank") } + validate :validate_merging_organisations has_paper_trail + +private + + def validate_merging_organisations + if MergeRequestOrganisation.where(merge_request_id:, merging_organisation:).count.positive? + errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) + end + + if MergeRequestOrganisation.where.not(merge_request_id:).where(merging_organisation_id: merging_organisation.id).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 MergeRequest.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 + end end diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index b9f963278..00cadaa00 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -65,7 +65,8 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do - MergeRequest.create!(requesting_organisation_id: other_organisation.id, merging_organisation_ids: [another_organisation.id]) + existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id) + MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id) patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -76,6 +77,21 @@ RSpec.describe MergeRequestsController, type: :request do 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 is a part of current merge" do + let(:another_organisation) { FactoryBot.create(:organisation, name: "Other Test Org") } + let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } + + before do + merge_request.merging_organisations << another_organisation + patch "/merge-request/#{merge_request.id}/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 end end end