Override i18n_key on ActiveRecord-derived classes

[fsateler]

This allows creating translations for the derived classes themselves instead of polluting the base class translations

Fixes: #115

[toddsiegel]

Any movement on this? I would love to see this or a similar fix merged.

[triskweline]

Sorry for the late review.

A concern that I have is: We have lots of existing models that use the existing logic and store their I18n keys in the namespace of their parent class. With this PR, the namespace will change for all ActiveType::Record classes.

Will this break existing code? Or is there some handling in Rails I18n that makes a key lookup bubble up the inheritance chain, so if a child class doesn't define a key, the lookup is repeated on the parent namespace? If yes, we should have a test for that.

[fsateler]

Will this break existing code? Or is there some handling in Rails I18n that makes a key lookup bubble up the inheritance chain, so if a child class doesn't define a key, the lookup is repeated on the parent namespace? If yes, we should have a test for that.

Yes there is.

But I'm not sure how to create a test for that. We don't have a full rails env in the test.

[triskweline]

Something like this could work (I haven't actually run it):

# in extended_record_spec.rb

describe 'i18n' do

  before :each do
    I18n.backend = I18n::Backend::KeyValue.new({})
  end

  describe 'translation of model name' do

    it 'has its own I18n key' do
      I18n.store_translations(:en, activerecord: { models:  => { extended_record: 'ExtendedRecord translation' } })
      expect(ExtendedRecord.model_name.)human.to eq('ExtendedRecord translation')
    end  

    it 'falls back to the I18n key of the base class if does not have its own I18n key' do
      I18n.store_translations(:en, activerecord: { models:  => { base_record: 'BaseRecord translation' } })
      expect(ExtendedRecord.model_name.human).to eq('BaseRecord translation')
    end

  end

  describe 'translation of attribute name' do

    it 'has its own I18n key' do
      I18n.store_translations(:en, activerecord: { attributes:  => { extended_record => { persisted_string: 'ExtendedRecord translation' } } })
      expect(ExtendedRecord.human_attribute_name(:persisted_string)).to eq('ExtendedRecord translation')
    end  

    it 'falls back to the I18n key of the base class if does not have its own I18n key' do
      I18n.store_translations(:en, activerecord: { attributes:  => { base_record: { persisted_string: 'BaseRecord translation' } } })
      expect(ExtendedRecord.human_attribute_name(:persisted_string)).to eq('ExtendedRecord translation')
    end

  end

end

[fsateler]

Thanks! I have added a test. Indeed, it appears some more code was required. I'm honestly not sure why the previous code worked on my codebase but not on this test, but the current code feels more correct as the class reference is preserved.

[triskweline]

Great, thanks!

I think we're seeing the CI failures because the test changes the I18n.backend globally, and this change bleeds over to the subsequent tests. I believe we need to store the original I18n.backend in a before and restore it in an after.

[fsateler]

Thanks for the hint. I have applied the around block. Tests passed locally. /me crosses fingers...

[fsateler]

aaand it failed with invalid syntax for older rubies. lets try again...

[triskweline]

One last failure from byebug requiring Ruby 2.4 in a Ruby 2.3 run.

Can we lock an older version of byebug for that Gemfile? byebug 11.0.1 seems to be the last version compatible with Ruby 2.3.

[fsateler]

doh! I never meant to commit that change (that was just for debugging). I'll roll it back

[triskweline]

I released this change as 1.4.0. Thanks for your help!