Skip to content

Use HTTP headers for i18n#1488

Merged
hursey013 merged 1 commit intomasterfrom
bh-header-i18n
Jun 26, 2017
Merged

Use HTTP headers for i18n#1488
hursey013 merged 1 commit intomasterfrom
bh-header-i18n

Conversation

@hursey013
Copy link
Contributor

@hursey013 hursey013 commented Jun 14, 2017

This uses the http_accept_language gem to parse the Accept-Language header and match it up with our available translations.

screen shot 2017-06-14 at 11 36 34 am

@hursey013 hursey013 self-assigned this Jun 14, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to drop this I18n.locale = line because it would mask the work that the Accept-Language header should be doing

Copy link
Contributor

Choose a reason for hiding this comment

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

I would set an explicit , locale: 'en') inside the t( here

Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to make this configurable, or different in the different environments? (ex make it %w[en] for production but then make it %w[en es] for test and development?

The reason is that if somebody visits the IDP today with a Spanish browser, they'd see a bunch of "NOT TRANSLATED YET" strings

@zachmargolis
Copy link
Contributor

Based on this build failure we probably need to add an after to these specs setting the locale back to English, or a default before for all specs:

  1) LOA3 Single Sign On canceling verification with js returns user to profile page if they have previously signed up
     Failure/Error: click_on t('links.cancel')
     
     Capybara::ElementNotFound:
       Unable to find link or button "Cancelar"
     # ./spec/features/saml/loa3_sso_spec.rb:135:in `block (4 levels) in <top (required)>'
  2) LOA3 Single Sign On canceling verification with js returns user to personal key page if they sign up via loa3
     Failure/Error: click_on t('links.cancel')
     
     Capybara::ElementNotFound:
       Unable to find link or button "Cancelar"
     # ./spec/features/saml/loa3_sso_spec.rb:124:in `block (4 levels) in <top (required)>'

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be plural per:
config.i18n.available_locales = Figaro.env.available_locales.split(' ')

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be an allow, IMO we should just set the locale each time. As written, during a test run, something could set the locale to something other than :en but it would always be read back as :en which is wrong

I18n.locale = :en

@hursey013
Copy link
Contributor Author

thanks @zachmargolis, still getting the hang of the semantics of rspec. updated PR w/ your feedback.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

Might want to wait to merge until there's devops support (or at least a default value for the lower envs) but the code here is fine to me

@amoose
Copy link
Contributor

amoose commented Jun 20, 2017

Copy link
Contributor

Choose a reason for hiding this comment

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

This method needs to be updated in order to accept the user's selection (URL param) for locale.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we need to add a method to sanitize the URL parameter. If the URL parameter does not match an available locale, it will throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amoose I am working on implementing the functionality with the params in a different branch, this was just focused on the HTTP headers, but I can roll it all into this one if that is preferred? https://github.com/18F/identity-idp/compare/bh-param-i18n

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok! no problem. Then, let's strip the params[:locale] from this PR and push without my commit in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amoose updated!

@amoose
Copy link
Contributor

amoose commented Jun 23, 2017

Looks like this needs to be rebased, Bhurst. I'm going to push the DevOps PR through today so I hope to merge this later. Please also squash the fixup commits 👍

@hursey013
Copy link
Contributor Author

@amoose done!

@hursey013 hursey013 force-pushed the bh-header-i18n branch 2 times, most recently from 60a6d51 to 22c981d Compare June 23, 2017 20:10
@hursey013 hursey013 merged commit d2866f3 into master Jun 26, 2017
@hursey013 hursey013 deleted the bh-header-i18n branch June 26, 2017 18:22
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.

3 participants