From bf08d8ab3a305e9db41efabd0c15b9b429c3771e Mon Sep 17 00:00:00 2001
From: Michael Bunsen <notbot@gmail.com>
Date: Wed, 21 Jan 2026 23:56:49 -0800
Subject: [PATCH 1/3] refactor: nest role fields in UserProjectMembership API
 response

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>
---
 ami/users/api/serializers.py                  | 30 +++++--------------
 .../tests/test_membership_management_api.py   |  4 ++-
 ui/src/data-services/hooks/team/useMembers.ts |  6 +---
 ui/src/data-services/models/member.ts         | 17 +++++++++--
 ui/src/data-services/models/role.ts           |  2 +-
 .../project/team/manage-access-dialog.tsx     |  4 +--
 ui/src/pages/project/team/team-columns.tsx    |  4 +--
 7 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/ami/users/api/serializers.py b/ami/users/api/serializers.py
index cd097d7ef..c80492786 100644
--- a/ami/users/api/serializers.py
+++ b/ami/users/api/serializers.py
@@ -109,8 +109,6 @@ class UserProjectMembershipSerializer(DefaultSerializer):
 
     user = MemberUserSerializer(read_only=True)
     role = serializers.SerializerMethodField(read_only=True)
-    role_display_name = serializers.SerializerMethodField(read_only=True)
-    role_description = serializers.SerializerMethodField(read_only=True)
 
     class Meta:
         model = UserProjectMembership
@@ -121,8 +119,6 @@ class Meta:
             "user",
             "project",
             "role",
-            "role_display_name",
-            "role_description",
             "created_at",
             "updated_at",
         ]
@@ -132,8 +128,6 @@ class Meta:
             "created_at",
             "updated_at",
             "role",
-            "role_display_name",
-            "role_description",
         ]
 
     def validate_email(self, value):
@@ -161,19 +155,13 @@ 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
+        if role_cls is None:
+            return None
+        return {
+            "id": role_cls.__name__,
+            "name": role_cls.display_name,
+            "description": role_cls.description,
+        }
 
     def validate(self, attrs):
         project = self.context["project"]
@@ -207,8 +195,6 @@ def validate(self, attrs):
 class UserProjectMembershipListSerializer(UserProjectMembershipSerializer):
     user = MemberUserSerializer(read_only=True)
     role = serializers.SerializerMethodField()
-    role_display_name = serializers.SerializerMethodField()
-    role_description = serializers.SerializerMethodField()
 
     class Meta:
         model = UserProjectMembership
@@ -216,8 +202,6 @@ class Meta:
             "id",
             "user",
             "role",
-            "role_display_name",
-            "role_description",
             "created_at",
             "updated_at",
         ]
diff --git a/ami/users/tests/test_membership_management_api.py b/ami/users/tests/test_membership_management_api.py
index a3e9010e5..53e7b6f80 100644
--- a/ami/users/tests/test_membership_management_api.py
+++ b/ami/users/tests/test_membership_management_api.py
@@ -112,7 +112,9 @@ def test_update_membership_functionality(self):
         self.assertEqual(resp.status_code, 200)
 
         updated = resp.json()
-        self.assertEqual(updated["role"], ProjectManager.__name__)
+        self.assertEqual(updated["role"]["id"], ProjectManager.__name__)
+        self.assertEqual(updated["role"]["name"], ProjectManager.display_name)
+        self.assertEqual(updated["role"]["description"], ProjectManager.description)
 
         membership = UserProjectMembership.objects.get(
             project=self.project,
diff --git a/ui/src/data-services/hooks/team/useMembers.ts b/ui/src/data-services/hooks/team/useMembers.ts
index 0154bbe0d..3afd07c30 100644
--- a/ui/src/data-services/hooks/team/useMembers.ts
+++ b/ui/src/data-services/hooks/team/useMembers.ts
@@ -14,11 +14,7 @@ const convertServerRecord = (record: ServerMember): Member => ({
   id: `${record.id}`,
   image: record.user.image,
   name: record.user.name,
-  role: {
-    description: record.role_description,
-    id: record.role,
-    name: record.role_display_name,
-  },
+  role: record.role,
   updatedAt: record.updated_at ? new Date(record.updated_at) : undefined,
   userId: `${record.user.id}`,
 })
diff --git a/ui/src/data-services/models/member.ts b/ui/src/data-services/models/member.ts
index be3d6f31a..e7dd88f30 100644
--- a/ui/src/data-services/models/member.ts
+++ b/ui/src/data-services/models/member.ts
@@ -1,6 +1,19 @@
 import { Role } from './role'
+import { UserPermission } from 'utils/user/types'
 
-export type ServerMember = any // TODO: Update this type
+export type ServerMember = {
+  created_at: string
+  id: string
+  role: Role | null
+  updated_at: string
+  user: {
+    id: string
+    name: string
+    email: string
+    image?: string
+  }
+  user_permissions: UserPermission[]
+}
 
 export type Member = {
   addedAt: Date
@@ -10,7 +23,7 @@ export type Member = {
   id: string
   image?: string
   name: string
-  role: Role
+  role: Role | null
   updatedAt?: Date
   userId: string
 }
diff --git a/ui/src/data-services/models/role.ts b/ui/src/data-services/models/role.ts
index b51e42444..cd636467b 100644
--- a/ui/src/data-services/models/role.ts
+++ b/ui/src/data-services/models/role.ts
@@ -1,5 +1,5 @@
 export type Role = {
   name: string
   id: string
-  description?: string
+  description: string
 }
diff --git a/ui/src/pages/project/team/manage-access-dialog.tsx b/ui/src/pages/project/team/manage-access-dialog.tsx
index 76ef5f35d..54a6082d6 100644
--- a/ui/src/pages/project/team/manage-access-dialog.tsx
+++ b/ui/src/pages/project/team/manage-access-dialog.tsx
@@ -19,7 +19,7 @@ import { RolesPicker } from './roles-picker'
 export const ManageAccessDialog = ({ member }: { member: Member }) => {
   const { projectId } = useParams()
   const [isOpen, setIsOpen] = useState(false)
-  const [roleId, setRoleId] = useState<string>(member.role.id)
+  const [roleId, setRoleId] = useState<string>(member.role?.id ?? '')
   const { updateMember, isLoading, isSuccess, error } = useUpdateMember(
     projectId as string,
     member.id
@@ -27,7 +27,7 @@ export const ManageAccessDialog = ({ member }: { member: Member }) => {
   const errorMessage = useFormError({ error })
 
   // Reset form on open state change
-  useEffect(() => setRoleId(member.role.id), [isOpen, member])
+  useEffect(() => setRoleId(member.role?.id ?? ''), [isOpen, member])
 
   return (
     <Dialog.Root open={isOpen} onOpenChange={setIsOpen}>
diff --git a/ui/src/pages/project/team/team-columns.tsx b/ui/src/pages/project/team/team-columns.tsx
index c9bc313c2..c20ae1d55 100644
--- a/ui/src/pages/project/team/team-columns.tsx
+++ b/ui/src/pages/project/team/team-columns.tsx
@@ -48,8 +48,8 @@ export const columns: (userId?: string) => TableColumn<Member>[] = (
     renderCell: (item: Member) => (
       <BasicTableCell>
         <div className="flex items-center gap-2">
-          <span>{item.role.name}</span>
-          {item.role.description ? (
+          <span>{item.role?.name ?? ''}</span>
+          {item.role?.description ? (
             <BasicTooltip asChild content={item.role.description}>
               <Button
                 aria-label={translate(STRING.ABOUT_ROLE)}

From c60c2a7a711a4c2df52c918ca0a7c673cc07f23f Mon Sep 17 00:00:00 2001
From: Michael Bunsen <notbot@gmail.com>
Date: Thu, 22 Jan 2026 15:49:18 -0800
Subject: [PATCH 2/3] feat: filter members without roles from API

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>
---
 ami/main/models.py                            | 92 +++++++++++++++++++
 ami/users/api/serializers.py                  |  7 ++
 ami/users/api/views.py                        |  2 +-
 .../tests/test_membership_management_api.py   | 16 +++-
 ui/src/data-services/models/member.ts         |  4 +-
 ui/src/data-services/models/role.ts           |  2 +-
 .../project/team/manage-access-dialog.tsx     |  4 +-
 ui/src/pages/project/team/team-columns.tsx    |  4 +-
 8 files changed, 120 insertions(+), 11 deletions(-)

diff --git a/ami/main/models.py b/ami/main/models.py
index abeab6424..bd1993469 100644
--- a/ami/main/models.py
+++ b/ami/main/models.py
@@ -274,6 +274,23 @@ def ensure_owner_membership(self):
         if self.owner and not self.members.filter(id=self.owner.pk).exists():
             self.members.add(self.owner)
 
+    def members_with_roles(self):
+        """
+        Get users who are members of this project with a role assigned.
+
+        This filters out memberships where the user has no role groups assigned,
+        which indicates a data inconsistency.
+
+        Returns:
+            User queryset filtered to users with roles
+
+        Example:
+            project.members_with_roles()
+            project.members_with_roles().values_list('email', flat=True)
+        """
+        memberships_with_roles = self.project_memberships.with_role()
+        return User.objects.filter(project_memberships__in=memberships_with_roles).distinct()
+
     def deployments_count(self) -> int:
         return self.deployments.count()
 
@@ -489,6 +506,78 @@ class Meta:
         ]
 
 
+class UserProjectMembershipQuerySet(BaseQuerySet):
+    """Custom queryset for UserProjectMembership with role filtering."""
+
+    def with_role(self, log_invalid=False):
+        """
+        Filter memberships to only include users who have a role assigned.
+
+        Memberships without roles indicate a data inconsistency between the
+        UserProjectMembership table and permission groups (roles).
+
+        Args:
+            log_invalid: If True, log warnings for memberships without roles
+
+        Returns:
+            Queryset filtered to members with at least one role for their project
+
+        Example:
+            # In a ViewSet:
+            UserProjectMembership.objects.filter(project=project).with_role(log_invalid=True)
+
+            # In a management command:
+            UserProjectMembership.objects.filter(project=project).with_role()
+        """
+        from django.contrib.auth.models import Group
+
+        # TODO: Once we migrate to FK-based role-project associations (custom Group model
+        # with project_id FK), this method should be updated to use a direct FK join:
+        #
+        # has_role=Exists(
+        #     Group.objects.filter(
+        #         user=OuterRef('user'),
+        #         project_id=OuterRef('project_id')  # Direct FK instead of string parsing
+        #     )
+        # )
+        #
+        # This will be more efficient and eliminate the need for name prefix matching.
+        # Related refactor: Remove string-based group naming pattern in ami/users/roles.py:24
+        # Subquery: Check if user has ANY group for their project
+        # Groups follow naming pattern: {project_id}_{project_name}_{RoleClassName}
+        # See: ami/users/roles.py:24 (Role.get_group_name)
+        queryset = self.annotate(
+            has_role=Exists(
+                Group.objects.filter(
+                    user=OuterRef("user"),
+                    name__startswith=models.functions.Concat(OuterRef("project_id"), models.Value("_")),
+                )
+            )
+        )
+
+        # 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}"
+                    )
+
+        # Return only members with valid roles
+        return queryset.filter(has_role=True)
+
+
+class UserProjectMembershipManager(models.Manager.from_queryset(UserProjectMembershipQuerySet)):
+    """Custom manager for UserProjectMembership."""
+
+    pass
+
+
 class UserProjectMembership(BaseModel):
     """
     Through model connecting User <-> Project.
@@ -508,6 +597,9 @@ class UserProjectMembership(BaseModel):
         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
diff --git a/ami/users/api/serializers.py b/ami/users/api/serializers.py
index c80492786..0c3fea3e4 100644
--- a/ami/users/api/serializers.py
+++ b/ami/users/api/serializers.py
@@ -152,6 +152,13 @@ def validate_role_id(self, value):
         return value
 
     def get_role(self, obj):
+        """
+        Get the primary role for this membership.
+
+        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).
+        """
         from ami.users.roles import Role
 
         role_cls = Role.get_primary_role(obj.project, obj.user)
diff --git a/ami/users/api/views.py b/ami/users/api/views.py
index aac43b5f9..880ae683f 100644
--- a/ami/users/api/views.py
+++ b/ami/users/api/views.py
@@ -36,7 +36,7 @@ class UserProjectMembershipViewSet(DefaultViewSet, ProjectMixin):
 
     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)
 
     def get_serializer_class(self):
         if self.action == "list":
diff --git a/ami/users/tests/test_membership_management_api.py b/ami/users/tests/test_membership_management_api.py
index 53e7b6f80..11c5f252e 100644
--- a/ami/users/tests/test_membership_management_api.py
+++ b/ami/users/tests/test_membership_management_api.py
@@ -22,18 +22,28 @@ def setUp(self):
         self.roles_url = "/api/v2/users/roles/"
         self.members_url = f"/api/v2/projects/{self.project.pk}/members/"
 
-    def create_membership(self, user=None):
+    def create_membership(self, user=None, role_cls=None):
         """
-        Create a membership for a user in this project.
-        Used in tests to guarantee isolation.
+        Create a membership for a user in this project with a role assigned.
+
+        Args:
+            user: User to add as member (defaults to self.user1)
+            role_cls: Role class to assign (defaults to BasicMember)
+
+        Returns:
+            UserProjectMembership instance with role assigned
         """
         if user is None:
             user = self.user1
+        if role_cls is None:
+            role_cls = BasicMember  # Default role for test memberships
 
         membership = UserProjectMembership.objects.create(
             project=self.project,
             user=user,
         )
+        # Assign role to ensure membership is valid
+        role_cls.assign_user(user, self.project)
         return membership
 
     def auth_super(self):
diff --git a/ui/src/data-services/models/member.ts b/ui/src/data-services/models/member.ts
index e7dd88f30..b4a543483 100644
--- a/ui/src/data-services/models/member.ts
+++ b/ui/src/data-services/models/member.ts
@@ -4,7 +4,7 @@ import { UserPermission } from 'utils/user/types'
 export type ServerMember = {
   created_at: string
   id: string
-  role: Role | null
+  role: Role
   updated_at: string
   user: {
     id: string
@@ -23,7 +23,7 @@ export type Member = {
   id: string
   image?: string
   name: string
-  role: Role | null
+  role: Role
   updatedAt?: Date
   userId: string
 }
diff --git a/ui/src/data-services/models/role.ts b/ui/src/data-services/models/role.ts
index cd636467b..a906778a8 100644
--- a/ui/src/data-services/models/role.ts
+++ b/ui/src/data-services/models/role.ts
@@ -1,5 +1,5 @@
 export type Role = {
   name: string
   id: string
-  description: string
+  description: string | null
 }
diff --git a/ui/src/pages/project/team/manage-access-dialog.tsx b/ui/src/pages/project/team/manage-access-dialog.tsx
index 54a6082d6..76ef5f35d 100644
--- a/ui/src/pages/project/team/manage-access-dialog.tsx
+++ b/ui/src/pages/project/team/manage-access-dialog.tsx
@@ -19,7 +19,7 @@ import { RolesPicker } from './roles-picker'
 export const ManageAccessDialog = ({ member }: { member: Member }) => {
   const { projectId } = useParams()
   const [isOpen, setIsOpen] = useState(false)
-  const [roleId, setRoleId] = useState<string>(member.role?.id ?? '')
+  const [roleId, setRoleId] = useState<string>(member.role.id)
   const { updateMember, isLoading, isSuccess, error } = useUpdateMember(
     projectId as string,
     member.id
@@ -27,7 +27,7 @@ export const ManageAccessDialog = ({ member }: { member: Member }) => {
   const errorMessage = useFormError({ error })
 
   // Reset form on open state change
-  useEffect(() => setRoleId(member.role?.id ?? ''), [isOpen, member])
+  useEffect(() => setRoleId(member.role.id), [isOpen, member])
 
   return (
     <Dialog.Root open={isOpen} onOpenChange={setIsOpen}>
diff --git a/ui/src/pages/project/team/team-columns.tsx b/ui/src/pages/project/team/team-columns.tsx
index c20ae1d55..c9bc313c2 100644
--- a/ui/src/pages/project/team/team-columns.tsx
+++ b/ui/src/pages/project/team/team-columns.tsx
@@ -48,8 +48,8 @@ export const columns: (userId?: string) => TableColumn<Member>[] = (
     renderCell: (item: Member) => (
       <BasicTableCell>
         <div className="flex items-center gap-2">
-          <span>{item.role?.name ?? ''}</span>
-          {item.role?.description ? (
+          <span>{item.role.name}</span>
+          {item.role.description ? (
             <BasicTooltip asChild content={item.role.description}>
               <Button
                 aria-label={translate(STRING.ABOUT_ROLE)}

From a55dbdc81ef7432c82bba3e3fcf44c1889f66f4a Mon Sep 17 00:00:00 2001
From: Michael Bunsen <notbot@gmail.com>
Date: Fri, 23 Jan 2026 23:09:57 -0800
Subject: [PATCH 3/3] description cannot be null (but can be a blank string)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---
 ui/src/data-services/models/role.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/src/data-services/models/role.ts b/ui/src/data-services/models/role.ts
index a906778a8..cd636467b 100644
--- a/ui/src/data-services/models/role.ts
+++ b/ui/src/data-services/models/role.ts
@@ -1,5 +1,5 @@
 export type Role = {
   name: string
   id: string
-  description: string | null
+  description: string
 }