IDS-4707 - Switch from InputCombo to InputText if there are too many connections to list #292

ubenzer · 2024-05-24T12:57:08Z

✏️ Changes

With this PR user creation modal box switches between a select and an input based on how many connections are there in a tenant. If too much, instead of listing them all we switch to a text field in which you can type the connection name.

📷 Screenshots

When < threshold:

When > threshold:

🔗 References

https://auth0team.atlassian.net/browse/IDS-4707

🎯 Testing

Reduce CONNECTIONS_FETCH_LIMIT to something low to be able to test it. Run DAE locally, setting up against the deployment of your choice.

See once the CONNECTIONS_FETCH_LIMIT is exceeded:

A textbox to provide connection name.
Optional user name area.

See once the CONNECTIONS_FETCH_LIMIT is not exceeded:

A select-box listing connection names to choose from.
Mandatory user name area only if connection requires a username.

Additionally see:

Only a single GET connection call is done from frontend.

🚫 This change has been tested in a Webtask

✅ This change has unit test coverage

🚫 This change has integration test coverage

🚫 This change has been tested for performance

🚀 Deployment

✅ This can be deployed any time

Will be released as a new minor version.

🎡 Rollout

In order to verify that the deployment was successful we will try this against a tenant of ours after upgrading DAE to the latest version.

🔥 Rollback

We will rollback given the new DAE version is defective.

📄 Procedure

We'll advertise the old version as the latest version in the extensions list.

…connections to list

sauntimo

Edit: I've just spotted a failing test, so I am going to have to make code changes anyway -> therefore I'll address my own comments as well.

I've tested this locally and verified that it works as designed with 20+ connections. Overall this works well and solves the immediate problem for ICIMS so happy to approve.

One final comment; if the limit of 20 connections is exceeded, do we still want the name of the first connection to appear in the create user dialogue in the free text box? I feel like it's one thing to set a default value in a drop down, but when you pre-set a value in a text field it feels more like we're telling the user that's what the value should be, which I think we should avoid. It would be pretty easy to add a conditional where this is set here. This is just my personal take on it, so if no one else thinks it's an issue, I'm happy to leave it.

sauntimo · 2024-05-28T17:27:04Z

client/utils/useDefaultFields.js

@@ -61,14 +63,15 @@ export const useConnectionsField = (isEditField, fields, connections, onConnecti
    return _.remove(fields, { property: 'connection' });
  }

+  const isConnectionListingLimitExceeded =  connections.length >= CONNECTIONS_LIST_LIMIT;
  const defaults = {
    property: 'connection',
    label: 'Connection',


Suggested change

label: 'Connection',

label: isConnectionListingLimitExceeded ? 'Connection Name' : 'Connection',

I think when it's free text, we should specify that we need the connection name, rather than ID which I initially assumed it would be

sauntimo · 2024-05-28T17:32:34Z

server/routes/connections.js

@@ -3,10 +3,12 @@ import { Router } from 'express';

 import multipartRequest from '../lib/multipartRequest';

+const CONNECTIONS_LIST_LIMIT = 50;


If the only thing we fetch connections for is the dropdown for adding a new user, and we only show up to 20 in that drop down as per here, is there any need to fetch more than 20 from the db? Should we re-use the same const and only fetch 20 connections?

to answer this, I think this number is set at 50 as it's the same as the perPage number, so we're fetching one full page of connections. Reducing this to 20 doesn't really result in perf improvements as it's still one API2 call.

sauntimo

I'm just voiding my own review now that I've pushed changes.

sauntimo · 2024-05-28T18:31:14Z

Changes I made:

fix failing test (connection field was expected to be "select", not "text")
If connections >= 20, connection field label is "Connection Name" to reinforce that we need the name not the ID
If connections >= 20, no longer pre-populates connection name field when creating a user

nkavtur · 2024-05-29T10:49:38Z

server/routes/connections.js

@@ -2,11 +2,12 @@ import _ from 'lodash';
 import { Router } from 'express';

 import multipartRequest from '../lib/multipartRequest';
+import { CONNECTIONS_LIST_LIMIT } from "../constants";


Should this constant be added to the server/constants.js file? Now you have it only in the client/constants.js

good spot - addressed in 340edf0

server/lib/multipartRequest.js

nkavtur · 2024-05-29T19:05:47Z

server/routes/connections.js


 export default (scriptManager) => {
  const api = Router();
  api.get('/', (req, res, next) => {
-    multipartRequest(req.auth0, 'connections', { strategy: 'auth0', fields: 'id,name,strategy,options' })
+    multipartRequest(req.auth0, 'connections', { strategy: 'auth0', fields: 'id,name,strategy,options' }, { perPage: 100, limit: CONNECTIONS_FETCH_LIMIT})


do we need perPage: 100? Should not the default 50 be fine?

Yeah I agree with this, we only use 20 anyway, and we'd still only make one request whether it was 50 or 100. I'll remove this and we can use the default.

sauntimo

Apparently I have to approve as I previously "requested changes"

ubenzer · 2024-06-03T10:29:30Z

server/routes/connections.js

@@ -2,11 +2,13 @@ import _ from 'lodash';
 import { Router } from 'express';

 import multipartRequest from '../lib/multipartRequest';
+// only fetch one page of connections
+const CONNECTIONS_FETCH_LIMIT = 50;


This limit should be equal or slightly more than export const CONNECTIONS_LIST_LIMIT = 20000;.

Explanation:
CONNECTIONS_LIST_LIMIT is the UI limit to control the logic between text and list. However this logic will be executed based on the list returned by backend.

CONNECTIONS_FETCH_LIMIT is the backend limit to control at most how many items will be retrieved. If more than this number, we'll cap at this number and report back to frontend.

For CONNECTIONS_LIST_LIMIT based frontend logic to work properly, we need to report cap CONNECTIONS_FETCH_LIMIT at lest at CONNECTIONS_FETCH_LIMIT, if possible slightly higher.

Right now, the connections will always be capped at 50 regardless of how many are there and everyone will see fist 50 connections in a list.

Note: As an alternative, we can fetch the count, notice that it exceeds the limit and skip traversing altogether and tell frontend "too much to fetch" in a special response. This would have the same effect with significantly less calls.

sauntimo · 2024-06-03T14:43:28Z

Setup

Testing using a limit of 20 records (rather than 20k because it's easier
db-1 connection has require username enabled
db-2 connection has require username disabled

With connections count greater than limit

db connections in tenant exceeds limit so no connections passed to frontend
therefore we show username field but don't require a value

Connection with require username enabled

Attempt to create a user in db-1 connection, we are allowed to make the call (pass form validation) but see API error: payload validation error: 'Missing required property: username'.
If we add a username, we can successfully create a user in this connection

Connection with require username disabled

Attempt to create user in db-2 connection without username: created successfully as connection does not require it

With connections count less than limit

both scenarios show connection drop down select as number of connection is less than limit

Connection with require username enabled

selecting db-1 connection shows username field as that connection requires it

Connection with require username disabled

selecting db-2 connection does not show username field as that connection does not require it

sauntimo · 2024-06-03T16:31:00Z

I found that the easiest way to communicate to the frontend that the connection limit was exceeded, was to return 0 connections. I think this is a reasonable approach; if there are actually 0 database connections in the tenant, user creation through the DAE would not be possible anyway as it requires a database connection. I've added comments to explain the approach. I'm happy to try and do something more explicit if anyone feels it is necessary but this seemed like a good compromise.

client/containers/Users/Users.jsx

server/routes/connections.js

server/lib/multipartRequest.js

…to its final value

IDS-4707 - Switch from InputCombo to InputText if there are too many …

974fcb9

…connections to list

sauntimo approved these changes May 28, 2024

View reviewed changes

sauntimo self-requested a review May 28, 2024 17:53

Fix test + QoL improvements

b398294

sauntimo requested changes May 28, 2024

View reviewed changes

sauntimo marked this pull request as ready for review May 28, 2024 18:31

nkavtur reviewed May 29, 2024

View reviewed changes

Fix connections fetch limit

340edf0

nkavtur reviewed May 29, 2024

View reviewed changes

server/lib/multipartRequest.js Show resolved Hide resolved

nkavtur reviewed May 29, 2024

View reviewed changes

Use default perPage for connections

b623d9f

nkavtur approved these changes May 31, 2024

View reviewed changes

set CONNECTIONS_LIST_LIMIT to 20k

eee0a90

nkavtur approved these changes May 31, 2024

View reviewed changes

sauntimo self-requested a review June 3, 2024 10:09

sauntimo approved these changes Jun 3, 2024

View reviewed changes

ubenzer commented Jun 3, 2024

View reviewed changes

Don't fetch records if total > limit

04b29ac

sauntimo added 3 commits June 3, 2024 17:21

Use 0 connections as special case

9079d12

Restore correct connections limit

24c4d6c

Fix commemnt

40aaf62

Fix failing test

a568730

ubenzer commented Jun 4, 2024

View reviewed changes

client/containers/Users/Users.jsx Outdated Show resolved Hide resolved

server/routes/connections.js Outdated Show resolved Hide resolved

server/lib/multipartRequest.js Outdated Show resolved Hide resolved

ubenzer and others added 3 commits June 4, 2024 10:51

Improve the way create user button is handled. Set connection cutoff …

9b05ec7

…to its final value

Fix failing test

f853423

Fix test

de76206

sauntimo requested a review from nkavtur June 4, 2024 11:13

nkavtur approved these changes Jun 4, 2024

View reviewed changes

sauntimo merged commit 340b18c into auth0-extensions:master Jun 5, 2024
5 checks passed

ubenzer deleted the ids-4707 branch June 5, 2024 08:56

sauntimo mentioned this pull request Jun 6, 2024

Bump minor version to release connections handling changes #295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IDS-4707 - Switch from InputCombo to InputText if there are too many connections to list #292

IDS-4707 - Switch from InputCombo to InputText if there are too many connections to list #292

ubenzer commented May 24, 2024 •

edited

Loading

sauntimo left a comment •

edited

Loading

sauntimo May 28, 2024

sauntimo May 28, 2024

sauntimo May 29, 2024

sauntimo left a comment

sauntimo commented May 28, 2024 •

edited

Loading

nkavtur May 29, 2024

sauntimo May 29, 2024

nkavtur May 29, 2024

sauntimo May 30, 2024

sauntimo May 30, 2024

sauntimo left a comment

ubenzer Jun 3, 2024

sauntimo commented Jun 3, 2024 •

edited

Loading

sauntimo commented Jun 3, 2024

	label: 'Connection',
	label: isConnectionListingLimitExceeded ? 'Connection Name' : 'Connection',

		@@ -3,10 +3,12 @@ import { Router } from 'express';

		import multipartRequest from '../lib/multipartRequest';

		const CONNECTIONS_LIST_LIMIT = 50;

IDS-4707 - Switch from InputCombo to InputText if there are too many connections to list #292

IDS-4707 - Switch from InputCombo to InputText if there are too many connections to list #292

Conversation

ubenzer commented May 24, 2024 • edited Loading

✏️ Changes

📷 Screenshots

🔗 References

🎯 Testing

🚀 Deployment

🎡 Rollout

🔥 Rollback

📄 Procedure

sauntimo left a comment • 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

sauntimo left a comment

Choose a reason for hiding this comment

sauntimo commented May 28, 2024 • 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

sauntimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauntimo commented Jun 3, 2024 • edited Loading

Setup

With connections count greater than limit

Connection with require username enabled

Connection with require username disabled

With connections count less than limit

Connection with require username enabled

Connection with require username disabled

sauntimo commented Jun 3, 2024

ubenzer commented May 24, 2024 •

edited

Loading

sauntimo left a comment •

edited

Loading

sauntimo commented May 28, 2024 •

edited

Loading

sauntimo commented Jun 3, 2024 •

edited

Loading