From c8d393ca9275f77146c3abbeb088501385bae08b Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 12:10:06 -0400 Subject: [PATCH 1/8] Add fake-submission cleanup: service, rake task, and admin purge button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fake test data created in prod needs removing without leaving orphans. Person.destroy cascades most of the graph, but payments, refunds, allocations, bulk-payment notifications, Pay::Customer rows, and the PaperTrail/Ahoy audit trail do not — so they're torn down explicitly. FakeSubmissionRemover centralizes the logic and the safety rules: it skips people who look real (active/admin account, grant donor, spotlighted facilitator) unless force-overridden, and the override surfaces account-owned content so an admin sees what blocks the delete (FK-restricted authored content) or cascades with it. Exposed two ways: the data:remove_fake_submissions rake task (dry run unless CONFIRM=true) for bulk cleanup, and an admin-only purge button on person edit (?admin=true) with an interstitial confirmation showing the full inventory. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 4 +- app/controllers/people_controller.rb | 40 ++- app/policies/person_policy.rb | 8 + app/services/fake_submission_remover.rb | 287 ++++++++++++++++++ app/views/people/edit.html.erb | 5 + app/views/people/purge_confirmation.html.erb | 87 ++++++ config/routes.rb | 2 + lib/tasks/remove_fake_submissions.rake | 82 +++++ spec/policies/person_policy_spec.rb | 14 + spec/requests/people_purge_spec.rb | 136 +++++++++ spec/services/fake_submission_remover_spec.rb | 255 ++++++++++++++++ spec/tasks/remove_fake_submissions_spec.rb | 52 ++++ spec/views/page_bg_class_alignment_spec.rb | 1 + 13 files changed, 971 insertions(+), 2 deletions(-) create mode 100644 app/services/fake_submission_remover.rb create mode 100644 app/views/people/purge_confirmation.html.erb create mode 100644 lib/tasks/remove_fake_submissions.rake create mode 100644 spec/requests/people_purge_spec.rb create mode 100644 spec/services/fake_submission_remover_spec.rb create mode 100644 spec/tasks/remove_fake_submissions_spec.rb diff --git a/AGENTS.md b/AGENTS.md index af9e50c08b..e6a92cf56a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -185,6 +185,7 @@ end - `WorkshopVariationFromIdeaService` — Variation creation from ideas - `TaggingSearchService` — Search and filter tagging data - `PersonFromUserService` — Create Person from User account +- `FakeSubmissionRemover` — Deletes a set of people and their full data + audit (PaperTrail/Ahoy) graph; protects real/admin accounts (override with `force`). Built for fake-submission cleanup but general-purpose. Used by the `data:remove_fake_submissions` rake task and the admin-only "Delete person & all data" purge button on person edit (`purge_confirmation`/`purge` actions) - `BulkInviteService` — Bulk send welcome instructions and reset created_at for users - `FormBuilderService` — Builds configurable forms from composable sections with per-field visibility - `ModelDeduper` — Deduplication logic @@ -413,8 +414,9 @@ RuboCop linting on PRs and pushes to main. ## Rake Tasks -Located in `lib/tasks/` (4 files): +Located in `lib/tasks/` (9 files): - `dev.rake` — Development database seeding from XML/CSV - `rhino_migrator.rake` — Rich text editor migration - `attachment_report.rake` — Attachment reporting - `migrate_internal_id_to_filemaker_code.rake` — FileMaker code migration +- `remove_fake_submissions.rake` (`data:remove_fake_submissions`) — Removes fake form-submission people via `FakeSubmissionRemover` (dry run unless `CONFIRM=true`) diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 7df2931117..7512d52c29 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -1,6 +1,6 @@ class PeopleController < ApplicationController include AhoyTracking, TagAssignable - before_action :set_person, only: %i[ show edit update destroy workshop_logs checkout bio ] + before_action :set_person, only: %i[ show edit update destroy workshop_logs checkout bio purge_confirmation purge ] def index authorize! @@ -227,6 +227,44 @@ def destroy end end + # Admin-only interstitial: shows exactly what would be deleted for this person + # (and why they're protected, if so) before anything is removed. + def purge_confirmation + authorize! @person, to: :purge? + @force = params[:force] == "true" + # force: true so the inventory reflects the full graph that WOULD be deleted, + # even when the person looks real — the page always shows what's at stake. + preview = FakeSubmissionRemover.new(person_ids: [ @person.id ], force: true) + @protections = preview.protection_reasons(@person) + @counts = preview.counts + # The linked account's own content — only relevant when overriding, since that's + # when the account itself gets destroyed. Blocking content makes the delete + # impossible; cascading content is removed with the account. + @account_blocking = @force ? preview.blocking_account_content : {} + @account_cascading = @force ? preview.cascading_account_content : {} + @person = @person.decorate + end + + # Admin-only deletion of a person and their whole data + audit graph. Skips a + # person who looks real unless force=true overrides the protections (see + # FakeSubmissionRemover). + def purge + authorize! @person, to: :purge? + remover = FakeSubmissionRemover.new(person_ids: [ @person.id ], force: params[:force] == "true") + + if remover.call.include?(@person.id) + redirect_to people_path, status: :see_other, + notice: "#{@person.full_name} and all their data were deleted." + else + reasons = remover.skipped.find { |skip| skip.person.id == @person.id }&.reasons + redirect_to edit_person_path(@person), status: :see_other, + alert: "Couldn't delete — this person looks real (#{reasons&.join(', ')}). Use “Delete anyway” to override." + end + rescue ActiveRecord::InvalidForeignKey + redirect_to edit_person_path(@person), status: :see_other, + alert: "Couldn't delete #{@person.full_name} — the linked User Account owns content (e.g. workshops, reports, or stories) that must be reassigned or removed first." + end + def check_duplicates authorize! diff --git a/app/policies/person_policy.rb b/app/policies/person_policy.rb index 1f12418ba9..d555d2ceb7 100644 --- a/app/policies/person_policy.rb +++ b/app/policies/person_policy.rb @@ -29,6 +29,14 @@ def destroy? admin? && record.persisted? && !has_associated_data? end + # Full purge of a person and their entire data graph. Bypasses the + # has_associated_data? guard on destroy? on purpose — purging exists precisely + # to remove people who have that data. The FakeSubmissionRemover service still + # refuses to touch anyone who looks real unless explicitly forced. + def purge? + admin? + end + def search? admin? end diff --git a/app/services/fake_submission_remover.rb b/app/services/fake_submission_remover.rb new file mode 100644 index 0000000000..37a29a5f68 --- /dev/null +++ b/app/services/fake_submission_remover.rb @@ -0,0 +1,287 @@ +# Deletes a set of people along with their entire data graph and audit trail. +# Built for cleaning up FAKE form submissions (hence the name and the protections +# below), but the mechanism is general — it backs both the +# data:remove_fake_submissions rake task and the admin-only "Delete person & all +# data" button on the person edit page. When general person-deletion grows beyond +# the fake-data use case, rename this to something like PersonPurger. +# +# Person.destroy cascades the submissions, answers, affiliations, registrations +# and scholarships (see Person). This also tears down what does NOT cascade: +# payments (+ their refunds and allocations), bulk-payment notifications, Stripe +# Pay::Customer rows, and the PaperTrail + Ahoy audit trail for every record +# removed. +# +# People that look real are SKIPPED, never deleted: a grant donor, a spotlighted +# facilitator, or anyone with a User account — unless delete_users: true AND the +# account is clearly an unused auto-created one (never signed in, not a +# super_user, no Ahoy activity of its own, authored no other people). A real +# admin is always protected. This is ONLY for fake data — never use it to delete +# a regular person. +# +# force: true is a deliberate escape hatch that ignores ALL of the protections +# above and deletes anyway (destroying any linked account). Use only when you are +# certain the heuristic is a false positive. The database still refuses to orphan +# real authored content (workshops, reports, stories are FK-restricted), so a +# force delete of an account that authored content fails and rolls back. +class FakeSubmissionRemover + Skip = Data.define(:person, :reasons) + + # Only these models carry has_paper_trail. + VERSIONED_TYPES = %w[Payment Refund Allocation].freeze + + def initialize(person_ids: [], form_submission_ids: [], delete_users: false, force: false) + @person_ids = Array(person_ids).map(&:to_i).uniq + @form_submission_ids = Array(form_submission_ids).map(&:to_i).uniq + @delete_users = delete_users + @force = force + end + + # Why a person is protected, independent of delete_users/force — for showing + # the operator what they'd be overriding. Empty means the person is safe. + def protection_reasons(person) + reasons = [] + reasons << account_description(person.user) if person.user.present? + reasons << "is a spotlighted facilitator" if person.stories_as_spotlighted_facilitator.exists? + reasons << "is a grant donor" if person.grants.exists? + reasons + end + + # People that will be deleted (passed every safety check). + def deletable + @deletable ||= people.select { |person| reasons_for(person).empty? } + end + + # People that look real, with the reason(s) they were skipped. + def skipped + @skipped ||= people.filter_map do |person| + reasons = reasons_for(person) + Skip.new(person: person, reasons: reasons) if reasons.any? + end + end + + def missing_person_ids + target_person_ids - people.map(&:id) + end + + def missing_form_submission_ids + @form_submission_ids - FormSubmission.where(id: @form_submission_ids).pluck(:id) + end + + # Content authored by the User account(s) that would be destroyed in a force + # delete and which the database REFUSES to orphan (created_by_id is FK-restricted + # with no dependent). If any of these exist the delete cannot proceed — the + # content must be reassigned or removed first. + def blocking_account_content + user_ids = collected[:users].map(&:id) + return {} if user_ids.empty? + + { + "Workshops" => Workshop.where(created_by_id: user_ids).count, + "Workshop logs" => WorkshopLog.where(created_by_id: user_ids).count, + "Workshop ideas" => WorkshopIdea.where(created_by_id: user_ids).count, + "Workshop variations" => WorkshopVariation.where(created_by_id: user_ids).count, + "Workshop variation ideas" => WorkshopVariationIdea.where(created_by_id: user_ids).count, + "Reports" => Report.where(created_by_id: user_ids).count, + "Resources" => Resource.where(created_by_id: user_ids).count, + "Stories" => Story.where(created_by_id: user_ids).count, + "Story ideas" => StoryIdea.where(created_by_id: user_ids).count, + "Comments" => Comment.where(created_by_id: user_ids).count + }.select { |_, count| count.positive? } + end + + # Records owned by the User account that ARE removed with it (dependent: destroy). + def cascading_account_content + user_ids = collected[:users].map(&:id) + return {} if user_ids.empty? + + { + "Bookmarks" => Bookmark.where(user_id: user_ids).count, + "User forms" => UserForm.where(user_id: user_ids).count + }.select { |_, count| count.positive? } + end + + # Per-record-type counts for a dry-run preview. Does not mutate anything. + def counts + c = collected + { + people: deletable.size, + users: c[:users].size, + form_submissions: c[:submission_ids].size, + form_answers: c[:answer_ids].size, + affiliations: c[:affiliation_ids].size, + event_registrations: c[:registration_ids].size, + scholarships: c[:scholarship_ids].size, + payments: c[:payment_ids].size, + refunds: c[:refund_ids].size, + allocations: c[:allocation_ids].size, + notifications: c[:notification_ids].size, + pay_customers: c[:pay_customer_ids].size, + paper_trail_versions: version_count, + ahoy_events: ahoy_count + } + end + + # Deletes the graph in a single transaction. Returns the ids of the deleted + # people (empty when nothing was safe to delete). + def call + return [] if deletable.empty? + + c = collected + ActiveRecord::Base.transaction do + # Non-cascading records first, in FK-safe order: allocations and refunds + # reference payments; payments reference submissions; everything else + # cascades off Person. Users are destroyed before their person so the + # has_one :user nullify (which the model forbids) never runs. + Allocation.where(id: c[:allocation_ids]).find_each(&:destroy!) + Refund.where(id: c[:refund_ids]).find_each(&:destroy!) + Payment.where(id: c[:payment_ids]).find_each(&:destroy!) + Notification.where(id: c[:notification_ids]).find_each(&:destroy!) + Pay::Customer.where(id: c[:pay_customer_ids]).find_each(&:destroy!) + c[:users].each(&:destroy!) + deletable.each { |person| person.association(:user).reset } + deletable.each(&:destroy!) + delete_audit_trail(c) + end + + deletable.map(&:id) + end + + private + + attr_reader :delete_users, :force + + def target_person_ids + @target_person_ids ||= begin + derived = FormSubmission.where(id: @form_submission_ids).distinct.pluck(:person_id) + (@person_ids + derived).uniq + end + end + + def people + @people ||= Person.where(id: target_person_ids).to_a + end + + def reasons_for(person) + return [] if force + + reasons = [] + if person.user.present? && (real_user?(person.user) || !delete_users) + reasons << account_description(person.user) + end + reasons << "is a spotlighted facilitator" if person.stories_as_spotlighted_facilitator.exists? + reasons << "is a grant donor" if person.grants.exists? + reasons + end + + def account_description(user) + if real_user?(user) + "has an active/admin User account (signed in, super_user, or has its own activity)" + else + "has a User account" + end + end + + # A User is real (and must never be deleted) if it has ever signed in, is a + # super_user, has Ahoy activity of its own, or authored other people. This runs + # before any deletion, so cleaning the fake graph can't erase the signal. + def real_user?(user) + user.super_user? || + user.sign_in_count.to_i.positive? || + user.current_sign_in_at.present? || user.last_sign_in_at.present? || + Ahoy::Event.where(user_id: user.id).exists? || + Person.where(created_by_id: user.id).where.not(id: target_person_ids).exists? || + Person.where(updated_by_id: user.id).where.not(id: target_person_ids).exists? + end + + def collected + @collected ||= begin + person_ids = deletable.map(&:id) + submission_ids = FormSubmission.where(person_id: person_ids).pluck(:id) + registration_ids = EventRegistration.where(registrant_id: person_ids).pluck(:id) + scholarship_ids = Scholarship.where(recipient_id: person_ids).pluck(:id) + payment_ids = Payment.where(person_id: person_ids) + .or(Payment.where(form_submission_id: submission_ids)).pluck(:id) + { + person_ids: person_ids, + submission_ids: submission_ids, + registration_ids: registration_ids, + scholarship_ids: scholarship_ids, + affiliation_ids: Affiliation.where(person_id: person_ids).pluck(:id), + answer_ids: FormAnswer.where(form_submission_id: submission_ids).pluck(:id), + ero_ids: EventRegistrationOrganization.where(event_registration_id: registration_ids).pluck(:id), + address_ids: Address.where(addressable_type: "Person", addressable_id: person_ids).pluck(:id), + contact_method_ids: ContactMethod.where(contactable_type: "Person", contactable_id: person_ids).pluck(:id), + sectorable_item_ids: SectorableItem.where(sectorable_type: "Person", sectorable_id: person_ids).pluck(:id), + categorizable_item_ids: CategorizableItem.where(categorizable_type: "Person", categorizable_id: person_ids).pluck(:id), + payment_ids: payment_ids, + refund_ids: Refund.where(refundable_type: "Payment", refundable_id: payment_ids).pluck(:id), + allocation_ids: allocation_ids(payment_ids, scholarship_ids, registration_ids), + notification_ids: Notification.where(noticeable_type: "FormSubmission", noticeable_id: submission_ids).pluck(:id), + pay_customer_ids: Pay::Customer.where(owner_type: "Person", owner_id: person_ids).pluck(:id), + users: (delete_users || force) ? deletable.filter_map(&:user) : [] + } + end + end + + def allocation_ids(payment_ids, scholarship_ids, registration_ids) + Allocation + .where(source_type: "Payment", source_id: payment_ids) + .or(Allocation.where(source_type: "Scholarship", source_id: scholarship_ids)) + .or(Allocation.where(allocatable_type: "EventRegistration", allocatable_id: registration_ids)) + .pluck(:id) + end + + # resource_type => ids, covering every record this task removes — for the Ahoy + # event and PaperTrail version cleanup. + def audit_targets(c) + { + "Person" => c[:person_ids], + "FormSubmission" => c[:submission_ids], + "FormAnswer" => c[:answer_ids], + "Affiliation" => c[:affiliation_ids], + "EventRegistration" => c[:registration_ids], + "EventRegistrationOrganization" => c[:ero_ids], + "Scholarship" => c[:scholarship_ids], + "Address" => c[:address_ids], + "ContactMethod" => c[:contact_method_ids], + "SectorableItem" => c[:sectorable_item_ids], + "CategorizableItem" => c[:categorizable_item_ids], + "Payment" => c[:payment_ids], + "Refund" => c[:refund_ids], + "Allocation" => c[:allocation_ids], + "User" => c[:users].map(&:id) + }.reject { |_, ids| ids.empty? } + end + + def delete_audit_trail(c) + targets = audit_targets(c) + + # Includes the destroy-event versions PaperTrail just wrote for the + # versioned models. + targets.slice(*VERSIONED_TYPES).each do |type, ids| + PaperTrail::Version.where(item_type: type, item_id: ids).delete_all + end + targets.each do |type, ids| + Ahoy::Event.where(resource_type: type, resource_id: ids).delete_all + end + + user_ids = c[:users].map(&:id) + return if user_ids.empty? + + Ahoy::Event.where(user_id: user_ids).delete_all + Ahoy::Visit.where(user_id: user_ids).delete_all + end + + def version_count + audit_targets(collected).slice(*VERSIONED_TYPES) + .sum { |type, ids| PaperTrail::Version.where(item_type: type, item_id: ids).count } + end + + def ahoy_count + resource = audit_targets(collected) + .sum { |type, ids| Ahoy::Event.where(resource_type: type, resource_id: ids).count } + user_ids = collected[:users].map(&:id) + actor = user_ids.any? ? Ahoy::Event.where(user_id: user_ids).count : 0 + resource + actor + end +end diff --git a/app/views/people/edit.html.erb b/app/views/people/edit.html.erb index 1df94ef0e4..e7c4a3eb69 100644 --- a/app/views/people/edit.html.erb +++ b/app/views/people/edit.html.erb @@ -2,6 +2,11 @@
+ <% if params[:admin] == "true" && allowed_to?(:purge?, @person) %> + <%= link_to "Delete person & all data", + purge_confirmation_person_path(@person), + class: "btn btn-danger-outline px-2 py-1" %> + <% end %> <% unless @person.user %>
No user! diff --git a/app/views/people/purge_confirmation.html.erb b/app/views/people/purge_confirmation.html.erb new file mode 100644 index 0000000000..1480119512 --- /dev/null +++ b/app/views/people/purge_confirmation.html.erb @@ -0,0 +1,87 @@ +<% content_for(:page_bg_class, "admin-only bg-blue-100") %> +
+
+ <%= link_to "← Back to edit", edit_person_path(@person), class: "text-sm text-gray-500 hover:text-gray-700" %> +
+ +

+ Delete person & all data: <%= @person.name %> +

+ + <% if @protections.any? %> +
+

This person looks real:

+
    + <% @protections.each do |reason| %> +
  • <%= reason %>
    • + <% end %> +
+
+ <% end %> + + <% if @force %> +
+

⚠️ Override: deleting anyway despite the protections above.

+

+ The linked User Account will be destroyed too. If they authored real content + (workshops, reports, stories), the database will reject this and nothing + will be deleted. +

+
+ + <% if @account_blocking.any? %> +
+

Cannot delete: this User Account created content that must be reassigned or removed first.

+
+
+ <% @account_blocking.each do |label, count| %> +
"> +
<%= label %>
+
"><%= count.zero? ? "—" : count %>
+
+ <% end %> +
+ <% end %> + + <% if @account_cascading.any? %> +

Also removed with the User Account

+
+ <% @account_cascading.each do |label, count| %> +
"> +
<%= label %>
+
"><%= count.zero? ? "—" : count %>
+
+ <% end %> +
+ <% end %> + <% elsif @protections.empty? %> +
+

⚠️ This permanently deletes the person and everything below. This cannot be undone.

+
+ <% end %> + +

What will be deleted

+
+ <% @counts.each do |label, count| %> +
"> +
<%= label.to_s.tr("_", " ").capitalize %>
+
"><%= count.zero? ? "—" : count %>
+
+ <% end %> +
+ +
+ <%= link_to "Cancel", edit_person_path(@person), class: "btn btn-secondary-outline" %> + <% if @protections.any? && !@force %> + <%= link_to "Delete anyway (override)", + purge_confirmation_person_path(@person, force: true), + class: "btn btn-danger-outline" %> + <% elsif @account_blocking.blank? %> + <%= button_to "Permanently delete", + purge_person_path(@person, force: @force), + method: :delete, + class: "btn btn-danger", + form: { data: { turbo_confirm: "Permanently delete #{@person.name} and ALL the data listed? This cannot be undone." } } %> + <% end %> +
+
diff --git a/config/routes.rb b/config/routes.rb index d5d8f257a9..9ee3605caf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -170,6 +170,8 @@ get :workshop_logs get :checkout get :bio + get :purge_confirmation + delete :purge end resources :comments, only: [ :index, :create, :update ] end diff --git a/lib/tasks/remove_fake_submissions.rake b/lib/tasks/remove_fake_submissions.rake new file mode 100644 index 0000000000..59d58c6a4a --- /dev/null +++ b/lib/tasks/remove_fake_submissions.rake @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +# Remove fake form-submission test data. Thin wrapper around FakeSubmissionRemover +# (app/services/fake_submission_remover.rb) — see it for the full deletion graph +# and the rules that protect real people. +# +# Usage (dry run — prints what WOULD be deleted, changes nothing): +# bin/rails data:remove_fake_submissions PERSON_IDS=12,34,56 +# bin/rails data:remove_fake_submissions FORM_SUBMISSION_IDS=78,90 +# +# To actually delete, add CONFIRM=true: +# bin/rails data:remove_fake_submissions PERSON_IDS=12,34 CONFIRM=true +# +# DELETE_USERS=true also removes a person's unused auto-created account (never a +# super_user or anyone who has signed in / has activity). +# +# FORCE=true overrides ALL protections and deletes even people who look real +# (destroying any linked account). Use only for confirmed false positives. +namespace :data do + desc "Remove fake form-submission people and their full data + audit graph (dry run unless CONFIRM=true)" + task remove_fake_submissions: :environment do + parse_ids = ->(raw) { raw.to_s.split(",").map(&:strip).reject(&:blank?).map(&:to_i).uniq } + + person_ids = parse_ids.call(ENV["PERSON_IDS"]) + submission_ids = parse_ids.call(ENV["FORM_SUBMISSION_IDS"]) + confirm = ENV["CONFIRM"] == "true" + + if person_ids.empty? && submission_ids.empty? + abort "Provide PERSON_IDS=1,2,3 and/or FORM_SUBMISSION_IDS=4,5,6" + end + + remover = FakeSubmissionRemover.new( + person_ids: person_ids, + form_submission_ids: submission_ids, + delete_users: ENV["DELETE_USERS"] == "true", + force: ENV["FORCE"] == "true" + ) + + puts "\n⚠️ WARNING: This task is ONLY for removing people created by FAKE form" + puts " submissions. It deletes a person and their entire data + audit graph." + puts " Do NOT use it to delete a regular/real person — verify every id is fake" + puts " against the dry-run output before re-running with CONFIRM=true.\n" + + if remover.missing_form_submission_ids.any? + puts "Warning: FormSubmission ids not found: #{remover.missing_form_submission_ids.sort.join(', ')}" + end + if remover.missing_person_ids.any? + puts "Warning: Person ids not found: #{remover.missing_person_ids.sort.join(', ')}" + end + + if remover.skipped.any? + puts "\nSkipping #{remover.skipped.size} person(s) that look real:" + remover.skipped.each do |skip| + puts " ##{skip.person.id} #{skip.person.full_name_with_email} — #{skip.reasons.join(', ')}" + end + end + + if remover.deletable.empty? + puts "\nNothing to delete." + next + end + + puts "\n#{confirm ? 'DELETING' : 'DRY RUN — would delete'} for #{remover.deletable.size} person(s):" + remover.deletable.each { |person| puts " ##{person.id} #{person.full_name_with_email}" } + puts "\nRecords:" + remover.counts.each { |label, count| puts " #{label.to_s.tr('_', ' ').capitalize.ljust(21)}#{count}" } + + unless confirm + puts "\nDry run only. Re-run with CONFIRM=true to delete." + next + end + + begin + deleted = remover.call + puts "\nDone. Deleted #{deleted.size} person(s) and their data + audit graph." + rescue ActiveRecord::InvalidForeignKey => e + abort "\nAborted (nothing deleted): a linked User Account owns content that " \ + "can't be orphaned (workshops, reports, stories, etc.). Reassign or " \ + "remove it first.\n#{e.message}" + end + end +end diff --git a/spec/policies/person_policy_spec.rb b/spec/policies/person_policy_spec.rb index c299741f0f..1efd3fb1a4 100644 --- a/spec/policies/person_policy_spec.rb +++ b/spec/policies/person_policy_spec.rb @@ -91,6 +91,20 @@ def policy_for(record: nil, user:) end end + describe "#purge?" do + context "with admin user" do + subject { policy_for(record: searchable_person, user: admin_user) } + + it { is_expected.to be_allowed_to(:purge?) } + end + + context "with regular user" do + subject { policy_for(record: searchable_person, user: regular_user) } + + it { is_expected.not_to be_allowed_to(:purge?) } + end + end + describe "#edit?" do context "with admin user" do subject { policy_for(record: searchable_person, user: admin_user) } diff --git a/spec/requests/people_purge_spec.rb b/spec/requests/people_purge_spec.rb new file mode 100644 index 0000000000..f956dba29e --- /dev/null +++ b/spec/requests/people_purge_spec.rb @@ -0,0 +1,136 @@ +require "rails_helper" + +RSpec.describe "People purge", type: :request do + let(:admin) { create(:user, :admin) } + let(:regular_user) { create(:user, :with_person) } + let!(:person) { create(:person, user: nil) } + let!(:submission) { create(:form_submission, person: person) } + + describe "GET /people/:id/purge_confirmation (preview)" do + context "as an admin" do + before { sign_in admin } + + it "shows the inventory and a delete button for a fake person" do + get purge_confirmation_person_path(person) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("What will be deleted") + expect(response.body).to include("Permanently delete") + end + + it "shows the inventory and an override (not a direct delete) for a real person" do + real = create(:person) # factory gives them a User account + get purge_confirmation_person_path(real) + + expect(response.body).to include("looks real") + expect(response.body).to include("What will be deleted") + expect(response.body).to include("Delete anyway") + expect(response.body).not_to include("Permanently delete") + end + + it "shows the override warning and a delete button for a real person when force=true" do + real = create(:person) + get purge_confirmation_person_path(real, force: true) + + expect(response.body).to include("Override") + expect(response.body).to include("Permanently delete") + end + + it "shows blocking content and no delete button when the account authored content" do + real = create(:person) + create(:workshop, created_by: real.user) + + get purge_confirmation_person_path(real, force: true) + + expect(response.body).to include("Cannot delete") + expect(response.body).to include("Workshops") + expect(response.body).not_to include("Permanently delete") + end + end + + context "as a regular user" do + before { sign_in regular_user } + + it "is forbidden" do + get purge_confirmation_person_path(person) + expect(response).not_to have_http_status(:ok) + end + end + end + + describe "DELETE /people/:id/purge" do + context "as an admin" do + before { sign_in admin } + + it "deletes the fake person and their data" do + delete purge_person_path(person) + + expect(response).to redirect_to(people_path) + expect(Person.exists?(person.id)).to be false + expect(FormSubmission.exists?(submission.id)).to be false + end + + it "skips a person who looks real and redirects back with an alert" do + real = create(:person) + delete purge_person_path(real) + + expect(response).to redirect_to(edit_person_path(real)) + expect(flash[:alert]).to be_present + expect(Person.exists?(real.id)).to be true + end + + it "deletes a person who looks real when force=true" do + real = create(:person) + real.user.update!(super_user: true) + + delete purge_person_path(real, force: true) + + expect(response).to redirect_to(people_path) + expect(Person.exists?(real.id)).to be false + end + + it "fails gracefully when the account owns FK-restricted content" do + real = create(:person) + create(:workshop, created_by: real.user) + + delete purge_person_path(real, force: true) + + expect(response).to redirect_to(edit_person_path(real)) + expect(flash[:alert]).to include("reassigned or removed") + expect(Person.exists?(real.id)).to be true + end + end + + context "as a regular user" do + before { sign_in regular_user } + + it "is forbidden and does not delete" do + delete purge_person_path(person) + + expect(response).not_to have_http_status(:ok) + expect(Person.exists?(person.id)).to be true + end + end + + context "as a visitor" do + it "redirects to sign in" do + delete purge_person_path(person) + + expect(response).to redirect_to(new_user_session_path) + expect(Person.exists?(person.id)).to be true + end + end + end + + describe "the edit-page button" do + before { sign_in admin } + + it "appears only when admin=true is in the query" do + get edit_person_path(person, admin: "true") + expect(response.body).to include("Delete person & all data") + + get edit_person_path(person) + expect(response.body).not_to include("Delete person & all data") + end + end +end diff --git a/spec/services/fake_submission_remover_spec.rb b/spec/services/fake_submission_remover_spec.rb new file mode 100644 index 0000000000..21ca49a171 --- /dev/null +++ b/spec/services/fake_submission_remover_spec.rb @@ -0,0 +1,255 @@ +require "rails_helper" + +RSpec.describe FakeSubmissionRemover do + # The :person factory associates a User by default; real fake-form people have + # none, so deletable candidates are built with user: nil. + + describe "#call deleting by person id" do + let!(:person) { create(:person, user: nil) } + let!(:other_person) { create(:person, user: nil) } + + let!(:submission) { create(:form_submission, person: person) } + let!(:form_field) { create(:form_field, form: submission.form) } + let!(:answer) { create(:form_answer, form_submission: submission, form_field: form_field) } + let!(:affiliation) { create(:affiliation, person: person) } + let!(:registration) { create(:event_registration, registrant: person) } + let!(:scholarship) { create(:scholarship, recipient: person) } + + # Records NOT cascaded by Person.destroy. + let!(:payment) { create(:payment, person: person, form_submission: submission) } + let!(:refund) { create(:refund, refundable: payment) } + let!(:scholarship_allocation) { create(:allocation, source: scholarship, allocatable: registration) } + let!(:bulk_notification) do + create(:notification, noticeable: submission, kind: :bulk_payment_confirmation, recipient_role: :person) + end + let!(:pay_customer) do + Pay::Customer.create!(owner: person, processor: "stripe", processor_id: "cus_fake_#{person.id}") + end + + # Audit trail for the removed records. + let!(:payment_version) { PaperTrail::Version.create!(item_type: "Payment", item_id: payment.id, event: "create") } + let!(:person_event) { create(:ahoy_event, name: "create.person", resource_type: "Person", resource_id: person.id) } + let!(:submission_event) do + create(:ahoy_event, name: "create.form_submission", resource_type: "FormSubmission", resource_id: submission.id) + end + + # An unrelated person's identical graph must survive untouched. + let!(:other_submission) { create(:form_submission, person: other_person) } + let!(:other_payment) { create(:payment, person: other_person) } + let!(:other_event) { create(:ahoy_event, name: "create.person", resource_type: "Person", resource_id: other_person.id) } + + def remove(**opts) + described_class.new(person_ids: [ person.id ], **opts).call + end + + it "returns the deleted person ids" do + expect(remove).to eq [ person.id ] + end + + it "destroys the person and the full cascaded graph" do + remove + + expect(Person.exists?(person.id)).to be false + expect(FormSubmission.exists?(submission.id)).to be false + expect(FormAnswer.exists?(answer.id)).to be false + expect(Affiliation.exists?(affiliation.id)).to be false + expect(EventRegistration.exists?(registration.id)).to be false + expect(Scholarship.exists?(scholarship.id)).to be false + end + + it "destroys records not cascaded by Person.destroy" do + remove + + expect(Payment.exists?(payment.id)).to be false + expect(Refund.exists?(refund.id)).to be false + expect(Allocation.exists?(scholarship_allocation.id)).to be false + expect(Notification.exists?(bulk_notification.id)).to be false + expect(Pay::Customer.exists?(pay_customer.id)).to be false + end + + it "deletes the PaperTrail and Ahoy audit trail for the removed records" do + remove + + expect(PaperTrail::Version.exists?(payment_version.id)).to be false + expect(Ahoy::Event.exists?(person_event.id)).to be false + expect(Ahoy::Event.exists?(submission_event.id)).to be false + end + + it "leaves unrelated people and their data untouched" do + remove + + expect(Person.exists?(other_person.id)).to be true + expect(FormSubmission.exists?(other_submission.id)).to be true + expect(Payment.exists?(other_payment.id)).to be true + expect(Ahoy::Event.exists?(other_event.id)).to be true + end + end + + describe "#call resolving by form submission id" do + let!(:person) { create(:person, user: nil) } + let!(:submission) { create(:form_submission, person: person) } + + it "resolves the owning person and deletes them" do + described_class.new(form_submission_ids: [ submission.id ]).call + + expect(Person.exists?(person.id)).to be false + expect(FormSubmission.exists?(submission.id)).to be false + end + end + + describe "protecting real-looking people" do + it "skips (does not delete) a person with a linked user account" do + person = create(:person) + create(:form_submission, person: person) + + remover = described_class.new(person_ids: [ person.id ]) + + expect(remover.call).to be_empty + expect(Person.exists?(person.id)).to be true + expect(remover.skipped.map { |s| s.person.id }).to include(person.id) + end + + it "skips a person who is a grant donor" do + person = create(:person, user: nil) + create(:grant, :donated_by_person, donor: person) + + described_class.new(person_ids: [ person.id ]).call + + expect(Person.exists?(person.id)).to be true + end + end + + describe "delete_users" do + it "destroys an unused auto-created account along with the person" do + person = create(:person) + user = person.user # factory default: not super_user, never signed in + + described_class.new(person_ids: [ person.id ], delete_users: true).call + + expect(Person.exists?(person.id)).to be false + expect(User.exists?(user.id)).to be false + end + + it "still protects a super_user" do + person = create(:person) + person.user.update!(super_user: true) + + described_class.new(person_ids: [ person.id ], delete_users: true).call + + expect(Person.exists?(person.id)).to be true + end + + it "still protects a user who has signed in" do + person = create(:person) + person.user.update!(sign_in_count: 3, current_sign_in_at: Time.current) + + described_class.new(person_ids: [ person.id ], delete_users: true).call + + expect(Person.exists?(person.id)).to be true + end + + it "protects a user with Ahoy activity of its own" do + person = create(:person) + create(:ahoy_event, user: person.user, name: "view.workshop") + + described_class.new(person_ids: [ person.id ], delete_users: true).call + + expect(Person.exists?(person.id)).to be true + end + end + + describe "force: true (override)" do + it "deletes a person with an active/admin account and destroys the account" do + person = create(:person) + person.user.update!(super_user: true) + user = person.user + + described_class.new(person_ids: [ person.id ], force: true).call + + expect(Person.exists?(person.id)).to be false + expect(User.exists?(user.id)).to be false + end + + it "deletes a grant donor that would otherwise be protected" do + person = create(:person, user: nil) + create(:grant, :donated_by_person, donor: person) + + described_class.new(person_ids: [ person.id ], force: true).call + + expect(Person.exists?(person.id)).to be false + end + end + + describe "#protection_reasons" do + it "lists why a person looks real, regardless of force" do + person = create(:person) # factory gives a User account + create(:grant, :donated_by_person, donor: person) + + reasons = described_class.new(person_ids: [ person.id ], force: true).protection_reasons(person) + + expect(reasons).to include(a_string_matching(/User account/)) + expect(reasons).to include("is a grant donor") + end + + it "is empty for a plain fake person" do + person = create(:person, user: nil) + + expect(described_class.new(person_ids: [ person.id ]).protection_reasons(person)).to be_empty + end + end + + describe "account content (force)" do + it "#blocking_account_content lists FK-restricted content owned by the account" do + person = create(:person) + user = person.user + create(:workshop, created_by: user) + create(:story, created_by: user) + + blocking = described_class.new(person_ids: [ person.id ], force: true).blocking_account_content + + expect(blocking["Workshops"]).to eq 1 + expect(blocking["Stories"]).to eq 1 + expect(blocking).not_to have_key("Reports") + end + + it "#cascading_account_content lists records removed with the account" do + person = create(:person) + create(:bookmark, user: person.user) + + cascading = described_class.new(person_ids: [ person.id ], force: true).cascading_account_content + + expect(cascading["Bookmarks"]).to eq 1 + end + + it "is empty when there is no linked account" do + person = create(:person, user: nil) + remover = described_class.new(person_ids: [ person.id ], force: true) + + expect(remover.blocking_account_content).to be_empty + expect(remover.cascading_account_content).to be_empty + end + + it "raises InvalidForeignKey rather than orphaning content when forced" do + person = create(:person) + create(:workshop, created_by: person.user) + + expect do + described_class.new(person_ids: [ person.id ], force: true).call + end.to raise_error(ActiveRecord::InvalidForeignKey) + expect(Person.exists?(person.id)).to be true + end + end + + describe "#counts" do + it "reports per-record-type counts without mutating anything" do + person = create(:person, user: nil) + create(:form_submission, person: person) + + remover = described_class.new(person_ids: [ person.id ]) + + expect(remover.counts[:people]).to eq 1 + expect(remover.counts[:form_submissions]).to eq 1 + expect(Person.exists?(person.id)).to be true + end + end +end diff --git a/spec/tasks/remove_fake_submissions_spec.rb b/spec/tasks/remove_fake_submissions_spec.rb new file mode 100644 index 0000000000..6080ea3716 --- /dev/null +++ b/spec/tasks/remove_fake_submissions_spec.rb @@ -0,0 +1,52 @@ +require "rails_helper" +require "rake" + +# Behaviour is covered in spec/services/fake_submission_remover_spec.rb. These +# specs just confirm the rake task wires ENV → FakeSubmissionRemover correctly. +RSpec.describe "data:remove_fake_submissions" do + before(:all) do + Rails.application.load_tasks unless Rake::Task.task_defined?("data:remove_fake_submissions") + end + + before { Rake::Task["data:remove_fake_submissions"].reenable } + + def run_task(env = {}) + keys = %w[PERSON_IDS FORM_SUBMISSION_IDS CONFIRM DELETE_USERS FORCE] + saved = keys.index_with { |k| ENV[k] } + keys.each { |k| ENV.delete(k) } + env.each { |k, v| ENV[k.to_s] = v.to_s } + suppress_output { Rake::Task["data:remove_fake_submissions"].invoke } + ensure + Rake::Task["data:remove_fake_submissions"].reenable + keys.each { |k| ENV[k] = saved[k] } + end + + def suppress_output + original_stdout = $stdout + $stdout = StringIO.new + yield + ensure + $stdout = original_stdout + end + + it "aborts when no ids are given" do + expect { run_task }.to raise_error(SystemExit) + end + + it "deletes nothing without CONFIRM (dry run)" do + person = create(:person, user: nil) + create(:form_submission, person: person) + + expect { run_task(PERSON_IDS: person.id) }.not_to change(Person, :count) + expect(Person.exists?(person.id)).to be true + end + + it "deletes the targeted person with CONFIRM=true" do + person = create(:person, user: nil) + create(:form_submission, person: person) + + run_task(PERSON_IDS: person.id, CONFIRM: true) + + expect(Person.exists?(person.id)).to be false + end +end diff --git a/spec/views/page_bg_class_alignment_spec.rb b/spec/views/page_bg_class_alignment_spec.rb index 279b99147f..d9b32da5dd 100644 --- a/spec/views/page_bg_class_alignment_spec.rb +++ b/spec/views/page_bg_class_alignment_spec.rb @@ -149,6 +149,7 @@ "app/views/organizations/new.html.erb" => "admin-only bg-blue-100", "app/views/organization_statuses/new.html.erb" => "admin-only bg-blue-100", "app/views/people/new.html.erb" => "admin-only bg-blue-100", + "app/views/people/purge_confirmation.html.erb" => "admin-only bg-blue-100", "app/views/quotes/new.html.erb" => "admin-only bg-blue-100", "app/views/resources/new.html.erb" => "admin-only bg-blue-100", "app/views/sectors/new.html.erb" => "admin-only bg-blue-100", From f3fbb4c794f49a839f84584e84cc698185bf46c0 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 12:33:27 -0400 Subject: [PATCH 2/8] Generalize fake-submission remover into PeopleRemover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deletion mechanism is general — it removes a person and their entire data graph regardless of how the person was created — so the fake-submission framing was too narrow now that we need to delete people going forward. Rename FakeSubmissionRemover -> PeopleRemover and data:remove_fake_submissions -> data:remove_people, and generalize the wording. Behaviour is unchanged for the person graph (already cascaded everything a person owns) and the protections. Also close a user-side orphan gap: a destroyed account's own notifications (noticeable: User) don't cascade off the user, so they're now removed explicitly and surfaced in the purge preview's "also removed with the account" list. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 4 +- app/controllers/people_controller.rb | 6 +-- app/policies/person_policy.rb | 2 +- ...ubmission_remover.rb => people_remover.rb} | 44 +++++++++++-------- ...ke_submissions.rake => remove_people.rake} | 28 ++++++------ ...remover_spec.rb => people_remover_spec.rb} | 13 +++++- ...missions_spec.rb => remove_people_spec.rb} | 14 +++--- 7 files changed, 66 insertions(+), 45 deletions(-) rename app/services/{fake_submission_remover.rb => people_remover.rb} (86%) rename lib/tasks/{remove_fake_submissions.rake => remove_people.rake} (71%) rename spec/services/{fake_submission_remover_spec.rb => people_remover_spec.rb} (95%) rename spec/tasks/{remove_fake_submissions_spec.rb => remove_people_spec.rb} (72%) diff --git a/AGENTS.md b/AGENTS.md index e6a92cf56a..79506025ba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -185,7 +185,7 @@ end - `WorkshopVariationFromIdeaService` — Variation creation from ideas - `TaggingSearchService` — Search and filter tagging data - `PersonFromUserService` — Create Person from User account -- `FakeSubmissionRemover` — Deletes a set of people and their full data + audit (PaperTrail/Ahoy) graph; protects real/admin accounts (override with `force`). Built for fake-submission cleanup but general-purpose. Used by the `data:remove_fake_submissions` rake task and the admin-only "Delete person & all data" purge button on person edit (`purge_confirmation`/`purge` actions) +- `PeopleRemover` — Deletes a set of people and ALL their associated data, their linked User + the user's own data where relevant (real authored content is FK-restricted and blocks), plus the full PaperTrail/Ahoy audit graph; protects real/admin accounts (override with `force`). Used by the `data:remove_people` rake task and the admin-only "Delete person & all data" purge button on person edit (`purge_confirmation`/`purge` actions) - `BulkInviteService` — Bulk send welcome instructions and reset created_at for users - `FormBuilderService` — Builds configurable forms from composable sections with per-field visibility - `ModelDeduper` — Deduplication logic @@ -419,4 +419,4 @@ Located in `lib/tasks/` (9 files): - `rhino_migrator.rake` — Rich text editor migration - `attachment_report.rake` — Attachment reporting - `migrate_internal_id_to_filemaker_code.rake` — FileMaker code migration -- `remove_fake_submissions.rake` (`data:remove_fake_submissions`) — Removes fake form-submission people via `FakeSubmissionRemover` (dry run unless `CONFIRM=true`) +- `remove_people.rake` (`data:remove_people`) — Removes fake form-submission people via `PeopleRemover` (dry run unless `CONFIRM=true`) diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 7512d52c29..828299ae2a 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -234,7 +234,7 @@ def purge_confirmation @force = params[:force] == "true" # force: true so the inventory reflects the full graph that WOULD be deleted, # even when the person looks real — the page always shows what's at stake. - preview = FakeSubmissionRemover.new(person_ids: [ @person.id ], force: true) + preview = PeopleRemover.new(person_ids: [ @person.id ], force: true) @protections = preview.protection_reasons(@person) @counts = preview.counts # The linked account's own content — only relevant when overriding, since that's @@ -247,10 +247,10 @@ def purge_confirmation # Admin-only deletion of a person and their whole data + audit graph. Skips a # person who looks real unless force=true overrides the protections (see - # FakeSubmissionRemover). + # PeopleRemover). def purge authorize! @person, to: :purge? - remover = FakeSubmissionRemover.new(person_ids: [ @person.id ], force: params[:force] == "true") + remover = PeopleRemover.new(person_ids: [ @person.id ], force: params[:force] == "true") if remover.call.include?(@person.id) redirect_to people_path, status: :see_other, diff --git a/app/policies/person_policy.rb b/app/policies/person_policy.rb index d555d2ceb7..59efe126f6 100644 --- a/app/policies/person_policy.rb +++ b/app/policies/person_policy.rb @@ -31,7 +31,7 @@ def destroy? # Full purge of a person and their entire data graph. Bypasses the # has_associated_data? guard on destroy? on purpose — purging exists precisely - # to remove people who have that data. The FakeSubmissionRemover service still + # to remove people who have that data. The PeopleRemover service still # refuses to touch anyone who looks real unless explicitly forced. def purge? admin? diff --git a/app/services/fake_submission_remover.rb b/app/services/people_remover.rb similarity index 86% rename from app/services/fake_submission_remover.rb rename to app/services/people_remover.rb index 37a29a5f68..2e4be4eb0c 100644 --- a/app/services/fake_submission_remover.rb +++ b/app/services/people_remover.rb @@ -1,29 +1,33 @@ -# Deletes a set of people along with their entire data graph and audit trail. -# Built for cleaning up FAKE form submissions (hence the name and the protections -# below), but the mechanism is general — it backs both the -# data:remove_fake_submissions rake task and the admin-only "Delete person & all -# data" button on the person edit page. When general person-deletion grows beyond -# the fake-data use case, rename this to something like PersonPurger. +# Deletes a set of people along with ALL of their associated data, their linked +# User account and the user's own data (where relevant), and the audit trail for +# everything removed. Backs both the data:remove_people rake task and +# the admin-only "Delete person & all data" button on the person edit page. # -# Person.destroy cascades the submissions, answers, affiliations, registrations -# and scholarships (see Person). This also tears down what does NOT cascade: -# payments (+ their refunds and allocations), bulk-payment notifications, Stripe -# Pay::Customer rows, and the PaperTrail + Ahoy audit trail for every record -# removed. +# Person.destroy cascades everything a person owns — submissions, answers, +# affiliations, registrations, scholarships, addresses, contact methods, +# bookmarks, comments, sector/category tags, grants, event staffing. This also +# tears down what does NOT cascade off Person: payments (+ their refunds and +# allocations), bulk-payment notifications, Stripe Pay::Customer rows, and the +# PaperTrail + Ahoy audit trail for every record removed. +# +# The linked User and its own data (bookmarks, comments, user_forms, the user's +# notifications, Ahoy activity) are removed too — but only "where relevant": a +# user that authored real content (workshops, reports, stories, ideas — see +# User#deletable?) is FK-restricted by the database, which blocks the delete +# rather than orphaning that content. # # People that look real are SKIPPED, never deleted: a grant donor, a spotlighted # facilitator, or anyone with a User account — unless delete_users: true AND the # account is clearly an unused auto-created one (never signed in, not a # super_user, no Ahoy activity of its own, authored no other people). A real -# admin is always protected. This is ONLY for fake data — never use it to delete -# a regular person. +# admin is always protected. # # force: true is a deliberate escape hatch that ignores ALL of the protections # above and deletes anyway (destroying any linked account). Use only when you are # certain the heuristic is a false positive. The database still refuses to orphan -# real authored content (workshops, reports, stories are FK-restricted), so a -# force delete of an account that authored content fails and rolls back. -class FakeSubmissionRemover +# real authored content, so a force delete of an account that authored content +# fails and rolls back. +class PeopleRemover Skip = Data.define(:person, :reasons) # Only these models carry has_paper_trail. @@ -89,14 +93,16 @@ def blocking_account_content }.select { |_, count| count.positive? } end - # Records owned by the User account that ARE removed with it (dependent: destroy). + # Records owned by the User account that ARE removed with it (cascade, or cleaned + # up explicitly like the user's own notifications). def cascading_account_content user_ids = collected[:users].map(&:id) return {} if user_ids.empty? { "Bookmarks" => Bookmark.where(user_id: user_ids).count, - "User forms" => UserForm.where(user_id: user_ids).count + "User forms" => UserForm.where(user_id: user_ids).count, + "Notifications" => Notification.where(noticeable_type: "User", noticeable_id: user_ids).count }.select { |_, count| count.positive? } end @@ -137,6 +143,8 @@ def call Payment.where(id: c[:payment_ids]).find_each(&:destroy!) Notification.where(id: c[:notification_ids]).find_each(&:destroy!) Pay::Customer.where(id: c[:pay_customer_ids]).find_each(&:destroy!) + # The user's own notifications (noticeable: User) don't cascade off the user. + Notification.where(noticeable_type: "User", noticeable_id: c[:users].map(&:id)).delete_all c[:users].each(&:destroy!) deletable.each { |person| person.association(:user).reset } deletable.each(&:destroy!) diff --git a/lib/tasks/remove_fake_submissions.rake b/lib/tasks/remove_people.rake similarity index 71% rename from lib/tasks/remove_fake_submissions.rake rename to lib/tasks/remove_people.rake index 59d58c6a4a..71add1386b 100644 --- a/lib/tasks/remove_fake_submissions.rake +++ b/lib/tasks/remove_people.rake @@ -1,15 +1,17 @@ # frozen_string_literal: true -# Remove fake form-submission test data. Thin wrapper around FakeSubmissionRemover -# (app/services/fake_submission_remover.rb) — see it for the full deletion graph -# and the rules that protect real people. +# Permanently remove people and ALL their associated data. Thin wrapper around +# PeopleRemover (app/services/people_remover.rb) — see it for the full deletion +# graph and the rules that protect real people. The original use case is cleaning +# up fake form-submission test data; FORM_SUBMISSION_IDS resolves those to their +# owning people. # # Usage (dry run — prints what WOULD be deleted, changes nothing): -# bin/rails data:remove_fake_submissions PERSON_IDS=12,34,56 -# bin/rails data:remove_fake_submissions FORM_SUBMISSION_IDS=78,90 +# bin/rails data:remove_people PERSON_IDS=12,34,56 +# bin/rails data:remove_people FORM_SUBMISSION_IDS=78,90 # # To actually delete, add CONFIRM=true: -# bin/rails data:remove_fake_submissions PERSON_IDS=12,34 CONFIRM=true +# bin/rails data:remove_people PERSON_IDS=12,34 CONFIRM=true # # DELETE_USERS=true also removes a person's unused auto-created account (never a # super_user or anyone who has signed in / has activity). @@ -17,8 +19,8 @@ # FORCE=true overrides ALL protections and deletes even people who look real # (destroying any linked account). Use only for confirmed false positives. namespace :data do - desc "Remove fake form-submission people and their full data + audit graph (dry run unless CONFIRM=true)" - task remove_fake_submissions: :environment do + desc "Permanently remove people and their full data + audit graph (dry run unless CONFIRM=true)" + task remove_people: :environment do parse_ids = ->(raw) { raw.to_s.split(",").map(&:strip).reject(&:blank?).map(&:to_i).uniq } person_ids = parse_ids.call(ENV["PERSON_IDS"]) @@ -29,17 +31,17 @@ namespace :data do abort "Provide PERSON_IDS=1,2,3 and/or FORM_SUBMISSION_IDS=4,5,6" end - remover = FakeSubmissionRemover.new( + remover = PeopleRemover.new( person_ids: person_ids, form_submission_ids: submission_ids, delete_users: ENV["DELETE_USERS"] == "true", force: ENV["FORCE"] == "true" ) - puts "\n⚠️ WARNING: This task is ONLY for removing people created by FAKE form" - puts " submissions. It deletes a person and their entire data + audit graph." - puts " Do NOT use it to delete a regular/real person — verify every id is fake" - puts " against the dry-run output before re-running with CONFIRM=true.\n" + puts "\n⚠️ WARNING: This permanently deletes each person and their ENTIRE data +" + puts " audit graph (and their User account where applicable). This cannot be" + puts " undone — verify every id against the dry-run output before re-running" + puts " with CONFIRM=true.\n" if remover.missing_form_submission_ids.any? puts "Warning: FormSubmission ids not found: #{remover.missing_form_submission_ids.sort.join(', ')}" diff --git a/spec/services/fake_submission_remover_spec.rb b/spec/services/people_remover_spec.rb similarity index 95% rename from spec/services/fake_submission_remover_spec.rb rename to spec/services/people_remover_spec.rb index 21ca49a171..38080f5cc2 100644 --- a/spec/services/fake_submission_remover_spec.rb +++ b/spec/services/people_remover_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe FakeSubmissionRemover do +RSpec.describe PeopleRemover do # The :person factory associates a User by default; real fake-form people have # none, so deletable candidates are built with user: nil. @@ -178,6 +178,17 @@ def remove(**opts) expect(Person.exists?(person.id)).to be false end + + it "removes the user's own notifications when the account is destroyed" do + person = create(:person) + user = person.user + notification = create(:notification, noticeable: user) + + described_class.new(person_ids: [ person.id ], force: true).call + + expect(User.exists?(user.id)).to be false + expect(Notification.exists?(notification.id)).to be false + end end describe "#protection_reasons" do diff --git a/spec/tasks/remove_fake_submissions_spec.rb b/spec/tasks/remove_people_spec.rb similarity index 72% rename from spec/tasks/remove_fake_submissions_spec.rb rename to spec/tasks/remove_people_spec.rb index 6080ea3716..9c6a8b815e 100644 --- a/spec/tasks/remove_fake_submissions_spec.rb +++ b/spec/tasks/remove_people_spec.rb @@ -1,23 +1,23 @@ require "rails_helper" require "rake" -# Behaviour is covered in spec/services/fake_submission_remover_spec.rb. These -# specs just confirm the rake task wires ENV → FakeSubmissionRemover correctly. -RSpec.describe "data:remove_fake_submissions" do +# Behaviour is covered in spec/services/people_remover_spec.rb. These specs just +# confirm the rake task wires ENV → PeopleRemover correctly. +RSpec.describe "data:remove_people" do before(:all) do - Rails.application.load_tasks unless Rake::Task.task_defined?("data:remove_fake_submissions") + Rails.application.load_tasks unless Rake::Task.task_defined?("data:remove_people") end - before { Rake::Task["data:remove_fake_submissions"].reenable } + before { Rake::Task["data:remove_people"].reenable } def run_task(env = {}) keys = %w[PERSON_IDS FORM_SUBMISSION_IDS CONFIRM DELETE_USERS FORCE] saved = keys.index_with { |k| ENV[k] } keys.each { |k| ENV.delete(k) } env.each { |k, v| ENV[k.to_s] = v.to_s } - suppress_output { Rake::Task["data:remove_fake_submissions"].invoke } + suppress_output { Rake::Task["data:remove_people"].invoke } ensure - Rake::Task["data:remove_fake_submissions"].reenable + Rake::Task["data:remove_people"].reenable keys.each { |k| ENV[k] = saved[k] } end From 9ccc4bb119227ed51ba26a3f1c64650b6dbbf3b5 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 12:47:15 -0400 Subject: [PATCH 3/8] Force-delete the account's authored content instead of blocking on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full purge should remove the person's account AND everything it created, not refuse because the account authored a story or workshop. Override (force) now destroys the authored content — workshops, logs, reports, resources, stories, ideas, variations — in FK-safe order before the account, and the user's own notifications too. The purge preview folds the former "blocking" and "cascading" lists into one "Also removed with the User Account" table so the operator sees the full blast radius, and the override link reads "Preview full delete". The FK rescue stays as a backstop for records the account merely references but doesn't own (a shared banner or event it created), which still block rather than being deleted or rewritten. Co-Authored-By: Claude Opus 4.8 --- app/controllers/people_controller.rb | 10 +-- app/services/people_remover.rb | 83 ++++++++++---------- app/views/people/purge_confirmation.html.erb | 29 ++----- spec/requests/people_purge_spec.rb | 27 +++---- spec/services/people_remover_spec.rb | 45 +++++------ 5 files changed, 84 insertions(+), 110 deletions(-) diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 828299ae2a..18b9d2d84c 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -237,11 +237,9 @@ def purge_confirmation preview = PeopleRemover.new(person_ids: [ @person.id ], force: true) @protections = preview.protection_reasons(@person) @counts = preview.counts - # The linked account's own content — only relevant when overriding, since that's - # when the account itself gets destroyed. Blocking content makes the delete - # impossible; cascading content is removed with the account. - @account_blocking = @force ? preview.blocking_account_content : {} - @account_cascading = @force ? preview.cascading_account_content : {} + # Everything removed with the linked account — only relevant when overriding, + # since that's when the account itself (and its content) gets destroyed. + @account_content = @force ? preview.account_content : {} @person = @person.decorate end @@ -262,7 +260,7 @@ def purge end rescue ActiveRecord::InvalidForeignKey redirect_to edit_person_path(@person), status: :see_other, - alert: "Couldn't delete #{@person.full_name} — the linked User Account owns content (e.g. workshops, reports, or stories) that must be reassigned or removed first." + alert: "Couldn't delete #{@person.full_name} — the linked User Account is still referenced by records it doesn't own (e.g. a shared banner or event it created). Reassign those first." end def check_duplicates diff --git a/app/services/people_remover.rb b/app/services/people_remover.rb index 2e4be4eb0c..6e1bc403e9 100644 --- a/app/services/people_remover.rb +++ b/app/services/people_remover.rb @@ -10,11 +10,11 @@ # allocations), bulk-payment notifications, Stripe Pay::Customer rows, and the # PaperTrail + Ahoy audit trail for every record removed. # -# The linked User and its own data (bookmarks, comments, user_forms, the user's -# notifications, Ahoy activity) are removed too — but only "where relevant": a -# user that authored real content (workshops, reports, stories, ideas — see -# User#deletable?) is FK-restricted by the database, which blocks the delete -# rather than orphaning that content. +# The linked User and ALL of its own data are removed too: bookmarks, comments, +# user_forms, the user's notifications, Ahoy activity, AND everything it authored +# (workshops, workshop logs, reports, resources, stories, and the various ideas +# and variations). The purge preview lists all of this so the operator sees the +# full blast radius before confirming. # # People that look real are SKIPPED, never deleted: a grant donor, a spotlighted # facilitator, or anyone with a User account — unless delete_users: true AND the @@ -23,10 +23,10 @@ # admin is always protected. # # force: true is a deliberate escape hatch that ignores ALL of the protections -# above and deletes anyway (destroying any linked account). Use only when you are -# certain the heuristic is a false positive. The database still refuses to orphan -# real authored content, so a force delete of an account that authored content -# fails and rolls back. +# above and deletes anyway — the account and everything it authored. Records that +# merely *reference* the account without being owned by it (e.g. it created a +# shared banner or event) are FK-restricted; the transaction rolls back and the +# caller surfaces a clear error rather than orphaning or rewriting them. class PeopleRemover Skip = Data.define(:person, :reasons) @@ -71,39 +71,18 @@ def missing_form_submission_ids @form_submission_ids - FormSubmission.where(id: @form_submission_ids).pluck(:id) end - # Content authored by the User account(s) that would be destroyed in a force - # delete and which the database REFUSES to orphan (created_by_id is FK-restricted - # with no dependent). If any of these exist the delete cannot proceed — the - # content must be reassigned or removed first. - def blocking_account_content + # Everything removed along with the destroyed User account(s) — its authored + # content plus the records that cascade with it — for the purge preview. Empty + # when no account is involved. + def account_content user_ids = collected[:users].map(&:id) return {} if user_ids.empty? - { - "Workshops" => Workshop.where(created_by_id: user_ids).count, - "Workshop logs" => WorkshopLog.where(created_by_id: user_ids).count, - "Workshop ideas" => WorkshopIdea.where(created_by_id: user_ids).count, - "Workshop variations" => WorkshopVariation.where(created_by_id: user_ids).count, - "Workshop variation ideas" => WorkshopVariationIdea.where(created_by_id: user_ids).count, - "Reports" => Report.where(created_by_id: user_ids).count, - "Resources" => Resource.where(created_by_id: user_ids).count, - "Stories" => Story.where(created_by_id: user_ids).count, - "Story ideas" => StoryIdea.where(created_by_id: user_ids).count, - "Comments" => Comment.where(created_by_id: user_ids).count - }.select { |_, count| count.positive? } - end - - # Records owned by the User account that ARE removed with it (cascade, or cleaned - # up explicitly like the user's own notifications). - def cascading_account_content - user_ids = collected[:users].map(&:id) - return {} if user_ids.empty? - - { - "Bookmarks" => Bookmark.where(user_id: user_ids).count, - "User forms" => UserForm.where(user_id: user_ids).count, - "Notifications" => Notification.where(noticeable_type: "User", noticeable_id: user_ids).count - }.select { |_, count| count.positive? } + counts = authored_content_scopes(user_ids).to_h { |label, relation| [ label, relation.count ] } + counts["Bookmarks"] = Bookmark.where(user_id: user_ids).count + counts["User forms"] = UserForm.where(user_id: user_ids).count + counts["Notifications"] = Notification.where(noticeable_type: "User", noticeable_id: user_ids).count + counts.select { |_, count| count.positive? } end # Per-record-type counts for a dry-run preview. Does not mutate anything. @@ -143,8 +122,12 @@ def call Payment.where(id: c[:payment_ids]).find_each(&:destroy!) Notification.where(id: c[:notification_ids]).find_each(&:destroy!) Pay::Customer.where(id: c[:pay_customer_ids]).find_each(&:destroy!) - # The user's own notifications (noticeable: User) don't cascade off the user. - Notification.where(noticeable_type: "User", noticeable_id: c[:users].map(&:id)).delete_all + # Destroy everything the account authored (FK-restricted, leaf → root) and + # its own notifications before the account itself — these don't cascade off + # the user, and the FKs would otherwise block its deletion. + user_ids = c[:users].map(&:id) + authored_content_scopes(user_ids).each { |_, relation| relation.find_each(&:destroy!) } + Notification.where(noticeable_type: "User", noticeable_id: user_ids).delete_all c[:users].each(&:destroy!) deletable.each { |person| person.association(:user).reset } deletable.each(&:destroy!) @@ -169,6 +152,24 @@ def people @people ||= Person.where(id: target_person_ids).to_a end + # Content authored by the account(s), as [label, relation] in FK-safe delete + # order (leaf → root). created_by_id-owned with no dependent, so it must be + # destroyed explicitly before the account or its FKs block the deletion. + def authored_content_scopes(user_ids) + [ + [ "Comments", Comment.where(created_by_id: user_ids) ], + [ "Story ideas", StoryIdea.where(created_by_id: user_ids) ], + [ "Workshop ideas", WorkshopIdea.where(created_by_id: user_ids) ], + [ "Workshop variation ideas", WorkshopVariationIdea.where(created_by_id: user_ids) ], + [ "Workshop variations", WorkshopVariation.where(created_by_id: user_ids) ], + [ "Workshop logs", WorkshopLog.where(created_by_id: user_ids) ], + [ "Stories", Story.where(created_by_id: user_ids) ], + [ "Workshops", Workshop.where(created_by_id: user_ids) ], + [ "Reports", Report.where(created_by_id: user_ids) ], + [ "Resources", Resource.where(created_by_id: user_ids) ] + ] + end + def reasons_for(person) return [] if force diff --git a/app/views/people/purge_confirmation.html.erb b/app/views/people/purge_confirmation.html.erb index 1480119512..82199225da 100644 --- a/app/views/people/purge_confirmation.html.erb +++ b/app/views/people/purge_confirmation.html.erb @@ -21,32 +21,17 @@ <% if @force %>
-

⚠️ Override: deleting anyway despite the protections above.

+

⚠️ Override: deleting despite the protections above.

- The linked User Account will be destroyed too. If they authored real content - (workshops, reports, stories), the database will reject this and nothing - will be deleted. + The linked User Account and everything it created (listed below) will be + permanently deleted too. This cannot be undone.

- <% if @account_blocking.any? %> -
-

Cannot delete: this User Account created content that must be reassigned or removed first.

-
-
- <% @account_blocking.each do |label, count| %> -
"> -
<%= label %>
-
"><%= count.zero? ? "—" : count %>
-
- <% end %> -
- <% end %> - - <% if @account_cascading.any? %> + <% if @account_content.any? %>

Also removed with the User Account

- <% @account_cascading.each do |label, count| %> + <% @account_content.each do |label, count| %>
">
<%= label %>
"><%= count.zero? ? "—" : count %>
@@ -73,10 +58,10 @@
<%= link_to "Cancel", edit_person_path(@person), class: "btn btn-secondary-outline" %> <% if @protections.any? && !@force %> - <%= link_to "Delete anyway (override)", + <%= link_to "Preview full delete", purge_confirmation_person_path(@person, force: true), class: "btn btn-danger-outline" %> - <% elsif @account_blocking.blank? %> + <% else %> <%= button_to "Permanently delete", purge_person_path(@person, force: @force), method: :delete, diff --git a/spec/requests/people_purge_spec.rb b/spec/requests/people_purge_spec.rb index f956dba29e..9a4b002db4 100644 --- a/spec/requests/people_purge_spec.rb +++ b/spec/requests/people_purge_spec.rb @@ -24,27 +24,20 @@ expect(response.body).to include("looks real") expect(response.body).to include("What will be deleted") - expect(response.body).to include("Delete anyway") + expect(response.body).to include("Preview full delete") expect(response.body).not_to include("Permanently delete") end - it "shows the override warning and a delete button for a real person when force=true" do - real = create(:person) - get purge_confirmation_person_path(real, force: true) - - expect(response.body).to include("Override") - expect(response.body).to include("Permanently delete") - end - - it "shows blocking content and no delete button when the account authored content" do + it "shows the override warning, the account's content, and a delete button when force=true" do real = create(:person) create(:workshop, created_by: real.user) get purge_confirmation_person_path(real, force: true) - expect(response.body).to include("Cannot delete") + expect(response.body).to include("Override") + expect(response.body).to include("Also removed with the User Account") expect(response.body).to include("Workshops") - expect(response.body).not_to include("Permanently delete") + expect(response.body).to include("Permanently delete") end end @@ -89,15 +82,15 @@ expect(Person.exists?(real.id)).to be false end - it "fails gracefully when the account owns FK-restricted content" do + it "deletes the account's authored content too when force=true" do real = create(:person) - create(:workshop, created_by: real.user) + workshop = create(:workshop, created_by: real.user) delete purge_person_path(real, force: true) - expect(response).to redirect_to(edit_person_path(real)) - expect(flash[:alert]).to include("reassigned or removed") - expect(Person.exists?(real.id)).to be true + expect(response).to redirect_to(people_path) + expect(Person.exists?(real.id)).to be false + expect(Workshop.exists?(workshop.id)).to be false end end diff --git a/spec/services/people_remover_spec.rb b/spec/services/people_remover_spec.rb index 38080f5cc2..95f81eca2f 100644 --- a/spec/services/people_remover_spec.rb +++ b/spec/services/people_remover_spec.rb @@ -209,45 +209,42 @@ def remove(**opts) end end - describe "account content (force)" do - it "#blocking_account_content lists FK-restricted content owned by the account" do + describe "#account_content (force)" do + it "lists the account's authored content and the records removed with it" do person = create(:person) user = person.user create(:workshop, created_by: user) create(:story, created_by: user) + create(:bookmark, user: user) - blocking = described_class.new(person_ids: [ person.id ], force: true).blocking_account_content + content = described_class.new(person_ids: [ person.id ], force: true).account_content - expect(blocking["Workshops"]).to eq 1 - expect(blocking["Stories"]).to eq 1 - expect(blocking).not_to have_key("Reports") - end - - it "#cascading_account_content lists records removed with the account" do - person = create(:person) - create(:bookmark, user: person.user) - - cascading = described_class.new(person_ids: [ person.id ], force: true).cascading_account_content - - expect(cascading["Bookmarks"]).to eq 1 + expect(content["Workshops"]).to eq 1 + expect(content["Stories"]).to eq 1 + expect(content["Bookmarks"]).to eq 1 + expect(content).not_to have_key("Reports") end it "is empty when there is no linked account" do person = create(:person, user: nil) - remover = described_class.new(person_ids: [ person.id ], force: true) - expect(remover.blocking_account_content).to be_empty - expect(remover.cascading_account_content).to be_empty + expect(described_class.new(person_ids: [ person.id ], force: true).account_content).to be_empty end + end - it "raises InvalidForeignKey rather than orphaning content when forced" do + describe "force delete of an account that authored content" do + it "deletes the person, the account, and everything it authored" do person = create(:person) - create(:workshop, created_by: person.user) + user = person.user + workshop = create(:workshop, created_by: user) + story = create(:story, created_by: user) - expect do - described_class.new(person_ids: [ person.id ], force: true).call - end.to raise_error(ActiveRecord::InvalidForeignKey) - expect(Person.exists?(person.id)).to be true + described_class.new(person_ids: [ person.id ], force: true).call + + expect(Person.exists?(person.id)).to be false + expect(User.exists?(user.id)).to be false + expect(Workshop.exists?(workshop.id)).to be false + expect(Story.exists?(story.id)).to be false end end From 035acb0629e1ce9bc2a74c18a25337fa8b0f1cac Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 13:03:03 -0400 Subject: [PATCH 4/8] Delete the person before its user to break the audit-column FK cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A purge was rolling back (and bouncing back to edit) whenever a person's own created_by_id/updated_by_id pointed at its linked user — which is the norm for in-app-created people. We destroyed the user first (to dodge the forbidden has_one :user nullify), but the person still referenced that user via people.created_by_id/updated_by_id and affiliations.user_id (all FK-restricted), so the user delete failed and the transaction rolled back. Detach the user link with update_column(:person_id, nil) — bypassing the User validation that forbids nulling it and making the nullify a no-op — then destroy the person (which removes those FK references and its affiliations) before destroying the user. Co-Authored-By: Claude Opus 4.8 --- app/services/people_remover.rb | 29 +++++++++++++------- app/views/people/purge_confirmation.html.erb | 4 +-- spec/services/people_remover_spec.rb | 13 +++++++++ 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/app/services/people_remover.rb b/app/services/people_remover.rb index 6e1bc403e9..fe01bc2aaa 100644 --- a/app/services/people_remover.rb +++ b/app/services/people_remover.rb @@ -112,25 +112,34 @@ def call return [] if deletable.empty? c = collected + user_ids = c[:users].map(&:id) ActiveRecord::Base.transaction do - # Non-cascading records first, in FK-safe order: allocations and refunds - # reference payments; payments reference submissions; everything else - # cascades off Person. Users are destroyed before their person so the - # has_one :user nullify (which the model forbids) never runs. + # Detach the user<->person link first. update_column bypasses the User + # validation that forbids nulling person_id and makes Person's has_one :user + # nullify a no-op — so we can delete the person before the user (next), even + # though the person references the user via created_by/updated_by. + c[:users].each { |user| user.update_column(:person_id, nil) } + deletable.each { |person| person.association(:user).reset } + + # Non-cascading person records, FK-safe order: allocations and refunds + # reference payments; payments reference submissions. Allocation.where(id: c[:allocation_ids]).find_each(&:destroy!) Refund.where(id: c[:refund_ids]).find_each(&:destroy!) Payment.where(id: c[:payment_ids]).find_each(&:destroy!) Notification.where(id: c[:notification_ids]).find_each(&:destroy!) Pay::Customer.where(id: c[:pay_customer_ids]).find_each(&:destroy!) - # Destroy everything the account authored (FK-restricted, leaf → root) and - # its own notifications before the account itself — these don't cascade off - # the user, and the FKs would otherwise block its deletion. - user_ids = c[:users].map(&:id) + + # Everything the account authored (FK-restricted, leaf → root) plus its own + # notifications — these don't cascade off the user and would block it. authored_content_scopes(user_ids).each { |_, relation| relation.find_each(&:destroy!) } Notification.where(noticeable_type: "User", noticeable_id: user_ids).delete_all - c[:users].each(&:destroy!) - deletable.each { |person| person.association(:user).reset } + + # People BEFORE users: destroying a person removes its created_by/updated_by + # and affiliations (all FK to users), so the account is unreferenced by the + # time it's destroyed. deletable.each(&:destroy!) + c[:users].each(&:destroy!) + delete_audit_trail(c) end diff --git a/app/views/people/purge_confirmation.html.erb b/app/views/people/purge_confirmation.html.erb index 82199225da..8a92a8a67d 100644 --- a/app/views/people/purge_confirmation.html.erb +++ b/app/views/people/purge_confirmation.html.erb @@ -32,7 +32,7 @@

Also removed with the User Account

<% @account_content.each do |label, count| %> -
"> +
">
<%= label %>
"><%= count.zero? ? "—" : count %>
@@ -48,7 +48,7 @@

What will be deleted

<% @counts.each do |label, count| %> -
"> +
">
<%= label.to_s.tr("_", " ").capitalize %>
"><%= count.zero? ? "—" : count %>
diff --git a/spec/services/people_remover_spec.rb b/spec/services/people_remover_spec.rb index 95f81eca2f..81d8d77274 100644 --- a/spec/services/people_remover_spec.rb +++ b/spec/services/people_remover_spec.rb @@ -232,6 +232,19 @@ def remove(**opts) end end + describe "force delete when the person's own audit columns point to its user" do + it "deletes despite people.created_by_id/updated_by_id referencing the account" do + person = create(:person) + user = person.user + person.update_columns(created_by_id: user.id, updated_by_id: user.id) + + described_class.new(person_ids: [ person.id ], force: true).call + + expect(Person.exists?(person.id)).to be false + expect(User.exists?(user.id)).to be false + end + end + describe "force delete of an account that authored content" do it "deletes the person, the account, and everything it authored" do person = create(:person) From 0de3108bc33f9ad8ecd742e9f24fe9f6d755eb2b Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 15:31:30 -0400 Subject: [PATCH 5/8] Flag PeopleRemover as a related file for Person/User associations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The purge deletion graph is partly hardcoded, so it silently drifts when new data gets connected to a person or user. Add a Related Files rule (CLAUDE.md + copilot-instructions) and an in-code MAINTENANCE note so adding an association to Person/User — or a model referencing them — prompts wiring it into PeopleRemover. Co-Authored-By: Claude Opus 4.8 --- .github/copilot-instructions.md | 1 + CLAUDE.md | 1 + app/services/people_remover.rb | 6 ++++++ 3 files changed, 8 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 56edb7820e..99e75f8f60 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -40,6 +40,7 @@ When changing a model or controller, check whether these related files need upda | Decorator | Decorator spec | | Mailer (add/remove) | Mailer spec, mailer preview (follow existing patterns) | | Add/remove model, concern, service, or gem | AGENTS.md | +| Add an association to `Person`/`User`, or a model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | `PeopleRemover` (`app/services/people_remover.rb`) + its spec — wire the new data into the deletion graph so a purge cascades it, tears it down explicitly if it doesn't cascade, or surfaces it as a blocker; otherwise purges will orphan rows or fail | ## Code Style diff --git a/CLAUDE.md b/CLAUDE.md index e28c34816e..1b9993bc81 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,7 @@ When changing a model or controller, check whether these related files need upda | Decorator | Decorator spec | | Mailer (add/remove) | Mailer spec, mailer preview (follow existing patterns) | | Add/remove model, concern, service, or gem | AGENTS.md | +| Add an association to `Person`/`User`, or a model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | `PeopleRemover` (`app/services/people_remover.rb`) + its spec — wire the new data into the deletion graph so a purge cascades it, tears it down explicitly if it doesn't cascade, or surfaces it as a blocker; otherwise purges will orphan rows or fail | ## Code Style diff --git a/app/services/people_remover.rb b/app/services/people_remover.rb index fe01bc2aaa..bd9a452390 100644 --- a/app/services/people_remover.rb +++ b/app/services/people_remover.rb @@ -27,6 +27,12 @@ # merely *reference* the account without being owned by it (e.g. it created a # shared banner or event) are FK-restricted; the transaction rolls back and the # caller surfaces a clear error rather than orphaning or rewriting them. +# +# MAINTENANCE: the deletion graph is partly hardcoded (collected, audit_targets, +# authored_content_scopes, account_content). When an association is added to +# Person/User — or a model starts referencing them (person_id, user_id, +# created_by_id, updated_by_id, polymorphic *able) — wire it in here so purges +# keep cascading/clearing it instead of orphaning or blocking. class PeopleRemover Skip = Data.define(:person, :reasons) From d3f4b806ab56fa242a888dca924bc41f17aae82a Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 15:37:10 -0400 Subject: [PATCH 6/8] Add a drift guard for PeopleRemover's person/user coverage The purge deletion graph is hardcoded, so a new Person/User association can silently slip through and orphan or block a purge. Add a coverage spec that fails when a Person or User association isn't accounted for, and point the AI files and the service's MAINTENANCE note at it as the source of truth (noting it can't catch a new model that only references them). Co-Authored-By: Claude Opus 4.8 --- .github/copilot-instructions.md | 2 +- CLAUDE.md | 2 +- app/services/people_remover.rb | 2 + ...eople_remover_association_coverage_spec.rb | 50 +++++++++++++++++++ 4 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 spec/services/people_remover_association_coverage_spec.rb diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 99e75f8f60..18b1301e68 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -40,7 +40,7 @@ When changing a model or controller, check whether these related files need upda | Decorator | Decorator spec | | Mailer (add/remove) | Mailer spec, mailer preview (follow existing patterns) | | Add/remove model, concern, service, or gem | AGENTS.md | -| Add an association to `Person`/`User`, or a model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | `PeopleRemover` (`app/services/people_remover.rb`) + its spec — wire the new data into the deletion graph so a purge cascades it, tears it down explicitly if it doesn't cascade, or surfaces it as a blocker; otherwise purges will orphan rows or fail | +| Add an association to `Person`/`User`, or a new model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | Wire it into `PeopleRemover` (`app/services/people_remover.rb`) so a purge cascades/clears it (or deliberately blocks). `spec/services/people_remover_association_coverage_spec.rb` is the source of truth — it fails on new **associations** until they're handled and allowlisted there; it can't catch a new model that only *references* Person/User, so check that case by hand | ## Code Style diff --git a/CLAUDE.md b/CLAUDE.md index 1b9993bc81..9895483cdd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,7 +40,7 @@ When changing a model or controller, check whether these related files need upda | Decorator | Decorator spec | | Mailer (add/remove) | Mailer spec, mailer preview (follow existing patterns) | | Add/remove model, concern, service, or gem | AGENTS.md | -| Add an association to `Person`/`User`, or a model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | `PeopleRemover` (`app/services/people_remover.rb`) + its spec — wire the new data into the deletion graph so a purge cascades it, tears it down explicitly if it doesn't cascade, or surfaces it as a blocker; otherwise purges will orphan rows or fail | +| Add an association to `Person`/`User`, or a new model referencing them (`person_id`, `user_id`, `created_by_id`, `updated_by_id`, polymorphic `*able`) | Wire it into `PeopleRemover` (`app/services/people_remover.rb`) so a purge cascades/clears it (or deliberately blocks). `spec/services/people_remover_association_coverage_spec.rb` is the source of truth — it fails on new **associations** until they're handled and allowlisted there; it can't catch a new model that only *references* Person/User, so check that case by hand | ## Code Style diff --git a/app/services/people_remover.rb b/app/services/people_remover.rb index bd9a452390..91b775d13f 100644 --- a/app/services/people_remover.rb +++ b/app/services/people_remover.rb @@ -33,6 +33,8 @@ # Person/User — or a model starts referencing them (person_id, user_id, # created_by_id, updated_by_id, polymorphic *able) — wire it in here so purges # keep cascading/clearing it instead of orphaning or blocking. +# spec/services/people_remover_association_coverage_spec.rb fails when a new +# Person/User association isn't accounted for. class PeopleRemover Skip = Data.define(:person, :reasons) diff --git a/spec/services/people_remover_association_coverage_spec.rb b/spec/services/people_remover_association_coverage_spec.rb new file mode 100644 index 0000000000..527098c547 --- /dev/null +++ b/spec/services/people_remover_association_coverage_spec.rb @@ -0,0 +1,50 @@ +require "rails_helper" + +# Drift guard: PeopleRemover hardcodes the person/user deletion graph, so adding +# an association to Person or User can silently leave a purge orphaning or +# blocking on the new data. This fails when a new association appears that isn't +# in the allowlist below — forcing a conscious decision to wire it into +# app/services/people_remover.rb (cascade it, tear it down explicitly, or make it +# a deliberate blocker) and then list it here. +RSpec.describe "PeopleRemover association coverage" do + # Every association currently accounted for. Keep alphabetized. + let(:person_associations) do + %w[ + addresses affiliations avatar_attachment avatar_blob bookmarks categories + categorizable_items comments communal_reports contact_methods created_by + event_registrations event_staffs events form_submissions grants organizations + pay_charges pay_customers pay_subscriptions payment_processor scholarships + sectorable_items sectors staffed_events stories_as_spotlighted_facilitator + updated_by user windows_types + ] + end + + let(:user_associations) do + %w[ + bookmarked_events bookmarked_resources bookmarked_video_recordings + bookmarked_workshops bookmarks comments created_by event_registrations events + favorite_event notifications organizations person reports resources + stories_as_creator story_ideas_as_creator updated_by user_form_form_fields + user_forms windows_types workshop_ideas_as_creator workshop_logs + workshop_variation_ideas_creator workshop_variations_as_creator workshops + ] + end + + it "accounts for every Person association" do + unhandled = Person.reflect_on_all_associations.map { |a| a.name.to_s } - person_associations + + expect(unhandled).to be_empty, + "New Person association(s) not accounted for: #{unhandled.join(', ')}.\n" \ + "Wire them into app/services/people_remover.rb (cascade, explicit teardown, " \ + "or a deliberate blocker), then add them to this allowlist." + end + + it "accounts for every User association" do + unhandled = User.reflect_on_all_associations.map { |a| a.name.to_s } - user_associations + + expect(unhandled).to be_empty, + "New User association(s) not accounted for: #{unhandled.join(', ')}.\n" \ + "Wire them into app/services/people_remover.rb (cascade, explicit teardown, " \ + "or a deliberate blocker), then add them to this allowlist." + end +end From fe67f90865e065fb9c6c77ae6d996efb853d876d Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 15:48:07 -0400 Subject: [PATCH 7/8] Discard buffered Ahoy destroy events when a purge rolls back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The destroy lifecycle events are the who/what/when deletion audit, but they're flushed in an after_action — outside the purge transaction — so a rolled-back delete still flushed phantom "destroy" events for records that still exist. Record the lifecycle buffer size before the purge and, on any error, truncate it back so the failed attempt's events never reach the controller's flush. Success is unchanged: the events stay and flush as the audit. Co-Authored-By: Claude Opus 4.8 --- app/services/people_remover.rb | 11 +++++++++++ spec/services/people_remover_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/services/people_remover.rb b/app/services/people_remover.rb index 91b775d13f..7b0693b70c 100644 --- a/app/services/people_remover.rb +++ b/app/services/people_remover.rb @@ -119,6 +119,12 @@ def counts def call return [] if deletable.empty? + # The destroys below buffer Ahoy "destroy" lifecycle events (the who/what/when + # deletion audit), but the buffer is flushed after the request — outside this + # transaction. Remember its size so a rollback can drop the events this purge + # added; otherwise the controller flushes phantom destroys for records that + # still exist. + buffered_lifecycle_events = Analytics::LifecycleBuffer.store.size c = collected user_ids = c[:users].map(&:id) ActiveRecord::Base.transaction do @@ -152,6 +158,11 @@ def call end deletable.map(&:id) + rescue + # Roll back the lifecycle events this purge buffered so a failed delete can't + # leave phantom Ahoy "destroy" records for records that still exist. + Analytics::LifecycleBuffer.store.slice!(buffered_lifecycle_events..) + raise end private diff --git a/spec/services/people_remover_spec.rb b/spec/services/people_remover_spec.rb index 81d8d77274..cda6164efa 100644 --- a/spec/services/people_remover_spec.rb +++ b/spec/services/people_remover_spec.rb @@ -245,6 +245,27 @@ def remove(**opts) end end + describe "lifecycle (Ahoy) events on a rolled-back purge" do + before { Current.user = create(:user, :admin) } + after { Current.reset } + + it "discards the destroy events the purge buffered so no phantom audit is left" do + person = create(:person) + create(:banner, created_by: person.user) # FK-restricted ref the purge can't remove → rollback + + Analytics::LifecycleBuffer.store.clear + Analytics::LifecycleBuffer.push({ name: "sentinel", properties: {} }) + + expect do + described_class.new(person_ids: [ person.id ], force: true).call + end.to raise_error(ActiveRecord::InvalidForeignKey) + + expect(Analytics::LifecycleBuffer.store).to eq([ { name: "sentinel", properties: {} } ]) + ensure + Analytics::LifecycleBuffer.store.clear + end + end + describe "force delete of an account that authored content" do it "deletes the person, the account, and everything it authored" do person = create(:person) From 05d4830825b83510a13f1075e408e1f2616a8d58 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 21 Jun 2026 16:07:22 -0400 Subject: [PATCH 8/8] Attribute rake-task purges to an admin actor for the Ahoy audit The web purge records a who/what/when Ahoy destroy trail via the logged-in user and the controller's lifecycle flush; the rake task had neither, so bulk deletions left no audit. Run the task as an admin actor (default umberto.user@example.com, overridable with ACTOR_EMAIL) and flush the buffered lifecycle events to Ahoy on success, so rake purges are attributed too. Co-Authored-By: Claude Opus 4.8 --- lib/tasks/remove_people.rake | 22 ++++++++++++++++++++++ spec/tasks/remove_people_spec.rb | 12 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/lib/tasks/remove_people.rake b/lib/tasks/remove_people.rake index 71add1386b..a5edbd8204 100644 --- a/lib/tasks/remove_people.rake +++ b/lib/tasks/remove_people.rake @@ -18,6 +18,9 @@ # # FORCE=true overrides ALL protections and deletes even people who look real # (destroying any linked account). Use only for confirmed false positives. +# +# The deletion is attributed to an admin actor for the Ahoy audit trail: by +# default umberto.user@example.com, overridable with ACTOR_EMAIL=someone@example.com. namespace :data do desc "Permanently remove people and their full data + audit graph (dry run unless CONFIRM=true)" task remove_people: :environment do @@ -72,13 +75,32 @@ namespace :data do next end + # Attribute the deletion to an admin actor so the Ahoy "destroy" audit records + # who did it. Unlike the web flow there's no controller to flush the buffered + # lifecycle events, so we flush them here on success. + actor_email = ENV.fetch("ACTOR_EMAIL", "umberto.user@example.com") + actor = User.where(email: actor_email).last + puts "Warning: audit actor #{actor_email} not found — deletions won't be attributed." unless actor + + Current.user = actor + Current.source = "rake:data:remove_people" begin deleted = remover.call + flush_audit = -> { + events = Analytics::LifecycleBuffer.store + return if events.empty? + tracker = Ahoy::Tracker.new(user: actor) + events.each { |payload| tracker.track(payload[:name], payload[:properties]) } + events.clear + } + flush_audit.call puts "\nDone. Deleted #{deleted.size} person(s) and their data + audit graph." rescue ActiveRecord::InvalidForeignKey => e abort "\nAborted (nothing deleted): a linked User Account owns content that " \ "can't be orphaned (workshops, reports, stories, etc.). Reassign or " \ "remove it first.\n#{e.message}" + ensure + Current.reset end end end diff --git a/spec/tasks/remove_people_spec.rb b/spec/tasks/remove_people_spec.rb index 9c6a8b815e..33f1c78080 100644 --- a/spec/tasks/remove_people_spec.rb +++ b/spec/tasks/remove_people_spec.rb @@ -49,4 +49,16 @@ def suppress_output expect(Person.exists?(person.id)).to be false end + + it "attributes the deletion audit to the admin actor" do + actor = create(:user, :admin, email: "umberto.user@example.com") + person = create(:person, user: nil) + + expect { run_task(PERSON_IDS: person.id, CONFIRM: true) } + .to change { Ahoy::Event.where(name: "destroy.person").count }.by(1) + + expect(Ahoy::Event.where(name: "destroy.person").last.user_id).to eq(actor.id) + ensure + Current.reset + end end