From ca2d670116f1399fd48db3d07fd36c63ec963020 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Wed, 11 Dec 2024 15:52:13 +0000
Subject: [PATCH] CLDC-3748 Check org name uniqueness (#2818)
* Check org name uniqueness
* Some tests
* Do not update org name
* Update some request specs
* Remove unnecessary updates
* Update error message
---
app/controllers/organisations_controller.rb | 1 +
app/models/organisation.rb | 1 +
config/locales/en.yml | 1 +
.../20241125153349_add_unique_index_to_org_name.rb | 5 +++++
db/schema.rb | 1 +
spec/fixtures/exports/general_needs_log.xml | 4 ++--
spec/fixtures/exports/general_needs_log_23_24.xml | 4 ++--
spec/fixtures/exports/general_needs_log_24_25.xml | 4 ++--
spec/fixtures/exports/organisation.xml | 2 +-
spec/fixtures/exports/supported_housing_logs.xml | 4 ++--
spec/fixtures/exports/user.xml | 2 +-
.../lettings/questions/managing_organisation_spec.rb | 2 +-
spec/models/organisation_spec.rb | 6 ++++++
.../organisation_relationships_controller_spec.rb | 8 ++++----
spec/requests/organisations_controller_spec.rb | 10 ++++++++--
.../exports/lettings_log_export_service_spec.rb | 2 ++
.../exports/organisation_export_service_spec.rb | 1 +
spec/services/exports/user_export_service_spec.rb | 1 +
18 files changed, 42 insertions(+), 17 deletions(-)
create mode 100644 db/migrate/20241125153349_add_unique_index_to_org_name.rb
diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index 8ffe426d7..93b667a99 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -155,6 +155,7 @@ class OrganisationsController < ApplicationController
end
redirect_to details_organisation_path(@organisation)
else
+ @used_rent_periods = @organisation.lettings_logs.pluck(:period).uniq.compact.map(&:to_s)
@rent_periods = helpers.rent_periods_with_checked_attr(checked_periods: selected_rent_periods)
render :edit, status: :unprocessable_entity
end
diff --git a/app/models/organisation.rb b/app/models/organisation.rb
index 23f91f1ad..69c80d198 100644
--- a/app/models/organisation.rb
+++ b/app/models/organisation.rb
@@ -58,6 +58,7 @@ class Organisation < ApplicationRecord
alias_method :la?, :LA?
validates :name, presence: { message: I18n.t("validations.organisation.name_missing") }
+ validates :name, uniqueness: { case_sensitive: false, message: I18n.t("validations.organisation.name_not_unique") }
validates :provider_type, presence: { message: I18n.t("validations.organisation.provider_type_missing") }
def self.find_by_id_on_multiple_fields(id)
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 851a9ea2c..965c1f7a6 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -235,6 +235,7 @@ en:
organisation:
data_sharing_agreement_not_signed: "Your organisation must accept the Data Sharing Agreement before you can create any logs."
name_missing: "Enter the name of the organisation."
+ name_not_unique: "An organisation with this name already exists. Use the organisation that already exists or add a location or other identifier to the name. For example: Organisation name (City)."
provider_type_missing: "Select the organisation type."
stock_owner:
blank: "You must choose a stock owner."
diff --git a/db/migrate/20241125153349_add_unique_index_to_org_name.rb b/db/migrate/20241125153349_add_unique_index_to_org_name.rb
new file mode 100644
index 000000000..a7a124183
--- /dev/null
+++ b/db/migrate/20241125153349_add_unique_index_to_org_name.rb
@@ -0,0 +1,5 @@
+class AddUniqueIndexToOrgName < ActiveRecord::Migration[7.0]
+ def change
+ add_index :organisations, :name, unique: true
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index d31a54da2..1a1c127c6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -546,6 +546,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_12_04_100518) do
t.datetime "discarded_at"
t.datetime "schemes_deduplicated_at"
t.index ["absorbing_organisation_id"], name: "index_organisations_on_absorbing_organisation_id"
+ t.index ["name"], name: "index_organisations_on_name", unique: true
t.index ["old_visible_id"], name: "index_organisations_on_old_visible_id", unique: true
end
diff --git a/spec/fixtures/exports/general_needs_log.xml b/spec/fixtures/exports/general_needs_log.xml
index bacc7e9f0..0341dd2d4 100644
--- a/spec/fixtures/exports/general_needs_log.xml
+++ b/spec/fixtures/exports/general_needs_log.xml
@@ -147,10 +147,10 @@
{id}
{owning_org_id}
- MHCLG
+ {owning_org_name}
1234
{managing_org_id}
- MHCLG
+ {managing_org_name}
1234
2022-05-01T00:00:00+01:00
2022-05-01T00:00:00+01:00
diff --git a/spec/fixtures/exports/general_needs_log_23_24.xml b/spec/fixtures/exports/general_needs_log_23_24.xml
index 9635cd0e4..ef0c4066c 100644
--- a/spec/fixtures/exports/general_needs_log_23_24.xml
+++ b/spec/fixtures/exports/general_needs_log_23_24.xml
@@ -148,10 +148,10 @@
{id}
{owning_org_id}
- MHCLG
+ {owning_org_name}
1234
{managing_org_id}
- MHCLG
+ {managing_org_name}
1234
2023-04-03T00:00:00+01:00
2023-04-03T00:00:00+01:00
diff --git a/spec/fixtures/exports/general_needs_log_24_25.xml b/spec/fixtures/exports/general_needs_log_24_25.xml
index a665a284e..00d8bb1a5 100644
--- a/spec/fixtures/exports/general_needs_log_24_25.xml
+++ b/spec/fixtures/exports/general_needs_log_24_25.xml
@@ -161,10 +161,10 @@
la as entered
{id}
{owning_org_id}
- MHCLG
+ {owning_org_name}
1234
{managing_org_id}
- MHCLG
+ {managing_org_name}
1234
2024-04-03T00:00:00+01:00
2024-04-03T00:00:00+01:00
diff --git a/spec/fixtures/exports/organisation.xml b/spec/fixtures/exports/organisation.xml
index 8d87da16c..70c699915 100644
--- a/spec/fixtures/exports/organisation.xml
+++ b/spec/fixtures/exports/organisation.xml
@@ -2,7 +2,7 @@
diff --git a/spec/models/form/lettings/questions/managing_organisation_spec.rb b/spec/models/form/lettings/questions/managing_organisation_spec.rb
index 776a873a6..00485e80f 100644
--- a/spec/models/form/lettings/questions/managing_organisation_spec.rb
+++ b/spec/models/form/lettings/questions/managing_organisation_spec.rb
@@ -185,7 +185,7 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do
context "when organisation has merged" do
let(:absorbing_org) { create(:organisation, name: "Absorbing org", holds_own_stock: true) }
let!(:merged_org) { create(:organisation, name: "Merged org", holds_own_stock: false) }
- let!(:merged_deleted_org) { create(:organisation, name: "Merged org", holds_own_stock: false, discarded_at: Time.zone.yesterday) }
+ let!(:merged_deleted_org) { create(:organisation, name: "Merged org 2", holds_own_stock: false, discarded_at: Time.zone.yesterday) }
let(:user) { create(:user, :data_coordinator, organisation: absorbing_org) }
let(:log) do
diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb
index 9b01845ae..b117feef7 100644
--- a/spec/models/organisation_spec.rb
+++ b/spec/models/organisation_spec.rb
@@ -19,6 +19,12 @@ RSpec.describe Organisation, type: :model do
.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Provider type #{I18n.t('validations.organisation.provider_type_missing')}")
end
+ it "validates uniqueness of name" do
+ org = build(:organisation, name: organisation.name.downcase)
+ org.valid?
+ expect(org.errors[:name]).to include(I18n.t("validations.organisation.name_not_unique"))
+ end
+
context "with parent/child associations", :aggregate_failures do
let!(:child_organisation) { create(:organisation, name: "MHCLG Child") }
let!(:grandchild_organisation) { create(:organisation, name: "MHCLG Grandchild") }
diff --git a/spec/requests/organisation_relationships_controller_spec.rb b/spec/requests/organisation_relationships_controller_spec.rb
index 8e8dc4f2d..8733fba4b 100644
--- a/spec/requests/organisation_relationships_controller_spec.rb
+++ b/spec/requests/organisation_relationships_controller_spec.rb
@@ -58,7 +58,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
context "when adding a stock owner" do
let!(:active_organisation) { FactoryBot.create(:organisation, name: "Active Org", active: true) }
- let!(:inactive_organisation) { FactoryBot.create(:organisation, name: "Inactive LTD", active: false) }
+ let!(:inactive_organisation) { FactoryBot.create(:organisation, name: "Inactive LTD 2", active: false) }
before do
get "/organisations/#{organisation.id}/stock-owners/add", headers:, params: {}
@@ -115,7 +115,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
let!(:managing_agent) { FactoryBot.create(:organisation) }
let!(:other_org_managing_agent) { FactoryBot.create(:organisation, name: "Foobar LTD") }
let!(:inactive_managing_agent) { FactoryBot.create(:organisation, name: "Inactive LTD", active: false) }
- let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD") }
+ let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD 3") }
before do
FactoryBot.create(:organisation_relationship, parent_organisation: organisation, child_organisation: managing_agent)
@@ -316,7 +316,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
context "with an organisation that the user belongs to" do
let!(:stock_owner) { FactoryBot.create(:organisation) }
let!(:other_org_stock_owner) { FactoryBot.create(:organisation, name: "Foobar LTD") }
- let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD") }
+ let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD 2") }
before do
FactoryBot.create(:organisation_relationship, child_organisation: organisation, parent_organisation: stock_owner)
@@ -452,7 +452,7 @@ RSpec.describe OrganisationRelationshipsController, type: :request do
context "with an organisation that the user belongs to" do
let!(:managing_agent) { FactoryBot.create(:organisation) }
let!(:other_org_managing_agent) { FactoryBot.create(:organisation, name: "Foobar LTD") }
- let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD") }
+ let!(:other_organisation) { FactoryBot.create(:organisation, name: "Foobar LTD 5") }
before do
FactoryBot.create(:organisation_relationship, parent_organisation: organisation, child_organisation: managing_agent)
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb
index 26723a563..6c01e50bf 100644
--- a/spec/requests/organisations_controller_spec.rb
+++ b/spec/requests/organisations_controller_spec.rb
@@ -1555,7 +1555,10 @@ RSpec.describe OrganisationsController, type: :request do
let(:total_organisations_count) { Organisation.all.count }
before do
- create_list(:organisation, 25)
+ build_list(:organisation, 25) do |organisation, index|
+ organisation.name = "Organisation #{index}"
+ organisation.save!
+ end
get "/organisations"
end
@@ -1644,7 +1647,10 @@ RSpec.describe OrganisationsController, type: :request do
let(:search_param) { "MHCLG" }
before do
- create_list(:organisation, 27, name: "MHCLG")
+ build_list(:organisation, 27) do |organisation, index|
+ organisation.name = "MHCLG #{index}"
+ organisation.save!
+ end
get "/organisations?search=#{search_param}"
end
diff --git a/spec/services/exports/lettings_log_export_service_spec.rb b/spec/services/exports/lettings_log_export_service_spec.rb
index 8c123b47e..c0dde5771 100644
--- a/spec/services/exports/lettings_log_export_service_spec.rb
+++ b/spec/services/exports/lettings_log_export_service_spec.rb
@@ -21,7 +21,9 @@ RSpec.describe Exports::LettingsLogExportService do
def replace_entity_ids(lettings_log, export_template)
export_template.sub!(/\{id\}/, (lettings_log["id"] + Exports::LettingsLogExportService::LOG_ID_OFFSET).to_s)
export_template.sub!(/\{owning_org_id\}/, (lettings_log["owning_organisation_id"] + Exports::LettingsLogExportService::LOG_ID_OFFSET).to_s)
+ export_template.sub!(/\{owning_org_name\}/, lettings_log.owning_organisation.name)
export_template.sub!(/\{managing_org_id\}/, (lettings_log["managing_organisation_id"] + Exports::LettingsLogExportService::LOG_ID_OFFSET).to_s)
+ export_template.sub!(/\{managing_org_name\}/, lettings_log.managing_organisation.name)
export_template.sub!(/\{location_id\}/, (lettings_log["location_id"]).to_s) if lettings_log.needstype == 2
export_template.sub!(/\{scheme_id\}/, (lettings_log["scheme_id"]).to_s) if lettings_log.needstype == 2
export_template.sub!(/\{log_id\}/, lettings_log["id"].to_s)
diff --git a/spec/services/exports/organisation_export_service_spec.rb b/spec/services/exports/organisation_export_service_spec.rb
index 43ca19095..51c8fe8cf 100644
--- a/spec/services/exports/organisation_export_service_spec.rb
+++ b/spec/services/exports/organisation_export_service_spec.rb
@@ -16,6 +16,7 @@ RSpec.describe Exports::OrganisationExportService do
def replace_entity_ids(organisation, export_template)
export_template.sub!(/\{id\}/, organisation["id"].to_s)
+ export_template.sub!(/\{name\}/, organisation["name"])
export_template.sub!(/\{dsa_signed_at\}/, organisation.data_protection_confirmation&.signed_at.to_s)
export_template.sub!(/\{dpo_email\}/, organisation.data_protection_confirmation&.data_protection_officer_email)
end
diff --git a/spec/services/exports/user_export_service_spec.rb b/spec/services/exports/user_export_service_spec.rb
index 8a0e22267..854dd1ce7 100644
--- a/spec/services/exports/user_export_service_spec.rb
+++ b/spec/services/exports/user_export_service_spec.rb
@@ -17,6 +17,7 @@ RSpec.describe Exports::UserExportService do
def replace_entity_ids(user, export_template)
export_template.sub!(/\{id\}/, user["id"].to_s)
export_template.sub!(/\{organisation_id\}/, user["organisation_id"].to_s)
+ export_template.sub!(/\{organisation_name\}/, user.organisation.name)
export_template.sub!(/\{email\}/, user["email"].to_s)
end