From 6d125c5dd841f1126d90732edcfd7620ed732b68 Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 18 Apr 2023 11:32:48 +0100 Subject: [PATCH] Add status, run validations unless the status is unsubmitted and save requesting organisation as a merging organisation as well --- app/controllers/merge_requests_controller.rb | 4 +- app/models/merge_request.rb | 7 +++ app/models/merge_request_organisation.rb | 7 ++- .../merge_requests/organisations.html.erb | 17 ++---- .../organisations/merge_request.html.erb | 1 + ...30418095819_add_status_to_merge_request.rb | 5 ++ db/schema.rb | 3 +- .../merge_requests_controller_spec.rb | 58 +++++++++++++++++-- 8 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20230418095819_add_status_to_merge_request.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index d24779b3d..12efd3fa3 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -5,6 +5,8 @@ class MergeRequestsController < ApplicationController def create @merge_request = MergeRequest.create!(merge_request_params) + MergeRequestOrganisation.create!({ merge_request: @merge_request, merging_organisation_id: merge_request_params.fetch(:requesting_organisation_id) }) + redirect_to organisations_merge_request_path(@merge_request) end @@ -48,7 +50,7 @@ private end def merge_request_params - merge_params = params.fetch(:merge_request, {}).permit(:requesting_organisation_id, :other_merging_organisations) + merge_params = params.fetch(:merge_request, {}).permit(:requesting_organisation_id, :other_merging_organisations, :status) if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?) merge_params[:requesting_organisation_id] = current_user.organisation.id diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2d3234070..79989d7e8 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -2,4 +2,11 @@ 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 + scope :unsubmitted, -> { where.not(status: "unsubmitted") } + + STATUS = { + "unsubmitted" => 0, + "submitted" => 1, + }.freeze + enum status: STATUS end diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index 18de56f0d..08c1d6d45 100644 --- a/app/models/merge_request_organisation.rb +++ b/app/models/merge_request_organisation.rb @@ -5,6 +5,9 @@ class MergeRequestOrganisation < ApplicationRecord validates :merging_organisation, presence: { message: I18n.t("validations.merge_request.merging_organisation_id.blank") } validate :validate_merging_organisations + scope :not_unsubmitted, -> { joins(:merge_request).where.not(merge_requests: { status: "unsubmitted" }) } + scope :with_merging_organisation, ->(merging_organisation) { where(merging_organisation:) } + has_paper_trail private @@ -14,12 +17,12 @@ private errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) end - if MergeRequestOrganisation.where.not(merge_request:).where(merging_organisation:).count.positive? + if MergeRequestOrganisation.not_unsubmitted.with_merging_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 - if MergeRequest.where(requesting_organisation: merging_organisation).count.positive? + if MergeRequest.not_unsubmitted.where.not(id: merge_request_id).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 diff --git a/app/views/merge_requests/organisations.html.erb b/app/views/merge_requests/organisations.html.erb index f2848c235..90d5004ec 100644 --- a/app/views/merge_requests/organisations.html.erb +++ b/app/views/merge_requests/organisations.html.erb @@ -21,15 +21,6 @@ } %> <%= f.govuk_submit "Add organisation", classes: "govuk-button--secondary" %> <%= govuk_table do |table| %> - <%= table.body do |body| %> - <%= body.row do |row| %> - <% row.cell(text: @merge_request.requesting_organisation.name) %> - <% row.cell(html_attributes: { - scope: "row", - class: "govuk-!-text-align-right", - }) %> - <% end %> - <% end %> <% @merge_request.merging_organisations.each do |merging_organisation| %> <%= table.body do |body| %> <%= body.row do |row| %> @@ -38,7 +29,9 @@ scope: "row", class: "govuk-!-text-align-right", }) do %> - <%= govuk_link_to("Remove", organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %> + <% if @merge_request.requesting_organisation != merging_organisation %> + <%= govuk_link_to("Remove", organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %> + <% end %> <% end %> <% end %> <% end %> @@ -46,9 +39,11 @@ <% end %> <% end %> <%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %> - <%= govuk_details(summary_text: "Can’t find the managing agent you’re looking for?") do %> + <%= govuk_details(summary_text: "I cannot find an organisation on the list") do %> <%= f.govuk_text_area :other_merging_organisations, label: { text: "Other organisations" }, hint: { text: "List other organisations that are part of the merge but not registered on CORE." }, rows: 9 %> <% end %> + <% if @merge_request.merging_organisations.count.positive? %> <%= f.govuk_submit "Continue" %> + <% end %> <% end %> diff --git a/app/views/organisations/merge_request.html.erb b/app/views/organisations/merge_request.html.erb index d8602b46c..3e064772c 100644 --- a/app/views/organisations/merge_request.html.erb +++ b/app/views/organisations/merge_request.html.erb @@ -43,6 +43,7 @@ <%= form_for @merge_request, url: merge_requests_path do |f| %> <%= f.hidden_field :requesting_organisation_id, value: @organisation.id %> + <%= f.hidden_field :status, value: "unsubmitted" %> <%= f.submit "Start now", class: "govuk-button govuk-button--start" %> <% end %> diff --git a/db/migrate/20230418095819_add_status_to_merge_request.rb b/db/migrate/20230418095819_add_status_to_merge_request.rb new file mode 100644 index 000000000..a6a3b01b9 --- /dev/null +++ b/db/migrate/20230418095819_add_status_to_merge_request.rb @@ -0,0 +1,5 @@ +class AddStatusToMergeRequest < ActiveRecord::Migration[7.0] + def change + add_column :merge_requests, :status, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 0d5c702db..e87364403 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_04_13_135407) do +ActiveRecord::Schema[7.0].define(version: 2023_04_18_095819) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -366,6 +366,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_13_135407) do t.text "other_merging_organisations" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "status" end create_table "organisation_relationships", force: :cascade do |t| diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index ca8b0ec5f..ac7094df6 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -16,7 +16,7 @@ RSpec.describe MergeRequestsController, type: :request do end describe "#organisations" do - let(:params) { { merge_request: { requesting_organisation_id: "9" } } } + let(:params) { { merge_request: { requesting_organisation_id: "9", status: "unsubmitted" } } } context "when creating a new merge request" do before do @@ -32,12 +32,14 @@ RSpec.describe MergeRequestsController, type: :request do end context "when passing a different requesting organisation id" do - let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id } } } + let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "unsubmitted" } } } it "creates merge request with current user organisation" do follow_redirect! expect(MergeRequest.count).to eq(1) expect(MergeRequest.first.requesting_organisation_id).to eq(organisation.id) + expect(MergeRequest.first.merging_organisations.count).to eq(1) + expect(MergeRequest.first.merging_organisations.first.id).to eq(organisation.id) end end end @@ -86,7 +88,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } before do - MergeRequest.create!(requesting_organisation_id: other_organisation.id) + MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "submitted") patch "/merge-request/#{merge_request.id}/organisations", headers:, params: end @@ -98,12 +100,27 @@ RSpec.describe MergeRequestsController, type: :request do end end + context "when the user selects an organisation that has another non submitted merge" do + let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } + + before do + MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "unsubmitted") + patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + end + + it "updates the merge request" 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) { FactoryBot.create(:organisation, name: "Other Test Org") } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do - existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id) + existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "submitted") 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 @@ -116,6 +133,23 @@ RSpec.describe MergeRequestsController, type: :request do end end + context "when the user selects an organisation that is a part of another unsubmitted merge" do + let(:another_organisation) { FactoryBot.create(:organisation, name: "Other Test Org") } + let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } + + before do + existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "unsubmitted") + 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 + + it "does not update the merge request" 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) { FactoryBot.create(:organisation, name: "Other Test Org") } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } @@ -131,6 +165,20 @@ RSpec.describe MergeRequestsController, type: :request do end end + context "when the user selects an organisation that is requesting this merge" do + let(:params) { { merge_request: { merging_organisation: merge_request.requesting_organisation_id } } } + + before do + patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + end + + it "does not update the merge request" do + merge_request.reload + expect(page).not_to have_content(I18n.t("validations.merge_request.organisation_part_of_another_merge")) + expect(merge_request.merging_organisations.count).to eq(1) + end + end + context "when the user does not select an organisation" do let(:params) { { merge_request: { merging_organisation: nil } } } @@ -213,7 +261,7 @@ RSpec.describe MergeRequestsController, type: :request do end describe "#organisations" do - let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id } } } + let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "unsubmitted" } } } before do organisation.update!(name: "Test Org")