From aeb496ceaec0a4ec27ebca04559fd255f82f1abd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 28 Oct 2024 14:34:44 -0600 Subject: [PATCH 1/3] initial changes --- src/registrar/admin.py | 34 +++++++++++++++++++----- src/registrar/assets/js/get-gov-admin.js | 3 +++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0b96b4c48..d3530101d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -5,6 +5,7 @@ from django import forms from django.db.models import Value, CharField, Q from django.db.models.functions import Concat, Coalesce from django.http import HttpResponseRedirect +from registrar.models.federal_agency import FederalAgency from registrar.utility.admin_helpers import ( get_action_needed_reason_default_email, get_rejection_reason_default_email, @@ -3192,6 +3193,9 @@ class PortfolioAdmin(ListHeaderAdmin): return self.add_fieldsets return super().get_fieldsets(request, obj) + # TODO - I think the solution to this may just be to set this as editable. + # Then in the underlying page override, we need a fake readonly display that + # is toggled on or off. def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. We have 2 conditions that determine which fields are read-only: @@ -3206,6 +3210,14 @@ class PortfolioAdmin(ListHeaderAdmin): # straightforward and the readonly_fields list can control their behavior readonly_fields.extend([field.name for field in self.model._meta.fields]) + # Make senior_official readonly for federal organizations + if obj and obj.organization_type == obj.OrganizationChoices.FEDERAL: + if "senior_official" not in readonly_fields: + readonly_fields.append("senior_official") + elif "senior_official" in readonly_fields: + # Remove senior_official from readonly_fields if org is non-federal + readonly_fields.remove("senior_official") + if request.user.has_perm("registrar.full_access_permission"): return readonly_fields @@ -3228,7 +3240,7 @@ class PortfolioAdmin(ListHeaderAdmin): extra_context["domain_requests"] = obj.get_domain_requests(order_by=["requested_domain__name"]) return super().change_view(request, object_id, form_url, extra_context) - def save_model(self, request, obj, form, change): + def save_model(self, request, obj: Portfolio, form, change): if hasattr(obj, "creator") is False: # ---- update creator ---- @@ -3243,12 +3255,20 @@ class PortfolioAdmin(ListHeaderAdmin): if is_federal and obj.organization_name is None: obj.organization_name = obj.federal_agency.agency - # Remove this line when senior_official is no longer readonly in /admin. - if obj.federal_agency: - if obj.federal_agency.so_federal_agency.exists(): - obj.senior_official = obj.federal_agency.so_federal_agency.first() - else: - obj.senior_official = None + # TODO - this should be handled almost entirely in javascript + # Handle the federal agency and senior official fields + if obj.organization_type == obj.OrganizationChoices.FEDERAL: + if obj.federal_agency: + if obj.federal_agency.so_federal_agency.exists(): + obj.senior_official = obj.federal_agency.so_federal_agency.first() + else: + obj.senior_official = None + else: + if obj.federal_agency and obj.federal_agency.agency != "Non-Federal Agency": + if obj.federal_agency.so_federal_agency.first() == obj.senior_official: + obj.senior_official = None + obj.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() + super().save_model(request, obj, form, change) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index fd50fbb0c..a3c8b1410 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -927,6 +927,7 @@ document.addEventListener('DOMContentLoaded', function() { // This is the additional information that exists beneath the SO element. var contactList = document.querySelector(".field-senior_official .dja-address-contact-list"); + const federalAgencyContainer = document.querySelector(".field-federal_agency"); document.addEventListener('DOMContentLoaded', function() { let isPortfolioPage = document.getElementById("portfolio_form"); @@ -975,11 +976,13 @@ document.addEventListener('DOMContentLoaded', function() { let selectedValue = organizationType.value; if (selectedValue === "federal") { hideElement(organizationNameContainer); + showElement(federalAgencyContainer); if (federalType) { showElement(federalType); } } else { showElement(organizationNameContainer); + hideElement(federalAgencyContainer); if (federalType) { hideElement(federalType); } From c2a07a8983682f7b80285112522b56799f135ccc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 29 Oct 2024 09:08:00 -0600 Subject: [PATCH 2/3] unit tests --- src/registrar/admin.py | 9 ++--- src/registrar/tests/test_admin.py | 60 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d3530101d..2151d6f3c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3193,9 +3193,6 @@ class PortfolioAdmin(ListHeaderAdmin): return self.add_fieldsets return super().get_fieldsets(request, obj) - # TODO - I think the solution to this may just be to set this as editable. - # Then in the underlying page override, we need a fake readonly display that - # is toggled on or off. def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. We have 2 conditions that determine which fields are read-only: @@ -3241,7 +3238,6 @@ class PortfolioAdmin(ListHeaderAdmin): return super().change_view(request, object_id, form_url, extra_context) def save_model(self, request, obj: Portfolio, form, change): - if hasattr(obj, "creator") is False: # ---- update creator ---- # Set the creator field to the current admin user @@ -3255,8 +3251,8 @@ class PortfolioAdmin(ListHeaderAdmin): if is_federal and obj.organization_name is None: obj.organization_name = obj.federal_agency.agency - # TODO - this should be handled almost entirely in javascript - # Handle the federal agency and senior official fields + # Set the senior official field to the senior official on the federal agency + # when federal - otherwise, clear the field. if obj.organization_type == obj.OrganizationChoices.FEDERAL: if obj.federal_agency: if obj.federal_agency.so_federal_agency.exists(): @@ -3269,7 +3265,6 @@ class PortfolioAdmin(ListHeaderAdmin): obj.senior_official = None obj.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() - super().save_model(request, obj, form, change) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9c5e3b582..2677462df 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2103,6 +2103,66 @@ class TestPortfolioAdmin(TestCase): display_members = self.admin.display_members(self.portfolio) self.assertIn(f'2 members', display_members) + @less_console_noise_decorator + def test_senior_official_readonly_for_federal_org(self): + """Test that senior_official field is readonly for federal organizations""" + request = self.factory.get("/") + request.user = self.superuser + + # Create a federal portfolio + portfolio = Portfolio.objects.create( + organization_name="Test Federal Org", + organization_type=DomainRequest.OrganizationChoices.FEDERAL, + creator=self.superuser, + ) + + readonly_fields = self.admin.get_readonly_fields(request, portfolio) + self.assertIn("senior_official", readonly_fields) + + # Change to non-federal org + portfolio.organization_type = DomainRequest.OrganizationChoices.CITY + readonly_fields = self.admin.get_readonly_fields(request, portfolio) + self.assertNotIn("senior_official", readonly_fields) + + @less_console_noise_decorator + def test_senior_official_auto_assignment(self): + """Test automatic senior official assignment based on organization type and federal agency""" + request = self.factory.get("/") + request.user = self.superuser + + # Create a federal agency with a senior official + federal_agency = FederalAgency.objects.create(agency="Test Agency") + senior_official = SeniorOfficial.objects.create( + first_name="Test", + last_name="Official", + title="Some guy", + email="test@example.gov", + federal_agency=federal_agency, + ) + + # Create a federal portfolio + portfolio = Portfolio.objects.create( + organization_name="Test Federal Org", + organization_type=DomainRequest.OrganizationChoices.FEDERAL, + creator=self.superuser, + ) + + # Test that the federal org gets senior official from agency when federal + portfolio.federal_agency = federal_agency + self.admin.save_model(request, portfolio, form=None, change=False) + self.assertEqual(portfolio.senior_official, senior_official) + + # Test non-federal org clears senior official when not city + portfolio.organization_type = DomainRequest.OrganizationChoices.CITY + self.admin.save_model(request, portfolio, form=None, change=True) + self.assertIsNone(portfolio.senior_official) + self.assertEqual(portfolio.federal_agency.agency, "Non-Federal Agency") + + # Cleanup + senior_official.delete() + federal_agency.delete() + portfolio.delete() + class TestTransferUser(WebTest): """User transfer custom admin page""" From 641692d9633d2cdced344596c8deb37cdb6fe9b1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:05:56 -0600 Subject: [PATCH 3/3] lint --- src/registrar/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 2151d6f3c..77ecb079a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3241,7 +3241,7 @@ class PortfolioAdmin(ListHeaderAdmin): if hasattr(obj, "creator") is False: # ---- update creator ---- # Set the creator field to the current admin user - obj.creator = request.user if request.user.is_authenticated else None + obj.creator = request.user if request.user.is_authenticated else None # type: ignore # ---- update organization name ---- # org name will be the same as federal agency, if it is federal, # otherwise it will be the actual org name. If nothing is entered for @@ -3263,7 +3263,7 @@ class PortfolioAdmin(ListHeaderAdmin): if obj.federal_agency and obj.federal_agency.agency != "Non-Federal Agency": if obj.federal_agency.so_federal_agency.first() == obj.senior_official: obj.senior_official = None - obj.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() + obj.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() # type: ignore super().save_model(request, obj, form, change)