From 038de393edd5ba721de574ed2ac1e809f85908de Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Wed, 13 Sep 2023 10:53:48 +0800
Subject: [PATCH] DEV: Raise an error in test env when I18n interpolate
 argument is missing (#23527)

Why this change?

We have been bitten by bugs where tests are not catching missing
interpolate argument in our client side code because the JavaScript
tests are also using `I18n.translate` to assert that the right message
is shown. Before this change, `I18n.interpolate` will just replace the
missing interpolation argument in the final translation with some
placeholder. As a result, we ended up comparing a broken translation
with another broken translation in the test environment.

Why does this change do?

This change introduces the `I18n.testing` property which when set to
`true` will cause `I18n.translate` to throw an error when an interpolate
argument is missing. With this commit, we also set `I18n.testing = true`
when running qunit acceptance test.
---
 .../discourse/app/components/d-lightbox.hbs   |  1 +
 .../d-lightbox/screen-reader-announcer.hbs    | 14 +++++----
 .../discourse/app/components/group-post.js    |  2 +-
 .../discourse/app/components/modal/history.js |  8 +++--
 .../components/modal/history/topic-footer.hbs | 16 +++++-----
 .../tests/fixtures/group-fixtures.js          | 20 +++++++++++++
 .../discourse/tests/helpers/qunit-helpers.js  |  4 +++
 .../discourse/tests/unit/lib/i18n-test.js     | 24 +++++++++++++++
 app/assets/javascripts/locales/i18n.js        | 29 +++++++++++++++----
 .../addon/components/notifications-filter.js  |  1 +
 .../javascripts/acceptance/mentions-test.js   |  1 +
 11 files changed, 97 insertions(+), 23 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/d-lightbox.hbs b/app/assets/javascripts/discourse/app/components/d-lightbox.hbs
index a4ab8c8b943..68a57c9806c 100644
--- a/app/assets/javascripts/discourse/app/components/d-lightbox.hbs
+++ b/app/assets/javascripts/discourse/app/components/d-lightbox.hbs
@@ -71,6 +71,7 @@
         @toggleZoom={{this.toggleZoom}}
         @zoomButtonIcon={{this.zoomButtonIcon}}
       />
+
       <DLightbox::ScreenReaderAnnouncer
         @currentItem={{this.currentItem}}
         @counterIndex={{this.counterIndex}}
diff --git a/app/assets/javascripts/discourse/app/components/d-lightbox/screen-reader-announcer.hbs b/app/assets/javascripts/discourse/app/components/d-lightbox/screen-reader-announcer.hbs
index 5220c12c72f..88871c2d5e3 100644
--- a/app/assets/javascripts/discourse/app/components/d-lightbox/screen-reader-announcer.hbs
+++ b/app/assets/javascripts/discourse/app/components/d-lightbox/screen-reader-announcer.hbs
@@ -6,11 +6,13 @@
     class="d-lightbox__screen-reader-title"
     id={{@titleElementId}}
   >
-    {{i18n
-      "experimental_lightbox.screen_reader_image_title"
-      current=@counterIndex
-      total=@totalItemCount
-      title=@currentItem.title
-    }}
+    {{#if @currentItem.title}}
+      {{i18n
+        "experimental_lightbox.screen_reader_image_title"
+        current=@counterIndex
+        total=@totalItemCount
+        title=@currentItem.title
+      }}
+    {{/if}}
   </h2>
 </div>
\ No newline at end of file
diff --git a/app/assets/javascripts/discourse/app/components/group-post.js b/app/assets/javascripts/discourse/app/components/group-post.js
index a233bc12cc0..5cbfbf211d5 100644
--- a/app/assets/javascripts/discourse/app/components/group-post.js
+++ b/app/assets/javascripts/discourse/app/components/group-post.js
@@ -43,7 +43,7 @@ export default Component.extend({
     return userPath(username.toLowerCase());
   },
 
-  @discourseComputed("post.topic.title", "post.post_number")
+  @discourseComputed("post.title", "post.post_number")
   titleAriaLabel(title, postNumber) {
     return I18n.t("groups.aria_post_number", { postNumber, title });
   },
diff --git a/app/assets/javascripts/discourse/app/components/modal/history.js b/app/assets/javascripts/discourse/app/components/modal/history.js
index c31b85bc85b..ce9470c412f 100644
--- a/app/assets/javascripts/discourse/app/components/modal/history.js
+++ b/app/assets/javascripts/discourse/app/components/modal/history.js
@@ -162,9 +162,11 @@ export default class History extends Component {
   }
 
   get revertToRevisionText() {
-    return I18n.t("post.revisions.controls.revert", {
-      revision: this.previousVersion,
-    });
+    if (this.previousVersion) {
+      return I18n.t("post.revisions.controls.revert", {
+        revision: this.previousVersion,
+      });
+    }
   }
 
   refresh(postId, postVersion) {
diff --git a/app/assets/javascripts/discourse/app/components/modal/history/topic-footer.hbs b/app/assets/javascripts/discourse/app/components/modal/history/topic-footer.hbs
index fc2f24c0675..436ae424b58 100644
--- a/app/assets/javascripts/discourse/app/components/modal/history/topic-footer.hbs
+++ b/app/assets/javascripts/discourse/app/components/modal/history/topic-footer.hbs
@@ -45,13 +45,15 @@
   {{/if}}
 
   {{#if @isStaff}}
-    <DButton
-      @action={{@revertToVersion}}
-      @icon="undo"
-      @translatedLabel={{@revertToRevisionText}}
-      class="btn-danger revert-to-version"
-      @disabled={{@loading}}
-    />
+    {{#if @revertToRevisionText}}
+      <DButton
+        @action={{@revertToVersion}}
+        @icon="undo"
+        @translatedLabel={{@revertToRevisionText}}
+        class="btn-danger revert-to-version"
+        @disabled={{@loading}}
+      />
+    {{/if}}
 
     {{#if @model.previous_hidden}}
       <DButton
diff --git a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
index ffba243c494..9bd6b38a6f6 100644
--- a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
+++ b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
@@ -269,6 +269,7 @@ export default {
       url: "/t/consistent-new-indicator/24355/1",
       user_title: "designerator",
       user_long_name: "",
+      post_number: 2,
       category: {
         id: 9,
         name: "ux",
@@ -320,6 +321,7 @@ export default {
       url: "/t/the-end-of-clown-vomit-or-simplified-category-styles/24249/63",
       user_title: "designerator",
       user_long_name: "",
+      post_number: 2,
       category: {
         id: 9,
         name: "ux",
@@ -371,6 +373,7 @@ export default {
       url: "/t/the-end-of-clown-vomit-or-simplified-category-styles/24249/62",
       user_title: "designerator",
       user_long_name: "",
+      post_number: 2,
       category: {
         id: 9,
         name: "ux",
@@ -422,6 +425,7 @@ export default {
       url: "/t/quote-reply-insertion-at-cursor-position/24344/4",
       user_title: "team",
       user_long_name: "Régis Hanol",
+      post_number: 2,
       category: {
         id: 2,
         name: "feature",
@@ -474,6 +478,7 @@ export default {
       url: "/t/quote-reply-insertion-at-cursor-position/24344/2",
       user_title: "team",
       user_long_name: "Régis Hanol",
+      post_number: 2,
       category: {
         id: 2,
         name: "feature",
@@ -526,6 +531,7 @@ export default {
       url: "/t/translations-frequently-broken/22546/27",
       user_title: "team",
       user_long_name: "Régis Hanol",
+      post_number: 2,
       category: {
         id: 27,
         name: "translations",
@@ -579,6 +585,7 @@ export default {
       url: "/t/introducing-discette-a-minimal-ember-cli-front-end-to-discourse/24321/3",
       user_title: "team",
       user_long_name: "Régis Hanol",
+      post_number: 2,
       category: {
         id: 7,
         name: "dev",
@@ -631,6 +638,7 @@ export default {
       url: "/t/after-sign-in-im-not-redirected-to-the-conversation/17753/8",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 9,
         name: "ux",
@@ -683,6 +691,7 @@ export default {
       url: "/t/dealing-with-ios-8-mobile-safari-bugs/24101/7",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 2,
         name: "feature",
@@ -735,6 +744,7 @@ export default {
       url: "/t/rss-is-not-valid/24338/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 6,
         name: "support",
@@ -787,6 +797,7 @@ export default {
       url: "/t/pasted-image-upload-size-error/24320/4",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 1,
         name: "bug",
@@ -839,6 +850,7 @@ export default {
       url: "/t/the-end-of-clown-vomit-or-simplified-category-styles/24249/57",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 9,
         name: "ux",
@@ -891,6 +903,7 @@ export default {
       url: "/t/what-is-born-mobile-born-to-touch-supposed-to-tell-me/24329/3",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 3,
         name: "meta",
@@ -943,6 +956,7 @@ export default {
       url: "/t/how-to-create-static-pages-in-discourse/24313/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 6,
         name: "support",
@@ -995,6 +1009,7 @@ export default {
       url: "/t/pasted-image-upload-size-error/24320/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 1,
         name: "bug",
@@ -1047,6 +1062,7 @@ export default {
       url: "/t/monetizing-discourse-talk/24316/4",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 6,
         name: "support",
@@ -1100,6 +1116,7 @@ export default {
       url: "/t/introducing-discette-a-minimal-ember-cli-front-end-to-discourse/24321/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 7,
         name: "dev",
@@ -1152,6 +1169,7 @@ export default {
       url: "/t/how-to-do-object-oriented-discussion-through-oneboxes/24328/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 5,
         name: "extensibility",
@@ -1204,6 +1222,7 @@ export default {
       url: "/t/update-failed-and-now-showing-currently-upgrading/24332/2",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 6,
         name: "support",
@@ -1256,6 +1275,7 @@ export default {
       url: "/t/dealing-with-ios-8-mobile-safari-bugs/24101/5",
       user_title: "co-founder",
       user_long_name: "Jeff Atwood",
+      post_number: 2,
       category: {
         id: 2,
         name: "feature",
diff --git a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js
index b0580dfe048..75975ee8821 100644
--- a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js
+++ b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js
@@ -1,3 +1,4 @@
+import I18n from "I18n";
 import QUnit, { module, skip, test } from "qunit";
 import { cloneJSON, deepMerge } from "discourse-common/lib/object";
 import MessageBus from "message-bus-client";
@@ -322,6 +323,8 @@ export function acceptance(name, optionsOrCallback) {
 
   const setup = {
     beforeEach() {
+      I18n.testing = true;
+
       resetMobile();
 
       resetExtraClasses();
@@ -359,6 +362,7 @@ export function acceptance(name, optionsOrCallback) {
     },
 
     afterEach() {
+      I18n.testing = false;
       resetMobile();
       let app = getApplication();
       options?.afterEach?.call(this);
diff --git a/app/assets/javascripts/discourse/tests/unit/lib/i18n-test.js b/app/assets/javascripts/discourse/tests/unit/lib/i18n-test.js
index 2b739eaf9c1..302967af854 100644
--- a/app/assets/javascripts/discourse/tests/unit/lib/i18n-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/lib/i18n-test.js
@@ -66,6 +66,7 @@ module("Unit | Utility | i18n", function (hooks) {
             other: "%{count} days",
           },
           dollar_sign: "Hi {{description}}",
+          with_multiple_interpolate_arguments: "Hi %{username}, %{username2}",
         },
       },
     };
@@ -296,4 +297,27 @@ module("Unit | Utility | i18n", function (hooks) {
       assert.strictEqual(myI18n.t("topic.reply.title"), "Répondre");
     });
   });
+
+  test("missing interpolation argument does not throw error when I18n.testing is `false`", function (assert) {
+    assert.strictEqual(
+      I18n.t("with_multiple_interpolate_arguments", { username: "username" }),
+      "Hi username, [missing %{username2} value]"
+    );
+  });
+
+  test("missing interpolation argument throws error when I18n.testing is true", function (assert) {
+    try {
+      I18n.testing = true;
+
+      assert.throws(function () {
+        I18n.t("with_multiple_interpolate_arguments", {
+          username: "username",
+        });
+      }, new I18n.missingInterpolationArgument(
+        "with_multiple_interpolate_arguments: [missing %{username2} value]"
+      ));
+    } finally {
+      I18n.testing = false;
+    }
+  });
 });
diff --git a/app/assets/javascripts/locales/i18n.js b/app/assets/javascripts/locales/i18n.js
index 686363acb5e..1b77f23f8d5 100644
--- a/app/assets/javascripts/locales/i18n.js
+++ b/app/assets/javascripts/locales/i18n.js
@@ -4,6 +4,15 @@ var I18n = I18n || {};
 // Set default locale to english
 I18n.defaultLocale = "en";
 
+I18n.testing = false;
+
+I18n.missingInterpolationArgument = class I18nMissingInterpolationArgument extends Error {
+  constructor(message) {
+    super(message);
+    this.name = "I18nMissingInterpolationArgument";
+  }
+}
+
 // Set default pluralization rule
 I18n.pluralizationRules = {
   en(n) {
@@ -101,7 +110,7 @@ I18n.prepareOptions = function () {
   return options;
 };
 
-I18n.interpolate = function (message, options) {
+I18n.interpolate = function (message, options, scope) {
   options = this.prepareOptions(options);
 
   var matches = message.match(this.PLACEHOLDER),
@@ -128,6 +137,10 @@ I18n.interpolate = function (message, options) {
 
     if (!this.isValidNode(options, name)) {
       value = "[missing " + placeholder + " value]";
+
+      if (I18n.testing) {
+        throw new I18n.missingInterpolationArgument(`${scope}: ${value}`);
+      }
     }
 
     var regex = new RegExp(
@@ -166,12 +179,16 @@ I18n.translate = function (scope, options) {
   }
 
   try {
-    return this.interpolate(translation, options);
+    return this.interpolate(translation, options, scope);
   } catch (error) {
-    return (
-      options.translatedFallback ||
-      this.missingTranslation(scope, null, options)
-    );
+    if (error instanceof I18n.missingInterpolationArgument) {
+      throw error;
+    } else {
+      return (
+        options.translatedFallback ||
+        this.missingTranslation(scope, null, options)
+      );
+    }
   }
 };
 
diff --git a/app/assets/javascripts/select-kit/addon/components/notifications-filter.js b/app/assets/javascripts/select-kit/addon/components/notifications-filter.js
index 08b05789be7..de17b6c43e1 100644
--- a/app/assets/javascripts/select-kit/addon/components/notifications-filter.js
+++ b/app/assets/javascripts/select-kit/addon/components/notifications-filter.js
@@ -4,6 +4,7 @@ import { computed } from "@ember/object";
 
 export default DropdownSelectBoxComponent.extend({
   classNames: ["notifications-filter"],
+  nameProperty: "label",
 
   content: computed(function () {
     return [
diff --git a/plugins/chat/test/javascripts/acceptance/mentions-test.js b/plugins/chat/test/javascripts/acceptance/mentions-test.js
index 02a51030122..80e215478c7 100644
--- a/plugins/chat/test/javascripts/acceptance/mentions-test.js
+++ b/plugins/chat/test/javascripts/acceptance/mentions-test.js
@@ -17,6 +17,7 @@ acceptance("Chat | Mentions", function (needs) {
     current_user_membership: { following: true },
     allow_channel_wide_mentions: false,
     chatable: { id: 1 },
+    title: "Some title",
   };
 
   needs.settings({ chat_enabled: true });