AO3-4693 Fix collection deletion for collections with a challenge

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

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

app/models/collection.rb

@@ -32,24 +32,14 @@ def ensure_associated
   end
 
   belongs_to :challenge, dependent: :destroy, polymorphic: true
+
   has_many :prompts, dependent: :destroy
 
   has_many :signups, class_name: "ChallengeSignup", dependent: :destroy
   has_many :potential_matches, dependent: :destroy
   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"

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")
@@ -91,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
@@ -111,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

spec/models/collection_spec.rb

@@ -692,4 +692,27 @@
       end
     end
   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!
+      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

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 references" 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) }
 

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 references" 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) }