From 3ec307326b99f68749a2a932d5a8004180cae2f3 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 19:23:59 -0400 Subject: [PATCH 1/3] Start a user (login) deduper Co-Authored-By: Claude Opus 5.5 (1M context) From 3837ce553c3aca4792d95f2e17e915ea9272f581 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 19:31:26 -0400 Subject: [PATCH 2/3] Let admins merge duplicate logins A person merge keeps both people's logins, so a person can end up with two. Admins had no way to fold the stale one away. Users now plug into the shared deduper: candidates are logins sharing a person, the most recently used is the suggested keeper, and logins of different people (or your own) can't be merged. The engine needed a few generic pieces for logins: reference columns without a DB foreign key (created_by/updated_by stamps, ahoy user_id), self-references on the model's own table, per-column labels when a table references the model twice, an editable-column allowlist on save, and credential filtering in the tracked deleted record. Co-Authored-By: Claude Opus 5.5 (1M context) --- AGENTS.md | 5 +- app/controllers/concerns/dedupable.rb | 47 ++++++-- app/controllers/users_controller.rb | 41 +++++++ app/helpers/admin_cards_helper.rb | 3 +- app/services/model_deduper.rb | 47 ++++++-- .../user_services/duplicate_finder.rb | 23 ++++ app/views/dedupes/_preview.html.erb | 27 ++++- app/views/dedupes/index.html.erb | 2 +- app/views/dedupes/preview.html.erb | 10 +- app/views/users/index.html.erb | 2 + config/features.yml | 21 ++++ config/routes.rb | 4 + spec/requests/dedupable_spec.rb | 107 ++++++++++++++++++ spec/services/model_deduper_spec.rb | 39 +++++++ .../user_services/duplicate_finder_spec.rb | 26 +++++ 15 files changed, 373 insertions(+), 31 deletions(-) create mode 100644 app/services/user_services/duplicate_finder.rb create mode 100644 spec/services/user_services/duplicate_finder_spec.rb diff --git a/AGENTS.md b/AGENTS.md index 6104a1dc04..c5b5c946ff 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), most recently updated first so the deduper suggests the login the person signs in with ## Decorators (Draper) diff --git a/app/controllers/concerns/dedupable.rb b/app/controllers/concerns/dedupable.rb index c11a8a5807..5ed58699f4 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,14 @@ 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) + # 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 @@ -200,6 +221,8 @@ def build_dedupe_vars(config) keep_param_key: "#{mn}_to_keep".to_sym, 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/users_controller.rb b/app/controllers/users_controller.rb index 967ebd5fc3..fa27b5a2c7 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,45 @@ 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 }, + 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", 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/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..3eaa3738ec --- /dev/null +++ b/app/services/user_services/duplicate_finder.rb @@ -0,0 +1,23 @@ +module UserServices + # Logins that share one person — typically left behind when a person merge keeps + # both people's logins. The most recently updated login (the one the person signs + # in with) leads each group so the deduper suggests keeping it. + 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| + 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..bd28608fd5 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4338,3 +4338,24 @@ 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 the + one they signed in with most recently. + 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." + - "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/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index 2ae0ac9fea..e618360d98 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -1058,4 +1058,111 @@ 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 "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/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..41265255e9 --- /dev/null +++ b/spec/services/user_services/duplicate_finder_spec.rb @@ -0,0 +1,26 @@ +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 "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 From 5ad6ecadb12b242ea5ffb5c0c49a96ceb35dcbed Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Mon, 12 Oct 2026 21:35:02 -0400 Subject: [PATCH 3/3] Let the login deduper pick logins that can't sign in Duplicate logins are often the unconfirmed or locked ones, and the shared user search only returns logins with access, so admins couldn't pick them by hand. The deduper's pickers now search every login and label each with its status. The suggested keeper also prefers a login that can sign in, so a stale unconfirmed login isn't suggested just because it was touched last. Co-Authored-By: Claude Opus 5.5 (1M context) --- AGENTS.md | 2 +- app/controllers/concerns/dedupable.rb | 4 ++- app/controllers/search_controller.rb | 14 ++++++++ app/controllers/users_controller.rb | 3 +- app/decorators/user_decorator.rb | 9 ++++++ .../user_services/duplicate_finder.rb | 5 +-- config/features.yml | 5 +-- spec/decorators/user_decorator_spec.rb | 15 +++++++++ spec/requests/dedupable_spec.rb | 9 ++++++ spec/requests/user_any_status_search_spec.rb | 32 +++++++++++++++++++ .../user_services/duplicate_finder_spec.rb | 8 +++++ 11 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 spec/decorators/user_decorator_spec.rb create mode 100644 spec/requests/user_any_status_search_spec.rb diff --git a/AGENTS.md b/AGENTS.md index c5b5c946ff..6baf152ff6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -338,7 +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), most recently updated first so the deduper suggests the login the person signs in with +- `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 5ed58699f4..527c613e1b 100644 --- a/app/controllers/concerns/dedupable.rb +++ b/app/controllers/concerns/dedupable.rb @@ -169,6 +169,8 @@ def dedupe_parameter_filter # 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 @@ -219,7 +221,7 @@ 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, 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 fa27b5a2c7..1698f22747 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -329,13 +329,14 @@ def dedupe_config 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", last_sign_in ].compact_blank.join(" · ") + [ 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." ] 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/services/user_services/duplicate_finder.rb b/app/services/user_services/duplicate_finder.rb index 3eaa3738ec..381efb18e6 100644 --- a/app/services/user_services/duplicate_finder.rb +++ b/app/services/user_services/duplicate_finder.rb @@ -1,7 +1,7 @@ module UserServices # Logins that share one person — typically left behind when a person merge keeps - # both people's logins. The most recently updated login (the one the person signs - # in with) leads each group so the deduper suggests keeping it. + # 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) @@ -15,6 +15,7 @@ def groups .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 } diff --git a/config/features.yml b/config/features.yml index bd28608fd5..ae49465852 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4347,8 +4347,8 @@ 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 the - one they signed in with most recently. + 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, @@ -4357,5 +4357,6 @@ 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/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 e618360d98..a0645e6e20 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -1078,6 +1078,15 @@ def registration_for(person_attrs) 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 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/user_services/duplicate_finder_spec.rb b/spec/services/user_services/duplicate_finder_spec.rb index 41265255e9..11437328eb 100644 --- a/spec/services/user_services/duplicate_finder_spec.rb +++ b/spec/services/user_services/duplicate_finder_spec.rb @@ -15,6 +15,14 @@ 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)