Add friendly name to list group members response #8413

idanovo · 2024-12-11T10:00:31Z

No description provided.

github-actions · 2024-12-11T10:08:12Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-12-11T10:08:37Z

E2E Test Results - Quickstart

Isan-Rivkin

spoke offline, please test and ask re-review when done :)

itaigilo

@idanovo thanks for jumping into some UI stuff -
The code is very clear and easy to understand.

Blocking mainly for possible performance issues, since if we're indeed fetching all users, we should make sure we do it only when necessary, to prevent from extra loads.

itaigilo · 2024-12-17T10:28:22Z

webui/src/lib/components/auth/forms.jsx

@@ -9,9 +9,10 @@ import {SearchIcon} from "@primer/octicons-react";
 import {useAPI} from "../../hooks/api";
 import {Checkbox, DataTable, DebouncedFormControl, AlertError, Loading} from "../controls";

-const resolveEntityDisplayName = (ent) => {
+export const ResolveEntityDisplayName = (ent) => {


In our JS code (and in React in general), capitalize React components, while functions should be camelCased -
No matter if they are private or public.
(So this one should remain resolveEntityDisplayName.)

More important:
Now that this func is exported, it seemed to me that it should reside in some utils file.
Well, apparently, there's already utils.ts, containing a resolveDisplayName func 😄
Please unify these.

@itaigilo, I talked with Idan F2F, and mentioned that this resolver method should be provided by the caller, given the fact that this form isn't aware of the caller and that it already mixes between groups and users
WDYT

Not sure this is in the context of this PR...
We can chat about it f2f if you'd like to.

itaigilo · 2024-12-17T10:35:23Z

webui/src/pages/auth/groups/group/members.jsx

    useEffect(() => {
        setAttachError(null);
    }, [refresh]);

+    const setAllUsersFromLakeFS = async () => {


This is not only setting the users, but mainly fetching them.
This is important, since when I'm calling this function, it should be clear that it gets data from the API.

So please rename to AllUsersFromLakeFS, or something else that will make it clear that this is about API calls.

itaigilo · 2024-12-17T10:36:34Z

webui/src/pages/auth/groups/group/members.jsx

+        let usersList = []
+        try {
+            do {
+                const results = await auth.listUsers("", after, MAX_LISTING_AMOUNT);


What happens after MAX_LISTING_AMOUNT?

Shouldn't we detect this scenario, and at least warn the user in some way?

I run do-while in order to list all the users with pagination

itaigilo · 2024-12-17T10:42:08Z

webui/src/pages/auth/groups/group/members.jsx

+        }
+    }
+    const searchUsers = async (prefix, maxResults) => {
+        let allUsersList = await setAllUsersFromLakeFS()


If I get it right, it fetches all the search results from the API for every prefix change? No caching?
What will happen (performance-wise) in case of 1000 users, for example?

No, only on the first time we load the "add member" pop-up
I load all the objects to the allUsers which is part of the state of this page
BTW, I've tested my PR with 10K users org and it ran pretty fast

itaigilo · 2024-12-17T10:49:41Z

And if I'm taking this up one level, I'm not sure that fetching all the users in the client is the best approach here.
Another option is to change the API to support different sorting for the List Users endpoint, and sort it by friendly_name in the server-side. This should both reduce the load on the clients, and add new capabilities to our API.

guy-har · 2024-12-17T11:07:43Z

webui/src/pages/auth/groups/group/members.jsx

+        try {
+            do {
+                const results = await auth.listUsers("", after, MAX_LISTING_AMOUNT);


Check out useAPIWithPagination I think it reduce a bit of code

I don't think it's the same case.
Here, I need to load all the users, not just a specific page.
The useAPIWithPagination load each time a different page to the state which is not what I need

guy-har · 2024-12-17T11:08:49Z

webui/src/pages/auth/groups/group/members.jsx

+        if (allUsers.length > 0) {
+            return allUsers
+        }


@itaigilo, isn't there some better way to force only one load, with useEffect or something?

Probably -
But I think that if some caching will be added here, it'll change the flow. Not sure though.

idanovo · 2024-12-17T13:23:45Z

And if I'm taking this up one level, I'm not sure that fetching all the users in the client is the best approach here. Another option is to change the API to support different sorting for the List Users endpoint, and sort it by friendly_name in the server-side. This should both reduce the load on the clients, and add new capabilities to our API.

@itaigilo We do it intentionally on the UI, the alternative for this was to change listUsers to not list users by ID (which is the key we use to save the users in the the KV).
This change will make all the listUsers calls to run slower.

itaigilo

Looking good 😄

Approving to unblock, but please check out my code-style comments.

itaigilo · 2024-12-17T15:30:54Z

webui/src/lib/components/auth/forms.jsx

-
-export const AttachModal = ({ show, searchFn, onAttach, onHide, addText = "Add",
+
+export const AttachModal = ({ show, searchFn, onAttach, onHide, resolveEntityFN = (ent => ent.id), addText = "Add",


Nit:
Should be resolveEntityFN (since we have searchFn).
I would also put but Fn params next to each other.

itaigilo · 2024-12-17T15:35:05Z

webui/src/pages/auth/groups/group/members.jsx

    useEffect(() => {
        setAttachError(null);
    }, [refresh]);

+    const allUsersFromLakeFS = async (resolveUserDisplayNameFN = (user => user.id)) => {


I think this is too much business logic to be in this file -
Well might be useful somewhere else.

How about extracting this?
Even into a file of its own, under components/auth, for example?

itaigilo · 2024-12-17T15:35:46Z

webui/src/pages/auth/users/user/groups.jsx

@@ -21,6 +21,12 @@ import { ConfirmationButton } from "../../../../lib/components/modals";
 import { useRouter } from "../../../../lib/hooks/router";
 import { Link } from "../../../../lib/components/nav";

+const resolveGroupDisplayName = (group) => {


Add friendly name to list group members response

2a3693a

idanovo added include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached labels Dec 11, 2024

idanovo requested a review from Isan-Rivkin December 11, 2024 10:00

idanovo self-assigned this Dec 11, 2024

Add friendly name to list group members response

2331a7b

Isan-Rivkin requested changes Dec 11, 2024

View reviewed changes

idanovo requested a review from Isan-Rivkin December 11, 2024 15:32

Isan-Rivkin approved these changes Dec 11, 2024

View reviewed changes

Changed UI to display user friendly name when possible

8892f3c

idanovo requested a review from Isan-Rivkin December 12, 2024 11:00

Searching users in UI

9aa365d

itaigilo mentioned this pull request Dec 16, 2024

Improve WebUI users sort #8421

Closed

Error handling

dd0f625

idanovo requested review from guy-har and itaigilo and removed request for Isan-Rivkin December 17, 2024 09:03

itaigilo requested changes Dec 17, 2024

View reviewed changes

guy-har reviewed Dec 17, 2024

View reviewed changes

Review changes

609d471

idanovo requested review from itaigilo and guy-har December 17, 2024 12:38

itaigilo approved these changes Dec 17, 2024

View reviewed changes

Fix review comments

6083e4b

idanovo merged commit e74938c into master Dec 17, 2024
39 checks passed

idanovo deleted the add-friendlyname-to-group-members-resp branch December 17, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add friendly name to list group members response #8413

Add friendly name to list group members response #8413

idanovo commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 •

edited

Loading

Isan-Rivkin left a comment

itaigilo left a comment

itaigilo Dec 17, 2024

itaigilo Dec 17, 2024

guy-har Dec 17, 2024 •

edited

Loading

itaigilo Dec 17, 2024

itaigilo Dec 17, 2024

itaigilo Dec 17, 2024

idanovo Dec 17, 2024

itaigilo Dec 17, 2024

idanovo Dec 17, 2024 •

edited

Loading

itaigilo commented Dec 17, 2024

guy-har Dec 17, 2024

idanovo Dec 17, 2024

guy-har Dec 17, 2024

itaigilo Dec 17, 2024

idanovo commented Dec 17, 2024

itaigilo left a comment

itaigilo Dec 17, 2024

itaigilo Dec 17, 2024

itaigilo Dec 17, 2024


		export const AttachModal = ({ show, searchFn, onAttach, onHide, addText = "Add",

		export const AttachModal = ({ show, searchFn, onAttach, onHide, resolveEntityFN = (ent => ent.id), addText = "Add",

Add friendly name to list group members response #8413

Add friendly name to list group members response #8413

Conversation

idanovo commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Dec 11, 2024 • edited Loading

E2E Test Results - Quickstart

Isan-Rivkin left a comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har Dec 17, 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

Choose a reason for hiding this comment

idanovo Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

itaigilo commented Dec 17, 2024

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

idanovo commented Dec 17, 2024

itaigilo 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

github-actions bot commented Dec 11, 2024 •

edited

Loading

guy-har Dec 17, 2024 •

edited

Loading

idanovo Dec 17, 2024 •

edited

Loading