refactor: nest role fields in UserProjectMembership API response #1096
Conversation
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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. 📝 WalkthroughWalkthroughConsolidates role fields in membership serialization into a single structured object (id, name, description). Adds membership queryset filtering for memberships that have roles. Frontend types and hooks updated to expect a structured role object and a concrete ServerMember shape. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant View as API View
participant QS as with_role() Queryset
participant DB as Database
participant Serializer as Serializer
Client->>View: GET /projects/{id}/members
View->>QS: .select_related('user').with_role(log_invalid=True)
QS->>DB: Query memberships filtered by has_role / group names
DB-->>QS: memberships rows
QS-->>Serializer: membership instances
Serializer->>Serializer: get_role(): build {id, name, description}
Serializer-->>Client: JSON response with role object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. ✨ Finishing touches
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 |
Changed the UserProjectMembership API to return role data as a nested
object {"id": "...", "name": "...", "description": "..."} instead of
three flattened fields. This provides better API consistency with the
roles list endpoint and simplifies the frontend data model.
Changes:
- Backend: Updated serializers to return nested role object with null support
- Frontend: Updated TypeScript types to match new structure
- Frontend: Simplified data conversion in useMembers hook
- Frontend: Added null safety to UI components (manage access dialog, team columns)
- Tests: Updated assertions to verify nested structure
All tests passing. Write operations (role_id parameter) unchanged.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
mihow
force-pushed
the
feat/nest-role-fields-in-api
branch
from
d0da91b to
bf08d8a
Compare
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| image?: string | ||
| name: string | ||
| role: Role | ||
| role: Role | null |
There was a problem hiding this comment.
Can we not assume a member will have a role?
There was a problem hiding this comment.
It doesn't make logical sense in the application, but its technically possible since roles & project membership are maintained in two different systems (roles are built on the Django permission system, membership is our own system). One option could be to filter the members on the API side - only return ones that have a valid role.
Enforce business rule that all project members must have a role by filtering the API queryset and logging warnings for data inconsistencies. Changes: - Add UserProjectMembershipQuerySet.with_role() method to filter memberships with valid role assignments - Add Project.members_with_roles() convenience method for easy access - Update UserProjectMembershipViewSet to use new filtering method - Log warnings when memberships without roles are detected - Update serializer docstring to document the invariant - Fix test helper to assign default BasicMember role - Make Role non-nullable in TypeScript types - Remove defensive null checks from frontend components This ensures: - API responses always include valid role data - Data inconsistencies are logged for investigation - Frontend code is simpler without null checks - Tests create valid memberships with roles Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Restructures the UserProjectMembership API response to return role details as a nested object, and updates the UI data model to consume the new shape.
Changes:
- Backend serializers updated to return
roleas an object ({id, name, description}) instead of flattened role fields. - Membership view/query logic updated to filter memberships based on whether a role is assigned.
- Frontend member/role types and the
useMembersmapping updated to use the nestedroleobject; tests updated accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/data-services/models/role.ts | Updates Role type to change description nullability. |
| ui/src/data-services/models/member.ts | Defines a concrete ServerMember shape including nested role. |
| ui/src/data-services/hooks/team/useMembers.ts | Simplifies server→client mapping by directly assigning record.role. |
| ami/users/tests/test_membership_management_api.py | Updates API test assertions for nested role response fields. |
| ami/users/api/views.py | Adds role-based filtering (with_role) to the membership queryset. |
| ami/users/api/serializers.py | Removes flattened role fields and returns nested role object via get_role. |
| ami/main/models.py | Introduces UserProjectMembershipQuerySet.with_role() and a custom manager; adds members_with_roles(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_queryset(self): | ||
| project = self.get_active_project() | ||
| return UserProjectMembership.objects.filter(project=project).select_related("user") | ||
| return UserProjectMembership.objects.filter(project=project).select_related("user").with_role(log_invalid=True) |
There was a problem hiding this comment.
get_queryset() now applies .with_role(log_invalid=True), which filters out memberships without a role group. This makes those members disappear from list/retrieve/update/delete (e.g., an inconsistent membership becomes impossible to PATCH to fix its role because it won’t be found). If the intended behavior is “return role: null when missing”, remove this filter (or scope it to list only) and rely on serializer nulls + explicit remediation tooling.
| return UserProjectMembership.objects.filter(project=project).select_related("user").with_role(log_invalid=True) | |
| queryset = UserProjectMembership.objects.filter(project=project).select_related("user") | |
| if getattr(self, "action", None) == "list": | |
| queryset = queryset.with_role(log_invalid=True) | |
| return queryset |
There was a problem hiding this comment.
memberships without a role are invalid and will be fixed by signals o BE functions. also can be hand fixed in the Django admin
| queryset = self.annotate( | ||
| has_role=Exists( | ||
| Group.objects.filter( | ||
| user=OuterRef("user"), | ||
| name__startswith=models.functions.Concat(OuterRef("project_id"), models.Value("_")), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
with_role() checks for any group whose name starts with "{project_id}_", but Role.get_primary_role()/Role.has_role() rely on exact group names including project.name (Role.get_group_name). If a project is renamed (or groups drift), with_role() can treat a membership as having a role while get_primary_role() still returns None, defeating the “should never be None” assumption and skipping log_invalid. Consider making the predicate consistent with Role.get_group_name (or updating role group naming to be stable).
There was a problem hiding this comment.
🧩 Analysis chain
I need to examine the actual implementation of the role system to verify Copilot's concern. Let me search for the relevant code.
🏁 Script executed:
#!/bin/bash
# Find the Role class and group naming implementation
fd -e py roles.py --exec cat {} \; | head -200
Length of output: 7594
🏁 Script executed:
#!/bin/bash
# Search for get_group_name, has_role, and get_primary_role methods
rg -n -A 10 "def get_group_name|def has_role|def get_primary_role" --type=py
Length of output: 2217
🏁 Script executed:
#!/bin/bash
# Search for how groups are created/named in the role system
rg -n -B 3 -A 10 "Group\.objects\..*create|group.*name.*=.*project" --type=py | head -100
Length of output: 2946
@mihow Yes, Copilot's concern is valid. I can confirm the issue by examining the code:
The Problem
Group naming pattern (Line 25 in ami/users/roles.py):
return f"{project.pk}_{project.name}_{cls.__name__}"The inconsistency occurs when a project is renamed:
-
Original state: Project ID=5, name="Wildlife Project"
- Group created:
5_Wildlife Project_BasicMember
- Group created:
-
After rename: Project renamed to "Forest Animals"
- Old group still exists:
5_Wildlife Project_BasicMember with_role()matches it (only checks prefix5_*) ✅has_role()looks for5_Forest Animals_BasicMember(doesn't exist!) ❌
- Old group still exists:
-
Result: A membership passes
with_role()filtering butRole.get_primary_role()returnsNone, defeating the data consistency check.
Root Cause
The has_role() method (line 48) always uses the current project name:
group_name = cls.get_group_name(project) # Uses current project.name
return user.groups.filter(name=group_name).exists()While with_role() only checks the project ID prefix, making it tolerant of stale group names.
The TODO Comment
The TODO in the diff already acknowledges this is technical debt and proposes the proper solution: migrating to FK-based role-project associations instead of string-based group names.
Recommended Actions
- Short-term: Add a migration/management command to update group names when projects are renamed, or handle this in the
Project.save()method - Long-term: Implement the FK-based approach mentioned in the TODO comment
- Immediate: The current implementation could cause silent failures where users appear to have roles but permissions don't work
Would you like me to search for whether there's already any project rename handling logic in the codebase?
There was a problem hiding this comment.
I believe there is already a signal or function on the save method to update group names when projects are renamed, correct @mohamedelabbas1996?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| # Log invalid memberships if requested (before filtering them out) | ||
| if log_invalid: | ||
| invalid = queryset.filter(has_role=False).select_related("user", "project") | ||
| if invalid.exists(): | ||
| for membership in invalid: | ||
| logger.warning( | ||
| f"Data inconsistency detected: UserProjectMembership {membership.pk} " | ||
| f"for user '{membership.user.email}' in project '{membership.project.name}' " | ||
| f"(ID: {membership.project.pk}) has no role assigned. This indicates " | ||
| f"the permission groups are out of sync. " | ||
| f"Fix by running: python manage.py update_roles --project-id={membership.project.pk}" | ||
| ) |
There was a problem hiding this comment.
When log_invalid=True, this does an invalid.exists() query and then iterates invalid (second query) and logs one warning per row. In API request paths this can add overhead and generate noisy logs. Consider logging a single summary (count + project id) and/or avoiding the double-query pattern (e.g., iterate once or limit results).
There was a problem hiding this comment.
Hopefully this is a rare thing
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| Note: Due to queryset filtering in UserProjectMembershipViewSet.get_queryset(), | ||
| this method should never return None in API responses. However, we maintain | ||
| the None check for safety (e.g., when called outside API context). | ||
| """ |
There was a problem hiding this comment.
This docstring claims the method “should never return None” due to queryset filtering, but get_role() still returns None if Role.get_primary_role() can’t resolve a primary role (and current with_role() doesn’t guarantee that). Please either enforce the invariant (and adjust filtering accordingly) or update the comment to avoid misleading future callers.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the with_role() implementation in UserProjectMembershipQuerySet
ast-grep --pattern 'def with_role($$$)'
# Find and examine the Role.get_primary_role() implementation
ast-grep --pattern 'def get_primary_role($$$)'
# Find and examine the ViewSet's get_queryset() method
rg -n -A 10 'class UserProjectMembershipViewSet' ami/users/api/views.py
rg -n -A 10 'def get_queryset' ami/users/api/views.py
Length of output: 6153
@mihow Yes, Copilot's concern is valid. There's a subtle but important mismatch between the filtering logic.
The issue:
-
with_role()filtering (line 553 in ami/main/models.py):- Uses a prefix match: checks if user has ANY group where
name__startswith={project_id}_
- Uses a prefix match: checks if user has ANY group where
-
get_primary_role()resolution (ami/users/roles.py line 86):- Calls
get_user_roles()which iterates throughRole.__subclasses__() - For each role class, calls
has_role()which checks for an exact match:{project_id}_{project_name}_{RoleClassName}
- Calls
Scenarios where they diverge:
-
Project renamed: A group
123_OldProjectName_BasicMemberexists, but the project is now calledNewProjectName. The prefix check passes, buthas_role()looks for123_NewProjectName_BasicMember(no match). -
Deleted role class: A group
123_ProjectName_ObsoleteRoleexists, butObsoleteRolewas removed from the codebase. The prefix check passes, butRole.__subclasses__()won't include it. -
Malformed group name: A group
123_garbageexists. The prefix check passes, but no role class will match it.
In all these cases, with_role() includes the membership in the queryset, but get_role() returns None.
Recommendation:
Either:
- Update the docstring to acknowledge that
Noneis possible despite the filtering, or - Strengthen
with_role()to verify that the group name matches an actual role pattern (more complex but enforces the invariant)
The current docstring creates a false sense of security for future developers.
🧬 Code Graph Analysis Results
ami/users/models.py
<line_range>10-44</line_range>
class User(AbstractUser):
"""
Default custom user model for Automated Monitoring of Insects ML Platform.
If adding fields that need to be filled at user signup,
check forms.SignupForm and forms.SocialSignupForms accordingly.
"""
# First and last name do not cover name patterns around the globe
name = CharField(_("Name of User"), blank=True, max_length=255)
first_name = None # type: ignore
last_name = None # type: ignore
email = EmailField(_("email address"), unique=True)
username = None # type: ignore
image = models.ImageField(upload_to="users", blank=True, null=True)
USERNAME_FIELD = "email"
REQUIRED_FIELDS = []
objects = UserManager()
def save(self, *args, **kwargs):
if not self.email:
raise ValueError("The Email field must be set")
self.email = UserManager.normalize_email(self.email)
super().save(*args, **kwargs)
def get_absolute_url(self) -> str:
"""Get URL for user's detail view.
Returns:
str: URL for user detail.
"""
# `@TODO` return frontend URL, not API URL
return reverse("api:user-detail", kwargs={"id": self.pk})
ami/main/models.py
<line_range>581-622</line_range>
class UserProjectMembership(BaseModel):
"""
Through model connecting User <-> Project.
This model represents membership ONLY.
Role assignment is handled separately via permission groups.
"""
user = models.ForeignKey(
User,
on_delete=models.CASCADE,
related_name="project_memberships",
)
project = models.ForeignKey(
"main.Project",
on_delete=models.CASCADE,
related_name="project_memberships",
)
# Add custom manager
objects = UserProjectMembershipManager()
def check_permission(self, user: AbstractUser | AnonymousUser, action: str) -> bool:
project = self.project
# Allow viewing membership details if the user has view permission on the project
if action == "retrieve":
return user.has_perm(Project.Permissions.VIEW_USER_PROJECT_MEMBERSHIP, project)
# Allow users to delete their own membership
if action == "destroy" and user == self.user:
return True
return super().check_permission(user, action)
def get_user_object_permissions(self, user) -> list[str]:
# Return delete permission if user is the same as the membership user
user_permissions = super().get_user_object_permissions(user)
if user == self.user:
if "delete" not in user_permissions:
user_permissions.append("delete")
return user_permissions
class Meta:
unique_together = ("user", "project")
ami/users/roles.py
<line_range>12-89</line_range>
class Role:
"""Base class for all roles."""
display_name = ""
description = ""
permissions = {Project.Permissions.VIEW_PROJECT}
# `@TODO` : Refactor after adding the project <-> Group formal relationship
`@classmethod`
def get_group_name(cls, project):
"""
Construct the name of the group that manages a role for a given project.
"""
return f"{project.pk}_{project.name}_{cls.__name__}"
`@classmethod`
def assign_user(cls, user, project):
# Get or create the Group
# `@TODO` Make the relationship between the group and the project more formal (use a many-to-many field)
group_name = cls.get_group_name(project)
group, created = Group.objects.get_or_create(name=group_name)
if created:
logger.info(f"Created permission group {group_name} for project {project}")
# Add user to group
user.groups.add(group)
`@classmethod`
def unassign_user(cls, user, project):
group_name = cls.get_group_name(project)
group = Group.objects.get(name=group_name)
# remove user from group
user.groups.remove(group)
`@classmethod`
def has_role(cls, user, project):
"""Checks if the user has the role permissions on the given project."""
group_name = cls.get_group_name(project)
return user.groups.filter(name=group_name).exists()
`@staticmethod`
def user_has_any_role(user, project):
"""Checks if the user has any role assigned to a given project."""
return any(role_class.has_role(user, project) for role_class in Role.__subclasses__())
`@staticmethod`
def get_supported_roles():
"""
Returns all supported role classes in the system.
"""
return list(Role.__subclasses__())
`@staticmethod`
def get_user_roles(project, user):
"""
Returns the names of roles assigned to a user for a specific project.
Or empty list if no role is found.
"""
user_roles = []
for role_cls in Role.__subclasses__():
if role_cls.has_role(user, project):
user_roles.append(role_cls)
return user_roles
`@staticmethod`
def get_primary_role(project, user):
"""
Return the role class with the most permissions for a user on a project.
In practice, a user should only have one role per project, but in case of multiple roles,
we return the one with the most permissions.
The original design allowed multiple roles per user per project, but it was later decided to
that from a UX and management perspective, a single role per user per project is preferable.
"""
roles = Role.get_user_roles(project, user)
if not roles:
return None
return max(roles, key=lambda r: len(r.permissions))
ami/base/serializers.py
<line_range>42-59</line_range>
class DefaultSerializer(serializers.HyperlinkedModelSerializer):
url_field_name = "details"
id = serializers.IntegerField(read_only=True)
def get_permissions(self, instance, instance_data):
request: Request = self.context["request"]
user = request.user
return add_object_level_permissions(
user=user,
instance=instance,
response_data=instance_data,
)
def to_representation(self, instance):
instance_data = super().to_representation(instance)
instance_data = self.get_permissions(instance=instance, instance_data=instance_data)
return instance_data
ami/base/serializers.py
<line_range>64-71</line_range>
class DefaultSerializer(serializers.HyperlinkedModelSerializer):
url_field_name = "details"
id = serializers.IntegerField(read_only=True)
def get_permissions(self, instance, instance_data):
request: Request = self.context["request"]
user = request.user
return add_object_level_permissions(
user=user,
instance=instance,
response_data=instance_data,
)
<no_relevant_code_snippets>
![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
🤖 Fix all issues with AI agents
In `@ami/main/models.py`:
- Around line 559-569: The warning logs currently include user emails (PII);
update the logger call in the block that iterates invalid =
queryset.filter(has_role=False).select_related("user", "project") to avoid
logging membership.user.email and instead log a non-PII identifier such as
membership.user.pk (or a hashed/obfuscated value) and keep project info; modify
the logger.warning invocation (referencing membership, membership.user,
membership.project) to remove the email and replace it with the chosen non-PII
identifier and retain the existing context and remediation message.
| if log_invalid: | ||
| invalid = queryset.filter(has_role=False).select_related("user", "project") | ||
| if invalid.exists(): | ||
| for membership in invalid: | ||
| logger.warning( | ||
| f"Data inconsistency detected: UserProjectMembership {membership.pk} " | ||
| f"for user '{membership.user.email}' in project '{membership.project.name}' " | ||
| f"(ID: {membership.project.pk}) has no role assigned. This indicates " | ||
| f"the permission groups are out of sync. " | ||
| f"Fix by running: python manage.py update_roles --project-id={membership.project.pk}" | ||
| ) |
There was a problem hiding this comment.
Avoid logging user emails in warnings.
User emails are PII; logging them can violate privacy/compliance expectations. Prefer user_id (or a hashed/obfuscated value) in logs.
🔐 Suggested redaction
- f"Data inconsistency detected: UserProjectMembership {membership.pk} "
- f"for user '{membership.user.email}' in project '{membership.project.name}' "
+ f"Data inconsistency detected: UserProjectMembership {membership.pk} "
+ f"for user_id={membership.user_id} in project '{membership.project.name}' "
f"(ID: {membership.project.pk}) has no role assigned. This indicates "🤖 Prompt for AI Agents
In `@ami/main/models.py` around lines 559 - 569, The warning logs currently
include user emails (PII); update the logger call in the block that iterates
invalid = queryset.filter(has_role=False).select_related("user", "project") to
avoid logging membership.user.email and instead log a non-PII identifier such as
membership.user.pk (or a hashed/obfuscated value) and keep project info; modify
the logger.warning invocation (referencing membership, membership.user,
membership.project) to remove the email and replace it with the chosen non-PII
identifier and retain the existing context and remediation message.
There was a problem hiding this comment.
@mihow, thank you for acknowledging this! Using membership.user_id instead of the email will help keep logs compliant with privacy best practices.
Code reviewFound 2 issues:
The TypeScript type declares antenna/ui/src/data-services/models/role.ts Lines 3 to 5 in c60c2a7 Compare with backend:
The antenna/ami/users/api/views.py Lines 37 to 42 in c60c2a7 Consider adding 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@annavik I updated the API (and other queries) to never return Members that have a |
Summary
This PR restructures the
UserProjectMembershipAPI to return role data as a nested object instead of three flattened fields, improving API consistency and simplifying the frontend data model.Changes
Backend:
UserProjectMembershipSerializerandUserProjectMembershipListSerializerto return role as nested object{"id": "...", "name": "...", "description": "..."}nullwhen no role assigned)role_idFrontend:
ServerMemberandMemberwith proper role typingRole.descriptionrequired (always present when role exists)useMembershook (direct assignment instead of manual object construction)Tests:
API Response Example
Before:
```json
{
"role": "ProjectManager",
"role_display_name": "Project Manager",
"role_description": "Manages project settings..."
}
```
After:
```json
{
"role": {
"id": "ProjectManager",
"name": "Project Manager",
"description": "Manages project settings..."
}
}
```
Risk Assessment
Low Risk:
role_id)Test Plan
Built with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.