(feat) O3-2484: The application should revert to default locale on logout #782
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +203 B (0%) Total Size: 2.84 MB
ℹ️ View Unchanged
|
brandones
reviewed
Oct 20, 2023
| }, | ||
| }); | ||
| }); | ||
| it("should render Logout and redirect to login page ig provider.type !== oauth2", async () => { |
There was a problem hiding this comment.
Suggested change
| it("should render Logout and redirect to login page ig provider.type !== oauth2", async () => { | |
| it("should render Logout and redirect to login page if provider.type !== oauth2", async () => { |
There was a problem hiding this comment.
I don't think the if provider.type !== oauth2 is informative or useful here. If we need something here, it might be better to say by default.
Basically, I don't think our test descriptions should hard-code the assumption that the only provider.types are "default" or "oauth2".
There was a problem hiding this comment.
@ibacher Could you take another look at this part and see if it makes sense to you? I don't know anything about provider type and oauth.
There was a problem hiding this comment.
I'm happy with the new test name.
How I'm thinking about this is there: there are various forms of SSO protocols (Kerberos, SAML, OIDC—which is what we're calling OAuth2) with slightly different mechanisms for communicating between the authentication server and the resource server (which is the role OpenMRS plays in any configuration). The provider attribute allows us to customize the way the login app works based on the expected authentication server type.
Currently, we only really have two modes: OpenMRS base authentication and OAuth2 (which is, at least OIDC-like), but it's helpful to have it be treated as more than just a binary value.
There was a problem hiding this comment.
Nice, and thanks for the explanation!
brandones
requested changes
Oct 20, 2023
| @@ -1,5 +1,5 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="<%= openmrsDefaultLocale %>"> | |||
| <html lang="<%= openmrsDefaultLocale %>" defaultLang="<%= openmrsDefaultLocale %>"> | |||
There was a problem hiding this comment.
Suggested change
| <html lang="<%= openmrsDefaultLocale %>" defaultLang="<%= openmrsDefaultLocale %>"> | |
| <html lang="<%= openmrsDefaultLocale %>" data-default-lang="<%= openmrsDefaultLocale %>"> |
Custom attributes must be prefixed.
| @@ -19,6 +20,13 @@ const RedirectLogout: React.FC<RedirectLogoutProps> = () => { | |||
| navigate({ to: "${openmrsSpaBase}/login" }); | |||
| } else { | |||
| performLogout().then(() => { | |||
| const defaultLang = | |||
| document.documentElement.getAttribute("defaultLang"); | |||
There was a problem hiding this comment.
Suggested change
| document.documentElement.getAttribute("defaultLang"); | |
| document.documentElement.getAttribute("data-default-lang"); |
There was a problem hiding this comment.
Shouldn't we just be using the lang attribute for this?
There was a problem hiding this comment.
The trouble is that the lang attribute gets updated with the user preference. That attribute is supposed to be set to the current language of that page. We need a way of storing the default language, which is passed in to the app shell and becomes an ejs parameter. I imagine there is also a way of obtaining the default without rendering it into the HTML. But I think this way of doing it is fine.
|
Everything LGTM other than the |
ibacher
approved these changes
Oct 26, 2023
ibacher
reviewed
Oct 26, 2023
Comment on lines
+23
to
+29
| const defaultLang = | ||
| document.documentElement.getAttribute("data-default-lang"); | ||
| setUserLanguage({ | ||
| locale: defaultLang, | ||
| authenticated: false, | ||
| sessionId: "", | ||
| }); |
There was a problem hiding this comment.
It might be better to make this part of the performLogout() or even the logic that happens here, which might solve similar issues (for instance, when a user's session times out, should the language be reset? If yes, it probably belongs in the refetchCurrentUser() logic), but this is only a suggestion.
There was a problem hiding this comment.
Right @ibacher , I'll look into that too.
Thanks!
|
@brandones , the changes are requested by you, can you approve the PR too? |
brandones
approved these changes
Oct 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
For changes to apps
If applicable
Summary
For an application having the default locale other than
en(say 'km'), if the logged in user changes the locale toen, after the logout, the application's locale is set toen, instead ofkm. This PR resets the application to the default locale on logout.Screenshots
Screen.Recording.2023-10-06.at.12.54.39.mov
Related Issue
https://issues.openmrs.org/projects/O3/issues/O3-2484
Other