Merge pull request #2622 from cisagov/rjm/2521-portfolio-members-admin

Issue #2521: Django admin portfolio members section - [RJM]

src/registrar/admin.py

@@ -11,6 +11,7 @@ from django.conf import settings
 from django.shortcuts import redirect
 from django_fsm import get_available_FIELD_transitions, FSMField
 from registrar.models.domain_information import DomainInformation
+from registrar.models.user_portfolio_permission import UserPortfolioPermission
 from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
 from waffle.decorators import flag_is_active
 from django.contrib import admin, messages
@@ -2968,11 +2969,7 @@ class PortfolioAdmin(ListHeaderAdmin):
     fieldsets = [
         # created_on is the created_at field, and portfolio_type is f"{organization_type} - {federal_type}"
         (None, {"fields": ["portfolio_type", "organization_name", "creator", "created_on", "notes"]}),
-        # TODO - uncomment in #2521
+        ("Portfolio members", {"fields": ["display_admins", "display_members"]}),
-        # ("Portfolio members", {
-        #     "classes": ("collapse", "closed"),
-        #     "fields": ["administrators", "members"]}
-        # ),
         ("Portfolio domains", {"fields": ["domains", "domain_requests"]}),
         ("Type of organization", {"fields": ["organization_type", "federal_type"]}),
         (
@@ -3020,15 +3017,118 @@ class PortfolioAdmin(ListHeaderAdmin):
     readonly_fields = [
         # This is the created_at field
         "created_on",
-        # Custom fields such as these must be defined as readonly.
+        # Django admin doesn't allow methods to be directly listed in fieldsets. We can
+        # display the custom methods display_admins amd display_members in the admin form if
+        # they are readonly.
         "federal_type",
         "domains",
         "domain_requests",
         "suborganizations",
         "portfolio_type",
+        "display_admins",
+        "display_members",
         "creator",
     ]
 
+    def get_admin_users(self, obj):
+        # Filter UserPortfolioPermission objects related to the portfolio
+        admin_permissions = UserPortfolioPermission.objects.filter(
+            portfolio=obj, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
+        # Get the user objects associated with these permissions
+        admin_users = User.objects.filter(portfolio_permissions__in=admin_permissions)
+
+        return admin_users
+
+    def get_non_admin_users(self, obj):
+        # Filter UserPortfolioPermission objects related to the portfolio that do NOT have the "Admin" role
+        non_admin_permissions = UserPortfolioPermission.objects.filter(portfolio=obj).exclude(
+            roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
+        # Get the user objects associated with these permissions
+        non_admin_users = User.objects.filter(portfolio_permissions__in=non_admin_permissions)
+
+        return non_admin_users
+
+    def display_admins(self, obj):
+        """Get joined users who are Admin, unpack and return an HTML block.
+
+        'DJA readonly can't handle querysets, so we need to unpack and return html here.
+        Alternatively, we could return querysets in context but that would limit where this
+        data would display in a custom change form without extensive template customization.
+
+        Will be used in the field_readonly block"""
+        admins = self.get_admin_users(obj)
+        if not admins:
+            return format_html("<p>No admins found.</p>")
+
+        admin_details = ""
+        for portfolio_admin in admins:
+            change_url = reverse("admin:registrar_user_change", args=[portfolio_admin.pk])
+            admin_details += "<address class='margin-bottom-2 dja-address-contact-list'>"
+            admin_details += f'<a href="{change_url}">{escape(portfolio_admin)}</a><br>'
+            admin_details += f"{escape(portfolio_admin.title)}<br>"
+            admin_details += f"{escape(portfolio_admin.email)}"
+            admin_details += "<div class='admin-icon-group admin-icon-group__clipboard-link'>"
+            admin_details += f"<input aria-hidden='true' class='display-none' value='{escape(portfolio_admin.email)}'>"
+            admin_details += (
+                "<button class='usa-button usa-button--unstyled padding-right-1 usa-button--icon padding-left-05"
+                + "button--clipboard copy-to-clipboard text-no-underline' type='button'>"
+            )
+            admin_details += "<svg class='usa-icon'>"
+            admin_details += "<use aria-hidden='true' xlink:href='/public/img/sprite.svg#content_copy'></use>"
+            admin_details += "</svg>"
+            admin_details += "Copy"
+            admin_details += "</button>"
+            admin_details += "</div><br>"
+            admin_details += f"{escape(portfolio_admin.phone)}"
+            admin_details += "</address>"
+        return format_html(admin_details)
+
+    display_admins.short_description = "Administrators"  # type: ignore
+
+    def display_members(self, obj):
+        """Get joined users who have roles/perms that are not Admin, unpack and return an HTML block.
+
+        DJA readonly can't handle querysets, so we need to unpack and return html here.
+        Alternatively, we could return querysets in context but that would limit where this
+        data would display in a custom change form without extensive template customization.
+
+        Will be used in the after_help_text block."""
+        members = self.get_non_admin_users(obj)
+        if not members:
+            return ""
+
+        member_details = (
+            "<table><thead><tr><th>Name</th><th>Title</th><th>Email</th>"
+            + "<th>Phone</th><th>Roles</th></tr></thead><tbody>"
+        )
+        for member in members:
+            full_name = member.get_formatted_name()
+            member_details += "<tr>"
+            member_details += f"<td>{escape(full_name)}</td>"
+            member_details += f"<td>{escape(member.title)}</td>"
+            member_details += f"<td>{escape(member.email)}</td>"
+            member_details += f"<td>{escape(member.phone)}</td>"
+            member_details += "<td>"
+            for role in member.portfolio_role_summary(obj):
+                member_details += f"<span class='usa-tag'>{escape(role)}</span> "
+            member_details += "</td></tr>"
+        member_details += "</tbody></table>"
+        return format_html(member_details)
+
+    display_members.short_description = "Members"  # type: ignore
+
+    def display_members_summary(self, obj):
+        """Will be passed as context and used in the field_readonly block."""
+        members = self.get_non_admin_users(obj)
+        if not members:
+            return {}
+
+        return self.get_field_links_as_list(members, "user", separator=", ")
+
     def federal_type(self, obj: models.Portfolio):
         """Returns the federal_type field"""
         return BranchChoices.get_branch_label(obj.federal_type) if obj.federal_type else "-"
@@ -3088,7 +3188,7 @@ class PortfolioAdmin(ListHeaderAdmin):
     ]
 
     def get_field_links_as_list(
-        self, queryset, model_name, attribute_name=None, link_info_attribute=None, seperator=None
+        self, queryset, model_name, attribute_name=None, link_info_attribute=None, separator=None
     ):
         """
         Generate HTML links for items in a queryset, using a specified attribute for link text.
@@ -3120,14 +3220,14 @@ class PortfolioAdmin(ListHeaderAdmin):
                 if link_info_attribute:
                     link += f" ({self.value_of_attribute(item, link_info_attribute)})"
 
-                if seperator:
+                if separator:
                     links.append(link)
                 else:
                     links.append(f"<li>{link}</li>")
 
-        # If no seperator is specified, just return an unordered list.
+        # If no separator is specified, just return an unordered list.
-        if seperator:
+        if separator:
-            return format_html(seperator.join(links)) if links else "-"
+            return format_html(separator.join(links)) if links else "-"
         else:
             links = "".join(links)
             return format_html(f'<ul class="add-list-reset">{links}</ul>') if links else "-"
@@ -3170,8 +3270,12 @@ class PortfolioAdmin(ListHeaderAdmin):
         return readonly_fields
 
     def change_view(self, request, object_id, form_url="", extra_context=None):
-        """Add related suborganizations and domain groups"""
+        """Add related suborganizations and domain groups.
-        extra_context = {"skip_additional_contact_info": True}
+        Add the summary for the portfolio members field (list of members that link to change_forms)."""
+        obj = self.get_object(request, object_id)
+        extra_context = extra_context or {}
+        extra_context["skip_additional_contact_info"] = True
+        extra_context["display_members_summary"] = self.display_members_summary(obj)
         return super().change_view(request, object_id, form_url, extra_context)
 
     def save_model(self, request, obj, form, change):

src/registrar/models/user.py

@@ -245,6 +245,49 @@ class User(AbstractUser):
             return permission.portfolio
         return None
 
+    def has_edit_requests(self, portfolio):
+        return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS)
+
+    def portfolio_role_summary(self, portfolio):
+        """Returns a list of roles based on the user's permissions."""
+        roles = []
+
+        # Define the conditions and their corresponding roles
+        conditions_roles = [
+            (self.has_edit_suborganization(portfolio), ["Admin"]),
+            (
+                self.has_view_all_domains_permission(portfolio)
+                and self.has_domain_requests_portfolio_permission(portfolio)
+                and self.has_edit_requests(portfolio),
+                ["View-only admin", "Domain requestor"],
+            ),
+            (
+                self.has_view_all_domains_permission(portfolio)
+                and self.has_domain_requests_portfolio_permission(portfolio),
+                ["View-only admin"],
+            ),
+            (
+                self.has_base_portfolio_permission(portfolio)
+                and self.has_edit_requests(portfolio)
+                and self.has_domains_portfolio_permission(portfolio),
+                ["Domain requestor", "Domain manager"],
+            ),
+            (self.has_base_portfolio_permission(portfolio) and self.has_edit_requests(portfolio), ["Domain requestor"]),
+            (
+                self.has_base_portfolio_permission(portfolio) and self.has_domains_portfolio_permission(portfolio),
+                ["Domain manager"],
+            ),
+            (self.has_base_portfolio_permission(portfolio), ["Member"]),
+        ]
+
+        # Evaluate conditions and add roles
+        for condition, role_list in conditions_roles:
+            if condition:
+                roles.extend(role_list)
+                break
+
+        return roles
+
     @classmethod
     def needs_identity_verification(cls, email, uuid):
         """A method used by our oidc classes to test whether a user needs email/uuid verification

src/registrar/templates/admin/input_with_clipboard.html

@@ -17,7 +17,7 @@ Template for an input field with a clipboard
             >
                 <use aria-hidden="true" xlink:href="{%static 'img/sprite.svg'%}#content_copy"></use>
             </svg>
-            <span>Copy</span>
+            Copy
         </div>
     </button>
 </div>
@@ -25,7 +25,7 @@ Template for an input field with a clipboard
 <div class="admin-icon-group admin-icon-group__clipboard-link">
     <input aria-hidden="true" class="display-none" value="{{ field.email }}" />
     <button 
-    class="usa-button usa-button--unstyled padding-right-1 usa-button--icon button--clipboard copy-to-clipboard text-no-underline" 
+    class="usa-button usa-button--unstyled padding-right-1 usa-button--icon button--clipboard copy-to-clipboard text-no-underline padding-left-05" 
     type="button"
     >
         <svg 
@@ -33,7 +33,7 @@ Template for an input field with a clipboard
         >
             <use aria-hidden="true" xlink:href="{%static 'img/sprite.svg'%}#content_copy"></use>
         </svg>
-        <span class="padding-left-05">Copy</span>
+        Copy
     </button>
 </div>
 {% endif %}

src/registrar/templates/django/admin/includes/detail_table_fieldset.html

@@ -137,6 +137,16 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
             {% endfor %}
             {% endwith %}
         </div>
+    {% elif field.field.name == "display_admins" %}
+        <div class="readonly">{{ field.contents|safe }}</div>
+    {% elif field.field.name == "display_members" %}
+        <div class="readonly">
+            {% if display_members_summary %}
+                {{ display_members_summary }}
+            {% else %}
+                <p>No additional members found.</p>
+            {% endif %}
+        </div>
     {% else %}
         <div class="readonly">{{ field.contents }}</div>
     {% endif %}
@@ -330,6 +340,13 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
             </details>
         {% endif %}
         {% endwith %}
+    {% elif field.field.name == "display_members" and field.contents %}
+        <details class="margin-top-1 dja-detail-table" aria-role="button" open>
+            <summary class="padding-1 padding-left-0 dja-details-summary">Details</summary>
+            <div class="grid-container margin-left-0 padding-left-0 padding-right-0 dja-details-contents">
+                {{ field.contents|safe }}
+            </div>
+        </details>
     {% elif field.field.name == "state_territory" and original_object|model_name_lowercase != 'portfolio' %}
         <div class="flex-container margin-top-2">
             <span>

src/registrar/templates/django/admin/portfolio_change_form.html

@@ -17,8 +17,7 @@
         This is a placeholder for now.
 
         Disclaimer:
-        When extending the fieldset view - *make a new one* that extends from detail_table_fieldset.
+        When extending the fieldset view consider whether you need to make a new one that extends from detail_table_fieldset.
-        For instance, "portfolio_fieldset.html".
         detail_table_fieldset is used on multiple admin pages, so a change there can have unintended consequences.
         {% endcomment %}
         {% include "django/admin/includes/detail_table_fieldset.html" with original_object=original %}

src/registrar/tests/test_admin.py

@@ -45,6 +45,8 @@ from registrar.models import (
 from registrar.models.portfolio_invitation import PortfolioInvitation
 from registrar.models.senior_official import SeniorOfficial
 from registrar.models.user_domain_role import UserDomainRole
+from registrar.models.user_portfolio_permission import UserPortfolioPermission
+from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
 from registrar.models.verified_by_staff import VerifiedByStaff
 from .common import (
     MockDbForSharedTests,
@@ -2066,6 +2068,7 @@ class TestPortfolioAdmin(TestCase):
         DomainRequest.objects.all().delete()
         Domain.objects.all().delete()
         Portfolio.objects.all().delete()
+        User.objects.all().delete()
 
     @less_console_noise_decorator
     def test_created_on_display(self):
@@ -2117,3 +2120,91 @@ class TestPortfolioAdmin(TestCase):
 
         domain_requests = self.admin.domain_requests(self.portfolio)
         self.assertIn("2 domain requests", domain_requests)
+
+    @less_console_noise_decorator
+    def test_portfolio_members_display(self):
+        """Tests the custom portfolio members field, admin and member sections"""
+        admin_user_1 = User.objects.create(
+            username="testuser1",
+            first_name="Gerald",
+            last_name="Meoward",
+            title="Captain",
+            email="meaoward@gov.gov",
+        )
+
+        UserPortfolioPermission.objects.all().create(
+            user=admin_user_1, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
+        admin_user_2 = User.objects.create(
+            username="testuser2",
+            first_name="Arnold",
+            last_name="Poopy",
+            title="Major",
+            email="poopy@gov.gov",
+        )
+
+        UserPortfolioPermission.objects.all().create(
+            user=admin_user_2, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
+        admin_user_3 = User.objects.create(
+            username="testuser3",
+            first_name="Mad",
+            last_name="Max",
+            title="Road warrior",
+            email="madmax@gov.gov",
+        )
+
+        UserPortfolioPermission.objects.all().create(
+            user=admin_user_3, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER]
+        )
+
+        admin_user_4 = User.objects.create(
+            username="testuser4",
+            first_name="Agent",
+            last_name="Smith",
+            title="Program",
+            email="thematrix@gov.gov",
+        )
+
+        UserPortfolioPermission.objects.all().create(
+            user=admin_user_4,
+            portfolio=self.portfolio,
+            additional_permissions=[
+                UserPortfolioPermissionChoices.VIEW_PORTFOLIO,
+                UserPortfolioPermissionChoices.EDIT_REQUESTS,
+            ],
+        )
+
+        display_admins = self.admin.display_admins(self.portfolio)
+
+        self.assertIn(
+            f'<a href="/admin/registrar/user/{admin_user_1.pk}/change/">Gerald Meoward meaoward@gov.gov</a>',
+            display_admins,
+        )
+        self.assertIn("Captain", display_admins)
+        self.assertIn(
+            f'<a href="/admin/registrar/user/{admin_user_2.pk}/change/">Arnold Poopy poopy@gov.gov</a>', display_admins
+        )
+        self.assertIn("Major", display_admins)
+
+        display_members_summary = self.admin.display_members_summary(self.portfolio)
+
+        self.assertIn(
+            f'<a href="/admin/registrar/user/{admin_user_3.pk}/change/">Mad Max madmax@gov.gov</a>',
+            display_members_summary,
+        )
+        self.assertIn(
+            f'<a href="/admin/registrar/user/{admin_user_4.pk}/change/">Agent Smith thematrix@gov.gov</a>',
+            display_members_summary,
+        )
+
+        display_members = self.admin.display_members(self.portfolio)
+
+        self.assertIn("Mad Max", display_members)
+        self.assertIn("<span class='usa-tag'>Member</span>", display_members)
+        self.assertIn("Road warrior", display_members)
+        self.assertIn("Agent Smith", display_members)
+        self.assertIn("<span class='usa-tag'>Domain requestor</span>", display_members)
+        self.assertIn("Program", display_members)

src/registrar/tests/test_models.py

@@ -1311,6 +1311,7 @@ class TestUser(TestCase):
         self.domain, _ = Domain.objects.get_or_create(name="igorville.gov")
         self.user, _ = User.objects.get_or_create(email=self.email)
         self.factory = RequestFactory()
+        self.portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.user)
 
     def tearDown(self):
         super().tearDown()
@@ -1325,6 +1326,65 @@ class TestUser(TestCase):
         User.objects.all().delete()
         UserDomainRole.objects.all().delete()
 
+    @patch.object(User, "has_edit_suborganization", return_value=True)
+    def test_portfolio_role_summary_admin(self, mock_edit_suborganization):
+        # Test if the user is recognized as an Admin
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Admin"])
+
+    @patch.multiple(
+        User,
+        has_view_all_domains_permission=lambda self, portfolio: True,
+        has_domain_requests_portfolio_permission=lambda self, portfolio: True,
+        has_edit_requests=lambda self, portfolio: True,
+    )
+    def test_portfolio_role_summary_view_only_admin_and_domain_requestor(self):
+        # Test if the user has both 'View-only admin' and 'Domain requestor' roles
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["View-only admin", "Domain requestor"])
+
+    @patch.multiple(
+        User,
+        has_view_all_domains_permission=lambda self, portfolio: True,
+        has_domain_requests_portfolio_permission=lambda self, portfolio: True,
+    )
+    def test_portfolio_role_summary_view_only_admin(self):
+        # Test if the user is recognized as a View-only admin
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["View-only admin"])
+
+    @patch.multiple(
+        User,
+        has_base_portfolio_permission=lambda self, portfolio: True,
+        has_edit_requests=lambda self, portfolio: True,
+        has_domains_portfolio_permission=lambda self, portfolio: True,
+    )
+    def test_portfolio_role_summary_member_domain_requestor_domain_manager(self):
+        # Test if the user has 'Member', 'Domain requestor', and 'Domain manager' roles
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain requestor", "Domain manager"])
+
+    @patch.multiple(
+        User, has_base_portfolio_permission=lambda self, portfolio: True, has_edit_requests=lambda self, portfolio: True
+    )
+    def test_portfolio_role_summary_member_domain_requestor(self):
+        # Test if the user has 'Member' and 'Domain requestor' roles
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain requestor"])
+
+    @patch.multiple(
+        User,
+        has_base_portfolio_permission=lambda self, portfolio: True,
+        has_domains_portfolio_permission=lambda self, portfolio: True,
+    )
+    def test_portfolio_role_summary_member_domain_manager(self):
+        # Test if the user has 'Member' and 'Domain manager' roles
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain manager"])
+
+    @patch.multiple(User, has_base_portfolio_permission=lambda self, portfolio: True)
+    def test_portfolio_role_summary_member(self):
+        # Test if the user is recognized as a Member
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Member"])
+
+    def test_portfolio_role_summary_empty(self):
+        # Test if the user has no roles
+        self.assertEqual(self.user.portfolio_role_summary(self.portfolio), [])
+
     @less_console_noise_decorator
     def test_check_transition_domains_without_domains_on_login(self):
         """A user's on_each_login callback does not check transition domains.