From 3513ce1faf7e01c4deb637e5bb755f61f204ce36 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 5 Aug 2024 14:55:32 -0600 Subject: [PATCH 001/101] Add no domains view --- src/registrar/config/urls.py | 5 ++++ .../templates/includes/domains_table.html | 14 ++++----- .../templates/includes/header_extended.html | 17 ++++++----- .../templates/no_portfolio_domains.html | 26 ++++++++++++++++ src/registrar/views/portfolios.py | 30 ++++++++++++++++++- .../views/utility/permission_views.py | 7 +++++ 6 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 src/registrar/templates/no_portfolio_domains.html diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 90137c4af..5e5762f70 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -64,6 +64,11 @@ urlpatterns = [ views.PortfolioDomainsView.as_view(), name="domains", ), + path( + "no-organization-domains/", + views.PortfolioNoDomainsView.as_view(), + name="no-portfolio-domains", + ), path( "requests/", views.PortfolioDomainRequestsView.as_view(), diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 64eddec41..ccf9707e6 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -1,15 +1,15 @@ {% load static %} -
-
- {% if not has_domains_portfolio_permission %} +
+
+ {% if not portfolio %}

Domains

{% else %} {% endif %} -
+
-
+
- {% if has_domains_portfolio_permission %} + {% if portfolio %}
Filter by
@@ -150,7 +150,7 @@ Domain name Expires Status - {% if has_domains_portfolio_permission %} + {% if portfolio %} Suborganization {% endif %}
    - {% if has_domains_portfolio_permission %} -
  • +
  • + {% if has_domains_portfolio_permission %} {% url 'domains' as url %} - - Domains - -
    • - {% endif %} + {%else %} + {% url 'no-portfolio-domains' as url %} + {% endif %} + + Domains + +
  • Domain groups
    • + {% if has_domain_requests_portfolio_permission %}
  • {% url 'domain-requests' as url %} diff --git a/src/registrar/templates/no_portfolio_domains.html b/src/registrar/templates/no_portfolio_domains.html new file mode 100644 index 000000000..e8c96dd98 --- /dev/null +++ b/src/registrar/templates/no_portfolio_domains.html @@ -0,0 +1,26 @@ +{% extends 'portfolio_base.html' %} + +{% load static %} + +{% block title %} Domains | {% endblock %} + +{% block portfolio_content %} +

    Domains

    +
    +
    +

    You aren’t managing any domains.

    +

    If you believe you should have access to a domain, reach out to your organization’s administrators.

    + +

    Your organizations administrators:

    +
      + {% for administrator in portfolio_administrators %} + {% if administrator.email %} +
    • {{ administrator.email }}
      • + {% else %} +
    • {{ administrator }}
      • + {% endif %} + {% endfor %} +
    +
    +
    +{% endblock %} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 63ebbaa01..67823333a 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -4,11 +4,13 @@ from django.shortcuts import render from django.urls import reverse from django.contrib import messages from registrar.forms.portfolio import PortfolioOrgAddressForm -from registrar.models.portfolio import Portfolio +from registrar.models import Portfolio, User +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, PortfolioDomainsPermissionView, PortfolioBasePermissionView, + NoPortfolioDomainsPermissionView, ) from django.views.generic import View from django.views.generic.edit import FormMixin @@ -35,6 +37,32 @@ class PortfolioDomainRequestsView(PortfolioDomainRequestsPermissionView, View): return render(request, "portfolio_requests.html") +class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): + """Some users have access to the underlying portfolio, but not any domains. + This is a custom view which explains that to the user - and denotes who to contact. + """ + + model = Portfolio + template_name = "no_portfolio_domains.html" + + def get(self, request): + return render(request, self.template_name, context=self.get_context_data()) + + def get_context_data(self, **kwargs): + """Add additional context data to the template.""" + # We can override the base class. This view only needs this item. + context = {} + portfolio = self.request.user.portfolio if self.request and self.request.user else None + if portfolio: + context["portfolio_administrators"] = User.objects.filter( + portfolio=portfolio, + portfolio_roles__overlap=[ + UserPortfolioRoleChoices.ORGANIZATION_ADMIN, + UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY + ] + ) + return context + class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """ View to handle displaying and updating the portfolio's organization details. diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 501658e9f..fb7e3717d 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -213,6 +213,13 @@ class PortfolioDomainsPermissionView(PortfolioDomainsPermission, PortfolioBasePe `template_name`. """ +class NoPortfolioDomainsPermissionView(PortfolioBasePermissionView, abc.ABC): + """Abstract base view for a user without access to the + portfolio domains views that enforces permissions. + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, PortfolioBasePermissionView, abc.ABC): """Abstract base view for portfolio domain request views that enforces permissions. From 0d2f98e6f10071ac909bb69eec794ee8fa4703fb Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 8 Aug 2024 14:05:05 -0600 Subject: [PATCH 002/101] Add unit test + lint --- src/registrar/tests/test_views_portfolio.py | 48 +++++++++++++++++++ src/registrar/views/portfolios.py | 5 +- .../views/utility/permission_views.py | 4 +- 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f72130d94..e46e1c5f8 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -325,3 +325,51 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_org_member_can_only_see_domains_with_appropriate_permissions(self): + """A user with the role organization_member should not have access to the domains page + if they do not have the right permissions. + """ + + # A default organization member should not be able to see any domains + self.app.set_user(self.user.username) + self.user.portfolio = self.portfolio + self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + self.user.save() + self.user.refresh_from_db() + + self.assertFalse(self.user.has_domains_portfolio_permission()) + + response = self.app.get(reverse("no-portfolio-domains")) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "You aren’t managing any domains.") + + # Test the domains page - this user should not have access + response = self.app.get(reverse("domains"), expect_errors=True) + self.assertEqual(response.status_code, 403) + + # Ensure that this user can see domains with the right permissions + self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + self.user.save() + self.user.refresh_from_db() + + self.assertTrue(self.user.has_domains_portfolio_permission()) + + # Test the domains page - this user should have access + response = self.app.get(reverse("domains")) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Domain name") + + # Test the managed domains permission + self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + self.user.save() + self.user.refresh_from_db() + + self.assertTrue(self.user.has_domains_portfolio_permission()) + + # Test the domains page - this user should have access + response = self.app.get(reverse("domains")) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Domain name") diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 67823333a..316b2b539 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -58,11 +58,12 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): portfolio=portfolio, portfolio_roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY - ] + UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY, + ], ) return context + class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """ View to handle displaying and updating the portfolio's organization details. diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index fb7e3717d..0ff7d1676 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -213,14 +213,16 @@ class PortfolioDomainsPermissionView(PortfolioDomainsPermission, PortfolioBasePe `template_name`. """ + class NoPortfolioDomainsPermissionView(PortfolioBasePermissionView, abc.ABC): - """Abstract base view for a user without access to the + """Abstract base view for a user without access to the portfolio domains views that enforces permissions. This abstract view cannot be instantiated. Actual views must specify `template_name`. """ + class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, PortfolioBasePermissionView, abc.ABC): """Abstract base view for portfolio domain request views that enforces permissions. From b05b11af214e5db5845c88a0a1d3e657eabc7c0f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 9 Aug 2024 11:57:27 -0600 Subject: [PATCH 003/101] Remove readonly --- 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 316b2b539..cae07ae04 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -58,7 +58,6 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): portfolio=portfolio, portfolio_roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY, ], ) return context From f1f1ea34efd130176b899c53dacea70dc6d02aba Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 13 Aug 2024 11:19:01 -0700 Subject: [PATCH 004/101] Attempt at adding ANDI in via Middleware --- src/registrar/config/settings.py | 1 + src/registrar/registrar_middleware.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 9d707a533..4b9d5c7fe 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -190,6 +190,7 @@ MIDDLEWARE = [ "waffle.middleware.WaffleMiddleware", "registrar.registrar_middleware.CheckUserProfileMiddleware", "registrar.registrar_middleware.CheckPortfolioMiddleware", + "registrar.registrar_middleware.ANDIMiddleware", ] # application object used by Django’s built-in servers (e.g. `runserver`) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 2af331bc9..2fcaa78b4 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -8,6 +8,7 @@ from django.urls import reverse from django.http import HttpResponseRedirect from registrar.models.user import User from waffle.decorators import flag_is_active +from django.utils.deprecation import MiddlewareMixin from registrar.models.utility.generic_helper import replace_url_queryparams @@ -157,3 +158,17 @@ class CheckPortfolioMiddleware: return HttpResponseRedirect(portfolio_redirect) return None + + +class ANDIMiddleware(MiddlewareMixin): + def process_response(self, request, response): + # Check if the response content type is HTML + if "text/html" in response.get("Content-Type", ""): + andi_script = """ + + """ + # Inject the ANDI script before the closing tag + content = response.content.decode("utf-8") + content = content.replace("", f"{andi_script}") + response.content = content.encode("utf-8") + return response From ebbd329c986770b498b0a62ed685e39db8351e8e Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 13 Aug 2024 11:40:45 -0700 Subject: [PATCH 005/101] Update ANDI middleware class --- src/registrar/registrar_middleware.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 2fcaa78b4..fe3c2260d 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -161,8 +161,15 @@ class CheckPortfolioMiddleware: class ANDIMiddleware(MiddlewareMixin): - def process_response(self, request, response): - # Check if the response content type is HTML + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + response = self.get_response(request) + return response + + def process_view(self, request, view_func, view_args, view_kwargs): + response = self.get_response(request) if "text/html" in response.get("Content-Type", ""): andi_script = """ @@ -171,4 +178,4 @@ class ANDIMiddleware(MiddlewareMixin): content = response.content.decode("utf-8") content = content.replace("", f"{andi_script}") response.content = content.encode("utf-8") - return response + return None From 5cd5cd645a962ad66d7c969d79679535f7f78d17 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 13 Aug 2024 12:04:25 -0700 Subject: [PATCH 006/101] Allowing ANDI images --- src/registrar/config/settings.py | 17 ++++++++++++++--- src/registrar/registrar_middleware.py | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 4b9d5c7fe..861ce3a94 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,9 +357,20 @@ CSP_FORM_ACTION = allowed_sources # strict CSP by allowing scripts to run from their domain # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN -CSP_SCRIPT_SRC_ELEM = ["'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js"] -CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/"] -CSP_INCLUDE_NONCE_IN = ["script-src-elem"] +CSP_DEFAULT_SRC = [ + "'self'", +] +CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov", "'unsafe-inline'"] +CSP_SCRIPT_SRC_ELEM = [ + "'self'", + "https://www.googletagmanager.com/", + "https://cdn.jsdelivr.net/npm/chart.js", + "https://www.ssa.gov", + "https://ajax.googleapis.com", +] +CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov"] +CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] +CSP_IMG_SRC = ["'self'", "https://www.ssa.gov"] # Cross-Origin Resource Sharing (CORS) configuration # Sets clients that allow access control to manage.get.gov diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index fe3c2260d..a772fbe0a 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -168,7 +168,7 @@ class ANDIMiddleware(MiddlewareMixin): response = self.get_response(request) return response - def process_view(self, request, view_func, view_args, view_kwargs): + def process_template_view(self, request, view_func, view_args, view_kwargs): response = self.get_response(request) if "text/html" in response.get("Content-Type", ""): andi_script = """ From aea8539d66559f2b636c995a27421bdb33e7d692 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 14 Aug 2024 11:18:47 -0600 Subject: [PATCH 007/101] Initial draft logic for updating action needed email behavior --- src/registrar/admin.py | 2 +- src/registrar/assets/js/get-gov-admin.js | 60 +++++++++++++------ .../admin/includes/detail_table_fieldset.html | 29 ++++++--- 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ca4038d51..2cd391435 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -223,7 +223,7 @@ class DomainRequestAdminForm(forms.ModelForm): "other_contacts": NoAutocompleteFilteredSelectMultiple("other_contacts", False), } labels = { - "action_needed_reason_email": "Auto-generated email", + "action_needed_reason_email": "E-mail", } def __init__(self, *args, **kwargs): diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 04f5417b0..c8fd45bd0 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -353,7 +353,7 @@ function initializeWidgetOnList(list, parentId) { let rejectionReasonFormGroup = document.querySelector('.field-rejection_reason') // This is the "action needed reason" field let actionNeededReasonFormGroup = document.querySelector('.field-action_needed_reason'); - // This is the "auto-generated email" field + // This is the "Email" field let actionNeededReasonEmailFormGroup = document.querySelector('.field-action_needed_reason_email') if (rejectionReasonFormGroup && actionNeededReasonFormGroup && actionNeededReasonEmailFormGroup) { @@ -510,7 +510,10 @@ function initializeWidgetOnList(list, parentId) { // Since this is an iife, these vars will be removed from memory afterwards var actionNeededReasonDropdown = document.querySelector("#id_action_needed_reason"); var actionNeededEmail = document.querySelector("#id_action_needed_reason_email"); - var readonlyView = document.querySelector("#action-needed-reason-email-readonly"); + var helptext = document.querySelector("#action-needed-reason-email-placeholder-text") + var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly"); + var actionNeededEmailReadonlyTextarea = document.querySelector("#action-needed-reason-email-readonly-textarea") + var editEmailButton = document.querySelector("#action-needed-reason-email-edit-button") let emailWasSent = document.getElementById("action-needed-email-sent"); let actionNeededEmailData = document.getElementById('action-needed-emails-data').textContent; @@ -536,6 +539,21 @@ function initializeWidgetOnList(list, parentId) { updateActionNeededEmailDisplay(reason) }); + editEmailButton.addEventListener("click", function() { + let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; + + if (emailHasBeenSentBefore) { + // Show warning Modal + + } + else { + // Show editable view + showElement(actionNeededEmail.parentElement) + hideElement(actionNeededEmailReadonly) + hideElement(helptext) + } + }); + // Add a change listener to the action needed reason dropdown actionNeededReasonDropdown.addEventListener("change", function() { let reason = actionNeededReasonDropdown.value; @@ -543,6 +561,7 @@ function initializeWidgetOnList(list, parentId) { if (reason && emailBody) { // Replace the email content actionNeededEmail.value = emailBody; + actionNeededEmailReadonlyTextarea.value = emailBody; // Reset the session object on change since change refreshes the email content. if (oldDropdownValue !== actionNeededReasonDropdown.value || oldEmailValue !== actionNeededEmail.value) { @@ -562,27 +581,30 @@ function initializeWidgetOnList(list, parentId) { // If the email doesn't exist or if we're of reason "other", display that no email was sent. // Likewise, if we've sent this email before, we should just display the content. function updateActionNeededEmailDisplay(reason) { - let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; - let collapseableDiv = readonlyView.querySelector(".collapse--dgsimple"); - let showMoreButton = document.querySelector("#action_needed_reason_email__show_details"); - if ((reason && reason != "other") && !emailHasBeenSentBefore) { - showElement(actionNeededEmail.parentElement) - hideElement(readonlyView) - hideElement(showMoreButton) - } else { - if (!reason || reason === "other") { - collapseableDiv.innerHTML = reason ? "No email will be sent." : "-"; - hideElement(showMoreButton) - if (collapseableDiv.classList.contains("collapsed")) { - showMoreButton.click() - } - }else { - showElement(showMoreButton) + + console.info("REASON: "+reason) + + if (reason) { + if (reason === "other") { + helptext.innerHTML = "No email will be sent."; + hideElement(actionNeededEmail.parentElement) + hideElement(actionNeededEmailReadonly) + showElement(helptext) } + else { + // Always show readonly view to start + hideElement(actionNeededEmail.parentElement) + showElement(actionNeededEmailReadonly) + hideElement(helptext) + } + } else { + helptext.innerHTML = "Select an action needed reason to see email"; hideElement(actionNeededEmail.parentElement) - showElement(readonlyView) + hideElement(actionNeededEmailReadonly) + showElement(helptext) } } + })(); 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 067b69c07..bcf5fd770 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -145,16 +145,29 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% block field_other %} {% if field.field.name == "action_needed_reason_email" %} -
{% endblock %} From 34c9cd27617c6c3df8306c1b152295db24318f51 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 15 Aug 2024 11:25:22 -0700 Subject: [PATCH 010/101] Convert CSP default src to tuple --- src/registrar/config/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 861ce3a94..e0533943e 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,9 +357,9 @@ CSP_FORM_ACTION = allowed_sources # strict CSP by allowing scripts to run from their domain # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN -CSP_DEFAULT_SRC = [ +CSP_DEFAULT_SRC = ( "'self'", -] +) CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov", "'unsafe-inline'"] CSP_SCRIPT_SRC_ELEM = [ "'self'", From 965724ec1d77b554a6d17b7d7fa1b96f3187de1a Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:02:57 -0700 Subject: [PATCH 011/101] Remove reference to ANDI hardware --- src/registrar/config/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index e0533943e..bc9e09103 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -190,7 +190,7 @@ MIDDLEWARE = [ "waffle.middleware.WaffleMiddleware", "registrar.registrar_middleware.CheckUserProfileMiddleware", "registrar.registrar_middleware.CheckPortfolioMiddleware", - "registrar.registrar_middleware.ANDIMiddleware", + # "registrar.registrar_middleware.ANDIMiddleware", ] # application object used by Django’s built-in servers (e.g. `runserver`) @@ -360,7 +360,7 @@ CSP_FORM_ACTION = allowed_sources CSP_DEFAULT_SRC = ( "'self'", ) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov", "'unsafe-inline'"] +CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov"] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", From cb9ce3792016a5836897d2d7f2a899703748c194 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:03:33 -0700 Subject: [PATCH 012/101] Fix lint errors --- src/registrar/config/settings.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index bc9e09103..4ae0c9fe5 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,9 +357,7 @@ CSP_FORM_ACTION = allowed_sources # strict CSP by allowing scripts to run from their domain # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN -CSP_DEFAULT_SRC = ( - "'self'", -) +CSP_DEFAULT_SRC = ("'self'",) CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov"] CSP_SCRIPT_SRC_ELEM = [ "'self'", From f68ef23dad07517c4816ec1f8354ad680271c5b8 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:19:54 -0700 Subject: [PATCH 013/101] Remove unused middleware class --- src/registrar/config/settings.py | 1 - src/registrar/registrar_middleware.py | 21 --------------------- 2 files changed, 22 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 4ae0c9fe5..1b5b2bee5 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -190,7 +190,6 @@ MIDDLEWARE = [ "waffle.middleware.WaffleMiddleware", "registrar.registrar_middleware.CheckUserProfileMiddleware", "registrar.registrar_middleware.CheckPortfolioMiddleware", - # "registrar.registrar_middleware.ANDIMiddleware", ] # application object used by Django’s built-in servers (e.g. `runserver`) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index a772fbe0a..98685061b 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -158,24 +158,3 @@ class CheckPortfolioMiddleware: return HttpResponseRedirect(portfolio_redirect) return None - - -class ANDIMiddleware(MiddlewareMixin): - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - response = self.get_response(request) - return response - - def process_template_view(self, request, view_func, view_args, view_kwargs): - response = self.get_response(request) - if "text/html" in response.get("Content-Type", ""): - andi_script = """ - - """ - # Inject the ANDI script before the closing tag - content = response.content.decode("utf-8") - content = content.replace("", f"{andi_script}") - response.content = content.encode("utf-8") - return None From f0219639644f6e9aa7f118a83ee318cf4d12b257 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 15 Aug 2024 13:29:02 -0700 Subject: [PATCH 014/101] Remove unused import --- src/registrar/registrar_middleware.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 98685061b..2af331bc9 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -8,7 +8,6 @@ from django.urls import reverse from django.http import HttpResponseRedirect from registrar.models.user import User from waffle.decorators import flag_is_active -from django.utils.deprecation import MiddlewareMixin from registrar.models.utility.generic_helper import replace_url_queryparams From 047b41e25e488e1f74c3966d9880dd943c13cb71 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 15 Aug 2024 14:22:07 -0700 Subject: [PATCH 015/101] Further define ANDI source --- src/registrar/config/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 1b5b2bee5..72bffdbb4 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -365,7 +365,7 @@ CSP_SCRIPT_SRC_ELEM = [ "https://www.ssa.gov", "https://ajax.googleapis.com", ] -CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov"] +CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] CSP_IMG_SRC = ["'self'", "https://www.ssa.gov"] From c022003539076d6143f1739c4b325d953e171aaf Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 15 Aug 2024 22:16:41 -0600 Subject: [PATCH 016/101] CSS updates --- src/registrar/assets/js/get-gov-admin.js | 63 ++++--- src/registrar/assets/sass/_theme/_admin.scss | 19 ++ .../admin/includes/detail_table_fieldset.html | 175 +++++++++--------- 3 files changed, 151 insertions(+), 106 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index e6d119d64..1e275e03e 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -544,6 +544,7 @@ function initializeWidgetOnList(list, parentId) { // Add a change listener to dom load document.addEventListener('DOMContentLoaded', function() { let reason = actionNeededReasonDropdown.value; + let emailBody = reason in actionNeededEmailsJson ? actionNeededEmailsJson[reason] : null; // Handle the session boolean (to enable/disable editing) if (emailWasSent && emailWasSent.value === "True") { @@ -552,29 +553,25 @@ function initializeWidgetOnList(list, parentId) { } // Show an editable email field or a readonly one - updateActionNeededEmailDisplay(reason) + updateActionNeededEmailDisplay(reason, emailBody) }); editEmailButton.addEventListener("click", function() { let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; - if (true) { //emailHasBeenSentBefore + if (emailHasBeenSentBefore) { // Show warning Modal setModalVisibility(actionNeededEmailAlreadySentModal, true) } else { // Show editable view - showElement(actionNeededEmail.parentElement) - hideElement(actionNeededEmailReadonly) - hideElement(placeholderText) + showEmail(true) } }); confirmEditEmailButton.addEventListener("click", function() { // Show editable view - showElement(actionNeededEmail.parentElement) - hideElement(actionNeededEmailReadonly) - hideElement(placeholderText) + showEmail(true) }); // Event delegation for data-close-modal buttons @@ -589,11 +586,8 @@ function initializeWidgetOnList(list, parentId) { actionNeededReasonDropdown.addEventListener("change", function() { let reason = actionNeededReasonDropdown.value; let emailBody = reason in actionNeededEmailsJson ? actionNeededEmailsJson[reason] : null; + if (reason && emailBody) { - // Replace the email content - actionNeededEmail.value = emailBody; - actionNeededEmailReadonlyTextarea.value = emailBody; - // Reset the session object on change since change refreshes the email content. if (oldDropdownValue !== actionNeededReasonDropdown.value || oldEmailValue !== actionNeededEmail.value) { let emailSent = sessionStorage.getItem(emailSentSessionVariableName) @@ -604,7 +598,7 @@ function initializeWidgetOnList(list, parentId) { } // Show an editable email field or a readonly one - updateActionNeededEmailDisplay(reason) + updateActionNeededEmailDisplay(reason, emailBody) }); const saveButton = document.querySelector('input[name=_save]'); @@ -632,9 +626,13 @@ function initializeWidgetOnList(list, parentId) { // Shows an editable email field or a readonly one. // If the email doesn't exist or if we're of reason "other", display that no email was sent. // Likewise, if we've sent this email before, we should just display the content. - function updateActionNeededEmailDisplay(reason) { + function updateActionNeededEmailDisplay(reason, emailBody) { - console.info("REASON: "+reason) + if (reason && emailBody) { + // Replace the email content + actionNeededEmail.value = emailBody; + actionNeededEmailReadonlyTextarea.value = emailBody; + } // showElement(actionNeededEmailHeader) // hideElement(actionNeededEmailHeaderOnSave) @@ -643,23 +641,42 @@ function initializeWidgetOnList(list, parentId) { if (reason) { if (reason === "other") { - placeholderText.innerHTML = "No email will be sent."; - hideElement(actionNeededEmailReadonly) - showElement(placeholderText) + showPlaceholderText("No email will be sent"); } else { // Always show readonly view to start - showElement(actionNeededEmailReadonly) - hideElement(placeholderText) + showEmail(false) } } else { - placeholderText.innerHTML = "Select an action needed reason to see email"; - hideElement(actionNeededEmailReadonly) - showElement(placeholderText) + showPlaceholderText("Select an action needed reason to see email"); } } + function showEmail(canEdit) + { + if(!canEdit) + { + showElement(actionNeededEmailReadonly) + hideElement(actionNeededEmail.parentElement) + } + else + { + hideElement(actionNeededEmailReadonly) + showElement(actionNeededEmail.parentElement) + } + showElement(actionNeededEmailFooter) // this is the same for both views, so it was separated out + hideElement(placeholderText) + } + function showPlaceholderText(innerHTML) + { + hideElement(actionNeededEmail.parentElement) + hideElement(actionNeededEmailReadonly) + hideElement(actionNeededEmailFooter) + + placeholderText.innerHTML = innerHTML; + showElement(placeholderText) + } })(); diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 711bfe960..74bdd6768 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -847,3 +847,22 @@ div.dja__model-description{ } } } + +.vertical-separator { + min-height: 20px; + height: 100%; + width: 1px; + background-color: #d1d2d2; + vertical-align: middle +} + +.usa-summary-box_admin { + border-color: #d1d2d2; + background-color: #f1f1f1; + max-width: fit-content !important; + padding: .5rem; +} + +.text-faded { + color: #{$dhs-gray-60}; +} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 6c495d81e..f7dd523a1 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -145,93 +145,102 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% block field_other %} {% if field.field.name == "action_needed_reason_email" %} -
- {{ field.field.value|linebreaks }} -
- {% else %} From fcf8ea613fcae3f6289f9c028e3d23fc84b12007 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Mon, 19 Aug 2024 16:54:43 -0600 Subject: [PATCH 040/101] update footer text --- .../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 f5e828b5c..bf4323002 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -239,8 +239,8 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {{ field.field }}
- + {% else %} {{ field.field }} From df9f3dcce57032153aea29906249170c1c40d70a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 20 Aug 2024 13:48:16 -0500 Subject: [PATCH 041/101] fix some tests --- src/registrar/fixtures_domain_requests.py | 2 +- src/registrar/models/domain_request.py | 2 +- .../already_has_domains.txt | 2 +- .../emails/action_needed_reasons/bad_name.txt | 2 +- .../eligibility_unclear.txt | 2 +- .../questionable_senior_official.txt | 2 +- .../emails/domain_request_withdrawn.txt | 2 +- .../emails/status_change_approved.txt | 2 +- .../emails/status_change_rejected.txt | 2 +- .../emails/submission_confirmation.txt | 2 +- .../includes/domain_requests_table.html | 2 +- src/registrar/tests/common.py | 6 +-- src/registrar/tests/test_admin_request.py | 26 +++++----- src/registrar/tests/test_reports.py | 2 +- .../tests/test_views_requests_json.py | 48 +++++++++---------- src/registrar/utility/csv_export.py | 6 +-- src/registrar/views/domain_requests_json.py | 2 +- src/registrar/views/report_views.py | 6 +-- 18 files changed, 60 insertions(+), 58 deletions(-) diff --git a/src/registrar/fixtures_domain_requests.py b/src/registrar/fixtures_domain_requests.py index 50f611474..a5ec3fc74 100644 --- a/src/registrar/fixtures_domain_requests.py +++ b/src/registrar/fixtures_domain_requests.py @@ -95,7 +95,7 @@ class DomainRequestFixture: # TODO for a future ticket: Allow for more than just "federal" here da.generic_org_type = app["generic_org_type"] if "generic_org_type" in app else "federal" - da.submission_date = fake.date() + da.last_submitted_date = fake.date() da.federal_type = ( app["federal_type"] if "federal_type" in app diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index e9711f0ae..77ebf88f8 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -821,7 +821,7 @@ class DomainRequest(TimeStampedModel): if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() - # Update last_submission_date to today + # Update last_submitted_date to today self.last_submitted_date = timezone.now().date() self.save() diff --git a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt index b1b3b0a1c..2e3012c91 100644 --- a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt +++ b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt index 7d088aa4e..9481a1e63 100644 --- a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt +++ b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt index d3a986183..705805998 100644 --- a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt +++ b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt b/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt index e20e4cb60..5967d7089 100644 --- a/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt +++ b/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/domain_request_withdrawn.txt b/src/registrar/templates/emails/domain_request_withdrawn.txt index 6efa92d64..0db00feea 100644 --- a/src/registrar/templates/emails/domain_request_withdrawn.txt +++ b/src/registrar/templates/emails/domain_request_withdrawn.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Your .gov domain request has been withdrawn and will not be reviewed by our team. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Withdrawn ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/status_change_approved.txt b/src/registrar/templates/emails/status_change_approved.txt index 70f813599..66f8f8b6c 100644 --- a/src/registrar/templates/emails/status_change_approved.txt +++ b/src/registrar/templates/emails/status_change_approved.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Congratulations! Your .gov domain request has been approved. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Approved You can manage your approved domain on the .gov registrar . diff --git a/src/registrar/templates/emails/status_change_rejected.txt b/src/registrar/templates/emails/status_change_rejected.txt index 2fcbb1d83..4e5250162 100644 --- a/src/registrar/templates/emails/status_change_rejected.txt +++ b/src/registrar/templates/emails/status_change_rejected.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Your .gov domain request has been rejected. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Rejected ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/submission_confirmation.txt b/src/registrar/templates/emails/submission_confirmation.txt index 740e6f393..c8ff4c7eb 100644 --- a/src/registrar/templates/emails/submission_confirmation.txt +++ b/src/registrar/templates/emails/submission_confirmation.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We received your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Submitted ---------------------------------------------------------------- diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index 487c1cee5..bd909350c 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -45,7 +45,7 @@ Domain name - Date submitted + Date submitted Status Action diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index a4c3e2ef4..85d1c56e7 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -775,13 +775,13 @@ class MockDb(TestCase): cls.domain_request_3.alternative_domains.add(website, website_2) cls.domain_request_3.current_websites.add(website_3, website_4) cls.domain_request_3.cisa_representative_email = "test@igorville.com" - cls.domain_request_3.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_3.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_3.save() - cls.domain_request_4.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_4.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_4.save() - cls.domain_request_6.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_6.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_6.save() @classmethod diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..d81f73156 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -422,7 +422,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that our sort works correctly self.test_helper.assert_table_sorted( - "11", + "13", ( "submitter__first_name", "submitter__last_name", @@ -431,7 +431,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that sorting in reverse works correctly self.test_helper.assert_table_sorted( - "-11", + "-13", ( "-submitter__first_name", "-submitter__last_name", @@ -454,7 +454,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that our sort works correctly self.test_helper.assert_table_sorted( - "12", + "14", ( "investigator__first_name", "investigator__last_name", @@ -463,7 +463,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that sorting in reverse works correctly self.test_helper.assert_table_sorted( - "-12", + "-14", ( "-investigator__first_name", "-investigator__last_name", @@ -476,7 +476,7 @@ class TestDomainRequestAdmin(MockEppLib): @less_console_noise_decorator def test_default_sorting_in_domain_requests_list(self): """ - Make sure the default sortin in on the domain requests list page is reverse submission_date + Make sure the default sortin in on the domain requests list page is reverse last_submitted_date then alphabetical requested_domain """ @@ -486,12 +486,12 @@ class TestDomainRequestAdmin(MockEppLib): for name in ["ccc.gov", "bbb.gov", "eee.gov", "aaa.gov", "zzz.gov", "ddd.gov"] ] - domain_requests[0].submission_date = timezone.make_aware(datetime(2024, 10, 16)) - domain_requests[1].submission_date = timezone.make_aware(datetime(2001, 10, 16)) - domain_requests[2].submission_date = timezone.make_aware(datetime(1980, 10, 16)) - domain_requests[3].submission_date = timezone.make_aware(datetime(1998, 10, 16)) - domain_requests[4].submission_date = timezone.make_aware(datetime(2013, 10, 16)) - domain_requests[5].submission_date = timezone.make_aware(datetime(1980, 10, 16)) + domain_requests[0].last_submitted_date = timezone.make_aware(datetime(2024, 10, 16)) + domain_requests[1].last_submitted_date = timezone.make_aware(datetime(2001, 10, 16)) + domain_requests[2].last_submitted_date = timezone.make_aware(datetime(1980, 10, 16)) + domain_requests[3].last_submitted_date = timezone.make_aware(datetime(1998, 10, 16)) + domain_requests[4].last_submitted_date = timezone.make_aware(datetime(2013, 10, 16)) + domain_requests[5].last_submitted_date = timezone.make_aware(datetime(1980, 10, 16)) # Save the modified domain requests to update their attributes in the database for domain_request in domain_requests: @@ -1556,7 +1556,9 @@ class TestDomainRequestAdmin(MockEppLib): "cisa_representative_last_name", "has_cisa_representative", "is_policy_acknowledged", - "submission_date", + "first_submitted_date", + "last_submitted_date", + "last_status_update", "notes", "alternative_domains", ] diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 52aaa8c38..4b58d6d22 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -762,7 +762,7 @@ class HelperFunctions(MockDbForSharedTests): with less_console_noise(): filter_condition = { "status": DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": self.end_date, + "last_submitted_date__lte": self.end_date, } submitted_requests_sliced_at_end_date = DomainRequestExport.get_sliced_requests(filter_condition) expected_content = [3, 2, 0, 0, 0, 0, 1, 0, 0, 1] diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index 7bdc922cf..c140b7e44 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -25,91 +25,91 @@ class GetRequestsJsonTest(TestWithUser, WebTest): DomainRequest.objects.create( creator=cls.user, requested_domain=lamb_chops, - submission_date="2024-01-01", + last_submitted_date="2024-01-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-01-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=short_ribs, - submission_date="2024-02-01", + last_submitted_date="2024-02-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-02-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=beef_chuck, - submission_date="2024-03-01", + last_submitted_date="2024-03-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-03-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=stew_beef, - submission_date="2024-04-01", + last_submitted_date="2024-04-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-04-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-05-01", + last_submitted_date="2024-05-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-05-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-06-01", + last_submitted_date="2024-06-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-06-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-07-01", + last_submitted_date="2024-07-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-07-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-08-01", + last_submitted_date="2024-08-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-08-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-09-01", + last_submitted_date="2024-09-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-09-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-10-01", + last_submitted_date="2024-10-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-10-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-11-01", + last_submitted_date="2024-11-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-11-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-11-02", + last_submitted_date="2024-11-02", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-11-02", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-12-01", + last_submitted_date="2024-12-01", status=DomainRequest.DomainRequestStatus.APPROVED, created_at="2024-12-01", ), @@ -138,7 +138,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Extract fields from response requested_domains = [request["requested_domain"] for request in data["domain_requests"]] - submission_dates = [request["submission_date"] for request in data["domain_requests"]] + last_submitted_dates = [request["last_submitted_date"] for request in data["domain_requests"]] statuses = [request["status"] for request in data["domain_requests"]] created_ats = [request["created_at"] for request in data["domain_requests"]] ids = [request["id"] for request in data["domain_requests"]] @@ -154,7 +154,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): self.domain_requests[i].requested_domain.name if self.domain_requests[i].requested_domain else None, requested_domains[i], ) - self.assertEqual(self.domain_requests[i].submission_date, submission_dates[i]) + self.assertEqual(self.domain_requests[i].last_submitted_date, last_submitted_dates[i]) self.assertEqual(self.domain_requests[i].get_status_display(), statuses[i]) self.assertEqual( parse_datetime(self.domain_requests[i].created_at.isoformat()), parse_datetime(created_ats[i]) @@ -287,26 +287,26 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json - # Check if sorted by submission_date in descending order - submission_dates = [req["submission_date"] for req in data["domain_requests"]] - self.assertEqual(submission_dates, sorted(submission_dates, reverse=True)) + # Check if sorted by last_submitted_date in descending order + last_submitted_dates = [req["last_submitted_date"] for req in data["domain_requests"]] + self.assertEqual(last_submitted_dates, sorted(last_submitted_dates, reverse=True)) - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "asc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "asc"}) self.assertEqual(response.status_code, 200) data = response.json - # Check if sorted by submission_date in ascending order - submission_dates = [req["submission_date"] for req in data["domain_requests"]] - self.assertEqual(submission_dates, sorted(submission_dates)) + # Check if sorted by last_submitted_date in ascending order + last_submitted_dates = [req["last_submitted_date"] for req in data["domain_requests"]] + self.assertEqual(last_submitted_dates, sorted(last_submitted_dates)) def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index db961a36d..6f8510418 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1235,7 +1235,7 @@ class DomainRequestExport(BaseExport): "State/territory": model.get("state_territory"), "Request purpose": model.get("purpose"), "CISA regional representative": model.get("cisa_representative_email"), - "Submitted at": model.get("submission_date"), + "Submitted at": model.get("last_submitted_date"), } row = [FIELDS.get(column, "") for column in columns] @@ -1279,8 +1279,8 @@ class DomainRequestGrowth(DomainRequestExport): end_date_formatted = format_end_date(end_date) return Q( status=DomainRequest.DomainRequestStatus.SUBMITTED, - submission_date__lte=end_date_formatted, - submission_date__gte=start_date_formatted, + last_submitted_date__lte=end_date_formatted, + last_submitted_date__gte=start_date_formatted, ) @classmethod diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index 2e58c8e48..6b0b346f8 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -46,7 +46,7 @@ def get_domain_requests_json(request): domain_requests_data = [ { "requested_domain": domain_request.requested_domain.name if domain_request.requested_domain else None, - "submission_date": domain_request.submission_date, + "last_submitted_date": domain_request.last_submitted_date, "status": domain_request.get_status_display(), "created_at": format(domain_request.created_at, "c"), # Serialize to ISO 8601 "id": domain_request.id, diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index 428298b52..abdbd37c9 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -26,7 +26,7 @@ class AnalyticsView(View): created_at__gt=thirty_days_ago, status=models.DomainRequest.DomainRequestStatus.APPROVED ) avg_approval_time = last_30_days_approved_applications.annotate( - approval_time=F("approved_domain__created_at") - F("submission_date") + approval_time=F("approved_domain__created_at") - F("last_submitted_date") ).aggregate(Avg("approval_time"))["approval_time__avg"] # Format the timedelta to display only days if avg_approval_time is not None: @@ -104,11 +104,11 @@ class AnalyticsView(View): filter_submitted_requests_start_date = { "status": models.DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": start_date_formatted, + "last_submitted_date__lte": start_date_formatted, } filter_submitted_requests_end_date = { "status": models.DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": end_date_formatted, + "last_submitted_date__lte": end_date_formatted, } submitted_requests_sliced_at_start_date = csv_export.DomainRequestExport.get_sliced_requests( filter_submitted_requests_start_date From 3c89557976742e4faa6c434f9b4db8d18470a8c4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 20 Aug 2024 15:29:28 -0400 Subject: [PATCH 042/101] wip --- src/djangooidc/views.py | 9 --- src/registrar/admin.py | 7 --- src/registrar/context_processors.py | 29 +++++---- .../0119_remove_user_portfolio_and_more.py | 11 ---- src/registrar/models/user.py | 60 ++++++++----------- src/registrar/registrar_middleware.py | 31 ++++++---- src/registrar/templates/domain_detail.html | 2 +- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/includes/domains_table.html | 2 +- src/registrar/views/domain.py | 8 +-- src/registrar/views/portfolios.py | 16 +++-- src/registrar/views/utility/mixins.py | 6 +- 12 files changed, 80 insertions(+), 103 deletions(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index c4d4cec81..815df4ecf 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -9,8 +9,6 @@ from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode -from waffle import flag_is_active - from djangooidc.oidc import Client from djangooidc import exceptions as o_e from registrar.models import User @@ -113,13 +111,6 @@ def login_callback(request): if not user.verification_type or is_fixture_user: user.set_user_verification_type() user.save() - - if not flag_is_active(request, "multiple_portfolios"): - user.set_default_last_selected_portfolio() - user.save() - else: - user.last_selected_portfolio = None - user.save() login(request, user) logger.info("Successfully logged in user %s" % user) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index fd555520d..a41035f71 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -717,17 +717,12 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "is_superuser", "groups", "user_permissions", - "last_selected_portfolio", ) }, ), ("Important dates", {"fields": ("last_login", "date_joined")}), ) - autocomplete_fields = [ - "last_selected_portfolio", - ] - readonly_fields = ("verification_type",) analyst_fieldsets = ( @@ -747,7 +742,6 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "fields": ( "is_active", "groups", - "last_selected_portfolio", ) }, ), @@ -802,7 +796,6 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "Important dates", "last_login", "date_joined", - "last_selected_portfolio", ] # TODO: delete after we merge organization feature diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index ca7c92892..7d28260ed 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,27 +61,34 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - if not request.user or not request.user.is_authenticated or not flag_is_active(request, "organization_feature"): + if request.session["portfolio"] is not None: return { - "has_base_portfolio_permission": False, - "has_domains_portfolio_permission": False, - "has_domain_requests_portfolio_permission": False, - "portfolio": None, - "has_organization_feature_flag": False, + "has_base_portfolio_permission": request.user.has_base_portfolio_permission(request.session["portfolio"]), + "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(request.session["portfolio"]), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(request.session["portfolio"]), + "has_view_suborganization": request.user.has_view_suborganization(request.session["portfolio"]), + "has_edit_suborganization": request.user.has_edit_suborganization(request.session["portfolio"]), + "portfolio": request.session["portfolio"], + "has_organization_feature_flag": True, } 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.last_selected_portfolio, - "has_organization_feature_flag": True, + "has_base_portfolio_permission": False, + "has_domains_portfolio_permission": False, + "has_domain_requests_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, + "portfolio": None, + "has_organization_feature_flag": False, } + except AttributeError: # Handles cases where request.user might not exist return { "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, "portfolio": None, "has_organization_feature_flag": False, } diff --git a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py index 7c9d2defc..84ed45cd1 100644 --- a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py +++ b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py @@ -25,17 +25,6 @@ class Migration(migrations.Migration): model_name="user", name="portfolio_roles", ), - migrations.AddField( - model_name="user", - name="last_selected_portfolio", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="portfolio_selected_by_users", - to="registrar.portfolio", - ), - ), migrations.CreateModel( name="UserPortfolioPermission", fields=[ diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 34f2f8bcf..b151f3800 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -110,14 +110,6 @@ class User(AbstractUser): related_name="users", ) - last_selected_portfolio = models.ForeignKey( - "registrar.Portfolio", - null=True, - blank=True, - related_name="portfolio_selected_by_users", - on_delete=models.SET_NULL, - ) - phone = PhoneNumberField( null=True, blank=True, @@ -208,50 +200,48 @@ class User(AbstractUser): def has_contact_info(self): return bool(self.title or self.email or self.phone) - def _has_portfolio_permission(self, portfolio_permission): + def _has_portfolio_permission(self, portfolio, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" - if not self.last_selected_portfolio: - return False - - portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio, user=self).first() + portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() if not portfolio_perms: return False portfolio_permissions = portfolio_perms._get_portfolio_permissions() return portfolio_permission in portfolio_permissions - def has_base_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + def has_base_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - def has_edit_org_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) + def has_edit_org_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) - def has_domains_portfolio_permission(self): - return self._has_portfolio_permission( + def has_domains_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) + ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) - def has_domain_requests_portfolio_permission(self): - return self._has_portfolio_permission( + def has_domain_requests_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) - def has_view_all_domains_permission(self): + def has_view_all_domains_permission(self, portfolio): """Determines if the current user can view all available domains in a given portfolio""" - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) # Field specific permission checks - def has_view_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) + def has_view_suborganization(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - def has_edit_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + def has_edit_suborganization(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) - def set_default_last_selected_portfolio(self): + def get_first_portfolio(self): permission = self.portfolio_permissions.first() if permission: - self.last_selected_portfolio = permission.portfolio + return permission.portfolio + return None @classmethod def needs_identity_verification(cls, email, uuid): @@ -367,7 +357,7 @@ class User(AbstractUser): email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): only_single_portfolio = ( - not flag_is_active(None, "multiple_portfolios") and self.last_selected_portfolio is None + not flag_is_active(None, "multiple_portfolios") and self.get_first_portfolio() is None ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: @@ -396,12 +386,12 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - return has_organization_feature_flag and self.has_base_portfolio_permission() + return has_organization_feature_flag and self.has_base_portfolio_permission(request.session["portfolio"]) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - if self.is_org_user(request) and self.has_view_all_domains_permission(): - return DomainInformation.objects.filter(portfolio=self.last_selected_portfolio).values_list( + if self.is_org_user(request) and self.has_view_all_domains_permission(request.session["portfolio"]): + return DomainInformation.objects.filter(portfolio=request.session["portfolio"]).values_list( "domain_id", flat=True ) else: diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 6e7d110fb..1da74a527 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -125,8 +125,9 @@ class CheckUserProfileMiddleware: class CheckPortfolioMiddleware: """ - Checks if the current user has a portfolio - If they do, redirect them to the portfolio homepage when they navigate to home. + this middleware should serve two purposes: + 1 - set the portfolio in session if appropriate # views will need the session portfolio + 2 - if path is home and session portfolio is set, redirect based on permissions of user """ def __init__(self, get_response): @@ -140,20 +141,28 @@ class CheckPortfolioMiddleware: def process_view(self, request, view_func, view_args, view_kwargs): current_path = request.path - if current_path == self.home and request.user.is_authenticated and request.user.is_org_user(request): + if not request.user.is_authenticated: + return None + + # set the portfolio in the session if it is not set + if not "portfolio" in request.session or request.session["portfolio"] is None: + # if user is a multiple portfolio + if flag_is_active(request, "multiple_portfolios"): + # NOTE: we will want to change later to have a workflow for selecting + # portfolio and another for switching portfolio; for now, select first + request.session["portfolio"] = request.user.get_first_portfolio() + elif flag_is_active(request, "organization_feature"): + request.session["portfolio"] = request.user.get_first_portfolio() + else: + request.session["portfolio"] = None - if request.user.has_base_portfolio_permission(): - portfolio = request.user.last_selected_portfolio - - # Add the portfolio to the request object - request.last_selected_portfolio = portfolio - - if request.user.has_domains_portfolio_permission(): + if request.session["portfolio"] is not None and current_path == self.home: + if request.user.has_base_portfolio_permission(request.session["portfolio"]): + if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") else: # View organization is the lowest access portfolio_redirect = reverse("organization") - return HttpResponseRedirect(portfolio_redirect) return None diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 19f196e40..cf55f7070 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -72,7 +72,7 @@ {% include "includes/summary_item.html" with title='DNSSEC' value='Not Enabled' edit_link=url editable=is_editable %} {% endif %} - {% if portfolio and has_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if portfolio and has_domains_portfolio_permission and has_view_suborganization %} {% url 'domain-suborganization' pk=domain.id as url %} {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:request.user.has_edit_suborganization %} {% else %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index f2e9f190c..24f92bf16 100644 --- a/src/registrar/templates/domain_sidebar.html +++ b/src/registrar/templates/domain_sidebar.html @@ -61,7 +61,7 @@ {% if portfolio %} {% comment %} Only show this menu option if the user has the perms to do so {% endcomment %} - {% if has_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if has_domains_portfolio_permission and has_view_suborganization %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 73331c3f0..f790da4d5 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -157,7 +157,7 @@ Domain name Expires Status - {% if has_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if has_domains_portfolio_permission and has_view_suborganization %} Suborganization {% endif %} Date: Tue, 20 Aug 2024 14:17:14 -0600 Subject: [PATCH 043/101] Fix edge cases --- src/registrar/context_processors.py | 15 ++++++++------- src/registrar/templates/domain_detail.html | 2 +- .../templates/domain_suborganization.html | 2 +- src/registrar/views/domain.py | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 7d28260ed..16d592982 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,14 +61,15 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - if request.session["portfolio"] is not None: + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + if portfolio: return { - "has_base_portfolio_permission": request.user.has_base_portfolio_permission(request.session["portfolio"]), - "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(request.session["portfolio"]), - "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(request.session["portfolio"]), - "has_view_suborganization": request.user.has_view_suborganization(request.session["portfolio"]), - "has_edit_suborganization": request.user.has_edit_suborganization(request.session["portfolio"]), - "portfolio": request.session["portfolio"], + "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), + "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(portfolio), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(portfolio), + "has_view_suborganization": request.user.has_view_suborganization(portfolio), + "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), + "portfolio": portfolio, "has_organization_feature_flag": True, } return { diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index cf55f7070..d7bc277b3 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -74,7 +74,7 @@ {% if portfolio and has_domains_portfolio_permission and has_view_suborganization %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:request.user.has_edit_suborganization %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_suborganization %} {% else %} {% 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=is_editable %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index 42bb766a3..ad96f1d65 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -15,7 +15,7 @@ If you believe there is an error please contact help@get.gov.

- {% if has_domains_portfolio_permission and request.user.has_edit_suborganization %} + {% if has_domains_portfolio_permission and has_edit_suborganization %}
{% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index d6ffbf64a..aac9d056d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,7 +174,7 @@ class DomainView(DomainBaseView): """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 self.request.user.has_domains_portfolio_permission(self.request.session["portfolio"]): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) if domain.domain_info.portfolio == self.request.session["portfolio"]: From 5c8ad8cd34fc9cd7677edabd10d00032b6c1697d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:30:04 -0600 Subject: [PATCH 044/101] Fix incorrect param name for completed domain request --- src/registrar/tests/common.py | 6 +++--- src/registrar/tests/test_admin.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index f3fd6709f..ceb3b6e92 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -906,7 +906,7 @@ def completed_domain_request( # noqa federal_agency=None, federal_type=None, action_needed_reason=None, - last_selected_portfolio=None, + portfolio=None, ): """A completed domain request.""" if not user: @@ -977,8 +977,8 @@ def completed_domain_request( # noqa if action_needed_reason: domain_request_kwargs["action_needed_reason"] = action_needed_reason - if last_selected_portfolio: - domain_request_kwargs["last_selected_portfolio"] = last_selected_portfolio + if portfolio: + domain_request_kwargs["portfolio"] = portfolio domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49d2b4802..144605b87 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -41,6 +41,7 @@ from registrar.models import ( TransitionDomain, Portfolio, Suborganization, + UserPortfolioPermission, ) from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial From 9e1643edf08520383e1a6970bdce09ffb8749edd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:49:04 -0600 Subject: [PATCH 045/101] Fix (some) test errors --- src/registrar/tests/test_models.py | 53 +++++++++++++-------- src/registrar/tests/test_reports.py | 1 + src/registrar/tests/test_views_domain.py | 5 +- src/registrar/tests/test_views_portfolio.py | 10 ++-- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 1c95257d3..52e950cdc 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1149,7 +1149,8 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieval(self): - self.assertFalse(self.user.portfolio) + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user).exists() + self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() self.assertEqual(self.user.last_selected_portfolio.organization_name, "Hotel California") @@ -1363,13 +1364,16 @@ class TestUser(TestCase): Note: This tests _get_portfolio_permissions as a side effect """ - portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - self.user.last_selected_portfolio = portfolio - self.user.save() - self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1380,8 +1384,13 @@ class TestUser(TestCase): portfolio_permission.refresh_from_db() self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1392,18 +1401,28 @@ class TestUser(TestCase): portfolio_permission.refresh_from_db() self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - UserDomainRole.objects.all().get_or_create( + UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER ) - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) @@ -1431,12 +1450,8 @@ class TestUser(TestCase): @less_console_noise_decorator def test_user_with_portfolio_roles_but_no_portfolio(self): - # Create an instance of User with a portfolio role but no portfolio - self.user.portfolio = None - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) # Try to remove the portfolio portfolio_permission.portfolio = None diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 8440e54e1..eca6bdb8b 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -345,6 +345,7 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): factory = RequestFactory() request = factory.get("/") request.user = self.user + request.session = {} # Get the csv content csv_content = self._run_domain_data_type_user_export(request) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index f3f4d1051..6ce842bc6 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -329,9 +329,8 @@ class TestDomainDetail(TestDomainOverview): email="bogus@example.gov", phone="8003111234", title="test title", - last_selected_portfolio=portfolio, ) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[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) @@ -1572,7 +1571,7 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index cb72f29fe..1c4cfcac9 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -31,6 +31,7 @@ class TestPortfolio(WebTest): ) def tearDown(self): + UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() UserDomainRole.objects.all().delete() DomainRequest.objects.all().delete() @@ -85,7 +86,6 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_portfolio(self): """Test that user with no assigned portfolio is not redirected when attempting to access home""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -97,7 +97,7 @@ class TestPortfolio(WebTest): def test_middleware_redirects_to_portfolio_organization_page(self): """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -111,7 +111,7 @@ class TestPortfolio(WebTest): """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -125,7 +125,7 @@ class TestPortfolio(WebTest): def test_portfolio_domains_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -137,7 +137,7 @@ class TestPortfolio(WebTest): def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. From a362c2ef6c49c9dde1d58747321b1eea536c5edb Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:53:16 -0600 Subject: [PATCH 046/101] Linting --- src/registrar/context_processors.py | 6 +- src/registrar/models/user.py | 8 +- .../models/user_portfolio_permission.py | 8 +- src/registrar/tests/test_models.py | 8 +- src/registrar/tests/test_views_domain.py | 16 +++- src/registrar/tests/test_views_portfolio.py | 76 +++++++++++++++---- src/registrar/views/portfolios.py | 4 +- 7 files changed, 90 insertions(+), 36 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 16d592982..92a89ca02 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -66,7 +66,9 @@ def portfolio_permissions(request): return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(portfolio), - "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(portfolio), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission( + portfolio + ), "has_view_suborganization": request.user.has_view_suborganization(portfolio), "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), "portfolio": portfolio, @@ -81,7 +83,7 @@ def portfolio_permissions(request): "portfolio": None, "has_organization_feature_flag": False, } - + except AttributeError: # Handles cases where request.user might not exist return { diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b151f3800..73b67b6ae 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -217,13 +217,13 @@ class User(AbstractUser): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) def has_domains_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + return self._has_portfolio_permission( + portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS + return self._has_portfolio_permission( + portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) def has_view_all_domains_permission(self, portfolio): diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index e00bf37f9..4582ffd08 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -75,11 +75,7 @@ class UserPortfolioPermission(TimeStampedModel): ) def __str__(self): - return ( - f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" - if self.roles - else "" - ) + return f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" if self.roles else "" def _get_portfolio_permissions(self): """ @@ -107,7 +103,7 @@ class UserPortfolioPermission(TimeStampedModel): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) - + if self.portfolio is None and self._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 52e950cdc..ec672c970 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1412,9 +1412,7 @@ class TestUser(TestCase): self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - UserDomainRole.objects.get_or_create( - user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER - ) + UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) # Create a dummy request request = self.factory.get("/") @@ -1451,7 +1449,9 @@ class TestUser(TestCase): @less_console_noise_decorator def test_user_with_portfolio_roles_but_no_portfolio(self): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) # Try to remove the portfolio portfolio_permission.portfolio = None diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 6ce842bc6..ab489c813 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -330,7 +330,9 @@ class TestDomainDetail(TestDomainOverview): phone="8003111234", title="test title", ) - UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + UserPortfolioPermission.objects.get_or_create( + user=user, portfolio=portfolio, roles=[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) @@ -1477,7 +1479,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1533,7 +1537,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] + ) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1571,7 +1577,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + ) # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 1c4cfcac9..f785a1551 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -54,7 +54,11 @@ class TestPortfolio(WebTest): self.portfolio.save() self.portfolio.refresh_from_db() - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) so_portfolio_page = self.app.get(reverse("senior-official")) # Assert that we're on the right page @@ -71,7 +75,9 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_permission(self): """Test that user with no portfolio permission is not redirected when attempting to access home""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) self.user.portfolio = self.portfolio self.user.save() self.user.refresh_from_db() @@ -97,7 +103,11 @@ class TestPortfolio(WebTest): def test_middleware_redirects_to_portfolio_organization_page(self): """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -111,7 +121,14 @@ class TestPortfolio(WebTest): """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + ], + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -125,7 +142,9 @@ class TestPortfolio(WebTest): def test_portfolio_domains_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -137,7 +156,9 @@ class TestPortfolio(WebTest): def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -149,7 +170,9 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_403_when_user_not_have_permission(self): """Test that user without proper permission is not allowed access to portfolio organization page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -161,7 +184,11 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_read_only(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) self.portfolio.city = "Los Angeles" self.portfolio.save() with override_flag("organization_feature", active=True): @@ -179,7 +206,14 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_edit_access(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ], + ) self.portfolio.city = "Los Angeles" self.portfolio.save() with override_flag("organization_feature", active=True): @@ -202,7 +236,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -217,7 +253,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.user.save() self.user.refresh_from_db() @@ -234,7 +272,9 @@ class TestPortfolio(WebTest): """Test that admin / memmber roles are associated with the right access""" self.app.set_user(self.user.username) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -269,7 +309,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) page = self.app.get(reverse("organization")) self.assertContains( page, "The name of your federal agency will be publicly listed as the domain registrant." @@ -284,7 +326,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.portfolio.organization_name = "Hotel California" self.portfolio.save() @@ -302,7 +346,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.portfolio.address_line1 = "1600 Penn Ave" self.portfolio.save() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 554576a2e..d6869c37a 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,7 +51,9 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(self.request.session["portfolio"]) + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( + self.request.session["portfolio"] + ) return context def get_object(self, queryset=None): From 381ac494870a4427524dae1116bae5b4246cc380 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:58:51 -0600 Subject: [PATCH 047/101] lint pt 2 --- src/registrar/models/user.py | 1 - src/registrar/registrar_middleware.py | 8 ++++---- src/registrar/tests/test_admin.py | 1 - 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 73b67b6ae..521093e2f 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -3,7 +3,6 @@ import logging from django.contrib.auth.models import AbstractUser from django.db import models from django.db.models import Q -from django.forms import ValidationError from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 1da74a527..d0f2dfa2f 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -143,20 +143,20 @@ class CheckPortfolioMiddleware: if not request.user.is_authenticated: return None - + # set the portfolio in the session if it is not set - if not "portfolio" in request.session or request.session["portfolio"] is None: + if "portfolio" not in request.session or request.session["portfolio"] is None: # if user is a multiple portfolio if flag_is_active(request, "multiple_portfolios"): # NOTE: we will want to change later to have a workflow for selecting # portfolio and another for switching portfolio; for now, select first request.session["portfolio"] = request.user.get_first_portfolio() - elif flag_is_active(request, "organization_feature"): + elif flag_is_active(request, "organization_feature"): request.session["portfolio"] = request.user.get_first_portfolio() else: request.session["portfolio"] = None - if request.session["portfolio"] is not None and current_path == self.home: + if request.session["portfolio"] is not None and current_path == self.home: if request.user.has_base_portfolio_permission(request.session["portfolio"]): if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 144605b87..49d2b4802 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -41,7 +41,6 @@ from registrar.models import ( TransitionDomain, Portfolio, Suborganization, - UserPortfolioPermission, ) from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial From 4f4dac272873c4cab07e84050fb63edfd7936f8b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 08:42:05 -0600 Subject: [PATCH 048/101] Add better error handling on session["portfolio"] This was causing an error on some functions where request can be None, like our csv exports --- src/registrar/models/user.py | 8 +++++--- src/registrar/tests/common.py | 4 ++++ src/registrar/tests/test_models.py | 26 ++++++++++++++------------ src/registrar/views/domain.py | 14 +++++++++----- src/registrar/views/portfolios.py | 13 ++++++++----- src/registrar/views/utility/mixins.py | 11 ++++++++--- 6 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 521093e2f..8789a628b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -385,12 +385,14 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - return has_organization_feature_flag and self.has_base_portfolio_permission(request.session["portfolio"]) + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - if self.is_org_user(request) and self.has_view_all_domains_permission(request.session["portfolio"]): - return DomainInformation.objects.filter(portfolio=request.session["portfolio"]).values_list( + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): + return DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True ) else: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index ceb3b6e92..96da5c14d 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -27,6 +27,8 @@ from registrar.models import ( PublicContact, Domain, FederalAgency, + UserPortfolioPermission, + Portfolio, ) from epplibwrapper import ( commands, @@ -791,6 +793,8 @@ class MockDb(TestCase): DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() UserDomainRole.objects.all().delete() + Portfolio.objects.all().delete() + UserPortfolioPermission.objects.all().delete() User.objects.all().delete() DomainInvitation.objects.all().delete() cls.federal_agency_1.delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ec672c970..8db11a0e7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1143,21 +1143,22 @@ class TestPortfolioInvitations(TestCase): def tearDown(self): super().tearDown() + UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() PortfolioInvitation.objects.all().delete() User.objects.all().delete() @less_console_noise_decorator def test_retrieval(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() - self.assertEqual(self.user.last_selected_portfolio.organization_name, "Hotel California") - portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) - self.assertEqual(portfolio_role.roles, [self.portfolio_role_base, self.portfolio_role_admin]) + created_role = UserPortfolioPermission.objects.get(user=self.user, portfolio=self.portfolio) + self.assertEqual(created_role.portfolio.organization_name, "Hotel California") + self.assertEqual(created_role.roles, [self.portfolio_role_base, self.portfolio_role_admin]) self.assertEqual( - portfolio_role.additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] + created_role.additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] ) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1170,15 +1171,15 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - self.assertFalse(self.user.last_selected_portfolio) - portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Tokyo Hotel") - portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio2) - self.assertEqual(self.user.last_selected_portfolio.organization_name, "Tokyo Hotel") - self.assertEqual(portfolio_role.portfolio, portfolio2) - self.user.save() + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + self.assertFalse(portfolio_role_exists) + portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) + self.assertEqual(portfolio_role.portfolio.organization_name, "Hotel California") self.user.check_portfolio_invitations_on_login() self.user.refresh_from_db() - self.assertEqual(self.user.portfolio.organization_name, "Tokyo Hotel") + + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 1) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) @@ -1192,6 +1193,7 @@ class TestUser(TestCase): self.domain_name = "igorvilleInTransition.gov" self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") self.user, _ = User.objects.get_or_create(email=self.email) + self.factory = RequestFactory() def tearDown(self): super().tearDown() diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index aac9d056d..7f31945f6 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,10 +174,11 @@ class DomainView(DomainBaseView): """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(self.request.session["portfolio"]): + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if self.request.user.has_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) - if domain.domain_info.portfolio == self.request.session["portfolio"]: + if domain.domain_info.portfolio == portfolio: return True return False @@ -236,7 +237,8 @@ class DomainOrgNameAddressView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return False else: return super().has_permission() @@ -255,7 +257,8 @@ class DomainSubOrganizationView(DomainFormBaseView): # non-org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return super().has_permission() else: return False @@ -335,7 +338,8 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return False else: return super().has_permission() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d6869c37a..d3a6d6055 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,16 +51,18 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( - self.request.session["portfolio"] + portfolio ) return context def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - if self.request.session["portfolio"] is None: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio is None: raise Http404("No organization found for this user") - return self.request.session["portfolio"] + return portfolio def get_form_kwargs(self): """Include the instance in the form kwargs.""" @@ -113,9 +115,10 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - if self.request.session["portfolio"] is None: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio is None: raise Http404("No organization found for this user") - return self.request.session["portfolio"] + return portfolio def get_form_kwargs(self): """Include the instance in the form kwargs.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index c6fdd5b0a..1f255f414 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,7 +419,8 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - return self.request.user.has_base_portfolio_permission(self.request.session["portfolio"]) + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_base_portfolio_permission(portfolio) class PortfolioDomainsPermission(PortfolioBasePermission): @@ -434,7 +435,9 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - return self.request.user.has_domains_portfolio_permission(self.request.session["portfolio"]) + + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_domains_portfolio_permission(portfolio) class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -449,4 +452,6 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - return self.request.user.has_domain_requests_portfolio_permission(self.request.session["portfolio"]) + + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_domain_requests_portfolio_permission(portfolio) From 729ce4c9f784089f7ce4a31cb67670d77be73210 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 08:46:47 -0600 Subject: [PATCH 049/101] Replace inline if with .get Not sure why I was doing that --- src/registrar/context_processors.py | 2 +- src/registrar/models/user.py | 4 ++-- src/registrar/views/domain.py | 8 ++++---- src/registrar/views/portfolios.py | 6 +++--- src/registrar/views/utility/mixins.py | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 92a89ca02..ea04dca80 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,7 +61,7 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") if portfolio: return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 8789a628b..c19778827 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -385,12 +385,12 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): return DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 7f31945f6..003f8dd0d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,7 +174,7 @@ class DomainView(DomainBaseView): """Most views should not allow permission to portfolio users. If particular views allow permissions, they will need to override this function.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if self.request.user.has_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) @@ -237,7 +237,7 @@ class DomainOrgNameAddressView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return False else: @@ -257,7 +257,7 @@ class DomainSubOrganizationView(DomainFormBaseView): # non-org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return super().has_permission() else: @@ -338,7 +338,7 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return False else: diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d3a6d6055..18285774b 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,7 +51,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( portfolio ) @@ -59,7 +59,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio is None: raise Http404("No organization found for this user") return portfolio @@ -115,7 +115,7 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio is None: raise Http404("No organization found for this user") return portfolio diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 1f255f414..376912634 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,7 +419,7 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_base_portfolio_permission(portfolio) @@ -436,7 +436,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_domains_portfolio_permission(portfolio) @@ -453,5 +453,5 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_domain_requests_portfolio_permission(portfolio) From ba2add8bc8346c76311964989a6d870cd1f7e9cf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:42:31 -0600 Subject: [PATCH 050/101] Fix redirect bug and fix permission issue --- src/registrar/models/user.py | 5 +++++ src/registrar/registrar_middleware.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index c19778827..38e971a3b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -202,6 +202,9 @@ class User(AbstractUser): def _has_portfolio_permission(self, portfolio, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" + if not portfolio: + return False + portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() if not portfolio_perms: return False @@ -383,6 +386,8 @@ class User(AbstractUser): self.check_domain_invitations_on_login() self.check_portfolio_invitations_on_login() + # NOTE TO DAVE: I'd simply suggest that we move these functions outside of the user object, + # and move them to some sort of utility file. That way we aren't calling request inside here. def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") portfolio = request.session.get("portfolio") diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index d0f2dfa2f..aa92ed5cd 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -157,7 +157,7 @@ class CheckPortfolioMiddleware: request.session["portfolio"] = None if request.session["portfolio"] is not None and current_path == self.home: - if request.user.has_base_portfolio_permission(request.session["portfolio"]): + if request.user.is_org_user(request): if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") else: From cbec3f2ed7b5e6445568cf83d4097df047467b41 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:43:24 -0600 Subject: [PATCH 051/101] Update mixins.py --- src/registrar/views/utility/mixins.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 376912634..c35804243 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,8 +419,7 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_base_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) class PortfolioDomainsPermission(PortfolioBasePermission): @@ -436,8 +435,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_domains_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -453,5 +451,4 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_domain_requests_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) From 5c077752770becfd67e7cf5d41d8284e1b084d9a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 10:15:49 -0600 Subject: [PATCH 052/101] Fix two tests --- src/registrar/tests/common.py | 9 +++++++++ src/registrar/tests/test_models.py | 8 +++++++- src/registrar/tests/test_reports.py | 27 ++++++++++++--------------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 96da5c14d..f5f62cf3e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -1747,3 +1747,12 @@ class MockEppLib(TestCase): def tearDown(self): self.mockSendPatch.stop() + + +def get_wsgi_request_object(client, user, url="/"): + """Returns client.get(url).wsgi_request for testing functions or classes + that need a request object directly passed to them.""" + client.force_login(user) + request = client.get(url).wsgi_request + request.user = user + return request diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8db11a0e7..33499eb91 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1459,9 +1459,15 @@ class TestUser(TestCase): portfolio_permission.portfolio = None portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + # Create a new UserPortfolioPermission instance without a portfolio + invalid_permission = UserPortfolioPermission( + user=self.user, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + portfolio=None # This should trigger the validation error + ) # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - portfolio_permission.clean() + invalid_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index eca6bdb8b..1e169b51d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -34,7 +34,7 @@ import boto3_mocking from registrar.utility.s3_bucket import S3ClientError, S3ClientErrorCodes # type: ignore from django.utils import timezone from api.tests.common import less_console_noise_decorator -from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, less_console_noise, get_time_aware_date +from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, get_wsgi_request_object, less_console_noise, get_time_aware_date from waffle.testutils import override_flag @@ -282,10 +282,8 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): # Create a user and associate it with some domains UserDomainRole.objects.create(user=self.user, domain=self.domain_2) - # Create a request object - factory = RequestFactory() - request = factory.get("/") - request.user = self.user + # Make a GET request using self.client to get a request object + request = get_wsgi_request_object(client=self.client, user=self.user) # Create a CSV file in memory csv_file = StringIO() @@ -339,13 +337,9 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] portfolio_permission.save() portfolio_permission.refresh_from_db() - self.user.refresh_from_db() - # Create a request object - factory = RequestFactory() - request = factory.get("/") - request.user = self.user - request.session = {} + # Make a GET request using self.client to get a request object + request = get_wsgi_request_object(client=self.client, user=self.user) # Get the csv content csv_content = self._run_domain_data_type_user_export(request) @@ -356,19 +350,22 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): self.assertNotIn(self.domain_2.name, csv_content) # Test the output for readonly admin - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] portfolio_permission.save() + portfolio_permission.refresh_from_db() + # Get the csv content + csv_content = self._run_domain_data_type_user_export(request) self.assertIn(self.domain_1.name, csv_content) self.assertIn(self.domain_3.name, csv_content) self.assertNotIn(self.domain_2.name, csv_content) - # Get the csv content - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] portfolio_permission.save() + portfolio_permission.refresh_from_db() + # Get the csv content csv_content = self._run_domain_data_type_user_export(request) - self.assertNotIn(self.domain_1.name, csv_content) self.assertNotIn(self.domain_3.name, csv_content) self.assertIn(self.domain_2.name, csv_content) From 08c42a6e8d48bb117e494e4ae82095c403106521 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Wed, 21 Aug 2024 12:28:41 -0500 Subject: [PATCH 053/101] merge main and resolve migration conflicts --- ...sion_dates.py => 0119_add_domainrequest_submission_dates.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/registrar/migrations/{0118_add_domainrequest_submission_dates.py => 0119_add_domainrequest_submission_dates.py} (92%) diff --git a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py similarity index 92% rename from src/registrar/migrations/0118_add_domainrequest_submission_dates.py rename to src/registrar/migrations/0119_add_domainrequest_submission_dates.py index c971b2f9a..0b94e1257 100644 --- a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -6,7 +6,7 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ("registrar", "0117_alter_portfolioinvitation_portfolio_additional_permissions_and_more"), + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), ] operations = [ From 32ed84a5e15d07c820d4553fb4804ad1c1a06e71 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:43:40 -0600 Subject: [PATCH 054/101] Fix logic in clean --- src/registrar/models/user_portfolio_permission.py | 10 +++++++--- src/registrar/tests/test_models.py | 8 +------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 4582ffd08..a3460a651 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -97,15 +97,19 @@ class UserPortfolioPermission(TimeStampedModel): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() - if not flag_is_active(None, "multiple_portfolios") and self.pk is None: + # Check if a user is set without accessing the related object. + has_user = bool(self.user_id) + if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) - if self.portfolio is None and self._get_portfolio_permissions(): + # Check if portfolio is set without accessing the related object. + has_portfolio = bool(self.portfolio_id) + if not has_portfolio and self._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - if self.portfolio is not None and not self._get_portfolio_permissions(): + if has_portfolio and not self._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 33499eb91..8db11a0e7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1459,15 +1459,9 @@ class TestUser(TestCase): portfolio_permission.portfolio = None portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - # Create a new UserPortfolioPermission instance without a portfolio - invalid_permission = UserPortfolioPermission( - user=self.user, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - portfolio=None # This should trigger the validation error - ) # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - invalid_permission.clean() + portfolio_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." From bfc2693a12e61f8629777a31f4612cdf02901d7d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:12:50 -0600 Subject: [PATCH 055/101] Fix a few more tests --- src/registrar/tests/test_models.py | 61 +++++++++------------ src/registrar/tests/test_views_domain.py | 12 ++-- src/registrar/tests/test_views_portfolio.py | 9 +-- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8db11a0e7..995f2a89c 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,6 +30,7 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, + get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1369,60 +1370,50 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) - portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - portfolio_permission.save() - portfolio_permission.refresh_from_db() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] portfolio_permission.save() portfolio_permission.refresh_from_db() - self.user.refresh_from_db() - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index ab489c813..e457f1b66 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -317,6 +317,7 @@ class TestDomainDetail(TestDomainOverview): self.assertContains(detail_page, "Domain missing domain information") @less_console_noise_decorator + @override_flag("organization_feature", active=True) 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""" @@ -330,11 +331,13 @@ class TestDomainDetail(TestDomainOverview): phone="8003111234", title="test title", ) + domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") + DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + UserPortfolioPermission.objects.get_or_create( user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") - DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + user.refresh_from_db() self.client.force_login(user) detail_page = self.client.get(f"/domain/{domain.id}") # Check that alert message displays properly @@ -1577,9 +1580,10 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) + self.user.refresh_from_db() # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..039f6f13e 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -252,12 +252,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions - ) - self.user.save() - self.user.refresh_from_db() + portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + portfolio_permission.save() + portfolio_permission.refresh_from_db() portfolio_page = self.app.get(reverse("home")).follow() From aa872d6e41457a73fe13f64e9eb3b38502144bed Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:22:03 -0600 Subject: [PATCH 056/101] Fix last test and lint --- src/registrar/models/user.py | 4 +--- src/registrar/tests/test_admin.py | 1 - src/registrar/tests/test_models.py | 15 +++++++++++---- src/registrar/tests/test_reports.py | 9 ++++++++- src/registrar/tests/test_views_portfolio.py | 2 +- src/registrar/views/portfolios.py | 4 +--- src/registrar/views/utility/mixins.py | 4 ++-- 7 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 38e971a3b..6aed8195c 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -397,8 +397,6 @@ class User(AbstractUser): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" portfolio = request.session.get("portfolio") if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): - return DomainInformation.objects.filter(portfolio=portfolio).values_list( - "domain_id", flat=True - ) + return DomainInformation.objects.filter(portfolio=portfolio).values_list("domain_id", flat=True) else: return UserDomainRole.objects.filter(user=self).values_list("domain_id", flat=True) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49d2b4802..827742ef1 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1209,7 +1209,6 @@ class TestMyUserAdmin(MockDbForSharedTests): domain_deleted.delete() role.delete() - # TODO - should refactor def test_analyst_cannot_see_selects_for_portfolio_role_and_permissions_in_user_form(self): """Can only test for the presence of a base element. The multiselects and the h2->h3 conversion are all dynamically generated.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 995f2a89c..3a5405fe8 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,7 +30,6 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, - get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1151,7 +1150,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieval(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() @@ -1172,7 +1173,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) self.assertEqual(portfolio_role.portfolio.organization_name, "Hotel California") @@ -1380,7 +1383,11 @@ class TestUser(TestCase): self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, + user=self.user, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + ) # TODO - uncomment this when we just pass request to these functions # request = get_wsgi_request_object(self.client, self.user) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 1e169b51d..8bb15921d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -34,7 +34,14 @@ import boto3_mocking from registrar.utility.s3_bucket import S3ClientError, S3ClientErrorCodes # type: ignore from django.utils import timezone from api.tests.common import less_console_noise_decorator -from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, get_wsgi_request_object, less_console_noise, get_time_aware_date +from .common import ( + MockDbForSharedTests, + MockDbForIndividualTests, + MockEppLib, + get_wsgi_request_object, + less_console_noise, + get_time_aware_date, +) from waffle.testutils import override_flag diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 039f6f13e..1568391fc 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -285,7 +285,7 @@ class TestPortfolio(WebTest): # removing non-basic portfolio role, which should remove domains # and domain requests from nav - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] portfolio_permission.save() portfolio_permission.refresh_from_db() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 18285774b..1b5cabea7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -52,9 +52,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) portfolio = self.request.session.get("portfolio") - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( - portfolio - ) + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(portfolio) return context def get_object(self, queryset=None): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index c35804243..d1edfc46e 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -434,7 +434,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) @@ -450,5 +450,5 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) From f4647caded419d5ea41d0b336a5120c9cff47999 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:28:23 -0400 Subject: [PATCH 057/101] working on tests --- src/registrar/tests/test_views_portfolio.py | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..ffcc87823 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -14,6 +14,7 @@ from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .common import create_test_user from waffle.testutils import override_flag +from django.contrib.sessions.middleware import SessionMiddleware import logging @@ -364,3 +365,47 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + + @less_console_noise_decorator + def test_portfolio_in_session_when_multiple_portfolios_active(self): + """When multiple_portfolios flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file From 01f05c7fde2a9cd1b6f80a6b5e01cab476aef58c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:42:36 -0400 Subject: [PATCH 058/101] test registrar_middleware for multiple_portfolios --- src/registrar/tests/test_views_portfolio.py | 77 ++++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 0104eefb8..3667b1460 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -363,6 +363,69 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): + """When organization_feature flag is false and user has a portfolio, + the portfolio should be set to None in session. + This test also satisfies the conidition when multiple_portfolios flag + is false and user has a portfolio, so won't add a redundant test for that.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): + """When organization_feature flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" + self.client.force_login(self.user) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, @@ -386,15 +449,11 @@ class TestPortfolio(WebTest): assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." @less_console_noise_decorator - def test_portfolio_in_session_when_organization_feature_active(self): - """When organization_feature flag is true and user has a portfolio, - the portfolio should be set in session.""" + def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): + """When multiple_portfolios flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" self.client.force_login(self.user) - portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) - with override_flag("organization_feature", active=True): + with override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -405,4 +464,4 @@ class TestPortfolio(WebTest): # Check if the 'portfolio' session variable exists assert 'portfolio' in session, "Portfolio session variable should exist." # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file + self.assertIsNone(session['portfolio']) From 035b4206f3f67a829287a6f707464527738f0477 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:07:52 -0600 Subject: [PATCH 059/101] Add test + fix perms --- src/registrar/tests/test_models.py | 65 +++++++++++++++++++++++++++ src/registrar/views/utility/mixins.py | 10 +++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..22758d4f6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1187,6 +1187,71 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) +class TestUserPortfolioPermission(TestCase): + @less_console_noise_decorator + def setUp(self): + self.user, _ = User.objects.get_or_create(email="mayor@igorville.gov") + super().setUp() + + def tearDown(self): + super().tearDown() + Domain.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + UserPortfolioPermission.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=True) + def test_clean_on_multiple_portfolios_when_flag_active(self): + """Ensures that a user can create multiple portfolio permission objects when the flag is enabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # Clean should pass on both of these objects + try: + portfolio_permission.clean() + portfolio_permission_2.clean() + except ValidationError as error: + self.fail(f"Raised ValidationError unexpectedly: {error}") + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=False) + def test_clean_on_creates_multiple_portfolios(self): + """Ensures that a user cannot create multiple portfolio permission objects when the flag is disabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # This should work as intended + portfolio_permission.clean() + + # Test if the ValidationError is raised with the correct message + with self.assertRaises(ValidationError) as cm: + portfolio_permission_2.clean() + + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + + self.assertEqual( + cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + ) + + class TestUser(TestCase): """Test actions that occur on user login, test class method that controls how users get validated.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index d1edfc46e..643405262 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,10 +432,11 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domains_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -448,7 +449,8 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() From 9e04e9f5bac29b02730ab0112b562427a92ec803 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:08:40 -0600 Subject: [PATCH 060/101] typo --- src/registrar/views/utility/mixins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 643405262..6f0745f41 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,7 +432,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domains_portfolio_permission(portfolio): return False @@ -449,7 +449,7 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False From 2fc8acb614f94e13c227a85949566b9207a6b520 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 14:02:03 -0600 Subject: [PATCH 061/101] Update urls.py --- src/registrar/config/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 413449896..882a71a6c 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -66,7 +66,7 @@ urlpatterns = [ name="domains", ), path( - "no-organization-domains/", + "domains/no-organization/", views.PortfolioNoDomainsView.as_view(), name="no-portfolio-domains", ), From ea5207f01741c4b94ebd4a597a4a83a32de2b50b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 14:07:13 -0600 Subject: [PATCH 062/101] Revert url (breaking tests) --- src/registrar/config/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 882a71a6c..413449896 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -66,7 +66,7 @@ urlpatterns = [ name="domains", ), path( - "domains/no-organization/", + "no-organization-domains/", views.PortfolioNoDomainsView.as_view(), name="no-portfolio-domains", ), From 4dadb715e84a2638356eed8687969bc61f4ebf4a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:03:28 -0600 Subject: [PATCH 063/101] Fix merge conflict (pt 1) --- src/registrar/tests/test_views_portfolio.py | 31 ++++++++++----------- src/registrar/views/portfolios.py | 14 ++++++---- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c87a2f3c8..f9501aa14 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user +from .common import create_test_user, get_wsgi_request_object from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -502,14 +502,12 @@ class TestPortfolio(WebTest): # A default organization member should not be able to see any domains self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - self.user.save() - self.user.refresh_from_db() - - self.assertFalse(self.user.has_domains_portfolio_permission()) + permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) response = self.app.get(reverse("no-portfolio-domains")) + self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "You aren’t managing any domains.") @@ -518,25 +516,24 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") + permission.delete() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 436d1b913..e317d5089 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -5,6 +5,7 @@ from django.urls import reverse from django.contrib import messages from registrar.forms.portfolio import PortfolioOrgAddressForm, PortfolioSeniorOfficialForm from registrar.models import Portfolio, User +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, @@ -55,14 +56,17 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): """Add additional context data to the template.""" # We can override the base class. This view only needs this item. context = {} - portfolio = self.request.user.portfolio if self.request and self.request.user else None + portfolio = self.request.session.get("portfolio") if portfolio: - context["portfolio_administrators"] = User.objects.filter( + admin_ids = UserPortfolioPermission.objects.filter( portfolio=portfolio, - portfolio_roles__overlap=[ + roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ], - ) + ] + ).values_list("user__id", flat=True) + + admin_users = User.objects.filter(id__in=admin_ids) + context["portfolio_administrators"] = admin_users return context From 0858a9663f11c1f4c4f5c1dd8d71f1a160944043 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:08:09 -0400 Subject: [PATCH 064/101] added more tests --- src/registrar/tests/test_models.py | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..e5ce01599 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1186,6 +1186,48 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(len(roles), 1) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations(self): + """Retrieve user portfolio invitations when there are multiple and multiple_options flag true.""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + with override_flag("multiple_portfolios", active=True): + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 2) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations_when_multiple_portfolios_inactive(self): + """Attempt to retrieve user portfolio invitations when there are multiple + but multiple_portfolios flag set to False""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 1) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + class TestUser(TestCase): """Test actions that occur on user login, From e866bcb2fbbf6d6dcd95e2456497d203cc81079f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:12:37 -0600 Subject: [PATCH 065/101] Fix test (still needs refinement) --- src/registrar/tests/test_views_portfolio.py | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..3a973b7c7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -500,19 +500,20 @@ class TestPortfolio(WebTest): if they do not have the right permissions. """ - # A default organization member should not be able to see any domains - self.app.set_user(self.user.username) permission, _ = UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) - response = self.app.get(reverse("no-portfolio-domains")) - self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + # A default organization member should not be able to see any domains + self.client.force_login(self.user) + response = self.client.get(reverse("home"), follow=True) + + self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren’t managing any domains.") + self.assertContains(response, "You aren't managing any domains.") # Test the domains page - this user should not have access - response = self.app.get(reverse("domains"), expect_errors=True) + response = self.client.get(reverse("domains")) self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions @@ -521,19 +522,19 @@ class TestPortfolio(WebTest): permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] permission.save() permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() From ff86edbc3538c57c99b1e770dbeb5035c6f7491f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:18:00 -0400 Subject: [PATCH 066/101] more updates to test and for linting --- src/registrar/tests/test_models.py | 13 +++++--- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/tests/test_views_portfolio.py | 36 +++++++++------------ src/registrar/views/portfolios.py | 2 +- 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 6c67aa31f..227548b61 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1202,7 +1202,9 @@ class TestPortfolioInvitations(TestCase): self.user.refresh_from_db() roles = UserPortfolioPermission.objects.filter(user=self.user) self.assertEqual(len(roles), 2) - updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create( + email=self.email, portfolio=self.portfolio + ) self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1244,7 +1246,7 @@ class TestUserPortfolioPermission(TestCase): Portfolio.objects.all().delete() User.objects.all().delete() UserDomainRole.objects.all().delete() - + @less_console_noise_decorator @override_flag("multiple_portfolios", active=True) def test_clean_on_multiple_portfolios_when_flag_active(self): @@ -1279,18 +1281,19 @@ class TestUserPortfolioPermission(TestCase): portfolio_permission_2 = UserPortfolioPermission( portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - + # This should work as intended portfolio_permission.clean() # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: portfolio_permission_2.clean() - + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) self.assertEqual( - cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + cm.exception.message, + "Only one portfolio permission is allowed per user when multiple portfolios are disabled.", ) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index e457f1b66..5ac24fd69 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -6,7 +6,7 @@ from django.urls import reverse from django.contrib.auth import get_user_model from waffle.testutils import override_flag from api.tests.common import less_console_noise_decorator -from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..277f0520a 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user, get_wsgi_request_object +from .common import create_test_user from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -396,9 +396,7 @@ class TestPortfolio(WebTest): the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -408,9 +406,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): @@ -420,9 +418,7 @@ class TestPortfolio(WebTest): is false and user has a portfolio, so won't add a redundant test for that.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -431,9 +427,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): @@ -449,19 +445,17 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) - + self.assertIsNone(session["portfolio"]) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -471,9 +465,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): @@ -489,9 +483,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index e317d5089..0232b50d7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -62,7 +62,7 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): portfolio=portfolio, roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ] + ], ).values_list("user__id", flat=True) admin_users = User.objects.filter(id__in=admin_ids) From 833b698bf28655a71e1eaf4ad209dc1b9973eac4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:52:02 -0400 Subject: [PATCH 067/101] adding request to a few flag_is_active instances --- src/registrar/models/user.py | 7 ++++++- src/registrar/models/user_portfolio_permission.py | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 6aed8195c..03d11e5bd 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -3,6 +3,7 @@ import logging from django.contrib.auth.models import AbstractUser from django.db import models from django.db.models import Q +from django.http import HttpRequest from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices @@ -358,8 +359,12 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): + # need to create a bogus request and assign user to it, in order to pass request + # to flag_is_active + request = HttpRequest() + request.user = self only_single_portfolio = ( - not flag_is_active(None, "multiple_portfolios") and self.get_first_portfolio() is None + not flag_is_active(request, "multiple_portfolios") and self.get_first_portfolio() is None ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index a3460a651..abbd20afb 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -1,5 +1,6 @@ from django.db import models from django.forms import ValidationError +from django.http import HttpRequest from waffle import flag_is_active from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel @@ -99,7 +100,10 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user + if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( From ee73893a20856b79d35a058bca0a9a1c06ccb192 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 21 Aug 2024 16:12:34 -0600 Subject: [PATCH 068/101] fix e-mail sent logic, fix text size in nested text area input --- src/registrar/assets/js/get-gov-admin.js | 91 ++++++++++--------- src/registrar/assets/sass/_theme/_admin.scss | 14 ++- .../admin/includes/detail_table_fieldset.html | 18 ++-- 3 files changed, 73 insertions(+), 50 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 23413ade1..d58bced42 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -509,32 +509,41 @@ function initializeWidgetOnList(list, parentId) { (function () { // Since this is an iife, these vars will be removed from memory afterwards var actionNeededReasonDropdown = document.querySelector("#id_action_needed_reason"); - var actionNeededEmail = document.querySelector("#id_action_needed_reason_email"); + + // Placeholder text (for certain "action needed" reasons that do not involve e=mails) var placeholderText = document.querySelector("#action-needed-reason-email-placeholder-text") - var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly"); + + // E-mail divs and textarea components + var actionNeededEmail = document.querySelector("#id_action_needed_reason_email") + var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly") var actionNeededEmailReadonlyTextarea = document.querySelector("#action-needed-reason-email-readonly-textarea") + + // Edit e-mail button (appears only in readonly view for e-mail text) var editEmailButton = document.querySelector("#action-needed-reason-email-edit-button") + // Edit e-mail modal (and its confirmation button) var actionNeededEmailAlreadySentModal = document.querySelector("#email-already-sent-modal") var confirmEditEmailButton = document.querySelector("#email-already-sent-modal_continue-editing-button") + // Headers and footers (which change depending on if the e-mail was sent or not) var actionNeededEmailHeader = document.querySelector("#action-needed-email-header") var actionNeededEmailHeaderOnSave = document.querySelector("#action-needed-email-header-email-sent") var actionNeededEmailFooter = document.querySelector("#action-needed-email-footer") let emailWasSent = document.getElementById("action-needed-email-sent"); + let lastSentEmailText = document.getElementById("action-needed-email-last-sent-text"); + // Get the list of e-mails associated with each action-needed dropdown value let emailData = document.getElementById('action-needed-emails-data'); if (!emailData) { return; } - let actionNeededEmailData = emailData.textContent; if(!actionNeededEmailData) { return; } - let actionNeededEmailsJson = JSON.parse(actionNeededEmailData); + const domainRequestId = actionNeededReasonDropdown ? document.querySelector("#domain_request_id").value : null const emailSentSessionVariableName = `actionNeededEmailSent-${domainRequestId}`; const oldDropdownValue = actionNeededReasonDropdown ? actionNeededReasonDropdown.value : null; @@ -544,22 +553,19 @@ function initializeWidgetOnList(list, parentId) { // Add a change listener to dom load document.addEventListener('DOMContentLoaded', function() { let reason = actionNeededReasonDropdown.value; - let emailBody = reason in actionNeededEmailsJson ? actionNeededEmailsJson[reason] : null; // Handle the session boolean (to enable/disable editing) if (emailWasSent && emailWasSent.value === "True") { // An email was sent out - store that information in a session variable addOrRemoveSessionBoolean(emailSentSessionVariableName, add=true); } - + // Show an editable email field or a readonly one - updateActionNeededEmailDisplay(reason, emailBody) + updateActionNeededEmailDisplay(reason) }); editEmailButton.addEventListener("click", function() { - let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; - - if (emailHasBeenSentBefore) { + if (checkEmailAlreadySent()) { // Show warning Modal setModalVisibility(actionNeededEmailAlreadySentModal, true) } @@ -590,25 +596,41 @@ function initializeWidgetOnList(list, parentId) { if (reason && emailBody) { // Reset the session object on change since change refreshes the email content. if (oldDropdownValue !== actionNeededReasonDropdown.value || oldEmailValue !== actionNeededEmail.value) { - let emailSent = sessionStorage.getItem(emailSentSessionVariableName) - if (emailSent !== null){ - addOrRemoveSessionBoolean(emailSentSessionVariableName, add=false) - } + // Replace the email content + actionNeededEmail.value = emailBody; + actionNeededEmailReadonlyTextarea.value = emailBody; + hideEmailAlreadySentView(); } } // Show either a preview of the email or some text describing no email will be sent - updateActionNeededEmailDisplay(reason, emailBody) + updateActionNeededEmailDisplay(reason) }); + } - const saveButton = document.querySelector('input[name=_save]'); - // The button does not exist if no fields are editable. - if (saveButton) { - saveButton.addEventListener('click', function(event) { - // Show readonly view with messaging to indicate email was sent - showEmailAlreadySentView() - }); - } + function checkEmailAlreadySent() + { + lastEmailSent = lastSentEmailText.value.replace(/\s+/g, '') + currentEmailInTextArea = actionNeededEmail.value.replace(/\s+/g, '') + console.log("old email value = " + lastEmailSent) + console.log("current email value = " + currentEmailInTextArea) + return lastEmailSent === currentEmailInTextArea + } + + // Shows a readonly preview of the email with updated messaging to indicate this email was sent + function showEmailAlreadySentView() + { + hideElement(actionNeededEmailHeader) + showElement(actionNeededEmailHeaderOnSave) + actionNeededEmailFooter.innerHTML = "This email has been sent to the creator of this request"; + } + + // Shows a readonly preview of the email with updated messaging to indicate this email was sent + function hideEmailAlreadySentView() + { + showElement(actionNeededEmailHeader) + hideElement(actionNeededEmailHeaderOnSave) + actionNeededEmailFooter.innerHTML = "This email will be sent to the creator of this request after saving"; } function setModalVisibility(modalToToggle, isVisible) { @@ -623,15 +645,7 @@ function initializeWidgetOnList(list, parentId) { // Shows either a preview of the email or some text describing no email will be sent. // If the email doesn't exist or if we're of reason "other", display that no email was sent. - function updateActionNeededEmailDisplay(reason, emailBody) { - - if (reason && emailBody) { - // Replace the email content - actionNeededEmail.value = emailBody; - actionNeededEmailReadonlyTextarea.value = emailBody; - } - - actionNeededEmailFooter.innerHTML = "This email will be sent to the creator of this request after saving"; + function updateActionNeededEmailDisplay(reason) { hideElement(actionNeededEmail.parentElement) if (reason) { @@ -642,6 +656,10 @@ function initializeWidgetOnList(list, parentId) { else { // Always show readonly view of email to start showEmail(canEdit=false) + if(checkEmailAlreadySent()) + { + showEmailAlreadySentView(); + } } } else { // Hide email preview and show this text instead @@ -649,15 +667,6 @@ function initializeWidgetOnList(list, parentId) { } } - // Shows a readonly preview of the email with updated messaging to indicate this email was sent - function showEmailAlreadySentView() - { - showEmail(canEdit=false) - hideElement(actionNeededEmailHeader) - showElement(actionNeededEmailHeaderOnSave) - actionNeededEmailFooter.innerHTML = "This email has been sent to the creator of this request"; - } - // Shows either a readonly view (canEdit=false) or editable view (canEdit=true) of the action needed email function showEmail(canEdit) { diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index db1741f97..f7d1e5788 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -66,6 +66,9 @@ html[data-theme="light"] { // --object-tools-fg: var(--button-fg); // --object-tools-bg: var(--close-button-bg); // --object-tools-hover-bg: var(--close-button-hover-bg); + + --summary-box-bg: #f1f1f1; + --summary-box-border: #d1d2d2; } // Fold dark theme settings into our main CSS @@ -104,6 +107,9 @@ html[data-theme="light"] { --close-button-bg: #333333; --close-button-hover-bg: #666666; + + --summary-box-bg: #121212; + --summary-box-border: #666666; } // Dark mode django (bug due to scss cascade) and USWDS tables @@ -857,10 +863,12 @@ div.dja__model-description{ } .usa-summary-box_admin { - border-color: #d1d2d2; - background-color: #f1f1f1; - max-width: fit-content !important; + color: var(--body-fg); + border-color: var(--summary-box-border); + background-color: var(--summary-box-bg); + min-width: fit-content; padding: .5rem; + border-radius: .25rem; } .text-faded { 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 bf4323002..087bacda6 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -150,7 +150,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) -
- {% else %} {{ field.field }} From 76fef5303b1e24460fb1a282fe7662b4c38f5df6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 18:41:14 -0400 Subject: [PATCH 069/101] removed pseudo apostrophe --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 01cd84743..42cf82fa4 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -504,7 +504,7 @@ class TestPortfolio(WebTest): self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren't managing any domains.") + self.assertContains(response, "You aren") # Test the domains page - this user should not have access response = self.client.get(reverse("domains")) From bef49a85c16de5fc9f2640a9c2ee3f1c37e25478 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 08:33:03 -0600 Subject: [PATCH 070/101] Cleanup --- .../models/user_portfolio_permission.py | 10 +++++----- src/registrar/tests/test_models.py | 16 ---------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index abbd20afb..c2cdc61ad 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -100,12 +100,12 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - # Have to create a bogus request to set the user and pass to flag_is_active - request = HttpRequest() - request.user = self.user - if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: + if self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) - if existing_permissions.exists(): + if not flag_is_active(request, "multiple_portfolios") and existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 227548b61..9f55fced1 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1483,10 +1483,6 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1499,10 +1495,6 @@ class TestUser(TestCase): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1513,10 +1505,6 @@ class TestUser(TestCase): portfolio_permission.save() portfolio_permission.refresh_from_db() - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1525,10 +1513,6 @@ class TestUser(TestCase): UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) From e59d4738e388ea2bad4a73ab99cdee68bbad475c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 16:44:25 -0400 Subject: [PATCH 071/101] filtering by portfolios --- src/registrar/admin.py | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..4be9c375d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -34,6 +34,7 @@ from django_fsm import TransitionNotAllowed # type: ignore from django.utils.safestring import mark_safe from django.utils.html import escape from django.contrib.auth.forms import UserChangeForm, UsernameField +from django.contrib.admin.views.main import ChangeList, IGNORED_PARAMS from django_admin_multiple_choice_list_filter.list_filters import MultipleChoiceListFilter from import_export import resources from import_export.admin import ImportExportModelAdmin @@ -45,6 +46,27 @@ from django.utils.translation import gettext_lazy as _ logger = logging.getLogger(__name__) +class CustomChangeListForPortfolioFiltering(ChangeList): + """CustomChangeList so that portfolio can be passed in a url, but not appear + in the list of filters on the right side of the page.""" + + def get_filters_params(self, params=None): + """ + Return all params except IGNORED_PARAMS. + """ + params = params or self.params + lookup_params = params.copy() # a dictionary of the query string + # Remove all the parameters that are globally and systematically + # ignored. + # Remove portfolio so that it does not error as an invalid + # filter parameter. + ignored_params = list(IGNORED_PARAMS) + ['portfolio'] + for ignored in ignored_params: + if ignored in lookup_params: + del lookup_params[ignored] + return lookup_params + + class FsmModelResource(resources.ModelResource): """ModelResource is extended to support importing of tables which have FSMFields. ModelResource is extended with the following changes @@ -644,6 +666,19 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): ) except models.User.DoesNotExist: pass + elif parameter_name == "portfolio": + # Retrieves the corresponding portfolio from Portfolio + id_value = request.GET.get(param) + try: + portfolio = models.Portfolio.objects.get(id=id_value) + filters.append( + { + "parameter_name": "portfolio", + "parameter_value": portfolio.organization_name, + } + ) + except models.Portfolio.DoesNotExist: + pass else: # For other parameter names, append a dictionary with the original # parameter_name and the corresponding parameter_value @@ -2235,6 +2270,23 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # objects rather than Contact objects. use_sort = db_field.name != "senior_official" return super().formfield_for_foreignkey(db_field, request, use_admin_sort_fields=use_sort, **kwargs) + + def get_queryset(self, request): + """Custom get_queryset to filter by portfolio if portfolio is in the + request params.""" + qs = super().get_queryset(request) + # Check if a 'portfolio' parameter is passed in the request + portfolio_id = request.GET.get('portfolio') + if portfolio_id: + # Further filter the queryset by the portfolio + qs = qs.filter(DomainRequest_info__portfolio=portfolio_id) + return qs + + def get_changelist(self, request, **kwargs): + """ + Return the ChangeList class for use on the changelist page. + """ + return CustomChangeListForPortfolioFiltering class TransitionDomainAdmin(ListHeaderAdmin): @@ -2688,6 +2740,23 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ): return True return super().has_change_permission(request, obj) + + def get_queryset(self, request): + """Custom get_queryset to filter by portfolio if portfolio is in the + request params.""" + qs = super().get_queryset(request) + # Check if a 'portfolio' parameter is passed in the request + portfolio_id = request.GET.get('portfolio') + if portfolio_id: + # Further filter the queryset by the portfolio + qs = qs.filter(domain_info__portfolio=portfolio_id) + return qs + + def get_changelist(self, request, **kwargs): + """ + Return the ChangeList class for use on the changelist page. + """ + return CustomChangeListForPortfolioFiltering class DraftDomainResource(resources.ModelResource): From 6286ae96be749deffd0176eaa180f9f61a2365c6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 17:24:33 -0400 Subject: [PATCH 072/101] updated portfolio changeform view in admin --- src/registrar/admin.py | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4be9c375d..1e20523cc 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2279,7 +2279,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): portfolio_id = request.GET.get('portfolio') if portfolio_id: # Further filter the queryset by the portfolio - qs = qs.filter(DomainRequest_info__portfolio=portfolio_id) + qs = qs.filter(portfolio=portfolio_id) return qs def get_changelist(self, request, **kwargs): @@ -2942,7 +2942,7 @@ class PortfolioAdmin(ListHeaderAdmin): # "classes": ("collapse", "closed"), # "fields": ["administrators", "members"]} # ), - ("Portfolio domains", {"classes": ("collapse", "closed"), "fields": ["domains", "domain_requests"]}), + ("Portfolio domains", {"fields": ["domains", "domain_requests"]}), ("Type of organization", {"fields": ["organization_type", "federal_type"]}), ( "Organization name and mailing address", @@ -3022,21 +3022,30 @@ class PortfolioAdmin(ListHeaderAdmin): return self.get_field_links_as_list(queryset, "suborganization") suborganizations.short_description = "Suborganizations" # type: ignore - + def domains(self, obj: models.Portfolio): - """Returns a list of links for each related domain""" - queryset = obj.get_domains() - return self.get_field_links_as_list( - queryset, "domaininformation", link_info_attribute="get_state_display_of_domain" - ) + """Returns the count of domains with a link to view them in the admin.""" + domain_count = obj.get_domains().count() # Count the related domains + if domain_count > 0: + # Construct the URL to the admin page, filtered by portfolio + url = reverse("admin:registrar_domain_changelist") + f"?portfolio={obj.id}" + label = "Domain" if domain_count == 1 else "Domains" + # Create a clickable link with the domain count + return format_html('{} {}', url, domain_count, label) + return "No Domains" domains.short_description = "Domains" # type: ignore def domain_requests(self, obj: models.Portfolio): - """Returns a list of links for each related domain request""" - queryset = obj.get_domain_requests() - return self.get_field_links_as_list(queryset, "domainrequest", link_info_attribute="get_status_display") - + """Returns the count of domain requests with a link to view them in the admin.""" + domain_request_count = obj.get_domain_requests().count() # Count the related domain requests + if domain_request_count > 0: + # Construct the URL to the admin page, filtered by portfolio + url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}" + # Create a clickable link with the domain request count + return format_html('{} Domain Requests', url, domain_request_count) + return "No Domain Requests" + domain_requests.short_description = "Domain requests" # type: ignore # Creates select2 fields (with search bars) From 62ac4757ec1b0f41b05d87d4d5cd2e3baa503f40 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 16:25:24 -0500 Subject: [PATCH 073/101] review changes --- src/registrar/assets/js/get-gov.js | 4 +-- .../commands/populate_domain_request_dates.py | 36 +++++++++---------- ...0119_add_domainrequest_submission_dates.py | 2 +- src/registrar/models/domain_request.py | 8 ++--- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index f3b41eb51..70659b009 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1599,7 +1599,7 @@ document.addEventListener('DOMContentLoaded', function() { const domainName = request.requested_domain ? request.requested_domain : `New domain request
(${utcDateString(request.created_at)})`; const actionUrl = request.action_url; const actionLabel = request.action_label; - const submissionDate = request.submission_date ? new Date(request.submission_date).toLocaleDateString('en-US', options) : `Not submitted`; + const submissionDate = request.last_submitted_date ? new Date(request.last_submitted_date).toLocaleDateString('en-US', options) : `Not submitted`; // Even if the request is not deletable, we may need this empty string for the td if the deletable column is displayed let modalTrigger = ''; @@ -1699,7 +1699,7 @@ document.addEventListener('DOMContentLoaded', function() { ${domainName} - + ${submissionDate} diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 90fc06dcf..19d8e4f00 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -17,26 +17,26 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - try: - # Retrieve and order audit log entries by timestamp in descending order - audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") + + # Retrieve and order audit log entries by timestamp in descending order + audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") - # Loop through logs in descending order to find most recent status change - for log_entry in audit_log_entries: - if "status" in log_entry.changes: - record.last_status_update = log_entry.timestamp.date() - break - - # Loop through logs in ascending order to find first submission - for log_entry in audit_log_entries.reverse(): - if log_entry.changes_dict['status'](1) == 'Submitted': - record.first_submitted_date = log_entry.timestamp.date() + # Loop through logs in descending order to find most recent status change + for log_entry in audit_log_entries: + if 'status' in LogEntry.changes_dict: + record.last_status_update = log_entry.timestamp.date() + break - except ObjectDoesNotExist as e: - logger.error(f"Object with object_pk {record.pk} does not exist: {e}") - except Exception as e: - logger.error(f"An error occurred during update_record: {e}") + # Loop through logs in ascending order to find first submission + for log_entry in audit_log_entries.reverse(): + if log_entry.changes_dict['status'](1) == 'Submitted': + record.first_submitted_date = log_entry.timestamp.date() + break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.OKCYAN}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) + + def should_skip_record(self, record) -> bool: + # make sure the record had some kind of history + return LogEntry.objects.filter(object_pk=record.pk).exists() diff --git a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py index 0b94e1257..ea209626e 100644 --- a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -35,7 +35,7 @@ class Migration(migrations.Migration): field=models.DateField( blank=True, default=None, - help_text="Date of last status updated", + help_text="Date of the last status update", null=True, verbose_name="last updated on", ), diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 74d275d95..09f200793 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -563,7 +563,7 @@ class DomainRequest(TimeStampedModel): help_text="Acknowledged .gov acceptable use policy", ) - # initial submission date records when domain request was first submitted + # Records when the domain request was first submitted first_submitted_date = models.DateField( null=True, blank=True, @@ -572,7 +572,7 @@ class DomainRequest(TimeStampedModel): help_text="Date initially submitted", ) - # last submission date records when domain request was last submitted + # Records when domain request was last submitted last_submitted_date = models.DateField( null=True, blank=True, @@ -581,13 +581,13 @@ class DomainRequest(TimeStampedModel): help_text="Date last submitted", ) - # last status update records when domain request status was last updated by an admin or analyst + # Records when domain request status was last updated by an admin or analyst last_status_update = models.DateField( null=True, blank=True, default=None, verbose_name="last updated on", - help_text="Date of last status updated", + help_text="Date of the last status update", ) notes = models.TextField( null=True, From 4a104b6ff6afbbe1870f630205af5fb66f557611 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 17:25:52 -0400 Subject: [PATCH 074/101] formatted for linter --- src/registrar/admin.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 1e20523cc..a49536102 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -60,7 +60,7 @@ class CustomChangeListForPortfolioFiltering(ChangeList): # ignored. # Remove portfolio so that it does not error as an invalid # filter parameter. - ignored_params = list(IGNORED_PARAMS) + ['portfolio'] + ignored_params = list(IGNORED_PARAMS) + ["portfolio"] for ignored in ignored_params: if ignored in lookup_params: del lookup_params[ignored] @@ -2270,18 +2270,18 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # objects rather than Contact objects. use_sort = db_field.name != "senior_official" return super().formfield_for_foreignkey(db_field, request, use_admin_sort_fields=use_sort, **kwargs) - + def get_queryset(self, request): """Custom get_queryset to filter by portfolio if portfolio is in the request params.""" qs = super().get_queryset(request) # Check if a 'portfolio' parameter is passed in the request - portfolio_id = request.GET.get('portfolio') + portfolio_id = request.GET.get("portfolio") if portfolio_id: # Further filter the queryset by the portfolio qs = qs.filter(portfolio=portfolio_id) return qs - + def get_changelist(self, request, **kwargs): """ Return the ChangeList class for use on the changelist page. @@ -2740,18 +2740,18 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ): return True return super().has_change_permission(request, obj) - + def get_queryset(self, request): """Custom get_queryset to filter by portfolio if portfolio is in the request params.""" qs = super().get_queryset(request) # Check if a 'portfolio' parameter is passed in the request - portfolio_id = request.GET.get('portfolio') + portfolio_id = request.GET.get("portfolio") if portfolio_id: # Further filter the queryset by the portfolio qs = qs.filter(domain_info__portfolio=portfolio_id) return qs - + def get_changelist(self, request, **kwargs): """ Return the ChangeList class for use on the changelist page. @@ -3022,7 +3022,7 @@ class PortfolioAdmin(ListHeaderAdmin): return self.get_field_links_as_list(queryset, "suborganization") suborganizations.short_description = "Suborganizations" # type: ignore - + def domains(self, obj: models.Portfolio): """Returns the count of domains with a link to view them in the admin.""" domain_count = obj.get_domains().count() # Count the related domains @@ -3045,7 +3045,7 @@ class PortfolioAdmin(ListHeaderAdmin): # Create a clickable link with the domain request count return format_html('{} Domain Requests', url, domain_request_count) return "No Domain Requests" - + domain_requests.short_description = "Domain requests" # type: ignore # Creates select2 fields (with search bars) From 5da8119f86be3a8654cfabf915a4433556ad7326 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 19:02:55 -0400 Subject: [PATCH 075/101] adjusted code for correct version of django --- src/registrar/admin.py | 51 +++++++++++-------------------- src/registrar/tests/test_admin.py | 8 ++--- src/registrar/tests/test_api.py | 6 ++++ 3 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a49536102..65974b42b 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -34,7 +34,7 @@ from django_fsm import TransitionNotAllowed # type: ignore from django.utils.safestring import mark_safe from django.utils.html import escape from django.contrib.auth.forms import UserChangeForm, UsernameField -from django.contrib.admin.views.main import ChangeList, IGNORED_PARAMS +from django.contrib.admin.views.main import IGNORED_PARAMS from django_admin_multiple_choice_list_filter.list_filters import MultipleChoiceListFilter from import_export import resources from import_export.admin import ImportExportModelAdmin @@ -46,27 +46,6 @@ from django.utils.translation import gettext_lazy as _ logger = logging.getLogger(__name__) -class CustomChangeListForPortfolioFiltering(ChangeList): - """CustomChangeList so that portfolio can be passed in a url, but not appear - in the list of filters on the right side of the page.""" - - def get_filters_params(self, params=None): - """ - Return all params except IGNORED_PARAMS. - """ - params = params or self.params - lookup_params = params.copy() # a dictionary of the query string - # Remove all the parameters that are globally and systematically - # ignored. - # Remove portfolio so that it does not error as an invalid - # filter parameter. - ignored_params = list(IGNORED_PARAMS) + ["portfolio"] - for ignored in ignored_params: - if ignored in lookup_params: - del lookup_params[ignored] - return lookup_params - - class FsmModelResource(resources.ModelResource): """ModelResource is extended to support importing of tables which have FSMFields. ModelResource is extended with the following changes @@ -450,6 +429,22 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): return ordering + def get_filters_params(self, params=None): + """ + Return all params except IGNORED_PARAMS. + """ + params = params or self.params + lookup_params = params.copy() # a dictionary of the query string + # Remove all the parameters that are globally and systematically + # ignored. + # Remove portfolio so that it does not error as an invalid + # filter parameter. + ignored_params = list(IGNORED_PARAMS) + ["portfolio"] + for ignored in ignored_params: + if ignored in lookup_params: + del lookup_params[ignored] + return lookup_params + class CustomLogEntryAdmin(LogEntryAdmin): """Overwrite the generated LogEntry admin class""" @@ -2282,12 +2277,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): qs = qs.filter(portfolio=portfolio_id) return qs - def get_changelist(self, request, **kwargs): - """ - Return the ChangeList class for use on the changelist page. - """ - return CustomChangeListForPortfolioFiltering - class TransitionDomainAdmin(ListHeaderAdmin): """Custom transition domain admin class.""" @@ -2752,12 +2741,6 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): qs = qs.filter(domain_info__portfolio=portfolio_id) return qs - def get_changelist(self, request, **kwargs): - """ - Return the ChangeList class for use on the changelist page. - """ - return CustomChangeListForPortfolioFiltering - class DraftDomainResource(resources.ModelResource): """defines how each field in the referenced model should be mapped to the corresponding fields in the diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..d4404b564 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2107,9 +2107,7 @@ class TestPortfolioAdmin(TestCase): domain_2.save() domains = self.admin.domains(self.portfolio) - self.assertIn("domain1.gov", domains) - self.assertIn("domain2.gov", domains) - self.assertIn('
    ', domains) + self.assertIn("2 Domains", domains) @less_console_noise_decorator def test_domain_requests_display(self): @@ -2118,6 +2116,4 @@ class TestPortfolioAdmin(TestCase): completed_domain_request(name="request2.gov", portfolio=self.portfolio) domain_requests = self.admin.domain_requests(self.portfolio) - self.assertIn("request1.gov", domain_requests) - self.assertIn("request2.gov", domain_requests) - self.assertIn('
      ', domain_requests) + self.assertIn("2 Domain Requests", domain_requests) diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 0025bc902..de52ad3d6 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user +from api.tests.common import less_console_noise_decorator + class GetSeniorOfficialJsonTest(TestCase): def setUp(self): @@ -26,6 +28,7 @@ class GetSeniorOfficialJsonTest(TestCase): SeniorOfficial.objects.all().delete() FederalAgency.objects.all().delete() + @less_console_noise_decorator def test_get_senior_official_json_authenticated_superuser(self): """Test that a superuser can fetch the senior official information.""" p = "adminpass" @@ -38,6 +41,7 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(data["last_name"], "Doe") self.assertEqual(data["title"], "Director") + @less_console_noise_decorator def test_get_senior_official_json_authenticated_analyst(self): """Test that an analyst user can fetch the senior official's information.""" p = "userpass" @@ -50,6 +54,7 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(data["last_name"], "Doe") self.assertEqual(data["title"], "Director") + @less_console_noise_decorator def test_get_senior_official_json_unauthenticated(self): """Test that an unauthenticated user receives a 403 with an error message.""" p = "password" @@ -57,6 +62,7 @@ class GetSeniorOfficialJsonTest(TestCase): response = self.client.get(self.api_url, {"agency_name": "Test Agency"}) self.assertEqual(response.status_code, 302) + @less_console_noise_decorator def test_get_senior_official_json_not_found(self): """Test that a request for a non-existent agency returns a 404 with an error message.""" p = "adminpass" From 851e176f83209dd8e902ac79d1848650d37e2268 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 21:45:00 -0400 Subject: [PATCH 076/101] added tests and comments --- src/registrar/admin.py | 10 ++++++-- src/registrar/tests/test_admin_domain.py | 28 +++++++++++++++++++++++ src/registrar/tests/test_admin_request.py | 25 ++++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 65974b42b..385c1e60e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -367,7 +367,9 @@ class DomainRequestAdminForm(forms.ModelForm): class MultiFieldSortableChangeList(admin.views.main.ChangeList): """ This class overrides the behavior of column sorting in django admin tables in order - to allow for multi field sorting on admin_order_field + to allow for multi field sorting on admin_order_field. It also overrides behavior + of getting the filter params to allow portfolio filters to be executed without + displaying on the right side of the ChangeList view. Usage: @@ -431,7 +433,11 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): def get_filters_params(self, params=None): """ - Return all params except IGNORED_PARAMS. + Overrides the default behavior which gets filter_params, except + those in IGNORED_PARAMS. The override is to also include + portfolio in the overrides. This allows the portfolio filter + not to throw an error as a valid filter while not listing the + portfolio filter on the right side of the Change List view. """ params = params or self.params lookup_params = params.copy() # a dictionary of the query string diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index e156dd377..64d4ee77d 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/src/registrar/tests/test_admin_domain.py @@ -14,6 +14,7 @@ from registrar.models import ( DomainInformation, User, Host, + Portfolio, ) from .common import ( MockSESClient, @@ -359,6 +360,7 @@ class TestDomainAdminWithClient(TestCase): Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() + Portfolio.objects.all().delete() @classmethod def tearDownClass(self): @@ -452,6 +454,32 @@ class TestDomainAdminWithClient(TestCase): domain_request.delete() _creator.delete() + @less_console_noise_decorator + def test_domains_by_portfolio(self): + """ + Tests that domains display for a portfolio. + """ + + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) + # Create a fake domain request and domain + _domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio + ) + _domain_request.approve() + + domain = _domain_request.approved_domain + + self.client.force_login(self.superuser) + response = self.client.get( + "/admin/registrar/domain/?portfolio={}".format(portfolio.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + self.assertContains(response, portfolio.organization_name) + @less_console_noise_decorator def test_helper_text(self): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..04faf0504 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -22,6 +22,7 @@ from registrar.models import ( Contact, Website, SeniorOfficial, + Portfolio, ) from .common import ( MockSESClient, @@ -78,6 +79,7 @@ class TestDomainRequestAdmin(MockEppLib): Contact.objects.all().delete() Website.objects.all().delete() SeniorOfficial.objects.all().delete() + Portfolio.objects.all().delete() self.mock_client.EMAILS_SENT.clear() @classmethod @@ -263,6 +265,29 @@ class TestDomainRequestAdmin(MockEppLib): self.assertContains(response, domain_request.requested_domain.name) self.assertContains(response, "Show details") + @less_console_noise_decorator + def test_domain_requests_by_portfolio(self): + """ + Tests that domain_requests display for a portfolio. + """ + + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) + # Create a fake domain request and domain + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio + ) + + self.client.force_login(self.superuser) + response = self.client.get( + "/admin/registrar/domainrequest/?portfolio={}".format(portfolio.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + self.assertContains(response, portfolio.organization_name) + @less_console_noise_decorator def test_analyst_can_see_and_edit_alternative_domain(self): """Tests if an analyst can still see and edit the alternative domain field""" From 8a9971ac79bb08a934e269674a28993d5470b608 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:50:24 -0400 Subject: [PATCH 077/101] initial implementation --- src/registrar/assets/js/get-gov-admin.js | 51 +++++++++++++++++++ src/registrar/config/urls.py | 10 +++- src/registrar/models/portfolio.py | 16 ++++-- .../django/admin/portfolio_change_form.html | 2 + src/registrar/views/utility/api_views.py | 33 ++++++++++++ 5 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 01c93abf6..11e3cae69 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,27 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; + fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) + .then(response => { + const statusCode = response.status; + return response.json().then(data => ({ statusCode, data })); + }) + .then(({ statusCode, data }) => { + if (data.error) { + console.error("Error in AJAX call: " + data.error); + return; + } + + let federal_type = data.federal_type; + let portfolio_type = data.portfolio_type; + console.log("portfolio type: " + portfolio_type); + console.log("federal type: " + federal_type); + updateFederalType(data.federal_type); + updatePortfolioType(data.portfolio_type); + }) + .catch(error => console.error("Error fetching federal and portfolio types: ", error)); + let seniorOfficialApi = document.getElementById("senior_official_from_agency_json_url").value; fetch(`${seniorOfficialApi}?agency_name=${selectedText}`) .then(response => { @@ -879,6 +900,7 @@ function initializeWidgetOnList(list, parentId) { } }) .catch(error => console.error("Error fetching senior official: ", error)); + } function handleStateTerritoryChange(stateTerritory, urbanizationField) { @@ -890,6 +912,35 @@ function initializeWidgetOnList(list, parentId) { } } + function updatePortfolioType(portfolioType) { + console.log("attempting to update portfolioType"); + // Find the div with class 'field-portfolio_type' + const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); + if (portfolioTypeDiv) { + console.log("found portfoliotype"); + // Find the nested div with class 'readonly' inside 'field-portfolio_type' + const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + console.log("found readonly div"); + // Update the text content of the readonly div + readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; + } + } + } + + function updateFederalType(federalType) { + // Find the div with class 'field-federal_type' + const federalTypeDiv = document.querySelector('.field-federal_type'); + if (federalTypeDiv) { + // Find the nested div with class 'readonly' inside 'field-federal_type' + const readonlyDiv = federalTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + // Update the text content of the readonly div + readonlyDiv.textContent = federalType !== null ? federalType : '-'; + } + } + } + function updateContactInfo(data) { if (!contactList) return; diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 413449896..5e58cf740 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -24,7 +24,10 @@ from registrar.views.report_views import ( from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json -from registrar.views.utility.api_views import get_senior_official_from_federal_agency_json +from registrar.views.utility.api_views import ( + get_senior_official_from_federal_agency_json, + get_federal_and_portfolio_types_from_federal_agency_json +) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 from api.views import available, get_current_federal, get_current_full @@ -139,6 +142,11 @@ urlpatterns = [ get_senior_official_from_federal_agency_json, name="get-senior-official-from-federal-agency-json", ), + path( + "admin/api/get-federal-and-portfolio-types-from-federal-agency-json/", + get_federal_and_portfolio_types_from_federal_agency_json, + name="get-federal-and-portfolio-types-from-federal-agency-json", + ), path("admin/", admin.site.urls), path( "reports/export_data_type_user/", diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index 0f9904c31..b9b0d4a22 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -131,9 +131,13 @@ class Portfolio(TimeStampedModel): Returns a combination of organization_type / federal_type, seperated by ' - '. If no federal_type is found, we just return the org type. """ - org_type_label = self.OrganizationChoices.get_org_label(self.organization_type) - agency_type_label = BranchChoices.get_branch_label(self.federal_type) - if self.organization_type == self.OrganizationChoices.FEDERAL and agency_type_label: + return self.get_portfolio_type(self.organization_type, self.federal_type) + + @classmethod + def get_portfolio_type(cls, organization_type, federal_type): + org_type_label = cls.OrganizationChoices.get_org_label(organization_type) + agency_type_label = BranchChoices.get_branch_label(federal_type) + if organization_type == cls.OrganizationChoices.FEDERAL and agency_type_label: return " - ".join([org_type_label, agency_type_label]) else: return org_type_label @@ -141,8 +145,12 @@ class Portfolio(TimeStampedModel): @property def federal_type(self): """Returns the federal_type value on the underlying federal_agency field""" - return self.federal_agency.federal_type if self.federal_agency else None + 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""" diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 9d59aae42..4eb941340 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/src/registrar/templates/django/admin/portfolio_change_form.html @@ -5,6 +5,8 @@ {% comment %} Stores the json endpoint in a url for easier access {% endcomment %} {% url 'get-senior-official-from-federal-agency-json' as url %} + {% url 'get-federal-and-portfolio-types-from-federal-agency-json' as url %} + {{ block.super }} {% endblock content %} diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index 2c9414d1d..e67a1974c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -5,6 +5,9 @@ from registrar.models import FederalAgency, SeniorOfficial from django.contrib.admin.views.decorators import staff_member_required from django.contrib.auth.decorators import login_required +from registrar.models.portfolio import Portfolio +from registrar.utility.constants import BranchChoices + logger = logging.getLogger(__name__) @@ -34,3 +37,33 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) + +@login_required +@staff_member_required +def get_federal_and_portfolio_types_from_federal_agency_json(request): + """Returns specific portfolio information as a JSON. Request must have + both agency_name and organization_type.""" + + # This API is only accessible to admins and analysts + superuser_perm = request.user.has_perm("registrar.full_access_permission") + analyst_perm = request.user.has_perm("registrar.analyst_access_permission") + if not request.user.is_authenticated or not any([analyst_perm, superuser_perm]): + return JsonResponse({"error": "You do not have access to this resource"}, status=403) + + federal_type = None + portfolio_type = None + + agency_name = request.GET.get("agency_name") + organization_type = request.GET.get("organization_type") + agency = FederalAgency.objects.filter(agency=agency_name).first() + if agency: + federal_type = Portfolio.get_federal_type(agency) + portfolio_type = Portfolio.get_portfolio_type(organization_type, federal_type) + federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" + + response_data = { + 'portfolio_type': portfolio_type, + 'federal_type': federal_type, + } + + return JsonResponse(response_data) \ No newline at end of file From ef7739dc556b9e9c7b5c046934e4ee44a5bf733e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:52:57 -0400 Subject: [PATCH 078/101] lint --- src/registrar/config/urls.py | 2 +- src/registrar/models/portfolio.py | 2 +- src/registrar/views/utility/api_views.py | 11 ++++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 5e58cf740..19fa99809 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -26,7 +26,7 @@ from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json from registrar.views.utility.api_views import ( get_senior_official_from_federal_agency_json, - get_federal_and_portfolio_types_from_federal_agency_json + get_federal_and_portfolio_types_from_federal_agency_json, ) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index b9b0d4a22..fadcf8cac 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -150,7 +150,7 @@ class Portfolio(TimeStampedModel): @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""" diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index e67a1974c..97eb7e86c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -37,7 +37,8 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) - + + @login_required @staff_member_required def get_federal_and_portfolio_types_from_federal_agency_json(request): @@ -62,8 +63,8 @@ def get_federal_and_portfolio_types_from_federal_agency_json(request): federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" response_data = { - 'portfolio_type': portfolio_type, - 'federal_type': federal_type, + "portfolio_type": portfolio_type, + "federal_type": federal_type, } - - return JsonResponse(response_data) \ No newline at end of file + + return JsonResponse(response_data) From 31285da3956043a0a924ef4ea2a30d785a6a1c1a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 18:16:01 -0400 Subject: [PATCH 079/101] added test code --- src/registrar/tests/test_api.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 0025bc902..bd96f3e24 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user +from api.tests.common import less_console_noise_decorator + class GetSeniorOfficialJsonTest(TestCase): def setUp(self): @@ -65,3 +67,32 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(response.status_code, 404) data = response.json() self.assertEqual(data["error"], "Senior Official not found") + + +class GetFederalPortfolioTypeJsonTest(TestCase): + def setUp(self): + self.client = Client() + p = "password" + self.user = get_user_model().objects.create_user(username="testuser", password=p) + + self.superuser = create_superuser() + self.analyst_user = create_user() + + self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type="judicial") + + self.api_url = reverse("get-federal-and-portfolio-types-from-federal-agency-json") + + def tearDown(self): + User.objects.all().delete() + FederalAgency.objects.all().delete() + + @less_console_noise_decorator + def test_get_federal_and_portfolio_types_json_authenticated_superuser(self): + """Test that a superuser can fetch the federal and portfolio types.""" + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["federal_type"], "Judicial") + self.assertEqual(data["portfolio_type"], "Federal - Judicial") From 4cee98edee40a611dd8d770c60fe22011115bfbd Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 22:39:49 -0400 Subject: [PATCH 080/101] A bit of cleanup --- src/registrar/assets/js/get-gov-admin.js | 29 +---------- .../admin/includes/detail_table_fieldset.html | 51 ++++++++++--------- 2 files changed, 27 insertions(+), 53 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index d58bced42..c05ef090c 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -518,9 +518,6 @@ function initializeWidgetOnList(list, parentId) { var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly") var actionNeededEmailReadonlyTextarea = document.querySelector("#action-needed-reason-email-readonly-textarea") - // Edit e-mail button (appears only in readonly view for e-mail text) - var editEmailButton = document.querySelector("#action-needed-reason-email-edit-button") - // Edit e-mail modal (and its confirmation button) var actionNeededEmailAlreadySentModal = document.querySelector("#email-already-sent-modal") var confirmEditEmailButton = document.querySelector("#email-already-sent-modal_continue-editing-button") @@ -565,12 +562,7 @@ function initializeWidgetOnList(list, parentId) { }); editEmailButton.addEventListener("click", function() { - if (checkEmailAlreadySent()) { - // Show warning Modal - setModalVisibility(actionNeededEmailAlreadySentModal, true) - } - else { - // Show editable view + if (!checkEmailAlreadySent()) { showEmail(canEdit=true) } }); @@ -580,13 +572,6 @@ function initializeWidgetOnList(list, parentId) { showEmail(canEdit=true) }); - // Event delegation for data-close-modal buttons - // (since manually opening the modal interferes with how this normally works) - document.addEventListener('click', function(event) { - if (event.target.matches('[data-close-modal]')) { - setModalVisibility(actionNeededEmailAlreadySentModal, false) - } - }); // Add a change listener to the action needed reason dropdown actionNeededReasonDropdown.addEventListener("change", function() { @@ -612,8 +597,6 @@ function initializeWidgetOnList(list, parentId) { { lastEmailSent = lastSentEmailText.value.replace(/\s+/g, '') currentEmailInTextArea = actionNeededEmail.value.replace(/\s+/g, '') - console.log("old email value = " + lastEmailSent) - console.log("current email value = " + currentEmailInTextArea) return lastEmailSent === currentEmailInTextArea } @@ -633,16 +616,6 @@ function initializeWidgetOnList(list, parentId) { actionNeededEmailFooter.innerHTML = "This email will be sent to the creator of this request after saving"; } - function setModalVisibility(modalToToggle, isVisible) { - if (!isVisible) { - modalToToggle.classList.remove('is-visible'); - modalToToggle.setAttribute('aria-hidden', 'true'); - } else { - modalToToggle.classList.add('is-visible'); - modalToToggle.setAttribute('aria-hidden', 'false'); - } - } - // Shows either a preview of the email or some text describing no email will be sent. // If the email doesn't exist or if we're of reason "other", display that no email was sent. function updateActionNeededEmailDisplay(reason) { 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 087bacda6..7819e3aff 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -151,12 +151,31 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
-
From 69a3db5f59af1a8eb83b40643d6b4ff3e985babb Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 22:54:47 -0400 Subject: [PATCH 082/101] accessibility fix --- .../templates/django/admin/includes/detail_table_fieldset.html | 3 ++- 1 file changed, 2 insertions(+), 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 8cfb31064..3b4047d39 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -232,7 +232,8 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
-
From 6091ff76c3ad71aaaa130842d5cdbdf0c24dd21c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 11:42:10 -0400 Subject: [PATCH 083/101] cleanup --- src/registrar/assets/js/get-gov-admin.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 11e3cae69..98a2395db 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,8 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + // Determine if any changes are necessary to the display of portfolio type or federal type + // based on changes to the Federal Agency let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) .then(response => { @@ -851,8 +853,6 @@ function initializeWidgetOnList(list, parentId) { let federal_type = data.federal_type; let portfolio_type = data.portfolio_type; - console.log("portfolio type: " + portfolio_type); - console.log("federal type: " + federal_type); updateFederalType(data.federal_type); updatePortfolioType(data.portfolio_type); }) @@ -912,22 +912,25 @@ function initializeWidgetOnList(list, parentId) { } } + /** + * Dynamically update the portfolio type text in the dom to portfolioType + */ function updatePortfolioType(portfolioType) { - console.log("attempting to update portfolioType"); // Find the div with class 'field-portfolio_type' const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); if (portfolioTypeDiv) { - console.log("found portfoliotype"); // Find the nested div with class 'readonly' inside 'field-portfolio_type' const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); if (readonlyDiv) { - console.log("found readonly div"); // Update the text content of the readonly div readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; } } } + /** + * Dynamically update the federal type text in the dom to federalType + */ function updateFederalType(federalType) { // Find the div with class 'field-federal_type' const federalTypeDiv = document.querySelector('.field-federal_type'); From 497837b09882731c121adbba88110d6155253e66 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 26 Aug 2024 11:18:43 -0500 Subject: [PATCH 084/101] Update src/registrar/management/commands/populate_domain_request_dates.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- .../management/commands/populate_domain_request_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 19d8e4f00..fe471fc9d 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -34,7 +34,7 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) def should_skip_record(self, record) -> bool: From f7b2e38bba21c91cc46b39da26464800cdf72ac9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Mon, 26 Aug 2024 11:54:08 -0500 Subject: [PATCH 085/101] linter fixes --- .../commands/populate_domain_request_dates.py | 11 +++++++---- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/test_views_requests_json.py | 6 ++++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index fe471fc9d..e20e35909 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -3,7 +3,6 @@ from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import DomainRequest from auditlog.models import LogEntry -from django.core.exceptions import ObjectDoesNotExist logger = logging.getLogger(__name__) @@ -12,12 +11,13 @@ class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" + """Loops through each DomainRequest object and populates + its last_status_update and first_submitted_date values""" self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - + # Retrieve and order audit log entries by timestamp in descending order audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") @@ -34,7 +34,10 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"""{TerminalColors.OKCYAN}Updating {record} => + first submitted date: {record.first_submitted_date}, + last status update: {record.last_status_update}{TerminalColors.ENDC} + """ ) def should_skip_record(self, record) -> bool: diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 09f200793..7ee80e43a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -823,7 +823,7 @@ class DomainRequest(TimeStampedModel): if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - # if the domain has not been submitted before this must be the first time + # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index c140b7e44..fd44bdc31 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,7 +287,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -306,7 +307,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json From c2f71c984ffe13e83bb768a7bef72c167cb1fe7b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 20:34:56 -0400 Subject: [PATCH 086/101] a few improvements to code, comments, and tests --- src/registrar/admin.py | 12 +++++------- src/registrar/tests/test_admin_domain.py | 8 +++++++- src/registrar/tests/test_admin_request.py | 6 +++++- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 385c1e60e..ca7742a89 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -433,11 +433,9 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): def get_filters_params(self, params=None): """ - Overrides the default behavior which gets filter_params, except - those in IGNORED_PARAMS. The override is to also include - portfolio in the overrides. This allows the portfolio filter - not to throw an error as a valid filter while not listing the - portfolio filter on the right side of the Change List view. + Add portfolio to ignored params to allow the portfolio filter while not + listing it as a filter option on the right side of Change List on the + portfolio list. """ params = params or self.params lookup_params = params.copy() # a dictionary of the query string @@ -3018,7 +3016,7 @@ class PortfolioAdmin(ListHeaderAdmin): if domain_count > 0: # Construct the URL to the admin page, filtered by portfolio url = reverse("admin:registrar_domain_changelist") + f"?portfolio={obj.id}" - label = "Domain" if domain_count == 1 else "Domains" + label = "Domain" if domain_count == 1 else "No domains" # Create a clickable link with the domain count return format_html('{} {}', url, domain_count, label) return "No Domains" @@ -3033,7 +3031,7 @@ class PortfolioAdmin(ListHeaderAdmin): url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}" # Create a clickable link with the domain request count return format_html('{} Domain Requests', url, domain_request_count) - return "No Domain Requests" + return "No domain requests" domain_requests.short_description = "Domain requests" # type: ignore diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index 64d4ee77d..385a00800 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/src/registrar/tests/test_admin_domain.py @@ -16,6 +16,7 @@ from registrar.models import ( Host, Portfolio, ) +from registrar.models.user_domain_role import UserDomainRole from .common import ( MockSESClient, completed_domain_request, @@ -357,6 +358,7 @@ class TestDomainAdminWithClient(TestCase): def tearDown(self): super().tearDown() Host.objects.all().delete() + UserDomainRole.objects.all().delete() Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() @@ -457,7 +459,7 @@ class TestDomainAdminWithClient(TestCase): @less_console_noise_decorator def test_domains_by_portfolio(self): """ - Tests that domains display for a portfolio. + Tests that domains display for a portfolio. And that domains outside the portfolio do not display. """ portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) @@ -468,6 +470,9 @@ class TestDomainAdminWithClient(TestCase): _domain_request.approve() domain = _domain_request.approved_domain + domain2, _ = Domain.objects.get_or_create(name="fake.gov", state=Domain.State.READY) + UserDomainRole.objects.get_or_create() + UserDomainRole.objects.get_or_create(user=self.superuser, domain=domain2, role=UserDomainRole.Roles.MANAGER) self.client.force_login(self.superuser) response = self.client.get( @@ -478,6 +483,7 @@ class TestDomainAdminWithClient(TestCase): # Make sure the page loaded, and that we're on the right page self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) + self.assertNotContains(response, domain2.name) self.assertContains(response, portfolio.organization_name) @less_console_noise_decorator diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 04faf0504..b54383b09 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -268,7 +268,7 @@ class TestDomainRequestAdmin(MockEppLib): @less_console_noise_decorator def test_domain_requests_by_portfolio(self): """ - Tests that domain_requests display for a portfolio. + Tests that domain_requests display for a portfolio. And requests not in portfolio do not display. """ portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) @@ -276,6 +276,9 @@ class TestDomainRequestAdmin(MockEppLib): domain_request = completed_domain_request( status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio ) + domain_request2 = completed_domain_request( + name="testdomain2.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW + ) self.client.force_login(self.superuser) response = self.client.get( @@ -286,6 +289,7 @@ class TestDomainRequestAdmin(MockEppLib): # Make sure the page loaded, and that we're on the right page self.assertEqual(response.status_code, 200) self.assertContains(response, domain_request.requested_domain.name) + self.assertNotContains(response, domain_request2.requested_domain.name) self.assertContains(response, portfolio.organization_name) @less_console_noise_decorator From 6f4c63c9952341c21c35865909751e926237cd17 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 20:38:29 -0400 Subject: [PATCH 087/101] lint --- src/registrar/tests/test_admin_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index b54383b09..4a828fe42 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -277,7 +277,7 @@ class TestDomainRequestAdmin(MockEppLib): status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio ) domain_request2 = completed_domain_request( - name="testdomain2.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW + name="testdomain2.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW ) self.client.force_login(self.superuser) From 88086a07d82bcfe931d9198068468553b0eec0e8 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 20:43:06 -0400 Subject: [PATCH 088/101] fixed some casing and associated tests --- src/registrar/admin.py | 6 +++--- src/registrar/tests/test_admin.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ca7742a89..f0adaab76 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3016,10 +3016,10 @@ class PortfolioAdmin(ListHeaderAdmin): if domain_count > 0: # Construct the URL to the admin page, filtered by portfolio url = reverse("admin:registrar_domain_changelist") + f"?portfolio={obj.id}" - label = "Domain" if domain_count == 1 else "No domains" + label = "domain" if domain_count == 1 else "domains" # Create a clickable link with the domain count return format_html('{} {}', url, domain_count, label) - return "No Domains" + return "No domains" domains.short_description = "Domains" # type: ignore @@ -3030,7 +3030,7 @@ class PortfolioAdmin(ListHeaderAdmin): # Construct the URL to the admin page, filtered by portfolio url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}" # Create a clickable link with the domain request count - return format_html('{} Domain Requests', url, domain_request_count) + return format_html('{} domain requests', url, domain_request_count) return "No domain requests" domain_requests.short_description = "Domain requests" # type: ignore diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index d4404b564..a435c6a69 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2107,7 +2107,7 @@ class TestPortfolioAdmin(TestCase): domain_2.save() domains = self.admin.domains(self.portfolio) - self.assertIn("2 Domains", domains) + self.assertIn("2 domains", domains) @less_console_noise_decorator def test_domain_requests_display(self): @@ -2116,4 +2116,4 @@ class TestPortfolioAdmin(TestCase): completed_domain_request(name="request2.gov", portfolio=self.portfolio) domain_requests = self.admin.domain_requests(self.portfolio) - self.assertIn("2 Domain Requests", domain_requests) + self.assertIn("2 domain requests", domain_requests) From b2a464668b5c475c4a263a0f745dfe49679ceed3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 27 Aug 2024 11:44:30 -0400 Subject: [PATCH 089/101] update from merge --- src/registrar/admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 57c8cdfaf..d1ffda23a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2982,6 +2982,7 @@ class PortfolioAdmin(ListHeaderAdmin): "domain_requests", "suborganizations", "portfolio_type", + "creator", ] def federal_type(self, obj: models.Portfolio): From 89c2e4543a3d37b64306b44829c13815e06f305f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 12:27:35 -0600 Subject: [PATCH 090/101] Update src/registrar/models/user.py Co-authored-by: Matt-Spence --- src/registrar/models/user.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 03d11e5bd..a7ea1e14a 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -206,12 +206,11 @@ class User(AbstractUser): if not portfolio: return False - portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() - if not portfolio_perms: + user_portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() + if not user_portfolio_perms: return False - portfolio_permissions = portfolio_perms._get_portfolio_permissions() - return portfolio_permission in portfolio_permissions + return portfolio_permission in user_portfolio_perms._get_portfolio_permissions() def has_base_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) From 207a3ac3aa2801e1d188f28349d1ea71a95b328a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 13:31:03 -0500 Subject: [PATCH 091/101] fix populate script --- .../management/commands/populate_domain_request_dates.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index e20e35909..f7ea5ce27 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -20,16 +20,16 @@ class Command(BaseCommand, PopulateScriptTemplate): # Retrieve and order audit log entries by timestamp in descending order audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") - # Loop through logs in descending order to find most recent status change for log_entry in audit_log_entries: - if 'status' in LogEntry.changes_dict: + if 'status' in log_entry.changes_dict: record.last_status_update = log_entry.timestamp.date() break # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): - if log_entry.changes_dict['status'](1) == 'Submitted': + status = log_entry.changes_dict.get('status') + if status and status[1] == 'Submitted': record.first_submitted_date = log_entry.timestamp.date() break @@ -42,4 +42,4 @@ class Command(BaseCommand, PopulateScriptTemplate): def should_skip_record(self, record) -> bool: # make sure the record had some kind of history - return LogEntry.objects.filter(object_pk=record.pk).exists() + return not LogEntry.objects.filter(object_pk=record.pk).exists() \ No newline at end of file From 990e16246d49b25173d85e00a6f88927690da071 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 13:26:01 -0600 Subject: [PATCH 092/101] Update src/registrar/tests/test_views_portfolio.py Co-authored-by: Matt-Spence --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 42cf82fa4..c5d1a9830 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -414,7 +414,7 @@ class TestPortfolio(WebTest): def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): """When organization_feature flag is false and user has a portfolio, the portfolio should be set to None in session. - This test also satisfies the conidition when multiple_portfolios flag + This test also satisfies the condition when multiple_portfolios flag is false and user has a portfolio, so won't add a redundant test for that.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] From 1fa80e8193c2650a1296ac8479cc036af633f569 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 13:26:13 -0600 Subject: [PATCH 093/101] Update src/registrar/registrar_middleware.py Co-authored-by: Matt-Spence --- src/registrar/registrar_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index caefee650..4b590db1e 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -146,7 +146,7 @@ class CheckPortfolioMiddleware: # set the portfolio in the session if it is not set if "portfolio" not in request.session or request.session["portfolio"] is None: - # if user is a multiple portfolio + # if multiple portfolios are allowed for this user if flag_is_active(request, "multiple_portfolios"): # NOTE: we will want to change later to have a workflow for selecting # portfolio and another for switching portfolio; for now, select first From 9e592cbd753052c6f5df68d5120bf732f6c43e25 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:11:43 -0500 Subject: [PATCH 094/101] fix dumb typo --- .../management/commands/populate_domain_request_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index f7ea5ce27..cef140cdd 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -29,7 +29,7 @@ class Command(BaseCommand, PopulateScriptTemplate): # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): status = log_entry.changes_dict.get('status') - if status and status[1] == 'Submitted': + if status and status[1] == 'submitted': record.first_submitted_date = log_entry.timestamp.date() break From 9c069a8e64e48b237caec9d41423aec264f66fb6 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:12:57 -0500 Subject: [PATCH 095/101] add instructions --- docs/operations/data_migration.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index cd748b22d..ed2f643d5 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -816,3 +816,30 @@ Example: `cf ssh getgov-za` | | Parameter | Description | |:-:|:-------------------------- |:-----------------------------------------------------------------------------------| | 1 | **federal_cio_csv_path** | Specifies where the federal CIO csv is | + +## Populate Domain Request Dates +This section outlines how to run the populate_domain_request_dates script + +### Running on sandboxes + +#### Step 1: Login to CloudFoundry +```cf login -a api.fr.cloud.gov --sso``` + +#### Step 2: SSH into your environment +```cf ssh getgov-{space}``` + +Example: `cf ssh getgov-za` + +#### Step 3: Create a shell instance +```/tmp/lifecycle/shell``` + +#### Step 4: Running the script +```./manage.py populate_domain_request_dates --debug``` + +### Running locally +```docker-compose exec app ./manage.py populate_domain_request_dates --debug``` + +##### Optional parameters +| | Parameter | Description | +|:-:|:-------------------------- |:----------------------------------------------------------------------------| +| 1 | **debug** | Increases logging detail. Defaults to False. | From 09ba7450b39d00e6cec1a3e91085ede27715db09 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:19:36 -0500 Subject: [PATCH 096/101] linter fixes --- .../management/commands/populate_domain_request_dates.py | 8 ++++---- src/registrar/tests/test_views_requests_json.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index cef140cdd..3bbe6b306 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -11,7 +11,7 @@ class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each DomainRequest object and populates + """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) @@ -34,12 +34,12 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"""{TerminalColors.OKCYAN}Updating {record} => - first submitted date: {record.first_submitted_date}, + f"""{TerminalColors.OKCYAN}Updating {record} => + first submitted date: {record.first_submitted_date}, last status update: {record.last_status_update}{TerminalColors.ENDC} """ ) def should_skip_record(self, record) -> bool: # make sure the record had some kind of history - return not LogEntry.objects.filter(object_pk=record.pk).exists() \ No newline at end of file + return not LogEntry.objects.filter(object_pk=record.pk).exists() diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index fd44bdc31..07a65b586 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,7 +287,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -307,7 +307,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json From 635e6e67351594a9242d6d704a94bbc63a91918d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 28 Aug 2024 09:32:12 -0500 Subject: [PATCH 097/101] linter fixes --- .../commands/populate_domain_request_dates.py | 6 +++--- .../0119_add_domainrequest_submission_dates.py | 6 +++++- src/registrar/tests/test_views_requests_json.py | 10 ++++++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 3bbe6b306..d975a035d 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -22,14 +22,14 @@ class Command(BaseCommand, PopulateScriptTemplate): audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") # Loop through logs in descending order to find most recent status change for log_entry in audit_log_entries: - if 'status' in log_entry.changes_dict: + if "status" in log_entry.changes_dict: record.last_status_update = log_entry.timestamp.date() break # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): - status = log_entry.changes_dict.get('status') - if status and status[1] == 'submitted': + status = log_entry.changes_dict.get("status") + if status and status[1] == "submitted": record.first_submitted_date = log_entry.timestamp.date() break diff --git a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py index ea209626e..35897e302 100644 --- a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -26,7 +26,11 @@ class Migration(migrations.Migration): model_name="domainrequest", name="first_submitted_date", field=models.DateField( - blank=True, default=None, help_text="Date initially submitted", null=True, verbose_name="first submitted on" + blank=True, + default=None, + help_text="Date initially submitted", + null=True, + verbose_name="first submitted on", ), ), migrations.AddField( diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index 07a65b586..20a4069f7 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,8 +287,9 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), - {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get( + reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"} + ) self.assertEqual(response.status_code, 200) data = response.json @@ -307,8 +308,9 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), - {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get( + reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"} + ) self.assertEqual(response.status_code, 200) data = response.json From 6849f13e6cba86890f6c2e620d5a2c74d12d98d9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 08:22:16 -0600 Subject: [PATCH 098/101] Update src/registrar/models/user_portfolio_permission.py Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com> --- src/registrar/models/user_portfolio_permission.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index c2cdc61ad..bf1c3e566 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -42,7 +42,7 @@ class UserPortfolioPermission(TimeStampedModel): user = models.ForeignKey( "registrar.User", null=False, - # when a portfolio is deleted, permissions are too + # when a user is deleted, permissions are too on_delete=models.CASCADE, related_name="portfolio_permissions", ) From 06aacd91ce464fd808d792a04f285b34e2954a05 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 29 Aug 2024 10:58:19 -0400 Subject: [PATCH 099/101] minor improvements --- src/registrar/assets/js/get-gov-admin.js | 51 +++++++++--------------- src/registrar/tests/test_api.py | 11 ++++- 2 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 25dd91e0a..24f020b75 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -908,10 +908,6 @@ function initializeWidgetOnList(list, parentId) { return; } - // Hide the contactList initially. - // If we can update the contact information, it'll be shown again. - hideElement(contactList.parentElement); - // Determine if any changes are necessary to the display of portfolio type or federal type // based on changes to the Federal Agency let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; @@ -925,14 +921,15 @@ function initializeWidgetOnList(list, parentId) { console.error("Error in AJAX call: " + data.error); return; } - - let federal_type = data.federal_type; - let portfolio_type = data.portfolio_type; - updateFederalType(data.federal_type); - updatePortfolioType(data.portfolio_type); + updateReadOnly(data.federal_type, '.field-federal_type'); + updateReadOnly(data.portfolio_type, '.field-portfolio_type'); }) .catch(error => console.error("Error fetching federal and portfolio types: ", error)); + // Hide the contactList initially. + // If we can update the contact information, it'll be shown again. + hideElement(contactList.parentElement); + let seniorOfficialApi = document.getElementById("senior_official_from_agency_json_url").value; fetch(`${seniorOfficialApi}?agency_name=${selectedText}`) .then(response => { @@ -988,33 +985,21 @@ function initializeWidgetOnList(list, parentId) { } /** - * Dynamically update the portfolio type text in the dom to portfolioType + * Utility that selects a div from the DOM using selectorString, + * and updates a div within that div which has class of 'readonly' + * so that the text of the div is updated to updateText + * @param {*} updateText + * @param {*} selectorString */ - function updatePortfolioType(portfolioType) { - // Find the div with class 'field-portfolio_type' - const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); - if (portfolioTypeDiv) { - // Find the nested div with class 'readonly' inside 'field-portfolio_type' - const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); + function updateReadOnly(updateText, selectorString) { + // find the div by selectorString + const selectedDiv = document.querySelector(selectorString); + if (selectedDiv) { + // find the nested div with class 'readonly' inside the selectorString div + const readonlyDiv = selectedDiv.querySelector('.readonly'); if (readonlyDiv) { // Update the text content of the readonly div - readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; - } - } - } - - /** - * Dynamically update the federal type text in the dom to federalType - */ - function updateFederalType(federalType) { - // Find the div with class 'field-federal_type' - const federalTypeDiv = document.querySelector('.field-federal_type'); - if (federalTypeDiv) { - // Find the nested div with class 'readonly' inside 'field-federal_type' - const readonlyDiv = federalTypeDiv.querySelector('.readonly'); - if (readonlyDiv) { - // Update the text content of the readonly div - readonlyDiv.textContent = federalType !== null ? federalType : '-'; + readonlyDiv.textContent = updateText !== null ? updateText : '-'; } } } diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 00bc9a1a2..2597e65c2 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -5,6 +5,7 @@ from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user from api.tests.common import less_console_noise_decorator +from registrar.utility.constants import BranchChoices class GetSeniorOfficialJsonTest(TestCase): @@ -82,7 +83,7 @@ class GetFederalPortfolioTypeJsonTest(TestCase): self.superuser = create_superuser() self.analyst_user = create_user() - self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type="judicial") + self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type=BranchChoices.JUDICIAL) self.api_url = reverse("get-federal-and-portfolio-types-from-federal-agency-json") @@ -100,3 +101,11 @@ class GetFederalPortfolioTypeJsonTest(TestCase): data = response.json() self.assertEqual(data["federal_type"], "Judicial") self.assertEqual(data["portfolio_type"], "Federal - Judicial") + + @less_console_noise_decorator + def test_get_federal_and_portfolio_types_json_authenticated_regularuser(self): + """Test that a regular user receives a 403 with an error message.""" + p = "password" + self.client.login(username="testuser", password=p) + response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) + self.assertEqual(response.status_code, 302) From ba1553a936aedf26c3d95177e1dddf73ad77a729 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 29 Aug 2024 11:49:45 -0500 Subject: [PATCH 100/101] add new dates to csv export --- src/registrar/utility/csv_export.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 6f8510418..7ca3b7e97 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1235,7 +1235,9 @@ class DomainRequestExport(BaseExport): "State/territory": model.get("state_territory"), "Request purpose": model.get("purpose"), "CISA regional representative": model.get("cisa_representative_email"), - "Submitted at": model.get("last_submitted_date"), + "Last submitted date": model.get("last_submitted_date"), + "First submitted date": model.get("first_submitted_date"), + "Last status update": model.get("last_status_update"), } row = [FIELDS.get(column, "") for column in columns] @@ -1304,7 +1306,9 @@ class DomainRequestDataFull(DomainRequestExport): """ return [ "Domain request", - "Submitted at", + "Last submitted date", + "First submitted date", + "Last status update", "Status", "Domain type", "Federal type", From fc17854529ecdebd544ba717a67bb979948f069f Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 29 Aug 2024 11:52:04 -0500 Subject: [PATCH 101/101] correct migration doc --- docs/operations/data_migration.md | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index ed2f643d5..5914eb179 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -834,12 +834,7 @@ Example: `cf ssh getgov-za` ```/tmp/lifecycle/shell``` #### Step 4: Running the script -```./manage.py populate_domain_request_dates --debug``` +```./manage.py populate_domain_request_dates``` ### Running locally -```docker-compose exec app ./manage.py populate_domain_request_dates --debug``` - -##### Optional parameters -| | Parameter | Description | -|:-:|:-------------------------- |:----------------------------------------------------------------------------| -| 1 | **debug** | Increases logging detail. Defaults to False. | +```docker-compose exec app ./manage.py populate_domain_request_dates```