887095 - cli locale was not set properly #1446

Merged
merged 1 commit into from Jan 24, 2013

5 participants

@lzap

Ok this patch completely replace ApiController.set_locale. It now properly parses locale coming from our CLI client and accept both UNIX locale format (cs_CZ) as well as HTTP standard one (cs-CZ).

Then it uses ApplicationController#extract_locale_from_accept_language_header method to properly find the right locale.

Also this patch correct ApplicationContoller#parse_locale method which is again instance method so it can access request environment and removes extract_locale_from_accept_language_header call from backend jobs (which will always return "en" anyway).

https://bugzilla.redhat.com/show_bug.cgi?id=887095
https://bugzilla.redhat.com/show_bug.cgi?id=896251

@lzap

Rebased, now it detects system language for delayed_jobs. Review.

@daviddavis daviddavis commented on the diff Jan 23, 2013
src/app/controllers/application_controller.rb
@@ -219,8 +219,8 @@ def no_env_available_msg
# Look for match to list of locales specified in request. If not found, try matching just
# first two letters. Finally, default to english if no matches at all.
# eg. [en_US, en] would match en
- def self.extract_locale_from_accept_language_header
- locales = parse_locale
+ # Expects list of locales to search as an array (use parse_locale for that)
+ def self.extract_locale_from_accept_language_header locales
@daviddavis
Katello Project member
daviddavis added a line comment Jan 23, 2013

MIssing parens here.

@pitr-ch
Katello Project member
pitr-ch added a line comment Jan 23, 2013

I missing something, why are the parenthesis missing? I use parenthesis as less as possible, with some exceptions like method signature. Space always looked more readable for me then parenthesis.

@daviddavis
Katello Project member
daviddavis added a line comment Jan 23, 2013

So according to our style guide, method defs should have parentheses. I changed the wording though from must to highly recommend but I think parentheses are more readable as a underscore looks a lot like a space at times especially with long method names.

https://fedorahosted.org/katello/wiki/RubyCodeConventions

@pitr-ch
Katello Project member
pitr-ch added a line comment Jan 24, 2013

Ok thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@daviddavis daviddavis commented on the diff Jan 23, 2013
src/app/controllers/application_controller.rb
@@ -169,7 +169,7 @@ def set_locale
if current_user && current_user.default_locale
I18n.locale = current_user.default_locale
else
- I18n.locale = ApplicationController.extract_locale_from_accept_language_header
+ I18n.locale = ApplicationController.extract_locale_from_accept_language_header parse_locale
@daviddavis
Katello Project member
daviddavis added a line comment Jan 23, 2013

Would prefer parens here but I can understand that it might be a DSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@knowncitizen

Very nice. Thanks for doing this Lukas - as I recently noticed that Katello was not respecting browser locale either. Does this still take into consideration the user's locale selection (which may be browser locale)?

@daviddavis daviddavis commented on the diff Jan 23, 2013
src/app/controllers/api/api_controller.rb
logger.debug "Setting locale: #{I18n.locale}"
end
+ def parse_locale
+ first, second = request.env['HTTP_ACCEPT_LANGUAGE'].split(/[-_]/)
+ if second.nil?
@daviddavis
Katello Project member
daviddavis added a line comment Jan 23, 2013

This might be a bit more concise:

if second
   # HTTP spec defines only dash-based languages, se we need to convert and add a fallback
   return ["#{first.downcase}-#{second.upcase}", first.downcase]
else
   return [first.downcase]
end
@pitr-ch
Katello Project member
pitr-ch added a line comment Jan 23, 2013

I would also omit the returns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lzap

Sure it does, basically I did not change anything, except I refactored the method back to instance method and fixed env variable to request.env variable so it finds user setting from the browser again. If user has his own setting, then it is taken with higher priority.

@thomasmckay
Katello Project member

ACK

@xsuchy xsuchy commented on the diff Jan 24, 2013
src/app/models/async_operation.rb
@@ -36,7 +36,10 @@ def perform
if User.current && User.current.default_locale
I18n.locale = User.current.default_locale
else
- I18n.locale = ApplicationController.extract_locale_from_accept_language_header
+ # if user did not set his locale we are not able to detect browser setting here and we have to
+ # fall back to system language or English
+ system_lang = ENV['LC_LANG'] || 'en'
@xsuchy
xsuchy added a line comment Jan 24, 2013

What is LC_LANG?
man locale knows only LANG.
end the order should be (according that man page) LC_ALL, if unset then LC_MESSAGES, if unset then LANG

@lzap
lzap added a line comment Jan 24, 2013

Fixed, repairing unit tests and merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@daviddavis
Katello Project member

Do not merge this. The code is being merged via #1461 which we created so we could get the change into the 1.1.2 puddle that we're releasing tonight.

@mccun934 mccun934 merged commit 3097b71 into Katello:master Jan 24, 2013

1 check failed

Details default The Travis build failed
@lzap lzap deleted the lzap:i18n-fallbacks-887095 branch Jan 25, 2013
@lzap

Oh ACK. Cool.

@lzap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment