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

Support ActiveModel::Error translation lookup on indexed attributes. #37447

Merged

Conversation

@jonathankwok
Copy link
Contributor

jonathankwok commented Oct 11, 2019

Summary

The intent of this PR is to allow instances of ActiveModel::Error where the attribute in question is an indexed attribute to lookup their message without the indexed suffix. This usually happens for ActiveRecord models that use index_errors: true.

Here's a generic example leading up to the error:

class Manager < ActiveRecord::Base
  has_many :reports, index_errors: true
end

class Report < ActiveRecord::Base
  belongs_to :manager
  validates_presence_of :name
end

manager = Manager.new
invalid_report = Report.new
manager.reports = [invalid_report]
manager.save

error = manager.errors.first
error.attribute
# => :"reports[0].name"
error.type
# => :presence

As you can see, the attribute is indexed - great for matching the appropriate Report record, but not great if you have a custom translation message, as the lookup for message will try look for it here:

defaults = base.class.lookup_ancestors.flat_map do |klass|
[ :"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type}",
:"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.#{type}" ]
end

Which, in our specific case, will resolve the first example to:

# :"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type}"
:"activerecord.errors.models.manager.attributes.reports[0].name.presence"

The index makes it impossible to form a generic message for our specific error message. While the index is useful for mapping a specific error to the relevant record, it's not particularly necessary when defining translation keys, as the index is probably irrelevant to what you want to surface.

By adding an additional lookup entry that strips the index from the lookup key (very similar to what was done in #33615), we're able to lookup translations without the index.

Other Information

The rationale for an additional lookup entry (as opposed to replacing the current one) is that I don't know if any users are working around the index lookup problem with embedded yml files that handle keys with indexes, e.g. with regex.

Not sure if this is something that I should add a changelog entry for. I'd be happy to add one if necessary.

@Larochelle @rafaelfranca @Edouard-chin

@rails-bot rails-bot bot added the activemodel label Oct 11, 2019
Instances of ActiveModel::Error where `attribute` is a nested attribute
can use translation keys that don't include the index in their lookup key.
@jonathankwok jonathankwok force-pushed the jonathankwok:i18n-error-lookup-for-indexed-attribute branch from e2df9b8 to 5c9b008 Oct 11, 2019
@jonathankwok jonathankwok marked this pull request as ready for review Oct 11, 2019
:"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.#{type}" ]
[
:"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type}",
:"#{i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{unindexed_attribute}.#{type}",

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Oct 15, 2019

Member

Do we need to add a new lookup? in full_message we just always remove the characters that matches /\[\d\]/.

This comment has been minimized.

Copy link
@jonathankwok

jonathankwok Oct 15, 2019

Author Contributor

I was wondering about that myself - I'm not sure how safe it is to modify the existing lookup, because there could theoretically be somebody using the \[\d\] as part of their lookup.

It is kind of ridiculous, and I imagine most people work around that instead, but removing that lookup would break the current expectation, right?

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Oct 15, 2019

Member

It could, be I doubt it will. If people are using the index as part of their lookup they would have a huge translation fine. I'd just remove it, and if someone is relying on that behavior we put it back.

This comment has been minimized.

Copy link
@jonathankwok

jonathankwok Oct 15, 2019

Author Contributor

Sounds good, I changed this to replace the existing lookup instead 👍

@rafaelfranca rafaelfranca merged commit c2a002b into rails:master Oct 15, 2019
1 of 2 checks passed
1 of 2 checks passed
buildkite/rails Build #64360 failed (17 minutes, 34 seconds)
Details
codeclimate All good!
Details
@jonathankwok jonathankwok deleted the jonathankwok:i18n-error-lookup-for-indexed-attribute branch Oct 15, 2019
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Oct 16, 2019
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Oct 16, 2019
georgeclaghorn added a commit that referenced this pull request Oct 17, 2019
Follow-up to #33615 and #37447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.