diff --git a/app/decorators/mention_decorator.rb b/app/decorators/mention_decorator.rb new file mode 100644 index 000000000..e1346593f --- /dev/null +++ b/app/decorators/mention_decorator.rb @@ -0,0 +1,10 @@ +class MentionDecorator < ApplicationDecorator + def formatted_mentionable_type + # Articles are colloquially referred to as "posts". + mentionable_type == "Article" ? "post" : mentionable_type.downcase + end + + def mentioned_by_blocked_user? + mentionable_type == "User" && UserBlock.blocking?(mentionable_id, user_id) + end +end diff --git a/app/mailers/notify_mailer.rb b/app/mailers/notify_mailer.rb index 2a62c3681..389154ba7 100644 --- a/app/mailers/notify_mailer.rb +++ b/app/mailers/notify_mailer.rb @@ -35,9 +35,11 @@ class NotifyMailer < ApplicationMailer @mentioner = User.find(@mention.mentionable.user_id) @mentionable = @mention.mentionable + @mentionable_type = @mention.decorate.formatted_mentionable_type + @unsubscribe = generate_unsubscribe_token(@user.id, :email_mention_notifications) - mail(to: @user.email, subject: "#{@mentioner.name} just mentioned you!") + mail(to: @user.email, subject: "#{@mentioner.name} just mentioned you in their #{@mentionable_type}") end def unread_notifications_email diff --git a/app/models/article.rb b/app/models/article.rb index f308daee5..fd9733023 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -31,6 +31,7 @@ class Article < ApplicationRecord # The date that we began limiting the number of user mentions in an article. MAX_USER_MENTION_LIVE_AT = Time.utc(2021, 4, 7).freeze + has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy has_many :comments, as: :commentable, inverse_of: :commentable, dependent: :nullify has_many :html_variant_successes, dependent: :nullify has_many :html_variant_trials, dependent: :nullify diff --git a/app/models/notification.rb b/app/models/notification.rb index f33bbe6d0..98d1d9e4a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -40,6 +40,18 @@ class Notification < ApplicationRecord Notifications::NewFollowerWorker.new.perform(follow_data, is_read) end + def send_to_mentioned_users_and_followers(notifiable, _action = nil) + return unless notifiable.is_a?(Article) && notifiable.published? + + # We need to create associated mentions inline because they need to exist _before_ creating any + # other Article-related notifications. This ensures that users will not receive a second notification for the + # post being published if they have already received an initial notification about being @-mentioned in the post. + Mentions::CreateAll.call(notifiable) + + # Kicks off a worker to send any notifications about the post being published, if necessary. + Notification.send_to_followers(notifiable, "Published") + end + def send_to_followers(notifiable, action = nil) Notifications::NotifiableActionWorker.perform_async(notifiable.id, notifiable.class.name, action) end @@ -70,11 +82,17 @@ class Notification < ApplicationRecord end def send_mention_notification(mention) - return if mention.mentionable_type == "User" && UserBlock.blocking?(mention.mentionable_id, mention.user_id) + return if MentionDecorator.new(mention).mentioned_by_blocked_user? Notifications::MentionWorker.perform_async(mention.id) end + def send_mention_notification_without_delay(mention) + return if MentionDecorator.new(mention).mentioned_by_blocked_user? + + Notifications::NewMention::Send.call(mention) if mention + end + def send_welcome_notification(receiver_id, broadcast_id) Notifications::WelcomeNotificationWorker.perform_async(receiver_id, broadcast_id) end diff --git a/app/services/articles/creator.rb b/app/services/articles/creator.rb index 9c2ab2aef..593c28dcc 100644 --- a/app/services/articles/creator.rb +++ b/app/services/articles/creator.rb @@ -16,10 +16,12 @@ module Articles article = save_article if article.persisted? + # Subscribe author to notifications for all comments on their article. NotificationSubscription.create(user: user, notifiable_id: article.id, notifiable_type: "Article", config: "all_comments") - Notification.send_to_followers(article, "Published") if article.published? + # Send notifications to any mentioned users, followed by any users who follow the article's author. + Notification.send_to_mentioned_users_and_followers(article) if article.published? dispatch_event(article) end diff --git a/app/services/articles/updater.rb b/app/services/articles/updater.rb index 112b1953f..a967951ef 100644 --- a/app/services/articles/updater.rb +++ b/app/services/articles/updater.rb @@ -16,7 +16,8 @@ module Articles def call user.rate_limiter.check_limit!(:article_update) - was_published = article.published + # Grab the state of the article's "publish" status before making any further updates to it. + was_previously_published = article.published # updated edited time only if already published and not edited by an admin update_edited_at = article.user == user && article.published @@ -27,21 +28,33 @@ module Articles if success user.rate_limiter.track_limit_by_action(:article_update) - # send notification only the first time an article is published - send_notification = article.published && article.saved_change_to_published_at.present? - Notification.send_to_followers(article, "Published") if send_notification + if article.published && article.saved_change_to_published_at.present? + # The first time that an article is published, we want to send notifications to any mentioned users first, + # and then send notifications to any users who follow the article's author so as to avoid double mentions. + Notification.send_to_mentioned_users_and_followers(article) + elsif article.published + # If the article has already been published and is only being updated, then we need to create + # mentions and send notifications to mentioned users inline via the Mentions::CreateAll service. + Mentions::CreateAll.call(article) + end - # remove related notifications if unpublished + # Remove any associated notifications if Article is unpublished if article.saved_changes["published"] == [true, false] Notification.remove_all_by_action_without_delay(notifiable_ids: article.id, notifiable_type: "Article", action: "Published") + if article.comments.exists? Notification.remove_all(notifiable_ids: article.comments.ids, notifiable_type: "Comment") end + if article.mentions.exists? + Notification.remove_all(notifiable_ids: article.comments.ids, + notifiable_type: "Article") + end end - # don't send only if article keeps being unpublished - dispatch_event(article) if article.published || was_published + + # Do not notify if the article was previously already in a published state or is continually unpublished. + dispatch_event(article) if article.published || was_previously_published end Result.new(success: success, article: article.decorate) end diff --git a/app/services/mentions/create_all.rb b/app/services/mentions/create_all.rb index a7d3753d6..d5675fbc0 100644 --- a/app/services/mentions/create_all.rb +++ b/app/services/mentions/create_all.rb @@ -1,4 +1,7 @@ module Mentions + # This class creates mentions + associated notifications for Articles and Comments. + # This class will check to see if there are any @-mentions in the post, and will + # create the associated mentions inline if necessary. class CreateAll def initialize(notifiable) @notifiable = notifiable @@ -9,7 +12,6 @@ module Mentions end def call - # Only works for comments right now. mentioned_users = users_mentioned_in_text_excluding_author delete_mentions_removed_from_notifiable_text(mentioned_users) @@ -48,7 +50,7 @@ module Mentions end def reject_notifiable_author(users) - users.reject { |user| authored_by?(user, @notifiable) } + users.reject { |user| authored_by?(user, notifiable) } end def authored_by?(user, notifiable) @@ -56,21 +58,34 @@ module Mentions end def delete_mentions_removed_from_notifiable_text(users) - mentions = @notifiable.mentions.where.not(user_id: users).destroy_all + mentions = notifiable.mentions.where.not(user_id: users).destroy_all Notification.remove_all(notifiable_ids: mentions.map(&:id), notifiable_type: "Mention") if mentions.present? end def user_has_comment_notifications?(user) - user.notifications.exists?(notifiable_id: @notifiable.id) + user.notifications.exists?(notifiable_id: notifiable.id, notifiable_type: "Comment") end def create_mention_for(user) - return if user_has_comment_notifications?(user) + # Do not create additional notifications for being mentioned in a comment. + if notifiable.is_a?(Comment) && user_has_comment_notifications?(user) + return + end + + # The mentionable_type is the model that created the mention, the user is the user to be mentioned. + mention = Mention.create(user_id: user.id, mentionable_id: notifiable.id, + mentionable_type: notifiable.class.name) + + # If notifiable is an Article, we need to create the notification for the mention immediately so + # that the notification exists in the database before we attempt to create other Article-related notifications. + # However, if notifiable is a Comment, we can create the notification for the mention in the background. + case notifiable + when Article + Notification.send_mention_notification_without_delay(mention) + when Comment + Notification.send_mention_notification(mention) + end - mention = Mention.create(user_id: user.id, mentionable_id: @notifiable.id, - mentionable_type: @notifiable.class.name) - # mentionable_type = model that created the mention, user = user to be mentioned - Notification.send_mention_notification(mention) mention end diff --git a/app/services/notifications/new_mention/send.rb b/app/services/notifications/new_mention/send.rb index 55ddd908b..2965c50dc 100644 --- a/app/services/notifications/new_mention/send.rb +++ b/app/services/notifications/new_mention/send.rb @@ -3,6 +3,7 @@ module Notifications class Send delegate :user_data, to: Notifications delegate :comment_data, to: Notifications + delegate :article_data, to: Notifications def initialize(mention) @mention = mention @@ -28,7 +29,14 @@ module Notifications def json_data data = { user: user_data(mention.mentionable.user) } - data[:comment] = comment_data(mention.mentionable) if mention.mentionable_type == "Comment" + + case mention.mentionable_type + when "Comment" + data[:comment] = comment_data(mention.mentionable) + when "Article" + data[:article] = article_data(mention.mentionable) + end + data end end diff --git a/app/services/notifications/notifiable_action/send.rb b/app/services/notifications/notifiable_action/send.rb index 1068c7fb4..9178ed69c 100644 --- a/app/services/notifications/notifiable_action/send.rb +++ b/app/services/notifications/notifiable_action/send.rb @@ -25,7 +25,16 @@ module Notifications json_data[:organization] = organization_data(notifiable.organization) if notifiable.organization_id notifications_attributes = [] - notifiable.followers.sort_by(&:updated_at).last(10_000).reverse_each do |follower| + + # If a user was mentioned in the article, they will have already received a mention. + # We explicitly need to exclude them from the article_followers array if they already + # have a mention in order to avoid sending a user multiple notifications for one article. + user_ids_with_article_mentions = notifiable.mentions&.pluck(:user_id) + article_followers = notifiable.followers.reject do |follower| + user_ids_with_article_mentions.include?(follower.id) + end + + article_followers.sort_by(&:updated_at).last(10_000).reverse_each do |follower| now = Time.current notifications_attributes.push( user_id: follower.id, diff --git a/app/views/mailers/notify_mailer/new_mention_email.html.erb b/app/views/mailers/notify_mailer/new_mention_email.html.erb index c42c9a74d..114bc143f 100644 --- a/app/views/mailers/notify_mailer/new_mention_email.html.erb +++ b/app/views/mailers/notify_mailer/new_mention_email.html.erb @@ -1,26 +1,31 @@ - +

- <%= @mentioner.name %> just mentioned you in their <%= @mention.mentionable_type.downcase %>! + <%= @mentioner.name %> just mentioned you in their <%= @mentionable_type %>

- - - - - - - -
-
- <%= @mentionable.processed_html.html_safe %> - View on <%= community_name %> -
+<% if @mentionable.is_a?(Comment) %> + + + + + + + +
+ +
+ <%= @mentionable.processed_html.html_safe %> + View on <%= community_name %> +
+<% else %> + Read their post on <%= community_name %> +<% end %> diff --git a/app/views/notifications/_article.html.erb b/app/views/notifications/_article.html.erb index 89445cae9..5ee39e4c7 100644 --- a/app/views/notifications/_article.html.erb +++ b/app/views/notifications/_article.html.erb @@ -21,40 +21,7 @@

<%= time_ago_in_words json_data["article"]["published_at"] %> ago

-
- " class="crayons-link block notification__preview__inner"> -

<%= h(json_data["article"]["title"]) %>

-
- <% json_data["article"]["cached_tag_list_array"].each do |tag| %> - #<%= tag %> - <% end %> -
- - - <% cache "activity-published-article-reactions-#{@last_user_reaction}-#{json_data['article']['updated_at']}-#{json_data['article']['id']}" do %> - - <% end %> -
+ <%= render "notifications/shared/article_preview", json_data: json_data, notification: notification, context: "default" %> <% end %> diff --git a/app/views/notifications/_mention.html.erb b/app/views/notifications/_mention.html.erb index a911cf5db..9e0966d76 100644 --- a/app/views/notifications/_mention.html.erb +++ b/app/views/notifications/_mention.html.erb @@ -5,9 +5,26 @@ <% end %>
-

- " class="crayons-link fw-bold"><%= json_data["user"]["name"] %> mentioned you in a comment -

- <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "default" %> + <% if notification.json_data["article"] %> +
+

+ " class="crayons-link fw-bold"> + <%= json_data["user"]["name"] %> + + mentioned you in a post + <% if json_data["organization"] %> + under " class="crayons-link fw-bold"><%= json_data["organization"]["name"] %> + <% end %> +

+

<%= time_ago_in_words json_data["article"]["published_at"] %> ago

+
+ + <%= render "notifications/shared/article_preview", json_data: json_data, notification: notification, context: "default" %> + <% elsif notification.json_data["comment"] %> +

+ " class="crayons-link fw-bold"><%= json_data["user"]["name"] %> mentioned you in a comment +

+ <%= render "notifications/shared/comment_box", json_data: json_data, notification: notification, context: "default" %> + <% end %>
diff --git a/app/views/notifications/shared/_article_preview.html.erb b/app/views/notifications/shared/_article_preview.html.erb new file mode 100644 index 000000000..a6c763634 --- /dev/null +++ b/app/views/notifications/shared/_article_preview.html.erb @@ -0,0 +1,34 @@ +
+ " class="crayons-link block notification__preview__inner"> +

<%= h(json_data["article"]["title"]) %>

+
+ <% json_data["article"]["cached_tag_list_array"].each do |tag| %> + #<%= tag %> + <% end %> +
+ + + <% cache "activity-published-article-reactions-#{@last_user_reaction}-#{json_data['article']['updated_at']}-#{json_data['article']['id']}" do %> + + <% end %> +
diff --git a/app/workers/mentions/create_all_worker.rb b/app/workers/mentions/create_all_worker.rb index fa29a945f..784059d71 100644 --- a/app/workers/mentions/create_all_worker.rb +++ b/app/workers/mentions/create_all_worker.rb @@ -1,4 +1,5 @@ module Mentions + # This worker is currently only used to create mentions on comments. class CreateAllWorker include Sidekiq::Worker sidekiq_options queue: :default, retry: 10 diff --git a/spec/decorators/mention_decorator_spec.rb b/spec/decorators/mention_decorator_spec.rb new file mode 100644 index 000000000..6ed379b54 --- /dev/null +++ b/spec/decorators/mention_decorator_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +RSpec.describe MentionDecorator, type: :decorator do + let(:user) { create(:user) } + + describe "#formatted_mentionable_type" do + let(:article) { create(:article) } + let(:comment) { create(:comment, user_id: user.id, commentable: article) } + + it "returns the correct mentionable type for mentions on articles" do + mention = create(:mention, mentionable: article, user: user).decorate + expect(mention.decorate.formatted_mentionable_type).to eq("post") + end + + it "returns the correct mentionable type for mentions on comments" do + mention = create(:mention, mentionable: comment, user: user).decorate + expect(mention.decorate.formatted_mentionable_type).to eq("comment") + end + end + + describe "#mentioned_by_blocked_user?" do + let(:blocked_user) { create(:user) } + let(:mention) { create(:mention, mentionable: user, user: blocked_user) } + + it "returns true if mentioned user has blocked the mentioner" do + create(:user_block, blocker: user, blocked: blocked_user, config: "default") + + expect(mention.decorate.mentioned_by_blocked_user?).to be(true) + end + + it "returns false if mentioned user has not blocked the mentioner" do + expect(mention.decorate.mentioned_by_blocked_user?).to be(false) + end + end +end diff --git a/spec/mailers/notify_mailer_spec.rb b/spec/mailers/notify_mailer_spec.rb index 9ed6b6f47..90435e4aa 100644 --- a/spec/mailers/notify_mailer_spec.rb +++ b/spec/mailers/notify_mailer_spec.rb @@ -48,21 +48,36 @@ RSpec.describe NotifyMailer, type: :mailer do end describe "#new_mention_email" do - let(:mention) { create(:mention, user: user2, mentionable: comment) } - let(:email) { described_class.with(mention: mention).new_mention_email } + context "when mentioning in a comment" do + let(:comment_mention) { create(:mention, user: user2, mentionable: comment) } + let(:email) { described_class.with(mention: comment_mention).new_mention_email } - it "renders proper subject" do - expect(email.subject).to eq("#{comment.user.name} just mentioned you!") + it "renders proper subject and receiver", :aggregate_failures do + expect(email.subject).to eq("#{comment.user.name} just mentioned you in their comment") + expect(email.to).to eq([user2.email]) + end + + it "renders proper sender", :aggregate_failures do + expect(email.from).to eq([SiteConfig.email_addresses[:default]]) + expected_from = "#{SiteConfig.community_name} <#{SiteConfig.email_addresses[:default]}>" + expect(email["from"].value).to eq(expected_from) + end end - it "renders proper sender" do - expect(email.from).to eq([SiteConfig.email_addresses[:default]]) - expected_from = "#{SiteConfig.community_name} <#{SiteConfig.email_addresses[:default]}>" - expect(email["from"].value).to eq(expected_from) - end + context "when mentioning in an article" do + let(:article_mention) { create(:mention, user: user2, mentionable: article) } + let(:email) { described_class.with(mention: article_mention).new_mention_email } - it "renders proper receiver" do - expect(email.to).to eq([user2.email]) + it "renders proper subject and receiver", :aggregate_failures do + expect(email.subject).to eq("#{article.user.name} just mentioned you in their post") + expect(email.to).to eq([user2.email]) + end + + it "renders proper sender", :aggregate_failures do + expect(email.from).to eq([SiteConfig.email_addresses[:default]]) + expected_from = "#{SiteConfig.community_name} <#{SiteConfig.email_addresses[:default]}>" + expect(email["from"].value).to eq(expected_from) + end end end diff --git a/spec/mailers/previews/notify_mailer_preview.rb b/spec/mailers/previews/notify_mailer_preview.rb index f27d9d3e1..057577ff1 100644 --- a/spec/mailers/previews/notify_mailer_preview.rb +++ b/spec/mailers/previews/notify_mailer_preview.rb @@ -13,11 +13,16 @@ class NotifyMailerPreview < ActionMailer::Preview NotifyMailer.with(user: User.last).unread_notifications_email end - def new_mention_email + def new_comment_mention_email mention = Mention.find_or_create_by(user: User.find(1), mentionable: Comment.find(1)) NotifyMailer.with(mention: mention).new_mention_email end + def new_article_mention_email + mention = Mention.find_or_create_by(user: User.find(1), mentionable: Article.find(1)) + NotifyMailer.with(mention: mention).new_mention_email + end + def video_upload_complete_email NotifyMailer.with(article: Article.last).video_upload_complete_email end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index c39d58609..543f38867 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -19,6 +19,7 @@ RSpec.describe Article, type: :model do it { is_expected.to belong_to(:user) } it { is_expected.to have_many(:comments).dependent(:nullify) } + it { is_expected.to have_many(:mentions).dependent(:destroy) } it { is_expected.to have_many(:html_variant_successes).dependent(:nullify) } it { is_expected.to have_many(:html_variant_trials).dependent(:nullify) } it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) } diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 8cac58f5e..631f98a27 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -392,6 +392,66 @@ RSpec.describe Notification, type: :model do end end + describe "send_to_mentioned_users_and_followers" do + let!(:mention_markdown) { "Hello there, @#{user2.username}!" } + + context "when the notifiable is an article from a user" do + before do + article.update!(body_markdown: mention_markdown) + user2.follow(user) + end + + it "sends a single notification to mentioned user", :aggregate_failures do + expect do + sidekiq_perform_enqueued_jobs do + described_class.send_to_mentioned_users_and_followers(article) + end + end.to change(user2.notifications, :count).from(0).to(1) + expect(user2.notifications.first.notifiable_type).to eq("Mention") + end + + it "sends a notification to the organization's followers (who were not mentioned)", :aggregate_failures do + user3.follow(user) + + expect do + sidekiq_perform_enqueued_jobs do + described_class.send_to_mentioned_users_and_followers(article) + end + end.to change(user3.notifications, :count).from(0).to(1) + expect(user3.notifications.first.notifiable_type).to eq("Article") + end + end + + context "when the notifiable is an article from an organization" do + let(:org_article) { create(:article, organization: organization, user: user) } + + before do + org_article.update!(body_markdown: mention_markdown) + user2.follow(user) + end + + it "sends a single notification to mentioned user", :aggregate_failures do + expect do + sidekiq_perform_enqueued_jobs do + described_class.send_to_mentioned_users_and_followers(org_article) + end + end.to change(user2.notifications, :count).from(0).to(1) + expect(user2.notifications.first.notifiable_type).to eq("Mention") + end + + it "sends a notification to the organization's followers (who were not mentioned)", :aggregate_failures do + user3.follow(organization) + + expect do + sidekiq_perform_enqueued_jobs do + described_class.send_to_mentioned_users_and_followers(org_article) + end + end.to change(user3.notifications, :count).from(0).to(1) + expect(user3.notifications.first.notifiable_type).to eq("Article") + end + end + end + describe "#send_to_followers" do context "when the notifiable is an article from a user" do it "sends a notification to the author's followers" do diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index 1dd20fdd6..18928a09e 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -22,6 +22,14 @@ RSpec.describe "NotificationsIndex", type: :request do expect(response.body).to include article.path end + def renders_authors_name(article) + expect(response.body).to include CGI.escapeHTML(article.user.name) + end + + def renders_article_published_at(article) + expect(response.body).to include time_ago_in_words(article.published_at) + end + def renders_comments_html(comment) expect(response.body).to include comment.processed_html end @@ -645,7 +653,7 @@ RSpec.describe "NotificationsIndex", type: :request do end end - context "when a user has a new mention notification" do + context "when a user has a new comment mention notification" do let(:user2) { create(:user) } let(:article) { create(:article, user_id: user.id) } let(:comment) do @@ -673,14 +681,36 @@ RSpec.describe "NotificationsIndex", type: :request do end end - context "when a user has a new article notification" do + context "when a user has a new article mention notification" do + let(:user2) { create(:user) } + let(:article) { create(:article, user_id: user2.id) } + let(:mention) { create(:mention, mentionable: article, user: user) } + + before do + article.update!(body_markdown: "Hello, @#{user.username}!") + sidekiq_perform_enqueued_jobs do + Notification.send_mention_notification(mention) + end + sign_in user + get "/notifications" + end + + it "renders the proper message" do + expect(response.body).to include "mentioned you in a post" + renders_article_path(article) + renders_authors_name(article) + renders_article_published_at(article) + end + end + + context "when a user has a new article created notification" do let(:user2) { create(:user) } let(:article) { create(:article, user_id: user.id) } before do user2.follow(user) sidekiq_perform_enqueued_jobs do - Notification.send_to_followers(article, "Published") + Notification.send_to_mentioned_users_and_followers(article) end sign_in user2 get "/notifications" @@ -693,14 +723,6 @@ RSpec.describe "NotificationsIndex", type: :request do renders_article_published_at(article) end - def renders_authors_name(article) - expect(response.body).to include CGI.escapeHTML(article.user.name) - end - - def renders_article_published_at(article) - expect(response.body).to include time_ago_in_words(article.published_at) - end - it "renders the reaction as previously reacted if it was reacted on" do Reaction.create(user: user2, reactable: article, category: "like") get "/notifications" @@ -720,7 +742,7 @@ RSpec.describe "NotificationsIndex", type: :request do before do user2.follow(user) sidekiq_perform_enqueued_jobs do - Notification.send_to_followers(article, "Published") + Notification.send_to_mentioned_users_and_followers(article) end sign_in admin end diff --git a/spec/services/articles/creator_spec.rb b/spec/services/articles/creator_spec.rb index f8d49df56..b21ddaeeb 100644 --- a/spec/services/articles/creator_spec.rb +++ b/spec/services/articles/creator_spec.rb @@ -26,6 +26,13 @@ RSpec.describe Articles::Creator, type: :service do end end + it "delegates to the Mentions::CreateAll service" do + valid_attributes[:published] = true + allow(Mentions::CreateAll).to receive(:call) + article = described_class.call(user, valid_attributes) + expect(Mentions::CreateAll).to have_received(:call).with(article) + end + it "creates a notification subscription" do expect do described_class.call(user, valid_attributes) @@ -76,6 +83,12 @@ RSpec.describe Articles::Creator, type: :service do end end + it "doesn't delegate to the Mentions::CreateAll service" do + allow(Mentions::CreateAll).to receive(:call) + article = described_class.call(user, invalid_body_attributes) + expect(Mentions::CreateAll).not_to have_received(:call).with(article) + end + it "doesn't create a notification subscription" do expect do described_class.call(user, invalid_body_attributes) diff --git a/spec/services/articles/updater_spec.rb b/spec/services/articles/updater_spec.rb index b848927d4..371696d24 100644 --- a/spec/services/articles/updater_spec.rb +++ b/spec/services/articles/updater_spec.rb @@ -49,24 +49,50 @@ RSpec.describe Articles::Updater, type: :service do end describe "notifications" do - it "sends notifications when an article was published" do - attributes[:published] = true - sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker) do + context "when an article is updated and published the first time" do + before { attributes[:published] = true } + + it "enqueues a job to send a notification" do + sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker) do + described_class.call(user, draft, attributes) + end + end + + it "delegates to the Mentions::CreateAll service to create mentions" do + allow(Mentions::CreateAll).to receive(:call) described_class.call(user, draft, attributes) + expect(Mentions::CreateAll).to have_received(:call).with(draft) end end - it "doesn't send when an article was unpublished" do - attributes[:published] = false - sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do + context "when an article is being updated and has already been published" do + it "doesn't enqueue a job to send a notification" do + attributes[:published] = true + sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do + described_class.call(user, article, attributes) + end + end + + it "delegates to the Mentions::CreateAll service to create mentions" do + allow(Mentions::CreateAll).to receive(:call) described_class.call(user, article, attributes) + expect(Mentions::CreateAll).to have_received(:call).with(article) end end - it "doesn't send when an article went from published to published" do - attributes[:published] = true - sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do + context "when an article is unpublished" do + before { attributes[:published] = false } + + it "doesn't send any notifications" do + sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do + described_class.call(user, article, attributes) + end + end + + it "doesn't delegate to the Mentions::CreateAll service to create mentions" do + allow(Mentions::CreateAll).to receive(:call) described_class.call(user, article, attributes) + expect(Mentions::CreateAll).not_to have_received(:call).with(article) end end end diff --git a/spec/services/mentions/create_all_spec.rb b/spec/services/mentions/create_all_spec.rb index f6bc5395f..04be04cc6 100644 --- a/spec/services/mentions/create_all_spec.rb +++ b/spec/services/mentions/create_all_spec.rb @@ -1,88 +1,139 @@ require "rails_helper" -RSpec.describe Mentions::CreateAll, type: :service do - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:article) { create(:article, user_id: user.id) } - let(:comment) { create(:comment, user_id: user2.id, commentable: article) } - let(:comment2) do - create( - :comment, - body_markdown: "Hello @#{user.username}, you are cool.", - user_id: user2.id, - commentable: article, - ) +RSpec.shared_examples "valid notifiable and no mentions" do + it "does not create mentions if a user is not mentioned" do + set_markdown_and_save(notifiable, markdown) + described_class.call(notifiable) + expect(Mention.all.size).to eq(0) end - it "creates mention if there is a user mentioned and if the user doenst own the comment" do - comment.body_markdown = "Hello @#{user.username}, you are cool." - comment.save - described_class.call(comment) + it "creates a mention if notifiable is updated to include mention", :aggregate_failures do + set_markdown_and_save(notifiable, markdown) + described_class.call(notifiable) + expect(Mention.all.size).to eq(0) + + set_markdown_and_save(notifiable, mention_markdown) + described_class.call(notifiable) + expect(Mention.all.size).to eq(1) + end +end + +RSpec.shared_examples "valid notifiable and has mentions" do + it "creates mention if there is a user mentioned and if the user doesn't own notifiable" do + set_markdown_and_save(notifiable, mention_markdown) + described_class.call(notifiable) expect(Mention.all.size).to eq(1) end - it "deletes mention if deleted from comment" do - comment.body_markdown = "Hello @#{user.username}, you are cool." - comment.save - described_class.call(comment) + it "deletes mention if deleted from notifiable", :aggregate_failures do + set_markdown_and_save(notifiable, mention_markdown) + described_class.call(notifiable) expect(Mention.all.size).to eq(1) - comment.body_markdown = "Hello, you are cool." - comment.save - described_class.call(comment) + + set_markdown_and_save(notifiable, "Hello, you are cool.") + described_class.call(notifiable) expect(Mention.all.size).to eq(0) end it "creates one mention even if multiple mentions of same user" do - comment.body_markdown = "Hello @#{user.username} @#{user.username} @#{user.username}, you rock." - comment.save - described_class.call(comment) + set_markdown_and_save(notifiable, "Hello @#{user.username} @#{user.username} @#{user.username}, you rock.") + described_class.call(notifiable) expect(Mention.all.size).to eq(1) end - it "ignores mentions when embedded within a comment liquid tag" do - markdown = "Check out this comment: {% comment #{comment2.id_code_generated} %}" - comment_with_liquid_tag = create(:comment, user_id: user2.id, commentable: article, body_markdown: markdown) - - comment_with_liquid_tag.save - described_class.call(comment_with_liquid_tag) - expect(Mention.all.size).to eq(0) - end - it "creates multiple mentions for multiple users" do - user2 = create(:user) - comment.body_markdown = "Hello @#{user.username} @#{user2.username}, you are cool." - comment.save - described_class.call(comment) + user3 = create(:user) + + set_markdown_and_save(notifiable, "Hello @#{user.username} @#{user3.username}, you are cool.") + described_class.call(notifiable) expect(Mention.all.size).to eq(2) end - it "deletes one of multiple mentions if one of multiple is deleted" do - user2 = create(:user) - comment.body_markdown = "Hello @#{user.username} @#{user2.username}, you are cool." - comment.save - described_class.call(comment) + it "deletes one of multiple mentions if one of multiple is deleted", :aggregate_failures do + user3 = create(:user) + + set_markdown_and_save(notifiable, "Hello @#{user.username} @#{user3.username}, you are cool.") + described_class.call(notifiable) expect(Mention.all.size).to eq(2) - comment.body_markdown = "Hello @#{user2.username}, you are cool." - comment.save - described_class.call(comment) + + set_markdown_and_save(notifiable, "Hello @#{user3.username}, you are cool.") + described_class.call(notifiable) expect(Mention.all.size).to eq(1) end - it "creates a mention on creation of comment" do - described_class.call(comment2) + it "creates a mention on creation of notifiable" do + described_class.call(notifiable) expect(Mention.all.size).to eq(1) end - it "does not create a mention without valid mentionable" do - comment2.update_column(:body_markdown, "") - described_class.call(comment2) - expect(Mention.all.size).to eq(0) - end - it "does not create a mention when the user mentions themselves" do - comment.body_markdown = "Me, Myself and I @#{user2.username}" - comment.save - described_class.call(comment) + set_markdown_and_save(notifiable, "Me, Myself and I @#{user2.username}") + described_class.call(notifiable) expect(Mention.all.size).to eq(0) end end + +RSpec.shared_examples "valid notifiable with embedded mentions" do + it "does not create a mention" do + comment = create(:comment, user_id: user2.id, commentable: article, body_markdown: "Hi there, @#{user.username}") + liquid_tag_markdown = "Check out this comment: {% comment #{comment.id_code_generated} %}" + + notifiable.update_column(:body_markdown, liquid_tag_markdown) + described_class.call(notifiable) + expect(Mention.all.size).to eq(0) + end +end + +RSpec.shared_examples "invalid notifiable and has mentions" do + it "does not create a mention without valid mentionable" do + notifiable.update_column(:body_markdown, "") + described_class.call(notifiable) + expect(Mention.all.size).to eq(0) + end +end + +RSpec.describe Mentions::CreateAll, type: :service do + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:article) { create(:article, user_id: user.id) } + let(:markdown) { "Hello, you are cool." } + let(:mention_markdown) { "Hello @#{user.username}, you are cool." } + + def set_markdown_and_save(notifiable, markdown) + notifiable.update(body_markdown: markdown) + end + + it_behaves_like "valid notifiable and no mentions" do + let(:notifiable) { create(:comment, body_markdown: markdown, user_id: user2.id, commentable: article) } + end + + it_behaves_like "valid notifiable and has mentions" do + let(:notifiable) { create(:comment, body_markdown: mention_markdown, user_id: user2.id, commentable: article) } + end + + it_behaves_like "valid notifiable with embedded mentions" do + # Explicitly use markdown here without a mention to test that embedded mentions do not trigger a notification. + let(:notifiable) { create(:comment, body_markdown: markdown, user_id: user2.id, commentable: article) } + end + + it_behaves_like "invalid notifiable and has mentions" do + let(:notifiable) { create(:comment, body_markdown: mention_markdown, user_id: user2.id, commentable: article) } + end + + it_behaves_like "valid notifiable and no mentions" do + let(:notifiable) { create(:article, user_id: user2.id) } + end + + it_behaves_like "valid notifiable and has mentions" do + let(:notifiable) { create(:article, user_id: user2.id) } + before { notifiable.update!(body_markdown: mention_markdown) } + end + + it_behaves_like "valid notifiable with embedded mentions" do + let(:notifiable) { create(:article, user_id: user2.id) } + end + + it_behaves_like "invalid notifiable and has mentions" do + let(:notifiable) { create(:article, user_id: user2.id) } + end +end diff --git a/spec/services/notifications/new_mention/send_spec.rb b/spec/services/notifications/new_mention/send_spec.rb index bf959b2d5..e083729d6 100644 --- a/spec/services/notifications/new_mention/send_spec.rb +++ b/spec/services/notifications/new_mention/send_spec.rb @@ -1,9 +1,7 @@ require "rails_helper" -RSpec.describe Notifications::NewMention::Send, type: :service do - let(:user) { create(:user) } - let(:comment) { create(:comment, commentable: create(:article)) } - let(:mention) { create(:mention, mentionable: comment, user: user) } +RSpec.shared_examples "mentionable" do + let(:mention) { create(:mention, mentionable: mentionable, user: user) } it "creates a mention notification" do expect do @@ -11,15 +9,29 @@ RSpec.describe Notifications::NewMention::Send, type: :service do end.to change(Notification, :count).by(1) end - it "creates a correct mention notification" do + it "creates a correct mention notification", :aggregate_failures do notification = described_class.call(mention) + mentionable_type = mentionable.class.to_s.downcase expect(notification.user_id).to eq(user.id) expect(notification.notifiable).to eq(mention) - expect(notification.json_data["comment"]["path"]).to eq(comment.path) + expect(notification.json_data[mentionable_type]["path"]).to eq(mentionable.path) end it "sends from proper mentioner" do notification = described_class.call(mention) - expect(notification.json_data["user"]["id"]).to eq(comment.user_id) + expect(notification.json_data["user"]["id"]).to eq(mentionable.user_id) + end +end + +RSpec.describe Notifications::NewMention::Send, type: :service do + let(:user) { create(:user) } + let!(:article) { create(:article) } + + it_behaves_like "mentionable" do + let(:mentionable) { create(:comment, commentable: article) } + end + + it_behaves_like "mentionable" do + let(:mentionable) { article } end end diff --git a/spec/services/notifications/notifiable_action/send_spec.rb b/spec/services/notifications/notifiable_action/send_spec.rb index e078566b6..8eef86437 100644 --- a/spec/services/notifications/notifiable_action/send_spec.rb +++ b/spec/services/notifications/notifiable_action/send_spec.rb @@ -8,51 +8,71 @@ RSpec.describe Notifications::NotifiableAction::Send, type: :service do let(:user2) { create(:user) } let(:user3) { create(:user) } - before do - user2.follow(user) - user3.follow(organization) - end + context "when following a user or organization" do + before do + user2.follow(user) + user3.follow(organization) + end - it "creates notifications" do - expect do + it "creates notifications" do + expect do + described_class.call(article, "Published") + end.to change(Notification, :count).by(2) + end + + it "creates a correct user notification", :aggregate_failures do described_class.call(article, "Published") - end.to change(Notification, :count).by(2) + notifications = Notification.where(user_id: user2.id, notifiable_id: article.id, notifiable_type: "Article") + expect(notifications.size).to eq(1) + notification = notifications.first + expect(notification.action).to eq("Published") + expect(notification.json_data["article"]["id"]).to eq(article.id) + expect(notification.json_data["user"]["id"]).to eq(user.id) + expect(notification.json_data["user"]["username"]).to eq(user.username) + end + + it "creates a correct organization notification", :aggregate_failures do + described_class.call(article, "Published") + notifications = Notification.where(user_id: user3.id, notifiable_id: article.id, notifiable_type: "Article") + expect(notifications.size).to eq(1) + notification = notifications.first + expect(notification.action).to eq("Published") + expect(notification.json_data["article"]["id"]).to eq(article.id) + expect(notification.json_data["user"]["id"]).to eq(user.id) + expect(notification.json_data["organization"]["id"]).to eq(organization.id) + expect(notification.json_data["organization"]["name"]).to eq(organization.name) + end + + it "does not create a notification if the follower has muted the user" do + user2.follows.first.update(subscription_status: "none") + user3.stop_following(organization) + described_class.call(article, "Published") + expect(Notification.count).to eq(0) + end + + it "doesn't fail if the notification already exists" do + notification = create(:notification, user: user2, action: "Published", notifiable: article) + result = described_class.call(article, "Published") + ids = result.to_a.map { |r| r["id"] } + expect(ids).to include(notification.id) + end end - it "creates a correct user notification" do - described_class.call(article, "Published") - notifications = Notification.where(user_id: user2.id, notifiable_id: article.id, notifiable_type: "Article") - expect(notifications.size).to eq(1) - notification = notifications.first - expect(notification.action).to eq("Published") - expect(notification.json_data["article"]["id"]).to eq(article.id) - expect(notification.json_data["user"]["id"]).to eq(user.id) - expect(notification.json_data["user"]["username"]).to eq(user.username) - end + context "when following a user or organization and being mentioned in an article" do + it "does not create a notification when following a user" do + user2.follow(user) + create(:mention, mentionable: article, user: user2) - it "creates a correct organization notification" do - described_class.call(article, "Published") - notifications = Notification.where(user_id: user3.id, notifiable_id: article.id, notifiable_type: "Article") - expect(notifications.size).to eq(1) - notification = notifications.first - expect(notification.action).to eq("Published") - expect(notification.json_data["article"]["id"]).to eq(article.id) - expect(notification.json_data["user"]["id"]).to eq(user.id) - expect(notification.json_data["organization"]["id"]).to eq(organization.id) - expect(notification.json_data["organization"]["name"]).to eq(organization.name) - end + described_class.call(article, "Published") + expect(Notification.count).to eq(0) + end - it "does not create a notification if the follower has muted the user" do - user2.follows.first.update(subscription_status: "none") - user3.stop_following(organization) - described_class.call(article, "Published") - expect(Notification.count).to eq 0 - end + it "does not create a notification when following an organization" do + user3.follow(organization) + create(:mention, mentionable: article, user: user3) - it "doesn't fail if the notification already exists" do - notification = create(:notification, user: user2, action: "Published", notifiable: article) - result = described_class.call(article, "Published") - ids = result.to_a.map { |r| r["id"] } - expect(ids).to include(notification.id) + described_class.call(article, "Published") + expect(Notification.count).to eq(0) + end end end diff --git a/spec/workers/mentions/send_email_notification_worker_spec.rb b/spec/workers/mentions/send_email_notification_worker_spec.rb index 8c655a104..bd74496b9 100644 --- a/spec/workers/mentions/send_email_notification_worker_spec.rb +++ b/spec/workers/mentions/send_email_notification_worker_spec.rb @@ -1,32 +1,42 @@ require "rails_helper" +RSpec.shared_examples "a valid mentionable" do + context "with a mention" do + it "calls on NotifyMailer" do + worker.perform(mention.id) do + expect(NotifyMailer).to have_received(:new_mention_email).with(mention) + end + end + end + + context "without a mention" do + it "does not error" do + expect { worker.perform(nil) }.not_to raise_error + end + + it "does not call NotifyMailer" do + worker.perform(nil) do + expect(NotifyMailer).not_to have_received(:new_mention_email) + end + end + end +end + RSpec.describe Mentions::SendEmailNotificationWorker, type: :worker do include_examples "#enqueues_on_correct_queue", "default", 1 describe "#perform" do - let(:worker) { subject } - let(:user) { create(:user) } - let(:mention) { create(:mention, user_id: user.id, mentionable_id: comment.id, mentionable_type: "Comment") } - let(:comment) { create(:comment, user_id: user.id, commentable: create(:article)) } + let(:worker) { subject } + let(:user) { create(:user) } + let(:article) { create(:article) } + let(:comment) { create(:comment, user_id: user.id, commentable: article) } - context "with mention" do - it "calls on NotifyMailer" do - worker.perform(mention.id) do - expect(NotifyMailer).to have_received(:new_mention_email).with(mention) - end - end + it_behaves_like "a valid mentionable" do + let(:mention) { create(:mention, user_id: user.id, mentionable_id: comment.id, mentionable_type: "Comment") } end - context "without a mention" do - it "does not error" do - expect { worker.perform(nil) }.not_to raise_error - end - - it "does not call NotifyMailer" do - worker.perform(nil) do - expect(NotifyMailer).not_to have_received(:new_mention_email) - end - end + it_behaves_like "a valid mentionable" do + let(:mention) { create(:mention, user_id: user.id, mentionable_id: article.id, mentionable_type: "Article") } end end end