Favoring dependent: :delete_all over :destroy (#15442)

* Favoring dependent: :delete_all over :destroy

The `dependent: :destroy` callback is slower than `dependent: :delete`
and `dependent: :delete_all`.  We need only favor the `dependent:
:destroy` when there's callbacks that happen.

In the case of :destroy, ActiveRecord instantiates each object and then
runs destroy.  Whereas in the case of :delete, ActiveRecord issues a SQL
command to delete the related files.

It is often "safer" to use :destroy, as it guarantees that you'll
instantiate the record and run it's callbacks.  But sometimes you have
to go with the speed of SQL.

This relates to #14140.  Note there is still more to consider, but given
that we're looking at moving from :destroy on all of an article's page
views to :delete, we might buy enough time in the callback.

* Removing redundancy of article destroy

Prior to this commit, `before_destroy_actions` called the `bust_cache`
method which in turn called `touch_actor_latest_article_updated_at` but
`bust_cache` did not pass the destroying parameter.  Then
`before_destroy_actions` immediately called
`touch_actor_latest_article_updated_at` with `destroying: true`.

With this change, we remove one of those duplicate calls.

* Fixing specs regarding relationships

* Fixing specs regarding relationships

app/models/article.rb

@@ -32,17 +32,21 @@ class Article < ApplicationRecord
   UNIQUE_URL_ERROR = "has already been taken. " \
                      "Email #{ForemInstance.email} for further details.".freeze
 
-  has_one :discussion_lock, dependent: :destroy
+  has_one :discussion_lock, dependent: :delete
 
-  has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy
+  has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :delete_all
   has_many :comments, as: :commentable, inverse_of: :commentable, dependent: :nullify
   has_many :html_variant_successes, dependent: :nullify
   has_many :html_variant_trials, dependent: :nullify
-  has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
+  has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
   has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
-  has_many :page_views, dependent: :destroy
+  has_many :page_views, dependent: :delete_all
+  # `dependent: :destroy` because in Poll we cascade the deletes of
+  #     the poll votes, options, and skips.
   has_many :polls, dependent: :destroy
-  has_many :profile_pins, as: :pinnable, inverse_of: :pinnable, dependent: :destroy
+  has_many :profile_pins, as: :pinnable, inverse_of: :pinnable, dependent: :delete_all
+  # `dependent: :destroy` because in RatingVote we're relying on
+  #     counter_culture to do some additional tallies
   has_many :rating_votes, dependent: :destroy
   has_many :top_comments,
            lambda {
@@ -585,8 +589,7 @@ class Article < ApplicationRecord
   end
 
   def before_destroy_actions
-    bust_cache
-    touch_actor_latest_article_updated_at(destroying: true)
+    bust_cache(destroying: true)
     article_ids = user.article_ids.dup
     if organization
       organization.touch(:last_article_at)
@@ -794,13 +797,13 @@ class Article < ApplicationRecord
     organization&.touch(:latest_article_updated_at)
   end
 
-  def bust_cache
+  def bust_cache(destroying: false)
     cache_bust = EdgeCache::Bust.new
     cache_bust.call(path)
     cache_bust.call("#{path}?i=i")
     cache_bust.call("#{path}?preview=#{password}")
     async_bust
-    touch_actor_latest_article_updated_at
+    touch_actor_latest_article_updated_at(destroying: destroying)
   end
 
   def calculate_base_scores

app/models/discussion_lock.rb

@@ -1,6 +1,10 @@
 #  @note When we destroy the related user, it's using dependent:
 #        :delete for the relationship.  That means no before/after
 #        destroy callbacks will be called on this object.
+#
+# @note When we destroy the related article, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class DiscussionLock < ApplicationRecord
   belongs_to :article
   belongs_to :locking_user, class_name: "User"

app/models/mention.rb

@@ -1,6 +1,10 @@
 #  @note When we destroy the related user, it's using dependent:
 #        :delete for the relationship.  That means no before/after
 #        destroy callbacks will be called on this object.
+#
+# @note When we destroy the related article, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class Mention < ApplicationRecord
   belongs_to :user
   belongs_to :mentionable, polymorphic: true

app/models/notification_subscription.rb

@@ -1,6 +1,10 @@
 #  @note When we destroy the related user, it's using dependent:
 #        :delete for the relationship.  That means no before/after
 #        destroy callbacks will be called on this object.
+#
+# @note When we destroy the related article, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class NotificationSubscription < ApplicationRecord
   belongs_to :notifiable, polymorphic: true
   belongs_to :user

app/models/page_view.rb

@@ -1,3 +1,6 @@
+# @note When we destroy the related article, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class PageView < ApplicationRecord
   belongs_to :user, optional: true
   belongs_to :article

app/models/poll.rb

@@ -5,9 +5,9 @@ class Poll < ApplicationRecord
 
   belongs_to :article
 
-  has_many :poll_options, dependent: :destroy
-  has_many :poll_skips, dependent: :destroy
-  has_many :poll_votes, dependent: :destroy
+  has_many :poll_options, dependent: :delete_all
+  has_many :poll_skips, dependent: :delete_all
+  has_many :poll_votes, dependent: :delete_all
 
   validates :poll_options_count, presence: true
   validates :poll_options_input_array, presence: true, length: { minimum: 2, maximum: 15 }

app/models/poll_option.rb

@@ -1,3 +1,6 @@
+# @note When we destroy the related poll, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class PollOption < ApplicationRecord
   belongs_to :poll
   has_many :poll_votes, dependent: :destroy

app/models/poll_skip.rb

@@ -1,6 +1,10 @@
 #  @note When we destroy the related user, it's using dependent:
 #        :delete for the relationship.  That means no before/after
 #        destroy callbacks will be called on this object.
+#
+# @note When we destroy the related poll, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class PollSkip < ApplicationRecord
   belongs_to :poll
   belongs_to :user

app/models/poll_vote.rb

@@ -1,6 +1,10 @@
 #  @note When we destroy the related user, it's using dependent:
 #        :delete for the relationship.  That means no before/after
 #        destroy callbacks will be called on this object.
+#
+# @note When we destroy the related poll, it's using dependent:
+#       :delete for the relationship.  That means no before/after
+#       destroy callbacks will be called on this object.
 class PollVote < ApplicationRecord
   belongs_to :user
   belongs_to :poll_option

app/models/profile_pin.rb

@@ -1,3 +1,6 @@
+# @note When we destroy the related article (via pinnable), it's using
+#       dependent: :delete for the relationship.  That means no
+#       before/after destroy callbacks will be called on this object.
 class ProfilePin < ApplicationRecord
   belongs_to :pinnable, polymorphic: true
   belongs_to :profile, polymorphic: true

spec/models/article_spec.rb

@@ -18,17 +18,17 @@ RSpec.describe Article, type: :model do
     it { is_expected.to belong_to(:organization).optional }
     it { is_expected.to belong_to(:user) }
 
-    it { is_expected.to have_one(:discussion_lock).dependent(:destroy) }
+    it { is_expected.to have_one(:discussion_lock).dependent(:delete) }
 
     it { is_expected.to have_many(:comments).dependent(:nullify) }
-    it { is_expected.to have_many(:mentions).dependent(:destroy) }
+    it { is_expected.to have_many(:mentions).dependent(:delete_all) }
     it { is_expected.to have_many(:html_variant_successes).dependent(:nullify) }
     it { is_expected.to have_many(:html_variant_trials).dependent(:nullify) }
-    it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) }
+    it { is_expected.to have_many(:notification_subscriptions).dependent(:delete_all) }
     it { is_expected.to have_many(:notifications).dependent(:delete_all) }
-    it { is_expected.to have_many(:page_views).dependent(:destroy) }
+    it { is_expected.to have_many(:page_views).dependent(:delete_all) }
     it { is_expected.to have_many(:polls).dependent(:destroy) }
-    it { is_expected.to have_many(:profile_pins).dependent(:destroy) }
+    it { is_expected.to have_many(:profile_pins).dependent(:delete_all) }
     it { is_expected.to have_many(:rating_votes).dependent(:destroy) }
     it { is_expected.to have_many(:sourced_subscribers) }
     it { is_expected.to have_many(:reactions).dependent(:destroy) }

spec/models/poll_spec.rb

@@ -9,9 +9,9 @@ RSpec.describe Poll, type: :model do
     describe "builtin validations" do
       subject { poll }
 
-      it { is_expected.to have_many(:poll_options).dependent(:destroy) }
-      it { is_expected.to have_many(:poll_skips).dependent(:destroy) }
-      it { is_expected.to have_many(:poll_votes).dependent(:destroy) }
+      it { is_expected.to have_many(:poll_options).dependent(:delete_all) }
+      it { is_expected.to have_many(:poll_skips).dependent(:delete_all) }
+      it { is_expected.to have_many(:poll_votes).dependent(:delete_all) }
 
       it { is_expected.to validate_length_of(:poll_options_input_array).is_at_least(2).is_at_most(15) }