citp / news-disinformation-study

A research project on how web users consume, are exposed to, and share news online.
8 stars 2 forks source link

Add-on Review Questions #68

Closed wagnerand closed 4 years ago

wagnerand commented 4 years ago

Hi, I am Andreas and I am working in the Firefox Add-ons team. I will conduct a review according to our add-on policies.

I'd like to start off with a few questions that should help facilitate the review:

Thanks in advance! I hope some of the code removal questions will speed up the code review.

akohlbre commented 4 years ago

Thanks Andreas!

wagnerand commented 4 years ago

Thank you @akohlbre!

* `study/domains`: We're finalizing the list of domains that will be used for the study. When we do, we'll combine them all into one file (essentially like the `study/newsDomains.js` file that currently exists). If it's easier for the review, we can move the csv/json files elsewhere and just replace `newsDomains.js` when it's ready

No, it's fine, I just wanted to double check if those files are used directly in the extension and I understand they are not.

* consent: yes, this was something our extension was originally going to handle, but it got moved out. I've removed all the code that used to handle it

Thank you.

* unused files: this is a little tricky, since this repo imports our WebScience repo as a subtree (and WebScience holds all the modules we've written, even those not used by this study). Instead of directly removing them, I added an option to the makefile to delete the unused files: `make prepare`. Will that work?

That approach works for now. It's not ideal though, we might follow up on that later. It appears there a more unused files, for example WebScience/Utilities/ResponseBody.js. While the module is imported, the exported method processResponseBody does not look like it's used anywhere. Since your code-base is rather large and generic, I kindly ask to have a look at all files and remove the unused ones. This also impacts further things like the declared permissions in your manifest, so it's important we have a good understanding of what is being used.

* third-party libraries:

  * I couldn't find the original source for the LocalForage library, so I replaced it with a recent version from [localForage/localForage@`1.9.0`.tar.gz](https://github.com/localForage/localForage/archive/1.9.0.tar.gz). However, we had to modify the release to allow it to be imported as an ES6 module -- I don't believe we ever found a version that had that already. The change is the addition of lines 7 and 2796 in `WebScience/dependencies/localforagees6.min.js`
  * localForageStartsWith: similarly, I replaced the old version with the one from [https://github.com/localForage/localForage-startsWith/blob/master/dist/localforage-startswith.es6.js](https://github.com/localForage/localForage-startsWith/blob/master/dist/localforage-startswith.es6.js?rgh-link-date=2020-10-26T04%3A06%3A06Z), again with two small modifications: add line 292, modify path in line 1 in `WebScience/
  * let me know if making these modifications is a problem for the review

We generally do not allow modified libraries. I am willing to make a one-time exception here, but we might come back to this later and ask for an unmodified version, or other libraries to use unmodified versions.

For LocalForage, I compared https://github.com/localForage/localForage/blob/1.9.0/dist/localforage.js to https://github.com/citp/news-disinformation-study/blob/514281e4a276bbfc054d6d7baf70a77badaddcc6/WebScience/dependencies/localforagees6.min.js and there are more changes than the ones you mentioned. Please make sure to limit the changes to ES6 module export only. (Nit: If you could also remove the "min" from the filename, that would be great.)

For LocalForageStartsWith I compared https://github.com/localForage/localForage-startsWith/blob/v1.4.0/dist/localforage-startswith.es6.js to https://github.com/citp/news-disinformation-study/blob/514281e4a276bbfc054d6d7baf70a77badaddcc6/WebScience/dependencies/localforagees6.min.js and the changes made here are ok. Please remember to always reference tagged revisions (ie releases) on github, as branch references are not permanent.

* user flow: users won't directly interact with anything in our extension: rather, they'll be led through the consent process by the page at `about:ion`. I'm not sure what the current status of that page is; we're still discussing the language that will describe our study, and I the page itself may still be in flux. @knowtheory can probably tell you more

It would be great if we could document the general install, consent and interaction flow. Unless I am mistaken, users to interact with the extension directly, when a survey popup notification appears. When does that happen? Who or what determines when it should appear? Where does the survey go to?

Using that popup notification comes with several drawbacks, we prefer if you could use something else, in particular something that doesn't require privileged code. That also gives the user a better experience and likely provides you with more survey responses. The "doorhanger" popup will close as soon as the user clicks away, and that is very likely to happen when it appears out of the context of the current user action, as this notification appears to be. Using a browser action (with a customizable icon) and/or a webextension notification (which integrates into the system notifications) will give you a more visible and more permanent opportunity that is not accidentally dismissed while not being intrusive at the same time.

akohlbre commented 4 years ago

We generally do not allow modified libraries. I am willing to make a one-time exception here, but we might come back to this later and ask for an unmodified version, or other libraries to use unmodified versions.

Got it, thanks. I haven't been able to find versions that will work completely unmodified with our module setup, but I'll keep looking.

For LocalForage, I compared https://github.com/localForage/localForage/blob/1.9.0/dist/localforage.js to https://github.com/citp/news-disinformation-study/blob/514281e4a276bbfc054d6d7baf70a77badaddcc6/WebScience/dependencies/localforagees6.min.js and there are more changes than the ones you mentioned. Please make sure to limit the changes to ES6 module export only. (Nit: If you could also remove the "min" from the filename, that would be great.)

Oops, sorry -- had too many versions floating around. I updated the version in the extension to be the one you compared with, with just the two lines modified. Updated the filename as well (note that doing so changed the import in WebScience/dependencies/localforage-startswith.js).

It would be great if we could document the general install, consent and interaction flow. Unless I am mistaken, users to interact with the extension directly, when a survey popup notification appears. When does that happen? Who or what determines when it should appear? Where does the survey go to?

Yes, users do interact with the extension for surveys, just not for install or consent. We need the consent process documented for our IRB as well, so I'll attach that once it's finalized.

For the survey, we're still finalizing the details, but the plan is that the survey will prompt immediately after the study is installed, with options for "No thanks" (survey never prompts again), "Later" (survey prompts again after three days), and "Open survey" (new tab opens with a Qualtrics survey page). Not all of that code is in the extension yet, and we're still setting up the survey, but I expect it all to be finalized in the next 1-2 days.

I'm still working on a couple of the other things you brought up, just wanted to send things along as they were ready.

akohlbre commented 4 years ago

Using that popup notification comes with several drawbacks, we prefer if you could use something else, in particular something that doesn't require privileged code. That also gives the user a better experience and likely provides you with more survey responses. The "doorhanger" popup will close as soon as the user clicks away, and that is very likely to happen when it appears out of the context of the current user action, as this notification appears to be. Using a browser action (with a customizable icon) and/or a webextension notification (which integrates into the system notifications) will give you a more visible and more permanent opportunity that is not accidentally dismissed while not being intrusive at the same time.

We considered both options back when we originally planned the surveys.

Browser action: the problem with this is that we can't trigger the popup open until the user clicks the icon. While the possibility of surveys is mentioned in the consent documents, we aren't sure users will necessarily be watching for surveys from Ion, so we aren't confident that they'd click and see the survey request. If there's a way I'm missing to trigger the popup, we'd definitely be interested.

Webextension notification: My understanding is that these can't contain buttons, just text. We need the users to be able to click into a survey (or reject/snooze the request).

Let me know if I'm misunderstanding either option. We don't love using the privileged code either, it just seems like our best option so far.

If we really want to avoid privileged code, we could try combining browser actions with webextension notifications -- a notification to get their attention and direct them to the icon, and then the browser action for the button into the survey. I haven't run that past anyone else yet: my main concern is that it would be confusing for users to be seeing things in two places. Open to opinions on this, and I'll keep thinking about other possibilities.

That approach works for now. It's not ideal though, we might follow up on that later.

We're changing the way we're going to handle the subtree, so I'll fully remove the unused files, instead of using the makefile.

akohlbre commented 4 years ago

Ok, I pushed a commit that removes more unused code. Note that the exported functions in WebScience/Utilities/Scheduling.js aren't currently used, but will be in the most final version (while we're still testing, we want the aggregation event to fire frequently to check that it aggregates correctly, but we'll use the Scheduling.js module to slow it down to weekly in the release version).

wagnerand commented 4 years ago

Browser action: the problem with this is that we can't trigger the popup open until the user clicks the icon. While the possibility of surveys is mentioned in the consent documents, we aren't sure users will necessarily be watching for surveys from Ion, so we aren't confident that they'd click and see the survey request. If there's a way I'm missing to trigger the popup, we'd definitely be interested.

Webextension notification: My understanding is that these can't contain buttons, just text. We need the users to be able to click into a survey (or reject/snooze the request).

You can pass options to the notification, including buttons: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/NotificationOptions

Let me know if I'm misunderstanding either option. We don't love using the privileged code either, it just seems like our best option so far.

If we really want to avoid privileged code, we could try combining browser actions with webextension notifications -- a notification to get their attention and direct them to the icon, and then the browser action for the button into the survey. I haven't run that past anyone else yet: my main concern is that it would be confusing for users to be seeing things in two places. Open to opinions on this, and I'll keep thinking about other possibilities.

If it's going to open a new tab with a survey right away (assuming the user clicks Yes), then it would be better to mention that in the description on the about:ion page and then just open the new tab after install.

akohlbre commented 4 years ago

You can pass options to the notification, including buttons: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/NotificationOptions

Perhaps I'm missing something, but I don't believe Firefox supports the buttons (as in the chart at the bottom of the page you linked), or the onButtonClicked listener (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/onButtonClicked).

I'll talk to the rest of the team about opening a tab or combining the notification options.

wagnerand commented 4 years ago

Perhaps I'm missing something, but I don't believe Firefox supports the buttons (as in the chart at the bottom of the page you linked), or the onButtonClicked listener (developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/onButtonClicked).

Ah, my bad, I missed that, because Firefox does show buttons for notifications, but they seem to come from Firefox itself always.

akohlbre commented 4 years ago

Ok, we've decided we could do a combined approach:

This would get rid of all the privileged code. Sound like a reasonable alternative to you?

If so, I should have some time to work on implementing this tonight (US east coast time), and get the copy approved tomorrow.

wagnerand commented 4 years ago

That sounds good, thank you. I agree that opening a new tab all of a sudden a few days after the study was installed can lead to confusion and degraded amounts of reponses. We have seen related reactions from users for other add-ons that did similar things.

akohlbre commented 4 years ago

Ok, I pushed a new version with no privileged code. New survey request flow:

I need to discuss several parts of this with other people (in particular: copy for all the requests, css for the popup (it's... ugly right now)), but those should be of the "small change" variety, rather than main functionality.

wagnerand commented 4 years ago

Excellent, thank you!

I completed the review of 13d60a95b2e5298797e2d7ddde31b617fe317d5b. The code looks good overall, I will open a separate github issue with some specific points; all minor, code-wise.