Skip to content

fix(user): restrict_with_error on case_assignments to prevent history loss#6947

Open
mvanhorn wants to merge 1 commit into
rubyforgood:mainfrom
mvanhorn:osc/6911-user-case-assignments-dependent
Open

fix(user): restrict_with_error on case_assignments to prevent history loss#6947
mvanhorn wants to merge 1 commit into
rubyforgood:mainfrom
mvanhorn:osc/6911-user-case-assignments-dependent

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

What github issue is this PR for, if any?

Resolves #6911

What changed, and why?

The has_many :case_assignments association on User carried dependent: :destroy with a # TODO destroy is wrong comment that has been on that line for a while. The cascade is wrong because case_assignments is the volunteer↔case join record and carries history (the active flag, timestamps, the relationship itself); hard-deleting it loses that history. The class also does not use Paranoia, so there is no soft-delete recovery path either.

The supported retirement flow is Volunteer#deactivate, which sets active: false on the user and updates every case_assignment with case_assignments.update_all(active: false), preserving the rows. There is no destroy path called from the app, so the cascade only ever fires from an out-of-band action (Rails console, future destroy controller, a fixture cleanup). The right semantics is "destroy is forbidden while assignments exist; you must deactivate."

Swapping dependent: :destroy for dependent: :restrict_with_error makes that invariant explicit. Anything that tries to hard-delete a volunteer with live assignments now fails fast: the destroy returns false, an error is added to errors[:base], and the case_assignment rows stay intact. The TODO is resolved both in code and in the comment that now documents the intent and points to the deactivation entry point.

Spec coverage on User:

  • A shoulda-matchers assertion replaces the bare have_many(:case_assignments) line so the dependent option is now part of the contract.
  • A new describe "destroying a volunteer with case_assignments" block adds two behavioral specs: one asserts the destroy is refused (volunteer stays persisted, CaseAssignment count unchanged), the other asserts the assignment row still exists after the destroy attempt.

I deliberately scoped the change to the case_assignments association called out in the issue's acceptance criteria. The sibling associations the issue's "What to investigate" list mentions (case_contacts, followups, other_duties, learning_hours, etc.) deserve their own audit, but each has a slightly different cascade story (case_contacts and followups already have no dependent: option; learning_hours is volunteer-owned historical data with similar concerns). Mixing them in here would inflate the diff and the review surface, so I left them for a follow-up.

How is this tested? (please write rspec and jest tests!) 💖💪

Two new RSpec examples in spec/models/user_spec.rb:

  • "refuses to destroy the volunteer": creates a volunteer with one case_assignment, calls destroy, and asserts the assignment count is unchanged and the volunteer is still persisted.
  • "leaves the volunteer's case_assignments intact": same setup, calls destroy, and asserts CaseAssignment.exists?(id) is still true afterward.

Plus the shoulda-matchers line is upgraded from have_many(:case_assignments) to have_many(:case_assignments).with_foreign_key(:volunteer_id).dependent(:restrict_with_error), so the dependent option is now part of the model contract.

The existing #deactivate specs in spec/models/volunteer_spec.rb already cover the supported flow and continue to pass; this PR doesn't change deactivate behavior.

I could not run bin/rails spec in my workspace (system Ruby 2.6, project uses 4.0.3), so CI is the final word on the suite. The change itself is two lines in user.rb (option swap + a comment explaining the invariant) and an additive spec block, so I'd be surprised by a regression.

Screenshots please :)

Behavior-only change to a model association, no UI surface. Closest thing to a screenshot is the new spec output, which CI will render.

Feelings gif (optional)

tidying up

AI disclosure: authored with Claude as a coding assistant; I reviewed the change before pushing.

… loss

The has_many :case_assignments association on User had `dependent:
:destroy` with a `# TODO destroy is wrong` comment. Cascading destroy
on a volunteer would hard-delete every case_assignment, losing the
audit trail (active flag, dates, the volunteer↔case mapping).

The supported retirement flow is Volunteer#deactivate, which marks the
user and each case_assignment inactive while preserving the rows.
`dependent: :restrict_with_error` makes that invariant explicit:
anything that tries to hard-delete a volunteer with live assignments
now fails fast and surfaces an error instead of silently losing
history.

Adds a shoulda-matchers assertion for the dependent option and two
behavioral specs that verify the destroy is refused and the
case_assignment rows are preserved.

Resolves rubyforgood#6911
@github-actions github-actions Bot added 🧪 Tests Tests ruby Touches Ruby code labels May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Touches Ruby code 🧪 Tests Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User#case_assignments has misleading dependent: :destroy behavior

1 participant