From 769388b8ba234c800b9c61f8ea6c2b8163dd3d06 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Thu, 16 Dec 2021 21:53:50 +0100 Subject: [PATCH] FIX: Translation overrides from fallback locale didn't work on client Discourse sent only translation overrides for the current language to the client instead of sending overrides from fallback locales as well. This especially impacted en_GB -> en since most overrides would be done in English instead of English (UK). This also adds lots of tests for previously untested code. There's a small caveat: The client currently doesn't handle fallback locales for MessageFormat strings. That is why overrides for those strings always have a higher priority than regular translations. So, as an example, the lookup order for MessageFormat strings in German is: 1. override for de 2. override for en 3. value from de 4. value from en --- .../app/initializers/localization.js | 43 +++--- .../discourse/tests/unit/localization-test.js | 140 ++++++++++++++++-- .../admin/site_texts_controller.rb | 2 +- lib/js_locale_helper.rb | 53 ++++--- .../admin/site_texts_controller_spec.rb | 13 ++ .../requests/extra_locales_controller_spec.rb | 37 ++++- 6 files changed, 223 insertions(+), 65 deletions(-) diff --git a/app/assets/javascripts/discourse/app/initializers/localization.js b/app/assets/javascripts/discourse/app/initializers/localization.js index 0c0657075c3..25d14c7864e 100644 --- a/app/assets/javascripts/discourse/app/initializers/localization.js +++ b/app/assets/javascripts/discourse/app/initializers/localization.js @@ -24,35 +24,28 @@ export default { } // Merge any overrides into our object - const overrides = I18n._overrides || {}; - Object.keys(overrides).forEach((k) => { - const v = overrides[k]; - k = k.replace("admin_js", "js"); + for (const [locale, overrides] of Object.entries(I18n._overrides || {})) { + for (const [key, value] of Object.entries(overrides)) { + const segs = key.replace(/^admin_js\./, "js.admin.").split("."); + let node = I18n.translations[locale] || {}; - const segs = k.split("."); - - let node = I18n.translations[I18n.locale] || {}; - let i = 0; - - for (; i < segs.length - 1; i++) { - if (!(segs[i] in node)) { - node[segs[i]] = {}; + for (let i = 0; i < segs.length - 1; i++) { + if (!(segs[i] in node)) { + node[segs[i]] = {}; + } + node = node[segs[i]]; + } + + if (typeof node === "object") { + node[segs[segs.length - 1]] = value; } - node = node[segs[i]]; } + } - if (typeof node === "object") { - node[segs[segs.length - 1]] = v; - } - }); - - const mfOverrides = I18n._mfOverrides || {}; - Object.keys(mfOverrides).forEach((k) => { - const v = mfOverrides[k]; - - k = k.replace(/^[a-z_]*js\./, ""); - I18n._compiledMFs[k] = v; - }); + for (let [key, value] of Object.entries(I18n._mfOverrides || {})) { + key = key.replace(/^[a-z_]*js\./, ""); + I18n._compiledMFs[key] = value; + } bootbox.addLocale(I18n.currentLocale(), { OK: I18n.t("composer.modal_ok"), diff --git a/app/assets/javascripts/discourse/tests/unit/localization-test.js b/app/assets/javascripts/discourse/tests/unit/localization-test.js index 6d8f303bbef..0c2cf9ba80e 100644 --- a/app/assets/javascripts/discourse/tests/unit/localization-test.js +++ b/app/assets/javascripts/discourse/tests/unit/localization-test.js @@ -6,7 +6,10 @@ import { getApplication } from "@ember/test-helpers"; module("initializer:localization", { _locale: I18n.locale, _translations: I18n.translations, + _extras: I18n.extras, + _compiledMFs: I18n._compiledMFs, _overrides: I18n._overrides, + _mfOverrides: I18n._mfOverrides, beforeEach() { I18n.locale = "fr"; @@ -15,16 +18,43 @@ module("initializer:localization", { fr: { js: { composer: { - reply: "Répondre", + both_languages1: "composer.both_languages1 (FR)", + both_languages2: "composer.both_languages2 (FR)", }, }, }, en: { js: { - topic: { - reply: { - help: "begin composing a reply to this topic", - }, + composer: { + both_languages1: "composer.both_languages1 (EN)", + both_languages2: "composer.both_languages2 (EN)", + only_english1: "composer.only_english1 (EN)", + only_english2: "composer.only_english2 (EN)", + }, + }, + }, + }; + + I18n._compiledMFs = { + "user.messages.some_key_MF": () => "user.messages.some_key_MF (FR)", + }; + + I18n.extras = { + fr: { + admin: { + api: { + both_languages1: "admin.api.both_languages1 (FR)", + both_languages2: "admin.api.both_languages2 (FR)", + }, + }, + }, + en: { + admin: { + api: { + both_languages1: "admin.api.both_languages1 (EN)", + both_languages2: "admin.api.both_languages2 (EN)", + only_english1: "admin.api.only_english1 (EN)", + only_english2: "admin.api.only_english2 (EN)", }, }, }, @@ -34,35 +64,115 @@ module("initializer:localization", { afterEach() { I18n.locale = this._locale; I18n.translations = this._translations; + I18n.extras = this._extras; + I18n._compiledMFs = this._compiledMFs; I18n._overrides = this._overrides; + I18n._mfOverrides = this._mfOverrides; }, }); test("translation overrides", function (assert) { I18n._overrides = { - "js.composer.reply": "WAT", - "js.topic.reply.help": "foobar", + fr: { + "js.composer.both_languages1": "composer.both_languages1 (FR override)", + "js.composer.only_english2": "composer.only_english2 (FR override)", + }, + en: { + "js.composer.both_languages2": "composer.both_languages2 (EN override)", + "js.composer.only_english1": "composer.only_english1 (EN override)", + }, }; LocalizationInitializer.initialize(getApplication()); assert.strictEqual( - I18n.t("composer.reply"), - "WAT", + I18n.t("composer.both_languages1"), + "composer.both_languages1 (FR override)", "overrides existing translation in current locale" ); + assert.strictEqual( - I18n.t("topic.reply.help"), - "foobar", - "overrides translation in default locale" + I18n.t("composer.only_english1"), + "composer.only_english1 (EN override)", + "overrides translation in fallback locale" + ); + + assert.strictEqual( + I18n.t("composer.only_english2"), + "composer.only_english2 (FR override)", + "overrides translation that doesn't exist in current locale" + ); + + assert.strictEqual( + I18n.t("composer.both_languages2"), + "composer.both_languages2 (FR)", + "prefers translation in current locale over override in fallback locale" + ); +}); + +test("translation overrides (admin_js)", function (assert) { + I18n._overrides = { + fr: { + "admin_js.api.both_languages1": "admin.api.both_languages1 (FR override)", + "admin_js.api.only_english2": "admin.api.only_english2 (FR override)", + }, + en: { + "admin_js.api.both_languages2": "admin.api.both_languages2 (EN override)", + "admin_js.api.only_english1": "admin.api.only_english1 (EN override)", + }, + }; + LocalizationInitializer.initialize(getApplication()); + + assert.strictEqual( + I18n.t("admin.api.both_languages1"), + "admin.api.both_languages1 (FR override)", + "overrides existing translation in current locale" + ); + + assert.strictEqual( + I18n.t("admin.api.only_english1"), + "admin.api.only_english1 (EN override)", + "overrides translation in fallback locale" + ); + + assert.strictEqual( + I18n.t("admin.api.only_english2"), + "admin.api.only_english2 (FR override)", + "overrides translation that doesn't exist in current locale" + ); + + assert.strictEqual( + I18n.t("admin.api.both_languages2"), + "admin.api.both_languages2 (FR)", + "prefers translation in current locale over override in fallback locale" + ); +}); + +test("translation overrides for MessageFormat strings", function (assert) { + I18n._mfOverrides = { + "js.user.messages.some_key_MF": () => + "user.messages.some_key_MF (FR override)", + }; + + LocalizationInitializer.initialize(getApplication()); + + assert.strictEqual( + I18n.messageFormat("user.messages.some_key_MF", {}), + "user.messages.some_key_MF (FR override)", + "overrides existing MessageFormat string" ); }); test("skip translation override if parent node is not an object", function (assert) { I18n._overrides = { - "js.composer.reply": "WAT", - "js.composer.reply.help": "foobar", + fr: { + "js.composer.both_languages1.foo": + "composer.both_languages1.foo (FR override)", + }, }; LocalizationInitializer.initialize(getApplication()); - assert.strictEqual(I18n.t("composer.reply.help"), "[fr.composer.reply.help]"); + assert.strictEqual( + I18n.t("composer.both_languages1.foo"), + "[fr.composer.both_languages1.foo]" + ); }); diff --git a/app/controllers/admin/site_texts_controller.rb b/app/controllers/admin/site_texts_controller.rb index 55e3611f1dd..a3ed9f7d607 100644 --- a/app/controllers/admin/site_texts_controller.rb +++ b/app/controllers/admin/site_texts_controller.rb @@ -183,7 +183,7 @@ class Admin::SiteTextsController < Admin::AdminController def find_translations(query, overridden, locale) translations = Hash.new { |hash, key| hash[key] = {} } search_results = I18n.with_locale(locale) do - I18n.search(query, overridden: overridden) + I18n.search(query, only_overridden: overridden) end search_results.each do |key, value| diff --git a/lib/js_locale_helper.rb b/lib/js_locale_helper.rb index e12fab50150..4ca3cd5864a 100644 --- a/lib/js_locale_helper.rb +++ b/lib/js_locale_helper.rb @@ -160,31 +160,44 @@ module JsLocaleHelper result end - def self.output_client_overrides(locale) - overrides = TranslationOverride - .where(locale: locale) - .where("translation_key LIKE 'js.%' OR translation_key LIKE 'admin_js.%'") - .pluck(:translation_key, :value, :compiled_js) + def self.output_client_overrides(main_locale) + all_overrides = {} + has_overrides = false - return "" if overrides.blank? + I18n.fallbacks[main_locale].each do |locale| + overrides = all_overrides[locale] = TranslationOverride + .where(locale: locale) + .where("translation_key LIKE 'js.%' OR translation_key LIKE 'admin_js.%'") + .pluck(:translation_key, :value, :compiled_js) - message_formats = [] - translations = {} - - overrides.each do |key, value, compiled_js| - if key.end_with?("_MF") - message_formats << "#{key.inspect}: #{compiled_js}" - else - translations[key] = value - end + has_overrides ||= overrides.present? end - message_formats = message_formats.join(", ") + return "" if !has_overrides - <<~JS - I18n._mfOverrides = {#{message_formats}}; - I18n._overrides = #{translations.to_json}; - JS + result = +"I18n._overrides = {};" + existing_keys = Set.new + message_formats = [] + + all_overrides.each do |locale, overrides| + translations = {} + + overrides.each do |key, value, compiled_js| + next if existing_keys.include?(key) + existing_keys << key + + if key.end_with?("_MF") + message_formats << "#{key.inspect}: #{compiled_js}" + else + translations[key] = value + end + end + + result << "I18n._overrides['#{locale}'] = #{translations.to_json};" if translations.present? + end + + result << "I18n._mfOverrides = {#{message_formats.join(", ")}};" + result end def self.output_extra_locales(bundle, locale) diff --git a/spec/requests/admin/site_texts_controller_spec.rb b/spec/requests/admin/site_texts_controller_spec.rb index 90a0bd546f1..01ccc442d0c 100644 --- a/spec/requests/admin/site_texts_controller_spec.rb +++ b/spec/requests/admin/site_texts_controller_spec.rb @@ -153,6 +153,19 @@ RSpec.describe Admin::SiteTextsController do expect(value).to eq('education.new-topic override') end + it "returns only overridden translations" do + TranslationOverride.upsert!(:en, 'education.new-topic', 'education.new-topic override') + + get "/admin/customize/site_texts.json", params: { locale: 'en', overridden: true } + expect(response.status).to eq(200) + + site_texts = response.parsed_body['site_texts'] + expect(site_texts.size).to eq(1) + + value = site_texts.find { |text| text['id'] == 'education.new-topic' }['value'] + expect(value).to eq('education.new-topic override') + end + context 'plural keys' do before do I18n.backend.store_translations(:en, colour: { one: '%{count} colour', other: '%{count} colours' }) diff --git a/spec/requests/extra_locales_controller_spec.rb b/spec/requests/extra_locales_controller_spec.rb index 4fad83de519..371e2c4f6bb 100644 --- a/spec/requests/extra_locales_controller_spec.rb +++ b/spec/requests/extra_locales_controller_spec.rb @@ -109,10 +109,39 @@ describe ExtraLocalesController do ctx.eval("I18n = {};") ctx.eval(response.body) - expect(ctx.eval('typeof I18n._mfOverrides["js.client_MF"]')).to eq("function") - expect(ctx.eval('I18n._overrides["js.some_key"]')).to eq("client-side translation") - expect(ctx.eval('I18n._overrides["js.client_MF"] === undefined')).to eq(true) - expect(ctx.eval('I18n._overrides["admin_js.another_key"]')).to eq("admin client js") + expect(ctx.eval("typeof I18n._mfOverrides['js.client_MF']")).to eq("function") + expect(ctx.eval("I18n._overrides['#{I18n.locale}']['js.some_key']")).to eq("client-side translation") + expect(ctx.eval("I18n._overrides['#{I18n.locale}']['js.client_MF'] === undefined")).to eq(true) + expect(ctx.eval("I18n._overrides['#{I18n.locale}']['admin_js.another_key']")).to eq("admin client js") + end + + it "returns overrides from fallback locale" do + TranslationOverride.upsert!(:en, 'js.some_key', 'some key (en)') + TranslationOverride.upsert!(:fr, 'js.some_key', 'some key (fr)') + TranslationOverride.upsert!(:en, 'js.only_en', 'only English') + TranslationOverride.upsert!(:fr, 'js.only_fr', 'only French') + TranslationOverride.upsert!(:en, 'js.some_client_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + TranslationOverride.upsert!(:fr, 'js.some_client_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + TranslationOverride.upsert!(:en, 'js.only_en_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + TranslationOverride.upsert!(:fr, 'js.only_fr_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }') + + SiteSetting.allow_user_locale = true + user = Fabricate(:user, locale: :fr) + sign_in(user) + + get "/extra-locales/overrides" + expect(response.status).to eq(200) + + ctx = MiniRacer::Context.new + ctx.eval("I18n = {};") + ctx.eval(response.body) + + overrides = ctx.eval("I18n._overrides") + expect(overrides.keys).to contain_exactly("en", "fr") + expect(overrides["en"]).to eq({ 'js.only_en' => 'only English' }) + expect(overrides["fr"]).to eq({ 'js.some_key' => 'some key (fr)', 'js.only_fr' => 'only French' }) + + expect(ctx.eval("Object.keys(I18n._mfOverrides)")).to contain_exactly("js.some_client_MF", "js.only_en_MF", "js.only_fr_MF") end end end