From 67d568f709a050553ef1f61544f0e4232c92bcd9 Mon Sep 17 00:00:00 2001 From: David Battersby Date: Mon, 6 Jan 2025 17:26:37 +0400 Subject: [PATCH] FIX: stuck notification for mentions within threads (#30546) For mentions within threads, the mentioned user can experience a stuck notification. This is due to thread memberships only being created for users who interact with a thread. Without the membership we cannot track if the message containing the mention was read by the user. The solution to this explored in this PR is: - auto add memberships for mentioned users (only direct mentions for performance reasons). - update channel/thread unread queries to check notification read status AND thread membership last read message id when counting mentions. Previously the mention count would remain until the user notification (containing the mention) was read. However this only happens if the user clicks the notification or clicks dismiss all notifications. When a user navigated to the thread without clicking the notification, the green/urgent badge on chat would remain even after a hard page refresh. --- plugins/chat/app/models/chat/message.rb | 4 ++++ .../app/queries/chat/channel_unreads_query.rb | 8 +++++--- .../app/queries/chat/thread_unreads_query.rb | 9 +++++---- .../components/chat-channel-unread-indicator.gjs | 2 +- .../chat/header/icon/unread-indicator.gjs | 2 +- plugins/chat/spec/models/chat/message_spec.rb | 16 ++++++++++++++++ .../queries/chat/channel_unreads_query_spec.rb | 3 ++- 7 files changed, 34 insertions(+), 10 deletions(-) diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index 271c233f41c..202cf243a8f 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -381,6 +381,10 @@ module Chat new_mentions = parsed_mentions.direct_mentions.pluck(:id) delete_mentions("Chat::UserMention", old_mentions - new_mentions) insert_mentions("Chat::UserMention", new_mentions - old_mentions) + + # add users to threads when they are mentioned to track read status + return if new_mentions.empty? || !in_thread? + User.where(id: new_mentions).each { |user| thread.add(user, notification_level: :normal) } end end end diff --git a/plugins/chat/app/queries/chat/channel_unreads_query.rb b/plugins/chat/app/queries/chat/channel_unreads_query.rb index d23a1a17c94..ec3d0e4e915 100644 --- a/plugins/chat/app/queries/chat/channel_unreads_query.rb +++ b/plugins/chat/app/queries/chat/channel_unreads_query.rb @@ -37,15 +37,17 @@ module Chat ( SELECT COUNT(*) AS mention_count FROM notifications - INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.user_id = :user_id + INNER JOIN user_chat_channel_memberships AS uccm ON uccm.user_id = :user_id INNER JOIN chat_messages ON (data::json->>'chat_message_id')::bigint = chat_messages.id LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id + LEFT JOIN user_chat_thread_memberships AS uctm ON uctm.thread_id = chat_messages.thread_id AND uctm.user_id = :user_id WHERE NOT read - AND user_chat_channel_memberships.chat_channel_id = memberships.chat_channel_id + AND uccm.chat_channel_id = memberships.chat_channel_id AND notifications.user_id = :user_id AND notifications.notification_type = :notification_type_mention - AND (data::json->>'chat_message_id')::bigint > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) AND (data::json->>'chat_channel_id')::bigint = memberships.chat_channel_id + AND (((chat_messages.thread_id IS NULL OR chat_messages.id = chat_threads.original_message_id) AND chat_messages.id > COALESCE(uccm.last_read_message_id, 0)) + OR (chat_messages.thread_id IS NOT NULL AND uctm.id IS NOT NULL AND chat_messages.id > COALESCE(uctm.last_read_message_id, 0))) ) AS mention_count, ( SELECT COUNT(*) AS watched_threads_unread_count diff --git a/plugins/chat/app/queries/chat/thread_unreads_query.rb b/plugins/chat/app/queries/chat/thread_unreads_query.rb index 442aad6c9dd..f1e39adecd6 100644 --- a/plugins/chat/app/queries/chat/thread_unreads_query.rb +++ b/plugins/chat/app/queries/chat/thread_unreads_query.rb @@ -64,16 +64,17 @@ module Chat FROM notifications INNER JOIN chat_messages ON chat_messages.id = (data::json->>'chat_message_id')::bigint INNER JOIN chat_channels ON chat_channels.id = chat_messages.chat_channel_id - INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_messages.chat_channel_id - LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id + INNER JOIN user_chat_channel_memberships AS uccm ON uccm.chat_channel_id = chat_messages.chat_channel_id + INNER JOIN user_chat_thread_memberships AS uctm ON uctm.thread_id = chat_messages.thread_id AND uctm.user_id = :user_id WHERE NOT read AND notifications.user_id = :user_id AND notifications.notification_type = :notification_type_mention - AND user_chat_channel_memberships.user_id = :user_id + AND uccm.user_id = :user_id AND chat_channels.threading_enabled AND chat_messages.deleted_at IS NULL AND chat_messages.thread_id = memberships.thread_id - AND NOT user_chat_channel_memberships.muted + AND (uctm.id IS NOT NULL AND chat_messages.id > COALESCE(uctm.last_read_message_id, 0)) + AND NOT uccm.muted ) AS mention_count, ( SELECT COUNT(*) AS watched_threads_unread_count diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.gjs index 9aeaeaa1481..72f8d7d9062 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.gjs @@ -39,7 +39,7 @@ export default class ChatChannelUnreadIndicator extends Component { } get urgentBadgeCount() { - let totalCount = this.urgentCount; + const totalCount = this.urgentCount; return totalCount > MAX_UNREAD_COUNT ? `${MAX_UNREAD_COUNT}+` : totalCount; } diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/header/icon/unread-indicator.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/header/icon/unread-indicator.gjs index 2a6a55cb0b2..6ae2440b923 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/header/icon/unread-indicator.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/header/icon/unread-indicator.gjs @@ -69,7 +69,7 @@ export default class ChatHeaderIconUnreadIndicator extends Component { } get urgentCountLabel() { - let totalCount = this.onlyMentions ? this.mentionCount : this.urgentCount; + const totalCount = this.onlyMentions ? this.mentionCount : this.urgentCount; return totalCount > MAX_UNREAD_COUNT ? `${MAX_UNREAD_COUNT}+` : totalCount; } diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index a650fdaebd4..bb2bd529c88 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -787,6 +787,22 @@ describe Chat::Message do expect(message.user_mentions.pluck(:target_id)).to match_array(already_mentioned) expect(message.user_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated end + + it "creates thread memberships for mentioned users when replying to a thread" do + thread = Fabricate(:chat_thread) + thread_message = + Fabricate( + :chat_message, + chat_channel: thread.channel, + thread: thread, + message: "cc @#{user3.username} and @#{user4.username}", + ) + + thread_message.cook + thread_message.upsert_mentions + + expect(thread.user_chat_thread_memberships.pluck(:user_id)).to include(user3.id, user4.id) + end end context "with group mentions" do diff --git a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb index a9802931eeb..f8581dfac1e 100644 --- a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb +++ b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb @@ -210,7 +210,7 @@ describe Chat::ChannelUnreadsQuery do end context "for unread mentions in a thread" do - fab!(:thread_om) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:thread_om) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) } fab!(:thread) { Fabricate(:chat_thread, channel: channel_1, original_message: thread_om) } it "does include the original message in the mention count" do @@ -230,6 +230,7 @@ describe Chat::ChannelUnreadsQuery do thread_message_2 = Fabricate(:chat_message, chat_channel: channel_1, thread: thread) create_mention(thread_message_1, channel_1) create_mention(thread_message_2, channel_1) + expect(query.first).to eq( { mention_count: 2,