Added a prompt asking users to enroll back in the insiders program

[karrtikr]

For #7473

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Has a news entry file (remember to thank yourself!)
• Appropriate comments and documentation strings in the code
• Has sufficient logging.
• Has telemetry for enhancements.
• Unit tests & system/integration tests are added/updated
• [ ] Test plan is updated as appropriate
• [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)
• [ ] The wiki is updated with any design decisions/details.

[codecov-io]

Codecov Report

Merging #7484 into master will increase coverage by 0.23%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7484      +/-   ##
==========================================
+ Coverage   58.51%   58.75%   +0.23%     
==========================================
  Files         494      494              
  Lines       21828    21862      +34     
  Branches     3499     3508       +9     
==========================================
+ Hits        12773    12845      +72     
+ Misses       8264     8216      -48     
- Partials      791      801      +10

Impacted Files	Coverage Δ
src/client/telemetry/index.ts	86.13% <ø> (ø)	⬆️
src/client/common/insidersBuild/types.ts	100% <ø> (ø)	⬆️
...t/common/insidersBuild/insidersExtensionService.ts	100% <100%> (ø)	⬆️
src/client/telemetry/constants.ts	100% <100%> (ø)	⬆️
...nt/common/insidersBuild/insidersExtensionPrompt.ts	97.5% <100%> (+0.35%)	⬆️
src/client/common/utils/localize.ts	93.86% <100%> (+0.01%)	⬆️
src/client/testing/explorer/commandHandlers.ts	77.27% <0%> (-0.51%)	⬇️
src/client/testing/nosetest/runner.ts	18.86% <0%> (ø)	⬆️
...nt/testing/common/services/discoveredTestParser.ts	16.1% <0%> (ø)	⬆️
...rc/client/testing/explorer/testTreeViewProvider.ts	66.21% <0%> (ø)	⬆️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb1b9de...ce08248. Read the comment docs.

[DonJayamanne]

Please get it reviewed by someone else as well.

I have a few concerns about:

• The structure of the tests (difficult to follow as we've tried to make it very dynamic)and some code. Again, please read my comments. Personally I'm not a big fan of trying to make tests dynamic, makes it difficult to follow (all for the benefit of code reuse in tests!)
• Some of the code seems too complicated (too many if conditions)

[ericsnowcurrently]

Mostly LGTM. There are a number of things you can do to make this code a bit easier to follow.

[ericsnowcurrently]

I see where you're coming from here, but I'm not sure these 3 new methods pull their own weight here. There's just not enough meat to them to pay for the extra cost to readers (e.g. extra boilerplate code, more indirection). Consider folding them back into handleEdgeCases() and perhaps even folding handleEdgeCases() into handleChannel(). Here's one way to do that without having the clutter of the conditions:

    public activate() {
        this.registerCommandsAndHandlers();
        this.handleChannel();
    }

    private handleChannel() {
        const installChannel = this.extensionChannelService.getChannel();
        switch (this.decideAction(installChannel)) {
            case 'promptToReEnroll':
                await this.insidersPrompt.promptToEnrollBackToInsiders();
                break;
            case 'promptToEnroll':
                await this.insidersPrompt.promptToInstallInsiders();
                break;
            case 'unEnroll':
                await this.extensionChannelService.updateChannel('off');
                break;
            case 'install':
                await this.installChannel(installChannel);
                break;
        }
    }

    private decideAction(installChannel: ExtensionChannels): string {
        if (installChannel === 'off' && !this.extensionChannelService.isChannelUsingDefaultConfiguration) {
            return 'promptToReEnroll';
        } else if (this.appEnvironment.channel === 'insiders' && !this.insidersPrompt.hasUserBeenNotified.value && this.extensionChannelService.isChannelUsingDefaultConfiguration) {
            return 'promptToEnroll';
        } else if (installChannel !== 'off' && this.appEnvironment.extensionChannel === 'stable') {
            return 'unEnroll';
        } else {
            return 'install';
        }
    }

[DonJayamanne]

To me each action changes based on the current state, I.e. state pattern. Gets rid of reach case statement and the complicated if condition.

E.g. tomorrow if we have another state we still modify both if conditions and the case statement, that's code smell.

Yes it might seem unnecessary, however I feel that code will be easier to follow, and each class does one thing. To me right now the following block alone is complicated, if (installChannel === 'off' && !this.extensionChannelService.isChannelUsingDefaultConfiguration) { return 'promptToReEnroll'; }, and to me it gets more complicated when you have more of those.

And one way to solve that is if this.shouldPromptToReEnroll, then we end up with around 4 different methods that basically return a state.
The break down further, we have a state of the class, next simply further using state pattern.

I like that because the state describes the next course of action, that's what we need. We have users who are in a particular state and we need to do something based on users state of extension.

[DonJayamanne]

We could also go with a strategy pattern here, that would still simplify the code and accommodate new methods (instead of the switch statement). However looking at the two, state is there right fit (at least IMHO).
Basically both solve O. In SOLID.

[karrtikr]

Was referring to @ericsnowcurrently 's review earlier :

So many if conditions in one place leads to harder testability and readability , which was the main reason for doing that. Having documentation for separate cases is also better when you use separate methods, rather than using if statements.

(Not to say I would've to refactor the entire test code as well, does not seem to be worth the effort for me)

[DonJayamanne]

Having documentation for separate cases is also better when you use separate methods, rather than using if statements.

Agreed, we could go another step, self documenting code. Separate classes would make documentation unnecessary 😁

[DonJayamanne]

Revised code is much better, and easier to follow. Would still prefer a state pattern or similar, but the over so objective has been achieved - simplification of code.

I leave further simplification upto you.

Hopefully we won't have further changes based on users current state of install of extension 😉

Addressed as I saw fit