FIX: correctly show user info (#21023)

This PR primarily fixes this case:

- USER A message
- USER B message
- USER B reply to USER A message <-- not showing user info when it should

Moreover, this commit also improves the spec to correctly test more cases.

plugins/chat/assets/javascripts/discourse/components/chat-message.js

@@ -212,27 +212,43 @@ export default class ChatMessage extends Component {
 
   get hideUserInfo() {
     const message = this.args.message;
-    const previousMessage = message?.previousMessage;
+
+    const previousMessage = message.previousMessage;
 
     if (!previousMessage) {
       return false;
     }
 
     // this is a micro optimization to avoid layout changes when we load more messages
-    if (message?.firstOfResults) {
+    if (message.firstOfResults) {
       return false;
     }
 
-    return (
-      !message?.chatWebhookEvent &&
-      (!message?.inReplyTo ||
-        message?.inReplyTo?.user?.id !== message?.user?.id) &&
-      !message?.previousMessage?.deletedAt &&
+    if (message.chatWebhookEvent) {
+      return false;
+    }
+
+    if (previousMessage.deletedAt) {
+      return false;
+    }
+
+    if (
       Math.abs(
-        new Date(message?.createdAt) - new Date(previousMessage?.createdAt)
-      ) < 300000 && // If the time between messages is over 5 minutes, break.
-      message?.user?.id === message?.previousMessage?.user?.id
-    );
+        new Date(message.createdAt) - new Date(previousMessage.createdAt)
+      ) > 300000
+    ) {
+      return false;
+    }
+
+    if (message.inReplyTo) {
+      if (message.inReplyTo?.id === previousMessage.id) {
+        return message.user?.id === previousMessage.user?.id;
+      } else {
+        return false;
+      }
+    }
+
+    return message.user?.id === previousMessage.user?.id;
   }
 
   get hideReplyToInfo() {

plugins/chat/spec/system/message_user_info.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe "Sticky date", type: :system, js: true do
+RSpec.describe "Message user info", type: :system, js: true do
   fab!(:current_user) { Fabricate(:user) }
   fab!(:channel_1) { Fabricate(:category_channel) }
 
@@ -8,53 +8,99 @@ RSpec.describe "Sticky date", type: :system, js: true do
 
   before do
     chat_system_bootstrap
+    channel_1.add(current_user)
     sign_in(current_user)
   end
 
-  context "when previous message is from a different user" do
+  context "with one message" do
     fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
-    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1) }
 
     it "shows user info on the message" do
       chat_page.visit_channel(channel_1)
 
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+    end
+  end
+
+  context "with two messages from the same user" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+    it "shows user info only on first message" do
+      chat_page.visit_channel(channel_1)
+
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_2.id}']")).to have_no_css(".chat-message-avatar")
+    end
+  end
+
+  context "with a deleted previous message" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+    it "shows user info only on second message" do
+      message_1.trash!
+      chat_page.visit_channel(channel_1)
+
       expect(page.find("[data-id='#{message_2.id}']")).to have_css(".chat-message-avatar")
     end
   end
 
-  context "when previous message is from the same user" do
-    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
-    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+  context "with messages from a webhook" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1) }
 
-    it "doesn’t show user info on the message" do
+    it "shows user info only on boths messages" do
+      Fabricate(:chat_webhook_event, chat_message: message_1)
+      Fabricate(:chat_webhook_event, chat_message: message_2)
       chat_page.visit_channel(channel_1)
 
-      expect(page.find("[data-id='#{message_2.id}']")).to have_no_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_2.id}']")).to have_css(".chat-message-avatar")
+    end
+  end
+
+  context "with large time difference between messages" do
+    fab!(:message_1) do
+      Fabricate(:chat_message, chat_channel: channel_1, user: current_user, created_at: 1.days.ago)
+    end
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+    it "shows user info on both messages" do
+      chat_page.visit_channel(channel_1)
+
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_2.id}']")).to have_css(".chat-message-avatar")
+    end
+  end
+
+  context "when replying to own previous message" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+    fab!(:message_2) do
+      Fabricate(:chat_message, in_reply_to: message_1, user: current_user, chat_channel: channel_1)
     end
 
-    context "when previous message is old" do
-      fab!(:message_1) do
-        Fabricate(
-          :chat_message,
-          chat_channel: channel_1,
-          user: current_user,
-          created_at: DateTime.parse("2018-11-10 17:00"),
-        )
-      end
-      fab!(:message_2) do
-        Fabricate(
-          :chat_message,
-          chat_channel: channel_1,
-          user: current_user,
-          created_at: DateTime.parse("2018-11-10 17:30"),
-        )
-      end
+    it "shows user info on first message only" do
+      chat_page.visit_channel(channel_1)
 
-      it "shows user info on the message" do
-        chat_page.visit_channel(channel_1)
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_2.id}']")).to have_no_css(".chat-message-avatar")
+    end
+  end
 
-        expect(page.find("[data-id='#{message_2.id}']")).to have_no_css(".chat-message-avatar")
-      end
+  context "when replying to another user previous message and previous message is yours" do
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+    fab!(:message_3) do
+      Fabricate(:chat_message, in_reply_to: message_1, user: current_user, chat_channel: channel_1)
+    end
+
+    it "shows user info on each message" do
+      chat_page.visit_channel(channel_1)
+
+      expect(page.find("[data-id='#{message_1.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_2.id}']")).to have_css(".chat-message-avatar")
+      expect(page.find("[data-id='#{message_3.id}']")).to have_css(".chat-message-avatar")
     end
   end
 end