From 6efae69c61b607ed3398a36bbc2ca1cf9ae0cca8 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 19 Jul 2023 10:09:22 +1000
Subject: [PATCH] FIX: Doubled up or not tracked threads in thread list
(#22631)
This commit fixes two issues with the thread list:
1. All threads were being shown regardless of whether the user had
a membership in the thread. This was happening because the list
and the channel share the same thread store, so if the channel
had OMs with threads we would load them and they showed in the list.
2. Threads created by the user from a staged thread would double up.
This is because the _cache in the channel threadsManager would use
the staged thread ID even after we'd replaced the object's ID with
the actual thread from the DB. The answer to this is to remove and
re-add the thread to the local cache with the actual ID.
---
.../discourse/components/chat/thread-list.js | 5 ++-
.../discourse/lib/chat-threads-manager.js | 4 ++
.../chat-pane-base-subscriptions-manager.js | 10 +++++
.../spec/system/thread_list/full_page_spec.rb | 44 +++++++++++++++++++
4 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread-list.js b/plugins/chat/assets/javascripts/discourse/components/chat/thread-list.js
index 00347e08e1d..c3d572d5560 100644
--- a/plugins/chat/assets/javascripts/discourse/components/chat/thread-list.js
+++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread-list.js
@@ -19,7 +19,10 @@ export default class ChatThreadList extends Component {
// are sent to the thread, and we want the list to react in realtime to this.
get sortedThreads() {
return this.threadsManager.threads
- .filter((thread) => !thread.originalMessage.deletedAt)
+ .filter(
+ (thread) =>
+ thread.currentUserMembership && !thread.originalMessage.deletedAt
+ )
.sort((threadA, threadB) => {
// If both are unread we just want to sort by last reply date + time descending.
if (threadA.tracking.unreadCount && threadB.tracking.unreadCount) {
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 f2c1d565232..c890616e166 100644
--- a/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js
+++ b/plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js
@@ -66,6 +66,10 @@ export default class ChatThreadsManager {
}
}
+ remove(threadObject) {
+ delete this._cached[threadObject.id];
+ }
+
add(channel, threadObject, options = {}) {
let model;
diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js
index c4f2b4bb9ff..27f42257b3f 100644
--- a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js
+++ b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js
@@ -231,6 +231,16 @@ export default class ChatPaneBaseSubscriptionsManager extends Service {
stagedThread.id = data.thread_id;
stagedThread.originalMessage.thread = stagedThread;
stagedThread.originalMessage.thread.preview.replyCount ??= 1;
+
+ // We have to do this because the thread manager cache is keyed by
+ // staged_thread_id, but the thread_id is what we want to use to
+ // look up the thread, otherwise calls to .find() will not return
+ // the thread by its actual ID, and we will end up with double-ups
+ // in places like the thread list when .add() is called.
+ this.model.threadsManager.remove({ id: data.staged_thread_id });
+ this.model.threadsManager.add(this.model, stagedThread, {
+ replace: true,
+ });
} else if (data.thread_id) {
this.model.threadsManager
.find(this.model.id, data.thread_id, { fetchIfNotFound: true })
diff --git a/plugins/chat/spec/system/thread_list/full_page_spec.rb b/plugins/chat/spec/system/thread_list/full_page_spec.rb
index 89cdf66a026..a9dc724c280 100644
--- a/plugins/chat/spec/system/thread_list/full_page_spec.rb
+++ b/plugins/chat/spec/system/thread_list/full_page_spec.rb
@@ -26,6 +26,50 @@ describe "Thread list in side panel | full page", type: :system do
end
end
+ context "for threads the user is not a participant in" do
+ fab!(:thread_om) { Fabricate(:chat_message, chat_channel: channel) }
+
+ before { chat_system_user_bootstrap(user: other_user, channel: channel) }
+
+ it "does not show existing threads in the channel if the user is not tracking them" do
+ Fabricate(:chat_thread, original_message: thread_om, channel: channel)
+ chat_page.visit_channel(channel)
+ channel_page.open_thread_list
+ expect(page).to have_content(I18n.t("js.chat.threads.none"))
+ end
+
+ it "does not show new threads in the channel in the thread list if the user is not tracking them" do
+ chat_page.visit_channel(channel)
+
+ using_session(:other_user) do |session|
+ sign_in(other_user)
+ chat_page.visit_channel(channel)
+ channel_page.reply_to(thread_om)
+ thread_page.send_message("hey everyone!")
+ expect(channel_page).to have_thread_indicator(thread_om)
+ session.quit
+ end
+
+ channel_page.open_thread_list
+ expect(page).to have_content(I18n.t("js.chat.threads.none"))
+ end
+
+ describe "when the user creates a new thread" do
+ it "does not double up the staged thread and the actual thread in the list" do
+ chat_page.visit_channel(channel)
+ channel_page.reply_to(thread_om)
+ thread_page.send_message("hey everyone!")
+ expect(channel_page).to have_thread_indicator(thread_om)
+ thread_page.close
+ channel_page.open_thread_list
+ expect(page).to have_css(
+ thread_list_page.item_by_id_selector(thread_om.reload.thread_id),
+ count: 1,
+ )
+ end
+ end
+ end
+
context "when there are threads that the user is participating in" do
fab!(:thread_1) do
chat_thread_chain_bootstrap(channel: channel, users: [current_user, other_user])