From 2484d4efb3f21b4714df7e56f1431c61e9d5b736 Mon Sep 17 00:00:00 2001 From: catborise Date: Wed, 4 Nov 2026 21:22:46 +0300 Subject: [PATCH] perf(admin): the Users page tells who can clone in one query Each row of the Users page asked has_perm whether its user may clone: two queries a user (the user's permissions and the groups'). The list's query now annotates it with one Exists over the clone permission, given to the user or to a group of the user; a blocked user is shown without it, as has_perm did. The page's query count no longer grows with the users. --- admin/templates/admin/user_list.html | 6 ++---- admin/tests/test_views.py | 28 +++++++++++++++++++++++++++- admin/views.py | 12 +++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/admin/templates/admin/user_list.html b/admin/templates/admin/user_list.html index 5d4c35fa..61810df9 100644 --- a/admin/templates/admin/user_list.html +++ b/admin/templates/admin/user_list.html @@ -1,7 +1,6 @@ {% extends "base.html" %} {% load i18n %} {% load static %} -{% load common_tags %} {% load bootstrap_icons %} {% block title %}{{ title }}{% endblock %} @@ -22,7 +21,6 @@
{% for user in users %} - {% has_perm user 'instances.clone_instances' as can_clone %}
@@ -46,11 +44,11 @@
{% for group in user.groups.all %}{{ group.name }}{% endfor %} {# a superuser has every permission: only a granted clone permission is news #} - {% if can_clone and not user.is_superuser %}{% trans "Can clone" %}{% endif %} + {% if user.is_active and user.can_clone and not user.is_superuser %}{% trans "Can clone" %}{% endif %} {% if user.api_tokens %} {% blocktrans count count=user.api_tokens %}{{ count }} API token{% plural %}{{ count }} API tokens{% endblocktrans %} {% endif %} - {% if not user.groups.all and not user.api_tokens %}{% if user.is_superuser or not can_clone %} + {% if not user.groups.all and not user.api_tokens %}{% if user.is_superuser or not user.is_active or not user.can_clone %} {% trans "No groups or API tokens" %} {% endif %}{% endif %}
diff --git a/admin/tests/test_views.py b/admin/tests/test_views.py index 6b9c2d75..63ef9bca 100644 --- a/admin/tests/test_views.py +++ b/admin/tests/test_views.py @@ -1,7 +1,10 @@ -from django.contrib.auth.models import Group, User +from django.contrib.auth.models import Group, Permission, User from django.core.exceptions import ObjectDoesNotExist +from django.db import connection from django.shortcuts import reverse from django.test import RequestFactory, TestCase +from django.test.utils import CaptureQueriesContext +from lxml import html from accounts.models import UserAttributes from logs.models import Logs @@ -54,6 +57,29 @@ class AdminTestCase(TestCase): response = self.client.get(reverse("admin:user_list")) self.assertEqual(response.status_code, 200) + def test_user_list_shows_who_can_clone_in_queries_independent_of_the_users(self): + clone = Permission.objects.get(content_type__app_label="instances", codename="clone_instances") + User.objects.create_user("direct").user_permissions.add(clone) + group = Group.objects.create(name="Cloners") + group.permissions.add(clone) + User.objects.create_user("by-group").groups.add(group) + blocked = User.objects.create_user("blocked", is_active=False) + blocked.user_permissions.add(clone) + User.objects.create_user("plain") + + def rows(): + with CaptureQueriesContext(connection) as queries: + page = html.fromstring(self.client.get(reverse("admin:user_list")).content) + return {item.xpath(".//a[contains(@class, 'fw-semibold')]")[0].text: "Can clone" in item.text_content() + for item in page.xpath("//div[contains(@class, 'list-group-item')]")}, len(queries) + + shown, count = rows() + self.assertEqual({name: shown[name] for name in ("direct", "by-group", "blocked", "plain", "admin")}, + {"direct": True, "by-group": True, "blocked": False, "plain": False, "admin": False}) + for i in range(5): + User.objects.create_user(f"more-{i}").user_permissions.add(clone) + self.assertEqual(rows()[1], count) + def test_users(self): response = self.client.get(reverse("admin:user_create")) self.assertEqual(response.status_code, 200) diff --git a/admin/views.py b/admin/views.py index 9c223209..aa5c87ba 100644 --- a/admin/views.py +++ b/admin/views.py @@ -5,11 +5,11 @@ from appsettings.settings import app_settings from django.conf import settings from django.contrib import messages from django.contrib.auth import update_session_auth_hash -from django.contrib.auth.models import Group, User +from django.contrib.auth.models import Group, Permission, User from django.core.paginator import Paginator from django.http import HttpResponse from django.db import transaction -from django.db.models import Count, Q +from django.db.models import Count, Exists, OuterRef, Q from django.shortcuts import get_object_or_404, redirect, render from django.utils import timezone from django.utils.http import url_has_allowed_host_and_scheme, urlencode @@ -107,7 +107,13 @@ OWN_ACCOUNT = _("You cannot block, delete or demote your own account") def user_list(request): # API tokens that still work: an expired one is deleted only at the user's next create live = Q(auth_token_set__expiry__isnull=True) | Q(auth_token_set__expiry__gt=timezone.now()) - users = User.objects.annotate(api_tokens=Count("auth_token_set", filter=live)).prefetch_related("groups") + # the clone permission, given to the user or to a group of the user: one + # query for the list, not a has_perm (two queries) a user + clone = Permission.objects.filter(content_type__app_label="instances", codename="clone_instances") + users = User.objects.annotate( + api_tokens=Count("auth_token_set", filter=live), + can_clone=Exists(clone.filter(Q(user=OuterRef("pk")) | Q(group__user=OuterRef("pk")))), + ).prefetch_related("groups") return render( request, "admin/user_list.html",