FIX: Validate MF strings when adding overrides

Currently, when adding translation overrides, values aren’t validated
for MF strings. This results in being able to add invalid plural keys or
even strings containing invalid syntax.

This patch addresses this issue by compiling the string when saving an
override if the key is detected as an MF one.

If there’s an error from the compiler, it’s added to the model errors,
which in turn is displayed to the user in the admin UI, helping them to
understand what went wrong.

app/models/translation_override.rb

@@ -44,6 +44,7 @@ class TranslationOverride < ActiveRecord::Base
   validates_presence_of :locale, :translation_key, :value
 
   validate :check_interpolation_keys
+  validate :check_MF_string, if: :message_format?
 
   attribute :status, :integer
   enum status: { up_to_date: 0, outdated: 1, invalid_interpolation_keys: 2, deprecated: 3 }
@@ -165,6 +166,10 @@ class TranslationOverride < ActiveRecord::Base
     I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) }
   end
 
+  def message_format?
+    translation_key.to_s.end_with?("_MF")
+  end
+
   private
 
   def transformed_key
@@ -185,6 +190,14 @@ class TranslationOverride < ActiveRecord::Base
       ),
     )
   end
+
+  def check_MF_string
+    require "messageformat"
+
+    MessageFormat.compile(locale, { key: value }, strict: true)
+  rescue MessageFormat::Compiler::CompileError => e
+    errors.add(:base, e.cause.message)
+  end
 end
 
 # == Schema Information

spec/lib/js_locale_helper_spec.rb

@@ -155,20 +155,10 @@ RSpec.describe JsLocaleHelper do
       )
     end
     fab!(:overriden_translation_ja) do
-      Fabricate(
-        :translation_override,
-        locale: "ja",
-        translation_key: "js.posts_likes_MF",
-        value: "{ count, plural, one {返信 # 件、} other {返信 # 件、} }",
-      )
+      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",
-        value: "{ count, plural, ",
-      )
+      Fabricate(:translation_override, locale: "he", 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)") }
@@ -176,7 +166,13 @@ RSpec.describe JsLocaleHelper do
       v8_ctx.eval("I18n._mfMessages.get('posts_likes_MF', {count: 3, ratio: 'med'})")
     end
 
-    before { v8_ctx.eval(output) }
+    before do
+      overriden_translation_ja.update_columns(
+        value: "{ count, plural, one {返信 # 件、} other {返信 # 件、} }",
+      )
+      overriden_translation_he.update_columns(value: "{ count, plural, ")
+      v8_ctx.eval(output)
+    end
 
     context "when locale is 'en'" do
       let(:locale) { "en" }

spec/models/translation_override_spec.rb

@@ -148,6 +148,32 @@ RSpec.describe TranslationOverride do
         end
       end
     end
+
+    describe "MessageFormat translations" do
+      subject(:override) do
+        described_class.new(
+          translation_key: "admin_js.admin.user.delete_all_posts_confirm_MF",
+          locale: "en",
+        )
+      end
+
+      it do
+        is_expected.to allow_value(
+          "This has {COUNT, plural, one{one member} other{# members}}.",
+        ).for(:value).against(:base)
+      end
+      it do
+        is_expected.not_to allow_value(
+          "This has {COUNT, plural, one{one member} many{# members} other{# members}}.",
+        ).for(:value).with_message(/plural case many is not valid/, against: :base)
+      end
+      it do
+        is_expected.not_to allow_value("This has {COUNT, ").for(:value).with_message(
+          /invalid syntax/,
+          against: :base,
+        )
+      end
+    end
   end
 
   it "upserts values" do
@@ -340,4 +366,20 @@ RSpec.describe TranslationOverride do
       expect(translation.invalid_interpolation_keys).to contain_exactly("foo")
     end
   end
+
+  describe "#message_format?" do
+    subject(:override) { described_class.new(translation_key: key) }
+
+    context "when override is for a MessageFormat translation" do
+      let(:key) { "admin_js.admin.user.delete_all_posts_confirm_MF" }
+
+      it { is_expected.to be_a_message_format }
+    end
+
+    context "when override is not for a MessageFormat translation" do
+      let(:key) { "admin_js.type_to_filter" }
+
+      it { is_expected.not_to be_a_message_format }
+    end
+  end
 end