From ec480e99ef9e9d5ba9ae1513e970aed778040b06 Mon Sep 17 00:00:00 2001 From: David Battersby Date: Thu, 31 Oct 2024 10:50:11 +0400 Subject: [PATCH] FIX: show chat thread notifications for direct message channels (#29414) When adding threads to DM channels in #29170 we intentionally didn't add them to the My Threads section. However this makes it easy to miss notifications as we don't get the new thread badge on the sidebar and footer tabs (drawer/mobile). However they were also missing from the chat header and sidebar too, which is fixed with this PR. When a new thread or a reply to an existing thread is created within a DM channel (either 1:1 or group), we now show the standard badges like we do for public channels. We now also show the green dot in the sidebar for My Threads and public channels when they contain an unread watched thread. --- .../app/queries/chat/thread_unreads_query.rb | 2 +- .../app/services/chat/list_user_channels.rb | 4 +- .../app/services/chat/lookup_user_threads.rb | 8 +-- .../chat-channel-unread-indicator.gjs | 3 +- .../discourse/initializers/chat-sidebar.js | 14 +++- .../discourse/models/chat-channel.js | 2 +- .../services/chat-channels-manager.js | 4 +- .../services/chat-tracking-state-manager.js | 10 +-- .../spec/services/chat/create_thread_spec.rb | 14 +++- plugins/chat/spec/system/chat_footer_spec.rb | 72 +++++++++++++------ 10 files changed, 89 insertions(+), 44 deletions(-) diff --git a/plugins/chat/app/queries/chat/thread_unreads_query.rb b/plugins/chat/app/queries/chat/thread_unreads_query.rb index f3d2f337bcc..4dd52780643 100644 --- a/plugins/chat/app/queries/chat/thread_unreads_query.rb +++ b/plugins/chat/app/queries/chat/thread_unreads_query.rb @@ -46,7 +46,7 @@ module Chat INNER JOIN user_chat_thread_memberships ON user_chat_thread_memberships.thread_id = chat_threads.id INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_messages.chat_channel_id INNER JOIN chat_messages AS original_message ON original_message.id = chat_threads.original_message_id - AND chat_messages.thread_id = memberships.thread_id + WHERE chat_messages.thread_id = memberships.thread_id AND chat_messages.user_id != :user_id AND user_chat_thread_memberships.user_id = :user_id AND chat_messages.id > COALESCE(user_chat_thread_memberships.last_read_message_id, 0) diff --git a/plugins/chat/app/services/chat/list_user_channels.rb b/plugins/chat/app/services/chat/list_user_channels.rb index 3dc5eed57af..e9106ab1362 100644 --- a/plugins/chat/app/services/chat/list_user_channels.rb +++ b/plugins/chat/app/services/chat/list_user_channels.rb @@ -24,9 +24,11 @@ module Chat end def inject_unread_thread_overview(structured:, guardian:) + channel_ids = + structured[:public_channels].map(&:id) + structured[:direct_message_channels].map(&:id) structured[:unread_thread_overview] = ::Chat::TrackingStateReportQuery.call( guardian: guardian, - channel_ids: structured[:public_channels].map(&:id), + channel_ids: channel_ids, include_threads: true, include_read: false, include_last_reply_details: true, diff --git a/plugins/chat/app/services/chat/lookup_user_threads.rb b/plugins/chat/app/services/chat/lookup_user_threads.rb index 863758e661c..eb5a2c10a78 100644 --- a/plugins/chat/app/services/chat/lookup_user_threads.rb +++ b/plugins/chat/app/services/chat/lookup_user_threads.rb @@ -77,13 +77,7 @@ module Chat ::Chat::Channel .joins(:user_chat_channel_memberships) .where(user_chat_channel_memberships: { user_id: guardian.user.id, following: true }) - .where( - { - chatable_type: "Category", - threading_enabled: true, - status: ::Chat::Channel.statuses[:open], - }, - ) + .where({ threading_enabled: true, status: ::Chat::Channel.statuses[:open] }) .select(:id), ) .where("original_message.chat_channel_id = chat_threads.channel_id") 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 21b6347e367..86178c1f789 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 @@ -13,7 +13,8 @@ export default class ChatChannelUnreadIndicator extends Component { this.args.channel.tracking.unreadCount > 0 || // We want to do this so we don't show a blue dot if the user is inside // the channel and a new unread thread comes in. - (this.chat.activeChannel?.id !== this.args.channel.id && + (this.args.channel.isCategoryChannel && + this.chat.activeChannel?.id !== this.args.channel.id && this.args.channel.unreadThreadsCountSinceLastViewed > 0) ); } diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js index faaf2ca0a04..55d6191ad09 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js @@ -63,7 +63,6 @@ export default { prefixType = "icon"; prefixValue = "discourse-threads"; suffixType = "icon"; - suffixCSSClass = "unread"; constructor() { super(...arguments); @@ -74,12 +73,20 @@ export default { } get suffixValue() { - return chatChannelsManager.publicMessageChannels.some( + return chatChannelsManager.allChannels.some( (channel) => channel.unreadThreadsCount > 0 ) ? "circle" : ""; } + + get suffixCSSClass() { + return chatChannelsManager.allChannels.some( + (channel) => channel.tracking.watchedThreadsUnreadCount > 0 + ) + ? "urgent" + : "unread"; + } }; const SidebarChatMyThreadsSection = class extends BaseCustomSidebarSection { @@ -196,7 +203,8 @@ export default { } get suffixCSSClass() { - return this.channel.tracking.mentionCount > 0 + return this.channel.tracking.mentionCount > 0 || + this.channel.tracking.watchedThreadsUnreadCount > 0 ? "urgent" : "unread"; } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index ab4ecfcedb4..2c6969d5999 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -111,7 +111,7 @@ export default class ChatChannel { } get unreadThreadsCount() { - return Array.from(this.threadsManager.unreadThreadOverview.values()).length; + return this.threadsManager.unreadThreadCount; } get watchedThreadsUnreadCount() { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js index 7708120edd2..56a3a8e4ca4 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js @@ -108,9 +108,7 @@ export default class ChatChannelsManager extends Service { @cached get hasThreadedChannels() { - return this.publicMessageChannels?.some( - (channel) => channel.threadingEnabled - ); + return this.allChannels?.some((channel) => channel.threadingEnabled); } get allChannels() { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-tracking-state-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-tracking-state-manager.js index f5943e9114e..2ba505b8e2f 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-tracking-state-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-tracking-state-manager.js @@ -78,13 +78,11 @@ export default class ChatTrackingStateManager extends Service { } get hasUnreadThreads() { - return this.#publicChannels.some( - (channel) => channel.unreadThreadsCount > 0 - ); + return this.#allChannels.some((channel) => channel.unreadThreadsCount > 0); } get watchedThreadsUnreadCount() { - return this.#publicChannels.reduce((unreadCount, channel) => { + return this.#allChannels.reduce((unreadCount, channel) => { return unreadCount + channel.tracking.watchedThreadsUnreadCount; }, 0); } @@ -129,4 +127,8 @@ export default class ChatTrackingStateManager extends Service { get #directMessageChannels() { return this.chatChannelsManager.directMessageChannels; } + + get #allChannels() { + return this.chatChannelsManager.allChannels; + } } diff --git a/plugins/chat/spec/services/chat/create_thread_spec.rb b/plugins/chat/spec/services/chat/create_thread_spec.rb index 83cc6d37bd4..218e42243f5 100644 --- a/plugins/chat/spec/services/chat/create_thread_spec.rb +++ b/plugins/chat/spec/services/chat/create_thread_spec.rb @@ -11,8 +11,11 @@ RSpec.describe Chat::CreateThread do subject(:result) { described_class.call(params:, **dependencies) } fab!(:current_user) { Fabricate(:user) } + fab!(:another_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) } fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:dm_channel) { Fabricate(:direct_message_channel, users: [current_user, another_user]) } + fab!(:dm_message) { Fabricate(:chat_message, chat_channel: dm_channel) } let(:guardian) { Guardian.new(current_user) } let(:title) { nil } @@ -39,11 +42,20 @@ RSpec.describe Chat::CreateThread do expect(result.membership).to eq(result.thread.membership_for(current_user)) end - it "publishes a `thread_created` MessageBus event" do + it "publishes a `thread_created` MessageBus event for public channels" do message = MessageBus.track_publish("/chat/#{channel_1.id}") { result }.first expect(message.data["type"]).to eq("thread_created") end + it "publishes a `thread_created` MessageBus event for DM channels" do + params[:channel_id] = dm_channel.id + params[:original_message_id] = dm_message.id + params[:guardian] = Guardian.new(another_user) + message = MessageBus.track_publish("/chat/#{dm_channel.id}") { result }.first + + expect(message.data["type"]).to eq("thread_created") + end + it "triggers a discourse event `chat_thread_created`" do event = DiscourseEvent.track_events(:chat_thread_created) { result }.first diff --git a/plugins/chat/spec/system/chat_footer_spec.rb b/plugins/chat/spec/system/chat_footer_spec.rb index d5d37c28344..403034b3b85 100644 --- a/plugins/chat/spec/system/chat_footer_spec.rb +++ b/plugins/chat/spec/system/chat_footer_spec.rb @@ -133,36 +133,64 @@ RSpec.describe "Mobile Chat footer", type: :system, mobile: true do end context "for my threads" do - fab!(:thread) { Fabricate(:chat_thread, channel: channel, original_message: message) } - fab!(:thread_message) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } + context "with public channels" do + fab!(:thread) { Fabricate(:chat_thread, channel: channel, original_message: message) } + fab!(:thread_message) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } - before { SiteSetting.chat_threads_enabled = true } + before { SiteSetting.chat_threads_enabled = true } - it "is unread" do - visit("/") - chat_page.open_from_header + it "is unread" do + visit("/") + chat_page.open_from_header - expect(page).to have_css("#c-footer-threads .c-unread-indicator") + expect(page).to have_css("#c-footer-threads .c-unread-indicator") + end + + it "is not unread when thread is from a muted channel" do + channel.membership_for(current_user).update!(muted: true) + + visit("/") + chat_page.open_from_header + + expect(page).to have_no_css("#c-footer-threads .c-unread-indicator") + end + + it "is urgent for watched thread messages" do + thread.membership_for(current_user).update!( + notification_level: ::Chat::NotificationLevels.all[:watching], + ) + + visit("/") + chat_page.open_from_header + + expect(page).to have_css("#c-footer-threads .c-unread-indicator.-urgent") + end end - it "is not unread when thread is from a muted channel" do - channel.membership_for(current_user).update!(muted: true) + context "with direct messages" do + fab!(:dm_channel) do + Fabricate( + :direct_message_channel, + threading_enabled: true, + users: [current_user, other_user], + ) + end + fab!(:dm_message) { Fabricate(:chat_message, chat_channel: dm_channel, user: current_user) } + fab!(:dm_thread) do + Fabricate(:chat_thread, channel: dm_channel, original_message: dm_message) + end + fab!(:dm_thread_message) do + Fabricate(:chat_message, chat_channel: dm_channel, thread: dm_thread, user: other_user) + end - visit("/") - chat_page.open_from_header + before { SiteSetting.chat_threads_enabled = true } - expect(page).to have_no_css("#c-footer-threads .c-unread-indicator") - end + it "is unread" do + visit("/") + chat_page.open_from_header - it "is urgent for watched thread messages" do - thread.membership_for(current_user).update!( - notification_level: ::Chat::NotificationLevels.all[:watching], - ) - - visit("/") - chat_page.open_from_header - - expect(page).to have_css("#c-footer-threads .c-unread-indicator.-urgent") + expect(page).to have_css("#c-footer-threads .c-unread-indicator") + end end end end