From b4158184a435f26901647024f58017c6dfe47368 Mon Sep 17 00:00:00 2001 From: Jack S Date: Wed, 26 Apr 2023 12:38:48 +0100 Subject: [PATCH] Refactor flow to store value when selecting new org option --- app/controllers/merge_requests_controller.rb | 14 +++++---- .../absorbing_organisation.html.erb | 3 +- ...absorbing_organisation_to_merge_request.rb | 5 +++- db/schema.rb | 1 + .../merge_requests_controller_spec.rb | 29 ++++++++++++++----- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 36a4f66c5..649a63b83 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -10,7 +10,6 @@ class MergeRequestsController < ApplicationController ] before_action :authenticate_user! before_action :authenticate_scope!, except: [:create] - before_action :redirect_to_next_page def absorbing_organisation; end def confirm_telephone_number; end @@ -77,10 +76,6 @@ private page end - def redirect_to_next_page - redirect_to next_page_path if create_new_org? - end - def create_new_org? params.dig(:merge_request, :absorbing_organisation_id) == "other" end @@ -106,6 +101,15 @@ private 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 diff --git a/app/views/merge_requests/absorbing_organisation.html.erb b/app/views/merge_requests/absorbing_organisation.html.erb index cffef43b8..cd2441d2b 100644 --- a/app/views/merge_requests/absorbing_organisation.html.erb +++ b/app/views/merge_requests/absorbing_organisation.html.erb @@ -23,11 +23,10 @@ :absorbing_organisation_id, org.id, label: { text: org.name }, - link_errors: true, ) %> <% end %> <%= f.govuk_radio_divider %> - <%= f.govuk_radio_button :absorbing_organisation_id, "other", label: { text: "These organisations are merging into a new one" } %> + <%= 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" %> diff --git a/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb b/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb index ae266b6fd..a038eebfa 100644 --- a/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb +++ b/db/migrate/20230421124536_add_absorbing_organisation_to_merge_request.rb @@ -1,5 +1,8 @@ class AddAbsorbingOrganisationToMergeRequest < ActiveRecord::Migration[7.0] def change - add_column :merge_requests, :absorbing_organisation_id, :integer + 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 66250f917..f891d1024 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -368,6 +368,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_21_124536) do 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| diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 2486c7925..5d5ed5638 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -112,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) { create(:organisation, name: "Other Test Org") } + let(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -130,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) { create(:organisation, name: "Other Test Org") } + let(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -147,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) { create(:organisation, name: "Other Test Org") } + let(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do @@ -261,20 +261,31 @@ RSpec.describe MergeRequestsController, type: :request do 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 - - before do + 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 @@ -289,8 +300,12 @@ RSpec.describe MergeRequestsController, type: :request do expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request)) end - it "updates the merge request" do - expect { request }.to change { merge_request.reload.absorbing_organisation_id }.from(nil).to(other_organisation.id) + 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