diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6b42cf96b..e3bd5c9f7 100644 --- a/src/registrar/admin.py +++ b/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", { - # "classes": ("collapse", "closed"), - # "fields": ["administrators", "members"]} - # ), + ("Portfolio members", {"fields": ["display_admins", "display_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("

No admins found.

") + + admin_details = "" + for portfolio_admin in admins: + change_url = reverse("admin:registrar_user_change", args=[portfolio_admin.pk]) + admin_details += "
" + admin_details += f'{escape(portfolio_admin)}
' + admin_details += f"{escape(portfolio_admin.title)}
" + admin_details += f"{escape(portfolio_admin.email)}" + admin_details += "
" + admin_details += f"" + admin_details += ( + "" + admin_details += "

" + admin_details += f"{escape(portfolio_admin.phone)}" + admin_details += "
" + 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 = ( + "" + + "" + ) + for member in members: + full_name = member.get_formatted_name() + member_details += "" + member_details += f"" + member_details += f"" + member_details += f"" + member_details += f"" + member_details += "" + member_details += "
NameTitleEmailPhoneRoles
{escape(full_name)}{escape(member.title)}{escape(member.email)}{escape(member.phone)}" + for role in member.portfolio_role_summary(obj): + member_details += f"{escape(role)} " + member_details += "
" + 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"
  • {link}
    • ") - # If no seperator is specified, just return an unordered list. - if seperator: - return format_html(seperator.join(links)) if links else "-" + # If no separator is specified, just return an unordered list. + if separator: + return format_html(separator.join(links)) if links else "-" else: links = "".join(links) return format_html(f'') 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""" - extra_context = {"skip_additional_contact_info": True} + """Add related suborganizations and domain groups. + 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): diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a7ea1e14a..8d91c2a8c 100644 --- a/src/registrar/models/user.py +++ b/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 diff --git a/src/registrar/templates/admin/input_with_clipboard.html b/src/registrar/templates/admin/input_with_clipboard.html index ea2fbce33..5ad2b27f7 100644 --- a/src/registrar/templates/admin/input_with_clipboard.html +++ b/src/registrar/templates/admin/input_with_clipboard.html @@ -17,7 +17,7 @@ Template for an input field with a clipboard > - Copy + Copy @@ -25,7 +25,7 @@ Template for an input field with a clipboard
    {% endif %} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 1c1a7c2a9..5e1057139 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/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 %} + {% elif field.field.name == "display_admins" %} +
    {{ field.contents|safe }}
    + {% elif field.field.name == "display_members" %} +
    + {% if display_members_summary %} + {{ display_members_summary }} + {% else %} +

    No additional members found.

    + {% endif %} +
    {% else %}
    {{ field.contents }}
    {% endif %} @@ -330,6 +340,13 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% endif %} {% endwith %} + {% elif field.field.name == "display_members" and field.contents %} +
    + Details +
    + {{ field.contents|safe }} +
    +
    {% elif field.field.name == "state_territory" and original_object|model_name_lowercase != 'portfolio' %}
    diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 6c7aca0ea..8dae8a080 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/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. - For instance, "portfolio_fieldset.html". + When extending the fieldset view consider whether you need to make a new one that extends from detail_table_fieldset. 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 %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index a435c6a69..93e611c1a 100644 --- a/src/registrar/tests/test_admin.py +++ b/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'Gerald Meoward meaoward@gov.gov', + display_admins, + ) + self.assertIn("Captain", display_admins) + self.assertIn( + f'Arnold Poopy poopy@gov.gov', display_admins + ) + self.assertIn("Major", display_admins) + + display_members_summary = self.admin.display_members_summary(self.portfolio) + + self.assertIn( + f'Mad Max madmax@gov.gov', + display_members_summary, + ) + self.assertIn( + f'Agent Smith thematrix@gov.gov', + display_members_summary, + ) + + display_members = self.admin.display_members(self.portfolio) + + self.assertIn("Mad Max", display_members) + self.assertIn("Member", display_members) + self.assertIn("Road warrior", display_members) + self.assertIn("Agent Smith", display_members) + self.assertIn("Domain requestor", display_members) + self.assertIn("Program", display_members) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a1b21ee2..f2e9a4bfa 100644 --- a/src/registrar/tests/test_models.py +++ b/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.