Ensure the prompt to install missing packages is displayed only once (#1649)

DonJayamanne · web-flow · commit ca854b88bfd4 · 2018-05-21T10:18:36.000-07:00
* Ensure the prompt to install missing packages is displayed only once * Add missing dependency * Fixes #980
diff --git a/news/2 Fixes/980 b/news/2 Fixes/980
@@ -0,0 +1 @@
+Ensure the prompt to install missing packages is not displayed more than once.
diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts
@@ -2,11 +2,12 @@ import { inject, injectable, named } from 'inversify';
 import * as os from 'os';
 import * as path from 'path';
 import * as vscode from 'vscode';
+import '../../common/extensions';
 import { IFormatterHelper } from '../../formatters/types';
 import { IServiceContainer } from '../../ioc/types';
 import { ILinterManager } from '../../linters/types';
 import { ITestsHelper } from '../../unittests/common/types';
-import { IApplicationShell } from '../application/types';
+import { IApplicationShell, IWorkspaceService } from '../application/types';
 import { STANDARD_OUTPUT_CHANNEL } from '../constants';
 import { IPlatformService } from '../platform/types';
 import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
@@ -28,16 +29,34 @@ enum ProductType {
 }
 
 // tslint:disable-next-line:max-classes-per-file
-abstract class BaseInstaller {
+export abstract class BaseInstaller {
+    private static readonly PromptPromises = new Map<string, Promise<InstallerResponse>>();
     protected appShell: IApplicationShell;
     protected configService: IConfigurationService;
+    private readonly workspaceService: IWorkspaceService;
 
     constructor(protected serviceContainer: IServiceContainer, protected outputChannel: vscode.OutputChannel) {
         this.appShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
         this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
+        this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
     }
 
-    public abstract promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse>;
+    public promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+        // If this method gets called twice, while previous promise has not been resolved, then return that same promise.
+        // E.g. previous promise is not resolved as a message has been displayed to the user, so no point displaying
+        // another message.
+        const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined;
+        const key = `${product}${workspaceFolder ? workspaceFolder.uri.fsPath : ''}`;
+        if (BaseInstaller.PromptPromises.has(key)) {
+            return BaseInstaller.PromptPromises.get(key)!;
+        }
+        const promise = this.promptToInstallImplementation(product, resource);
+        BaseInstaller.PromptPromises.set(key, promise);
+        promise.then(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
+        promise.catch(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
+
+        return promise;
+    }
 
     public async install(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         if (product === Product.unittest) {
@@ -83,22 +102,17 @@ abstract class BaseInstaller {
                 .catch(() => false);
         }
     }
-
+    protected abstract promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse>;
     protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string {
         throw new Error('getExecutableNameFromSettings is not supported on this object');
     }
 }
 
-class CTagsInstaller extends BaseInstaller {
+export class CTagsInstaller extends BaseInstaller {
     constructor(serviceContainer: IServiceContainer, outputChannel: vscode.OutputChannel) {
         super(serviceContainer, outputChannel);
     }
 
-    public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
-        const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No');
-        return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
-    }
-
     public async install(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         if (this.serviceContainer.get<IPlatformService>(IPlatformService).isWindows) {
             this.outputChannel.appendLine('Install Universal Ctags Win32 to enable support for Workspace Symbols');
@@ -115,15 +129,19 @@ class CTagsInstaller extends BaseInstaller {
         }
         return InstallerResponse.Ignore;
     }
+    protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+        const item = await this.appShell.showErrorMessage('Install CTags to enable Python workspace symbols?', 'Yes', 'No');
+        return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
+    }
 
     protected getExecutableNameFromSettings(product: Product, resource?: vscode.Uri): string {
         const settings = this.configService.getSettings(resource);
         return settings.workspaceSymbols.ctagsPath;
     }
 }
 
-class FormatterInstaller extends BaseInstaller {
-    public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+export class FormatterInstaller extends BaseInstaller {
+    protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         // Hard-coded on purpose because the UI won't necessarily work having
         // another formatter.
         const formatters = [Product.autopep8, Product.black, Product.yapf];
@@ -159,8 +177,8 @@ class FormatterInstaller extends BaseInstaller {
 }
 
 // tslint:disable-next-line:max-classes-per-file
-class LinterInstaller extends BaseInstaller {
-    public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+export class LinterInstaller extends BaseInstaller {
+    protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         const productName = ProductNames.get(product)!;
         const install = 'Install';
         const disableAllLinting = 'Disable linting';
@@ -188,8 +206,8 @@ class LinterInstaller extends BaseInstaller {
 }
 
 // tslint:disable-next-line:max-classes-per-file
-class TestFrameworkInstaller extends BaseInstaller {
-    public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+export class TestFrameworkInstaller extends BaseInstaller {
+    protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         const productName = ProductNames.get(product)!;
         const item = await this.appShell.showErrorMessage(`Test framework ${productName} is not installed. Install?`, 'Yes', 'No');
         return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
@@ -208,8 +226,8 @@ class TestFrameworkInstaller extends BaseInstaller {
 }
 
 // tslint:disable-next-line:max-classes-per-file
-class RefactoringLibraryInstaller extends BaseInstaller {
-    public async promptToInstall(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
+export class RefactoringLibraryInstaller extends BaseInstaller {
+    protected async promptToInstallImplementation(product: Product, resource?: vscode.Uri): Promise<InstallerResponse> {
         const productName = ProductNames.get(product)!;
         const item = await this.appShell.showErrorMessage(`Refactoring library ${productName} is not installed. Install?`, 'Yes', 'No');
         return item === 'Yes' ? this.install(product, resource) : InstallerResponse.Ignore;
diff --git a/src/client/linters/errorHandlers/notInstalled.ts b/src/client/linters/errorHandlers/notInstalled.ts
@@ -1,5 +1,4 @@
 import { OutputChannel, Uri } from 'vscode';
-import { isNotInstalledError } from '../../common/helpers';
 import { IPythonExecutionFactory } from '../../common/process/types';
 import { ExecutionInfo, Product } from '../../common/types';
 import { IServiceContainer } from '../../ioc/types';
diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts
@@ -1,7 +1,7 @@
 import * as path from 'path';
 import * as TypeMoq from 'typemoq';
 import { ConfigurationTarget, Uri } from 'vscode';
-import { IApplicationShell } from '../../client/common/application/types';
+import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types';
 import { ConfigurationService } from '../../client/common/configuration/service';
 import { EnumEx } from '../../client/common/enumUtils';
 import { createDeferred } from '../../client/common/helpers';
@@ -58,6 +58,10 @@ suite('Installer', () => {
         ioc.serviceManager.addSingletonInstance<IApplicationShell>(IApplicationShell, TypeMoq.Mock.ofType<IApplicationShell>().object);
         ioc.serviceManager.addSingleton<IConfigurationService>(IConfigurationService, ConfigurationService);
 
+        const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
+        workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => undefined);
+        ioc.serviceManager.addSingletonInstance<IWorkspaceService>(IWorkspaceService, workspaceService.object);
+
         ioc.registerMockProcessTypes();
         ioc.serviceManager.addSingletonInstance<boolean>(IsWindows, false);
     }
diff --git a/src/test/common/installer/installer.test.ts b/src/test/common/installer/installer.test.ts
@@ -1,19 +1,23 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+// tslint:disable:max-func-body-length no-invalid-this
+
 import { expect, use } from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
 import * as TypeMoq from 'typemoq';
-import { Disposable, OutputChannel, Uri } from 'vscode';
+import { Disposable, OutputChannel, Uri, WorkspaceFolder } from 'vscode';
+import { IApplicationShell, IWorkspaceService } from '../../../client/common/application/types';
 import { EnumEx } from '../../../client/common/enumUtils';
+import '../../../client/common/extensions';
+import { createDeferred, Deferred } from '../../../client/common/helpers';
 import { ProductInstaller } from '../../../client/common/installer/productInstaller';
 import { IInstallationChannelManager, IModuleInstaller } from '../../../client/common/installer/types';
 import { IDisposableRegistry, ILogger, InstallerResponse, ModuleNamePurpose, Product } from '../../../client/common/types';
 import { IServiceContainer } from '../../../client/ioc/types';
 
 use(chaiAsPromised);
 
-// tslint:disable-next-line:max-func-body-length
 suite('Module Installer', () => {
     [undefined, Uri.file('resource')].forEach(resource => {
         EnumEx.getNamesAndValues<Product>(Product).forEach(product => {
@@ -22,7 +26,11 @@ suite('Module Installer', () => {
             let installationChannel: TypeMoq.IMock<IInstallationChannelManager>;
             let moduleInstaller: TypeMoq.IMock<IModuleInstaller>;
             let serviceContainer: TypeMoq.IMock<IServiceContainer>;
+            let app: TypeMoq.IMock<IApplicationShell>;
+            let promptDeferred: Deferred<string>;
+            let workspaceService: TypeMoq.IMock<IWorkspaceService>;
             setup(() => {
+                promptDeferred = createDeferred<string>();
                 serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
                 const outputChannel = TypeMoq.Mock.ofType<OutputChannel>();
 
@@ -33,6 +41,10 @@ suite('Module Installer', () => {
 
                 installationChannel = TypeMoq.Mock.ofType<IInstallationChannelManager>();
                 serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInstallationChannelManager), TypeMoq.It.isAny())).returns(() => installationChannel.object);
+                app = TypeMoq.Mock.ofType<IApplicationShell>();
+                serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => app.object);
+                workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
+                serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IWorkspaceService), TypeMoq.It.isAny())).returns(() => workspaceService.object);
 
                 moduleInstaller = TypeMoq.Mock.ofType<IModuleInstaller>();
                 // tslint:disable-next-line:no-any
@@ -41,6 +53,8 @@ suite('Module Installer', () => {
                 installationChannel.setup(i => i.getInstallationChannel(TypeMoq.It.isAny())).returns(() => Promise.resolve(moduleInstaller.object));
             });
             teardown(() => {
+                // This must be resolved, else all subsequent tests will fail (as this same promise will be used for other tests).
+                promptDeferred.resolve();
                 disposables.forEach(disposable => {
                     if (disposable) {
                         disposable.dispose();
@@ -92,6 +106,47 @@ suite('Module Installer', () => {
                             moduleInstaller.verify(m => m.installModule(TypeMoq.It.isValue(moduleName), TypeMoq.It.isValue(resource)), TypeMoq.Times.once());
                         }
                     });
+                    test(`Ensure the prompt is displayed only once, untill the prompt is closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () {
+                        if (product.value === Product.unittest) {
+                            return this.skip();
+                        }
+                        workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!)))
+                            .returns(() => TypeMoq.Mock.ofType<WorkspaceFolder>().object)
+                            .verifiable(TypeMoq.Times.exactly(resource ? 5 : 0));
+                        app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+                            .returns(() => promptDeferred.promise)
+                            .verifiable(TypeMoq.Times.once());
+
+                        // Display first prompt.
+                        installer.promptToInstall(product.value, resource).ignoreErrors();
+
+                        // Display a few more prompts.
+                        installer.promptToInstall(product.value, resource).ignoreErrors();
+                        installer.promptToInstall(product.value, resource).ignoreErrors();
+                        installer.promptToInstall(product.value, resource).ignoreErrors();
+                        installer.promptToInstall(product.value, resource).ignoreErrors();
+
+                        app.verifyAll();
+                        workspaceService.verifyAll();
+                    });
+                    test(`Ensure the prompt is displayed again when previous prompt has been closed, ${product.name} (${resource ? 'With a resource' : 'without a resource'})`, async function () {
+                        if (product.value === Product.unittest) {
+                            return this.skip();
+                        }
+                        workspaceService.setup(w => w.getWorkspaceFolder(TypeMoq.It.isValue(resource!)))
+                            .returns(() => TypeMoq.Mock.ofType<WorkspaceFolder>().object)
+                            .verifiable(TypeMoq.Times.exactly(resource ? 3 : 0));
+                        app.setup(a => a.showErrorMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+                            .returns(() => Promise.resolve(undefined))
+                            .verifiable(TypeMoq.Times.exactly(3));
+
+                        await installer.promptToInstall(product.value, resource);
+                        await installer.promptToInstall(product.value, resource);
+                        await installer.promptToInstall(product.value, resource);
+
+                        app.verifyAll();
+                        workspaceService.verifyAll();
+                    });
                 }
             }
         });

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Ensure the prompt to install missing packages is not displayed more than once.`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`import { OutputChannel, Uri } from 'vscode';`
`2`		`-import { isNotInstalledError } from '../../common/helpers';`
`3`	`2`	`import { IPythonExecutionFactory } from '../../common/process/types';`
`4`	`3`	`import { ExecutionInfo, Product } from '../../common/types';`
`5`	`4`	`import { IServiceContainer } from '../../ioc/types';`