[shaohuzhang1]

refactor: enhance user role management with admin role checks and batch processing improvements

[shaohuzhang1]

Here are my observations and suggestions:

1. Correctness / Logic Issues

a) Inconsistent is_admin semantics between the two functions

• In _get_user_roles, is_admin is a parameter that means "the caller is an admin" (controls filtering).
• At line ~506, is_admin is reassigned to mean "this particular user is a builtin admin" — then passed into _get_user_roles as the first function's is_admin param.

This works only if the two meanings happen to align (i.e., you're processing a list of users and use one user's admin status to decide visibility for all of them). But since user_ids here is a list of many users, checking is_admin for a single user_id and using it to control visibility for the whole batch is a latent bug:

is_admin = user_role_relation_model.objects.filter(user_id=user_id, ...).exists()
...
_get_user_roles(user_ids, is_admin=is_admin)

• Problem: _get_user_roles will suppress builtin-admin roles for all users in the batch if the single user_id isn't a builtin admin — meaning actual admins in the list will show no admin role.
• Fix: Filter per-user inside _get_user_roles, or compute a set of admin user IDs and pass that in, e.g.:

  admin_user_ids = set(user_role_relation_model.objects.filter(
      user_id__in=user_ids,
      role__type=RoleConstants.ADMIN.name,
      role__internal=True,
  ).values_list("user_id", flat=True))

  Then inside the loop: if not is_admin and str(relation.user_id) not in admin_user_ids and is_builtin_admin_role: continue.

b) role.internal vs role__internal

• The added code assumes a field named internal exists on the Role model. Verify the attribute name matches your schema (internal, is_internal, etc.). If it's is_internal, both places need updating.

c) relation.role.internal may cause N+1 queries

• In the loop, relation.role.internal and relation.role.type each trigger lazy loads unless you had .select_related("role"). Confirm _get_user_roles uses select_related("role"); otherwise adding another relation.role.X access increases query count. Prefer prefetch_related/select_related.

2. Variable Shadowing (fixed, but note context)

The rename user_id → record_user_id in the final block is good cleanup — but double-check there wasn't previously intended reuse. Note the outer user_id now retains the admin-check value. Consider renaming the outer one too (e.g., current_user_id / request_user_id) to avoid confusion with the loop variable user["id"].

3. Style / Minor

• Comment says "非内置系统管理员只能看到除内置系统管理员之外的角色" — good, but make sure the condition matches: it skips if not is_admin AND is_builtin_admin_role. That's "non-admin callers don't see builtin-admin roles." Consistent. ✅
• Consider extracting is_builtin_admin_role logic into a helper/mixin (role.is_builtin_admin()) since it's repeated conceptually in both places (DB filter + object check). This avoids drift if RoleConstants changes.

4. Optimization

• Replace the single-user .exists() query with the batched values_list(...) query above — fewer queries and fixes the batching bug simultaneously.
• Ensure both role__type and role__internal filters have DB indexes (or a composite index), especially if user_role_relation_model is large.

Summary of key fix

The main irregularity is using a single user's is_admin to gate role visibility for a batch of users. Make admin determination per-user (pass a set of admin IDs), and verify role.internal is the correct field name and that role is select_related to avoid N+1.