Conversation
There was a problem hiding this comment.
Is this overriding an existing method? Do we need to call super here at all?
config/routes.rb
Outdated
There was a problem hiding this comment.
We should not scope the /api routes with a locale IMO
config/routes.rb
Outdated
There was a problem hiding this comment.
Ditto for the /openid_connect URLs (or anything we publish in the developer docs, those should not be locale-scoped IMO
There was a problem hiding this comment.
Are we doing to default en to be not there (like the marketing site)? Or are we going to always have a locale in the URL? I think it might be nice to remove it (aka default to english)?
There was a problem hiding this comment.
Yeah caught this when I saw all the failing specs, fixing now.
|
Can we add feature specs that test a few URLs? |
There was a problem hiding this comment.
This still needs to be moved to a method to check against available locales, to avoid the InvalidLocale exception.
app/services/marketing_site.rb
Outdated
There was a problem hiding this comment.
can we update the specs for these marketing site URLs as well?
spec/services/marketing_site_spec.rb
Outdated
There was a problem hiding this comment.
I would move this into a before block (so it's part of the context) and the same for all of these:
context 'when the user has their locale set to :es' do
before { I18n.locale = :es }
it 'points to the privacy page with the locale embedded' do
expect # ...
end
end
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM! I tested it out locally and it works great!
Once we fix the codeclimate issues, LGTM! (I think we can just tell reek to ignore feature envy)
spec/services/marketing_site_spec.rb
Outdated
There was a problem hiding this comment.
rubocop detected some trailign whitespace here
amoose
left a comment
amoose
left a comment
There was a problem hiding this comment.
LGTM once Bhurst adds the spec for unsupported locales + revert behavior 👍
There was a problem hiding this comment.
What do you think about moving this to a separate i18n param validator class? That's what Reek was suggesting, which I agree with. I'd prefer not to disable Reek offenses if there is an easy way to resolve them.
There was a problem hiding this comment.
@monfresh updated based on feedback, let me know if that is what you were thinking.
hursey013
force-pushed
the
bh-param-i18n
branch
4 times, most recently
from
2e56eb0 to
6a4f198
Compare
|
@zachmargolis @amoose @monfresh pushed latest changes and squashed - if you wouldn't mind giving it one more look and then i'll merge! |
4 participants
In preparation for a language picker UI element, this allows users to set language preference using a param. The selected language can be accessed by prepending the url path with the language tag (
/es/for example). The priority of the language will be param > HTTP header > default language (EN).This also contains an update to pass the language preference to the marketing site if one is selected.