Fix bugs with article moderator notifications (and make tests less flaky) (#19688)

* Try to mitigate flaky spec by clearing mock space

* Fix for flaky article published spec

* Rubocop

* Tidy up the specs a bit, update the comment

* Modernize article factory (thanks, rubocop)

* Update update spec to use factory

* Use PublishWorker instead of callback

* nth_published_at -> from_newish_author

* Separate published/unpublished specs

* published_at is correct if unchanged

* Add spec for scheduled post

app/models/article.rb

@@ -205,8 +205,6 @@ class Article < ApplicationRecord
   after_save :bust_cache
   after_save :collection_cleanup
 
-  after_create_commit :send_to_moderator, if: :published?
-
   after_update_commit :update_notifications, if: proc { |article|
                                                    article.notifications.any? && !article.saved_changes.empty?
                                                  }
@@ -614,13 +612,6 @@ class Article < ApplicationRecord
     @privileged_reaction_counts ||= reactions.privileged_category.group(:category).count
   end
 
-  def send_to_moderator
-    # using nth_published because it doesn't count draft articles by the new author
-    return if nth_published_by_author > 2
-
-    Notification.send_moderation_notification(self)
-  end
-
   private
 
   def collection_cleanup
@@ -876,7 +867,7 @@ class Article < ApplicationRecord
   end
 
   def correct_published_at?
-    return unless changes["published_at"]
+    return true unless changes["published_at"]
 
     # for drafts (that were never published before) or scheduled articles
     # => allow future or current dates, or no published_at

app/workers/articles/publish_worker.rb

@@ -14,6 +14,10 @@ module Articles
         # create mentions and notifications
         # Send notifications to any mentioned users, followed by any users who follow the article's author.
         Notification.send_to_mentioned_users_and_followers(article)
+
+        # using nth_published because it doesn't count draft articles by the new author
+        from_newish_author = article.nth_published_by_author < 3
+        Notification.send_moderation_notification(article) if from_newish_author
       end
     end
   end

spec/factories/articles.rb

@@ -24,7 +24,7 @@ FactoryBot.define do
     end
 
     co_author_ids { [] }
-    association :user, factory: :user, strategy: :create
+    user factory: %i[user], strategy: :create
     description { Faker::Hipster.paragraph(sentence_count: 1)[0..100] }
     main_image    do
       if with_main_image
@@ -67,6 +67,7 @@ FactoryBot.define do
       past_published_at { 3.days.ago }
     end
 
+    user
     title { generate(:title) }
     published { true }
     tag_list { "javascript, html, discuss" }
@@ -75,6 +76,7 @@ FactoryBot.define do
   end
 
   factory :unpublished_article, class: "Article" do
+    user
     title { generate(:title) }
     published { false }
     published_at { nil }

spec/models/article_spec.rb

@@ -1420,17 +1420,4 @@ RSpec.describe Article do
       end
     end
   end
-
-  xit "does not send moderator notifications when a draft post" do
-    allow(Notification).to receive(:send_moderation_notification)
-
-    draft_post = build(:article, published: false)
-    draft_post.save!
-    expect(draft_post).not_to be_published
-    expect(Notification).not_to have_received(:send_moderation_notification)
-
-    published_post = build(:article, published: true)
-    published_post.save!
-    expect(Notification).to have_received(:send_moderation_notification)
-  end
 end

spec/requests/articles/articles_update_spec.rb

@@ -195,7 +195,7 @@ RSpec.describe "ArticlesUpdate" do
 
     # draft => scheduled
     it "sets published_at according to the timezone when updating draft => scheduled" do
-      draft = create(:article, published: false, user_id: user.id, published_at: nil)
+      draft = create(:unpublished_article, user_id: user.id, published_at: nil)
       attributes[:published] = true
       attributes[:timezone] = "America/Mexico_City"
       put "/articles/#{draft.id}", params: { article: attributes }

spec/requests/notifications_spec.rb

@@ -496,8 +496,6 @@ RSpec.describe "NotificationsIndex" do
       let(:comment)  { create(:comment, user_id: user2.id, commentable_id: article.id, commentable_type: "Article") }
 
       before do
-        # Avoid notifying the mod on the article
-        Article.skip_callback(:commit, :after, :send_to_moderator)
         moderator.add_role(:trusted)
         sign_in moderator
         sidekiq_perform_enqueued_jobs do
@@ -506,10 +504,6 @@ RSpec.describe "NotificationsIndex" do
         get "/notifications"
       end
 
-      after do
-        Article.set_callback(:commit, :after, :send_to_moderator)
-      end
-
       it "renders a round robin notification with the option to opt out", :aggregate_failures do
         expect(response.body).to include "Hey Alice 👋"
         expect(response.body).to include(

spec/services/notifications/moderation/send_spec.rb

@@ -70,12 +70,6 @@ RSpec.describe Notifications::Moderation::Send, type: :service do
       u = create(:user, :trusted, last_reacted_at: 2.days.ago, last_moderation_notification: last_moderation_time)
       u.notification_setting.update(mod_roundrobin_notifications: true)
       allow(User).to receive(:staff_account).and_return(staff_account)
-      # Creating an article calls moderation job which itself calls moderation service
-      Article.skip_callback(:commit, :after, :send_to_moderator)
-    end
-
-    after do
-      Article.set_callback(:commit, :after, :send_to_moderator)
     end
 
     it "calls article_data since parameter is an article" do

spec/workers/articles/publish_worker_spec.rb

@@ -1,104 +1,132 @@
 require "rails_helper"
 
 RSpec.describe Articles::PublishWorker, type: :worker do
+  let(:worker) { subject }
+  let!(:article) { create(:article, published: true) }
+
   include_examples "#enqueues_on_correct_queue", "medium_priority"
 
-  describe "#perform" do
-    let(:worker) { subject }
-    let!(:article) { create(:article, published: true) }
+  it "calls Slack::Messengers::ArticlePublished to send slack notifications" do
+    allow(Slack::Messengers::ArticlePublished).to receive(:call)
+    worker.perform
+    expect(Slack::Messengers::ArticlePublished).to have_received(:call).with(article: article)
+  end
 
-    it "calls Slack::Messengers::ArticlePublished to send slack notifications" do
-      allow(Slack::Messengers::ArticlePublished).to receive(:call)
+  it "sends notifications to mentioned users and followers" do
+    allow(Notification).to receive(:send_to_mentioned_users_and_followers)
+    worker.perform
+    expect(Notification).to have_received(:send_to_mentioned_users_and_followers).with(article)
+  end
+
+  it "schedules Notifications::NotifiableActionWorker" do
+    args = [article.id, "Article", "Published"]
+    sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker, args: args) do
       worker.perform
-      expect(Slack::Messengers::ArticlePublished).to have_received(:call).with(article: article)
     end
+  end
 
-    it "sends notifications to mentioned users and followers" do
-      allow(Notification).to receive(:send_to_mentioned_users_and_followers)
-      worker.perform
-      expect(Notification).to have_received(:send_to_mentioned_users_and_followers).with(article)
-    end
+  it "doesn't send notifications for an old article" do
+    old_article = create(:article, :past, published: true, past_published_at: 1.year.ago)
+    allow(Notification).to receive(:send_to_mentioned_users_and_followers)
+    worker.perform
+    expect(Notification).not_to have_received(:send_to_mentioned_users_and_followers).with(old_article)
+  end
 
-    it "schedules Notifications::NotifiableActionWorker" do
-      args = [article.id, "Article", "Published"]
-      sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker, args: args) do
+  it "doesn't send notifications for a scheduled article" do
+    scheduled_article = create(:article, published: true, published_at: 1.day.from_now)
+    allow(Notification).to receive(:send_to_mentioned_users_and_followers)
+    worker.perform
+    expect(Notification).not_to have_received(:send_to_mentioned_users_and_followers).with(scheduled_article)
+  end
+
+  it "sends moderation notification for a published article" do
+    allow(Notification).to receive(:send_moderation_notification)
+    worker.perform
+    expect(Notification).to have_received(:send_moderation_notification).with(article)
+  end
+
+  it "does not send moderation notification for an unpublished article" do
+    unpublished = create(:unpublished_article, user: article.user)
+    allow(Notification).to receive(:send_moderation_notification)
+    worker.perform
+    expect(Notification).not_to have_received(:send_moderation_notification).with(unpublished)
+  end
+
+  it "does not send moderation notification for a scheduled article" do
+    scheduled = create(:article, published: true, published_at: Date.tomorrow)
+    allow(Notification).to receive(:send_moderation_notification)
+    worker.perform
+    expect(Notification).not_to have_received(:send_moderation_notification).with(scheduled)
+  end
+
+  it "does not send moderation notification for second published article" do
+    second = create(:published_article, title: "This is number two", user: article.user)
+    third = create(:published_article, title: "This is number three", user: article.user)
+    allow(Notification).to receive(:send_moderation_notification)
+    worker.perform
+    expect(Notification).to have_received(:send_moderation_notification).with(article)
+    expect(Notification).to have_received(:send_moderation_notification).with(second)
+    expect(Notification).not_to have_received(:send_moderation_notification).with(third)
+  end
+
+  context "with 2 articles" do
+    let!(:article2) { create(:article, published: true) }
+
+    it "schedules Notifications::NotifiableActionWorker twice for 2 articles" do
+      sidekiq_assert_enqueued_jobs(2, only: Notifications::NotifiableActionWorker) do
         worker.perform
       end
     end
 
-    it "doesn't send notifications for an old article" do
-      old_article = create(:article, :past, published: true, past_published_at: 1.year.ago)
+    it "sends notifications to mentioned users and followers for 2 articles" do
       allow(Notification).to receive(:send_to_mentioned_users_and_followers)
       worker.perform
-      expect(Notification).not_to have_received(:send_to_mentioned_users_and_followers).with(old_article)
+      expect(Notification).to have_received(:send_to_mentioned_users_and_followers).with(article2)
+    end
+  end
+
+  describe "creating notifications" do
+    let!(:user2) { create(:user) }
+    let(:article2) { create(:article, published: true, user: user2) }
+
+    before do
+      user2.follow(article.user)
     end
 
-    it "doesn't send notifications for a scheduled article" do
-      scheduled_article = create(:article, published: true, published_at: 1.day.from_now)
-      allow(Notification).to receive(:send_to_mentioned_users_and_followers)
-      worker.perform
-      expect(Notification).not_to have_received(:send_to_mentioned_users_and_followers).with(scheduled_article)
-    end
-
-    context "with 2 articles" do
-      let!(:article2) { create(:article, published: true) }
-
-      it "schedules Notifications::NotifiableActionWorker twice for 2 articles" do
-        sidekiq_assert_enqueued_jobs(2, only: Notifications::NotifiableActionWorker) do
+    it "creates a notification eventually" do
+      expect do
+        sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
           worker.perform
         end
-      end
-
-      it "sends notifications to mentioned users and followers for 2 articles" do
-        allow(Notification).to receive(:send_to_mentioned_users_and_followers)
-        worker.perform
-        expect(Notification).to have_received(:send_to_mentioned_users_and_followers).with(article2)
-      end
+      end.to change(Notification, :count).by(1)
     end
 
-    describe "creating notifications" do
-      let!(:user2) { create(:user) }
-      let(:article2) { create(:article, published: true, user: user2) }
+    it "creates a context notification as well" do
+      expect do
+        sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
+          worker.perform
+        end
+      end.to change(ContextNotification, :count).by(1)
+    end
 
-      before do
-        user2.follow(article.user)
-      end
+    it "creates a notification for each article" do
+      article2
+      article.user.follow(article2.user)
+      expect do
+        sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
+          worker.perform
+        end
+      end.to change(Notification, :count).by(2)
+    end
 
-      it "creates a notification eventually" do
-        expect do
-          sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
-            worker.perform
-          end
-        end.to change(Notification, :count).by(1)
-      end
-
-      it "creates a context notification as well" do
-        expect do
-          sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
-            worker.perform
-          end
-        end.to change(ContextNotification, :count).by(1)
-      end
-
-      it "creates a notification for each article" do
-        article2
-        article.user.follow(article2.user)
-        expect do
-          sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
-            worker.perform
-          end
-        end.to change(Notification, :count).by(2)
-      end
-
-      it "creates a ContextNotification for each article" do
-        article2
-        article.user.follow(article2.user)
-        expect do
-          sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
-            worker.perform
-          end
-        end.to change(ContextNotification, :count).by(2)
-      end
+    it "creates a ContextNotification for each article" do
+      article2
+      article.user.follow(article2.user)
+      expect do
+        sidekiq_perform_enqueued_jobs(only: Notifications::NotifiableActionWorker) do
+          worker.perform
+        end
+      end.to change(ContextNotification, :count).by(2)
     end
   end
 end