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. 📝 WalkthroughWalkthroughAdds explicit project membership via a new through model, membership APIs and permissions, frontend team management UI/hooks/routes, signal/role integration for role assignment, and a migration moving existing M2M data to UserProjectMembership. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend UI
participant API as Membership API
participant RoleSvc as Role system
participant DB as Database
participant Signal as m2m_changed Signal
User->>Frontend: Submit add-member (email, role_id) for project
Frontend->>API: POST /projects/{project_id}/members (email, role_id)
API->>DB: Lookup user by email
API->>DB: Create or get UserProjectMembership record
API->>Signal: Temporarily disconnect m2m_changed
API->>RoleSvc: Unassign existing roles for user in project
RoleSvc->>DB: Remove role assignments
API->>RoleSvc: Assign new role to user (role_id)
RoleSvc->>DB: Persist role assignment
API->>Signal: Reconnect m2m_changed
API-->>Frontend: 201 Created (member payload)
Frontend->>Frontend: Refresh members list / show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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) |
ami/users/api/serializers.py
Outdated
| 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: 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.
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 canceled.
|
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.
- Combined user membership and data export permissions in Project Meta - Updated Researcher role: kept display_name/description, use new data export permissions Co-Authored-By: Claude <noreply@anthropic.com>
- Fix typo: "projet" -> "project" in models.py comment - Comment out unused variables in migration 0079 - Comment out unused assignment in views.py perform_create - Pin drf-nested-routers to version 0.95.0 Co-Authored-By: Claude <noreply@anthropic.com>
Use getattr() with explicit error handling instead of direct attribute access to ensure consistent defensive programming and clearer error messages when role_cls is not set during validation. Co-Authored-By: Claude <noreply@anthropic.com>
Split user serializers to control email exposure: - UserListSerializer: excludes email for general privacy - MemberUserSerializer: includes email for membership management only This ensures user emails are only exposed in membership endpoints where they are needed for management purposes. Co-Authored-By: Claude <noreply@anthropic.com>
Add tests for error cases: - Invalid role_id returns 400 - Nonexistent email returns 400 - Duplicate membership returns 400 - Missing email returns 400 - Missing role_id returns 400 Co-Authored-By: Claude <noreply@anthropic.com>
|
@annavik @mohamedelabbas1996 I brought this branch up-to-date with main and did some final testing! It seems to be working well. Do we need a migration or method to assign existing users to a role? For example, all of these users are members of the project, but don't have a role in the UI until I set one. 
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@ami/main/migrations/0080_userprojectmembership_migration.py`:
- Around line 33-39: The rollback SQL that recreates the join table in the
migration (main_project_members) is missing the foreign key constraints and the
uniqueness constraint; update the CREATE TABLE/DDL in
0080_userprojectmembership_migration.py to add FK constraints from project_id ->
main_project(id) and user_id -> main_user(id) and create a UNIQUE constraint or
unique index on (project_id, user_id) (name constraints clearly, e.g.,
main_project_members_project_id_fkey, main_project_members_user_id_fkey,
main_project_members_project_user_key) so that a backwards migration fully
restores the original M2M integrity.
In `@ami/users/api/views.py`:
- Around line 89-98: perform_destroy currently unassigns roles (via
Role.__subclasses__()) while membership deletion signal handlers are still
connected, so a signal can delete the membership mid-loop and make the later
instance.delete() operate on a stale object; fix this by temporarily
disconnecting the membership-related signal handler(s) (the same handler(s)
disconnected in perform_update) before the role-unassignment loop, perform the
Role.unassign_user(...) calls inside the transaction.atomic block, then
reconnect the signal handler(s) immediately after (ensure reconnect happens even
on error, e.g., using try/finally), so that instance.delete() runs against the
real DB row.
- Around line 73-87: perform_update can trigger the manage_project_membership
signal while wiping and reassigning roles, causing the membership to be deleted
and recreated (changing ID/timestamps); fix by temporarily disconnecting the
manage_project_membership signal around the role manipulation in perform_update
(after membership.save() and before the loop over Role.__subclasses__() and
role_cls.assign_user), then reconnect it afterward so unassign_user and
assign_user do not fire the signal during the update; keep the existing
transaction.atomic() block and ensure reconnection happens even on errors (i.e.,
use a try/finally around the signal disconnect/reconnect).
In `@ami/users/roles.py`:
- Around line 86-88: The user-facing description string for BasicMember contains
a typo: replace the substring "processsing" with "processing" in the description
variable (the description assigned in the BasicMember role/class) so the
sentence reads "...run single image processing jobs." and ensure the corrected
string preserves the parentheses and spacing.
♻️ Duplicate comments (1)
ami/main/migrations/0080_userprojectmembership_migration.py (1)
15-18: Prefer a plain SQL string for the fixed table name.Line 17 uses an f-string for a constant table name; keep it literal to avoid the S608 warning.
🧹 Suggested cleanup
- cursor.execute(f"SELECT project_id, user_id FROM {through_table};") + cursor.execute("SELECT project_id, user_id FROM main_project_members;")
🧹 Nitpick comments (2)
ami/users/api/serializers.py (1)
160-176: Avoid triple role lookups per membership serialization.Lines 160–176 compute the primary role three times; in list views this can add unnecessary DB/group checks. Cache once per object in the serializer.
♻️ Suggested refactor
class UserProjectMembershipSerializer(DefaultSerializer): @@ - 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_primary_role(self, obj): + from ami.users.roles import Role + + cache = getattr(self, "_primary_role_cache", None) + if cache is None: + cache = self._primary_role_cache = {} + return cache.setdefault(obj.pk, Role.get_primary_role(obj.project, obj.user)) + + def get_role(self, obj): + role_cls = self._get_primary_role(obj) + 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_display_name(self, obj): + role_cls = self._get_primary_role(obj) + 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 + def get_role_description(self, obj): + role_cls = self._get_primary_role(obj) + return role_cls.description if role_cls else Noneami/users/api/views.py (1)
51-57: Remove dead code and apply consistent defensive access pattern.Line 53 contains commented-out dead code. Line 54 directly accesses
_validated_role_clswithout the defensivegetattrpattern used inperform_update(line 77). Apply consistent error handling across both methods.♻️ Suggested fix
def perform_create(self, serializer): project = self.get_active_project() - # user = serializer._validated_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 = serializer.save(project=project) user = membership.user
| cursor.execute( | ||
| """ | ||
| CREATE TABLE IF NOT EXISTS main_project_members ( | ||
| id serial PRIMARY KEY, | ||
| project_id integer NOT NULL, | ||
| user_id integer NOT NULL | ||
| ); |
There was a problem hiding this comment.
Backwards migration should restore the M2M constraints.
Lines 33–39 recreate the join table without FK constraints or a unique index on (project_id, user_id). That weakens data integrity if a rollback is needed.
🧩 Suggested fix
def backwards(apps, schema_editor):
+ Project = apps.get_model("main", "Project")
+ User = apps.get_model(settings.AUTH_USER_MODEL.split(".")[0], settings.AUTH_USER_MODEL.split(".")[1])
+ project_table = Project._meta.db_table
+ user_table = User._meta.db_table
UserProjectMembership = apps.get_model("main", "UserProjectMembership")
with schema_editor.connection.cursor() as cursor:
# Recreate old table
cursor.execute(
"""
CREATE TABLE IF NOT EXISTS main_project_members (
id serial PRIMARY KEY,
- project_id integer NOT NULL,
- user_id integer NOT NULL
+ project_id integer NOT NULL REFERENCES {project_table} (id) ON DELETE CASCADE,
+ user_id integer NOT NULL REFERENCES {user_table} (id) ON DELETE CASCADE,
+ UNIQUE (project_id, user_id)
);
"""
)🤖 Prompt for AI Agents
In `@ami/main/migrations/0080_userprojectmembership_migration.py` around lines 33
- 39, The rollback SQL that recreates the join table in the migration
(main_project_members) is missing the foreign key constraints and the uniqueness
constraint; update the CREATE TABLE/DDL in
0080_userprojectmembership_migration.py to add FK constraints from project_id ->
main_project(id) and user_id -> main_user(id) and create a UNIQUE constraint or
unique index on (project_id, user_id) (name constraints clearly, e.g.,
main_project_members_project_id_fkey, main_project_members_user_id_fkey,
main_project_members_project_user_key) so that a backwards migration fully
restores the original M2M integrity.
|
|
||
|
|
||
| class MemberUserSerializer(UserListSerializer): | ||
| """User serializer for membership context - includes email for management purposes.""" |
There was a problem hiding this comment.
@annavik @mohamedelabbas1996 new serializer that includes email. the other one doesn't now.
Create explicit through model for Project.members M2M relationship to support role-based permissions. This migration: 1. Creates UserProjectMembership model with timestamps 2. Migrates existing membership data from implicit M2M table 3. Drops old implicit table 4. Updates Project.members to use through model 5. Adds membership permissions to Project This enables per-project role assignment via django-guardian groups while maintaining backward compatibility with existing memberships. Co-Authored-By: Claude <noreply@anthropic.com>
Changed 'processsing' to 'processing' in the user-facing role description text. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Disconnect the manage_project_membership signal during role manipulation in perform_update and perform_destroy to prevent the signal from deleting and recreating memberships with new IDs. This ensures: - Membership IDs remain stable during role updates - Timestamps (created_at) are preserved - No stale instance operations during deletion Uses the same pattern already proven in perform_create. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
mihow
force-pushed
the
feat/role-management-api
branch
from
7074063 to
c7be19f
Compare
…flict The 0.95.0 version conflicts with djangorestframework==3.14.0 and drf-spectacular==0.26.3, causing pip dependency resolution to fail during Docker build. Version 0.94.1 maintains compatibility with our existing DRF ecosystem while providing the nested routing functionality needed for the role management API.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@requirements/base.txt`:
- Around line 1-6: The file ends without a trailing newline causing pre-commit
EOF fixer failures; open the requirements/base.txt and add a single newline
character at the end of the file (after the last line, e.g. after the commented
line "# drf-nested-routers==0.94.1") so the file terminates with a newline and
the pre-commit hook passes.
|
Thanks a lot Michael!! Sorry I have been slow with my testing here 🐌 For your comment about migration, I think in most cases members should have a role assigned, but if not should be pretty fast to set from UI now, so I'm fine skipping the migration step! |
Or are you saying in some cases the role is set i admin, but not exposed from API? I have not seen that myself, but that would be more of a problem! |
|
Yes I believe these existing users have a Role already, but I am not seeing
it in the UI until I change it there.
…On Tue, Jan 20, 2026 at 1:30 AM Anna Viklund ***@***.***> wrote:
*annavik* left a comment (RolnickLab/antenna#801)
<#801 (comment)>
Thanks a lot Michael!! Sorry I have been slow with my testing here 🐌
For your comment about migration, I think in most cases members should
have a role assigned, but if not should be pretty fast to set from UI now,
so I'm fine skipping the migration step!
Or are you saying in some cases the role is set i admin, but not exposed
from API? That would be more of a problem!
—
Reply to this email directly, view it on GitHub
<#801 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABGTX2TJ32LQ73AITSIGSD4HXYS3AVCNFSM6AAAAAB2WTFF5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONZRHA4DIOBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hey @mihow! I double checked with Mohamed about your comments. The answer is yes we need a migration, but good news it's prepared! :) Will you try apply |
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
Test coverage for membership API
Related Issues
#727
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.