From 776e2cf6e743188f9fe7d3c64782f0ba345dc277 Mon Sep 17 00:00:00 2001 From: Yurong He Date: Thu, 23 May 2019 15:11:10 -0700 Subject: [PATCH] Fixed #4567 adding open connectionDialog when no connection is available (#5581) * Fixed #4567 adding open connectionDialog when no connection is available --- .../workbench/parts/notebook/models/cell.ts | 6 +- .../parts/notebook/models/notebookModel.ts | 15 +++- .../parts/notebook/notebook.component.ts | 2 +- .../parts/notebook/notebookActions.ts | 74 ++++++++++--------- .../notebook/model/notebookModel.test.ts | 12 +-- 5 files changed, 65 insertions(+), 44 deletions(-) diff --git a/src/sql/workbench/parts/notebook/models/cell.ts b/src/sql/workbench/parts/notebook/models/cell.ts index 1a385d0ac9..2d6f92f444 100644 --- a/src/sql/workbench/parts/notebook/models/cell.ts +++ b/src/sql/workbench/parts/notebook/models/cell.ts @@ -217,8 +217,10 @@ export class CellModel implements ICellModel { } else { // TODO update source based on editor component contents if (kernel.requiresConnection && !this.notebookModel.activeConnection) { - this.sendNotification(notificationService, Severity.Error, localize('kernelRequiresConnection', "Please select a connection to run cells for this kernel")); - return false; + let connected = await this.notebookModel.requestConnection(); + if (!connected) { + return false; + } } let content = this.source; if (content) { diff --git a/src/sql/workbench/parts/notebook/models/notebookModel.ts b/src/sql/workbench/parts/notebook/models/notebookModel.ts index dcd792db7e..4c3f70bda2 100644 --- a/src/sql/workbench/parts/notebook/models/notebookModel.ts +++ b/src/sql/workbench/parts/notebook/models/notebookModel.ts @@ -16,7 +16,7 @@ import * as notebookUtils from '../notebookUtils'; import { INotebookManager, SQL_NOTEBOOK_PROVIDER, DEFAULT_NOTEBOOK_PROVIDER } from 'sql/workbench/services/notebook/common/notebookService'; import { NotebookContexts } from 'sql/workbench/parts/notebook/models/notebookContexts'; import { IConnectionProfile } from 'sql/platform/connection/common/interfaces'; -import { INotification, Severity } from 'vs/platform/notification/common/notification'; +import { INotification, Severity, INotificationService } from 'vs/platform/notification/common/notification'; import { URI } from 'vs/base/common/uri'; import { ISingleNotebookEditOperation } from 'sql/workbench/api/common/sqlExtHostTypes'; import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile'; @@ -72,11 +72,13 @@ export class NotebookModel extends Disposable implements INotebookModel { private _oldKernel: nb.IKernel; private _clientSessionListeners: IDisposable[] = []; private _connectionUrisToDispose: string[] = []; + public requestConnectionHandler: () => Promise; constructor( private _notebookOptions: INotebookModelOptions, public connectionProfile: IConnectionProfile | undefined, - @ILogService private readonly logService: ILogService + @ILogService private readonly logService: ILogService, + @INotificationService private readonly notificationService: INotificationService ) { super(); if (!_notebookOptions || !_notebookOptions.notebookUri || !_notebookOptions.notebookManagers) { @@ -301,6 +303,15 @@ export class NotebookModel extends Disposable implements INotebookModel { } } + public async requestConnection(): Promise { + if (this.requestConnectionHandler) { + return this.requestConnectionHandler(); + } else if (this.notificationService) { + this.notificationService.notify({ severity: Severity.Error, message: localize('kernelRequiresConnection', "Please select a connection to run cells for this kernel") }); + } + return false; + } + public findCellIndex(cellModel: ICellModel): number { return this._cells.findIndex((cell) => cell.equals(cellModel)); } diff --git a/src/sql/workbench/parts/notebook/notebook.component.ts b/src/sql/workbench/parts/notebook/notebook.component.ts index c77cd0ef34..dd43aa47fd 100644 --- a/src/sql/workbench/parts/notebook/notebook.component.ts +++ b/src/sql/workbench/parts/notebook/notebook.component.ts @@ -279,7 +279,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe defaultKernel: this._notebookParams.input.defaultKernel, layoutChanged: this._notebookParams.input.layoutChanged, capabilitiesService: this.capabilitiesService - }, this.profile, this.logService); + }, this.profile, this.logService, this.notificationService); model.onError((errInfo: INotification) => this.handleModelError(errInfo)); let trusted = await this.notebookService.isNotebookTrustCached(this._notebookParams.notebookUri, this.isDirty()); await model.requestModelLoad(trusted); diff --git a/src/sql/workbench/parts/notebook/notebookActions.ts b/src/sql/workbench/parts/notebook/notebookActions.ts index 616f4301d3..8c012920d3 100644 --- a/src/sql/workbench/parts/notebook/notebookActions.ts +++ b/src/sql/workbench/parts/notebook/notebookActions.ts @@ -350,6 +350,7 @@ export class AttachToDropdown extends SelectBox { this._register(this.model.contextsLoading(() => { this.setOptions([msgLoadingContexts], 0); })); + this.model.requestConnectionHandler = () => this.openConnectionDialog(true); this.handleContextsChanged(); } @@ -489,46 +490,53 @@ export class AttachToDropdown extends SelectBox { * Bind the server value to 'Attach To' drop down * Connected server is displayed at the top of drop down **/ - public async openConnectionDialog(useProfile: boolean = false): Promise { + public async openConnectionDialog(useProfile: boolean = false): Promise { try { - await this._connectionDialogService.openDialogAndWait(this._connectionManagementService, { connectionType: 1, providers: this.model.getApplicableConnectionProviderIds(this.model.clientSession.kernel.name) }, useProfile ? this.model.connectionProfile : undefined).then(connection => { - let attachToConnections = this.values; - if (!connection) { - this.loadAttachToDropdown(this.model, this.getKernelDisplayName()); - this.doChangeContext(undefined, true); - return; - } - let connectionUri = this._connectionManagementService.getConnectionUri(connection); - let connectionProfile = new ConnectionProfile(this._capabilitiesService, connection); - let connectedServer = connectionProfile.title ? connectionProfile.title : connectionProfile.serverName; - //Check to see if the same server is already there in dropdown. We only have server names in dropdown - if (attachToConnections.some(val => val === connectedServer)) { - this.loadAttachToDropdown(this.model, this.getKernelDisplayName()); - this.doChangeContext(); - return; - } - else { - attachToConnections.unshift(connectedServer); - } - //To ignore n/a after we have at least one valid connection - attachToConnections = attachToConnections.filter(val => val !== msgSelectConnection); + let connection = await this._connectionDialogService.openDialogAndWait(this._connectionManagementService, + { + connectionType: 1, + providers: this.model.getApplicableConnectionProviderIds(this.model.clientSession.kernel.name) + }, + useProfile ? this.model.connectionProfile : undefined); - let index = attachToConnections.findIndex((connection => connection === connectedServer)); - this.setOptions([]); - this.setOptions(attachToConnections); - if (!index || index < 0 || index >= attachToConnections.length) { - index = 0; - } - this.select(index); + let attachToConnections = this.values; + if (!connection) { + this.loadAttachToDropdown(this.model, this.getKernelDisplayName()); + this.doChangeContext(undefined, true); + return false; + } + let connectionUri = this._connectionManagementService.getConnectionUri(connection); + let connectionProfile = new ConnectionProfile(this._capabilitiesService, connection); + let connectedServer = connectionProfile.title ? connectionProfile.title : connectionProfile.serverName; + //Check to see if the same server is already there in dropdown. We only have server names in dropdown + if (attachToConnections.some(val => val === connectedServer)) { + this.loadAttachToDropdown(this.model, this.getKernelDisplayName()); + this.doChangeContext(); + return true; + } + else { + attachToConnections.unshift(connectedServer); + } + //To ignore n/a after we have at least one valid connection + attachToConnections = attachToConnections.filter(val => val !== msgSelectConnection); - this.model.addAttachToConnectionsToBeDisposed(connectionUri); - // Call doChangeContext to set the newly chosen connection in the model - this.doChangeContext(connectionProfile); - }); + let index = attachToConnections.findIndex((connection => connection === connectedServer)); + this.setOptions([]); + this.setOptions(attachToConnections); + if (!index || index < 0 || index >= attachToConnections.length) { + index = 0; + } + this.select(index); + + this.model.addAttachToConnectionsToBeDisposed(connectionUri); + // Call doChangeContext to set the newly chosen connection in the model + this.doChangeContext(connectionProfile); + return true; } catch (error) { const actions: INotificationActions = { primary: [] }; this._notificationService.notify({ severity: Severity.Error, message: getErrorMessage(error), actions }); + return false; } } } diff --git a/src/sqltest/parts/notebook/model/notebookModel.test.ts b/src/sqltest/parts/notebook/model/notebookModel.test.ts index 872b83be8c..2122027a58 100644 --- a/src/sqltest/parts/notebook/model/notebookModel.test.ts +++ b/src/sqltest/parts/notebook/model/notebookModel.test.ts @@ -131,7 +131,7 @@ suite('notebook model', function (): void { mockContentManager.setup(c => c.getNotebookContents(TypeMoq.It.isAny())).returns(() => Promise.resolve(emptyNotebook)); notebookManagers[0].contentManager = mockContentManager.object; // When I initialize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined); await model.requestModelLoad(); // Then I expect to have 0 code cell as the contents @@ -146,7 +146,7 @@ suite('notebook model', function (): void { mockContentManager.setup(c => c.getNotebookContents(TypeMoq.It.isAny())).returns(() => Promise.resolve(expectedNotebookContent)); notebookManagers[0].contentManager = mockContentManager.object; // When I initialize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined); await model.requestModelLoad(true); // Then Trust should be true @@ -233,7 +233,7 @@ suite('notebook model', function (): void { let options: INotebookModelOptions = Object.assign({}, defaultModelOptions, >{ factory: mockModelFactory.object }); - let model = new NotebookModel(options, undefined, logService); + let model = new NotebookModel(options, undefined, logService, undefined); model.onClientSessionReady((session) => actualSession = session); await model.requestModelLoad(); await model.startSession(notebookManagers[0]); @@ -251,14 +251,14 @@ suite('notebook model', function (): void { }); test('Should sanitize kernel display name when IP is included', async function (): Promise { - let model = new NotebookModel(defaultModelOptions, undefined, logService); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined); let displayName = 'PySpark (1.1.1.1)'; let sanitizedDisplayName = model.sanitizeDisplayName(displayName); should(sanitizedDisplayName).equal('PySpark'); }); test('Should sanitize kernel display name properly when IP is not included', async function (): Promise { - let model = new NotebookModel(defaultModelOptions, undefined, logService); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined); let displayName = 'PySpark'; let sanitizedDisplayName = model.sanitizeDisplayName(displayName); should(sanitizedDisplayName).equal('PySpark'); @@ -269,7 +269,7 @@ suite('notebook model', function (): void { let mockContentManager = TypeMoq.Mock.ofType(LocalContentManager); mockContentManager.setup(c => c.getNotebookContents(TypeMoq.It.isAny())).returns(() => Promise.resolve(expectedNotebookContent)); notebookManagers[0].contentManager = mockContentManager.object; - let model = new NotebookModel(defaultModelOptions, undefined, logService); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined); await model.requestModelLoad(false); let actualChanged: NotebookContentChange;