Edit Connection Feature added, edit existing connection in connection tree. (#10214)

* Added Edit Connection Command

* Wip changes for new connection dialog

* Testing

* WIP commit

* added ID check to ensure connection

* wip commit

* model id check implemented

* addfooterbutton now accepts events

* wip commit

* message explaining check

* temporary change

* connectionManagementService restored

* Revert "connectionManagementService restored"

This reverts commit 9704a63184a06a33bee2648ef0a899229d117cc0.

* formatting test

* editConnection promise testing

* edit existing connection command added

* WIP Connection Edit

* disconnect added to editConnection promise

* WIP on editExistingConnection

* changed isEdit to true

* Amir/edit connection (#10112)

* Get edit connection working

* Delete unused code

* check for isEdit as well

* connection tree test added

* WIP connection management tests

* comment out test to find out what's wrong

* fix for one error

* added note about test skipped

* changed signature of saveprofile

* saveprofile fixed

* wrote working test

* added additional test

* changed message

* Fixes made

* fix for matcher

Co-authored-by: Amir Omidi <amomidi@microsoft.com>

src/sql/workbench/contrib/objectExplorer/test/browser/connectionTreeActions.test.ts

@@ -8,7 +8,7 @@ import * as assert from 'assert';
 import { ConnectionProfileGroup } from 'sql/platform/connection/common/connectionProfileGroup';
 import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile';
 import {
-	RefreshAction, AddServerAction, DeleteConnectionAction, DisconnectConnectionAction,
+	RefreshAction, EditConnectionAction, AddServerAction, DeleteConnectionAction, DisconnectConnectionAction,
 	ActiveConnectionsFilterAction, RecentConnectionsFilterAction
 }
 	from 'sql/workbench/services/objectExplorer/browser/connectionTreeAction';
@@ -68,7 +68,7 @@ suite('SQL Connection Tree Action tests', () => {
 		connectionManagementService.setup(x => x.deleteConnectionGroup(TypeMoq.It.isAny())).returns(() => Promise.resolve(true));
 		connectionManagementService.setup(x => x.deleteConnection(TypeMoq.It.isAny())).returns(() => Promise.resolve(true));
 		connectionManagementService.setup(x => x.getConnectionProfile(TypeMoq.It.isAny())).returns(() => profileToReturn);
-
+		connectionManagementService.setup(x => x.showEditConnectionDialog(TypeMoq.It.isAny())).returns(() => new Promise<void>((resolve, reject) => resolve()));
 		return connectionManagementService;
 	}
 
@@ -527,4 +527,32 @@ suite('SQL Connection Tree Action tests', () => {
 		});
 	});
 
+	test('EditConnectionAction - test if show connection dialog is called', () => {
+		let connectionManagementService = createConnectionManagementService(true, undefined);
+
+		let connection: ConnectionProfile = new ConnectionProfile(capabilitiesService, {
+			connectionName: 'Test',
+			savePassword: false,
+			groupFullName: 'testGroup',
+			serverName: 'testServerName',
+			databaseName: 'testDatabaseName',
+			authenticationType: 'integrated',
+			password: 'test',
+			userName: 'testUsername',
+			groupId: undefined,
+			providerName: mssqlProviderName,
+			options: {},
+			saveProfile: true,
+			id: 'testId'
+		});
+
+		let connectionAction: EditConnectionAction = new EditConnectionAction(EditConnectionAction.ID,
+			EditConnectionAction.LABEL,
+			connection,
+			connectionManagementService.object);
+
+		return connectionAction.run().then((value) => {
+			connectionManagementService.verify(x => x.showEditConnectionDialog(TypeMoq.It.isAny()), TypeMoq.Times.once());
+		});
+	});
 });

src/sql/workbench/services/connection/browser/connectionDialogService.ts

@@ -89,7 +89,7 @@ export class ConnectionDialogService implements IConnectionDialogService {
 		@IConfigurationService private _configurationService: IConfigurationService,
 		@IClipboardService private _clipboardService: IClipboardService,
 		@ICommandService private _commandService: ICommandService,
-		@ILogService private _logService: ILogService
+		@ILogService private _logService: ILogService,
 	) {
 		this.initializeConnectionProviders();
 	}
@@ -355,6 +355,7 @@ export class ConnectionDialogService implements IConnectionDialogService {
 	}
 
 	private updateModelServerCapabilities(model: IConnectionProfile) {
+
 		if (this._model) {
 			this._model.dispose();
 		}
@@ -410,32 +411,32 @@ export class ConnectionDialogService implements IConnectionDialogService {
 		return this._dialogDeferredPromise.promise;
 	}
 
-	public showDialog(
+	public async showDialog(
 		connectionManagementService: IConnectionManagementService,
 		params?: INewConnectionParams,
 		model?: IConnectionProfile,
 		connectionResult?: IConnectionResult,
-		connectionOptions?: IConnectionCompletionOptions): Promise<void> {
+		connectionOptions?: IConnectionCompletionOptions,
+	): Promise<void> {
 
 		this._connectionManagementService = connectionManagementService;
 
 		this._options = connectionOptions;
 		this._params = params;
 		this._inputModel = model;
-		return new Promise<void>((resolve, reject) => {
-			this.updateModelServerCapabilities(model);
-			// If connecting from a query editor set "save connection" to false
-			if (params && (params.input && params.connectionType === ConnectionType.editor ||
-				params.connectionType === ConnectionType.temporary)) {
-				this._model.saveProfile = false;
-			}
 
-			resolve(this.showDialogWithModel().then(() => {
-				if (connectionResult && connectionResult.errorMessage) {
-					this.showErrorDialog(Severity.Error, this._connectionErrorTitle, connectionResult.errorMessage, connectionResult.callStack);
-				}
-			}));
-		});
+		this.updateModelServerCapabilities(model);
+
+		// If connecting from a query editor set "save connection" to false
+		if (params && (params.input && params.connectionType === ConnectionType.editor ||
+			params.connectionType === ConnectionType.temporary)) {
+			this._model.saveProfile = false;
+		}
+		await this.showDialogWithModel();
+
+		if (connectionResult && connectionResult.errorMessage) {
+			this.showErrorDialog(Severity.Error, this._connectionErrorTitle, connectionResult.errorMessage, connectionResult.callStack);
+		}
 	}
 
 	private async doShowDialog(params: INewConnectionParams): Promise<void> {

src/sql/workbench/services/connection/browser/connectionManagementService.ts

@@ -225,6 +225,26 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		});
 	}
 
+	/**
+	 * Opens the edit connection dialog
+	 * @param model the existing connection profile to edit on.
+	 */
+	public async showEditConnectionDialog(model: interfaces.IConnectionProfile): Promise<void> {
+		if (!model) {
+			throw new Error('Connection Profile is undefined');
+		}
+		let params = {
+			connectionType: ConnectionType.default,
+			isEditConnection: true
+		};
+
+		try {
+			return await this._connectionDialogService.showDialog(this, params, model);
+		} catch (dialogError) {
+			this._logService.warn('failed to open the connection dialog. error: ' + dialogError);
+		}
+	}
+
 	/**
 	 * Load the password for the profile
 	 * @param connectionProfile Connection Profile
@@ -420,6 +440,8 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		connection.options['groupId'] = connection.groupId;
 		connection.options['databaseDisplayName'] = connection.databaseName;
 
+		let isEdit = options?.params?.isEditConnection ?? false;
+
 		if (!uri) {
 			uri = Utils.generateUri(connection);
 		}
@@ -449,6 +471,7 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		if (!tokenFillSuccess) {
 			throw new Error(nls.localize('connection.noAzureAccount', "Failed to get Azure account token for connection"));
 		}
+
 		return this.createNewConnection(uri, connection).then(async connectionResult => {
 			if (connectionResult && connectionResult.connected) {
 				// The connected succeeded so add it to our active connections now, optionally adding it to the MRU based on
@@ -459,8 +482,15 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 				if (callbacks.onConnectSuccess) {
 					callbacks.onConnectSuccess(options.params, connectionResult.connectionProfile);
 				}
-				if (options.saveTheConnection) {
-					await this.saveToSettings(uri, connection).then(value => {
+				if (options.saveTheConnection || isEdit) {
+					let matcher: interfaces.ProfileMatcher;
+					if (isEdit) {
+						matcher = (a: interfaces.IConnectionProfile, b: interfaces.IConnectionProfile) => {
+							return a.id === b.id;
+						};
+					}
+
+					await this.saveToSettings(uri, connection, matcher).then(value => {
 						this._onAddConnectionProfile.fire(connection);
 						this.doActionsAfterConnectionComplete(value, options);
 					});
@@ -496,6 +526,7 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		});
 	}
 
+
 	private handleConnectionError(connection: interfaces.IConnectionProfile, uri: string, options: IConnectionCompletionOptions, callbacks: IConnectionCallbacks, connectionResult: IConnectionResult) {
 		let connectionNotAcceptedError = nls.localize('connectionNotAcceptedError', "Connection Not Accepted");
 		if (options.showFirewallRuleOnError && connectionResult.errorCode) {
@@ -529,7 +560,7 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		});
 	}
 
-	private doActionsAfterConnectionComplete(uri: string, options: IConnectionCompletionOptions,) {
+	private doActionsAfterConnectionComplete(uri: string, options: IConnectionCompletionOptions): void {
 		let connectionManagementInfo = this._connectionStatusManager.findConnection(uri);
 		if (options.showDashboard) {
 			this.showDashboardForConnectionManagementInfo(connectionManagementInfo.connectionProfile);
@@ -873,11 +904,10 @@ export class ConnectionManagementService extends Disposable implements IConnecti
 		});
 	}
 
-	private saveToSettings(id: string, connection: interfaces.IConnectionProfile): Promise<string> {
-		return this._connectionStore.saveProfile(connection).then(savedProfile => {
-			let newId = this._connectionStatusManager.updateConnectionProfile(savedProfile, id);
-			return newId;
-		});
+
+	private async saveToSettings(id: string, connection: interfaces.IConnectionProfile, matcher?: interfaces.ProfileMatcher): Promise<string> {
+		const savedProfile = await this._connectionStore.saveProfile(connection, undefined, matcher);
+		return this._connectionStatusManager.updateConnectionProfile(savedProfile, id);
 	}
 
 	/**

src/sql/workbench/services/connection/test/browser/connectionManagementService.test.ts

@@ -102,7 +102,7 @@ suite('SQL ConnectionManagementService tests', () => {
 		connectionDialogService.setup(x => x.showDialog(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(none));
 
 		connectionStore.setup(x => x.addRecentConnection(TypeMoq.It.isAny())).returns(() => Promise.resolve());
-		connectionStore.setup(x => x.saveProfile(TypeMoq.It.isAny())).returns(() => Promise.resolve(connectionProfile));
+		connectionStore.setup(x => x.saveProfile(TypeMoq.It.isAny(), TypeMoq.It.is(x => true), TypeMoq.It.is(x => true))).returns(() => Promise.resolve(connectionProfile));
 		workbenchEditorService.setup(x => x.openEditor(undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(undefined));
 		connectionStore.setup(x => x.addSavedPassword(TypeMoq.It.is<IConnectionProfile>(
 			c => c.serverName === connectionProfile.serverName))).returns(() => Promise.resolve({ profile: connectionProfile, savedCred: true }));
@@ -204,7 +204,7 @@ suite('SQL ConnectionManagementService tests', () => {
 
 		if (options) {
 			if (options.saveTheConnection) {
-				connectionStore.verify(x => x.saveProfile(TypeMoq.It.isAny()), TypeMoq.Times.once());
+				connectionStore.verify(x => x.saveProfile(TypeMoq.It.isAny(), TypeMoq.It.is(x => true), TypeMoq.It.is(x => true)), TypeMoq.Times.once());
 			}
 			if (options.showDashboard) {
 				workbenchEditorService.verify(x => x.openEditor(undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once());
@@ -358,7 +358,7 @@ suite('SQL ConnectionManagementService tests', () => {
 			showFirewallRuleOnError: true
 		};
 
-		return connect(uri, options).then(() => {
+		return connect(uri, options).then((result) => {
 			verifyOptions(options);
 			assert.notEqual(paramsInOnConnectSuccess, undefined);
 			assert.equal(paramsInOnConnectSuccess.connectionType, options.params.connectionType);
@@ -441,6 +441,82 @@ suite('SQL ConnectionManagementService tests', () => {
 		});
 	});
 
+	test('Edit Connection - Changing connection profile name for same URI should persist after edit', () => {
+		let profile = connectionProfile;
+		let uri1 = 'test_uri1';
+		let newname = 'connection renamed';
+		let options: IConnectionCompletionOptions = {
+			params: {
+				connectionType: ConnectionType.editor,
+				input: {
+					onConnectSuccess: undefined,
+					onConnectReject: undefined,
+					onConnectStart: undefined,
+					onDisconnect: undefined,
+					onConnectCanceled: undefined,
+					uri: uri1,
+				},
+				querySelection: undefined,
+				runQueryOnCompletion: RunQueryOnConnectionMode.none,
+				isEditConnection: false
+			},
+			saveTheConnection: true,
+			showDashboard: false,
+			showConnectionDialogOnError: true,
+			showFirewallRuleOnError: true
+		};
+
+		return connect(uri1, options, true, profile).then(result => {
+			assert.equal(result.connected, true);
+			let newProfile = connectionProfile;
+			newProfile.connectionName = newname;
+			options.params.isEditConnection = true;
+			return connect(uri1, options, true, profile).then(result => {
+				assert.equal(result.connected, true);
+				assert.equal(connectionManagementService.getConnectionProfile(uri1).connectionName, newname);
+			});
+		});
+	});
+
+	test('Edit Connection - Connecting a different URI with same profile via edit should not change profile ID.', () => {
+		let profile = connectionProfile;
+		profile.id = '0451';
+		let uri1 = 'test_uri1';
+		let uri2 = 'test_uri2';
+		let options: IConnectionCompletionOptions = {
+			params: {
+				connectionType: ConnectionType.editor,
+				input: {
+					onConnectSuccess: undefined,
+					onConnectReject: undefined,
+					onConnectStart: undefined,
+					onDisconnect: undefined,
+					onConnectCanceled: undefined,
+					uri: uri1
+				},
+				querySelection: undefined,
+				runQueryOnCompletion: RunQueryOnConnectionMode.none,
+				isEditConnection: false
+			},
+			saveTheConnection: true,
+			showDashboard: false,
+			showConnectionDialogOnError: true,
+			showFirewallRuleOnError: true
+		};
+
+		return connect(uri1, options, true, profile).then(result => {
+			assert.equal(result.connected, true);
+			options.params.isEditConnection = true;
+			return connect(uri2, options, true, profile).then(result => {
+				assert.equal(result.connected, true);
+				let uri1info = connectionManagementService.getConnectionInfo(uri1);
+				let uri2info = connectionManagementService.getConnectionInfo(uri2);
+				assert.equal(uri1info.connectionProfile.id, uri2info.connectionProfile.id);
+			});
+		});
+	});
+
+
 	test('failed firewall rule should open the firewall rule dialog', () => {
 		handleFirewallRuleResult.canHandleFirewallRule = true;
 		resolveHandleFirewallRuleDialog = true;

src/sql/workbench/services/objectExplorer/browser/connectionTreeAction.ts

@@ -83,6 +83,30 @@ export class RefreshAction extends Action {
 	}
 }
 
+export class EditConnectionAction extends Action {
+	public static ID = 'registeredServers.editConnection';
+	public static LABEL = localize('connectionTree.editConnection', "Edit Connection");
+
+	constructor(
+		id: string,
+		label: string,
+		private _connectionProfile: ConnectionProfile,
+		@IConnectionManagementService private _connectionManagementService: IConnectionManagementService
+	) {
+		super(id, label);
+		this.class = 'edit-server-action';
+	}
+
+	public async run(): Promise<boolean> {
+		if (!this._connectionProfile) {
+			return false;
+		}
+
+		await this._connectionManagementService.showEditConnectionDialog(this._connectionProfile);
+		return true;
+	}
+}
+
 export class DisconnectConnectionAction extends Action {
 	public static ID = 'objectExplorer.disconnect';
 	public static LABEL = localize('DisconnectAction', "Disconnect");

src/sql/workbench/services/objectExplorer/browser/serverTreeActionProvider.ts

@@ -9,7 +9,7 @@ import { IInstantiationService } from 'vs/platform/instantiation/common/instanti
 import { IContextKeyService } from 'vs/platform/contextkey/common/contextkey';
 
 import {
-	DisconnectConnectionAction, AddServerAction,
+	DisconnectConnectionAction, AddServerAction, EditConnectionAction,
 	DeleteConnectionAction, RefreshAction, EditServerGroupAction
 } from 'sql/workbench/services/objectExplorer/browser/connectionTreeAction';
 import { TreeNode } from 'sql/workbench/services/objectExplorer/common/treeNode';
@@ -129,6 +129,7 @@ export class ServerTreeActionProvider {
 		if (this._connectionManagementService.isProfileConnected(context.profile)) {
 			actions.push(this._instantiationService.createInstance(DisconnectConnectionAction, DisconnectConnectionAction.ID, DisconnectConnectionAction.LABEL, context.profile));
 		}
+		actions.push(this._instantiationService.createInstance(EditConnectionAction, EditConnectionAction.ID, EditConnectionAction.LABEL, context.profile));
 		actions.push(this._instantiationService.createInstance(DeleteConnectionAction, DeleteConnectionAction.ID, DeleteConnectionAction.DELETE_CONNECTION_LABEL, context.profile));
 
 		// Contribute refresh action for scriptable objects via contribution