From 3c095b1997b937ea8605c98ac3fa12cd728a4a3e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 18 Aug 2023 14:02:05 +1000 Subject: [PATCH] Unify input capture with back buttons --- .../common/utils/inputValueProvider.ts | 59 ++++++++++ .../jupyterHubPasswordConnect.ts | 75 ++++++------- .../jupyterPasswordConnect.unit.test.ts | 60 ++--------- .../jupyterPasswordConnect.ts | 41 ++++--- .../jupyterPasswordConnect.unit.test.ts | 60 +++-------- .../userServerUriProvider.unit.test.ts | 102 +++++++++++++----- .../userServerUrlProvider.ts | 72 ++++--------- .../jupyter/connection.vscode.test.ts | 12 ++- 8 files changed, 241 insertions(+), 240 deletions(-) create mode 100644 src/platform/common/utils/inputValueProvider.ts diff --git a/src/platform/common/utils/inputValueProvider.ts b/src/platform/common/utils/inputValueProvider.ts new file mode 100644 index 000000000000..abb10d933faa --- /dev/null +++ b/src/platform/common/utils/inputValueProvider.ts @@ -0,0 +1,59 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { CancellationError, QuickInputButtons, window } from 'vscode'; +import { CancellationTokenSource, Disposable } from 'vscode-jsonrpc'; +import { raceCancellationError } from '../cancellation'; +import { disposeAllDisposables } from '../helpers'; +import { Disposables } from '../utils'; +import { createDeferred } from './async'; + +class BackError extends Error {} +export class WorkflowInputValueProvider extends Disposables { + private readonly token = this._register(new CancellationTokenSource()); + public async getValue(options: { + title: string; + value?: string; + ignoreFocusOut?: boolean; + prompt?: string; + validationMessage?: string; + password?: boolean; + }): Promise<{ value: string; navigation?: undefined } | { value?: undefined; navigation: 'cancel' | 'back' }> { + console.error('Why was this called'); + const disposables: Disposable[] = []; + try { + const input = window.createInputBox(); + disposables.push(input); + input.ignoreFocusOut = true; + input.title = options.title; + input.ignoreFocusOut = options.ignoreFocusOut === true; + input.prompt = options.prompt || ''; + input.value = options.value || ''; + input.password = options.password === true; + input.validationMessage = options.validationMessage || ''; + input.buttons = [QuickInputButtons.Back]; + input.show(); + const deferred = createDeferred(); + disposables.push(input.onDidHide(() => deferred.reject(new CancellationError()))); + input.onDidTriggerButton( + (e) => { + if (e === QuickInputButtons.Back) { + deferred.reject(new BackError()); + } + }, + this, + disposables + ); + input.onDidAccept(() => deferred.resolve(input.value.trim() || options.value), this, disposables); + const value = await raceCancellationError(this.token.token, deferred.promise); + return { value }; + } catch (ex) { + if (ex instanceof BackError) { + return { navigation: 'back' }; + } + return { navigation: 'cancel' }; + } finally { + disposeAllDisposables(disposables); + } + } +} diff --git a/src/standalone/userJupyterHubServer/jupyterHubPasswordConnect.ts b/src/standalone/userJupyterHubServer/jupyterHubPasswordConnect.ts index c4666297e46d..e8a3fa0ea064 100644 --- a/src/standalone/userJupyterHubServer/jupyterHubPasswordConnect.ts +++ b/src/standalone/userJupyterHubServer/jupyterHubPasswordConnect.ts @@ -11,7 +11,6 @@ import { } from '../../platform/common/types'; import { DataScience } from '../../platform/common/utils/localize'; import { noop } from '../../platform/common/utils/misc'; -import { IMultiStepInputFactory, IMultiStepInput } from '../../platform/common/utils/multiStepInput'; import { traceWarning } from '../../platform/logging'; import { sendTelemetryEvent, Telemetry } from '../../telemetry'; import { @@ -21,6 +20,7 @@ import { IJupyterServerUriStorage } from '../../kernels/jupyter/types'; import { disposeAllDisposables } from '../../platform/common/helpers'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; export interface IJupyterPasswordConnectInfo { requiresPassword: boolean; @@ -37,7 +37,6 @@ export class JupyterHubPasswordConnect { constructor( private appShell: IApplicationShell, - private readonly multiStepFactory: IMultiStepInputFactory, private readonly asyncDisposableRegistry: IAsyncDisposableRegistry, private readonly configService: IConfigurationService, private readonly agentCreator: IJupyterRequestAgentCreator | undefined, @@ -305,47 +304,39 @@ export class JupyterHubPasswordConnect { } private async getUserNameAndPassword(validationMessage?: string): Promise<{ username: string; password: string }> { - const multistep = this.multiStepFactory.create<{ - username: string; - password: string; - validationMessage?: string; - }>(); - const state = { username: '', password: '', validationMessage }; - await multistep.run(this.getUserNameMultiStep.bind(this), state); - return state; - } - - private async getUserNameMultiStep( - input: IMultiStepInput<{ username: string; password: string; validationErrorMessage?: string }>, - state: { username: string; password: string; validationMessage?: string } - ) { - state.username = await input.showInputBox({ - title: DataScience.jupyterSelectUserAndPasswordTitle, - prompt: DataScience.jupyterSelectUserPrompt, - validate: this.validateUserNameOrPassword, - validationMessage: state.validationMessage, - value: '' - }); - if (state.username) { - return this.getPasswordMultiStep.bind(this); + let username = ''; + let password = ''; + const inputValueCapture = new WorkflowInputValueProvider(); + this.disposables.push(inputValueCapture); + type Step = 'UserName' | 'Password'; + let nextStep: Step = 'UserName'; + while (true) { + if (nextStep === 'UserName') { + const errorMessage = validationMessage; + validationMessage = ''; + const result = await inputValueCapture.getValue({ + title: DataScience.jupyterSelectUserAndPasswordTitle, + prompt: DataScience.jupyterSelectUserPrompt, + validationMessage: errorMessage + }); + if (result.value) { + username = result.value; + nextStep = 'Password'; + continue; + } + break; + } else { + const result = await inputValueCapture.getValue({ + title: DataScience.jupyterSelectUserAndPasswordTitle, + prompt: DataScience.jupyterSelectPasswordPrompt + }); + if (result.value) { + password = result.value; + } + break; + } } - } - - private async validateUserNameOrPassword(_value: string): Promise { - return undefined; - } - - private async getPasswordMultiStep( - input: IMultiStepInput<{ username: string; password: string }>, - state: { username: string; password: string } - ) { - state.password = await input.showInputBox({ - title: DataScience.jupyterSelectUserAndPasswordTitle, - prompt: DataScience.jupyterSelectPasswordPrompt, - validate: this.validateUserNameOrPassword, - value: '', - password: true - }); + return { username, password }; } private async makeRequest(url: string, options: RequestInit): Promise { diff --git a/src/standalone/userJupyterHubServer/jupyterPasswordConnect.unit.test.ts b/src/standalone/userJupyterHubServer/jupyterPasswordConnect.unit.test.ts index 2ddfe4898d47..2d80f7af5865 100644 --- a/src/standalone/userJupyterHubServer/jupyterPasswordConnect.unit.test.ts +++ b/src/standalone/userJupyterHubServer/jupyterPasswordConnect.unit.test.ts @@ -1,8 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import * as sinon from 'sinon'; import { assert } from 'chai'; +import * as sinon from 'sinon'; import * as nodeFetch from 'node-fetch'; import { anything, instance, mock, when } from 'ts-mockito'; import { JupyterRequestCreator } from '../../kernels/jupyter/session/jupyterRequestCreator.web'; @@ -10,13 +10,10 @@ import { IJupyterRequestCreator, IJupyterServerUriStorage } from '../../kernels/ import { ApplicationShell } from '../../platform/common/application/applicationShell'; import { AsyncDisposableRegistry } from '../../platform/common/asyncDisposableRegistry'; import { ConfigurationService } from '../../platform/common/configuration/service.node'; -import { IDisposableRegistry } from '../../platform/common/types'; -import { MultiStepInputFactory } from '../../platform/common/utils/multiStepInput'; -import { MockInputBox } from '../../test/datascience/mockInputBox'; -import { MockQuickPick } from '../../test/datascience/mockQuickPick'; +import { IDisposable } from '../../platform/common/types'; import { JupyterHubPasswordConnect } from './jupyterHubPasswordConnect'; -import { Disposable, InputBox } from 'vscode'; -import { noop } from '../../test/core'; +import { disposeAllDisposables } from '../../platform/common/helpers'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; /* eslint-disable @typescript-eslint/no-explicit-any, , */ suite('Jupyter Hub Password Connect', () => { @@ -24,59 +21,28 @@ suite('Jupyter Hub Password Connect', () => { let appShell: ApplicationShell; let configService: ConfigurationService; let requestCreator: IJupyterRequestCreator; - - let inputBox: InputBox; + const disposables: IDisposable[] = []; setup(() => { - inputBox = { - show: noop, - onDidAccept: noop as any, - onDidHide: noop as any, - hide: noop, - dispose: noop as any, - onDidChangeValue: noop as any, - onDidTriggerButton: noop as any, - valueSelection: undefined, - totalSteps: undefined, - validationMessage: '', - busy: false, - buttons: [], - enabled: true, - ignoreFocusOut: false, - password: false, - step: undefined, - title: '', - value: '', - prompt: '', - placeholder: '' - }; - sinon.stub(inputBox, 'show').callsFake(noop); - sinon.stub(inputBox, 'onDidHide').callsFake(() => new Disposable(noop)); - sinon.stub(inputBox, 'onDidAccept').callsFake((cb) => { - (cb as Function)(); - return new Disposable(noop); - }); - appShell = mock(ApplicationShell); - when(appShell.showInputBox(anything())).thenReturn(Promise.resolve('Python')); - when(appShell.createInputBox()).thenReturn(inputBox); - const multiStepFactory = new MultiStepInputFactory(instance(appShell)); const mockDisposableRegistry = mock(AsyncDisposableRegistry); configService = mock(ConfigurationService); requestCreator = mock(JupyterRequestCreator); const serverUriStorage = mock(); - const disposables = mock(); jupyterPasswordConnect = new JupyterHubPasswordConnect( instance(appShell), - multiStepFactory, instance(mockDisposableRegistry), instance(configService), undefined, instance(requestCreator), instance(serverUriStorage), - instance(disposables) + disposables ); }); + teardown(() => { + sinon.restore(); + disposeAllDisposables(disposables); + }); function createJupyterHubSetup() { const dsSettings = { @@ -85,11 +51,6 @@ suite('Jupyter Hub Password Connect', () => { } as any; when(configService.getSettings(anything())).thenReturn(dsSettings as any); - const quickPick = new MockQuickPick(''); - const input = new MockInputBox('test', 2); // We want the input box to enter twice for this scenario - when(appShell.createQuickPick()).thenReturn(quickPick!); - when(appShell.createInputBox()).thenReturn(input); - const hubActiveResponse = mock(nodeFetch.Response); when(hubActiveResponse.ok).thenReturn(true); when(hubActiveResponse.status).thenReturn(200); @@ -134,6 +95,7 @@ suite('Jupyter Hub Password Connect', () => { }; } test('Jupyter hub', async () => { + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').resolves({ value: 'test' }); const fetch = createJupyterHubSetup(); when(requestCreator.getFetchMethod()).thenReturn(fetch as any); diff --git a/src/standalone/userJupyterServer/jupyterPasswordConnect.ts b/src/standalone/userJupyterServer/jupyterPasswordConnect.ts index 670528e366f9..dc9b469bb8d9 100644 --- a/src/standalone/userJupyterServer/jupyterPasswordConnect.ts +++ b/src/standalone/userJupyterServer/jupyterPasswordConnect.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import { CancellationError, ConfigurationTarget, QuickInputButtons } from 'vscode'; +import { CancellationError, ConfigurationTarget } from 'vscode'; import { IApplicationShell } from '../../platform/common/application/types'; import { IConfigurationService, IDisposable, IDisposableRegistry } from '../../platform/common/types'; import { DataScience } from '../../platform/common/utils/localize'; @@ -16,6 +16,7 @@ import { IJupyterServerUriStorage } from '../../kernels/jupyter/types'; import { disposeAllDisposables } from '../../platform/common/helpers'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; export interface IJupyterPasswordConnectInfo { requiresPassword: boolean; @@ -123,29 +124,23 @@ export class JupyterPasswordConnect { friendlyUrl = `${uri.protocol}//${uri.hostname}`; friendlyUrl = options.displayName ? `${options.displayName} (${friendlyUrl})` : friendlyUrl; if (requiresPassword && options.isTokenEmpty) { - const input = this.appShell.createInputBox(); - options.disposables.push(input); - input.title = DataScience.jupyterSelectPasswordTitle(friendlyUrl); - input.prompt = DataScience.jupyterSelectPasswordPrompt; - input.ignoreFocusOut = true; - input.password = true; - input.validationMessage = options.validationErrorMessage || ''; - input.show(); - input.buttons = [QuickInputButtons.Back]; - userPassword = await new Promise((resolve, reject) => { - input.onDidTriggerButton( - (e) => { - if (e === QuickInputButtons.Back) { - reject(InputFlowAction.back); - } - }, - this, - options.disposables - ); - input.onDidChangeValue(() => (input.validationMessage = ''), this, options.disposables); - input.onDidAccept(() => resolve(input.value), this, options.disposables); - input.onDidHide(() => reject(InputFlowAction.cancel), this, options.disposables); + const inputProvider = new WorkflowInputValueProvider(); + options.disposables.push(inputProvider); + const result = await inputProvider.getValue({ + title: DataScience.jupyterSelectPasswordTitle(friendlyUrl), + prompt: DataScience.jupyterSelectPasswordPrompt, + ignoreFocusOut: true, + password: true, + validationMessage: options.validationErrorMessage || '' }); + if (result.navigation) { + if (result.navigation === 'back') { + throw InputFlowAction.back; + } + throw InputFlowAction.cancel; + } else { + userPassword = result.value; + } } if (typeof userPassword === undefined && !userPassword && options.isTokenEmpty) { diff --git a/src/standalone/userJupyterServer/jupyterPasswordConnect.unit.test.ts b/src/standalone/userJupyterServer/jupyterPasswordConnect.unit.test.ts index 9f4357a69397..fb2327da638f 100644 --- a/src/standalone/userJupyterServer/jupyterPasswordConnect.unit.test.ts +++ b/src/standalone/userJupyterServer/jupyterPasswordConnect.unit.test.ts @@ -1,8 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import * as sinon from 'sinon'; import { assert } from 'chai'; +import * as sinon from 'sinon'; import * as nodeFetch from 'node-fetch'; import * as typemoq from 'typemoq'; import { anything, instance, mock, when } from 'ts-mockito'; @@ -10,10 +10,10 @@ import { JupyterRequestCreator } from '../../kernels/jupyter/session/jupyterRequ import { IJupyterRequestCreator, IJupyterServerUriStorage } from '../../kernels/jupyter/types'; import { ApplicationShell } from '../../platform/common/application/applicationShell'; import { ConfigurationService } from '../../platform/common/configuration/service.node'; -import { IDisposableRegistry } from '../../platform/common/types'; +import { IDisposable } from '../../platform/common/types'; import { JupyterPasswordConnect } from './jupyterPasswordConnect'; -import { Disposable, InputBox } from 'vscode'; -import { noop } from '../../test/core'; +import { disposeAllDisposables } from '../../platform/common/helpers'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; /* eslint-disable @typescript-eslint/no-explicit-any, , */ suite('JupyterServer Password Connect', () => { @@ -25,44 +25,12 @@ suite('JupyterServer Password Connect', () => { const xsrfValue: string = '12341234'; const sessionName: string = 'sessionName'; const sessionValue: string = 'sessionValue'; - let inputBox: InputBox; + const disposables: IDisposable[] = []; setup(() => { - inputBox = { - show: noop, - onDidAccept: noop as any, - onDidHide: noop as any, - hide: noop, - dispose: noop as any, - onDidChangeValue: noop as any, - onDidTriggerButton: noop as any, - valueSelection: undefined, - totalSteps: undefined, - validationMessage: '', - busy: false, - buttons: [], - enabled: true, - ignoreFocusOut: false, - password: false, - step: undefined, - title: '', - value: '', - prompt: '', - placeholder: '' - }; - sinon.stub(inputBox, 'show').callsFake(noop); - sinon.stub(inputBox, 'onDidHide').callsFake(() => new Disposable(noop)); - sinon.stub(inputBox, 'onDidAccept').callsFake((cb) => { - (cb as Function)(); - return new Disposable(noop); - }); - appShell = mock(ApplicationShell); - when(appShell.showInputBox(anything())).thenReturn(Promise.resolve('Python')); - when(appShell.createInputBox()).thenReturn(inputBox); configService = mock(ConfigurationService); requestCreator = mock(JupyterRequestCreator); const serverUriStorage = mock(); - const disposables = mock(); jupyterPasswordConnect = new JupyterPasswordConnect( instance(appShell), @@ -70,9 +38,13 @@ suite('JupyterServer Password Connect', () => { undefined, instance(requestCreator), instance(serverUriStorage), - instance(disposables) + disposables ); }); + teardown(() => { + sinon.restore(); + disposeAllDisposables(disposables); + }); function createMockSetup(secure: boolean, ok: boolean, xsrfReponseStatusCode: 200 | 302 | 401 = 302) { const dsSettings = { @@ -139,8 +111,7 @@ suite('JupyterServer Password Connect', () => { } test('With Password', async () => { - inputBox.value = 'Python'; - when(appShell.showInputBox(anything())).thenReturn(Promise.resolve('Python')); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').resolves({ value: 'Python' }); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(false, true); // Mock our second call to get session cookie @@ -198,7 +169,7 @@ suite('JupyterServer Password Connect', () => { fetchMock.verifyAll(); }); test('Empty Password and empty token', async () => { - when(appShell.showInputBox(anything())).thenReject(new Error('Should not be called')); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').rejects(new Error('Should not be called')); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(false, true, 200); // Mock our second call to get session cookie @@ -256,7 +227,7 @@ suite('JupyterServer Password Connect', () => { fetchMock.verifyAll(); }); test('Password required and non-empty token', async () => { - when(appShell.showInputBox(anything())).thenReject(new Error('Should not be called')); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').rejects(new Error('Should not be called')); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(false, true, 401); // Mock our second call to get session cookie @@ -318,6 +289,7 @@ suite('JupyterServer Password Connect', () => { }); test('Without a Password and allowUnauthorized', async () => { + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').resolves({ value: '' }); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(true, true); // Mock our second call to get session cookie @@ -371,6 +343,7 @@ suite('JupyterServer Password Connect', () => { }); test('Failure', async () => { + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').resolves({ value: '' }); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(false, false); when(requestCreator.getFetchMethod()).thenReturn(fetchMock.object as any); @@ -391,8 +364,7 @@ suite('JupyterServer Password Connect', () => { test('Bad password followed by good password.', async () => { // Reconfigure our app shell to first give a bad password - inputBox.value = 'JUNK'; - when(appShell.showInputBox(anything())).thenReturn(Promise.resolve('JUNK')); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').resolves({ value: 'JUNK' }); const { fetchMock, mockXsrfHeaders, mockXsrfResponse } = createMockSetup(false, true); diff --git a/src/standalone/userJupyterServer/userServerUriProvider.unit.test.ts b/src/standalone/userJupyterServer/userServerUriProvider.unit.test.ts index dc5b6b06453b..9870c9973d65 100644 --- a/src/standalone/userJupyterServer/userServerUriProvider.unit.test.ts +++ b/src/standalone/userJupyterServer/userServerUriProvider.unit.test.ts @@ -15,10 +15,9 @@ import { IDisposable, IExtensionContext } from '../../platform/common/types'; -import { IMultiStepInputFactory, InputFlowAction } from '../../platform/common/utils/multiStepInput'; +import { InputFlowAction } from '../../platform/common/utils/multiStepInput'; import { SecureConnectionValidator, - UserJupyterServerDisplayName, UserJupyterServerUriInput, UserJupyterServerUriListKey, UserJupyterServerUriListKeyV2, @@ -42,6 +41,9 @@ import { generateIdFromRemoteProvider } from '../../kernels/jupyter/jupyterUtils import { IJupyterPasswordConnectInfo, JupyterPasswordConnect } from './jupyterPasswordConnect'; import { IFileSystem } from '../../platform/common/platform/types'; import { IJupyterServerUri, JupyterServerCollection } from '../../api'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; +import { DataScience } from '../../platform/common/utils/localize'; +import { JupyterHubPasswordConnect } from '../userJupyterHubServer/jupyterHubPasswordConnect'; /* eslint-disable @typescript-eslint/no-explicit-any, , */ suite('User Uri Provider', () => { @@ -54,7 +56,6 @@ suite('User Uri Provider', () => { let serverUriStorage: IJupyterServerUriStorage; let globalMemento: Memento; const disposables: IDisposable[] = []; - let multiStepFactory: IMultiStepInputFactory; let asyncDisposables: IAsyncDisposable[] = []; let asyncDisposableRegistry: IAsyncDisposableRegistry = { dispose: async function () { @@ -118,7 +119,6 @@ suite('User Uri Provider', () => { encryptedStorage = mock(); serverUriStorage = mock(); globalMemento = mock(); - multiStepFactory = mock(); commands = mock(); requestCreator = mock(); tokenSource = new CancellationTokenSource(); @@ -167,6 +167,7 @@ suite('User Uri Provider', () => { return Promise.resolve(); }); getPasswordConnectionInfoStub = sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo'); + sinon.stub(JupyterHubPasswordConnect.prototype, 'isJupyterHub').resolves(false); getPasswordConnectionInfoStub.resolves({ requiresPassword: false }); when(serverUriStorage.add(anything())).thenResolve(); @@ -189,7 +190,6 @@ suite('User Uri Provider', () => { instance(serverUriStorage), instance(globalMemento), disposables, - instance(multiStepFactory), asyncDisposableRegistry, instance(commands), undefined, @@ -322,8 +322,14 @@ suite('User Uri Provider', () => { }); test('Add the provided Url and verify it is in the storage', async () => { await testMigration(); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + let displayNameCaptured = false; + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + displayNameCaptured = true; + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); const getUriFromUserStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser'); getUriFromUserStub.resolves(undefined); @@ -339,7 +345,7 @@ suite('User Uri Provider', () => { assert.ok(server.id); assert.strictEqual(server.label, 'Foo Bar'); - assert.ok(displayNameStub.called, 'We should have prompted the user for a display name'); + assert.ok(displayNameCaptured, 'We should have prompted the user for a display name'); assert.isFalse(getUriFromUserStub.called, 'Should not prompt for a Url, as one was provided'); const authInfo = await provider.resolveConnectionInformation(server, token); assert.strictEqual(authInfo.baseUrl.toString(), 'https://localhost:3333/'); @@ -360,8 +366,14 @@ suite('User Uri Provider', () => { }); test('Prompt user for a Url and use what is in clipboard, then verify it is in the storage', async () => { await testMigration(); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + let displayNameCaptured = false; + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + displayNameCaptured = true; + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); when(clipboard.readText()).thenResolve('https://localhost:3333?token=ABCD'); const [cmd] = await provider.getCommands('', token); @@ -376,7 +388,7 @@ suite('User Uri Provider', () => { assert.ok(server.id); assert.strictEqual(server.label, 'Foo Bar'); - assert.ok(displayNameStub.called, 'We should have prompted the user for a display name'); + assert.ok(displayNameCaptured, 'We should have prompted the user for a display name'); verify(clipboard.readText()).once(); const servers = await provider.getJupyterServers(token); @@ -398,8 +410,12 @@ suite('User Uri Provider', () => { when(clipboard.readText()).thenResolve('https://localhost:3333'); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(true); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); const [cmd] = await provider.getCommands('', token); const server = await provider.handleCommand(cmd, token); @@ -432,8 +448,12 @@ suite('User Uri Provider', () => { await testMigration(); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(true); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); const getUriFromUserStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser'); getUriFromUserStub.resolves(undefined); @@ -467,8 +487,12 @@ suite('User Uri Provider', () => { await testMigration(); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(false); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); const getUriFromUserStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser'); getUriFromUserStub.resolves(undefined); @@ -494,8 +518,12 @@ suite('User Uri Provider', () => { sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: true }); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(false); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.resolves('Foo Bar'); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + return Promise.resolve({ value: 'Foo Bar' }); + } + return Promise.resolve({ value: '' }); + }); const [cmd] = await provider.getCommands('http://localhost:3333', token); const server = await provider.handleCommand(cmd, token); @@ -530,14 +558,20 @@ suite('User Uri Provider', () => { sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false }); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(false); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.rejects(InputFlowAction.back); + let displayNameCaptured = false; + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + displayNameCaptured = true; + return Promise.resolve({ navigation: 'back' }); + } + return Promise.resolve({ value: '' }); + }); const [cmd] = await provider.getCommands('https://localhost:3333', token); const server = await provider.handleCommand(cmd, token); assert.strictEqual(server, 'back'); - assert.strictEqual(displayNameStub.callCount, 1); + assert.isOk(displayNameCaptured); assert.strictEqual(urlInputStub.callCount, 0); // Since a url was provided we should never prompt for this, even when clicking back in display name. }); test('When pre-populating with https url and without password auth, the next step should be the displayName & cancel from displayName should get out of this UI flow (without displaying the Url picker)', async () => { @@ -549,14 +583,20 @@ suite('User Uri Provider', () => { sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false }); const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections'); secureConnectionStub.resolves(false); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.rejects(InputFlowAction.cancel); + let displayNameCaptured = false; + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + displayNameCaptured = true; + return Promise.resolve({ navigation: 'cancel' }); + } + return Promise.resolve({ value: '' }); + }); const [cmd] = await provider.getCommands('https://localhost:3333', token); const server = await provider.handleCommand(cmd, token); assert.isUndefined(server); - assert.strictEqual(displayNameStub.callCount, 1); + assert.isOk(displayNameCaptured); assert.strictEqual(urlInputStub.callCount, 0); // Since a url was provided we should never prompt for this, even when clicking back in display name. }); test('When pre-populating with https url and without password auth, and the server is invalid the next step should be the url display', async () => { @@ -591,8 +631,14 @@ suite('User Uri Provider', () => { } }); sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false }); - const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName'); - displayNameStub.rejects(InputFlowAction.back); + let displayNameCaptured = false; + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + displayNameCaptured = true; + return Promise.resolve({ navigation: 'back' }); + } + return Promise.resolve({ value: '' }); + }); when(jupyterConnection.validateRemoteUri(anything(), anything(), true)).thenCall( (_, uri: IJupyterServerUri) => { if (!uri) { @@ -616,7 +662,7 @@ suite('User Uri Provider', () => { const server = await provider.handleCommand(cmd, token); assert.strictEqual(server, 'back'); - assert.strictEqual(displayNameStub.callCount, 1); + assert.isOk(displayNameCaptured); assert.strictEqual(urlInputStub.callCount, 2); // Displayed twice, first time for error message, second time when hit back button from display UI. }); }); diff --git a/src/standalone/userJupyterServer/userServerUrlProvider.ts b/src/standalone/userJupyterServer/userServerUrlProvider.ts index 1ec0b13e680d..145109479669 100644 --- a/src/standalone/userJupyterServer/userServerUrlProvider.ts +++ b/src/standalone/userJupyterServer/userServerUrlProvider.ts @@ -57,7 +57,7 @@ import { JupyterServerCommandProvider, JupyterServerProvider } from '../../api'; -import { IMultiStepInputFactory, InputFlowAction } from '../../platform/common/utils/multiStepInput'; +import { InputFlowAction } from '../../platform/common/utils/multiStepInput'; import { JupyterSelfCertsError } from '../../platform/errors/jupyterSelfCertsError'; import { JupyterSelfCertsExpiredError } from '../../platform/errors/jupyterSelfCertsExpiredError'; import { Deferred, createDeferred } from '../../platform/common/utils/async'; @@ -66,6 +66,7 @@ import { RemoteKernelSpecCacheFileName } from '../../kernels/jupyter/constants'; import { disposeAllDisposables } from '../../platform/common/helpers'; import { Disposables } from '../../platform/common/utils'; import { JupyterHubPasswordConnect } from '../userJupyterHubServer/jupyterHubPasswordConnect'; +import { WorkflowInputValueProvider } from '../../platform/common/utils/inputValueProvider'; export const UserJupyterServerUriListKey = 'user-jupyter-server-uri-list'; export const UserJupyterServerUriListKeyV2 = 'user-jupyter-server-uri-list-version2'; @@ -94,7 +95,9 @@ export class UserJupyterServerUrlProvider onDidChangeServers = this._onDidChangeServers.event; private secureConnectionValidator: SecureConnectionValidator; private jupyterServerUriInput: UserJupyterServerUriInput; - private jupyterServerUriDisplayName: UserJupyterServerDisplayName; + // private jupyterServerUriDisplayName: UserJupyterServerDisplayName; + private readonly inputProvider: WorkflowInputValueProvider; + private readonly displayNamesOfHandles = new Map(); constructor( @inject(IClipboard) clipboard: IClipboard, @inject(IApplicationShell) applicationShell: IApplicationShell, @@ -105,7 +108,6 @@ export class UserJupyterServerUrlProvider @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage, @inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento, @inject(IDisposableRegistry) disposables: IDisposableRegistry, - @inject(IMultiStepInputFactory) multiStepFactory: IMultiStepInputFactory, @inject(IAsyncDisposableRegistry) asyncDisposableRegistry: IAsyncDisposableRegistry, @inject(ICommandManager) private readonly commands: ICommandManager, @inject(IJupyterRequestAgentCreator) @@ -128,7 +130,8 @@ export class UserJupyterServerUrlProvider // eslint-disable-next-line @typescript-eslint/no-use-before-define this.jupyterServerUriInput = new UserJupyterServerUriInput(clipboard, applicationShell); // eslint-disable-next-line @typescript-eslint/no-use-before-define - this.jupyterServerUriDisplayName = new UserJupyterServerDisplayName(applicationShell); + this.inputProvider = new WorkflowInputValueProvider(); + // this.jupyterServerUriDisplayName = new UserJupyterServerDisplayName(applicationShell); this.jupyterPasswordConnect = new JupyterPasswordConnect( applicationShell, configService, @@ -139,7 +142,6 @@ export class UserJupyterServerUrlProvider ); this.jupyterHubPasswordConnect = new JupyterHubPasswordConnect( applicationShell, - multiStepFactory, asyncDisposableRegistry, configService, agentCreator, @@ -575,11 +577,20 @@ export class UserJupyterServerUrlProvider // If we were given a Url, then back should get out of this flow. previousStep = initialUrlWasValid && initialUrl ? undefined : 'Get Url'; } - - jupyterServerUri.displayName = await this.jupyterServerUriDisplayName.getDisplayName( - handle, - jupyterServerUri.displayName || new URL(jupyterServerUri.baseUrl).hostname - ); + const result = await this.inputProvider.getValue({ + ignoreFocusOut: true, + title: DataScience.jupyterRenameServer, + value: jupyterServerUri.displayName || new URL(jupyterServerUri.baseUrl).hostname + }); + if (result.navigation) { + if (result.navigation === 'back') { + throw InputFlowAction.back; + } + throw InputFlowAction.cancel; + } else { + this.displayNamesOfHandles.set(handle, result.value); + jupyterServerUri.displayName = result.value; + } break; } } catch (ex) { @@ -630,7 +641,7 @@ export class UserJupyterServerUrlProvider const serverInfo = server.serverInfo; // Hacky due to the way display names are stored in uri storage. // Should be cleaned up later. - const displayName = this.jupyterServerUriDisplayName.displayNamesOfHandles.get(id); + const displayName = this.displayNamesOfHandles.get(id); if (displayName) { serverInfo.displayName = displayName; } @@ -719,45 +730,6 @@ export class UserJupyterServerUriInput { } } -export class UserJupyterServerDisplayName { - constructor(@inject(IApplicationShell) private readonly applicationShell: IApplicationShell) {} - public displayNamesOfHandles = new Map(); - public async getDisplayName(handle: string, defaultValue: string): Promise { - const disposables: Disposable[] = []; - try { - const input = this.applicationShell.createInputBox(); - disposables.push(input); - input.ignoreFocusOut = true; - input.title = DataScience.jupyterRenameServer; - input.value = defaultValue; - input.buttons = [QuickInputButtons.Back]; - input.show(); - const deferred = createDeferred(); - disposables.push(input.onDidHide(() => deferred.reject(InputFlowAction.cancel))); - input.onDidTriggerButton( - (e) => { - if (e === QuickInputButtons.Back) { - deferred.reject(InputFlowAction.back); - } - }, - this, - disposables - ); - input.onDidAccept( - () => { - const displayName = input.value.trim() || defaultValue; - this.displayNamesOfHandles.set(handle, displayName); - deferred.resolve(displayName); - }, - this, - disposables - ); - return await deferred.promise; - } finally { - disposeAllDisposables(disposables); - } - } -} export class SecureConnectionValidator { constructor( @inject(IApplicationShell) private readonly applicationShell: IApplicationShell, diff --git a/src/test/datascience/jupyter/connection.vscode.test.ts b/src/test/datascience/jupyter/connection.vscode.test.ts index 64cccb8c80d5..10d14de0f1bc 100644 --- a/src/test/datascience/jupyter/connection.vscode.test.ts +++ b/src/test/datascience/jupyter/connection.vscode.test.ts @@ -18,7 +18,6 @@ import { IS_REMOTE_NATIVE_TEST, initialize } from '../../initialize.node'; import { startJupyterServer, closeNotebooksAndCleanUpAfterTests } from '../notebook/helper.node'; import { SecureConnectionValidator, - UserJupyterServerDisplayName, UserJupyterServerUriInput, UserJupyterServerUrlProvider, parseUri @@ -39,8 +38,9 @@ import { DataScience } from '../../../platform/common/utils/localize'; import * as sinon from 'sinon'; import assert from 'assert'; import { createDeferred, createDeferredFromPromise } from '../../../platform/common/utils/async'; -import { IMultiStepInputFactory, InputFlowAction } from '../../../platform/common/utils/multiStepInput'; +import { InputFlowAction } from '../../../platform/common/utils/multiStepInput'; import { IFileSystem } from '../../../platform/common/platform/types'; +import { WorkflowInputValueProvider } from '../../../platform/common/utils/inputValueProvider'; suite('Connect to Remote Jupyter Servers', function () { // On conda these take longer for some reason. @@ -176,7 +176,6 @@ suite('Connect to Remote Jupyter Servers', function () { instance(serverUriStorage), instance(memento), disposables, - api.serviceContainer.get(IMultiStepInputFactory), api.serviceContainer.get(IAsyncDisposableRegistry), instance(commands), api.serviceContainer.get(IJupyterRequestAgentCreator), @@ -214,7 +213,12 @@ suite('Connect to Remote Jupyter Servers', function () { jupyterServerUri: parseUri(userUri, '')! }); sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections').resolves(true); - sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName').resolves(displayName); + sinon.stub(WorkflowInputValueProvider.prototype, 'getValue').callsFake((options) => { + if (options.title === DataScience.jupyterRenameServer) { + return Promise.resolve({ value: displayName }); + } + return Promise.resolve({ value: '' }); + }); const errorMessageDisplayed = createDeferred(); inputBox.value = password || ''; sinon.stub(inputBox, 'validationMessage').set((msg) => (msg ? errorMessageDisplayed.resolve(msg) : undefined));