From ef0c9e700fce60b5646dad2082073caba142953c Mon Sep 17 00:00:00 2001 From: Slim Date: Mon, 28 Jul 2025 11:04:13 -0700 Subject: [PATCH] #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 | 54 +++++++++---- src/registrar/forms/domain_request_wizard.py | 5 ++ src/registrar/models/domain_information.py | 81 +++++++++++++------- src/registrar/models/domain_request.py | 4 +- src/registrar/tests/test_admin_domain.py | 36 ++++++++- src/registrar/tests/test_admin_request.py | 2 +- src/registrar/tests/test_models.py | 16 ++-- 7 files changed, 143 insertions(+), 55 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6664b0bfb..159557f77 100644 --- a/src/registrar/admin.py +++ b/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(portfolio__isnull=True, federal_agency__federal_type=self.value()) + Q(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, - then="domain_info__federal_agency__federal_type", + domain_info__federal_type__isnull=True, + 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__portfolio__isnull=True, domain_info__federal_agency__federal_type=self.value()) + Q(domain_info__federal_type__iexact=val) + | Q(domain_info__federal_agency__federal_type__iexact=val) + | Q(domain_info__portfolio__federal_agency__federal_type__iexact=val) ) return queryset diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index fd90e3108..b659bf030 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/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( diff --git a/src/registrar/models/domain_information.py b/src/registrar/models/domain_information.py index 410e42f28..c7bef3aea 100644 --- a/src/registrar/models/domain_information.py +++ b/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 - da_dict = {} - da_many_to_many_dict = {} - 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) + # Extract dictionaries for normal and many-to-many fields + dr_dict, dr_many_to_many_dict = cls._get_dr_and_many_to_many_dicts( + domain_request, common_fields, info_many_to_many_fields + ) # 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""" diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 6a981321a..cebe96a51 100644 --- a/src/registrar/models/domain_request.py +++ b/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. diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index d38d962ba..3b46d6034 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/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, 'Federal', 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. diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index ebefde866..d76af9c27 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/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, 'Federal', count=1) # Now let's make sure the long description does not exist diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8d066c7b0..bf45db4f0 100644 --- a/src/registrar/tests/test_models.py +++ b/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