From d8a67040b3d90ddec02bd6f9362fbc6248662b77 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Thu, 4 May 2023 12:48:56 +0100
Subject: [PATCH] CLDC-2058 Add merging organisation name question (#1598)

* Add new organisation name column

* Update new organisation name page

* Refactor validate_response

* Add placeholder new_organisation_address page

* Add existing organisation name validation

* Refactor error messages

* Extract some validations to the model

* Update which field we add the errors to
---
 app/controllers/merge_requests_controller.rb  | 51 +++++------
 app/models/merge_request.rb                   |  8 ++
 .../confirm_telephone_number.html.erb         |  4 +-
 .../merge_requests/new_org_name.html.erb      |  7 --
 .../new_organisation_address.html.erb         |  5 ++
 .../new_organisation_name.html.erb            | 19 ++++
 config/locales/en.yml                         | 17 +++-
 config/routes.rb                              |  3 +-
 ...0230502135049_add_new_organisation_name.rb |  5 ++
 db/schema.rb                                  |  3 +-
 .../merge_requests_controller_spec.rb         | 89 ++++++++++++++++++-
 11 files changed, 168 insertions(+), 43 deletions(-)
 delete mode 100644 app/views/merge_requests/new_org_name.html.erb
 create mode 100644 app/views/merge_requests/new_organisation_address.html.erb
 create mode 100644 app/views/merge_requests/new_organisation_name.html.erb
 create mode 100644 db/migrate/20230502135049_add_new_organisation_name.rb

diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb
index 930a79b04..17959e264 100644
--- a/app/controllers/merge_requests_controller.rb
+++ b/app/controllers/merge_requests_controller.rb
@@ -6,16 +6,17 @@ class MergeRequestsController < ApplicationController
     remove_merging_organisation
     absorbing_organisation
     confirm_telephone_number
-    new_org_name
+    new_organisation_name
+    new_organisation_address
     merge_date
   ]
   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 new_organisation_name; end
+  def new_organisation_address; end
   def merge_date; end
 
   def create
@@ -33,7 +34,9 @@ class MergeRequestsController < ApplicationController
   end
 
   def update
-    if @merge_request.update(merge_request_params)
+    validate_response
+
+    if @merge_request.errors.blank? && @merge_request.update(merge_request_params)
       redirect_to next_page_path
     else
       render previous_template, status: :unprocessable_entity
@@ -65,8 +68,8 @@ private
   def next_page_path
     case page
     when "absorbing_organisation"
-      if create_new_org?
-        new_org_name_merge_request_path(@merge_request)
+      if create_new_organisation?
+        new_organisation_name_merge_request_path(@merge_request)
       else
         confirm_telephone_number_merge_request_path(@merge_request)
       end
@@ -74,6 +77,8 @@ private
       absorbing_organisation_merge_request_path(@merge_request)
     when "confirm_telephone_number"
       merge_date_merge_request_path(@merge_request)
+    when "new_organisation_name"
+      new_organisation_address_merge_request_path(@merge_request)
     end
   end
 
@@ -81,7 +86,7 @@ private
     page
   end
 
-  def create_new_org?
+  def create_new_organisation?
     params.dig(:merge_request, :absorbing_organisation_id) == "other"
   end
 
@@ -102,6 +107,7 @@ private
       :absorbing_organisation_id,
       :telephone_number_correct,
       :new_telephone_number,
+      :new_organisation_name,
     )
 
     if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?)
@@ -109,7 +115,7 @@ private
     end
 
     if merge_params[:absorbing_organisation_id].present?
-      if create_new_org?
+      if create_new_organisation?
         merge_params[:new_absorbing_organisation] = true
         merge_params[:absorbing_organisation_id] = nil
       else
@@ -117,7 +123,7 @@ private
       end
     end
 
-    if merge_params[:telephone_number_correct] == "1"
+    if merge_params[:telephone_number_correct] == "true"
       merge_params[:new_telephone_number] = nil
     end
 
@@ -125,23 +131,18 @@ private
   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, status: :unprocessable_entity
-    end
-
-    if page == "confirm_telephone_number"
-      if merge_request_params[:telephone_number_correct].blank? && merge_request_params[:new_telephone_number].blank?
-        if @merge_request.absorbing_organisation.phone.present?
-          @merge_request.errors.add(:telephone_number_correct, I18n.t("validations.merge_request.telephone_number_correct_blank"))
-        else
-          @merge_request.errors.add(:telephone_number_correct, I18n.t("validations.merge_request.new_telephone_number_blank"))
-        end
-        render previous_template, status: :unprocessable_entity
-      elsif merge_request_params[:telephone_number_correct] == "0" && merge_request_params[:new_telephone_number].blank?
-        @merge_request.errors.add(:new_telephone_number, I18n.t("validations.merge_request.new_telephone_number_blank"))
-        render previous_template, status: :unprocessable_entity
+    case page
+    when "absorbing_organisation"
+      if merge_request_params[:absorbing_organisation_id].blank? && merge_request_params[:new_absorbing_organisation].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?
       end
+    when "new_organisation_name"
+      @merge_request.errors.add(:new_organisation_name, :blank) if merge_request_params[:new_organisation_name].blank?
     end
   end
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 94e2f6d00..e62b1d39d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -3,10 +3,18 @@ class MergeRequest < ApplicationRecord
   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
+  validate :organisation_name_uniqueness, if: :new_organisation_name
+  validates :new_telephone_number, presence: true, if: -> { telephone_number_correct == false }
 
   STATUS = {
     "unsubmitted" => 0,
     "submitted" => 1,
   }.freeze
   enum status: STATUS
+
+  def organisation_name_uniqueness
+    if Organisation.where("lower(name) = ?", new_organisation_name&.downcase).exists?
+      errors.add(:new_organisation_name, :invalid)
+    end
+  end
 end
diff --git a/app/views/merge_requests/confirm_telephone_number.html.erb b/app/views/merge_requests/confirm_telephone_number.html.erb
index 9a05ee54e..0c92a4f28 100644
--- a/app/views/merge_requests/confirm_telephone_number.html.erb
+++ b/app/views/merge_requests/confirm_telephone_number.html.erb
@@ -20,8 +20,8 @@
 
       <% 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, 1, checked: @merge_request.telephone_number_correct?, label: { text: "This telephone number is correct" }, link_errors: true %>
-          <%= f.govuk_radio_button :telephone_number_correct, 0, checked: @merge_request.new_telephone_number.present?, label: { text: "Enter a new phone number" } 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?, 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" %>
diff --git a/app/views/merge_requests/new_org_name.html.erb b/app/views/merge_requests/new_org_name.html.erb
deleted file mode 100644
index 9c4075735..000000000
--- a/app/views/merge_requests/new_org_name.html.erb
+++ /dev/null
@@ -1,7 +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 %>
-
-Provide new org name
diff --git a/app/views/merge_requests/new_organisation_address.html.erb b/app/views/merge_requests/new_organisation_address.html.erb
new file mode 100644
index 000000000..32dc57c86
--- /dev/null
+++ b/app/views/merge_requests/new_organisation_address.html.erb
@@ -0,0 +1,5 @@
+<% content_for :before_content do %>
+  <% title = "New organisation address" %>
+  <% content_for :title, title %>
+  <%= govuk_back_link href: new_organisation_name_merge_request_path(id: @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
new file mode 100644
index 000000000..8b391e735
--- /dev/null
+++ b/app/views/merge_requests/new_organisation_name.html.erb
@@ -0,0 +1,19 @@
+<% 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 %>
+
+  <h2 class="govuk-heading-l">What is the new organisation called?</h2>
+
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds-from-desktop">
+      <%= f.govuk_text_field :new_organisation_name, label: nil %>
+      <%= f.hidden_field :page, value: "new_organisation_name" %>
+      <%= f.govuk_submit %>
+    <% end %>
+  </div>
+</div>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 5a3189f03..4cf4e2c0e 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -123,6 +123,20 @@ en:
             role:
               invalid: "Role must be data accessor, data provider or data coordinator"
               blank: "Select role"
+        merge_request:
+          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"
+
+
 
   validations:
     organisation:
@@ -515,9 +529,6 @@ 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
-      telephone_number_correct_blank: Select to confirm or enter a new telephone number
-      new_telephone_number_blank: Enter a valid telephone number
 
   soft_validations:
     net_income:
diff --git a/config/routes.rb b/config/routes.rb
index bec7aed6b..48afc3357 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -138,7 +138,8 @@ Rails.application.routes.draw do
       get "organisations/remove", to: "merge_requests#remove_merging_organisation"
       get "absorbing-organisation"
       get "confirm-telephone-number"
-      get "new-org-name"
+      get "new-organisation-name"
+      get "new-organisation-address"
       get "merge-date"
     end
   end
diff --git a/db/migrate/20230502135049_add_new_organisation_name.rb b/db/migrate/20230502135049_add_new_organisation_name.rb
new file mode 100644
index 000000000..beb18d0e2
--- /dev/null
+++ b/db/migrate/20230502135049_add_new_organisation_name.rb
@@ -0,0 +1,5 @@
+class AddNewOrganisationName < ActiveRecord::Migration[7.0]
+  change_table :merge_requests, bulk: true do |t|
+    t.column :new_organisation_name, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 73956bd24..cbe185ed4 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_27_130334) do
+ActiveRecord::Schema[7.0].define(version: 2023_05_02_135049) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -371,6 +371,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_27_130334) do
     t.boolean "new_absorbing_organisation"
     t.boolean "telephone_number_correct"
     t.string "new_telephone_number"
+    t.string "new_organisation_name"
   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 943be6374..3ffe1fe13 100644
--- a/spec/requests/merge_requests_controller_spec.rb
+++ b/spec/requests/merge_requests_controller_spec.rb
@@ -325,7 +325,7 @@ RSpec.describe MergeRequestsController, type: :request do
           it "redirects to new org path" do
             request
 
-            expect(response).to redirect_to(new_org_name_merge_request_path(merge_request))
+            expect(response).to redirect_to(new_organisation_name_merge_request_path(merge_request))
           end
 
           it "resets absorbing_organisation and sets new_absorbing_organisation to true" do
@@ -367,7 +367,7 @@ RSpec.describe MergeRequestsController, type: :request 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: "1", page: "confirm_telephone_number" } }
+            { merge_request: { telephone_number_correct: true, page: "confirm_telephone_number" } }
           end
 
           let(:request) do
@@ -392,7 +392,7 @@ RSpec.describe MergeRequestsController, type: :request do
         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: "0", new_telephone_number: "123", page: "confirm_telephone_number" } }
+            { merge_request: { telephone_number_correct: false, new_telephone_number: "123", page: "confirm_telephone_number" } }
           end
 
           let(:request) do
@@ -455,7 +455,7 @@ RSpec.describe MergeRequestsController, type: :request do
         context "when not answering the phone number" 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: "0" } }
+            { merge_request: { page: "confirm_telephone_number", telephone_number_correct: false } }
           end
           let(:request) do
             patch "/merge-request/#{merge_request.id}", headers:, params:
@@ -472,6 +472,87 @@ RSpec.describe MergeRequestsController, type: :request do
           end
         end
       end
+
+      describe "#new_organsation_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
+
+          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 "renders the error" 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 })
+          end
+        end
+      end
     end
   end