FIX: async reload of locales could result in missing translations

lib/freedom_patches/translate_accelerator.rb

@@ -15,23 +15,21 @@ module I18n
     alias_method :translate_no_cache, :translate
     alias_method :exists_no_cache?, :exists?
     alias_method :reload_no_cache!, :reload!
+    alias_method :locale_no_cache=, :locale=
+
     LRU_CACHE_SIZE = 300
 
     def init_accelerator!
       @overrides_enabled = true
-      reload!
+      execute_reload
     end
 
     def reload!
-      @loaded_locales = []
+      @requires_reload = true
-      @cache = nil
-      @overrides_by_site = {}
-
-      reload_no_cache!
-      ensure_all_loaded!
     end
 
     LOAD_MUTEX = Mutex.new
+
     def load_locale(locale)
       LOAD_MUTEX.synchronize do
         return if @loaded_locales.include?(locale)
@@ -61,7 +59,9 @@ module I18n
       backend.fallbacks(locale).each { |l| ensure_loaded!(l) }
     end
 
-    def search(query, opts = nil)
+    def search(query, opts = {})
+      execute_reload if @requires_reload
+
       locale = opts[:locale] || config.locale
 
       load_locale(locale) unless @loaded_locales.include?(locale)
@@ -140,6 +140,8 @@ module I18n
     end
 
     def translate(*args)
+      execute_reload if @requires_reload
+
       options  = args.last.is_a?(Hash) ? args.pop.dup : {}
       key      = args.shift
       locale   = options[:locale] || config.locale
@@ -177,10 +179,35 @@ module I18n
     alias_method :t, :translate
 
     def exists?(key, locale = nil)
+      execute_reload if @requires_reload
+
       locale ||= config.locale
       load_locale(locale) unless @loaded_locales.include?(locale)
       exists_no_cache?(key, locale)
     end
 
+    def locale=(value)
+      execute_reload if @requires_reload
+      self.locale_no_cache = value
+    end
+
+    private
+
+    RELOAD_MUTEX = Mutex.new
+
+    def execute_reload
+      RELOAD_MUTEX.synchronize do
+        return unless @requires_reload
+
+        @loaded_locales = []
+        @cache = nil
+        @overrides_by_site = {}
+
+        reload_no_cache!
+        ensure_all_loaded!
+
+        @requires_reload = false
+      end
+    end
   end
 end

lib/i18n/backend/discourse_i18n.rb

@@ -12,7 +12,6 @@ module I18n
       end
 
       def reload!
-        @overrides = {}
         @pluralizers = {}
         super
       end

spec/components/discourse_i18n_spec.rb

@@ -7,24 +7,23 @@ describe I18n::Backend::DiscourseI18n do
   let(:backend) { I18n::Backend::DiscourseI18n.new }
 
   before do
-    I18n.reload!
+    backend.reload!
-    backend.store_translations(:en, foo: 'Foo in :en', bar: 'Bar in :en', wat: "Hello %{count}")
+    backend.store_translations(:en, foo: 'Foo in :en', bar: 'Bar in :en', wat: 'Hello %{count}')
-    backend.store_translations(:en, items: { one: 'one item', other: "%{count} items" })
+    backend.store_translations(:en, items: { one: 'one item', other: '%{count} items' })
     backend.store_translations(:de, bar: 'Bar in :de')
     backend.store_translations(:ru, baz: 'Baz in :ru')
     backend.store_translations(:en, link: '[text](url)')
   end
 
   after do
-    I18n.locale = :en
+    backend.reload!
-    I18n.reload!
   end
 
   it 'translates the basics as expected' do
-    expect(backend.translate(:en, 'foo')).to eq("Foo in :en")
+    expect(backend.translate(:en, 'foo')).to eq('Foo in :en')
-    expect(backend.translate(:en, 'items', count: 1)).to eq("one item")
+    expect(backend.translate(:en, 'items', count: 1)).to eq('one item')
-    expect(backend.translate(:en, 'items', count: 3)).to eq("3 items")
+    expect(backend.translate(:en, 'items', count: 3)).to eq('3 items')
-    expect(backend.translate(:en, 'wat', count: 3)).to eq("Hello 3")
+    expect(backend.translate(:en, 'wat', count: 3)).to eq('Hello 3')
   end
 
   it 'can be searched by key or value' do
@@ -93,136 +92,4 @@ describe I18n::Backend::DiscourseI18n do
       expect(backend.exists?(:ru, :bogus)).to eq(false)
     end
   end
-
-  describe 'with overrides' do
-    it 'returns the overridden key' do
-      TranslationOverride.upsert!('en', 'foo', 'Overwritten foo')
-      expect(I18n.translate('foo')).to eq('Overwritten foo')
-
-      TranslationOverride.upsert!('en', 'foo', 'new value')
-      expect(I18n.translate('foo')).to eq('new value')
-    end
-
-    it 'returns the overridden key after switching the locale' do
-      TranslationOverride.upsert!('en', 'foo', 'Overwritten foo in EN')
-      TranslationOverride.upsert!('de', 'foo', 'Overwritten foo in DE')
-
-      expect(I18n.translate('foo')).to eq('Overwritten foo in EN')
-      I18n.locale = :de
-      expect(I18n.translate('foo')).to eq('Overwritten foo in DE')
-    end
-
-    it "can be searched" do
-      TranslationOverride.upsert!('en', 'wat', 'Overwritten value')
-      expect(I18n.search('wat', backend: backend)).to eq('wat' => 'Overwritten value')
-      expect(I18n.search('Overwritten', backend: backend)).to eq('wat' => 'Overwritten value')
-
-      TranslationOverride.upsert!('en', 'wat', 'Overwritten with (parentheses)')
-      expect(I18n.search('Overwritten with (', backend: backend)).to eq('wat' => 'Overwritten with (parentheses)')
-    end
-
-    it 'supports disabling' do
-      orig_title = I18n.t('title')
-      TranslationOverride.upsert!('en', 'title', 'overridden title')
-
-      I18n.overrides_disabled do
-        expect(I18n.translate('title')).to eq(orig_title)
-      end
-      expect(I18n.translate('title')).to eq('overridden title')
-    end
-
-    it 'supports interpolation' do
-      TranslationOverride.upsert!('en', 'foo', 'hello %{world}')
-      I18n.backend.store_translations(:en, foo: 'bar')
-      expect(I18n.translate('foo', world: 'foo')).to eq('hello foo')
-    end
-
-    it 'supports interpolation named count' do
-      TranslationOverride.upsert!('en', 'wat', 'goodbye %{count}')
-      I18n.backend.store_translations(:en, wat: 'bar')
-      expect(I18n.translate('wat', count: 123)).to eq('goodbye 123')
-    end
-
-    it 'ignores interpolation named count if it is not applicable' do
-      TranslationOverride.upsert!('en', 'test', 'goodbye')
-      I18n.backend.store_translations(:en, test: 'foo')
-      I18n.backend.store_translations(:en, wat: 'bar')
-      expect(I18n.translate('wat', count: 1)).to eq('bar')
-    end
-
-    it 'supports one and other' do
-      TranslationOverride.upsert!('en', 'items.one', 'one fish')
-      TranslationOverride.upsert!('en', 'items.other', '%{count} fishies')
-      I18n.backend.store_translations(:en, items: { one: 'one item', other: "%{count} items" })
-      expect(I18n.translate('items', count: 13)).to eq('13 fishies')
-      expect(I18n.translate('items', count: 1)).to eq('one fish')
-    end
-
-    it 'supports one and other when only a single pluralization key is overridden' do
-      TranslationOverride.upsert!('en', 'keys.magic.other', "no magic keys")
-      I18n.backend.store_translations(:en, keys: { magic: { one: 'one magic key', other: "%{count} magic keys" } })
-      expect(I18n.translate('keys.magic', count: 1)).to eq("one magic key")
-      expect(I18n.translate('keys.magic', count: 2)).to eq("no magic keys")
-    end
-
-    it "returns the overriden text when falling back" do
-      TranslationOverride.upsert!('en', 'got', "summer")
-      I18n.backend.store_translations(:en, got: 'winter')
-
-      expect(I18n.translate('got')).to eq('summer')
-      expect(I18n.with_locale(:zh_TW) { I18n.translate('got') }).to eq('summer')
-
-      TranslationOverride.upsert!('en', 'throne', "%{title} is the new queen")
-      I18n.backend.store_translations(:en, throne: "%{title} is the new king")
-
-      expect(I18n.t('throne', title: 'snow')).to eq('snow is the new queen')
-
-      expect(I18n.with_locale(:en) { I18n.t('throne', title: 'snow') })
-        .to eq('snow is the new queen')
-    end
-
-    it "returns override if it exists before falling back" do
-      I18n.backend.store_translations(:en, got: 'winter')
-
-      expect(I18n.translate('got', default: '')).to eq('winter')
-      expect(I18n.with_locale(:ru) { I18n.translate('got', default: '') }).to eq('winter')
-
-      TranslationOverride.upsert!('ru', 'got', "summer")
-      I18n.backend.store_translations(:en, got: 'winter')
-
-      expect(I18n.translate('got', default: '')).to eq('winter')
-      expect(I18n.with_locale(:ru) { I18n.translate('got', default: '') }).to eq('summer')
-    end
-
-    it 'does not affect ActiveModel::Naming#human' do
-      Fish = Class.new(ActiveRecord::Base)
-
-      TranslationOverride.upsert!('en', 'fish', "fake fish")
-      I18n.backend.store_translations(:en, fish: "original fish")
-
-      expect(Fish.model_name.human).to eq('Fish')
-    end
-
-    describe "client json" do
-      it "is empty by default" do
-        expect(I18n.client_overrides_json('en')).to eq("{}")
-      end
-
-      it "doesn't return server overrides" do
-        TranslationOverride.upsert!('en', 'foo', 'bar')
-        expect(I18n.client_overrides_json('en')).to eq("{}")
-      end
-
-      it "returns client overrides" do
-        TranslationOverride.upsert!('en', 'js.foo', 'bar')
-        TranslationOverride.upsert!('en', 'admin_js.beep', 'boop')
-        json = ::JSON.parse(I18n.client_overrides_json('en'))
-
-        expect(json).to be_present
-        expect(json['js.foo']).to eq('bar')
-        expect(json['admin_js.beep']).to eq('boop')
-      end
-    end
-  end
-
 end

spec/components/freedom_patches/translate_accelerator_spec.rb

@@ -1,35 +1,51 @@
 require "rails_helper"
 
 describe "translate accelerator" do
+  before(:all) do
+    @original_i18n_load_path = I18n.load_path.dup
+    I18n.load_path += Dir["#{Rails.root}/spec/fixtures/i18n/translate_accelerator.*.yml"]
+    I18n.reload!
+  end
+
+  after(:all) do
+    I18n.load_path = @original_i18n_load_path
+    I18n.reload!
+  end
+
   after do
     I18n.reload!
   end
 
+  def override_translation(locale, key, value)
+    expect(I18n.exists?(key, locale)).to eq(true)
+    override = TranslationOverride.upsert!(locale, key, value)
+    expect(override.persisted?).to eq(true)
+  end
+
   it "overrides for both string and symbol keys" do
-    key = "user.email.not_allowed"
+    key = 'user.email.not_allowed'
-    text_overriden = "foobar"
+    text_overriden = 'foobar'
 
     expect(I18n.t(key)).to be_present
 
-    TranslationOverride.upsert!("en", key, text_overriden)
+    override_translation('en', key, text_overriden)
 
     expect(I18n.t(key)).to eq(text_overriden)
     expect(I18n.t(key.to_sym)).to eq(text_overriden)
   end
 
-  describe '.overrides_by_locale' do
+  describe ".overrides_by_locale" do
-    it 'should cache overrides for each locale' do
+    it "should cache overrides for each locale" do
-      TranslationOverride.upsert!('en', 'got', "summer")
+      override_translation('en', 'got', 'summer')
-      TranslationOverride.upsert!('zh_TW', 'got', "冬季")
+      override_translation('zh_TW', 'got', '冬季')
-      I18n.backend.store_translations(:en, got: 'winter')
 
       I18n.overrides_by_locale('en')
       I18n.overrides_by_locale('zh_TW')
 
       expect(I18n.instance_variable_get(:@overrides_by_site)).to eq(
-        "default" => {
+        'default' => {
-          "en" => { "got" => "summer" },
+          'en' => { 'got' => 'summer' },
-          "zh_TW" => { "got" => "冬季" }
+          'zh_TW' => { 'got' => '冬季' }
         }
       )
     end
@@ -38,9 +54,9 @@ describe "translate accelerator" do
   context "plugins" do
     before do
       DiscoursePluginRegistry.register_locale(
-        "foo",
+        'foo',
-        name: "Foo",
+        name: 'Foo',
-        nativeName: "Foo Bar",
+        nativeName: 'Foo Bar',
         plural: {
           keys: [:one, :few, :other],
           rule: lambda do |n|
@@ -61,7 +77,6 @@ describe "translate accelerator" do
     end
 
     it "loads plural rules from plugins" do
-      I18n.backend.store_translations(:foo, items: { one: 'one item', few: 'some items', other: "%{count} items" })
       I18n.locale = :foo
 
       expect(I18n.t('i18n.plural.keys')).to eq([:one, :few, :other])
@@ -70,4 +85,118 @@ describe "translate accelerator" do
       expect(I18n.t('items', count: 20)).to eq('20 items')
     end
   end
+
+  describe "with overrides" do
+    it "returns the overridden key" do
+      override_translation('en', 'foo', 'Overwritten foo')
+      expect(I18n.t('foo')).to eq('Overwritten foo')
+
+      override_translation('en', 'foo', 'new value')
+      expect(I18n.t('foo')).to eq('new value')
+    end
+
+    it "returns the overridden key after switching the locale" do
+      override_translation('en', 'foo', 'Overwritten foo in EN')
+      override_translation('de', 'foo', 'Overwritten foo in DE')
+
+      expect(I18n.t('foo')).to eq('Overwritten foo in EN')
+      I18n.locale = :de
+      expect(I18n.t('foo')).to eq('Overwritten foo in DE')
+    end
+
+    it "can be searched" do
+      override_translation('en', 'wat', 'Overwritten value')
+      expect(I18n.search('wat')).to include('wat' => 'Overwritten value')
+      expect(I18n.search('Overwritten')).to include('wat' => 'Overwritten value')
+
+      override_translation('en', 'wat', 'Overwritten with (parentheses)')
+      expect(I18n.search('Overwritten with (')).to include('wat' => 'Overwritten with (parentheses)')
+    end
+
+    it "supports disabling" do
+      orig_title = I18n.t('title')
+      override_translation('en', 'title', 'overridden title')
+
+      I18n.overrides_disabled do
+        expect(I18n.t('title')).to eq(orig_title)
+      end
+      expect(I18n.t('title')).to eq('overridden title')
+    end
+
+    it "supports interpolation" do
+      override_translation('en', 'world', 'my %{world}')
+      expect(I18n.t('world', world: 'foo')).to eq('my foo')
+    end
+
+    it "supports interpolation named count" do
+      override_translation('en', 'wat', 'goodbye %{count}')
+      expect(I18n.t('wat', count: 123)).to eq('goodbye 123')
+    end
+
+    it "ignores interpolation named count if it is not applicable" do
+      override_translation('en', 'wat', 'bar')
+      expect(I18n.t('wat', count: 1)).to eq('bar')
+    end
+
+    it "supports one and other" do
+      override_translation('en', 'items.one', 'one fish')
+      override_translation('en', 'items.other', '%{count} fishies')
+      expect(I18n.t('items', count: 13)).to eq('13 fishies')
+      expect(I18n.t('items', count: 1)).to eq('one fish')
+    end
+
+    it "supports one and other when only a single pluralization key is overridden" do
+      override_translation('en', 'keys.magic.other', 'no magic keys')
+      expect(I18n.t('keys.magic', count: 1)).to eq('one magic key')
+      expect(I18n.t('keys.magic', count: 2)).to eq('no magic keys')
+    end
+
+    it "returns the overriden text when falling back" do
+      override_translation('en', 'got', 'summer')
+      expect(I18n.t('got')).to eq('summer')
+      expect(I18n.with_locale(:zh_TW) { I18n.t('got') }).to eq('summer')
+
+      override_translation('en', 'throne', '%{title} is the new queen')
+      expect(I18n.t('throne', title: 'snow')).to eq('snow is the new queen')
+      expect(I18n.with_locale(:en) { I18n.t('throne', title: 'snow') })
+        .to eq('snow is the new queen')
+    end
+
+    it "returns override if it exists before falling back" do
+      expect(I18n.t('got', default: '')).to eq('winter')
+      expect(I18n.with_locale(:ru) { I18n.t('got', default: '') }).to eq('winter')
+
+      override_translation('ru', 'got', 'summer')
+      expect(I18n.t('got', default: '')).to eq('winter')
+      expect(I18n.with_locale(:ru) { I18n.t('got', default: '') }).to eq('summer')
+    end
+
+    it "does not affect ActiveModel::Naming#human" do
+      Fish = Class.new(ActiveRecord::Base)
+
+      override_translation('en', 'fish', 'fake fish')
+      expect(Fish.model_name.human).to eq('Fish')
+    end
+
+    describe "client json" do
+      it "is empty by default" do
+        expect(I18n.client_overrides_json('en')).to eq('{}')
+      end
+
+      it "doesn't return server overrides" do
+        override_translation('en', 'foo', 'bar')
+        expect(I18n.client_overrides_json('en')).to eq('{}')
+      end
+
+      it "returns client overrides" do
+        override_translation('en', 'js.foo', 'bar')
+        override_translation('en', 'admin_js.beep', 'boop')
+        json = ::JSON.parse(I18n.client_overrides_json('en'))
+
+        expect(json).to be_present
+        expect(json['js.foo']).to eq('bar')
+        expect(json['admin_js.beep']).to eq('boop')
+      end
+    end
+  end
 end

spec/fixtures/i18n/translate_accelerator.de.yml

@@ -0,0 +1,4 @@
+en:
+  foo: 'Foo in :de'
+  bar: 'Bar in :de'
+  wat: "Hello %{count}"

spec/fixtures/i18n/translate_accelerator.en.yml

@@ -0,0 +1,20 @@
+en:
+  got: "winter"
+  foo: 'Foo in :en'
+  bar: 'Bar in :en'
+  wat: "Hello %{count}"
+  world: "Hello %{world}"
+  items:
+    one: "one item"
+    other: "%{count} items"
+  keys:
+    magic:
+      one: "one magic key"
+      other: "%{count} magic keys"
+  throne: "%{title} is the new king"
+  fish: "original fish"
+
+  js:
+    foo: "foo"
+  admin_js:
+    beep: "beep"

spec/fixtures/i18n/translate_accelerator.foo.yml

@@ -0,0 +1,5 @@
+foo:
+  items:
+    one: "one item"
+    few: "some items"
+    other: "%{count} items"