From fe5af5010010dc65ada5f64c06fd75949a5dcae1 Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Wed, 31 Jul 2024 21:42:11 -0400
Subject: [PATCH 1/8] initial implementation

---
 src/registrar/config/settings.py              |  1 -
 src/registrar/config/urls.py                  | 19 +++++----
 src/registrar/context_processors.py           | 10 +++--
 src/registrar/registrar_middleware.py         |  4 +-
 src/registrar/templates/domain_detail.html    | 29 +++++++++-----
 src/registrar/templates/domain_sidebar.html   |  2 +-
 .../templates/includes/header_extended.html   | 11 +++---
 src/registrar/templatetags/custom_filters.py  |  5 +++
 src/registrar/views/__init__.py               |  1 +
 src/registrar/views/domain.py                 | 11 ++++++
 src/registrar/views/portfolios.py             | 39 ++++---------------
 src/registrar/views/utility/mixins.py         |  8 +++-
 .../views/utility/permission_views.py         | 19 +++++++++
 13 files changed, 94 insertions(+), 65 deletions(-)

diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py
index 3da0a104a..2f20aeb66 100644
--- a/src/registrar/config/settings.py
+++ b/src/registrar/config/settings.py
@@ -241,7 +241,6 @@ TEMPLATES = [
                 "registrar.context_processors.is_demo_site",
                 "registrar.context_processors.is_production",
                 "registrar.context_processors.org_user_status",
-                "registrar.context_processors.add_portfolio_to_context",
                 "registrar.context_processors.add_path_to_context",
                 "registrar.context_processors.add_has_profile_feature_flag_to_context",
                 "registrar.context_processors.portfolio_permissions",
diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py
index b91e0980c..651346b10 100644
--- a/src/registrar/config/urls.py
+++ b/src/registrar/config/urls.py
@@ -25,7 +25,6 @@ from registrar.views.domain_request import Step
 from registrar.views.domain_requests_json import get_domain_requests_json
 from registrar.views.domains_json import get_domains_json
 from registrar.views.utility import always_404
-from registrar.views.portfolios import PortfolioDomainsView, PortfolioDomainRequestsView, PortfolioOrganizationView
 from api.views import available, get_current_federal, get_current_full
 
 
@@ -60,19 +59,19 @@ for step, view in [
 urlpatterns = [
     path("", views.index, name="home"),
     path(
-        "portfolio/<int:portfolio_id>/domains/",
-        PortfolioDomainsView.as_view(),
-        name="portfolio-domains",
+        "domains/",
+        views.PortfolioDomainsView.as_view(),
+        name="domains",
     ),
     path(
-        "portfolio/<int:portfolio_id>/domain_requests/",
-        PortfolioDomainRequestsView.as_view(),
-        name="portfolio-domain-requests",
+        "requests/",
+        views.PortfolioDomainRequestsView.as_view(),
+        name="domain-requests",
     ),
     path(
-        "portfolio/<int:portfolio_id>/organization/",
-        PortfolioOrganizationView.as_view(),
-        name="portfolio-organization",
+        "organization/",
+        views.PortfolioOrganizationView.as_view(),
+        name="organization",
     ),
     path(
         "admin/logout/",
diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py
index 06ef07050..861a4e701 100644
--- a/src/registrar/context_processors.py
+++ b/src/registrar/context_processors.py
@@ -50,10 +50,6 @@ def org_user_status(request):
     }
 
 
-def add_portfolio_to_context(request):
-    return {"portfolio": getattr(request, "portfolio", None)}
-
-
 def add_path_to_context(request):
     return {"path": getattr(request, "path", None)}
 
@@ -70,11 +66,15 @@ def portfolio_permissions(request):
                 "has_base_portfolio_permission": False,
                 "has_domains_portfolio_permission": False,
                 "has_domain_requests_portfolio_permission": False,
+                "portfolio": None,
+                "has_organization_feature_flag": False,
             }
         return {
             "has_base_portfolio_permission": request.user.has_base_portfolio_permission(),
             "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(),
             "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(),
+            "portfolio": request.user.portfolio,
+            "has_organization_feature_flag": flag_is_active(request, "organization_feature"),
         }
     except AttributeError:
         # Handles cases where request.user might not exist
@@ -82,4 +82,6 @@ def portfolio_permissions(request):
             "has_base_portfolio_permission": False,
             "has_domains_portfolio_permission": False,
             "has_domain_requests_portfolio_permission": False,
+            "portfolio": None,
+            "has_organization_feature_flag": False,
         }
diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py
index dd9b5541a..2af331bc9 100644
--- a/src/registrar/registrar_middleware.py
+++ b/src/registrar/registrar_middleware.py
@@ -149,10 +149,10 @@ class CheckPortfolioMiddleware:
                 request.portfolio = portfolio
 
                 if request.user.has_domains_portfolio_permission():
-                    portfolio_redirect = reverse("portfolio-domains", kwargs={"portfolio_id": portfolio.id})
+                    portfolio_redirect = reverse("domains")
                 else:
                     # View organization is the lowest access
-                    portfolio_redirect = reverse("portfolio-organization", kwargs={"portfolio_id": portfolio.id})
+                    portfolio_redirect = reverse("organization")
 
                 return HttpResponseRedirect(portfolio_redirect)
 
diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html
index 08137c094..414b28a22 100644
--- a/src/registrar/templates/domain_detail.html
+++ b/src/registrar/templates/domain_detail.html
@@ -40,39 +40,50 @@
 
     {% include "includes/domain_dates.html" %}
 
+    {% if is_portfolio_user and not is_domain_manager %}
+    <div class="usa-alert usa-alert--info usa-alert--slim">
+      <div class="usa-alert__body">
+        <p class="usa-alert__text ">
+          To manage information for this domain, you must add yourself as a domain manager.
+        </p>
+      </div>
+    </div>
+    {% endif %}
+
+
     {% url 'domain-dns-nameservers' pk=domain.id as url %}
     {% if domain.nameservers|length > 0 %}
-      {% include "includes/summary_item.html" with title='DNS name servers' domains='true' value=domain.nameservers list='true' edit_link=url editable=domain.is_editable %}
+      {% include "includes/summary_item.html" with title='DNS name servers' domains='true' value=domain.nameservers list='true' edit_link=url editable=is_editable %}
     {% else %}
-      {% if domain.is_editable %}
+      {% if is_editable %}
       <h2 class="margin-top-3"> DNS name servers </h2>
       <p> No DNS name servers have been added yet. Before your domain can be used we’ll need information about your domain name servers.</p>
       <a class="usa-button margin-bottom-1" href="{{url}}"> Add DNS name servers </a>
       {% else %}
-        {% include "includes/summary_item.html" with title='DNS name servers' domains='true' value='' edit_link=url editable=domain.is_editable %}
+        {% include "includes/summary_item.html" with title='DNS name servers' domains='true' value='' edit_link=url editable=is_editable %}
       {% endif %}
     {% endif %}
 
     {% url 'domain-org-name-address' pk=domain.id as url %}
-    {% include "includes/summary_item.html" with title='Organization name and mailing address' value=domain.domain_info address='true' edit_link=url editable=domain.is_editable %}
+    {% include "includes/summary_item.html" with title='Organization name and mailing address' value=domain.domain_info address='true' edit_link=url editable=is_editable %}
 
     {% url 'domain-senior-official' pk=domain.id as url %}
-    {% include "includes/summary_item.html" with title='Senior official' value=domain.domain_info.senior_official contact='true' edit_link=url editable=domain.is_editable %}
+    {% include "includes/summary_item.html" with title='Senior official' value=domain.domain_info.senior_official contact='true' edit_link=url editable=is_editable %}
 
     {# Conditionally display profile #}
     {% if not has_profile_feature_flag %}
     {% url 'domain-your-contact-information' pk=domain.id as url %}
-    {% include "includes/summary_item.html" with title='Your contact information' value=request.user contact='true' edit_link=url editable=domain.is_editable %}
+    {% include "includes/summary_item.html" with title='Your contact information' value=request.user contact='true' edit_link=url editable=is_editable %}
     {% endif %}
 
     {% url 'domain-security-email' pk=domain.id as url %}
     {% if security_email is not None and security_email not in hidden_security_emails%}
-      {% include "includes/summary_item.html" with title='Security email' value=security_email edit_link=url editable=domain.is_editable %}
+      {% include "includes/summary_item.html" with title='Security email' value=security_email edit_link=url editable=is_editable %}
     {% else %}
-      {% include "includes/summary_item.html" with title='Security email' value='None provided' edit_link=url editable=domain.is_editable %}
+      {% include "includes/summary_item.html" with title='Security email' value='None provided' edit_link=url editable=is_editable %}
     {% endif %}
     {% url 'domain-users' pk=domain.id as url %}
-    {% include "includes/summary_item.html" with title='Domain managers' users='true' list=True value=domain.permissions.all edit_link=url editable=domain.is_editable %}
+    {% include "includes/summary_item.html" with title='Domain managers' users='true' list=True value=domain.permissions.all edit_link=url editable=is_editable %}
 
   </div>
 {% endblock %}  {# domain_content #}
diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html
index d61e5f45c..603822d0d 100644
--- a/src/registrar/templates/domain_sidebar.html
+++ b/src/registrar/templates/domain_sidebar.html
@@ -12,7 +12,7 @@
         </a>
       </li>
 
-      {% if domain.is_editable %}
+      {% if is_editable %}
       <li class="usa-sidenav__item">
         {% url 'domain-dns' pk=domain.id as url %}
         <a href="{{ url }}" {% if request.path|startswith:url %}class="usa-current"{% endif %}>
diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html
index c5fc02ebb..c516b9744 100644
--- a/src/registrar/templates/includes/header_extended.html
+++ b/src/registrar/templates/includes/header_extended.html
@@ -1,4 +1,5 @@
 {% load static %}
+{% load custom_filters %}
 
 <header class="usa-header usa-header--extended">
     <div class="usa-navbar">
@@ -14,8 +15,8 @@
         <ul class="usa-nav__primary usa-accordion">
             {% if has_domains_portfolio_permission %}
                 <li class="usa-nav__primary-item">
-                    {% url 'portfolio-domains' portfolio.id as url %}
-                    <a href="{{ url }}" class="usa-nav-link{% if request.path == url %} usa-current{% endif %}"> 
+                    {% url 'domains' as url %}
+                    <a href="{{ url }}" class="usa-nav-link{% if 'domain'|in_path:request.path %} usa-current{% endif %}"> 
                     Domains
                     </a>
                 </li>
@@ -27,8 +28,8 @@
             </li>
             {% if has_domain_requests_portfolio_permission %}
                 <li class="usa-nav__primary-item">
-                    {% url 'portfolio-domain-requests' portfolio.id as url %}
-                    <a href="{{ url }}" class="usa-nav-link{% if request.path == url %} usa-current{% endif %}"> 
+                    {% url 'domain-requests' as url %}
+                    <a href="{{ url }}" class="usa-nav-link{% if 'request'|in_path:request.path %} usa-current{% endif %}"> 
                     Domain requests
                     </a>
                 </li>
@@ -39,7 +40,7 @@
                 </a>
             </li>
             <li class="usa-nav__primary-item">
-                {% url 'portfolio-organization' portfolio.id as url %}
+                {% url 'organization' as url %}
                 <!-- Move the padding from the a to the span so that the descenders do not get cut off -->
                 <a href="{{ url }}" class="usa-nav-link padding-y-0">
                     <span class="ellipsis ellipsis--23 ellipsis--desktop-50 padding-y-1 desktop:padding-y-2">
diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py
index 62afa1acb..8338eaf9d 100644
--- a/src/registrar/templatetags/custom_filters.py
+++ b/src/registrar/templatetags/custom_filters.py
@@ -145,3 +145,8 @@ def format_phone(value):
             phone_number = PhoneNumber.from_string(value)
         return phone_number.as_national
     return value
+
+
+@register.filter
+def in_path(url, path):
+    return url in path
diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py
index 37977f334..a00cc1d73 100644
--- a/src/registrar/views/__init__.py
+++ b/src/registrar/views/__init__.py
@@ -17,3 +17,4 @@ from .domain import (
 from .user_profile import UserProfileView, FinishProfileSetupView
 from .health import *
 from .index import *
+from .portfolios import *
diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py
index 766bef7c2..da34ce646 100644
--- a/src/registrar/views/domain.py
+++ b/src/registrar/views/domain.py
@@ -170,6 +170,17 @@ class DomainView(DomainBaseView):
         context["security_email"] = security_email
         return context
 
+    def can_access_domain_via_portfolio(self, pk):
+        """Most views should not allow permission to portfolio users.
+        If particular views allow permissions, they will need to override
+        this function."""
+        if self.request.user.has_domains_portfolio_permission():
+            if Domain.objects.filter(id=pk).exists():
+                domain = Domain.objects.get(id=pk)
+                if domain.domain_info.portfolio == self.request.user.portfolio:
+                    return True
+        return False
+
     def in_editable_state(self, pk):
         """Override in_editable_state from DomainPermission
         Allow detail page to be viewable"""
diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py
index f693e2cc9..4af06cbe8 100644
--- a/src/registrar/views/portfolios.py
+++ b/src/registrar/views/portfolios.py
@@ -1,11 +1,9 @@
-from django.shortcuts import get_object_or_404, render
-from registrar.models.portfolio import Portfolio
+from django.shortcuts import render
 from registrar.views.utility.permission_views import (
     PortfolioDomainRequestsPermissionView,
     PortfolioDomainsPermissionView,
     PortfolioBasePermissionView,
 )
-from waffle.decorators import flag_is_active
 from django.views.generic import View
 
 
@@ -13,46 +11,23 @@ class PortfolioDomainsView(PortfolioDomainsPermissionView, View):
 
     template_name = "portfolio_domains.html"
 
-    def get(self, request, portfolio_id):
-        context = {}
-
-        if self.request.user.is_authenticated:
-            context["has_profile_feature_flag"] = flag_is_active(request, "profile_feature")
-            context["has_organization_feature_flag"] = flag_is_active(request, "organization_feature")
-            portfolio = get_object_or_404(Portfolio, id=portfolio_id)
-            context["portfolio"] = portfolio
-
-        return render(request, "portfolio_domains.html", context)
+    def get(self, request):
+        return render(request, "portfolio_domains.html")
 
 
 class PortfolioDomainRequestsView(PortfolioDomainRequestsPermissionView, View):
 
     template_name = "portfolio_requests.html"
 
-    def get(self, request, portfolio_id):
-        context = {}
-
+    def get(self, request):
         if self.request.user.is_authenticated:
-            context["has_profile_feature_flag"] = flag_is_active(request, "profile_feature")
-            context["has_organization_feature_flag"] = flag_is_active(request, "organization_feature")
-            portfolio = get_object_or_404(Portfolio, id=portfolio_id)
-            context["portfolio"] = portfolio
             request.session["new_request"] = True
-
-        return render(request, "portfolio_requests.html", context)
+        return render(request, "portfolio_requests.html")
 
 
 class PortfolioOrganizationView(PortfolioBasePermissionView, View):
 
     template_name = "portfolio_organization.html"
 
-    def get(self, request, portfolio_id):
-        context = {}
-
-        if self.request.user.is_authenticated:
-            context["has_profile_feature_flag"] = flag_is_active(request, "profile_feature")
-            context["has_organization_feature_flag"] = flag_is_active(request, "organization_feature")
-            portfolio = get_object_or_404(Portfolio, id=portfolio_id)
-            context["portfolio"] = portfolio
-
-        return render(request, "portfolio_organization.html", context)
+    def get(self, request):
+        return render(request, "portfolio_organization.html")
diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py
index 5f8485ec5..5b8f81087 100644
--- a/src/registrar/views/utility/mixins.py
+++ b/src/registrar/views/utility/mixins.py
@@ -184,11 +184,17 @@ class DomainPermission(PermissionsLoginMixin):
 
         # user needs to have a role on the domain
         if not UserDomainRole.objects.filter(user=self.request.user, domain__id=pk).exists():
-            return False
+            return self.can_access_domain_via_portfolio(pk)
 
         # if we need to check more about the nature of role, do it here.
         return True
 
+    def can_access_domain_via_portfolio(self, pk):
+        """Most views should not allow permission to portfolio users.
+        If particular views allow permissions, they will need to override
+        this function."""
+        return False
+
     def in_editable_state(self, pk):
         """Is the domain in an editable state"""
 
diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py
index 181934787..501658e9f 100644
--- a/src/registrar/views/utility/permission_views.py
+++ b/src/registrar/views/utility/permission_views.py
@@ -43,6 +43,9 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
         context["is_analyst_or_superuser"] = user.has_perm("registrar.analyst_access_permission") or user.has_perm(
             "registrar.full_access_permission"
         )
+        context["is_domain_manager"] = UserDomainRole.objects.filter(user=user, domain=self.object).exists()
+        context["is_portfolio_user"] = self.can_access_domain_via_portfolio(self.object.pk)
+        context["is_editable"] = self.is_editable()
         # Stored in a variable for the linter
         action = "analyst_action"
         action_location = "analyst_action_location"
@@ -54,6 +57,22 @@ class DomainPermissionView(DomainPermission, DetailView, abc.ABC):
 
         return context
 
+    def is_editable(self):
+        """Returns whether domain is editable in the context of the view"""
+        logger.info("checking if is_editable")
+        domain_editable = self.object.is_editable()
+        if not domain_editable:
+            return False
+
+        # if user is domain manager or analyst or admin, return True
+        if (
+            self.can_access_other_user_domains(self.object.id)
+            or UserDomainRole.objects.filter(user=self.request.user, domain=self.object).exists()
+        ):
+            return True
+
+        return False
+
     # Abstract property enforces NotImplementedError on an attribute.
     @property
     @abc.abstractmethod

From 7860d8c58727569ed176a4d0852a4003e3810c2a Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Wed, 31 Jul 2024 22:25:59 -0400
Subject: [PATCH 2/8] updated existing tests

---
 src/registrar/tests/test_views_portfolio.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py
index 3596bf567..ebda0bcf3 100644
--- a/src/registrar/tests/test_views_portfolio.py
+++ b/src/registrar/tests/test_views_portfolio.py
@@ -112,7 +112,7 @@ class TestPortfolio(WebTest):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
             response = self.app.get(
-                reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}), status=403
+                reverse("domains"), status=403
             )
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
@@ -128,7 +128,7 @@ class TestPortfolio(WebTest):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
             response = self.app.get(
-                reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}), status=403
+                reverse("domain-requests"), status=403
             )
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
@@ -144,7 +144,7 @@ class TestPortfolio(WebTest):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
             response = self.app.get(
-                reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}), status=403
+                reverse("organization"), status=403
             )
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
@@ -170,10 +170,10 @@ class TestPortfolio(WebTest):
             self.assertNotContains(portfolio_page, "<h1>Organization</h1>")
             self.assertContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
             self.assertContains(
-                portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk})
+                portfolio_page, reverse("domains")
             )
             self.assertContains(
-                portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk})
+                portfolio_page, reverse("domain-requests")
             )
 
             # reducing portfolio permissions to just VIEW_PORTFOLIO, which should remove domains
@@ -188,10 +188,10 @@ class TestPortfolio(WebTest):
             self.assertContains(portfolio_page, "<h1>Organization</h1>")
             self.assertNotContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
             self.assertNotContains(
-                portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk})
+                portfolio_page, reverse("domains")
             )
             self.assertNotContains(
-                portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk})
+                portfolio_page, reverse("domain-requests")
             )
 
 
@@ -209,7 +209,7 @@ class TestPortfolioOrganization(TestPortfolio):
             self.user.save()
             self.user.refresh_from_db()
 
-            page = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}))
+            page = self.app.get(reverse("organization"))
             self.assertContains(
                 page, "The name of your federal agency will be publicly listed as the domain registrant."
             )
@@ -228,7 +228,7 @@ class TestPortfolioOrganization(TestPortfolio):
 
             self.portfolio.organization_name = "Hotel California"
             self.portfolio.save()
-            page = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}))
+            page = self.app.get(reverse("organization"))
             # Once in the sidenav, once in the main nav, once in the form
             self.assertContains(page, "Hotel California", count=3)
 
@@ -247,7 +247,7 @@ class TestPortfolioOrganization(TestPortfolio):
             self.portfolio.address_line1 = "1600 Penn Ave"
             self.portfolio.save()
             portfolio_org_name_page = self.app.get(
-                reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})
+                reverse("organization")
             )
             session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
 

From 978ea903e708692c0b71fc777b3dff9c0c4a8213 Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Wed, 31 Jul 2024 22:40:35 -0400
Subject: [PATCH 3/8] formatted for code readability

---
 src/registrar/tests/test_views_portfolio.py | 32 ++++++---------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py
index ebda0bcf3..09f34efcf 100644
--- a/src/registrar/tests/test_views_portfolio.py
+++ b/src/registrar/tests/test_views_portfolio.py
@@ -111,9 +111,7 @@ class TestPortfolio(WebTest):
         with override_flag("organization_feature", active=True):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
-            response = self.app.get(
-                reverse("domains"), status=403
-            )
+            response = self.app.get(reverse("domains"), status=403)
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
 
@@ -127,9 +125,7 @@ class TestPortfolio(WebTest):
         with override_flag("organization_feature", active=True):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
-            response = self.app.get(
-                reverse("domain-requests"), status=403
-            )
+            response = self.app.get(reverse("domain-requests"), status=403)
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
 
@@ -143,9 +139,7 @@ class TestPortfolio(WebTest):
         with override_flag("organization_feature", active=True):
             # This will redirect the user to the portfolio page.
             # Follow implicity checks if our redirect is working.
-            response = self.app.get(
-                reverse("organization"), status=403
-            )
+            response = self.app.get(reverse("organization"), status=403)
             # Assert the response is a 403 Forbidden
             self.assertEqual(response.status_code, 403)
 
@@ -169,12 +163,8 @@ class TestPortfolio(WebTest):
             self.assertContains(portfolio_page, self.portfolio.organization_name)
             self.assertNotContains(portfolio_page, "<h1>Organization</h1>")
             self.assertContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
-            self.assertContains(
-                portfolio_page, reverse("domains")
-            )
-            self.assertContains(
-                portfolio_page, reverse("domain-requests")
-            )
+            self.assertContains(portfolio_page, reverse("domains"))
+            self.assertContains(portfolio_page, reverse("domain-requests"))
 
             # reducing portfolio permissions to just VIEW_PORTFOLIO, which should remove domains
             # and domain requests from nav
@@ -187,12 +177,8 @@ class TestPortfolio(WebTest):
             self.assertContains(portfolio_page, self.portfolio.organization_name)
             self.assertContains(portfolio_page, "<h1>Organization</h1>")
             self.assertNotContains(portfolio_page, '<h1 id="domains-header">Domains</h1>')
-            self.assertNotContains(
-                portfolio_page, reverse("domains")
-            )
-            self.assertNotContains(
-                portfolio_page, reverse("domain-requests")
-            )
+            self.assertNotContains(portfolio_page, reverse("domains"))
+            self.assertNotContains(portfolio_page, reverse("domain-requests"))
 
 
 class TestPortfolioOrganization(TestPortfolio):
@@ -246,9 +232,7 @@ class TestPortfolioOrganization(TestPortfolio):
 
             self.portfolio.address_line1 = "1600 Penn Ave"
             self.portfolio.save()
-            portfolio_org_name_page = self.app.get(
-                reverse("organization")
-            )
+            portfolio_org_name_page = self.app.get(reverse("organization"))
             session_id = self.app.cookies[settings.SESSION_COOKIE_NAME]
 
             portfolio_org_name_page.form["address_line1"] = "6 Downing st"

From 1ccd59a0aa02fbb99c9206d757f5e6b7d1da71ae Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Wed, 31 Jul 2024 22:46:26 -0400
Subject: [PATCH 4/8] formatted for readability

---
 src/registrar/views/portfolios.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py
index 3d6a41fbb..bec634715 100644
--- a/src/registrar/views/portfolios.py
+++ b/src/registrar/views/portfolios.py
@@ -11,7 +11,6 @@ from registrar.views.utility.permission_views import (
 )
 from django.views.generic import View
 from django.views.generic.edit import FormMixin
-from waffle.decorators import flag_is_active
 
 
 logger = logging.getLogger(__name__)

From 7f112c12956a0df3c7feefb366955366252453c0 Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Thu, 1 Aug 2024 09:48:51 -0400
Subject: [PATCH 5/8] test functionality

---
 src/registrar/tests/test_views_domain.py | 29 ++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py
index 8aca75682..e24085b30 100644
--- a/src/registrar/tests/test_views_domain.py
+++ b/src/registrar/tests/test_views_domain.py
@@ -6,6 +6,7 @@ from django.urls import reverse
 from django.contrib.auth import get_user_model
 
 from api.tests.common import less_console_noise_decorator
+from registrar.models.portfolio import Portfolio
 from .common import MockEppLib, MockSESClient, create_user  # type: ignore
 from django_webtest import WebTest  # type: ignore
 import boto3_mocking  # type: ignore
@@ -138,6 +139,7 @@ class TestWithDomainPermissions(TestWithUser):
             Host.objects.all().delete()
             Domain.objects.all().delete()
             UserDomainRole.objects.all().delete()
+            Portfolio.objects.all().delete()
         except ValueError:  # pass if already deleted
             pass
         super().tearDown()
@@ -310,6 +312,33 @@ class TestDomainDetail(TestDomainOverview):
             self.assertContains(detail_page, "noinformation.gov")
             self.assertContains(detail_page, "Domain missing domain information")
 
+    @less_console_noise_decorator
+    def test_domain_readonly_on_detail_page(self):
+        """Test that a domain, which is part of a portfolio, but for which the user is not a domain manager,
+        properly displays read only"""
+
+        portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test org", creator=self.user)
+        # need to create a different user than self.user because the user needs permission assignments
+        user = get_user_model().objects.create(
+            first_name="Test",
+            last_name="User",
+            email="bogus@example.gov",
+            phone="8003111234",
+            title="test title",
+            portfolio=portfolio,
+            portfolio_roles=[User.UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+        )
+        domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov")
+        DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio)
+        self.client.force_login(user)
+        detail_page = self.client.get(f"/domain/{domain.id}")
+        # Check that alert message displays properly
+        self.assertContains(
+            detail_page, "To manage information for this domain, you must add yourself as a domain manager."
+        )
+        # Check that user does not have option to Edit domain
+        self.assertNotContains(detail_page, "Edit")
+
 
 class TestDomainManagers(TestDomainOverview):
     def tearDown(self):

From 1dfe34d9c6a370cd3e66cb1e77f272109e67fe7a Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Thu, 1 Aug 2024 18:09:54 -0400
Subject: [PATCH 6/8] fixed view icon

---
 src/registrar/views/domains_json.py   | 10 +++-------
 src/registrar/views/utility/mixins.py |  2 +-
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py
index d4c09d808..5bb9b037f 100644
--- a/src/registrar/views/domains_json.py
+++ b/src/registrar/views/domains_json.py
@@ -124,7 +124,7 @@ def serialize_domain(domain, user):
 
     # Check if there is a UserDomainRole for this domain and user
     user_domain_role_exists = UserDomainRole.objects.filter(domain_id=domain.id, user=user).exists()
-
+    view_only = not user_domain_role_exists or domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD]
     return {
         "id": domain.id,
         "name": domain.name,
@@ -133,11 +133,7 @@ def serialize_domain(domain, user):
         "state_display": domain.state_display(),
         "get_state_help_text": domain.get_state_help_text(),
         "action_url": reverse("domain", kwargs={"pk": domain.id}),
-        "action_label": (
-            "View"
-            if not user_domain_role_exists or domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD]
-            else "Manage"
-        ),
-        "svg_icon": ("visibility" if domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD] else "settings"),
+        "action_label": ("View" if view_only else "Manage"),
+        "svg_icon": ("visibility" if view_only else "settings"),
         "suborganization": suborganization_name,
     }
diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py
index 5b8f81087..16a896100 100644
--- a/src/registrar/views/utility/mixins.py
+++ b/src/registrar/views/utility/mixins.py
@@ -191,7 +191,7 @@ class DomainPermission(PermissionsLoginMixin):
 
     def can_access_domain_via_portfolio(self, pk):
         """Most views should not allow permission to portfolio users.
-        If particular views allow permissions, they will need to override
+        If particular views allow access to the domain pages, they will need to override
         this function."""
         return False
 

From e614a0e6be27d60a770e2161bd7abf6e5b263a9f Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Thu, 1 Aug 2024 18:41:43 -0400
Subject: [PATCH 7/8] improved handling of edge case where no portfolio defined

---
 src/registrar/views/portfolios.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py
index bec634715..4b7374d81 100644
--- a/src/registrar/views/portfolios.py
+++ b/src/registrar/views/portfolios.py
@@ -1,5 +1,6 @@
 import logging
-from django.shortcuts import get_object_or_404, render
+from django.http import Http404
+from django.shortcuts import render
 from django.urls import reverse
 from django.contrib import messages
 from registrar.forms.portfolio import PortfolioOrgAddressForm
@@ -50,8 +51,11 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin):
         return context
 
     def get_object(self, queryset=None):
-        """Get the portfolio object based on the URL parameter."""
-        return get_object_or_404(Portfolio, id=self.request.user.portfolio.id)
+        """Get the portfolio object based on the request user."""
+        portfolio = self.request.user.portfolio
+        if portfolio is None:
+            raise Http404("No organization found for this user")
+        return portfolio
 
     def get_form_kwargs(self):
         """Include the instance in the form kwargs."""

From e0392920b8aba0abac84d307d9433b39c13b1ecf Mon Sep 17 00:00:00 2001
From: David Kennedy <david.kennedy@associates.cisa.dhs.gov>
Date: Thu, 1 Aug 2024 18:54:41 -0400
Subject: [PATCH 8/8] fixed a test for view icon

---
 src/registrar/tests/test_views_domains_json.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py
index 09a233768..70ae23b43 100644
--- a/src/registrar/tests/test_views_domains_json.py
+++ b/src/registrar/tests/test_views_domains_json.py
@@ -181,7 +181,8 @@ class GetDomainsJsonTest(TestWithUser, WebTest):
             # Check svg_icon
             svg_icon_expected = (
                 "visibility"
-                if expected_domains[i].state
+                if not user_domain_role_exists
+                or expected_domains[i].state
                 in [
                     Domain.State.DELETED,
                     Domain.State.ON_HOLD,