From 311b28d485c39bd01f16a706ca14b757c25c274c Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 25 Aug 2023 15:20:56 +0200 Subject: [PATCH] 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 --- .../components/chat-message-reaction.js | 91 +------- .../javascripts/discourse/lib/fabricators.js | 4 +- .../discourse/lib/get-reaction-text.js | 87 ++++++++ .../unit/lib/get-reaction-text-test.js | 198 ++++++++++++++++++ 4 files changed, 289 insertions(+), 91 deletions(-) create mode 100644 plugins/chat/assets/javascripts/discourse/lib/get-reaction-text.js create mode 100644 plugins/chat/test/javascripts/unit/lib/get-reaction-text-test.js diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-reaction.js b/plugins/chat/assets/javascripts/discourse/components/chat-message-reaction.js index ef5b1bac146..1511eec04a1 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-reaction.js +++ b/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)); } } diff --git a/plugins/chat/assets/javascripts/discourse/lib/fabricators.js b/plugins/chat/assets/javascripts/discourse/lib/fabricators.js index 7c360f020d6..2bf8cac53dd 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/fabricators.js +++ b/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, }); } diff --git a/plugins/chat/assets/javascripts/discourse/lib/get-reaction-text.js b/plugins/chat/assets/javascripts/discourse/lib/get-reaction-text.js new file mode 100644 index 00000000000..eb5b5383ac2 --- /dev/null +++ b/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); + } +} diff --git a/plugins/chat/test/javascripts/unit/lib/get-reaction-text-test.js b/plugins/chat/test/javascripts/unit/lib/get-reaction-text-test.js new file mode 100644 index 00000000000..bda19f03fc7 --- /dev/null +++ b/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:" + ); + }); +});