From 281570226b1d6a2ef681e47e2aebbf955ae05b65 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Fri, 4 Oct 2024 15:21:58 +0200
Subject: [PATCH] DEV: Output failing MF keys when compilation fails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently, when the MessageFormat compiler fails on some translations,
we just have the raw output from the compiler in the logs and that’s not
always very helpful.

Now, when there is an error, we iterate over the translation keys and
try to compile them one by one. When we detect one that is failing, it’s
added to a list that is now outputted in the logs. That way, it’s easier
to know which keys are not properly translated, and the problems can be
addressed quicker.

---

The previous implementation of this patch had a bug: it wasn’t handling
locales with country/region code properly. So instead of iterating over
the problematic keys, it was raising an error.
---
 lib/js_locale_helper.rb           | 20 ++++++++++++++++----
 spec/lib/js_locale_helper_spec.rb | 23 ++++++++++++++++-------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/lib/js_locale_helper.rb b/lib/js_locale_helper.rb
index 8b396707e1f..54fed140228 100644
--- a/lib/js_locale_helper.rb
+++ b/lib/js_locale_helper.rb
@@ -139,9 +139,9 @@ module JsLocaleHelper
 
     message_formats =
       I18n.fallbacks[locale]
-        .each_with_object({}) do |l, hash|
+        .each_with_object(HashWithIndifferentAccess.new) do |l, hash|
           translations = translations_for(l, no_fallback: true)
-          hash[l.to_s.dasherize] = remove_message_formats!(translations, l).merge(
+          hash[l] = remove_message_formats!(translations, l).merge(
             TranslationOverride
               .mf_locales(l)
               .pluck(:translation_key, :value)
@@ -150,7 +150,8 @@ module JsLocaleHelper
           )
         end
         .compact_blank
-    compiled = MessageFormat.compile(message_formats.keys, message_formats, strict: false)
+    js_message_formats = message_formats.transform_keys(&:dasherize)
+    compiled = MessageFormat.compile(js_message_formats.keys, js_message_formats, strict: false)
     transpiled = DiscourseJsProcessor.transpile(<<~JS, "", "discourse-mf")
       import Messages from '@messageformat/runtime/messages';
       #{compiled.sub("export default", "const msgData =")};
@@ -163,7 +164,18 @@ module JsLocaleHelper
       require("discourse-mf");
     JS
   rescue => e
-    Rails.logger.error("Failed to compile message formats for #{locale} '#{e}'")
+    js_locale = locale.to_s.dasherize
+    message_formats[locale]
+      .filter_map do |key, value|
+        next if MessageFormat.compile(js_locale, value, strict: false)
+      rescue StandardError
+        key
+      end
+      .then do |strings|
+        Rails.logger.error(
+          "Failed to compile message formats for #{locale}.\n\nBroken strings are: #{strings.join(", ")}\n\nError: #{e}",
+        )
+      end
     <<~JS
       console.error("Failed to compile message formats for #{locale}. Some translation strings will be missing.");
     JS
diff --git a/spec/lib/js_locale_helper_spec.rb b/spec/lib/js_locale_helper_spec.rb
index 55b8e908be0..5aa6a27d09c 100644
--- a/spec/lib/js_locale_helper_spec.rb
+++ b/spec/lib/js_locale_helper_spec.rb
@@ -161,25 +161,29 @@ RSpec.describe JsLocaleHelper do
     fab!(:overriden_translation_ja) do
       Fabricate(:translation_override, locale: "ja", translation_key: "js.posts_likes_MF")
     end
-    fab!(:overriden_translation_he) do
-      Fabricate(:translation_override, locale: "he", translation_key: "js.posts_likes_MF")
+    fab!(:overriden_translation_zh_tw) do
+      Fabricate(:translation_override, locale: "zh_TW", translation_key: "js.posts_likes_MF")
     end
     let(:output) { described_class.output_MF(locale).gsub(/^import.*$/, "") }
     let(:generated_locales) { v8_ctx.eval("Object.keys(I18n._mfMessages._data)") }
     let(:translated_message) do
       v8_ctx.eval("I18n._mfMessages.get('posts_likes_MF', {count: 3, ratio: 'med'})")
     end
+    let(:fake_logger) { FakeLogger.new }
 
     before do
+      Rails.logger.broadcast_to(fake_logger)
       overriden_translation_ja.update_columns(
         value: "{ count, plural, one {返信 # 件、} other {返信 # 件、} }",
       )
-      overriden_translation_he.update_columns(value: "{ count, plural, ")
+      overriden_translation_zh_tw.update_columns(value: "{ count, plural, ")
       v8_ctx.eval(output)
     end
 
+    after { Rails.logger.stop_broadcasting_to(fake_logger) }
+
     context "when locale is 'en'" do
-      let(:locale) { "en" }
+      let(:locale) { :en }
 
       it "generates messages for the 'en' locale only" do
         expect(generated_locales).to eq %w[en]
@@ -205,7 +209,7 @@ RSpec.describe JsLocaleHelper do
     end
 
     context "when locale is not 'en'" do
-      let(:locale) { "fr" }
+      let(:locale) { :fr }
 
       it "generates messages for the current locale and uses 'en' as fallback" do
         expect(generated_locales).to match(%w[fr en])
@@ -245,7 +249,7 @@ RSpec.describe JsLocaleHelper do
     end
 
     context "when locale contains invalid plural keys" do
-      let(:locale) { "ja" }
+      let(:locale) { :ja }
 
       it "does not raise an error" do
         expect(generated_locales).to match(%w[ja en])
@@ -253,11 +257,16 @@ RSpec.describe JsLocaleHelper do
     end
 
     context "when locale contains malformed messages" do
-      let(:locale) { "he" }
+      let(:locale) { :zh_TW }
 
       it "raises an error" do
         expect(output).to match(/Failed to compile message formats/)
       end
+
+      it "logs which keys are problematic" do
+        output
+        expect(fake_logger.errors).to include(/posts_likes_MF/)
+      end
     end
   end