Extract background jobs from Reaction (#1312)

* Refactor Reaction

* Update reaction_spec

* Extract update_reactable to job class

* Create ApplicationJob

* Rename Engagement::UpdateReactableJob to ReactionCascadeJob

* Rename ReactionCascadeJob to Reaction::UpdateRecordsJob

* Refactor Reaction::UpdateRecordsJob

* Remove unused method

* Change module to class

app/jobs/application_job.rb

@@ -0,0 +1,7 @@
+class ApplicationJob < ActiveJob::Base
+  # Automatically retry jobs that encountered a deadlock
+  # retry_on ActiveRecord::Deadlocked
+
+  # Most jobs are safe to ignore if the underlying records are no longer available
+  # discard_on ActiveJob::DeserializationError
+end

app/jobs/reaction/update_records_job.rb

@@ -0,0 +1,46 @@
+class Reaction
+  class UpdateRecordsJob < ApplicationJob
+    queue_as :default
+
+    attr_reader :reactable, :user
+
+    def perform(reactable, user)
+      @reactable = reactable
+      @user = user
+
+      reactable_type = reactable.class.name
+
+      if reactable_type == "Article"
+        update_article
+      elsif reactable_type == "Comment"
+        update_comment
+      end
+      user.touch
+      occasionally_sync_reaction_counts
+    end
+
+    private
+
+    def update_article
+      cache_buster = CacheBuster.new
+      reactable.async_score_calc
+      reactable.index!
+      cache_buster.bust "/reactions?article_id=#{reactable.id}"
+      cache_buster.bust user.path
+    end
+
+    def update_comment
+      cache_buster = CacheBuster.new
+      reactable.save
+      cache_buster.bust "/reactions?commentable_id=#{reactable.commentable_id}&commentable_type=#{reactable.commentable_type}"
+      cache_buster.bust user.path
+    end
+
+    def occasionally_sync_reaction_counts
+      # Fixes any out-of-sync positive_reactions_count
+      if rand(6) == 1 || reactable.positive_reactions_count.negative?
+        reactable.update_column(:positive_reactions_count, reactable.reactions.where("points > ?", 0).size)
+      end
+    end
+  end
+end

app/models/reaction.rb

@@ -1,84 +1,60 @@
 class Reaction < ApplicationRecord
+  CATEGORIES = %w(like readinglist unicorn thinking hands thumbsdown vomit).freeze
+
   belongs_to :reactable, polymorphic: true
+  belongs_to :user
+
   counter_culture :reactable,
     column_name: proc { |model|
       model.points.positive? ? "positive_reactions_count" : "reactions_count"
     }
   counter_culture :user
-  belongs_to :user
 
-  validates :category, inclusion: { in: %w(like thinking hands unicorn thumbsdown vomit readinglist) }
+  validates :category, inclusion: { in: CATEGORIES }
   validates :reactable_type, inclusion: { in: %w(Comment Article) }
   validates :status, inclusion: { in: %w(valid invalid confirmed) }
   validates :user_id, uniqueness: { scope: %i[reactable_id reactable_type category] }
   validate  :permissions
 
   before_save :assign_points
-  after_save :update_reactable
-  after_save :touch_user
-  after_save :async_bust
-  before_destroy :update_reactable_without_delay
-  before_destroy :clean_up_before_destroy
+  after_save :update_reactable, :async_bust
+  before_destroy :update_reactable, :clean_up_before_destroy
 
-  scope :for_article, ->(id) { where(reactable_id: id, reactable_type: "Article") }
-
-  def self.count_for_article(id)
-    Rails.cache.fetch("count_for_reactable-Article-#{id}", expires_in: 1.hour) do
-      reactions = Reaction.for_article(id)
-
-      ["like", "readinglist", "unicorn"].map do |type|
-        { category: type, count: reactions.where(category: type).size }
+  class << self
+    def count_for_article(id)
+      Rails.cache.fetch("count_for_reactable-Article-#{id}", expires_in: 1.hour) do
+        reactions = Reaction.where(reactable_id: id, reactable_type: "Article")
+        %w(like readinglist unicorn).map do |type|
+          { category: type, count: reactions.where(category: type).size }
+        end
       end
     end
-  end
 
-  def self.for_display(user)
-    includes(:reactable).
-      where(reactable_type: "Article", user_id: user.id).
-      where("created_at > ?", 5.days.ago).
-      select("distinct on (reactable_id) *").
-      take(15)
-  end
+    def for_display(user)
+      includes(:reactable).
+        where(reactable_type: "Article", user: user).
+        where("created_at > ?", 5.days.ago).
+        select("distinct on (reactable_id) *").
+        take(15)
+    end
 
-  def self.cached_any_reactions_for?(reactable, user, category)
-    Rails.cache.fetch("any_reactions_for-#{reactable.class.name}-#{reactable.id}-#{user.updated_at}-#{category}", expires_in: 24.hours) do
-      Reaction.
-        where(reactable_id: reactable.id, reactable_type: reactable.class.name, user_id: user.id, category: category).
-        any?
+    def cached_any_reactions_for?(reactable, user, category)
+      cache_name = "any_reactions_for-#{reactable.class.name}-#{reactable.id}-#{user.updated_at}-#{category}"
+      Rails.cache.fetch(cache_name, expires_in: 24.hours) do
+        Reaction.where(reactable: reactable, user: user, category: category).any?
+      end
     end
   end
 
   private
 
   def update_reactable
-    if reactable_type == "Article"
-      update_article
-    elsif reactable_type == "Comment" && reactable
-      update_comment
+    if destroyed?
+      Reaction::UpdateRecordsJob.perform_now(reactable, user)
+    else
+      Reaction::UpdateRecordsJob.perform_later(reactable, user)
     end
-    occasionally_sync_reaction_counts
   end
-  handle_asynchronously :update_reactable
-
-  def update_article
-    cache_buster = CacheBuster.new
-    reactable.async_score_calc
-    reactable.index!
-    cache_buster.bust "/reactions?article_id=#{reactable_id}"
-    cache_buster.bust user.path
-  end
-
-  def update_comment
-    cache_buster = CacheBuster.new
-    reactable.save unless destroyed_by_association
-    cache_buster.bust "/reactions?commentable_id=#{reactable.commentable_id}&commentable_type=#{reactable.commentable_type}"
-    cache_buster.bust user.path
-  end
-
-  def touch_user
-    user.touch
-  end
-  handle_asynchronously :touch_user
 
   def async_bust
     featured_articles = Article.where(featured: true).order("hotness_score DESC").limit(3).pluck(:id)
@@ -119,13 +95,6 @@ class Reaction < ApplicationRecord
     end
   end
 
-  def occasionally_sync_reaction_counts
-    # Fixes any out-of-sync positive_reactions_count
-    if rand(6) == 1 || reactable.positive_reactions_count.negative?
-      reactable.update_column(:positive_reactions_count, reactable.reactions.where("points > ?", 0).size)
-    end
-  end
-
   def negative_reaction_from_untrusted_user?
     negative? && !user.trusted
   end

spec/jobs/reaction/update_records_job_spec.rb

@@ -0,0 +1,5 @@
+require "rails_helper"
+
+RSpec.describe Reaction::UpdateRecordsJob, type: :job do
+  pending "add some examples to (or delete) #{__FILE__}"
+end

spec/models/reaction_spec.rb

@@ -2,15 +2,10 @@
 require "rails_helper"
 
 RSpec.describe Reaction, type: :model do
-  let(:user)        { create(:user) }
-  let(:author)      { create(:user) }
-  let(:article)     { create(:article, user_id: author.id, featured: true) }
-  let(:comment) do
-    create(:comment, user_id: user.id, commentable_id: article.id, commentable_type: "Article")
-  end
-  let(:reaction) do
-    build(:reaction, reactable: comment, reactable_type: "Comment")
-  end
+  let(:user) { create(:user) }
+  let(:article) { create(:article, featured: true) }
+  let(:comment) { create(:comment, user: user, commentable: article) }
+  let(:reaction) { build(:reaction, reactable: comment) }
 
   describe "actual validation" do
     subject { Reaction.new(reactable: article, reactable_type: "Article", user: user) }
@@ -18,7 +13,7 @@ RSpec.describe Reaction, type: :model do
     before { user.add_role(:trusted) }
 
     it { is_expected.to belong_to(:user) }
-    it { is_expected.to validate_inclusion_of(:category).in_array(%w(like thinking hands unicorn thumbsdown vomit readinglist)) }
+    it { is_expected.to validate_inclusion_of(:category).in_array(Reaction::CATEGORIES) }
     it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(%i[reactable_id reactable_type category]) }
 
     # Thumbsdown and Vomits test needed
@@ -47,14 +42,10 @@ RSpec.describe Reaction, type: :model do
     end
 
     it "does not allow reaction on unpublished article" do
-      reaction = build(
-        :reaction, user_id: user.id, reactable_id: article.id, reactable_type: "Article"
-      )
+      reaction = build(:reaction, user: user, reactable: article)
       expect(reaction).to be_valid
       article.update_column(:published, false)
-      reaction = build(
-        :reaction, user_id: user.id, reactable_id: article.id, reactable_type: "Article"
-      )
+      reaction = build(:reaction, user: user, reactable: article)
       expect(reaction).not_to be_valid
     end
 
@@ -88,8 +79,8 @@ RSpec.describe Reaction, type: :model do
     it "runs async jobs effectively" do
       u2 = create(:user)
       c2 = create(:comment, commentable_id: article.id)
-      create(:reaction, user_id: u2.id, reactable_id: c2.id, reactable_type: "Comment")
-      create(:reaction, user_id: u2.id, reactable_id: article.id, reactable_type: "Article")
+      create(:reaction, user: u2, reactable: c2)
+      create(:reaction, user: u2, reactable: article)
       expect(reaction).to be_valid
     end
   end