FIX: ensures an empty last message won't cause errors (#23647)

This would cause an error when deleting the original message of a thread, due to the non existing `last_message`. This fix is implemented using the null pattern.

Note this commit is also using this opportunity to unify naming of null objects, `Chat::DeletedUser` becomes `Chat::NullUser`, it feels better to have a name describing what is the object, instead of a name describing why this object has to be used, which can change depending on cases.

plugins/chat/app/models/chat/channel.rb

@@ -27,6 +27,9 @@ module Chat
                class_name: "Chat::Message",
                foreign_key: :last_message_id,
                optional: true
+    def last_message
+      super || NullMessage.new
+    end
 
     enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false
 

plugins/chat/app/models/chat/direct_message.rb

@@ -31,9 +31,7 @@ module Chat
 
     def chat_channel_title_for_user(chat_channel, acting_user)
       users =
-        (direct_message_users.map(&:user) - [acting_user]).map do |user|
-          user || Chat::DeletedUser.new
-        end
+        (direct_message_users.map(&:user) - [acting_user]).map { |user| user || Chat::NullUser.new }
 
       # direct message to self
       if users.empty?

plugins/chat/app/models/chat/null_message.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module Chat
+  class NullMessage < Chat::Message
+    def user
+      nil
+    end
+
+    def excerpt(max_length: nil)
+      nil
+    end
+
+    def id
+      nil
+    end
+
+    def created_at
+      Time.now # a proper NullTime object would be better, but this is good enough for now
+    end
+  end
+end

plugins/chat/app/models/chat/null_user.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Chat
-  class DeletedUser < User
+  class NullUser < User
     def username
       I18n.t("chat.deleted_chat_username")
     end

plugins/chat/app/models/chat/thread.rb

@@ -30,6 +30,9 @@ module Chat
                class_name: "Chat::Message",
                foreign_key: :last_message_id,
                optional: true
+    def last_message
+      super || NullMessage.new
+    end
 
     enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false
 

plugins/chat/app/serializers/chat/direct_message_serializer.rb

@@ -7,7 +7,7 @@ module Chat
     has_many :users, serializer: Chat::ChatableUserSerializer, embed: :objects
 
     def users
-      users = object.direct_message_users.map(&:user).map { |u| u || Chat::DeletedUser.new }
+      users = object.direct_message_users.map(&:user).map { |u| u || Chat::NullUser.new }
 
       return users - [scope.user] if users.count > 1
       users

plugins/chat/app/serializers/chat/in_reply_to_serializer.rb

@@ -12,7 +12,7 @@ module Chat
     end
 
     def user
-      object.user || Chat::DeletedUser.new
+      object.user || Chat::NullUser.new
     end
   end
 end

plugins/chat/app/serializers/chat/message_serializer.rb

@@ -49,7 +49,7 @@ module Chat
     end
 
     def user
-      object.user || Chat::DeletedUser.new
+      object.user || Chat::NullUser.new
     end
 
     def excerpt

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

@@ -11,6 +11,14 @@ RSpec.describe Chat::Channel do
   it { is_expected.to validate_length_of(:chatable_type).is_at_most(100) }
   it { is_expected.to validate_length_of(:type).is_at_most(100) }
 
+  describe ".last_message" do
+    context "when there are no last message" do
+      it "returns an instance of NullMessage" do
+        expect(channel.last_message).to be_a(Chat::NullMessage)
+      end
+    end
+  end
+
   describe ".find_by_id_or_slug" do
     subject(:find_channel) { described_class.find_by_id_or_slug(channel_id) }
 

plugins/chat/spec/models/chat/deleted_chat_user_spec.rb

@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-require "rails_helper"
-
-describe Chat::DeletedUser do
-  subject(:deleted_user) { described_class.new }
-
-  describe "#username" do
-    it "returns a default username" do
-      expect(deleted_user.username).to eq(I18n.t("chat.deleted_chat_username"))
-    end
-  end
-
-  describe "#avatar_template" do
-    it "returns a default path" do
-      expect(deleted_user.avatar_template).to eq(
-        "/plugins/chat/images/deleted-chat-user-avatar.png",
-      )
-    end
-  end
-end

plugins/chat/spec/models/chat/null_message_spec.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Chat::NullMessage do
+  subject(:null_message) { described_class.new }
+
+  describe "#user" do
+    it "returns nil" do
+      expect(null_message.user).to be_nil
+    end
+  end
+
+  describe "#excerpt" do
+    it "returns nil" do
+      expect(null_message.excerpt(max_length: 1)).to be_nil
+    end
+  end
+
+  describe "#id" do
+    it "returns nil" do
+      expect(null_message.id).to be_nil
+    end
+  end
+
+  describe "#create_at" do
+    it "returns a Time object" do
+      expect(null_message.created_at).to be_a(Time)
+    end
+  end
+end

plugins/chat/spec/models/chat/null_user_spec.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Chat::NullUser do
+  subject(:null_user) { described_class.new }
+
+  describe "#username" do
+    it "returns a default username" do
+      expect(null_user.username).to eq(I18n.t("chat.deleted_chat_username"))
+    end
+  end
+
+  describe "#avatar_template" do
+    it "returns a default path" do
+      expect(null_user.avatar_template).to eq("/plugins/chat/images/deleted-chat-user-avatar.png")
+    end
+  end
+end

plugins/chat/spec/services/chat/create_message_spec.rb

@@ -143,7 +143,7 @@ RSpec.describe Chat::CreateMessage do
       end
 
       it "doesn't update channel last_message attribute" do
-        expect { result }.not_to change { channel.reload.last_message }
+        expect { result }.not_to change { channel.reload.last_message.id }
       end
 
       it "updates thread last_message attribute" do

plugins/chat/spec/system/page_objects/chat/components/message.rb

@@ -7,7 +7,7 @@ module PageObjects
         attr_reader :context
         attr_reader :component
 
-        SELECTOR = ".chat-message-container:not(.has-thread-indicator)"
+        SELECTOR = ".chat-message-container"
 
         def initialize(context)
           @context = context

plugins/chat/spec/system/thread_preview_spec.rb

@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+describe "Thread preview", type: :system do
+  fab!(:current_user) { Fabricate(:admin) }
+  fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) }
+  fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, use_service: true) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+
+  before do
+    chat_system_bootstrap
+    channel_1.add(current_user)
+    sign_in(current_user)
+  end
+
+  context "when message has no thread" do
+    it "shows no preview" do
+      chat_page.visit_channel(channel_1)
+
+      expect(channel_page.messages).to have_message(id: message_1.id)
+      expect(channel_page).to have_no_thread_indicator(message_1)
+    end
+  end
+
+  context "when message has thread with no replies" do
+    before { Fabricate(:chat_thread, channel: channel_1) }
+
+    it "shows no preview" do
+      chat_page.visit_channel(channel_1)
+
+      expect(channel_page.messages).to have_message(id: message_1.id)
+      expect(channel_page).to have_no_thread_indicator(message_1)
+    end
+  end
+
+  context "when message has thread with replies" do
+    fab!(:thread_1) { Fabricate(:chat_thread, channel: channel_1, original_message: message_1) }
+    fab!(:thread_1_message_1) do
+      Fabricate(:chat_message, thread: thread_1, in_reply_to: message_1, use_service: true)
+    end
+
+    it "shows preview" do
+      chat_page.visit_channel(channel_1)
+
+      expect(channel_page.messages).to have_message(id: message_1.id)
+      expect(channel_page).to have_thread_indicator(message_1)
+    end
+  end
+
+  context "when message has thread with deleted original message" do
+    fab!(:thread_1) { Fabricate(:chat_thread, channel: channel_1, original_message: message_1) }
+
+    before do
+      Chat::TrashMessage.call(
+        message_id: message_1.id,
+        channel_id: channel_1.id,
+        guardian: message_1.user.guardian,
+      )
+    end
+
+    it "shows preview" do
+      chat_page.visit_channel(channel_1)
+
+      expect(channel_page.messages).to have_message(id: message_1.id, deleted: 1)
+      expect(channel_page).to have_no_thread_indicator(message_1)
+    end
+  end
+end