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 cd097d7ef..0c3fea3e4 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): @@ -158,22 +152,23 @@ def validate_role_id(self, value): return value 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 + """ + Get the primary role for this membership. - def get_role_description(self, obj): + 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) - 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 +202,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 +209,6 @@ class Meta: "id", "user", "role", - "role_display_name", - "role_description", "created_at", "updated_at", ] 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 a3e9010e5..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): @@ -112,7 +122,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..b4a543483 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 + updated_at: string + user: { + id: string + name: string + email: string + image?: string + } + user_permissions: UserPermission[] +} export type Member = { addedAt: Date 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 }