Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Sep 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/7473.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added a prompt asking users to enroll back in the insiders program
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
"ExtensionChannels.reloadToUseInsidersMessage": "Please reload Visual Studio Code to use the insiders build of the Python extension.",
"ExtensionChannels.downloadCompletedOutputMessage": "Insiders build download complete.",
"ExtensionChannels.startingDownloadOutputMessage": "Starting download for Insiders build.",
"ExtensionChannels.optIntoProgramAgainMessage": "It looks like you were previously in the Insiders Program of the Python extension. Would you like to opt into the program again?",
"Interpreters.environmentPromptMessage": "We noticed a new virtual environment has been created. Do you want to select it for the workspace folder?",
"DataScience.restartKernelMessage": "Do you want to restart the IPython kernel? All variables will be lost.",
"DataScience.restartKernelMessageYes": "Yes",
Expand Down
41 changes: 27 additions & 14 deletions src/client/common/insidersBuild/insidersExtensionPrompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,30 @@ import { noop } from '../utils/misc';
import { IExtensionChannelService, IInsiderExtensionPrompt } from './types';

export const insidersPromptStateKey = 'INSIDERS_PROMPT_STATE_KEY';
export const optIntoInsidersPromptAgainStateKey = 'OPT_INTO_INSIDERS_PROGRAM_AGAIN_STATE_KEY';

@injectable()
export class InsidersExtensionPrompt implements IInsiderExtensionPrompt {
public readonly hasUserBeenNotified: IPersistentState<boolean>;
public readonly hasUserBeenAskedToOptInAgain: IPersistentState<boolean>;
constructor(
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IExtensionChannelService) private readonly insidersDownloadChannelService: IExtensionChannelService,
@inject(ICommandManager) private readonly cmdManager: ICommandManager,
@inject(IPersistentStateFactory) private readonly persistentStateFactory: IPersistentStateFactory
) {
this.hasUserBeenNotified = this.persistentStateFactory.createGlobalPersistentState(insidersPromptStateKey, false);
this.hasUserBeenAskedToOptInAgain = this.persistentStateFactory.createGlobalPersistentState(optIntoInsidersPromptAgainStateKey, false);
}

@traceDecorators.error('Error in prompting to install insiders')
public async notifyToInstallInsiders(): Promise<void> {
const prompts = [ExtensionChannels.yesWeekly(), ExtensionChannels.yesDaily(), DataScienceSurveyBanner.bannerLabelNo()];
const telemetrySelections: ['Yes, weekly', 'Yes, daily', 'No, thanks'] = ['Yes, weekly', 'Yes, daily', 'No, thanks'];
const selection = await this.appShell.showInformationMessage(ExtensionChannels.promptMessage(), ...prompts);
sendTelemetryEvent(EventName.INSIDERS_PROMPT, undefined, { selection: selection ? telemetrySelections[prompts.indexOf(selection)] : undefined });
await this.hasUserBeenNotified.updateValue(true);
if (!selection) {
return;
}
if (selection === ExtensionChannels.yesWeekly()) {
await this.insidersDownloadChannelService.updateChannel('weekly');
} else if (selection === ExtensionChannels.yesDaily()) {
await this.insidersDownloadChannelService.updateChannel('daily');
}
public async promptToInstallInsiders(): Promise<void> {
await this.promptAndUpdate(ExtensionChannels.promptMessage(), this.hasUserBeenNotified, EventName.INSIDERS_PROMPT);
}

@traceDecorators.error('Error in prompting to enroll back to insiders program')
public async promptToEnrollBackToInsiders(): Promise<void> {
await this.promptAndUpdate(ExtensionChannels.optIntoProgramAgainMessage(), this.hasUserBeenAskedToOptInAgain, EventName.OPT_INTO_INSIDERS_AGAIN_PROMPT);
}

@traceDecorators.error('Error in prompting to reload')
Expand All @@ -54,4 +51,20 @@ export class InsidersExtensionPrompt implements IInsiderExtensionPrompt {
this.cmdManager.executeCommand('workbench.action.reloadWindow').then(noop);
}
}

private async promptAndUpdate(message: string, hasPromptBeenShownAlreadyState: IPersistentState<boolean>, telemetryEventKey: EventName.INSIDERS_PROMPT | EventName.OPT_INTO_INSIDERS_AGAIN_PROMPT) {
const prompts = [ExtensionChannels.yesWeekly(), ExtensionChannels.yesDaily(), DataScienceSurveyBanner.bannerLabelNo()];
const telemetrySelections: ['Yes, weekly', 'Yes, daily', 'No, thanks'] = ['Yes, weekly', 'Yes, daily', 'No, thanks'];
const selection = await this.appShell.showInformationMessage(message, ...prompts);
sendTelemetryEvent(telemetryEventKey, undefined, { selection: selection ? telemetrySelections[prompts.indexOf(selection)] : undefined });
await hasPromptBeenShownAlreadyState.updateValue(true);
if (!selection) {
return;
}
if (selection === ExtensionChannels.yesWeekly()) {
await this.insidersDownloadChannelService.updateChannel('weekly');
} else if (selection === ExtensionChannels.yesDaily()) {
await this.insidersDownloadChannelService.updateChannel('daily');
}
}
}
62 changes: 53 additions & 9 deletions src/client/common/insidersBuild/insidersExtensionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ export class InsidersExtensionService implements IExtensionSingleActivationServi
public async activate() {
this.registerCommandsAndHandlers();
const installChannel = this.extensionChannelService.getChannel();
await this.handleEdgeCases(installChannel);
const alreadyHandled = await this.handleEdgeCases(installChannel);
if (alreadyHandled) {
// Simply return if channel is already handled and doesn't need further handling
return;
}
this.handleChannel(installChannel).ignoreErrors();
}

Expand All @@ -45,15 +49,17 @@ export class InsidersExtensionService implements IExtensionSingleActivationServi

/**
* Choose what to do in miscellaneous situations
* * 'Notify to install insiders prompt' - Only when using VSC insiders and if they have not been notified before (usually the first session)
* * 'Resolve discrepency' - When install channel is not in sync with what is installed.
* @returns `true` if install channel is handled in these miscellaneous cases, `false` if install channel needs further handling
*/
public async handleEdgeCases(installChannel: ExtensionChannels): Promise<void> {
if (this.appEnvironment.channel === 'insiders' && !this.insidersPrompt.hasUserBeenNotified.value && this.extensionChannelService.isChannelUsingDefaultConfiguration) {
await this.insidersPrompt.notifyToInstallInsiders();
} else if (installChannel !== 'off' && this.appEnvironment.extensionChannel === 'stable') {
// Install channel is set to "weekly" or "daily" but stable version of extension is installed. Switch channel to "off" to use the installed version
await this.extensionChannelService.updateChannel('off');
public async handleEdgeCases(installChannel: ExtensionChannels): Promise<boolean> {
if (await this.promptToEnrollBackToInsidersIfApplicable(installChannel)) {
return true;
} else if (await this.promptToInstallInsidersIfApplicable()) {
return true;
} else if (await this.setInsidersChannelToOffIfApplicable(installChannel)) {
return true;
} else {
return false;
}
}

Expand All @@ -63,4 +69,42 @@ export class InsidersExtensionService implements IExtensionSingleActivationServi
this.disposables.push(this.cmdManager.registerCommand(Commands.SwitchToInsidersDaily, () => this.extensionChannelService.updateChannel('daily')));
this.disposables.push(this.cmdManager.registerCommand(Commands.SwitchToInsidersWeekly, () => this.extensionChannelService.updateChannel('weekly')));
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';
        }
    }

Copy link

@DonJayamanne DonJayamanne Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁

* If previously in the Insiders Program but not now, request them enroll in the program again
* @returns `true` if prompt is shown, `false` otherwise
*/
private async promptToEnrollBackToInsidersIfApplicable(installChannel: ExtensionChannels): Promise<boolean> {
if (installChannel === 'off' && !this.extensionChannelService.isChannelUsingDefaultConfiguration) {
// If install channel is explicitly set to off, it means that user has used the insiders program before
await this.insidersPrompt.promptToEnrollBackToInsiders();
return true;
}
return false;
}

/**
* Only when using VSC insiders and if they have not been notified before (usually the first session), notify to enroll into the insiders program
* @returns `true` if prompt is shown, `false` otherwise
*/
private async promptToInstallInsidersIfApplicable(): Promise<boolean> {
if (this.appEnvironment.channel === 'insiders' && !this.insidersPrompt.hasUserBeenNotified.value && this.extensionChannelService.isChannelUsingDefaultConfiguration) {
await this.insidersPrompt.promptToInstallInsiders();
return true;
}
return false;
}

/**
* When install channel is not in sync with what is installed, resolve discrepency by setting channel to "off"
* @returns `true` if channel is set to off, `false` otherwise
*/
private async setInsidersChannelToOffIfApplicable(installChannel: ExtensionChannels): Promise<boolean> {
if (installChannel !== 'off' && this.appEnvironment.extensionChannel === 'stable') {
// Install channel is set to "weekly" or "daily" but stable version of extension is installed. Switch channel to "off" to use the installed version
await this.extensionChannelService.updateChannel('off');
return true;
}
return false;
}
}
3 changes: 2 additions & 1 deletion src/client/common/insidersBuild/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ export interface IInsiderExtensionPrompt {
* Gets updated to `true` once user has been prompted to install insiders.
*/
readonly hasUserBeenNotified: IPersistentState<boolean>;
notifyToInstallInsiders(): Promise<void>;
promptToInstallInsiders(): Promise<void>;
promptToEnrollBackToInsiders(): Promise<void>;
promptToReload(): Promise<void>;
}

Expand Down
1 change: 1 addition & 0 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export namespace ExtensionChannels {
export const yesWeekly = localize('ExtensionChannels.yesWeekly', 'Yes, weekly');
export const yesDaily = localize('ExtensionChannels.yesDaily', 'Yes, daily');
export const promptMessage = localize('ExtensionChannels.promptMessage', 'We noticed you are using Visual Studio Code Insiders. Would you like to use the Insiders build of the Python extension?');
export const optIntoProgramAgainMessage = localize('ExtensionChannels.optIntoProgramAgainMessage', 'It looks like you were previously in the Insiders Program of the Python extension. Would you like to opt into the program again?');
export const reloadToUseInsidersMessage = localize('ExtensionChannels.reloadToUseInsidersMessage', 'Please reload Visual Studio Code to use the insiders build of the Python extension.');
export const downloadCompletedOutputMessage = localize('ExtensionChannels.downloadCompletedOutputMessage', 'Insiders build download complete.');
export const startingDownloadOutputMessage = localize('ExtensionChannels.startingDownloadOutputMessage', 'Starting download for Insiders build.');
Expand Down
1 change: 1 addition & 0 deletions src/client/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export enum EventName {
PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT',
INSIDERS_RELOAD_PROMPT = 'INSIDERS_RELOAD_PROMPT',
INSIDERS_PROMPT = 'INSIDERS_PROMPT',
OPT_INTO_INSIDERS_AGAIN_PROMPT = 'OPT_INTO_INSIDERS_AGAIN_PROMPT',
ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION',
WORKSPACE_SYMBOLS_BUILD = 'WORKSPACE_SYMBOLS.BUILD',
WORKSPACE_SYMBOLS_GO_TO = 'WORKSPACE_SYMBOLS.GO_TO',
Expand Down
20 changes: 15 additions & 5 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -942,11 +942,21 @@ export interface IEventNamePropertyMapping {
*/
[EventName.INSIDERS_PROMPT]: {
/**
* @type {'Yes, weekly'} When user selects to use "weekly" as extension channel in insiders prompt
* @type {'Yes, daily'} When user selects to use "daily" as extension channel in insiders prompt
* @type {'No, thanks'} When user decides to keep using the same extension channel as before
*
* @type {('Yes, weekly' | 'Yes, daily' | 'No, thanks' | undefined)}
* `Yes, weekly` When user selects to use "weekly" as extension channel in insiders prompt
* `Yes, daily` When user selects to use "daily" as extension channel in insiders prompt
* `No, thanks` When user decides to keep using the same extension channel as before
*/
selection: 'Yes, weekly' | 'Yes, daily' | 'No, thanks' | undefined;
};
/**
* Telemetry event sent with details when user clicks a button in the following prompt
* `Prompt message` :- 'It looks like you were previously in the Insiders Program of the Python extension. Would you like to opt into the program again?'
*/
[EventName.OPT_INTO_INSIDERS_AGAIN_PROMPT]: {
/**
* `Yes, weekly` When user selects to use "weekly" as extension channel
* `Yes, daily` When user selects to use "daily" as extension channel
* `No, thanks` When user decides to keep using the same extension channel as before
*/
selection: 'Yes, weekly' | 'Yes, daily' | 'No, thanks' | undefined;
};
Expand Down
Loading