Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n_key for derived record is the same as parent record #115

Closed
fsateler opened this issue Feb 12, 2020 · 1 comment · Fixed by #116
Closed

i18n_key for derived record is the same as parent record #115

fsateler opened this issue Feb 12, 2020 · 1 comment · Fixed by #116

Comments

@fsateler
Copy link
Contributor

fsateler commented Feb 12, 2020

Which creates pollution for the base class translations.

The setup

en:
  activerecord:
    attributes:
      some_record_extended:
        new_attribute: My Fancy Attribute
class SomeRecord < ActiveRecord::Base
end

class SomeRecordExtended < ActiveType::Record[::SomeRecord]
  attr_accessor :new_attribute
end

Expected Result

SomeRecordExtended.human_attribute_name :new_attribute # => 'My Fancy Attribute'

Actual Result

SomeRecordExtended.human_attribute_name :new_attribute # => 'New attribute'

Investigation

Turns out that ActiveType::RecordExtension::Inheritance simply delegates model_name to the base record class:

def model_name
extended_record_base_class.model_name
end

AFAICT, this necessary because it is desired that param_key and friends be the same. However, this has the problem that i18n_key is the same too:

SomeRecord.model_name.i18n_key # => 'some_record'
SomeRecordExtended.model_name.i18n_key # => 'some_record'

This results in having to declare the extended properties in the translation for the base record class, which is confusing and pollutes the translations.

Solutions

My first local hack was to do this:

def self.model_name
  super.tap{|n| n.instance_variable_set(:@i18n_key, name.underscore) }
end

Which is of course super hacky. An alternative would be to create a Name subclass:

class ActiveTypeName < ActiveModel::Name
  def initialize(klass, extended_record_base_klass)
    super(extended_record_base_klass)
    @i18n_key = klass.name.underscore
  end

  attr_reader :i18n_key # Redefine to avoid depending on internal rails behavior
end

and then redefine model_name:

def model_name
   @model_name ||= ActiveTypeName.new(self, extended_record_base_class)
end

A third alternative would be to monkey_patch i18n_key:

def model_name
   @model_name ||= begin
     dup_model_name = extended_record_base_class.model_name.dup
     key = name.underscore
     dup_model_name.define_singleton_method(:i18n_key) { key }
     dup_model_name
   end
end
fsateler added a commit to fsateler/active_type that referenced this issue Feb 12, 2020
This allows creating translations for the derived classes themselves instead of polluting
the base class translations

Fixes: makandra#115
@fsateler
Copy link
Contributor Author

I've posted a PR implementing the third option. Seems like the less intrusive to me.

fsateler added a commit to fsateler/active_type that referenced this issue Jul 21, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
fsateler added a commit to fsateler/active_type that referenced this issue Jul 21, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
fsateler added a commit to fsateler/active_type that referenced this issue Jul 21, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
fsateler added a commit to fsateler/active_type that referenced this issue Jul 21, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
fsateler added a commit to fsateler/active_type that referenced this issue Jul 22, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
fsateler added a commit to fsateler/active_type that referenced this issue Jul 24, 2020
This allows creating translations for the derived classes themselves
instead of polluting the base class translations.

For this we need to create an ActiveModel::Name, which uses the parent
class name, but derived class reference. We then override the i18n_key,
but not the other ones, to override i18n lookup but keep the same
parameter and route names.

A test is added to ensure the behavior

Fixes: makandra#115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant