diff --git a/app/models/article.rb b/app/models/article.rb index 421ef47cd..4f874def3 100644 --- a/app/models/article.rb +++ b/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 diff --git a/app/workers/articles/publish_worker.rb b/app/workers/articles/publish_worker.rb index 949d92df1..d60cfe66a 100644 --- a/app/workers/articles/publish_worker.rb +++ b/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 diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index ff8621e57..ab7a9ac99 100644 --- a/spec/factories/articles.rb +++ b/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 } diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index e2d3ccf71..8c22d5b9a 100644 --- a/spec/models/article_spec.rb +++ b/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 diff --git a/spec/requests/articles/articles_update_spec.rb b/spec/requests/articles/articles_update_spec.rb index d25b430f9..4e0919850 100644 --- a/spec/requests/articles/articles_update_spec.rb +++ b/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 } diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index 715b84b62..8ad2bf6f5 100644 --- a/spec/requests/notifications_spec.rb +++ b/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( diff --git a/spec/services/notifications/moderation/send_spec.rb b/spec/services/notifications/moderation/send_spec.rb index 6c8490121..9781633ea 100644 --- a/spec/services/notifications/moderation/send_spec.rb +++ b/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 diff --git a/spec/workers/articles/publish_worker_spec.rb b/spec/workers/articles/publish_worker_spec.rb index 43615a57c..353319c88 100644 --- a/spec/workers/articles/publish_worker_spec.rb +++ b/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