feat: Organization users management #575
Conversation
WalkthroughThis pull request introduces several components and functionalities for managing organization members within a React application and its backend. Key changes include the addition of a Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
engine
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (19)
tracecat/organization/models.py (2)
9-17: Consider adding docstrings to theOrgMemberReadclass.Adding docstrings will improve code readability and help other developers understand the purpose and usage of this model.
23-25: Add docstrings to theOrgReadclass for better documentation.Including a brief description of the
OrgReadmodel enhances maintainability and provides clarity on its role within the application.frontend/src/lib/auth.ts (1)
22-27: Use an enumeration or constants for user roles for type safety.Currently, the role comparison uses a string literal
"admin". Defining an enum or constants for user roles can prevent typos and enhance code maintainability.Apply this change:
// Define a UserRole enum export enum UserRole { Admin = "admin", User = "user", // Add other roles as needed } // Update the function export function userIsPrivileged(user: UserRead | null): boolean { if (!user) { return false } return user.is_superuser || user.role === UserRole.Admin }frontend/src/app/organization/members/page.tsx (3)
18-18: Remove emptydivto clean up the markupThere is an empty
divwith class names but no content. If it's not required for layout purposes, consider removing it to simplify the code.Apply this diff to remove the unnecessary
div:- <div className="ml-auto flex items-center space-x-2"></div>
21-24: Remove unnecessary React fragmentThe fragment
<>...</>is unnecessary here since the elements are already wrapped within a parentdiv. Removing it can simplify the code.Apply this diff to remove the fragment:
- <> <h6 className="text-sm font-semibold">Manage Members</h6> <OrgMembersTable /> - </>
22-22: Use appropriate heading levels for accessibilityConsider using a heading level that follows the hierarchical structure. Since the main heading is
h2, usingh3here would be more appropriate thanh6.Apply this diff to adjust the heading level:
- <h6 className="text-sm font-semibold">Manage Members</h6> + <h3 className="text-sm font-semibold">Manage Members</h3>tracecat/organization/router.py (2)
16-21: Implement or remove theget_organizationendpointThe
get_organizationendpoint currently raises aNotImplementedError. If this endpoint is intended to provide organization details, consider implementing it. Otherwise, remove it to avoid exposing an unimplemented endpoint.Would you like assistance in implementing this endpoint or opening a GitHub issue to track this task?
65-74: Refactor duplicated exception handling into a decoratorThe exception handling for
NoResultFoundandTracecatAuthorizationErroris duplicated in bothdelete_org_memberandupdate_org_memberendpoints. Refactoring this into a decorator can reduce code duplication and improve maintainability.Apply this diff to create a
handle_member_exceptionsdecorator:+def handle_member_exceptions(func): + async def wrapper(*args, **kwargs): + try: + return await func(*args, **kwargs) + except NoResultFound as e: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="User not found" + ) from e + except TracecatAuthorizationError as e: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, detail="Forbidden" + ) from e + return wrapper @router.delete("/members/{user_id}", status_code=status.HTTP_204_NO_CONTENT) +@handle_member_exceptions async def delete_org_member( *, role: Role = RoleACL( allow_user=True, allow_service=False, require_workspace="no", min_access_level=AccessLevel.ADMIN, ), session: AsyncDBSession, user_id: UserID, ): service = OrgService(session, role=role) await service.delete_member(user_id) @router.patch("/members/{user_id}", response_model=OrgMemberRead) +@handle_member_exceptions async def update_org_member( *, role: Role = RoleACL( allow_user=True, allow_service=False, require_workspace="no", min_access_level=AccessLevel.ADMIN, ), session: AsyncDBSession, user_id: UserID, params: UserUpdate, ): service = OrgService(session, role=role) user = await service.update_member(user_id, params) return OrgMemberRead( user_id=user.id, first_name=user.first_name, last_name=user.last_name, email=user.email, role=user.role, is_active=user.is_active, is_superuser=user.is_superuser, is_verified=user.is_verified, )Also applies to: 103-110
tracecat/organization/service.py (1)
84-89: Extract superuser check to a helper method to reduce duplicationThe check for
user.is_superuseris duplicated in bothdelete_memberandupdate_membermethods. Consider extracting this logic into a private helper method to improve code readability and maintainability.Apply this diff to implement the change:
+ async def _ensure_not_superuser(self, user: User) -> None: + if user.is_superuser: + raise TracecatAuthorizationError("Cannot modify superuser") @require_access_level(AccessLevel.ADMIN) async def delete_member(self, user_id: UserID) -> None: user = await self.get_member(user_id) - if user.is_superuser: - raise TracecatAuthorizationError("Cannot delete superuser") + await self._ensure_not_superuser(user) async with self._manager() as user_manager: await user_manager.delete(user) @require_access_level(AccessLevel.ADMIN) async def update_member(self, user_id: UserID, params: UserUpdate) -> User: user = await self.get_member(user_id) - if user.is_superuser: - raise TracecatAuthorizationError("Cannot update superuser") + await self._ensure_not_superuser(user) async with self._manager() as user_manager: updated_user = await user_manager.update( user_update=params, user=user, safe=True ) return updated_userAlso applies to: 108-115
frontend/src/components/table/table.tsx (1)
53-53: Ensure consistent naming forinitialSortingStatepropThe prop is defined as
initialSortingState, but it's destructured and renamed toinitialSortingin the function parameters. For clarity and consistency, consider using the same name throughout.Apply this diff to align the naming:
export function DataTable<TData, TValue>({ columns, data, onClickRow, toolbarProps, tableHeaderAuxOptions, isLoading, error, emptyMessage, errorMessage, showSelectedRows = false, - initialSortingState: initialSorting = [], + initialSortingState = [], }: DataTableProps<TData, TValue>) { // ... - const [sorting, setSorting] = React.useState<SortingState>(initialSorting) + const [sorting, setSorting] = React.useState<SortingState>(initialSortingState)Also applies to: 67-67
frontend/src/components/organization/org-members-table.tsx (5)
71-71: Remove debuggingconsolestatements from production codeThe
console.logandconsole.debugstatements used for debugging should be removed or replaced with proper logging mechanisms before deploying to production. This helps maintain clean code and prevents unintended exposure of sensitive information.Apply this diff:
- console.log("Changing role", selectedMember, role) - console.log("Removing member", selectedMember) - console.debug("Selected user", row.original)Also applies to: 87-87, 262-262
77-79: Provide user feedback on role change failureCurrently, if an error occurs during the role change, the user is not notified. Consider adding a toast notification to inform the user about the failure, enhancing the user experience.
Example:
} catch (error) { console.error("Failed to change role", error) + toast({ + title: "Error changing role", + description: "An error occurred while changing the user's role.", + variant: "destructive", + }) } finally {
92-94: Notify user on member removal failureIf an error occurs while removing a member, the user is not informed. Adding a toast notification will provide immediate feedback to the user about the failure.
Example:
} catch (error) { console.error("Failed to remove member", error) + toast({ + title: "Error removing member", + description: "An error occurred while removing the member.", + variant: "destructive", + }) } finally {
265-265: Update action label to "Remove from organization" for consistencyThe action label is currently "Remove from workspace," which may cause confusion since this component manages organization members. Updating it to "Remove from organization" aligns with the context and improves clarity.
Apply this diff:
- Remove from workspace + Remove from organization
315-315: InitializenewRolestate with the selected user's current roleWhen opening the change role dialog, the default selected role is always set to "basic," which may not reflect the user's actual current role. Initializing
newRolewith the selected user's current role enhances usability.Apply this diff:
- const [newRole, setNewRole] = useState<UserRole>("basic") + const [newRole, setNewRole] = useState<UserRole>( + selectedUser?.role ?? "basic" + )frontend/src/lib/hooks.tsx (2)
1070-1073: Expose loading and error states fromuseOrgMembershookThe
useOrgMembershook currently returns onlyorgMembers. To handle loading and error states effectively in components that use this hook, consider exposingisLoadinganderrorfrom theuseQuery.Update the code as follows:
-export function useOrgMembers() { +export function useOrgMembers(): { + orgMembers?: OrgMemberRead[] + isLoading: boolean + error: unknown + // other properties +} { const queryClient = useQueryClient() - const { data: orgMembers } = useQuery<OrgMemberRead[]>({ + const { + data: orgMembers, + isLoading, + error, + } = useQuery<OrgMemberRead[]>({ queryKey: ["org-members"], queryFn: async () => await organizationListOrgMembers(), })And return the new variables:
return { orgMembers, + isLoading, + error, updateOrgMember, updateOrgMemberIsPending, updateOrgMemberError,
1090-1105: Refactor error handling to reduce code duplicationThe
onErrorhandlers in bothupdateOrgMemberanddeleteOrgMembermutations have similar structures. Extracting the common error handling logic into a shared function enhances maintainability and reduces repetition.Example refactor:
+function handleOrgMemberError( + error: TracecatApiError, + action: "update" | "delete" +) { + const apiError = error + switch (apiError.status) { + case 403: + toast({ + title: "You cannot perform this action", + description: `${apiError.message}: ${apiError.body.detail}`, + }) + break + default: + toast({ + title: `Failed to ${action} organization member`, + description: `An unexpected error occurred while trying to ${action} the organization member. ${apiError.message}: ${apiError.body.detail}`, + variant: "destructive", + }) + } +} // In updateOrgMember mutation onError: (error: TracecatApiError) => { - const apiError = error as TracecatApiError - switch (apiError.status) { - case 403: - toast({ - title: "You cannot perform this action", - description: `${apiError.message}: ${apiError.body.detail}`, - }) - break - default: - toast({ - title: "Failed to update organization member", - description: `An unexpected error occurred while updating the organization member. ${apiError.message}: ${apiError.body.detail}`, - }) - } + handleOrgMemberError(error, "update") }, // In deleteOrgMember mutation onError: (error: TracecatApiError) => { - const apiError = error as TracecatApiError - switch (apiError.status) { - case 403: - toast({ - title: "You cannot perform this action", - description: `${apiError.message}: ${apiError.body.detail}`, - }) - break - default: - toast({ - title: "Failed to delete organization member", - description: `An unexpected error occurred while deleting the organization member. ${apiError.message}: ${apiError.body.detail}`, - }) - } + handleOrgMemberError(error, "delete") },Also applies to: 1122-1137
frontend/src/client/services.gen.ts (2)
Line range hint
1685-1685: Replace 'void' with 'undefined' in type declarationUsing
voidoutside of return types or parameters can be confusing in TypeScript. Consider usingundefinedinstead.Apply this diff to fix the issue:
-export type OrganizationDeleteOrgMemberResponse = void; +export type OrganizationDeleteOrgMemberResponse = undefined;
Line range hint
2654-2654: Replace 'void' with 'undefined' in response typeUsing
voidoutside of return types or parameters can be confusing in TypeScript. Replacevoidwithundefinedto correctly represent the absence of a value.Apply this diff to fix the issue:
- 204: void; + 204: undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
frontend/src/app/organization/members/page.tsx(1 hunks)frontend/src/app/organization/sidebar-nav.tsx(4 hunks)frontend/src/client/schemas.gen.ts(1 hunks)frontend/src/client/services.gen.ts(2 hunks)frontend/src/client/types.gen.ts(3 hunks)frontend/src/components/organization/org-members-table.tsx(1 hunks)frontend/src/components/table/table.tsx(2 hunks)frontend/src/lib/auth.ts(1 hunks)frontend/src/lib/hooks.tsx(2 hunks)tracecat/api/app.py(3 hunks)tracecat/auth/router.py(2 hunks)tracecat/organization/models.py(1 hunks)tracecat/organization/router.py(1 hunks)tracecat/organization/service.py(1 hunks)tracecat/service.py(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/client/types.gen.ts
[error] 1685-1685: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 2654-2654: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (7)
tracecat/service.py (1)
3-3: Ensure compatibility of Self type annotation with Python version.
The Self type hint is available in Python 3.11 and later. If the project supports earlier Python versions, consider importing Self from typing_extensions or adjusting the type annotations to maintain compatibility.
frontend/src/app/organization/sidebar-nav.tsx (1)
Line range hint 82-83: Review active link highlighting logic
The current logic for highlighting the active navigation link may not work as intended for nested routes or dynamic segments. Consider revising the condition to ensure accurate highlighting.
Run the following script to check how the active link is determined:
✅ Verification successful
The active link highlighting logic is implemented correctly
The current implementation is appropriate for the navigation structure used in this application. Here's why:
- The navigation items are flat (not nested), with simple routes like
/organization/credentialsand/organization/members - The
leafRouteextraction usingpathname.split("/").pop()correctly matches the last segment of the URL - The fallback to
defaultRoutehandles the root organization path - The highlighting is applied using the
endsWithcheck which works well for this flat route structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the active link highlighting in the sidebar navigation.
# Test: Find all instances where `item.href` is compared to `leafRoute`. Expect: Correct handling of routes.
rg 'item\.href\.endsWith\(leafRoute.*\)' frontend/src/app/organization/sidebar-nav.tsx
Length of output: 149
Script:
#!/bin/bash
# Let's get more context around the active link highlighting logic
rg -B 5 -A 5 'item\.href\.endsWith\(leafRoute.*\)' frontend/src/app/organization/sidebar-nav.tsx
# Also check how leafRoute and defaultRoute are determined
rg 'const.*(?:leafRoute|defaultRoute)' frontend/src/app/organization/sidebar-nav.tsx
# Check the structure of navigation items
ast-grep --pattern 'const $_ = [
$$$
] as const'
Length of output: 1661
Script:
#!/bin/bash
# Let's check the navigation items structure and how they're defined
rg -B 2 -A 10 'const.*items.*=.*\[' frontend/src/app/organization/sidebar-nav.tsx
# Also check if there are any nested routes in the codebase
fd -e tsx -e ts -t f . frontend/src/app/organization | xargs rg 'href.*=.*/'
Length of output: 162
Script:
#!/bin/bash
# Let's try a different approach to find the navigation items and route structure
cat frontend/src/app/organization/sidebar-nav.tsx
# Also check for any route definitions or patterns
rg -l 'href.*=.*["\']/' frontend/src/app/organization/
Length of output: 2319
tracecat/api/app.py (1)
16-16: Ensure consistency after router import changes
The import for users_router has been updated, and a new org_router has been added. Verify that these changes maintain the expected functionality and that all routes are correctly registered. Ensure that any dependencies or configurations affected by the import changes are also updated accordingly.
Also applies to: 27-27, 255-255
frontend/src/client/services.gen.ts (1)
1354-1402: New organization member API methods implemented correctly
The methods for listing, deleting, and updating organization members are correctly defined and properly implement the intended functionality.
frontend/src/client/types.gen.ts (2)
439-449: Type OrgMemberRead is correctly defined
The OrgMemberRead type includes all necessary fields to represent an organization member.
1679-1693: Organization member types are correctly defined
The types supporting organization member operations are appropriately defined and align with the expected data structures.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1685-1685: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
frontend/src/client/schemas.gen.ts (1)
1316-1369: Schema $OrgMemberRead added correctly
The new schema $OrgMemberRead is properly defined and matches the expected structure for organization members, including all required properties with appropriate types.
Changes
/organization/members. Will also support managing settings under/organization/settingsin the futurePS
Most of the LOC added here are from copying and pasting frontend table code
Screens
In this clip i show:
Screen.Recording.2024-11-30.at.13.09.15.mov
In this second clip I start in [email protected] (1), then log into [email protected] and delete (1). Then i try to log into (1) whose password is password1234, and am no longer able to login.
Screen.Recording.2024-11-30.at.15.29.59.mov
Security
Registering and trying to list org members is blocked by default beacuse of the admin level requirement
Summary by CodeRabbit
Release Notes
New Features
MembersPagecomponent for managing organization members.OrgMembersTablecomponent to display member details with management options.Bug Fixes
Documentation