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 01/29] 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 02/29] 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 03/29] 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 aea8539d66559f2b636c995a27421bdb33e7d692 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 14 Aug 2024 11:18:47 -0600 Subject: [PATCH 04/29] 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 c022003539076d6143f1739c4b325d953e171aaf Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 15 Aug 2024 22:16:41 -0600 Subject: [PATCH 07/29] 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 15/29] 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 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 16/29] 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 17/29] 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 ee73893a20856b79d35a058bca0a9a1c06ccb192 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 21 Aug 2024 16:12:34 -0600 Subject: [PATCH 18/29] 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 e59d4738e388ea2bad4a73ab99cdee68bbad475c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 16:44:25 -0400 Subject: [PATCH 19/29] 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 20/29] 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 4a104b6ff6afbbe1870f630205af5fb66f557611 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 17:25:52 -0400 Subject: [PATCH 21/29] 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 22/29] 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 23/29] 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 4cee98edee40a611dd8d770c60fe22011115bfbd Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 22:39:49 -0400 Subject: [PATCH 24/29] 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 26/29] 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 c2f71c984ffe13e83bb768a7bef72c167cb1fe7b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 20:34:56 -0400 Subject: [PATCH 27/29] 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 28/29] 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 29/29] 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)