diff --git a/AGENTS.md b/AGENTS.md index 6104a1dc04..6baf152ff6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -49,7 +49,7 @@ This codebase (Rails 8.1) | Directory | Purpose | Count | |---|---|---| | `app/models/` | ActiveRecord models | ~90 files | -| `app/services/` | Service objects and POROs (e.g. `MoneyFormatter` for currency display, `StoryImporter` for the stories spreadsheet import) | ~72 files | +| `app/services/` | Service objects and POROs (e.g. `MoneyFormatter` for currency display, `StoryImporter` for the stories spreadsheet import) | ~73 files | | `app/jobs/` | SolidQueue background jobs | 6 files | | `app/models/concerns/` | Shared model modules | 23 concerns | @@ -248,7 +248,7 @@ action, or `authorize! :workshop, to: :summary?`). - `PersonInviter` — Invites a single person to the portal: creates a user account if they have none, then sends the welcome/invite email attributed to the sender. Backs the bulk "Send Portal invite emails" flow on the reminders page (`?mode=invite`) - `FormBuilderService` — Builds configurable forms from composable sections with per-field visibility - `FormResponseAggregator` — Rolls up every submission to one form into a per-question report for the `/forms/:id/results` page (`FormsController#results`): selectable questions become chartable `[ label, count ]` tallies (sector/age-group ids resolved to names, `Other: ` write-ins split into their own list), geographic fields (`mailing_state`/`organization_state`/`ce_license_issuing_state` → US map, `mailing_country`/`organization_country` → world map) become choropleths, free-text questions become newest-first answer lists, file questions a count. Returns `FieldReport` structs in form order; rendered via the shared `events/_breakdown_card` partial (pie/bar/map/world_map) plus `forms/_text_responses_card` -- `ModelDeduper` — Deduplication logic. On a natural-key collision it now merges the losing row into the survivor (recursively, via itself) so the loser's own children move first — instead of destroying the loser and cascading them away or aborting on a protective `before_destroy` (e.g. a professional license with CE registrations). It finds reassignable children two ways: the model's own `has_many` declarations, and a scan of the DB's foreign keys. **A polymorphic child carries no foreign key**, so it is reachable only through an explicit `has_many … as:` — without one, `#unhandled_references` sees it and the merge is refused outright (this is why `Person`/`Organization` declare `has_many :invoices, as: :invoicee`) +- `ModelDeduper` — Deduplication logic. On a natural-key collision it now merges the losing row into the survivor (recursively, via itself) so the loser's own children move first — instead of destroying the loser and cascading them away or aborting on a protective `before_destroy` (e.g. a professional license with CE registrations). It finds reassignable children two ways: the model's own `has_many` declarations, and a scan of the DB's foreign keys. **A polymorphic child carries no foreign key**, so it is reachable only through an explicit `has_many … as:` — without one, `#unhandled_references` sees it and the merge is refused outright (this is why `Person`/`Organization` declare `has_many :invoices, as: :invoicee`). A config can also pass `reference_columns:` — column names that point at the model in any table without a DB foreign key (the users deduper names `user_id`/`created_by_id`/`updated_by_id`/`welcome_instructions_sent_by_id`). Self-references on the model's own table follow the keeper, except the keeper's own reference to the duplicate, which clears - `ContinuingEducationDeduper` — Collapses duplicate CE registrations left sharing one event registration + license after a merge, moving the losers' allocations (payments) and comments onto the survivor. Keeps the real cost (larger of the two) rather than inventing one to cover the combined payments — if they now exceed it, `ContinuingEducationRegistration#over_allocated?` flags it (`ContinuingEducationRegistrationDecorator#overpayment_badge` → "Check payments") for an admin. CE has no DB unique index, so `ModelDeduper` never compares CE rows and just moves them all onto the keeper; a unique index is deliberately avoided because it would force the engine to move CE via validated `update!`, which trips `ContinuingEducationRegistration#license_belongs_to_registrant` mid-merge. Run from the people deduper's `after_merge` - `AddressDeduper` — After a person or organization merge, folds duplicate addresses the generic merge moves onto the survivor wholesale (addresses have no unique index, so nothing collapses them). Groups the survivor's addresses by a normalized identity signature (`street_address`, `city`, `state`, `zip_code`, `country`, `county`, `district`, `address_type`, `locality`) and folds each duplicate into its match via `ModelDeduper#merge`, which repoints the folded address's inbound FKs — `contact_methods.address_id`, `invoices.bill_to_address_id`, `affiliations.organization_address_id` — onto the survivor. Within a group the survivor is the copy still in use — active before inactive, then the primary, then the oldest — since `Address.active` drives bill-to addresses, an org's program location and the geographic breakdowns; columns outside the identity (`CARRIED_COLUMNS` — `phone` and the three `la_*` districts) are copied up from the folded duplicate when the survivor has none. Then settles a single primary matching the signature captured in the deduper's `merge_keeper` (the survivor's own pre-merge primary address, else the deleted record's), demoting the rest. Runs in the people and organizations dedupers' `after_merge` - `PersonServices::ReconcilePrimaryDesignations` — After a person merge, keeps the survivor's own primary sector + primary age range and demotes the duplicate's, which would otherwise leave the kept person with two primaries and trip Person's single-primary validations. The survivor's pre-merge primary ids are captured in the people deduper's `merge_keeper` (they're indistinguishable once merged); reconciliation runs in `after_merge` @@ -338,6 +338,7 @@ Both flows record on `EventRegistrationOrganization` what the submission meant f - `UserServices::ProcessEmailChange` — Email change processing - `UserServices::ProcessEmailManualConfirm` — Manual email confirmation +- `UserServices::DuplicateFinder` — Candidate groups for the users (logins) deduper: logins sharing one person (e.g. left by a person merge); a login that can sign in leads, most recently updated first, so the deduper suggests the login the person uses. The deduper's pickers query `SearchController`'s virtual `user_any_status` model (every login via `User.activity_search`, labelled with `UserDecorator#access_statuses`) ## Decorators (Draper) diff --git a/app/controllers/concerns/dedupable.rb b/app/controllers/concerns/dedupable.rb index c11a8a5807..527c613e1b 100644 --- a/app/controllers/concerns/dedupable.rb +++ b/app/controllers/concerns/dedupable.rb @@ -38,7 +38,8 @@ def dedupe_preview return redirect_to url_for(action: :dedupe_index), alert: "#{mc.model_name.human} not found (ID: #{missing.join(', ')})." end - deduper = ModelDeduper.new(model_class: mc, movable_attachments: config[:movable_attachments]) + deduper = ModelDeduper.new(model_class: mc, movable_attachments: config[:movable_attachments], + reference_columns: config[:reference_columns]) @reassignment_delete = deduper.reassignment_preview(@record_to_delete) @reassignment_keep = deduper.reassignment_preview(@record_to_keep) @lost_references = deduper.lost_references(@record_to_delete) @@ -60,10 +61,7 @@ def dedupe_update_keep record = mc.find(params[:id]) keep_param_key = "#{mn}_to_keep" - if params[keep_param_key].present? - editable = mc.column_names - %w[id created_at updated_at legacy_id] - record.update!(params.require(keep_param_key).permit(editable)) - end + record.update!(params.require(keep_param_key).permit(dedupe_editable_columns(config))) if params[keep_param_key].present? head :ok rescue ActionPolicy::Unauthorized @@ -81,7 +79,8 @@ def dedupe_perform record_to_delete = mc.find(params["#{mn}_to_delete_id"]) record_to_keep = mc.find(params["#{mn}_to_keep_id"]) - unhandled = ModelDeduper.new(model_class: mc).unhandled_references(record_to_delete) + unhandled = ModelDeduper.new(model_class: mc, reference_columns: config[:reference_columns]) + .unhandled_references(record_to_delete) if unhandled.any? tables = unhandled.map { |ref| ref[:table] }.uniq.join(", ") return redirect_to url_for(action: :dedupe_index), @@ -95,8 +94,7 @@ def dedupe_perform keep_param_key = "#{mn}_to_keep" if params[keep_param_key].present? - editable = mc.column_names - %w[id created_at updated_at legacy_id] - record_to_keep.assign_attributes(params.require(keep_param_key).permit(editable)) + record_to_keep.assign_attributes(params.require(keep_param_key).permit(dedupe_editable_columns(config))) end # Combine values that must survive the merge rather than be replaced wholesale @@ -104,14 +102,15 @@ def dedupe_perform config[:merge_keeper]&.call(record_to_keep, record_to_delete) deduper = ModelDeduper.new(model_class: mc, logger: Rails.logger, dry_run: false, min_usage: 0, - movable_attachments: config[:movable_attachments]) + movable_attachments: config[:movable_attachments], + reference_columns: config[:reference_columns]) if respond_to?(:track_event, true) track_event("dedupe.#{mn}", { resource_type: mc.name, resource_id: record_to_keep.id, - deleted_record: record_to_delete.attributes, - kept_record: { id: record_to_keep.id, name: record_to_keep.name }, + deleted_record: dedupe_parameter_filter.filter(record_to_delete.attributes), + kept_record: { id: record_to_keep.id, name: dedupe_record_label(config, record_to_keep) }, associations_moved: deduper.reassignment_counts(record_to_delete).values.sum }) end @@ -130,7 +129,7 @@ def dedupe_perform label = mc.model_name.human.pluralize redirect_to url_for(action: :index), - notice: "#{label} merged successfully. '#{record_to_delete.name}' was merged into '#{record_to_keep.name}'." + notice: "#{label} merged successfully. '#{dedupe_record_label(config, record_to_delete)}' was merged into '#{dedupe_record_label(config, record_to_keep)}'." rescue ActionPolicy::Unauthorized raise rescue StandardError => e @@ -140,6 +139,20 @@ def dedupe_perform private + # The preview only renders editable_columns when a config lists them, so only those are accepted. + def dedupe_editable_columns(config) + config[:editable_columns].presence || (config[:model_class].column_names - %w[id created_at updated_at legacy_id]) + end + + def dedupe_record_label(config, record) + (config[:record_label] || :name.to_proc).call(record) + end + + # Keeps credentials (a login's password hash and tokens) out of the tracked deleted record. + def dedupe_parameter_filter + ActiveSupport::ParameterFilter.new(%i[passw crypt salt token secret otp]) + end + # Subclasses must implement this, returning a hash with: # model_class: The ActiveRecord model (e.g. Category) # domain: Symbol for DomainTheme (e.g. :categories) (optional, derived from model) @@ -150,6 +163,16 @@ def dedupe_perform # field_notes: Hash of { column_name => hint } rendered under the keeper's field, to guide # an admin on what to enter (e.g. full_name is a legacy free-text author) (optional) # deprecated_columns: Array of column names to render muted with a "Deprecated" badge (optional) + # editable_columns: Array of column names the preview shows and lets an admin edit on the keeper; + # only these are accepted on save (optional, defaults to every column) + # read_only_columns: Array of column names shown side by side for comparison but not editable — + # e.g. a login's email and sign-in history (optional) + # record_label: Lambda(record) naming a record across the deduper when #name doesn't tell two + # apart, e.g. two logins of one person (optional, defaults to #name) + # search_model: Search model the index pickers query (optional, defaults to the model's own + # remote search), e.g. "user_any_status" to reach logins that can't sign in + # reference_columns: Array of column names that reference this model in any table but carry no DB + # foreign key, so the merge moves them too (e.g. created_by_id stamps) (optional) # movable_attachments: Array of has_one_attached names to move to the keeper when it has none # (otherwise dropped with the deleted record), e.g. %w[thumbnail header] (optional) # preview_images: Lambda(record) returning that record's attached images (ActiveStorage @@ -198,8 +221,10 @@ def build_dedupe_vars(config) delete_id_param: "#{mn}_to_delete_id", keep_id_param: "#{mn}_to_keep_id", keep_param_key: "#{mn}_to_keep".to_sym, - search_model: mn, + search_model: config[:search_model] || mn, editable_columns: config[:editable_columns], + read_only_columns: Array(config[:read_only_columns]).map(&:to_s), + record_label: config[:record_label] || :name.to_proc, union_columns: Array(config[:union_columns]).map(&:to_s), belongs_to_options: opts.is_a?(Proc) ? opts.call : (opts || {}), remote_select_options: config[:remote_select_options] || {}, diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 48fdd627cc..c0807363e9 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -4,9 +4,12 @@ class SearchController < ApplicationController # Virtual "model" that searches people and organizations together for the # compound payer / funder pickers, listing organizations first. COMPOUND_MODEL = "person_or_organization".freeze + # Every login, whatever its access, labelled with why it can't sign in — for the users deduper. + USER_ANY_STATUS_MODEL = "user_any_status".freeze def index return render json: compound_results if params[:model] == COMPOUND_MODEL + return render json: user_any_status_results if params[:model] == USER_ANY_STATUS_MODEL model_class = allowed_model(params[:model]) unless model_class @@ -54,6 +57,17 @@ def compound_results records.map(&:compound_search_label) end + def user_any_status_results + authorize! User, to: :search? + + query = params[:q].to_s.strip + return [] if query.blank? + + records = authorized_scope(User.activity_search(query)) + records = records.where.not(id: params[:exclude].split(",").map(&:to_i)) if params[:exclude].present? + records.includes(:person).limit(25).map { |user| user.decorate.search_label_with_status } + end + def allowed_model(model_param) { "person" => Person, diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 967ebd5fc3..1698f22747 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,4 +1,6 @@ class UsersController < ApplicationController + include AhoyTracking, Dedupable + before_action :set_user, only: [ :show, :edit, :update, :destroy, :toggle_lock_status, :confirm_email, :send_welcome_instructions, :send_reset_password_instructions, @@ -321,6 +323,46 @@ def flow_diagram private + def dedupe_config + { + model_class: User, + domain: :users, + candidate_finder: -> { UserServices::DuplicateFinder.new.groups }, + record_label: ->(user) { user.full_name_with_email }, + search_model: SearchController::USER_ANY_STATUS_MODEL, + read_only_columns: %w[email super_user confirmed_at last_sign_in_at sign_in_count inactive locked_at], + editable_columns: %w[time_zone notes], + # Audit stamps and analytics carry no DB foreign key to users, so name them for the merge to move. + reference_columns: %w[user_id created_by_id updated_by_id welcome_instructions_sent_by_id], + record_extras: ->(user) { + last_sign_in = user.last_sign_in_at ? "last signed in #{user.last_sign_in_at.to_date}" : "never signed in" + [ user.super_user? && "Admin", *user.decorate.access_statuses, last_sign_in ].compact_blank.join(" · ") + }, + merge_notes: ->(keep, delete) { + notes = [ "Signing in as #{delete.email} stops working after the merge — #{keep.email} stays the login. Swap to keep the other address." ] + notes << "#{delete.email} is an admin login and #{keep.email} isn't, so admin access ends with the merge." if delete.super_user? && !keep.super_user? + notes << "The kept login has no person, so it takes over #{delete.person.full_name}'s profile." if delete.person && !keep.person + notes + }, + merge_blocks: ->(keep, delete) { + blocks = [] + blocks << "You're signed in as #{delete.email}. Swap to keep it — you can't merge away your own login." if delete == current_user + if keep.person && delete.person && keep.person_id != delete.person_id + blocks << "These logins belong to different people (#{keep.person.full_name} and #{delete.person.full_name}). Merge the people first, then their logins." + end + blocks + }, + merge_keeper: ->(keep, delete) { + keep.person_id ||= delete.person_id + @merged_login_id = delete.id + }, + # paper_trail records the acting login as a string id, outside the foreign-key scan. + after_merge: ->(keep) { + PaperTrail::Version.where(whodunnit: @merged_login_id.to_s).update_all(whodunnit: keep.id.to_s) + } + } + end + def set_user @user = User.find(params[:id]) end diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index d4a5e8330d..bd85714167 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -29,6 +29,15 @@ def full_name end end + # Why this login can't sign in — empty when it can. + def access_statuses + [ ("Inactive" if inactive?), ("Locked" if locked_at.present?), ("Unconfirmed" if confirmed_at.nil?) ].compact + end + + def search_label_with_status + { id: id, label: [ full_name_with_email, access_statuses.join(", ").presence ].compact.join(" · ") } + end + def last_logged_in return "never" unless last_sign_in_at "#{h.time_ago_in_words(last_sign_in_at)} ago" diff --git a/app/helpers/admin_cards_helper.rb b/app/helpers/admin_cards_helper.rb index 6d670971cf..d14469fe2e 100644 --- a/app/helpers/admin_cards_helper.rb +++ b/app/helpers/admin_cards_helper.rb @@ -71,7 +71,8 @@ def deduper_cards custom_card("Dedupe organizations", dedupe_index_organizations_path, icon: "🧹", color: DomainTheme.color_for(:organizations), intensity: 100), custom_card("Dedupe categories", dedupe_index_categories_path, icon: "🧹", color: DomainTheme.color_for(:categories), intensity: 100), custom_card("Dedupe event registrations", dedupe_index_event_registrations_path, icon: "🧹", color: DomainTheme.color_for(:event_registrations), intensity: 100), - custom_card("Dedupe sectors", dedupe_index_sectors_path, icon: "🧹", color: DomainTheme.color_for(:sectors), intensity: 100) + custom_card("Dedupe sectors", dedupe_index_sectors_path, icon: "🧹", color: DomainTheme.color_for(:sectors), intensity: 100), + custom_card("Dedupe users", dedupe_index_users_path, icon: "🧹", color: DomainTheme.color_for(:users), intensity: 100) ] end diff --git a/app/services/model_deduper.rb b/app/services/model_deduper.rb index cc16553909..190122b3c8 100644 --- a/app/services/model_deduper.rb +++ b/app/services/model_deduper.rb @@ -5,12 +5,16 @@ class ModelDeduper attr_reader :model_class, :logger, :dry_run, :min_usage - def initialize(model_class:, logger: Logger.new($stdout), dry_run: true, min_usage: 0, movable_attachments: []) + # reference_columns: column names that point at this model in any table that has them, + # for references that carry no DB foreign key (e.g. a user's created_by_id stamps). + def initialize(model_class:, logger: Logger.new($stdout), dry_run: true, min_usage: 0, movable_attachments: [], + reference_columns: []) @model_class = model_class @logger = logger @dry_run = dry_run @min_usage = min_usage @movable_attachments = Array(movable_attachments).map(&:to_s) + @reference_columns = Array(reference_columns).map(&:to_s) end def call @@ -45,7 +49,7 @@ def reassignment_counts(record) count = join_references(join, record.id).count next if count.zero? - counts[join_label(join[:join_class])] = count + counts[join_label(join)] = count end end @@ -62,7 +66,7 @@ def reassignment_preview(record) next if count.zero? { - label: join_label(join[:join_class]), + label: join_label(join), count: count, names: scope.limit(NAME_LIMIT).map { |related| record_display(related) } } @@ -213,7 +217,7 @@ def unhandled_references(record) def reassignable_joins @reassignable_joins ||= begin seen = Set.new - (has_many_joins + belongs_to_joins).select do |join| + (has_many_joins + belongs_to_joins + reference_column_joins).select do |join| seen.add?([ join[:join_class].table_name, join[:foreign_key] ]) end end @@ -244,12 +248,11 @@ def has_many_joins # Schema-driven: every DB foreign key pointing at this model's table, so a child # with an organization_id but no inverse has_many (payments, stories, …) is still # reassigned. Reading the schema avoids eager-loading every model (which would - # touch databases not configured in every environment). + # touch databases not configured in every environment). Includes the model's own + # table, so a sibling referencing the duplicate (users.created_by_id) follows the keeper. def belongs_to_joins connection = model_class.connection connection.tables.flat_map do |table| - next [] if table == model_class.table_name - connection.foreign_keys(table).filter_map do |fk_def| next unless fk_def.to_table == model_class.table_name @@ -260,6 +263,16 @@ def belongs_to_joins [] end + def reference_column_joins + return [] if @reference_columns.empty? + + connection = model_class.connection + connection.tables.flat_map do |table| + column_names = connection.columns(table).map(&:name) + (@reference_columns & column_names).map { |column| join_for(model_for_table(table), column) } + end + end + # The model backing a table, or an anonymous one bound to it. Guards against a # name that resolves to a class mapped elsewhere (e.g. an STI child of another # table), which would reassign the wrong table. @@ -314,8 +327,14 @@ def model_label (model_class.name || model_class.table_name).underscore.humanize.downcase end - def join_label(join_class) - (join_class.name || join_class.table_name.classify).underscore.humanize.pluralize + # Names the column too when a table references this model more than once + # (created_by_id + updated_by_id), so each join gets its own preview row. + def join_label(join) + join_class = join[:join_class] + label = (join_class.name || join_class.table_name.classify).underscore.humanize.pluralize + return label if reassignable_joins.one? { |other| other[:join_class].table_name == join_class.table_name } + + "#{label} (#{join[:foreign_key].to_s.delete_suffix("_id").humanize(capitalize: false)})" end def usage_counts @@ -406,6 +425,8 @@ def merge_join(primary, dupe, join) fk = join[:foreign_key] natural_key = join[:natural_key] + detach_self_reference(primary, dupe, join) + if natural_key.empty? moved = join_references(join, dupe.id).update_all(fk => primary.id) logger.info " moved #{moved} #{jc.name} to primary" if moved > 0 @@ -431,6 +452,14 @@ def merge_join(primary, dupe, join) raise "ABORT: #{remaining} #{jc.name} items still reference #{model_label} #{dupe.id}" if remaining > 0 end + # The keeper can't point at the record merged into it (e.g. its parent or creator), + # so that one reference clears rather than becoming a self-reference. + def detach_self_reference(primary, dupe, join) + return unless join[:join_class].table_name == model_class.table_name + + join[:join_class].where(id: primary.id, join[:foreign_key] => dupe.id).update_all(join[:foreign_key] => nil) + end + # The primary already holds an equivalent row (same natural key), so the loser can't # just move onto it. Merge the loser into the survivor with the deduper itself, which # moves the loser's own children onto the survivor first, then destroys the diff --git a/app/services/user_services/duplicate_finder.rb b/app/services/user_services/duplicate_finder.rb new file mode 100644 index 0000000000..381efb18e6 --- /dev/null +++ b/app/services/user_services/duplicate_finder.rb @@ -0,0 +1,24 @@ +module UserServices + # Logins that share one person — typically left behind when a person merge keeps + # both people's logins. A login that can sign in leads each group, most recently + # updated first, so the deduper suggests keeping the one the person uses. + class DuplicateFinder + Group = Struct.new(:key, :label, :records, :reasons, keyword_init: true) + + def initialize(scope = User.all) + @scope = scope + end + + def groups + shared_person_ids = @scope.where.not(person_id: nil).group(:person_id).having("COUNT(*) > 1").select(:person_id) + @scope.where(person_id: shared_person_ids).includes(:person) + .order(updated_at: :desc, id: :desc) + .group_by(&:person_id) + .map do |person_id, records| + records = records.sort_by.with_index { |user, index| [ user.has_access? ? 0 : 1, index ] } + Group.new(key: person_id.to_s, label: records.first.person.full_name.to_s, records: records, reasons: [ "Same person" ]) + end + .sort_by { |group| group.label.downcase } + end + end +end diff --git a/app/views/dedupes/_preview.html.erb b/app/views/dedupes/_preview.html.erb index db49d99d61..94d1d02647 100644 --- a/app/views/dedupes/_preview.html.erb +++ b/app/views/dedupes/_preview.html.erb @@ -8,6 +8,8 @@ model_label: Human label (e.g. "Organization") keep_param_key: Param key for keep fields (e.g. :organization_to_keep) editable_columns: Optional allowlist of column names to show/edit; nil shows all + read_only_columns: Column names shown side by side for comparison but not editable + record_label: Callable naming a record in the headers (defaults to #name) union_columns: Column names whose values are combined into the keeper (not replaced) form_builder: The parent form builder (f) belongs_to_options: Hash of { column_name => collection } for select fields @@ -31,6 +33,17 @@ field_notes ||= {} deprecated_columns ||= [] preview_images ||= nil + read_only_columns = Array(local_assigns[:read_only_columns]).filter_map { |name| model_class.columns_hash[name.to_s] } + record_label ||= :name.to_proc + read_only_display = ->(col, value) { + if col.type == :boolean + value ? "Yes" : "No" + elsif col.type == :datetime + value&.strftime("%Y-%m-%d %H:%M") || "—" + else + value.presence || "—" + end + } delete_images = preview_images ? Array(preview_images.call(record_to_delete)) : [] keep_images = preview_images ? Array(preview_images.call(record_to_keep)) : [] %> @@ -47,7 +60,7 @@ <%= model_label %> to delete: <%= link_to edit_polymorphic_path(record_to_delete), target: "_blank", rel: "noopener", class: "group inline-flex items-center gap-1.5 font-semibold text-gray-900 hover:underline" do %> - <%= record_to_delete.name %> + <%= record_label.call(record_to_delete) %> <% end %> @@ -56,7 +69,7 @@ <%= model_label %> to keep: <%= link_to edit_polymorphic_path(record_to_keep), target: "_blank", rel: "noopener", class: "group inline-flex items-center gap-1.5 font-semibold text-gray-900 hover:underline" do %> - <%= record_to_keep.name %> + <%= record_label.call(record_to_keep) %> <% end %> @@ -69,6 +82,16 @@ ID <%= record_to_keep.id %> + + <% read_only_columns.each do |col| %> + <% [ record_to_delete, record_to_keep ].each do |record| %> +
+ <%= col.name.humanize %> + <%= read_only_display.call(col, record.public_send(col.name)) %> +
+ <% end %> + <% end %> + <% if preview_images && (delete_images.any? || keep_images.any?) %> diff --git a/app/views/dedupes/index.html.erb b/app/views/dedupes/index.html.erb index f747b28f5f..6e5b412555 100644 --- a/app/views/dedupes/index.html.erb +++ b/app/views/dedupes/index.html.erb @@ -82,7 +82,7 @@ <% group.records.each do |record| %>
  • - <%= record.name.presence || "(no name)" %> + <%= @dedupe[:record_label].call(record).presence || "(no name)" %> · ID <%= record.id %><% if @dedupe[:record_extras] && (extra = @dedupe[:record_extras].call(record)).present? %> · <%= extra %><% end %> diff --git a/app/views/dedupes/preview.html.erb b/app/views/dedupes/preview.html.erb index 800eeba163..1285812c40 100644 --- a/app/views/dedupes/preview.html.erb +++ b/app/views/dedupes/preview.html.erb @@ -92,6 +92,8 @@ model_label: @dedupe[:model_label], keep_param_key: @dedupe[:keep_param_key], editable_columns: @dedupe[:editable_columns], + read_only_columns: @dedupe[:read_only_columns], + record_label: @dedupe[:record_label], union_columns: @dedupe[:union_columns], form_builder: f, belongs_to_options: @dedupe[:belongs_to_options], @@ -129,8 +131,8 @@

    Deleted, not moved

    - These belong to "<%= @record_to_delete.name %>" and are removed with it — they are - not transferred to "<%= @record_to_keep.name %>": + These belong to "<%= @dedupe[:record_label].call(@record_to_delete) %>" and are removed with it — they are + not transferred to "<%= @dedupe[:record_label].call(@record_to_keep) %>":

      <% @lost_references.each do |entry| %> @@ -149,10 +151,10 @@

      This can't be undone. - "<%= @record_to_delete.name %>" (ID <%= @record_to_delete.id %>) will be deleted and + "<%= @dedupe[:record_label].call(@record_to_delete) %>" (ID <%= @record_to_delete.id %>) will be deleted and <%= moved_total %> associated <%= "record".pluralize(moved_total) %> moved to - "<%= @record_to_keep.name %>" (ID <%= @record_to_keep.id %>). + "<%= @dedupe[:record_label].call(@record_to_keep) %>" (ID <%= @record_to_keep.id %>).

    diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 2d4c96d3ac..3c3b2dbd99 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -14,6 +14,8 @@ <%= link_to flow_diagram_users_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> Account flow diagram <% end %> + <%= link_to "Dedupe", dedupe_index_users_path, + class: "admin-only bg-blue-100 text-sm #{eyebrow_link_class} px-2 py-1" %> <% if allowed_to?(:new?, User) %> <%= link_to "New #{User.model_name.human.downcase}", new_user_path, diff --git a/config/features.yml b/config/features.yml index c1d3a4b997..ae49465852 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4338,3 +4338,25 @@ copy had, like a phone number, is filled in rather than lost. It also settles a single primary: the kept organization's own if it had one, otherwise the other organization's. Addresses that genuinely differ are all kept. + +- name: "Merge duplicate logins" + area: people + display_status: admin_facing + released_on: 2026-10-13 + action_path: "/users/dedupe_index" + pr_number: 2662 + summary: >- + Admins can now merge two logins into one. The deduper lists people who have + more than one login, such as after a person merge, and suggests keeping one + that can still sign in. + description: >- + Reach it from the Dedupe link on the Users page or the admin home. The + preview shows both logins side by side — email, admin access, confirmation, + and sign-in history — plus everything that moves to the kept login: its + bookmarks, activity history, and the records it created or last edited. The + other login is deleted, so its email no longer signs in. It can't be undone. + pro_tips: + - "Use Swap to keep the other email as the login." + - "The search finds every login, including unconfirmed, locked, and inactive ones, and labels each with its status." + - "Logins of two different people can't be merged. Merge the people first." + - "You can't merge away the login you're signed in with." diff --git a/config/routes.rb b/config/routes.rb index 7e5bf96aa9..196cb8c8f7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -25,6 +25,10 @@ collection do get :check_duplicates get :flow_diagram + get :dedupe_index + get :dedupe_preview + post :dedupe_perform + patch :dedupe_update_keep end member do post :send_reset_password_instructions diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb new file mode 100644 index 0000000000..959c175e68 --- /dev/null +++ b/spec/decorators/user_decorator_spec.rb @@ -0,0 +1,15 @@ +require "rails_helper" + +RSpec.describe UserDecorator do + describe "#access_statuses" do + it "is empty for a login that can sign in" do + expect(build(:user).decorate.access_statuses).to eq([]) + end + + it "names every reason a login can't sign in" do + user = build(:user, :unconfirmed, :locked, inactive: true) + + expect(user.decorate.access_statuses).to eq(%w[Inactive Locked Unconfirmed]) + end + end +end diff --git a/spec/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index 2ae0ac9fea..a0645e6e20 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -1058,4 +1058,120 @@ def registration_for(person_attrs) end end end + + # ============================================================ + # USERS — merging two logins of one person + # ============================================================ + + describe "Users" do + let(:person) { create(:person, first_name: "Jane", last_name: "Doe", user: nil) } + let!(:keep) { create(:user, person: person, email: "jane.keep@example.com", updated_at: 1.hour.ago) } + let!(:delete_rec) { create(:user, person: person, email: "jane.old@example.com", updated_at: 1.week.ago) } + + before { sign_in admin } + + describe "GET dedupe_index" do + it "lists logins that share a person, labelled by email" do + get dedupe_index_users_path + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Same person", "jane.keep@example.com", "jane.old@example.com") + end + + it "searches every login and shows a login's status in the suggestions" do + delete_rec.update_columns(confirmed_at: nil) + + get dedupe_index_users_path + + expect(response.body).to include('data-remote-select-model-value="user_any_status"') + expect(response.body).to include("Unconfirmed") + end + + it "denies access to a regular user" do + sign_in regular_user + + get dedupe_index_users_path + expect(response).to redirect_to(root_path) + end + end + + describe "GET dedupe_preview" do + it "compares the logins and warns which email stops signing in" do + get dedupe_preview_users_path(user_to_keep_id: keep.id, user_to_delete_id: delete_rec.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Last sign in at", "Sign in count") + expect(response.body).to include("Signing in as jane.old@example.com stops working") + expect(response.body).not_to include("Merge blocked") + end + + it "blocks merging logins that belong to different people" do + delete_rec.update!(person: create(:person, first_name: "Other", last_name: "Human", user: nil)) + + get dedupe_preview_users_path(user_to_keep_id: keep.id, user_to_delete_id: delete_rec.id) + + expect(response.body).to include("Merge blocked", "Merge the people first") + end + + it "blocks merging away the admin's own login" do + get dedupe_preview_users_path(user_to_keep_id: keep.id, user_to_delete_id: admin.id) + + expect(response.body).to include("Merge blocked", "can't merge away your own login") + end + end + + describe "PATCH dedupe_update_keep" do + it "ignores columns the preview doesn't offer for editing" do + patch dedupe_update_keep_users_path, params: { id: keep.id, user_to_keep: { notes: "Merged", super_user: "1" } } + + expect(response).to have_http_status(:ok) + expect(keep.reload.notes).to eq("Merged") + expect(keep.super_user).to be(false) + end + end + + describe "POST dedupe_perform" do + it "moves the deleted login's records and history onto the keeper" do + bookmark = create(:bookmark, user: delete_rec) + grant = create(:grant, created_by: delete_rec) + event = create(:ahoy_event, user_id: delete_rec.id) + version = PaperTrail::Version.create!(event: "update", item_type: "Payment", item_id: 1, whodunnit: delete_rec.id.to_s) + + expect { + post dedupe_perform_users_path, params: { user_to_delete_id: delete_rec.id, user_to_keep_id: keep.id } + }.to change(User, :count).by(-1) + + expect(response).to redirect_to(users_path) + expect(bookmark.reload.user_id).to eq(keep.id) + expect(grant.reload.created_by_id).to eq(keep.id) + expect(event.reload.user_id).to eq(keep.id) + expect(version.reload.whodunnit).to eq(keep.id.to_s) + expect(person.reload.user).to eq(keep) + end + + it "hands the person to a kept login that had none" do + keep.update_columns(person_id: nil) + + post dedupe_perform_users_path, params: { user_to_delete_id: delete_rec.id, user_to_keep_id: keep.id } + + expect(keep.reload.person).to eq(person) + end + + it "keeps the deleted login's password hash out of the tracked event" do + expect(Analytics::AhoyTracker).to receive(:track_event).with(anything, "dedupe.user", hash_including( + deleted_record: hash_including("encrypted_password" => "[FILTERED]", "email" => "jane.old@example.com") + )) + + post dedupe_perform_users_path, params: { user_to_delete_id: delete_rec.id, user_to_keep_id: keep.id } + end + + it "refuses to merge logins that belong to different people" do + delete_rec.update!(person: create(:person, user: nil)) + + expect { + post dedupe_perform_users_path, params: { user_to_delete_id: delete_rec.id, user_to_keep_id: keep.id } + }.not_to change(User, :count) + end + end + end end diff --git a/spec/requests/user_any_status_search_spec.rb b/spec/requests/user_any_status_search_spec.rb new file mode 100644 index 0000000000..3f941919fb --- /dev/null +++ b/spec/requests/user_any_status_search_spec.rb @@ -0,0 +1,32 @@ +require "rails_helper" + +RSpec.describe "Login search across every access status", type: :request do + let(:admin) { create(:user, :admin) } + + describe "GET /search/user_any_status" do + let!(:active) { create(:user, email: "pat.active@example.com") } + let!(:unconfirmed) { create(:user, :unconfirmed, email: "pat.unconfirmed@example.com") } + let!(:locked) { create(:user, :locked, email: "pat.locked@example.com") } + let!(:inactive) { create(:user, email: "pat.inactive@example.com", inactive: true) } + + it "finds logins that can't sign in, labelled with their status" do + sign_in admin + + get "/search/user_any_status", params: { q: "pat." } + + labels = response.parsed_body.to_h { |row| [ row["id"], row["label"] ] } + expect(labels[active.id]).to eq(active.full_name_with_email) + expect(labels[unconfirmed.id]).to eq("#{unconfirmed.full_name_with_email} · Unconfirmed") + expect(labels[locked.id]).to eq("#{locked.full_name_with_email} · Locked") + expect(labels[inactive.id]).to eq("#{inactive.full_name_with_email} · Inactive") + end + + it "is forbidden to a non-admin" do + sign_in create(:user) + + get "/search/user_any_status", params: { q: "pat." } + + expect(response).to redirect_to(root_path) + end + end +end diff --git a/spec/services/model_deduper_spec.rb b/spec/services/model_deduper_spec.rb index f07f281943..899264da6b 100644 --- a/spec/services/model_deduper_spec.rb +++ b/spec/services/model_deduper_spec.rb @@ -250,6 +250,45 @@ end end + describe "user merges (reference columns without a foreign key)" do + subject(:service) do + described_class.new(model_class: User, logger: logger, dry_run: false, + reference_columns: %w[user_id created_by_id updated_by_id welcome_instructions_sent_by_id]) + end + + let!(:keep) { create(:user) } + let!(:dupe) { create(:user) } + + it "moves stamp columns that carry no foreign key" do + grant = create(:grant, created_by: dupe, updated_by: dupe) + + service.merge(keep, dupe) + expect(grant.reload.attributes.values_at("created_by_id", "updated_by_id")).to eq([ keep.id, keep.id ]) + end + + it "moves other logins' references to the duplicate onto the keeper" do + other = create(:user, created_by: dupe, welcome_instructions_sent_by: dupe) + + service.merge(keep, dupe) + expect(other.reload.attributes.values_at("created_by_id", "welcome_instructions_sent_by_id")).to eq([ keep.id, keep.id ]) + end + + it "clears the keeper's own reference to the duplicate rather than pointing it at itself" do + keep.update_columns(created_by_id: dupe.id) + + service.merge(keep, dupe) + expect(keep.reload.created_by_id).to be_nil + expect(User.exists?(dupe.id)).to be(false) + end + + it "labels each column separately when a table references the model twice" do + create(:grant, created_by: dupe, updated_by: dupe) + + labels = service.reassignment_preview(dupe).map { |entry| entry[:label] } + expect(labels).to include("Grants (created by)", "Grants (updated by)") + end + end + describe "#lost_references" do subject(:service) { described_class.new(model_class: Organization) } diff --git a/spec/services/user_services/duplicate_finder_spec.rb b/spec/services/user_services/duplicate_finder_spec.rb new file mode 100644 index 0000000000..11437328eb --- /dev/null +++ b/spec/services/user_services/duplicate_finder_spec.rb @@ -0,0 +1,34 @@ +require "rails_helper" + +RSpec.describe UserServices::DuplicateFinder do + describe "#groups" do + it "groups logins that share a person, most recently updated first" do + person = create(:person, first_name: "Jane", last_name: "Doe", user: nil) + older = create(:user, person: person, updated_at: 1.week.ago) + recent = create(:user, person: person, updated_at: 1.hour.ago) + + groups = described_class.new.groups + + expect(groups.size).to eq(1) + expect(groups.first.records).to eq([ recent, older ]) + expect(groups.first.label).to eq("Jane Doe") + expect(groups.first.reasons).to eq([ "Same person" ]) + end + + it "leads with a login that can sign in over a more recently updated one that can't" do + person = create(:person, user: nil) + usable = create(:user, person: person, updated_at: 1.week.ago) + unconfirmed = create(:user, :unconfirmed, person: person, updated_at: 1.hour.ago) + + expect(described_class.new.groups.first.records).to eq([ usable, unconfirmed ]) + end + + it "ignores a person with one login and logins without a person" do + create(:user, :with_person) + create(:user, person: nil) + create(:user, person: nil) + + expect(described_class.new.groups).to be_empty + end + end +end