diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d91021225..aed06021c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -14,6 +14,8 @@ from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models.domain import Domain from registrar.models.user import User from registrar.utility import csv_export +from registrar.views.utility.mixins import OrderableFieldsMixin +from django.contrib.admin.views.main import ORDER_VAR from . import models from auditlog.models import LogEntry # type: ignore from auditlog.admin import LogEntryAdmin # type: ignore @@ -22,6 +24,73 @@ from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) +# Based off of this excellent example: https://djangosnippets.org/snippets/10471/ +class MultiFieldSortableChangeList(admin.views.main.ChangeList): + """ + This class overrides the behavior of column sorting in django admin tables in order + to allow for multi field sorting on admin_order_field + + + Usage: + + class MyCustomAdmin(admin.ModelAdmin): + + ... + + def get_changelist(self, request, **kwargs): + return MultiFieldSortableChangeList + + ... + + """ + + def get_ordering(self, request, queryset): + """ + Returns the list of ordering fields for the change list. + + 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()) + + 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_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) + + except (IndexError, ValueError): + continue # Invalid ordering specified, skip it. + + # 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") + + return ordering + + class CustomLogEntryAdmin(LogEntryAdmin): """Overwrite the generated LogEntry admin class""" @@ -119,8 +188,19 @@ class AuditedAdmin(admin.ModelAdmin): return super().formfield_for_foreignkey(db_field, request, **kwargs) -class ListHeaderAdmin(AuditedAdmin): - """Custom admin to add a descriptive subheader to list views.""" +class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): + """Custom admin to add a descriptive subheader to list views + and custom table sort behaviour""" + + def get_changelist(self, request, **kwargs): + """Returns a custom ChangeList class, as opposed to the default. + This is so we can override the behaviour of the `admin_order_field` field. + By default, django does not support ordering by multiple fields for this + particular field (i.e. self.admin_order_field=["first_name", "last_name"] is invalid). + + Reference: https://code.djangoproject.com/ticket/31975 + """ + return MultiFieldSortableChangeList def changelist_view(self, request, extra_context=None): if extra_context is None: @@ -399,6 +479,11 @@ class UserDomainRoleAdmin(ListHeaderAdmin): "role", ] + orderable_fk_fields = [ + ("domain", "name"), + ("user", ["first_name", "last_name", "email"]), + ] + # Search search_fields = [ "user__first_name", @@ -468,6 +553,11 @@ class DomainInformationAdmin(ListHeaderAdmin): "submitter", ] + orderable_fk_fields = [ + ("domain", "name"), + ("submitter", ["first_name", "last_name"]), + ] + # Filters list_filter = ["organization_type"] @@ -624,6 +714,12 @@ class DomainApplicationAdmin(ListHeaderAdmin): "investigator", ] + orderable_fk_fields = [ + ("requested_domain", "name"), + ("submitter", ["first_name", "last_name"]), + ("investigator", ["first_name", "last_name"]), + ] + # Filters list_filter = ("status", "organization_type", InvestigatorFilter) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 80f55d584..5efabdb47 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -9,7 +9,7 @@ import uuid from django.test import TestCase from unittest.mock import MagicMock, Mock, patch from typing import List, Dict - +from django.contrib.sessions.middleware import SessionMiddleware from django.conf import settings from django.contrib.auth import get_user_model, login @@ -93,6 +93,73 @@ def less_console_noise(output_stream=None): output_stream.close() +class GenericTestHelper(TestCase): + """A helper class that contains various helper functions for TestCases""" + + def __init__(self, admin, model=None, url=None, user=None, factory=None, **kwargs): + """ + Parameters: + admin (ModelAdmin): The Django ModelAdmin instance associated with the model. + model (django.db.models.Model, optional): The Django model associated with the admin page. + url (str, optional): The URL of the Django Admin page to test. + user (User, optional): The Django User who is making the request. + factory (RequestFactory, optional): An instance of Django's RequestFactory. + """ + super().__init__() + self.factory = factory + self.user = user + self.admin = admin + self.model = model + self.url = url + + def assert_table_sorted(self, o_index, sort_fields): + """ + This helper function validates the sorting functionality of a Django Admin table view. + + It creates a mock HTTP GET request to the provided URL with a specific ordering parameter, + and compares the returned sorted queryset with the expected sorted queryset. + + Parameters: + o_index (str): The index of the field in the table to sort by. This is passed as a string + to the 'o' parameter in the GET request. + sort_fields (tuple): The fields of the model to sort by. These fields are used to generate + the expected sorted queryset. + + + Example Usage: + ``` + self.assert_sort_helper( + self.factory, self.superuser, self.admin, self.url, DomainInformation, "1", ("domain__name",) + ) + ``` + + The function asserts that the returned sorted queryset from the admin page matches the + expected sorted queryset. If the assertion fails, it means the sorting functionality + on the admin page is not working as expected. + """ + # 'o' is a search param defined by the current index position in the + # table, plus one. + dummy_request = self.factory.get( + self.url, + {"o": o_index}, + ) + dummy_request.user = self.user + + # Mock a user request + middleware = SessionMiddleware(lambda req: req) + middleware.process_request(dummy_request) + dummy_request.session.save() + + expected_sort_order = list(self.model.objects.order_by(*sort_fields)) + + # Use changelist_view to get the sorted queryset + response = self.admin.changelist_view(dummy_request) + response.render() # Render the response before accessing its content + returned_sort_order = list(response.context_data["cl"].result_list) + + self.assertEqual(expected_sort_order, returned_sort_order) + + class MockUserLogin: def __init__(self, get_response): self.get_response = get_response @@ -273,6 +340,7 @@ class AuditedAdminMockData: creator: User = self.dummy_user(item_name, "creator"), } """ # noqa + creator = self.dummy_user(item_name, "creator") common_args = dict( organization_type=org_type, federal_type=federal_type, @@ -287,7 +355,7 @@ class AuditedAdminMockData: anything_else="There is more", authorizing_official=self.dummy_contact(item_name, "authorizing_official"), submitter=self.dummy_contact(item_name, "submitter"), - creator=self.dummy_user(item_name, "creator"), + creator=creator, ) return common_args diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index cc7b97449..810e36170 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3,7 +3,6 @@ from django.contrib.admin.sites import AdminSite from contextlib import ExitStack from django.contrib import messages from django.urls import reverse - from registrar.admin import ( DomainAdmin, DomainApplicationAdmin, @@ -13,11 +12,13 @@ from registrar.admin import ( MyUserAdmin, AuditedAdmin, ContactAdmin, + DomainInformationAdmin, UserDomainRoleAdmin, ) from registrar.models import Domain, DomainApplication, DomainInformation, User, DomainInvitation, Contact, Website from registrar.models.user_domain_role import UserDomainRole from .common import ( + AuditedAdminMockData, completed_application, generic_domain_object, mock_user, @@ -26,6 +27,7 @@ from .common import ( create_ready_domain, multiple_unalphabetical_domain_objects, MockEppLib, + GenericTestHelper, ) from django.contrib.sessions.backends.db import SessionStore from django.contrib.auth import get_user_model @@ -317,6 +319,85 @@ class TestDomainApplicationAdmin(MockEppLib): self.superuser = create_superuser() self.staffuser = create_user() self.client = Client(HTTP_HOST="localhost:8080") + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/DomainApplication/", + model=DomainApplication, + ) + + def test_domain_sortable(self): + """Tests if the DomainApplication sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted("1", ("requested_domain__name",)) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-1", ("-requested_domain__name",)) + + def test_submitter_sortable(self): + """Tests if the DomainApplication sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + + additional_application = generic_domain_object("application", "Xylophone") + new_user = User.objects.filter(username=additional_application.investigator.username).get() + new_user.first_name = "Xylophonic" + new_user.save() + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted( + "5", + ( + "submitter__first_name", + "submitter__last_name", + ), + ) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted( + "-5", + ( + "-submitter__first_name", + "-submitter__last_name", + ), + ) + + def test_investigator_sortable(self): + """Tests if the DomainApplication sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + multiple_unalphabetical_domain_objects("application") + additional_application = generic_domain_object("application", "Xylophone") + new_user = User.objects.filter(username=additional_application.investigator.username).get() + new_user.first_name = "Xylophonic" + new_user.save() + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted( + "6", + ( + "investigator__first_name", + "investigator__last_name", + ), + ) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted( + "-6", + ( + "-investigator__first_name", + "-investigator__last_name", + ), + ) def test_short_org_name_in_applications_list(self): """ @@ -928,52 +1009,6 @@ class TestDomainApplicationAdmin(MockEppLib): ], ) - def test_investigator_filter_filters_correctly(self): - """ - This test verifies that the investigator filter in the admin interface for - the DomainApplication model works correctly. - - It creates two DomainApplication instances, each with a different investigator. - It then simulates a staff user logging in and applying the investigator filter - on the DomainApplication admin page. - - It then verifies that it was applied correctly. - The test checks that the response contains the expected DomainApplication pbjects - in the table. - """ - - # Create a mock DomainApplication object, with a fake investigator - application: DomainApplication = generic_domain_object("application", "SomeGuy") - investigator_user = User.objects.filter(username=application.investigator.username).get() - investigator_user.is_staff = True - investigator_user.save() - - # Create a second mock DomainApplication object, to test filtering - application: DomainApplication = generic_domain_object("application", "BadGuy") - another_user = User.objects.filter(username=application.investigator.username).get() - another_user.is_staff = True - another_user.save() - - p = "userpass" - self.client.login(username="staffuser", password=p) - response = self.client.get( - "/admin/registrar/domainapplication/", - { - "investigator__id__exact": investigator_user.id, - }, - follow=True, - ) - - expected_name = "SomeGuy first_name:investigator SomeGuy last_name:investigator" - # We expect to see this four times, two of them are from the html for the filter, - # and the other two are the html from the list entry in the table. - self.assertContains(response, expected_name, count=4) - - # Check that we don't also get the thing we aren't filtering for. - # We expect to see this two times in the filter - unexpected_name = "BadGuy first_name:investigator BadGuy last_name:investigator" - self.assertContains(response, unexpected_name, count=2) - def test_investigator_dropdown_displays_only_staff(self): """ This test verifies that the dropdown for the 'investigator' field in the DomainApplicationAdmin @@ -1098,14 +1133,98 @@ class DomainInvitationAdminTest(TestCase): self.assertContains(response, retrieved_html, count=1) +class DomainInformationAdminTest(TestCase): + def setUp(self): + """Setup environment for a mock admin user""" + self.site = AdminSite() + self.factory = RequestFactory() + self.admin = DomainInformationAdmin(model=DomainInformation, admin_site=self.site) + self.client = Client(HTTP_HOST="localhost:8080") + self.superuser = create_superuser() + self.mock_data_generator = AuditedAdminMockData() + + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/DomainInformation/", + model=DomainInformation, + ) + + # Create fake DomainInformation objects + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Apple"), + submitter=self.mock_data_generator.dummy_contact("Zebra", "submitter"), + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Zebra"), + submitter=self.mock_data_generator.dummy_contact("Apple", "submitter"), + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Circus"), + submitter=self.mock_data_generator.dummy_contact("Xylophone", "submitter"), + ) + + DomainInformation.objects.create( + creator=self.mock_data_generator.dummy_user("fake", "creator"), + domain=self.mock_data_generator.dummy_domain("Xylophone"), + submitter=self.mock_data_generator.dummy_contact("Circus", "submitter"), + ) + + def tearDown(self): + """Delete all Users, Domains, and UserDomainRoles""" + DomainInformation.objects.all().delete() + DomainApplication.objects.all().delete() + Domain.objects.all().delete() + Contact.objects.all().delete() + User.objects.all().delete() + + def test_domain_sortable(self): + """Tests if DomainInformation sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted("1", ("domain__name",)) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-1", ("-domain__name",)) + + def test_submitter_sortable(self): + """Tests if DomainInformation sorts by submitter correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted( + "4", + ("submitter__first_name", "submitter__last_name"), + ) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-4", ("-submitter__first_name", "-submitter__last_name")) + + class UserDomainRoleAdminTest(TestCase): def setUp(self): """Setup environment for a mock admin user""" self.site = AdminSite() self.factory = RequestFactory() - self.admin = ListHeaderAdmin(model=UserDomainRoleAdmin, admin_site=None) + self.admin = UserDomainRoleAdmin(model=UserDomainRole, admin_site=self.site) self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() + self.test_helper = GenericTestHelper( + factory=self.factory, + user=self.superuser, + admin=self.admin, + url="/admin/registrar/UserDomainRole/", + model=UserDomainRole, + ) def tearDown(self): """Delete all Users, Domains, and UserDomainRoles""" @@ -1113,6 +1232,48 @@ class UserDomainRoleAdminTest(TestCase): Domain.objects.all().delete() UserDomainRole.objects.all().delete() + def test_domain_sortable(self): + """Tests if the UserDomainrole sorts by domain correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + fake_user = User.objects.create( + username="dummyuser", first_name="Stewart", last_name="Jones", email="AntarcticPolarBears@example.com" + ) + + # Create a list of UserDomainRoles that are in random order + mocks_to_create = ["jkl.gov", "ghi.gov", "abc.gov", "def.gov"] + for name in mocks_to_create: + fake_domain = Domain.objects.create(name=name) + UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted("2", ("domain__name",)) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-2", ("-domain__name",)) + + def test_user_sortable(self): + """Tests if the UserDomainrole sorts by user correctly""" + p = "adminpass" + self.client.login(username="superuser", password=p) + + mock_data_generator = AuditedAdminMockData() + + fake_domain = Domain.objects.create(name="igorville.gov") + # Create a list of UserDomainRoles that are in random order + mocks_to_create = ["jkl", "ghi", "abc", "def"] + for name in mocks_to_create: + # Creates a fake "User" object + fake_user = mock_data_generator.dummy_user(name, "user") + UserDomainRole.objects.create(user=fake_user, domain=fake_domain, role="manager") + + # Assert that our sort works correctly + self.test_helper.assert_table_sorted("1", ("user__first_name", "user__last_name")) + + # Assert that sorting in reverse works correctly + self.test_helper.assert_table_sorted("-1", ("-user__first_name", "-user__last_name")) + def test_email_not_in_search(self): """Tests the search bar in Django Admin for UserDomainRoleAdmin. Should return no results for an invalid email.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ceabef11e..6124b76f3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -22,6 +22,7 @@ from django_fsm import TransitionNotAllowed boto3_mocking.clients.register_handler("sesv2", MockSESClient) +# Test comment for push -- will remove # The DomainApplication submit method has a side effect of sending an email # with AWS SES, so mock that out in all of these test cases @boto3_mocking.patching diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 5ff291d69..0cf5970df 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -15,6 +15,136 @@ import logging logger = logging.getLogger(__name__) +class OrderableFieldsMixin: + """ + Mixin to add multi-field ordering capabilities to a Django ModelAdmin on admin_order_field. + """ + + custom_sort_name_prefix = "get_sortable_" + orderable_fk_fields = [] # type: ignore + + def __new__(cls, *args, **kwargs): + """ + This magic method is called when a new instance of the class (or subclass) is created. + It dynamically adds a new method to the class for each field in `orderable_fk_fields`. + Then, it will update the `list_display` attribute such that it uses these generated methods. + """ + new_class = super().__new__(cls) + + # If the class doesn't define anything for orderable_fk_fields, then we should + # just skip this additional logic + if not hasattr(cls, "orderable_fk_fields") or len(cls.orderable_fk_fields) == 0: + return new_class + + # Check if the list_display attribute exists, and if it does, create a local copy of that list. + list_display_exists = hasattr(cls, "list_display") and isinstance(cls.list_display, list) + new_list_display = cls.list_display.copy() if list_display_exists else [] + + for field, sort_field in cls.orderable_fk_fields: + updated_name = cls.custom_sort_name_prefix + field + + # For each item in orderable_fk_fields, create a function and associate it with admin_order_field. + setattr(new_class, updated_name, cls._create_orderable_field_method(field, sort_field)) + + # Update the list_display variable to use our newly created functions + if list_display_exists and field in cls.list_display: + index = new_list_display.index(field) + new_list_display[index] = updated_name + elif list_display_exists: + new_list_display.append(updated_name) + + # Replace the old list with the updated one + if list_display_exists: + cls.list_display = new_list_display + + return new_class + + @classmethod + def _create_orderable_field_method(cls, field, sort_field): + """ + This class method is a factory for creating dynamic methods that will be attached + to the ModelAdmin subclass. + It is used to customize how fk fields are ordered. + + In essence, this function will more or less generate code that looks like this, + for a given tuple defined in orderable_fk_fields: + + ``` + def get_sortable_requested_domain(self, obj): + return obj.requested_domain + # Allows column order sorting + get_sortable_requested_domain.admin_order_field = "requested_domain__name" + # Sets column's header name + get_sortable_requested_domain.short_description = "requested domain" + ``` + + Or for fields with multiple order_fields: + + ``` + def get_sortable_submitter(self, obj): + return obj.submitter + # Allows column order sorting + get_sortable_submitter.admin_order_field = ["submitter__first_name", "submitter__last_name"] + # Sets column's header + get_sortable_submitter.short_description = "submitter" + ``` + + Parameters: + cls: The class that this method is being called on. In the context of this mixin, + it would be the ModelAdmin subclass. + field: A string representing the name of the attribute that + the dynamic method will fetch from the model instance. + sort_field: A string or list of strings representing the + field(s) to sort by (ex: "name" or "creator") + + Returns: + method: The dynamically created method. + + The dynamically created method has the following attributes: + __name__: A string representing the name of the method. This is set to "get_{field}". + admin_order_field: A string or list of strings representing the field(s) that + Django should sort by when the column is clicked in the admin interface. + short_description: A string used as the column header in the admin interface. + Will replace underscores with spaces. + """ + + def method(obj): + """ + Template method for patterning. + + Returns (example): + ``` + def get_submitter(self, obj): + return obj.submitter + ``` + """ + attr = getattr(obj, field) + return attr + + # Set the function name. For instance, if the field is "domain", + # then this will generate a function called "get_sort_domain". + # This is done rather than just setting the name to the attribute to avoid + # naming conflicts. + method.__name__ = cls.custom_sort_name_prefix + field + + # Check if a list is passed in, or just a string. + if isinstance(sort_field, list): + sort_list = [] + for sort_field_item in sort_field: + order_field_string = f"{field}__{sort_field_item}" + sort_list.append(order_field_string) + # If its a list, return an array of fields to sort on. + # For instance, ["creator__first_name", "creator__last_name"] + method.admin_order_field = sort_list + else: + # If its not a list, just return a string + method.admin_order_field = f"{field}__{sort_field}" + + # Infer the column name in a similar manner to how Django does + method.short_description = field.replace("_", " ") + return method + + class PermissionsLoginMixin(PermissionRequiredMixin): """Mixin that redirects to login page if not logged in, otherwise 403."""