From 4f0c562c2ace785a7ccb9118b9891eb06cf17493 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Wed, 26 Apr 2023 16:14:56 +0100 Subject: [PATCH] CLDC-2056 - Ask absorbing organisation (#1580) * CLDC-2056 Add absorbing organisation question * Handle next page and previous_template with `page` * Hardcode backlinks * Refactor flow to store value when selecting new org option * Handle unanswered question * Update error copy --- app/controllers/merge_requests_controller.rb | 76 +++++++++-- app/models/merge_request.rb | 1 + .../absorbing_organisation.html.erb | 37 ++++++ .../confirm_telephone_number.html.erb | 13 ++ .../merge_requests/new_org_name.html.erb | 7 + .../merge_requests/organisations.html.erb | 62 ++++----- config/locales/en.yml | 1 + config/routes.rb | 2 + ...absorbing_organisation_to_merge_request.rb | 8 ++ db/schema.rb | 8 +- .../merge_requests_controller_spec.rb | 120 ++++++++++++++---- 11 files changed, 269 insertions(+), 66 deletions(-) create mode 100644 app/views/merge_requests/absorbing_organisation.html.erb create mode 100644 app/views/merge_requests/confirm_telephone_number.html.erb create mode 100644 app/views/merge_requests/new_org_name.html.erb create mode 100644 db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 3d1aa3f5f..047925722 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -1,7 +1,20 @@ class MergeRequestsController < ApplicationController - before_action :find_resource, only: %i[update organisations update_organisations remove_merging_organisation] + before_action :find_resource, only: %i[ + update + organisations + update_organisations + remove_merging_organisation + absorbing_organisation + confirm_telephone_number + new_org_name + ] before_action :authenticate_user! before_action :authenticate_scope!, except: [:create] + before_action :validate_response, only: %i[update] + + def absorbing_organisation; end + def confirm_telephone_number; end + def new_org_name; end def create ActiveRecord::Base.transaction do @@ -43,6 +56,31 @@ class MergeRequestsController < ApplicationController private + def page + params.dig(:merge_request, :page) + end + + def next_page_path + case page + when "absorbing_organisation" + if create_new_org? + new_org_name_merge_request_path(@merge_request) + else + confirm_telephone_number_merge_request_path(@merge_request) + end + when "organisations" + absorbing_organisation_merge_request_path(@merge_request) + end + end + + def previous_template + page + end + + def create_new_org? + params.dig(:merge_request, :absorbing_organisation_id) == "other" + end + def organisations_answer_options answer_options = { "" => "Select an option" } @@ -53,31 +91,47 @@ private end def merge_request_params - merge_params = params.fetch(:merge_request, {}).permit(:requesting_organisation_id, :other_merging_organisations, :status) + merge_params = params.fetch(:merge_request, {}).permit( + :requesting_organisation_id, + :other_merging_organisations, + :status, + :absorbing_organisation_id, + ) if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?) merge_params[:requesting_organisation_id] = current_user.organisation.id end + if merge_params[:absorbing_organisation_id].present? + if create_new_org? + merge_params[:new_absorbing_organisation] = true + merge_params[:absorbing_organisation_id] = nil + else + merge_params[:new_absorbing_organisation] = false + end + end + merge_params end + def validate_response + if page == "absorbing_organisation" && merge_request_params[:absorbing_organisation_id].blank? && merge_request_params[:new_absorbing_organisation].blank? + @merge_request.errors.add(:absorbing_organisation_id, I18n.t("validations.merge_request.absorbing_organisation_blank")) + render previous_template + end + end + def merge_request_organisation_params - { merge_request: @merge_request, merging_organisation_id: params[:merge_request][:merging_organisation] } + { + merge_request: @merge_request, + merging_organisation_id: params.dig(:merge_request, :merging_organisation), + } end def find_resource @merge_request = MergeRequest.find(params[:id]) end - def next_page_path - absorbing_organisation_merge_request_path(@merge_request) - end - - def previous_template - :organisations - end - def authenticate_scope! if current_user.organisation != @merge_request.requesting_organisation && !current_user.support? render_not_found diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 111d25321..94e2f6d00 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,6 +1,7 @@ class MergeRequest < ApplicationRecord belongs_to :requesting_organisation, class_name: "Organisation" has_many :merge_request_organisations + belongs_to :absorbing_organisation, class_name: "Organisation", optional: true has_many :merging_organisations, through: :merge_request_organisations, source: :merging_organisation STATUS = { diff --git a/app/views/merge_requests/absorbing_organisation.html.erb b/app/views/merge_requests/absorbing_organisation.html.erb new file mode 100644 index 000000000..cd2441d2b --- /dev/null +++ b/app/views/merge_requests/absorbing_organisation.html.erb @@ -0,0 +1,37 @@ +<% content_for :before_content do %> + <% title = "Tell us if your organisation is merging" %> + <% content_for :title, title %> + <%= govuk_back_link href: organisations_merge_request_path(id: @merge_request) %> +<% end %> + +

Which organisation is absorbing the others?

+ +
+
+

Select the organisation that the other organisations are merging into.

+ + <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> + + <%= f.govuk_radio_buttons_fieldset( + :absorbing_organisation_id, + hint: { text: "For example, if Skype and Yammer merged into Microsoft, you would select Microsoft." }, + legend: nil, + ) do %> + <% @merge_request.merging_organisations.order(:name).each do |org| %> + <%= f.govuk_radio_button( + :absorbing_organisation_id, + org.id, + label: { text: org.name }, + ) %> + <% end %> + <%= f.govuk_radio_divider %> + <%= f.govuk_radio_button :absorbing_organisation_id, "other", checked: @merge_request.new_absorbing_organisation?, label: { text: "These organisations are merging into a new one" } %> + <% end %> + + <%= f.hidden_field :page, value: "absorbing_organisation" %> + + <%= f.govuk_submit %> + <% end %> +
+
diff --git a/app/views/merge_requests/confirm_telephone_number.html.erb b/app/views/merge_requests/confirm_telephone_number.html.erb new file mode 100644 index 000000000..2a585bc1f --- /dev/null +++ b/app/views/merge_requests/confirm_telephone_number.html.erb @@ -0,0 +1,13 @@ +<% content_for :before_content do %> + <% title = "Tell us if your organisation is merging" %> + <% content_for :title, title %> + <%= govuk_back_link href: absorbing_organisation_merge_request_path(id: @merge_request) %> +<% end %> + +

What is <%= @merge_request.absorbing_organisation.name %>'s telephone number?

+ +
+
+

Confirm the telephone number on file, or enter a new one.

+
+
diff --git a/app/views/merge_requests/new_org_name.html.erb b/app/views/merge_requests/new_org_name.html.erb new file mode 100644 index 000000000..9c4075735 --- /dev/null +++ b/app/views/merge_requests/new_org_name.html.erb @@ -0,0 +1,7 @@ +<% content_for :before_content do %> + <% title = "Tell us if your organisation is merging" %> + <% content_for :title, title %> + <%= govuk_back_link href: absorbing_organisation_merge_request_path(id: @merge_request) %> +<% end %> + +Provide new org name diff --git a/app/views/merge_requests/organisations.html.erb b/app/views/merge_requests/organisations.html.erb index 14b70c17c..221fbe87e 100644 --- a/app/views/merge_requests/organisations.html.erb +++ b/app/views/merge_requests/organisations.html.erb @@ -1,5 +1,4 @@ <% content_for :before_content do %> - <% title = "Tell us if your organisation is merging" %> <% content_for :title, title %> <%= govuk_back_link href: merge_request_organisation_path(id: @merge_request.requesting_organisation_id) %> @@ -9,41 +8,44 @@
-

Add all organisations to be merged - we have already added your own.

+

+ Add all organisations to be merged - we have already added your own. +

-<%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> -

Start typing to search

- <%= render partial: "organisation_relationships/related_organisation_select_question", locals: { - field: :merging_organisation, - question: Form::Question.new("", { "answer_options" => @answer_options }, nil), - f:, - } %> - <%= f.govuk_submit "Add organisation", classes: "govuk-button--secondary" %> - <%= govuk_table do |table| %> - <% @merge_request.merging_organisations.each do |merging_organisation| %> - <%= table.body do |body| %> - <%= body.row do |row| %> - <% row.cell(text: merging_organisation.name) %> - <% row.cell(html_attributes: { - scope: "row", - class: "govuk-!-text-align-right", - }) do %> - <% if @merge_request.requesting_organisation != merging_organisation %> - <%= govuk_link_to("Remove", organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %> + <%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> +

Start typing to search

+ <%= render partial: "organisation_relationships/related_organisation_select_question", locals: { + field: :merging_organisation, + question: Form::Question.new("", { "answer_options" => @answer_options }, nil), + f:, + } %> + <%= f.govuk_submit "Add organisation", classes: "govuk-button--secondary" %> + <%= govuk_table do |table| %> + <% @merge_request.merging_organisations.order(:name).each do |merging_organisation| %> + <%= table.body do |body| %> + <%= body.row do |row| %> + <% row.cell(text: merging_organisation.name) %> + <% row.cell(html_attributes: { + scope: "row", + class: "govuk-!-text-align-right", + }) do %> + <% 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 %> <% end %> <% end %> <% end %> -<% end %> -<%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %> - <%= 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 > 1 %> - <%= f.govuk_submit "Continue" %> + <%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %> + <%= 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 > 1 %> + <%= f.hidden_field :page, value: "organisations" %> + <%= f.govuk_submit "Continue" %> + <% end %> <% end %> -<% end %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 7bf8839a9..ff409f2e0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -512,6 +512,7 @@ en: merge_request: organisation_part_of_another_merge: "This organisation is part of another merge - select a different one" organisation_not_selected: "Select an organisation from the search list" + absorbing_organisation_blank: Select the organisation absorbing the others soft_validations: net_income: diff --git a/config/routes.rb b/config/routes.rb index a9eaa86ef..cdbfc4616 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -137,6 +137,8 @@ Rails.application.routes.draw do patch "organisations", to: "merge_requests#update_organisations" get "organisations/remove", to: "merge_requests#remove_merging_organisation" get "absorbing-organisation" + get "confirm-telephone-number" + get "new-org-name" end end diff --git a/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb b/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb new file mode 100644 index 000000000..a038eebfa --- /dev/null +++ b/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb @@ -0,0 +1,8 @@ +class AddAbsorbingOrganisationToMergeRequest < ActiveRecord::Migration[7.0] + def change + change_table :merge_requests, bulk: true do |t| + t.column :absorbing_organisation_id, :integer + t.column :new_absorbing_organisation, :boolean + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 734968255..f891d1024 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_19_153741) do +ActiveRecord::Schema[7.0].define(version: 2023_04_21_124536) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -367,6 +367,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_19_153741) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "status" + t.integer "absorbing_organisation_id" + t.boolean "new_absorbing_organisation" end create_table "organisation_relationships", force: :cascade do |t| @@ -575,8 +577,6 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_19_153741) do t.integer "ethnicbuy2" t.integer "proplen_asked" t.string "old_id" - t.integer "buy2living" - t.integer "prevtenbuy2" t.integer "pregblank" t.string "uprn" t.integer "uprn_known" @@ -585,6 +585,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_19_153741) do t.string "address_line2" t.string "town_or_city" t.string "county" + t.integer "buy2living" + t.integer "prevtenbuy2" t.integer "nationalbuy2" t.integer "discounted_sale_value_check" t.integer "student_not_child_value_check" diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index ac7094df6..5e2479189 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -2,32 +2,29 @@ require "rails_helper" RSpec.describe MergeRequestsController, type: :request do let(:organisation) { user.organisation } - let(:other_organisation) { FactoryBot.create(:organisation, name: "Other Test Org") } + let(:other_organisation) { create(:organisation, name: "Other Test Org") } let(:headers) { { "Accept" => "text/html" } } let(:page) { Capybara::Node::Simple.new(response.body) } - let(:user) { FactoryBot.create(:user, :data_coordinator) } - let(:support_user) { FactoryBot.create(:user, :support, organisation:) } + let(:user) { create(:user, :data_coordinator) } + let(:support_user) { create(:user, :support, organisation:) } let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:other_merge_request) { MergeRequest.create!(requesting_organisation: other_organisation) } context "when user is signed in with a data coordinator user" do - before do - sign_in user - end + before { sign_in user } describe "#organisations" do let(:params) { { merge_request: { requesting_organisation_id: "9", status: "unsubmitted" } } } context "when creating a new merge request" do before do - organisation.update!(name: "Test Org") post "/merge-request", headers:, params: end it "creates merge request with requesting organisation" do follow_redirect! expect(page).to have_content("Which organisations are merging?") - expect(page).to have_content("Test Org") + expect(page).to have_content(organisation.name) expect(page).not_to have_link("Remove") end @@ -46,13 +43,12 @@ RSpec.describe MergeRequestsController, type: :request do context "when viewing existing merge request" do before do - organisation.update!(name: "Test Org") get "/merge-request/#{merge_request.id}/organisations", headers:, params: end it "shows merge request with requesting organisation" do expect(page).to have_content("Which organisations are merging?") - expect(page).to have_content("Test Org") + expect(page).to have_content(organisation.name) end end @@ -116,7 +112,7 @@ RSpec.describe MergeRequestsController, type: :request do 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(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -134,7 +130,7 @@ RSpec.describe MergeRequestsController, type: :request do 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(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -151,7 +147,7 @@ RSpec.describe MergeRequestsController, type: :request do 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(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -237,18 +233,99 @@ RSpec.describe MergeRequestsController, type: :request do end end - describe "#other_merging_organisations" do - let(:params) { { merge_request: { other_merging_organisations: "A list of other merging organisations" } } } + describe "#update" do + before { sign_in user } - context "when adding other merging organisations" do - before do - MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) + describe "#other_merging_organisations" do + let(:other_merging_organisations) { "A list of other merging organisations" } + let(:params) { { merge_request: { other_merging_organisations:, page: "organisations" } } } + let(:request) do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "updates the merge request" do - merge_request.reload - expect(merge_request.other_merging_organisations).to eq("A list of other merging organisations") + context "when adding other merging organisations" do + before do + MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) + end + + it "updates the merge request" do + expect { request }.to change { merge_request.reload.other_merging_organisations }.from(nil).to(other_merging_organisations) + end + + it "redirects telephone number path" do + request + + expect(response).to redirect_to(absorbing_organisation_merge_request_path(merge_request)) + end + end + end + + 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: "absorbing_organisation" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(page).to have_content("Select the organisation absorbing the others") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end + end + + context "when absorbing_organisation_id set to other" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { absorbing_organisation_id: "other", page: "absorbing_organisation" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "redirects to new org path" do + request + + expect(response).to redirect_to(new_org_name_merge_request_path(merge_request)) + end + + it "resets absorbing_organisation and sets new_absorbing_organisation to true" do + expect { request }.to change { + merge_request.reload.absorbing_organisation + }.from(other_organisation).to(nil).and change { + merge_request.reload.new_absorbing_organisation + }.from(nil).to(true) + end + end + + context "when absorbing_organisation_id set to id" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:params) do + { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } + end + + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "redirects telephone number path" do + request + + expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request)) + end + + it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do + expect { request }.to change { + merge_request.reload.absorbing_organisation + }.from(nil).to(other_organisation).and change { + merge_request.reload.new_absorbing_organisation + }.from(true).to(false) end end end @@ -264,7 +341,6 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "unsubmitted" } } } before do - organisation.update!(name: "Test Org") post "/merge-request", headers:, params: end