Linter things / Rachid suggestions

src/registrar/admin.py

@@ -13,8 +13,8 @@ audited_admin_item_names = ["submitter", "authorizing_official",
                             "investigator", "creator", "user"]
 audited_admin_orderby_names = ['first_name', 'last_name']
 
-contact_admin_item_names = ["domain", "requested_domain"]
+special_audited_admin_item_names = ["domain", "requested_domain"]
-contact_admin_orderby_names = ["name"]
+special_audited_admin_orderby_names = ["name"]
 # Used to keep track of how we want to order_by certain FKs
 foreignkey_orderby_dict: list[SortingDictInterface] = [
     # foreign_key - order_by
@@ -22,9 +22,10 @@ foreignkey_orderby_dict: list[SortingDictInterface] = [
                         audited_admin_item_names,
                         audited_admin_orderby_names
                         ).sorting_dict,
+    # Handles fields that are sorted by 'name'
     SortingDictInterface(
-                        contact_admin_item_names,
+                        special_audited_admin_item_names,
-                        contact_admin_orderby_names
+                        special_audited_admin_orderby_names
                         ).sorting_dict
 ]
 
@@ -186,15 +187,9 @@ class DomainAdmin(ListHeaderAdmin):
 
 class ContactAdmin(ListHeaderAdmin):
     """Custom contact admin class to add search."""
-
     search_fields = ["email", "first_name", "last_name"]
     search_help_text = "Search by firstname, lastname or email."
 
-    def formfield_for_foreignkey(self, db_field, request, **kwargs):
-        """Used to sort dropdown fields alphabetically but can be expanded upon"""
-        form_field = super().formfield_for_foreignkey(db_field, request, **kwargs)
-        return form_field_order_helper(form_field, db_field)
-
 
 class DomainApplicationAdmin(ListHeaderAdmin):
 
@@ -333,7 +328,8 @@ class DomainApplicationAdmin(ListHeaderAdmin):
 
 # For readability purposes, but can be replaced with a one liner
 def form_field_order_helper(form_field, db_field):
-    """A shorthand for AdminFormOrderHelper(foreignkey_orderby_dict).get_ordered_form_field(form_field, db_field)""" # noqa
+    """A shorthand for AdminFormOrderHelper(foreignkey_orderby_dict)
+    .get_ordered_form_field(form_field, db_field)"""
 
     form = AdminFormOrderHelper(foreignkey_orderby_dict)
     return form.get_ordered_form_field(form_field, db_field)

src/registrar/models/utility/admin_form_order_helper.py

@@ -18,16 +18,18 @@ class SortingDictInterface:
 
 
 class AdminFormOrderHelper():
-    """A helper class to order a dropdown field in Django Admin, takes the fields you want to order by as an array""" # noqa
+    """A helper class to order a dropdown field in Django Admin,
+    takes the fields you want to order by as an array"""
 
     # Used to keep track of how we want to order_by certain FKs
-    _sorting_dict: list[SortingDictInterface] = [] # noqa
+    _sorting_dict: list[SortingDictInterface] = []
 
     def __init__(self, sort):
         self._sorting_dict = sort
 
     def get_ordered_form_field(self, form_field, db_field) -> (ModelChoiceField | None):
-        """Orders the queryset for a ModelChoiceField based on the order_by_dict dictionary""" # noqa
+        """Orders the queryset for a ModelChoiceField
+        based on the order_by_dict dictionary"""
         _order_by_list = []
 
         for item in self._sorting_dict:
@@ -41,7 +43,7 @@ class AdminFormOrderHelper():
                 break
 
         # Only order if we choose to do so
-        if not _order_by_list is None:
+        if _order_by_list is not None:
             form_field.queryset = form_field.queryset.order_by(*_order_by_list)
 
         return form_field

src/registrar/tests/common.py

@@ -10,14 +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, DomainInvitation, User
+from registrar.models import Contact, DraftDomain, Website, DomainApplication
-import logging
+# For the linter
-from registrar.models.domain import Domain
+from registrar.models import DomainInvitation, User, DomainInformation, Domain
-
-from registrar.models.domain_information import DomainInformation
 
 logger = logging.getLogger(__name__)
 
+
 def get_handlers():
     """Obtain pointers to all StreamHandlers."""
     handlers = {}
@@ -103,14 +102,16 @@ class AuditedAdminMockData:
     APPLICATION = "application"
     INVITATION = "invitation"
 
-    # These all can likely be generalized more if necessary, particulary with shorthands.
+    # 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
+    # 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"""
@@ -132,23 +133,31 @@ class AuditedAdminMockData:
         )[0]
         return contact
 
-    def dummy_draft_domain(self,item_name):
+    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):
+    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]
+        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"):
+    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,
@@ -160,38 +169,52 @@ class AuditedAdminMockData:
             is_policy_acknowledged=True,
             state_territory="NY",
             zipcode="10002",
-            type_of_work = 'e-Government',
+            type_of_work='e-Government',
-            anything_else = "There is more",
+            anything_else="There is more",
-            authorizing_official = self.dummy_contact(item_name, 'authorizing_official'),
+            authorizing_official=self.dummy_contact(item_name, 'authorizing_official'),
-            submitter = self.dummy_contact(item_name, 'submitter'),
+            submitter=self.dummy_contact(item_name, 'submitter'),
-            creator = self.dummy_user(item_name, 'creator'),
+            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"):
+    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)
+        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),
+                    requested_domain=self.dummy_draft_domain(item_name),
-                    investigator = self.dummy_user(item_name, 'investigator'),
+                    investigator=self.dummy_user(item_name, 'investigator'),
-                    status = status,
+                    status=status,
                 )
             case self.INFORMATION:
+                domain_app = self.create_full_dummy_domain_application(item_name)
                 full_arg_list = dict(
                     **common_args,
-                    domain = self.dummy_domain(item_name),
+                    domain=self.dummy_domain(item_name),
-                    domain_application = self.create_full_dummy_domain_application(item_name)
+                    domain_application=domain_app
                 )
             case self.INVITATION:
                 full_arg_list = dict(
-                    email = "test_mail@mail.com",
+                    email="test_mail@mail.com",
-                    domain = self.dummy_domain(item_name),
+                    domain=self.dummy_domain(item_name),
-                    status = DomainInvitation.INVITED
+                    status=DomainInvitation.INVITED
                 )
         return full_arg_list
 
@@ -201,8 +224,14 @@ class AuditedAdminMockData:
         status=DomainApplication.STARTED
     ):
         """Creates a dummy domain application object"""
-        domain_application_kwargs = self.dummy_kwarg_boilerplate(self.APPLICATION, object_name, status)
+        domain_application_kwargs = self.dummy_kwarg_boilerplate(
-        application = DomainApplication.objects.get_or_create(**domain_application_kwargs)[0]
+                                        self.APPLICATION,
+                                        object_name,
+                                        status
+                                    )
+        application = DomainApplication.objects.get_or_create(
+                        **domain_application_kwargs
+                    )[0]
         return application
 
     def create_full_dummy_domain_information(
@@ -211,8 +240,14 @@ class AuditedAdminMockData:
         status=DomainApplication.STARTED
     ):
         """Creates a dummy domain information object"""
-        domain_application_kwargs = self.dummy_kwarg_boilerplate(self.INFORMATION, object_name, status)
+        domain_application_kwargs = self.dummy_kwarg_boilerplate(
-        application = DomainInformation.objects.get_or_create(**domain_application_kwargs)[0]
+                                        self.INFORMATION,
+                                        object_name,
+                                        status
+                                    )
+        application = DomainInformation.objects.get_or_create(
+                        **domain_application_kwargs
+                    )[0]
         return application
 
     def create_full_dummy_domain_invitation(
@@ -221,8 +256,14 @@ class AuditedAdminMockData:
         status=DomainApplication.STARTED
     ):
         """Creates a dummy domain invitation object"""
-        domain_application_kwargs = self.dummy_kwarg_boilerplate(self.INVITATION, object_name, status)
+        domain_application_kwargs = self.dummy_kwarg_boilerplate(
-        application = DomainInvitation.objects.get_or_create(**domain_application_kwargs)[0]
+                                        self.INVITATION,
+                                        object_name,
+                                        status
+                                    )
+        application = DomainInvitation.objects.get_or_create(
+                        **domain_application_kwargs
+                    )[0]
 
         return application
 
@@ -239,11 +280,17 @@ class AuditedAdminMockData:
         application = None
         match domain_type:
             case self.APPLICATION:
-                application = self.create_full_dummy_domain_application(object_name, status)
+                application = self.create_full_dummy_domain_application(
+                                object_name, status
+                            )
             case self.INVITATION:
-                application = self.create_full_dummy_domain_invitation(object_name, status)
+                application = self.create_full_dummy_domain_invitation(
+                                object_name, status
+                            )
             case self.INFORMATION:
-                application = self.create_full_dummy_domain_information(object_name, status)
+                application = self.create_full_dummy_domain_information(
+                                object_name, status
+                            )
             case _:
                 raise ValueError("Invalid domain_type, must conform to given constants")
 
@@ -259,6 +306,7 @@ class AuditedAdminMockData:
 
         return application
 
+
 def mock_user():
     """A simple user."""
     user_kwargs = dict(
@@ -361,7 +409,9 @@ def completed_application(
     return application
 
 
-def multiple_unalphabetical_domain_objects(domain_type = AuditedAdminMockData.APPLICATION):
+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)
@@ -372,4 +422,3 @@ def multiple_unalphabetical_domain_objects(domain_type = AuditedAdminMockData.AP
         application = mock.create_full_dummy_domain_object(domain_type, object_name)
         applications.append(application)
     return applications
-

src/registrar/tests/test_admin.py

@@ -384,7 +384,13 @@ 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):
+    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))
@@ -408,7 +414,7 @@ class AuditedAdminTest(TestCase):
                         DomainApplication.creator.field,
                         ]
 
-        # Create a sample application - review status does not matter
+        # Creates multiple domain applications - review status does not matter
         applications = multiple_unalphabetical_domain_objects("application")
 
         # Create a mock request
@@ -424,7 +430,13 @@ class AuditedAdminTest(TestCase):
         for field in tested_fields:
 
             # 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, "first_name", "last_name")
+            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, request).queryset
                 )
@@ -443,9 +455,11 @@ class AuditedAdminTest(TestCase):
                 if name_tuple:
                     current_sort_order_coerced_type.append((first, last))
 
-            self.assertEqual(desired_order,
+            self.assertEqual(
+                desired_order,
                 current_sort_order_coerced_type,
-                            "{} is not ordered alphabetically".format(field.name))
+                "{} is not ordered alphabetically".format(field.name)
+            )
 
     def test_alphabetically_sorted_fk_fields_domain_information(self):
         tested_fields = [
@@ -455,7 +469,7 @@ class AuditedAdminTest(TestCase):
                         DomainInformation.creator.field
                         ]
 
-        # Create a sample application - review status does not matter
+        # Creates multiple domain applications - review status does not matter
         applications = multiple_unalphabetical_domain_objects("information")
 
         # Create a mock request
@@ -471,13 +485,18 @@ class AuditedAdminTest(TestCase):
         # For test case purposes, this should be performant.
         for field in tested_fields:
             isNamefield: bool = (field == DomainInformation.domain.field)
-            if(isNamefield):
+            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)
+            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
             )
@@ -497,17 +516,19 @@ class AuditedAdminTest(TestCase):
                     last = None
 
                 name_tuple = self.coerced_fk_field_helper(first, last, field.name, ':')
-                if not name_tuple is None:
+                if name_tuple is not None:
                     current_sort_order_coerced_type.append(name_tuple)
 
-            self.assertEqual(desired_order,
+            self.assertEqual(
+                desired_order,
                 current_sort_order_coerced_type,
-                            "{} is not ordered alphabetically".format(field.name))
+                "{} 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
+        # Creates multiple domain applications - review status does not matter
         applications = multiple_unalphabetical_domain_objects("invitation")
 
         # Create a mock request
@@ -525,7 +546,12 @@ class AuditedAdminTest(TestCase):
             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)
+            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
             )
@@ -541,20 +567,31 @@ class AuditedAdminTest(TestCase):
                 last = None
 
                 name_tuple = self.coerced_fk_field_helper(first, last, field.name, ':')
-                if not name_tuple is None:
+                if name_tuple is not None:
                     current_sort_order_coerced_type.append(name_tuple)
 
-            self.assertEqual(desired_order,
+            self.assertEqual(
+                desired_order,
                 current_sort_order_coerced_type,
-                            "{} is not ordered alphabetically".format(field.name))
+                "{} is not ordered alphabetically".format(field.name)
+            )
+
+    def coerced_fk_field_helper(
+        self,
+        first_name,
+        last_name,
+        field_name,
+        queryset_shorthand
+    ):
+        if first_name is None:
+            raise ValueError('Invalid value for first_name, must be defined')
 
-    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
+        if(first_name.split(queryset_shorthand)[1] == field_name):
             return returned_tuple
         else:
             return None