Skip to content
This repository has been archived by the owner on Oct 14, 2020. It is now read-only.

Despite claims to the contrary by me, Disable doesn't actually force uninstall. #247

Closed
gregglind opened this issue Jul 12, 2018 · 4 comments · Fixed by #297
Closed

Despite claims to the contrary by me, Disable doesn't actually force uninstall. #247

gregglind opened this issue Jul 12, 2018 · 4 comments · Fixed by #297
Labels
fixed-elsewhere Eg irrelevant with built-in Normandy support pr submitted

Comments

@gregglind
Copy link
Contributor

  • issue: disable ALSO shuts down the getApi, so calling disable DOESN't uninstall the addon, because the signal doesn't go through.

TO FIX: make the uninstall happen in the onShutdown method.

@motin?

@motin
Copy link
Contributor

motin commented Jul 18, 2018

If we can't signal onEndStudy to the add-on after user-disable, then yes, we need to at least uninstall in onShutdown.

Related: https://github.com/motin/taar-experiment-v3-shield-study passed QA with the following comment:

We have finished testing the TAAR Experiment v3 and the following issues are still reproducible:

QA’s recommendation: YELLOW - SHIP IT CONDITIONALLY

Reasoning:

As per previous discussions about this behavior, Shield Studies were not designed to be disabled and then re-enabled. That would cause weird issues with the data, so once you've opted out of a particular study you cannot re-enroll.

Since this behavior is in discussion again and no final decision was made, we are suggesting to discuss this behavior and decide if it blocks or not the launch. Considering this, we recommend contacting Matt Grimes regarding a decision in the case.

Other than this, the experiment looks really good.

@motin
Copy link
Contributor

motin commented Jul 28, 2018

Any ETA on this? This affects all studies that use v5 utils, not allowing them to be greenlighted by QA

@motin
Copy link
Contributor

motin commented Jul 28, 2018

Related: #231

@motin
Copy link
Contributor

motin commented Jul 29, 2018

@gregglind Could you devise a patch for this and push it to release/5.0.4?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-elsewhere Eg irrelevant with built-in Normandy support pr submitted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants