FIX: Validate interpolation keys used in translation overrides.

https://meta.discourse.org/t/discobot-translation-missing-error/64429/6?u=tgxworld

app/controllers/admin/site_texts_controller.rb

@@ -43,12 +43,19 @@ class Admin::SiteTextsController < Admin::AdminController
 
   def update
     site_text = find_site_text
-    site_text[:value] = params[:site_text][:value]
-    old_text = I18n.t(site_text[:id])
-    StaffActionLogger.new(current_user).log_site_text_change(site_text[:id], site_text[:value], old_text)
+    value = site_text[:value] = params[:site_text][:value]
+    id = site_text[:id]
+    old_value = I18n.t(id)
+    translation_override = TranslationOverride.upsert!(I18n.locale, id, value)
 
-    TranslationOverride.upsert!(I18n.locale, site_text[:id], site_text[:value])
-    render_serialized(site_text, SiteTextSerializer, root: 'site_text', rest_serializer: true)
+    if translation_override.errors.empty?
+      StaffActionLogger.new(current_user).log_site_text_change(id, value, old_value)
+      render_serialized(site_text, SiteTextSerializer, root: 'site_text', rest_serializer: true)
+    else
+      render json: failed_json.merge(
+        message: translation_override.errors.full_messages.join("\n\n")
+      ), status: 422
+    end
   end
 
   def revert

app/models/translation_override.rb

@@ -1,9 +1,12 @@
 require 'js_locale_helper'
+require "i18n/i18n_interpolation_keys_finder"
 
 class TranslationOverride < ActiveRecord::Base
   validates_uniqueness_of :translation_key, scope: :locale
   validates_presence_of :locale, :translation_key, :value
 
+  validate :check_interpolation_keys
+
   def self.upsert!(locale, key, value)
     params = { locale: locale, translation_key: key }
 
@@ -12,9 +15,10 @@ class TranslationOverride < ActiveRecord::Base
       data[:compiled_js] = JsLocaleHelper.compile_message_format(locale, value)
     end
 
-    row_count = where(params).update_all(data)
-    create!(params.merge(data)) if row_count == 0
-    i18n_changed
+    translation_override = find_or_initialize_by(params)
+    params.merge!(data) if translation_override.new_record?
+    i18n_changed if translation_override.update(data)
+    translation_override
   end
 
   def self.revert!(locale, *keys)
@@ -22,13 +26,48 @@ class TranslationOverride < ActiveRecord::Base
     i18n_changed
   end
 
-  protected
+  private
 
     def self.i18n_changed
       I18n.reload!
       MessageBus.publish('/i18n-flush', { refresh: true })
     end
 
+    def lookup_original_text
+      I18n::Backend::Simple.new.send(
+        :lookup, self.locale, self.translation_key
+      )
+    end
+
+    def check_interpolation_keys
+      if original_text = lookup_original_text
+        original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text)
+        new_interpolation_keys = I18nInterpolationKeysFinder.find(value)
+        missing_keys = (original_interpolation_keys - new_interpolation_keys)
+        invalid_keys = (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys
+
+        if missing_keys.present?
+          self.errors.add(:base, I18n.t(
+            'activerecord.errors.models.translation_overrides.attributes.value.missing_interpolation_keys',
+            keys: missing_keys.join(', ')
+          ))
+
+          return false
+        end
+
+        invalid_keys = (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys
+
+        if invalid_keys.present?
+          self.errors.add(:base, I18n.t(
+            'activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys',
+            keys: invalid_keys.join(', ')
+          ))
+
+          return false
+        end
+      end
+    end
+
 end
 
 # == Schema Information

config/locales/server.en.yml

@@ -409,6 +409,11 @@ en:
           attributes:
             execute_at:
               in_the_past: "must be in the future."
+        translation_overrides:
+          attributes:
+            value:
+              invalid_interpolation_keys: 'The following interpolation key(s) are invalid: "%{keys}"'
+              missing_interpolation_keys: 'The following interpolation key(s) are missing: "%{keys}"'
 
   user_profile:
     no_info_me: "<div class='missing-profile'>the About Me field of your profile is currently blank, <a href='/u/%{username_lower}/preferences/about-me'>would you like to fill it out?</a></div>"

lib/i18n/i18n_interpolation_keys_finder.rb

@@ -0,0 +1,9 @@
+class I18nInterpolationKeysFinder
+  def self.find(text)
+    keys = text.scan(I18n::INTERPOLATION_PATTERN)
+    keys.flatten!
+    keys.compact!
+    keys.uniq!
+    keys
+  end
+end

spec/controllers/admin/site_texts_controller_spec.rb

@@ -40,11 +40,36 @@ describe Admin::SiteTextsController do
       end
     end
 
-    context '.update and .revert' do
+    context '#update and #revert' do
+      after do
+        TranslationOverride.delete_all
+        I18n.reload!
+      end
+
+      describe 'failure' do
+        before do
+          TranslationOverride.any_instance.expects(:lookup_original_text)
+            .returns('%{first} %{second}')
+        end
+
+        it 'returns the right error message' do
+          xhr :put, :update, id: 'title', site_text: { value: 'hello %{key}' }
+
+          expect(response.status).to eq(422)
+
+          body = JSON.parse(response.body)
+
+          expect(body['message']).to eq(I18n.t(
+            'activerecord.errors.models.translation_overrides.attributes.value.missing_interpolation_keys',
+            keys: 'first, second'
+          ))
+        end
+      end
+
       it 'updates and reverts the key' do
         orig_title = I18n.t(:title)
 
-        xhr :put, :update, id: 'title', site_text: {value: 'hello'}
+        xhr :put, :update, id: 'title', site_text: { value: 'hello' }
         expect(response).to be_success
 
         json = ::JSON.parse(response.body)
@@ -56,7 +81,6 @@ describe Admin::SiteTextsController do
         expect(site_text['id']).to eq('title')
         expect(site_text['value']).to eq('hello')
 
-
         # Revert
         xhr :put, :revert, id: 'title'
         expect(response).to be_success
@@ -72,13 +96,28 @@ describe Admin::SiteTextsController do
       end
 
       it 'returns not found for missing keys' do
-        xhr :put, :update, id: 'made_up_no_key_exists', site_text: {value: 'hello'}
+        xhr :put, :update, id: 'made_up_no_key_exists', site_text: { value: 'hello' }
         expect(response).not_to be_success
       end
 
       it 'logs the change' do
-        StaffActionLogger.any_instance.expects(:log_site_text_change).once
-        xhr :put, :update, id: 'title', site_text: {value: 'hello'}
+        original_title = I18n.t(:title)
+
+        xhr :put, :update, id: 'title', site_text: { value: 'yay' }
+
+        log = UserHistory.last
+
+        expect(log.previous_value).to eq(original_title)
+        expect(log.new_value).to eq('yay')
+        expect(log.action).to eq(UserHistory.actions[:change_site_text])
+
+        xhr :put, :revert, id: 'title'
+
+        log = UserHistory.last
+
+        expect(log.previous_value).to eq('yay')
+        expect(log.new_value).to eq(original_title)
+        expect(log.action).to eq(UserHistory.actions[:change_site_text])
       end
     end
   end

spec/models/translation_override_spec.rb

@@ -1,6 +1,40 @@
 require 'rails_helper'
 
 describe TranslationOverride do
+  context 'validations' do
+    describe '#value' do
+      before do
+        described_class.any_instance.expects(:lookup_original_text)
+          .returns('%{first} %{second}')
+      end
+
+      describe 'when interpolation keys are missing' do
+        it 'should not be valid' do
+          translation_override = TranslationOverride.upsert!(
+            I18n.locale, 'some_key', '%{first}'
+          )
+
+          expect(translation_override.errors.full_messages).to include(I18n.t(
+            'activerecord.errors.models.translation_overrides.attributes.value.missing_interpolation_keys',
+            keys: 'second'
+          ))
+        end
+      end
+
+      describe 'when interpolation keys are invalid' do
+        it 'should not be valid' do
+          translation_override = TranslationOverride.upsert!(
+            I18n.locale, 'some_key', '%{first} %{second} %{third}'
+          )
+
+          expect(translation_override.errors.full_messages).to include(I18n.t(
+            'activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys',
+            keys: 'third'
+          ))
+        end
+      end
+    end
+  end
 
   it "upserts values" do
     TranslationOverride.upsert!('en', 'some.key', 'some value')
@@ -19,4 +53,3 @@ describe TranslationOverride do
   end
 
 end
-

spec/services/i18n_interpolation_keys_finder_spec.rb

@@ -0,0 +1,11 @@
+require 'rails_helper'
+require "i18n/i18n_interpolation_keys_finder"
+
+RSpec.describe I18nInterpolationKeysFinder do
+  describe '#find' do
+    it 'should return the right keys' do
+      expect(described_class.find('%{first} %{second}'))
+        .to eq(['first', 'second'])
+    end
+  end
+end