diff --git a/app/controllers/concerns/dedupable.rb b/app/controllers/concerns/dedupable.rb index 527c613e1b..175ddca897 100644 --- a/app/controllers/concerns/dedupable.rb +++ b/app/controllers/concerns/dedupable.rb @@ -3,6 +3,10 @@ module Dedupable extend ActiveSupport::Concern + included do + helper_method :dedupe_return + end + CandidateGroup = Struct.new(:label, :records, :reasons, keyword_init: true) def dedupe_index @@ -128,7 +132,7 @@ def dedupe_perform end label = mc.model_name.human.pluralize - redirect_to url_for(action: :index), + redirect_to dedupe_return&.dig(:path) || url_for(action: :index), 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 @@ -139,6 +143,18 @@ def dedupe_perform private + # Where a merge started from outside the dedupe index returns to, with the params that carry it. + def dedupe_return + person_id = params[:person_id].presence + return unless person_id + + carry = { return_to: params[:return_to], person_id: person_id } + case params[:return_to] + when "person_show" then { label: "Back to profile", path: person_path(person_id), params: carry } + when "person_edit" then { label: "Back to edit person", path: edit_person_path(person_id), params: carry } + end + end + # 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]) diff --git a/app/decorators/person_decorator.rb b/app/decorators/person_decorator.rb index 1c78b56ae1..ada6260be9 100644 --- a/app/decorators/person_decorator.rb +++ b/app/decorators/person_decorator.rb @@ -28,6 +28,21 @@ def uncancelled_membership_count end end + # A merge can leave several logins on one person — flagged for admins on the profile + # and edit pages. Ordered like Person#user, so the first is the one the profile shows. + def user_accounts + @user_accounts ||= User.where(person_id: id).order(updated_at: :desc, id: :desc).to_a + end + + def multiple_user_accounts? + user_accounts.size > 1 + end + + # Matches the logins deduper's pick (UserServices::DuplicateFinder): the most recent login that can sign in. + def suggested_user_account + user_accounts.find(&:has_access?) || user_accounts.first + end + # The subscriptions index filtered to this person's News (mailing-list) topic — # where mailing-list interest now lives since the person-level consent flag retired. def news_subscriptions_path diff --git a/app/views/dedupes/preview.html.erb b/app/views/dedupes/preview.html.erb index 1285812c40..8dfff74042 100644 --- a/app/views/dedupes/preview.html.erb +++ b/app/views/dedupes/preview.html.erb @@ -5,7 +5,7 @@
- <%= link_to "← Back to dedupe", url_for(action: :dedupe_index), + <%= link_to "← #{dedupe_return&.dig(:label) || "Back to dedupe"}", dedupe_return&.dig(:path) || url_for(action: :dedupe_index), class: "text-sm #{eyebrow_link_class} px-2 py-1" %>

Preview <%= @dedupe[:model_label].downcase %> merge @@ -15,7 +15,7 @@ move to the one on the right. Edit the keeper's fields before merging.

- <%= link_to url_for(action: :dedupe_preview, @dedupe[:delete_id_param] => @record_to_keep.id, @dedupe[:keep_id_param] => @record_to_delete.id), + <%= link_to url_for(action: :dedupe_preview, @dedupe[:delete_id_param] => @record_to_keep.id, @dedupe[:keep_id_param] => @record_to_delete.id, **dedupe_return&.dig(:params).to_h), class: button_classes(:secondary_outline, size: :sm, extra: "shrink-0") do %> Swap <% end %> @@ -82,6 +82,9 @@ turbo_confirm: "Merge these #{@dedupe[:model_label_plural].downcase}? This can't be undone." } do |f| %> <%= f.hidden_field @dedupe[:delete_id_param].to_sym, value: @record_to_delete.id %> <%= f.hidden_field @dedupe[:keep_id_param].to_sym, value: @record_to_keep.id %> + <% dedupe_return&.dig(:params)&.each do |name, value| %> + <%= hidden_field_tag name, value %> + <% end %> <%= render "dedupes/preview", domain: @dedupe[:domain], @@ -167,7 +170,7 @@ Merge disabled (see above). <% end %> - <%= link_to "Cancel", url_for(action: :dedupe_index), class: button_classes(:secondary_outline) %> + <%= link_to "Cancel", dedupe_return&.dig(:path) || url_for(action: :dedupe_index), class: button_classes(:secondary_outline) %> <%= f.submit "Merge #{@dedupe[:model_label_plural].downcase}", disabled: blocked, class: button_classes(:danger, extra: ("cursor-not-allowed opacity-50" if blocked)) %> diff --git a/app/views/people/_multiple_user_accounts_warning.html.erb b/app/views/people/_multiple_user_accounts_warning.html.erb new file mode 100644 index 0000000000..26f5d132a8 --- /dev/null +++ b/app/views/people/_multiple_user_accounts_warning.html.erb @@ -0,0 +1,34 @@ +<% decorated = person.decorate %> +<% user_accounts = decorated.user_accounts %> +<% if user_accounts.size > 1 %> + <% keeper = decorated.suggested_user_account %> +
+
+
+ +
+
+

Multiple user accounts

+

+ This person has <%= user_accounts.size %> user accounts. A person should have only one — the extras are usually a leftover from a merge. +

+
    + <% user_accounts.each_with_index do |user, index| %> +
  • + <%= link_to user.email, user_path(user), class: "underline hover:text-yellow-900", data: { turbo_frame: "_top" } %> + · last signed in <%= user.decorate.last_logged_in %><%= " · shown on profile" if index.zero? %> + <% if user == keeper %> + Suggested keeper + <% else %> + <%= link_to "Preview merge →", + dedupe_preview_users_path(user_to_delete_id: user.id, user_to_keep_id: keeper.id, return_to: return_to, person_id: person.id), + class: "ml-1 font-medium underline hover:text-yellow-900", + data: { turbo: false } %> + <% end %> +
    • + <% end %> +
+
+
+
+<% end %> diff --git a/app/views/people/edit.html.erb b/app/views/people/edit.html.erb index 0715d3a882..0285354c9e 100644 --- a/app/views/people/edit.html.erb +++ b/app/views/people/edit.html.erb @@ -47,6 +47,9 @@ title: truncate(@person.name, length: 40), back: back, actions: actions, width: "max-w-7xl", sticky_header: true, card: false } do %> + <% if allowed_to?(:index?, User) %> + <%= render "people/multiple_user_accounts_warning", person: @person, return_to: "person_edit" %> + <% end %>
<%= render "form" %> <%= render "shared/audit_info", resource: @person %> diff --git a/app/views/people/show.html.erb b/app/views/people/show.html.erb index 9369db0179..2330973c01 100644 --- a/app/views/people/show.html.erb +++ b/app/views/people/show.html.erb @@ -12,6 +12,9 @@ <% if allowed_to?(:index?, MembershipInvoice) %> <%= render "people/multiple_memberships_warning", person: @person %> <% end %> +<% if allowed_to?(:index?, User) %> + <%= render "people/multiple_user_accounts_warning", person: @person, return_to: "person_show" %> +<% end %>
<%= render accent_strip_partial %> diff --git a/config/features.yml b/config/features.yml index 095ff0ac2c..89d290ad37 100644 --- a/config/features.yml +++ b/config/features.yml @@ -4527,13 +4527,16 @@ - "If the resubmission adds a scholarship request, the subject ends with \"(scholarship now requested)\"." - "Filter the Communications page by \"Admin FYI: duplicate event registration\" or \"Duplicate event registration received\" to find these emails." -- name: "Internal notes on people" +- name: "Flag people with more than one user account" area: people display_status: admin_facing - released_on: 2026-10-14 - pr_number: 2689 + released_on: 2026-10-12 + pr_number: 2661 summary: >- - Admins can see and edit a person's internal notes, which hold comments - carried over from FileMaker, on the person's edit page. + A person's profile and edit pages now warn admins when the person has more + than one user account, link to each one, and offer to merge the extras into + the suggested keeper. pro_tips: - - "Add ?admin=true to the end of the edit page's address to show the Internal notes box, the same as on organizations and users." + - "The extra account is usually left over from a merge." + - "Preview merge opens the logins deduper with the suggested keeper picked. Its back link and the finished merge return you to the person." + - "The account marked \"shown on profile\" is the one used most recently." diff --git a/spec/decorators/person_decorator_spec.rb b/spec/decorators/person_decorator_spec.rb index af9c5db099..2668098ae1 100644 --- a/spec/decorators/person_decorator_spec.rb +++ b/spec/decorators/person_decorator_spec.rb @@ -1,6 +1,30 @@ require "rails_helper" RSpec.describe PersonDecorator do + describe "#user_accounts" do + let(:person) { create(:person) } + + it "lists every login on the person, most recently updated first" do + older = person.user.tap { |user| user.update_columns(updated_at: 2.days.ago) } + newer = create(:user, person: person, updated_at: 1.hour.ago) + create(:user) + + expect(person.decorate.user_accounts).to eq([ newer, older ]) + expect(person.decorate.multiple_user_accounts?).to be(true) + end + + it "suggests keeping the most recent login that can still sign in" do + usable = person.user + create(:user, person: person, confirmed_at: nil, updated_at: 1.minute.from_now) + + expect(person.decorate.suggested_user_account).to eq(usable) + end + + it "does not flag a person with one login" do + expect(person.decorate.multiple_user_accounts?).to be(false) + end + end + describe "#news_subscriptions_path" do let(:person) { create(:person) } diff --git a/spec/requests/people_multiple_user_accounts_warning_spec.rb b/spec/requests/people_multiple_user_accounts_warning_spec.rb new file mode 100644 index 0000000000..02d805d8b0 --- /dev/null +++ b/spec/requests/people_multiple_user_accounts_warning_spec.rb @@ -0,0 +1,65 @@ +require "rails_helper" + +RSpec.describe "People multiple-user-accounts warning", type: :request do + let(:admin) { create(:user, :admin) } + + before { sign_in admin } + + it "flags a person with more than one user account on show and edit, linking to each" do + person = create(:person) + first_login = person.user + second_login = create(:user, person: person) + + get person_path(person) + expect(response).to have_http_status(:ok) + expect(response.body).to include("Multiple user accounts") + expect(response.body).to include(user_path(first_login), user_path(second_login)) + + get edit_person_path(person) + expect(response).to have_http_status(:ok) + expect(response.body).to include("Multiple user accounts") + expect(response.body).to include(user_path(first_login), user_path(second_login)) + end + + it "does not flag a person with a single user account" do + person = create(:person) + + get person_path(person) + expect(response.body).not_to include("Multiple user accounts") + + get edit_person_path(person) + expect(response.body).not_to include("Multiple user accounts") + end + + it "links each extra account to a merge preview into the suggested keeper, returning to the page it came from" do + person = create(:person) + stale = person.user.tap { |user| user.update_columns(confirmed_at: nil, updated_at: 1.minute.from_now) } + keeper = create(:user, person: person) + + get edit_person_path(person) + + expect(response.body).to include("Suggested keeper") + expect(response.body).to include(CGI.escapeHTML(dedupe_preview_users_path(user_to_delete_id: stale.id, user_to_keep_id: keeper.id, return_to: "person_edit", person_id: person.id))) + end + + describe "returning from the logins merge" do + let(:person) { create(:person) } + let!(:keeper) { person.user } + let!(:extra) { create(:user, person: person) } + + it "points the preview's back link at the person page it came from" do + get dedupe_preview_users_path(user_to_delete_id: extra.id, user_to_keep_id: keeper.id, return_to: "person_show", person_id: person.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Back to profile") + expect(response.body).to include(%(href="#{person_path(person)}")) + end + + it "lands back on the person page after merging" do + post dedupe_perform_users_path, params: { user_to_delete_id: extra.id, user_to_keep_id: keeper.id, return_to: "person_edit", person_id: person.id } + + expect(response).to redirect_to(edit_person_path(person)) + expect(User.exists?(extra.id)).to be(false) + end + end +end