Update :server connect form to pick database #1194

OskarDamkjaer · 2020-09-15T15:10:25Z

Introduces a db url param for selecting a database and dbms as an alternative param to connectURL.

Prefills form from the url like so:

src/browser/components/buttons/index.jsx

src/shared/modules/connections/connectionsDuck.js

src/shared/modules/discovery/discoveryDuck.js

OskarDamkjaer · 2020-09-16T09:30:10Z

src/shared/modules/discovery/discoveryDuck.js

@@ -156,7 +167,11 @@ export const discoveryOnStartupEpic = (some$, store) => {
              getAllowedBoltSchemes(store.getState()),
              host
            )
-            store.dispatch(updateDiscoveryConnection({ host })) // Update discovery host in redux
+            const supportsMultiDb =
+              parseInt((result.neo4j_version || '0').charAt(0)) >= 4


I'm thinking there's a better way to get version number? Or is this fine?

This is actually not the whole truth, I think. Some Aura editions has version 4 but do not support multi-db.
Better check with them what version they expose in the discovery endpoint.
Ugly things like

neo4j-browser/src/shared/modules/dbMeta/dbMeta.utils.js

Line 46 in 5869d51

if (['4.0-aura', '4.0-AuraProfessional'].includes(serverInfo.version)) {

So do you suggest we check that the version starts with 4 and is not running on aura?

Also, good catch!

Some aura editions do support multi-db, so it's tricky. I suggest you talk to Aura and ask which editions that has multi-db support and what version string those editions expose in the discovery endpoint.

Asked just now!

It seems aura only supports the system + 1 other database at this moment. I think it'd make sense to disable the field for aura, WDYT?

src/browser/modules/Stream/Auth/ConnectForm.tsx

src/shared/modules/dbMeta/dbMetaDuck.js

src/shared/modules/connections/connectionsDuck.js

oskarhane

Cool, well done!

I have a few comments on naming and UI.

Connect frame UI

I don't think this looks particularly good to be honest. The form looks very unusual.

Auto-connect on refresh

It looks like the auto-connect doesn't work anymore. Successfully connect. Refresh page. You should auto-connect.

Continuously trying to `:use`

In the scenario of connecting successfully but using a db that doesn't exist for some reason, it looks like it's running :use on every dbmeta-tick.

e2e_tests/integration/connect-form.spec.ts

src/browser/modules/Stream/Auth/ConnectionForm.jsx

src/shared/modules/connections/connectionsDuck.js

src/shared/modules/discovery/discoveryDuck.js

oskarhane · 2020-09-18T12:56:37Z

src/shared/modules/discovery/discoveryDuck.js

@@ -156,7 +167,11 @@ export const discoveryOnStartupEpic = (some$, store) => {
              getAllowedBoltSchemes(store.getState()),
              host
            )
-            store.dispatch(updateDiscoveryConnection({ host })) // Update discovery host in redux
+            const supportsMultiDb =
+              parseInt((result.neo4j_version || '0').charAt(0)) >= 4


This is actually not the whole truth, I think. Some Aura editions has version 4 but do not support multi-db.
Better check with them what version they expose in the discovery endpoint.
Ugly things like

neo4j-browser/src/shared/modules/dbMeta/dbMeta.utils.js

Line 46 in 5869d51

if (['4.0-aura', '4.0-AuraProfessional'].includes(serverInfo.version)) {

src/shared/modules/discovery/discoveryDuck.js

oskarhane · 2020-09-18T13:11:26Z

src/browser/modules/Stream/Auth/ConnectForm.tsx

+                data-testid="database"
+                onChange={props.onDatabaseChange}
+                value={props.database}
+                placeholder="default database"


Maybe change this to "Database to use" or something

You mean the label? I think the current placeholder makes sense

I don't :)
It's not a default db the users enters, it's the target db that the user wants to use when a connection is made.

Yeah and that's why the place holder is there to show what happens if you leave the field empty! Right now it think it makes sense to have a field titled "Database to use" and the default/placeholder being "default database". Having both be "database to use" I think would lead to me thinking the field was required

I see your reasoning, but placeholders are hints on what the users should enter, not to indicate value if left blank.
The label could suggest "leave empty for default)" or something to let the users know what happens if it's left empty.

OskarDamkjaer · 2020-09-22T08:28:15Z

Good catches, didn't see the screen shots you sent! Thanks a lot! :))

OskarDamkjaer · 2020-09-22T13:31:57Z

Updated the form styling when I was thinking we'd always show the db field. Then it got really tall, but now when I know that's not the case I reverted to the older styling because it was simpler code wise and familiar.

OskarDamkjaer · 2020-09-23T11:42:07Z

Not 100% sure the aura check works. What would be the best way to try it out? Not sure where to find the discovery response object

Form allows connecting to any database Don't show error until db-meta has loaded Add reasonable error handling Try using rxjs Only works once Connect to correct db Form and query param both work Autofills form Cleanup Fix styling

Add better tests

e2e_tests/integration/connect-form.spec.ts

oskarhane

Good changes!

Still has a few more issues though:

When choosing something other than the default db, there's a delay until next dbMeta to update the meta items in the sidebar. See gif.
Same thing when entering a db that doesn't exist for the user. It takes until next dbmeta tick to update the UI with the correct info.

Also, would it be possible to add e2e test for checking so browser actually switches to use the entered db?
And maybe the error case as well?

oskarhane · 2020-10-01T06:56:24Z

src/shared/modules/discovery/discoveryDuck.js

+
+            const isAura = getEnv(store.getState()) === CLOUD
+            const supportsMultiDb =
+              !isAura && parseInt((result.neo4j_version || '0').charAt(0)) >= 4


Aren't there Aura editions that support multi-db?

From asking there's only editions with proper support for multi-db. At most they support the system-db in addition to the default one. So I think if it's important there, you'll have to use the url param

e2e_tests/integration/connect-form.spec.ts

oskarhane

Seems to behave as expected now, well done!
You just need to be extra cautious to make sure one failure in the forkJoin doesn't cancel the other ones + cancels the whole listener.

But other than that, 👍

Ping me to review when you're catching any errors and I'll approve it

oskarhane · 2020-10-05T09:02:56Z

e2e_tests/integration/connect-form.spec.ts

@@ -105,5 +111,68 @@ describe('Connect form', () => {
      cy.connect('neo4j', Cypress.config('password'), boltUrl)
      cy.executeCommand(':server disconnect')
    })
+
+    if (isEnterpriseEdition()) {
+      it.only('shows correct metadata when using db field', () => {


No .only please

I'll set up an on commit hook for this so I won't forget it again

oskarhane · 2020-10-05T09:04:20Z

src/shared/modules/dbMeta/dbMetaDuck.js

+            return Rx.Observable.forkJoin([
+              getLabelsAndTypes(store),
+              clusterRole(store),
+              databaseList(store)


Even if one of these errors we'll need to keep the stream going, so I'f like to see some error catching here

And if I remember correctly, if one fail the other ones are cancelled as well, so I think you need to catch on each one of the inner ones.

oskarhane

🌮 Merge on green tests 🌮

OskarDamkjaer force-pushed the db_form branch from 20ac6e0 to a7c1c44 Compare September 15, 2020 15:11

OskarDamkjaer commented Sep 16, 2020

View reviewed changes

OskarDamkjaer requested review from oskarhane and HerrEmil September 16, 2020 09:39

OskarDamkjaer marked this pull request as ready for review September 16, 2020 09:39

HerrEmil approved these changes Sep 17, 2020

View reviewed changes

src/shared/modules/dbMeta/dbMetaDuck.js Outdated Show resolved Hide resolved

src/shared/modules/connections/connectionsDuck.js Show resolved Hide resolved

oskarhane reviewed Sep 18, 2020

View reviewed changes

OskarDamkjaer requested a review from oskarhane September 23, 2020 11:48

OskarDamkjaer added 12 commits September 25, 2020 09:16

Add support for database selection in connect form

0027d89

Form allows connecting to any database Don't show error until db-meta has loaded Add reasonable error handling Try using rxjs Only works once Connect to correct db Form and query param both work Autofills form Cleanup Fix styling

Add unit test

649b7c6

Add better tests

Don't discover password from url

4dc0cee

Show db field only when we know it's supported

0f00b65

Fix tests

1850eec

Use styled components

5cad034

Rename variables

0363639

Don't retry when database is not available

7f3d90f

Revert style

2a05690

Disable form for aura

5209eda

Update test to ensure automatic reconnection

ecd613a

Improve aura check

7c516a2

OskarDamkjaer force-pushed the db_form branch from 20ea421 to 7c516a2 Compare September 25, 2020 07:16

Unflake display metadata item

b2d2861

OskarDamkjaer force-pushed the db_form branch from 99ad48f to b2d2861 Compare September 25, 2020 08:17

OskarDamkjaer added 2 commits September 25, 2020 13:15

Improve connect test

34d0042

Update connect-test

cd85430

oskarhane reviewed Sep 28, 2020

View reviewed changes

e2e_tests/integration/connect-form.spec.ts Outdated Show resolved Hide resolved

Update connect-form.spec.ts

ffa9567

OskarDamkjaer requested a review from oskarhane September 28, 2020 08:43

oskarhane reviewed Oct 1, 2020

View reviewed changes

Split dbmeta actions in parts

e66e972

OskarDamkjaer force-pushed the db_form branch from c725f56 to e66e972 Compare October 1, 2020 15:06

OskarDamkjaer self-assigned this Oct 2, 2020

OskarDamkjaer added 2 commits October 3, 2020 11:32

Add metadata test

3d4435d

Update form field

dade571

oskarhane reviewed Oct 5, 2020

View reviewed changes

Remove it.only

634be01

oskarhane approved these changes Oct 5, 2020

View reviewed changes

Improve some tests

3e5ed06

OskarDamkjaer merged commit 6eb5201 into neo4j:master Oct 5, 2020

OskarDamkjaer deleted the db_form branch October 5, 2020 14:44

oskarhane added the changelog label Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update :server connect form to pick database #1194

Update :server connect form to pick database #1194

OskarDamkjaer commented Sep 15, 2020 •

edited

Loading

OskarDamkjaer Sep 16, 2020

oskarhane Sep 18, 2020

OskarDamkjaer Sep 21, 2020

OskarDamkjaer Sep 21, 2020

oskarhane Sep 22, 2020

OskarDamkjaer Sep 22, 2020

OskarDamkjaer Sep 22, 2020

oskarhane left a comment

oskarhane Sep 18, 2020

oskarhane Sep 18, 2020

OskarDamkjaer Sep 21, 2020

oskarhane Sep 22, 2020

OskarDamkjaer Sep 22, 2020

oskarhane Oct 1, 2020

OskarDamkjaer commented Sep 22, 2020

OskarDamkjaer commented Sep 22, 2020

OskarDamkjaer commented Sep 23, 2020

oskarhane left a comment

oskarhane Oct 1, 2020

OskarDamkjaer Oct 1, 2020

oskarhane left a comment

oskarhane Oct 5, 2020

OskarDamkjaer Oct 5, 2020

oskarhane Oct 5, 2020

oskarhane Oct 5, 2020 •

edited

Loading

oskarhane left a comment

Update :server connect form to pick database #1194

Update :server connect form to pick database #1194

Conversation

OskarDamkjaer commented Sep 15, 2020 • edited Loading

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

Choose a reason for hiding this comment

oskarhane left a comment

Choose a reason for hiding this comment

Connect frame UI

Auto-connect on refresh

Continuously trying to :use

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

OskarDamkjaer commented Sep 22, 2020

OskarDamkjaer commented Sep 22, 2020

OskarDamkjaer commented Sep 23, 2020

oskarhane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarhane left a 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

oskarhane Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

oskarhane left a comment

Choose a reason for hiding this comment

OskarDamkjaer commented Sep 15, 2020 •

edited

Loading

Continuously trying to `:use`

oskarhane Oct 5, 2020 •

edited

Loading