From eb061be71bf34ec3f75fabe282089e5c80d80457 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 21 Aug 2024 12:25:27 -0400 Subject: [PATCH 01/10] Django admin portfolio members section --- src/registrar/admin.py | 110 ++++++++++++++++-- src/registrar/models/user.py | 43 +++++++ .../admin/includes/detail_table_fieldset.html | 11 ++ .../django/admin/portfolio_change_form.html | 3 +- src/registrar/tests/test_admin.py | 80 +++++++++++++ src/registrar/tests/test_models.py | 55 +++++++++ 6 files changed, 288 insertions(+), 14 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 423c0a01b..12c5d8cf8 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2867,11 +2867,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", {"classes": ("collapse", "closed"), "fields": ["domains", "domain_requests"]}), ("Type of organization", {"fields": ["organization_type", "federal_type"]}), ( @@ -2925,8 +2921,94 @@ class PortfolioAdmin(ListHeaderAdmin): "domain_requests", "suborganizations", "portfolio_type", + # 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. + "display_admins", + "display_members", ] + 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 = [user for user in obj.user.all() if "Admin" in user.portfolio_role_summary] + 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'{portfolio_admin}
' + admin_details += f"{portfolio_admin.title}
" + admin_details += f"{portfolio_admin.email}" + admin_details += "
" + admin_details += f"" + admin_details += ( + "" + admin_details += "

" + admin_details += f"{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 = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] + if not members: + return format_html("

No members found.

") + + member_details = ( + "" + + "" + ) + for member in members: + full_name = ( + f"{member.first_name} {member.middle_name} {member.last_name}" + if member.middle_name + else f"{member.first_name} {member.last_name}" + ) + member_details += "" + member_details += f"" + member_details += f"" + member_details += f"" + member_details += f"" + member_details += "" + member_details += "
NameTitleEmailPhoneRoles
{full_name}{member.title}{member.email}{member.phone}" + for role in member.portfolio_role_summary: + member_details += f"{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 = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] + 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 "-" @@ -2977,7 +3059,7 @@ def domain_requests(self, obj: models.Portfolio): ] 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. @@ -3009,14 +3091,14 @@ def get_field_links_as_list( 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 "-" @@ -3059,8 +3141,12 @@ def get_readonly_fields(self, request, obj=None): 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 81d3b9b61..a6026b63a 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -293,6 +293,49 @@ def has_view_suborganization(self): def has_edit_suborganization(self): return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + def has_edit_requests(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_REQUESTS) + + @property + def portfolio_role_summary(self): + """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(), ["Admin"]), + ( + self.has_view_all_domains_permission() + and self.has_domain_requests_portfolio_permission() + and self.has_edit_requests(), + ["View-only admin", "Domain requestor"], + ), + ( + self.has_view_all_domains_permission() and self.has_domain_requests_portfolio_permission(), + ["View-only admin"], + ), + ( + self.has_base_portfolio_permission() + and self.has_edit_requests() + and self.has_domains_portfolio_permission(), + ["Member", "Domain requestor", "Domain manager"], + ), + (self.has_base_portfolio_permission() and self.has_edit_requests(), ["Member", "Domain requestor"]), + ( + self.has_base_portfolio_permission() and self.has_domains_portfolio_permission(), + ["Member", "Domain manager"], + ), + (self.has_base_portfolio_permission(), ["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/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 683f33117..5da6c2ca2 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,10 @@ {% endfor %} {% endwith %} + {% elif field.field.name == "display_admins" %} +
    {{ field.contents|safe }}
    + {% elif field.field.name == "display_members" %} +
    {{ display_members_summary }}
    {% else %}
    {{ field.contents }}
    {% endif %} @@ -240,6 +244,13 @@ {% endif %} {% endwith %} + {% elif field.field.name == "display_members" %} +
    + Details +
    + {{ field.contents|safe }} +
    +
    {% elif field.field.name == "state_territory" %}
    diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 9d59aae42..a2d70611c 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/src/registrar/templates/django/admin/portfolio_change_form.html @@ -14,8 +14,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 827742ef1..7ef1a4a97 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ 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.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -2066,6 +2067,7 @@ def tearDown(self): 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): @@ -2121,3 +2123,81 @@ def test_domain_requests_display(self): self.assertIn("request1.gov", domain_requests) self.assertIn("request2.gov", domain_requests) self.assertIn('
      ', 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", + portfolio=self.portfolio, + 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", + portfolio=self.portfolio, + 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", + portfolio=self.portfolio, + 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", + portfolio=self.portfolio, + 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 f4e998fff..5b9d0ade3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1201,6 +1201,61 @@ def tearDown(self): 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, ["Admin"]) + + @patch.multiple( + User, + has_view_all_domains_permission=lambda self: True, + has_domain_requests_portfolio_permission=lambda self: True, + has_edit_requests=lambda self: 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, ["View-only admin", "Domain requestor"]) + + @patch.multiple( + User, + has_view_all_domains_permission=lambda self: True, + has_domain_requests_portfolio_permission=lambda self: 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, ["View-only admin"]) + + @patch.multiple( + User, + has_base_portfolio_permission=lambda self: True, + has_edit_requests=lambda self: True, + has_domains_portfolio_permission=lambda self: 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, ["Member", "Domain requestor", "Domain manager"]) + + @patch.multiple(User, has_base_portfolio_permission=lambda self: True, has_edit_requests=lambda self: 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, ["Member", "Domain requestor"]) + + @patch.multiple( + User, has_base_portfolio_permission=lambda self: True, has_domains_portfolio_permission=lambda self: 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, ["Member", "Domain manager"]) + + @patch.multiple(User, has_base_portfolio_permission=lambda self: True) + def test_portfolio_role_summary_member(self): + # Test if the user is recognized as a Member + self.assertEqual(self.user.portfolio_role_summary, ["Member"]) + + def test_portfolio_role_summary_empty(self): + # Test if the user has no roles + self.assertEqual(self.user.portfolio_role_summary, []) + @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. From e82326346c5af4ac1b175418d21a229cc4634463 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 21 Aug 2024 12:43:38 -0400 Subject: [PATCH 02/10] clean up roles logic --- src/registrar/models/user.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a6026b63a..8584bc0a1 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -318,12 +318,12 @@ def portfolio_role_summary(self): self.has_base_portfolio_permission() and self.has_edit_requests() and self.has_domains_portfolio_permission(), - ["Member", "Domain requestor", "Domain manager"], + ["Domain requestor", "Domain manager"], ), - (self.has_base_portfolio_permission() and self.has_edit_requests(), ["Member", "Domain requestor"]), + (self.has_base_portfolio_permission() and self.has_edit_requests(), ["Domain requestor"]), ( self.has_base_portfolio_permission() and self.has_domains_portfolio_permission(), - ["Member", "Domain manager"], + ["Domain manager"], ), (self.has_base_portfolio_permission(), ["Member"]), ] From d07872bfa1697b077e7316e2b8a1b51eb7af674f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:55:41 -0400 Subject: [PATCH 03/10] clean up tamplate and fix unit tests --- src/registrar/admin.py | 2 +- .../django/admin/includes/detail_table_fieldset.html | 10 ++++++++-- src/registrar/tests/test_models.py | 6 +++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 12c5d8cf8..e13353fe6 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2975,7 +2975,7 @@ def display_members(self, obj): Will be used in the after_help_text block.""" members = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] if not members: - return format_html("

      No members found.

      ") + return '' member_details = ( "" 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 5da6c2ca2..359897c04 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -140,7 +140,13 @@ {% elif field.field.name == "display_admins" %}
      {{ field.contents|safe }}
      {% elif field.field.name == "display_members" %} -
      {{ display_members_summary }}
      +
      + {% if display_members_summary %} + {{ display_members_summary }} + {% else %} +

      No members found.

      + {% endif %} +
      {% else %}
      {{ field.contents }}
      {% endif %} @@ -244,7 +250,7 @@ {% endif %} {% endwith %} - {% elif field.field.name == "display_members" %} + {% elif field.field.name == "display_members" and field.contents %}
      Details
      diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 5b9d0ade3..2ab53a1c7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1233,19 +1233,19 @@ def test_portfolio_role_summary_view_only_admin(self): ) 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, ["Member", "Domain requestor", "Domain manager"]) + self.assertEqual(self.user.portfolio_role_summary, ["Domain requestor", "Domain manager"]) @patch.multiple(User, has_base_portfolio_permission=lambda self: True, has_edit_requests=lambda self: 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, ["Member", "Domain requestor"]) + self.assertEqual(self.user.portfolio_role_summary, ["Domain requestor"]) @patch.multiple( User, has_base_portfolio_permission=lambda self: True, has_domains_portfolio_permission=lambda self: 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, ["Member", "Domain manager"]) + self.assertEqual(self.user.portfolio_role_summary, ["Domain manager"]) @patch.multiple(User, has_base_portfolio_permission=lambda self: True) def test_portfolio_role_summary_member(self): From 4e0416e514da83748418851d05e64635b712cb9d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:13:14 -0400 Subject: [PATCH 04/10] lint --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e13353fe6..a5c7a16d3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2975,7 +2975,7 @@ def display_members(self, obj): Will be used in the after_help_text block.""" members = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] if not members: - return '' + return "" member_details = ( "
      NameTitleEmail
      " From 2af57c8aa8a9a258cf99bbd0c7a004aadf29adea Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 17:21:04 -0400 Subject: [PATCH 05/10] Revise for multiple portfolios --- src/registrar/admin.py | 35 +++++++++++++++---- src/registrar/models/user.py | 30 ++++++++-------- .../templates/admin/input_with_clipboard.html | 6 ++-- src/registrar/tests/test_admin.py | 25 +++++++++---- 4 files changed, 65 insertions(+), 31 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e1bf25449..a47617c28 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -10,6 +10,7 @@ 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 @@ -3008,6 +3009,28 @@ class PortfolioAdmin(ListHeaderAdmin): "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. @@ -3016,7 +3039,7 @@ def display_admins(self, obj): data would display in a custom change form without extensive template customization. Will be used in the field_readonly block""" - admins = [user for user in obj.user.all() if "Admin" in user.portfolio_role_summary] + admins = self.get_admin_users(obj) if not admins: return format_html("

      No admins found.

      ") @@ -3030,13 +3053,13 @@ def display_admins(self, obj): admin_details += "
      " admin_details += f"" admin_details += ( - "" admin_details += "

      " admin_details += f"{portfolio_admin.phone}" @@ -3053,7 +3076,7 @@ def display_members(self, obj): data would display in a custom change form without extensive template customization. Will be used in the after_help_text block.""" - members = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] + members = self.get_non_admin_users(obj) if not members: return "" @@ -3073,7 +3096,7 @@ def display_members(self, obj): member_details += f"      " member_details += f"" member_details += "" member_details += "
      NameTitleEmail{member.email}{member.phone}" - for role in member.portfolio_role_summary: + for role in member.portfolio_role_summary(obj): member_details += f"{role} " member_details += "
      " @@ -3083,7 +3106,7 @@ def display_members(self, obj): def display_members_summary(self, obj): """Will be passed as context and used in the field_readonly block.""" - members = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] + members = self.get_non_admin_users(obj) if not members: return {} diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 2c17b7b27..8d91c2a8c 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -245,39 +245,39 @@ def get_first_portfolio(self): return permission.portfolio return None - def has_edit_requests(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_REQUESTS) + def has_edit_requests(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS) - @property - def portfolio_role_summary(self): + 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(), ["Admin"]), + (self.has_edit_suborganization(portfolio), ["Admin"]), ( - self.has_view_all_domains_permission() - and self.has_domain_requests_portfolio_permission() - and self.has_edit_requests(), + 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() and self.has_domain_requests_portfolio_permission(), + self.has_view_all_domains_permission(portfolio) + and self.has_domain_requests_portfolio_permission(portfolio), ["View-only admin"], ), ( - self.has_base_portfolio_permission() - and self.has_edit_requests() - and self.has_domains_portfolio_permission(), + 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() and self.has_edit_requests(), ["Domain requestor"]), + (self.has_base_portfolio_permission(portfolio) and self.has_edit_requests(portfolio), ["Domain requestor"]), ( - self.has_base_portfolio_permission() and self.has_domains_portfolio_permission(), + self.has_base_portfolio_permission(portfolio) and self.has_domains_portfolio_permission(portfolio), ["Domain manager"], ), - (self.has_base_portfolio_permission(), ["Member"]), + (self.has_base_portfolio_permission(portfolio), ["Member"]), ] # Evaluate conditions and add roles 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 @@ > - Copy + Copy
    @@ -25,7 +25,7 @@
    {% endif %} \ No newline at end of file diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 02cd6e0f6..93e611c1a 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ 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 ( @@ -2129,8 +2130,10 @@ def test_portfolio_members_display(self): last_name="Meoward", title="Captain", email="meaoward@gov.gov", - portfolio=self.portfolio, - portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_1, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) admin_user_2 = User.objects.create( @@ -2139,8 +2142,10 @@ def test_portfolio_members_display(self): last_name="Poopy", title="Major", email="poopy@gov.gov", - portfolio=self.portfolio, - portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_2, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) admin_user_3 = User.objects.create( @@ -2149,8 +2154,10 @@ def test_portfolio_members_display(self): last_name="Max", title="Road warrior", email="madmax@gov.gov", - portfolio=self.portfolio, - portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_3, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) admin_user_4 = User.objects.create( @@ -2159,8 +2166,12 @@ def test_portfolio_members_display(self): last_name="Smith", title="Program", email="thematrix@gov.gov", + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_4, portfolio=self.portfolio, - portfolio_additional_permissions=[ + additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_REQUESTS, ], From ef2adaaf288e7a386ce67e2cbec0b5632ed8fcad Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 17:25:56 -0400 Subject: [PATCH 06/10] No additional members found --- .../templates/django/admin/includes/detail_table_fieldset.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 da2753b4c..7eea7b72b 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -144,7 +144,7 @@ {% if display_members_summary %} {{ display_members_summary }} {% else %} -

    No members found.

    +

    No additional members found.

    {% endif %} {% else %} From ca1c9bbb0424313425848f5e004b64968994112b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 17:35:57 -0400 Subject: [PATCH 07/10] fix model tests? --- src/registrar/tests/test_models.py | 39 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 19f9d46c3..3510ff65e 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1308,6 +1308,7 @@ def setUp(self): 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,57 +1326,57 @@ def tearDown(self): @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, ["Admin"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Admin"]) @patch.multiple( User, - has_view_all_domains_permission=lambda self: True, - has_domain_requests_portfolio_permission=lambda self: True, - has_edit_requests=lambda self: True, + 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, ["View-only admin", "Domain requestor"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["View-only admin", "Domain requestor"]) @patch.multiple( User, - has_view_all_domains_permission=lambda self: True, - has_domain_requests_portfolio_permission=lambda self: True, + 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, ["View-only admin"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["View-only admin"]) @patch.multiple( User, - has_base_portfolio_permission=lambda self: True, - has_edit_requests=lambda self: True, - has_domains_portfolio_permission=lambda self: True, + 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, ["Domain requestor", "Domain manager"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain requestor", "Domain manager"]) - @patch.multiple(User, has_base_portfolio_permission=lambda self: True, has_edit_requests=lambda self: True) + @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, ["Domain requestor"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain requestor"]) @patch.multiple( - User, has_base_portfolio_permission=lambda self: True, has_domains_portfolio_permission=lambda self: True + 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, ["Domain manager"]) + self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain manager"]) - @patch.multiple(User, has_base_portfolio_permission=lambda self: True) + @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, ["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.assertEqual(self.user.portfolio_role_summary(self.portfolio), []) @less_console_noise_decorator def test_check_transition_domains_without_domains_on_login(self): From e20bfa55678ee12c7ba897ecf52f4c3cad0ad84b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 17:44:09 -0400 Subject: [PATCH 08/10] lint --- src/registrar/admin.py | 6 +----- src/registrar/tests/test_models.py | 8 ++++++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a47617c28..c8be04715 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3085,11 +3085,7 @@ def display_members(self, obj): + "PhoneRoles" ) for member in members: - full_name = ( - f"{member.first_name} {member.middle_name} {member.last_name}" - if member.middle_name - else f"{member.first_name} {member.last_name}" - ) + full_name = member.get_formatted_name() member_details += "" member_details += f"{full_name}" member_details += f"{member.title}" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3510ff65e..7983fed36 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1357,13 +1357,17 @@ 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) + @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 + 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 From 79fbc5c54026c7516e39347a7d7be50d0f9a6263 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 17:46:05 -0400 Subject: [PATCH 09/10] cleanup --- src/registrar/admin.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index c8be04715..fd32238f7 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2943,11 +2943,6 @@ class PortfolioAdmin(ListHeaderAdmin): # 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"]}), ("Portfolio members", {"fields": ["display_admins", "display_members"]}), - # TODO - uncomment in #2521 - # ("Portfolio members", { - # "classes": ("collapse", "closed"), - # "fields": ["administrators", "members"]} - # ), ("Portfolio domains", {"fields": ["domains", "domain_requests"]}), ("Type of organization", {"fields": ["organization_type", "federal_type"]}), ( @@ -2995,15 +2990,14 @@ 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", - # 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. "display_admins", "display_members", "creator", From cba2b27d79122951ee67c680a62021a72beebe48 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 4 Sep 2024 13:44:45 -0400 Subject: [PATCH 10/10] escape dynamic values --- src/registrar/admin.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4b36c9589..e3bd5c9f7 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3068,11 +3068,11 @@ def display_admins(self, obj): for portfolio_admin in admins: change_url = reverse("admin:registrar_user_change", args=[portfolio_admin.pk]) admin_details += "
    " - admin_details += f'{portfolio_admin}
    ' - admin_details += f"{portfolio_admin.title}
    " - admin_details += f"{portfolio_admin.email}" + 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 += f"" admin_details += ( "" admin_details += "

    " - admin_details += f"{portfolio_admin.phone}" + admin_details += f"{escape(portfolio_admin.phone)}" admin_details += "
    " return format_html(admin_details) @@ -3108,13 +3108,13 @@ def display_members(self, obj): for member in members: full_name = member.get_formatted_name() member_details += "" - member_details += f"{full_name}" - member_details += f"{member.title}" - member_details += f"{member.email}" - member_details += f"{member.phone}" + member_details += f"{escape(full_name)}" + member_details += f"{escape(member.title)}" + member_details += f"{escape(member.email)}" + member_details += f"{escape(member.phone)}" member_details += "" for role in member.portfolio_role_summary(obj): - member_details += f"{role} " + member_details += f"{escape(role)} " member_details += "" member_details += "" return format_html(member_details)