From 10d155ea41d4a14f477df225b39f1246700749b3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 14 Jul 2023 16:08:35 +1000 Subject: [PATCH] DEV: Further improve thread list query and add spec (#22610) Followup to d7ef7b9c030a1694eee8e957a6f287613637687a, this adds a spec to test the case where old threads are still unread for the user and should show at the top regardless of pagination, and fixes some issues/makes some slight refactors. --- plugins/chat/app/models/chat/thread.rb | 6 ++++ .../services/chat/lookup_channel_threads.rb | 11 +++++-- .../chat/update_user_thread_last_read.rb | 13 +++------ .../chat/lookup_channel_threads_spec.rb | 29 +++++++++++++++++++ 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 5372ef880c9..236bc72dba4 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -49,6 +49,12 @@ module Chat user_chat_thread_memberships.find_by(user: user) end + def mark_read_for_user!(user, last_read_message_id: nil) + membership_for(user)&.update!( + last_read_message_id: last_read_message_id || self.last_message_id, + ) + end + def replies self.chat_messages.where.not(id: self.original_message_id).order("created_at ASC, id ASC") end diff --git a/plugins/chat/app/services/chat/lookup_channel_threads.rb b/plugins/chat/app/services/chat/lookup_channel_threads.rb index afcb5ead5b0..307df0cec5d 100644 --- a/plugins/chat/app/services/chat/lookup_channel_threads.rb +++ b/plugins/chat/app/services/chat/lookup_channel_threads.rb @@ -97,7 +97,10 @@ module Chat user: :user_status, ], ) - .joins(:user_chat_thread_memberships, :last_message, :original_message) + .joins(:user_chat_thread_memberships, :original_message) + .joins( + "LEFT JOIN chat_messages AS last_message ON last_message.id = chat_threads.last_message_id", + ) .where("user_chat_thread_memberships.user_id = ?", guardian.user.id) .where( "user_chat_thread_memberships.notification_level IN (?)", @@ -107,10 +110,14 @@ module Chat ], ) .where("chat_threads.channel_id = ?", channel.id) + .where("last_message.deleted_at IS NULL") .limit(context.limit) .offset(context.offset) .order( - "CASE WHEN chat_threads.last_message_id > user_chat_thread_memberships.last_read_message_id THEN 0 ELSE 1 END, chat_messages.created_at DESC", + "CASE WHEN ( + chat_threads.last_message_id > user_chat_thread_memberships.last_read_message_id OR + user_chat_thread_memberships.last_read_message_id IS NULL + ) THEN 0 ELSE 1 END, last_message.created_at DESC", ) end diff --git a/plugins/chat/app/services/chat/update_user_thread_last_read.rb b/plugins/chat/app/services/chat/update_user_thread_last_read.rb index ef4958fa143..8785b41b92c 100644 --- a/plugins/chat/app/services/chat/update_user_thread_last_read.rb +++ b/plugins/chat/app/services/chat/update_user_thread_last_read.rb @@ -44,16 +44,11 @@ module Chat guardian.can_join_chat_channel?(thread.channel) end - # NOTE: In future we will pass in the last_read_message_id - # to the service and this query will be unnecessary. + # NOTE: In future we will pass in a specific last_read_message_id + # to the service, so this will need to change because currently it's + # just using the thread's last_message_id. def mark_thread_read(thread:, guardian:, **) - query = <<~SQL - UPDATE user_chat_thread_memberships - SET last_read_message_id = chat_threads.last_message_id - FROM chat_threads - WHERE user_id = :user_id AND thread_id = :thread_id AND chat_threads.id = :thread_id - SQL - DB.exec(query, thread_id: thread.id, user_id: guardian.user.id) + thread.mark_read_for_user!(guardian.user) end def mark_associated_mentions_as_read(thread:, guardian:, **) diff --git a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb index e1f83745ed2..bad3021add4 100644 --- a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb +++ b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb @@ -155,6 +155,35 @@ RSpec.describe ::Chat::LookupChannelThreads do expect(result.threads.map(&:id)).to eq([thread_2.id, thread_1.id, thread_3.id]) end + describe "when there are more threads than the limit" do + let(:limit) { 5 } + + it "sorts very old unreads to top over recency, and sorts both unreads and other threads by recency" do + thread_4 = Fabricate(:chat_thread, channel: channel_1) + thread_5 = Fabricate(:chat_thread, channel: channel_1) + thread_6 = Fabricate(:chat_thread, channel: channel_1) + thread_7 = Fabricate(:chat_thread, channel: channel_1) + + [thread_4, thread_5, thread_6, thread_7].each do |t| + t.add(current_user) + t.mark_read_for_user!(current_user) + end + [thread_1, thread_2, thread_3].each { |t| t.mark_read_for_user!(current_user) } + + # The old unread messages. + Fabricate(:chat_message, chat_channel: channel_1, thread: thread_7).update!( + created_at: 2.months.ago, + ) + Fabricate(:chat_message, chat_channel: channel_1, thread: thread_6).update!( + created_at: 3.months.ago, + ) + + expect(result.threads.map(&:id)).to eq( + [thread_7.id, thread_6.id, thread_5.id, thread_4.id, thread_1.id], + ) + end + end + it "does not return threads where the original message is trashed" do thread_1.original_message.trash!