Closed Bug 1576170 Opened 4 months ago Closed 3 months ago

Land Sign in feature strings for Onboarding & Activity Stream

Categories

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

Unspecified
Android
task

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ verified
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- verified

People

(Reporter: vlad.baicu, Assigned: vlad.baicu)

References

Details

(Whiteboard: [fennec68.2])

Attachments

(3 files)

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.

Keywords: checkin-needed

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93ef1d6383d6
Sign in strings for Onboarding & Activity Stream. r=petru

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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.

Whiteboard: [fennec68.1]

(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.

Flags: needinfo?(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.

Flags: needinfo?(vlad.baicu)

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

Flags: needinfo?(cpeterson)
Keywords: checkin-needed

Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b36ad9d6c8a
Relocate the localization note above the strings for consistency. r=flod

Keywords: checkin-needed

(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?

Flags: needinfo?(cpeterson)

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!

Depends on: 1577868

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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 4 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: Firefox 70 → Firefox 71

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

Flags: needinfo?(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
Flags: needinfo?(vlad.baicu)
Attachment #9088165 - Flags: approval-mozilla-esr68?
Attachment #9087728 - Flags: approval-mozilla-esr68?
Attachment #9092071 - Flags: approval-mozilla-esr68?

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

Approved for Fennec 68.2b4.

Attachment #9087728 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9088165 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9092071 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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.

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

Flags: needinfo?(csabou)

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

Regressions: 1585132

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

Flags: needinfo?(liuche)

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

Flags: needinfo?(vlad.baicu)

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

Flags: needinfo?(liuche) → needinfo?(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.

Flags: needinfo?(chutten)

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)

Flags: needinfo?(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

Flags: needinfo?(chutten)

(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.

Depends on: 1580649
Whiteboard: [fennec68.1] → [fennec68.2]

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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.