PR changes

src/registrar/admin.py

@@ -4,10 +4,7 @@ from django.contrib.auth.admin import UserAdmin as BaseUserAdmin
 from django.contrib.contenttypes.models import ContentType
 from django.http.response import HttpResponseRedirect
 from django.urls import reverse
-from registrar.models.utility.admin_form_order_helper import AdminFormOrderHelper
-
-# Split up for the linter
-from registrar.models.utility.admin_form_order_helper import SortingDict
+from registrar.models.utility.admin_form_order_helper import AdminFormOrderHelper, SortingDict # noqa
 from . import models
 
 logger = logging.getLogger(__name__)

src/registrar/models/utility/admin_form_order_helper.py

@@ -10,24 +10,17 @@ class SortingDict:
 
     _sorting_dict: Dict[type, type] = {}
 
-    # model_list can be will be called multiple times.
-    # Not super necessary, but it'd be nice
-    # to have the perf advantage of a dictionary,
-    # while minimizing typing when
-    # adding a new SortingDict (input as a list)
-    def convert_list_to_dict(self, value_list):
-        """Used internally to convert model_list to a dictionary"""
-        dictionary: Dict[type, type] = {}
-        for item in value_list:
-            dictionary[item] = item
-        return dictionary
-
     def __init__(self, model_list, sort_list):
         self._sorting_dict = {
             "dropDownSelected": self.convert_list_to_dict(model_list),
             "sortBy": sort_list,
         }
 
+    # Used in __init__ for model_list for performance reasons
+    def convert_list_to_dict(self, value_list):
+        """Used internally to convert model_list to a dictionary"""
+        return {item: item for item in value_list}
+
     def get_dict(self):
         """Grabs the associated dictionary item,
         has two fields: 'dropDownSelected': model_list and 'sortBy': sort_list"""
@@ -59,10 +52,12 @@ class AdminFormOrderHelper:
 
             if db_field.name in drop_down_selected:
                 _order_by_list = sort_by
+                # Exit loop when order_by_list is found
                 break
 
         # Only order if we choose to do so
-        if _order_by_list is not None:
+        # noqa for the linter... reduces readability otherwise
+        if _order_by_list is not None and _order_by_list != []: # noqa
             form_field.queryset = form_field.queryset.order_by(*_order_by_list)
 
         return form_field

src/registrar/tests/common.py

@@ -10,10 +10,7 @@ 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
-
-# For the linter
-from registrar.models import DomainInvitation, User, DomainInformation, Domain
+from registrar.models import Contact, DraftDomain, Website, DomainApplication, DomainInvitation, User, DomainInformation, Domain # noqa
 
 logger = logging.getLogger(__name__)
 
@@ -96,39 +93,51 @@ class MockSESClient(Mock):
 
 
 class AuditedAdminMockData:
-    """Creates simple data mocks for AuditedAdminTest"""
+    """Creates simple data mocks for AuditedAdminTest.
+    Can likely be more generalized, but the primary purpose of this class is to simplify
+    mock data creation, especially for lists of items,
+    by making the assumption that for most use cases we don't have to worry about
+    data 'accuracy' ('testy 2' is not an accurate first_name for example), we just care about
+    implementing some kind of patterning, especially with lists of items.
+
+    Two variables are used across multiple functions:
+
+    *item_name* - Used in patterning. Will be appended en masse to multiple string fields,
+    like first_name. For example, item_name 'egg' will return a user object of:
+
+    first_name: 'egg first_name:user',
+    last_name: 'egg last_name:user',
+    username: 'egg username:user'
+
+    where 'user' is the short_hand
+
+    *short_hand* - Used in patterning. Certain fields will have ':{shorthand}' appended to it,
+    as a way to optionally include metadata in the string itself. Can be further expanded on.
+    Came from a bug where different querysets used in testing would effectively be 'anonymized', wherein
+    it would only display a list of types, but not include the variable name.
+    """ # noqa
 
     # 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):
+    def dummy_user(self, item_name, short_hand):
         """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),
+            first_name="{} first_name:{}".format(item_name, short_hand),
+            last_name="{} last_name:{}".format(item_name, short_hand),
+            username="{} username:{}".format(item_name, short_hand),
         )[0]
         return user
 
-    def dummy_contact(self, item_name, shorthand):
+    def dummy_contact(self, item_name, short_hand):
         """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),
+            first_name="{} first_name:{}".format(item_name, short_hand),
+            last_name="{} last_name:{}".format(item_name, short_hand),
+            title="{} title:{}".format(item_name, short_hand),
             email="{}testy@town.com".format(item_name),
             phone="(555) 555 5555",
         )[0]
@@ -164,9 +173,9 @@ class AuditedAdminMockData:
             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),
+            organization_name="{} organization".format(item_name),
+            address_line1="{} address_line1".format(item_name),
+            address_line2="{} address_line2".format(item_name),
             is_policy_acknowledged=True,
             state_territory="NY",
             zipcode="10002",
@@ -178,7 +187,6 @@ class AuditedAdminMockData:
         )
         return common_args
 
-    # This can be boiled down more, though for our purposes this is OK
     def dummy_kwarg_boilerplate(
         self,
         domain_type,
@@ -188,7 +196,18 @@ class AuditedAdminMockData:
         federal_type="executive",
         purpose="Purpose of the site",
     ):
-        """Returns kwargs for different domain object types"""
+        """
+        A helper function that returns premade kwargs for easily creating different domain object types.
+        There is a decent amount of boilerplate associated with
+        creating new domain objects (such as domain_application, or domain_information),
+        so for test case purposes, we can make some assumptions and utilize that to simplify
+        the object creation process.
+
+        *domain_type* uses constants. Used to identify what kind of 'Domain' object you'd like to make.
+
+        In more detail: domain_type specifies what kind of domain object you'd like to create, i.e.
+        domain_application (APPLICATION), or domain_information (INFORMATION).
+        """
         common_args = self.get_common_domain_arg_dictionary(
             item_name, org_type, federal_type, purpose
         )

src/registrar/tests/test_admin.py

@@ -1,16 +1,10 @@
 from django.test import TestCase, RequestFactory, Client
 from django.contrib.admin.sites import AdminSite
-from registrar.admin import DomainApplicationAdmin, ListHeaderAdmin
+# noqa is used on all three of these as the linter doesn't like the length of this line
+from registrar.admin import DomainApplicationAdmin, ListHeaderAdmin, MyUserAdmin, AuditedAdmin # noqa
+from registrar.models import DomainApplication, DomainInformation, User, Contact, DomainInvitation # noqa
+from .common import completed_application, mock_user, create_superuser, create_user, multiple_unalphabetical_domain_objects # noqa
 
-# 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 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
@@ -393,7 +387,6 @@ class AuditedAdminTest(TestCase):
         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)
@@ -402,8 +395,6 @@ class AuditedAdminTest(TestCase):
 
         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 = [
             DomainApplication.authorizing_official.field,