-
Notifications
You must be signed in to change notification settings - Fork 141
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
[BaseController] Localize OPTIONS (allow optional user auth) #1043
[BaseController] Localize OPTIONS (allow optional user auth) #1043
Conversation
6110815
to
39e340e
Compare
wow this is cool...great work @NickLaMuro |
Wondering if we should cross repo test this |
@Fryguy not opposed to it. Do you have some repos in mind where this makes sense? Not sure what might be using the API in their specs myself. Maybe the UI I guess? |
@NickLaMuro I verified the fix, it is working , tomorrow i will try to do more testing on other pages, i am not sure if we are calling options in all pages or not, Thank you. |
@miq-bot cross-repo-test ManageIQ/manageiq-ui-clasic |
From Pull Request: ManageIQ/manageiq-api#1043
def optional_api_user_or_token | ||
authenticate_user | ||
rescue AuthenticationError => e | ||
api_log_error("AuthenticationError: #{e.message} (on #{request.method}... ignoring)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we should log this more as a warning, since it's not really an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just threw something in there for getting something pushed up, but definitely wasn't satisfied when I wrote it.
We can absolutely change it to a warning. I will see if there is a method for this somewhere. Happy to change anything else with this if you can think of anything better as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment
This allows translating of OPTION data if a user is logged in. To achieve this, the commit does a few things: - Split up require_api_user_or_token to pull out the "authentication" step into it's own method: `.authenticate_user` - Create a new method called `.optional_api_user_or_token` for only OPTIONS requests - Logs in a user in on OPTIONS calls, but no-op if it fails - in otherwords: if you don't pass auth, still work as we did, but allow a user to log in and retrieve settings By doing this, we can make sure that `:set_gettext_locale` can have a user if one is provided, and allow translations to be processed further down the line.
39e340e
to
e8de60d
Compare
@Fryguy let me know if that phrasing change is also good. |
Checked commit NickLaMuro@e8de60d with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint |
Backported to
|
[BaseController] Localize OPTIONS (allow optional user auth) (cherry picked from commit b44f339)
This allows translating of OPTION data if a user is logged in.
To achieve this, the commit does a few things:
.require_api_user_or_token
to pull out the "authentication" step into it's own method:.authenticate_user
.optional_api_user_or_token
for onlyOPTIONS
requestsOPTIONS
calls, but no-op if it failsBy doing this, we can make sure that
:set_gettext_locale
can have a user if one is provided, and allow translations to be processed further down the line.FYI
There are places where we are using
_N()
instead of_()
, which will cause some of the translations to still result in English:https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/9b5b107dfcf702901683f02ac19263e2730d217a/app/models/manageiq/providers/kubernetes/container_manager/options.rb#L5-L13
This probably should be addressed, but can be handled on a case by case basis.
Links
Testing/QA
The spec in
spec/requests/providers_spec.rb
is a pretty good framework for seeing if this works:Update your user record to use a different locale ("es" for Spanish)
Get an API auth token
curl
the OPTIONS endpoint:(the
curl
above is missing some options for auth. I usemiqperf
frommanageiq-performance
myself so I don't have to worry about these options)