FIX: incorrect chat message reaction text (#23260)

Prior to this fix the text would be incorrect when the current user reacted and number of reactions was above 2.

This commit fixes the bug and also makes the following changes:
- separates text computation in a standalone lib to make it easier to test
- increases the number of displayed usernames in reaction text (from 5 to 15)
- adds a full test suite for this new `getReactionText` function
- fixes a bug in reaction fabricator which would prevent to change the count to zero

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

@@ -1,12 +1,12 @@
 import Component from "@glimmer/component";
 import { action } from "@ember/object";
 import { emojiUnescape, emojiUrlFor } from "discourse/lib/text";
-import I18n from "I18n";
 import { cancel } from "@ember/runloop";
 import { inject as service } from "@ember/service";
 import setupPopover from "discourse/lib/d-popover";
 import discourseLater from "discourse-common/lib/later";
 import { tracked } from "@glimmer/tracking";
+import { getReactionText } from "discourse/plugins/chat/discourse/lib/get-reaction-text";
 
 export default class ChatMessageReaction extends Component {
   @service capabilities;
@@ -151,93 +151,6 @@ export default class ChatMessageReaction extends Component {
       return;
     }
 
-    return emojiUnescape(
-      this.args.reaction.reacted
-        ? this.#reactionTextWithSelf
-        : this.#reactionText
-    );
-  }
-
-  get #reactionTextWithSelf() {
-    const reactionCount = this.args.reaction.count;
-
-    if (reactionCount === 0) {
-      return;
-    }
-
-    if (reactionCount === 1) {
-      return I18n.t("chat.reactions.only_you", {
-        emoji: this.args.reaction.emoji,
-      });
-    }
-
-    const maxUsernames = 5;
-    const usernames = this.args.reaction.users
-      .filter((user) => user.id !== this.currentUser?.id)
-      .slice(0, maxUsernames)
-      .mapBy("username");
-
-    if (reactionCount === 2) {
-      return I18n.t("chat.reactions.you_and_single_user", {
-        emoji: this.args.reaction.emoji,
-        username: usernames.pop(),
-      });
-    }
-
-    const unnamedUserCount = reactionCount - usernames.length;
-    if (unnamedUserCount > 0) {
-      return I18n.t("chat.reactions.you_multiple_users_and_more", {
-        emoji: this.args.reaction.emoji,
-        commaSeparatedUsernames: this.#joinUsernames(usernames),
-        count: unnamedUserCount,
-      });
-    }
-
-    return I18n.t("chat.reactions.you_and_multiple_users", {
-      emoji: this.args.reaction.emoji,
-      username: usernames.pop(),
-      commaSeparatedUsernames: this.#joinUsernames(usernames),
-    });
-  }
-
-  get #reactionText() {
-    const reactionCount = this.args.reaction.count;
-
-    if (reactionCount === 0) {
-      return;
-    }
-
-    const maxUsernames = 5;
-    const usernames = this.args.reaction.users
-      .filter((user) => user.id !== this.currentUser?.id)
-      .slice(0, maxUsernames)
-      .mapBy("username");
-
-    if (reactionCount === 1) {
-      return I18n.t("chat.reactions.single_user", {
-        emoji: this.args.reaction.emoji,
-        username: usernames.pop(),
-      });
-    }
-
-    const unnamedUserCount = reactionCount - usernames.length;
-
-    if (unnamedUserCount > 0) {
-      return I18n.t("chat.reactions.multiple_users_and_more", {
-        emoji: this.args.reaction.emoji,
-        commaSeparatedUsernames: this.#joinUsernames(usernames),
-        count: unnamedUserCount,
-      });
-    }
-
-    return I18n.t("chat.reactions.multiple_users", {
-      emoji: this.args.reaction.emoji,
-      username: usernames.pop(),
-      commaSeparatedUsernames: this.#joinUsernames(usernames),
-    });
-  }
-
-  #joinUsernames(usernames) {
-    return usernames.join(I18n.t("word_connector.comma"));
+    return emojiUnescape(getReactionText(this.args.reaction, this.currentUser));
   }
 }

plugins/chat/assets/javascripts/discourse/lib/fabricators.js

@@ -142,10 +142,10 @@ function threadPreviewFabricator(args = {}) {
 
 function reactionFabricator(args = {}) {
   return ChatMessageReaction.create({
-    count: args.count || 1,
+    count: args.count ?? 1,
     users: args.users || [userFabricator()],
     emoji: args.emoji || "heart",
-    reacted: args.reacted || false,
+    reacted: args.reacted ?? false,
   });
 }
 

plugins/chat/assets/javascripts/discourse/lib/get-reaction-text.js

@@ -0,0 +1,87 @@
+import I18n from "I18n";
+
+export const MAX_DISPLAYED_USERNAMES = 15;
+
+function filterUsernames(users, currentUser) {
+  return users
+    .filter((user) => user.id !== currentUser?.id)
+    .slice(0, MAX_DISPLAYED_USERNAMES)
+    .mapBy("username");
+}
+
+function reactionIncludingCurrentUser(reaction, currentUser) {
+  if (reaction.count === 1) {
+    return I18n.t("chat.reactions.only_you", {
+      emoji: reaction.emoji,
+    });
+  }
+
+  const usernames = filterUsernames(reaction.users, currentUser);
+
+  if (reaction.count === 2) {
+    return I18n.t("chat.reactions.you_and_single_user", {
+      emoji: reaction.emoji,
+      username: usernames.pop(),
+    });
+  }
+
+  // - 1 for "you"
+  const unnamedUserCount = reaction.count - usernames.length - 1;
+
+  if (unnamedUserCount > 0) {
+    return I18n.t("chat.reactions.you_multiple_users_and_more", {
+      emoji: reaction.emoji,
+      commaSeparatedUsernames: joinUsernames(usernames),
+      count: unnamedUserCount,
+    });
+  }
+
+  return I18n.t("chat.reactions.you_and_multiple_users", {
+    emoji: reaction.emoji,
+    username: usernames.pop(),
+    commaSeparatedUsernames: joinUsernames(usernames),
+  });
+}
+
+function reactionText(reaction, currentUser) {
+  const usernames = filterUsernames(reaction.users, currentUser);
+
+  if (reaction.count === 1) {
+    return I18n.t("chat.reactions.single_user", {
+      emoji: reaction.emoji,
+      username: usernames.pop(),
+    });
+  }
+
+  const unnamedUserCount = reaction.count - usernames.length;
+
+  if (unnamedUserCount > 0) {
+    return I18n.t("chat.reactions.multiple_users_and_more", {
+      emoji: reaction.emoji,
+      commaSeparatedUsernames: joinUsernames(usernames),
+      count: unnamedUserCount,
+    });
+  }
+
+  return I18n.t("chat.reactions.multiple_users", {
+    emoji: reaction.emoji,
+    username: usernames.pop(),
+    commaSeparatedUsernames: joinUsernames(usernames),
+  });
+}
+
+function joinUsernames(usernames) {
+  return usernames.join(I18n.t("word_connector.comma"));
+}
+
+export function getReactionText(reaction, currentUser) {
+  if (reaction.count === 0) {
+    return;
+  }
+
+  if (reaction.reacted) {
+    return reactionIncludingCurrentUser(reaction, currentUser);
+  } else {
+    return reactionText(reaction, currentUser);
+  }
+}

plugins/chat/test/javascripts/unit/lib/get-reaction-text-test.js

@@ -0,0 +1,198 @@
+import { module, test } from "qunit";
+import {
+  MAX_DISPLAYED_USERNAMES,
+  getReactionText,
+} from "discourse/plugins/chat/discourse/lib/get-reaction-text";
+import fabricators from "discourse/plugins/chat/discourse/lib/fabricators";
+
+module("Discourse Chat | Unit | get-reaction-text", function () {
+  test("no reaction ", function (assert) {
+    const reaction = fabricators.reaction({ count: 0, users: [] });
+    const currentUser = fabricators.user();
+
+    assert.strictEqual(getReactionText(reaction, currentUser), undefined);
+  });
+
+  test("current user reacted - one reaction", function (assert) {
+    const currentUser = fabricators.user();
+    const reaction = fabricators.reaction({
+      count: 1,
+      users: [currentUser],
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - two reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const secondUser = fabricators.user({ username: "martin" });
+    const reaction = fabricators.reaction({
+      count: 2,
+      users: [currentUser, secondUser],
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You and martin reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - more than display limit reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const otherUsers = Array.from(Array(MAX_DISPLAYED_USERNAMES + 1)).map(
+      (_, i) => fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: [currentUser].concat(otherUsers).length,
+      users: [currentUser].concat(otherUsers),
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You, user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11, user12, user13, user14 and 1 other reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - less or equal than display limit reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const otherUsers = Array.from(Array(MAX_DISPLAYED_USERNAMES - 2)).map(
+      (_, i) => fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: [currentUser].concat(otherUsers).length,
+      users: [currentUser].concat(otherUsers),
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You, user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11 and user12 reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - one reaction", function (assert) {
+    const currentUser = fabricators.user();
+    const reaction = fabricators.reaction({
+      count: 1,
+      users: [currentUser],
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - two reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const secondUser = fabricators.user({ username: "martin" });
+    const reaction = fabricators.reaction({
+      count: 2,
+      users: [currentUser, secondUser],
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You and martin reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - more than display limit reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const otherUsers = Array.from(Array(MAX_DISPLAYED_USERNAMES + 1)).map(
+      (_, i) => fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: [currentUser].concat(otherUsers).length,
+      users: [currentUser].concat(otherUsers),
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You, user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11, user12, user13, user14 and 1 other reacted with :heart:"
+    );
+  });
+
+  test("current user reacted - less or equal than display limit reactions", function (assert) {
+    const currentUser = fabricators.user();
+    const otherUsers = Array.from(Array(MAX_DISPLAYED_USERNAMES - 2)).map(
+      (_, i) => fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: [currentUser].concat(otherUsers).length,
+      users: [currentUser].concat(otherUsers),
+      reacted: true,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, currentUser),
+      "You, user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11 and user12 reacted with :heart:"
+    );
+  });
+
+  test("current user didn't react - one reaction", function (assert) {
+    const user = fabricators.user({ username: "martin" });
+    const reaction = fabricators.reaction({
+      count: 1,
+      users: [user],
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, fabricators.user()),
+      "martin reacted with :heart:"
+    );
+  });
+
+  test("current user didn't react - two reactions", function (assert) {
+    const firstUser = fabricators.user({ username: "claude" });
+    const secondUser = fabricators.user({ username: "martin" });
+    const reaction = fabricators.reaction({
+      count: 2,
+      users: [firstUser, secondUser],
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, fabricators.user()),
+      "claude and martin reacted with :heart:"
+    );
+  });
+
+  test("current user didn't react - more than display limit reactions", function (assert) {
+    const users = Array.from(Array(MAX_DISPLAYED_USERNAMES + 1)).map((_, i) =>
+      fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: users.length,
+      users,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, fabricators.user()),
+      "user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11, user12, user13, user14 and 1 other reacted with :heart:"
+    );
+  });
+
+  test("current user didn't react - less or equal than display limit reactions", function (assert) {
+    const users = Array.from(Array(MAX_DISPLAYED_USERNAMES - 1)).map((_, i) =>
+      fabricators.user({ username: "user" + i })
+    );
+    const reaction = fabricators.reaction({
+      count: users.length,
+      users,
+    });
+
+    assert.strictEqual(
+      getReactionText(reaction, fabricators.user()),
+      "user0, user1, user2, user3, user4, user5, user6, user7, user8, user9, user10, user11, user12 and user13 reacted with :heart:"
+    );
+  });
+});