Bug 1958080: CONSOLE-2535: Internationalize login page #71

jhadvig · 2021-03-25T14:07:32Z

@sg00dwin the locales will be available in locales.go where all the locales will be living in a form of key-value struct. They are hardcoded into the oauth-server since there is just a handful of them. We need to update all the login related templates so they contain the go template macros.

Had to add golang.org/x/text/language package to get helpers for parsing the Accept-Language header.

/assign @stlaz

relates to openshift/oauth-server#71

sg00dwin · 2021-03-29T19:24:48Z

@jhadvig
Related pr openshift/oauth-templates#15

relates to openshift/oauth-server#71

openshift/oauth-server#71

jhadvig · 2021-04-06T09:26:14Z

@stlaz we are ready for review. PTAL

jhadvig · 2021-04-07T08:52:47Z

/retest

stlaz

Our repositories require enhancements in order to merge new features. Can you please link an enhancement describing this change? It would make it simpler to reason about the alternatives to just getting "Accept-Language" header and about the timing of the translations.

stlaz · 2021-03-30T13:00:24Z

pkg/server/login/locales.go

+}
+
+var locale_en = map[string]string{
+	"": "",


what's the empty string key for?

stlaz · 2021-04-07T11:28:24Z

pkg/server/login/locales.go

+	switch langBase {
+	case language.English.String():
+		return locale_en
+	// Uncomment once given locale is available


When is that going to be?

Thats up to translations team that is providing those.
@sg00dwin any estimations ?

Full-time translators typically use specialized file formats/tools/editors, a repository of previously-translated strings, and the like. How is this going to interoperate with such workflows?

Is this really such a unique and special case that asking translators to provide Go patches, or setting up a special process to manually turn contributed translations into Go patches, is worthwhile and practical?

@mtrmac We have these tools in place for the main console repo. We thought that would be overkill here for a handful of messages that rarely change. We were planning to include whatever strings we need with the other console strings, and the console team would manage patching them when needed.

Thats up to translations team that is providing those.
@sg00dwin any estimations ?

The translation team doesn't provide estimated turn around times. Still waiting.

Translated strings are now included for each language.

stlaz · 2021-04-07T11:48:22Z

pkg/server/login/locales.go

+
+func getLocale(langBase string) map[string]string {
+	switch langBase {
+	case language.English.String():


How do I know this works?

Tested these methods with various Accept-Language header locally and it was working. Not sure why?

stlaz · 2021-04-07T11:51:46Z

pkg/server/login/login.go

@@ -128,6 +130,8 @@ func (l *Login) handleLoginForm(w http.ResponseWriter, req *http.Request) {
 		return
 	}

+	form.Locale = getLocale(getPrefferedLang(req.Header.Get("Accept-Language")))


What if I want to specify the language myself, manually? That's actually very often the case for myself, I default my system to English language, but prefer websites on the Czech web to be in Czech, which oftentimes I have to do manually on Czech pages since the autodetection is wrong.

I would expect some kind of this + a language button choice of which is stored in a cookie.

What if I want to specify the language myself, manually

if we have locale for that lang it will be used, if not we default to English

I would expect some kind of this + a language button choice of which is stored in a cookie.

This story only contains reading the lang preferences from the header and parsing the go template with appropriate locale.

@stlaz We have this for console itself. The problem is that it's stored as a user preference, which is tied to a specific user. Here you haven't logged in yet, so we can't load your user preferences.

We could also store the last selected language in localStorage and pass it as a query parameter to the login page, however. Then the query parameter would override Accept-Language if present. @jhadvig Do you might also checking a lng query parameter (to match what we support in console) first, then falling back to Accept-Language?

Note that "preferred" is misspelled here :)

jhadvig · 2021-04-07T12:22:58Z

@stlaz thanks for review.
@spadgett shall we incorporate an enhancement into the story ?

spadgett · 2021-04-07T12:40:07Z

@stlaz We can update our existing i18n enhancement to cover the login page:

https://github.com/openshift/enhancements/blob/master/enhancements/console/internationalization.md

spadgett · 2021-04-07T12:46:54Z

@jhadvig What are the messages we need? We can reach out to the globalization team to try to get them right away. We might already have "Username" and "Password" from other parts of the UI.

jhadvig · 2021-04-07T13:51:48Z

@spadgett its just a handful of strings, check

oauth-server/pkg/server/login/locales.go

Lines 41 to 50 in 6c96a14

    
           var locale_en = map[string]string{ 
        
           	"LogInToYourAccount": "Log in to your account", 
        
           	"Username":           "Username", 
        
           	"Password":           "Password", 
        
           	"LogIn":              "Log in", 
        
           	"LoginTitle":         "Login", 
        
           	"WelcomeTo":          "Welcome to", 
        
           	"LogInWith":          "Log in with", 
        
           	"Error":              "Error", 
        
           }

We might already have "Username" and "Password" from other parts of the UI.

I would rather have all of the string translated before enabling locale.

spadgett · 2021-04-07T14:03:47Z

@jhadvig I think we'll need to use i18next-parser comments in the console code base so that these messages are added automatically for translation.

// t('Log in to your account')
// t('Username')
// ... etc ...

Then our existing process for sending messages for translation will work. It would be a manual update to this repo when something changes, but I think that would be rare and something the console team can take on if @stlaz and @mtrmac agree. We should add this detail to our existing i18n enhancement.

We have "Username", "Password", and "Error" translated already, but not the other strings.

mtrmac · 2021-04-07T14:09:11Z

FWIW I’m just a curious observer. I just wanted to make sure that a viable process of contributing translations actually exists before settling on this mechanism.

spadgett · 2021-04-07T14:11:40Z

cc @rebeccaalpert

spadgett · 2021-04-07T14:13:18Z

pkg/server/login/templates.go

              </div>
            </form>
          </div>
        </main>
        <footer class="pf-c-login__footer">
-          <p>Welcome to OKD.</p>
+          <p>{{ .Locale.WelcomeTo }} OKD</p>


I think we lost the product name here.

did we ? I still see the OKD there, we just pulled it out of the double curly brackets.

Sorry, I missed that. Really you're not supposed to concat strings, instead making them a parameter in the message so the order of the words can change. That might be tricky here.

OKD should be in the double curly brackets.

@rebeccaalpert That doesn't work here since it's a Go template and not using i18next :/

Bleh. Aiko did raise this as an issue re: ordering.

I let them all know the context, but it's better if we can give them good inputs from the beginning.

Sorry, I missed that. Really you're not supposed to concat strings, instead making them a parameter in the message so the order of the words can change. That might be tricky here.

@jhadvig how will this need to be structured to handle translation changes to word order? eg. if locale_jp becomes OKD {{ .Locale.WelcomeTo }}

rebeccaalpert · 2021-04-07T15:02:03Z

@spadgett - @sgoodwin had sent these strings over to me and I'd manually created a PO file and sent it over to Terry's team on March 30 (you were CCed on that one). They assigned it out and said they'd give it back to us with the other strings from the last sprint.

If we can get the parser to pick these up, that would be great!

rebeccaalpert · 2021-04-07T15:17:48Z

pkg/server/selectprovider/templates.go

              <ul>
                {{ range $provider := .Providers }}
                  <li class="idp">
-                    <a href="{{$provider.URL}}" class="pf-c-button pf-m-secondary pf-m-block" title="Log in with {{$provider.Name}}">{{$provider.Name}}</a>
+                    <a href="{{$provider.URL}}" class="pf-c-button pf-m-secondary pf-m-block" title="{{ .Locale.LogInWith }} {{$provider.Name}}">{{$provider.Name}}</a>


The translators generally need full sentences. I think this may cause issues because it's broken up.

@rebeccaalpert so we will need to have atranslations for the Log In With string together with all the product variations ?

I think that's safer, yeah. Like Sam mentioned above sometimes the ordering of the sentence can change depending on the language.

The platform name is now included with the key string for the translators
"Welcome to {{platformTitle}}": "Welcome to {{platformTitle}}",
openshift/console#8587 (comment)

@sg00dwin @rebeccaalpert I'm not sure we can make that work in this context. The OAuth server won't know which platform name to use. Branding is done by replacing the entire login page template.

We might have to use string concatenation for now since we don't have the i18next library here.

relates to openshift/oauth-server#71 update README testing instructions.

jhadvig · 2021-05-05T15:01:02Z

@stlaz comments addressed.

relates to openshift/oauth-server#71 update README testing instructions.

Switch title separator from dash to dot for consistency with rest of console relates to openshift/oauth-server#71 update README testing instructions.

stlaz · 2021-05-06T10:58:00Z

pkg/server/locales/locales.go

+import "golang.org/x/text/language"
+
+func GetLocale(acceptLangHeader string) map[string]string {
+	langBase := getPreferredLang(acceptLangHeader)


this should also work and be slightly faster, right?

type Localization map[string]string var supportedLocalizations = map[string]Localization language.English.String(): locale_en, language.Chinese.String(): locale_zh, ... } func GetLocale(acceotedLangHeader string) Localization { locale, ok := supportedLocalizations[getPreferredLang(acceptLangHeader)] if !ok { return locale_en } return locale }

Can we index by Tags?

we can 👍

stlaz · 2021-05-06T11:05:56Z

pkg/server/locales/locales.go

+	matcher := language.NewMatcher(supportedLangs)
+	userPrefs, _, err := language.ParseAcceptLanguage(acceptLangHeader)
+	if err != nil {
+		// if error occurs, fallback to English


dump this error to debug logs

stlaz · 2021-05-06T11:10:06Z

pkg/server/locales/locales.go

+		return language.English.String()
+	}
+	tag, _, _ := matcher.Match(userPrefs...)
+	base, _ := tag.Base()


no language variations?

jhadvig · 2021-05-06T13:18:44Z

@stlaz thanks for review, comments addressed 👍

stlaz · 2021-05-06T13:27:09Z

pkg/server/locales/locales.go

+	matcher := language.NewMatcher(supportedLangs)
+	userPrefs, _, err := language.ParseAcceptLanguage(acceptLangHeader)
+	if err != nil {
+		klog.Warningf("Error parsing 'Accept-Language' header, falling back to English language: %v", err)


debug log please (lower verbosity)

stlaz · 2021-05-06T13:35:13Z

/lgtm

openshift-ci-robot · 2021-05-06T13:35:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, stlaz

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:

~~OWNERS~~ [stlaz]

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

openshift-bot · 2021-05-06T15:51:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-05-07T03:31:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-05-07T06:57:21Z

@jhadvig: This pull request references Bugzilla bug 1958080, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @yapei

In response to this:

Bug 1958080: CONSOLE-2535: Internationalize login page

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-05-07T06:58:30Z

@jhadvig: All pull requests linked via external trackers have merged:

openshift/oauth-server#71

Bugzilla bug 1958080 has been moved to the MODIFIED state.

In response to this:

Bug 1958080: CONSOLE-2535: Internationalize login page

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot assigned stlaz Mar 25, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2021

openshift-ci-robot requested review from smarterclayton and stlaz March 25, 2021 14:09

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request Mar 29, 2021

Translation of login templates console 2535 story

8a28cbc

relates to openshift/oauth-server#71

sg00dwin mentioned this pull request Mar 29, 2021

CONSOLE-2535: Translation of login templates openshift/oauth-templates#15

Merged

jhadvig force-pushed the CONSOLE-2535 branch from de8cfa7 to 298cd4c Compare March 31, 2021 13:52

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request Mar 31, 2021

Translation of login templates console 2535 story

45b61c0

relates to openshift/oauth-server#71

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request Mar 31, 2021

Translation of login templates console 2535 story

8ab728e

relates to openshift/oauth-server#71

sg00dwin added a commit to sg00dwin/cluster-authentication-operator that referenced this pull request Mar 31, 2021

OCP oauth translations console-2535

c2dd5d5

openshift/oauth-server#71

sg00dwin mentioned this pull request Mar 31, 2021

Bug 1955749: OCP oauth translations for login pages openshift/cluster-authentication-operator#428

Merged

jhadvig force-pushed the CONSOLE-2535 branch 2 times, most recently from 5efca2a to 6c96a14 Compare April 6, 2021 09:03

jhadvig changed the title ~~[WIP] CONSOLE-2535: Internationalize login page~~ CONSOLE-2535: Internationalize login page Apr 6, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2021

stlaz suggested changes Apr 7, 2021

View reviewed changes

spadgett reviewed Apr 7, 2021

View reviewed changes

rebeccaalpert reviewed Apr 7, 2021

View reviewed changes

spadgett mentioned this pull request Apr 7, 2021

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

Merged

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request May 5, 2021

Translation of login templates console 2535 story

c129c59

relates to openshift/oauth-server#71 update README testing instructions.

jhadvig force-pushed the CONSOLE-2535 branch from 19aea15 to 39cbde2 Compare May 5, 2021 15:00

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request May 5, 2021

Translation of login templates console 2535 story

9b728ec

relates to openshift/oauth-server#71 update README testing instructions.

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request May 5, 2021

Translation of login templates console 2535 story

25e8775

relates to openshift/oauth-server#71 update README testing instructions.

sg00dwin added a commit to sg00dwin/oauth-templates that referenced this pull request May 5, 2021

Translation of login templates console 2535 story

78d99de

relates to openshift/oauth-server#71 update README testing instructions.

Add golang.org/x/text in order to get the language package

e82c73c

jhadvig force-pushed the CONSOLE-2535 branch 2 times, most recently from 5e13b1e to 903a9fd Compare May 5, 2021 15:35

stlaz reviewed May 6, 2021

View reviewed changes

jhadvig force-pushed the CONSOLE-2535 branch from 903a9fd to 3dd26ca Compare May 6, 2021 13:17

stlaz reviewed May 6, 2021

View reviewed changes

Internationalize login, error and selectprovider page

30f1a4d

jhadvig force-pushed the CONSOLE-2535 branch from 3dd26ca to 30f1a4d Compare May 6, 2021 13:34

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2021

jhadvig changed the title ~~CONSOLE-2535: Internationalize login page~~ Bug 1958080: CONSOLE-2535: Internationalize login page May 7, 2021

openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label May 7, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 7, 2021

openshift-ci bot requested a review from yapei May 7, 2021 06:57

openshift-merge-robot merged commit 3da20bd into openshift:master May 7, 2021

openshift-ci bot mentioned this pull request May 19, 2021

Bug 1958080: Add missing error page strings #77

Merged

Bug 1958080: CONSOLE-2535: Internationalize login page #71

Bug 1958080: CONSOLE-2535: Internationalize login page #71

Conversation

jhadvig commented Mar 25, 2021 • edited Loading

sg00dwin commented Mar 29, 2021

jhadvig commented Apr 6, 2021

jhadvig commented Apr 7, 2021

stlaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Apr 7, 2021

spadgett commented Apr 7, 2021

spadgett commented Apr 7, 2021

jhadvig commented Apr 7, 2021

spadgett commented Apr 7, 2021 • edited Loading

mtrmac commented Apr 7, 2021

spadgett commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebeccaalpert Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

sg00dwin Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

rebeccaalpert commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented May 6, 2021

Choose a reason for hiding this comment

stlaz commented May 6, 2021

openshift-ci-robot commented May 6, 2021

openshift-bot commented May 6, 2021

openshift-bot commented May 7, 2021

openshift-ci bot commented May 7, 2021 • edited Loading

openshift-ci bot commented May 7, 2021 • edited Loading

jhadvig commented Mar 25, 2021 •

edited

Loading

spadgett commented Apr 7, 2021 •

edited

Loading

rebeccaalpert Apr 7, 2021 •

edited

Loading

sg00dwin Apr 8, 2021 •

edited

Loading

rebeccaalpert commented Apr 7, 2021 •

edited

Loading

openshift-ci bot commented May 7, 2021 •

edited

Loading

openshift-ci bot commented May 7, 2021 •

edited

Loading