search

mozilla / security-advisor-shield-study

Mozilla Public License 2.0
2 stars 7 forks source link

Fix #59: End the study if users agree to sign up for sync. #60

Closed Osmose closed 7 years ago

Osmose commented 7 years ago

Also includes some fixes for previously-existing ways for ending the study. The Advisor class is now an EventEmitter (copied from the SHIELD utils lib because it's not exported) and emits events when the user agrees to the study, asks to disable it completely, or signs up for sync while the study is running.

Because the SHIELD utils don't currently have a good way for ending a study with a custom reason, we manually log the final telemetry packet, trigger the survey, and end the study.

@groovecoder Are you available to r? this before the end of the week? We're hoping to push this out next Thursday and I'd like to get a copy out to S&I before the weekend. If not, just lemme know and I'll bug someone else. :D

Osmose commented 7 years ago

@casebenton r?

casebenton commented 7 years ago

@Osmose When the study ends because I've signed up for sync, the exit survey opens two times, in a new tab and in a new window. One of the surveys lists the reason in the uri as "user-ended-study" and the other as "user-converted-no-offer". In telemetry, it looks like there are two end payloads, one for "user-ended-study" and "user-converted-no-offer".

It looks like any event that ends the study early, like disabling notifications or signing up for sync, will trigger the "user-ended-study" survey. In these cases we also present the custom survey to users, explaining why I saw two survey windows in the testing described above.

Other than that issue, I think that everything looks good! r+wc!

Do you know of any ways that we can disable the "user-ended-study" event when any of these special cases occur? It looks like we can't just remove it, since then there would be nothing to handle the case when users manually uninstall the add-on.

casebenton commented 7 years ago

Looks good! The surveys and telemetry logging seem to be correct now. r+