Refactor remove_all and remove_each notifications and fix orphaned notifications (#3570)

app/controllers/articles_controller.rb

@@ -279,8 +279,8 @@ class ArticlesController < ApplicationController
     if updated && @article.published && @article.saved_changes["published"] == [false, true]
       Notification.send_to_followers(@article, "Published")
     elsif @article.saved_changes["published"] == [true, false]
-      Notification.remove_all_without_delay(notifiable_id: @article.id, notifiable_type: "Article", action: "Published")
-      Notification.remove_all(notifiable_id: @article.id, notifiable_type: "Article", action: "Reaction")
+      Notification.remove_all_by_action_without_delay(notifiable_ids: @article.id, notifiable_type: "Article", action: "Published")
+      Notification.remove_all(notifiable_ids: @article.comments.pluck(:id), notifiable_type: "Comment") if @article.comments.exists?
     end
   end
 

app/jobs/notifications/remove_all_by_action_job.rb

@@ -0,0 +1,11 @@
+module Notifications
+  class RemoveAllByActionJob < ApplicationJob
+    queue_as :remove_all_by_action_notifications
+
+    def perform(notifiable_ids, notifiable_type, action, service = Notifications::RemoveAllByAction)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      service.call(Array.wrap(notifiable_ids), notifiable_type, action)
+    end
+  end
+end

app/jobs/notifications/remove_all_job.rb

@@ -2,8 +2,10 @@ module Notifications
   class RemoveAllJob < ApplicationJob
     queue_as :remove_all_notifications
 
-    def perform(notifiable_id, notifiable_type, action, service = Notifications::RemoveAll)
-      service.call(notifiable_id, notifiable_type, action)
+    def perform(notifiable_ids, notifiable_type, service = Notifications::RemoveAll)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      service.call(Array.wrap(notifiable_ids), notifiable_type)
     end
   end
 end

app/jobs/notifications/remove_each_job.rb

@@ -1,11 +0,0 @@
-module Notifications
-  class RemoveEachJob < ApplicationJob
-    queue_as :remove_each_notifications
-
-    def perform(notifiable_collection_ids, service = Notifications::RemoveEach)
-      return unless notifiable_collection_ids.any?
-
-      service.call(notifiable_collection_ids)
-    end
-  end
-end

app/models/article.rb

@@ -25,7 +25,7 @@ class Article < ApplicationRecord
   has_many :comments, as: :commentable, inverse_of: :commentable
   has_many :profile_pins, as: :pinnable, inverse_of: :pinnable
   has_many :buffer_updates, dependent: :destroy
-  has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
+  has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
   has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
   has_many :rating_votes
   has_many :page_views

app/models/comment.rb

@@ -7,7 +7,7 @@ class Comment < ApplicationRecord
   belongs_to :user
   counter_culture :user
   has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy
-  has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
+  has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all
   has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy
 
   validates :body_markdown, presence: true, length: { in: 1..25_000 },
@@ -186,9 +186,7 @@ class Comment < ApplicationRecord
   end
 
   def remove_notifications
-    Notification.remove_all_without_delay(notifiable_id: id, notifiable_type: "Comment")
-    Notification.remove_all_without_delay(notifiable_id: id, notifiable_type: "Comment", action: "Moderation")
-    Notification.remove_all_without_delay(notifiable_id: id, notifiable_type: "Comment", action: "Reaction")
+    Notification.remove_all_without_delay(notifiable_ids: id, notifiable_type: "Comment")
   end
 
   private
@@ -272,7 +270,6 @@ class Comment < ApplicationRecord
   def before_destroy_actions
     commentable.touch(:last_comment_at) if commentable.respond_to?(:last_comment_at)
     ancestors.update_all(updated_at: Time.current)
-    remove_notifications
     bust_cache_without_delay
     remove_algolia_index
   end

app/models/notification.rb

@@ -128,20 +128,28 @@ class Notification < ApplicationRecord
       Notifications::MilestoneJob.perform_now(type, article_id)
     end
 
-    def remove_all(notifiable_id:, notifiable_type:, action: nil)
-      Notifications::RemoveAllJob.perform_later(notifiable_id, notifiable_type, action)
+    def remove_all_by_action(notifiable_ids:, notifiable_type:, action: nil)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      Notifications::RemoveAllByActionJob.perform_later(notifiable_ids, notifiable_type, action)
     end
 
-    def remove_all_without_delay(notifiable_id:, notifiable_type:, action: nil)
-      Notifications::RemoveAllJob.perform_now(notifiable_id, notifiable_type, action)
+    def remove_all_by_action_without_delay(notifiable_ids:, notifiable_type:, action: nil)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      Notifications::RemoveAllByActionJob.perform_now(notifiable_ids, notifiable_type, action)
     end
 
-    def remove_each(notifiable_collection)
-      Notifications::RemoveEachJob.perform_later(notifiable_collection.pluck(:id))
+    def remove_all(notifiable_ids:, notifiable_type:)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      Notifications::RemoveAllJob.perform_later(notifiable_ids, notifiable_type)
     end
 
-    def remove_each_without_delay(notifiable_collection)
-      Notifications::RemoveEachJob.perform_now(notifiable_collection.pluck(:id))
+    def remove_all_without_delay(notifiable_ids:, notifiable_type:)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      Notifications::RemoveAllJob.perform_now(notifiable_ids, notifiable_type)
     end
 
     def update_notifications(notifiable, action = nil)

app/services/articles/destroyer.rb

@@ -4,8 +4,8 @@ module Articles
 
     def call(article, event_dispatcher = Webhook::DispatchEvent)
       article.destroy!
-      Notification.remove_all_without_delay(notifiable_id: article.id, notifiable_type: "Article", action: "Published")
-      Notification.remove_all(notifiable_id: article.id, notifiable_type: "Article", action: "Reaction")
+      Notification.remove_all_without_delay(notifiable_ids: article.id, notifiable_type: "Article")
+      Notification.remove_all(notifiable_ids: article.comments.pluck(:id), notifiable_type: "Comment") if article.comments.exists?
       event_dispatcher.call("article_destroyed", article) if article.published?
     end
   end

app/services/articles/updater.rb

@@ -39,6 +39,11 @@ module Articles
       send_notification = article.published && article.saved_change_to_published_at.present?
       Notification.send_to_followers(article, "Published") if send_notification
 
+      # remove related notifications if unpublished
+      if article.saved_changes["published"] == [true, false]
+        Notification.remove_all_by_action_without_delay(notifiable_ids: article.id, notifiable_type: "Article", action: "Published")
+        Notification.remove_all(notifiable_ids: article.comments.pluck(:id), notifiable_type: "Comment") if article.comments.exists?
+      end
       # don't send only if article keeps being unpublished
       dispatch_event(article) if article.published || was_published
 

app/services/mentions/create_all.rb

@@ -30,7 +30,7 @@ module Mentions
     def delete_removed_mentions(usernames)
       users = User.where(username: usernames)
       mentions = @notifiable.mentions.where.not(user_id: users).destroy_all
-      Notification.remove_each(mentions) if mentions.present?
+      Notification.remove_all(notifiable_ids: mentions.pluck(:id), notifiable_type: "Mention") if mentions.present?
     end
 
     def create_mention(user)

app/services/notifications/remove_all.rb

@@ -1,9 +1,9 @@
 module Notifications
   class RemoveAll
-    def initialize(notifiable_id, notifiable_type, action)
-      @notifiable_id = notifiable_id
-      @notifiable_type = notifiable_type
-      @action = action
+    def initialize(notifiable_ids, notifiable_type)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      @notifiable_collection = notifiable_type.constantize.where(id: notifiable_ids)
     end
 
     def self.call(*args)
@@ -12,14 +12,12 @@ module Notifications
 
     def call
       Notification.where(
-        notifiable_id: notifiable_id,
-        notifiable_type: notifiable_type,
-        action: action,
+        notifiable: notifiable_collection,
       ).delete_all
     end
 
     private
 
-    attr_reader :notifiable_id, :notifiable_type, :action
+    attr_reader :notifiable_collection
   end
 end

app/services/notifications/remove_all_by_action.rb

@@ -0,0 +1,25 @@
+module Notifications
+  class RemoveAllByAction
+    def initialize(notifiable_ids, notifiable_type, action)
+      return unless %w[Article Comment Mention].include?(notifiable_type) && notifiable_ids.present?
+
+      @notifiable_collection = notifiable_type.constantize.where(id: notifiable_ids)
+      @action = action
+    end
+
+    def self.call(*args)
+      new(*args).call
+    end
+
+    def call
+      Notification.where(
+        notifiable: notifiable_collection,
+        action: action,
+      ).delete_all
+    end
+
+    private
+
+    attr_reader :notifiable_collection, :action
+  end
+end

app/services/notifications/remove_each.rb

@@ -1,24 +0,0 @@
-module Notifications
-  class RemoveEach
-    # notifiable_collection_ids: an array of
-    # notifiable objects ids (eg mention)
-    def initialize(notifiable_collection_ids)
-      @notifiable_collection_ids = notifiable_collection_ids
-    end
-
-    def self.call(*args)
-      new(*args).call
-    end
-
-    def call
-      Notification.where(
-        notifiable_id: notifiable_collection_ids,
-        notifiable_type: "Mention",
-      ).delete_all
-    end
-
-    private
-
-    attr_reader :notifiable_collection_ids
-  end
-end

spec/jobs/notifications/remove_all_by_action_job_spec.rb

@@ -0,0 +1,19 @@
+require "rails_helper"
+
+RSpec.describe Notifications::RemoveAllByActionJob, type: :job do
+  include_examples "#enqueues_job", "remove_all_by_action_notifications", {}
+
+  describe "#perform_now" do
+    let(:remove_all_by_action_service) { double }
+    let(:article) { create(:article) }
+
+    before do
+      allow(remove_all_by_action_service).to receive(:call)
+    end
+
+    it "calls the service" do
+      described_class.perform_now(article.id, "Article", "Published", remove_all_by_action_service)
+      expect(remove_all_by_action_service).to have_received(:call).with([article.id], "Article", "Published")
+    end
+  end
+end

spec/jobs/notifications/remove_all_job_spec.rb

@@ -11,8 +11,8 @@ RSpec.describe Notifications::RemoveAllJob, type: :job do
     end
 
     it "calls the service" do
-      described_class.perform_now(666, "Article", "Published", remove_all_service)
-      expect(remove_all_service).to have_received(:call).with(666, "Article", "Published")
+      described_class.perform_now(1, "Article", remove_all_service)
+      expect(remove_all_service).to have_received(:call).with([1], "Article")
     end
   end
 end

spec/jobs/notifications/remove_each_job_spec.rb

@@ -1,25 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Notifications::RemoveEachJob, type: :job do
-  include_examples "#enqueues_job", "remove_each_notifications", []
-
-  describe "#perform_now" do
-    let(:remove_each_service) { double }
-
-    before do
-      allow(remove_each_service).to receive(:call)
-    end
-
-    context "when array is empty" do
-      it "does not call the service" do
-        described_class.perform_now([], remove_each_service)
-        expect(remove_each_service).not_to have_received(:call)
-      end
-    end
-
-    it "calls the service" do
-      described_class.perform_now([935, 936], remove_each_service)
-      expect(remove_each_service).to have_received(:call).with([935, 936])
-    end
-  end
-end

spec/models/article_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe Article, type: :model do
   it { is_expected.to belong_to(:collection).optional }
   it { is_expected.to have_many(:comments) }
   it { is_expected.to have_many(:reactions).dependent(:destroy) }
-  it { is_expected.to have_many(:notifications).dependent(:destroy) }
+  it { is_expected.to have_many(:notifications).dependent(:delete_all) }
   it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) }
   it { is_expected.to validate_presence_of(:user_id) }
   it { is_expected.not_to allow_value("foo").for(:main_image_background_hex_color) }

spec/models/comment_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Comment, type: :model do
     it { is_expected.to belong_to(:commentable) }
     it { is_expected.to have_many(:reactions).dependent(:destroy) }
     it { is_expected.to have_many(:mentions).dependent(:destroy) }
-    it { is_expected.to have_many(:notifications).dependent(:destroy) }
+    it { is_expected.to have_many(:notifications).dependent(:delete_all) }
     it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) }
     it { is_expected.to validate_presence_of(:commentable_id) }
 

spec/models/notification_spec.rb

@@ -457,7 +457,7 @@ RSpec.describe Notification, type: :model do
     end
   end
 
-  describe "#remove_each" do
+  describe "#remove_all" do
     let(:mention) { create(:mention, user_id: user.id, mentionable_id: comment.id, mentionable_type: "Comment") }
     let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id) }
     let(:notifiable_collection) { [mention] }
@@ -466,10 +466,10 @@ RSpec.describe Notification, type: :model do
       create(:notification, user_id: mention.user_id, notifiable_id: mention.id, notifiable_type: "Mention")
     end
 
-    it "removes each mention related notifiable" do
+    it "removes all mention related notifications" do
       perform_enqueued_jobs do
         expect do
-          described_class.remove_each(notifiable_collection)
+          described_class.remove_all(notifiable_ids: mention.id, notifiable_type: "Mention")
         end.to change(described_class, :count).by(-1)
       end
     end

spec/requests/articles/articles_destroy_spec.rb

@@ -14,12 +14,20 @@ RSpec.describe "ArticlesDestroy", type: :request do
     expect(destroyed_article).to be_nil
   end
 
-  it "schedules a RemoveAllJob" do
+  it "schedules a RemoveAllJob if there are comments" do
+    create(:comment, commentable: article, user: user)
     expect do
       delete "/articles/#{article.id}"
     end.to have_enqueued_job(Notifications::RemoveAllJob).once
   end
 
+  it "removes all previous published notifications" do
+    create(:notification, notifiable: article, action: "Published", user: user)
+    expect do
+      delete "/articles/#{article.id}"
+    end.to change(Notification, :count).by(-1)
+  end
+
   it "doesn't destroy another person's article" do
     article2 = create(:article, user_id: create(:user).id)
     expect do

spec/services/articles/destroyer_spec.rb

@@ -13,7 +13,8 @@ RSpec.describe Articles::Destroyer do
     expect(Article.find_by(id: article.id)).to be_nil
   end
 
-  it "schedules removing notifications" do
+  it "schedules removing notifications if there are comments" do
+    create(:comment, commentable: article)
     expect do
       described_class.call(article)
     end.to have_enqueued_job(Notifications::RemoveAllJob).once

spec/services/notifications/remove_all_by_action_spec.rb

@@ -0,0 +1,20 @@
+require "rails_helper"
+
+RSpec.describe Notifications::RemoveAllByAction do
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:organization) { create(:organization) }
+  let(:article) { create(:article) }
+  let(:article2) { create(:article) }
+  let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id, commentable_type: "Article") }
+
+  before do
+    create(:notification, user: user, notifiable_id: article.id, notifiable_type: "Article", action: "Published")
+    create(:notification, user: user2, notifiable_id: article.id, notifiable_type: "Article", action: "Published")
+    create(:notification, organization: organization, notifiable_id: comment.id, notifiable_type: "Comment", action: "Reaction")
+  end
+
+  it "checks all notifications for an article are deleted and only for an article" do
+    expect { described_class.call(article.id, "Article", "Published") }.to change(Notification, :count).by(-2)
+  end
+end

spec/services/notifications/remove_all_spec.rb

@@ -3,18 +3,20 @@ require "rails_helper"
 RSpec.describe Notifications::RemoveAll do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let(:organization) { create(:organization) }
-  let(:article) { create(:article) }
-  let(:article2) { create(:article) }
-  let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id, commentable_type: "Article") }
+  let(:article) { create(:article, user_id: user.id) }
+  let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id) }
+  let(:comment2) { create(:comment, user_id: user2.id, commentable_id: article.id) }
+  let(:mention) { create(:mention, user_id: user.id, mentionable_id: comment.id, mentionable_type: "Comment") }
+  let(:mention2) { create(:mention, user_id: user2.id, mentionable_id: comment2.id, mentionable_type: "Comment") }
+  let(:notifiable_collection_ids) { [mention.id, mention2.id] }
 
   before do
-    create(:notification, user: user, notifiable_id: article.id, notifiable_type: "Article", action: "Published")
-    create(:notification, user: user2, notifiable_id: article.id, notifiable_type: "Article", action: "Published")
-    create(:notification, organization: organization, notifiable_id: comment.id, notifiable_type: "Comment", action: "Reaction")
+    create(:notification, user_id: mention.user.id, notifiable_id: mention.id, notifiable_type: "Mention")
+    create(:notification, user_id: mention2.user.id, notifiable_id: mention2.id, notifiable_type: "Mention")
   end
 
-  it "checks all notifications for an article are deleted and only for an article" do
-    expect { described_class.call(article.id, "Article", "Published") }.to change(Notification, :count).by(-2)
+  it "checks all notifiables are deleted" do
+    notifiables = Mention.all
+    expect { described_class.call(notifiables.pluck(:id), "Mention") }.to change(Notification, :count).by(-2)
   end
 end

spec/services/notifications/remove_each_spec.rb

@@ -1,21 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Notifications::RemoveEach do
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
-  let(:article) { create(:article, user_id: user.id) }
-  let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id) }
-  let(:comment2) { create(:comment, user_id: user2.id, commentable_id: article.id) }
-  let(:mention) { create(:mention, user_id: user.id, mentionable_id: comment.id, mentionable_type: "Comment") }
-  let(:mention2) { create(:mention, user_id: user2.id, mentionable_id: comment2.id, mentionable_type: "Comment") }
-  let(:notifiable_collection_ids) { [mention.id, mention2.id] }
-
-  before do
-    create(:notification, user_id: mention.user.id, notifiable_id: mention.id, notifiable_type: "Mention")
-    create(:notification, user_id: mention2.user.id, notifiable_id: mention2.id, notifiable_type: "Mention")
-  end
-
-  it "checks all notifiables are deleted" do
-    expect { described_class.call(notifiable_collection_ids) }.to change(Notification, :count).by(-2)
-  end
-end