-
Notifications
You must be signed in to change notification settings - Fork 21
WIP on schemas for the experiment APIs - for review by all #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked many questions and proposed some changes. My comments are not just for Gregg, but also @motin , @raymak ...
In general, I think some methods could be private methods (and therefore don't need to be in schema.json
), and it would be great to have a description
field for each entity in the final version.
webExtensionApis/study/schema.yaml
Outdated
- name: sendTelemetry | ||
async: true | ||
parameters: | ||
- name: payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does payload
need to be declared at the top under types
along with weightedVariations
, etc.?
webExtensionApis/study/schema.yaml
Outdated
- namespace: study | ||
description: Interface for shield studies, for use by `background.js` scripts. | ||
apiVersion: 5 # for dev use | ||
types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be separate schema for these objects? How does that get incorporated with this API schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Yes, those will go in there. It's just a TBD at the moment. They are also jsonschema. I haven't tested that that part actually works, marked as TODO.
webExtensionApis/study/schema.yaml
Outdated
returns: none | ||
|
||
## informational things | ||
- name: permissions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just call this "isEligible"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could choose to have "eligibilityMetadata" or similar method which would return several flags like isEligible, isPioneer, isPrivateMode, profileAge etc, or we could choose to implement one method per check (better documentation that way, less magic)
webExtensionApis/study/schema.yaml
Outdated
- name: permissions | ||
async: true | ||
# I don't like this name! | ||
# TODO: should there be an arg to just ask for pioneer? shield? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me to have an isPioneer
flag for additional eligibility checks.
webExtensionApis/study/schema.yaml
Outdated
async: true | ||
# I don't like this name! | ||
# TODO: should there be an arg to just ask for pioneer? shield? | ||
returns: object of permissions with keys shield, pioneer, telemetry, 'ok' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding this function -- can you give an example of what this return object might look like? How would that object get used? I'm thinking this method behaves like isEligible
with Pioneer/extra considerations and could just return true
or false
.
webExtensionApis/study/schema.yaml
Outdated
type: string | ||
returns: a url with queryArgs appended / mixed | ||
|
||
- name: validateJSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the schema validation method need to be exposed in the API? Seems like an internal method for public methods that have parameters that need to be validated.
webExtensionApis/study/schema.yaml
Outdated
description: Configure the study. Most things can't work without this | ||
async: true | ||
parameters: | ||
- name: studySetup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this method is only called once for the lifetime of a study, we may want to consider adding a duration
key (say, in weeks) to the studySetup
parameter, and adding an isExpired
method elsewhere in the API.
webExtensionApis/study/schema.yaml
Outdated
# I don't like this name! | ||
# TODO: should there be an arg to just ask for pioneer? shield? | ||
returns: object of permissions with keys shield, pioneer, telemetry, 'ok' | ||
- name: userInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that this method needs to be part of the public API? Perhaps as an internal method to append query strings to the survey URL...
webExtensionApis/study/schema.yaml
Outdated
- name: telemetrySelectionOptions | ||
description: `{ type, n, minTimestamp, headersOnly }` | ||
|
||
- name: setActiveExperiment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claim: setActiveExperiment
and unsetActiveExperiment
can be private methods called in the public methods configure
and endStudy
. Thoughts?
webExtensionApis/study/schema.yaml
Outdated
|
||
|
||
# shield-utils (`browser.study.`) | ||
# - chooseBranch (weights, optional rng?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a log
method? Perhaps this log could be used everywhere in the study, privileged and unprivileged code. See #139 . At the very least, we could use this one instance for the privileged code.
webExtensionApis/study/schema.yaml
Outdated
async: true | ||
parameters: | ||
- name: studySetup | ||
type: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this being set to "any", the following error is received:
Error: Type error for parameter studySetup (Unexpected properties: eligible, expired, study, variationOverridePreference, weightedVariations) for study.configure.
webExtensionApis/study/schema.yaml
Outdated
returns: Nothing | ||
parameters: [] | ||
|
||
## prefs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API implementations (like study, introNotificationBar, etc) may get and set prefs internally, but this does not mean that they need to expose getPref and setPref via their APIs.
For completely general get/set preferences methods, I vote to have a separate API namespace:
browser.prefs.get() - gets a preference
browser.prefs.set() - sets a preference
These can be used to check for eligibility etc.
If we include get/set pref methods in the study API (as per this line of code), it should only be study-related. For instance:
browser.study.getPref() - gets a study-namespaced preference
browser.study.setPref() - sets a study-namespaced preference
These can be used to store study-specific state, counters and test-related preferences for the study.
webExtensionApis/study/schema.yaml
Outdated
# expiration? | ||
|
||
|
||
- name: simpleDeterministicVariation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple in contrast to what? If we plan to include several different deterministic variation methods, we should prefix them (or move them to a separate namespace), otherwise deterministicVariation is enough
webExtensionApis/study/schema.yaml
Outdated
- name: type | ||
returns: Nothing | ||
|
||
- name: getTelemetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is necessary. getTelemetry
however sounds like it expects a single argument, similar to getPref
. Maybe searchTelemetry
instead?
webExtensionApis/study/schema.yaml
Outdated
returns: none | ||
|
||
## informational things | ||
- name: permissions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could choose to have "eligibilityMetadata" or similar method which would return several flags like isEligible, isPioneer, isPrivateMode, profileAge etc, or we could choose to implement one method per check (better documentation that way, less magic)
0412f2e
to
803a904
Compare
@@ -0,0 +1,76 @@ | |||
// Allow outside override for debug and testing using prefs | |||
const STUDYPREFS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have browser.study.getPref and browser.study.setPref automatically prefix the arguments with shield.${addonWidgetId}.
and remove it from the boilerplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. For now, keeping it explicit for these reasons:
- Moved prefs stuff to a different API.
- The autoprefixing has caused a lot of trouble in the past, becuaes things change names, the control prefs move, and all kinds of unexpected stuff.
constructor(variation) { | ||
// also sets activeExperiment key | ||
// makes telmemetry possible | ||
browser.study.configure(variation, ...STUDYCONFIG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not validate the schema https://github.com/mozilla/shield-studies-addon-utils/blob/121-finalize-schema/webExtensionApis/study/src/schemas/schema.studySetup.json - is the suggestion to simplify the schema to match the current small-study or will small-study be updated to validate the schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of both. I just finally went through Config.jsm to see what we can drop. There are some keys we don't need any more, and some that I would like to rename. Configure was badly designed then, and we can do better.
8974714
to
587d5a9
Compare
} | ||
} | ||
|
||
watchImportantPrefs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. It does however require something like browser.study.watchPreferences() or similar to the API which is currently missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems like it should be in the studyUtils API (at least the preferences we end the study for that are common to all studies like auto private browsing mode).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better solution here is "watchPermissions". Prefs.watch belongs over in prefs.
74a4a9b
to
00cf5d3
Compare
00cf5d3
to
94da23d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more comments/questions.
In general:
npm run build
results in hundreds of eslint errors/warnings. Might be worth getting that to pass/updating.eslintignore
etc.- Really cool to see there's a
web-ext-config.js
that handles a lot of what ournpm run firefox
config did with webdriver. - I don't see any references to
./examples/test-addon
. Is it used? If not, should we remove it?
@@ -0,0 +1,18 @@ | |||
#!/usr/bin/env bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a comment explaining what this script does explicitly at the top.
My best guess: Copies StudyUtils API to example add-on directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. Linting is fixed, and the bash stuff is very very temporary.
# bundle the study web extension experiment | ||
mkdir -p $ADDON_SRC_PATH/privileged/study | ||
cp $WEBEXTAPIS_PATH/study/api.js $ADDON_SRC_PATH/privileged/study/api.js | ||
cp $WEBEXTAPIS_PATH/study/fakeApi.js $ADDON_SRC_PATH/privileged/study/fakeApi.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's fakeApi
?
From package.json
, it looks like a skeleton API built from the StudyUtils schema.json
? Is there not an automated way to validate the API with schema.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fakeApi is so that we can advance on trying the api withougt having to actuaally have it all be working. This comment reflects old work, when the patch was a mess.
// Allow outside override for debug and testing using prefs | ||
const STUDYPREFS = { | ||
variationPref: `shield.${addonWidgetId}.testingBranch`, | ||
firstrunPref: `shield.${addonWidgetId}.firstrun`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this firstrunPref
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, seems like we should also have an override pref for study duration and a method in the API to set up study duration and check for expiration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firstrunPref is 'installTimestamp', and indeed, used for exactly what you describe.
} | ||
} | ||
|
||
async watchExpire() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I claim this should be in the StudyUtils API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I claim not, but I am swayable.
} | ||
} | ||
|
||
watchImportantPrefs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems like it should be in the studyUtils API (at least the preferences we end the study for that are common to all studies like auto private browsing mode).
browser.runtime.onInstalled.addListener(study.installOrDie); | ||
|
||
// 4. handle install vs. regular run. | ||
await browser.shield.attemptRun(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the attemptRun
method? Is that supposed to be in the StudyUtils API? I don't see it.
By browser.shield
do you mean browser.study
? There are a few instances of browser.shield
further down as well.
package.json
Outdated
@@ -46,7 +47,7 @@ | |||
"url": "git://github.com/mozilla/shield-studies-addon-utils.git" | |||
}, | |||
"scripts": { | |||
"build": "cd webExtensionApis/study && webpack", | |||
"build": "cd webExtensionApis/study && webpack && yaml2json schema.yaml -p > schema.json ; node ../../bin/schemaToInterface.js ./schema.json > fakeApi.js ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a schemaToInterface.js
file, what is the last of the three commands intended to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a github foulup.
webExtensionApis/study/schema.json
Outdated
"name": "weightedVariations" | ||
}, | ||
{ | ||
"name": "anEnding" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see schemas inside .webExtensionApis/study/schemas
for anEnding
, telemetrySelectOptions
and prefTypes
-- should there be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another pass thinking about the functions, what I expect them to do, arity, etc.
The only thing I can think of that's missing is a log
function. I don't know what that method is going to look like right now, but I can guess it'd have one parameter at least for the message.
This would likely also require an initLog
method with two parameters: logLevel
(String or number) and shouldLog
(boolean).
webExtensionApis/study/schema.yaml
Outdated
## study lifecycle | ||
- name: configure | ||
type: function | ||
description: Configure the study. Most things can't work without this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would edit the description here to say something like: Validates the studySetup
schema and (insert whatever else it does...).
webExtensionApis/study/schema.yaml
Outdated
type: function | ||
description: Configure the study. Most things can't work without this | ||
async: true | ||
fakereturn: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is fakereturn
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(gone, defaultReturn replaced that. )
webExtensionApis/study/schema.yaml
Outdated
|
||
- name: startup | ||
async: true | ||
description: all for side effects (setActiveExperiment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't setActiveExperiment
only need to be called once on install, not every time the extension starts up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description field, we could specify this performs the tasks that have to be performed every time the extension starts up. The only thing I can think of is the check for permissions/eligibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setActiveExperiment happens every session. It doesn't persist. We don't know what permissions the study needs :/ Maybe I need to put that in the configuration. Annoying.
webExtensionApis/study/schema.yaml
Outdated
|
||
- name: install | ||
async: true | ||
description: all for side effects (sending 'install', 'enter') pings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can specify in the description here that this method handles tasks that occur on study install only. Seems like the install
and enter
pings and also setActiveExperiment
? That is based off the old StudyUtils.jsm startup method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old StudyUtils.jsm called setActiveExperiment
on every startup, but do we need to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
webExtensionApis/study/schema.yaml
Outdated
- name: endStudy | ||
async: true | ||
defaultReturn: "endingName" | ||
description: Optionally opens url, then ends study with pings ending, exit. Study can only have one ending. Uninstalls addon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote this optionally uninstalls the extension (we may need to perform some cleanup in the privileged code before uninstalling the extension, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a technical challenge.
webExtensionApis/study/schema.yaml
Outdated
parameters: | ||
- name: anEnding | ||
type: object | ||
# should be anEnding Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have the option to uninstall (default to true?), then we'd want another parameter here... something like (or whatever the correct syntax would be):
name: shouldUninstall
type: Boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I will think on this, and learn if it is possible.
webExtensionApis/study/schema.yaml
Outdated
defaultReturn: undefined # exception if out of policy based on config | ||
returns: Nothing | ||
|
||
- name: getTelemetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would you want to use getTelemetry
? Can you add a description
field here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
webExtensionApis/study/schema.yaml
Outdated
type: string | ||
returns: a url with queryArgs appended / mixed using our usual pattern | ||
|
||
- name: validateJSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When might a developer want to use this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
webExtensionApis/study/schema.yaml
Outdated
- name: anObject | ||
type: object | ||
- name: schema | ||
type: object # a jsonschema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a jsonschema
? What would that look like?
There should be no linting errors on a clean check-out since #152 was merged. Please open another issue if this is not the case (confirmed no linting errors for me and gregg).
Yes, and we can even build in a local-only include for devs to have their own overrides.
All tests for the utils are run (and passes) in ./examples/test-addon so we can't remove it without changing the way tests are run. |
); | ||
} | ||
browser.browserAction.setTitle({ title: studyInfo.variation }); | ||
console.log(`changed the browser action title: ${studyInfo.variation}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not forget browser.study.log in the api draft and small-study example, to use instead of console.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Recall that we can't pass arbitrary arity here, so it's a string or array. Sorry :/
/** | ||
* Run every startup to get config and instantiate the feature | ||
*/ | ||
async function everyStartup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onEveryRun or onEveryExtensionLoad or similar, as to not confuse it with the onStartup event which means: on browser startup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
examples/small-study/src/study.js
Outdated
* Will be augmented by 'getstudySetup' | ||
*/ | ||
const studySetup = { | ||
// telemetryEnvirment.setActiveExperiment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: typo telemetryEnvironment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
examples/small-study/src/study.js
Outdated
|
||
/** Base for studySetup, as used by `browser.study.setup`. | ||
* | ||
* Will be augmented by 'getstudySetup' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: casing on getStudySetup
bin/verifyWeeSchema.js
Outdated
const proposed = require(path.resolve(process.argv[2])); | ||
// const weeSchemaSchema = require(path.resolve("./wee-schema-schema.json")); | ||
|
||
const ajv = new require("ajv")() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add allErrors
to the constructor here?
allErrors
: check all rules collecting all errors. Default is to return after the first error.
— via https://github.com/epoberezkin/ajv#options
bin/verifyWeeSchema.js
Outdated
if (!valid) { | ||
console.error(`# ERRORS IN ${i}:${j} ${type.name} ${ns.namespace}.functions[${j}].paramters[${k}]`) | ||
console.error(ajv.errors) | ||
debugger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rogue debugger statement?
webExtensionApis/study/schema.yaml
Outdated
|
||
defaultReturn: undefined # exception if out of policy based on config | ||
|
||
- name: filterTelemetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name suggests that telemetry will be filtered as a side-effect somewhere in the telemetry archives, but all this method does is return telemetry packets that have been previously sent, optionally filtered. Candidate name suggestions: getSentTelemetry, searchTelemetryArchive, queryTelemetryArchive, searchSentTelemetry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like queryTelemetryArchive
or searchSentTelemetry
Note: | ||
- no conversions / coercion of data happens. | ||
|
||
Note: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Throw exceptions in these cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree on that. Might be easier to say than do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more questions/comments throughout. Great progress on the schema for study
!
My assumptions about things that are still incomplete/WIP (we should track this somewhere, but not sure where):
./webExtensionAPIs/index.js
(we fill this out based onfakeAPI.js
once it's finalized?)test-addon/small-study
not tested/working yet (do we want to keep both?)package.json
scripts need to be culled.- I'm sure there are a lot of other things that you're aware of that I'm not
Re: README improvements:
- Consider adding an Under Construction note at the top?
- The intro at the top could read something like: "This is the home of the
shield-studies-addon-utils
npm package, which provides astudy
andprefs
API for use in developing Shield study WebExtension Experiments." - In the Overview section, consider including directory URLs for those listed, and adding a note for the
./webExtensionApis
list item that the most helpful files in that directory are theschema.yaml
forstudy
andprefs
to understand the methods that are exposed. - The content from the "
browser.prefs.*
section down seems outdated. I'd say updatebrowser.prefs.*
and consider updating or removing other sections?
- OR deterministicVariation | ||
for the studyType using `weightedVariations` | ||
|
||
- During firstRun[1] only: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it also validate the studySetup
schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(All arguments are validated as part of being a wee. Are you suggesting we make that clearer / offer a tool? Because that might be worth a bug, since these are complex objects.)
webExtensionApis/study/schema.yaml
Outdated
type: string | ||
enum: ['shield', 'pioneer'] | ||
|
||
- name: surveyUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: keep consistent convention - getFullSurveyUrl
?
|
||
type: function | ||
async: true | ||
defaultReturn: "styleA" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not a normal return
key in yaml for at least the type and description of the return value for a function? I'm surprised none of these functions that return stuff describe what they return in an explicit manner.
Ex: deterministicVariation
returns a string literal corresponding to the value of a name
key from the weightedVariations
array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'return' is somewhat magical. File that as an issue. I had it and it broke stuff, and I haven't dug more.
webExtensionApis/study/schema.yaml
Outdated
# should be anEnding Object | ||
|
||
## study / client information things | ||
- name: info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: keep consistent naming conventions, suggesting: getStudyInfo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, okay. Ugh.
webExtensionApis/study/schema.yaml
Outdated
- name: onEndStudy | ||
type: function | ||
defaultReturn: {urls: [], reason: 'some-reason'} | ||
description: Listen for when the study wants to end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a note in the description that this could be useful if you have privileged code that needs to clean itself up before add-on uninstall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
/** | ||
* - set up expiration alarms | ||
* - make ui with the particular variation for this user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same as above, most but not all studies have UI.
|
||
/** handles `study:end` signals | ||
* | ||
* calls cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also opens survey URLs if any.
* do some cleanup / 'feature reset' | ||
*/ | ||
async cleanup() { | ||
await browser.storage.local.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add suggestions here to cleanup privileged code by, say:
- removing added listeners
- unloading study-specific JSMs
- removing study-specific UI elements that were added
- ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WEE makes this easier. Added as a doc, but it's better for the code to listen for uninstall.
See: https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/lifecycle.html
@@ -0,0 +1,248 @@ | |||
/* eslint-disable */ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note at top that this is a temporary file until the body of the study
API methods are written in ./webExtensionApis/study/src/index.js
? Isn't this a generated file? Do we want to include a generated file in the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's generated for now. After this finalized api lands we can immediately rip it back out. If you prefer, we can do it as the LAST patch of this series?
@@ -1,113 +1,353 @@ | |||
[ | |||
{ | |||
"namespace": "study", | |||
"description": "study", | |||
"description": "Interface for Shield and Pioneer studies.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this also a generated file from schema.yaml
? Do we want to include generated files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See the answer re fakeApi. happy to do that as the last patch here.)
Yes, I will implement ./webExtensionApis/study/index.js based on fakeApi.js once this PR is merged. Created the issue: #160
test-addon is for testing the API directly and has tests (but those needs to be restored). small-study is an example of an actual small study, missing tests so far (should be added. both should be kept since they test different aspects (test-addon is more like unit testing). Keeping both necessary until we find a way to test things properly some other way.
True, should be a separate issue. It is related to the decision of keeping this as a mono-repo or not. |
For #121