Connection dialog: dropdown for databases #489

abist · 2018-01-16T12:51:19Z

Fixes #22

Functionality is similar to SSDT's functionality. I've attached some screenshots -

Dropdown when a supported server name is populated

UI when a connection is made to a server (Loading in database input)

Error thrown when trying to fetch databases from a server that doesn't exist

Dropdown populated when a connection is picked from the recent list

MattIrv

I left a couple minor code comments, but I have some bigger comments on the overall design that I think need to be figured out before getting this merged:

Thanks for the comments @MattIrv ! These are very helpful! This was an open-ended design, so I went with a few assumptions, definitely wanted a few opinions. I've addressed the points you made in this comment itself so it's easier to read.

What's the correct behavior for non-integrated auth? Right now it just seems to always show <Default>, which isn't very helpful. Additionally the first time I opened the connection dialog on Mac, where the default auth type is SQL Login, it showed "undefined" as the database name, though I didn't see an error in the developer console
I'll look into this
Can we do this in a way where users can still type in database names? This could be useful if the connection is slow, for example, and may also help address some of my other concerns here
I did dig into this, and found that the Dropdown class supported this, however the UI looked way out of place, so I went ahead with SelectBox for now. I can switch to using Dropdown if we all agree to it.
The behavior when a user clicks a recent or saved connection is a bit weird right now because the saved database shows up in the drop-down, but can't be changed
Agreed, this is a flaw which can be rectified by using the Dropdown I mentioned above. But to make the UI consistent, I had to use SelectBox.
Is there a better way to show error messages? It might be best to just silently fail, assuming the subsequent connection will also fail and display an error. Right now if the error message pops up it's not entirely clear what user action caused the error message to show up or how the user needs to fix it
SSDT shows the same error, and I tried to adhere what they were doing. Would just blocking the input boxes until the connection fails suffice, or do you have another idea?
Am I correct from looking at the code that this requires users to have access to master to make the initial connection to list databases? It's not necessarily the case that all users will have access is it?
I'm not sure about this yet.

MattIrv · 2018-01-16T19:13:48Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

@@ -51,14 +53,28 @@ export class ConnectionWidget {
 		[Constants.mssqlProviderName]: [new AuthenticationType(Constants.integrated, false), new AuthenticationType(Constants.sqlLogin, true)]
 	};
 	private _saveProfile: boolean;
+	private _defaultDatabaseName : string = '<Default>';


The call to localize should be here, instead of on each use of it

MattIrv · 2018-01-16T19:26:42Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

+					this._updateDatabaseNames();
+				} else {
+
+					this._databaseNameInputBox.setOptions([this.DefaultDatabaseGroup].map(d => d.name), 0);


extra blank lines

anthonydresser · 2018-01-16T19:45:15Z

How does a user activate the connection being made? sssms specifically tells you it will make a connection then opens a new dialog. The problem is we shouldn't be making connections to servers unless it explicit that thats what we are doing. Before @kburtram and I were thinking of a refresh icon next to the database input that would then populate it, perhaps a yes/no/always message that pops up before making the connection.
The refresh button seems like a good idea. I agree that there are unnecessary connection attempts when fetching the list of databases.

kburtram · 2018-01-16T20:40:19Z

SSMS explicitly prompts, but SSDT will try to connect automatically. I'm fine with either approach, assuming the auto-connect doesn't freeze up the UI or otherwise harm the scenario. We should probably outline the two approaches and get PM and UX to provide guidance.

In reply to: 358081856 [](ancestors = 358081856)

kburtram · 2018-01-16T20:46:13Z

Regarding this previous comment...

Can we do this in a way where users can still type in database names? This could be useful if the connection is slow, for example, and may also help address some of my other concerns here
I did dig into this, and found that the Dropdown class supported this, however the UI looked way out of place, so I went ahead with SelectBox for now. I can switch to using Dropdown if we all agree to it.

...we must continue to support allowing the user to type in the database name. Many people will know what they want DB to connect to and requiring a separate connection to populate the dropdown will be a cumbersome experience.

kburtram · 2018-01-16T20:47:43Z

src/sql/parts/connection/connectionDialog/connectionController.ts

@@ -54,6 +57,29 @@ export class ConnectionController implements IConnectionComponentController {
 		});
 	}

+	private updateDatabaseNames(serverName : string): Promise<string[]> {
+		let tempProfile = this._model;
+		tempProfile.authenticationType = Constants.integrated;


tempProfile.authenticationType = Constants.integrated; [](start = 2, length = 54)

why are we doing this? Shouldn't we try to connect with what ever authentication type and credentials the user provided?

SSDT only does it for Integrated Auth, because it does it automatically. If we're going the SSMS way, then it can support both auth types.

kburtram · 2018-01-16T20:48:54Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

+		if (this.serverName) {
+			// connect to server and fetch database names
+			if (this.authenticationType === Constants.integrated) {
+				this._databaseNameInputBox.setOptions(['Loading...'], 0);


'Loading... [](start = 43, length = 11)

should this be localized?

kburtram · 2018-01-16T20:49:08Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

+	private _updateDatabaseNames() : void {
+		if (this.serverName) {
+			// connect to server and fetch database names
+			if (this.authenticationType === Constants.integrated) {


this.authenticationType === Constants.integrated [](start = 7, length = 48)

why?

kburtram · 2018-01-16T20:52:06Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

+					} else {
+						this._databaseNameInputBox.setOptions([this.DefaultDatabaseGroup, this.LoadingDatabaseGroup].map(d => d.name), 0);
+					}
+					this._serverNameInputBox.enable();


this._serverNameInputBox [](start = 5, length = 24)

is there any concern that the dialog may be closed while this async task is running? i.e., have you verified this task is properly canceled in this scenario?

kburtram · 2018-01-16T20:54:38Z

src/sql/parts/connection/connectionDialog/connectionWidget.ts

 	}

 	public connect(model: IConnectionProfile): boolean {
 		let validInputs = this.validateInputs();
 		if (validInputs) {
 			model.serverName = this.serverName;
-			model.databaseName = this.databaseName;
+			model.databaseName = this.databaseName === this._defaultDatabaseName ? 'master' : this.databaseName;


this._defaultDatabaseName ? 'master' [](start = 46, length = 36)

the default DB won't always be master, so I think this is incorrect. Can we go back to the previous behavior where a blank value means default and we let the server fill in what the default DB is once it connects?

kburtram

There are a few minor fixes that should be made and some design decisions that need to be discussed. Could you please aggregate all the design issues and start an email thread to drive to resolution? Thanks!

abist · 2018-01-30T01:30:45Z

Closing this PR because a newer one available here - #583

* Multi-thread server lookup to increase perf for multiple subscriptions * Use parallel execution for listsubscriptions - Also refactored parallel execution code to be a bit more generic

Aditya Bist added 7 commits January 15, 2018 14:25

functional dropdown without validation and auth type change behavior

2a863d2

removed unused imports

2659565

added support for default database

8642958

disabled server and db input when connecting

5558c7a

support recent connections

62fcce4

fixed formatting

6410f5e

removed unused imports

0089534

abist requested review from kburtram, MattIrv, anthonydresser, kevcunnane and llali January 16, 2018 12:51

Merge branch 'master' into connDialog/dropdown

a417c0c

MattIrv reviewed Jan 16, 2018

View reviewed changes

abist changed the title ~~Conn dialog/dropdown~~ Connection dialog: dropdown for databases Jan 16, 2018

kburtram reviewed Jan 16, 2018

View reviewed changes

kburtram requested changes Jan 16, 2018

View reviewed changes

abist mentioned this pull request Jan 30, 2018

Connection Dialog: Dropdown for database name #583

Merged

abist closed this Jan 30, 2018

abist deleted the connDialog/dropdown branch February 10, 2018 00:13

anthonydresser pushed a commit to anthonydresser/azuredatastudio that referenced this pull request Mar 15, 2019

Listen on formatter registrations (microsoft#489)

c3fbe63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection dialog: dropdown for databases #489

Connection dialog: dropdown for databases #489

abist commented Jan 16, 2018 •

edited

Loading

MattIrv left a comment •

edited by abist

Loading

MattIrv Jan 16, 2018

MattIrv Jan 16, 2018

anthonydresser commented Jan 16, 2018 •

edited by abist

Loading

kburtram commented Jan 16, 2018

kburtram commented Jan 16, 2018

kburtram Jan 16, 2018

abist Jan 17, 2018

kburtram Jan 16, 2018

kburtram Jan 16, 2018

kburtram Jan 16, 2018

kburtram Jan 16, 2018

kburtram left a comment

abist commented Jan 30, 2018

Connection dialog: dropdown for databases #489

Connection dialog: dropdown for databases #489

Conversation

abist commented Jan 16, 2018 • edited Loading

MattIrv left a comment • edited by abist Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonydresser commented Jan 16, 2018 • edited by abist Loading

kburtram commented Jan 16, 2018

kburtram commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kburtram left a comment

Choose a reason for hiding this comment

abist commented Jan 30, 2018

abist commented Jan 16, 2018 •

edited

Loading

MattIrv left a comment •

edited by abist

Loading

anthonydresser commented Jan 16, 2018 •

edited by abist

Loading