From 7245292fe1c51b4cb450d0742cb2abb0bbef7a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 3 Feb 2025 23:56:47 +0100 Subject: [PATCH] FIX: chat was enqueueing too many "chat summary" emails (#31133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit due to an issue with LEFT JOIN, we were enqueue a "chat summary" email for every new messages in a channel, instead of for every new mentions 😬 This bloated the sidekiq queue with a lot of unecessary jobs as seen in - https://meta.discourse.org/t/-/347197 - https://meta.discourse.org/t/-/346542 Thankfully, it wasn't sending those emails as the query for listing the unread mentions and dms was correct when generating the chat summary email. --- plugins/chat/lib/chat/mailer.rb | 54 +++++++++++-------- .../chat/spec/components/chat/mailer_spec.rb | 42 ++++++++++++++- 2 files changed, 73 insertions(+), 23 deletions(-) diff --git a/plugins/chat/lib/chat/mailer.rb b/plugins/chat/lib/chat/mailer.rb index f9c8311f09d..2e043c1a290 100644 --- a/plugins/chat/lib/chat/mailer.rb +++ b/plugins/chat/lib/chat/mailer.rb @@ -44,29 +44,39 @@ module Chat AND uo.chat_email_frequency = #{UserOption.chat_email_frequencies[:when_away]} AND uo.email_level <> #{UserOption.email_level_types[:never]} ), channel_messages AS ( - SELECT DISTINCT ON (chat_channel_id) chat_channel_id, cm.id AS first_unread_id, user_id AS sender_id - FROM chat_messages cm - JOIN users sender ON sender.id = cm.user_id - WHERE cm.created_at > now() - interval '7 days' - AND cm.deleted_at IS NULL - AND NOT cm.created_by_sdk - ORDER BY chat_channel_id, cm.id - ) - SELECT DISTINCT uccm.user_id - FROM user_chat_channel_memberships uccm - JOIN chat_channels cc ON cc.id = uccm.chat_channel_id AND cc.deleted_at IS NULL - JOIN channel_messages cm ON cm.chat_channel_id = cc.id AND cm.sender_id <> uccm.user_id - JOIN eligible_users eu ON eu.id = uccm.user_id - LEFT JOIN chat_mentions mn ON mn.chat_message_id = cm.first_unread_id - LEFT JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = mn.id - LEFT JOIN notifications n ON n.id = cmn.notification_id AND n.user_id = uccm.user_id - WHERE NOT uccm.muted - AND (uccm.last_read_message_id IS NULL OR cm.first_unread_id > uccm.last_read_message_id) - AND (uccm.last_unread_mention_when_emailed_id IS NULL OR cm.first_unread_id > uccm.last_unread_mention_when_emailed_id) - AND ( - (cc.chatable_type = 'DirectMessage' AND eu.allow_private_messages) OR - (cc.chatable_type = 'Category' AND uccm.following AND (n.id IS NULL OR NOT n.read)) + SELECT DISTINCT ON (chat_channel_id) chat_channel_id, cm.id AS first_unread_id, user_id AS sender_id + FROM chat_messages cm + JOIN users sender ON sender.id = cm.user_id + WHERE cm.created_at > now() - interval '1 week' + AND cm.deleted_at IS NULL + AND NOT cm.created_by_sdk + ORDER BY chat_channel_id, cm.id + ), unread_dms AS ( + SELECT DISTINCT uccm.user_id + FROM user_chat_channel_memberships uccm + JOIN chat_channels cc ON cc.id = uccm.chat_channel_id AND cc.deleted_at IS NULL AND cc.chatable_type = 'DirectMessage' + JOIN channel_messages cm ON cm.chat_channel_id = cc.id AND cm.sender_id <> uccm.user_id + JOIN eligible_users eu ON eu.id = uccm.user_id AND eu.allow_private_messages + WHERE NOT uccm.muted + AND (uccm.last_read_message_id IS NULL OR cm.first_unread_id > uccm.last_read_message_id) + AND (uccm.last_unread_mention_when_emailed_id IS NULL OR cm.first_unread_id > uccm.last_unread_mention_when_emailed_id) + ), unread_mentions AS ( + SELECT DISTINCT uccm.user_id + FROM user_chat_channel_memberships uccm + JOIN chat_channels cc ON cc.id = uccm.chat_channel_id AND cc.deleted_at IS NULL AND cc.chatable_type = 'Category' + JOIN channel_messages cm ON cm.chat_channel_id = cc.id AND cm.sender_id <> uccm.user_id + JOIN eligible_users eu ON eu.id = uccm.user_id + JOIN chat_mentions mn ON mn.chat_message_id = cm.first_unread_id + JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = mn.id + JOIN notifications n ON n.id = cmn.notification_id AND n.user_id = uccm.user_id AND NOT n.read + WHERE NOT uccm.muted + AND uccm.following + AND (uccm.last_read_message_id IS NULL OR cm.first_unread_id > uccm.last_read_message_id) + AND (uccm.last_unread_mention_when_emailed_id IS NULL OR cm.first_unread_id > uccm.last_unread_mention_when_emailed_id) ) + SELECT user_id FROM unread_dms + UNION + SELECT user_id FROM unread_mentions SQL end end diff --git a/plugins/chat/spec/components/chat/mailer_spec.rb b/plugins/chat/spec/components/chat/mailer_spec.rb index 6feaacc16dc..2a50d3fe1f5 100644 --- a/plugins/chat/spec/components/chat/mailer_spec.rb +++ b/plugins/chat/spec/components/chat/mailer_spec.rb @@ -53,6 +53,14 @@ describe Chat::Mailer do describe "in a followed channel" do before { followed_channel.add(user) } + describe "there is a new message" do + let!(:chat_message) { create_message(followed_channel, "hello y'all :wave:") } + + it "does not queue a chat summary" do + expect_not_enqueued + end + end + describe "user is @direct mentioned" do let!(:chat_message) do create_message(followed_channel, "hello @#{user.username}", Chat::UserMention) @@ -193,6 +201,14 @@ describe Chat::Mailer do describe "in a non-followed channel" do before { non_followed_channel.add(user).update!(following: false) } + describe "there is a new message" do + let!(:chat_message) { create_message(non_followed_channel, "hello y'all :wave:") } + + it "does not queue a chat summary" do + expect_not_enqueued + end + end + describe "user is @direct mentioned" do before { create_message(non_followed_channel, "hello @#{user.username}", Chat::UserMention) } @@ -221,6 +237,14 @@ describe Chat::Mailer do describe "in a muted channel" do before { muted_channel.add(user).update!(muted: true) } + describe "there is a new message" do + let!(:chat_message) { create_message(muted_channel, "hello y'all :wave:") } + + it "does not queue a chat summary" do + expect_not_enqueued + end + end + describe "user is @direct mentioned" do before { create_message(muted_channel, "hello @#{user.username}", Chat::UserMention) } @@ -247,6 +271,14 @@ describe Chat::Mailer do end describe "in an unseen channel" do + describe "there is a new message" do + let!(:chat_message) { create_message(unseen_channel, "hello y'all :wave:") } + + it "does not queue a chat summary" do + expect_not_enqueued + end + end + describe "user is @direct mentioned" do before { create_message(unseen_channel, "hello @#{user.username}") } @@ -262,6 +294,14 @@ describe Chat::Mailer do expect_not_enqueued end end + + describe "there is an @all mention" do + before { create_message(unseen_channel, "hello @all", Chat::AllMention) } + + it "does not queue a chat summary email" do + expect_not_enqueued + end + end end describe "in a direct message" do @@ -271,7 +311,7 @@ describe Chat::Mailer do expect_enqueued end - it "queues a chat summary email when user isn't following the direct message anymore" do + it "queues a chat summary email even when user isn't following the direct message anymore" do direct_message.membership_for(user).update!(following: false) expect_enqueued end