From aa760ac3da8ed423372b1e0957b2fe9cab88b16d Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Sat, 4 Apr 2026 19:52:26 +0100 Subject: [PATCH 1/8] AO3-4693 Destroy challenge before collection on destroy --- app/models/collection.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/collection.rb b/app/models/collection.rb index ed78ea4ce3f..252cf692c21 100755 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -31,7 +31,13 @@ def ensure_associated self.collection_profile = CollectionProfile.new unless self.collection_profile end - belongs_to :challenge, dependent: :destroy, polymorphic: true + belongs_to :challenge, polymorphic: true + + before_destroy :destroy_challenge + def destroy_challenge + challenge.destroy if challenge + end + has_many :prompts, dependent: :destroy has_many :signups, class_name: "ChallengeSignup", dependent: :destroy From eda254c3dbd286d3b5bcb69f69311c5c0e1d77b1 Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Sat, 4 Apr 2026 20:24:51 +0100 Subject: [PATCH 2/8] AO3-4693 Add tests for destroying collection with challenge --- spec/models/collection_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index d266ed30f1d..972efb99b28 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -692,4 +692,21 @@ end end end + + describe "#destroy" do + it "destroys the collection and its gift exchange challenge" do + challenge = create(:gift_exchange) + collection = create(:collection, challenge: challenge) + collection.destroy + expect(Collection.exists?(collection.id)).to be false + expect(GiftExchange.exists?(challenge.id)).to be false + end + it "destroys the collection and its prompt meme challenge" do + challenge = create(:prompt_meme) + collection = create(:collection, challenge: challenge) + collection.destroy + expect(Collection.exists?(collection.id)).to be false + expect(PromptMeme.exists?(challenge.id)).to be false + end + end end From 53f4ddf38e2229b2243b828a024e385a0b2d72df Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Sat, 4 Apr 2026 20:47:49 +0100 Subject: [PATCH 3/8] AO3-4693 Use safe navigation in destroy_challenge --- app/models/collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/collection.rb b/app/models/collection.rb index 252cf692c21..cd92a4f219d 100755 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -35,7 +35,7 @@ def ensure_associated before_destroy :destroy_challenge def destroy_challenge - challenge.destroy if challenge + challenge&.destroy end has_many :prompts, dependent: :destroy From 8a56d3da705bd71e2e8486bd04c6627ef9fcd4d9 Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Thu, 9 Apr 2026 13:12:08 +0100 Subject: [PATCH 4/8] AO3-4693 Use dependent :destroy/:nullify instead of before_destroy callbacks --- app/models/challenge_models/gift_exchange.rb | 5 +---- app/models/challenge_models/prompt_meme.rb | 5 +---- app/models/collection.rb | 7 +------ lib/challenge_core.rb | 8 -------- 4 files changed, 3 insertions(+), 22 deletions(-) diff --git a/app/models/challenge_models/gift_exchange.rb b/app/models/challenge_models/gift_exchange.rb index 5f49de68738..c34bd2da1c3 100644 --- a/app/models/challenge_models/gift_exchange.rb +++ b/app/models/challenge_models/gift_exchange.rb @@ -4,7 +4,7 @@ class GiftExchange < ApplicationRecord override_datetime_setters - has_one :collection, as: :challenge + has_one :collection, as: :challenge, dependent: :nullify # limits the kind of prompts users can submit belongs_to :request_restriction, class_name: "PromptRestriction", dependent: :destroy @@ -44,9 +44,6 @@ def update_allowed_values # make sure that challenge sign-up / close / open dates aren't contradictory validate :validate_signup_dates - # When Challenges are deleted, there are two references left behind that need to be reset to nil - before_destroy :clear_challenge_references - after_save :copy_tag_set_from_offer_to_request def copy_tag_set_from_offer_to_request return unless self.offer_restriction diff --git a/app/models/challenge_models/prompt_meme.rb b/app/models/challenge_models/prompt_meme.rb index e6b59607e0f..5424a6e0985 100644 --- a/app/models/challenge_models/prompt_meme.rb +++ b/app/models/challenge_models/prompt_meme.rb @@ -4,7 +4,7 @@ class PromptMeme < ApplicationRecord override_datetime_setters - has_one :collection, as: :challenge + has_one :collection, as: :challenge, dependent: :nullify # limits the kind of prompts users can submit belongs_to :request_restriction, class_name: "PromptRestriction", dependent: :destroy @@ -35,9 +35,6 @@ def update_allowed_prompts end end - # When Challenges are deleted, there are two references left behind that need to be reset to nil - before_destroy :clear_challenge_references - def user_allowed_to_see_signups?(user) true end diff --git a/app/models/collection.rb b/app/models/collection.rb index cd92a4f219d..3f86fa97c6f 100755 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -31,12 +31,7 @@ def ensure_associated self.collection_profile = CollectionProfile.new unless self.collection_profile end - belongs_to :challenge, polymorphic: true - - before_destroy :destroy_challenge - def destroy_challenge - challenge&.destroy - end + belongs_to :challenge, dependent: :destroy, polymorphic: true has_many :prompts, dependent: :destroy diff --git a/lib/challenge_core.rb b/lib/challenge_core.rb index 7d4ee38ad00..ff42f1abacd 100644 --- a/lib/challenge_core.rb +++ b/lib/challenge_core.rb @@ -34,14 +34,6 @@ def validate_signup_dates end end end - - # When Challenges are deleted, there are two references left behind that need to be reset to nil - def clear_challenge_references - collection.challenge_id = nil - collection.challenge_type = nil - collection.save! - end - # a couple of handy shorthand methods def required(type) self.send("#{type}_num_required") From c3b76f3d6c5009e52a165b4f1b1b679b0cb6ff68 Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Thu, 9 Apr 2026 13:14:56 +0100 Subject: [PATCH 5/8] AO3-4693 Test that destroying a challenge nullifies collection FK --- spec/models/gift_exchange_spec.rb | 17 +++++++++++++++++ spec/models/prompt_meme_spec.rb | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/spec/models/gift_exchange_spec.rb b/spec/models/gift_exchange_spec.rb index 74e6346b329..573e9ba27f4 100644 --- a/spec/models/gift_exchange_spec.rb +++ b/spec/models/gift_exchange_spec.rb @@ -43,6 +43,23 @@ end end + describe "#destroy" do + let!(:challenge) { create(:gift_exchange) } + let!(:collection) { create(:collection, challenge: challenge) } + + it "does not destroy the collection" do + challenge.destroy! + expect(Collection.exists?(collection.id)).to be true + end + + it "nullifies the collection's challenge reference" do + challenge.destroy! + collection.reload + expect(collection.challenge_id).to be_nil + expect(collection.challenge_type).to be_nil + end + end + describe "reindexing" do let!(:collection) { create(:collection) } diff --git a/spec/models/prompt_meme_spec.rb b/spec/models/prompt_meme_spec.rb index fa88ab19568..a6582bb41e4 100644 --- a/spec/models/prompt_meme_spec.rb +++ b/spec/models/prompt_meme_spec.rb @@ -1,6 +1,23 @@ require "spec_helper" describe PromptMeme do + describe "#destroy" do + let!(:challenge) { create(:prompt_meme) } + let!(:collection) { create(:collection, challenge: challenge) } + + it "does not destroy the collection" do + challenge.destroy! + expect(Collection.exists?(collection.id)).to be true + end + + it "nullifies the collection's challenge reference" do + challenge.destroy! + collection.reload + expect(collection.challenge_id).to be_nil + expect(collection.challenge_type).to be_nil + end + end + describe "reindexing" do let!(:collection) { create(:collection) } From f97ff04a3ebdd88b79ae10eae32f6ca9377cc284 Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Thu, 9 Apr 2026 13:15:28 +0100 Subject: [PATCH 6/8] AO3-4693 Add no-challenge destroy case and use destroy! consistently --- spec/models/collection_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index 972efb99b28..f88ecbede7d 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -694,17 +694,23 @@ end describe "#destroy" do + it "destroys the collection when it has no challenge" do + collection = create(:collection) + expect { collection.destroy! } + .not_to raise_error + expect(Collection.exists?(collection.id)).to be false + end it "destroys the collection and its gift exchange challenge" do challenge = create(:gift_exchange) collection = create(:collection, challenge: challenge) - collection.destroy + collection.destroy! expect(Collection.exists?(collection.id)).to be false expect(GiftExchange.exists?(challenge.id)).to be false end it "destroys the collection and its prompt meme challenge" do challenge = create(:prompt_meme) collection = create(:collection, challenge: challenge) - collection.destroy + collection.destroy! expect(Collection.exists?(collection.id)).to be false expect(PromptMeme.exists?(challenge.id)).to be false end From f8f36d76e21b6f1358a8a1d74e7660c5c09647c2 Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Thu, 9 Apr 2026 15:40:24 +0100 Subject: [PATCH 7/8] AO3-4693 Move clean_up_challenge from Collection into ChallengeCore --- app/models/collection.rb | 11 ----------- lib/challenge_core.rb | 10 ++++++++++ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/models/collection.rb b/app/models/collection.rb index 3f86fa97c6f..c837b95cba3 100755 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -40,17 +40,6 @@ def ensure_associated has_many :assignments, class_name: "ChallengeAssignment", dependent: :destroy has_many :claims, class_name: "ChallengeClaim", dependent: :destroy - # We need to get rid of all of these if the challenge is destroyed - after_save :clean_up_challenge - def clean_up_challenge - return if self.challenge_id - - assignments.each(&:destroy) - potential_matches.each(&:destroy) - signups.each(&:destroy) - prompts.each(&:destroy) - end - has_many :collection_items, dependent: :destroy accepts_nested_attributes_for :collection_items, allow_destroy: true has_many :approved_collection_items, -> { approved_by_both }, class_name: "CollectionItem" diff --git a/lib/challenge_core.rb b/lib/challenge_core.rb index ff42f1abacd..d1c6c749d35 100644 --- a/lib/challenge_core.rb +++ b/lib/challenge_core.rb @@ -83,6 +83,15 @@ def update_collection_index IndexQueue.enqueue_id(Collection, collection.id, :main) end + def clean_up_challenge + return unless collection + + collection.assignments.each(&:destroy) + collection.potential_matches.each(&:destroy) + collection.signups.each(&:destroy) + collection.prompts.each(&:destroy) + end + module ClassMethods # override datetime setters so we can take strings def override_datetime_setters @@ -103,6 +112,7 @@ def override_datetime_setters def self.included(base) base.class_eval do after_commit :update_collection_index, if: :should_update_collection_index? + before_destroy :clean_up_challenge end base.extend(ClassMethods) end From e63839883530d48b33087392f07a5b993d7a3f0b Mon Sep 17 00:00:00 2001 From: ayasayadi1 Date: Fri, 10 Apr 2026 16:48:32 +0100 Subject: [PATCH 8/8] AO3-4693 Retry CI --- spec/models/gift_exchange_spec.rb | 2 +- spec/models/prompt_meme_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/gift_exchange_spec.rb b/spec/models/gift_exchange_spec.rb index 573e9ba27f4..4a2d4e47115 100644 --- a/spec/models/gift_exchange_spec.rb +++ b/spec/models/gift_exchange_spec.rb @@ -52,7 +52,7 @@ expect(Collection.exists?(collection.id)).to be true end - it "nullifies the collection's challenge reference" do + it "nullifies the collection's challenge references" do challenge.destroy! collection.reload expect(collection.challenge_id).to be_nil diff --git a/spec/models/prompt_meme_spec.rb b/spec/models/prompt_meme_spec.rb index a6582bb41e4..209c554b16b 100644 --- a/spec/models/prompt_meme_spec.rb +++ b/spec/models/prompt_meme_spec.rb @@ -10,7 +10,7 @@ expect(Collection.exists?(collection.id)).to be true end - it "nullifies the collection's challenge reference" do + it "nullifies the collection's challenge references" do challenge.destroy! collection.reload expect(collection.challenge_id).to be_nil