From 534e8c1628d0b16087b1d1135e655ca2a13839d1 Mon Sep 17 00:00:00 2001 From: David Battersby Date: Fri, 8 Nov 2024 12:31:03 +0400 Subject: [PATCH] UX: update chat channel sorting to include unread threads (#29617) Adds channels with unread threads (watching/tracking) to the sorting logic for both public and direct message channels. Previously channels with unread threads could easily be missed as we didn't bump them to the top when new thread replies were created. We are also adding a blue unread badge next to DM channels when there is an unread thread, as previously they weren't appearing as unread within the DMs tab (they only showed within the My Threads section). --- .../chat-channel-unread-indicator.gjs | 35 +++--- .../services/chat-channels-manager.js | 57 ++++++--- .../chat/spec/fabricators/chat_fabricator.rb | 9 +- .../spec/system/list_channels/drawer_spec.rb | 109 ++++++++++++++---- .../chat/components/channels_index.rb | 10 +- .../page_objects/chat_drawer/chat_drawer.rb | 4 + 6 files changed, 167 insertions(+), 57 deletions(-) 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 86178c1f789..02a6a6d3036 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 @@ -11,44 +11,45 @@ export default class ChatChannelUnreadIndicator extends Component { 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.args.channel.isCategoryChannel && - this.chat.activeChannel?.id !== this.args.channel.id && - this.args.channel.unreadThreadsCountSinceLastViewed > 0) + this.args.channel.unreadThreadsCountSinceLastViewed > 0 ); } - get unreadCount() { - if (this.#hasChannelMentions()) { + get urgentCount() { + if (this.hasChannelMentions) { return this.args.channel.tracking.mentionCount; } - if (this.#hasWatchedThreads()) { + if (this.hasWatchedThreads) { return this.args.channel.tracking.watchedThreadsUnreadCount; } return this.args.channel.tracking.unreadCount; } get isUrgent() { - if (this.#onlyMentions()) { - return this.#hasChannelMentions(); + if (this.onlyMentions) { + return this.hasChannelMentions; } return ( - this.args.channel.isDirectMessageChannel || - this.#hasChannelMentions() || - this.#hasWatchedThreads() + this.isDirectMessage || this.hasChannelMentions || this.hasWatchedThreads ); } - #hasChannelMentions() { + get isDirectMessage() { + return ( + this.args.channel.isDirectMessageChannel && + this.args.channel.tracking.unreadCount > 0 + ); + } + + get hasChannelMentions() { return this.args.channel.tracking.mentionCount > 0; } - #hasWatchedThreads() { + get hasWatchedThreads() { return this.args.channel.tracking.watchedThreadsUnreadCount > 0; } - #onlyMentions() { + get onlyMentions() { return hasChatIndicator(this.currentUser).ONLY_MENTIONS; } @@ -62,7 +63,7 @@ export default class ChatChannelUnreadIndicator extends Component { >
{{#if this.isUrgent - }}{{this.unreadCount}}{{else}} {{/if}}
+ }}{{this.urgentCount}}{{else}} {{/if}} {{/if}} 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 56a3a8e4ca4..7c5106402f3 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js @@ -198,24 +198,37 @@ export default class ChatChannelsManager extends Service { #sortChannelsByActivity(channels) { return channels.sort((a, b) => { - // if both channels have mention count, sort by slug - // otherwise prioritize channel with mention count - if (a.tracking.mentionCount > 0 && b.tracking.mentionCount > 0) { + const stats = { + a: { + urgent: + a.tracking.mentionCount + a.tracking.watchedThreadsUnreadCount, + unread: a.tracking.unreadCount + a.threadsManager.unreadThreadCount, + }, + b: { + urgent: + b.tracking.mentionCount + b.tracking.watchedThreadsUnreadCount, + unread: b.tracking.unreadCount + b.threadsManager.unreadThreadCount, + }, + }; + + // if both channels have urgent count, sort by slug + // otherwise prioritize channel with urgent count + if (stats.a.urgent > 0 && stats.b.urgent > 0) { return a.slug?.localeCompare?.(b.slug); } - if (a.tracking.mentionCount > 0 || b.tracking.mentionCount > 0) { - return a.tracking.mentionCount > b.tracking.mentionCount ? -1 : 1; + if (stats.a.urgent > 0 || stats.b.urgent > 0) { + return stats.a.urgent > stats.b.urgent ? -1 : 1; } - // if both channels have unread count, sort by slug + // if both channels have unread messages or threads, sort by slug // otherwise prioritize channel with unread count - if (a.tracking.unreadCount > 0 && b.tracking.unreadCount > 0) { + if (stats.a.unread > 0 && stats.b.unread > 0) { return a.slug?.localeCompare?.(b.slug); } - if (a.tracking.unreadCount > 0 || b.tracking.unreadCount > 0) { - return a.tracking.unreadCount > b.tracking.unreadCount ? -1 : 1; + if (stats.a.unread > 0 || stats.b.unread > 0) { + return stats.a.unread > stats.b.unread ? -1 : 1; } return a.slug?.localeCompare?.(b.slug); @@ -232,14 +245,30 @@ export default class ChatChannelsManager extends Service { return -1; } - if (a.tracking.unreadCount === b.tracking.unreadCount) { - return new Date(a.lastMessage.createdAt) > - new Date(b.lastMessage.createdAt) + if ( + a.tracking.unreadCount + a.tracking.watchedThreadsUnreadCount > 0 || + b.tracking.unreadCount + b.tracking.watchedThreadsUnreadCount > 0 + ) { + return a.tracking.unreadCount + a.tracking.watchedThreadsUnreadCount > + b.tracking.unreadCount + b.tracking.watchedThreadsUnreadCount ? -1 : 1; - } else { - return a.tracking.unreadCount > b.tracking.unreadCount ? -1 : 1; } + + if ( + a.threadsManager.unreadThreadCount > 0 || + b.threadsManager.unreadThreadCount > 0 + ) { + return a.threadsManager.unreadThreadCount > + b.threadsManager.unreadThreadCount + ? -1 + : 1; + } + + return new Date(a.lastMessage.createdAt) > + new Date(b.lastMessage.createdAt) + ? -1 + : 1; }); } } diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index ee32819a44a..4e1cd9d4b91 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -220,7 +220,12 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do thread.channel = original_message.chat_channel end - transient :with_replies, :channel, :original_message_user, :old_om, use_service: false + transient :with_replies, + :channel, + :original_message_user, + :old_om, + use_service: false, + notification_level: :tracking original_message do |attrs| Fabricate( @@ -239,7 +244,7 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do attrs[:created_at] = 1.week.ago if transients[:old_om] thread.original_message.update!(**attrs) - thread.add(thread.original_message_user) + thread.add(thread.original_message_user, notification_level: transients[:notification_level]) if transients[:with_replies] Fabricate diff --git a/plugins/chat/spec/system/list_channels/drawer_spec.rb b/plugins/chat/spec/system/list_channels/drawer_spec.rb index 1a27e4a27fb..757d76baad4 100644 --- a/plugins/chat/spec/system/list_channels/drawer_spec.rb +++ b/plugins/chat/spec/system/list_channels/drawer_spec.rb @@ -36,8 +36,20 @@ RSpec.describe "List channels | Drawer", type: :system do context "when multiple channels are present" do fab!(:channel_1) { Fabricate(:category_channel, name: "a channel") } fab!(:channel_2) { Fabricate(:category_channel, name: "b channel") } - fab!(:channel_3) { Fabricate(:category_channel, name: "c channel") } + fab!(:channel_3) { Fabricate(:category_channel, name: "c channel", threading_enabled: true) } fab!(:channel_4) { Fabricate(:category_channel, name: "d channel") } + fab!(:message) do + Fabricate(:chat_message, chat_channel: channel_3, user: current_user, use_service: true) + end + fab!(:thread) do + Fabricate( + :chat_thread, + channel: channel_3, + original_message: message, + with_replies: 2, + use_service: true, + ) + end before do channel_1.add(current_user) @@ -46,32 +58,31 @@ RSpec.describe "List channels | Drawer", type: :system do channel_4.add(current_user) end - it "sorts them by urgent, unread, then by slug" do + it "sorts them by urgent, unread messages or threads, then by slug" do drawer_page.visit_index Fabricate( :chat_message, - chat_channel: channel_1, - created_at: 5.minutes.ago, - use_service: true, - ) - Fabricate( - :chat_message, - chat_channel: channel_2, - created_at: 2.minutes.ago, - use_service: true, - ) - Fabricate( - :chat_message, - chat_channel: channel_3, + chat_channel: channel_4, message: "@#{current_user.username}", use_service: true, ) + + expect(drawer_page).to have_channel_at_position(channel_4, 1) + expect(drawer_page).to have_channel_at_position(channel_3, 2) + expect(drawer_page).to have_channel_at_position(channel_1, 3) + expect(drawer_page).to have_channel_at_position(channel_2, 4) + end + + it "sorts by slug when multiple channels have the same unread count" do + drawer_page.visit_index + Fabricate(:chat_message, chat_channel: channel_2, use_service: true) Fabricate(:chat_message, chat_channel: channel_4, use_service: true) - expect(drawer_page).to have_channel_at_position(channel_3, 1) - expect(drawer_page).to have_channel_at_position(channel_1, 2) - expect(drawer_page).to have_channel_at_position(channel_2, 3) + expect(drawer_page).to have_channel_at_position(channel_2, 1) + expect(drawer_page).to have_channel_at_position(channel_3, 2) + expect(drawer_page).to have_channel_at_position(channel_4, 3) + expect(drawer_page).to have_channel_at_position(channel_1, 4) end end end @@ -108,19 +119,71 @@ RSpec.describe "List channels | Drawer", type: :system do context "when multiple channels are present" do fab!(:user_1) { Fabricate(:user) } + fab!(:user_2) { Fabricate(:user) } + fab!(:user_3) { Fabricate(:user) } fab!(:dm_channel_1) { Fabricate(:direct_message_channel, users: [current_user]) } fab!(:dm_channel_2) { Fabricate(:direct_message_channel, users: [current_user, user_1]) } - - before do - Fabricate(:chat_message, chat_channel: dm_channel_2, user: user_1, use_service: true) - end + fab!(:dm_channel_3) { Fabricate(:direct_message_channel, users: [current_user, user_2]) } + fab!(:dm_channel_4) { Fabricate(:direct_message_channel, users: [current_user, user_3]) } it "sorts them by latest activity" do drawer_page.visit_index drawer_page.click_direct_messages + Fabricate(:chat_message, chat_channel: dm_channel_2, user: user_1, use_service: true) + + # last message was read but the created at date is used for sorting + message = + Fabricate(:chat_message, chat_channel: dm_channel_4, user: user_3, use_service: true) + dm_channel_4.membership_for(current_user).mark_read!(message.id) + expect(drawer_page).to have_channel_at_position(dm_channel_2, 1) - expect(drawer_page).to have_channel_at_position(dm_channel_1, 2) + expect(drawer_page).to have_urgent_channel(dm_channel_2) + + expect(drawer_page).to have_channel_at_position(dm_channel_4, 2) + expect(drawer_page).to have_channel_at_position(dm_channel_1, 3) + expect(drawer_page).to have_channel_at_position(dm_channel_3, 4) + end + + it "sorts channels with threads by urgency" do + drawer_page.visit_index + drawer_page.click_direct_messages + + Fabricate( + :chat_thread, + notification_level: :watching, + original_message: + Fabricate( + :chat_message, + chat_channel: dm_channel_4, + user: current_user, + use_service: true, + ), + with_replies: 2, + use_service: true, + ) + + Fabricate( + :chat_thread, + original_message: + Fabricate( + :chat_message, + chat_channel: dm_channel_3, + user: current_user, + use_service: true, + ), + with_replies: 2, + use_service: true, + ) + + expect(drawer_page).to have_channel_at_position(dm_channel_4, 1) + expect(drawer_page).to have_urgent_channel(dm_channel_4) + + expect(drawer_page).to have_channel_at_position(dm_channel_3, 2) + expect(drawer_page).to have_unread_channel(dm_channel_3) + + expect(drawer_page).to have_channel_at_position(dm_channel_1, 3) + expect(drawer_page).to have_channel_at_position(dm_channel_2, 4) end end end diff --git a/plugins/chat/spec/system/page_objects/chat/components/channels_index.rb b/plugins/chat/spec/system/page_objects/chat/components/channels_index.rb index 342daa5ee88..f61b651ce86 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/channels_index.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/channels_index.rb @@ -37,9 +37,17 @@ module PageObjects has_no_css?(channel_row_selector(channel)) end - def has_unread_channel?(channel, count: nil, wait: Capybara.default_max_wait_time) + def has_unread_channel?( + channel, + count: nil, + urgent: false, + wait: Capybara.default_max_wait_time + ) unread_indicator_selector = "#{channel_row_selector(channel)} .chat-channel-unread-indicator" + + unread_indicator_selector += ".-urgent" if urgent + has_css?(unread_indicator_selector) && if count has_css?( 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 3109b5160d9..a10d8503381 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 @@ -70,6 +70,10 @@ module PageObjects channels_index.has_no_unread_channel?(channel) end + def has_urgent_channel?(channel) + channels_index.has_unread_channel?(channel, urgent: true) + end + def has_user_threads_section? has_css?("#c-footer-threads") end