From 33c3b6982d5853e54955ef403b5b8ec45a9bbbe5 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Fri, 20 Dec 2024 14:39:04 +0530 Subject: [PATCH] Optimize the groups query and add test --- hypha/apply/users/forms.py | 33 +++++++++++++-------------- hypha/apply/users/models.py | 27 +++++++++------------- hypha/apply/users/tests/test_forms.py | 20 +++++++++++++++- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/hypha/apply/users/forms.py b/hypha/apply/users/forms.py index 7ed3b0080f..765279c9c1 100644 --- a/hypha/apply/users/forms.py +++ b/hypha/apply/users/forms.py @@ -216,26 +216,25 @@ def clean_full_name(self): def get_become_user_choices(): - """Returns list of active non-superusers with their roles as choice tuples.""" - active_users = User.objects.filter(is_active=True, is_superuser=False) - choices = [] + """Returns list of active non-superusers with their roles as choice tuples. - for user in active_users: - role_names = user.get_role_names() - if role_names: - roles_html = ", ".join( - [ - f'{str(g)}' - for g in role_names - ] - ) - label = f"{user} ({roles_html})" - else: - label = str(user) + Returns: + list: Tuples of (user_id, formatted_label) for form choices + """ + active_users = ( + User.objects.filter(is_active=True, is_superuser=False) + .prefetch_related("groups") + .order_by("last_login") + ) - choices.append((user.pk, mark_safe(label))) + def format_user_label(user): + role_names = user.get_role_names() + if not role_names: + return str(user) + roles_text = ", ".join(map(str, role_names)) + return mark_safe(f"{user} ({roles_text})") - return choices + return [(user.pk, format_user_label(user)) for user in active_users] class BecomeUserForm(forms.Form): diff --git a/hypha/apply/users/models.py b/hypha/apply/users/models.py index 5e1b77cc15..6e4dd5eeeb 100644 --- a/hypha/apply/users/models.py +++ b/hypha/apply/users/models.py @@ -244,41 +244,39 @@ def get_role_names(self): @cached_property def roles(self): - return list(self.groups.values_list("name", flat=True)) + return [g.name for g in self.groups.all()] @cached_property def is_apply_staff(self): - return self.groups.filter(name=STAFF_GROUP_NAME).exists() or self.is_superuser + return STAFF_GROUP_NAME in self.roles or self.is_superuser @cached_property def is_apply_staff_admin(self): - return ( - self.groups.filter(name=TEAMADMIN_GROUP_NAME).exists() or self.is_superuser - ) + return TEAMADMIN_GROUP_NAME in self.roles or self.is_superuser @cached_property def is_reviewer(self): - return self.groups.filter(name=REVIEWER_GROUP_NAME).exists() + return REVIEWER_GROUP_NAME in self.roles @cached_property def is_partner(self): - return self.groups.filter(name=PARTNER_GROUP_NAME).exists() + return PARTNER_GROUP_NAME in self.roles @cached_property def is_community_reviewer(self): - return self.groups.filter(name=COMMUNITY_REVIEWER_GROUP_NAME).exists() + return COMMUNITY_REVIEWER_GROUP_NAME in self.roles @cached_property def is_applicant(self): - return self.groups.filter(name=APPLICANT_GROUP_NAME).exists() + return APPLICANT_GROUP_NAME in self.roles @cached_property def is_approver(self): - return self.groups.filter(name=APPROVER_GROUP_NAME).exists() + return APPROVER_GROUP_NAME in self.roles @cached_property def is_finance(self): - return self.groups.filter(name=FINANCE_GROUP_NAME).exists() + return FINANCE_GROUP_NAME in self.roles @cached_property def is_org_faculty(self): @@ -298,14 +296,11 @@ def can_access_dashboard(self): @cached_property def is_contracting(self): - return self.groups.filter(name=CONTRACTING_GROUP_NAME).exists() + return CONTRACTING_GROUP_NAME in self.roles @cached_property def is_contracting_approver(self): - return ( - self.groups.filter(name=CONTRACTING_GROUP_NAME).exists() - and self.groups.filter(name=APPROVER_GROUP_NAME).exists() - ) + return self.is_contracting and self.is_approver def get_absolute_url(self): """Used in the activities messages to generate URL for user instances. diff --git a/hypha/apply/users/tests/test_forms.py b/hypha/apply/users/tests/test_forms.py index 1b3c5f35c5..0d04c6fac2 100644 --- a/hypha/apply/users/tests/test_forms.py +++ b/hypha/apply/users/tests/test_forms.py @@ -1,7 +1,8 @@ +import pytest from django.forms.models import model_to_dict from django.test import RequestFactory, TestCase -from ..forms import EmailChangePasswordForm, ProfileForm +from ..forms import BecomeUserForm, EmailChangePasswordForm, ProfileForm from .factories import StaffFactory, UserFactory @@ -113,3 +114,20 @@ def test_can_update_slack(self): form = EmailChangePasswordForm(self.user) form.save("", "", slack_name) self.assertEqual(self.user.slack, slack_name) + + +@pytest.mark.django_db +def test_become_user_form_query_count(django_assert_num_queries): + # create a three user + UserFactory(is_superuser=False) + UserFactory(is_superuser=False) + UserFactory(is_superuser=False) + + # Enable query counting and verify only 2 queries are made + with django_assert_num_queries(2): + # This should trigger 2 queries: + # 1. Get active non-superusers + # 2. Prefetch related groups + form = BecomeUserForm() + # Access choices to trigger query execution + list(form.fields["user_pk"].choices)