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.
This commit is contained in:
Andrei Prigorshnev 2023-02-27 14:41:28 +04:00 committed by GitHub
parent e82b8c2b06
commit 69c7df2e56
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 96 additions and 55 deletions

View File

@ -40,7 +40,10 @@ describe Chat::ChatMailer do
end end
describe "for chat mentions" do 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 it "skips users without chat access" do
chatters_group.remove(user_1) chatters_group.remove(user_1)
@ -151,7 +154,13 @@ describe Chat::ChatMailer do
last_unread_mention_when_emailed_id: chat_message.id, last_unread_mention_when_emailed_id: chat_message.id,
) )
unread_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) 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 described_class.send_unread_mentions_summary
@ -178,7 +187,8 @@ describe Chat::ChatMailer do
last_read_message_id: nil, last_read_message_id: nil,
) )
new_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) 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 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) 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( expect { described_class.send_unread_mentions_summary }.not_to change(
Jobs::UserEmail.jobs, Jobs::UserEmail.jobs,
@ -229,7 +245,13 @@ describe Chat::ChatMailer do
another_channel = Fabricate(:category_channel) another_channel = Fabricate(:category_channel)
another_channel_message = another_channel_message =
Fabricate(:chat_message, chat_channel: another_channel, user: sender) 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 = another_channel_membership =
Fabricate( Fabricate(
:user_chat_channel_membership, :user_chat_channel_membership,
@ -259,7 +281,8 @@ describe Chat::ChatMailer do
end end
it "only queues the job once when the user has mentions and private messages" do 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 described_class.send_unread_mentions_summary
@ -275,7 +298,8 @@ describe Chat::ChatMailer do
chat_channel: chat_channel, chat_channel: chat_channel,
last_read_message_id: chat_message.id, 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 described_class.send_unread_mentions_summary

View File

@ -65,44 +65,6 @@ Fabricator(:chat_mention) do
user { Fabricate(:user) } user { Fabricate(:user) }
chat_message { Fabricate(:chat_message) } 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 end
Fabricator(:chat_message_reaction) do Fabricator(:chat_message_reaction) do

View File

@ -109,7 +109,14 @@ describe Chat::MessageMover do
it "updates references for reactions, uploads, revisions, mentions, etc." do it "updates references for reactions, uploads, revisions, mentions, etc." do
reaction = Fabricate(:chat_message_reaction, chat_message: message1) reaction = Fabricate(:chat_message_reaction, chat_message: message1)
upload = Fabricate(:upload_reference, target: 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) revision = Fabricate(:chat_message_revision, chat_message: message3)
webhook_event = Fabricate(:chat_webhook_event, chat_message: message3) webhook_event = Fabricate(:chat_webhook_event, chat_message: message3)
move! move!

View File

@ -163,7 +163,15 @@ describe UserNotifications do
describe "email subject" do describe "email subject" do
context "with regular mentions" 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 it "includes the sender username in the subject" do
expected_subject = expected_subject =
@ -194,7 +202,13 @@ describe UserNotifications do
user: user, user: user,
last_read_message_id: another_chat_message.id - 2, 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, {}) email = described_class.chat_summary(user, {})
@ -227,7 +241,13 @@ describe UserNotifications do
user: user, user: user,
last_read_message_id: another_chat_message.id - 2, 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 end
expected_subject = expected_subject =
@ -253,7 +273,13 @@ describe UserNotifications do
target_users: [sender, user], target_users: [sender, user],
) )
Fabricate(:chat_message, user: sender, chat_channel: channel) 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 end
it "always includes the DM second" do it "always includes the DM second" do
@ -273,7 +299,15 @@ describe UserNotifications do
end end
describe "When there are mentions" do 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 describe "selecting mentions" do
it "doesn't return an email if the user can't see chat" 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) 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, {}) 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 it "includes a view more link when there are more than two mentions" do
2.times do 2.times do
msg = Fabricate(:chat_message, user: sender, chat_channel: channel) 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 end
email = described_class.chat_summary(user, {}) email = described_class.chat_summary(user, {})
@ -499,7 +540,13 @@ describe UserNotifications do
new_message = new_message =
Fabricate(:chat_message, user: sender, chat_channel: channel, cooked: "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, {}) email = described_class.chat_summary(user, {})
body = email.html_part.body.to_s body = email.html_part.body.to_s

View File

@ -465,7 +465,8 @@ describe ChatMessage do
it "destroys chat_mention" do it "destroys chat_mention" do
message_1 = Fabricate(:chat_message) 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! message_1.destroy!