From 69c7df2e56ef761f221bc0f92586779641e2ea47 Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Mon, 27 Feb 2023 14:41:28 +0400 Subject: [PATCH] DEV: do not fabricate a Notification when fabricating a ChatMention (#20450) Initially, the chat_mention db table was created to support notifications. So when creating a `chat_mention` record we were always creating a related `notification` record. So did the ChatMention fabricator. Now we want to use the chat_mention db table in other scenarios. So we started decoupling mentions from notification in 75b81b68. This removes fabrication of Notifications from the ChatMention fabricator. We need to be able to fabricate a ChatMention without a Notification. --- .../chat/spec/components/chat_mailer_spec.rb | 38 ++++++++--- .../chat/spec/fabricators/chat_fabricator.rb | 38 ----------- plugins/chat/spec/lib/message_mover_spec.rb | 9 ++- .../spec/mailers/user_notifications_spec.rb | 63 ++++++++++++++++--- plugins/chat/spec/models/chat_message_spec.rb | 3 +- 5 files changed, 96 insertions(+), 55 deletions(-) diff --git a/plugins/chat/spec/components/chat_mailer_spec.rb b/plugins/chat/spec/components/chat_mailer_spec.rb index 20229526a89..53edd640be3 100644 --- a/plugins/chat/spec/components/chat_mailer_spec.rb +++ b/plugins/chat/spec/components/chat_mailer_spec.rb @@ -40,7 +40,10 @@ describe Chat::ChatMailer do end describe "for chat mentions" do - fab!(:mention) { Fabricate(:chat_mention, user: user_1, chat_message: chat_message) } + fab!(:notification) { Fabricate(:notification) } + fab!(:mention) do + Fabricate(:chat_mention, user: user_1, chat_message: chat_message, notification: notification) + end it "skips users without chat access" do chatters_group.remove(user_1) @@ -151,7 +154,13 @@ describe Chat::ChatMailer do last_unread_mention_when_emailed_id: chat_message.id, ) unread_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: unread_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user_1, + chat_message: unread_message, + notification: notification, + ) described_class.send_unread_mentions_summary @@ -178,7 +187,8 @@ describe Chat::ChatMailer do last_read_message_id: nil, ) new_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_2, chat_message: new_message) + notification = Fabricate(:notification) + Fabricate(:chat_mention, user: user_2, chat_message: new_message, notification: notification) described_class.send_unread_mentions_summary @@ -217,7 +227,13 @@ describe Chat::ChatMailer do ) another_channel_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: another_channel_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user_1, + chat_message: another_channel_message, + notification: notification, + ) expect { described_class.send_unread_mentions_summary }.not_to change( Jobs::UserEmail.jobs, @@ -229,7 +245,13 @@ describe Chat::ChatMailer do another_channel = Fabricate(:category_channel) another_channel_message = Fabricate(:chat_message, chat_channel: another_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: another_channel_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user_1, + chat_message: another_channel_message, + notification: notification, + ) another_channel_membership = Fabricate( :user_chat_channel_membership, @@ -259,7 +281,8 @@ describe Chat::ChatMailer do end it "only queues the job once when the user has mentions and private messages" do - Fabricate(:chat_mention, user: user_1, chat_message: chat_message) + notification = Fabricate(:notification) + Fabricate(:chat_mention, user: user_1, chat_message: chat_message, notification: notification) described_class.send_unread_mentions_summary @@ -275,7 +298,8 @@ describe Chat::ChatMailer do chat_channel: chat_channel, last_read_message_id: chat_message.id, ) - Fabricate(:chat_mention, user: user_2, chat_message: chat_message) + notification = Fabricate(:notification) + Fabricate(:chat_mention, user: user_2, chat_message: chat_message, notification: notification) described_class.send_unread_mentions_summary diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 58953e9cd16..c0b4480325a 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -65,44 +65,6 @@ Fabricator(:chat_mention) do user { Fabricate(:user) } chat_message { Fabricate(:chat_message) } - notification do |attrs| - # All this setup should be in a service we could just call here - # At the moment the logic is all split in a job - channel = attrs[:chat_message].chat_channel - - payload = { - is_direct_message_channel: channel.direct_message_channel?, - mentioned_by_username: attrs[:chat_message].user.username, - chat_channel_id: channel.id, - chat_message_id: attrs[:chat_message].id, - } - - if channel.direct_message_channel? - payload[:chat_channel_title] = channel.title(membership.user) - payload[:chat_channel_slug] = channel.slug - end - - unless attrs[:identifier] == :direct_mentions - case attrs[:identifier] - when :here_mentions - payload[:identifier] = "here" - when :global_mentions - payload[:identifier] = "all" - else - payload[:identifier] = attrs[:identifier] if attrs[:identifier] - payload[:is_group_mention] = true - end - end - - Fabricate( - :notification, - notification_type: Notification.types[:chat_mention], - user: attrs[:user], - data: payload.to_json, - read: attrs[:read], - high_priority: attrs[:high_priority], - ) - end end Fabricator(:chat_message_reaction) do diff --git a/plugins/chat/spec/lib/message_mover_spec.rb b/plugins/chat/spec/lib/message_mover_spec.rb index f78d3f6b0f3..38a5bb513f8 100644 --- a/plugins/chat/spec/lib/message_mover_spec.rb +++ b/plugins/chat/spec/lib/message_mover_spec.rb @@ -109,7 +109,14 @@ describe Chat::MessageMover do it "updates references for reactions, uploads, revisions, mentions, etc." do reaction = Fabricate(:chat_message_reaction, chat_message: message1) upload = Fabricate(:upload_reference, target: message1) - mention = Fabricate(:chat_mention, chat_message: message2, user: acting_user) + notification = Fabricate(:notification) + mention = + Fabricate( + :chat_mention, + chat_message: message2, + user: acting_user, + notification: notification, + ) revision = Fabricate(:chat_message_revision, chat_message: message3) webhook_event = Fabricate(:chat_webhook_event, chat_message: message3) move! diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index fa282a8a133..0aad0db8285 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -163,7 +163,15 @@ describe UserNotifications do describe "email subject" do context "with regular mentions" do - before { Fabricate(:chat_mention, user: user, chat_message: chat_message) } + before do + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: chat_message, + notification: notification, + ) + end it "includes the sender username in the subject" do expected_subject = @@ -194,7 +202,13 @@ describe UserNotifications do user: user, last_read_message_id: another_chat_message.id - 2, ) - Fabricate(:chat_mention, user: user, chat_message: another_chat_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: another_chat_message, + notification: notification, + ) email = described_class.chat_summary(user, {}) @@ -227,7 +241,13 @@ describe UserNotifications do user: user, last_read_message_id: another_chat_message.id - 2, ) - Fabricate(:chat_mention, user: user, chat_message: another_chat_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: another_chat_message, + notification: notification, + ) end expected_subject = @@ -253,7 +273,13 @@ describe UserNotifications do target_users: [sender, user], ) Fabricate(:chat_message, user: sender, chat_channel: channel) - Fabricate(:chat_mention, user: user, chat_message: chat_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: chat_message, + notification: notification, + ) end it "always includes the DM second" do @@ -273,7 +299,15 @@ describe UserNotifications do end describe "When there are mentions" do - before { Fabricate(:chat_mention, user: user, chat_message: chat_message) } + before do + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: chat_message, + notification: notification, + ) + end describe "selecting mentions" do it "doesn't return an email if the user can't see chat" do @@ -377,7 +411,13 @@ describe UserNotifications do ) new_message = Fabricate(:chat_message, user: sender, chat_channel: channel) - Fabricate(:chat_mention, user: user, chat_message: new_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: new_message, + notification: notification, + ) email = described_class.chat_summary(user, {}) @@ -478,7 +518,8 @@ describe UserNotifications do it "includes a view more link when there are more than two mentions" do 2.times do msg = Fabricate(:chat_message, user: sender, chat_channel: channel) - Fabricate(:chat_mention, user: user, chat_message: msg) + notification = Fabricate(:notification) + Fabricate(:chat_mention, user: user, chat_message: msg, notification: notification) end email = described_class.chat_summary(user, {}) @@ -499,7 +540,13 @@ describe UserNotifications do new_message = Fabricate(:chat_message, user: sender, chat_channel: channel, cooked: "New message") - Fabricate(:chat_mention, user: user, chat_message: new_message) + notification = Fabricate(:notification) + Fabricate( + :chat_mention, + user: user, + chat_message: new_message, + notification: notification, + ) email = described_class.chat_summary(user, {}) body = email.html_part.body.to_s diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index 0ff44ba4999..7d16da0ebb2 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -465,7 +465,8 @@ describe ChatMessage do it "destroys chat_mention" do message_1 = Fabricate(:chat_message) - mention_1 = Fabricate(:chat_mention, chat_message: message_1) + notification = Fabricate(:notification) + mention_1 = Fabricate(:chat_mention, chat_message: message_1, notification: notification) message_1.destroy!