From de69a693d7fd7d091db8bf9c7a84a196c7208860 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 11:43:21 -0400 Subject: [PATCH 1/3] Start: enforce unique organization names Co-Authored-By: Claude Opus 5.5 (1M context) From bcfb26d3d6f1ef303a962ebc7d0a4110c8334ecc Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 11:59:14 -0400 Subject: [PATCH 2/3] Enforce one organization per name Duplicate organizations could be saved freely, which is why the deduper keeps finding them. A unique index closes the gap for every create path (including races), and the model validation turns it into a form error. The column's case-insensitive collation means "Hope House" and "hope house " collide too. The migration refuses to run while duplicates exist so they get merged first. With names unique, the "two orgs share this name" branches in form auto-processing and the resource-org backfill can no longer happen, so they go. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/models/organization.rb | 5 +++- app/services/public_form_submission.rb | 23 ++++++----------- config/features.yml | 13 ++++++++++ ..._add_unique_name_index_to_organizations.rb | 25 +++++++++++++++++++ db/schema.rb | 1 + lib/tasks/resolve_resource_organizations.rake | 17 +++---------- spec/factories/organizations.rb | 2 +- spec/models/organization_spec.rb | 16 ++++++++++++ spec/requests/dedupable_spec.rb | 2 +- spec/requests/form_submissions_spec.rb | 5 ++-- .../duplicate_finder_spec.rb | 6 ++--- spec/services/public_form_submission_spec.rb | 24 ------------------ .../resolve_resource_organizations_spec.rb | 10 -------- .../views/organizations/edit.html.erb_spec.rb | 2 +- 14 files changed, 78 insertions(+), 73 deletions(-) create mode 100644 db/migrate/20261012154339_add_unique_name_index_to_organizations.rb diff --git a/app/models/organization.rb b/app/models/organization.rb index e5686a54e1..067cf0bf0d 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -85,7 +85,10 @@ def self.info_email validates :logo, content_type: %w[image/png image/jpeg image/webp], size: { less_than: 5.megabytes } - validates :name, presence: true, length: { maximum: 255 } + NAME_INDEX = "index_organizations_on_name".freeze + + normalizes :name, with: ->(name) { name.strip } + validates :name, presence: true, length: { maximum: 255 }, uniqueness: { case_sensitive: false } validates :email, format: { with: URI::MailTo::EMAIL_REGEXP, message: "must be a valid email address" }, allow_blank: true, length: { maximum: 255 } validates :organization_type_other, length: { maximum: 255 } validates :website_url, length: { maximum: 255 } diff --git a/app/services/public_form_submission.rb b/app/services/public_form_submission.rb index dd401292fc..a69c6fae95 100644 --- a/app/services/public_form_submission.rb +++ b/app/services/public_form_submission.rb @@ -89,16 +89,15 @@ def register_for_on_demand_training(submission) # A close-program submission end-dates the person's affiliations at the named # organization (see AffiliationServices::CloseProgram). We only auto-process - # when the submitted organization name resolves to exactly one org — an exact, - # case-insensitive name match. Anything ambiguous (no match, or two same-named - # orgs) is left for an admin to resolve on the submission's processing panel, - # which runs the same service once they link the organization by hand. + # when the submitted organization name matches an org exactly (case-insensitive). + # An unmatched name is left for an admin to resolve on the submission's processing + # panel, which runs the same service once they link the organization by hand. def process_close_program(submission) return unless @form.role == "close_program" return unless submission.person answers = submission.answers_by_identifier - organization = sole_organization_named(answers[FormField::ORGANIZATION_NAME_FIELD_IDENTIFIER]) + organization = organization_named(answers[FormField::ORGANIZATION_NAME_FIELD_IDENTIFIER]) return unless organization ended = AffiliationServices::CloseProgram.call( @@ -145,7 +144,7 @@ def capture_organization(submission) # core would have to guess at is left for an admin on the submission's linking # page. def process_agreement_organization(submission) - organization = sole_organization_named(field_value(FormField::ORGANIZATION_NAME_FIELD_IDENTIFIER)) + organization = organization_named(field_value(FormField::ORGANIZATION_NAME_FIELD_IDENTIFIER)) return unless organization result = OrganizationServices::LinkSubmittedOrganization.call( @@ -161,16 +160,8 @@ def process_agreement_organization(submission) organization end - # Only an unambiguous name match is auto-processed. Organization names aren't - # unique and the column collates case-insensitively, so two orgs can answer to - # one submitted name — and picking the wrong one here would end a facilitator's - # affiliations at the org they actually work for. Ambiguity goes to an admin. - def sole_organization_named(name) - name = name.to_s.strip - return if name.blank? - - matches = Organization.where("LOWER(name) = ?", name.downcase) - matches.first if matches.count == 1 + def organization_named(name) + Organization.find_by(name: name) if name.present? end def field_value(identifier) diff --git a/config/features.yml b/config/features.yml index f787e493ab..c9a062f985 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4580,3 +4580,16 @@ - "Someone recorded as serving families is tagged with every age range, since that's what serving families means." - "Where an old answer was typed by hand, the reading of it is marked as proposed, so staff can check the list and correct anything that reads wrong." - "The mapping is written up in ADR-0007 for anyone who needs to see the whole picture." + +- name: "Organization names are unique" + area: people + display_status: admin_facing + released_on: 2026-10-12 + pr_number: 2637 + action_path: "/organizations/dedupe_index" + summary: >- + Two organizations can no longer share the same name. Names that differ only + by capitalization or extra spaces count as the same name. + pro_tips: + - "To tell apart two real organizations with the same name, add the city or program, e.g. \"Hope House (Oakland)\"." + - "Use Dedupe on the Organizations page to combine records that already exist twice." diff --git a/db/migrate/20261012154339_add_unique_name_index_to_organizations.rb b/db/migrate/20261012154339_add_unique_name_index_to_organizations.rb new file mode 100644 index 0000000000..f3bd3e29cb --- /dev/null +++ b/db/migrate/20261012154339_add_unique_name_index_to_organizations.rb @@ -0,0 +1,25 @@ +class AddUniqueNameIndexToOrganizations < ActiveRecord::Migration[8.1] + INDEX_NAME = "index_organizations_on_name" + + def up + duplicates = select_rows(<<~SQL) + SELECT MIN(name), GROUP_CONCAT(id ORDER BY id) AS organization_ids + FROM organizations + WHERE name IS NOT NULL + GROUP BY name + HAVING COUNT(*) > 1 + SQL + + if duplicates.any? + groups = duplicates.map { |name, ids| "#{name} (organizations #{ids})" } + raise ActiveRecord::MigrationError, + "Merge these duplicate organizations at /organizations/dedupe_index before adding #{INDEX_NAME}:\n #{groups.join("\n ")}" + end + + add_index :organizations, :name, unique: true, name: INDEX_NAME + end + + def down + remove_index :organizations, name: INDEX_NAME, if_exists: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 8b3ad43ff5..bd7720db51 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1477,6 +1477,7 @@ t.integer "windows_type_id" t.index ["created_by_id"], name: "index_organizations_on_created_by_id" t.index ["location_id"], name: "index_organizations_on_location_id" + t.index ["name"], name: "index_organizations_on_name", unique: true t.index ["organization_status_id"], name: "index_organizations_on_organization_status_id" t.index ["parent_id"], name: "index_organizations_on_parent_id" t.index ["updated_by_id"], name: "index_organizations_on_updated_by_id" diff --git a/lib/tasks/resolve_resource_organizations.rake b/lib/tasks/resolve_resource_organizations.rake index 96f3c3fd77..c91e49ee76 100644 --- a/lib/tasks/resolve_resource_organizations.rake +++ b/lib/tasks/resolve_resource_organizations.rake @@ -4,9 +4,8 @@ # # For each resource that has an `agency` name but no organization linked yet: # - match an Organization by exact, case-insensitive name (agency == org.name) -# - exactly one match -> link it -# - no match -> create the Organization, link it -# - more than one match -> ambiguous; skip and report (reconcile by hand) +# - match -> link it +# - no match -> create the Organization, link it # # New orgs get status "Unknown" — not because the link cares (it doesn't), but # because Organization requires organization_status_id (org status is legacy and @@ -27,7 +26,7 @@ namespace :data do unknown_status = OrganizationStatus.find_by(name: "Unknown") raise "OrganizationStatus \"Unknown\" is required to create organizations" if !dry_run && unknown_status.nil? - linked = created = ambiguous = 0 + linked = created = 0 created_by_name = {} ActiveRecord::Base.transaction do @@ -36,14 +35,7 @@ namespace :data do next if name.blank? key = name.downcase - matches = Organization.where("LOWER(name) = ?", key).to_a - if matches.size > 1 - ambiguous += 1 - puts " ! resource ##{resource.id} #{resource.title.inspect}: #{matches.size} orgs named #{name.inspect} (ids: #{matches.map(&:id).join(', ')}) — skipped" - next - end - - if (organization = matches.first) + if (organization = Organization.find_by(name: name)) linked += 1 puts " = resource ##{resource.id}: link existing org ##{organization.id} #{name.inspect}" elsif created_by_name.key?(key) @@ -66,7 +58,6 @@ namespace :data do puts puts "linked to existing: #{linked}" puts "orgs created: #{created}" - puts "ambiguous, skipped: #{ambiguous}" puts(dry_run ? "\nDRY RUN — rolled back, nothing written. Set DRY_RUN=false to execute." : "\nDone.") end end diff --git a/spec/factories/organizations.rb b/spec/factories/organizations.rb index b122c35b7e..c016ce6bf5 100644 --- a/spec/factories/organizations.rb +++ b/spec/factories/organizations.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :organization do - name { Faker::Company.name.gsub("'", " ") } + sequence(:name) { |n| "#{Faker::Company.name.gsub("'", " ")} #{n}" } # association :windows_type # association :location association :organization_status diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index aff1b0d5f5..79ddec7398 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -19,6 +19,22 @@ subject { build(:organization) } it { should validate_presence_of(:name) } it { should_not validate_presence_of(:organization_status_id) } + it { should validate_uniqueness_of(:name).case_insensitive } + + it "strips surrounding whitespace from the name" do + expect(build(:organization, name: " Hope House ").name).to eq("Hope House") + end + + it "rejects a name that differs only by case or trailing whitespace" do + create(:organization, name: "Hope House") + expect(build(:organization, name: "hope house ")).not_to be_valid + end + + it "is enforced by a unique index when validations are skipped" do + create(:organization, name: "Hope House") + duplicate = build(:organization, name: "HOPE HOUSE") + expect { duplicate.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique, /#{Organization::NAME_INDEX}/) + end it { should allow_value("info@example.com").for(:email) } it { should allow_value("").for(:email) } diff --git a/spec/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index ff18dda1c8..b85ff7fc11 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -314,7 +314,7 @@ describe "GET dedupe_index" do it "renders and surfaces candidate groups from the duplicate finder" do create(:organization, name: "Hope Center") - create(:organization, name: "hope center") + create(:organization, name: "Hope-Center") get dedupe_index_organizations_path diff --git a/spec/requests/form_submissions_spec.rb b/spec/requests/form_submissions_spec.rb index 386650b680..c66ec6523e 100644 --- a/spec/requests/form_submissions_spec.rb +++ b/spec/requests/form_submissions_spec.rb @@ -244,12 +244,11 @@ # A matching affiliation the person holds does not mark it linked — only a # direct link made in the editor does, so it stays Pending (the queue). - create(:affiliation, person: submission.person, - organization: create(:organization, name: "Harbor Family Shelter")) + organization = create(:organization, name: "Harbor Family Shelter") + create(:affiliation, person: submission.person, organization: organization) get form_submissions_path, headers: frame_headers expect(response.body).to include("Pending") - organization = create(:organization, name: "Harbor Family Shelter") submission.link_organization!(organization.id) get form_submissions_path, headers: frame_headers # Once linked, the org shows as its own chip (linking to the org) — no diff --git a/spec/services/organization_services/duplicate_finder_spec.rb b/spec/services/organization_services/duplicate_finder_spec.rb index 9b7845d857..b429c0b23e 100644 --- a/spec/services/organization_services/duplicate_finder_spec.rb +++ b/spec/services/organization_services/duplicate_finder_spec.rb @@ -10,7 +10,7 @@ def org(attrs = {}) describe "#groups" do it "groups organizations with the same normalized name" do a = org(name: "Hope Center") - b = org(name: "hope center") + b = org(name: "Hope-Center") org(name: "Unrelated") groups = described_class.new.groups @@ -52,14 +52,14 @@ def org(attrs = {}) it "flags a FileMaker code present on only one record" do org(name: "Sunrise", filemaker_code: "FM123") - org(name: "Sunrise", filemaker_code: nil) + org(name: "Sunrise!", filemaker_code: nil) expect(described_class.new.groups.first.reasons).to include("FileMaker code on only one record") end it "surfaces multiple FileMaker codes as a keep-all conflict" do org(name: "Rivertown", filemaker_code: "FM1") - org(name: "Rivertown", filemaker_code: "FM2") + org(name: "Rivertown!", filemaker_code: "FM2") expect(described_class.new.groups.first.reasons) .to include(a_string_matching(/Multiple FileMaker codes.*merging keeps all/)) diff --git a/spec/services/public_form_submission_spec.rb b/spec/services/public_form_submission_spec.rb index 499979ed1c..013e283438 100644 --- a/spec/services/public_form_submission_spec.rb +++ b/spec/services/public_form_submission_spec.rb @@ -264,20 +264,6 @@ def org_params(overrides = {}) .to include("Facilitator") end - # Two orgs answering to one submitted name would make the scenario guess - # which one to end affiliations at, so it stays in the admin's queue. - it "leaves an ambiguous name for an admin rather than picking an org" do - form.update!(role: "new_job") - create(:organization, name: "helping hands") - - result = described_class.call(form: form, form_params: org_params) - - expect(result.person.affiliations).to be_empty - expect(result.form_submission.reload.linked_organization_ids).to be_empty - expect(Organization.where("LOWER(name) = ?", "helping hands").count).to eq(2) - expect(FormSubmission.org_link_status("pending")).to include(result.form_submission) - end - # Fill-blanks, not latest-wins: an agreement runs the admin linking core, so # a curated value is kept and reported as a conflict on the linking page. it "does not overwrite the organization's curated profile" do @@ -419,15 +405,5 @@ def close_params(org_name: "Sunset Youth Services", date: "2026-06-30", reason: expect(facilitator.reload.end_date).to be_nil expect(result.form_submission.linked_organization_ids).to be_empty end - - it "does not auto-process when two organizations share the submitted name" do - create(:organization, name: "Sunset Youth Services") - person = create(:person, user: nil, first_name: "Casey", last_name: "Closing", email: "casey@example.com") - facilitator = create(:affiliation, person: person, organization: organization, title: "Facilitator") - - described_class.call(form: form, form_params: close_params) - - expect(facilitator.reload.end_date).to be_nil - end end end diff --git a/spec/tasks/resolve_resource_organizations_spec.rb b/spec/tasks/resolve_resource_organizations_spec.rb index c4497b5335..26f21ec634 100644 --- a/spec/tasks/resolve_resource_organizations_spec.rb +++ b/spec/tasks/resolve_resource_organizations_spec.rb @@ -55,16 +55,6 @@ def suppress_output expect(r1.reload.organization).to eq(r2.reload.organization) end - it "skips a resource whose agency name is ambiguous across multiple orgs" do - create(:organization, name: "Twins") - create(:organization, name: "Twins") - resource = create(:resource, agency: "Twins") - - run_task - - expect(resource.reload.organization_id).to be_nil - end - it "leaves an already-linked resource untouched" do existing = create(:organization, name: "Existing Link") other = create(:organization, name: "Someplace Else") diff --git a/spec/views/organizations/edit.html.erb_spec.rb b/spec/views/organizations/edit.html.erb_spec.rb index 37fe0aff68..2d875787ec 100644 --- a/spec/views/organizations/edit.html.erb_spec.rb +++ b/spec/views/organizations/edit.html.erb_spec.rb @@ -42,7 +42,7 @@ def org_with_status(name) Organization.create!( windows_type: windows_type, organization_status: OrganizationStatus.find_or_create_by!(name: name), - name: "MyString", + name: "Status org", description: "MyString", notes: "MyText" ) From 88ecf767ea4ba653c2c43f120527da1d9400afc3 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Tue, 13 Oct 2026 19:03:13 -0400 Subject: [PATCH 3/3] Inline the org name index in its spec Nothing in the app rescues the duplicate-name error, so the constant only existed for the spec. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/models/organization.rb | 2 -- spec/models/organization_spec.rb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/organization.rb b/app/models/organization.rb index 067cf0bf0d..bdffe0a4d0 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -85,8 +85,6 @@ def self.info_email validates :logo, content_type: %w[image/png image/jpeg image/webp], size: { less_than: 5.megabytes } - NAME_INDEX = "index_organizations_on_name".freeze - normalizes :name, with: ->(name) { name.strip } validates :name, presence: true, length: { maximum: 255 }, uniqueness: { case_sensitive: false } validates :email, format: { with: URI::MailTo::EMAIL_REGEXP, message: "must be a valid email address" }, allow_blank: true, length: { maximum: 255 } diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 79ddec7398..37279ca2c3 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -33,7 +33,7 @@ it "is enforced by a unique index when validations are skipped" do create(:organization, name: "Hope House") duplicate = build(:organization, name: "HOPE HOUSE") - expect { duplicate.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique, /#{Organization::NAME_INDEX}/) + expect { duplicate.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique, /index_organizations_on_name/) end it { should allow_value("info@example.com").for(:email) }