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

Add support to translate model_name #462

Closed

Conversation

nyamadori
Copy link
Contributor

@nyamadori nyamadori commented Nov 21, 2017

This PR allows to translate a model's name in unauthorized message using ActiveSupport I18n.

@nyamadori nyamadori force-pushed the translate_model_name branch from 7e725b4 to 7907b26 Compare November 21, 2017 11:15
Copy link
Member

@coorasse coorasse left a comment

Choose a reason for hiding this comment

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

I think a test case is missing and I would prefer english in the tests. Thanks. Nice job 👍

message = I18n.translate(keys.shift, variables.merge(scope: :unauthorized, default: keys + ['']))
message.blank? ? nil : message
end

def translate_subject(subject)
klass = (subject.class == Class ? subject : subject.class)
if klass.respond_to?(:model_name)
Copy link
Member

Choose a reason for hiding this comment

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

in which case the class does not respond to model_name ? I see two cases here but only one test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in which case the class does not respond to model_name ?

I think there are cases which the class does not respond to model_name because in following spec use Array as ability subject. So I wrote this method considering cases which doesn't respond to model_name.

https://github.com/CanCanCommunity/cancancan/blob/develop/spec/cancan/ability_spec.rb#L498

I see two cases here but only one test.

Test case which does not respond to model_name has wrote already here:

https://github.com/CanCanCommunity/cancancan/blob/develop/spec/cancan/ability_spec.rb#L496


I18n.default_locale = :ja
I18n.backend.store_translations :ja,
activemodel: { models: { account: 'アカウント' } },
Copy link
Member

Choose a reason for hiding this comment

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

could you please use english? You can specify it as ja but please use english words anyway.
Maybe also add translations in another language en to see that they actually differ in your result.

I18n.backend.store_translations :de, activemodel: { models: { account: 'english name' } }
I18n.backend.store_translations :ja, activemodel: { models: { account: 'japanese name' } }
expect(...).to eq('japanese name...')

activemodel: { models: { account: 'アカウント' } },
unauthorized: { update: { all: '%{subject}の更新権限がありません' } }
expect(@ability.unauthorized_message(:update, Account)).to eq('アカウントの更新権限がありません')
# rubocop:enable Style/FormatString
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed.

unauthorized: { update: { all: '%{subject}' } }

I18n.default_locale = :en
expect(@ability.unauthorized_message(:update, Account)).to eq('english name')
Copy link
Member

Choose a reason for hiding this comment

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

could you please use a

I18n.with_locale(:fr) do
end

block? this avoids collateral effects and allow us to avoid the line 494

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coorasse Thx! I fixed: d78398f

@coorasse coorasse changed the base branch from develop to feature/2.2.0 November 22, 2017 13:12
@nyamadori
Copy link
Contributor Author

@coorasse I want to fix CI failures below. Is it better to suppress Rubocop offence (Metrics/ModuleLength: ) in Ability module or weaken the offence (e.g. Metrics/ModuleLength: 100 => 150)?

Offenses:
lib/cancan/ability.rb:19:3: C: Module has too many lines. [103/100]
  module Ability ...
  ^^^^^^^^^^^^^^
41 files inspected, 1 offense detected

https://travis-ci.org/CanCanCommunity/cancancan/jobs/306370981

@coorasse
Copy link
Member

both of those solutions are not fixes. we probably need to split the module.

@coorasse
Copy link
Member

coorasse commented Feb 3, 2019

fixed and merged. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants