From bede43312c691761320a0123a883e268fef08302 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:39:33 +0100 Subject: [PATCH 1/2] CLDC-3584 Update merging orgs question (#2567) * Update merging organisations question * Update rebase tests * Update routing between CYA and questions --- app/controllers/merge_requests_controller.rb | 34 +++-- app/helpers/merge_requests_helper.rb | 22 ++- app/models/merge_request_organisation.rb | 5 - .../merging_organisations.html.erb | 45 ++++++ .../merge_requests/organisations.html.erb | 52 ------- ...ated_organisation_select_question.html.erb | 2 +- .../add_managing_agent.html.erb | 1 + .../add_stock_owner.html.erb | 1 + config/routes.rb | 6 +- ...13072041_remove_other_merging_org_field.rb | 5 + db/schema.rb | 3 +- .../merge_requests_controller_spec.rb | 136 ++++++------------ .../merge_requests/show.html.erb_spec.rb | 9 -- 13 files changed, 142 insertions(+), 179 deletions(-) create mode 100644 app/views/merge_requests/merging_organisations.html.erb delete mode 100644 app/views/merge_requests/organisations.html.erb create mode 100644 db/migrate/20240813072041_remove_other_merging_org_field.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 6d7738204..46798d261 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -2,10 +2,10 @@ class MergeRequestsController < ApplicationController before_action :find_resource, exclude: %i[create new] before_action :authenticate_user! before_action :authenticate_scope! - before_action :set_organisations_answer_options, only: %i[organisations absorbing_organisation update_organisations remove_merging_organisation update] + before_action :set_organisations_answer_options, only: %i[merging_organisations absorbing_organisation update_merging_organisations remove_merging_organisation update] def absorbing_organisation; end - def organisations; end + def merging_organisations; end def confirm_telephone_number; end def new_organisation_name; end def new_organisation_address; end @@ -32,18 +32,18 @@ class MergeRequestsController < ApplicationController end end - def update_organisations + def update_merging_organisations merge_request_organisation = MergeRequestOrganisation.new(merge_request_organisation_params) if merge_request_organisation.save - render :organisations + render :merging_organisations else - render :organisations, status: :unprocessable_entity + render :merging_organisations, status: :unprocessable_entity end end def remove_merging_organisation MergeRequestOrganisation.find_by(merge_request_organisation_params)&.destroy! - render :organisations + render :merging_organisations end private @@ -53,10 +53,12 @@ private end def next_page_path + return merge_request_path if is_referrer_type?("check_answers") + case page when "absorbing_organisation" - organisations_merge_request_path(@merge_request) - when "organisations" + merging_organisations_merge_request_path(@merge_request) + when "merging_organisations" if create_new_organisation? new_organisation_name_merge_request_path(@merge_request) else @@ -96,7 +98,6 @@ private def merge_request_params merge_params = params.fetch(:merge_request, {}).permit( :requesting_organisation_id, - :other_merging_organisations, :status, :absorbing_organisation_id, :telephone_number_correct, @@ -162,4 +163,19 @@ private render_not_found end end + + def is_referrer_type?(referrer_type) + from_referrer_query("referrer") == referrer_type + end + + def from_referrer_query(query_param) + referrer = request.headers["HTTP_REFERER"] + return unless referrer + + query_params = URI.parse(referrer).query + return unless query_params + + parsed_params = CGI.parse(query_params) + parsed_params[query_param]&.first + end end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 9f6afe1df..4975a238f 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -13,9 +13,9 @@ module MergeRequestsHelper def merge_details(merge_request) [ - { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: absorbing_organisation_merge_request_path(merge_request), visually_hidden_text: "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.status == "request_merged" ? nil : { text: "Change", href: organisations_merge_request_path(merge_request), visually_hidden_text: "merging organisations" } }, - { label: "Merge date", value: display_value_or_placeholder(merge_request.merge_date), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: merge_date_merge_request_path(merge_request), visually_hidden_text: "merge date" } }, + { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: absorbing_organisation_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "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.status == "request_merged" ? nil : { text: "Change", href: merging_organisations_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "merging organisations" } }, + { label: "Merge date", value: display_value_or_placeholder(merge_request.merge_date), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: merge_date_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "merge date" } }, ] end @@ -27,4 +27,20 @@ module MergeRequestsHelper { label: "Total stock owners & managing agents after merge", value: merge_request.total_stock_owners.present? || merge_request.total_managing_agents.present? ? "#{merge_request.total_stock_owners} stock owners
#{merge_request.total_managing_agents} managing agents".html_safe : display_value_or_placeholder(nil), action: { text: "View", href: "#", visually_hidden_text: "total stock owners & managing agents after merge" } }, ] end + + def ordered_merging_organisations(merge_request) + merge_request.merge_request_organisations.order(created_at: :desc).map(&:merging_organisation) + end + + def submit_merge_request_button_text(referrer) + if accessed_from_check_answers?(referrer) + "Save changes" + else + "Save and continue" + end + end + + def accessed_from_check_answers?(referrer) + %w[check_answers].include?(referrer) + end end diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index fd57b5e51..e181ca304 100644 --- a/app/models/merge_request_organisation.rb +++ b/app/models/merge_request_organisation.rb @@ -26,11 +26,6 @@ private merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_part_of_another_merge")) end - if MergeRequest.merged.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 - if merging_organisation_id.blank? || !Organisation.where(id: merging_organisation_id).exists? merge_request.errors.add(:merging_organisation, I18n.t("validations.merge_request.organisation_not_selected")) end diff --git a/app/views/merge_requests/merging_organisations.html.erb b/app/views/merge_requests/merging_organisations.html.erb new file mode 100644 index 000000000..f7dbeb378 --- /dev/null +++ b/app/views/merge_requests/merging_organisations.html.erb @@ -0,0 +1,45 @@ +<% 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) %> +<% end %> + +<%= form_with model: @merge_request, url: merging_organisations_merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> +

Which organisations are merging into <%= @merge_request.absorbing_organisation&.name %>?

+ +
+
+

Add all organisations that are merging.

+ + <%= render partial: "organisation_relationships/related_organisation_select_question", locals: { + label: { text: "Select an organisation", class: "govuk-label--m" }, + field: :merging_organisation, + question: Form::Question.new("", { "answer_options" => @answer_options.reject { |id, _org_name| id != "" && id == @merge_request.absorbing_organisation_id } }, nil), + f:, + } %> + <%= f.govuk_submit "Add organisation", secondary: true, classes: "govuk-button--secondary" %> + <%= govuk_table do |table| %> + <% ordered_merging_organisations(@merge_request).each do |merging_organisation| %> + <%= table.with_body do |body| %> + <%= body.with_row do |row| %> + <% row.with_cell(text: merging_organisation.name) %> + <% row.with_cell(html_attributes: { + scope: "row", + class: "govuk-!-text-align-right", + }) do %> + <%= govuk_link_to("Remove", merging_organisations_remove_merge_request_path(merge_request: { merging_organisation: merging_organisation.id })) %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <%= form_with model: @merge_request, url: merge_request_path(id: @merge_request.id), method: :patch do |f| %> + <% if @merge_request.merging_organisations.count.positive? %> + <%= f.hidden_field :page, value: "merging_organisations" %> + <%= f.govuk_submit submit_merge_request_button_text(request.query_parameters["referrer"]) %> + <% end %> + <% end %> +
+
diff --git a/app/views/merge_requests/organisations.html.erb b/app/views/merge_requests/organisations.html.erb deleted file mode 100644 index 6b6a0783c..000000000 --- a/app/views/merge_requests/organisations.html.erb +++ /dev/null @@ -1,52 +0,0 @@ -<% 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) %> -<% end %> - -<%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> -

Which organisations are merging?

- -
-
-

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

- -

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.with_body do |body| %> - <%= body.with_row do |row| %> - <% row.with_cell(text: merging_organisation.name) %> - <% row.with_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 %> - <%= 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 %> -
-
diff --git a/app/views/organisation_relationships/_related_organisation_select_question.html.erb b/app/views/organisation_relationships/_related_organisation_select_question.html.erb index 551bce5ed..f0d1aa2eb 100644 --- a/app/views/organisation_relationships/_related_organisation_select_question.html.erb +++ b/app/views/organisation_relationships/_related_organisation_select_question.html.erb @@ -1,3 +1,3 @@ <% answers = question.answer_options.map { |key, value| OpenStruct.new(id: key, name: value) } %> -<%= f.govuk_collection_select field, answers, :id, :name, label: { hidden: true }, "data-controller": "accessible-autocomplete" do %> +<%= f.govuk_collection_select field, answers, :id, :name, label:, "data-controller": "accessible-autocomplete" do %> <% end %> diff --git a/app/views/organisation_relationships/add_managing_agent.html.erb b/app/views/organisation_relationships/add_managing_agent.html.erb index b3e3a4a42..25c7c53a2 100644 --- a/app/views/organisation_relationships/add_managing_agent.html.erb +++ b/app/views/organisation_relationships/add_managing_agent.html.erb @@ -19,6 +19,7 @@ <% end %> <%= render partial: "organisation_relationships/related_organisation_select_question", locals: { field: :child_organisation_id, + label: { hidden: true }, question: Form::Question.new("", { "answer_options" => answer_options }, nil), f:, } %> diff --git a/app/views/organisation_relationships/add_stock_owner.html.erb b/app/views/organisation_relationships/add_stock_owner.html.erb index b74d812ec..042442125 100644 --- a/app/views/organisation_relationships/add_stock_owner.html.erb +++ b/app/views/organisation_relationships/add_stock_owner.html.erb @@ -19,6 +19,7 @@ <% end %> <%= render partial: "organisation_relationships/related_organisation_select_question", locals: { field: :parent_organisation_id, + label: { hidden: true }, question: Form::Question.new("", { "answer_options" => answer_options }, nil), f:, } %> diff --git a/config/routes.rb b/config/routes.rb index fbb59a7d0..b304ecb10 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -195,9 +195,9 @@ Rails.application.routes.draw do resources :merge_requests, path: "/merge-request" do member do - get "organisations" - patch "organisations", to: "merge_requests#update_organisations" - get "organisations/remove", to: "merge_requests#remove_merging_organisation" + get "merging-organisations" + patch "merging-organisations", to: "merge_requests#update_merging_organisations" + get "merging-organisations/remove", to: "merge_requests#remove_merging_organisation" get "absorbing-organisation" get "confirm-telephone-number" get "new-organisation-name" diff --git a/db/migrate/20240813072041_remove_other_merging_org_field.rb b/db/migrate/20240813072041_remove_other_merging_org_field.rb new file mode 100644 index 000000000..eb1ddc2e4 --- /dev/null +++ b/db/migrate/20240813072041_remove_other_merging_org_field.rb @@ -0,0 +1,5 @@ +class RemoveOtherMergingOrgField < ActiveRecord::Migration[7.0] + def change + remove_column :merge_requests, :other_merging_organisations, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 8296332f5..ca0241179 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_09_154241) do +ActiveRecord::Schema[7.0].define(version: 2024_08_13_072041) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -417,7 +417,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_09_154241) do create_table "merge_requests", force: :cascade do |t| t.integer "requesting_organisation_id" - t.text "other_merging_organisations" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "status" diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 2ae5dce42..805f91f1c 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -16,56 +16,56 @@ RSpec.describe MergeRequestsController, type: :request do sign_in support_user end - describe "#organisations" do + context "when creating a new merge request" do let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id, status: "incomplete" } } } - context "when creating a new merge request" do - before do - post "/merge-request", headers:, params: - end + before do + post "/merge-request", headers:, params: + end - it "creates merge request with requesting organisation" do - follow_redirect! - expect(page).to have_content("Which organisation is absorbing the others?") - expect(page).to have_content(support_user.organisation.name) - end + it "creates merge request with requesting organisation" do + follow_redirect! + expect(page).to have_content("Which organisation is absorbing the others?") + expect(MergeRequest.first.requesting_organisation_id).to eq(support_user.organisation_id) + end - context "when passing a different requesting organisation id" do - let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } + context "when passing a different requesting organisation id" do + let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } - 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(support_user.organisation_id) - expect(MergeRequest.first.merging_organisations.count).to eq(0) - end + 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(support_user.organisation_id) + expect(MergeRequest.first.merging_organisations.count).to eq(0) end end + end - context "when viewing existing merge request" do + describe "#merging-organisations" do + context "when viewing merging organisations page" do before do - get "/merge-request/#{merge_request.id}/organisations", headers:, params: + merge_request.update!(absorbing_organisation_id: organisation.id) + get "/merge-request/#{merge_request.id}/merging-organisations", headers: end - it "shows merge request with requesting organisation" do - expect(page).to have_content("Which organisations are merging?") - expect(page).to have_content(organisation.name) + it "shows the correct content" do + expect(page).to have_content("Which organisations are merging into MHCLG?") end end end - describe "#update_organisations" do + describe "#update_merging_organisations" do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } - context "when updating a merge request with a new organisation" do + context "when updating a merge request with a new merging organisation" do before do - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "updates the merge request" do merge_request.reload expect(merge_request.merging_organisations.count).to eq(1) - expect(page).to have_content("Test Org") + expect(page).to have_content("MHCLG") expect(page).to have_content("Other Test Org") expect(page).to have_link("Remove") end @@ -76,23 +76,7 @@ RSpec.describe MergeRequestsController, type: :request do before do MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged") - 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(0) - expect(response).to have_http_status(:unprocessable_entity) - 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 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: "incomplete") - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "updates the merge request" do @@ -109,7 +93,7 @@ RSpec.describe MergeRequestsController, type: :request do before do existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged") MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id) - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not update the merge request" do @@ -120,17 +104,17 @@ RSpec.describe MergeRequestsController, type: :request do end end - context "when the user selects an organisation that is a part of another unsubmitted merge" do + context "when the user selects an organisation that is a part of another incomplete merge" do let(:another_organisation) { create(:organisation) } let(:params) { { merge_request: { merging_organisation: another_organisation.id } } } before do existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete") MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id) - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end - it "does not update the merge request" do + 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")) @@ -143,7 +127,7 @@ RSpec.describe MergeRequestsController, type: :request do before do merge_request.merging_organisations << another_organisation - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not update the merge request" do @@ -152,25 +136,11 @@ 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 } } } before do - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not update the merge request" do @@ -185,7 +155,7 @@ RSpec.describe MergeRequestsController, type: :request do let(:params) { { merge_request: { merging_organisation: "clearly_not_an_id" } } } before do - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not update the merge request" do @@ -203,7 +173,7 @@ RSpec.describe MergeRequestsController, type: :request do context "when removing an organisation from merge request" do before do MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) - get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params: + get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params: end it "updates the merge request" do @@ -214,7 +184,7 @@ RSpec.describe MergeRequestsController, type: :request do context "when removing an organisation that is not part of a merge from merge request" do before do - get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params: + get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params: end it "does not throw an error" do @@ -306,7 +276,7 @@ RSpec.describe MergeRequestsController, type: :request do it "redirects to merging organisations path" do request - expect(response).to redirect_to(organisations_merge_request_path(merge_request)) + expect(response).to redirect_to(merging_organisations_merge_request_path(merge_request)) end it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do @@ -319,30 +289,6 @@ RSpec.describe MergeRequestsController, type: :request do end end - 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 - - 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(confirm_telephone_number_merge_request_path(merge_request)) - end - end - end - describe "from confirm_telephone_number page" do context "when confirming the number" do let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true, new_telephone_number: "123") } @@ -694,7 +640,7 @@ RSpec.describe MergeRequestsController, type: :request do context "when viewing existing merge request" do before do - get "/merge-request/#{merge_request.id}/organisations", headers:, params: + get "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not allow viewing a merge request" do @@ -708,7 +654,7 @@ RSpec.describe MergeRequestsController, type: :request do context "when updating a merge request with a new organisation" do before do - patch "/merge-request/#{merge_request.id}/organisations", headers:, params: + patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params: end it "does not allow updaing a merge request" do @@ -723,7 +669,7 @@ RSpec.describe MergeRequestsController, type: :request do context "when removing an organisation from merge request" do before do MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) - get "/merge-request/#{merge_request.id}/organisations/remove", headers:, params: + get "/merge-request/#{merge_request.id}/merging-organisations/remove", headers:, params: end it "does not allow removing an organisation" do diff --git a/spec/views/merge_requests/show.html.erb_spec.rb b/spec/views/merge_requests/show.html.erb_spec.rb index 0f0e42b2d..72974fc9e 100644 --- a/spec/views/merge_requests/show.html.erb_spec.rb +++ b/spec/views/merge_requests/show.html.erb_spec.rb @@ -46,15 +46,6 @@ RSpec.describe "merge_requests/show.html.erb", type: :view do expect(rendered).to have_selector("dd", text: merge_request.absorbing_organisation_name) end - it "displays the merging organisations details" do - expect(rendered).to have_selector("dt", text: "Merging organisations") - if merge_request.other_merging_organisations.present? - merge_request.other_merging_organisations.split(",").each do |organisation| - expect(rendered).to have_selector("dd", text: organisation.strip) - end - end - end - it "displays the merge date details" do expect(rendered).to have_selector("dt", text: "Merge date") expect(rendered).to have_selector("dd", text: merge_request.merge_date || "You didn't answer this question") From b7bb79e7ef07a76e5c32811e4740736788ba3c64 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:58:57 +0100 Subject: [PATCH 2/2] CLDC-3586 Update merge date merge request question (#2571) * Remove unused paths and update merge date * lint --- app/controllers/merge_requests_controller.rb | 66 +-- app/models/merge_request.rb | 8 - .../confirm_telephone_number.html.erb | 41 -- .../merge_requests/helpdesk_ticket.html.erb | 5 + app/views/merge_requests/merge_date.html.erb | 22 +- .../new_organisation_address.html.erb | 33 -- .../new_organisation_name.html.erb | 19 - ...new_organisation_telephone_number.html.erb | 20 - .../new_organisation_type.html.erb | 5 - config/locales/en.yml | 13 +- config/routes.rb | 6 +- ...119_remove_new_org_merge_request_fields.rb | 27 ++ db/schema.rb | 10 +- .../merge_requests_controller_spec.rb | 383 ++---------------- 14 files changed, 111 insertions(+), 547 deletions(-) delete mode 100644 app/views/merge_requests/confirm_telephone_number.html.erb create mode 100644 app/views/merge_requests/helpdesk_ticket.html.erb delete mode 100644 app/views/merge_requests/new_organisation_address.html.erb delete mode 100644 app/views/merge_requests/new_organisation_name.html.erb delete mode 100644 app/views/merge_requests/new_organisation_telephone_number.html.erb delete mode 100644 app/views/merge_requests/new_organisation_type.html.erb create mode 100644 db/migrate/20240813112119_remove_new_org_merge_request_fields.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 46798d261..9874cfffb 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -6,12 +6,8 @@ class MergeRequestsController < ApplicationController def absorbing_organisation; end def merging_organisations; end - def confirm_telephone_number; end - def new_organisation_name; end - def new_organisation_address; end - def new_organisation_telephone_number; end - def new_organisation_type; end def merge_date; end + def helpdesk_ticket; end def create ActiveRecord::Base.transaction do @@ -59,19 +55,9 @@ private when "absorbing_organisation" merging_organisations_merge_request_path(@merge_request) when "merging_organisations" - if create_new_organisation? - new_organisation_name_merge_request_path(@merge_request) - else - confirm_telephone_number_merge_request_path(@merge_request) - end - when "confirm_telephone_number" merge_date_merge_request_path(@merge_request) - when "new_organisation_name" - new_organisation_address_merge_request_path(@merge_request) - when "new_organisation_address" - new_organisation_telephone_number_merge_request_path(@merge_request) - when "new_organisation_telephone_number" - new_organisation_type_merge_request_path(@merge_request) + when "merge_date" + helpdesk_ticket_merge_request_path(@merge_request) end end @@ -79,10 +65,6 @@ private page end - def create_new_organisation? - params.dig(:merge_request, :absorbing_organisation_id) == "other" - end - def set_organisations_answer_options answer_options = { "" => "Select an option" } @@ -100,48 +82,32 @@ private :requesting_organisation_id, :status, :absorbing_organisation_id, - :telephone_number_correct, - :new_telephone_number, - :new_organisation_name, - :new_organisation_address_line1, - :new_organisation_address_line2, - :new_organisation_postcode, - :new_organisation_telephone_number, + :merge_date, ) merge_params[:requesting_organisation_id] = current_user.organisation.id - if merge_params[:absorbing_organisation_id].present? - if create_new_organisation? - merge_params[:new_absorbing_organisation] = true - merge_params[:absorbing_organisation_id] = nil - else - merge_params[:new_absorbing_organisation] = false - end - end - - if merge_params[:telephone_number_correct] == "true" - merge_params[:new_telephone_number] = nil - end - merge_params end def validate_response case page when "absorbing_organisation" - if merge_request_params[:absorbing_organisation_id].blank? && merge_request_params[:new_absorbing_organisation].blank? + if merge_request_params[:absorbing_organisation_id].blank? @merge_request.errors.add(:absorbing_organisation_id, :blank) end - when "confirm_telephone_number" - if merge_request_params[:telephone_number_correct].blank? - @merge_request.errors.add(:telephone_number_correct, :blank) if @merge_request.absorbing_organisation.phone.present? - @merge_request.errors.add(:new_telephone_number, :blank) if @merge_request.absorbing_organisation.phone.blank? + when "merge_date" + day = merge_request_params["merge_date(3i)"] + month = merge_request_params["merge_date(2i)"] + year = merge_request_params["merge_date(1i)"] + + return @merge_request.errors.add(:merge_date, :blank) if [day, month, year].all?(&:blank?) + + if [day, month, year].none?(&:blank?) && Date.valid_date?(year.to_i, month.to_i, day.to_i) + merge_request_params["merge_date"] = Time.zone.local(year.to_i, month.to_i, day.to_i) + else + @merge_request.errors.add(:merge_date, :invalid) end - when "new_organisation_name" - @merge_request.errors.add(:new_organisation_name, :blank) if merge_request_params[:new_organisation_name].blank? - when "new_organisation_telephone_number" - @merge_request.errors.add(:new_organisation_telephone_number, :blank) if merge_request_params[:new_organisation_telephone_number].blank? end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ec844576b..2abea9d7f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -4,8 +4,6 @@ class MergeRequest < ApplicationRecord belongs_to :absorbing_organisation, class_name: "Organisation", optional: true has_many :merging_organisations, through: :merge_request_organisations, source: :merging_organisation belongs_to :requester, class_name: "User", optional: true - validate :organisation_name_uniqueness, if: :new_organisation_name - validates :new_telephone_number, presence: true, if: -> { telephone_number_correct == false } STATUS = { "merge_issues" => 0, @@ -23,12 +21,6 @@ class MergeRequest < ApplicationRecord merged.where("merge_requests.merge_date >= ?", open_collection_period_start_date).or(not_merged) } - def organisation_name_uniqueness - if Organisation.where("lower(name) = ?", new_organisation_name&.downcase).exists? - errors.add(:new_organisation_name, :invalid) - end - end - def absorbing_organisation_name absorbing_organisation&.name || "" end diff --git a/app/views/merge_requests/confirm_telephone_number.html.erb b/app/views/merge_requests/confirm_telephone_number.html.erb deleted file mode 100644 index 8b3a354b4..000000000 --- a/app/views/merge_requests/confirm_telephone_number.html.erb +++ /dev/null @@ -1,41 +0,0 @@ -<% 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 %> - -<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> - -

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

- -
-
- <% if @merge_request.absorbing_organisation.phone.present? %> -

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

-
- <%= @merge_request.absorbing_organisation.phone %> -
- <% end %> - - <% if @merge_request.absorbing_organisation.phone.present? %> - <%= f.govuk_radio_buttons_fieldset(:telephone_number_correct, legend: nil) do %> - <%= f.govuk_radio_button :telephone_number_correct, true, checked: @merge_request.telephone_number_correct?, label: { text: "This telephone number is correct" }, link_errors: true %> - <%= f.govuk_radio_button( - :telephone_number_correct, - false, - checked: (@merge_request.new_telephone_number.present? || @merge_request.errors.key?(:new_telephone_number)), - label: { text: "Enter a new phone number" }, - ) do %> - <%= f.govuk_text_field :new_telephone_number, label: { text: "Telephone number" }, width: "two-thirds" %> - <% end %> - <%= f.hidden_field :page, value: "confirm_telephone_number" %> - <% end %> - <% else %> - <%= f.govuk_text_field :new_telephone_number, label: nil, width: "two-thirds" %> - <%= f.hidden_field :page, value: "confirm_telephone_number" %> - <% end %> - <%= f.govuk_submit %> - <% end %> -
-
diff --git a/app/views/merge_requests/helpdesk_ticket.html.erb b/app/views/merge_requests/helpdesk_ticket.html.erb new file mode 100644 index 000000000..3cd6b047f --- /dev/null +++ b/app/views/merge_requests/helpdesk_ticket.html.erb @@ -0,0 +1,5 @@ +<% content_for :before_content do %> + <% title = "Helpdesk ticket" %> + <% content_for :title, title %> + <%= govuk_back_link href: merge_date_merge_request_path(@merge_request) %> +<% end %> diff --git a/app/views/merge_requests/merge_date.html.erb b/app/views/merge_requests/merge_date.html.erb index 35d9ef36d..6766cb552 100644 --- a/app/views/merge_requests/merge_date.html.erb +++ b/app/views/merge_requests/merge_date.html.erb @@ -1,8 +1,24 @@ <% content_for :before_content do %> <% title = "Tell us if your organisation is merging" %> <% content_for :title, title %> - <%# TODO: Update this backlink to also work with the create org flow %> - <%= govuk_back_link href: confirm_telephone_number_merge_request_path(@merge_request) %> + <%= govuk_back_link href: merging_organisations_merge_request_path(@merge_request) %> <% end %> +
+
+ <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> -

What is the merge date?

+

What is the merge date?

+

+ Enter the official merge date. Log and organisation page data will show the new organisation name from this date.

+ For example, 27 3 2024

+ <%= f.govuk_date_field :merge_date, + legend: { hidden: true }, + width: 20 do %> + <% end %> + <%= f.hidden_field :page, value: "merge_date" %> + + <%= f.govuk_submit %> + <% end %> +
+
diff --git a/app/views/merge_requests/new_organisation_address.html.erb b/app/views/merge_requests/new_organisation_address.html.erb deleted file mode 100644 index 7dd331507..000000000 --- a/app/views/merge_requests/new_organisation_address.html.erb +++ /dev/null @@ -1,33 +0,0 @@ -<% content_for :before_content do %> - <% title = "New organisation address" %> - <% content_for :title, title %> - <%= govuk_back_link href: new_organisation_name_merge_request_path(@merge_request) %> -<% end %> - -<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> - -

What is <%= @merge_request.new_organisation_name.possessive %> address?

-
-
- <%= f.govuk_text_field :new_organisation_address_line1, - label: { text: "Address line 1", size: "m" }, - autocomplete: "address-line1" %> - - <%= f.govuk_text_field :new_organisation_address_line2, - label: { text: "Address line 2", size: "m" }, - autocomplete: "address-line2" %> - - <%= f.govuk_text_field :new_organisation_postcode, - label: { text: "Postcode", size: "m" }, - autocomplete: "postal-code", - width: 10 %> - - <%= f.hidden_field :page, value: "new_organisation_address" %> -
- <%= f.govuk_submit %> - <%= govuk_link_to("Skip for now", new_organisation_telephone_number_merge_request_path(@merge_request)) %> -
-
-
-<% end %> diff --git a/app/views/merge_requests/new_organisation_name.html.erb b/app/views/merge_requests/new_organisation_name.html.erb deleted file mode 100644 index 8b391e735..000000000 --- a/app/views/merge_requests/new_organisation_name.html.erb +++ /dev/null @@ -1,19 +0,0 @@ -<% content_for :before_content do %> - <% title = "New organisation name" %> - <% content_for :title, title %> - <%= govuk_back_link href: absorbing_organisation_merge_request_path(id: @merge_request) %> -<% end %> - -<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> - -

What is the new organisation called?

- -
-
- <%= f.govuk_text_field :new_organisation_name, label: nil %> - <%= f.hidden_field :page, value: "new_organisation_name" %> - <%= f.govuk_submit %> - <% end %> -
-
diff --git a/app/views/merge_requests/new_organisation_telephone_number.html.erb b/app/views/merge_requests/new_organisation_telephone_number.html.erb deleted file mode 100644 index 6c0c1d81e..000000000 --- a/app/views/merge_requests/new_organisation_telephone_number.html.erb +++ /dev/null @@ -1,20 +0,0 @@ -<% content_for :before_content do %> - <% title = "New organisation telephone number" %> - <% content_for :title, title %> - <%= govuk_back_link href: new_organisation_address_merge_request_path(@merge_request) %> -<% end %> - -<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> - -

What is <%= @merge_request.new_organisation_name.possessive %> telephone number?

-
-
- <%= f.govuk_text_field :new_organisation_telephone_number, label: nil, width: "two-thirds" %> - <%= f.hidden_field :page, value: "new_organisation_telephone_number" %> -
- <%= f.govuk_submit %> -
-
-
-<% end %> diff --git a/app/views/merge_requests/new_organisation_type.html.erb b/app/views/merge_requests/new_organisation_type.html.erb deleted file mode 100644 index 2e22071e7..000000000 --- a/app/views/merge_requests/new_organisation_type.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% content_for :before_content do %> - <% title = "New organisation type" %> - <% content_for :title, title %> - <%= govuk_back_link href: new_organisation_telephone_number_merge_request_path(@merge_request) %> -<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index a53b375fe..8ac3f0d53 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -176,16 +176,9 @@ en: attributes: absorbing_organisation_id: blank: "Select the organisation absorbing the others" - telephone_number_correct: - blank: "Select to confirm or enter a new telephone number" - invalid: "Enter a valid telephone number" - new_telephone_number: - blank: "Enter a valid telephone number" - new_organisation_name: - blank: "Enter an organisation name" - invalid: "An organisation with this name already exists" - new_organisation_telephone_number: - blank: "Enter a valid telephone number" + merge_date: + blank: "Enter a merge date" + invalid: "Enter a valid merge date" notification: logs_deleted: diff --git a/config/routes.rb b/config/routes.rb index b304ecb10..8920e1587 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -199,12 +199,8 @@ Rails.application.routes.draw do patch "merging-organisations", to: "merge_requests#update_merging_organisations" get "merging-organisations/remove", to: "merge_requests#remove_merging_organisation" get "absorbing-organisation" - get "confirm-telephone-number" - get "new-organisation-name" - get "new-organisation-address" - get "new-organisation-telephone-number" - get "new-organisation-type" get "merge-date" + get "helpdesk-ticket" get "details", to: "merge_requests#details" end end diff --git a/db/migrate/20240813112119_remove_new_org_merge_request_fields.rb b/db/migrate/20240813112119_remove_new_org_merge_request_fields.rb new file mode 100644 index 000000000..c7050b54e --- /dev/null +++ b/db/migrate/20240813112119_remove_new_org_merge_request_fields.rb @@ -0,0 +1,27 @@ +class RemoveNewOrgMergeRequestFields < ActiveRecord::Migration[7.0] + def up + change_table :merge_requests, bulk: true do |t| + t.remove :new_absorbing_organisation + t.remove :telephone_number_correct + t.remove :new_telephone_number + t.remove :new_organisation_name + t.remove :new_organisation_address_line1 + t.remove :new_organisation_address_line2 + t.remove :new_organisation_postcode + t.remove :new_organisation_telephone_number + end + end + + def down + change_table :merge_requests, bulk: true do |t| + t.column :new_absorbing_organisation, :boolean + t.column :telephone_number_correct, :boolean + t.column :new_telephone_number, :string + t.column :new_organisation_name, :string + t.column :new_organisation_address_line1, :string + t.column :new_organisation_address_line2, :string + t.column :new_organisation_postcode, :string + t.column :new_organisation_telephone_number, :string + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ca0241179..2d728a72d 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_13_072041) do +ActiveRecord::Schema[7.0].define(version: 2024_08_13_112119) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -421,14 +421,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_13_072041) do t.datetime "updated_at", null: false t.integer "status" t.integer "absorbing_organisation_id" - t.boolean "new_absorbing_organisation" - t.boolean "telephone_number_correct" - t.string "new_telephone_number" - t.string "new_organisation_name" - t.string "new_organisation_address_line1" - t.string "new_organisation_address_line2" - t.string "new_organisation_postcode" - t.string "new_organisation_telephone_number" t.datetime "merge_date" t.integer "requester_id" t.string "helpdesk_ticket" diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 805f91f1c..8bbd419f7 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -194,38 +194,6 @@ RSpec.describe MergeRequestsController, type: :request do end end - describe "#confirm_telephone_number" do - let(:merge_request) do - MergeRequest.create!( - absorbing_organisation: create(:organisation, phone: phone_number), - requesting_organisation: organisation, - ) - end - - before { get "/merge-request/#{merge_request.id}/confirm-telephone-number", headers: } - - context "when org has phone number" do - let(:phone_number) { 123 } - - it "asks to confirm or provide new number" do - expect(page).to have_content("This telephone number is correct") - expect(page).to have_content("Confirm the telephone number on file, or enter a new one.") - expect(page).to have_content(phone_number) - expect(page).to have_content("What is #{merge_request.absorbing_organisation.name}'s telephone number?") - end - end - - context "when org does not have a phone number set" do - let(:phone_number) { nil } - - it "asks provide new number" do - expect(page).not_to have_content("This telephone number is correct") - expect(page).not_to have_content("Confirm the telephone number on file, or enter a new one.") - expect(page).to have_content("What is #{merge_request.absorbing_organisation.name}'s telephone number?") - end - end - end - describe "#absorbing_organisation" do let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } @@ -241,6 +209,19 @@ RSpec.describe MergeRequestsController, type: :request do end end + describe "#merge_date" do + context "when viewing merge date page" do + before do + merge_request.update!(absorbing_organisation_id: organisation.id) + get "/merge-request/#{merge_request.id}/merge-date", headers: + end + + it "shows the correct content" do + expect(page).to have_content("What is the merge date?") + end + end + end + describe "#update" do describe "from absorbing_organisation page" do context "when not answering the question" do @@ -264,7 +245,7 @@ RSpec.describe MergeRequestsController, type: :request do end context "when absorbing_organisation_id set to id" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:params) do { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } end @@ -279,109 +260,19 @@ RSpec.describe MergeRequestsController, type: :request do expect(response).to redirect_to(merging_organisations_merge_request_path(merge_request)) end - it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do + it "updates absorbing_organisation_id" 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) + }.from(nil).to(other_organisation) end end end - describe "from confirm_telephone_number page" do - context "when confirming the number" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true, new_telephone_number: "123") } - let(:params) do - { merge_request: { telephone_number_correct: true, page: "confirm_telephone_number" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "redirects merge date path" do - request - - expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) - end - - it "updates telephone_number_correct and sets new_telephone_number to nil" do - expect { request }.to change { - merge_request.reload.telephone_number_correct - }.from(nil).to(true).and change { - merge_request.reload.new_telephone_number - }.from("123").to(nil) - end - end - - context "when setting new number" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } - let(:params) do - { merge_request: { telephone_number_correct: false, new_telephone_number: "123", page: "confirm_telephone_number" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "redirects merge date path" do - request - - expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) - end - - it "updates telephone_number_correct and sets new_telephone_number to nil" do - expect { request }.to change { - merge_request.reload.new_telephone_number - }.from(nil).to("123") - end - end - - context "when not answering the question and the org has phone number" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: create(:organisation, phone: "123")) } - let(:params) do - { merge_request: { page: "confirm_telephone_number" } } - 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 to confirm or enter a new telephone number") - end - - it "does not update the request" do - expect { request }.not_to(change { merge_request.reload.attributes }) - end - end - - context "when not answering the question and the org does not have a phone number" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } - let(:params) do - { merge_request: { page: "confirm_telephone_number" } } - end - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "renders the error" do - request - - expect(page).to have_content("Enter a valid telephone number") - end - - it "does not update the request" do - expect { request }.not_to(change { merge_request.reload.attributes }) - end - end - - context "when not answering the phone number" do + describe "from merge_date 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: "confirm_telephone_number", telephone_number_correct: false } } + { merge_request: { page: "merge_date" } } end let(:request) do patch "/merge-request/#{merge_request.id}", headers:, params: @@ -390,230 +281,53 @@ RSpec.describe MergeRequestsController, type: :request do it "renders the error" do request - expect(page).to have_content("Enter a valid telephone number") + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("Enter a merge date") end it "does not update the request" do expect { request }.not_to(change { merge_request.reload.attributes }) end end - end - - describe "#new_organisation_name" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } - - context "when viewing the new organisation name page" do - before do - get "/merge-request/#{merge_request.id}/new-organisation-name", headers: - end - - it "displays the correct question" do - expect(page).to have_content("What is the new organisation called?") - end - - it "has the correct back button" do - expect(page).to have_link("Back", href: absorbing_organisation_merge_request_path(merge_request)) - end - end - - context "when updating the new organisation name" do - let(:params) do - { merge_request: { new_organisation_name: "new org name", page: "new_organisation_name" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "redirects to new organisation address path" do - request - expect(response).to redirect_to(new_organisation_address_merge_request_path(merge_request)) - end - - it "updates new organisation name to the correct name" do - expect { request }.to change { - merge_request.reload.new_organisation_name - }.from(nil).to("new org name") - end - end - - context "when the new organisation name is not answered" do - let(:params) do - { merge_request: { new_organisation_name: nil, page: "new_organisation_name" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "renders the error" do - request - expect(page).to have_content("Enter an organisation name") - end - - it "does not update the organisation name" do - expect { request }.not_to(change { merge_request.reload.attributes }) - end - end - - context "when the new organisation name already exists" do - before do - create(:organisation, name: "new org name") - end + context "when merge date set to an invalid date" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:params) do - { merge_request: { new_organisation_name: "New org name", page: "new_organisation_name" } } + { merge_request: { page: "merge_date", "merge_date(3i)": "10", "merge_date(2i)": "44", "merge_date(1i)": "2022" } } end let(:request) do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "renders the error" do + it "displays the page with an error message" do request - expect(page).to have_content("An organisation with this name already exists") - end - it "does not update the organisation name" do - expect { request }.not_to(change { merge_request.reload.attributes }) + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("Enter a valid merge date") end end - end - - describe "#new_organisation_address" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_organisation_name: "New name", new_absorbing_organisation: true) } - context "when viewing the new organisation name page" do - before do - get "/merge-request/#{merge_request.id}/new-organisation-address", headers: - end - - it "displays the correct question" do - expect(page).to have_content("What is New name’s address?") - end - - it "has the correct back button" do - expect(page).to have_link("Back", href: new_organisation_name_merge_request_path(merge_request)) - end - - it "has a skip link" do - expect(page).to have_link("Skip for now", href: new_organisation_telephone_number_merge_request_path(merge_request)) - end - end - - context "when updating the new organisation address" do + context "when merge date set to a valid date" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:params) do - { merge_request: { - new_organisation_address_line1: "first address line", - new_organisation_address_line2: "second address line", - new_organisation_postcode: "new postcode", - page: "new_organisation_address", - } } + { merge_request: { page: "merge_date", "merge_date(3i)": "10", "merge_date(2i)": "4", "merge_date(1i)": "2022" } } end let(:request) do patch "/merge-request/#{merge_request.id}", headers:, params: end - it "redirects to new organisation telephone path" do + it "redirects to helpdesk ticket path" do request - expect(response).to redirect_to(new_organisation_telephone_number_merge_request_path(merge_request)) - end - - it "updates new organisation address line 1 to correct address line" do - expect { request }.to change { - merge_request.reload.new_organisation_address_line1 - }.from(nil).to("first address line") - end - it "updates new organisation address line 2 to correct address line" do - expect { request }.to change { - merge_request.reload.new_organisation_address_line2 - }.from(nil).to("second address line") + expect(response).to redirect_to(helpdesk_ticket_merge_request_path(merge_request)) end - it "updates new organisation postcode to correct address line" do + it "updates merge_date" do expect { request }.to change { - merge_request.reload.new_organisation_postcode - }.from(nil).to("new postcode") - end - end - - context "when address is not provided" do - let(:params) do - { merge_request: { - new_organisation_address_line1: nil, - new_organisation_address_line2: nil, - new_organisation_postcode: nil, - page: "new_organisation_address", - } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "does not throw an error" do - request - expect(response).to redirect_to(new_organisation_telephone_number_merge_request_path(merge_request)) - end - end - end - - describe "#new_organisation_telephone_number" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_organisation_name: "New name", new_absorbing_organisation: true) } - - context "when viewing the new organisation telephone number page" do - before do - get "/merge-request/#{merge_request.id}/new-organisation-telephone-number", headers: - end - - it "displays the correct question" do - expect(page).to have_content("What is New name’s telephone number?") - end - - it "has the correct back button" do - expect(page).to have_link("Back", href: new_organisation_address_merge_request_path(merge_request)) - end - end - - context "when updating the new organisation telephone number" do - let(:params) do - { merge_request: { new_organisation_telephone_number: "1234", page: "new_organisation_telephone_number" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "redirects to new organisation type path" do - request - expect(response).to redirect_to(new_organisation_type_merge_request_path(merge_request)) - end - - it "updates new organisation name to the correct telephone number" do - expect { request }.to change { - merge_request.reload.new_organisation_telephone_number - }.from(nil).to("1234") - end - end - - context "when the new organisation telephone number is not answered" do - let(:params) do - { merge_request: { new_organisation_telephone_number: nil, page: "new_organisation_telephone_number" } } - end - - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end - - it "renders the error" do - request - expect(page).to have_content("Enter a valid telephone number") - end - - it "does not update the organisation telephone number" do - expect { request }.not_to(change { merge_request.reload.attributes }) + merge_request.reload.merge_date + }.from(nil).to(Time.zone.local(2022, 4, 10)) end end end @@ -625,7 +339,7 @@ RSpec.describe MergeRequestsController, type: :request do sign_in user end - describe "#organisations" do + describe "#merging_organisations" do let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } } context "when creating a new merge request" do @@ -649,7 +363,7 @@ RSpec.describe MergeRequestsController, type: :request do end end - describe "#update_organisations" do + describe "#update_merging_organisations" do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } context "when updating a merge request with a new organisation" do @@ -663,7 +377,7 @@ RSpec.describe MergeRequestsController, type: :request do end end - describe "#remove_organisation" do + describe "#remove_merging_organisation" do let(:params) { { merge_request: { merging_organisation: other_organisation.id } } } context "when removing an organisation from merge request" do @@ -681,7 +395,7 @@ RSpec.describe MergeRequestsController, type: :request do describe "#update" do describe "from absorbing_organisation page" do context "when absorbing_organisation_id set to id" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation) } let(:params) do { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } end @@ -695,25 +409,6 @@ RSpec.describe MergeRequestsController, type: :request do end end end - - 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 - - context "when adding other merging organisations" do - before do - MergeRequestOrganisation.create!(merge_request_id: merge_request.id, merging_organisation_id: other_organisation.id) - request - end - - it "does not allow updating merging organisations" do - expect(response).to have_http_status(:not_found) - end - end - end end end end