1576170 - Land Sign in feature strings for Onboarding & Activity Stream

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, task, P1)

Description

[Vlad Baicu]

The first step in the implementation of the new Sign In buttons feature for Onboarding & Activity Stream is to land the new strings in order to allow the L10n team to start working on the translation process.

Comment 1

[Vlad Baicu]

Attachment: Bug 1576170 - Sign in strings for Onboarding & Activity Stream. r=petru

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93ef1d6383d6
Sign in strings for Onboarding & Activity Stream. r=petru

Comment 3

[Delphine Lebédel [:delphine - Use needinfo]]

Localization notes should appear above each string in question, not under. Please update accordingly. Thanks!

Comment 4

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/93ef1d6383d6

Comment 5

[Chris Peterson [:cpeterson]]

Adding [fennec68.1.x] whiteboard tag because we'll want to uplift these new strings to Fennec ESR 68.1. But we should first fix the localization notes problem Delphine reported in comment 3.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #3)

Localization notes should appear above each string in question, not under. Please update accordingly. Thanks!

And these landed already in m-c.

To be honest, it's quite wrong to preland strings and not having someone from localization to double check that everything looks good.

Comment 7

[Vlad Baicu]

Attachment: Bug 1576170 - Relocate the localization note above the strings for consistency. r=flod

Comment 8

[Vlad Baicu]

Thank you all for your input. We will make sure to add someone from localization as a reviewer when we change something in this area again.

FYI - During our meeting with Chris, he let us know there is a possibility that we might have to add even more strings for this feature.

Comment 9

[Delphine Lebédel [:delphine - Use needinfo]]

That's a bit unfortunate as these strings have already landed in Pontoon, so they are going to be localized by our volunteers at this point (and they won't have localization comment to refer to).
Other than new strings coming in - are we expecting these strings to change a lot at this point? thanks

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b36ad9d6c8a
Relocate the localization note above the strings for consistency. r=flod

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9b36ad9d6c8a

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #9)

That's a bit unfortunate as these strings have already landed in Pontoon, so they are going to be localized by our volunteers at this point (and they won't have localization comment to refer to).
Other than new strings coming in - are we expecting these strings to change a lot at this point?

I know at least two of these Fennec strings will be changed, though I won't have confirmation on the new wording until Wednesday. I'll post the confirmed strings in a new bug linked to this bug.

If we need to change strings that have already landed, should we land the changed strings with new string IDs? If we revise an changed string in place in the android_strings.dtd file, will Pontoon recognize that a string has changed and needs to be localized again?

Comment 13

[Delphine Lebédel [:delphine - Use needinfo]]

Yes, please update the string IDs so localizers are notified and see that there is a change to the strings they need to tend to. Thank you!

Comment 14

[Vlad Baicu]

Attachment: Bug 1576170 - Onboarding Sign up strings. r=flod

Comment 15

[Vlad Baicu]

Reopening this as we have one more patch needed for this feature. We initially thought that the panels would be configurable through Leanplum. After disscussing the issue with Chris we've determined that the best course of action would be to change the Onboarding panels directly in the app. Out of all the strings available in these 3 new panels I have noticed that we are only missing 3 of them. The rest of which are already available and translated.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f055322f882
Onboarding Sign up strings. r=delphine

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0f055322f882

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request ESR uplift if that's the intent here? Thanks!

Comment 19

[Vlad Baicu]

Comment on attachment 9088165 [details]
Bug 1576170 - Relocate the localization note above the strings for consistency. r=flod

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: These string patches are the baseline for the new sign in button features.
• User impact if declined: The new sign up buttons cannot be implemented.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are merely adding new strings at this point.
• String or UUID changes made by this patch: firstrun_signin_button2, activity_stream_signin_title, activity_stream_signin_description, activity_stream_signin_button, firstrun_sync_message3, firstrun_sendtab_title, firstrun_sendtab_message

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9087728 [details]
Bug 1576170 - Sign in strings for Onboarding & Activity Stream. r=petru

Approved for Fennec 68.2b4.

Comment 21

[Cosmin Sabou [:CosminS]]

This conflicts with Bug 1577868 when trying to uplift https://bugzilla.mozilla.org/show_bug.cgi?id=1577868#c5

Comment 22

[Eliza Balazs]

Verified as fixed on 71.0a1 (2019-09-25) with Samsung Galaxy Tab S3 (Android 8; tablet), Prestigio Grace X5 (Android 4.4.2), Nexus 6P (Android 8.1.0).
The strings from https://bugzilla.mozilla.org/show_bug.cgi?id=1570880 and https://bugzilla.mozilla.org/show_bug.cgi?id=1570878 are correct.
I will mark this as verified on Firefox 71.

Comment 23

[Vlad Baicu]

We've begun the process to land all of this feature's patches on ESR, the landing order to these diffs should be: D43245, D43468, D44388, D44817, D45528, D46608, D47111

Comment 24

[Vlad Baicu]

Please let me know if you encounter any other conflicts or issues

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/903a5ee0bdbb
https://hg.mozilla.org/releases/mozilla-esr68/rev/16518db36570
https://hg.mozilla.org/releases/mozilla-esr68/rev/cd7b46c03966

Comment 26

[Cosmin Sabou [:CosminS]]

All went smooth. Thanks Vlad for the uplift order.
Th push with the bugs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&resultStatus=testfailed%2Cbusted%2Cexception&revision=749f9c937ab9e821a4dd89d9cde2da20f96a199a

Comment 27

[Cosmin Sabou [:CosminS]]

Vlad, could you check these lint failures? https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&resultStatus=testfailed%2Cbusted%2Cexception&revision=749f9c937ab9e821a4dd89d9cde2da20f96a199a&selectedJob=269083543

Comment 28

[Chris Peterson [:cpeterson]]

Chenxia, question about telemetry data review:

This bug adds some new FxA Sign-in buttons to Fennec's Onboarding and the New Tab screens. The buttons add new code that triggers existing FxA UI telemetry events (with new payload values indicating the new buttons' names: "awesomescreen-signup", "awesomescreen-signin", and "awesomescreen-signup-dismiss").

Does this new code need a data review for these new payload values? This code hasn't shipped yet. It just landed in Fennec Nightly and Beta today.

Here is the changelist that added the new UI telemetry event triggers:

https://hg.mozilla.org/releases/mozilla-esr68/diff/4396e184de5d1cd29566231b70d7da85f6b52743/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/SignInRow.java

Comment 29

[Vlad Baicu]

Submitted a patch in bug 1585132 for the lint failures. Thanks

Comment 30

[Chris Peterson [:cpeterson]]

chutten, I have a telemetry data review question in comment 28 above. liuche is on PTO so I'm redirecting this needinfo to you. :)

Comment 31

[Chris H-C :chutten]

I'm assuming that there's an existing review for the other payload values. Are these new payload values covered in the description of the collection mentioned in the data review? And do they adhere to the same restrictions? (population, expiry, ownership, etc)

If yes: proceed under the existing review. If no, or unsure: let's have ourselves a quick Data Collection Review.

Comment 32

[Vlad Baicu]

For the signup buttons feature we've changed and added telemetry UI events, specifically:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPanel.java#62

I was unable to get the dxr esr link for the other two, but I can reference the link of the landed diff - https://phabricator.services.mozilla.com/D46608#change-AGaUmfeY8L2O
Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.BUTTON, "awesomescreen-signup"); Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.BUTTON, "awesomescreen-signin");

Do UI events warrant a telemetry data review? I don't see anything of the sort on older bugs that added them (e.g. bug 1366664, bug 1199859)

Comment 33

[Chris H-C :chutten]

All new or expanded data collection in Firefox (and most other moz products) requires Data Collection Review. The modern Data Review policy postdates those earlier bugs by a few months, which might explain why they don't have any.

This is an opportunity to bring these collections "up to code" if you'd like. But at the very least there needs to be review for these new pieces. You can find the policy here: https://wiki.mozilla.org/Firefox/Data_Collection

Comment 34

[Chris Peterson [:cpeterson]]

(In reply to Chris H-C :chutten from comment #31)

I'm assuming that there's an existing review for the other payload values. Are these new payload values covered in the description of the collection mentioned in the data review? And do they adhere to the same restrictions? (population, expiry, ownership, etc)

If yes: proceed under the existing review. If no, or unsure: let's have ourselves a quick Data Collection Review.

I filled out the Data Collection Review form just to be safe. We can use Fennec bug 1580649 for this Data Collection Review. I attached it to that bug and set the data-review? flag for you and Chenxia.

Comment 35

[Eliza Balazs]

Verified as fixed on ESR 68.2b7 with Samsung Galaxy Tab S3 (Android 8; tablet), OnePlus 5T (Android 9), Prestigio Grace X5 (Android 4.4.2).
The strings from https://bugzilla.mozilla.org/show_bug.cgi?id=1570880 and https://bugzilla.mozilla.org/show_bug.cgi?id=1570878 are correct.
I will mark this as Verified.