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

translation_lookup_prefix for records decorated by draper #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karwank
Copy link
Contributor

@karwank karwank commented Oct 15, 2019

When draper gem is used to decorate models in controller translation_lookup_prefix method creates wrong path to the translation.

@@ -132,6 +132,8 @@ def header_sort_link(column, options={}, &block)
def translation_lookup_prefix
if global_options[:records].respond_to?(:model)
"activerecord.attributes.#{global_options[:records].model.to_s.underscore}"
elsif global_options[:records].all? {|record| record.is_a?(ApplicationDecorator) && record.class == global_options[:records].first.class && record.respond_to?(:object) }
Copy link
Owner

@hunterae hunterae Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break if the application does not have ApplicationDecorator defined. Perhaps the elsif should start with something like:

elsif defined?(ApplicationDecorator) && global_options[:records].all? {|record| record.is_a?(ApplicationDecorator) && record.class == global_options[:records].first.class && record.respond_to?(:object) }

This would also be dependent on whether ApplicationDecorator is expected to exist when using the draper gem (which I haven't used in several years so I can't remember).

Also, please add a test case to handle this scenario.

@karwank
Copy link
Contributor Author

karwank commented Oct 15, 2019

Yes, you are right. I forgot that it may crash when ApplicationDecorator class is not defined. I also realized that ApplicationDecorator is only an extension of Draper::Decorator so I changed it according to your suggestion. Sorry, I don't know how to write such test.

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 this pull request may close these issues.

2 participants