From a51949da0b98b15134312839238b381b85fc6095 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 13 Feb 2024 13:57:15 -0700 Subject: [PATCH 01/15] Improve performance --- src/registrar/admin.py | 231 +++++++++++++++++++++++++---------------- 1 file changed, 141 insertions(+), 90 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4034bf35b..8f3f75105 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1,6 +1,8 @@ import logging +import time 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 @@ -12,6 +14,7 @@ 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.domain_application import DomainApplication from registrar.models.user import User from registrar.utility import csv_export from registrar.views.utility.mixins import OrderableFieldsMixin @@ -707,6 +710,17 @@ class DomainInformationAdmin(ListHeaderAdmin): return readonly_fields # Read-only fields for analysts +class Timer: + def __enter__(self): + self.start = time.time() + return self # This allows usage of the instance within the with block + + def __exit__(self, *args): + self.end = time.time() + self.duration = self.end - self.start + logger.info(f"Execution time: {self.duration} seconds") + + class DomainApplicationAdminForm(forms.ModelForm): """Custom form to limit transitions to available transitions""" @@ -753,15 +767,41 @@ 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] + logger.info("timing lookups") + with Timer() as t: + + # 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""" - if self.value() is None: - return queryset - else: - return queryset.filter(investigator__id__exact=self.value()) + logger.info("timing queryset") + with Timer() as t: + if self.value() is None: + return queryset + else: + return queryset.filter(investigator__id__exact=self.value()) # Columns list_display = [ @@ -863,77 +903,83 @@ class DomainApplicationAdmin(ListHeaderAdmin): # 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) + logger.info("timing formfield_for_manytomany") + with Timer() as t: + 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) + logger.info("timing formfield_for_foreignkey") + with Timer() as t: + # 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: - if change: # Check if the application is being edited - # Get the original application from the database - original_obj = models.DomainApplication.objects.get(pk=obj.pk) + logger.info("timing save_model") + with Timer() as t: + if obj and obj.creator.status != models.User.RESTRICTED: + if change: # Check if the application is being edited + # Get the original application from the database + original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if ( - obj - and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED - and obj.status != models.DomainApplication.ApplicationStatus.APPROVED - and not obj.domain_is_not_active() - ): - # If an admin tried to set an approved application to - # another status and the related domain is already - # active, shortcut the action and throw a friendly - # error message. This action would still not go through - # shortcut or not as the rules are duplicated on the model, - # but the error would be an ugly Django error screen. + if ( + obj + and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED + and obj.status != models.DomainApplication.ApplicationStatus.APPROVED + and not obj.domain_is_not_active() + ): + # If an admin tried to set an approved application to + # another status and the related domain is already + # active, shortcut the action and throw a friendly + # error message. This action would still not go through + # shortcut or not as the rules are duplicated on the model, + # but the error would be an ugly Django error screen. - # Clear the success message - messages.set_level(request, messages.ERROR) + # Clear the success message + messages.set_level(request, messages.ERROR) - messages.error( - request, - "This action is not permitted. The domain is already active.", - ) + messages.error( + request, + "This action is not permitted. The domain is already active.", + ) - else: - if obj.status != original_obj.status: - status_method_mapping = { - models.DomainApplication.ApplicationStatus.STARTED: None, - models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, - models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, - models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, - models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, - models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, - models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, - models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), - } - selected_method = status_method_mapping.get(obj.status) - if selected_method is None: - logger.warning("Unknown status selected in django admin") - else: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. - obj.status = original_obj.status - selected_method() + else: + if obj.status != original_obj.status: + status_method_mapping = { + models.DomainApplication.ApplicationStatus.STARTED: None, + models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, + models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, + models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, + models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, + models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, + models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, + models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), + } + selected_method = status_method_mapping.get(obj.status) + if selected_method is None: + logger.warning("Unknown status selected in django admin") + else: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. + obj.status = original_obj.status + selected_method() - super().save_model(request, obj, form, change) - else: - # Clear the success message - messages.set_level(request, messages.ERROR) + super().save_model(request, obj, form, change) + else: + # Clear the success message + messages.set_level(request, messages.ERROR) - messages.error( - request, - "This action is not permitted for applications with a restricted creator.", - ) + messages.error( + request, + "This action is not permitted for applications with a restricted creator.", + ) def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. @@ -941,36 +987,41 @@ 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. """ + logger.info("timing get_readonly_fields") + with Timer() as t: + readonly_fields = list(self.readonly_fields) - readonly_fields = list(self.readonly_fields) + # Check if the creator is restricted + if obj and obj.creator.status == models.User.RESTRICTED: + # For fields like CharField, IntegerField, etc., the widget used is + # straightforward and the readonly_fields list can control their behavior + readonly_fields.extend([field.name for field in self.model._meta.fields]) + # Add the multi-select fields to readonly_fields: + # Complex fields like ManyToManyField require special handling + readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) - # Check if the creator is restricted - if obj and obj.creator.status == models.User.RESTRICTED: - # For fields like CharField, IntegerField, etc., the widget used is - # straightforward and the readonly_fields list can control their behavior - readonly_fields.extend([field.name for field in self.model._meta.fields]) - # Add the multi-select fields to readonly_fields: - # Complex fields like ManyToManyField require special handling - readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) - - if request.user.has_perm("registrar.full_access_permission"): + if request.user.has_perm("registrar.full_access_permission"): + return readonly_fields + # Return restrictive Read-only fields for analysts and + # users who might not belong to groups + readonly_fields.extend([field for field in self.analyst_readonly_fields]) return readonly_fields - # Return restrictive Read-only fields for analysts and - # users who might not belong to groups - readonly_fields.extend([field for field in self.analyst_readonly_fields]) - return readonly_fields def display_restricted_warning(self, request, obj): - if obj and obj.creator.status == models.User.RESTRICTED: - messages.warning( - request, - "Cannot edit an application with a restricted creator.", - ) + logger.info("timing display_restricted_warning") + with Timer() as t: + if obj and obj.creator.status == models.User.RESTRICTED: + messages.warning( + request, + "Cannot edit an application with a restricted creator.", + ) def change_view(self, request, object_id, form_url="", extra_context=None): - obj = self.get_object(request, object_id) - self.display_restricted_warning(request, obj) - return super().change_view(request, object_id, form_url, extra_context) + logger.info("timing change_view") + with Timer() as t: + obj = self.get_object(request, object_id) + self.display_restricted_warning(request, obj) + return super().change_view(request, object_id, form_url, extra_context) class TransitionDomainAdmin(ListHeaderAdmin): From ee41a8ac73d1cbf5fd873ff645fc4d07d75e256b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 13 Feb 2024 14:50:41 -0700 Subject: [PATCH 02/15] Add some timing --- src/registrar/admin.py | 84 +++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8f3f75105..dda3b0c3c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -205,17 +205,21 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): Reference: https://code.djangoproject.com/ticket/31975 """ - return MultiFieldSortableChangeList + logger.info("timing get_changelist") + with Timer() as t: + return MultiFieldSortableChangeList def changelist_view(self, request, extra_context=None): - if extra_context is None: - extra_context = {} - # Get the filtered values - filters = self.get_filters(request) - # Pass the filtered values to the template context - extra_context["filters"] = filters - extra_context["search_query"] = request.GET.get("q", "") # Assuming the search query parameter is 'q' - return super().changelist_view(request, extra_context=extra_context) + logger.info("timing changelist_view") + with Timer() as t: + if extra_context is None: + extra_context = {} + # Get the filtered values + filters = self.get_filters(request) + # Pass the filtered values to the template context + extra_context["filters"] = filters + extra_context["search_query"] = request.GET.get("q", "") # Assuming the search query parameter is 'q' + return super().changelist_view(request, extra_context=extra_context) def get_filters(self, request): """Retrieve the current set of parameters being used to filter the table @@ -224,39 +228,40 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): parameter_value: string} TODO: convert investigator id to investigator username """ + logger.info("timing get_filters") + with Timer() as t: + filters = [] + # Retrieve the filter parameters + for param in request.GET.keys(): + # Exclude the default search parameter 'q' + if param != "q" and param != "o": + parameter_name = param.replace("__exact", "").replace("_type", "").replace("__id", " id") - filters = [] - # Retrieve the filter parameters - for param in request.GET.keys(): - # Exclude the default search parameter 'q' - if param != "q" and param != "o": - parameter_name = param.replace("__exact", "").replace("_type", "").replace("__id", " id") - - if parameter_name == "investigator id": - # Retrieves the corresponding contact from Users - id_value = request.GET.get(param) - try: - contact = models.User.objects.get(id=id_value) - investigator_name = contact.first_name + " " + contact.last_name + if parameter_name == "investigator id": + # Retrieves the corresponding contact from Users + id_value = request.GET.get(param) + try: + contact = models.User.objects.get(id=id_value) + investigator_name = contact.first_name + " " + contact.last_name + filters.append( + { + "parameter_name": "investigator", + "parameter_value": investigator_name, + } + ) + except models.User.DoesNotExist: + pass + else: + # For other parameter names, append a dictionary with the original + # parameter_name and the corresponding parameter_value filters.append( { - "parameter_name": "investigator", - "parameter_value": investigator_name, + "parameter_name": parameter_name, + "parameter_value": request.GET.get(param), } ) - except models.User.DoesNotExist: - pass - else: - # For other parameter names, append a dictionary with the original - # parameter_name and the corresponding parameter_value - filters.append( - { - "parameter_name": parameter_name, - "parameter_value": request.GET.get(param), - } - ) - return filters + return filters class UserContactInline(admin.StackedInline): @@ -803,6 +808,11 @@ class DomainApplicationAdmin(ListHeaderAdmin): else: return queryset.filter(investigator__id__exact=self.value()) + def __new__(self, *args, **kwargs): + logger.info("timing __new__") + with Timer() as t: + return super().__new__(self, *args, **kwargs) + # Columns list_display = [ "requested_domain", @@ -905,7 +915,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): def formfield_for_manytomany(self, db_field, request, **kwargs): logger.info("timing formfield_for_manytomany") with Timer() as t: - if db_field.name in ("current_websites", "alternative_domains"): + 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) From f55d5ef93428462f549ba9bf3630efb173e19e8f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 08:16:26 -0700 Subject: [PATCH 03/15] Logging --- src/registrar/admin.py | 66 ++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index dda3b0c3c..18b664498 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -56,44 +56,46 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): Mostly identical to the base implementation, except that now it can return a list of order_field objects rather than just one. """ - params = self.params - ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) + logger.info("timing get_ordering") + with Timer() as t: + params = self.params + ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) - if ORDER_VAR in params: - # Clear ordering and used params - ordering = [] + if ORDER_VAR in params: + # Clear ordering and used params + ordering = [] - order_params = params[ORDER_VAR].split(".") - for p in order_params: - try: - none, pfx, idx = p.rpartition("-") - field_name = self.list_display[int(idx)] + order_params = params[ORDER_VAR].split(".") + for p in order_params: + try: + none, pfx, idx = p.rpartition("-") + field_name = self.list_display[int(idx)] - order_fields = self.get_ordering_field(field_name) + order_fields = self.get_ordering_field(field_name) - if isinstance(order_fields, list): - for order_field in order_fields: - if order_field: - ordering.append(pfx + order_field) - else: - ordering.append(pfx + order_fields) + if isinstance(order_fields, list): + for order_field in order_fields: + if order_field: + ordering.append(pfx + order_field) + else: + ordering.append(pfx + order_fields) - except (IndexError, ValueError): - continue # Invalid ordering specified, skip it. + except (IndexError, ValueError): + continue # Invalid ordering specified, skip it. - # Add the given query's ordering fields, if any. - ordering.extend(queryset.query.order_by) + # Add the given query's ordering fields, if any. + ordering.extend(queryset.query.order_by) - # Ensure that the primary key is systematically present in the list of - # ordering fields so we can guarantee a deterministic order across all - # database backends. - pk_name = self.lookup_opts.pk.name - if not (set(ordering) & set(["pk", "-pk", pk_name, "-" + pk_name])): - # The two sets do not intersect, meaning the pk isn't present. So - # we add it. - ordering.append("-pk") + # Ensure that the primary key is systematically present in the list of + # ordering fields so we can guarantee a deterministic order across all + # database backends. + pk_name = self.lookup_opts.pk.name + if not (set(ordering) & set(["pk", "-pk", pk_name, "-" + pk_name])): + # The two sets do not intersect, meaning the pk isn't present. So + # we add it. + ordering.append("-pk") - return ordering + return ordering class CustomLogEntryAdmin(LogEntryAdmin): @@ -913,14 +915,14 @@ class DomainApplicationAdmin(ListHeaderAdmin): # lists in filter_horizontal are not sorted properly, sort them # by website def formfield_for_manytomany(self, db_field, request, **kwargs): - logger.info("timing formfield_for_manytomany") + logger.info(f"timing formfield_for_manytomany -> {db_field.name}") with Timer() as t: 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): - logger.info("timing formfield_for_foreignkey") + logger.info(f"timing formfield_for_foreignkey -> {db_field.name}") with Timer() as t: # Removes invalid investigator options from the investigator dropdown if db_field.name == "investigator": From 63438f0193c659e75f1b1e7abfa8ea7485667123 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 11:16:13 -0700 Subject: [PATCH 04/15] AdminSortFields refactor --- src/registrar/admin.py | 71 ++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 18b664498..16fb1b1d1 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -13,9 +13,12 @@ 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.contact import Contact from registrar.models.domain import Domain from registrar.models.domain_application import DomainApplication +from registrar.models.draft_domain import DraftDomain from registrar.models.user import User +from registrar.models.website import Website from registrar.utility import csv_export from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR @@ -123,41 +126,39 @@ class CustomLogEntryAdmin(LogEntryAdmin): class AdminSortFields: - def get_queryset(db_field): + _name_sort = Concat("first_name", "last_name", "email") + # Define a mapping of field names to model querysets and sort expressions + sort_mapping = { + "other_contacts": (Contact, _name_sort), + "authorizing_official": (Contact, _name_sort), + "submitter": (Contact, _name_sort), + "current_websites": (Website, "website"), + "alternative_domains": (Website, "website"), + "creator": (User, _name_sort), + "user": (User, _name_sort), + "investigator": (User, _name_sort), + "domain": (Domain, "name"), + "approved_domain": (Domain, "name"), + "requested_domain": (DraftDomain, "name"), + "domain_application": (DomainApplication, "requested_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 + 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 no case is defined, return the default + return model.objects.order_by(order_by) + class AuditedAdmin(admin.ModelAdmin): """Custom admin to make auditing easier.""" @@ -922,12 +923,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): return super().formfield_for_manytomany(db_field, request, **kwargs) def formfield_for_foreignkey(self, db_field, request, **kwargs): - logger.info(f"timing formfield_for_foreignkey -> {db_field.name}") with Timer() as t: - # 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) + print(f"This is the db_field: {db_field}") return super().formfield_for_foreignkey(db_field, request, **kwargs) # Trigger action when a fieldset is changed From 6a0587d5fe0936e939a5bf82f880cf7a77341076 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 13:22:10 -0700 Subject: [PATCH 05/15] Test autocomplete dropdown --- src/registrar/admin.py | 376 +++++++++---------- src/registrar/assets/sass/_theme/_admin.scss | 4 + 2 files changed, 183 insertions(+), 197 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 16fb1b1d1..18f1970da 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1,5 +1,6 @@ import logging import time +from django.db import transaction from django import forms from django.db.models.functions import Concat, Coalesce from django.db.models import Value, CharField @@ -59,46 +60,44 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): Mostly identical to the base implementation, except that now it can return a list of order_field objects rather than just one. """ - logger.info("timing get_ordering") - with Timer() as t: - params = self.params - ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) + params = self.params + ordering = list(self.model_admin.get_ordering(request) or self._get_default_ordering()) - if ORDER_VAR in params: - # Clear ordering and used params - ordering = [] + if ORDER_VAR in params: + # Clear ordering and used params + ordering = [] - order_params = params[ORDER_VAR].split(".") - for p in order_params: - try: - none, pfx, idx = p.rpartition("-") - field_name = self.list_display[int(idx)] + order_params = params[ORDER_VAR].split(".") + for p in order_params: + try: + none, pfx, idx = p.rpartition("-") + field_name = self.list_display[int(idx)] - order_fields = self.get_ordering_field(field_name) + order_fields = self.get_ordering_field(field_name) - if isinstance(order_fields, list): - for order_field in order_fields: - if order_field: - ordering.append(pfx + order_field) - else: - ordering.append(pfx + order_fields) + if isinstance(order_fields, list): + for order_field in order_fields: + if order_field: + ordering.append(pfx + order_field) + else: + ordering.append(pfx + order_fields) - except (IndexError, ValueError): - continue # Invalid ordering specified, skip it. + except (IndexError, ValueError): + continue # Invalid ordering specified, skip it. - # Add the given query's ordering fields, if any. - ordering.extend(queryset.query.order_by) + # Add the given query's ordering fields, if any. + ordering.extend(queryset.query.order_by) - # Ensure that the primary key is systematically present in the list of - # ordering fields so we can guarantee a deterministic order across all - # database backends. - pk_name = self.lookup_opts.pk.name - if not (set(ordering) & set(["pk", "-pk", pk_name, "-" + pk_name])): - # The two sets do not intersect, meaning the pk isn't present. So - # we add it. - ordering.append("-pk") + # Ensure that the primary key is systematically present in the list of + # ordering fields so we can guarantee a deterministic order across all + # database backends. + pk_name = self.lookup_opts.pk.name + if not (set(ordering) & set(["pk", "-pk", pk_name, "-" + pk_name])): + # The two sets do not intersect, meaning the pk isn't present. So + # we add it. + ordering.append("-pk") - return ordering + return ordering class CustomLogEntryAdmin(LogEntryAdmin): @@ -129,18 +128,24 @@ class AdminSortFields: _name_sort = Concat("first_name", "last_name", "email") # Define a mapping of field names to model querysets and sort expressions sort_mapping = { + # == Contact == # "other_contacts": (Contact, _name_sort), "authorizing_official": (Contact, _name_sort), "submitter": (Contact, _name_sort), - "current_websites": (Website, "website"), - "alternative_domains": (Website, "website"), + # == 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"), - "requested_domain": (DraftDomain, "name"), - "domain_application": (DomainApplication, "requested_domain__name"), } @classmethod @@ -155,6 +160,9 @@ class AdminSortFields: case "investigator": # We should only return users who are staff return model.objects.filter(is_staff=True).order_by(order_by) + case "submitter": + db_field_model = db_field.model + db_field_model.objects.select_related("submitter") case _: # If no case is defined, return the default return model.objects.order_by(order_by) @@ -208,21 +216,17 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): Reference: https://code.djangoproject.com/ticket/31975 """ - logger.info("timing get_changelist") - with Timer() as t: - return MultiFieldSortableChangeList + return MultiFieldSortableChangeList def changelist_view(self, request, extra_context=None): - logger.info("timing changelist_view") - with Timer() as t: - if extra_context is None: - extra_context = {} - # Get the filtered values - filters = self.get_filters(request) - # Pass the filtered values to the template context - extra_context["filters"] = filters - extra_context["search_query"] = request.GET.get("q", "") # Assuming the search query parameter is 'q' - return super().changelist_view(request, extra_context=extra_context) + if extra_context is None: + extra_context = {} + # Get the filtered values + filters = self.get_filters(request) + # Pass the filtered values to the template context + extra_context["filters"] = filters + extra_context["search_query"] = request.GET.get("q", "") # Assuming the search query parameter is 'q' + return super().changelist_view(request, extra_context=extra_context) def get_filters(self, request): """Retrieve the current set of parameters being used to filter the table @@ -231,40 +235,38 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): parameter_value: string} TODO: convert investigator id to investigator username """ - logger.info("timing get_filters") - with Timer() as t: - filters = [] - # Retrieve the filter parameters - for param in request.GET.keys(): - # Exclude the default search parameter 'q' - if param != "q" and param != "o": - parameter_name = param.replace("__exact", "").replace("_type", "").replace("__id", " id") + filters = [] + # Retrieve the filter parameters + for param in request.GET.keys(): + # Exclude the default search parameter 'q' + if param != "q" and param != "o": + parameter_name = param.replace("__exact", "").replace("_type", "").replace("__id", " id") - if parameter_name == "investigator id": - # Retrieves the corresponding contact from Users - id_value = request.GET.get(param) - try: - contact = models.User.objects.get(id=id_value) - investigator_name = contact.first_name + " " + contact.last_name + if parameter_name == "investigator id": + # Retrieves the corresponding contact from Users + id_value = request.GET.get(param) + try: + contact = models.User.objects.get(id=id_value) + investigator_name = contact.first_name + " " + contact.last_name - filters.append( - { - "parameter_name": "investigator", - "parameter_value": investigator_name, - } - ) - except models.User.DoesNotExist: - pass - else: - # For other parameter names, append a dictionary with the original - # parameter_name and the corresponding parameter_value filters.append( { - "parameter_name": parameter_name, - "parameter_value": request.GET.get(param), + "parameter_name": "investigator", + "parameter_value": investigator_name, } ) - return filters + except models.User.DoesNotExist: + pass + else: + # For other parameter names, append a dictionary with the original + # parameter_name and the corresponding parameter_value + filters.append( + { + "parameter_name": parameter_name, + "parameter_value": request.GET.get(param), + } + ) + return filters class UserContactInline(admin.StackedInline): @@ -775,46 +777,36 @@ class DomainApplicationAdmin(ListHeaderAdmin): """Lookup reimplementation, gets users of is_staff. Returns a list of tuples consisting of (user.id, user) """ - logger.info("timing lookups") - with Timer() as t: - - # 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" - ) + # 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") + # 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 + return privileged_users_annotated def queryset(self, request, queryset): """Custom queryset implementation, filters by investigator""" - logger.info("timing queryset") - with Timer() as t: - if self.value() is None: - return queryset - else: - return queryset.filter(investigator__id__exact=self.value()) - - def __new__(self, *args, **kwargs): - logger.info("timing __new__") - with Timer() as t: - return super().__new__(self, *args, **kwargs) + if self.value() is None: + return queryset + else: + return queryset.filter(investigator__id__exact=self.value()) # Columns list_display = [ @@ -907,7 +899,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): "anything_else", "is_policy_acknowledged", ] - + autocomplete_fields = ["submitter"] filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") # Table ordering @@ -918,77 +910,73 @@ class DomainApplicationAdmin(ListHeaderAdmin): def formfield_for_manytomany(self, db_field, request, **kwargs): logger.info(f"timing formfield_for_manytomany -> {db_field.name}") with Timer() as t: - 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): + logger.info(f"timing formfield_for_foreignkey -> {db_field.name}") with Timer() as t: - print(f"This is the db_field: {db_field}") return super().formfield_for_foreignkey(db_field, request, **kwargs) # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): - logger.info("timing save_model") - with Timer() as t: - if obj and obj.creator.status != models.User.RESTRICTED: - if change: # Check if the application is being edited - # Get the original application from the database - original_obj = models.DomainApplication.objects.get(pk=obj.pk) + if obj and obj.creator.status != models.User.RESTRICTED: + if change: # Check if the application is being edited + # Get the original application from the database + original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if ( - obj - and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED - and obj.status != models.DomainApplication.ApplicationStatus.APPROVED - and not obj.domain_is_not_active() - ): - # If an admin tried to set an approved application to - # another status and the related domain is already - # active, shortcut the action and throw a friendly - # error message. This action would still not go through - # shortcut or not as the rules are duplicated on the model, - # but the error would be an ugly Django error screen. + if ( + obj + and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED + and obj.status != models.DomainApplication.ApplicationStatus.APPROVED + and not obj.domain_is_not_active() + ): + # If an admin tried to set an approved application to + # another status and the related domain is already + # active, shortcut the action and throw a friendly + # error message. This action would still not go through + # shortcut or not as the rules are duplicated on the model, + # but the error would be an ugly Django error screen. - # Clear the success message - messages.set_level(request, messages.ERROR) + # Clear the success message + messages.set_level(request, messages.ERROR) - messages.error( - request, - "This action is not permitted. The domain is already active.", - ) + messages.error( + request, + "This action is not permitted. The domain is already active.", + ) - else: - if obj.status != original_obj.status: - status_method_mapping = { - models.DomainApplication.ApplicationStatus.STARTED: None, - models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, - models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, - models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, - models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, - models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, - models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, - models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), - } - selected_method = status_method_mapping.get(obj.status) - if selected_method is None: - logger.warning("Unknown status selected in django admin") - else: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. - obj.status = original_obj.status - selected_method() + else: + if obj.status != original_obj.status: + status_method_mapping = { + models.DomainApplication.ApplicationStatus.STARTED: None, + models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, + models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, + models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, + models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, + models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, + models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, + models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), + } + selected_method = status_method_mapping.get(obj.status) + if selected_method is None: + logger.warning("Unknown status selected in django admin") + else: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. + obj.status = original_obj.status + selected_method() - super().save_model(request, obj, form, change) - else: - # Clear the success message - messages.set_level(request, messages.ERROR) + super().save_model(request, obj, form, change) + else: + # Clear the success message + messages.set_level(request, messages.ERROR) - messages.error( - request, - "This action is not permitted for applications with a restricted creator.", - ) + messages.error( + request, + "This action is not permitted for applications with a restricted creator.", + ) def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. @@ -996,41 +984,35 @@ 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. """ - logger.info("timing get_readonly_fields") - with Timer() as t: - readonly_fields = list(self.readonly_fields) + readonly_fields = list(self.readonly_fields) - # Check if the creator is restricted - if obj and obj.creator.status == models.User.RESTRICTED: - # For fields like CharField, IntegerField, etc., the widget used is - # straightforward and the readonly_fields list can control their behavior - readonly_fields.extend([field.name for field in self.model._meta.fields]) - # Add the multi-select fields to readonly_fields: - # Complex fields like ManyToManyField require special handling - readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) + # Check if the creator is restricted + if obj and obj.creator.status == models.User.RESTRICTED: + # For fields like CharField, IntegerField, etc., the widget used is + # straightforward and the readonly_fields list can control their behavior + readonly_fields.extend([field.name for field in self.model._meta.fields]) + # Add the multi-select fields to readonly_fields: + # Complex fields like ManyToManyField require special handling + readonly_fields.extend(["current_websites", "other_contacts", "alternative_domains"]) - if request.user.has_perm("registrar.full_access_permission"): - return readonly_fields - # Return restrictive Read-only fields for analysts and - # users who might not belong to groups - readonly_fields.extend([field for field in self.analyst_readonly_fields]) + if request.user.has_perm("registrar.full_access_permission"): return readonly_fields + # Return restrictive Read-only fields for analysts and + # users who might not belong to groups + readonly_fields.extend([field for field in self.analyst_readonly_fields]) + return readonly_fields def display_restricted_warning(self, request, obj): - logger.info("timing display_restricted_warning") - with Timer() as t: - if obj and obj.creator.status == models.User.RESTRICTED: - messages.warning( - request, - "Cannot edit an application with a restricted creator.", - ) + if obj and obj.creator.status == models.User.RESTRICTED: + messages.warning( + request, + "Cannot edit an application with a restricted creator.", + ) def change_view(self, request, object_id, form_url="", extra_context=None): - logger.info("timing change_view") - with Timer() as t: - obj = self.get_object(request, object_id) - self.display_restricted_warning(request, obj) - return super().change_view(request, object_id, form_url, extra_context) + obj = self.get_object(request, object_id) + self.display_restricted_warning(request, obj) + return super().change_view(request, object_id, form_url, extra_context) class TransitionDomainAdmin(ListHeaderAdmin): diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 760c4f13a..bc0a99067 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -270,3 +270,7 @@ h1, h2, h3, margin: 0!important; } } + +.select2-dropdown { + display: inline-grid !important; +} \ No newline at end of file From 9cb0f7ec62632dd23e01013b51a7f457fd6b3aae Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 13:43:38 -0700 Subject: [PATCH 06/15] Test add autocomplete --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 18f1970da..e4e7c919f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -899,7 +899,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): "anything_else", "is_policy_acknowledged", ] - autocomplete_fields = ["submitter"] + autocomplete_fields = ["approved_domain", "requested_domain", "submitter", "creator", "authorizing_official", "investigator"] filter_horizontal = ("current_websites", "alternative_domains", "other_contacts") # Table ordering From 397e71463ef1a64a6328d8e68d95e08719cdac4d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 14:37:06 -0700 Subject: [PATCH 07/15] Add ordering --- src/registrar/admin.py | 14 +++++++++----- src/registrar/assets/sass/_theme/_admin.scss | 4 ++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e4e7c919f..5c82a42f4 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -160,9 +160,6 @@ class AdminSortFields: case "investigator": # We should only return users who are staff return model.objects.filter(is_staff=True).order_by(order_by) - case "submitter": - db_field_model = db_field.model - db_field_model.objects.select_related("submitter") case _: # If no case is defined, return the default return model.objects.order_by(order_by) @@ -275,7 +272,7 @@ class UserContactInline(admin.StackedInline): model = models.Contact -class MyUserAdmin(BaseUserAdmin): +class UserAdmin(BaseUserAdmin): """Custom user admin class to use our inlines.""" inlines = [UserContactInline] @@ -430,6 +427,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 @@ -1342,6 +1342,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") @@ -1368,7 +1372,7 @@ class VerifiedByStaffAdmin(ListHeaderAdmin): admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) -admin.site.register(models.User, MyUserAdmin) +admin.site.register(models.User, UserAdmin) # Unregister the built-in Group model admin.site.unregister(Group) # Register UserGroup diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index bc0a99067..f08531095 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -271,6 +271,10 @@ h1, h2, h3, } } +.select2-selection__clear { + display: none; +} + .select2-dropdown { display: inline-grid !important; } \ No newline at end of file From 46d6022aed77eb81a530d4cd05a257de6b9657e4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 14 Feb 2024 15:29:10 -0700 Subject: [PATCH 08/15] Test removing concat --- src/registrar/admin.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5c82a42f4..d4ff2e5dc 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -125,7 +125,7 @@ class CustomLogEntryAdmin(LogEntryAdmin): class AdminSortFields: - _name_sort = Concat("first_name", "last_name", "email") + _name_sort = ["first_name", "last_name", "email"] # Define a mapping of field names to model querysets and sort expressions sort_mapping = { # == Contact == # @@ -159,10 +159,13 @@ class AdminSortFields: 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) + return model.objects.filter(is_staff=True).order_by(*order_by) case _: # If no case is defined, return the default - return model.objects.order_by(order_by) + 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): From 3672aaa401a8af69b1fd6b7b72578a698952d1c0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 15 Feb 2024 09:45:18 -0700 Subject: [PATCH 09/15] Fix investigator edge case --- src/registrar/admin.py | 44 +++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d4ff2e5dc..e27ce24d1 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1,6 +1,5 @@ import logging import time -from django.db import transaction from django import forms from django.db.models.functions import Concat, Coalesce from django.db.models import Value, CharField @@ -14,12 +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.contact import Contact -from registrar.models.domain import Domain -from registrar.models.domain_application import DomainApplication -from registrar.models.draft_domain import DraftDomain -from registrar.models.user import User -from registrar.models.website import Website +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 @@ -366,6 +360,42 @@ class UserAdmin(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): From c5f27769becceaf3b9bc3770ba9a01c2d39f6614 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 15 Feb 2024 13:25:49 -0700 Subject: [PATCH 10/15] Fix unit tests and linting --- src/registrar/admin.py | 51 ++++++------- src/registrar/tests/common.py | 1 + src/registrar/tests/test_admin.py | 119 ++++++++++++++++++++---------- 3 files changed, 107 insertions(+), 64 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e27ce24d1..a71014c40 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -13,7 +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 import (Contact, Domain, DomainApplication, DraftDomain, User, Website) +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 @@ -272,6 +272,14 @@ class UserContactInline(admin.StackedInline): class UserAdmin(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 = ( @@ -371,16 +379,16 @@ class UserAdmin(BaseUserAdmin): # 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 - + 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) + 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"} @@ -814,21 +822,15 @@ class DomainApplicationAdmin(ListHeaderAdmin): privileged_users = ( DomainApplication.objects.select_related("investigator") .filter(investigator__is_staff=True) - .order_by( - "investigator__first_name", - "investigator__last_name", - "investigator__email" - ) + .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() - ), + Concat("investigator__first_name", Value(" "), "investigator__last_name", output_field=CharField()), "investigator__email", - output_field=CharField() + output_field=CharField(), ) ).values_list("investigator__id", "full_name") @@ -932,24 +934,19 @@ class DomainApplicationAdmin(ListHeaderAdmin): "anything_else", "is_policy_acknowledged", ] - autocomplete_fields = ["approved_domain", "requested_domain", "submitter", "creator", "authorizing_official", "investigator"] + 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): - logger.info(f"timing formfield_for_manytomany -> {db_field.name}") - with Timer() as t: - return super().formfield_for_manytomany(db_field, request, **kwargs) - - def formfield_for_foreignkey(self, db_field, request, **kwargs): - logger.info(f"timing formfield_for_foreignkey -> {db_field.name}") - with Timer() as t: - 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: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 2865bf5c5..8d2385c23 100644 --- a/src/registrar/tests/common.py +++ b/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 diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index f90b18584..85853cba2 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -9,7 +9,7 @@ from registrar.admin import ( DomainApplicationAdminForm, DomainInvitationAdmin, ListHeaderAdmin, - MyUserAdmin, + UserAdmin, AuditedAdmin, ContactAdmin, DomainInformationAdmin, @@ -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 = UserAdmin(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) @@ -1335,10 +1344,10 @@ class ListHeaderAdminTest(TestCase): User.objects.all().delete() -class MyUserAdminTest(TestCase): +class UserAdminTest(TestCase): def setUp(self): admin_site = AdminSite() - self.admin = MyUserAdmin(model=get_user_model(), admin_site=admin_site) + self.admin = UserAdmin(model=get_user_model(), admin_site=admin_site) def test_list_display_without_username(self): request = self.client.request().wsgi_request @@ -1360,7 +1369,7 @@ class MyUserAdminTest(TestCase): request = self.client.request().wsgi_request request.user = create_superuser() fieldsets = self.admin.get_fieldsets(request) - expected_fieldsets = super(MyUserAdmin, self.admin).get_fieldsets(request) + expected_fieldsets = super(UserAdmin, self.admin).get_fieldsets(request) self.assertEqual(fieldsets, expected_fieldsets) def test_get_fieldsets_cisa_analyst(self): @@ -1396,11 +1405,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 = UserAdmin(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,39 +1462,40 @@ class AuditedAdminTest(TestCase): # but both fields are of a fixed length. # For test case purposes, this should be performant. for field in tested_fields: - isNamefield: bool = field == DomainApplication.requested_domain.field - if isNamefield: - sorted_fields = ["name"] - else: - sorted_fields = ["first_name", "last_name"] - # We want both of these to be lists, as it is richer test wise. - - desired_order = self.order_by_desired_field_helper(model_admin, request, field.name, *sorted_fields) - current_sort_order = list(model_admin.formfield_for_foreignkey(field, request).queryset) - - # Conforms to the same object structure as desired_order - current_sort_order_coerced_type = [] - - # This is necessary as .queryset and get_queryset - # return lists of different types/structures. - # We need to parse this data and coerce them into the same type. - for contact in current_sort_order: - if not isNamefield: - first = contact.first_name - last = contact.last_name + with self.subTest(field=field): + isNamefield: bool = field == DomainApplication.requested_domain.field + if isNamefield: + sorted_fields = ["name"] else: - first = contact.name - last = None + sorted_fields = ["first_name", "last_name"] + # We want both of these to be lists, as it is richer test wise. - name_tuple = self.coerced_fk_field_helper(first, last, field.name, ":") - if name_tuple is not None: - current_sort_order_coerced_type.append(name_tuple) + desired_order = self.order_by_desired_field_helper(model_admin, request, field.name, *sorted_fields) + current_sort_order = list(model_admin.formfield_for_foreignkey(field, request).queryset) - self.assertEqual( - desired_order, - current_sort_order_coerced_type, - "{} is not ordered alphabetically".format(field.name), - ) + # Conforms to the same object structure as desired_order + current_sort_order_coerced_type = [] + + # This is necessary as .queryset and get_queryset + # return lists of different types/structures. + # We need to parse this data and coerce them into the same type. + for contact in current_sort_order: + if not isNamefield: + first = contact.first_name + last = contact.last_name + else: + first = contact.name + last = None + + name_tuple = self.coerced_fk_field_helper(first, last, field.name, ":") + if name_tuple is not None: + current_sort_order_coerced_type.append(name_tuple) + + self.assertEqual( + desired_order, + current_sort_order_coerced_type, + "{} is not ordered alphabetically".format(field.name), + ) def test_alphabetically_sorted_fk_fields_domain_information(self): tested_fields = [ From 2d1982c252dda463d5de5aa51a1bad26dfde9e5b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 15 Feb 2024 13:50:48 -0700 Subject: [PATCH 11/15] Code cleanup --- src/registrar/admin.py | 33 +++++++++-------- src/registrar/assets/sass/_theme/_admin.scss | 2 ++ .../models/utility/generic_helper.py | 36 +++++++++++++++++++ 3 files changed, 57 insertions(+), 14 deletions(-) create mode 100644 src/registrar/models/utility/generic_helper.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a71014c40..27acc0cea 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1,5 +1,5 @@ import logging -import time + from django import forms from django.db.models.functions import Concat, Coalesce from django.db.models import Value, CharField @@ -120,7 +120,11 @@ class CustomLogEntryAdmin(LogEntryAdmin): class AdminSortFields: _name_sort = ["first_name", "last_name", "email"] - # Define a mapping of field names to model querysets and sort expressions + + # 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), @@ -149,10 +153,12 @@ class AdminSortFields: 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 + # We should only return users who are staff. + # Currently a fallback. Consider removing this if it is not needed. return model.objects.filter(is_staff=True).order_by(*order_by) case _: # If no case is defined, return the default @@ -178,9 +184,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 = ( @@ -192,9 +203,14 @@ class AuditedAdmin(admin.ModelAdmin): 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""" + + # 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) @@ -761,17 +777,6 @@ class DomainInformationAdmin(ListHeaderAdmin): return readonly_fields # Read-only fields for analysts -class Timer: - def __enter__(self): - self.start = time.time() - return self # This allows usage of the instance within the with block - - def __exit__(self, *args): - self.end = time.time() - self.duration = self.end - self.start - logger.info(f"Execution time: {self.duration} seconds") - - class DomainApplicationAdminForm(forms.ModelForm): """Custom form to limit transitions to available transitions""" diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index f08531095..93a0d7338 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -271,10 +271,12 @@ h1, h2, h3, } } +// 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; } \ No newline at end of file diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py new file mode 100644 index 000000000..8fdd3eb7a --- /dev/null +++ b/src/registrar/models/utility/generic_helper.py @@ -0,0 +1,36 @@ +"""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") From 133c435f033d8ce75e8a42509c10e6b7030fa7c3 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 15 Feb 2024 14:04:36 -0700 Subject: [PATCH 12/15] Linting, fix test --- src/registrar/models/utility/generic_helper.py | 1 + src/registrar/tests/test_admin.py | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py index 8fdd3eb7a..01d4e6b33 100644 --- a/src/registrar/models/utility/generic_helper.py +++ b/src/registrar/models/utility/generic_helper.py @@ -1,4 +1,5 @@ """This file contains general purpose helpers that don't belong in any specific location""" + import time import logging diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 85853cba2..ce2b40122 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -985,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)] From 92e8877cec3f3a6110d05f28636f9d91b2840f42 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Feb 2024 13:23:57 -0700 Subject: [PATCH 13/15] Update _admin.scss --- src/registrar/assets/sass/_theme/_admin.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 93a0d7338..06a737d62 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -279,4 +279,4 @@ h1, h2, h3, // Fixes a display issue where the list was entirely white, or had too much whitespace .select2-dropdown { display: inline-grid !important; -} \ No newline at end of file +} From 1a088c4b35c4a32a1884133eb81e13c3f7d30fea Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Feb 2024 09:03:35 -0700 Subject: [PATCH 14/15] Readd my user --- src/registrar/admin.py | 14 ++++++-------- src/registrar/tests/test_admin.py | 10 +++++----- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f1680c76a..fc7edf1ca 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -158,10 +158,8 @@ class AdminSortFields: match db_field.name: case "investigator": # We should only return users who are staff. - # Currently a fallback. Consider removing this if it is not needed. return model.objects.filter(is_staff=True).order_by(*order_by) case _: - # If no case is defined, return the default if isinstance(order_by, list) or isinstance(order_by, tuple): return model.objects.order_by(*order_by) else: @@ -201,8 +199,8 @@ 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. @@ -285,7 +283,7 @@ class UserContactInline(admin.StackedInline): model = models.Contact -class UserAdmin(BaseUserAdmin): +class MyUserAdmin(BaseUserAdmin): """Custom user admin class to use our inlines.""" class Meta: @@ -1095,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 @@ -1406,7 +1404,7 @@ class VerifiedByStaffAdmin(ListHeaderAdmin): admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) -admin.site.register(models.User, UserAdmin) +admin.site.register(models.User, MyUserAdmin) # Unregister the built-in Group model admin.site.unregister(Group) # Register UserGroup diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index ce2b40122..4f432728b 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -9,7 +9,7 @@ from registrar.admin import ( DomainApplicationAdminForm, DomainInvitationAdmin, ListHeaderAdmin, - UserAdmin, + MyUserAdmin, AuditedAdmin, ContactAdmin, DomainInformationAdmin, @@ -949,7 +949,7 @@ class TestDomainApplicationAdmin(MockEppLib): user_request = self.factory.post( "/admin/autocomplete/?app_label=registrar&model_name=domainapplication&field_name=investigator" ) - user_admin = UserAdmin(User, self.site) + user_admin = MyUserAdmin(User, self.site) user_queryset = user_admin.get_search_results(user_request, application_queryset, None)[0] current_dropdown = list(user_queryset) @@ -1350,10 +1350,10 @@ class ListHeaderAdminTest(TestCase): User.objects.all().delete() -class UserAdminTest(TestCase): +class MyUserAdminTest(TestCase): def setUp(self): admin_site = AdminSite() - self.admin = UserAdmin(model=get_user_model(), admin_site=admin_site) + self.admin = MyUserAdmin(model=get_user_model(), admin_site=admin_site) def test_list_display_without_username(self): request = self.client.request().wsgi_request @@ -1375,7 +1375,7 @@ class UserAdminTest(TestCase): request = self.client.request().wsgi_request request.user = create_superuser() fieldsets = self.admin.get_fieldsets(request) - expected_fieldsets = super(UserAdmin, self.admin).get_fieldsets(request) + expected_fieldsets = super(MyUserAdmin, self.admin).get_fieldsets(request) self.assertEqual(fieldsets, expected_fieldsets) def test_get_fieldsets_cisa_analyst(self): From 31c24446da46c8f0a7b3cd4e9e679322909d4f17 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Feb 2024 09:06:42 -0700 Subject: [PATCH 15/15] Update test_admin.py --- src/registrar/tests/test_admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 4f432728b..6f1460144 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1435,7 +1435,7 @@ class AuditedAdminTest(TestCase): desired_sort_order = list(User.objects.filter(is_staff=True).order_by(*sorted_fields)) # Grab the data returned from get search results - admin = UserAdmin(User, self.site) + admin = MyUserAdmin(User, self.site) search_queryset = admin.get_search_results(request, application_queryset, None)[0] current_sort_order = list(search_queryset)