diff --git a/app/models/user.rb b/app/models/user.rb index 99c3f24e98..0e2766f15d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,7 +18,12 @@ class User < ApplicationRecord belongs_to :casa_org - has_many :case_assignments, foreign_key: "volunteer_id", dependent: :destroy # TODO destroy is wrong + # Volunteers are deactivated, not destroyed: Volunteer#deactivate sets the + # user and each case_assignment inactive while preserving the rows for + # audit/history. restrict_with_error catches anything that tries to + # hard-delete a volunteer that still has assignments, rather than silently + # cascading and losing the history. Issue #6911. + has_many :case_assignments, foreign_key: "volunteer_id", dependent: :restrict_with_error has_many :casa_cases, -> { where(case_assignments: {active: true}) }, through: :case_assignments has_many :case_contacts, foreign_key: "creator_id" diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index def203c0dc..b13ae3fb8b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3,8 +3,32 @@ RSpec.describe User, type: :model do it { is_expected.to belong_to(:casa_org) } - it { is_expected.to have_many(:case_assignments) } + it { is_expected.to have_many(:case_assignments).with_foreign_key(:volunteer_id).dependent(:restrict_with_error) } it { is_expected.to have_many(:casa_cases).through(:case_assignments) } + + describe "destroying a volunteer with case_assignments" do + # The deactivate flow (Volunteer#deactivate) is the supported way to retire + # a volunteer: it sets `active: false` on the user and on every + # case_assignment, preserving rows for audit. Hard-deleting a volunteer + # that still has assignments would silently discard that history, so the + # association is :restrict_with_error. Issue #6911. + let(:volunteer) { create(:volunteer) } + + before do + create(:case_assignment, casa_case: create(:casa_case, casa_org: volunteer.casa_org), volunteer: volunteer) + end + + it "refuses to destroy the volunteer" do + expect { volunteer.destroy }.not_to change(CaseAssignment, :count) + expect(volunteer.persisted?).to eq(true) + end + + it "leaves the volunteer's case_assignments intact" do + assignment = volunteer.case_assignments.first + volunteer.destroy + expect(CaseAssignment.exists?(assignment.id)).to eq(true) + end + end it { is_expected.to have_many(:case_contacts) } it { is_expected.to have_many(:sent_emails) } it { is_expected.to have_many(:user_languages) }