Merge pull request #2627 from cisagov/dk/2590-portfolio-domains-requests

Issue #2590: - Domain and requests tables filterable by portfolio, from portfolio page

src/registrar/admin.py

@@ -34,6 +34,7 @@ from django_fsm import TransitionNotAllowed  # type: ignore
 from django.utils.safestring import mark_safe
 from django.utils.html import escape
 from django.contrib.auth.forms import UserChangeForm, UsernameField
+from django.contrib.admin.views.main import IGNORED_PARAMS
 from django_admin_multiple_choice_list_filter.list_filters import MultipleChoiceListFilter
 from import_export import resources
 from import_export.admin import ImportExportModelAdmin
@@ -366,7 +367,9 @@ class DomainRequestAdminForm(forms.ModelForm):
 class MultiFieldSortableChangeList(admin.views.main.ChangeList):
     """
     This class overrides the behavior of column sorting in django admin tables in order
-    to allow for multi field sorting on admin_order_field
+    to allow for multi field sorting on admin_order_field.  It also overrides behavior
+    of getting the filter params to allow portfolio filters to be executed without
+    displaying on the right side of the ChangeList view.
 
 
     Usage:
@@ -428,6 +431,24 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList):
 
         return ordering
 
+    def get_filters_params(self, params=None):
+        """
+        Add portfolio to ignored params to allow the portfolio filter while not
+        listing it as a filter option on the right side of Change List on the
+        portfolio list.
+        """
+        params = params or self.params
+        lookup_params = params.copy()  # a dictionary of the query string
+        # Remove all the parameters that are globally and systematically
+        # ignored.
+        # Remove portfolio so that it does not error as an invalid
+        # filter parameter.
+        ignored_params = list(IGNORED_PARAMS) + ["portfolio"]
+        for ignored in ignored_params:
+            if ignored in lookup_params:
+                del lookup_params[ignored]
+        return lookup_params
+
 
 class CustomLogEntryAdmin(LogEntryAdmin):
     """Overwrite the generated LogEntry admin class"""
@@ -644,6 +665,19 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin):
                         )
                     except models.User.DoesNotExist:
                         pass
+                elif parameter_name == "portfolio":
+                    # Retrieves the corresponding portfolio from Portfolio
+                    id_value = request.GET.get(param)
+                    try:
+                        portfolio = models.Portfolio.objects.get(id=id_value)
+                        filters.append(
+                            {
+                                "parameter_name": "portfolio",
+                                "parameter_value": portfolio.organization_name,
+                            }
+                        )
+                    except models.Portfolio.DoesNotExist:
+                        pass
                 else:
                     # For other parameter names, append a dictionary with the original
                     # parameter_name and the corresponding parameter_value
@@ -2236,6 +2270,17 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin):
         use_sort = db_field.name != "senior_official"
         return super().formfield_for_foreignkey(db_field, request, use_admin_sort_fields=use_sort, **kwargs)
 
+    def get_queryset(self, request):
+        """Custom get_queryset to filter by portfolio if portfolio is in the
+        request params."""
+        qs = super().get_queryset(request)
+        # Check if a 'portfolio' parameter is passed in the request
+        portfolio_id = request.GET.get("portfolio")
+        if portfolio_id:
+            # Further filter the queryset by the portfolio
+            qs = qs.filter(portfolio=portfolio_id)
+        return qs
+
 
 class TransitionDomainAdmin(ListHeaderAdmin):
     """Custom transition domain admin class."""
@@ -2689,6 +2734,17 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin):
             return True
         return super().has_change_permission(request, obj)
 
+    def get_queryset(self, request):
+        """Custom get_queryset to filter by portfolio if portfolio is in the
+        request params."""
+        qs = super().get_queryset(request)
+        # Check if a 'portfolio' parameter is passed in the request
+        portfolio_id = request.GET.get("portfolio")
+        if portfolio_id:
+            # Further filter the queryset by the portfolio
+            qs = qs.filter(domain_info__portfolio=portfolio_id)
+        return qs
+
 
 class DraftDomainResource(resources.ModelResource):
     """defines how each field in the referenced model should be mapped to the corresponding fields in the
@@ -2873,7 +2929,7 @@ class PortfolioAdmin(ListHeaderAdmin):
         #     "classes": ("collapse", "closed"),
         #     "fields": ["administrators", "members"]}
         # ),
-        ("Portfolio domains", {"classes": ("collapse", "closed"), "fields": ["domains", "domain_requests"]}),
+        ("Portfolio domains", {"fields": ["domains", "domain_requests"]}),
         ("Type of organization", {"fields": ["organization_type", "federal_type"]}),
         (
             "Organization name and mailing address",
@@ -2955,18 +3011,27 @@ class PortfolioAdmin(ListHeaderAdmin):
     suborganizations.short_description = "Suborganizations"  # type: ignore
 
     def domains(self, obj: models.Portfolio):
-        """Returns a list of links for each related domain"""
+        """Returns the count of domains with a link to view them in the admin."""
-        queryset = obj.get_domains()
+        domain_count = obj.get_domains().count()  # Count the related domains
-        return self.get_field_links_as_list(
+        if domain_count > 0:
-            queryset, "domaininformation", link_info_attribute="get_state_display_of_domain"
+            # Construct the URL to the admin page, filtered by portfolio
-        )
+            url = reverse("admin:registrar_domain_changelist") + f"?portfolio={obj.id}"
+            label = "domain" if domain_count == 1 else "domains"
+            # Create a clickable link with the domain count
+            return format_html('<a href="{}">{} {}</a>', url, domain_count, label)
+        return "No domains"
 
     domains.short_description = "Domains"  # type: ignore
 
     def domain_requests(self, obj: models.Portfolio):
-        """Returns a list of links for each related domain request"""
+        """Returns the count of domain requests with a link to view them in the admin."""
-        queryset = obj.get_domain_requests()
+        domain_request_count = obj.get_domain_requests().count()  # Count the related domain requests
-        return self.get_field_links_as_list(queryset, "domainrequest", link_info_attribute="get_status_display")
+        if domain_request_count > 0:
+            # Construct the URL to the admin page, filtered by portfolio
+            url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}"
+            # Create a clickable link with the domain request count
+            return format_html('<a href="{}">{} domain requests</a>', url, domain_request_count)
+        return "No domain requests"
 
     domain_requests.short_description = "Domain requests"  # type: ignore
 

src/registrar/tests/test_admin.py

@@ -2107,9 +2107,7 @@ class TestPortfolioAdmin(TestCase):
         domain_2.save()
 
         domains = self.admin.domains(self.portfolio)
-        self.assertIn("domain1.gov", domains)
+        self.assertIn("2 domains", domains)
-        self.assertIn("domain2.gov", domains)
-        self.assertIn('<ul class="add-list-reset">', domains)
 
     @less_console_noise_decorator
     def test_domain_requests_display(self):
@@ -2118,6 +2116,4 @@ class TestPortfolioAdmin(TestCase):
         completed_domain_request(name="request2.gov", portfolio=self.portfolio)
 
         domain_requests = self.admin.domain_requests(self.portfolio)
-        self.assertIn("request1.gov", domain_requests)
+        self.assertIn("2 domain requests", domain_requests)
-        self.assertIn("request2.gov", domain_requests)
-        self.assertIn('<ul class="add-list-reset">', domain_requests)

src/registrar/tests/test_admin_domain.py

@@ -14,7 +14,9 @@ from registrar.models import (
     DomainInformation,
     User,
     Host,
+    Portfolio,
 )
+from registrar.models.user_domain_role import UserDomainRole
 from .common import (
     MockSESClient,
     completed_domain_request,
@@ -356,9 +358,11 @@ class TestDomainAdminWithClient(TestCase):
     def tearDown(self):
         super().tearDown()
         Host.objects.all().delete()
+        UserDomainRole.objects.all().delete()
         Domain.objects.all().delete()
         DomainInformation.objects.all().delete()
         DomainRequest.objects.all().delete()
+        Portfolio.objects.all().delete()
 
     @classmethod
     def tearDownClass(self):
@@ -452,6 +456,36 @@ class TestDomainAdminWithClient(TestCase):
         domain_request.delete()
         _creator.delete()
 
+    @less_console_noise_decorator
+    def test_domains_by_portfolio(self):
+        """
+        Tests that domains display for a portfolio.  And that domains outside the portfolio do not display.
+        """
+
+        portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser)
+        # Create a fake domain request and domain
+        _domain_request = completed_domain_request(
+            status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio
+        )
+        _domain_request.approve()
+
+        domain = _domain_request.approved_domain
+        domain2, _ = Domain.objects.get_or_create(name="fake.gov", state=Domain.State.READY)
+        UserDomainRole.objects.get_or_create()
+        UserDomainRole.objects.get_or_create(user=self.superuser, domain=domain2, role=UserDomainRole.Roles.MANAGER)
+
+        self.client.force_login(self.superuser)
+        response = self.client.get(
+            "/admin/registrar/domain/?portfolio={}".format(portfolio.pk),
+            follow=True,
+        )
+
+        # Make sure the page loaded, and that we're on the right page
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, domain.name)
+        self.assertNotContains(response, domain2.name)
+        self.assertContains(response, portfolio.organization_name)
+
     @less_console_noise_decorator
     def test_helper_text(self):
         """

src/registrar/tests/test_admin_request.py

@@ -22,6 +22,7 @@ from registrar.models import (
     Contact,
     Website,
     SeniorOfficial,
+    Portfolio,
 )
 from .common import (
     MockSESClient,
@@ -78,6 +79,7 @@ class TestDomainRequestAdmin(MockEppLib):
         Contact.objects.all().delete()
         Website.objects.all().delete()
         SeniorOfficial.objects.all().delete()
+        Portfolio.objects.all().delete()
         self.mock_client.EMAILS_SENT.clear()
 
     @classmethod
@@ -263,6 +265,33 @@ class TestDomainRequestAdmin(MockEppLib):
         self.assertContains(response, domain_request.requested_domain.name)
         self.assertContains(response, "<span>Show details</span>")
 
+    @less_console_noise_decorator
+    def test_domain_requests_by_portfolio(self):
+        """
+        Tests that domain_requests display for a portfolio. And requests not in portfolio do not display.
+        """
+
+        portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser)
+        # Create a fake domain request and domain
+        domain_request = completed_domain_request(
+            status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio
+        )
+        domain_request2 = completed_domain_request(
+            name="testdomain2.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW
+        )
+
+        self.client.force_login(self.superuser)
+        response = self.client.get(
+            "/admin/registrar/domainrequest/?portfolio={}".format(portfolio.pk),
+            follow=True,
+        )
+
+        # Make sure the page loaded, and that we're on the right page
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, domain_request.requested_domain.name)
+        self.assertNotContains(response, domain_request2.requested_domain.name)
+        self.assertContains(response, portfolio.organization_name)
+
     @less_console_noise_decorator
     def test_analyst_can_see_and_edit_alternative_domain(self):
         """Tests if an analyst can still see and edit the alternative domain field"""

src/registrar/tests/test_api.py

@@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User
 from django.contrib.auth import get_user_model
 from registrar.tests.common import create_superuser, create_user
 
+from api.tests.common import less_console_noise_decorator
+
 
 class GetSeniorOfficialJsonTest(TestCase):
     def setUp(self):
@@ -26,6 +28,7 @@ class GetSeniorOfficialJsonTest(TestCase):
         SeniorOfficial.objects.all().delete()
         FederalAgency.objects.all().delete()
 
+    @less_console_noise_decorator
     def test_get_senior_official_json_authenticated_superuser(self):
         """Test that a superuser can fetch the senior official information."""
         p = "adminpass"
@@ -38,6 +41,7 @@ class GetSeniorOfficialJsonTest(TestCase):
         self.assertEqual(data["last_name"], "Doe")
         self.assertEqual(data["title"], "Director")
 
+    @less_console_noise_decorator
     def test_get_senior_official_json_authenticated_analyst(self):
         """Test that an analyst user can fetch the senior official's information."""
         p = "userpass"
@@ -50,6 +54,7 @@ class GetSeniorOfficialJsonTest(TestCase):
         self.assertEqual(data["last_name"], "Doe")
         self.assertEqual(data["title"], "Director")
 
+    @less_console_noise_decorator
     def test_get_senior_official_json_unauthenticated(self):
         """Test that an unauthenticated user receives a 403 with an error message."""
         p = "password"
@@ -57,6 +62,7 @@ class GetSeniorOfficialJsonTest(TestCase):
         response = self.client.get(self.api_url, {"agency_name": "Test Agency"})
         self.assertEqual(response.status_code, 302)
 
+    @less_console_noise_decorator
     def test_get_senior_official_json_not_found(self):
         """Test that a request for a non-existent agency returns a 404 with an error message."""
         p = "adminpass"