From b5ec241716b6fb09598e9e82080a3b5a2f645473 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 15 Jun 2017 17:08:23 +0900 Subject: [PATCH] FIX: Validate interpolation keys used in translation overrides. https://meta.discourse.org/t/discobot-translation-missing-error/64429/6?u=tgxworld --- .../admin/site_texts_controller.rb | 17 +++++-- app/models/translation_override.rb | 47 +++++++++++++++-- config/locales/server.en.yml | 5 ++ lib/i18n/i18n_interpolation_keys_finder.rb | 9 ++++ .../admin/site_texts_controller_spec.rb | 51 ++++++++++++++++--- spec/models/translation_override_spec.rb | 35 ++++++++++++- .../i18n_interpolation_keys_finder_spec.rb | 11 ++++ 7 files changed, 159 insertions(+), 16 deletions(-) create mode 100644 lib/i18n/i18n_interpolation_keys_finder.rb create mode 100644 spec/services/i18n_interpolation_keys_finder_spec.rb diff --git a/app/controllers/admin/site_texts_controller.rb b/app/controllers/admin/site_texts_controller.rb index 500968f9532..33c7b716fc3 100644 --- a/app/controllers/admin/site_texts_controller.rb +++ b/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 diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb index d39c48221b1..05991ddc0ad 100644 --- a/app/models/translation_override.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bea39723365..131a69c221d 100644 --- a/config/locales/server.en.yml +++ b/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: "
the About Me field of your profile is currently blank, would you like to fill it out?
" diff --git a/lib/i18n/i18n_interpolation_keys_finder.rb b/lib/i18n/i18n_interpolation_keys_finder.rb new file mode 100644 index 00000000000..cc91e806b96 --- /dev/null +++ b/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 diff --git a/spec/controllers/admin/site_texts_controller_spec.rb b/spec/controllers/admin/site_texts_controller_spec.rb index 557646000fb..1577e1a3493 100644 --- a/spec/controllers/admin/site_texts_controller_spec.rb +++ b/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 diff --git a/spec/models/translation_override_spec.rb b/spec/models/translation_override_spec.rb index d1c40d61cdf..823faebe97d 100644 --- a/spec/models/translation_override_spec.rb +++ b/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 - diff --git a/spec/services/i18n_interpolation_keys_finder_spec.rb b/spec/services/i18n_interpolation_keys_finder_spec.rb new file mode 100644 index 00000000000..dde36f40407 --- /dev/null +++ b/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