From 342ab6f0822869b5a35a6776db0aceb062ee2b81 Mon Sep 17 00:00:00 2001 From: David Battersby Date: Mon, 24 Feb 2025 14:25:52 +0400 Subject: [PATCH] FIX: delayed chat summary email (#31255) Updates the chat summary email to account for: - unread mentions in category channels (same as before) - unread direct messages (now excluding threads) - unread watched thread replies (for both channels and DM channels) We have also reduced the window from 1 week down to 1 day for all 3 criteria. The DM unreads query is now properly selecting the first unread message within the window (rather than the first message regardless of read status). --- plugins/chat/config/locales/server.en.yml | 6 ++ ...n_emailed_id_to_chat_thread_memberships.rb | 6 ++ plugins/chat/lib/chat/mailer.rb | 87 ++++++++++++------- .../lib/chat/user_notifications_extension.rb | 64 ++++++++++++-- .../chat/spec/components/chat/mailer_spec.rb | 38 ++++++++ .../chat/spec/fabricators/chat_fabricator.rb | 1 + .../spec/mailers/user_notifications_spec.rb | 62 +++++++++++++ 7 files changed, 228 insertions(+), 36 deletions(-) create mode 100644 plugins/chat/db/migrate/20250220090521_add_last_unread_message_when_emailed_id_to_chat_thread_memberships.rb diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index 0691033ba22..91ac5c90a3d 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -242,6 +242,12 @@ en: private_email: one: "[%{site_name}] New message" other: "[%{site_name}] New messages" + watched_thread: + one: "[%{site_name}] New thread message in %{channel}" + other: "[%{site_name}] New thread messages in %{channel}" + watched_threads: + one: "[%{site_name}] New thread messages in %{channel} and %{count} other" + other: "[%{site_name}] New thread messages in %{channel} and %{count} others" chat_dm_1: one: "[%{site_name}] New message from %{name}" other: "[%{site_name}] New messages from %{name}" diff --git a/plugins/chat/db/migrate/20250220090521_add_last_unread_message_when_emailed_id_to_chat_thread_memberships.rb b/plugins/chat/db/migrate/20250220090521_add_last_unread_message_when_emailed_id_to_chat_thread_memberships.rb new file mode 100644 index 00000000000..6666363fa84 --- /dev/null +++ b/plugins/chat/db/migrate/20250220090521_add_last_unread_message_when_emailed_id_to_chat_thread_memberships.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddLastUnreadMessageWhenEmailedIdToChatThreadMemberships < ActiveRecord::Migration[7.2] + def change + add_column :user_chat_thread_memberships, :last_unread_message_when_emailed_id, :bigint + end +end diff --git a/plugins/chat/lib/chat/mailer.rb b/plugins/chat/lib/chat/mailer.rb index 2e043c1a290..92905904fb8 100644 --- a/plugins/chat/lib/chat/mailer.rb +++ b/plugins/chat/lib/chat/mailer.rb @@ -35,48 +35,73 @@ module Chat DB.query_single <<~SQL WITH eligible_users AS ( - SELECT DISTINCT u.id, uo.allow_private_messages + SELECT u.id, uo.allow_private_messages FROM users u - JOIN user_options uo ON uo.user_id = u.id #{groups_join_sql} + JOIN user_options uo ON uo.user_id = u.id + AND uo.chat_enabled + AND uo.chat_email_frequency = #{UserOption.chat_email_frequencies[:when_away]} + AND uo.email_level <> #{UserOption.email_level_types[:never]} WHERE u.last_seen_at < now() - interval '15 minutes' - AND uo.chat_enabled - 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 '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 + FROM user_chat_channel_memberships uccm 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 + AND eu.allow_private_messages + JOIN chat_messages cm ON cm.chat_channel_id = uccm.chat_channel_id + AND cm.deleted_at IS NULL + AND (cm.thread_id IS NULL OR cm.thread_id IN (SELECT id FROM chat_threads WHERE original_message_id = cm.id)) + AND NOT cm.created_by_sdk + AND cm.created_at > now() - interval '1 day' + JOIN users sender ON sender.id = cm.user_id + JOIN chat_channels cc ON cc.id = cm.chat_channel_id + AND cc.deleted_at IS NULL + AND cc.chatable_type = 'DirectMessage' 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) + AND (uccm.last_read_message_id IS NULL OR uccm.last_read_message_id < cm.id) + AND (uccm.last_unread_mention_when_emailed_id IS NULL OR uccm.last_unread_mention_when_emailed_id < cm.id) + ), unread_mentions AS ( + SELECT DISTINCT n.user_id + FROM notifications n + JOIN eligible_users eu ON eu.id = n.user_id + JOIN chat_mention_notifications cmn ON cmn.notification_id = n.id + JOIN chat_mentions mn ON mn.id = cmn.chat_mention_id + JOIN chat_messages cm ON cm.id = mn.chat_message_id + AND cm.deleted_at IS NULL + AND cm.thread_id IS NULL + AND NOT cm.created_by_sdk + AND cm.created_at > now() - interval '1 day' + JOIN users sender ON sender.id = cm.user_id + JOIN chat_channels cc ON cc.id = cm.chat_channel_id + AND cc.deleted_at IS NULL + AND cc.chatable_type = 'Category' + JOIN user_chat_channel_memberships uccm ON uccm.chat_channel_id = cc.id + AND uccm.user_id = n.user_id + AND NOT uccm.muted + AND uccm.following + AND (uccm.last_read_message_id IS NULL OR uccm.last_read_message_id < cm.id) + AND (uccm.last_unread_mention_when_emailed_id IS NULL OR uccm.last_unread_mention_when_emailed_id < cm.id) + WHERE NOT n.read + ), unread_threads AS ( + SELECT DISTINCT uctm.user_id + FROM user_chat_thread_memberships uctm + JOIN eligible_users eu ON eu.id = uctm.user_id + JOIN chat_threads ct ON ct.id = uctm.thread_id + JOIN chat_messages cm ON cm.thread_id = ct.id + AND cm.deleted_at IS NULL + AND NOT cm.created_by_sdk + AND cm.created_at > now() - interval '1 day' + JOIN users sender ON sender.id = cm.user_id + JOIN chat_channels cc ON cc.id = ct.channel_id + AND cc.deleted_at IS NULL + WHERE uctm.notification_level = #{Chat::NotificationLevels.all[:watching]} + AND (uctm.last_read_message_id IS NULL OR uctm.last_read_message_id < cm.id) ) SELECT user_id FROM unread_dms UNION SELECT user_id FROM unread_mentions + UNION + SELECT user_id FROM unread_threads SQL end end diff --git a/plugins/chat/lib/chat/user_notifications_extension.rb b/plugins/chat/lib/chat/user_notifications_extension.rb index bdf8babb071..48dadd6d6e9 100644 --- a/plugins/chat/lib/chat/user_notifications_extension.rb +++ b/plugins/chat/lib/chat/user_notifications_extension.rb @@ -40,7 +40,7 @@ module Chat AND chat_channels.chatable_type = 'Category' AND chat_messages.deleted_at IS NULL AND chat_messages.user_id != uccm.user_id - AND chat_messages.created_at > now() - interval '1 week' + AND chat_messages.created_at > now() - interval '1 day' AND (uccm.last_read_message_id IS NULL OR uccm.last_read_message_id < chat_messages.id) AND (uccm.last_unread_mention_when_emailed_id IS NULL OR uccm.last_unread_mention_when_emailed_id < chat_messages.id) AND ( @@ -67,15 +67,17 @@ module Chat JOIN chat_channels ON chat_channels.id = uccm.chat_channel_id JOIN chat_messages ON chat_messages.chat_channel_id = chat_channels.id JOIN users ON users.id = chat_messages.user_id + LEFT JOIN chat_threads om ON om.original_message_id = chat_messages.id WHERE uccm.user_id = #{user.id} AND NOT uccm.muted AND chat_channels.deleted_at IS NULL AND chat_channels.chatable_type = 'DirectMessage' AND chat_messages.deleted_at IS NULL AND chat_messages.user_id != uccm.user_id - AND chat_messages.created_at > now() - interval '1 week' + AND chat_messages.created_at > now() - interval '1 day' AND (uccm.last_read_message_id IS NULL OR uccm.last_read_message_id < chat_messages.id) AND (uccm.last_unread_mention_when_emailed_id IS NULL OR uccm.last_unread_mention_when_emailed_id < chat_messages.id) + AND (chat_messages.thread_id IS NULL OR chat_messages.thread_id IN (SELECT id FROM chat_threads WHERE original_message_id = chat_messages.id)) GROUP BY uccm.id ) UPDATE user_chat_channel_memberships uccm @@ -86,12 +88,39 @@ module Chat RETURNING um.membership_id, um.chat_channel_id, um.first_chat_message_id SQL + unread_threads = DB.query_array <<~SQL + WITH unread_threads AS ( + SELECT uctm.id membership_id, uctm.thread_id, MIN(chat_messages.id) first_chat_message_id, MAX(chat_messages.id) last_chat_message_id + FROM user_chat_thread_memberships uctm + JOIN chat_threads ON chat_threads.id = uctm.thread_id + JOIN chat_messages ON chat_messages.thread_id = chat_threads.id + JOIN users ON users.id = chat_messages.user_id + WHERE uctm.user_id = #{user.id} + AND uctm.notification_level = #{Chat::NotificationLevels.all[:watching]} + AND chat_messages.deleted_at IS NULL + AND chat_messages.created_at > now() - interval '1 day' + AND (uctm.last_read_message_id IS NULL OR uctm.last_read_message_id < chat_messages.id) + AND (uctm.last_unread_message_when_emailed_id IS NULL OR uctm.last_unread_message_when_emailed_id < chat_messages.id) + GROUP BY uctm.id + ) + UPDATE user_chat_thread_memberships uctm + SET last_unread_message_when_emailed_id = ut.last_chat_message_id + FROM unread_threads ut + WHERE uctm.id = ut.membership_id + AND uctm.user_id = #{user.id} + RETURNING ut.membership_id, ut.thread_id, ut.first_chat_message_id + SQL + @grouped_channels = chat_messages_for(unread_mentions, guardian) @grouped_dms = user.user_option.allow_private_messages ? chat_messages_for(unread_messages, guardian) : {} - @count = @grouped_channels.values.sum(&:size) + @grouped_dms.values.sum(&:size) + @grouped_threads = chat_messages_for_threads(unread_threads, guardian) + + @count = + @grouped_channels.values.sum(&:size) + @grouped_dms.values.sum(&:size) + + @grouped_threads.values.sum(&:size) return if @count.zero? @@ -103,7 +132,7 @@ module Chat build_email( user.email, from_alias: chat_summary_from_alias, - subject: chat_summary_subject(@grouped_channels, @grouped_dms, @count), + subject: chat_summary_subject(@grouped_channels, @grouped_dms, @grouped_threads, @count), ) end @@ -129,6 +158,22 @@ module Chat .select { |channel, _| guardian.can_join_chat_channel?(channel) } end + def chat_messages_for_threads(data, guardian) + Chat::Message + .includes(:user, :chat_channel) + .where(thread_id: data.map { _1[1] }) + .where( + "chat_messages.id >= ( + SELECT min_unread_id + FROM (VALUES #{data.map { "(#{_1[1]}, #{_1[2]})" }.join(",")}) AS t(thread_id, min_unread_id) + WHERE t.thread_id = chat_messages.thread_id + )", + ) + .order(created_at: :asc) + .group_by(&:chat_channel) + .select { |channel, _| guardian.can_join_chat_channel?(channel) } + end + def chat_summary_from_alias I18n.t("user_notifications.chat_summary.from", site_name: @site_name) end @@ -137,7 +182,7 @@ module Chat I18n.t("user_notifications.chat_summary.subject.#{type}", { site_name: @site_name, **args }) end - def chat_summary_subject(grouped_channels, grouped_dms, count) + def chat_summary_subject(grouped_channels, grouped_dms, grouped_threads, count) return subject(:private_email, count:) if SiteSetting.private_email # consider "direct messages" with more than 2 users as group messages (aka. channels) @@ -177,6 +222,15 @@ module Chat subject(:chat_dm_2, name_1: dms.first.title(@user), name_2: dms.second.title(@user)) elsif dms.size >= 3 subject(:chat_dm_3_or_more, name: dms.first.title(@user), count: dms.size - 1) + elsif grouped_threads.any? + channel = grouped_threads.keys.first + others_count = grouped_threads.keys.size - 1 + + if grouped_threads.keys.size == 1 + subject(:watched_thread, channel: channel.title(@user), count: others_count) + else + subject(:watched_threads, channel: channel.title(@user), count: others_count) + end else subject(:private_email, count:) end diff --git a/plugins/chat/spec/components/chat/mailer_spec.rb b/plugins/chat/spec/components/chat/mailer_spec.rb index 2a50d3fe1f5..65644f56af3 100644 --- a/plugins/chat/spec/components/chat/mailer_spec.rb +++ b/plugins/chat/spec/components/chat/mailer_spec.rb @@ -325,5 +325,43 @@ describe Chat::Mailer do user.user_option.update!(allow_private_messages: false) expect_not_enqueued end + + it "queues a chat summary email when message is the original thread message" do + Fabricate(:chat_thread, channel: direct_message, original_message: Chat::Message.last) + expect_enqueued + end + end + + describe "in direct message channel with threads" do + fab!(:dm_channel) { Fabricate(:direct_message_channel, users: [user, other]) } + fab!(:message) do + Fabricate(:chat_message, chat_channel: dm_channel, user: other, created_at: 2.weeks.ago) + end + fab!(:thread) do + Fabricate(:chat_thread, channel: dm_channel, original_message: message, with_replies: 1) + end + + it "does not queue a chat summary email for thread replies" do + expect_not_enqueued + end + + it "queues a chat summary email when user is watching the thread" do + Fabricate( + :user_chat_thread_membership, + user: user, + thread: thread, + notification_level: Chat::NotificationLevels.all[:watching], + ) + + expect_enqueued + end + + it "does not queue a chat summary for threads watched by other users" do + thread.membership_for(other).update!( + notification_level: Chat::NotificationLevels.all[:watching], + ) + + expect_not_enqueued + end end end diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 60ed5ebc06d..f3c1ac69597 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -265,6 +265,7 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do transients[:with_replies], :chat_message, thread: thread, + chat_channel_id: thread.channel_id, use_service: transients[:use_service], ) .each { |message| thread.add(message.user) } diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index ec5d61b1f8c..04a827128cc 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -579,4 +579,66 @@ describe UserNotifications do end end end + + describe "in a direct message channel with threads" do + fab!(:message) do + Fabricate(:chat_message, chat_channel: direct_message, user: other, created_at: 2.days.ago) + end + fab!(:thread) { Fabricate(:chat_thread, channel: direct_message, original_message: message) } + fab!(:reply) { Fabricate(:chat_message, chat_channel: direct_message, thread:, user: other) } + let(:watching) { Chat::NotificationLevels.all[:watching] } + + it "does not send a chat summary email for thread replies" do + no_chat_summary_email + end + + describe "when the user is watching the thread" do + before do + Fabricate(:user_chat_thread_membership, user: user, thread:, notification_level: watching) + end + + it "sends a chat summary email" do + chat_summary_email + end + end + + describe "when the user has 2 watched threads" do + fab!(:message_2) do + Fabricate( + :chat_message, + chat_channel: direct_message_2, + user: another, + created_at: 2.days.ago, + ) + end + fab!(:thread_2) do + Fabricate(:chat_thread, channel: direct_message_2, original_message: message_2) + end + fab!(:thread_2_reply) do + Fabricate(:chat_message, chat_channel: direct_message_2, thread: thread_2, user: another) + end + + before do + Fabricate(:user_chat_thread_membership, user: user, thread:, notification_level: watching) + Fabricate( + :user_chat_thread_membership, + user: user, + thread: thread_2, + notification_level: watching, + ) + end + + it "sends a chat summary email" do + chat_summary_with_subject(:watched_threads, channel: direct_message.title(user), count: 1) + end + end + + describe "when another user is watching a thread" do + before { thread.membership_for(other).update!(notification_level: watching) } + + it "does not send current user a chat summary email" do + no_chat_summary_email + end + end + end end