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 = [