Skip to content

WAIT: Add PeopleRemover: delete a person and all associated data#1813

Open
maebeale wants to merge 8 commits into
mainfrom
maebeale/cleanup-fake-submissions
Open

WAIT: Add PeopleRemover: delete a person and all associated data#1813
maebeale wants to merge 8 commits into
mainfrom
maebeale/cleanup-fake-submissions

Conversation

@maebeale

@maebeale maebeale commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

🔬 Inspect

Goal

We made fake submission-form data in prod and need to remove those people cleanly — and we'll need to delete people generally going forward. Adds a service + tooling to delete a person and their entire associated data graph (and their User account + everything it owns) without leaving orphans.

What's here

  • PeopleRemover service — deletes a person and all owned data. Person.destroy cascades most of it (submissions, affiliations, registrations, scholarships, addresses, contact methods, bookmarks, comments, sector/category tags, grants, event staffing); payments/refunds/allocations, bulk-payment notifications, Pay::Customer, and the PaperTrail/Ahoy audit trail don't, so they're torn down explicitly.
  • User + user data — when the account is destroyed (override), it takes everything it authored with it: workshops, logs, reports, resources, stories, ideas, variations, plus bookmarks/user_forms/notifications. Records the account merely references (a shared banner/event it created) are FK-restricted and block, caught gracefully (never a 500).
  • Safety model — skips people who look real (active/admin account, grant donor, spotlighted facilitator); delete_users removes an unused account; force overrides protections and does the full delete.
  • data:remove_people rake task — bulk cleanup by PERSON_IDS / FORM_SUBMISSION_IDS; dry-run unless CONFIRM=true (DELETE_USERS / FORCE flags).
  • Admin purge button on person edit (?admin=true) → interstitial confirmation page showing the full inventory (zero counts dashed, non-zero bold, tinted/hoverable rows). For a protected person, "Preview full delete" refreshes into an override preview listing the account's content under "Also removed with the User Account" before the final delete.

Notes for reviewers

  • Person-side deletion is exhaustive: every table referencing a person cascades, is handled, or is the facilitator protection (verified against schema FKs).
  • Authored content is destroyed in FK-safe order (leaf → root) inside one transaction; any unenumerated FK rolls the whole thing back and surfaces a clear message.
  • Two policy calls made per discussion: user-deletion stays opt-in (force/DELETE_USERS), and references the account doesn't own are left to block rather than nullified.

🤖 Generated with Claude Code

maebeale and others added 2 commits June 21, 2026 12:10
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
end

def collected
@collected ||= begin

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 From Claude: This is the heart of the "looks real" protection — an account is shielded if it has signed in, is a super_user, has its own Ahoy activity, or authored other people. It runs before any deletion so the audit cleanup can't erase the signal we judge by.

Comment thread app/services/people_remover.rb Outdated
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!)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 From Claude: Users are destroyed before their person on purpose: Person has_one :user, dependent: :nullify would try to null users.person_id, which the User model forbids once set — so we remove the account first and reset the cached association.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 From Claude: Backstop for any FK we don't enumerate (an account that authored real content is FK-restricted). The DB rolls the whole transaction back, so this turns a would-be 500 into a clear "reassign or remove that content first" message.

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 <noreply@anthropic.com>
# 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!) }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 From Claude: Force now deletes the account's authored content (workshops/stories/reports/…) in FK-safe leaf→root order rather than blocking on it. It runs before the account destroy because that content is created_by_id-owned with no dependent:. Anything the account only references but doesn't own (a shared banner/event) still hits the FK and rolls the transaction back — surfaced by the controller rescue.

maebeale and others added 5 commits June 21, 2026 13:03
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@maebeale maebeale marked this pull request as ready for review June 21, 2026 20:13
@maebeale maebeale requested review from jimnanney and jmilljr24 and removed request for jimnanney June 21, 2026 20:14
@maebeale

Copy link
Copy Markdown
Collaborator Author

@jmilljr24 this has UI and a rake task for deep person+associated data deletion.
the ui feature is hidden behind ?admin=true, and the rake task attributes deletions to Umberto.
this is not critical for this week, but, would be nice and is a need i think they'll have occasionally going forward.

@maebeale maebeale changed the title Add PeopleRemover: delete a person and all associated data WAIT: Add PeopleRemover: delete a person and all associated data Jun 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant