DEV: Add last_message_id to channel and thread (#22488)

Initial migration and changes to models as well as
changing the following services to update last_message_id:

* Chat::MessageCreator
* Chat::RestoreMessage
* Chat::TrashMessage

The data migration will set the `last_message_id` for all existing
threads and channels in the database.

When we query the thread list as well as the channel,
we look at the last message ID for the following:

* Channel - Sorting DM channels, and channel metadata for the list of channels
* Thread - Last reply details for thread indicators and thread list

plugins/chat/spec/components/chat/message_creator_spec.rb

@@ -120,16 +120,14 @@ describe Chat::MessageCreator do
       }.to change { Chat::Message.count }.by(1)
     end
 
-    it "updates the channel’s last message date" do
-      previous_last_message_sent_at = public_chat_channel.last_message_sent_at
-
-      described_class.create(
-        chat_channel: public_chat_channel,
-        user: user1,
-        content: "this is a message",
-      )
-
-      expect(previous_last_message_sent_at).to be < public_chat_channel.reload.last_message_sent_at
+    it "updates the last_message for the channel" do
+      message =
+        described_class.create(
+          chat_channel: public_chat_channel,
+          user: user1,
+          content: "this is a message",
+        ).chat_message
+      expect(public_chat_channel.reload.last_message).to eq(message)
     end
 
     it "sets the last_editor_id to the user who created the message" do
@@ -640,6 +638,18 @@ describe Chat::MessageCreator do
         expect(message.in_reply_to.thread).to eq(message.thread)
         expect(message.thread.original_message).to eq(reply_message)
         expect(message.thread.original_message_user).to eq(reply_message.user)
+        expect(message.thread.last_message).to eq(message)
+      end
+
+      it "does not change the last_message of the channel for a thread reply" do
+        original_last_message = public_chat_channel.last_message
+        described_class.create(
+          chat_channel: public_chat_channel,
+          user: user1,
+          content: "this is a message",
+          in_reply_to_id: reply_message.id,
+        )
+        expect(public_chat_channel.reload.last_message).to eq(original_last_message)
       end
 
       it "creates a user thread membership" do
@@ -756,6 +766,7 @@ describe Chat::MessageCreator do
           }.not_to change { Chat::Thread.count }
 
           expect(message.reload.thread).to eq(existing_thread)
+          expect(existing_thread.reload.last_message).to eq(message)
         end
 
         it "creates a user thread membership if one does not exist" do

plugins/chat/spec/fabricators/chat_fabricator.rb

@@ -160,6 +160,7 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do
   transient :with_replies
   transient :channel
   transient :original_message_user
+  transient :old_om
 
   original_message do |attrs|
     Fabricate(
@@ -170,7 +171,13 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do
   end
 
   after_create do |thread, transients|
-    thread.original_message.update!(thread_id: thread.id)
+    attrs = { thread_id: thread.id }
+
+    # Sometimes we  make this older via created_at so any messages fabricated for this thread
+    # afterwards are not created earlier in time than the OM.
+    attrs[:created_at] = 1.week.ago if transients[:old_om]
+
+    thread.original_message.update!(**attrs)
     thread.add(thread.original_message_user)
 
     if transients[:with_replies]

plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe "Chat::Thread replies_count cache accuracy" do
 
     # Lose the cache intentionally.
     Chat::Thread.clear_caches!(thread.id)
-    message_to_destroy = thread.last_reply
+    message_to_destroy = thread.last_message
     Chat::TrashMessage.call(
       message_id: message_to_destroy.id,
       channel_id: thread.channel_id,

plugins/chat/spec/lib/chat/channel_fetcher_spec.rb

@@ -332,7 +332,7 @@ describe Chat::ChannelFetcher do
   end
 
   describe ".secured_direct_message_channels" do
-    it "includes direct message channels the user is a member of ordered by last_message_sent_at" do
+    it "includes direct message channels the user is a member of ordered by last_message.created_at" do
       Fabricate(
         :user_chat_channel_membership_for_dm,
         chat_channel: direct_message_channel1,
@@ -350,8 +350,11 @@ describe Chat::ChannelFetcher do
       Chat::DirectMessageUser.create!(direct_message: dm_channel2, user: user1)
       Chat::DirectMessageUser.create!(direct_message: dm_channel2, user: user2)
 
-      direct_message_channel1.update!(last_message_sent_at: 1.day.ago)
-      direct_message_channel2.update!(last_message_sent_at: 1.hour.ago)
+      Fabricate(:chat_message, user: user1, chat_channel: direct_message_channel1)
+      Fabricate(:chat_message, user: user1, chat_channel: direct_message_channel2)
+
+      direct_message_channel1.last_message.update!(created_at: 1.day.ago)
+      direct_message_channel2.last_message.update!(created_at: 1.hour.ago)
 
       expect(described_class.secured_direct_message_channels(user1.id, guardian).map(&:id)).to eq(
         [direct_message_channel2.id, direct_message_channel1.id],

plugins/chat/spec/models/chat/channel_spec.rb

@@ -186,4 +186,43 @@ RSpec.describe Chat::Channel do
       )
     end
   end
+
+  describe "#latest_not_deleted_message_id" do
+    fab!(:channel) { Fabricate(:category_channel) }
+    fab!(:old_message) { Fabricate(:chat_message, chat_channel: channel) }
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
+
+    before { old_message.update!(created_at: 1.day.ago) }
+
+    it "accepts an anchor message to only get messages of a lower id" do
+      expect(channel.latest_not_deleted_message_id(anchor_message_id: message_1.id)).to eq(
+        old_message.id,
+      )
+    end
+
+    it "gets the latest message by created_at" do
+      expect(channel.latest_not_deleted_message_id).to eq(message_1.id)
+    end
+
+    it "does not get other channel messages" do
+      Fabricate(:chat_message)
+      expect(channel.latest_not_deleted_message_id).to eq(message_1.id)
+    end
+
+    it "does not get thread replies" do
+      thread = Fabricate(:chat_thread, channel: channel, old_om: true)
+      message_1.update!(thread: thread)
+      expect(channel.latest_not_deleted_message_id).to eq(old_message.id)
+    end
+
+    it "does get thread original message" do
+      thread = Fabricate(:chat_thread, channel: channel)
+      expect(channel.latest_not_deleted_message_id).to eq(thread.original_message_id)
+    end
+
+    it "does not get deleted messages" do
+      message_1.trash!
+      expect(channel.latest_not_deleted_message_id).to eq(old_message.id)
+    end
+  end
 end

plugins/chat/spec/models/chat/thread_spec.rb

@@ -213,4 +213,33 @@ RSpec.describe Chat::Thread do
       end
     end
   end
+
+  describe "#latest_not_deleted_message_id" do
+    fab!(:channel) { Fabricate(:category_channel) }
+    fab!(:thread) { Fabricate(:chat_thread, channel: channel, old_om: true) }
+    fab!(:old_message) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
+
+    before { old_message.update!(created_at: 1.day.ago) }
+
+    it "accepts an anchor message to only get messages of a lower id" do
+      expect(thread.latest_not_deleted_message_id(anchor_message_id: message_1.id)).to eq(
+        old_message.id,
+      )
+    end
+
+    it "gets the latest message by created_at" do
+      expect(thread.latest_not_deleted_message_id).to eq(message_1.id)
+    end
+
+    it "does not get other channel messages" do
+      Fabricate(:chat_message)
+      expect(thread.latest_not_deleted_message_id).to eq(message_1.id)
+    end
+
+    it "does not get deleted messages" do
+      message_1.trash!
+      expect(thread.latest_not_deleted_message_id).to eq(old_message.id)
+    end
+  end
 end

plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb

@@ -27,7 +27,9 @@ RSpec.describe Chat::Action::ResetUserLastReadChannelMessage do
   context "when there are non-deleted messages left in the channel" do
     before do
       message_3.trash!
+      message_3.chat_channel.update_last_message_id!
       message_6.trash!
+      message_6.chat_channel.update_last_message_id!
     end
 
     it "sets the matching membership last_read_message_ids to the most recently created message ID" do
@@ -38,7 +40,11 @@ RSpec.describe Chat::Action::ResetUserLastReadChannelMessage do
   end
 
   context "when there are no more non-deleted messages left in the channel" do
-    before { [message_1, message_2, message_4, message_5].each(&:trash!) }
+    before do
+      [message_1, message_2, message_4, message_5].each(&:trash!)
+      channel_1.update_last_message_id!
+      channel_2.update_last_message_id!
+    end
 
     it "sets the matching membership last_read_message_ids to NULL" do
       described_class.call([message_3.id, message_6.id], [channel_1.id, channel_2.id])

plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb

@@ -186,15 +186,17 @@ RSpec.describe ::Chat::LookupChannelThreads do
         thread_4.membership_for(current_user).update!(
           notification_level: ::Chat::UserChatThreadMembership.notification_levels[:muted],
         )
-        thread_5 = Fabricate(:chat_thread, channel: channel_1)
+        Fabricate(:chat_thread, channel: channel_1)
 
         expect(result.threads.map(&:id)).to eq([thread_1.id, thread_2.id, thread_3.id])
       end
 
       it "does not count deleted messages for sort order" do
+        original_last_message_id = thread_3.reload.last_message_id
         unread_message = Fabricate(:chat_message, chat_channel: channel_1, thread: thread_3)
         unread_message.update!(created_at: 2.days.ago)
         unread_message.trash!
+        thread_3.reload.update!(last_message_id: original_last_message_id)
 
         expect(result.threads.map(&:id)).to eq([thread_1.id, thread_2.id, thread_3.id])
       end

plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb

@@ -109,6 +109,7 @@ RSpec.describe Chat::MarkAllUserChannelsRead do
 
       it "does not use deleted messages for the last_read_message_id" do
         message_2.trash!
+        message_2.chat_channel.update_last_message_id!
         result
         expect(membership_1.reload.last_read_message_id).to eq(message_1.id)
       end

plugins/chat/spec/services/chat/message_destroyer_spec.rb

@@ -62,5 +62,11 @@ RSpec.describe Chat::MessageDestroyer do
       expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound)
       expect(message_2.reload).to be_present
     end
+
+    it "sets the last_message_id for the channel if that message is deleted" do
+      expect(message_1.chat_channel.last_message_id).to eq(message_1.id)
+      described_class.new.destroy_in_batches(Chat::Message.where(id: message_1.id))
+      expect(message_1.chat_channel.reload.last_message_id).to eq(nil)
+    end
   end
 end

plugins/chat/spec/services/chat/publisher_spec.rb

@@ -281,10 +281,12 @@ describe Chat::Publisher do
           {
             type: "channel",
             channel_id: channel.id,
-            message_id: message_1.id,
-            user_id: message_1.user_id,
-            username: message_1.user.username,
             thread_id: nil,
+            message:
+              Chat::MessageSerializer.new(
+                message_1,
+                { scope: Guardian.new(nil), root: false },
+              ).as_json,
           },
         )
       end
@@ -340,10 +342,12 @@ describe Chat::Publisher do
               {
                 type: "thread",
                 channel_id: channel.id,
-                message_id: message_1.id,
-                user_id: message_1.user_id,
-                username: message_1.user.username,
                 thread_id: thread.id,
+                message:
+                  Chat::MessageSerializer.new(
+                    message_1,
+                    { scope: Guardian.new(nil), root: false },
+                  ).as_json,
               },
             )
           end

plugins/chat/spec/services/chat/restore_message_spec.rb

@@ -5,7 +5,12 @@ RSpec.describe Chat::RestoreMessage do
   let!(:guardian) { Guardian.new(current_user) }
   fab!(:message) { Fabricate(:chat_message, user: current_user) }
 
-  before { message.trash! }
+  before do
+    message.trash!
+    message.chat_channel.update!(
+      last_message_id: message.chat_channel.latest_not_deleted_message_id,
+    )
+  end
 
   describe ".call" do
     subject(:result) { described_class.call(params) }
@@ -45,6 +50,19 @@ RSpec.describe Chat::RestoreMessage do
           expect(Chat::Message.find_by(id: message.id)).not_to be_nil
         end
 
+        it "updates the channel last_message_id if the message is now the last one in the channel" do
+          expect(message.chat_channel.reload.last_message_id).to be_nil
+          result
+          expect(message.chat_channel.reload.last_message_id).to eq(message.id)
+        end
+
+        it "does not update the channel last_message_id if the message is not the last one in the channel" do
+          next_message = Fabricate(:chat_message, chat_channel: message.chat_channel)
+          expect(message.chat_channel.reload.last_message_id).to eq(next_message.id)
+          result
+          expect(message.chat_channel.reload.last_message_id).to eq(next_message.id)
+        end
+
         it "publishes associated Discourse and MessageBus events" do
           freeze_time
           messages = nil
@@ -60,13 +78,30 @@ RSpec.describe Chat::RestoreMessage do
         context "when the message has a thread" do
           fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) }
 
-          before { message.update!(thread: thread) }
+          before do
+            message.update!(thread: thread)
+            thread.update_last_message_id!
+            thread.original_message.update!(created_at: message.created_at - 2.hours)
+          end
 
           it "increments the thread reply count" do
             thread.set_replies_count_cache(1)
             result
             expect(thread.replies_count_cache).to eq(2)
           end
+
+          it "updates the thread last_message_id if the message is now the last one in the thread" do
+            expect(message.thread.reload.last_message_id).to eq(thread.original_message_id)
+            result
+            expect(message.thread.reload.last_message_id).to eq(message.id)
+          end
+
+          it "does not update the thread last_message_id if the message is not the last one in the channel" do
+            next_message = Fabricate(:chat_message, thread: message.thread)
+            expect(message.thread.reload.last_message_id).to eq(next_message.id)
+            result
+            expect(message.thread.reload.last_message_id).to eq(next_message.id)
+          end
         end
       end
     end

plugins/chat/spec/services/chat/trash_message_spec.rb

@@ -115,10 +115,23 @@ RSpec.describe Chat::TrashMessage do
           expect(membership_2.reload.last_read_message_id).to be_nil
         end
 
+        it "updates the channel last_message_id to the previous message in the channel" do
+          next_message =
+            Fabricate(:chat_message, chat_channel: message.chat_channel, user: current_user)
+          params[:message_id] = next_message.id
+          expect(message.chat_channel.reload.last_message).to eq(next_message)
+          result
+          expect(message.chat_channel.reload.last_message).to eq(message)
+        end
+
         context "when the message has a thread" do
           fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) }
 
-          before { message.update!(thread: thread) }
+          before do
+            message.update!(thread: thread)
+            thread.update!(last_message: message)
+            thread.original_message.update!(created_at: message.created_at - 2.hours)
+          end
 
           it "decrements the thread reply count" do
             thread.set_replies_count_cache(5)
@@ -154,6 +167,22 @@ RSpec.describe Chat::TrashMessage do
             expect(membership_1.reload.last_read_message_id).to be_nil
             expect(membership_2.reload.last_read_message_id).to be_nil
           end
+
+          it "updates the thread last_message_id to the previous message in the thread" do
+            next_message = Fabricate(:chat_message, thread: thread, user: current_user)
+            params[:message_id] = next_message.id
+            expect(thread.reload.last_message).to eq(next_message)
+            result
+            expect(thread.reload.last_message).to eq(message)
+          end
+
+          context "when there are no other messages left in the thread except the original message" do
+            it "updates the thread last_message_id to the original message" do
+              expect(thread.last_message).to eq(message)
+              result
+              expect(thread.reload.last_message).to eq(thread.original_message)
+            end
+          end
         end
 
         context "when message is already deleted" do

plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Chat::UpdateUserThreadLastRead do
 
     fab!(:current_user) { Fabricate(:user) }
     fab!(:channel) { Fabricate(:chat_channel) }
-    fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+    fab!(:thread) { Fabricate(:chat_thread, channel: channel, old_om: true) }
     fab!(:thread_reply_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
     fab!(:thread_reply_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
 
@@ -95,7 +95,7 @@ RSpec.describe Chat::UpdateUserThreadLastRead do
 
           it "updates the last_read_message_id of the thread" do
             result
-            expect(membership.reload.last_read_message_id).to eq(thread.last_reply.id)
+            expect(membership.reload.last_read_message_id).to eq(thread.reload.last_message.id)
           end
         end
       end

plugins/chat/spec/support/api/schemas/category_chat_channel.json

@@ -21,7 +21,7 @@
     "chatable_url": { "type": "string" },
     "title": { "type": "string" },
     "chatable_id": { "type": "number" },
-    "last_message_sent_at": { "type": "string" },
+    "last_message": { "type": ["object", "null"] },
     "status": { "type": "string" },
     "chatable": {
       "type": "object",

plugins/chat/spec/system/channel_thread_message_echoing_spec.rb

@@ -79,10 +79,10 @@ describe "Channel thread message echoing", type: :system do
       current_user
         .user_chat_channel_memberships
         .find_by(chat_channel: channel)
-        .update!(last_read_message_id: thread.last_reply.id)
+        .update!(last_read_message_id: thread.last_message_id)
       chat_page.visit_channel(channel)
       expect(channel_page).not_to have_css(
-        channel_page.message_by_id_selector(thread.last_reply.id),
+        channel_page.message_by_id_selector(thread.last_message_id),
       )
     end
 

plugins/chat/spec/system/chat/composer/shortcuts/thread_spec.rb

@@ -59,7 +59,10 @@ RSpec.describe "Chat | composer | shortcuts | thread", type: :system do
     end
 
     context "when last message is deleted" do
-      before { last_thread_message.trash! }
+      before do
+        last_thread_message.trash!
+        thread_1.update_last_message_id!
+      end
 
       it "does not edit a message" do
         chat_page.visit_thread(thread_1)

plugins/chat/spec/system/message_thread_indicator_spec.rb

@@ -141,19 +141,19 @@ describe "Thread indicator for chat messages", type: :system do
     end
 
     it "shows an excerpt of the last reply in the thread" do
-      thread_1.last_reply.update!(message: "test for excerpt")
-      thread_1.last_reply.rebake!
+      thread_1.last_message.update!(message: "test for excerpt")
+      thread_1.last_message.rebake!
 
       chat_page.visit_channel(channel)
       expect(
         channel_page.message_thread_indicator(thread_1.original_message).excerpt,
-      ).to have_content(thread_excerpt(thread_1.last_reply))
+      ).to have_content(thread_excerpt(thread_1.last_message))
     end
 
     it "updates the last reply excerpt and participants when a new message is added to the thread" do
       new_user = Fabricate(:user)
       chat_system_user_bootstrap(user: new_user, channel: channel)
-      original_last_reply = thread_1.replies.last
+      original_last_reply = thread_1.last_message
       original_last_reply.update!(message: "test for excerpt")
       original_last_reply.rebake!
 

plugins/chat/spec/system/single_thread_spec.rb

@@ -112,7 +112,7 @@ describe "Single thread in side panel", type: :system do
         expect(side_panel).to have_open_thread(thread)
         thread_page.send_message("new thread message")
         expect(thread_page).to have_message(thread_id: thread.id, text: "new thread message")
-        thread_message = thread.last_reply
+        thread_message = thread.last_message
         expect(thread_message.chat_channel_id).to eq(channel.id)
         expect(thread_message.thread.channel_id).to eq(channel.id)
       end