Role Management API and UI #801
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
mohamedelabbas1996
force-pushed
the
feat/role-management-api
branch
from
6cbef28 to
562f734
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConvert the implicit Project–User many-to-many into an explicit UserProjectMembership through-model with migrations; add membership permissions, APIs and nested routing; implement permission handling and tests; and add a Team UI with frontend hooks, types, routes, components, and i18n/text updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI)
participant API as Django API
participant DB as Database
participant RoleSys as Role system
Note over UI,API: Create membership
UI->>API: POST /projects/{project_id}/members (email, role_id)
API->>DB: find or create User
API->>DB: create UserProjectMembership (transaction)
API->>RoleSys: unassign existing roles for user in project
API->>RoleSys: assign new role to user
RoleSys->>DB: persist role assignments
API-->>UI: 201 Created (membership)
sequenceDiagram
participant UI as Client (UI)
participant API as Django API
participant DB as Database
participant RoleSys as Role system
Note over UI,API: Update membership role
UI->>API: PATCH /projects/{project_id}/members/{id} (role_id)
API->>API: permission check (UserMembershipPermission)
API->>DB: update UserProjectMembership (transaction)
API->>RoleSys: unassign previous roles
API->>RoleSys: assign new role
RoleSys->>DB: persist role changes
API-->>UI: 200 OK (updated membership)
sequenceDiagram
participant UI as Client (UI)
participant API as Django API
participant DB as Database
participant RoleSys as Role system
Note over UI,API: Remove membership
UI->>API: DELETE /projects/{project_id}/members/{id}
API->>API: permission check (or allow self-delete)
API->>RoleSys: unassign all roles for user in project
RoleSys->>DB: persist removals
API->>DB: delete UserProjectMembership
API-->>UI: 204 No Content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…sions to members viewset
…Lab/antenna into feat/role-management-api
Role Management APIThis API provides endpoints for managing project roles and member memberships. 1. List Available RolesEndpoint Name: List Available Roles Request: Request Body: None Response: [
{
"id": "BasicMember",
"name": "Basic member",
"description": "Basic project member with access to star source images, create jobs, and run single image processsing jobs."
},
{
"id": "Researcher",
"name": "Researcher",
"description": "Researcher with all basic member permissions, plus the ability to trigger data exports"
},
{
"id": "Identifier",
"name": "Identifier",
"description": "Identifier with all basic member permissions, plus the ability to create, update, and delete occurrence identifications."
},
{
"id": "MLDataManager",
"name": "ML Data manager",
"description": "Machine Learning Data Manager with all basic member permissions, plus the ability to manage ML jobs, run collection population jobs, sync data storage, export data, and delete occurrences."
},
{
"id": "ProjectManager",
"name": "Project manager",
"description": "Project manager with full administrative access, including all permissions from all roles plus the ability to manage project settings, members, deployments, collections, storage, and all project resources."
}
]Access Requirements: None 2. List Project MembersEndpoint Name: List Project Members Request: Request Body: None Response: {
"count": 23,
"next": "http://localhost:8000/api/v2/projects/11/members/?limit=20&offset=20",
"previous": null,
"results": [
{
"id": 187,
"user": {
"id": 9,
"name": "",
"email": "",
"image": null,
"details": "http://localhost:8000/api/v2/users/9/",
"user_permissions": []
},
"role": "Researcher",
"role_display_name": "Researcher",
"role_description": "Researcher with all basic member permissions, plus the ability to trigger data exports",
"created_at": "2025-12-05T01:15:43.701856",
"updated_at": "2025-12-05T01:15:43.701898",
"user_permissions": [
"update",
"delete"
]
},
{
"id": 82,
"user": {
"id": 13,
"name": "",
"email": "",
"image": null,
"details": "http://localhost:8000/api/v2/users/13/",
"user_permissions": []
},
"role": "BasicMember",
"role_display_name": "Basic member",
"role_description": "Basic project member with access to star source images, create jobs, and run single image processsing jobs.",
"created_at": "2025-12-01T08:26:49.846599",
"updated_at": "2025-12-01T08:26:49.846607",
"user_permissions": [
"update",
"delete"
]
}
],
"user_permissions": ["create"]
}Access Requirements: User must be a superuser or at least a project basic member 3. Get Member DetailsEndpoint Name: Get Member Details Request: Request Body: None Response: {
"id": 187,
"user": {
"id": 9,
"name": "",
"email": "",
"image": null,
"details": "http://localhost:8000/api/v2/users/9/",
"user_permissions": []
},
"project": "http://localhost:8000/api/v2/projects/11/",
"role": "Researcher",
"role_display_name": "Researcher",
"role_description": "Researcher with all basic member permissions, plus the ability to trigger data exports",
"created_at": "2025-12-05T01:15:43.701856",
"updated_at": "2025-12-05T01:15:43.701898",
"user_permissions": [
"update",
"delete"
]
}Access Requirements: User must be a superuser or at least a project basic member 4. Add MemberEndpoint Name: Add Member Request: Request Body: {
"email": "user@example.com",
"role_id": "Researcher"
}Request Fields:
Response: {
"id": 188,
"user": {
"id": 51,
"name": "",
"email": "",
"image": null,
"details": "http://localhost:8000/api/v2/users/51/",
"user_permissions": []
},
"project": "http://localhost:8000/api/v2/projects/11/",
"role": "Identifier",
"role_display_name": "Identifier",
"role_description": "Identifier with all basic member permissions, plus the ability to create, update, and delete occurrence identifications.",
"created_at": "2025-12-05T02:06:57.212676",
"updated_at": "2025-12-05T02:06:57.212794",
"user_permissions": [
"update",
"delete"
]
}Error Response (User already member): {
"non_field_errors": [
"User is already a member of this project."
]
}Error Response (User not found): {
"email": [
"User does not exist in the system."
]
}Error Response (Invalid role): {
"role_id": [
"Invalid role_id. Must be one of: ['BasicMember', 'Researcher', 'Identifier', 'MLDataManager', 'ProjectManager']"
]
}Access Requirements: User must be ProjectManager or superuser 5. Update Member RoleEndpoint Name: Update Member Role Request: Note: Use Request Body: {
"role_id": "Identifier"
}Request Fields:
Response: {
"id": 187,
"user": {
"id": 9,
"name": "",
"email": "",
"image": null,
"details": "http://localhost:8000/api/v2/users/9/",
"user_permissions": []
},
"project": "http://localhost:8000/api/v2/projects/11/",
"role": "MLDataManager",
"role_display_name": "ML Data manager",
"role_description": "Machine Learning Data Manager with all basic member permissions, plus the ability to manage ML jobs, run collection population jobs, sync data storage, export data, and delete occurrences.",
"created_at": "2025-12-05T01:15:43.701856",
"updated_at": "2025-12-05T02:10:15.123456",
"user_permissions": [
"update",
"delete"
]
}Access Requirements: User must be a ProjectManager or superuser 6. Remove MemberEndpoint Name: Remove Member Request: Note: Use Request Body: None Response: HTTP 204 No Content (empty response body) Access Requirements: User must be a project manager OR a superuser. Also, the user can delete their own membership (self-removal is allowed for all members) |
| class Meta: | ||
| model = User | ||
| fields = ["id", "name", "details", "image"] | ||
| fields = ["id", "name", "details", "image", "email"] |
There was a problem hiding this comment.
Note: I added email here because I needed it for the teams table, however I think this might also effect other API endpoints where we don't want to expose this in the response
|
Thank you so much @mohamedelabbas1996, this looks great! Here are some notes from when hooking this up with FE. Notes
|
…izer and update the members viewset to use the UserProjectMembership model
…Lab/antenna into feat/role-management-api
…eck can access it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ami/users/api/views.py (2)
52-66: Remove unnecessary assignment.Line 54 assigns
user = serializer._validated_userbut this value is immediately overwritten at line 58 withuser = membership.userbefore being used. The initial assignment serves no purpose.Apply this diff:
def perform_create(self, serializer): project = self.get_active_project() - user = serializer._validated_user role_cls = serializer._validated_role_cls with transaction.atomic(): membership = serializer.save(project=project) user = membership.user
67-80: Apply defensive consistency for role_cls.Line 70 uses
hasattr()to safely access_validated_userwith a fallback, but line 71 directly accesses_validated_role_clswithout the same guard. For defensive consistency, apply the same pattern.Apply this diff:
def perform_update(self, serializer): membership = self.get_object() project = membership.project user = serializer._validated_user if hasattr(serializer, "_validated_user") else membership.user - role_cls = serializer._validated_role_cls + role_cls = getattr(serializer, "_validated_role_cls", None) + if not role_cls: + raise ValueError("role_cls not set during validation") with transaction.atomic(): membership.user = user membership.save()
🧹 Nitpick comments (2)
ami/users/api/views.py (2)
21-29: LGTM! Optional typing refinement available.The viewset correctly exposes supported project roles via
Role.get_supported_roles()and applies appropriate permissions.For strict typing compliance, annotate DRF class attributes with
typing.ClassVar:+from typing import ClassVar + class ProjectRolesViewSet(viewsets.ViewSet, ProjectMixin): require_project = True - permission_classes = [IsActiveStaffOrReadOnly] + permission_classes: ClassVar = [IsActiveStaffOrReadOnly]Note: The static analysis warning about unused
requestandkwargsparameters is a false positive—these are required by DRF's method signature.
32-51: LGTM! Efficient queryset and appropriate serializer selection.The queryset correctly filters by project and uses
select_related("user")to optimize database queries. Serializer selection and context augmentation are properly implemented.For strict typing compliance, annotate DRF class attributes with
typing.ClassVar:class UserProjectMembershipViewSet(DefaultViewSet, ProjectMixin): require_project = True queryset = UserProjectMembership.objects.all() - permission_classes = [UserMembershipPermission] - ordering_fields = ["created_at", "updated_at", "user__email"] + permission_classes: ClassVar = [UserMembershipPermission] + ordering_fields: ClassVar = ["created_at", "updated_at", "user__email"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/api/views.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
ami/users/api/views.py
24-24: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
26-26: Unused method argument: request
(ARG002)
26-26: Unused method argument: kwargs
(ARG002)
35-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
36-36: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
ami/users/api/views.py (1)
81-90: LGTM!The destroy operation correctly unassigns all project roles before deleting the membership, maintaining referential integrity within a transaction.
Thanks! I think doing some more testing would be useful,especially on a live snapshot. I haven’t tried that yet, but I think it would be a good next step. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ami/users/api/serializers.py (1)
92-184: Prevent email from being changed on membership updates.The current implementation allows
IntegrityError risk: When updating with a different email, line 181 sets
attrs["user"]to the new user, but the uniqueness check on line 173 only runs on create. If the new user already has a membership for this project, theunique_togetherconstraint will raiseIntegrityError(500) instead of a clean validation error (400).API design mismatch: Per the PR objectives, the PATCH endpoint is intended to update a member's role only, not reassign the membership to a different user.
Solution: Prevent
def validate_email(self, value): """Validate user email and store actual user object.""" + # Email should only be provided when creating a membership + if self.instance is not None: + raise serializers.ValidationError("Cannot change user on an existing membership.") + try: user = User.objects.get(email=value) except User.DoesNotExist: raise serializers.ValidationError("User does not exist in the system.") # Save for use in .validate() self._validated_user = user return valueAdditionally, make field requirements explicit:
-email = serializers.EmailField(write_only=True) -role_id = serializers.CharField(write_only=True) +email = serializers.EmailField(write_only=True, required=True) +role_id = serializers.CharField(write_only=True, required=True)Then in
validate():def validate(self, attrs): + # On update, email should not be in attrs (caught by validate_email) + # On update, only role_id is expected + if self.instance is not None and "email" in self.initial_data: + # This should have been caught by validate_email, but double-check + raise serializers.ValidationError({"email": "Cannot change user on an existing membership."}) + project = self.context["project"] user = getattr(self, "_validated_user", None) role_cls = getattr(self, "_validated_role_cls", None)Based on learnings, this is a continuation of the previous review discussion about update behavior and IntegrityError handling.
🧹 Nitpick comments (1)
ami/users/api/serializers.py (1)
186-202: Consider removing redundant field declarations.The
user,role,role_display_name, androle_descriptionfield declarations on lines 187-190 are redundant since they're inherited fromUserProjectMembershipSerializer. TheMeta.fieldslist is sufficient to control which fields appear in the serialized output.class UserProjectMembershipListSerializer(UserProjectMembershipSerializer): - user = UserListSerializer(read_only=True) - role = serializers.SerializerMethodField() - role_display_name = serializers.SerializerMethodField() - role_description = serializers.SerializerMethodField() - class Meta: model = UserProjectMembership fields = [ "id", "user", "role", "role_display_name", "role_description", "created_at", "updated_at", ]The inheritance already provides these field definitions, so re-declaring them adds no functional value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/api/serializers.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ami/users/api/serializers.py (4)
ami/main/models.py (2)
UserProjectMembership(486-524)name(1062-1063)ami/users/models.py (1)
User(10-44)ami/base/serializers.py (1)
DefaultSerializer(42-59)ami/users/roles.py (2)
Role(12-81)get_primary_role(76-81)
🪛 Ruff (0.14.8)
ami/users/api/serializers.py
14-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-114: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
115-123: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
130-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
194-202: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
ami/users/api/serializers.py (2)
55-90: LGTM! Well-documented role serializer.The comprehensive docstring clearly explains that roles are class-based (not DB records) and addresses the previous review feedback. The implementation correctly serializes role class attributes via
SerializerMethodField.
146-162: LGTM! Role field computation is appropriate.The
SerializerMethodFieldusage for role-related fields is correct, as these are computed dynamically viaRole.get_primary_role()and are not stored model attributes. The implementation properly handles cases where no role is assigned (returnsNone).
| class Meta: | ||
| model = User | ||
| fields = ["id", "name", "details", "image"] | ||
| fields = ["id", "name", "details", "image", "email"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "UserListSerializer" --type=py -C 3Repository: RolnickLab/antenna
Length of output: 2166
🏁 Script executed:
rg -n "UserProjectMembershipSerializer|UserSerializer" --type=py ami/users/api/views.py -C 5Repository: RolnickLab/antenna
Length of output: 865
🏁 Script executed:
rg -n "UserSerializer" --type=py ami/ -C 2 | grep -v "UserProjectMembership" | grep -v "class User"Repository: RolnickLab/antenna
Length of output: 877
🏁 Script executed:
rg -n "CurrentUserSerializer" --type=py ami/Repository: RolnickLab/antenna
Length of output: 138
🏁 Script executed:
git log --oneline -n 20 -- ami/users/api/serializers.pyRepository: RolnickLab/antenna
Length of output: 114
🏁 Script executed:
cat -n ami/users/api/serializers.py | head -50Repository: RolnickLab/antenna
Length of output: 1848
Email field placement violates the intended design pattern.
The docstring in CurrentUserSerializer (line 38-39) states that email should only be exposed in the /users/me/ endpoint. However, email is added to UserListSerializer (line 14), which exposes it everywhere this serializer is used. This makes the redundant email field in CurrentUserSerializer (lines 43, 46) dead code.
Move the email field from UserListSerializer.Meta.fields to CurrentUserSerializer.Meta.fields only, and update UserListSerializer to exclude email. This will ensure email is only exposed for the authenticated user's own profile, as the design documentation indicates.
🧰 Tools
🪛 Ruff (0.14.8)
14-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In ami/users/api/serializers.py around line 14, the UserListSerializer currently
includes "email" in Meta.fields which exposes email everywhere; move "email" out
of UserListSerializer.Meta.fields so it no longer appears in the list
serializer, and add "email" to CurrentUserSerializer.Meta.fields (remove any
redundant email declarations there if necessary) so the email field is only
present on the /users/me/ serializer as per the docstring; ensure imports/field
definitions remain valid and run tests to confirm no serializer references
break.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ui/src/utils/language.ts (2)
508-509: Fix spelling and grammar in password reset message.The message contains a typo ("intructions" → "instructions") and incorrect subject-verb agreement ("has been" → "have been").
Apply this diff to correct the message:
[STRING.MESSAGE_RESET_INSTRUCTIONS_SENT]: - 'Reset intructions has been sent to {{email}}!', + 'Reset instructions have been sent to {{email}}!',
620-620: Correct typo in password setup message.The word "acccount" is misspelled and should be "account".
Apply this diff to fix the typo:
- [STRING.SET_PASSWORD_DETAILS]: 'Please set a new password for your acccount.', + [STRING.SET_PASSWORD_DETAILS]: 'Please set a new password for your account.',
🧹 Nitpick comments (1)
ui/src/pages/project/team/team-columns.tsx (1)
24-30: Add descriptive alt text to user avatar image.The avatar image has an empty
altattribute. For better accessibility, consider providing descriptive alt text such as the user's name or email so screen reader users can identify whose avatar is displayed.Apply this diff to improve accessibility:
<div className="flex items-center justify-center w-12 h-12 border border-border rounded-full text-muted-foreground overflow-hidden"> {item.image ? ( - <img className="object-cover" alt="" src={item.image} /> + <img className="object-cover" alt={item.name || item.email} src={item.image} /> ) : ( <UserIcon className="w-4 h-4" /> )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ui/src/data-services/hooks/team/useMembers.ts(1 hunks)ui/src/data-services/models/member.ts(1 hunks)ui/src/pages/project/team/team-columns.tsx(1 hunks)ui/src/pages/project/team/team.tsx(1 hunks)ui/src/utils/language.ts(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/data-services/hooks/team/useMembers.ts
- ui/src/data-services/models/member.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test
| <Link | ||
| className={buttonVariants({ | ||
| size: 'small', | ||
| variant: 'outline', | ||
| })} | ||
| to={project.permissionsAdminUrl} | ||
| target="_blank" | ||
| > |
There was a problem hiding this comment.
Add security attributes to external link.
The Link component opens in a new tab with target="_blank" but is missing rel="noopener noreferrer", which can expose the page to security risks (tabnapping) and performance issues.
Apply this diff to add the security attribute:
<Link
className={buttonVariants({
size: 'small',
variant: 'outline',
})}
to={project.permissionsAdminUrl}
target="_blank"
+ rel="noopener noreferrer"
>
<LockIcon className="w-4 h-4" />
<span>{translate(STRING.ADMIN)}</span>
</Link>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link | |
| className={buttonVariants({ | |
| size: 'small', | |
| variant: 'outline', | |
| })} | |
| to={project.permissionsAdminUrl} | |
| target="_blank" | |
| > | |
| <Link | |
| className={buttonVariants({ | |
| size: 'small', | |
| variant: 'outline', | |
| })} | |
| to={project.permissionsAdminUrl} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > |
🤖 Prompt for AI Agents
In ui/src/pages/project/team/team.tsx around lines 60 to 67, the Link opens with
target="_blank" but lacks rel="noopener noreferrer"; update the Link element to
include rel="noopener noreferrer" so external links opened in a new tab are
protected against tabnapping and related security/performance issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/src/pages/project/team/team-columns.tsx (1)
27-27: Consider adding descriptive alt text for the avatar image.The empty
altattribute is acceptable for decorative images, but providing descriptive text (e.g., the user's name or email) would improve accessibility for screen reader users.Apply this diff to add descriptive alt text:
- <img className="object-cover" alt="" src={item.image} /> + <img className="object-cover" alt={item.name || item.email} src={item.image} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/pages/project/team/team-columns.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/src/pages/project/team/team-columns.tsx (12)
ui/src/design-system/components/table/types.ts (1)
TableColumn(15-27)ui/src/data-services/models/member.ts (1)
Member(5-16)ui/src/utils/language.ts (1)
translate(638-655)ui/src/design-system/components/table/basic-table-cell/basic-table-cell.tsx (1)
BasicTableCell(15-43)ui/src/design-system/components/badge/badge.tsx (1)
Badge(4-18)ui/src/design-system/components/tooltip/basic-tooltip.tsx (1)
BasicTooltip(12-37)ui/src/design-system/components/button/button.tsx (1)
Button(27-86)ui/src/utils/date/getFormatedDateString/getFormatedDateString.ts (1)
getFormatedDateString(7-13)ui/src/utils/date/getFormatedTimeString/getFormatedTimeString.ts (1)
getFormatedTimeString(6-20)ui/src/pages/project/team/leave-team-dialog.tsx (1)
LeaveTeamDialog(14-82)ui/src/pages/project/team/manage-access-dialog.tsx (1)
ManageAccessDialog(19-76)ui/src/pages/project/team/remove-member-dialog.tsx (1)
RemoveMemberDialog(13-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (5)
ui/src/pages/project/team/team-columns.tsx (5)
40-44: LGTM!The name column implementation is straightforward and correct.
45-66: LGTM!The role column correctly implements the tooltip with role description, and the previous accessibility concern has been addressed with the
aria-labelon line 55.
67-77: LGTM!The added-at column correctly formats and displays the timestamp with proper sorting support.
78-96: LGTM!The updated-at column properly handles the optional field and maintains consistency with the added-at column implementation.
106-115: LGTM!The conditional logic correctly handles different scenarios:
- Shows appropriate dialog for the current user (self-removal via LeaveTeamDialog)
- Shows management dialogs for other members based on permissions
The permission checks (
canUpdate,canDelete) properly gate the actions.
| styles: { | ||
| padding: '16px', | ||
| width: '100%', | ||
| }, | ||
| renderCell: (item: Member) => ( | ||
| <div className="p-4 flex items-center justify-end gap-2"> |
There was a problem hiding this comment.
Remove duplicative padding and reconsider the width setting.
Two issues with the actions column styling:
-
Duplicative padding: The column styles specify
padding: '16px'(line 101), and the inner div also hasp-4which is 16px in Tailwind (line 105). This results in 32px total padding instead of the intended 16px. -
Incorrect width: Setting
width: '100%'(line 102) for an actions column is unusual—actions columns should typically have a fixed or minimal width to size to their content, not expand to fill available space.
Apply this diff to remove the duplicative padding and fix the width:
id: 'actions',
name: '',
styles: {
- padding: '16px',
- width: '100%',
+ textAlign: 'right',
},
renderCell: (item: Member) => (
- <div className="p-4 flex items-center justify-end gap-2">
+ <div className="flex items-center justify-end gap-2">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| styles: { | |
| padding: '16px', | |
| width: '100%', | |
| }, | |
| renderCell: (item: Member) => ( | |
| <div className="p-4 flex items-center justify-end gap-2"> | |
| styles: { | |
| textAlign: 'right', | |
| }, | |
| renderCell: (item: Member) => ( | |
| <div className="flex items-center justify-end gap-2"> |
🤖 Prompt for AI Agents
In ui/src/pages/project/team/team-columns.tsx around lines 100–105, remove the
duplicated padding and stop forcing the actions column to fill the row: delete
the padding property from the column styles (so the inner div's p-4 provides the
single 16px) and replace width: '100%' with a non-expanding size (e.g., remove
the width or set width: 'auto' or a small fixed value like '120px') so the
actions column sizes to its content.
…egrityError when reassigning a membership to a user who already has one in the project.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ami/users/api/serializers.py (1)
14-14: Email field exposure remains unresolved.Per the past review and the design documented in
CurrentUserSerializerdocstring (lines 38-39), email should only be exposed via the/users/me/endpoint, not in all endpoints usingUserListSerializer. This issue was previously flagged but remains in the code.
🧹 Nitpick comments (5)
ami/users/api/serializers.py (5)
125-134: Add exception chaining to distinguish from handler errors.The exception at line 130 should use
raise ... from errorraise ... from Noneto make clear whether the ValidationError stems from a User.DoesNotExist or from error-handling logic itself.Apply this diff:
def validate_email(self, value): """Validate user email and store actual user object.""" try: user = User.objects.get(email=value) - except User.DoesNotExist: - raise serializers.ValidationError("User does not exist in the system.") + except User.DoesNotExist as err: + raise serializers.ValidationError("User does not exist in the system.") from err # Save for use in .validate() self._validated_user = user return valueBased on static analysis hints.
146-162: Consider moving repeated Role import to module level.The
Roleclass is imported locally three times (lines 147, 153, 159). Moving this import to the module level would improve clarity and reduce redundancy.Apply this diff:
+from ami.users.roles import Role from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from rest_framework import serializersThen remove the local imports in
get_role,get_role_display_name, andget_role_description:def get_role(self, obj): - from ami.users.roles import Role - role_cls = Role.get_primary_role(obj.project, obj.user) return role_cls.__name__ if role_cls else None def get_role_display_name(self, obj): - from ami.users.roles import Role - role_cls = Role.get_primary_role(obj.project, obj.user) return role_cls.display_name if role_cls else None def get_role_description(self, obj): - from ami.users.roles import Role - role_cls = Role.get_primary_role(obj.project, obj.user) return role_cls.description if role_cls else None
177-183: Clarify update validation logic for user reassignment.The condition at line 180 checks
if user != self.instance.user, but whenuserwill beNone(fromgetattrat line 166). This makes the comparisonNone != self.instance.userevaluate toTrue, triggering an unnecessary existence check withuser=None.While the query will correctly return
False(no harm done), the logic is confusing. Consider checking whetherelse: # updating - # Check if another membership with same project+user exists - # Only check if user is being changed - if user != self.instance.user: + # Only validate uniqueness if email was provided and user is changing + if user is not None and user != self.instance.user: exists = UserProjectMembership.objects.filter(project=project, user=user).exists() if exists: raise serializers.ValidationError("User is already a member of this project.")
136-144: Move local Role import to validate_email for consistency.
validate_role_idimportsRolelocally (line 137), while other methods also import it locally. For consistency with the suggestion to move Role imports to module level, update this method as well after applying that refactor.
193-209: Remove redundant field declarations in list serializer.Lines 194-197 redeclare
user,role,role_display_name, androle_description, which are already defined identically in the parentUserProjectMembershipSerializer(lines 96-99). Since the child doesn't modify these declarations, they can be removed—only theMeta.fieldsoverride is needed.Apply this diff:
class UserProjectMembershipListSerializer(UserProjectMembershipSerializer): - user = UserListSerializer(read_only=True) - role = serializers.SerializerMethodField() - role_display_name = serializers.SerializerMethodField() - role_description = serializers.SerializerMethodField() - class Meta: model = UserProjectMembership fields = [ "id", "user", "role", "role_display_name", "role_description", "created_at", "updated_at", ]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/api/serializers.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ami/users/api/serializers.py (4)
ami/main/models.py (2)
UserProjectMembership(486-524)name(1062-1063)ami/users/models.py (1)
User(10-44)ami/base/serializers.py (1)
DefaultSerializer(42-59)ami/users/roles.py (2)
Role(12-81)get_primary_role(76-81)
🪛 Ruff (0.14.8)
ami/users/api/serializers.py
14-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-114: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
115-123: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
130-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
183-183: Avoid specifying long messages outside the exception class
(TRY003)
201-209: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
ami/users/api/serializers.py (1)
56-89: Well-documented role serializer implementation.The comprehensive docstring clearly explains that roles are class-based rather than database-backed, addressing previous feedback. The implementation correctly extracts role metadata from class attributes.
config/api_router.py
Outdated
| basename="project-members", | ||
| ) | ||
|
|
||
| # NESTED: /projects/{project_id}/roles/ |
There was a problem hiding this comment.
If this returns a fixed list of Roles for all of Antenna, independent of a project, than perhaps it should go somewhere else. But! if this will ever be configurable per project, then leave it here and consider our current list the "default roles".
perhaps /users/roles/ is a good home for the global static roles
config/api_router.py
Outdated
| # NESTED: /projects/{project_id}/roles/ | ||
| projects_router.register( | ||
| r"roles", | ||
| ProjectRolesViewSet, |
There was a problem hiding this comment.
Does this need to be a ViewSet or just a simple View that returns a static list view?
ami/users/api/views.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ProjectRolesViewSet(viewsets.ViewSet, ProjectMixin): |
There was a problem hiding this comment.
Is there a generic View class that only has what is needed here? perhaps just APIView?
ami/users/api/views.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ProjectRolesViewSet(viewsets.ViewSet, ProjectMixin): |
There was a problem hiding this comment.
Is there a generic View class that only has what is needed here? perhaps just APIView
There was a problem hiding this comment.
also you don't need project or permissions here correct?
class ProjectRolesView(APIView):
def get(self, request, **kwargs):
roles = Role.get_supported_roles()
serializer = ProjectRoleSerializer(roles, many=True)
return Response(serializer.data)
| "Ownership & Access", | ||
| { | ||
| "fields": ("owner", "members"), | ||
| "fields": ("owner",), |
There was a problem hiding this comment.
Note from conversation: if we wanted to allow membership editing from the admin, use an inline model with our new through model (Membership). Or a read-only list to show the members, but force editing from the React UI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ui/src/pages/project/team/about-roles.tsx (1)
8-35: Consider adding loading and error state handling.The component currently doesn't handle loading or error states from
useRoles. Users opening the dialog while roles are loading will see an empty list without feedback, and fetch errors will be silently ignored.Consider destructuring the loading and error states from
useRolesand providing appropriate UI feedback:export const AboutRoles = () => { - const { roles = [] } = useRoles(true) + const { roles = [], isLoading, error } = useRoles(true) return ( <Dialog.Root> <Dialog.Trigger asChild> <Button size="small" variant="ghost"> <InfoIcon className="w-4 h-4" /> <span>{translate(STRING.ABOUT_ROLES)}</span> </Button> </Dialog.Trigger> <Dialog.Content ariaCloselabel={translate(STRING.CLOSE)} className="max-w-lg h-fit" > <Dialog.Header title={translate(STRING.ABOUT_ROLES)} /> <FormSection> + {isLoading && <p>Loading roles...</p>} + {error && <p>Failed to load roles.</p>} + {!isLoading && !error && roles.length === 0 && <p>No roles available.</p>} {roles.map((role) => ( <div key={role.id}> <h3 className="body-base font-medium mb-2">{role.name}</h3> <p className="body-base">{role.description}</p> </div> ))} </FormSection> </Dialog.Content> </Dialog.Root> ) }ami/users/api/views.py (3)
31-32: Consider annotating mutable class attributes withClassVar.For improved type safety and clarity, consider annotating these mutable class attributes with
typing.ClassVaras suggested by static analysis.+from typing import ClassVar + class UserProjectMembershipViewSet(DefaultViewSet, ProjectMixin): require_project = True queryset = UserProjectMembership.objects.all() - permission_classes = [UserMembershipPermission] - ordering_fields = ["created_at", "updated_at", "user__email"] + permission_classes: ClassVar = [UserMembershipPermission] + ordering_fields: ClassVar = ["created_at", "updated_at", "user__email"]
48-62: Remove redundant user assignment.Line 50 assigns
user = serializer._validated_userbut this value is immediately overwritten on line 54 withuser = membership.userbefore being used. The first assignment serves no purpose.def perform_create(self, serializer): project = self.get_active_project() - user = serializer._validated_user role_cls = serializer._validated_role_cls with transaction.atomic(): membership = serializer.save(project=project) user = membership.user # unassign all existing roles for this project for r in Role.__subclasses__(): r.unassign_user(user, project) # assign new role role_cls.assign_user(user, project)
63-76: Make defensive checks consistent.Line 66 uses a
hasattrguard for_validated_user, but line 67 directly accesses_validated_role_clswithout a similar check. For consistency and defensive programming, apply the same pattern to both:def perform_update(self, serializer): membership = self.get_object() project = membership.project user = serializer._validated_user if hasattr(serializer, "_validated_user") else membership.user - role_cls = serializer._validated_role_cls + role_cls = getattr(serializer, "_validated_role_cls", None) + if not role_cls: + raise ValueError("role_cls not set during validation") with transaction.atomic(): membership.user = user membership.save() for r in Role.__subclasses__(): r.unassign_user(user, project) role_cls.assign_user(user, project)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ami/users/api/views.py(1 hunks)config/api_router.py(2 hunks)ui/src/data-services/constants.ts(1 hunks)ui/src/data-services/hooks/team/useRoles.ts(1 hunks)ui/src/pages/project/team/about-roles.tsx(1 hunks)ui/src/pages/project/team/roles-picker.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src/data-services/constants.ts
- ui/src/pages/project/team/roles-picker.tsx
- config/api_router.py
🧰 Additional context used
🧬 Code graph analysis (2)
ui/src/pages/project/team/about-roles.tsx (4)
ui/src/data-services/hooks/team/useRoles.ts (1)
useRoles(6-21)ui/src/design-system/components/button/button.tsx (1)
Button(27-86)ui/src/utils/language.ts (1)
translate(638-655)ui/src/components/form/layout/layout.tsx (1)
FormSection(24-46)
ui/src/data-services/hooks/team/useRoles.ts (4)
ui/src/data-services/models/role.ts (1)
Role(1-5)ui/src/data-services/hooks/auth/useAuthorizedQuery.ts (1)
useAuthorizedQuery(6-40)ui/src/data-services/constants.ts (1)
API_ROUTES(3-33)ui/src/data-services/utils.ts (1)
getFetchUrl(7-47)
🪛 Ruff (0.14.8)
ami/users/api/views.py
22-22: Unused method argument: request
(ARG002)
22-22: Unused method argument: kwargs
(ARG002)
31-31: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-32: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Redirect rules
🔇 Additional comments (4)
ui/src/data-services/hooks/team/useRoles.ts (1)
6-21: LGTM!The hook follows established patterns, properly leverages
useAuthorizedQueryfor authentication, and appropriately caches role data when requested. The conditionalstaleTimewithInfinityfor internal caching is a good optimization for relatively static role data.ami/users/api/views.py (3)
21-25: LGTM!The view correctly returns supported roles using the established
Role.get_supported_roles()method and serializes them appropriately. The unusedrequestandkwargsparameters flagged by static analysis are required by the Django REST framework signature and can be safely ignored.
34-46: LGTM!These methods follow Django REST framework best practices:
- Query optimization with
select_related("user")prevents N+1 queries- Conditional serializer class for list vs. detail actions is a standard pattern
- Augmenting serializer context with the active project enables proper validation
77-86: LGTM!The destroy operation correctly cleans up role assignments before deleting the membership within a transaction, ensuring data consistency.
| import logging | ||
|
|
||
| from django.db import transaction | ||
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
|
|
||
| from ami.base.permissions import UserMembershipPermission | ||
| from ami.base.views import ProjectMixin | ||
| from ami.main.api.views import DefaultViewSet | ||
| from ami.main.models import UserProjectMembership | ||
| from ami.users.api.serializers import ( | ||
| ProjectRoleSerializer, | ||
| UserProjectMembershipListSerializer, | ||
| UserProjectMembershipSerializer, | ||
| ) | ||
| from ami.users.roles import Role | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class RolesAPIView(APIView): | ||
| def get(self, request, **kwargs): | ||
| roles = Role.get_supported_roles() | ||
| serializer = ProjectRoleSerializer(roles, many=True) | ||
| return Response(serializer.data) | ||
|
|
||
|
|
||
| class UserProjectMembershipViewSet(DefaultViewSet, ProjectMixin): | ||
| require_project = True | ||
| queryset = UserProjectMembership.objects.all() | ||
| permission_classes = [UserMembershipPermission] | ||
| ordering_fields = ["created_at", "updated_at", "user__email"] | ||
|
|
||
| def get_queryset(self): | ||
| project = self.get_active_project() | ||
| return UserProjectMembership.objects.filter(project=project).select_related("user") | ||
|
|
||
| def get_serializer_class(self): | ||
| if self.action == "list": | ||
| return UserProjectMembershipListSerializer | ||
| return UserProjectMembershipSerializer | ||
|
|
||
| def get_serializer_context(self): | ||
| ctx = super().get_serializer_context() | ||
| ctx["project"] = self.get_active_project() | ||
| return ctx | ||
|
|
||
| def perform_create(self, serializer): | ||
| project = self.get_active_project() | ||
| user = serializer._validated_user | ||
| role_cls = serializer._validated_role_cls | ||
| with transaction.atomic(): | ||
| membership = serializer.save(project=project) | ||
| user = membership.user | ||
|
|
||
| # unassign all existing roles for this project | ||
| for r in Role.__subclasses__(): | ||
| r.unassign_user(user, project) | ||
|
|
||
| # assign new role | ||
| role_cls.assign_user(user, project) | ||
|
|
||
| def perform_update(self, serializer): | ||
| membership = self.get_object() | ||
| project = membership.project | ||
| user = serializer._validated_user if hasattr(serializer, "_validated_user") else membership.user | ||
| role_cls = serializer._validated_role_cls | ||
| with transaction.atomic(): | ||
| membership.user = user | ||
| membership.save() | ||
|
|
||
| for r in Role.__subclasses__(): | ||
| r.unassign_user(user, project) | ||
|
|
||
| role_cls.assign_user(user, project) | ||
|
|
||
| def perform_destroy(self, instance): | ||
| user = instance.user | ||
| project = instance.project | ||
|
|
||
| with transaction.atomic(): | ||
| # remove roles for this project | ||
| for r in Role.__subclasses__(): | ||
| r.unassign_user(user, project) | ||
|
|
||
| instance.delete() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test files related to membership API
echo "=== Searching for test files in ami/users/tests ==="
fd -e py . ami/users/tests/ 2>/dev/null | head -20
echo ""
echo "=== Searching for test methods related to membership/roles ==="
rg "def test.*member|def test.*role|class.*Test.*Membership" ami/users/tests/ -n 2>/dev/null | head -30
echo ""
echo "=== Searching for UserProjectMembershipViewSet references in tests ==="
rg "UserProjectMembership|RolesAPIView" ami/users/tests/ -n 2>/dev/null | head -20Repository: RolnickLab/antenna
Length of output: 2275
🏁 Script executed:
#!/bin/bash
# Read the test file to verify actual test coverage
echo "=== Reading test file content ==="
wc -l ami/users/tests/test_membership_management_api.py
echo ""
cat -n ami/users/tests/test_membership_management_api.pyRepository: RolnickLab/antenna
Length of output: 8619
Test coverage exists but lacks transaction error handling tests.
The test file ami/users/tests/test_membership_management_api.py covers most critical paths:
- Role listing ✓
- Membership CRUD operations ✓
- Role assignment and reassignment ✓
- Permission checks (all four operations) ✓
- Self-removal edge cases ✓
However, tests for transaction rollback on validation errors are missing. Add tests to verify that membership and role changes are properly rolled back when validation fails during perform_create and perform_update operations.
🧰 Tools
🪛 Ruff (0.14.8)
22-22: Unused method argument: request
(ARG002)
22-22: Unused method argument: kwargs
(ARG002)
31-31: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-32: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In ami/users/api/views.py lines 1-86: tests are missing to ensure DB
transactions roll back when errors occur during perform_create and
perform_update; add two tests that mock Role.assign_user (or similarly cause an
exception) to raise a ValidationError after serializer.save is called, call the
create/update endpoints via the test client, assert the response is an error,
then assert the membership was not created/changed in the database and no role
assignment persisted (check membership count, returned object state, and that
role side-effects did not occur); do this for both create and update paths to
verify transaction.atomic() properly rolls back.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ami/users/tests/test_membership_management_api.py (1)
10-207: Add input validation and error handling tests.The test suite still lacks coverage for error cases that were identified in the previous review. Consider adding tests for:
- Invalid role_id: POST/PATCH with a non-existent role should return 400
- Non-existent user email: POST with an email not in the system should return 400/404
- Duplicate membership: POST for a user already in the project should return 400
- Missing required fields: POST without
role_idshould return 400These validation tests are essential to ensure API robustness and catch regressions in error handling.
Example test structure:
def test_create_membership_with_invalid_role_id(self): self.auth_super() payload = {"email": self.user2.email, "role_id": "NonExistentRole"} resp = self.client.post(self.members_url, payload, format="json") self.assertEqual(resp.status_code, 400) def test_create_membership_with_nonexistent_email(self): self.auth_super() payload = {"email": "nonexistent@example.com", "role_id": BasicMember.__name__} resp = self.client.post(self.members_url, payload, format="json") self.assertIn(resp.status_code, [400, 404]) def test_create_duplicate_membership(self): self.auth_super() self.create_membership(self.user1) payload = {"email": self.user1.email, "role_id": BasicMember.__name__} resp = self.client.post(self.members_url, payload, format="json") self.assertEqual(resp.status_code, 400) def test_create_membership_missing_required_fields(self): self.auth_super() resp = self.client.post(self.members_url, {}, format="json") self.assertEqual(resp.status_code, 400)Would you like me to generate a complete set of validation tests or open an issue to track this enhancement?
🧹 Nitpick comments (2)
ami/users/tests/test_membership_management_api.py (2)
16-16: Hardcoded test password is acceptable but could use a constant.The static analysis tool flags the hardcoded password, but this is standard practice in test files. Optionally, consider extracting it to a constant like
TEST_PASSWORD = "test_password_123"for clarity and reusability.
65-79: Consider verifying response structure for completeness.The test only checks the count of returned members. Per the PR objectives, the response should include role details, user information, permissions, timestamps, etc. Consider adding assertions to verify key fields are present in the response.
Example addition:
# Verify response structure if results: member = results[0] self.assertIn("id", member) self.assertIn("user", member) self.assertIn("role", member) self.assertIn("role_display_name", member) self.assertIn("user_permissions", member)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/tests/test_membership_management_api.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ami/users/tests/test_membership_management_api.py (2)
ami/main/models.py (3)
UserProjectMembership(486-524)delete(2231-2262)Permissions(350-424)ami/users/roles.py (2)
BasicMember(84-95)get_primary_role(76-81)
🪛 Ruff (0.14.8)
ami/users/tests/test_membership_management_api.py
16-16: Possible hardcoded password assigned to argument: "password"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
ami/users/tests/test_membership_management_api.py (3)
25-43: LGTM!The helper methods are well-structured. The
create_membershiphelper correctly creates only the membership record without assigning a role, which aligns with the separation of membership and role assignment in the new through-model design.
144-192: LGTM!The permission tests comprehensively cover all CRUD operations and correctly verify both the denial (403) without permissions and success after permissions are granted.
193-207: LGTM!The self-removal test correctly verifies that users can delete their own membership, which is a key feature per the PR objectives.
| def test_create_membership_functionality(self): | ||
| """ | ||
| Ensure that a membership is actually created and belongs to the project+user. | ||
| """ | ||
| self.auth_super() | ||
|
|
||
| payload = { | ||
| "email": self.user2.email, | ||
| "role_id": ProjectManager.__name__, | ||
| } | ||
|
|
||
| resp = self.client.post(self.members_url, payload, format="json") | ||
| self.assertEqual(resp.status_code, 201) | ||
|
|
||
| # Retrieve membership using project + user | ||
| membership = UserProjectMembership.objects.get( | ||
| project=self.project, | ||
| user__email=self.user2.email, | ||
| ) | ||
|
|
||
| self.assertEqual(membership.user, self.user2) | ||
| self.assertEqual(membership.project, self.project) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify that the role is actually assigned to the user.
The test confirms the membership record is created but doesn't verify that the ProjectManager role was assigned to self.user2. Consider adding an assertion to check the role assignment.
Apply this diff to verify role assignment:
self.assertEqual(membership.user, self.user2)
self.assertEqual(membership.project, self.project)
+
+ # Verify role assignment
+ assigned_role = Role.get_primary_role(self.project, self.user2)
+ self.assertEqual(assigned_role.__name__, ProjectManager.__name__)🤖 Prompt for AI Agents
ami/users/tests/test_membership_management_api.py around lines 80-102: the test
creates a membership but doesn't assert the assigned role; update the test to
verify the ProjectManager role is actually set on the created membership by
adding an assertion that checks the membership's role corresponds to
ProjectManager (e.g., assert membership.role_id == ProjectManager.__name__ or
assert membership.role.name == ProjectManager.__name__ or otherwise assert
membership.role == ProjectManager depending on the membership model API).
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ami/users/api/views.py (1)
76-77: Defensive check inconsistency flagged in past review.Line 76 uses
hasattr(serializer, "_validated_user")for defensive access, but line 77 directly accessesserializer._validated_role_clswithout a similar check. A past review comment already flagged this inconsistency. Apply the same defensive pattern to_validated_role_clsfor consistency.
🧹 Nitpick comments (2)
ami/users/api/views.py (2)
51-72: Remove redundant user assignment.Line 57 reassigns
userfrommembership.user, butuseris already set on line 53 fromserializer._validated_user. The serializer validation ensures these will be the same value, making line 57 redundant.🔎 Proposed fix
def perform_create(self, serializer): project = self.get_active_project() user = serializer._validated_user role_cls = serializer._validated_role_cls with transaction.atomic(): membership = serializer.save(project=project) - user = membership.user # Disconnect signal before unassigning/assigning roles to prevent signal interference # The membership is already created above, so we don't need the signal to modify it m2m_changed.disconnect(manage_project_membership, sender=Group.user_set.through)
73-86: Consider adding signal handling for consistency.
perform_createdisconnects themanage_project_membershipsignal before role manipulation (lines 61-71), butperform_updatedoes not. While the signal may not cause issues here since the membership already exists, adding the disconnect/reconnect pattern would ensure consistency and prevent potential race conditions if the signal logic changes in the future.🔎 Proposed refactor
def perform_update(self, serializer): membership = self.get_object() project = membership.project user = serializer._validated_user if hasattr(serializer, "_validated_user") else membership.user role_cls = serializer._validated_role_cls with transaction.atomic(): membership.user = user membership.save() + # Disconnect signal to prevent interference during role updates + m2m_changed.disconnect(manage_project_membership, sender=Group.user_set.through) + try: - for r in Role.__subclasses__(): - r.unassign_user(user, project) + for r in Role.__subclasses__(): + r.unassign_user(user, project) - role_cls.assign_user(user, project) + role_cls.assign_user(user, project) + finally: + m2m_changed.connect(manage_project_membership, sender=Group.user_set.through)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ami/main/models.py(3 hunks)ami/users/api/views.py(1 hunks)ami/users/roles.py(5 hunks)ami/users/signals.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ami/users/api/views.py (8)
ami/base/permissions.py (1)
UserMembershipPermission(92-118)ami/base/views.py (1)
ProjectMixin(62-86)ami/main/api/views.py (1)
DefaultViewSet(114-131)ami/main/models.py (2)
UserProjectMembership(485-523)delete(2230-2261)ami/users/api/serializers.py (2)
ProjectRoleSerializer(56-89)UserProjectMembershipSerializer(92-190)ami/users/roles.py (4)
Role(12-81)get_supported_roles(57-61)unassign_user(39-43)assign_user(28-36)ami/users/signals.py (1)
manage_project_membership(32-78)ami/users/models.py (1)
save(30-34)
ami/users/signals.py (2)
ami/main/models.py (4)
Project(228-482)UserProjectMembership(485-523)name(1061-1062)delete(2230-2261)ami/users/roles.py (2)
Role(12-81)user_has_any_role(52-54)
ami/users/roles.py (2)
ui/src/data-services/models/role.ts (1)
Role(1-5)ami/main/models.py (2)
Project(228-482)Permissions(350-423)
🪛 Ruff (0.14.8)
ami/users/api/views.py
25-25: Unused method argument: request
(ARG002)
25-25: Unused method argument: kwargs
(ARG002)
34-34: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
35-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (5)
ami/main/models.py (2)
235-240: LGTM! Explicit through-model for membership.The explicit
UserProjectMembershipthrough-model is a solid design choice that enables independent membership management with its own permissions and lifecycle, while keeping role assignment separate via permission groups.
485-524: LGTM! Self-deletion logic is correctly implemented.The through-model correctly:
- Enforces unique membership per user-project pair
- Allows users to view memberships if they have
VIEW_USER_PROJECT_MEMBERSHIPpermission- Grants users permission to delete their own membership via both
check_permissionandget_user_object_permissionsThis self-service removal capability aligns with the PR objectives.
ami/users/signals.py (1)
58-73: LGTM! Signal handling correctly uses the through-model.The signal correctly:
- Uses
get_or_createfor idempotent membership creation (lines 61-65)- Checks for remaining roles before removing membership (lines 69-72)
- Logs both new and existing membership cases for observability
This ensures membership state stays synchronized with role assignments.
ami/users/roles.py (2)
15-16: LGTM! Role metadata and helper methods enhance API discoverability.The additions of
display_nameanddescriptionattributes (lines 15-16) plus the new static methodsget_supported_roles(),get_user_roles(), andget_primary_role()(lines 56-82) provide clean introspection capabilities that directly support the role management API and UI requirements described in the PR objectives.Using
max(roles, key=lambda r: len(r.permissions))inget_primary_roleis a reasonable heuristic for determining the "highest" role when a user has multiple roles on a project.Also applies to: 56-82
85-95: LGTM! Permission assignments align with membership model.The permission updates correctly reflect the new membership management capabilities:
- BasicMember gains
VIEW_USER_PROJECT_MEMBERSHIP(line 94), allowing all project members to see the team roster- ProjectManager gains
CREATE/UPDATE/DELETE_USER_PROJECT_MEMBERSHIP(lines 177-179), granting full member management authorityThis permission distribution aligns with the role descriptions and PR objectives.
Also applies to: 177-181
| RESET_PASSWORD_CONFIRM: 'users/reset_password_confirm', | ||
| RESET_PASSWORD: 'users/reset_password', | ||
| ROLES: (projectId: string) => `projects/${projectId}/roles`, | ||
| ROLES: 'users/roles', |
There was a problem hiding this comment.
@annavik did you two chat about this change? we decided to move the list of roles to a different endpoint, since it's just a global constant of choices, it's not dependent on a project or permissions, so it can be a much simpler implementation on the backend.
Summary
This update introduces a Role Management feature that includes both API and UI support in Antenna, allowing project managers to manage project members and their roles directly from the Antenna site.
Previously, project membership and role assignment were handled primarily through the Django admin interface, which limited role management to administrators and required manual intervention. With this change, role and membership management are exposed through the API and can be managed by project managers directly through the Antenna UI, without needing admin access.
List of Changes
Added project role management API endpoints
GET /projects/<project-id>/roles/to list available project rolesGET /projects/<project-id>/members/to list project membersPOST /projects/<project-id>/members/to add a member and assign a rolePATCH /projects/<project-id>/members/<membership-id>/to update a member’s roleDELETE /projects/<project-id>/members/<membership-id>/to remove a member from the projectIntroduced
UserProjectMembershipas an explicit through modelAdded project-scoped permissions for membership management
Added nested routing for roles and members under projects
Added
is_memberfield to the project details responsetrueif the user is a project member or a superuserAdded UI support for managing project members and roles
Removed member management from the Project details admin page
Related Issues
#727
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style/UX
Tests
✏️ Tip: You can customize this high-level summary in your review settings.