diff --git a/app/decorators/person_decorator.rb b/app/decorators/person_decorator.rb index 09bb4d5421..cc572d4c6a 100644 --- a/app/decorators/person_decorator.rb +++ b/app/decorators/person_decorator.rb @@ -83,6 +83,38 @@ def facilitator_since_date end end + # Label shown for each deletion-blocker key (see Person#deletion_blockers), so + # the "Can't be deleted" notice names the actual kinds of records the person + # has rather than a generic catch-all. + DELETION_BLOCKER_LABELS = { + user_account: "a user account", + workshop_logs: "authored workshop logs", + story_ideas: "authored story ideas", + workshop_ideas: "authored workshop ideas", + workshop_variation_ideas: "authored workshop variation ideas", + affiliations: "organization affiliations", + stories: "authored stories", + workshops: "authored workshops", + workshop_variations: "authored workshop variations", + community_news: "authored community news", + resources: "authored resources", + payments: "payments", + scholarships: "scholarships", + grants: "grants", + event_registrations: "event registrations", + event_staffing: "event staff assignments", + form_submissions: "form submissions" + }.freeze + + # Human-readable explanation of why the Delete button is unavailable, or nil + # when the person is deletable — naming each kind of record that's blocking it. + def deletion_blocked_reason + labels = deletion_blockers.map { |key| DELETION_BLOCKER_LABELS.fetch(key) } + return if labels.empty? + + "Can't be deleted — this person has #{labels.to_sentence}." + end + def affiliated_since_date # Compute in Ruby from the (eager-loaded) association so list pages that # preload affiliations don't fire a MIN(start_date) query per row. diff --git a/app/models/person.rb b/app/models/person.rb index 0d2032be50..7e07ff519c 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -38,6 +38,7 @@ class Person < ApplicationRecord has_many :event_staffs, dependent: :destroy has_many :scholarships, foreign_key: :recipient_id, dependent: :destroy has_many :grants, as: :donor, dependent: :destroy + has_many :payments, dependent: :restrict_with_error has_many :events, through: :event_registrations has_many :staffed_events, through: :event_staffs, source: :event has_many :categories, through: :categorizable_items @@ -327,6 +328,39 @@ def age_range_items_ordered age_range_categorizable_items.sort_by { |item| [ item.category&.position || 0, item.category&.name.to_s ] } end + # Keys for each kind of record that deleting this person would cascade-destroy + # or orphan — the things worth keeping (identity, authored content, financial + # and event history), not disposable profile detail like addresses or tags. + # Empty means the person is safe to delete. Gates PersonPolicy#destroy?; the + # decorator maps each key to the label shown in the "Can't be deleted" notice. + def deletion_blockers + blockers = [] + if user.present? + blockers << :user_account + blockers << :workshop_logs if user.workshop_logs.exists? + blockers << :story_ideas if user.story_ideas_as_creator.exists? + blockers << :workshop_ideas if user.workshop_ideas_as_creator.exists? + blockers << :workshop_variation_ideas if user.workshop_variation_ideas_creator.exists? + end + blockers << :affiliations if affiliations.exists? + blockers << :stories if stories_as_author.exists? || stories_as_spotlighted_facilitator.exists? + blockers << :workshops if workshops_as_author.exists? + blockers << :workshop_variations if workshop_variations_as_author.exists? + blockers << :community_news if community_news_as_author.exists? + blockers << :resources if resources_as_author.exists? + blockers << :payments if payments.exists? + blockers << :scholarships if scholarships.exists? + blockers << :grants if grants.exists? + blockers << :event_registrations if event_registrations.exists? + blockers << :event_staffing if event_staffs.exists? + blockers << :form_submissions if form_submissions.exists? + blockers + end + + def deletable? + deletion_blockers.empty? + end + private # Count the in-memory set (not a DB query): nested attributes build the items in diff --git a/app/policies/person_policy.rb b/app/policies/person_policy.rb index e6d21f8cf7..3fb8242ed6 100644 --- a/app/policies/person_policy.rb +++ b/app/policies/person_policy.rb @@ -30,7 +30,7 @@ def update? end def destroy? - admin? && record.persisted? && !has_associated_data? + admin? && record.persisted? && record.deletable? end def search? @@ -51,15 +51,4 @@ def owner? return false unless authenticated? record.user == user end - - def has_associated_data? - record.user.present? || - record.affiliations.exists? || - record.stories_as_spotlighted_facilitator.exists? || - record.stories_as_author.exists? || - record.workshop_variations_as_author.exists? || - record.workshops_as_author.exists? || - record.community_news_as_author.exists? || - record.resources_as_author.exists? - end end diff --git a/app/views/people/_form.html.erb b/app/views/people/_form.html.erb index f79f3f5ceb..6fb5408930 100644 --- a/app/views/people/_form.html.erb +++ b/app/views/people/_form.html.erb @@ -566,6 +566,11 @@ <% if allowed_to?(:destroy?, f.object) %> <%= link_to "Delete", @person, class: "btn btn-danger-outline", data: { turbo_method: :delete, turbo_confirm: "Are you sure you want to delete?" } %> + <% elsif f.object.persisted? %> + + + <%= f.object.decorate.deletion_blocked_reason %> + <% end %> <%= link_to "Cancel", people_path, class: "btn btn-secondary-outline", diff --git a/spec/decorators/person_decorator_spec.rb b/spec/decorators/person_decorator_spec.rb index 307b06189b..f90e528381 100644 --- a/spec/decorators/person_decorator_spec.rb +++ b/spec/decorators/person_decorator_spec.rb @@ -52,4 +52,46 @@ expect(person.decorate.affiliated_since_date).to be_nil end end + + describe "#deletion_blocked_reason" do + it "is nil when the person has no blocking associations" do + person = create(:person, user: nil) + + expect(person.decorate.deletion_blocked_reason).to be_nil + end + + it "names payments when the person has them" do + person = create(:person, user: nil) + create(:payment, person: person) + + expect(person.decorate.deletion_blocked_reason) + .to eq("Can't be deleted — this person has payments.") + end + + it "names event registrations and form submissions" do + person = create(:person, user: nil) + create(:event_registration, registrant: person) + create(:form_submission, person: person) + + expect(person.decorate.deletion_blocked_reason) + .to eq("Can't be deleted — this person has event registrations and form submissions.") + end + + it "names authored records owned through the person's user" do + person = create(:person) + create(:workshop_log, created_by: person.user) + + expect(person.decorate.deletion_blocked_reason) + .to eq("Can't be deleted — this person has a user account and authored workshop logs.") + end + + it "names each kind of blocking record the person actually has" do + person = create(:person, user: nil) + create(:scholarship, recipient: person) + create(:grant, donor: person) + + expect(person.decorate.deletion_blocked_reason) + .to eq("Can't be deleted — this person has scholarships and grants.") + end + end end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 2ba7b56755..0974748020 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -523,4 +523,63 @@ def answer(identifier, value) expect(person.completed_facilitator_trainings).to contain_exactly(training) end end + + describe "#deletable?" do + it "is true for a person with no user, affiliations, content, or financial records" do + expect(create(:person, user: nil)).to be_deletable + end + + it "is false when the person has a linked user account" do + expect(create(:person)).not_to be_deletable + end + + it "is false when the person has payments" do + person = create(:person, user: nil) + create(:payment, person: person) + + expect(person).not_to be_deletable + end + + it "is false when the person is a scholarship recipient" do + person = create(:person, user: nil) + create(:scholarship, recipient: person) + + expect(person).not_to be_deletable + end + + it "is false when the person is a grant donor" do + person = create(:person, user: nil) + create(:grant, donor: person) + + expect(person).not_to be_deletable + end + + it "is false when the person has an event registration" do + person = create(:person, user: nil) + create(:event_registration, registrant: person) + + expect(person).not_to be_deletable + end + + it "is false when the person has staffed an event" do + person = create(:person, user: nil) + create(:event_staff, person: person) + + expect(person).not_to be_deletable + end + + it "is false when the person has a form submission" do + person = create(:person, user: nil) + create(:form_submission, person: person) + + expect(person).not_to be_deletable + end + + it "is false when the person's user authored workshop logs or ideas" do + person = create(:person) + create(:workshop_log, created_by: person.user) + + expect(person).not_to be_deletable + end + end end diff --git a/spec/policies/person_policy_spec.rb b/spec/policies/person_policy_spec.rb index 70b4ccb6fc..055f60e0ef 100644 --- a/spec/policies/person_policy_spec.rb +++ b/spec/policies/person_policy_spec.rb @@ -212,6 +212,107 @@ def policy_for(record: nil, user:) expect(policy).not_to be_allowed_to(:destroy?) end end + + context "when person has payments" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:payment, person: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has scholarships" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:scholarship, recipient: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has grants" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:grant, donor: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has an event registration" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:event_registration, registrant: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has staffed an event" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:event_staff, person: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has a form submission" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + before do + create(:form_submission, person: person) + end + + it "is not allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).not_to be_allowed_to(:destroy?) + end + end + + context "when person has no associated data" do + let(:admin) { create(:user, :admin) } + let(:person) { create(:person, user: nil) } + + it "is allowed" do + policy = policy_for(record: person, user: admin) + + expect(policy).to be_allowed_to(:destroy?) + end + end end describe "relation_scope" do