From 2d2989cc6718352e74e1578dd40de2cbb05a4e19 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 30 Aug 2024 12:57:24 +0100 Subject: [PATCH] CLDC-3609 Add existing absorbing organisation merge request question (#2600) * Add existing absorbing organisation page and field * Update status calculation and add new question to check answers * Call correct merge service flow * Update test * Update test --- app/controllers/merge_requests_controller.rb | 8 +++ app/helpers/merge_requests_helper.rb | 5 +- app/jobs/process_merge_request_job.rb | 5 +- app/models/merge_request.rb | 7 +++ .../existing_absorbing_organisation.html.erb | 27 ++++++++++ config/locales/en.yml | 2 + config/routes.rb | 1 + ...d_existing_absorbing_organisation_field.rb | 5 ++ db/schema.rb | 3 +- spec/jobs/process_merge_request_job_spec.rb | 22 +++++++- spec/models/merge_request_spec.rb | 10 +++- .../merge_requests_controller_spec.rb | 50 +++++++++++++++++-- 12 files changed, 133 insertions(+), 12 deletions(-) create mode 100644 app/views/merge_requests/existing_absorbing_organisation.html.erb create mode 100644 db/migrate/20240822080228_add_existing_absorbing_organisation_field.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 7b5ed1ba9..017b1d1fc 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -6,6 +6,7 @@ class MergeRequestsController < ApplicationController def absorbing_organisation; end def merge_date; end + def existing_absorbing_organisation; end def helpdesk_ticket; end def merge_start_confirmation; end def user_outcomes; end @@ -94,6 +95,8 @@ private when "merging_organisations" merge_date_merge_request_path(@merge_request) when "merge_date" + existing_absorbing_organisation_merge_request_path(@merge_request) + when "existing_absorbing_organisation" helpdesk_ticket_merge_request_path(@merge_request) when "helpdesk_ticket" merge_request_path(@merge_request) @@ -123,6 +126,7 @@ private :status, :absorbing_organisation_id, :merge_date, + :existing_absorbing_organisation, ) merge_params[:requesting_organisation_id] = current_user.organisation.id @@ -148,6 +152,10 @@ private else @merge_request.errors.add(:merge_date, :invalid) end + when "existing_absorbing_organisation" + if merge_request_params[:existing_absorbing_organisation].nil? + @merge_request.errors.add(:existing_absorbing_organisation, :blank) + end end end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index a5d2ea14e..bda0b114f 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -19,6 +19,7 @@ module MergeRequestsHelper { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name), action: merge_request_action(merge_request, "absorbing_organisation") }, { label: "Merging organisations", value: merge_request.merge_request_organisations.any? ? merge_request.merge_request_organisations.map(&:merging_organisation_name).join("
").html_safe : display_value_or_placeholder(nil), action: merge_request_action(merge_request, "merging_organisations") }, { label: "Merge date", value: display_value_or_placeholder(merge_request.merge_date), action: merge_request_action(merge_request, "merge_date") }, + { label: "Absorbing organisation already active?", value: display_value_or_placeholder(merge_request.existing_absorbing_organisation_label), action: merge_request_action(merge_request, "existing_absorbing_organisation") }, ] end @@ -67,8 +68,10 @@ module MergeRequestsHelper absorbing_organisation_merge_request_path(merge_request) when "merge_date" merging_organisations_merge_request_path(merge_request) - when "helpdesk_ticket" + when "existing_absorbing_organisation" merge_date_merge_request_path(merge_request) + when "helpdesk_ticket" + existing_absorbing_organisation_merge_request_path(merge_request) end end diff --git a/app/jobs/process_merge_request_job.rb b/app/jobs/process_merge_request_job.rb index be506e1e4..253aab547 100644 --- a/app/jobs/process_merge_request_job.rb +++ b/app/jobs/process_merge_request_job.rb @@ -5,9 +5,10 @@ class ProcessMergeRequestJob < ApplicationJob absorbing_organisation_id = merge_request.absorbing_organisation_id merging_organisation_ids = merge_request.merging_organisations.pluck(:id) merge_date = merge_request.merge_date + absorbing_organisation_active_from_merge_date = !merge_request.existing_absorbing_organisation unless merge_request.existing_absorbing_organisation.nil? - Merge::MergeOrganisationsService.new(absorbing_organisation_id:, merging_organisation_ids:, merge_date:).call - merge_request.update!(request_merged: true) + Merge::MergeOrganisationsService.new(absorbing_organisation_id:, merging_organisation_ids:, merge_date:, absorbing_organisation_active_from_merge_date:).call + merge_request.update!(request_merged: true, last_failed_attempt: nil) rescue StandardError merge_request.update!(last_failed_attempt: Time.zone.now, processing: false, total_users: nil, total_schemes: nil) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 3560756e4..806d77bf8 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -47,6 +47,7 @@ class MergeRequest < ApplicationRecord def required_questions_answered? absorbing_organisation_id.present? && merge_date.present? && + existing_absorbing_organisation.present? && merging_organisations.count.positive? && errors.empty? end @@ -99,6 +100,12 @@ class MergeRequest < ApplicationRecord ([absorbing_organisation] + merging_organisations).reject(&:has_visible_schemes?) end + def existing_absorbing_organisation_label + return if existing_absorbing_organisation.nil? + + existing_absorbing_organisation ? "Yes" : "No" + end + def filter_relationships(absorbing_relationships, merging_relationships, absorbing_organisation, merging_organisations) filtered_absorbing_relationships = absorbing_relationships.reject do |relationship| merging_relationships.include?(relationship) || merging_organisations.include?(relationship) diff --git a/app/views/merge_requests/existing_absorbing_organisation.html.erb b/app/views/merge_requests/existing_absorbing_organisation.html.erb new file mode 100644 index 000000000..ce6628a35 --- /dev/null +++ b/app/views/merge_requests/existing_absorbing_organisation.html.erb @@ -0,0 +1,27 @@ +<% content_for :before_content do %> + <% title = "Tell us if your organisation is merging" %> + <% content_for :title, title %> + <%= govuk_back_link href: merge_request_back_link(@merge_request, "existing_absorbing_organisation", request.query_parameters["referrer"]) %> +<% end %> +
+
+ <%= form_with model: @merge_request, url: submit_merge_request_url(request.query_parameters["referrer"]), method: :patch do |f| %> + <%= f.govuk_error_summary %> + <%= f.govuk_radio_buttons_fieldset :existing_absorbing_organisation, + legend: { text: "Was #{@merge_request.absorbing_organisation&.name} already active before the merge date?", size: "l" } do %> + <%= f.govuk_radio_button :existing_absorbing_organisation, + "true", + label: { text: "Yes, this organisation existed before the merge" } %> + <%= f.govuk_radio_button :existing_absorbing_organisation, + "false", + label: { text: "No, it is a new organisation created by this merge" } %> + <% end %> + + <%= f.hidden_field :page, value: "existing_absorbing_organisation" %> +
+ <%= 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 %> +
+
diff --git a/config/locales/en.yml b/config/locales/en.yml index 649ec358a..7d52a92e0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -179,6 +179,8 @@ en: merge_date: blank: "Enter a merge date" invalid: "Enter a valid merge date" + existing_absorbing_organisation: + blank: "You must answer absorbing organisation already active?" notification: logs_deleted: diff --git a/config/routes.rb b/config/routes.rb index a6f2ae8c7..1706a140c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -208,6 +208,7 @@ Rails.application.routes.draw do get "merging-organisations/remove", to: "merge_requests#remove_merging_organisation" get "absorbing-organisation" get "merge-date" + get "existing-absorbing-organisation" get "helpdesk-ticket" get "merge-start-confirmation" get "user-outcomes" diff --git a/db/migrate/20240822080228_add_existing_absorbing_organisation_field.rb b/db/migrate/20240822080228_add_existing_absorbing_organisation_field.rb new file mode 100644 index 000000000..f1a0d8b1e --- /dev/null +++ b/db/migrate/20240822080228_add_existing_absorbing_organisation_field.rb @@ -0,0 +1,5 @@ +class AddExistingAbsorbingOrganisationField < ActiveRecord::Migration[7.0] + def change + add_column :merge_requests, :existing_absorbing_organisation, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 153770ccf..05f9bfde2 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: 2024_08_19_143150) do +ActiveRecord::Schema[7.0].define(version: 2024_08_22_080228) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -446,6 +446,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do t.datetime "last_failed_attempt" t.boolean "request_merged" t.boolean "processing" + t.boolean "existing_absorbing_organisation" end create_table "notifications", force: :cascade do |t| diff --git a/spec/jobs/process_merge_request_job_spec.rb b/spec/jobs/process_merge_request_job_spec.rb index c026db6f1..7a056ae9d 100644 --- a/spec/jobs/process_merge_request_job_spec.rb +++ b/spec/jobs/process_merge_request_job_spec.rb @@ -13,7 +13,7 @@ describe ProcessMergeRequestJob do let(:organisation) { create(:organisation) } let(:merging_organisation) { create(:organisation) } let(:other_merging_organisation) { create(:organisation) } - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3), total_users: 5, total_schemes: 5) } + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3), total_users: 5, total_schemes: 5, existing_absorbing_organisation: true) } before do create(:merge_request_organisation, merge_request:, merging_organisation:) @@ -21,12 +21,30 @@ describe ProcessMergeRequestJob do end it "calls the merge organisations service with correct arguments" do - expect(Merge::MergeOrganisationsService).to receive(:new).with(absorbing_organisation_id: organisation.id, merging_organisation_ids: [merging_organisation.id, other_merging_organisation.id], merge_date: Time.zone.local(2022, 3, 3)) + expect(Merge::MergeOrganisationsService).to receive(:new).with(absorbing_organisation_id: organisation.id, merging_organisation_ids: [merging_organisation.id, other_merging_organisation.id], merge_date: Time.zone.local(2022, 3, 3), absorbing_organisation_active_from_merge_date: false) job.perform(merge_request:) expect(merge_request.reload.status).to eq("request_merged") end + context "with new absorbing organisation" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3), existing_absorbing_organisation: false) } + + it "calls the merge organisations service with correct arguments" do + expect(Merge::MergeOrganisationsService).to receive(:new).with(absorbing_organisation_id: organisation.id, merging_organisation_ids: [merging_organisation.id, other_merging_organisation.id], merge_date: Time.zone.local(2022, 3, 3), absorbing_organisation_active_from_merge_date: true) + + job.perform(merge_request:) + expect(merge_request.reload.status).to eq("request_merged") + end + end + + it "clears last_failed_attempt value" do + merge_request.update!(last_failed_attempt: Time.zone.now) + job.perform(merge_request:) + + expect(merge_request.reload.last_failed_attempt).to be_nil + end + it "sets last_failed_attempt value, sets processing to false and clears total_schemes and total_users if there's an error" do allow(merge_organisations_service).to receive(:call).and_raise(ActiveRecord::Rollback) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 855fe03da..4cab8b661 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -55,13 +55,13 @@ RSpec.describe MergeRequest, type: :model do end it "returns the correct status for a ready to merge request" do - merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), merge_date: Time.zone.today) + merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), merge_date: Time.zone.today, existing_absorbing_organisation: true) create(:merge_request_organisation, merge_request:) expect(merge_request.status).to eq MergeRequest::STATUS[:ready_to_merge] end it "returns the merge issues if dsa is not signed for absorbing organisation" do - merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation, with_dsa: false), merge_date: Time.zone.today) + merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation, with_dsa: false), merge_date: Time.zone.today, existing_absorbing_organisation: true) create(:merge_request_organisation, merge_request:) expect(merge_request.status).to eq MergeRequest::STATUS[:merge_issues] end @@ -83,6 +83,12 @@ RSpec.describe MergeRequest, type: :model do expect(merge_request.status).to eq MergeRequest::STATUS[:incomplete] end + it "returns the incomplete if existing absorbing organisation is missing" do + merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation, with_dsa: false), merge_date: Time.zone.today) + create(:merge_request_organisation, merge_request:) + expect(merge_request.status).to eq MergeRequest::STATUS[:incomplete] + end + it "returns processing if merge is processing" do merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), processing: true) create(:merge_request_organisation, merge_request:) diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 530656858..7bcfc75ec 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -219,6 +219,23 @@ RSpec.describe MergeRequestsController, type: :request do end end + describe "#existing_absorbing_organisation" do + context "when viewing helpdesk ticket page" do + before do + merge_request.update!(absorbing_organisation_id: organisation.id, merge_date: Time.zone.today) + get "/merge-request/#{merge_request.id}/existing-absorbing-organisation", headers: + end + + it "shows the correct content" do + expect(page).to have_content("Was #{merge_request.absorbing_organisation.name} already active before the merge date?") + expect(page).to have_content("Yes, this organisation existed before the merge") + expect(page).to have_content("No, it is a new organisation created by this merge") + expect(page).to have_link("Back", href: merge_date_merge_request_path(merge_request)) + expect(page).to have_button("Save and continue") + end + end + end + describe "#helpdesk_ticket" do context "when viewing helpdesk ticket page" do before do @@ -228,6 +245,8 @@ RSpec.describe MergeRequestsController, type: :request do it "shows the correct content" do expect(page).to have_content("Which helpdesk ticket reported this merge?") + expect(page).to have_link("Back", href: existing_absorbing_organisation_merge_request_path(merge_request)) + expect(page).to have_button("Save and continue") end end end @@ -365,10 +384,10 @@ RSpec.describe MergeRequestsController, type: :request do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "redirects to helpdesk ticket path" do + it "redirects to existing absorbing organisation path" do request - expect(response).to redirect_to(helpdesk_ticket_merge_request_path(merge_request)) + expect(response).to redirect_to(existing_absorbing_organisation_merge_request_path(merge_request)) end it "updates merge_date" do @@ -400,6 +419,29 @@ RSpec.describe MergeRequestsController, type: :request do end end end + + describe "from existing_absorbing_organisation page" do + context "when not answering the question" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "existing_absorbing_organisation" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("You must answer absorbing organisation already active?") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end + end + end end describe "#merge_start_confirmation" do @@ -416,7 +458,7 @@ RSpec.describe MergeRequestsController, type: :request do end describe "#start_merge" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3)) } + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3), existing_absorbing_organisation: true) } let(:merging_organisation) { create(:organisation, name: "Merging Test Org") } before do @@ -481,7 +523,7 @@ RSpec.describe MergeRequestsController, type: :request do end context "with unmerged request" do - let(:merge_request) { create(:merge_request, absorbing_organisation_id: organisation.id, merge_date: Time.zone.today) } + let(:merge_request) { create(:merge_request, absorbing_organisation_id: organisation.id, merge_date: Time.zone.today, existing_absorbing_organisation: true) } it "shows users and schemes count and has links to view merge outcomes" do expect(page).to have_link("View", href: scheme_outcomes_merge_request_path(merge_request))