From f341c35670ec0150e24af71270b51b7ba72f1b5d Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 14:43:34 -0400 Subject: [PATCH] Read a FileMaker code column as the list of codes it can hold Organization already treated the column as a comma-separated set and Person didn't, so one concern now owns the reading and combining for both. A caller can't compare the whole column against a single code any more, which is what these three were doing: - Merging two people kept one code and dropped the other. Both are carried onto the keeper now, the way an org merge already does it. - The person duplicate finder clustered on the whole column, so "00006" and "00006, 00012" read as different people with conflicting codes rather than one shared code. - The five archive links assumed a single value, so a person or org carrying two codes silently lost its link to the archive. One link per code, each opening in its own tab so the record page keeps its place. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 3 ++- app/controllers/people_controller.rb | 2 ++ app/helpers/filemaker_helper.rb | 18 +++++++++++++++ app/models/concerns/filemaker_coded.rb | 23 +++++++++++++++++++ app/models/organization.rb | 18 +-------------- app/models/person.rb | 1 + .../person_services/duplicate_finder.rb | 9 +++++--- app/views/organizations/_form.html.erb | 7 +++--- app/views/organizations/edit.html.erb | 5 ++-- app/views/people/_admin_details.html.erb | 9 ++------ app/views/people/_form.html.erb | 7 +++--- app/views/people/edit.html.erb | 5 ++-- config/features.yml | 13 +++++++++++ spec/models/person_spec.rb | 20 ++++++++++++++++ spec/requests/dedupable_spec.rb | 12 ++++++++++ .../person_services/duplicate_finder_spec.rb | 10 ++++++++ 16 files changed, 122 insertions(+), 40 deletions(-) create mode 100644 app/helpers/filemaker_helper.rb create mode 100644 app/models/concerns/filemaker_coded.rb diff --git a/AGENTS.md b/AGENTS.md index 401843082a..fa22fbe5ab 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,7 +62,7 @@ This codebase (Rails 8.1) | `app/decorators/` | Draper decorators for view logic | ~50 files | | `app/policies/` | ActionPolicy authorization rules | ~63 files | | `app/presenters/` | Presentation objects | 6 files | -| `app/helpers/` | View helpers | ~47 files | +| `app/helpers/` | View helpers | ~48 files | | `app/mailers/` | ActionMailer classes | 6 files | | `app/inputs/` | Custom SimpleForm inputs | 1 file | @@ -146,6 +146,7 @@ This codebase (Rails 8.1) | `Communicable` | Gives a record the `notifications` association, nested attributes, a default `communications_scope`, and a `before_validation` addressing a hand-logged communication to the record's `communications_email`. Included by every model whose form renders `shared/_comments_and_communications` | | `AuthorCreditable` | Author attribution. Credits are formatted by the credited **person's profile** (`Person#display_name_preference`), not by the record. The record's `author_credit_preference` is the consent snapshot taken at create time and is human-editable only on the author credit divergences page. Anonymity is a one-way latch: the profile or the record can set it, neither can strip it from the other. `credits_to_org` flips the unattributed label to "AWBW Staff"; `credits_creator` (idea models, workshop logs) credits the submitting account's person when no author is named | | `Featureable` | `featured`, `publicly_featured` scopes | +| `FilemakerCoded` | Reads `filemaker_code` as the trimmed, comma-separated **list** it is (Person, Organization): `#filemaker_codes` for the set a record answers to, `.join_filemaker_codes` to combine any mix of strings/lists into one normalized column value (both dedupers union the codes on merge). Nothing should compare the whole column against a single code — the `filemaker_code_links` helper renders one archive link per code | | `Mentioner` | ActionText @mention extraction and grouping | | `NameFilterable` | Name-based filtering | | `PayChargeExtensions` | Included into `Pay::Charge` (via `config/initializers/pay.rb`): turns a Stripe charge into an AWBW `Payment` + `Allocation`, resolving the membership invoice it pays | diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 6f3bd41b24..2dcee25c9a 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -377,6 +377,7 @@ def dedupe_config first_name legal_first_name last_name email email_type email_2 email_2_type date_of_birth pronouns filemaker_code member_since notes ], + union_columns: %w[filemaker_code], # Merging two people who each have a login leaves the kept person with both # (users.person_id has no unique constraint); both logins still sign in to the # one record. Surface it so the admin isn't surprised — it doesn't block. @@ -397,6 +398,7 @@ def dedupe_config # they become indistinguishable), capture which primary sector + age range to # keep: the survivor's own, or — when it has none — the deleted person's. merge_keeper: ->(keep, delete) { + keep.filemaker_code = Person.join_filemaker_codes(keep.filemaker_code, delete.filemaker_code) @keeper_primary_sector_id = keep.sectorable_items.find_by(is_primary: true)&.sector_id || delete.sectorable_items.find_by(is_primary: true)&.sector_id @keeper_primary_age_category_id = keep.age_range_categorizable_items.find_by(is_primary: true)&.category_id || diff --git a/app/helpers/filemaker_helper.rb b/app/helpers/filemaker_helper.rb new file mode 100644 index 0000000000..e5fff83072 --- /dev/null +++ b/app/helpers/filemaker_helper.rb @@ -0,0 +1,18 @@ +module FilemakerHelper + # One archive link per FileMaker code a record carries, since a record can answer to + # several (see FilemakerCoded). A code the archive has no row for renders as plain + # text. `label` prefixes each link, for a button or hint that has to read as an action. + def filemaker_code_links(record, archive:, table:, link_class: nil, label: nil) + codes = record.filemaker_codes + return if codes.empty? + + stored = archive.where(fm_id: codes).pluck(:fm_id).to_set + + safe_join(codes.map do |code| + text = [ label, code ].compact.join(" ") + next tag.span(text) unless stored.include?(code) + + link_to text, fm_archive_path(code, table: table), class: link_class, target: "_blank", rel: "noopener" + end, " ") + end +end diff --git a/app/models/concerns/filemaker_coded.rb b/app/models/concerns/filemaker_coded.rb new file mode 100644 index 0000000000..ce5bb971ad --- /dev/null +++ b/app/models/concerns/filemaker_coded.rb @@ -0,0 +1,23 @@ +# FileMaker is the source of truth, and one record can answer to more than one +# FileMaker code — two records that each mapped to a FileMaker row get merged, or the +# archive itself holds several rows for one person. The `filemaker_code` column holds +# them as a trimmed, comma-separated list; this is the seam for reading and combining +# them, so nothing compares the whole column against a single code. +module FilemakerCoded + extend ActiveSupport::Concern + + def filemaker_codes + self.class.split_filemaker_codes(filemaker_code) + end + + class_methods do + def split_filemaker_codes(value) + value.to_s.split(",").map(&:strip).reject(&:blank?).uniq + end + + # A single normalized column value from any mix of code strings/lists. + def join_filemaker_codes(*values) + values.flatten.flat_map { |value| split_filemaker_codes(value) }.uniq.sort.join(", ").presence + end + end +end diff --git a/app/models/organization.rb b/app/models/organization.rb index c7b15c36a6..91af0f0cd4 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -1,6 +1,7 @@ class Organization < ApplicationRecord include RemoteSearchable, TagFilterable, Trendable, WindowsTypeFilterable, SectorsTaggable, AgeGroupTaggable # Publishable include Communicable + include FilemakerCoded belongs_to :organization_status belongs_to :created_by, class_name: "User", optional: true belongs_to :updated_by, class_name: "User", optional: true @@ -354,23 +355,6 @@ def rollup_cache_version remote_searchable_by :name - # FileMaker is the source of truth, and a record can carry more than one - # FileMaker code (e.g. when two orgs that each mapped to a FileMaker record are - # merged). The `filemaker_code` column holds them as a trimmed, comma-separated - # list; these are the seam for reading and combining them. - def filemaker_codes - Organization.split_filemaker_codes(filemaker_code) - end - - def self.split_filemaker_codes(value) - value.to_s.split(",").map(&:strip).reject(&:blank?).uniq - end - - # A single normalized column value from any mix of code strings/lists. - def self.join_filemaker_codes(*values) - values.flatten.flat_map { |value| split_filemaker_codes(value) }.uniq.sort.join(", ").presence - end - # Returns the website as a clickable, scheme-qualified URL — prepending # https:// to a bare domain like "awbw.org" — or nil when the value is blank or # not a usable web address. Drives the external links on the org profile and diff --git a/app/models/person.rb b/app/models/person.rb index dc95d257f3..cef678c39f 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -1,6 +1,7 @@ class Person < ApplicationRecord include RemoteSearchable, TagFilterable, Trendable, WindowsTypeFilterable, SectorsTaggable, AgeGroupTaggable, StaffTaggable include Communicable + include FilemakerCoded pay_customer default_payment_processor: :stripe diff --git a/app/services/person_services/duplicate_finder.rb b/app/services/person_services/duplicate_finder.rb index 21423c962c..792435c08a 100644 --- a/app/services/person_services/duplicate_finder.rb +++ b/app/services/person_services/duplicate_finder.rb @@ -60,8 +60,12 @@ def reasons_for(records) end def filemaker_reasons(records) - codes = records.filter_map { |person| person.filemaker_code.presence }.uniq.sort + code_sets = records.map(&:filemaker_codes) + codes = code_sets.flatten.uniq.sort return [] if codes.empty? + + shared = codes.select { |code| code_sets.count { |set| set.include?(code) } > 1 } + return [ "Same FileMaker code (#{shared.join(", ")})" ] if shared.any? return [ "⚠ Different FileMaker codes (#{codes.join(", ")}) — verify these are the same person" ] if codes.size > 1 [ "Same FileMaker code (#{codes.first})" ] @@ -91,8 +95,7 @@ def email_keys(person) end def filemaker_key(person) - code = person.filemaker_code.to_s.strip.downcase - "fm|#{code}" if code.present? + person.filemaker_codes.map { |code| "fm|#{code.downcase}" } end def dob_key(person) diff --git a/app/views/organizations/_form.html.erb b/app/views/organizations/_form.html.erb index 217f56f7ae..7d9c15319b 100644 --- a/app/views/organizations/_form.html.erb +++ b/app/views/organizations/_form.html.erb @@ -183,11 +183,10 @@ input_html: { class: "rounded-md border-gray-300 bg-blue-100 shadow-sm focus:ring-blue-500 focus:border-blue-500" } %> - <%- if params[:admin] == "true" && f.object.filemaker_code.present? && FmProject.exists?(fm_id: f.object.filemaker_code) %> + <%- if params[:admin] == "true" %>

- <%= link_to "View filemaker archive", - fm_archive_path(f.object.filemaker_code, table: "projects"), - class: "text-blue-600 hover:text-blue-800 underline" %> + <%= filemaker_code_links(f.object, archive: FmProject, table: "projects", label: "View archive", + link_class: "text-blue-600 hover:text-blue-800 underline") %>

<% end %> <% high_profile_gem = tag.i(class: "fa-solid fa-gem text-purple-600", aria: { hidden: true }) %> diff --git a/app/views/organizations/edit.html.erb b/app/views/organizations/edit.html.erb index ec7d1d7e06..31a36d4c4b 100644 --- a/app/views/organizations/edit.html.erb +++ b/app/views/organizations/edit.html.erb @@ -9,8 +9,9 @@ <% end %> <% end %> <% actions = capture do %> - <% if params[:admin] == "true" && @organization.filemaker_code.present? && FmProject.exists?(fm_id: @organization.filemaker_code) %> - <%= link_to "FM Project", fm_archive_path(@organization.filemaker_code, table: "projects"), class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1" %> + <% if params[:admin] == "true" %> + <%= filemaker_code_links(@organization, archive: FmProject, table: "projects", label: "FM project", + link_class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1") %> <% end %> <%= link_to "Home", root_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "Organizations", organizations_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> diff --git a/app/views/people/_admin_details.html.erb b/app/views/people/_admin_details.html.erb index c6a8c969db..959aebb3c8 100644 --- a/app/views/people/_admin_details.html.erb +++ b/app/views/people/_admin_details.html.erb @@ -98,13 +98,8 @@
FileMaker code
- <% if person.filemaker_code.present? %> - <% if FmRolodex.exists?(fm_id: person.filemaker_code) %> - <%= link_to person.filemaker_code, fm_archive_path(person.filemaker_code, table: "rolodexes"), class: "text-blue-600 underline hover:text-blue-800", target: "_blank", rel: "noopener" %> - <% else %> - <%= person.filemaker_code %> - <% end %> - <% else %>—<% end %> + <%= filemaker_code_links(person, archive: FmRolodex, table: "rolodexes", + link_class: "text-blue-600 underline hover:text-blue-800") || "—" %>
Racial/ethnic identity
<%= person.racial_ethnic_identity.presence || "—" %>
diff --git a/app/views/people/_form.html.erb b/app/views/people/_form.html.erb index 1c86fe17f0..a2f373c923 100644 --- a/app/views/people/_form.html.erb +++ b/app/views/people/_form.html.erb @@ -469,11 +469,10 @@ label: "FileMaker code", input_html: { class: "w-full" }, wrapper_html: { class: "w-full" } %> - <%- if params[:admin] == "true" && f.object.filemaker_code.present? && FmRolodex.exists?(fm_id: f.object.filemaker_code) %> + <%- if params[:admin] == "true" %>

- <%= link_to "View filemaker archive", - fm_archive_path(f.object.filemaker_code, table: "rolodexes"), - class: "text-blue-600 hover:text-blue-800 underline" %> + <%= filemaker_code_links(f.object, archive: FmRolodex, table: "rolodexes", label: "View archive", + link_class: "text-blue-600 hover:text-blue-800 underline") %>

<% end %> diff --git a/app/views/people/edit.html.erb b/app/views/people/edit.html.erb index 2ce6d585fc..0120c27367 100644 --- a/app/views/people/edit.html.erb +++ b/app/views/people/edit.html.erb @@ -35,8 +35,9 @@ <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> <%= link_to "Activity", admin_activities_events_path(person_id: @person.id, time_period: "all_time", audience: %w[visitors users staff], hide_account: "1", hide_interactions: "1"), class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1" %> <% end %> - <% if params[:admin] == "true" && @person.filemaker_code.present? && FmRolodex.exists?(fm_id: @person.filemaker_code) %> - <%= link_to "Rolodex", fm_archive_path(@person.filemaker_code, table: "rolodexes"), class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1" %> + <% if params[:admin] == "true" %> + <%= filemaker_code_links(@person, archive: FmRolodex, table: "rolodexes", label: "Rolodex", + link_class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1") %> <% end %> <%= link_to "Change password", change_password_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "Home", root_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> diff --git a/config/features.yml b/config/features.yml index 2575bb4e5a..6b99f3d9c2 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4163,3 +4163,16 @@ details on ones nobody has touched. pro_tips: - "After syncing, the message tells you how many edited features were kept as-is." + +- name: "Merging people keeps both FileMaker codes" + area: people + display_status: admin_facing + released_on: 2026-10-12 + pr_number: 2647 + action_path: "/people/dedupe_index" + summary: >- + Merging two people who each came from FileMaker now keeps both of their + FileMaker codes on the kept person instead of dropping one. + pro_tips: + - "A record can answer to several codes — they're shown as a comma-separated list, and each one links to its own row in the archive." + - "The duplicate list now reads two records as a match when their codes overlap, rather than flagging them as conflicting." diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 4bf10d69c3..fe2d83f899 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -979,6 +979,26 @@ def answer(identifier, value) end end + describe "FileMaker codes" do + it "parses a trimmed, de-duplicated list from the column" do + person = build(:person, filemaker_code: " 00006 , 00012,00006 ") + + expect(person.filemaker_codes).to eq(%w[00006 00012]) + end + + it "is empty when the column is blank" do + expect(build(:person, filemaker_code: nil).filemaker_codes).to eq([]) + end + + it "combines mixed strings and lists into one sorted normalized value" do + expect(Person.join_filemaker_codes("00012, 00006", " 00031 ", nil, "00006")).to eq("00006, 00012, 00031") + end + + it "returns nil when there is nothing to combine" do + expect(Person.join_filemaker_codes(nil, "")).to be_nil + end + end + describe ".find_by_any_email" do it "matches on email, email_2, or the login email, case-insensitively" do by_primary = create(:person, user: nil, email: "primary@example.com", email_2: nil) diff --git a/spec/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index ef6ab0e682..e6f48cdb5a 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -489,6 +489,18 @@ expect(keep.reload.notes).to eq("Canonical record") end + it "combines both people's FileMaker codes onto the keeper" do + keep.update!(filemaker_code: "00006") + delete_rec.update!(filemaker_code: "00012") + + post dedupe_perform_people_path, params: { + person_to_delete_id: delete_rec.id, + person_to_keep_id: keep.id + } + + expect(keep.reload.filemaker_code).to eq("00006, 00012") + end + # invoicee is polymorphic, so it carries no DB foreign key for the schema scan to # find — without Person's `has_many :invoices, as: :invoicee` the safeguard sees an # unhandled reference and refuses the merge outright. diff --git a/spec/services/person_services/duplicate_finder_spec.rb b/spec/services/person_services/duplicate_finder_spec.rb index 76bf4ee31d..7da051e61a 100644 --- a/spec/services/person_services/duplicate_finder_spec.rb +++ b/spec/services/person_services/duplicate_finder_spec.rb @@ -64,6 +64,16 @@ def person(attrs = {}) expect(group.reasons).to include("Same FileMaker code (FM777)") end + it "groups people whose FileMaker code lists overlap" do + a = person(first_name: "Erin", last_name: "Three", email: "e3@example.com", filemaker_code: "FM777") + b = person(first_name: "Erin", last_name: "Four", email: "e4@example.com", filemaker_code: "FM888, FM777") + + group = described_class.new.groups.first + + expect(group.records).to contain_exactly(a, b) + expect(group.reasons).to include("Same FileMaker code (FM777)") + end + it "flags different FileMaker codes on records grouped by another signal as a caution" do person(first_name: "Fay", last_name: "Nguyen", email: "fay@example.com", filemaker_code: "FM1") person(first_name: "Fay", last_name: "Nguyen", email: "fay2@example.com", filemaker_code: "FM2")