Change Notifications::UpdateJob to Notifications::UpdateWorker and mo…

…ve to sidekiq (#5689) [deploy]

app/models/article.rb

@@ -55,6 +55,7 @@ class Article < ApplicationRecord
   validates :video_closed_caption_track_url, url: { allow_blank: true, schemes: ["https"] }
   validates :video_source_url, url: { allow_blank: true, schemes: ["https"] }
 
+  after_update_commit :update_notifications, if: proc { |article| article.notifications.any? && !article.saved_changes.empty? }
   before_validation :evaluate_markdown
   before_validation :create_slug
   before_create     :create_password
@@ -68,7 +69,6 @@ class Article < ApplicationRecord
   after_save        :update_main_image_background_hex
   after_save        :detect_human_language
   before_save       :update_cached_user
-  after_update      :update_notifications, if: proc { |article| article.notifications.any? && !article.saved_changes.empty? }
   before_destroy    :before_destroy_actions, prepend: true
 
   serialize :ids_for_suggested_articles

app/models/comment.rb

@@ -22,6 +22,7 @@ class Comment < ApplicationRecord
 
   after_create   :after_create_checks
   after_commit   :calculate_score
+  after_update_commit :update_notifications, if: proc { |comment| comment.saved_changes.include? "body_markdown" }
   after_save     :bust_cache
   after_save     :synchronous_bust
   after_destroy  :after_destroy_actions
@@ -31,7 +32,6 @@ class Comment < ApplicationRecord
   after_create_commit :send_to_moderator
   before_save    :set_markdown_character_count, if: :body_markdown
   before_create  :adjust_comment_parent_based_on_depth
-  after_update   :update_notifications, if: proc { |comment| comment.saved_changes.include? "body_markdown" }
   after_update   :remove_notifications, if: :deleted
   after_update   :update_descendant_notifications, if: :deleted
   before_validation :evaluate_markdown, if: -> { body_markdown && commentable }

app/models/notification.rb

@@ -112,7 +112,7 @@ def remove_all_without_delay(notifiable_ids:, notifiable_type:)
     end
 
     def update_notifications(notifiable, action = nil)
-      Notifications::UpdateJob.perform_later(notifiable.id, notifiable.class.name, action)
+      Notifications::UpdateWorker.perform_async(notifiable.id, notifiable.class.name, action)
     end
 
     def fast_destroy_old_notifications(destroy_before_timestamp = 4.months.ago)

app/workers/notifications/update_worker.rb

@@ -0,0 +1,19 @@
+module Notifications
+  class UpdateWorker
+    include Sidekiq::Worker
+
+    sidekiq_options queue: :medium_priority, retry: 10
+
+    def perform(notifiable_id, notifiable_class, action = nil)
+      raise InvalidNotifiableForUpdate, notifiable_class unless %w[Article Comment].include?(notifiable_class)
+
+      notifiable = notifiable_class.constantize.find_by(id: notifiable_id)
+
+      return unless notifiable
+
+      Notifications::Update.call(notifiable, action)
+    end
+  end
+
+  class InvalidNotifiableForUpdate < StandardError; end
+end

spec/models/comment_spec.rb

@@ -325,7 +325,7 @@
       comment = create(:comment, commentable: article)
       child_comment = create(:comment, parent: comment, commentable: article, user: user)
       create(:notification, notifiable: child_comment, user: user)
-      perform_enqueued_jobs do
+      sidekiq_perform_enqueued_jobs do
         comment.update(deleted: true)
       end
       notification = child_comment.notifications.first

spec/models/notification_spec.rb

@@ -1,4 +1,5 @@
 require "rails_helper"
+require "sidekiq/testing"
 
 RSpec.describe Notification, type: :model do
   let_it_be_readonly(:user)            { create(:user) }
@@ -451,22 +452,22 @@
       it "updates the notification with the new article title" do
         new_title = "hehehe hohoho!"
         article.update_attribute(:title, new_title)
+        described_class.update_notifications(article, "Published")
 
-        perform_enqueued_jobs do
-          described_class.update_notifications(article, "Published")
-          expected_notification_article_title = user2.notifications.last.json_data["article"]["title"]
-          expect(expected_notification_article_title).to eq(new_title)
-        end
+        sidekiq_perform_enqueued_jobs
+
+        expected_notification_article_title = user2.notifications.last.json_data["article"]["title"]
+        expect(expected_notification_article_title).to eq(new_title)
       end
 
       it "adds organization data when the article now belongs to an org" do
         article.update(organization_id: organization.id)
+        described_class.update_notifications(article, "Published")
 
-        perform_enqueued_jobs do
-          described_class.update_notifications(article, "Published")
-          expected_notification_organization_id = described_class.last.json_data["organization"]["id"]
-          expect(expected_notification_organization_id).to eq(organization.id)
-        end
+        sidekiq_perform_enqueued_jobs
+
+        expected_notification_organization_id = described_class.last.json_data["organization"]["id"]
+        expect(expected_notification_organization_id).to eq(organization.id)
       end
     end
   end

spec/workers/notifications/update_worker_spec.rb

@@ -0,0 +1,38 @@
+require "rails_helper"
+
+RSpec.describe Notifications::UpdateWorker do
+  describe "#perform_now" do
+    let(:id) { rand(1000) }
+    let(:worker) { subject }
+
+    before do
+      allow(Notifications::Update).to receive(:call)
+    end
+
+    describe "when wrong class is passed" do
+      it "raises an exception" do
+        allow(User).to receive(:find_by).with(id).and_return(double)
+        expect do
+          worker.perform(id, "User")
+        end.to raise_error(Notifications::InvalidNotifiableForUpdate, "User")
+      end
+    end
+
+    describe "when notifiable is not found" do
+      it "does not call the service" do
+        allow(Comment).to receive(:find_by).with(id: id).and_return(nil)
+        worker.perform(id, "Comment", nil)
+        expect(Notifications::Update).not_to have_received(:call)
+      end
+    end
+
+    describe "when notifiable is found" do
+      it "calls the service" do
+        article = double
+        allow(Article).to receive(:find_by).with(id: id).and_return(article)
+        worker.perform(id, "Article", "Published")
+        expect(Notifications::Update).to have_received(:call).with(article, "Published")
+      end
+    end
+  end
+end