DEV: Further improve thread list query and add spec (#22610)

Followup to d7ef7b9c03,
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

@@ -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

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
 

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
+    # NOTE: In future we will pass in a specific last_read_message_id
-    # to the service and this query will be unnecessary.
+    # 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
+      thread.mark_read_for_user!(guardian.user)
-        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)
     end
 
     def mark_associated_mentions_as_read(thread:, guardian:, **)

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!