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.
2023-09-13 10:53:48 +08:00 · 2023-09-13 10:53:48 +08:00 · 038de393ed
parent 7ab94d0ec6
commit 038de393ed
11 changed files with 97 additions and 23 deletions
--- 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}}
--- 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>
--- 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 });
  },
--- 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) {
--- 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
--- 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",
--- 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);
--- 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;
+    }
+  });
 });
--- 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)
+      );
+    }
  }
 };

--- 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 [
--- 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 });