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

RESOLVED FIXED

Status

RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

1.19 MB, application/zip
sevaan
: feedback+
Details
50.31 KB, application/x-xpinstall
Details
54.69 KB, application/x-xpinstall
Details
Comment hidden (empty)
(Assignee)

Comment 1

a year ago
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
(Assignee)

Comment 2

a year ago
Created attachment 8907237 [details]
shield-cloudstorage.xpi

Attaching XPI with prompt interval set to ~2mins
(Assignee)

Updated

a year ago
Blocks: 1399198
(Assignee)

Comment 3

a year ago
Created attachment 8908276 [details]
shield-cloudstorage.xpi

Updated add-on with telemetry, schema and license
Attachment #8907237 - Attachment is obsolete: true
(Assignee)

Comment 4

a year ago
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
(Assignee)

Updated

a year ago
Attachment #8908276 - Flags: feedback?(mozilla)
(Assignee)

Comment 5

a year ago
Created attachment 8908917 [details]
shield-cloudstorage.xpi

Updated with download prefs and providers keys telemetry data
Attachment #8908276 - Attachment is obsolete: true
Attachment #8908276 - Flags: feedback?(mozilla)
Attachment #8908917 - Flags: feedback?(mozilla)
(Assignee)

Comment 6

a year ago
Created attachment 8909441 [details]
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!
Attachment #8909441 - Flags: feedback?(mheubusch)
(Assignee)

Comment 7

a year ago
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
Flags: needinfo?(mcooper)
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.
Flags: needinfo?(mcooper)
(Assignee)

Comment 9

a year ago
Created attachment 8910065 [details]
shield-cloudstorage.xpi

Updated with cloud services API pref enabled by shield branch
Attachment #8908917 - Attachment is obsolete: true
Attachment #8908917 - Flags: feedback?(mozilla)
Attachment #8910065 - Flags: feedback?(mozilla)
(Assignee)

Comment 10

a year ago
(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"
(Assignee)

Comment 11

a year ago
Created attachment 8910883 [details]
shield-cloudstorage.xpi

Updated with multiprocess compatible check for 57 nightly
Attachment #8910065 - Attachment is obsolete: true
Attachment #8910065 - Flags: feedback?(mozilla)
(Assignee)

Comment 12

a year ago
Created attachment 8912069 [details]
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
Attachment #8910883 - Attachment is obsolete: true
Attachment #8912069 - Flags: feedback?(mozilla)
(Assignee)

Comment 13

a year ago
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
Flags: needinfo?(kardekani)
Flags: needinfo?(glind)
(Assignee)

Comment 14

a year ago
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?
(Assignee)

Comment 15

a year ago
Created attachment 8913754 [details]
shield-cloudstorage.xpi

Updated to use latest StudyUtils.jsm
Attachment #8912069 - Attachment is obsolete: true
Attachment #8912069 - Flags: feedback?(mozilla)
(Assignee)

Updated

a year ago
Attachment #8913754 - Flags: feedback?(mozilla)
(Assignee)

Comment 16

a year ago
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 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.
Attachment #8913754 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 18

a year ago
(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
(Assignee)

Comment 19

a year ago
Created attachment 8914557 [details]
shield-cloudstorage.xpi

XPI updated with feedback
Attachment #8913754 - Attachment is obsolete: true
Attachment #8914557 - Flags: feedback+
(Assignee)

Updated

a year ago
Attachment #8914557 - Flags: feedback+
(Assignee)

Comment 20

a year ago
Created attachment 8915198 [details]
shield-cloudstorage.xpi

Updated xpi with UI feedback
Attachment #8914557 - Attachment is obsolete: true
(Assignee)

Comment 21

a year ago
Created attachment 8915323 [details]
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
Attachment #8915198 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
Created attachment 8915329 [details]
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!
Attachment #8909441 - Attachment is obsolete: true
Attachment #8909441 - Flags: feedback?(mheubusch)
Attachment #8915329 - Flags: feedback?(sfranks)
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.
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
Attachment #8915329 - Flags: feedback?(sfranks) → feedback+
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.
Attachment #8915329 - Flags: feedback+ → feedback-

Comment 26

a year ago
(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",
},
Flags: needinfo?(kardekani)

Comment 27

a year ago
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

Updated

a year ago
Flags: needinfo?(glind)
(Assignee)

Comment 28

a year ago
(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.
(Assignee)

Comment 29

a year ago
(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.
(Assignee)

Comment 30

a year ago
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.
(Assignee)

Comment 31

a year ago
Created attachment 8917980 [details]
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!
Attachment #8915329 - Attachment is obsolete: true
Attachment #8917980 - Flags: feedback?(sfranks)
(Assignee)

Comment 32

a year ago
Created attachment 8917995 [details]
shield-cloudstorage.xpi

Xpi updated with UI feedback and QA fixes
Attachment #8915323 - Attachment is obsolete: true
(Assignee)

Comment 33

a year ago
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
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.
Attachment #8917980 - Flags: feedback?(sfranks) → feedback+
(Assignee)

Comment 35

a year ago
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!
Flags: needinfo?(mcooper)
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.
Flags: needinfo?(mcooper)
(Assignee)

Comment 38

a year ago
Thanks Mike, I have updated xpi with feedback, has final post survey URL and is ready for signing.
Flags: needinfo?(mcooper)
Created attachment 8919526 [details]
cloud-storage-study@shield.mozilla.org-v1.0-signed.xpi

Thanks Punam, these changes look good. I've attached a signed version for you.
Flags: needinfo?(mcooper)
(Assignee)

Comment 40

a year ago
Created attachment 8921223 [details]
cloud-storage-study@shield.mozilla.org-v1.0.1.xpi

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
Attachment #8919512 - Attachment is obsolete: true
Attachment #8919526 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Flags: needinfo?(mcooper)
(Assignee)

Comment 42

a year ago
Created attachment 8922119 [details]
cloud-storage-study@shield.mozilla.org-v1.0.2.xpi

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
Attachment #8921223 - Attachment is obsolete: true
Attachment #8921242 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Flags: needinfo?(mcooper)
Hey Punam. Now that this study has ended can you please recap the outcome here and close the bug? A couple sentences is fine.
Flags: needinfo?(pdahiya)
(Assignee)

Comment 45

9 months ago
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!
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Flags: needinfo?(pdahiya)
Resolution: --- → FIXED
(Assignee)

Updated

7 months ago
Blocks: 1357160
You need to log in before you can comment on or make changes to this bug.