Skip to content
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

Update internationalization enhancement for how to handle login template translation strings #724

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Apr 7, 2021

@sg00dwin
Copy link
Member Author

sg00dwin commented Apr 7, 2021

/retest

1 similar comment
@sg00dwin
Copy link
Member Author

sg00dwin commented Apr 7, 2021

/retest

@@ -110,6 +110,8 @@ The desired fallback experience if not all pieces/parts of the UI are translated

We will also modify our existing file conversion tools to handle translation files spread across multiple packages so we can easily export/import translation files.

Translation of the login templates within [oauth-templates](https://github.com/openshift/oauth-templates) and [oauth-server](https://github.com/openshift/oauth-server) repositories will be manually updated by the console team since their are only a small number of strings that rarely change. These strings are to be included in the console repository so that the i18next-parser will add them in the files sent to the Globalization team for translation.
Copy link
Member

Choose a reason for hiding this comment

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

We should add more detail here like how we are using the Accept-Language header and optional lng query parameter to determine what language to show on the backend. We might want @jhadvig to contribute that detail since he did the backend work. We should also discuss if/how we plan to handle the language user preference for the login page. (See discussion in openshift/oauth-server#71)

Copy link
Member Author

Choose a reason for hiding this comment

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

@spadgett The text has been updated with additional detail provided by @jhadvig

@sg00dwin sg00dwin force-pushed the login-template-internationalization branch 4 times, most recently from 36d8860 to afabd43 Compare April 14, 2021 14:56
@jhadvig
Copy link
Member

jhadvig commented Apr 14, 2021

cc'ing @stlaz @sttts

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @sg00dwin and @jhadvig. We should get approval from @stlaz

@@ -110,6 +110,15 @@ The desired fallback experience if not all pieces/parts of the UI are translated

We will also modify our existing file conversion tools to handle translation files spread across multiple packages so we can easily export/import translation files.

Translation of the login page will be handled by [oauth-server](https://github.com/openshift/oauth-server), which will check firstly the request URL for `lng` query. If the query is missing, `oauth-server` will check for request `Accept-Language` header. If both the query and header are missing in the request, `oauth-server` will differ to english language for localization.
Copy link
Member

Choose a reason for hiding this comment

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

s/differ/default

Copy link
Contributor

Choose a reason for hiding this comment

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

Why lng query and header? Why is the latter not enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

which will check firstly the request URL for lng query

Which request? If it's the authorization request, you should use ui_locales as in https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest, and drop the header part

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/english/English

Copy link
Member

Choose a reason for hiding this comment

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

Why lng query and header? Why is the latter not enough?

@sttts We need to be able to override the system setting since we have a user preference for selecting a language in console. If there is a standard query parameter, we can definitely use that instead.

I wouldn't drop the Accept-Language header because we want the default to be the system language when the user hasn't explicitly selected something different as a language preference in console.

Copy link
Contributor

@stlaz stlaz Apr 20, 2021

Choose a reason for hiding this comment

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

I wouldn't drop the Accept-Language header because we want the default to be the system language when the user hasn't explicitly selected something different as a language preference in console.

That's fair. I would also expect the ui_locales to configure the session if set but I see no cookie handling here. Also, I am missing a description of the UI element that allows configuring this on the oauth-server side.

Edit: let's leave sessions out of the question for now, this may not even be the case here. The real question is, though: how does one configure the ui_locales argument? Modifying the URL is not human-friendly.

Copy link
Member

Choose a reason for hiding this comment

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

Thats a good question. I think for now it has to be manual.

Copy link
Member

Choose a reason for hiding this comment

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

I think console would store the last selected language in browser localStorage and then add ui_locales to the request when redirecting the user to login page. I haven't looked closely at the spec, though. Is this a query parameter?

Copy link
Member

Choose a reason for hiding this comment

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

@spadgett thoughts?

enhancements/console/internationalization.md Outdated Show resolved Hide resolved

[oauth-server](https://github.com/openshift/oauth-server) contains default login templates. Custom templates are stored in [oauth-templates](https://github.com/openshift/oauth-templates) repository.

Since there are only a small number of strings, that rarely change, the localized strings for each of the supported languages will be directly part of the `oauth-server`, so we don't have to load them during runtime.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little unclear what this is saying.

Copy link
Member

Choose a reason for hiding this comment

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

@spadgett it's referring to https://github.com/openshift/oauth-server/blob/60b81ecc54f02b6f2fd4e9bba36c06888bee0272/pkg/server/login/locales.go#L41-L50

Maybe to make it clear we could say:

Suggested change
Since there are only a small number of strings, that rarely change, the localized strings for each of the supported languages will be directly part of the `oauth-server`, so we don't have to load them during runtime.
Since there are only a small number of strings, that rarely change, the localized strings for each of the supported languages will be directly hardcoded into the `oauth-server`, so we don't have to load them during runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the alternative? What does the web console use? Can we use that here?

Copy link
Member

Choose a reason for hiding this comment

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

We use https://www.i18next.com/, but this seems heavy weight for a handful of strings in a static HTML page. I'm not sure how this would work with login page customization as well, since those pages would need to include the script. It's something we could explore though.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan going forward, then? This only localizes the console, but the information about the cluster state (operator statuses, for example) are still going to be in English. Is that ok or do you expect these to be translated eventually, too?

If they are going to be translated, we'd need a bit more modularity in those translations. But I guess for now we can go with what you're proposing.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, since there are just a handful of strings I would prefer to put them directly into a go pkg, as is currently proposed.

Copy link
Member

Choose a reason for hiding this comment

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

What's the plan going forward, then? This only localizes the console, but the information about the cluster state (operator statuses, for example) are still going to be in English. Is that ok or do you expect these to be translated eventually, too?

We don't currently have any plan to translate data coming from k8s resources, only the messages in console itself.

@@ -110,6 +110,15 @@ The desired fallback experience if not all pieces/parts of the UI are translated

We will also modify our existing file conversion tools to handle translation files spread across multiple packages so we can easily export/import translation files.

Translation of the login page will be handled by [oauth-server](https://github.com/openshift/oauth-server), which will check firstly the request URL for `lng` query. If the query is missing, `oauth-server` will check for request `Accept-Language` header. If both the query and header are missing in the request, `oauth-server` will differ to english language for localization.
Copy link
Contributor

Choose a reason for hiding this comment

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

which will check firstly the request URL for lng query

Which request? If it's the authorization request, you should use ui_locales as in https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest, and drop the header part

@@ -110,6 +110,15 @@ The desired fallback experience if not all pieces/parts of the UI are translated

We will also modify our existing file conversion tools to handle translation files spread across multiple packages so we can easily export/import translation files.

Translation of the login page will be handled by [oauth-server](https://github.com/openshift/oauth-server), which will check firstly the request URL for `lng` query. If the query is missing, `oauth-server` will check for request `Accept-Language` header. If both the query and header are missing in the request, `oauth-server` will differ to english language for localization.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/english/English


[oauth-server](https://github.com/openshift/oauth-server) contains default login templates. Custom templates are stored in [oauth-templates](https://github.com/openshift/oauth-templates) repository.

Since there are only a small number of strings, that rarely change, the localized strings for each of the supported languages will be directly part of the `oauth-server`, so we don't have to load them during runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the alternative? What does the web console use? Can we use that here?


Since there are only a small number of strings, that rarely change, the localized strings for each of the supported languages will be directly part of the `oauth-server`, so we don't have to load them during runtime.

These strings are to be included in the console repository so that the i18next-parser will add them in the files sent to the Globalization team for translation. Afterwards console team will contribute them back to the [oauth-server](https://github.com/openshift/oauth-server).
Copy link
Contributor

Choose a reason for hiding this comment

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

style: this should be a part of the previous paragraph

Think about a way to make sure the current and updated translated strings are correctly synced to the oauth-server repository.

@sg00dwin sg00dwin force-pushed the login-template-internationalization branch 4 times, most recently from 58c26c4 to 8307c2d Compare April 20, 2021 14:36
@jhadvig
Copy link
Member

jhadvig commented Apr 26, 2021

@stlaz @sttts @spadgett is there anything else needed? Or can we merge this one ?

@jhadvig
Copy link
Member

jhadvig commented Apr 28, 2021

@sg00dwin based on chat with @stlaz and @sttts we agreed that we will drop the ui_locales param check. Also we should add to the enhancement's text that the ConsoleTeam is reponsible for updating the oauth-server repo for any i18n related changes.

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

@sg00dwin added two comments with changes that are needed.

enhancements/console/internationalization.md Outdated Show resolved Hide resolved
enhancements/console/internationalization.md Show resolved Hide resolved
@sg00dwin sg00dwin force-pushed the login-template-internationalization branch 2 times, most recently from 7957900 to 9e908c3 Compare April 28, 2021 13:44
@sg00dwin
Copy link
Member Author

/retest

@jhadvig
Copy link
Member

jhadvig commented Apr 28, 2021

/test markdownlint

@sg00dwin sg00dwin force-pushed the login-template-internationalization branch 3 times, most recently from 8fbbb5b to 3190a41 Compare April 28, 2021 15:02
@sg00dwin sg00dwin force-pushed the login-template-internationalization branch from 3190a41 to 8844869 Compare April 29, 2021 04:22
@stlaz
Copy link
Contributor

stlaz commented Apr 29, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2021
@sttts
Copy link
Contributor

sttts commented Apr 29, 2021

/lgtm
/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 9dac088 into openshift:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants