diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9729e018c..124553c31 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -16,7 +16,7 @@ audited_admin_orderby_names = ['first_name', 'last_name'] contact_admin_item_names = ["domain", "requested_domain"] contact_admin_orderby_names = ["name"] # Used to keep track of how we want to order_by certain FKs -foreignkey_orderby_dict: [SortingDictInterface] = [ +foreignkey_orderby_dict: list[SortingDictInterface] = [ # foreign_key - order_by SortingDictInterface( audited_admin_item_names, diff --git a/src/registrar/models/utility/admin_form_order_helper.py b/src/registrar/models/utility/admin_form_order_helper.py index d29951cf7..8957262b3 100644 --- a/src/registrar/models/utility/admin_form_order_helper.py +++ b/src/registrar/models/utility/admin_form_order_helper.py @@ -31,14 +31,17 @@ class AdminFormOrderHelper(): _order_by_list = [] for item in self._sorting_dict: + # Used to disable black as this is a false positive + # fmt: off drop_down_selected = item.get("dropDownSelected") + # fmt: on sort_by = item.get("sortBy") if db_field.name in drop_down_selected: _order_by_list = sort_by break # Only order if we choose to do so - if _order_by_list: + if not _order_by_list is None: form_field.queryset = form_field.queryset.order_by(*_order_by_list) return form_field diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 9597065b7..ef4428177 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -10,8 +10,13 @@ from typing import List, Dict from django.conf import settings from django.contrib.auth import get_user_model, login -from registrar.models import Contact, DraftDomain, Website, DomainApplication, User +from registrar.models import Contact, DraftDomain, Website, DomainApplication, DomainInvitation, User +import logging +from registrar.models.domain import Domain +from registrar.models.domain_information import DomainInformation + +logger = logging.getLogger(__name__) def get_handlers(): """Obtain pointers to all StreamHandlers.""" @@ -90,6 +95,170 @@ class MockSESClient(Mock): self.EMAILS_SENT.append({"args": args, "kwargs": kwargs}) +class AuditedAdminMockData: + """Creates simple data mocks for AuditedAdminTest""" + + # Constants for different domain object types + INFORMATION = "information" + APPLICATION = "application" + INVITATION = "invitation" + + # These all can likely be generalized more if necessary, particulary with shorthands. + # These are kept basic for now. + # As for why we have shorthands to begin with: + # .queryset returns a list of all objects of the same type, + # rather than by seperating out those fields. + # For such scenarios, the shorthand allows us to not only id a field, + # but append additional information to it. + # This is useful for that scenario and outside it for identifying if values swapped unexpectedly + def dummy_user(self, item_name, shorthand): + """Creates a dummy user object, + but with a shorthand and support for multiple""" + user = User.objects.get_or_create( + first_name="{} First:{}".format(item_name, shorthand), + last_name="{} Last:{}".format(item_name, shorthand), + username="{} username:{}".format(item_name, shorthand) + )[0] + return user + + def dummy_contact(self, item_name, shorthand): + """Creates a dummy contact object""" + contact = Contact.objects.get_or_create( + first_name="{} First:{}".format(item_name, shorthand), + last_name="{} Last:{}".format(item_name, shorthand), + title="{} title:{}".format(item_name, shorthand), + email="{}testy@town.com".format(item_name), + phone="(555) 555 5555" + )[0] + return contact + + def dummy_draft_domain(self,item_name): + """Creates a dummy draft domain object""" + return DraftDomain.objects.get_or_create(name="city{}.gov".format(item_name))[0] + + def dummy_domain(self,item_name): + """Creates a dummy domain object""" + return Domain.objects.get_or_create(name="city{}.gov".format(item_name))[0] + + def dummy_alt(self, item_name): + """Creates a dummy website object for alternates""" + return Website.objects.get_or_create(website="cityalt{}.gov".format(item_name))[0] + + def dummy_current(self, item_name): + """Creates a dummy website object for current""" + return Website.objects.get_or_create(website="city{}.com".format(item_name))[0] + + def get_common_domain_arg_dictionary(self, item_name, org_type="federal", federal_type="executive", purpose="Purpose of the site"): + """Generates a generic argument list for most domains""" + common_args = dict( + organization_type=org_type, + federal_type=federal_type, + purpose=purpose, + organization_name="{} Testorg".format(item_name), + address_line1="{} address 1".format(item_name), + address_line2="{} address 2".format(item_name), + is_policy_acknowledged=True, + state_territory="NY", + zipcode="10002", + type_of_work = 'e-Government', + 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'), + ) + return common_args + + # This can be boiled down more, though for our purposes this is OK + def dummy_kwarg_boilerplate(self, domain_type, item_name, status, org_type="federal", federal_type="executive", purpose="Purpose of the site"): + """Returns kwargs for different domain object types""" + common_args = self.get_common_domain_arg_dictionary(item_name, org_type, federal_type, purpose) + full_arg_list = None + match domain_type: + case self.APPLICATION: + full_arg_list = dict( + **common_args, + requested_domain = self.dummy_draft_domain(item_name), + investigator = self.dummy_user(item_name, 'investigator'), + status = status, + ) + case self.INFORMATION: + full_arg_list = dict( + **common_args, + domain = self.dummy_domain(item_name), + domain_application = self.create_full_dummy_domain_application(item_name) + ) + case self.INVITATION: + full_arg_list = dict( + email = "test_mail@mail.com", + domain = self.dummy_domain(item_name), + status = DomainInvitation.INVITED + ) + return full_arg_list + + def create_full_dummy_domain_application( + self, + object_name, + status=DomainApplication.STARTED + ): + """Creates a dummy domain application object""" + domain_application_kwargs = self.dummy_kwarg_boilerplate(self.APPLICATION, object_name, status) + application = DomainApplication.objects.get_or_create(**domain_application_kwargs)[0] + return application + + def create_full_dummy_domain_information( + self, + object_name, + status=DomainApplication.STARTED + ): + """Creates a dummy domain information object""" + domain_application_kwargs = self.dummy_kwarg_boilerplate(self.INFORMATION, object_name, status) + application = DomainInformation.objects.get_or_create(**domain_application_kwargs)[0] + return application + + def create_full_dummy_domain_invitation( + self, + object_name, + status=DomainApplication.STARTED + ): + """Creates a dummy domain invitation object""" + domain_application_kwargs = self.dummy_kwarg_boilerplate(self.INVITATION, object_name, status) + application = DomainInvitation.objects.get_or_create(**domain_application_kwargs)[0] + + return application + + def create_full_dummy_domain_object( + self, + domain_type, + object_name, + has_other_contacts=True, + has_current_website=True, + has_alternative_gov_domain=True, + status=DomainApplication.STARTED + ): + """A helper to create a dummy domain application object""" + application = None + match domain_type: + case self.APPLICATION: + application = self.create_full_dummy_domain_application(object_name, status) + case self.INVITATION: + application = self.create_full_dummy_domain_invitation(object_name, status) + case self.INFORMATION: + application = self.create_full_dummy_domain_information(object_name, status) + case _: + raise ValueError("Invalid domain_type, must conform to given constants") + + if has_other_contacts and domain_type != self.INVITATION: + other = self.dummy_contact(object_name, 'other') + application.other_contacts.add(other) + if has_current_website and domain_type == self.APPLICATION: + current = self.dummy_current(object_name) + application.current_websites.add(current) + if has_alternative_gov_domain and domain_type == self.APPLICATION: + alt = self.dummy_alt(object_name) + application.alternative_domains.add(alt) + + return application + def mock_user(): """A simple user.""" user_kwargs = dict( @@ -192,84 +361,15 @@ def completed_application( return application -def multiple_unalphabetical_applications( - has_other_contacts=True, - has_current_website=True, - has_alternative_gov_domain=True, - has_type_of_work=True, - has_anything_else=True, - status=DomainApplication.STARTED, - user=False, -): +def multiple_unalphabetical_domain_objects(domain_type = AuditedAdminMockData.APPLICATION): + """Returns a list of generic domain objects for testing purposes""" applications = [] list_of_letters = list(ascii_uppercase) random.shuffle(list_of_letters) - for x in list_of_letters: - user = get_user_model().objects.create( - first_name="{} First:cre".format(x), - last_name="{} Last:cre".format(x), - username="{} username:cre".format(x) - ) - ao, _ = Contact.objects.get_or_create( - first_name="{} First:ao".format(x), - last_name="{} Last:ao".format(x), - title="{} Chief Tester".format(x), - email="testy@town.com", - phone="(555) 555 5555", - ) - domain, _ = DraftDomain.objects.get_or_create(name="city{}.gov".format(x)) - alt, _ = Website.objects.get_or_create(website="cityalt{}.gov".format(x)) - current, _ = Website.objects.get_or_create(website="city{}.com".format(x)) - you, _ = Contact.objects.get_or_create( - first_name="{} First:you".format(x), - last_name="{} Last:you".format(x), - title="{} Admin Tester".format(x), - email="mayor@igorville.gov", - phone="(555) 555 5556", - ) - other, _ = Contact.objects.get_or_create( - first_name="{} First:other".format(x), - last_name="{} Last:other".format(x), - title="{} Another Tester".format(x), - email="{}testy2@town.com".format(x), - phone="(555) 555 5557", - ) - inv, _ = User.objects.get_or_create( - first_name="{} First:inv".format(x), - last_name="{} Last:inv".format(x), - username="{} username:inv".format(x) - ) - domain_application_kwargs = dict( - organization_type="federal", - federal_type="executive", - purpose="Purpose of the site", - is_policy_acknowledged=True, - organization_name="{}Testorg".format(x), - address_line1="address 1", - address_line2="address 2", - state_territory="NY", - zipcode="10002", - authorizing_official=ao, - requested_domain=domain, - submitter=you, - creator=user, - status=status, - investigator=inv - ) - if has_type_of_work: - domain_application_kwargs["type_of_work"] = "e-Government" - if has_anything_else: - domain_application_kwargs["anything_else"] = "There is more" - application, _ = DomainApplication.objects.get_or_create( - **domain_application_kwargs - ) - - if has_other_contacts: - application.other_contacts.add(other) - if has_current_website: - application.current_websites.add(current) - if has_alternative_gov_domain: - application.alternative_domains.add(alt) + mock = AuditedAdminMockData() + for object_name in list_of_letters: + application = mock.create_full_dummy_domain_object(domain_type, object_name) applications.append(application) return applications + diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 6575f3a4b..490442f00 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1,9 +1,14 @@ from django.test import TestCase, RequestFactory, Client from django.contrib.admin.sites import AdminSite -from registrar.admin import DomainApplicationAdmin, ListHeaderAdmin, MyUserAdmin, AuditedAdmin # noqa +from registrar.admin import DomainApplicationAdmin, ListHeaderAdmin +# Need to split these up due to the linter +from registrar.admin import MyUserAdmin, AuditedAdmin from registrar.models import DomainApplication, DomainInformation, User from registrar.models.contact import Contact -from .common import completed_application, mock_user, create_superuser, create_user, multiple_unalphabetical_applications # noqa +from registrar.models.domain_invitation import DomainInvitation +from .common import completed_application, mock_user, create_superuser, create_user +# Need to split these up due to the linter +from .common import multiple_unalphabetical_domain_objects from django.contrib.auth import get_user_model from django.conf import settings @@ -379,20 +384,32 @@ class AuditedAdminTest(TestCase): self.factory = RequestFactory() self.client = Client(HTTP_HOST="localhost:8080") + def order_by_desired_field_helper(self, obj_to_sort: AuditedAdmin, request, field_name, *obj_names): + formatted_sort_fields = [] + for obj in obj_names: + formatted_sort_fields.append("{}__{}".format(field_name, obj)) + + # Not really a fan of how this looks, but as the linter demands... + ordered_list = list( + obj_to_sort.get_queryset(request).order_by( + *formatted_sort_fields).values_list( + *formatted_sort_fields) + ) + + return ordered_list + + # Q: These three tests can be generalized into an object, + # is it worth the time investment to do so? def test_alphabetically_sorted_fk_fields_domain_application(self): tested_fields = [ - # field (0) - field shorthand (1) - # the 'field shorthand' is used for type coercion. - # It is only used here to reduce boilerplate, - # but keep in mind that it is not needed outside this class. - (DomainApplication.authorizing_official.field, 'ao'), - (DomainApplication.submitter.field, 'you'), - (DomainApplication.investigator.field, 'inv'), - (DomainApplication.creator.field, 'cre') + DomainApplication.authorizing_official.field, + DomainApplication.submitter.field, + DomainApplication.investigator.field, + DomainApplication.creator.field, ] # Create a sample application - review status does not matter - applications = multiple_unalphabetical_applications() + applications = multiple_unalphabetical_domain_objects("application") # Create a mock request request = self.factory.post( @@ -405,19 +422,11 @@ class AuditedAdminTest(TestCase): # but both fields are of a fixed length. # For test case purposes, this should be performant. for field in tested_fields: - field_name = field[0].name - first_name_field = "{}__first_name".format(field_name) - last_name_field = "{}__last_name".format(field_name) # We want both of these to be lists, as it is richer test wise. - # Not really a fan of how this looks, but as the linter demands... - desired_order = list( - model_admin.get_queryset(request).order_by( - first_name_field, last_name_field).values_list( - first_name_field, last_name_field) - ) + desired_order = self.order_by_desired_field_helper(model_admin, request, field.name, "first_name", "last_name") current_sort_order: Contact = list( - model_admin.formfield_for_foreignkey(field[0], request).queryset + model_admin.formfield_for_foreignkey(field, request).queryset ) # Conforms to the same object structure as desired_order @@ -430,24 +439,127 @@ class AuditedAdminTest(TestCase): first = contact.first_name last = contact.last_name - name_tuple = self.coerced_fk_field_helper(first, last, field[1], ':') + name_tuple = self.coerced_fk_field_helper(first, last, field.name, ':') if name_tuple: current_sort_order_coerced_type.append((first, last)) self.assertEqual(desired_order, - current_sort_order_coerced_type, - "{} is not ordered alphabetically".format(field_name)) + current_sort_order_coerced_type, + "{} is not ordered alphabetically".format(field.name)) + + def test_alphabetically_sorted_fk_fields_domain_information(self): + tested_fields = [ + DomainInformation.authorizing_official.field, + DomainInformation.submitter.field, + DomainInformation.domain.field, + DomainInformation.creator.field + ] + + # Create a sample application - review status does not matter + applications = multiple_unalphabetical_domain_objects("information") + + # Create a mock request + request = self.factory.post( + "/admin/registrar/domaininformation/{}/change/".format(applications[0].pk) + ) + + model_admin = AuditedAdmin(DomainInformation, self.site) + + sorted_fields = [] + # Typically we wouldn't want two nested for fields, + # but both fields are of a fixed length. + # For test case purposes, this should be performant. + for field in tested_fields: + isNamefield: bool = (field == DomainInformation.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 + else: + first = contact.name + last = None + + name_tuple = self.coerced_fk_field_helper(first, last, field.name, ':') + if not name_tuple is 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_invitation(self): + tested_fields = [DomainInvitation.domain.field] + + # Create a sample application - review status does not matter + applications = multiple_unalphabetical_domain_objects("invitation") + + # Create a mock request + request = self.factory.post( + "/admin/registrar/domaininvitation/{}/change/".format(applications[0].pk) + ) + + model_admin = AuditedAdmin(DomainInvitation, self.site) + + sorted_fields = [] + # Typically we wouldn't want two nested for fields, + # but both fields are of a fixed length. + # For test case purposes, this should be performant. + for field in tested_fields: + sorted_fields = ["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: + first = contact.name + last = None + + name_tuple = self.coerced_fk_field_helper(first, last, field.name, ':') + if not name_tuple is 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)) - # I originally spent some time trying to fully - # generalize this to replace the match/arg fields, - # but I think for this specific use case - # its not necessary since it'll only be used here. def coerced_fk_field_helper(self, first_name, last_name, field_name, queryset_shorthand): # noqa + returned_tuple = (first_name, last_name) + # Handles edge case for names - structured strangely + if last_name is None: + return (first_name,) + if(first_name and first_name.split(queryset_shorthand)[1] == field_name): # noqa - return (first_name, last_name) + return returned_tuple else: return None def tearDown(self): DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() + DomainInvitation.objects.all().delete()