From 9f4f5c651d58c7ee281ce949ec6928eaf92841e2 Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Wed, 16 Apr 2025 17:19:13 +0100
Subject: [PATCH] Refactor organisation name change validations to use I18n for
 error messages

---
 app/models/organisation_name_change.rb       | 16 ++++++++--------
 config/locales/en.yml                        | 14 +++++++++++++-
 spec/models/organisation_name_change_spec.rb | 10 +++++-----
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/app/models/organisation_name_change.rb b/app/models/organisation_name_change.rb
index 787448131..975394b5b 100644
--- a/app/models/organisation_name_change.rb
+++ b/app/models/organisation_name_change.rb
@@ -5,8 +5,8 @@ class OrganisationNameChange < ApplicationRecord
   scope :before_date, ->(date) { where("startdate < ?", date) }
   scope :after_date, ->(date) { where("startdate > ?", date) }
 
-  validates :name, presence: { message: "New name is required and cannot be left blank." }
-  validates :startdate, presence: { message: "Start date must be provided unless this is an immediate change." }, unless: -> { immediate_change }
+  validates :name, presence: { message: I18n.t("validations.organisation.name_changes.name.blank") }
+  validates :startdate, presence: { message: I18n.t("validations.organisation.name_changes.startdate.blank") }, unless: -> { immediate_change }
   validate :startdate_must_be_after_last_change
   validate :name_must_be_different_from_current
   validate :startdate_must_be_unique_for_organisation
@@ -75,7 +75,7 @@ private
                                    .first&.startdate
 
     if last_startdate && startdate <= last_startdate
-      errors.add(:startdate, "Start date must be after the last change date (#{last_startdate}).")
+      errors.add(:startdate, I18n.t("validations.organisation.name_changes.startdate.must_be_after_last_change", last_startdate:))
     end
   end
 
@@ -83,8 +83,8 @@ private
     return if startdate.blank?
 
     if organisation.organisation_name_changes.visible.select(&:persisted?).any? { |record| record.startdate == startdate }
-      errors.add(:startdate, "Start date cannot be the same as another name change.") unless immediate_change
-      errors.add(:immediate_change, "Start date cannot be the same as another name change.") if immediate_change
+      errors.add(:startdate, I18n.t("validations.organisation.name_changes.startdate.cannot_be_the_same_as_another_change")) unless immediate_change
+      errors.add(:immediate_change, I18n.t("validations.organisation.name_changes.immediate_change.cannot_be_the_same_as_another_change")) if immediate_change
     end
   end
 
@@ -92,7 +92,7 @@ private
     return if name.blank? || startdate.blank?
 
     if name == organisation.name(date: startdate)
-      errors.add(:name, "New name must be different from the current name on the change date.")
+      errors.add(:name, I18n.t("validations.organisation.name_changes.name.must_be_different"))
     end
   end
 
@@ -100,8 +100,8 @@ private
     return if startdate.blank? || organisation.merge_date.blank?
 
     if startdate >= organisation.merge_date
-      errors.add(:startdate, "Start date must be earlier than the organisation's merge date (#{organisation.merge_date.to_formatted_s(:govuk_date)}). You cannot make changes to the name of an organisation after it has merged.") unless immediate_change
-      errors.add(:immediate_change, "Start date must be earlier than the organisation's merge date (#{organisation.merge_date.to_formatted_s(:govuk_date)}). You cannot make changes to the name of an organisation after it has merged.") if immediate_change
+      errors.add(:startdate, I18n.t("validations.organisation.name_changes.startdate.must_be_before_merge_date", merge_date: organisation.merge_date.to_formatted_s(:govuk_date))) unless immediate_change
+      errors.add(:immediate_change, I18n.t("validations.organisation.name_changes.immediate_change.must_be_before_merge_date", merge_date: organisation.merge_date.to_formatted_s(:govuk_date))) if immediate_change
     end
   end
 end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 086a26923..bcdd1cc88 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -229,12 +229,23 @@ en:
         already_added: "You have already added this managing agent."
       merged: "That organisation has already been merged. Select a different organisation."
       scheme_duplicates_not_resolved: "You must resolve all duplicates or indicate that there are no duplicates"
+      name_changes:
+        name:
+          blank: "New name is required and cannot be left blank."
+          must_be_different: "New name must be different from the current name on the change date."
+        startdate:
+          blank: "Start date must be provided unless this is an immediate change."
+          must_be_after_last_change: "Start date must be after the last change date (%{last_startdate})."
+          cannot_be_the_same_as_another_change: "Start date cannot be the same as another name change."
+          must_be_before_merge_date: "Start date must be earlier than the organisation's merge date (%{merge_date}). You cannot make changes to the name of an organisation after it has merged."
+        immediate_change:
+          cannot_be_the_same_as_another_change: "Start date cannot be the same as another name change."
+          must_be_before_merge_date: "Start date must be earlier than the organisation's merge date (%{merge_date}). You cannot make changes to the name of an organisation after it has merged."
     not_answered: "You must answer %{question}"
     not_number: "%{field} must be a number."
     invalid_option: "Enter a valid value for %{question}"
     invalid_number: "Enter a number for %{question}"
     no_address_found: "We could not find this address. Check the address data in your CSV file is correct and complete, or select the correct address using the CORE site."
-
     date:
       outside_collection_window: "Enter a date within the %{year_combo} collection year, which is between 1st April %{start_year} and 31st March %{end_year}."
     postcode: "Enter a postcode in the correct format, for example AA1 1AA."
@@ -361,6 +372,7 @@ en:
         before_deactivation: "This location was deactivated on %{date}. The reactivation date must be on or after deactivation date."
       deactivation:
         during_deactivated_period: "The location is already deactivated during this date, please enter a different date."
+
     merge_request:
       organisation_part_of_another_merge: "This organisation is part of another merge - select a different one."
       organisation_part_of_another_incomplete_merge: "Another merge request records %{organisation} as merging into %{absorbing_organisation} on %{merge_date}. Select another organisation or remove this organisation from the other merge request."
diff --git a/spec/models/organisation_name_change_spec.rb b/spec/models/organisation_name_change_spec.rb
index 16cf50e72..db32874bd 100644
--- a/spec/models/organisation_name_change_spec.rb
+++ b/spec/models/organisation_name_change_spec.rb
@@ -12,34 +12,34 @@ RSpec.describe OrganisationNameChange, type: :model do
     it "is invalid without a name" do
       name_change = build(:organisation_name_change, organisation:, name: nil)
       expect(name_change).not_to be_valid
-      expect(name_change.errors[:name]).to include("New name is required and cannot be left blank.")
+      expect(name_change.errors[:name]).to include(I18n.t("validations.organisation.name_changes.name.blank"))
     end
 
     it "is invalid without a startdate if not immediate" do
       name_change = build(:organisation_name_change, organisation:, startdate: nil, immediate_change: false)
       expect(name_change).not_to be_valid
-      expect(name_change.errors[:startdate]).to include("Start date must be provided unless this is an immediate change.")
+      expect(name_change.errors[:startdate]).to include(I18n.t("validations.organisation.name_changes.startdate.blank"))
     end
 
     it "is invalid if startdate is not unique for the organisation" do
       create(:organisation_name_change, organisation:, startdate: Time.zone.tomorrow)
       name_change = build(:organisation_name_change, organisation:, immediate_change: false, startdate: Time.zone.tomorrow)
       expect(name_change).not_to be_valid
-      expect(name_change.errors[:startdate]).to include("Start date cannot be the same as another name change.")
+      expect(name_change.errors[:startdate]).to include(I18n.t("validations.organisation.name_changes.startdate.cannot_be_the_same_as_another_change"))
     end
 
     it "is invalid if name is the same as the current name on the change date" do
       create(:organisation_name_change, organisation:, name: "New Name", startdate: 1.day.ago)
       name_change = build(:organisation_name_change, organisation:, name: "New Name", startdate: Time.zone.now)
       expect(name_change).not_to be_valid
-      expect(name_change.errors[:name]).to include("New name must be different from the current name on the change date.")
+      expect(name_change.errors[:name]).to include(I18n.t("validations.organisation.name_changes.name.must_be_different"))
     end
 
     it "is invalid if startdate is after the organisation's merge date" do
       organisation.update!(merge_date: Time.zone.now)
       name_change = build(:organisation_name_change, organisation:, immediate_change: false, startdate: Time.zone.tomorrow)
       expect(name_change).not_to be_valid
-      expect(name_change.errors[:startdate]).to include("Start date must be earlier than the organisation's merge date (#{organisation.merge_date.to_formatted_s(:govuk_date)}). You cannot make changes to the name of an organisation after it has merged.")
+      expect(name_change.errors[:startdate]).to include(I18n.t("validations.organisation.name_changes.startdate.must_be_before_merge_date", merge_date: organisation.merge_date.to_formatted_s(:govuk_date)))
     end
   end