review changes: minor tweaks

src/registrar/admin.py

@@ -2894,6 +2894,15 @@ class VerifiedByStaffAdmin(ListHeaderAdmin):
 
 
 class PortfolioAdmin(ListHeaderAdmin):
+    
+    class Meta:
+        """Contains meta information about this class"""
+
+        model = models.Portfolio
+        fields = "__all__"
+
+    _meta = Meta()
+
     change_form_template = "django/admin/portfolio_change_form.html"
     fieldsets = [
         # created_on is the created_at field, and portfolio_type is f"{organization_type} - {federal_type}"
@@ -2940,16 +2949,12 @@ class PortfolioAdmin(ListHeaderAdmin):
         ("Senior official", {"fields": ["senior_official"]}),
     ]
 
-    list_display = ("organization_name", "federal_agency", "creator")
+    list_display = ("organization_name", "organization_type", "federal_type", "creator")
     search_fields = ["organization_name"]
-    search_help_text = "Search by organization name."
+    search_help_text = "Search by portfolio organization."
     readonly_fields = [
         # This is the created_at field
         "created_on",
-        # 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",
@@ -2959,17 +2964,48 @@ class PortfolioAdmin(ListHeaderAdmin):
         "creator",
     ]
 
+    analyst_readonly_fields = [
+        "organization_name",
+        "organization_type",
+    ]
+
+    def get_readonly_fields(self, request, obj=None):
+        """Set the read-only state on form elements.
+        We have 2 conditions that determine which fields are read-only:
+        admin user permissions and the creator's status, so
+        we'll use the baseline readonly_fields and extend it as needed.
+        """
+        readonly_fields = list(self.readonly_fields)
+
+        # Check if the creator is restricted
+        if obj and obj.creator.status == models.User.RESTRICTED:
+            # For fields like CharField, IntegerField, etc., the widget used is
+            # straightforward and the readonly_fields list can control their behavior
+            readonly_fields.extend([field.name for field in self.model._meta.fields])
+
+        if request.user.has_perm("registrar.full_access_permission"):
+            return readonly_fields
+
+        # Return restrictive Read-only fields for analysts and
+        # users who might not belong to groups
+        readonly_fields.extend([field for field in self.analyst_readonly_fields])
+        return readonly_fields
+
     def get_admin_users(self, obj):
         # Filter UserPortfolioPermission objects related to the portfolio
-        admin_permissions = UserPortfolioPermission.objects.filter(
+        admin_permissions = self.get_user_portfolio_permission_admins(obj)
-            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_user_portfolio_permission_admins(self, obj):
+        """Returns each admin on UserPortfolioPermission for a given portfolio."""
+        return obj.portfolio_users.filter(
+            portfolio=obj, roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
     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(
@@ -2981,6 +3017,13 @@ class PortfolioAdmin(ListHeaderAdmin):
 
         return non_admin_users
     
+    def get_user_portfolio_permission_non_admins(self, obj):
+        """Returns each admin on UserPortfolioPermission for a given portfolio."""
+        return obj.portfolio_users.exclude(
+            roles__contains=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]
+        )
+
+
     def display_admins(self, obj):
         """Get joined users who are Admin, unpack and return an HTML block.
 
@@ -2989,19 +3032,23 @@ class PortfolioAdmin(ListHeaderAdmin):
         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)
+        admins = self.get_user_portfolio_permission_admins(obj)
         if not admins:
             return format_html("<p>No admins found.</p>")
 
         admin_details = ""
-        for portfolio_admin in admins:
+        for i, portfolio_admin in enumerate(admins):
-            change_url = reverse("admin:registrar_user_change", args=[portfolio_admin.pk])
+            change_url = reverse("admin:registrar_userportfoliopermission_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>'
+            address_id = f"portfolio-administrator-{portfolio_admin.pk}"
-            admin_details += f"{escape(portfolio_admin.title)}<br>"
+            if len(admins) > 1:
-            admin_details += f"{escape(portfolio_admin.email)}"
+                admin_details += f'<label for="{address_id}">Organization admin {i}</label>'
+            admin_details += f'<address id="{address_id}" class="margin-bottom-2 dja-address-contact-list">'
+            admin_details += f'<a href="{change_url}">{escape(portfolio_admin.user)}</a><br>'
+            admin_details += f"{escape(portfolio_admin.user.title)}<br>"
+            admin_details += f"{escape(portfolio_admin.user.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 += f"<input aria-hidden='true' class='display-none' value='{escape(portfolio_admin.user.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'>"
@@ -3012,7 +3059,7 @@ class PortfolioAdmin(ListHeaderAdmin):
             admin_details += "Copy"
             admin_details += "</button>"
             admin_details += "</div><br>"
-            admin_details += f"{escape(portfolio_admin.phone)}"
+            admin_details += f"{escape(portfolio_admin.user.phone)}"
             admin_details += "</address>"
         return format_html(admin_details)
 
@@ -3026,7 +3073,7 @@ class PortfolioAdmin(ListHeaderAdmin):
         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)
+        members = self.get_user_portfolio_permission_non_admins(obj)
         if not members:
             return ""
 
@@ -3035,14 +3082,14 @@ class PortfolioAdmin(ListHeaderAdmin):
             + "<th>Phone</th><th>Roles</th></tr></thead><tbody>"
         )
         for member in members:
-            full_name = member.get_formatted_name()
+            full_name = member.user.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.user.title)}</td>"
-            member_details += f"<td>{escape(member.email)}</td>"
+            member_details += f"<td>{escape(member.user.email)}</td>"
-            member_details += f"<td>{escape(member.phone)}</td>"
+            member_details += f"<td>{escape(member.user.phone)}</td>"
             member_details += "<td>"
-            for role in member.portfolio_role_summary(obj):
+            for role in member.user.portfolio_role_summary(obj):
                 member_details += f"<span class='usa-tag'>{escape(role)}</span> "
             member_details += "</td></tr>"
         member_details += "</tbody></table>"
@@ -3052,11 +3099,11 @@ class PortfolioAdmin(ListHeaderAdmin):
 
     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)
+        members = self.get_user_portfolio_permission_non_admins(obj)
         if not members:
             return {}
 
-        return self.get_field_links_as_list(members, "user", separator=", ")
+        return self.get_field_links_as_list(members, "userportfoliopermission", attribute_name="user", separator=", ")
 
     def federal_type(self, obj: models.Portfolio):
         """Returns the federal_type field"""

src/registrar/assets/js/get-gov-admin.js

@@ -858,10 +858,11 @@ function initializeWidgetOnList(list, parentId) {
         // $ symbolically denotes that this is using jQuery
         let $federalAgency = django.jQuery("#id_federal_agency");
         let organizationType = document.getElementById("id_organization_type");
-        if ($federalAgency && organizationType) {
+        let federalType = document.getElementById("id_federal_type")
+        if ($federalAgency && organizationType && federalType) {
             // Attach the change event listener
             $federalAgency.on("change", function() {
-                handleFederalAgencyChange($federalAgency, organizationType);
+                handleFederalAgencyChange($federalAgency, organizationType, federalType);
             });
         }
         
@@ -879,7 +880,7 @@ function initializeWidgetOnList(list, parentId) {
         }
     });
 
-    function handleFederalAgencyChange(federalAgency, organizationType) {
+    function handleFederalAgencyChange(federalAgency, organizationType, federalType) {
         // Don't do anything on page load
         if (isInitialPageLoad) {
             isInitialPageLoad = false;
@@ -924,7 +925,11 @@ function initializeWidgetOnList(list, parentId) {
                 console.error("Error in AJAX call: " + data.error);
                 return;
             }
-            updateReadOnly(data.federal_type, '.field-federal_type');
+            if (data.federal_type && selectedText !== "Non-Federal Agency") {
+                federalType.value = data.federal_type.toLowerCase();
+            }else {
+                federalType.value = "";
+            }
             updateReadOnly(data.portfolio_type, '.field-portfolio_type');
         })
         .catch(error => console.error("Error fetching federal and portfolio types: ", error));

src/registrar/migrations/0129_portfolio_federal_type.py

@@ -0,0 +1,24 @@
+# Generated by Django 4.2.10 on 2024-09-23 15:29
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("registrar", "0128_create_groups_v17"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="portfolio",
+            name="federal_type",
+            field=models.CharField(
+                blank=True,
+                choices=[("executive", "Executive"), ("judicial", "Judicial"), ("legislative", "Legislative")],
+                help_text="Federal agency type (executive, judicial, legislative, etc.)",
+                max_length=20,
+                null=True,
+            ),
+        ),
+    ]

src/registrar/models/portfolio.py

@@ -58,6 +58,14 @@ class Portfolio(TimeStampedModel):
         default=FederalAgency.get_non_federal_agency,
     )
 
+    federal_type = models.CharField(
+        max_length=20,
+        choices=BranchChoices.choices,
+        null=True,
+        blank=True,
+        help_text="Federal agency type (executive, judicial, legislative, etc.)",
+    )
+
     senior_official = models.ForeignKey(
         "registrar.SeniorOfficial",
         on_delete=models.PROTECT,
@@ -123,8 +131,13 @@ class Portfolio(TimeStampedModel):
         if self.state_territory != self.StateTerritoryChoices.PUERTO_RICO and self.urbanization:
             self.urbanization = None
 
+        # Set the federal type field if it doesn't exist already
+        if self.federal_type is None and self.federal_agency and self.federal_agency.federal_type:
+            self.federal_type = self.federal_agency.federal_type if self.federal_agency else None
+
         super().save(*args, **kwargs)
 
+
     @property
     def portfolio_type(self):
         """
@@ -142,15 +155,6 @@ class Portfolio(TimeStampedModel):
         else:
             return org_type_label
 
-    @property
-    def federal_type(self):
-        """Returns the federal_type value on the underlying federal_agency field"""
-        return self.get_federal_type(self.federal_agency)
-
-    @classmethod
-    def get_federal_type(cls, federal_agency):
-        return federal_agency.federal_type if federal_agency else None
-
     # == Getters for domains == #
     def get_domains(self):
         """Returns all DomainInformations associated with this portfolio"""

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

@@ -137,16 +137,6 @@ 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 %}
@@ -335,13 +325,6 @@ 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/includes/portfolio_fieldset.html

@@ -0,0 +1,35 @@
+{% extends "django/admin/includes/detail_table_fieldset.html" %}
+{% load custom_filters %}
+{% load static url_helpers %}
+
+{% block field_readonly %}
+    {% if 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 %}
+{% endblock field_readonly%}
+
+{% block after_help_text %}
+    {% if field.field.name == "senior_official" %}
+        <div class="flex-container">
+        <label aria-label="Senior official contact details"></label>
+        {% include "django/admin/includes/contact_detail_list.html" with user=original_object.senior_official no_title_top_padding=field.is_readonly %}
+        </div>
+    {% 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>
+    {% endif %}
+{% endblock after_help_text %}

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

@@ -20,7 +20,7 @@
         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 %}
+        {% include "django/admin/includes/portfolio_fieldset.html" with original_object=original %}
     {% endfor %}
 {% endblock %}
 

src/registrar/views/utility/api_views.py

@@ -58,7 +58,7 @@ def get_federal_and_portfolio_types_from_federal_agency_json(request):
     organization_type = request.GET.get("organization_type")
     agency = FederalAgency.objects.filter(agency=agency_name).first()
     if agency:
-        federal_type = Portfolio.get_federal_type(agency)
+        federal_type = agency.federal_type
         portfolio_type = Portfolio.get_portfolio_type(organization_type, federal_type)
         federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-"