diff --git a/src/registrar/admin.py b/src/registrar/admin.py index c5f5be276..fc7edf1ca 100644 --- a/src/registrar/admin.py +++ b/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") diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 760c4f13a..06a737d62 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/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; +} diff --git a/src/registrar/models/utility/generic_helper.py b/src/registrar/models/utility/generic_helper.py new file mode 100644 index 000000000..01d4e6b33 --- /dev/null +++ b/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") 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..6f1460144 100644 --- a/src/registrar/tests/test_admin.py +++ b/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,39 +1468,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 = [