Move Articles::ScoreCalcJob to Sidekiq (#5875) [deploy]

* Create Articles::ScoreCalcWorker

* Create Articles::ScoreCalcWorker spec

* Update references to the new ScoreCalcWorker

* Refactoring conditional callbacks and biz logic

- Move update_columns to a model method
- Move conditions on callback to guard clause in callback method
- Make article_destroy_spec more explicit

app/models/article.rb

@@ -64,7 +64,7 @@ class Article < ApplicationRecord
   before_save       :set_caches
   before_save       :fetch_video_duration
   before_save       :clean_data
-  after_save        :async_score_calc, if: :published
+  after_commit      :async_score_calc
   after_save        :bust_cache
   after_save        :update_main_image_background_hex
   after_save        :detect_human_language
@@ -393,6 +393,13 @@ class Article < ApplicationRecord
     "articles-#{id}"
   end
 
+  def update_score
+    update_columns(score: reactions.sum(:points),
+                   comment_score: comments.sum(:score),
+                   hotness_score: BlackBox.article_hotness_score(self),
+                   spaminess_rating: BlackBox.calculate_spaminess(self))
+  end
+
   private
 
   def delete_related_objects
@@ -450,7 +457,9 @@ class Article < ApplicationRecord
   end
 
   def async_score_calc
-    Articles::ScoreCalcJob.perform_later(id)
+    return if !published? || destroyed?
+
+    Articles::ScoreCalcWorker.perform_async(id)
   end
 
   def fetch_video_duration

app/workers/articles/score_calc_worker.rb

@@ -0,0 +1,15 @@
+module Articles
+  class ScoreCalcWorker
+    include Sidekiq::Worker
+
+    sidekiq_options queue: :medium_priority
+
+    def perform(article_id)
+      article = Article.find_by(id: article_id)
+      return unless article
+
+      article.update_score
+      article.index!
+    end
+  end
+end

spec/jobs/reactions/update_reactable_job_spec.rb

@@ -10,9 +10,9 @@ RSpec.describe Reactions::UpdateReactableJob, type: :job do
     let(:comment_reaction) { create(:reaction, reactable: comment) }
 
     it "updates the reactable Article" do
-      expect do
+      sidekiq_assert_enqueued_with(job: Articles::ScoreCalcWorker, args: [article.id]) do
         described_class.perform_now(reaction.id)
-      end.to have_enqueued_job(Articles::ScoreCalcJob).exactly(:once).with(article.id)
+      end
     end
 
     it "updates the reactable Comment" do

spec/models/article_destroy_spec.rb

@@ -2,12 +2,16 @@ require "rails_helper"
 
 RSpec.describe Article, type: :model do
   context "when no organization" do
-    let(:article) { create(:article) }
+    # Setting published explicitly to true to pass guard clause in the async_score_calc method on
+    # the Article model that returns early if the article is unpublished
+    let(:article) { create(:article, published: true) }
 
     before { create(:reaction, reactable: article) }
 
-    it "doesn't create ScoreCalcJob on destroy" do
-      expect { article.destroy }.not_to have_enqueued_job(Articles::ScoreCalcJob)
+    it "doesn't create ScoreCalcWorker on destroy" do
+      sidekiq_assert_no_enqueued_jobs(only: Articles::ScoreCalcWorker) do
+        article.destroy
+      end
     end
   end
 

spec/models/article_spec.rb

@@ -597,15 +597,15 @@ RSpec.describe Article, type: :model do
     end
 
     describe "async score calc" do
-      it "enqueues Articles::ScoreCalcJob if published" do
-        assert_enqueued_with(job: Articles::ScoreCalcJob) do
+      it "enqueues Articles::ScoreCalcWorker if published" do
+        sidekiq_assert_enqueued_with(job: Articles::ScoreCalcWorker, args: [article.id]) do
           article.save
         end
       end
 
-      it "does not enqueue Articles::ScoreCalcJob if not published" do
+      it "does not enqueue Articles::ScoreCalcWorker if not published" do
         article = build(:article, published: false)
-        assert_no_enqueued_jobs(only: Articles::ScoreCalcJob) do
+        sidekiq_assert_no_enqueued_jobs(only: Articles::ScoreCalcWorker) do
           article.save
         end
       end

spec/models/reaction_spec.rb

@@ -150,9 +150,11 @@ RSpec.describe Reaction, type: :model do
   end
 
   context "when callbacks are called before destroy" do
-    it "enqueues a ScoreCalcJob on article reaction destroy" do
+    it "enqueues a ScoreCalcWorker on article reaction destroy" do
       reaction = create(:reaction, reactable: article, user: user)
-      expect { reaction.destroy }.to have_enqueued_job(Articles::ScoreCalcJob).exactly(:once)
+      sidekiq_assert_enqueued_with(job: Articles::ScoreCalcWorker, args: [article.id]) do
+        reaction.destroy
+      end
     end
   end
 end

spec/workers/articles/score_calc_worker_spec.rb

@@ -0,0 +1,47 @@
+require "rails_helper"
+
+RSpec.describe Articles::ScoreCalcWorker, type: :worker do
+  let(:worker) { subject }
+
+  # Passing in a random article_id since the worker doesn't actually run
+  include_examples "#enqueues_on_correct_queue", "medium_priority", [456]
+
+  describe "#perform_now" do
+    before do
+      allow(BlackBox).to receive(:article_hotness_score).and_return(373)
+      allow(BlackBox).to receive(:calculate_spaminess).and_return(2)
+    end
+
+    context "with article" do
+      let_it_be(:article) { create(:article) }
+      let_it_be(:comment) { create(:comment, commentable: article, score: 5) }
+      let_it_be(:second_comment) { create(:comment, commentable: article, score: 7) }
+
+      it "updates article scores", :aggregate_failures do
+        allow(Article).to receive(:find_by).and_return(article)
+        allow(article.reactions).to receive(:sum).and_return(7)
+
+        worker.perform(article.id)
+        article.reload
+
+        expect(article.score).to be(7)
+        expect(article.comment_score).to be(12)
+        expect(article.hotness_score).to be(373)
+        expect(article.spaminess_rating).to be(2)
+      end
+    end
+
+    context "without article" do
+      it "does not error" do
+        expect { worker.perform(nil) }.not_to raise_error
+      end
+
+      it "does not calculate scores", :aggregate_failures do
+        worker.perform(nil)
+
+        expect(BlackBox).not_to have_received(:article_hotness_score)
+        expect(BlackBox).not_to have_received(:calculate_spaminess)
+      end
+    end
+  end
+end