[shaohuzhang1]

refactor: simplify role visibility logic for non-admin users 【【用户管理】用户包含角色为继承系统管理员时，角色列可以看见继承系统管理员的角色，用户详情中的角色设置仍可见继承系统管理员】 https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001079300

[shaohuzhang1]

Here is the code review, highlighting potential issues and optimization suggestions:

1. Logic Inconsistency & Potential Bug

Issue: The logic for filtering "ADMIN" roles is inconsistent between _get_user_roles and update_user_role.

• In _get_user_roles: You previously filtered based on role.internal == True AND role.type == ADMIN. Now you filter based only on role.type == ADMIN. This means if there are non-internal roles with type ADMIN, they are now also hidden from non-admins. Ensure this change in behavior is intentional.
• In update_user_role: The determination of is_admin checks if the user has a role that is ADMIN type AND internal=True. However, when saving (deleting) mappings, it removes any role where role__type != ADMIN.name.
  • Risk: If a user is not an internal admin but has a valid, assigned "Admin-like" role (type=ADMIN but not internal), is_admin will be False. The code will then delete mappings excluding role__type=ADMIN. This effectively preserves the non-internal admin role assignments. This seems correct based on your comment, but it relies heavily on the distinction between type and internal. Ensure your data model consistently enforces that only system admins use type=ADMIN or explicitly handle the case where custom roles exist with type=ADMIN.

2. N+1 Query Problem in _get_user_roles

Issue: The snippet shows accessing relation.role.internal and relation.role.type. If relation.role is accessed via a loop without select_related, this triggers a database query for every relation.
Suggestion: Ensure the queryset that generates relation uses .select_related('role').

3. Hardcoded User ID

Issue:

if str(user.id) == "f0dd8f71-e4ee-11ee-8c84-a8a1595801ab":

Using hardcoded UUIDs is bad practice. It makes the code brittle, difficult to maintain, and hard to understand why this specific user is treated differently.
Suggestion: Use a constant with a descriptive name (e.g., DEFAULT_SYSTEM_ADMIN_ID) or a configuration flag/setting. Ideally, determine superuser status via user.is_superuser or a similar generic flag unless this specific ID serves a very unique migration legacy purpose.

4. Ambiguous Naming / Magic Strings

Issue: RoleConstants.ADMIN.name likely evaluates to the string "ADMIN".

• In _get_user_roles: relation.role.type == RoleConstants.ADMIN.name
• In update_user_role: role_id=RoleConstants.ADMIN.name
  Question: Are you comparing role.type (usually a string category like 'BUILTIN' vs 'CUSTOM'?) with role.role_name?
  If role_id actually maps to the Role Identifier UUID or Name, ensure you are querying the correct field. Comparing role_id to a name string is suspicious unless role_id is genuinely stored as that string. If role_id is a ForeignKey ID, this query will fail or cause a type mismatch error. role__type is a safer check if you are looking for role category.

5. Race Condition in update_user_role

Issue:

if is_admin:
    ...delete()
else:
    ...exclude(...).delete()

Deleting all mappings for user.id can be dangerous if other concurrent processes are adding/updating roles. This is not atomic.
Suggestion: Instead of "Delete All Then Add," consider using bulk_create with ignore_conflicts or performing deletions based on precise IDs of the old mappings you are replacing. Alternatively, wrap the operation in a transaction.atomic() block with appropriate locking if concurrency is a concern.

6. Optimization Suggestion

In update_user_role, the workspace_user_role_mapping_model.objects.filter(...).exists() call to determine is_admin might be slightly inefficient if you need the full role object later. If you don't, .exists() is fine. However, ensure there are proper database indexes on (user_id, role_id) and (user_id, role__type) to speed up the filter/delete operations.

---

Summary of Recommended Actions:

1. Confirm the hardcoded UUID usage intent; refactor if possible.
2. Verify that role_id=RoleConstants.ADMIN.name targets the correct database column (ID vs Name).
3. Add .select_related('role') to the queryset feeding into _get_user_roles.
4. Double-check the semantic consistency of role.type filtering: does "ADMIN" strictly mean "System Builtin Admin" in your schema?