From f06a5635b3fb4d8bb7153803d9dd64ae2485d078 Mon Sep 17 00:00:00 2001 From: Gary Pendergast Date: Thu, 21 Nov 2024 08:24:26 +1100 Subject: [PATCH] FIX: Improve the reliability of the unread channel keyboard shortcuts (#29814) Additionally, a bunch of related flaky tests were unflakified. --- .../discourse/models/chat-channel.js | 10 +++ .../services/chat-channels-manager.js | 18 +---- .../javascripts/discourse/services/chat.js | 27 +++++++ .../system/page_objects/sidebar/sidebar.rb | 14 +++- .../spec/system/shortcuts/sidebar_spec.rb | 79 ++++++++++++------- 5 files changed, 103 insertions(+), 45 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 209941f5bc5..440f9f84a62 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -201,6 +201,16 @@ export default class ChatChannel { return this.meta.can_join_chat_channel; } + get hasUnread() { + return ( + this.tracking.unreadCount + + this.tracking.mentionCount + + this.tracking.watchedThreadsUnreadCount + + this.threadsManager.unreadThreadCount > + 0 + ); + } + async stageMessage(message) { message.id = guid(); message.staged = true; 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 fbfd0d552d5..b06fa4e6913 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js @@ -133,14 +133,7 @@ export default class ChatChannelsManager extends Service { @cached get publicMessageChannelsWithActivity() { - return this.publicMessageChannels.filter( - (channel) => - channel.tracking.unreadCount + - channel.tracking.mentionCount + - channel.tracking.watchedThreadsUnreadCount + - channel.threadsManager.unreadThreadCount > - 0 - ); + return this.publicMessageChannels.filter((channel) => channel.hasUnread); } get publicMessageChannelsByActivity() { @@ -159,14 +152,7 @@ export default class ChatChannelsManager extends Service { @cached get directMessageChannelsWithActivity() { - return this.directMessageChannels.filter( - (channel) => - channel.tracking.unreadCount + - channel.tracking.mentionCount + - channel.tracking.watchedThreadsUnreadCount + - channel.threadsManager.unreadThreadCount > - 0 - ); + return this.directMessageChannels.filter((channel) => channel.hasUnread); } get truncatedDirectMessageChannels() { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index f976fb7615e..d5c39bad8f5 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -291,6 +291,33 @@ export default class Chat extends Service { this.chatChannelsManager.publicMessageChannelsWithActivity; directChannels = this.chatChannelsManager.directMessageChannelsWithActivity; + + // If the active channel has no unread messages, we need to manually insert it into + // the list, so we can find the next/previous unread channel. + if (!activeChannel.hasUnread) { + const allChannels = activeChannel.isDirectMessageChannel + ? this.chatChannelsManager.directMessageChannels + : this.chatChannelsManager.publicMessageChannels; + + // Find the ID of the channel before the active channel, which is unread + let checkChannelIndex = + allChannels.findIndex((c) => c.id === activeChannel.id) - 1; + + // If we get back to the start of the list, we can stop + while (checkChannelIndex >= 0) { + if (allChannels[checkChannelIndex].hasUnread) { + break; + } + checkChannelIndex--; + } + + // Insert the active channel after unread channel we found (or at the start of the list) + if (activeChannel.isDirectMessageChannel) { + directChannels.splice(checkChannelIndex + 1, 0, activeChannel); + } else { + publicChannels.splice(checkChannelIndex + 1, 0, activeChannel); + } + } } else { publicChannels = this.chatChannelsManager.publicMessageChannels; directChannels = this.chatChannelsManager.directMessageChannels; diff --git a/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb b/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb index c95bb847741..f1ae291fcf1 100644 --- a/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb +++ b/plugins/chat/spec/system/page_objects/sidebar/sidebar.rb @@ -64,14 +64,24 @@ module PageObjects end def has_unread_channel?(channel) - has_css?(".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread") + has_css?( + ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix:is(.unread, .urgent)", + ) end def has_no_unread_channel?(channel) has_no_css?( - ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread", + ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix:is(.unread, .urgent)", ) end + + def has_active_channel?(channel) + has_css?(".sidebar-section-link.channel-#{channel.id}.active") + end + + def has_no_active_channel?(channel) + has_no_css?(".sidebar-section-link.channel-#{channel.id}.active") + end end end end diff --git a/plugins/chat/spec/system/shortcuts/sidebar_spec.rb b/plugins/chat/spec/system/shortcuts/sidebar_spec.rb index 71dcf74397e..48956303dd0 100644 --- a/plugins/chat/spec/system/shortcuts/sidebar_spec.rb +++ b/plugins/chat/spec/system/shortcuts/sidebar_spec.rb @@ -4,6 +4,7 @@ RSpec.describe "Shortcuts | sidebar", type: :system do fab!(:current_user) { Fabricate(:admin) } let(:chat) { PageObjects::Pages::Chat.new } + let(:sidebar_page) { PageObjects::Pages::Sidebar.new } before do SiteSetting.navigation_menu = "sidebar" @@ -22,41 +23,43 @@ RSpec.describe "Shortcuts | sidebar", type: :system do visit("/") find("body").send_keys(%i[alt arrow_down]) - expect(page).to have_no_selector(".channel-#{channel_1.id}.active") - expect(page).to have_no_selector(".channel-#{dm_channel_1.id}.active") + expect(sidebar_page).to have_no_active_channel(channel_1) + expect(sidebar_page).to have_no_active_channel(dm_channel_1) end end context "when on chat page" do it "navigates through the channels" do chat.visit_channel(channel_1) - - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) find("body").send_keys(%i[alt arrow_down]) - expect(page).to have_selector(".channel-#{dm_channel_1.id}.active") + expect(sidebar_page).to have_active_channel(dm_channel_1) find("body").send_keys(%i[alt arrow_down]) - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) find("body").send_keys(%i[alt arrow_up]) - expect(page).to have_selector(".channel-#{dm_channel_1.id}.active") + expect(sidebar_page).to have_active_channel(dm_channel_1) end end end context "when using Alt+Shift+Up/Down arrows" do - fab!(:channel_1) { Fabricate(:chat_channel) } - fab!(:channel_2) { Fabricate(:chat_channel) } + fab!(:channel_1) { Fabricate(:chat_channel, name: "Channel 1") } + fab!(:channel_2) { Fabricate(:chat_channel, name: "Channel 2") } + fab!(:channel_3) { Fabricate(:chat_channel, name: "Channel 3") } fab!(:dm_channel_1) { Fabricate(:direct_message_channel, users: [current_user]) } - fab!(:dm_channel_2) { Fabricate(:direct_message_channel, users: [current_user]) } + fab!(:other_user) { Fabricate(:user) } + fab!(:dm_channel_2) { Fabricate(:direct_message_channel, users: [current_user, other_user]) } before do channel_1.add(current_user) channel_2.add(current_user) + channel_3.add(current_user) end context "when on homepage" do @@ -64,34 +67,37 @@ RSpec.describe "Shortcuts | sidebar", type: :system do visit("/") find("body").send_keys(%i[alt shift arrow_down]) - expect(page).to have_no_selector(".channel-#{channel_1.id}.active") - expect(page).to have_no_selector(".channel-#{channel_2.id}.active") - expect(page).to have_no_selector(".channel-#{dm_channel_1.id}.active") - expect(page).to have_no_selector(".channel-#{dm_channel_2.id}.active") + expect(sidebar_page).to have_no_active_channel(channel_1) + expect(sidebar_page).to have_no_active_channel(channel_2) + expect(sidebar_page).to have_no_active_channel(dm_channel_1) + expect(sidebar_page).to have_no_active_channel(dm_channel_2) end end context "when on chat page" do it "does nothing when no channels have activity" do chat.visit_channel(channel_1) - - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) find("body").send_keys(%i[alt shift arrow_down]) - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) find("body").send_keys(%i[alt shift arrow_down]) - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) find("body").send_keys(%i[alt shift arrow_up]) - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) end it "navigates through the channels with activity" do + chat.visit_channel(channel_1) + expect(sidebar_page).to have_active_channel(channel_1) + Fabricate(:chat_message, chat_channel: channel_2, message: "hello!", use_service: true) + expect(sidebar_page).to have_unread_channel(channel_2) Fabricate( :chat_message, @@ -99,18 +105,17 @@ RSpec.describe "Shortcuts | sidebar", type: :system do message: "hello here!", use_service: true, ) - - chat.visit_channel(channel_1) - - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_unread_channel(dm_channel_2) find("body").send_keys(%i[alt shift arrow_down]) - expect(page).to have_selector(".channel-#{channel_2.id}.active") + expect(sidebar_page).to have_active_channel(channel_2) + expect(sidebar_page).to have_no_unread_channel(channel_2) find("body").send_keys(%i[alt shift arrow_down]) - expect(page).to have_selector(".channel-#{dm_channel_2.id}.active") + expect(sidebar_page).to have_active_channel(dm_channel_2) + expect(sidebar_page).to have_no_unread_channel(dm_channel_2) Fabricate( :chat_message, @@ -118,16 +123,36 @@ RSpec.describe "Shortcuts | sidebar", type: :system do message: "hello again!", use_service: true, ) + expect(sidebar_page).to have_unread_channel(channel_1) find("body").send_keys(%i[alt shift arrow_up]) - expect(page).to have_selector(".channel-#{channel_1.id}.active") + expect(sidebar_page).to have_active_channel(channel_1) + expect(sidebar_page).to have_no_unread_channel(channel_1) Fabricate(:chat_message, chat_channel: dm_channel_1, message: "bye now!", use_service: true) + expect(sidebar_page).to have_unread_channel(dm_channel_1) find("body").send_keys(%i[alt shift arrow_up]) - expect(page).to have_selector(".channel-#{dm_channel_1.id}.active") + expect(sidebar_page).to have_active_channel(dm_channel_1) + expect(sidebar_page).to have_no_unread_channel(dm_channel_1) + end + + it "remembers where the current channel is, even if that channel is unread" do + chat.visit_channel(channel_2) + expect(sidebar_page).to have_active_channel(channel_2) + + Fabricate(:chat_message, chat_channel: channel_1, message: "hello!", use_service: true) + expect(sidebar_page).to have_unread_channel(channel_1) + + Fabricate(:chat_message, chat_channel: channel_3, message: "hello!", use_service: true) + expect(sidebar_page).to have_unread_channel(channel_3) + + find("body").send_keys(%i[alt shift arrow_down]) + + expect(sidebar_page).to have_active_channel(channel_3) + expect(sidebar_page).to have_no_unread_channel(channel_3) end end end