Closed Bug 1391210 Opened 2 years ago Closed 2 years ago

[Shield][Onboarding Tour] Create a shield-compatible add-on for Onboarding

Categories

(Shield :: Shield Study, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rexboy, Assigned: rexboy, NeedInfo)

References

Details

Attachments

(3 files, 5 obsolete files)

Create a shield-study compatible add-on for Onboarding so we can do Shield study by it.

Actually we may have another way of doing system add-on uplift per Matt's reply, but given the schedule and detail unknown yet I'd like to do possible packaging works in advance.

The ongoing works is on https://github.com/mozilla/shield-onboarding thanks to Gregg's example. It is now a full-featured runnable xpi, and can choose variations based on Cindy's proposal. The remaining works should be the telemetry part I think.
Attached file WIP xpi file (obsolete) —
WIP xpi file built from the repository.

It can be run under firefox 55 beta from about:debugging.
Duplicate of this bug: 1390450
your signing needed
Flags: needinfo?(mkelly)
Flags: needinfo?(mcooper)
Since this study is intended for release users, it needs approval from QA before we sign it. You can request a test plan and execution from the product integrity group via the PI request process: https://mana.mozilla.org/wiki/display/PI/PI+Request

Also, is the attached XPI ready for signing? It's marked as a work in progress.
Flags: needinfo?(mkelly)
Flags: needinfo?(mcooper)
Attached file Extension XPI (obsolete) —
Updated with telemetry added. I think this is ready, but not sure if I'm doing right to met Shield's requirement.

Also, I let the addon pref-off upon installation so no one will be affected by it until Shield pref-on it and restart the plugin. But I don't know how Shield do it specifically so it needs Shield team's confirmation too.

See repository here:
https://github.com/mozilla/shield-onboarding
Gregg:
As described above, this extension is modified based on the example you gave me earlier. Could you help confirm if it's Shield-ready, or instruct me any processes I missed.


Thank you!
Flags: needinfo?(glind)
(In reply to Michael Kelly [:mkelly,:Osmose] from comment #4)
> Since this study is intended for release users, it needs approval from QA
> before we sign it. You can request a test plan and execution from the
> product integrity group via the PI request process:
> https://mana.mozilla.org/wiki/display/PI/PI+Request
> 
> Also, is the attached XPI ready for signing? It's marked as a work in
> progress.
Thanks for the quick reply. We had some conversion with Matt before about doing QA so we'll sync with him about current process first.
Matt, do you have any updates for setting up contact with QA?
Please tell me if we need further actions like sending PI Requests. Thank you!
Flags: needinfo?(mgrimes)
That's correct. You'll need to go through the PI request process for formal QA.
Flags: needinfo?(mgrimes)
> Also, I let the addon pref-off upon installation so no one will be affected
> by it until Shield pref-on it and restart the plugin. But I don't know how
> Shield do it specifically so it needs Shield team's confirmation too.

This method will be a bit unreliable if a default branch study is used (which is most common). The study pref will be flipped when Shield starts up during each Firefox startup, since we can't change default branch prefs permanently. Since Shield is also an add-on, that could happen after your add-on has run.

I'd recommend one of two options:

1) Modify your code to both check the pref on startup (as you are doing now), and also watch the pref for changes, using `Services.prefs.addObserver`. When the pref changes from false to true, you should enable the feature, and disable the feature when the pref changes from true to false.

2) Use a user branch study. This is only appropriate if the preference you are changing for the study is not used by any other code outside of your add-on. If the preference is a pre-existing preference used in other parts of Firefox, this risks erasing user choice, which we'd like to avoid.
Attachment #8898237 - Attachment is obsolete: true
Attached file Extension API v2 (obsolete) —
Updated per comment 10.

The repository is here:
https://github.com/mozilla/shield-onboarding
(In reply to Mike Cooper [:mythmon] from comment #10)
> 1) Modify your code to both check the pref on startup (as you are doing
> now), and also watch the pref for changes, using
> `Services.prefs.addObserver`. When the pref changes from false to true, you
> should enable the feature, and disable the feature when the pref changes
> from true to false.

Thanks for your advise. I just did that with the option 1 so it'll be enabled once detecting pref-on.

What I'm not quite sure is if we can call studyUtils.firstSeen() at first install then,  because that seem to be apply to all users rather than just Shield study attendees if we go through system add-on.
Mike do you have more informations on this?
Flags: needinfo?(mcooper)
And there are some questions I also posted on Shield channel. Many Shield people is on PTO and I'm not sure who can ask so let me post them here too:

Matt suggests us to go through system add-on update with shield to pref it on. but I don't really sure how the process goes. Does anybody know:
- Is it enough to make an XPI and make it pass QA test, then we can send it to the system add-on update? Or we need to do something like uplifting all the codes into version 55 source code?
- Is there any people in responsible for checking if I put shield related things correctly in the add-on?
- Should we get the XPI reviewed before starting QA test?
Responding to comment 12:

I think you should only call studyUtils.firstSeen() after the preference is true. I'd imagine a design very similar to what you have before in that the add-on does very little if the pref is false. Previously you did nothing. In the design I proposed you would do exactly one thing: register a listener for the pref to change. Any other initialization tasks should probably occur only when the pref is true.

Responding to comment 13:

- It should be enough to make an XPI. That's the artifact that we'll send to users as a system add-on update. Uplift won't be needed. You should make sure that all versions of Firefox you care about are covered by either a system add-on update or code in tree however, because system add-ons are removed between versions.

- We don't have a dedicated process for this, but I would be happy to look over the add-on to see if there are any surprising-to-Shield things to watch out for.

- Review and QA can happen in parallel.
Flags: needinfo?(mcooper)
Attached file Extension XPI V3 (obsolete) —
Thanks a lot for your response Mike!

I've updated the XPI; now it calls studyUtils.firstSeen() only on first time pref-on after booting. Not sure if it's fine for ship.
I guess I've addressed all the things I needed so can you take a look on it? Not sure if that's okay to just send such a whole package for you to review so tell me if I can do anything to get the process more smooth.

And another question is, for QA testing, I'm not quite sure how QA can test behaviors against different variations because the variation choosing seems to be happened internally inside Shield library. Is that a thing we need to get concerned about? We've already got a QA contact and sent out some details.

Again the repository of the XPI locates here:
https://github.com/mozilla/shield-onboarding
Attachment #8899398 - Attachment is obsolete: true
Attachment #8900623 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Some notification:
the perf for enabling the XPI is now browser.onboarding.shieldstudy-enabled.
KM, this add-on doesn't have any thing I'm concerned about with respect to Shield.

One issue that I'd bring up is that the XPI file includes the tests for the add-on. I think those should not be included in the XPI we use for the study.
Flags: needinfo?(mcooper)
Hi Rex,
Cristian has completed the test and here is some test results. Kindly help address them for shipping on 9/4 US time. 
https://public.etherpad-mozilla.org/p/onboarding_shield_QA

Thanks.
Cindy
Flags: needinfo?(rexboy)
Attached file Extension XPI V4 (obsolete) —
Revised v4.

Things fixed are:
1. Remove test codes.
https://github.com/mozilla/shield-onboarding/commit/38525297cb643976e0a2750e6702b651004d8151

2. Fixed the issue of sync tour not opening sign-in page with email.
https://github.com/mozilla/shield-onboarding/commit/e1b021efc4f7df246d17a8fee5c6d899f4f46111

We're discussing QA issues on etherpad:
https://public.etherpad-mozilla.org/p/onboarding_shield_QA
Attachment #8901141 - Attachment is obsolete: true
Attached file Extension XPI V5
Workaround the problem that about:home and about:newtab in different fonts.

This is quite weird because they both use default system font just with a font: inherit for button in about:Home... (And that should inherit default system font as well!)
Workaround it by changing it back to a div for now since we're not doing a11y in this study.
https://github.com/mozilla/shield-onboarding/commit/596469ebc5e1cce566440560f0d535bad22d1a62
Attachment #8903517 - Attachment is obsolete: true
Discussed with QA yesterday and Cristian has come up a first report. Many thanks to Cristian.
Based on the status he proposed we may need to have a meeting to discuss further progress.
Flags: needinfo?(rexboy)
Comment on attachment 8903557 [details]
Extension XPI V5

Mike: Please take a look at the updated patch. Tests has been removed.


I'm not sure given the date from QA (see mail) what will our shipping plan be though. Maybe we can have a short meeting discussing about that.
Flags: needinfo?(mcooper)
Update: We have already clarified the pending bugs on the etherpad and make the XPI ready to ship.
Here's a list for the telemetry data that the XPI above would send during the study, for your reference.
https://docs.google.com/document/d/1rnDdBbsevwcDin0R5KmeCx4Byt7Hs2jMP7Fbf6t2Wxw/edit

Given the XPI is checked and ready, Cyndi has already send the intent-to-ship mail for shipping it on Sep. 11.
Gregg or Matt may you help the remaining step to get it into system add-on update, and enable the opt-out shield study?
Thanks a lot!
Flags: needinfo?(mgrimes)
I looked over the changes from v3 to v5, and it looks ok to me. I've signed the attachment marked as v5 as a Mozilla extension, so it can be used even when legacy extensions are blocked. This file is ready to be used in an opt-out study.
Flags: needinfo?(mcooper)
Sounds good!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Jason, could you please sign this addon:
https://bugzilla.mozilla.org/attachment.cgi?id=8903557
Flags: needinfo?(jthomas)
Please see attached.
Flags: needinfo?(jthomas)
Cristian, Osmose, is this sufficient for signoff? Do we need any more testing?
Assignee: nobody → rexboy
Flags: needinfo?(mkelly)
Flags: needinfo?(cristian.comorasu)
I can sign off on testing the Balrog rule changes, specifically on Firefox 55 on MacOS. I think that's sufficient for the Balrog changes (no platform-specific rules involved here).

We still need feature testing sign-off from QA to confirm that the functionality of the add-on itself has been tested.
Flags: needinfo?(mkelly)
(In reply to Michael Kelly [:mkelly,:Osmose] from comment #31)
> I can sign off on testing the Balrog rule changes, specifically on Firefox
> 55 on MacOS. I think that's sufficient for the Balrog changes (no
> platform-specific rules involved here).
> 
> We still need feature testing sign-off from QA to confirm that the
> functionality of the add-on itself has been tested.

We've gone through most of the features with QA (see comment 19). Cristian has sent a report to both Shield team and Onboarding team; and we have discussed those issues in a meeting last week before Mike signing-off it.
I retested with the XPI from comment 28 and added the results in the etherpad ( https://public.etherpad-mozilla.org/p/onboarding_shield_QA lines 98-111 ) There are significant improvements. I think this is sufficient for the release.
Flags: needinfo?(cristian.comorasu)
Sounds good to me, let's go ahead and ship when you are ready.
This is in Balrog and ready for sign off
Flags: needinfo?(lhenry)
Flags: needinfo?(cristian.comorasu)
Liz signed off for relman, Cornel just did for qe, so we're now shipping the following at 100% for release 55:

    "clicktoplay-rollout@mozilla.org-1.3",
    "e10srollout@mozilla.org-2.05",
    "followonsearch@mozilla.com-0.9.3",
    "onboarding@mozilla.org-0.1",
    "screenshots@mozilla.org-10.12.0"
Flags: needinfo?(lhenry)
Flags: needinfo?(cristian.comorasu)
Flags: needinfo?(mgrimes)
You need to log in before you can comment on or make changes to this bug.