#3892: FederalTypeFilter to display for Domain on /admin - [HOTGOV] (#3995)

* Biz logic fix for both DomainRequestAdmin and DomainAdmin

* Remove DRA code and clean up and comment code query logic

* Refactor createfromda and add unit tests

* Remove extra code

* Remove extraneous comment I meant to comment in the PR itself

* Add in fix for Domain Request admin

* Fix unit test for Domain Request Admin

* Update comments for the lookups and querysets

* Update create from da to dr because domain request

* Some more da to dr clean up

src/registrar/admin.py

@@ -2508,19 +2508,22 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
         parameter_name = "converted_federal_types"
 
         def lookups(self, request, model_admin):
-            # Annotate the queryset for efficient filtering
+            """
+            1. Search for existing federal type
+            2. Then search for federal type from associated portfolio
+            3. Then search for federal type from associated federal agency
+            """
             queryset = (
                 DomainRequest.objects.annotate(
                     converted_federal_type=Case(
+                        When(federal_type__isnull=False, then=F("federal_type")),
                         When(
-                            portfolio__isnull=False,
                             portfolio__federal_agency__federal_type__isnull=False,
-                            then="portfolio__federal_agency__federal_type",
+                            then=F("portfolio__federal_agency__federal_type"),
                         ),
                         When(
-                            portfolio__isnull=True,
                             federal_agency__federal_type__isnull=False,
-                            then="federal_agency__federal_type",
+                            then=F("federal_agency__federal_type"),
                         ),
                         default=Value(""),
                         output_field=CharField(),
@@ -2530,7 +2533,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
                 .distinct()
             )
 
-            # Filter out empty values and return sorted unique entries
             return sorted(
                 [
                     (federal_type, BranchChoices.get_branch_label(federal_type))
@@ -2542,8 +2544,9 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
         def queryset(self, request, queryset):
             if self.value():
                 return queryset.filter(
-                    Q(portfolio__federal_agency__federal_type=self.value())
+                    Q(federal_type=self.value())
-                    | Q(portfolio__isnull=True, federal_agency__federal_type=self.value())
+                    | Q(portfolio__federal_agency__federal_type=self.value())
+                    | Q(federal_agency__federal_type=self.value())
                 )
             return queryset
 
@@ -3931,10 +3934,23 @@ class DomainAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
         parameter_name = "converted_federal_types"
 
         def lookups(self, request, model_admin):
-            # Annotate the queryset for efficient filtering
+            """
+            1. Search for existing federal type
+            2. Then search for federal type from associated portfolio
+            3. Then search for federal type from associated federal agency, where:
+                A. Make sure there's domain_info, if none do nothing
+                B. Check for if no portfolio, if there is then use portfolio
+                C. Check if federal_type is missing - if has don't replace
+                D. Make sure agency has a federal_type as fallback
+                E. Otherwise assign federal_type from agency
+            """
             queryset = (
                 Domain.objects.annotate(
                     converted_federal_type=Case(
+                        When(
+                            domain_info__federal_type__isnull=False,
+                            then=F("domain_info__federal_type"),
+                        ),
                         When(
                             domain_info__isnull=False,
                             domain_info__portfolio__isnull=False,
@@ -3943,8 +3959,9 @@ class DomainAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
                         When(
                             domain_info__isnull=False,
                             domain_info__portfolio__isnull=True,
-                            domain_info__federal_type__isnull=False,
+                            domain_info__federal_type__isnull=True,
-                            then="domain_info__federal_agency__federal_type",
+                            domain_info__federal_agency__federal_type__isnull=False,
+                            then=F("domain_info__federal_agency__federal_type"),
                         ),
                         default=Value(""),
                         output_field=CharField(),
@@ -3954,7 +3971,6 @@ class DomainAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
                 .distinct()
             )
 
-            # Filter out empty values and return sorted unique entries
             return sorted(
                 [
                     (federal_type, BranchChoices.get_branch_label(federal_type))
@@ -3964,10 +3980,18 @@ class DomainAdmin(ListHeaderAdmin, ImportExportRegistrarModelAdmin):
             )
 
         def queryset(self, request, queryset):
-            if self.value():
+            """
+            1. Does domain's direct federal_type match what was selected
+            2. If not, check domains federal agency if it has a federal_type that matches
+            3. If not, check domain’s portfolio's (if present) link to agency that has
+            federal_type
+            """
+            val = self.value()
+            if val:
                 return queryset.filter(
-                    Q(domain_info__portfolio__federal_type=self.value())
+                    Q(domain_info__federal_type__iexact=val)
-                    | Q(domain_info__portfolio__isnull=True, domain_info__federal_agency__federal_type=self.value())
+                    | Q(domain_info__federal_agency__federal_type__iexact=val)
+                    | Q(domain_info__portfolio__federal_agency__federal_type__iexact=val)
                 )
             return queryset
 

src/registrar/forms/domain_request_wizard.py

@@ -287,6 +287,11 @@ class OrganizationFederalForm(RegistrarForm):
         error_messages={"required": ("Select the part of the federal government your organization is in.")},
     )
 
+    def to_database(self, domain_request):
+        federal_type = self.cleaned_data.get("federal_type")
+        domain_request.federal_type = federal_type
+        domain_request.save(update_fields=["federal_type"])
+
 
 class OrganizationElectionForm(RegistrarForm):
     is_election_board = forms.NullBooleanField(

src/registrar/models/domain_information.py

@@ -348,25 +348,21 @@ class DomainInformation(TimeStampedModel):
         super().save(*args, **kwargs)
 
     @classmethod
-    def create_from_da(cls, domain_request: DomainRequest, domain=None):
+    def create_from_dr(cls, domain_request: DomainRequest, domain=None):
         """Takes in a DomainRequest and converts it into DomainInformation"""
 
         # Throw an error if we get None - we can't create something from nothing
         if domain_request is None:
             raise ValueError("The provided DomainRequest is None")
 
-        # Throw an error if the da doesn't have an id
+        # Throw an error if the dr doesn't have an id
         if not hasattr(domain_request, "id"):
             raise ValueError("The provided DomainRequest has no id")
 
         # check if we have a record that corresponds with the domain
         # domain_request, if so short circuit the create
-        existing_domain_info = cls.objects.filter(domain_request__id=domain_request.id).first()
+        existing_domain_info = cls._short_circuit_if_exists(domain_request)
         if existing_domain_info:
-            logger.info(
-                f"create_from_da() -> Shortcircuting create on {existing_domain_info}. "
-                "This record already exists. No values updated!"
-            )
             return existing_domain_info
 
         # Get the fields that exist on both DomainRequest and DomainInformation
@@ -375,28 +371,17 @@ class DomainInformation(TimeStampedModel):
         # Get a list of all many_to_many relations on DomainInformation (needs to be saved differently)
         info_many_to_many_fields = DomainInformation._get_many_to_many_fields()
 
-        # Create a dictionary with only the common fields, and create a DomainInformation from it
+        # Extract dictionaries for normal and many-to-many fields
-        da_dict = {}
+        dr_dict, dr_many_to_many_dict = cls._get_dr_and_many_to_many_dicts(
-        da_many_to_many_dict = {}
+            domain_request, common_fields, info_many_to_many_fields
-        for field in common_fields:
+        )
-            # If the field isn't many_to_many, populate the da_dict.
-            # If it is, populate da_many_to_many_dict as we need to save this later.
-            if hasattr(domain_request, field):
-                if field not in info_many_to_many_fields:
-                    da_dict[field] = getattr(domain_request, field)
-                else:
-                    da_many_to_many_dict[field] = getattr(domain_request, field).all()
-
-        # This will not happen in normal code flow, but having some redundancy doesn't hurt.
-        # da_dict should not have "id" under any circumstances.
-        # If it does have it, then this indicates that common_fields is overzealous in the data
-        # that it is returning. Try looking in DomainHelper.get_common_fields.
-        if "id" in da_dict:
-            logger.warning("create_from_da() -> Found attribute 'id' when trying to create")
-            da_dict.pop("id", None)
 
         # Create a placeholder DomainInformation object
-        domain_info = DomainInformation(**da_dict)
+        domain_info = DomainInformation(**dr_dict)
+
+        # Explicitly copy over extra fields (currently only federal agency)
+        # that aren't covered in the common fields
+        cls._copy_federal_agency_explicit_fields(domain_request, domain_info)
 
         # Add the domain_request and domain fields
         domain_info.domain_request = domain_request
@@ -408,11 +393,51 @@ class DomainInformation(TimeStampedModel):
         # This bundles them all together, and then saves it in a single call.
         with transaction.atomic():
             domain_info.save()
-            for field, value in da_many_to_many_dict.items():
+            for field, value in dr_many_to_many_dict.items():
                 getattr(domain_info, field).set(value)
 
         return domain_info
 
+    @classmethod
+    def _short_circuit_if_exists(cls, domain_request):
+        existing_domain_info = cls.objects.filter(domain_request__id=domain_request.id).first()
+        if existing_domain_info:
+            logger.info(
+                f"create_from_dr() -> Shortcircuting create on {existing_domain_info}. "
+                "This record already exists. No values updated!"
+            )
+        return existing_domain_info
+
+    @classmethod
+    def _get_dr_and_many_to_many_dicts(cls, domain_request, common_fields, info_many_to_many_fields):
+        # Create a dictionary with only the common fields, and create a DomainInformation from it
+        dr_dict = {}
+        dr_many_to_many_dict = {}
+        for field in common_fields:
+            # If the field isn't many_to_many, populate the dr_dict.
+            # If it is, populate dr_many_to_many_dict as we need to save this later.
+            if hasattr(domain_request, field):
+                if field not in info_many_to_many_fields:
+                    dr_dict[field] = getattr(domain_request, field)
+                else:
+                    dr_many_to_many_dict[field] = getattr(domain_request, field).all()
+
+        # This will not happen in normal code flow, but having some redundancy doesn't hurt.
+        # dr_dict should not have "id" under any circumstances.
+        # If it does have it, then this indicates that common_fields is overzealous in the data
+        # that it is returning. Try looking in DomainHelper.get_common_fields.
+        if "id" in dr_dict:
+            logger.warning("create_from_dr() -> Found attribute 'id' when trying to create")
+            dr_dict.pop("id", None)
+
+        return dr_dict, dr_many_to_many_dict
+
+    @classmethod
+    def _copy_federal_agency_explicit_fields(cls, domain_request, domain_info):
+        """Explicitly copy federal_agency from DomainRequest (if present)"""
+        if hasattr(domain_request, "federal_agency") and domain_request.federal_agency is not None:
+            domain_info.federal_agency = domain_request.federal_agency
+
     @staticmethod
     def _get_many_to_many_fields():
         """Returns a set of each field.name that has the many to many relation"""

src/registrar/models/domain_request.py

@@ -1223,7 +1223,7 @@ class DomainRequest(TimeStampedModel):
 
         # copy the information from DomainRequest into domaininformation
         DomainInformation = apps.get_model("registrar.DomainInformation")
-        DomainInformation.create_from_da(domain_request=self, domain=created_domain)
+        DomainInformation.create_from_dr(domain_request=self, domain=created_domain)
 
         # create the permission for the user
         UserDomainRole = apps.get_model("registrar.UserDomainRole")
@@ -1340,7 +1340,7 @@ class DomainRequest(TimeStampedModel):
     def is_requesting_new_suborganization(self) -> bool:
         """Determines if a user is trying to request
         a new suborganization using the domain request form, rather than one that already exists.
-        Used for the RequestingEntity page and on DomainInformation.create_from_da().
+        Used for the RequestingEntity page and on DomainInformation.create_from_dr().
 
         Returns True if a sub_organization does not exist and if requested_suborganization,
         suborganization_city, and suborganization_state_territory all exist.

src/registrar/tests/test_admin_domain.py

@@ -516,6 +516,23 @@ class TestDomainAdminAsStaff(MockEppLib):
             )
         self.assertEqual(domain.state, Domain.State.DELETED)
 
+    def test_has_correct_filters_staff_view(self):
+        """Ensure DomainAdmin has the correct filters configured."""
+        with less_console_noise():
+            request = self.factory.get("/")
+            request.user = self.superuser
+
+            filters = self.admin.get_list_filter(request)
+
+            expected_filters = [
+                DomainAdmin.GenericOrgFilter,
+                DomainAdmin.FederalTypeFilter,
+                DomainAdmin.ElectionOfficeFilter,
+                "state",
+            ]
+
+            self.assertEqual(filters, expected_filters)
+
 
 class TestDomainInformationInline(MockEppLib):
     """Test DomainAdmin class, specifically the DomainInformationInline class, as staff user.
@@ -1027,7 +1044,7 @@ class TestDomainAdminWithClient(TestCase):
         response = self.client.get("/admin/registrar/domain/")
         # There are 4 template references to Federal (4) plus four references in the table
         # for our actual domain_request
-        self.assertContains(response, "Federal", count=57)
+        self.assertContains(response, "Federal", count=58)
         # This may be a bit more robust
         self.assertContains(response, '<td class="field-converted_generic_org_type">Federal</td>', count=1)
         # Now let's make sure the long description does not exist
@@ -1041,6 +1058,23 @@ class TestDomainAdminWithClient(TestCase):
         self.assertContains(response, ">Export<")
         self.assertNotContains(response, ">Import<")
 
+    def test_has_correct_filters_client_view(self):
+        """Ensure DomainAdmin has the correct filters configured"""
+        with less_console_noise():
+            request = self.factory.get("/")
+            request.user = self.superuser
+
+            filters = self.admin.get_list_filter(request)
+
+            expected_filters = [
+                DomainAdmin.GenericOrgFilter,
+                DomainAdmin.FederalTypeFilter,
+                DomainAdmin.ElectionOfficeFilter,
+                "state",
+            ]
+
+            self.assertEqual(filters, expected_filters)
+
 
 class TestDomainAdminWebTest(MockEppLib, WebTest):
     """Test DomainAdmin class as super user, using WebTest.

src/registrar/tests/test_admin_request.py

@@ -739,7 +739,7 @@ class TestDomainRequestAdmin(MockEppLib):
         response = self.client.get("/admin/registrar/domainrequest/?generic_org_type__exact=federal")
         # There are 2 template references to Federal (4) and two in the results data
         # of the request
-        self.assertContains(response, "Federal", count=55)
+        self.assertContains(response, "Federal", count=56)
         # This may be a bit more robust
         self.assertContains(response, '<td class="field-converted_generic_org_type">Federal</td>', count=1)
         # Now let's make sure the long description does not exist

src/registrar/tests/test_models.py

@@ -1811,7 +1811,7 @@ class TestDomainInformationCustomSave(TestCase):
             is_election_board=True,
         )
 
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
         self.assertEqual(domain_information.organization_type, DomainRequest.OrgChoicesElectionOffice.CITY_ELECTION)
 
     @less_console_noise_decorator
@@ -1824,7 +1824,7 @@ class TestDomainInformationCustomSave(TestCase):
             is_election_board=True,
         )
 
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
         self.assertEqual(domain_information.organization_type, DomainRequest.OrgChoicesElectionOffice.FEDERAL)
         self.assertEqual(domain_information.is_election_board, None)
 
@@ -1837,7 +1837,7 @@ class TestDomainInformationCustomSave(TestCase):
             generic_org_type=DomainRequest.OrganizationChoices.CITY,
             is_election_board=False,
         )
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
         domain_information.is_election_board = True
         domain_information.save()
 
@@ -1863,7 +1863,7 @@ class TestDomainInformationCustomSave(TestCase):
             generic_org_type=DomainRequest.OrganizationChoices.CITY,
             is_election_board=None,
         )
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
         domain_information.is_election_board = True
         domain_information.save()
 
@@ -1887,7 +1887,7 @@ class TestDomainInformationCustomSave(TestCase):
             generic_org_type=DomainRequest.OrganizationChoices.CITY,
             is_election_board=True,
         )
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
 
         domain_information.generic_org_type = DomainRequest.OrganizationChoices.INTERSTATE
         domain_information.save()
@@ -1903,7 +1903,7 @@ class TestDomainInformationCustomSave(TestCase):
             generic_org_type=DomainRequest.OrganizationChoices.TRIBAL,
             is_election_board=True,
         )
-        domain_information_tribal = DomainInformation.create_from_da(domain_request_tribal)
+        domain_information_tribal = DomainInformation.create_from_dr(domain_request_tribal)
         self.assertEqual(
             domain_information_tribal.organization_type, DomainRequest.OrgChoicesElectionOffice.TRIBAL_ELECTION
         )
@@ -1930,7 +1930,7 @@ class TestDomainInformationCustomSave(TestCase):
             generic_org_type=DomainRequest.OrganizationChoices.CITY,
             is_election_board=False,
         )
-        domain_information = DomainInformation.create_from_da(domain_request)
+        domain_information = DomainInformation.create_from_dr(domain_request)
         domain_information.save()
         self.assertEqual(domain_information.organization_type, DomainRequest.OrgChoicesElectionOffice.CITY)
         self.assertEqual(domain_information.is_election_board, False)
@@ -1945,7 +1945,7 @@ class TestDomainInformationCustomSave(TestCase):
             is_election_board=True,
             organization_type=DomainRequest.OrgChoicesElectionOffice.CITY_ELECTION,
         )
-        domain_information_election = DomainInformation.create_from_da(domain_request_election)
+        domain_information_election = DomainInformation.create_from_dr(domain_request_election)
 
         self.assertEqual(
             domain_information_election.organization_type, DomainRequest.OrgChoicesElectionOffice.CITY_ELECTION