1516617 - Port bug 1507595: Convert about:support to Fluent

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Richard Marti (:Paenglab)]

Bug 1507595 is ready to land and we need to follow to not break our tests.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1516617-aboutSupport-fluent.patch

This is a port of the m-c patch. I created it through diffing.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: 1516617-aboutSupport-part2.patch

This patch is optional to make our strings also fluent and we don't mix the two localization technologies.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #0)
> Bug 1507595 is ready to land and we need to follow to not break our tests.
Thanks, I'll keep an eye on it. Did you apply the M-C patches? I might do that otherwise and do a try run.

Comment 4

[Richard Marti (:Paenglab)]

Yes, tested with the m-c patches. This are the errors I get:

10:29:13.192 Missing translations in en-US: none Localization.jsm:197:9
10:29:13.191 ReferenceError: reference to undefined property "data-l10n-args"[Learn More] aboutSupport.js:897:1 

The first must be missing in m-c ftl file as before much more where missing and with the last version are fixed.
The second I don't know why. The files are in sync there.

Comment 5

[Jorg K (CEST = GMT+2)]

OK, let's see:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a37f683e90d316c5840cbac6b33304494960f29
and using this
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=028538596afea433b12c1313cecdbcd0fd0cdd9e

I'll build it myself to check the errors you mentioned.

Comment 6

[Jorg K (CEST = GMT+2)]

We'll need to address the linting errors:
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/about-support/content/aboutSupport.js:298:17 | Async method 'graphics' has a complexity of 38. (complexity)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/about-support/content/aboutSupport.js:888:9 | Unexpected named function '$_new'. (func-names)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/about-support/content/aboutSupport.js:916:12 | Unexpected named function '$_append'. (func-names)

Comment 7

[Richard Marti (:Paenglab)]

It seems m-c doesn't lint like we in this files.

The first is probably simple with inserting /* eslint-enable complexity */

But what should I do with the two others?

Comment 8

[Jorg K (CEST = GMT+2)]

I'll take a look at part of the review later today. Bug 1507595 hasn't relanded, so it's not extremely urgent, just "normal" urgent ;-)

Comment 9

[Richard Marti (:Paenglab)]

Attachment: 1516617-aboutSupport-fluent.patch ESLint fixed

Fixed the ESLint failures.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033517 [details] [diff] [review]
1516617-aboutSupport-part2.patch

Yes, let's go with this. So in the end, we'll get all the strings re-translated? I though M-C had some sort of automated process to update the translations.

Comment 11

[Jorg K (CEST = GMT+2)]

One test failure:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-tabs/test-about-support.js | test-about-support.js::test_display_about_support
I'll take a look later on.

Also, the linting problem isn't fixed:
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/about-support/content/aboutSupport.js:299:17 | Async method 'graphics' has a complexity of 38. (complexity)

I think you need to re-enable the complexity later with:
/* eslint-enable complexity */

Comment 12

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #10)
> Comment on attachment 9033517 [details] [diff] [review]
> 1516617-aboutSupport-part2.patch
> 
> Yes, let's go with this. So in the end, we'll get all the strings
> re-translated? I though M-C had some sort of automated process to update the
> translations.

No, we'd need to do something like this: https://hg.mozilla.org/try/file/4a37f683e90d/python/l10n/fluent_migrations/bug_1507595_aboutsupport.py

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #11)
> One test failure:
> TEST-UNEXPECTED-FAIL |
> /builds/worker/workspace/build/tests/mozmill/content-tabs/test-about-support.
> js | test-about-support.js::test_display_about_support
> I'll take a look later on.
> 
> Also, the linting problem isn't fixed:
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/comm/mail/components/about-support/content/
> aboutSupport.js:299:17 | Async method 'graphics' has a complexity of 38.
> (complexity)
> 
> I think you need to re-enable the complexity later with:
> /* eslint-enable complexity */

It's enabled in line 664.

Comment 14

[Richard Marti (:Paenglab)]

I meant in the new patch I applied which is newer than the try.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #14)
> I meant in the new patch I applied which is newer than the try.
Well, with the new patch eslint passes locally:
$ ../mach eslint mail/components/
? 0 problems (0 errors, 0 warnings)
Did yo run it locally?

So there's only the test failure left to look at and the problem mentioned in comment #4. Later today ... ;-)

Comment 16

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #15)
> (In reply to Richard Marti (:Paenglab) from comment #14)
> > I meant in the new patch I applied which is newer than the try.
> Well, with the new patch eslint passes locally:
> $ ../mach eslint mail/components/
> ? 0 problems (0 errors, 0 warnings)
> Did yo run it locally?

Yes. Before my changes I saw the same errors as try.

> So there's only the test failure left to look at and the problem mentioned
> in comment #4. Later today ... ;-)

Correct.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #4)
> 10:29:13.192 Missing translations in en-US: none Localization.jsm:197:9
> 10:29:13.191 ReferenceError: reference to undefined property
> "data-l10n-args"[Learn More] aboutSupport.js:897:1 
I see the first one, but not the second, I guess you have "extra" JS errors switched on. I asked in bug 1507595 comment #9. Let's ignore this for now and get the test fixed.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033524 [details] [diff] [review]
1516617-aboutSupport-fluent.patch ESLint fixed

Thanks, this looks like a lot of work. The test failure is due to a timing issue. I'll attach a temporary fix. Aceman is away, and I don't know how to do it properly, but we can back that out later and fix it properly.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1516617-await-l10n.patch

We need this as part 3 to get the test to pass. Copied from:
https://searchfox.org/comm-central/rev/594e7486df6cc320d60069a4e6462e31479a56af/mail/test/mozmill/downloads/test-about-downloads.js#62

That's an example of how to wait for a promise to resolve.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033568 [details] [diff] [review]
1516617-await-l10n.patch

First reviewer wins.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: 1516617-aboutSupport-fluent.patch

Patch with fixed reference to undefined property "data-l10n-args" like it landed on m-i.

Carrying the r+ from previous patch

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9033568 [details] [diff] [review]
1516617-await-l10n.patch

Review of attachment 9033568 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I imagine this is the first of many times this particular code gets written.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e1df268e0c14
Port bug 1507595: Convert about:support to Fluent. r=jorgk
https://hg.mozilla.org/comm-central/rev/af79aa826c3a
Port bug 1507595: Convert our own about:support strings to Fluent. r=jorgk
https://hg.mozilla.org/comm-central/rev/dde486474a5e
Add waiting for L10n to open_about_support(). r=darktrojan DONTBUILD