From 07c3782e51805c73dbb56e5fd053a9c6978712ff Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 17 Jul 2023 13:00:49 +1000 Subject: [PATCH] FEATURE: Show unread in sidebar for unread channel threads (#22342) This commit makes it so that when the user has unread threads for a channel we show a blue dot in the sidebar (or channel index for mobile/drawer). This blue dot is slightly different from the channel unread messages: 1. It will only show if the new thread messages were created since the user last viewed the channel 2. It will be cleared when the user views the channel, but the threads are still considered unread because we want the user to click into the thread list to view them This necessitates a change to the current user serializer to also include the unread thread overview, which is all unread threads across all channels and their last reply date + time. --- .../app/models/chat/tracking_state_report.rb | 41 +++++++++++--- plugins/chat/app/models/chat/view.rb | 6 +-- .../chat/tracking_state_report_query.rb | 51 ++++++++++++------ .../chat/structured_channel_serializer.rb | 10 +++- .../app/serializers/chat/view_serializer.rb | 8 +-- .../app/services/chat/channel_view_builder.rb | 39 +++++++------- plugins/chat/app/services/chat/publisher.rb | 16 +++--- .../components/chat-channel-metadata.hbs | 1 + .../chat-channel-unread-indicator.hbs | 15 ++---- .../chat-channel-unread-indicator.js | 32 +++++++++++ .../discourse/components/chat-channel.js | 18 ++++--- .../chat/thread/header-unread-indicator.js | 2 +- .../chat/thread/threads-list-button.hbs | 2 +- .../discourse/initializers/chat-sidebar.js | 8 ++- .../discourse/lib/chat-threads-manager.js | 28 +++++++++- .../discourse/models/chat-channel.js | 17 +++--- .../models/user-chat-channel-membership.js | 4 +- .../discourse/routes/chat-index.js | 4 ++ .../services/chat-subscriptions-manager.js | 29 ++++++++-- .../javascripts/discourse/services/chat.js | 54 +++++++++++-------- plugins/chat/plugin.rb | 11 ++++ .../chat/tracking_state_report_query_spec.rb | 52 ++++++++++++++++++ .../chat/channel_view_builder_spec.rb | 3 +- .../chat/mark_all_user_channels_read_spec.rb | 3 ++ .../chat/spec/services/chat/publisher_spec.rb | 16 ++++-- .../schemas/user_chat_channel_membership.json | 3 +- .../message_notifications_mobile_spec.rb | 2 - .../page_objects/chat_drawer/chat_drawer.rb | 16 ++++-- .../system/page_objects/sidebar/sidebar.rb | 10 ++++ .../system/thread_tracking/drawer_spec.rb | 48 +++++++++++++++++ .../system/thread_tracking/full_page_spec.rb | 42 +++++++++++++++ 31 files changed, 459 insertions(+), 132 deletions(-) create mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.js diff --git a/plugins/chat/app/models/chat/tracking_state_report.rb b/plugins/chat/app/models/chat/tracking_state_report.rb index e946f5cc9e5..b15f273f9c3 100644 --- a/plugins/chat/app/models/chat/tracking_state_report.rb +++ b/plugins/chat/app/models/chat/tracking_state_report.rb @@ -8,11 +8,12 @@ module Chat attr_accessor :channel_tracking, :thread_tracking class TrackingStateInfo - attr_accessor :unread_count, :mention_count + attr_accessor :unread_count, :mention_count, :last_reply_created_at def initialize(info) @unread_count = info.present? ? info[:unread_count] : 0 @mention_count = info.present? ? info[:mention_count] : 0 + @last_reply_created_at = info.present? ? info[:last_reply_created_at] : nil end def to_hash @@ -20,7 +21,11 @@ module Chat end def to_h - { unread_count: unread_count, mention_count: mention_count } + { + unread_count: unread_count, + mention_count: mention_count, + last_reply_created_at: last_reply_created_at, + } end end @@ -38,10 +43,34 @@ module Chat end def find_channel_threads(channel_id) - thread_tracking - .select { |_, thread| thread[:channel_id] == channel_id } - .map { |thread_id, thread| [thread_id, TrackingStateInfo.new(thread)] } - .to_h + thread_tracking.inject({}) do |result, (thread_id, thread)| + if thread[:channel_id] == channel_id + result.merge(thread_id => TrackingStateInfo.new(thread)) + else + result + end + end + end + + def find_channel_thread_overviews(channel_id) + thread_tracking.inject({}) do |result, (thread_id, thread)| + if thread[:channel_id] == channel_id + result.merge(thread_id => thread[:last_reply_created_at]) + else + result + end + end + end + + def thread_unread_overview_by_channel + thread_tracking.inject({}) do |acc, tt| + thread_id = tt.first + data = tt.second + + acc[data[:channel_id]] = {} if !acc[data[:channel_id]] + acc[data[:channel_id]][thread_id] = data[:last_reply_created_at] + acc + end end end end diff --git a/plugins/chat/app/models/chat/view.rb b/plugins/chat/app/models/chat/view.rb index 3b69bdf87a6..927df9ae3fd 100644 --- a/plugins/chat/app/models/chat/view.rb +++ b/plugins/chat/app/models/chat/view.rb @@ -7,7 +7,7 @@ module Chat :chat_messages, :can_load_more_past, :can_load_more_future, - :unread_thread_ids, + :unread_thread_overview, :threads, :tracking, :thread_memberships, @@ -19,7 +19,7 @@ module Chat user:, can_load_more_past: nil, can_load_more_future: nil, - unread_thread_ids: nil, + unread_thread_overview: nil, threads: nil, tracking: nil, thread_memberships: nil, @@ -30,7 +30,7 @@ module Chat @user = user @can_load_more_past = can_load_more_past @can_load_more_future = can_load_more_future - @unread_thread_ids = unread_thread_ids + @unread_thread_overview = unread_thread_overview @threads = threads @tracking = tracking @thread_memberships = thread_memberships diff --git a/plugins/chat/app/queries/chat/tracking_state_report_query.rb b/plugins/chat/app/queries/chat/tracking_state_report_query.rb index 490b4188eff..21e1d21fa38 100644 --- a/plugins/chat/app/queries/chat/tracking_state_report_query.rb +++ b/plugins/chat/app/queries/chat/tracking_state_report_query.rb @@ -35,7 +35,8 @@ module Chat thread_ids: nil, include_missing_memberships: false, include_threads: false, - include_read: true + include_read: true, + include_last_reply_details: false ) report = ::Chat::TrackingStateReport.new @@ -59,24 +60,40 @@ module Chat if !include_threads || (thread_ids.blank? && channel_ids.blank?) report.thread_tracking = {} else + tracking = + ::Chat::ThreadUnreadsQuery.call( + channel_ids: channel_ids, + thread_ids: thread_ids, + user_id: guardian.user.id, + include_missing_memberships: include_missing_memberships, + include_read: include_read, + ) + + last_reply_details = + DB.query(<<~SQL, tracking.map(&:thread_id)) if include_last_reply_details + SELECT chat_threads.id AS thread_id, last_message.created_at + FROM chat_threads + INNER JOIN chat_messages AS last_message ON last_message.id = chat_threads.last_message_id + WHERE chat_threads.id IN (?) + AND last_message.deleted_at IS NULL + SQL + report.thread_tracking = - ::Chat::ThreadUnreadsQuery - .call( - channel_ids: channel_ids, - thread_ids: thread_ids, - user_id: guardian.user.id, - include_missing_memberships: include_missing_memberships, - include_read: include_read, - ) + tracking .map do |tt| - [ - tt.thread_id, - { - channel_id: tt.channel_id, - mention_count: tt.mention_count, - unread_count: tt.unread_count, - }, - ] + data = { + channel_id: tt.channel_id, + mention_count: tt.mention_count, + unread_count: tt.unread_count, + } + + if include_last_reply_details + data[:last_reply_created_at] = last_reply_details + .find { |details| details.thread_id == tt.thread_id } + &.created_at + end + + [tt.thread_id, data] end .to_h end diff --git a/plugins/chat/app/serializers/chat/structured_channel_serializer.rb b/plugins/chat/app/serializers/chat/structured_channel_serializer.rb index af86baeea99..10b31b4a0e9 100644 --- a/plugins/chat/app/serializers/chat/structured_channel_serializer.rb +++ b/plugins/chat/app/serializers/chat/structured_channel_serializer.rb @@ -2,12 +2,20 @@ module Chat class StructuredChannelSerializer < ApplicationSerializer - attributes :public_channels, :direct_message_channels, :tracking, :meta + attributes :public_channels, :direct_message_channels, :tracking, :meta, :unread_thread_overview def tracking object[:tracking] end + def include_unread_thread_overview? + SiteSetting.enable_experimental_chat_threaded_discussions + end + + def unread_thread_overview + object[:unread_thread_overview] + end + def public_channels object[:public_channels].map do |channel| Chat::ChannelSerializer.new( diff --git a/plugins/chat/app/serializers/chat/view_serializer.rb b/plugins/chat/app/serializers/chat/view_serializer.rb index aa4277957b7..f1399bf522a 100644 --- a/plugins/chat/app/serializers/chat/view_serializer.rb +++ b/plugins/chat/app/serializers/chat/view_serializer.rb @@ -2,7 +2,7 @@ module Chat class ViewSerializer < ApplicationSerializer - attributes :meta, :chat_messages, :threads, :tracking, :unread_thread_ids, :channel + attributes :meta, :chat_messages, :threads, :tracking, :unread_thread_overview, :channel def threads return [] if !object.threads @@ -23,15 +23,15 @@ module Chat object.tracking || {} end - def unread_thread_ids - object.unread_thread_ids || [] + def unread_thread_overview + object.unread_thread_overview || {} end def include_threads? include_thread_data? end - def include_unread_thread_ids? + def include_unread_thread_overview? include_thread_data? end diff --git a/plugins/chat/app/services/chat/channel_view_builder.rb b/plugins/chat/app/services/chat/channel_view_builder.rb index e6b4f72b7e5..61ea22c9f99 100644 --- a/plugins/chat/app/services/chat/channel_view_builder.rb +++ b/plugins/chat/app/services/chat/channel_view_builder.rb @@ -34,7 +34,7 @@ module Chat step :determine_threads_enabled step :determine_include_thread_messages step :fetch_messages - step :fetch_unread_thread_ids + step :fetch_unread_thread_overview step :fetch_threads_for_messages step :fetch_tracking step :fetch_thread_memberships @@ -155,24 +155,25 @@ module Chat end end - # The thread tracking overview is a simple array of thread IDs - # that have unread messages, only threads with unread messages - # will be included in this array. This is a low-cost way to know - # how many threads the user has unread across the entire channel. - def fetch_unread_thread_ids(guardian:, channel:, threads_enabled:, **) + # The thread tracking overview is a simple array of hashes consisting + # of thread IDs that have unread messages as well as the datetime of the + # last reply in the thread. + # + # Only threads with unread messages will be included in this array. + # This is a low-cost way to know how many threads the user has unread + # across the entire channel. + def fetch_unread_thread_overview(guardian:, channel:, threads_enabled:, **) if !threads_enabled - context.unread_thread_ids = [] + context.unread_thread_overview = {} else - context.unread_thread_ids = - ::Chat::TrackingStateReportQuery - .call( - guardian: guardian, - channel_ids: [channel.id], - include_threads: true, - include_read: false, - ) - .find_channel_threads(channel.id) - .keys + context.unread_thread_overview = + ::Chat::TrackingStateReportQuery.call( + guardian: guardian, + channel_ids: [channel.id], + include_threads: true, + include_read: false, + include_last_reply_details: true, + ).find_channel_thread_overviews(channel.id) end end @@ -238,7 +239,7 @@ module Chat messages:, threads:, tracking:, - unread_thread_ids:, + unread_thread_overview:, can_load_more_past:, can_load_more_future:, thread_memberships:, @@ -252,7 +253,7 @@ module Chat user: guardian.user, can_load_more_past: can_load_more_past, can_load_more_future: can_load_more_future, - unread_thread_ids: unread_thread_ids, + unread_thread_overview: unread_thread_overview, threads: threads, tracking: tracking, thread_memberships: thread_memberships, diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 7c3c8b9357b..1f9e22502c4 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -297,15 +297,13 @@ module Chat # and a message is sent in the thread. We also need to pass the actual # thread tracking state. if channel.threading_enabled && message.thread_reply? - data[:unread_thread_ids] = ::Chat::TrackingStateReportQuery - .call( - guardian: user.guardian, - channel_ids: [channel.id], - include_threads: true, - include_read: false, - ) - .find_channel_threads(channel.id) - .keys + data[:unread_thread_overview] = ::Chat::TrackingStateReportQuery.call( + guardian: user.guardian, + channel_ids: [channel.id], + include_threads: true, + include_read: false, + include_last_reply_details: true, + ).find_channel_thread_overviews(channel.id) data[:thread_tracking] = ::Chat::TrackingStateReportQuery.call( guardian: user.guardian, diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs index eaaa128bd70..f2bda8d1d69 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs @@ -5,6 +5,7 @@ {{/if}} + {{! TODO (martin) We need to have the thread unread count in the mobile channel row too }} {{#if this.unreadIndicator}} {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.hbs index a035246dcff..b9a7506b474 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.hbs @@ -1,17 +1,12 @@ -{{#if (gt @channel.tracking.unreadCount 0)}} +{{#if this.showUnreadIndicator}}
-
{{@channel.tracking.unreadCount}}
+
{{#if + this.showUnreadCount + }}{{this.unreadCount}}{{else}} {{/if}}
{{/if}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.js new file mode 100644 index 00000000000..3614f0f09a6 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-unread-indicator.js @@ -0,0 +1,32 @@ +import Component from "@glimmer/component"; +import { inject as service } from "@ember/service"; + +export default class ChatChannelUnreadIndicator extends Component { + @service chat; + @service site; + + get showUnreadIndicator() { + return ( + 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.unreadThreadsCountSinceLastViewed > 0) + ); + } + + get unreadCount() { + return this.args.channel.tracking.unreadCount; + } + + get isUrgent() { + return ( + this.args.channel.isDirectMessageChannel || + this.args.channel.tracking.mentionCount > 0 + ); + } + + get showUnreadCount() { + return this.args.channel.isDirectMessageChannel; + } +} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index dcf44e2fe4d..79380665187 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -214,6 +214,10 @@ export default class ChatLivePane extends Component { this.args.channel.addMessages(messages); this.args.channel.details = meta; + // We update this value server-side when we load the Channel + // here, so this reflects reality for sidebar unread logic. + this.args.channel.updateLastViewedAt(); + if (result.threads) { result.threads.forEach((thread) => { const storedThread = this.args.channel.threadsManager.add( @@ -237,8 +241,9 @@ export default class ChatLivePane extends Component { }); } - if (result.unread_thread_ids) { - this.args.channel.unreadThreadIds = result.unread_thread_ids; + if (result.unread_thread_overview) { + this.args.channel.threadsManager.unreadThreadOverview = + result.unread_thread_overview; } if (this.requestedTargetMessageId) { @@ -353,12 +358,9 @@ export default class ChatLivePane extends Component { }); } - if (result.thread_tracking_overview) { - result.thread_tracking_overview.forEach((threadId) => { - if (!this.args.channel.threadTrackingOverview.includes(threadId)) { - this.args.channel.threadTrackingOverview.push(threadId); - } - }); + if (result.unread_thread_overview) { + this.args.channel.threadsManager.unreadThreadOverview = + result.unread_thread_overview; } this.args.channel.details = meta; diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header-unread-indicator.js b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header-unread-indicator.js index 6abaf2f08cb..f81c0ca7755 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread/header-unread-indicator.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread/header-unread-indicator.js @@ -9,7 +9,7 @@ export default class ChatThreadHeaderUnreadIndicator extends Component { } get unreadCount() { - return this.args.channel.unreadThreadCount; + return this.args.channel.threadsManager.unreadThreadCount; } get showUnreadIndicator() { diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread/threads-list-button.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/thread/threads-list-button.hbs index 0c0182492b1..c4b7263d1b3 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread/threads-list-button.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread/threads-list-button.hbs @@ -4,7 +4,7 @@ title={{i18n "chat.threads.list"}} class={{concat-class "chat-threads-list-button btn btn-flat" - (if @channel.unreadThreadCount "has-unreads") + (if @channel.threadsManager.unreadThreadCount "has-unreads") }} > {{d-icon "discourse-threads"}} diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js index acb1bd1c225..81ee9a3adfa 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js @@ -93,7 +93,13 @@ export default { } get suffixValue() { - return this.channel.tracking.unreadCount > 0 ? "circle" : ""; + return this.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.chatService.activeChannel?.id !== this.channel.id && + this.channel.unreadThreadsCountSinceLastViewed > 0) + ? "circle" + : ""; } get suffixCSSClass() { diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js b/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js index 4b341fbb666..f2c1d565232 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js +++ b/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js @@ -3,7 +3,7 @@ import { setOwner } from "@ember/application"; import Promise from "rsvp"; import ChatThread from "discourse/plugins/chat/discourse/models/chat-thread"; import { cached, tracked } from "@glimmer/tracking"; -import { TrackedObject } from "@ember-compat/tracked-built-ins"; +import { TrackedMap, TrackedObject } from "@ember-compat/tracked-built-ins"; /* The ChatThreadsManager is responsible for managing the loaded chat threads @@ -19,11 +19,37 @@ export default class ChatThreadsManager { @service chatApi; @tracked _cached = new TrackedObject(); + @tracked _unreadThreadOverview = new TrackedMap(); constructor(owner) { setOwner(this, owner); } + get unreadThreadCount() { + return this.unreadThreadOverview.size; + } + + get unreadThreadOverview() { + return this._unreadThreadOverview; + } + + set unreadThreadOverview(unreadThreadOverview) { + this._unreadThreadOverview.clear(); + + for (const [threadId, lastReplyCreatedAt] of Object.entries( + unreadThreadOverview + )) { + this.markThreadUnread(threadId, lastReplyCreatedAt); + } + } + + markThreadUnread(threadId, lastReplyCreatedAt) { + this.unreadThreadOverview.set( + parseInt(threadId, 10), + new Date(lastReplyCreatedAt) + ); + } + @cached get threads() { return Object.values(this._cached); diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index a832125d07e..dadffdd364d 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -1,6 +1,5 @@ import UserChatChannelMembership from "discourse/plugins/chat/discourse/models/user-chat-channel-membership"; import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; -import { TrackedSet } from "@ember-compat/tracked-built-ins"; import { escapeExpression } from "discourse/lib/utilities"; import { tracked } from "@glimmer/tracking"; import slugifyChannel from "discourse/plugins/chat/discourse/lib/slugify-channel"; @@ -80,7 +79,6 @@ export default class ChatChannel { threadsManager = new ChatThreadsManager(getOwner(this)); messagesManager = new ChatMessagesManager(getOwner(this)); - @tracked _unreadThreadIds = new TrackedSet(); @tracked _currentUserMembership; @tracked _lastMessage; @@ -119,16 +117,15 @@ export default class ChatChannel { this.lastMessage = args.last_message; } - get unreadThreadCount() { - return this.unreadThreadIds.size; + get unreadThreadsCountSinceLastViewed() { + return Array.from(this.threadsManager.unreadThreadOverview.values()).filter( + (lastReplyCreatedAt) => + lastReplyCreatedAt >= this.currentUserMembership.lastViewedAt + ).length; } - get unreadThreadIds() { - return this._unreadThreadIds; - } - - set unreadThreadIds(unreadThreadIds) { - this._unreadThreadIds = new TrackedSet(unreadThreadIds); + updateLastViewedAt() { + this.currentUserMembership.lastViewedAt = new Date(); } findIndexOfMessage(id) { diff --git a/plugins/chat/assets/javascripts/discourse/models/user-chat-channel-membership.js b/plugins/chat/assets/javascripts/discourse/models/user-chat-channel-membership.js index 538774b5038..7a87d280d8d 100644 --- a/plugins/chat/assets/javascripts/discourse/models/user-chat-channel-membership.js +++ b/plugins/chat/assets/javascripts/discourse/models/user-chat-channel-membership.js @@ -11,8 +11,8 @@ export default class UserChatChannelMembership { @tracked desktopNotificationLevel = null; @tracked mobileNotificationLevel = null; @tracked lastReadMessageId = null; - @tracked user = null; @tracked lastViewedAt = null; + @tracked user = null; constructor(args = {}) { this.following = args.following; @@ -20,7 +20,7 @@ export default class UserChatChannelMembership { this.desktopNotificationLevel = args.desktop_notification_level; this.mobileNotificationLevel = args.mobile_notification_level; this.lastReadMessageId = args.last_read_message_id; - this.lastViewedAt = args.last_viewed_at; + this.lastViewedAt = new Date(args.last_viewed_at); this.user = this.#initUserModel(args.user); } diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-index.js b/plugins/chat/assets/javascripts/discourse/routes/chat-index.js index 09c62b06e30..e818e1c62ab 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-index.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-index.js @@ -6,6 +6,10 @@ export default class ChatIndexRoute extends DiscourseRoute { @service chatChannelsManager; @service router; + activate() { + this.chat.activeChannel = null; + } + redirect() { // Always want the channel index on mobile. if (this.site.mobileView) { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js index 314907f1f71..44d441823f7 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js @@ -211,7 +211,11 @@ export default class ChatSubscriptionsManager extends Service { .find(channel.id, busData.thread_id) .then((thread) => { if (thread.currentUserMembership) { - channel.unreadThreadIds.add(busData.thread_id); + channel.threadsManager.markThreadUnread( + busData.thread_id, + busData.message.created_at + ); + this._updateActiveLastViewedAt(channel); } }); } @@ -228,7 +232,9 @@ export default class ChatSubscriptionsManager extends Service { if (busData.message.user.id === this.currentUser.id) { // Thread should no longer be considered unread. if (thread.currentUserMembership) { - channel.unreadThreadIds.delete(busData.thread_id); + channel.threadsManager.unreadThreadOverview.delete( + parseInt(busData.thread_id, 10) + ); thread.currentUserMembership.lastReadMessageId = busData.message.id; } @@ -251,8 +257,12 @@ export default class ChatSubscriptionsManager extends Service { (thread.currentUserMembership.lastReadMessageId || 0) && !thread.currentUserMembership.isQuiet ) { - channel.unreadThreadIds.add(busData.thread_id); + channel.threadsManager.markThreadUnread( + busData.thread_id, + busData.message.created_at + ); thread.tracking.unreadCount++; + this._updateActiveLastViewedAt(channel); } } } @@ -260,6 +270,14 @@ export default class ChatSubscriptionsManager extends Service { }); } + // If the user is currently looking at this channel via activeChannel, we don't want the unread + // indicator to show in the sidebar for unread threads (since that is based on the lastViewedAt). + _updateActiveLastViewedAt(channel) { + if (this.chat.activeChannel?.id === channel.id) { + channel.updateLastViewedAt(); + } + } + _startUserTrackingStateSubscription(lastId) { if (!this.currentUser) { return; @@ -316,8 +334,9 @@ export default class ChatSubscriptionsManager extends Service { channel.tracking.unreadCount = busData.unread_count; channel.tracking.mentionCount = busData.mention_count; - if (busData.hasOwnProperty("unread_thread_ids")) { - channel.unreadThreadIds = busData.unread_thread_ids; + if (busData.hasOwnProperty("unread_thread_overview")) { + channel.threadsManager.unreadThreadOverview = + busData.unread_thread_overview; } if (busData.thread_id && busData.hasOwnProperty("thread_tracking")) { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index d3a03a70626..c1ed707f8c4 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -173,36 +173,44 @@ export default class Chat extends Service { this.set("isNetworkUnreliable", false); } - setupWithPreloadedChannels(channels) { + setupWithPreloadedChannels(channelsView) { this.chatSubscriptionsManager.startChannelsSubscriptions( - channels.meta.message_bus_last_ids + channelsView.meta.message_bus_last_ids ); - this.presenceChannel.subscribe(channels.global_presence_channel_state); + this.presenceChannel.subscribe(channelsView.global_presence_channel_state); - [...channels.public_channels, ...channels.direct_message_channels].forEach( - (channelObject) => { - const channel = this.chatChannelsManager.store(channelObject); - const storedDraft = (this.currentUser?.chat_drafts || []).find( - (draft) => draft.channel_id === channel.id - ); + [ + ...channelsView.public_channels, + ...channelsView.direct_message_channels, + ].forEach((channelObject) => { + const storedChannel = this.chatChannelsManager.store(channelObject); + const storedDraft = (this.currentUser?.chat_drafts || []).find( + (draft) => draft.channel_id === storedChannel.id + ); - if (storedDraft) { - this.chatDraftsManager.add( - ChatMessage.createDraftMessage( - channel, - Object.assign( - { user: this.currentUser }, - JSON.parse(storedDraft.data) - ) + if (storedDraft) { + this.chatDraftsManager.add( + ChatMessage.createDraftMessage( + storedChannel, + Object.assign( + { user: this.currentUser }, + JSON.parse(storedDraft.data) ) - ); - } - - return this.chatChannelsManager.follow(channel); + ) + ); } - ); - this.chatTrackingStateManager.setupWithPreloadedState(channels.tracking); + if (channelsView.unread_thread_overview?.[storedChannel.id]) { + storedChannel.threadsManager.unreadThreadOverview = + channelsView.unread_thread_overview[storedChannel.id]; + } + + return this.chatChannelsManager.follow(storedChannel); + }); + + this.chatTrackingStateManager.setupWithPreloadedState( + channelsView.tracking + ); } willDestroy() { diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 0ef9df5832f..f54db49a6bf 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -237,6 +237,17 @@ after_initialize do add_to_serializer(:current_user, :chat_channels) do structured = Chat::ChannelFetcher.structured(self.scope) + + if SiteSetting.enable_experimental_chat_threaded_discussions + structured[:unread_thread_overview] = ::Chat::TrackingStateReportQuery.call( + guardian: self.scope, + channel_ids: structured[:public_channels].map(&:id), + include_threads: true, + include_read: false, + include_last_reply_details: true, + ).thread_unread_overview_by_channel + end + Chat::ChannelIndexSerializer.new(structured, scope: self.scope, root: false).as_json end diff --git a/plugins/chat/spec/queries/chat/tracking_state_report_query_spec.rb b/plugins/chat/spec/queries/chat/tracking_state_report_query_spec.rb index 5a8ad346cef..4649a171f7e 100644 --- a/plugins/chat/spec/queries/chat/tracking_state_report_query_spec.rb +++ b/plugins/chat/spec/queries/chat/tracking_state_report_query_spec.rb @@ -9,6 +9,7 @@ RSpec.describe Chat::TrackingStateReportQuery do include_missing_memberships: include_missing_memberships, include_threads: include_threads, include_read: include_read, + include_last_reply_details: include_last_reply_details, ) end @@ -20,6 +21,7 @@ RSpec.describe Chat::TrackingStateReportQuery do let(:include_missing_memberships) { false } let(:include_threads) { false } let(:include_read) { true } + let(:include_last_reply_details) { false } context "when channel_ids empty" do it "returns empty object for channel_tracking" do expect(query.channel_tracking).to eq({}) @@ -129,6 +131,56 @@ RSpec.describe Chat::TrackingStateReportQuery do ) end + context "when include_last_reply_details is true" do + let(:include_last_reply_details) { true } + + before do + thread_1.add(current_user) + thread_2.add(current_user) + Fabricate(:chat_message, chat_channel: channel_1, thread: thread_1) + Fabricate(:chat_message, chat_channel: channel_2, thread: thread_2) + end + + it "gets the last_reply_created_at for each thread based on the last_message" do + expect(query.thread_tracking).to eq( + { + thread_1.id => { + unread_count: 1, + mention_count: 0, + channel_id: channel_1.id, + last_reply_created_at: thread_1.reload.last_message.created_at, + }, + thread_2.id => { + unread_count: 1, + mention_count: 0, + channel_id: channel_2.id, + last_reply_created_at: thread_2.reload.last_message.created_at, + }, + }, + ) + end + + it "does not get the last_reply_created_at for threads where the last_message is deleted" do + thread_1.reload.last_message.trash! + expect(query.thread_tracking).to eq( + { + thread_1.id => { + unread_count: 0, + mention_count: 0, + channel_id: channel_1.id, + last_reply_created_at: nil, + }, + thread_2.id => { + unread_count: 1, + mention_count: 0, + channel_id: channel_2.id, + last_reply_created_at: thread_2.reload.last_message.created_at, + }, + }, + ) + end + end + context "when thread_ids and channel_ids is empty" do let(:thread_ids) { [] } let(:channel_ids) { [] } diff --git a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb index 39354b8128d..71e925ca6b1 100644 --- a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb +++ b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb @@ -173,6 +173,7 @@ RSpec.describe Chat::ChannelViewBuilder do channel_ids: [channel.id], include_threads: true, include_read: false, + include_last_reply_details: true, ) .returns(Chat::TrackingStateReport.new) .once @@ -188,7 +189,7 @@ RSpec.describe Chat::ChannelViewBuilder do thread = Fabricate(:chat_thread, channel: channel) thread.add(current_user) message_1 = Fabricate(:chat_message, chat_channel: channel, thread: thread) - expect(result.view.unread_thread_ids).to eq([message_1.thread.id]) + expect(result.view.unread_thread_overview).to eq({ thread.id => message_1.created_at }) end it "fetches the tracking state of threads in the channel" do diff --git a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb index d12c9bf04f4..4d1d0d32904 100644 --- a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb +++ b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb @@ -137,18 +137,21 @@ RSpec.describe Chat::MarkAllUserChannelsRead do expect(message.data).to eq( channel_1.id.to_s => { "last_read_message_id" => message_2.id, + "last_reply_created_at" => nil, "membership_id" => membership_1.id, "mention_count" => 0, "unread_count" => 0, }, channel_2.id.to_s => { "last_read_message_id" => message_4.id, + "last_reply_created_at" => nil, "membership_id" => membership_2.id, "mention_count" => 0, "unread_count" => 0, }, channel_3.id.to_s => { "last_read_message_id" => message_6.id, + "last_reply_created_at" => nil, "membership_id" => membership_3.id, "mention_count" => 0, "unread_count" => 0, diff --git a/plugins/chat/spec/services/chat/publisher_spec.rb b/plugins/chat/spec/services/chat/publisher_spec.rb index f601b6d96d4..0f966c41905 100644 --- a/plugins/chat/spec/services/chat/publisher_spec.rb +++ b/plugins/chat/spec/services/chat/publisher_spec.rb @@ -96,8 +96,10 @@ describe Chat::Publisher do context "when the channel has threading enabled and the message is a thread reply" do fab!(:thread) { Fabricate(:chat_thread, channel: channel) } + before do message_1.update!(thread: thread) + thread.update_last_message_id! channel.update!(threading_enabled: true) end @@ -106,16 +108,22 @@ describe Chat::Publisher do it "publishes the tracking state with correct counts" do expect(data["thread_id"]).to eq(thread.id) - expect(data["unread_thread_ids"]).to eq([thread.id]) - expect(data["thread_tracking"]).to eq({ "unread_count" => 1, "mention_count" => 0 }) + expect(data["unread_thread_overview"]).to eq( + { thread.id.to_s => thread.reload.last_message.created_at.iso8601(3) }, + ) + expect(data["thread_tracking"]).to eq( + { "unread_count" => 1, "mention_count" => 0, "last_reply_created_at" => nil }, + ) end end context "when the user has no thread membership" do it "publishes the tracking state with zeroed out counts" do expect(data["thread_id"]).to eq(thread.id) - expect(data["unread_thread_ids"]).to eq([]) - expect(data["thread_tracking"]).to eq({ "unread_count" => 0, "mention_count" => 0 }) + expect(data["unread_thread_overview"]).to eq({}) + expect(data["thread_tracking"]).to eq( + { "unread_count" => 0, "mention_count" => 0, "last_reply_created_at" => nil }, + ) end end end diff --git a/plugins/chat/spec/support/api/schemas/user_chat_channel_membership.json b/plugins/chat/spec/support/api/schemas/user_chat_channel_membership.json index 8dcf8207d80..4111fe5fa22 100644 --- a/plugins/chat/spec/support/api/schemas/user_chat_channel_membership.json +++ b/plugins/chat/spec/support/api/schemas/user_chat_channel_membership.json @@ -26,6 +26,7 @@ "avatar_template": { "type": "string" }, "username": { "type": "string" } } - } + }, + "last_viewed_at": { "type": "datetime" } } } diff --git a/plugins/chat/spec/system/message_notifications_mobile_spec.rb b/plugins/chat/spec/system/message_notifications_mobile_spec.rb index 22dfc4cc61f..acfbd38a029 100644 --- a/plugins/chat/spec/system/message_notifications_mobile_spec.rb +++ b/plugins/chat/spec/system/message_notifications_mobile_spec.rb @@ -109,7 +109,6 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "") expect(page).to have_css( ".chat-channel-row[data-chat-channel-id=\"#{channel_1.id}\"] .chat-channel-unread-indicator", - text: 1, ) end end @@ -130,7 +129,6 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator") expect(page).to have_css( ".chat-channel-row[data-chat-channel-id=\"#{channel_1.id}\"] .chat-channel-unread-indicator", - text: 1, ) end end diff --git a/plugins/chat/spec/system/page_objects/chat_drawer/chat_drawer.rb b/plugins/chat/spec/system/page_objects/chat_drawer/chat_drawer.rb index 925e4e711ee..a908c2fec07 100644 --- a/plugins/chat/spec/system/page_objects/chat_drawer/chat_drawer.rb +++ b/plugins/chat/spec/system/page_objects/chat_drawer/chat_drawer.rb @@ -20,12 +20,22 @@ module PageObjects end def open_channel(channel) - find( - "#{VISIBLE_DRAWER} .channels-list .chat-channel-row[data-chat-channel-id='#{channel.id}']", - ).click + find("#{VISIBLE_DRAWER} .channels-list #{channel_row_selector(channel)}").click has_no_css?(".chat-skeleton") end + def channel_row_selector(channel) + ".chat-channel-row[data-chat-channel-id='#{channel.id}']" + end + + def has_unread_channel?(channel) + has_css?("#{channel_row_selector(channel)} .chat-channel-unread-indicator") + end + + def has_no_unread_channel?(channel) + has_no_css?("#{channel_row_selector(channel)} .chat-channel-unread-indicator") + end + def maximize mouseout find("#{VISIBLE_DRAWER} .chat-drawer-header__full-screen-btn").click diff --git a/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb b/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb index 341970ad139..448c9e5b0b1 100644 --- a/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb +++ b/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb @@ -40,6 +40,16 @@ module PageObjects find(".sidebar-section-link.channel-#{channel.id}") self end + + def has_unread_channel?(channel) + has_css?(".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread") + end + + def has_no_unread_channel?(channel) + has_no_css?( + ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread", + ) + end end end end diff --git a/plugins/chat/spec/system/thread_tracking/drawer_spec.rb b/plugins/chat/spec/system/thread_tracking/drawer_spec.rb index 32e284e8f64..2e0667eb87c 100644 --- a/plugins/chat/spec/system/thread_tracking/drawer_spec.rb +++ b/plugins/chat/spec/system/thread_tracking/drawer_spec.rb @@ -64,5 +64,53 @@ describe "Thread tracking state | drawer", type: :system do expect(drawer_page).to have_unread_thread_indicator(count: 1) expect(thread_list_page).to have_unread_item(thread.id) end + + describe "channel index unread indicators" do + fab!(:other_channel) { Fabricate(:chat_channel) } + + before { other_channel.add(current_user) } + + it "shows an unread indicator for the channel with unread threads in the index" do + visit("/") + chat_page.open_from_header + expect(drawer_page).to have_unread_channel(channel) + end + + it "does not show an unread indicator for the channel if the user has visited the channel since the unread thread message arrived" do + channel.membership_for(current_user).update!(last_viewed_at: Time.zone.now) + visit("/") + chat_page.open_from_header + expect(drawer_page).to have_no_unread_channel(channel) + end + + it "clears the index unread indicator for the channel when opening it but keeps the thread list unread indicator" do + visit("/") + chat_page.open_from_header + drawer_page.open_channel(channel) + expect(channel_page).to have_unread_thread_indicator(count: 1) + drawer_page.back + expect(drawer_page).to have_no_unread_channel(channel) + end + + it "does not show an unread indicator for the channel index if a new thread message arrives while the user is looking at the channel" do + visit("/") + chat_page.open_from_header + expect(drawer_page).to have_unread_channel(channel) + drawer_page.open_channel(channel) + Fabricate(:chat_message, thread: thread) + drawer_page.back + expect(drawer_page).to have_no_unread_channel(channel) + end + + it "shows an unread indicator for the channel index if a new thread message arrives while the user is not looking at the channel" do + visit("/") + chat_page.open_from_header + drawer_page.open_channel(channel) + drawer_page.back + expect(drawer_page).to have_no_unread_channel(channel) + Fabricate(:chat_message, thread: thread) + expect(drawer_page).to have_unread_channel(channel) + end + end end end diff --git a/plugins/chat/spec/system/thread_tracking/full_page_spec.rb b/plugins/chat/spec/system/thread_tracking/full_page_spec.rb index 7c091fbb8d0..0a041c6aa2c 100644 --- a/plugins/chat/spec/system/thread_tracking/full_page_spec.rb +++ b/plugins/chat/spec/system/thread_tracking/full_page_spec.rb @@ -10,6 +10,7 @@ describe "Thread tracking state | full page", type: :system do let(:channel_page) { PageObjects::Pages::ChatChannel.new } let(:thread_page) { PageObjects::Pages::ChatThread.new } let(:thread_list_page) { PageObjects::Components::Chat::ThreadList.new } + let(:sidebar_page) { PageObjects::Pages::Sidebar.new } before do SiteSetting.enable_experimental_chat_threaded_discussions = true @@ -86,6 +87,47 @@ describe "Thread tracking state | full page", type: :system do expect(thread_list_page).to have_thread(new_thread) end + describe "sidebar unread indicators" do + fab!(:other_channel) { Fabricate(:chat_channel) } + + before do + other_channel.add(current_user) + SiteSetting.navigation_menu = "sidebar" + end + + it "shows an unread indicator for the channel with unread threads in the sidebar" do + chat_page.visit_channel(other_channel) + expect(sidebar_page).to have_unread_channel(channel) + end + + it "does not show an unread indicator for the channel if the user has visited the channel since the unread thread message arrived" do + channel.membership_for(current_user).update!(last_viewed_at: Time.zone.now) + chat_page.visit_channel(other_channel) + expect(sidebar_page).to have_no_unread_channel(channel) + end + + it "clears the sidebar unread indicator for the channel when opening it but keeps the thread list unread indicator" do + chat_page.visit_channel(channel) + expect(sidebar_page).to have_no_unread_channel(channel) + expect(channel_page).to have_unread_thread_indicator(count: 1) + end + + it "does not show an unread indicator for the channel sidebar if a new thread message arrives while the user is looking at the channel" do + chat_page.visit_channel(channel) + expect(sidebar_page).to have_no_unread_channel(channel) + Fabricate(:chat_message, thread: thread) + expect(sidebar_page).to have_no_unread_channel(channel) + end + + it "shows an unread indicator for the channel sidebar if a new thread message arrives while the user is not looking at the channel" do + chat_page.visit_channel(channel) + expect(sidebar_page).to have_no_unread_channel(channel) + chat_page.visit_channel(other_channel) + Fabricate(:chat_message, thread: thread) + expect(sidebar_page).to have_unread_channel(channel) + end + end + context "when the user's notification level for the thread is set to normal" do before { thread.membership_for(current_user).update!(notification_level: :normal) }