1399231 - [Shield][Cloud Storage] Shield add-on for Cloud Storage

Status: RESOLVED FIXED

(Shield :: Shield Study, enhancement)

Comment 1

[Punam Dahiya [:pdahiya]]

Cloud Storage add-on to be used in shield study Bug 1399198.

Add-on uses shield utils library and supports variations defined in config.jsm. 

Add-on can be tested in Firefox 56 by enabling API using pref 'cloud.services.api.enabled' set to true

Working on integrating data set sent to telemetry for analysis. 

https://github.com/mozilla/shield-cloudstorage

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Attaching XPI with prompt interval set to ~2mins

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated add-on with telemetry, schema and license

Comment 4

[Punam Dahiya [:pdahiya]]

Hi Mike

The code for attached cloud storage add-on xpi is in github https://github.com/mozilla/shield-cloudstorage

Its integrated with shield utils library and telemetry. This is first shield add-on I worked on and would like to
pass it with you before sending it for signing and subsequent steps for shield study. I will appreciate if you can provide your feedback.

Thanks
Punam

Comment 5

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated with download prefs and providers keys telemetry data

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: Add-on UI screenshots

Hi Michelle
Attached are screenshots asking user to opt-in to save download file to cloud provider folder.

Add-on supports testing two variations of content in door-hangar prompt and would like your feedback on content used.

https://github.com/mozilla/shield-cloudstorage/tree/master/addon/locale/en-US

Thanks!

Comment 7

[Punam Dahiya [:pdahiya]]

Hi Mike
Looking at comment https://bugzilla.mozilla.org/show_bug.cgi?id=1391210#c10, cloud storage shield study (Bug 1388678)  has similar dependency on pref ‘cloud.services.api.enabled’ before cloud storage add-on starts. CloudStorageView init has CloudStorage API calls that will fail if pref is not true. 
https://github.com/mozilla/shield-cloudstorage/blob/master/addon/bootstrap.js#L105

Is it safe to assume shield will enable ‘cloud.services.api.enabled’ pref before running cloud storage add-on for all branches. Please confirm. Thanks

Comment 8

[Michael Cooper [:mythmon]]

Hi Punam,

Onboarding performed a different kind of study, where they were prepared to be installed as a system add-on, and then enabled by a Shield preference study. To me it looks like Cloud Storage is a add-on study, where Shield is only installing the add-on. In this case, Shield does not manipulate any preferences, and it is up to you to do that in the add-on. You'll likely want to do this using the variation chosen using shield-study-utils.

For example, in your control variation, it is your responsibility to have minimal impact on the browser, including not enabling the feature.

Comment 9

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated with cloud services API pref enabled by shield branch

Comment 10

[Punam Dahiya [:pdahiya]]

(In reply to Mike Cooper [:mythmon] from comment #8)
> Hi Punam,
> 
> Onboarding performed a different kind of study, where they were prepared to
> be installed as a system add-on, and then enabled by a Shield preference
> study. To me it looks like Cloud Storage is a add-on study, where Shield is
> only installing the add-on. In this case, Shield does not manipulate any
> preferences, and it is up to you to do that in the add-on. You'll likely
> want to do this using the variation chosen using shield-study-utils.
> 
> For example, in your control variation, it is your responsibility to have
> minimal impact on the browser, including not enabling the feature.

Thanks Mike, I updated attached xpi to check and enable ‘cloud.services.api.enabled’ pref for branches other than "control"

Comment 11

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated with multiprocess compatible check for 57 nightly

Comment 12

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated xpi to handle
a) Identified shield variations Fully persistent prompt, transient prompt (go away after configurable seconds), non-persistent prompt (disappear on click outside) , no prompt interval, prompt interval (from config.jsm)
b) Handle content in door hanger prompt for multiple downloads - this file ( 1 download) vs these files (multiple downloads) 
c) Updated cloudstorage.xml to extend download.xml

Mike, please provide your feedback. Thanks

Comment 13

[Punam Dahiya [:pdahiya]]

Hi Kamyar, Greg
Attached xpi when installed runs 'prompt_persistent_with_interval' variation, is there a way to configure the variation for testing, I tried giving higher weight to branch I would like to test but that didn't help.

https://github.com/mozilla/shield-cloudstorage/blob/master/addon/content/Config.jsm#L14

Thanks

Comment 14

[Punam Dahiya [:pdahiya]]

In Firefox 57 beta, StudyUtils.jsm throws errors https://pastebin.mozilla.org/9068778 when add-on is installed which are not seen in Firefox 56. Greg, Kamyar, thoughts?

Comment 15

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated to use latest StudyUtils.jsm

Comment 16

[Punam Dahiya [:pdahiya]]

Updated add-on to use recent StudyUtils.jsm pulled from https://github.com/mozilla/shield-studies-addon-utils, errors in pastebin in #comment 14 stays same for 57 beta. Please note these errors are limited to StudyUtils.jsm and not impacting the functionality of add-on. Thanks

Comment 17

[Mike Kaply [:mkaply]]

Comment on attachment 8913754 [details]
shield-cloudstorage.xpi

1. Why are there mutiple copies of the icons? They are all the same.
2. StudyUtils_old.jsm is in the XPI.
3. Extra _1 jsm files in the XPI.
4. const {classes: Cc, interfaces: Ci, utils: Cu} = Components; is a nice way to do all the var you have at the top of CloudStorageView.jsm
5. WindowMediator is available as Services.wm
6. doorHangerNotificiation is misspelled.
7. this.inProgressDownloads.size === 2 - What is this 2? Perhaps use a well named constant for this?
8. I wouldn't use spaces in filenames, just to be save. So use googledrive or google_drive as the provider name.
9. Nit - binding shouldn't be at the same level as bindings in cloudstorage.xml.
10. Do we need to worry about HiDpi (retina) icons?

I love how much simpler cloudstorage.xml is.
And you did a great job with comments.

Comment 18

[Punam Dahiya [:pdahiya]]

(In reply to Mike Kaply [:mkaply] from comment #17)
> Comment on attachment 8913754 [details]
> shield-cloudstorage.xpi
> 
> 1. Why are there mutiple copies of the icons? They are all the same.
> 2. StudyUtils_old.jsm is in the XPI.
> 3. Extra _1 jsm files in the XPI.
> 4. const {classes: Cc, interfaces: Ci, utils: Cu} = Components; is a nice
> way to do all the var you have at the top of CloudStorageView.jsm
> 5. WindowMediator is available as Services.wm
> 6. doorHangerNotificiation is misspelled.
> 7. this.inProgressDownloads.size === 2 - What is this 2? Perhaps use a well
> named constant for this?
> 8. I wouldn't use spaces in filenames, just to be save. So use googledrive
> or google_drive as the provider name.
> 9. Nit - binding shouldn't be at the same level as bindings in
> cloudstorage.xml.
> 10. Do we need to worry about HiDpi (retina) icons?
> 
> I love how much simpler cloudstorage.xml is.
> And you did a great job with comments.

Thanks Mike! Updated add-on with feedback
https://github.com/mozilla/shield-cloudstorage/commit/3069ead0da4c2a5244bfc4453c2ad307625879a4

Comment 19

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

PI updated with feedback

Comment 20

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated xpi with UI feedback

Comment 21

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

Updated XPI with below UI changes
- Cloud Storage Prompt points to downloads button in nav bar
- Showing Download Panel hides cloud storage prompt to avoid overlap
- Updated provider svg icons
- Icon placement updated in Download Panel and Download History

Comment 22

[Punam Dahiya [:pdahiya]]

Attachment: Add-on UI ScreenShots

Hi Seevan
As discussed, I have updated cloud storage prompt arrow point to downloads button. Attaching updated screenshots for UI feedback. Thanks!

Comment 23

[Sevaan Franks [:sevaan]]

Hey Punam,

That all looks great barring one nit. In the panel, the margin on the right of the Dropbox icon is larger than the margin on the right. They should be the same (just reduce the right margin to match the left).

After that, you've got a +r from me on the visuals.

Comment 24

[Sevaan Franks [:sevaan]]

Turns out my nit about the panels is just how the panels currently look and is not directly related to this implementation. Giving a +R

Comment 25

[Sevaan Franks [:sevaan]]

Comment on attachment 8915329 [details]
Add-on UI ScreenShots

Sorry, just caught one thing. There should not be a close "X" on the panel asking if you want to save it. The user's choice is what closes the panel.

Comment 26

[Kamyar Ardekani (he/him)]

(In reply to Punam Dahiya [:pdahiya] from comment #13)
> Hi Kamyar, Greg
> Attached xpi when installed runs 'prompt_persistent_with_interval'
> variation, is there a way to configure the variation for testing, I tried
> giving higher weight to branch I would like to test but that didn't help.
> 
> https://github.com/mozilla/shield-cloudstorage/blob/master/addon/content/
> Config.jsm#L14
> 
> Thanks

You can add a 'variation' property to the 'study' object in your Config.js file. This overrides the variation assignment. Example:

"study": {
    "studyName": "mostImportantExperiment", // no spaces, for all the reasons
    "variation": {
      "name": "kittens",
},

Comment 27

[mheubusch]

Hi - sorry for the last minute content request - main string looks good but can you change the label for the checkbox to read: Remember this decision

(the always isn't necessary, and we try not to use "my" or "me" in the product)

Also, am following up on the labels for the buttons and will post any changes to those separately

Comment 28

[Punam Dahiya [:pdahiya]]

(In reply to Sevaan Franks [:sevaan] from comment #25)
> Comment on attachment 8915329 [details]
> Add-on UI ScreenShots
> 
> Sorry, just caught one thing. There should not be a close "X" on the panel
> asking if you want to save it. The user's choice is what closes the panel.

Concern here is user might confuse label ‘Save locally’ and fail to relate that it’s same as x - close button, in which case its useful to show x.

Discussed this with Kamyar over IRC, it’s possible we can test x vs no x as two variations.

Comment 29

[Punam Dahiya [:pdahiya]]

(In reply to mheubusch from comment #27)
> Hi - sorry for the last minute content request - main string looks good but
> can you change the label for the checkbox to read: Remember this decision
> 
> (the always isn't necessary, and we try not to use "my" or "me" in the
> product)
> 
> Also, am following up on the labels for the buttons and will post any
> changes to those separately

Thanks Michelle, will update the label for checkbox. Looking forward to your feedback on button label which can help decide if we can safely hide x button.

Comment 30

[Punam Dahiya [:pdahiya]]

Thanks Justin for helping QA cloud storage add-on. Putting link in the bug to log QA results.

https://testrail.stage.mozaws.net/index.php?/plans/view/6526

https://public.etherpad-mozilla.org/p/cloudstorage_shield_QA

Will be updating API with fixes.

Comment 31

[Punam Dahiya [:pdahiya]]

Attachment: Add-on UI ScreenShots

Attaching door-hanger prompt updated with checkbox label updated and close 'x' button hidden as per feedback. In shield study, by default close button will be hidden and we will be testing x vs no x using a separate branch. Thanks!

Comment 32

[Punam Dahiya [:pdahiya]]

Attachment: shield-cloudstorage.xpi

pi updated with UI feedback and QA fixes

Comment 33

[Punam Dahiya [:pdahiya]]

Attached Cloud Storage xpi updated with below fixes: 

- For users with Google Drive as provider and download folder set to a custom path, exit without prompting after sending telemetry data 'gdrive_custom_path'

- Updated to record last prompt time when prompt is dismissed by clicking outside

- Due to PopUpNotifications API limitations, door-hanger notification might display outside window boundaries when window is minimized. Capture how many users sees prompt outside using telemetry ping 'prompt_alignment'.

- On Add-on Uninstall, users opted-in should continue using their download settings and is shown support page with study end message and instructions to reset download settings. Place holder support URL https://github.com/mozilla/shield-cloudstorage/blob/master/addon/Config.jsm#L55

- Post survey placeholder link in config.jsm, to be used when shield study ends.

By default attached xpi shows variation 'prompt_persistent' and has close button hidden and has no prompt interval set. These setting can be configured inside Config.jsm

Comment 34

[Justin [:JW_SoftvisionQA]]

Testing has been complete. All bugs have been resolved:

https://public.etherpad-mozilla.org/p/cloudstorage_shield_QA

The test cases can be found here:

https://testrail.stage.mozaws.net/index.php?/plans/view/6526

(To signin use LDAP info example: username: jwilliams and LDAP password)

QA Has approved this study to proceed.

Comment 35

[Punam Dahiya [:pdahiya]]

Thank you Justin, Sevaan! 

Hi Mike,

NI you for feedback if xpi need any final updates for signing. We do need to update place holder URLs in config.jsm with the final link to post survey and support page URL. There can be some string changes based on early feedback from pre-survey we are planning to launch this week. Thanks!

Comment 36

[Michael Cooper [:mythmon]]

Looking over the XPI from comment 32, I have a couple suggestions

1) The add-on id should end with "{study-name}@shield.mozilla.org". This is a new pattern that should help us identify Shield Studies in Telemetry in the future, and will become a hard requirement in the future.
2) The XPI file should be named "{full-addon-id}-{version}.xpi".
3) I noticed there are some .DS_Store files in the XPI. I'd recommend removing these.
4) There are some uses  of console.log I'd remove, in favor of the logging facilities provided by shield-study-utils.
5) There is some unneeded code, such as the Jsm object at the bottom of the file (it is used to load an unload modules, but the list of modules is empty). I'd recommend a pass over bootstrap.js and Config.jsm to find and remove dead, unused, or unneeded code. Removing unneeded fields from Config.jsm can make things easier to read and make this study a better example to future study authors.

I haven't signed this add-on, since comment #35 makes it seems there is some pending work to be done. Let me know when it needs signed, and I can do that. If QA needs a signed add-on, I can help with that.

Comment 37

[Punam Dahiya [:pdahiya]]

Attachment: [e-mail]

Comment 38

[Punam Dahiya [:pdahiya]]

Thanks Mike, I have updated xpi with feedback, has final post survey URL and is ready for signing.

Comment 39

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Thanks Punam, these changes look good. I've attached a signed version for you.

Comment 40

[Punam Dahiya [:pdahiya]]

Attachment: [e-mail]

Hi Mike
As per pre-survey feedback, we are removing close button variations from shield study. Updated xpi with below changes 

1) Removed close button variations in config.jsm
2) Updated xpi version to 1.0.1
3) Added explicit return when shield study expires in bootstrap.js
https://github.com/mozilla/shield-cloudstorage/blob/master/addon/bootstrap.js#L96

Please sign attached xpi. Thanks

Comment 41

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Signed.

Comment 42

[Punam Dahiya [:pdahiya]]

Attachment: [e-mail]

Hi Mike
Sorry for multiple sign requests. We need to update add-on with last change to enable telemetry pings message:”download_started” and message: “addon_init” for control branch.

The commit with changeset
https://github.com/mozilla/shield-cloudstorage/commit/af0dbc2b6a06bd4119130bf8e41207559d460fdc


Will appreciate if you can help sign updated xpi. Thanks

Comment 43

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Signed.

Comment 44

[Matt Grimes [:Matt_G]]

Hey Punam. Now that this study has ended can you please recap the outcome here and close the bug? A couple sentences is fine.

Comment 45

[Punam Dahiya [:pdahiya]]

Hi Matt

Please find below this study high level findings

- 12-14% of enrolled users engaged and used cloud storage feature

- 1.2-1.5% of users permanently opted-in to always save their downloads to cloud storage provider folder

- In post study survey, 60% of users who saw this feature responded with ‘Yes’ when asked if they would like to use this feature

- Want to access your downloads on mobile device has > 60% with ‘often, sometimes and rarely’ as response, hinting sync downloads to mobile as desired feature

Based on findings and user feedback from this study, next steps involve validate with
a) Improved opt-in experience by refining text and replace door-hangar prompts with UI that doesn’t seem interruptive
b) Provide user with option to save downloads individually to their provider folder
b) Syncing downloads to user other devices - Bug 1432349

Thanks!