Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add friendly name to list group members response #8413

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Add friendly name to list group members response #8413

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a3693a
Add friendly name to list group members response
idanovo Dec 11, 2024
2331a7b
Add friendly name to list group members response
idanovo Dec 11, 2024
8892f3c
Changed UI to display user friendly name when possible
idanovo Dec 12, 2024
9aa365d
Searching users in UI
idanovo Dec 16, 2024
dd0f625
Error handling
idanovo Dec 16, 2024
609d471
Review changes
idanovo Dec 17, 2024
6083e4b
Fix review comments
idanovo Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contrib/auth/acl/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ func (c *Controller) ListGroupMembers(w http.ResponseWriter, r *http.Request, gr
Username: u.Username,
CreationDate: u.CreatedAt.Unix(),
Email: u.Email,
FriendlyName: u.FriendlyName,
})
}
writeResponse(w, http.StatusOK, response)
Expand Down
1 change: 1 addition & 0 deletions pkg/api/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,6 +1112,7 @@ func (c *Controller) ListGroupMembers(w http.ResponseWriter, r *http.Request, gr
Id: u.Username,
Email: u.Email,
CreationDate: u.CreatedAt.Unix(),
FriendlyName: u.FriendlyName,
})
}
writeResponse(w, r, http.StatusOK, response)
Expand Down
7 changes: 4 additions & 3 deletions webui/src/lib/components/auth/forms.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@guy-har guy-har Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// for users
if (ent?.email?.length) return ent.email;
if (ent?.friendly_name?.length) return ent.friendly_name;
// for groups
if (ent?.name?.length) return ent.name;
return ent.id;
Expand Down Expand Up @@ -49,7 +50,7 @@ export const AttachModal = ({ show, searchFn, onAttach, onHide, addText = "Add",
onAdd={() => setSelected([...selected, ent])}
onRemove={() => setSelected(selected.filter(selectedEnt => selectedEnt.id !== ent.id))}
name={'selected'}/>,
<strong>{resolveEntityDisplayName(ent)}</strong>
<strong>{ResolveEntityDisplayName(ent)}</strong>
]}/>

<div className="mt-3">
Expand All @@ -58,7 +59,7 @@ export const AttachModal = ({ show, searchFn, onAttach, onHide, addText = "Add",
<strong>Selected: </strong>
{(selected.map(item => (
<Badge key={item.id} pill variant="primary" className="me-1">
{resolveEntityDisplayName(item)}
{ResolveEntityDisplayName(item)}
</Badge>
)))}
</p>
Expand Down
36 changes: 31 additions & 5 deletions webui/src/pages/auth/groups/group/members.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import Button from "react-bootstrap/Button";

import {GroupHeader} from "../../../../lib/components/auth/nav";
import {useAPIWithPagination} from "../../../../lib/hooks/api";
import {auth} from "../../../../lib/api";
import {auth, MAX_LISTING_AMOUNT} from "../../../../lib/api";
import {Paginator} from "../../../../lib/components/pagination";
import {AttachModal} from "../../../../lib/components/auth/forms";
import {AttachModal, ResolveEntityDisplayName} from "../../../../lib/components/auth/forms";
import {ConfirmationButton} from "../../../../lib/components/modals";
import {
ActionGroup,
Expand All @@ -26,15 +26,41 @@ const GroupMemberList = ({ groupId, after, onPaginate }) => {
const [refresh, setRefresh] = useState(false);
const [showAddModal, setShowAddModal] = useState(false);
const [attachError, setAttachError] = useState(null);

const [allUsers, setAllUsers] = useState([]);
const {results, loading, error, nextPage} = useAPIWithPagination(() => {
return auth.listGroupMembers(groupId, after);
}, [groupId, after, refresh]);

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

const setAllUsersFromLakeFS = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (allUsers.length > 0) {
return allUsers
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

after = ""
let hasMore = true
let usersList = []
try {
do {
const results = await auth.listUsers("", after, MAX_LISTING_AMOUNT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens after MAX_LISTING_AMOUNT?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out useAPIWithPagination I think it reduce a bit of code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

usersList = usersList.concat(results.results);
after = results.pagination.next_offset;
hasMore = results.pagination.has_more;
} while (hasMore);
usersList.sort((a, b) => ResolveEntityDisplayName(a).localeCompare(ResolveEntityDisplayName(b)));
setAllUsers(usersList);
return usersList;
} catch (error) {
console.error("Error fetching users:", error);
return [];
}
}
const searchUsers = async (prefix, maxResults) => {
let allUsersList = await setAllUsersFromLakeFS()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@idanovo idanovo Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let filteredUsers = allUsersList.filter(user => ResolveEntityDisplayName(user).startsWith(prefix));
return filteredUsers.slice(0, maxResults);
};
let content;
if (loading) content = <Loading/>;
else if (error) content= <AlertError error={error}/>;
Expand Down Expand Up @@ -75,7 +101,7 @@ const GroupMemberList = ({ groupId, after, onPaginate }) => {
filterPlaceholder={'Find User...'}
modalTitle={'Add to Group'}
addText={'Add to Group'}
searchFn={prefix => auth.listUsers(prefix, "", 5).then(res => res.results)}
searchFn={prefix => searchUsers(prefix, 5).then(res => res)}
onHide={() => setShowAddModal(false)}
onAttach={(selected) => {
Promise.all(selected.map(user => auth.addUserToGroup(user.id, groupId)))
Expand Down
Loading