Improvements sourced from several shield add-on repositories #49

motin · 2018-02-15T16:14:37Z

Many improvements to the template. Primarily sourced from:

Version bump to 1.3.0

The idea of this PR is to, in one big swoop, bring this template forward to reflect today's improvements and desired changes discovered while developing the above add-ons, as well as make it easier (via npm run format addition and linting improvements) to merge and backport code between add-ons and the template (with less differences only from code style and whitespace).

motin · 2018-02-15T16:21:24Z

This PR currently includes commits from several open PRs in this repo. They can be approved in this order: First #37 then #41. Don't approve #35 (since it's change is already included in #41). Approve #39 and #30 in any order.
After that, I'll rebase this PR on top of the current master.

gregglind · 2018-02-15T16:30:57Z

Thanks for these. I am PTO until Tuesday.

…

On Thu, Feb 15, 2018 at 9:21 AM Fredrik Wollsén ***@***.***> wrote: This PR currently includes commits from several open PRs in this repo. They can be approved in this order: First #37 <#37> then #41 <#41>. Don't approve #35 <#35> (since it's change is already included in #41 <#41>). Approve #39 <#39> and #30 <#30> in any order. After that, I'll rebase this PR on top of the current master. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAj63gMLoyfCAqOQ7WqtBIIWR3XmPfks5tVFmEgaJpZM4SHGSV> .

pdehaan · 2018-02-15T19:47:48Z

.eslintrc.js

-    "eqeqeq": "error",
-    "indent": ["warn", 2, {SwitchCase: 1}],
+    eqeqeq: "error",
+    indent: ["warn", 2, { SwitchCase: 1 }],
    "mozilla/no-aArgs": "warn",
    "mozilla/balanced-listeners": 0,


nit: this should be "off", for consistency.

pdehaan · 2018-02-15T19:48:10Z

.eslintrc.js

-    "eqeqeq": "error",
-    "indent": ["warn", 2, {SwitchCase: 1}],
+    eqeqeq: "error",
+    indent: ["warn", 2, { SwitchCase: 1 }],
    "mozilla/no-aArgs": "warn",
    "mozilla/balanced-listeners": 0,
    "no-console": "warn",
    "no-shadow": ["error"],


nit: this could just be "error", and doesn't need to be wrapped in an array.

pdehaan · 2018-02-15T19:49:57Z

DEV.md

+
+Repositories that should no longer be used as templates for new studies:
+
+[https://github.com/gregglind/template-shield-study]() - The incubation repo for the updated structure and contents of this repo, implemented in late 2017.


This looks "off". Not sure if we should duplicate the URL in the parens, or just use the <https://github.com/gregglind/template-shield-study> syntax since the URL and label are the same.

Same w/ the hello world example link below.

pdehaan · 2018-02-15T19:50:51Z

README.md

-During FIRST INSTALL and EVERY OTHER STARTUP, users see:
-
- a 'toolbar button' (webExtension BrowserAction)
+**Note**: This is toy / demonstration [Shield Study](https://wiki.mozilla.org/Firefox/Shield/Shield_Studies) Legacy Addon. Use this as a template for yours


nit "Add-on", for consistency.

pdehaan · 2018-02-15T19:56:18Z

TESTPLAN.md


-    Click on the 'x' button.
+   * notice closes
+   * addon uninstalls


pdehaan · 2018-02-15T19:56:45Z

TESTPLAN.md

-    1.  Open a 2nd firefox window.
-    2.  Close the initial window.
+   * notice closes
+   * addon uninstalls


nit: add-on

pdehaan · 2018-02-15T19:57:16Z

TESTPLAN.md

---
-## Helper code and tips
+   * notice closes
+   * addon uninstalls


nit: add-on

pdehaan · 2018-02-15T19:58:04Z

TESTPLAN.md

-}
-
-printPings()
+Example log output after installing the addon:


nit: add-on (probably worth a global search and see what needs to be updated).

motin · 2018-02-15T20:19:46Z

Btw, this PR also fixes #38

biancadanforth · 2018-02-17T18:12:22Z

addon/bootstrap.js

+   * - note first seen,
+   * - check eligible
+   */
+  if (reason === REASONS.ADDON_INSTALL || reason === REASONS.ADDON_UPGRADE) {


I can see how this would be useful for testing, but what about in deployment? @motin, @gregglind , If we have already deployed an addon, and we have to patch it, is that an upgrade or a fresh install?

If we do upgrade addons after deployment, do we want the addon to completely reset? I.e. there would multiple firstSeen and other initial pings from the old version and the user might see an "install" UI element more than once.

This point is moot if we never see the ADDON_UPGRADE reason when the study is re-deployed through Normandy.

Based on @mythmon's comments:

The add-on manager calls something an upgrade when an add-on with the same ID as an existing add-on is installed, and a greater or equal version

Normandy generally doesn't trigger upgrades, because Normandy doesn't change studies in practice. Instead it installs the add-on once, and then doesn't touch it until unenrollment time. In the future, when Normandy supports changing add-ons in place, it will trigger upgrades.

In light of this, my vote is that we future-proof the template and not equate ADDON_INSTALL with ADDON_UPGRADE. QA may have to remove the old version before re-installing a new version, but that seems a small price to pay to avoid potential unintended consequences of studies resetting on upgrade in the future. I think what happens on ADDON_UPGRADE will vary by study. @mythmon , @gregglind , @raymak, @motin , any feelings one way or the other?

gregglind · 2018-02-17T20:36:48Z

It varies by addon, alas, and the author has to be aware of it. Normandy tends to do new installs that pave over the old one. I don’t know if that is what they *desire* to do.

…

On Sat, Feb 17, 2018 at 11:12 AM Bianca Danforth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In addon/bootstrap.js <#49 (comment)> : > @@ -62,14 +62,14 @@ async function startup(addonData, reason) { studyUtils.setVariation(variation); log.debug(`studyUtils has config and variation.name: ${variation.name}. Ready to send telemetry`); - - /** addon_install ONLY: - * - note first seen, - * - check eligible - */ - if ((REASONS[reason]) === "ADDON_INSTALL") { - // telemetry "enter" ONCE + /** addon_install and addon_upgrade (which is considered a new study) ONLY: + * - note first seen, + * - check eligible + */ + if (reason === REASONS.ADDON_INSTALL || reason === REASONS.ADDON_UPGRADE) { I can see how this would be useful for testing, but what about in deployment? @motin <https://github.com/motin>, @gregglind <https://github.com/gregglind> , If we have already deployed an addon, and we have to patch it, is that an upgrade or a fresh install? If we do upgrade addons after deployment, do we want the addon to completely reset? I.e. there would multiple firstSeen and other initial pings from the old version and the user might see an "install" UI element more than once. This point is moot if we never see the ADDON_UPGRADE reason when the study is re-deployed through Normandy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAjwo4_tSkGKR1TP521uPQ5dxPGs1Mks5tVxaHgaJpZM4SHGSV> .

mythmon · 2018-02-20T17:44:16Z

The add-on manager calls something an upgrade when an add-on with the same ID as an existing add-on is installed, and a greater or equal version

Normandy generally doesn't trigger upgrades, because Normandy doesn't change studies in practice. Instead it installs the add-on once, and then doesn't touch it until unenrollment time. In the future, when Normandy supports changing add-ons in place, it will trigger upgrades.

biancadanforth · 2018-02-20T17:48:21Z

package.json

@@ -20,6 +20,9 @@
  "bugs": {
    "url": "https://github.com/mozilla/shield-studies-addon-template/issues"
  },
+  "engines": {
+    "node": ">=8.9.0"


I know we require Node 7.6+ for async/await, but is there a specific reason why we require >= 8.9.0? I read that it's the LTS for Node v8. Just curious mostly.

biancadanforth · 2018-02-20T17:48:46Z

test/utils.js

@@ -33,6 +33,9 @@ const FIREFOX_PREFERENCES = {
  // NECESSARY for all 57+ builds
  "extensions.legacy.enabled": true,

+  // Include log output in browser console
+  "shield.testing.logging.level": 10, // Trace


This doesn't look to be used in bootstrap.js any longer. In this PR's bootstrap, you pull the logging level from Config.jsm instead in initLog(). We should probably do one or the other, but not both.

I'm actually in favor of a pref in utils.js, since then we wouldn't have to remember to turn off logging in Config.jsm when shipping the addon. SSAU also pulls the logging level from Config.jsm, so we'd need a patch for it to read the pref instead as well.

Can you update this PR to read the logging level for initLog() from this pref in bootstrap.js and in any other files like Feature.jsm as well?

gregglind · 2018-02-20T18:10:56Z

Install and Upgrade are different, and the behaviours for each should be described / differ by study. Do not equate them.

…

On Tue, Feb 20, 2018 at 11:58 AM, Bianca Danforth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In addon/bootstrap.js <#49 (comment)> : > @@ -62,14 +62,14 @@ async function startup(addonData, reason) { studyUtils.setVariation(variation); log.debug(`studyUtils has config and variation.name: ${variation.name}. Ready to send telemetry`); - - /** addon_install ONLY: - * - note first seen, - * - check eligible - */ - if ((REASONS[reason]) === "ADDON_INSTALL") { - // telemetry "enter" ONCE + /** addon_install and addon_upgrade (which is considered a new study) ONLY: + * - note first seen, + * - check eligible + */ + if (reason === REASONS.ADDON_INSTALL || reason === REASONS.ADDON_UPGRADE) { Based on @mythmon <https://github.com/mythmon>'s comments: The add-on manager calls something an upgrade when an add-on with the same ID as an existing add-on is installed, and a greater or equal version Normandy generally doesn't trigger upgrades, because Normandy doesn't change studies in practice. Instead it installs the add-on once, and then doesn't touch it until unenrollment time. In the future, when Normandy supports changing add-ons in place, it will trigger upgrades. In light of this, my vote is that we future-proof the template and not equate ADDON_INSTALL with ADDON_UPGRADE. QA may have to remove the old version before re-installing a new version, but that seems a small price to pay to avoid potential unintended consequences of studies resetting on upgrade in the future. I think what happens on ADDON_UPGRADE will vary by study. @mythmon <https://github.com/mythmon> , @gregglind <https://github.com/gregglind> , @raymak <https://github.com/raymak>, @motin <https://github.com/motin> , any feelings one way or the other? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAj5u0oueYBj7Oh3AesCY2jKjk22-Hks5tWwfQgaJpZM4SHGSV> .

motin · 2018-02-20T18:54:48Z

Rebased upon latest master. Addressed open concerns via 815ff06 and 9d7fdaa

motin · 2018-02-20T19:58:22Z

@biancadanforth I removed the configuration of study-specific logging since this should not belong in the template to begin with. We should expect a studyUtils.log API, like pioneerUtils has. Good target for mozilla/shield-studies-addon-utils#101

Lazy-loading StudyUtils makes it unavailable outside startup(), this affects when and where logging and REASONS is available. @gregglind Do we have to do a straight Cu.import to make this available in all of bootstrap.js?

biancadanforth · 2018-02-21T17:22:18Z

@motin That's a great point about the logging. Could you add a link to the pioneerUtils as a comment in that issue as a great example of how to do that?

biancadanforth · 2018-02-21T17:28:54Z

addon/bootstrap.js

-  // Start up your feature, with specific variation info.
-  this.feature = new Feature({variation, studyUtils, reasonName: REASONS[reason]});
+  // start up the chrome-privileged part of the study
+  this.feature.privilegedStartup();


Just curious, why is this better than instantiating the feature once and having its constructor method startup the feature?

Originally, I separated them for the ability to have to extension startup hooks - one for when the webextension had loaded asynchronously, and one at the end of startup() for privileged startup logic. Currently, there is only one startup method but it is anyway clearer to not have side-effects happen in a constructor event.

biancadanforth · 2018-02-21T17:36:15Z

package.json

@@ -45,7 +45,6 @@
    "npm-run-all": "^4.1.1",
    "nsp": "^2.8.1",
    "onchange": "^3.2.1",
-    "path": "^0.12.7",


We use this devDependency in run-firefox.js; can you add it back?

That removal comes from #41 - comment that this dependency is used, close the pr and I will rebase this pr to not include this removal :)

The path module is the built in Node.js module. Adding it in package.json includes a module from npm — which could potentially be dangerous.

Noted, and I have merged #41. Thank you Peter for clarifying!

biancadanforth · 2018-02-21T17:41:50Z

addon/bootstrap.js

+  // if you have code to handle expiration / long-timers, it could go here
+  /*
+  if (this.feature.hasExpired()) {
+    // Please note that the general study expiration should probably be taken care of by Normandy.


This is news to me! I have been handling general study expiration in my addon code -- @mythmon , does Normandy handle this? What do you recommend study authors do re: general study expiration?

Normandy does not handle this, and I do not agree with this comment.

Normandy will handle removal of the add-on when the study is ended on the server, but that is not what this feature is generally used for. The first expiration timer that I'm aware of was designed to expire an experiment as a fail safe, in case Normandy could not be contacted, or someone forgot to disable the recipe on the server.

In that original case, the expiry timer was not a normal end to the study, but a failsafe in case something went wrong. As such, it should be implemented in a very reliable way by the add-on.

Thanks for clarifying @mythmon, adding this information as a comment in the template

motin · 2018-02-21T17:43:22Z

@motin That's a great point about the logging. Could you add a link to the pioneerUtils as a comment in that issue as a great example of how to do that?

Done

biancadanforth · 2018-02-21T18:01:04Z

TESTPLAN.md


-During INSTALL ONLY users see:
+* Download a Release version of Firefox (Release is required for the recommendation heuristics to work)


What do you mean by "recommendation heuristics"? Can you expand on that a little here?

Sorry, a leftover from taar-v2, removing...

biancadanforth · 2018-02-23T20:57:51Z

addon/bootstrap.js

@@ -119,22 +119,26 @@ function shutdown(addonData, reason) {
  // FRAGILE: handle uninstalls initiated by USER or by addon
  if (reason === REASONS.ADDON_UNINSTALL || reason === REASONS.ADDON_DISABLE) {
    log.debug("uninstall or disable");
+


Can you add a note here to say we check if feature exists because it's possible the study is shutting down before it has instantiated the feature. Ex: if the user is ineligible or if the study has expired.

Should probably have clicked a few lines below for this comment. :X

Clarifying this

… after recent bootstrap.js updates

… that we can rely on

…standable

…e already on the way to be merged into) shield-study-addon-utils

…ly by default

… will not be relevant for study-specific QA

motin · 2018-03-09T21:35:14Z

Rebased upon the latest master. Ready for merge.

motin force-pushed the improvements-2 branch from 57ad937 to 57d78eb Compare February 15, 2018 16:15

pdehaan reviewed Feb 15, 2018

View reviewed changes

TESTPLAN.md Outdated

Click on the 'x' button.

* notice closes

* addon uninstalls

Copy link

Contributor

pdehaan Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add-on

pdehaan reviewed Feb 15, 2018

View reviewed changes

TESTPLAN.md Outdated

1. Open a 2nd firefox window.

2. Close the initial window.

* notice closes

* addon uninstalls

Copy link

Contributor

pdehaan Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add-on

pdehaan reviewed Feb 15, 2018

View reviewed changes

TESTPLAN.md Outdated

---

## Helper code and tips

* notice closes

* addon uninstalls

Copy link

Contributor

pdehaan Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add-on

pdehaan reviewed Feb 15, 2018

View reviewed changes

motin force-pushed the improvements-2 branch from 183f995 to 451a796 Compare February 15, 2018 20:10

biancadanforth reviewed Feb 17, 2018

View reviewed changes

biancadanforth mentioned this pull request Feb 18, 2018

WIP - Debugging final unit test, browser_pluginnotification.js. biancadanforth/tracking-protection-shield-study#94

Closed

biancadanforth reviewed Feb 20, 2018

View reviewed changes

motin force-pushed the improvements-2 branch from 4f76af7 to 9d7fdaa Compare February 20, 2018 18:53

biancadanforth reviewed Feb 21, 2018

View reviewed changes

biancadanforth reviewed Feb 23, 2018

View reviewed changes

motin added 24 commits March 9, 2018 23:23

Not setting shield.testing.logging.level since it may not be relevant…

c56a90e

… after recent bootstrap.js updates

Npm run format

a6ad237

Use console as our logger until there is a log() method in studyUtils…

8eac435

… that we can rely on

Scoped usage of the reason variable makes more sense

a91f939

Referring to studyUtils.REASONS instead of this.REASONS is more under…

9a251c8

…standable

Not using studyUtils.REASONS outside startup since it is not available

fdf888b

Minor jsdoc tweaks

e3af842

Removed a study-specific notice

05915ef

Updated comments about study expiration handling

a4ea763

Clarified a comment in bootstrap shutdown()

4370525

Removed shield-generic docs that are better suited to live in (and ar…

b77eb53

…e already on the way to be merged into) shield-study-addon-utils

Moved docs into docs folder

695227b

Added link to the general shield study engineering docs in ssau

9c33a9c

DEV.md documentation updates

cb0d0a8

Updated readme to reflect the state of the repo after mozilla#49

2e7dcf0

Don't lint/format package.json since npm install formats it different…

768a9d6

…ly by default

Doc clarifications

4eb8463

Moved list of legacy template repositories to addon-utils since these…

2fc3668

… will not be relevant for study-specific QA

Cleaner implementation of npm run format

44394bc

Better documentation of eslintignore

7fcbfe7

Clarified jsdoc about VARIATION_OVERRIDE_PREF

72784fe

Clarified the important notice in the readme

750ce78

Removed unused example study icons

886f845

Removed unused icons from file tree docs

ba792ec

motin force-pushed the improvements-2 branch from 8cb98d9 to 2fbb84c Compare March 9, 2018 21:25

Added note about design specifications to test plan

8257f9b

motin force-pushed the improvements-2 branch from 2fbb84c to 8257f9b Compare March 9, 2018 21:34

biancadanforth merged commit 6aabef0 into mozilla:master Mar 9, 2018

motin deleted the improvements-2 branch March 10, 2018 07:33


		Repositories that should no longer be used as templates for new studies:

		[https://github.com/gregglind/template-shield-study]() - The incubation repo for the updated structure and contents of this repo, implemented in late 2017.


		During INSTALL ONLY users see:
		* Download a Release version of Firefox (Release is required for the recommendation heuristics to work)

Improvements sourced from several shield add-on repositories #49

Improvements sourced from several shield add-on repositories #49

Conversation

motin commented Feb 15, 2018 • edited Loading

motin commented Feb 15, 2018

gregglind commented Feb 15, 2018 via email

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

motin commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregglind commented Feb 17, 2018 via email

mythmon commented Feb 20, 2018

Choose a reason for hiding this comment

biancadanforth Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

gregglind commented Feb 20, 2018 via email

motin commented Feb 20, 2018

motin commented Feb 20, 2018

biancadanforth commented Feb 21, 2018

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

motin commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motin commented Mar 9, 2018

motin commented Feb 15, 2018 •

edited

Loading

biancadanforth Feb 20, 2018 •

edited

Loading

biancadanforth Feb 23, 2018 •

edited

Loading