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.

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
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
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
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
15 changes: 4 additions & 11 deletions webui/src/lib/components/auth/forms.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,8 @@ import {SearchIcon} from "@primer/octicons-react";
import {useAPI} from "../../hooks/api";
import {Checkbox, DataTable, DebouncedFormControl, AlertError, Loading} from "../controls";

const resolveEntityDisplayName = (ent) => {
// for users
if (ent?.email?.length) return ent.email;
// for groups
if (ent?.name?.length) return ent.name;
return ent.id;
}

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

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

Choose a reason for hiding this comment

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

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

emptyState = 'No matches', modalTitle = 'Add', headers = ['Select', 'ID'],
filterPlaceholder = 'Filter...'}) => {
const search = useRef(null);
Expand Down Expand Up @@ -49,7 +42,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>{resolveEntityFN(ent)}</strong>
]}/>

<div className="mt-3">
Expand All @@ -58,7 +51,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)}
{resolveEntityFN(item)}
</Badge>
)))}
</p>
Expand Down
2 changes: 1 addition & 1 deletion webui/src/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ interface User {
friendly_name: string;
}

export const resolveDisplayName = (user: User): string => {
export const resolveUserDisplayName = (user: User): string => {
if (!user) return "";
if (user?.email?.length) return user.email;
if (user?.friendly_name?.length) return user.friendly_name;
Expand Down
4 changes: 2 additions & 2 deletions webui/src/pages/auth/credentials.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {auth} from "../../lib/api";
import {useState} from "react";
import {CredentialsShowModal, CredentialsTable} from "../../lib/components/auth/credentials";
import {useRouter} from "../../lib/hooks/router";
import {resolveDisplayName} from "../../lib/utils";
import {resolveUserDisplayName} from "../../lib/utils";

const CredentialsContainer = () => {
const router = useRouter();
Expand Down Expand Up @@ -40,7 +40,7 @@ const CredentialsContainer = () => {
<ConfirmationButton
variant="success"
modalVariant="success"
msg={<span>Create a new Access Key for user <strong>{resolveDisplayName(user)}</strong>?</span>}
msg={<span>Create a new Access Key for user <strong>{resolveUserDisplayName(user)}</strong>?</span>}
onConfirm={hide => {
createKey()
.then(key => { setCreatedKey(key) })
Expand Down
41 changes: 34 additions & 7 deletions webui/src/pages/auth/groups/group/members.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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 {ConfirmationButton} from "../../../../lib/components/modals";
Expand All @@ -19,22 +19,48 @@ import {
} from "../../../../lib/components/controls";
import {useRouter} from "../../../../lib/hooks/router";
import {Link} from "../../../../lib/components/nav";
import {resolveDisplayName} from "../../../../lib/utils";
import {resolveUserDisplayName} from "../../../../lib/utils";


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 allUsersFromLakeFS = async (resolveUserDisplayNameFN = (user => user.id)) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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) => resolveUserDisplayNameFN(a).localeCompare(resolveUserDisplayNameFN(b)));
setAllUsers(usersList);
return usersList;
} catch (error) {
console.error("Error fetching users:", error);
return [];
}
}
const searchUsers = async (prefix, maxResults, resolveUserDisplayNameFN = (user => user.id)) => {
let allUsersList = await allUsersFromLakeFS(resolveUserDisplayNameFN)
let filteredUsers = allUsersList.filter(user => resolveUserDisplayNameFN(user).startsWith(prefix));
return filteredUsers.slice(0, maxResults);
};
let content;
if (loading) content = <Loading/>;
else if (error) content= <AlertError error={error}/>;
Expand All @@ -45,7 +71,7 @@ const GroupMemberList = ({ groupId, after, onPaginate }) => {
<DataTable
keyFn={user => user.id}
rowFn={user => [
<Link href={{pathname: '/auth/users/:userId', params: {userId: user.id}}}>{resolveDisplayName(user)}</Link>,
<Link href={{pathname: '/auth/users/:userId', params: {userId: user.id}}}>{resolveUserDisplayName(user)}</Link>,
<FormattedDate dateValue={user.creation_date}/>
]}
headers={['User ID', 'Created At']}
Expand All @@ -54,7 +80,7 @@ const GroupMemberList = ({ groupId, after, onPaginate }) => {
buttonFn: user => <ConfirmationButton
size="sm"
variant="outline-danger"
msg={<span>Are you sure you{'\''}d like to remove user <strong>{resolveDisplayName(user)}</strong> from group <strong>{groupId}</strong>?</span>}
msg={<span>Are you sure you{'\''}d like to remove user <strong>{resolveUserDisplayName(user)}</strong> from group <strong>{groupId}</strong>?</span>}
onConfirm={() => {
auth.removeUserFromGroup(user.id, groupId)
.catch(error => alert(error))
Expand All @@ -75,7 +101,8 @@ 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)}
resolveEntityFN={resolveUserDisplayName}
searchFn={prefix => searchUsers(prefix, 5, resolveUserDisplayName).then(res => res)}
onHide={() => setShowAddModal(false)}
onAttach={(selected) => {
Promise.all(selected.map(user => auth.addUserToGroup(user.id, groupId)))
Expand Down
4 changes: 2 additions & 2 deletions webui/src/pages/auth/users/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
} from "../../../lib/components/controls";
import validator from "validator/es";
import { disallowPercentSign, INVALID_USER_NAME_ERROR_MESSAGE } from "../validation";
import { resolveDisplayName } from "../../../lib/utils";
import { resolveUserDisplayName } from "../../../lib/utils";

const USER_NOT_FOUND = "unknown";
export const GetUserEmailByIdContext = createContext();
Expand Down Expand Up @@ -119,7 +119,7 @@ const UsersContainer = ({nextPage, refresh, setRefresh, error, loading, userList
onRemove={() => setSelected(selected.filter(u => u !== user))}
/>,
<Link href={{pathname: '/auth/users/:userId', params: {userId: user.id}}}>
{ resolveDisplayName(user) }
{ resolveUserDisplayName(user) }
</Link>,
<FormattedDate dateValue={user.creation_date}/>
]}/>
Expand Down
10 changes: 9 additions & 1 deletion webui/src/pages/auth/users/user/groups.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

if(!group) return "";
if (group?.name?.length) return group.name;
return group.id;
}

const UserGroupsList = ({ userId, after, onPaginate }) => {
const [refresh, setRefresh] = useState(false);
const [showAddModal, setShowAddModal] = useState(false);
Expand Down Expand Up @@ -97,6 +103,7 @@ const UserGroupsList = ({ userId, after, onPaginate }) => {
searchFn={(prefix) =>
auth.listGroups(prefix, "", 5).then((res) => res.results)
}
resolveEntityFN={resolveGroupDisplayName}
onHide={() => setShowAddModal(false)}
onAttach={(selected) => {
Promise.all(
Expand All @@ -112,7 +119,8 @@ const UserGroupsList = ({ userId, after, onPaginate }) => {
.finally(() => {
setShowAddModal(false);
});
}}
}
}
/>
)}
</>
Expand Down
Loading