Merge pull request #1775 from cisagov/za/1753-improve-admin-load-performance

(on getgov-za) Ticket #1753, #1706: Improve DomainApplication admin load times

src/registrar/admin.py

@@ -1,6 +1,8 @@
 import logging
+
 from django import forms
-from django.db.models.functions import Concat
+from django.db.models.functions import Concat, Coalesce
+from django.db.models import Value, CharField
 from django.http import HttpResponse
 from django.shortcuts import redirect
 from django_fsm import get_available_FIELD_transitions
@@ -11,8 +13,7 @@ from django.contrib.contenttypes.models import ContentType
 from django.http.response import HttpResponseRedirect
 from django.urls import reverse
 from epplibwrapper.errors import ErrorCode, RegistryError
-from registrar.models.domain import Domain
-from registrar.models.user import User
+from registrar.models import Contact, Domain, DomainApplication, DraftDomain, User, Website
 from registrar.utility import csv_export
 from registrar.views.utility.mixins import OrderableFieldsMixin
 from django.contrib.admin.views.main import ORDER_VAR
@@ -118,41 +119,52 @@ class CustomLogEntryAdmin(LogEntryAdmin):
 
 
 class AdminSortFields:
-    def get_queryset(db_field):
+    _name_sort = ["first_name", "last_name", "email"]
+
+    # Define a mapping of field names to model querysets and sort expressions.
+    # A dictionary is used for specificity, but the downside is some degree of repetition.
+    # To eliminate this, this list can be generated dynamically but the readability of that
+    # is impacted.
+    sort_mapping = {
+        # == Contact == #
+        "other_contacts": (Contact, _name_sort),
+        "authorizing_official": (Contact, _name_sort),
+        "submitter": (Contact, _name_sort),
+        # == User == #
+        "creator": (User, _name_sort),
+        "user": (User, _name_sort),
+        "investigator": (User, _name_sort),
+        # == Website == #
+        "current_websites": (Website, "website"),
+        "alternative_domains": (Website, "website"),
+        # == DraftDomain == #
+        "requested_domain": (DraftDomain, "name"),
+        # == DomainApplication == #
+        "domain_application": (DomainApplication, "requested_domain__name"),
+        # == Domain == #
+        "domain": (Domain, "name"),
+        "approved_domain": (Domain, "name"),
+    }
+
+    @classmethod
+    def get_queryset(cls, db_field):
         """This is a helper function for formfield_for_manytomany and formfield_for_foreignkey"""
-        # customize sorting
-        if db_field.name in (
-            "other_contacts",
-            "authorizing_official",
-            "submitter",
-        ):
-            # Sort contacts by first_name, then last_name, then email
-            return models.Contact.objects.all().order_by(Concat("first_name", "last_name", "email"))
-        elif db_field.name in ("current_websites", "alternative_domains"):
-            # sort web sites
-            return models.Website.objects.all().order_by("website")
-        elif db_field.name in (
-            "creator",
-            "user",
-            "investigator",
-        ):
-            # Sort users by first_name, then last_name, then email
-            return models.User.objects.all().order_by(Concat("first_name", "last_name", "email"))
-        elif db_field.name in (
-            "domain",
-            "approved_domain",
-        ):
-            # Sort domains by name
-            return models.Domain.objects.all().order_by("name")
-        elif db_field.name in ("requested_domain",):
-            # Sort draft domains by name
-            return models.DraftDomain.objects.all().order_by("name")
-        elif db_field.name in ("domain_application",):
-            # Sort domain applications by name
-            return models.DomainApplication.objects.all().order_by("requested_domain__name")
-        else:
+        queryset_info = cls.sort_mapping.get(db_field.name, None)
+        if queryset_info is None:
             return None
 
+        # Grab the model we want to order, and grab how we want to order it
+        model, order_by = queryset_info
+        match db_field.name:
+            case "investigator":
+                # We should only return users who are staff.
+                return model.objects.filter(is_staff=True).order_by(*order_by)
+            case _:
+                if isinstance(order_by, list) or isinstance(order_by, tuple):
+                    return model.objects.order_by(*order_by)
+                else:
+                    return model.objects.order_by(order_by)
+
 
 class AuditedAdmin(admin.ModelAdmin):
     """Custom admin to make auditing easier."""
@@ -170,9 +182,14 @@ class AuditedAdmin(admin.ModelAdmin):
     def formfield_for_manytomany(self, db_field, request, **kwargs):
         """customize the behavior of formfields with manytomany relationships.  the customized
         behavior includes sorting of objects in lists as well as customizing helper text"""
+
+        # Define a queryset. Note that in the super of this,
+        # a new queryset will only be generated if one does not exist.
+        # Thus, the order in which we define queryset matters.
         queryset = AdminSortFields.get_queryset(db_field)
         if queryset:
             kwargs["queryset"] = queryset
+
         formfield = super().formfield_for_manytomany(db_field, request, **kwargs)
         # customize the help text for all formfields for manytomany
         formfield.help_text = (
@@ -182,11 +199,16 @@ class AuditedAdmin(admin.ModelAdmin):
         return formfield
 
     def formfield_for_foreignkey(self, db_field, request, **kwargs):
-        """customize the behavior of formfields with foreign key relationships.  this will customize
-        the behavior of selects.  customized behavior includes sorting of objects in list"""
+        """Customize the behavior of formfields with foreign key relationships. This will customize
+        the behavior of selects. Customized behavior includes sorting of objects in list."""
+
+        # Define a queryset. Note that in the super of this,
+        # a new queryset will only be generated if one does not exist.
+        # Thus, the order in which we define queryset matters.
         queryset = AdminSortFields.get_queryset(db_field)
         if queryset:
             kwargs["queryset"] = queryset
+
         return super().formfield_for_foreignkey(db_field, request, **kwargs)
 
 
@@ -221,7 +243,6 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin):
             parameter_value: string}
         TODO: convert investigator id to investigator username
         """
-
         filters = []
         # Retrieve the filter parameters
         for param in request.GET.keys():
@@ -265,6 +286,14 @@ class UserContactInline(admin.StackedInline):
 class MyUserAdmin(BaseUserAdmin):
     """Custom user admin class to use our inlines."""
 
+    class Meta:
+        """Contains meta information about this class"""
+
+        model = models.User
+        fields = "__all__"
+
+    _meta = Meta()
+
     inlines = [UserContactInline]
 
     list_display = (
@@ -353,6 +382,42 @@ class MyUserAdmin(BaseUserAdmin):
     # in autocomplete_fields for user
     ordering = ["first_name", "last_name", "email"]
 
+    def get_search_results(self, request, queryset, search_term):
+        """
+        Override for get_search_results. This affects any upstream model using autocomplete_fields,
+        such as DomainApplication. This is because autocomplete_fields uses an API call to fetch data,
+        and this fetch comes from this method.
+        """
+        # Custom filtering logic
+        queryset, use_distinct = super().get_search_results(request, queryset, search_term)
+
+        # If we aren't given a request to modify, we shouldn't try to
+        if request is None or not hasattr(request, "GET"):
+            return queryset, use_distinct
+
+        # Otherwise, lets modify it!
+        request_get = request.GET
+
+        # The request defines model name and field name.
+        # For instance, model_name could be "DomainApplication"
+        # and field_name could be "investigator".
+        model_name = request_get.get("model_name", None)
+        field_name = request_get.get("field_name", None)
+
+        # Make sure we're only modifying requests from these models.
+        models_to_target = {"domainapplication"}
+        if model_name in models_to_target:
+            # Define rules per field
+            match field_name:
+                case "investigator":
+                    # We should not display investigators who don't have a staff role
+                    queryset = queryset.filter(is_staff=True)
+                case _:
+                    # In the default case, do nothing
+                    pass
+
+        return queryset, use_distinct
+
     # Let's define First group
     # (which should in theory be the ONLY group)
     def group(self, obj):
@@ -417,6 +482,9 @@ class ContactAdmin(ListHeaderAdmin):
         "contact",
         "email",
     ]
+    # this ordering effects the ordering of results
+    # in autocomplete_fields for user
+    ordering = ["first_name", "last_name", "email"]
 
     # We name the custom prop 'contact' because linter
     # is not allowing a short_description attr on it
@@ -752,8 +820,23 @@ class DomainApplicationAdmin(ListHeaderAdmin):
             """Lookup reimplementation, gets users of is_staff.
             Returns a list of tuples consisting of (user.id, user)
             """
-            privileged_users = User.objects.filter(is_staff=True).order_by("first_name", "last_name", "email")
-            return [(user.id, user) for user in privileged_users]
+            # Select all investigators that are staff, then order by name and email
+            privileged_users = (
+                DomainApplication.objects.select_related("investigator")
+                .filter(investigator__is_staff=True)
+                .order_by("investigator__first_name", "investigator__last_name", "investigator__email")
+            )
+
+            # Annotate the full name and return a values list that lookups can use
+            privileged_users_annotated = privileged_users.annotate(
+                full_name=Coalesce(
+                    Concat("investigator__first_name", Value(" "), "investigator__last_name", output_field=CharField()),
+                    "investigator__email",
+                    output_field=CharField(),
+                )
+            ).values_list("investigator__id", "full_name")
+
+            return privileged_users_annotated
 
         def queryset(self, request, queryset):
             """Custom queryset implementation, filters by investigator"""
@@ -853,26 +936,19 @@ class DomainApplicationAdmin(ListHeaderAdmin):
         "anything_else",
         "is_policy_acknowledged",
     ]
-
+    autocomplete_fields = [
+        "approved_domain",
+        "requested_domain",
+        "submitter",
+        "creator",
+        "authorizing_official",
+        "investigator",
+    ]
     filter_horizontal = ("current_websites", "alternative_domains", "other_contacts")
 
     # Table ordering
     ordering = ["requested_domain__name"]
 
-    # lists in filter_horizontal are not sorted properly, sort them
-    # by website
-    def formfield_for_manytomany(self, db_field, request, **kwargs):
-        if db_field.name in ("current_websites", "alternative_domains"):
-            kwargs["queryset"] = models.Website.objects.all().order_by("website")  # Sort websites
-        return super().formfield_for_manytomany(db_field, request, **kwargs)
-
-    def formfield_for_foreignkey(self, db_field, request, **kwargs):
-        # Removes invalid investigator options from the investigator dropdown
-        if db_field.name == "investigator":
-            kwargs["queryset"] = User.objects.filter(is_staff=True)
-            return db_field.formfield(**kwargs)
-        return super().formfield_for_foreignkey(db_field, request, **kwargs)
-
     # Trigger action when a fieldset is changed
     def save_model(self, request, obj, form, change):
         if obj and obj.creator.status != models.User.RESTRICTED:
@@ -940,7 +1016,6 @@ class DomainApplicationAdmin(ListHeaderAdmin):
         admin user permissions and the application creator's status, so
         we'll use the baseline readonly_fields and extend it as needed.
         """
-
         readonly_fields = list(self.readonly_fields)
 
         # Check if the creator is restricted
@@ -1018,8 +1093,8 @@ class DomainInformationInline(admin.StackedInline):
         return formfield
 
     def formfield_for_foreignkey(self, db_field, request, **kwargs):
-        """customize the behavior of formfields with foreign key relationships.  this will customize
-        the behavior of selects.  customized behavior includes sorting of objects in list"""
+        """Customize the behavior of formfields with foreign key relationships. This will customize
+        the behavior of selects. Customized behavior includes sorting of objects in list."""
         queryset = AdminSortFields.get_queryset(db_field)
         if queryset:
             kwargs["queryset"] = queryset
@@ -1299,6 +1374,10 @@ class DraftDomainAdmin(ListHeaderAdmin):
     search_fields = ["name"]
     search_help_text = "Search by draft domain name."
 
+    # this ordering effects the ordering of results
+    # in autocomplete_fields for user
+    ordering = ["name"]
+
 
 class VerifiedByStaffAdmin(ListHeaderAdmin):
     list_display = ("email", "requestor", "truncated_notes", "created_at")

src/registrar/assets/sass/_theme/_admin.scss

@@ -270,3 +270,13 @@ h1, h2, h3,
         margin: 0!important;
     }
 }
+
+// Hides the "clear" button on autocomplete, as we already have one to use
+.select2-selection__clear {
+    display: none;
+}
+
+// Fixes a display issue where the list was entirely white, or had too much whitespace
+.select2-dropdown {
+    display: inline-grid !important;
+}

src/registrar/models/utility/generic_helper.py

@@ -0,0 +1,37 @@
+"""This file contains general purpose helpers that don't belong in any specific location"""
+
+import time
+import logging
+
+
+logger = logging.getLogger(__name__)
+
+
+class Timer:
+    """
+    This class is used to measure execution time for performance profiling.
+    __enter__ and __exit__ is used such that you can wrap any code you want
+    around a with statement. After this exits, logger.info will print
+    the execution time in seconds.
+
+    Note that this class does not account for general randomness as more
+    robust libraries do, so there is some tiny amount of latency involved
+    in using this, but it is minimal enough that for most applications it is not
+    noticable.
+
+    Usage:
+    with Timer():
+        ...some code
+    """
+
+    def __enter__(self):
+        """Starts the timer"""
+        self.start = time.time()
+        # This allows usage of the instance within the with block
+        return self
+
+    def __exit__(self, *args):
+        """Ends the timer and logs what happened"""
+        self.end = time.time()
+        self.duration = self.end - self.start
+        logger.info(f"Execution time: {self.duration} seconds")

src/registrar/tests/common.py

@@ -231,6 +231,7 @@ class AuditedAdminMockData:
             first_name="{} first_name:{}".format(item_name, short_hand),
             last_name="{} last_name:{}".format(item_name, short_hand),
             username="{} username:{}".format(item_name + str(uuid.uuid4())[:8], short_hand),
+            is_staff=True,
         )[0]
         return user
 

src/registrar/tests/test_admin.py

@@ -941,8 +941,17 @@ class TestDomainApplicationAdmin(MockEppLib):
         investigator_field = DomainApplication._meta.get_field("investigator")
 
         # We should only be displaying staff users, in alphabetical order
-        expected_dropdown = list(User.objects.filter(is_staff=True))
-        current_dropdown = list(self.admin.formfield_for_foreignkey(investigator_field, request).queryset)
+        sorted_fields = ["first_name", "last_name", "email"]
+        expected_dropdown = list(User.objects.filter(is_staff=True).order_by(*sorted_fields))
+
+        # Grab the current dropdown. We do an API call to autocomplete to get this info.
+        application_queryset = self.admin.formfield_for_foreignkey(investigator_field, request).queryset
+        user_request = self.factory.post(
+            "/admin/autocomplete/?app_label=registrar&model_name=domainapplication&field_name=investigator"
+        )
+        user_admin = MyUserAdmin(User, self.site)
+        user_queryset = user_admin.get_search_results(user_request, application_queryset, None)[0]
+        current_dropdown = list(user_queryset)
 
         self.assertEqual(expected_dropdown, current_dropdown)
 
@@ -976,7 +985,13 @@ class TestDomainApplicationAdmin(MockEppLib):
         self.client.login(username="staffuser", password=p)
         request = RequestFactory().get("/")
 
-        expected_list = list(User.objects.filter(is_staff=True).order_by("first_name", "last_name", "email"))
+        # These names have metadata embedded in them. :investigator implicitly tests if
+        # these are actually from the attribute "investigator".
+        expected_list = [
+            "AGuy AGuy last_name:investigator",
+            "FinalGuy FinalGuy last_name:investigator",
+            "SomeGuy first_name:investigator SomeGuy last_name:investigator",
+        ]
 
         # Get the actual sorted list of investigators from the lookups method
         actual_list = [item for _, item in self.admin.InvestigatorFilter.lookups(self, request, self.admin)]
@@ -1396,11 +1411,46 @@ class AuditedAdminTest(TestCase):
 
         return ordered_list
 
+    def test_alphabetically_sorted_domain_application_investigator(self):
+        """Tests if the investigator field is alphabetically sorted by mimicking
+        the call event flow"""
+        # Creates multiple domain applications - review status does not matter
+        applications = multiple_unalphabetical_domain_objects("application")
+
+        # Create a mock request
+        application_request = self.factory.post(
+            "/admin/registrar/domainapplication/{}/change/".format(applications[0].pk)
+        )
+
+        # Get the formfield data from the application page
+        application_admin = AuditedAdmin(DomainApplication, self.site)
+        field = DomainApplication.investigator.field
+        application_queryset = application_admin.formfield_for_foreignkey(field, application_request).queryset
+
+        request = self.factory.post(
+            "/admin/autocomplete/?app_label=registrar&model_name=domainapplication&field_name=investigator"
+        )
+
+        sorted_fields = ["first_name", "last_name", "email"]
+        desired_sort_order = list(User.objects.filter(is_staff=True).order_by(*sorted_fields))
+
+        # Grab the data returned from get search results
+        admin = MyUserAdmin(User, self.site)
+        search_queryset = admin.get_search_results(request, application_queryset, None)[0]
+        current_sort_order = list(search_queryset)
+
+        self.assertEqual(
+            desired_sort_order,
+            current_sort_order,
+            "Investigator is not ordered alphabetically",
+        )
+
+    # This test case should be refactored in general, as it is too overly specific and engineered
     def test_alphabetically_sorted_fk_fields_domain_application(self):
         tested_fields = [
             DomainApplication.authorizing_official.field,
             DomainApplication.submitter.field,
-            DomainApplication.investigator.field,
+            # DomainApplication.investigator.field,
             DomainApplication.creator.field,
             DomainApplication.requested_domain.field,
         ]
@@ -1418,6 +1468,7 @@ class AuditedAdminTest(TestCase):
         # but both fields are of a fixed length.
         # For test case purposes, this should be performant.
         for field in tested_fields:
+            with self.subTest(field=field):
                 isNamefield: bool = field == DomainApplication.requested_domain.field
                 if isNamefield:
                     sorted_fields = ["name"]