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

Fix: Group Tables/Pagination Fixes and Improvements #2610

Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion backend/src/ee/routes/v1/group-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ export const registerGroupRouter = async (server: FastifyZodProvider) => {
querystring: z.object({
offset: z.coerce.number().min(0).max(100).default(0).describe(GROUPS.LIST_USERS.offset),
limit: z.coerce.number().min(1).max(100).default(10).describe(GROUPS.LIST_USERS.limit),
username: z.string().optional().describe(GROUPS.LIST_USERS.username)
username: z.string().trim().optional().describe(GROUPS.LIST_USERS.username),
search: z.string().trim().optional().describe(GROUPS.LIST_USERS.search)
}),
response: {
200: z.object({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ export const accessApprovalPolicyServiceFactory = ({
const verifyAllApprovers = [...approverUserIds];

for (const groupId of groupApprovers) {
usersPromises.push(groupDAL.findAllGroupPossibleMembers({ orgId: actorOrgId, groupId, offset: 0 }));
usersPromises.push(
groupDAL.findAllGroupPossibleMembers({ orgId: actorOrgId, groupId, offset: 0 }).then((group) => group.members)
);
}
const verifyGroupApprovers = (await Promise.all(usersPromises))
.flat()
Expand Down Expand Up @@ -327,7 +329,11 @@ export const accessApprovalPolicyServiceFactory = ({
>[] = [];

for (const groupId of groupApprovers) {
usersPromises.push(groupDAL.findAllGroupPossibleMembers({ orgId: actorOrgId, groupId, offset: 0 }));
usersPromises.push(
groupDAL
.findAllGroupPossibleMembers({ orgId: actorOrgId, groupId, offset: 0 })
.then((group) => group.members)
);
}
const verifyGroupApprovers = (await Promise.all(usersPromises))
.flat()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,12 @@ export const accessApprovalRequestServiceFactory = ({
const groupUsers = (
await Promise.all(
approverGroupIds.map((groupApproverId) =>
groupDAL.findAllGroupPossibleMembers({
orgId: actorOrgId,
groupId: groupApproverId
})
groupDAL
.findAllGroupPossibleMembers({
orgId: actorOrgId,
groupId: groupApproverId
})
.then((group) => group.members)
)
)
).flat();
Expand Down
44 changes: 27 additions & 17 deletions backend/src/ee/services/group/group-dal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,18 @@ export const groupDALFactory = (db: TDbClient) => {
groupId,
offset = 0,
limit,
username
username, // depreciated in favor of search
search
}: {
orgId: string;
groupId: string;
offset?: number;
limit?: number;
username?: string;
search?: string;
}) => {
try {
let query = db
const query = db
.replicaNode()(TableName.OrgMembership)
.where(`${TableName.OrgMembership}.orgId`, orgId)
.join(TableName.Users, `${TableName.OrgMembership}.userId`, `${TableName.Users}.id`)
Expand All @@ -92,31 +94,39 @@ export const groupDALFactory = (db: TDbClient) => {
db.ref("username").withSchema(TableName.Users),
db.ref("firstName").withSchema(TableName.Users),
db.ref("lastName").withSchema(TableName.Users),
db.ref("id").withSchema(TableName.Users).as("userId")
db.ref("id").withSchema(TableName.Users).as("userId"),
db.raw(`count(*) OVER() as total_count`)
)
.where({ isGhost: false })
.offset(offset);
.offset(offset)
.orderBy("firstName", "asc");

if (limit) {
query = query.limit(limit);
void query.limit(limit);
}

if (username) {
query = query.andWhere(`${TableName.Users}.username`, "ilike", `%${username}%`);
if (search) {
void query.andWhereRaw(`CONCAT_WS(' ', "firstName", "lastName", "username") ilike '%${search}%'`);
} else if (username) {
void query.andWhere(`${TableName.Users}.username`, "ilike", `%${username}%`);
}

const members = await query;

return members.map(
({ email, username: memberUsername, firstName, lastName, userId, groupId: memberGroupId }) => ({
id: userId,
email,
username: memberUsername,
firstName,
lastName,
isPartOfGroup: !!memberGroupId
})
);
return {
members: members.map(
({ email, username: memberUsername, firstName, lastName, userId, groupId: memberGroupId }) => ({
id: userId,
email,
username: memberUsername,
firstName,
lastName,
isPartOfGroup: !!memberGroupId
})
),
// @ts-expect-error col select is raw and not strongly typed
totalCount: Number(members?.[0]?.total_count ?? 0)
};
} catch (error) {
throw new DatabaseError({ error, name: "Find all org members" });
}
Expand Down
12 changes: 6 additions & 6 deletions backend/src/ee/services/group/group-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ export const groupServiceFactory = ({
actor,
actorId,
actorAuthMethod,
actorOrgId
actorOrgId,
search
}: TListGroupUsersDTO) => {
if (!actorOrgId) throw new UnauthorizedError({ message: "No organization ID provided in request" });

Expand All @@ -244,17 +245,16 @@ export const groupServiceFactory = ({
message: `Failed to find group with ID ${id}`
});

const users = await groupDAL.findAllGroupPossibleMembers({
const { members, totalCount } = await groupDAL.findAllGroupPossibleMembers({
orgId: group.orgId,
groupId: group.id,
offset,
limit,
username
username,
search
});

const count = await orgDAL.countAllOrgMembers(group.orgId);

return { users, totalCount: count };
return { users: members, totalCount };
};

const addUserToGroup = async ({ id, username, actor, actorId, actorAuthMethod, actorOrgId }: TAddUserToGroupDTO) => {
Expand Down
1 change: 1 addition & 0 deletions backend/src/ee/services/group/group-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export type TListGroupUsersDTO = {
offset: number;
limit: number;
username?: string;
search?: string;
} & TGenericPermission;

export type TAddUserToGroupDTO = {
Expand Down
10 changes: 6 additions & 4 deletions backend/src/ee/services/scim/scim-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -834,10 +834,12 @@ export const scimServiceFactory = ({
});
}

const users = await groupDAL.findAllGroupPossibleMembers({
orgId: group.orgId,
groupId: group.id
});
const users = await groupDAL
.findAllGroupPossibleMembers({
orgId: group.orgId,
groupId: group.id
})
.then((g) => g.members);

const orgMemberships = await orgDAL.findMembership({
[`${TableName.OrgMembership}.orgId` as "orgId"]: orgId,
Expand Down
3 changes: 2 additions & 1 deletion backend/src/lib/api-docs/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ export const GROUPS = {
id: "The id of the group to list users for",
offset: "The offset to start from. If you enter 10, it will start from the 10th user.",
limit: "The number of users to return.",
username: "The username to search for."
username: "The username to search for.",
search: "The text string that user email or name will be filtered by."
},
ADD_USER: {
id: "The id of the group to add the user to.",
Expand Down
15 changes: 8 additions & 7 deletions frontend/src/hooks/api/groups/queries.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ export const groupKeys = {
slug,
offset,
limit,
username
search
}: {
slug: string;
offset: number;
limit: number;
username: string;
}) => [...groupKeys.forGroupUserMemberships(slug), { offset, limit, username }] as const
search: string;
}) => [...groupKeys.forGroupUserMemberships(slug), { offset, limit, search }] as const
};

type TUser = {
Expand All @@ -33,27 +33,28 @@ export const useListGroupUsers = ({
groupSlug,
offset = 0,
limit = 10,
username
search
}: {
id: string;
groupSlug: string;
offset: number;
limit: number;
username: string;
search: string;
}) => {
return useQuery({
queryKey: groupKeys.specificGroupUserMemberships({
slug: groupSlug,
offset,
limit,
username
search
}),
enabled: Boolean(groupSlug),
keepPreviousData: true,
queryFn: async () => {
const params = new URLSearchParams({
offset: String(offset),
limit: String(limit),
username
search
});

const { data } = await apiRequest.get<{ users: TUser[]; totalCount: number }>(
Expand Down
1 change: 1 addition & 0 deletions frontend/src/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export { useLeaveConfirm } from "./useLeaveConfirm";
export { usePagination } from "./usePagination";
export { usePersistentState } from "./usePersistentState";
export { usePopUp } from "./usePopUp";
export { useResetPageHelper } from "./useResetPageHelper";
export { useSyntaxHighlight } from "./useSyntaxHighlight";
export { useTimedReset } from "./useTimedReset";
export { useToggle } from "./useToggle";
16 changes: 16 additions & 0 deletions frontend/src/hooks/useResetPageHelper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Dispatch, SetStateAction, useEffect } from "react";

export const useResetPageHelper = ({
totalCount,
offset,
setPage
}: {
totalCount: number;
offset: number;
setPage: Dispatch<SetStateAction<number>>;
}) => {
useEffect(() => {
// reset page if no longer valid
if (totalCount <= offset) setPage(1);
}, [totalCount]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
Tr
} from "@app/components/v2";
import { OrgPermissionActions, OrgPermissionSubjects } from "@app/context";
import { useDebounce, useResetPageHelper } from "@app/hooks";
import { useAddUserToGroup, useListGroupUsers, useRemoveUserFromGroup } from "@app/hooks/api";
import { UsePopUpState } from "@app/hooks/usePopUp";

Expand All @@ -33,18 +34,28 @@ export const OrgGroupMembersModal = ({ popUp, handlePopUpToggle }: Props) => {
const [page, setPage] = useState(1);
const [perPage, setPerPage] = useState(10);
const [searchMemberFilter, setSearchMemberFilter] = useState("");
const [debouncedSearch] = useDebounce(searchMemberFilter);

const popUpData = popUp?.groupMembers?.data as {
groupId: string;
slug: string;
};

const offset = (page - 1) * perPage;
const { data, isLoading } = useListGroupUsers({
id: popUpData?.groupId,
groupSlug: popUpData?.slug,
offset: (page - 1) * perPage,
offset,
limit: perPage,
username: searchMemberFilter
search: debouncedSearch
});

const { totalCount = 0 } = data ?? {};

useResetPageHelper({
totalCount,
offset,
setPage
});

const { mutateAsync: assignMutateAsync } = useAddUserToGroup();
Expand Down Expand Up @@ -140,17 +151,20 @@ export const OrgGroupMembersModal = ({ popUp, handlePopUpToggle }: Props) => {
})}
</TBody>
</Table>
{!isLoading && data?.totalCount !== undefined && (
{!isLoading && totalCount > 0 && (
<Pagination
count={data.totalCount}
count={totalCount}
page={page}
perPage={perPage}
onChangePage={(newPage) => setPage(newPage)}
onChangePerPage={(newPerPage) => setPerPage(newPerPage)}
/>
)}
{!isLoading && !data?.users?.length && (
<EmptyState title="No users found" icon={faUsers} />
<EmptyState
title={debouncedSearch ? "No users match search" : "No users found"}
icon={faUsers}
/>
)}
</TableContainer>
</ModalContent>
Expand Down
Loading
Loading