From f2c05c91ec85d99d1020b0630597e7042e5f002c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 26 Mar 2024 14:44:19 -0400 Subject: [PATCH 01/45] Sticky submit bar on domain request admin --- src/registrar/admin.py | 3 +- src/registrar/assets/js/get-gov-admin.js | 57 +++++++++ src/registrar/assets/js/get-gov-reports.js | 121 ++++++++++-------- src/registrar/assets/sass/_theme/_admin.scss | 32 +++++ src/registrar/assets/sass/_theme/_base.scss | 34 ++--- .../assets/sass/_theme/_buttons.scss | 7 + .../assets/sass/_theme/_tooltips.scss | 26 ++++ src/registrar/assets/sass/_theme/styles.scss | 1 + .../admin/domain_request_change_form.html | 21 ++- src/registrar/templates/domain_users.html | 2 +- src/registrar/tests/test_admin.py | 23 ++++ ...s_application.py => test_views_request.py} | 0 12 files changed, 244 insertions(+), 83 deletions(-) create mode 100644 src/registrar/assets/sass/_theme/_tooltips.scss rename src/registrar/tests/{test_views_application.py => test_views_request.py} (100%) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0096f59b5..9d00493e9 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -958,6 +958,7 @@ class DomainRequestAdmin(ListHeaderAdmin): """Custom domain requests admin class.""" form = DomainRequestAdminForm + change_form_template = "django/admin/domain_request_change_form.html" class InvestigatorFilter(admin.SimpleListFilter): """Custom investigator filter that only displays users with the manager role""" @@ -1019,8 +1020,6 @@ class DomainRequestAdmin(ListHeaderAdmin): if self.value() == "0": return queryset.filter(Q(is_election_board=False) | Q(is_election_board=None)) - change_form_template = "django/admin/domain_request_change_form.html" - # Columns list_display = [ "requested_domain", diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 8c60c534f..c23bf4870 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -410,3 +410,60 @@ function enableRelatedWidgetButtons(changeLink, deleteLink, viewLink, elementPk, }); observer.observe({ type: "navigation" }); })(); + +/** An IIFE for toggling the submit bar on domain request forms +*/ +(function (){ + // Get a reference to the button element + const toggleButton = document.getElementById('submitRowToggle'); + const submitRowWrapper = document.querySelector('.submit-row-wrapper'); + + if (toggleButton) { + // Add event listener to toggle the class and update content on click + toggleButton.addEventListener('click', function() { + // Toggle the 'collapsed' class on the bar + submitRowWrapper.classList.toggle('collapsed'); + + // Get a reference to the span element inside the button + const spanElement = this.querySelector('span'); + + // Get a reference to the use element inside the button + const useElement = this.querySelector('use'); + + // Check if the span element text is 'Hide' + if (spanElement.textContent.trim() === 'Hide') { + // Update the span element text to 'Show' + spanElement.textContent = 'Show'; + + // Update the xlink:href attribute to expand_more + useElement.setAttribute('xlink:href', '/public/img/sprite.svg#expand_less'); + } else { + // Update the span element text to 'Hide' + spanElement.textContent = 'Hide'; + + // Update the xlink:href attribute to expand_less + useElement.setAttribute('xlink:href', '/public/img/sprite.svg#expand_more'); + } + }); + + // We have a scroll indicator at the end of the page. + // Observe it. Once it gets on screen, test to see if the row is collapsed. + // If it is, expand it. + const targetElement = document.querySelector(".scroll-indicator"); + const options = { + threshold: 1 // Adjust the threshold as needed (1 indicates when the target element is fully visible) + }; + // Create a new Intersection Observer + const observer = new IntersectionObserver((entries, observer) => { + entries.forEach(entry => { + if (entry.isIntersecting) { + // Refresh reference to submit row wrapper and check if it's collapsed + if (document.querySelector('.submit-row-wrapper').classList.contains('collapsed')) { + toggleButton.click(); + } + } + }); + }, options); + observer.observe(targetElement); + } +})(); \ No newline at end of file diff --git a/src/registrar/assets/js/get-gov-reports.js b/src/registrar/assets/js/get-gov-reports.js index d10cf2dc6..025a94275 100644 --- a/src/registrar/assets/js/get-gov-reports.js +++ b/src/registrar/assets/js/get-gov-reports.js @@ -54,64 +54,75 @@ })(); -document.addEventListener("DOMContentLoaded", function () { - createComparativeColumnChart("myChart1", "Managed domains", "Start Date", "End Date"); - createComparativeColumnChart("myChart2", "Unmanaged domains", "Start Date", "End Date"); - createComparativeColumnChart("myChart3", "Deleted domains", "Start Date", "End Date"); - createComparativeColumnChart("myChart4", "Ready domains", "Start Date", "End Date"); - createComparativeColumnChart("myChart5", "Submitted requests", "Start Date", "End Date"); - createComparativeColumnChart("myChart6", "All requests", "Start Date", "End Date"); -}); +/** An IIFE to initialize the analytics page +*/ +(function () { + function createComparativeColumnChart(canvasId, title, labelOne, labelTwo) { + var canvas = document.getElementById(canvasId); + if (!canvas) { + return + } -function createComparativeColumnChart(canvasId, title, labelOne, labelTwo) { - var canvas = document.getElementById(canvasId); - var ctx = canvas.getContext("2d"); + var ctx = canvas.getContext("2d"); - var listOne = JSON.parse(canvas.getAttribute('data-list-one')); - var listTwo = JSON.parse(canvas.getAttribute('data-list-two')); + var listOne = JSON.parse(canvas.getAttribute('data-list-one')); + var listTwo = JSON.parse(canvas.getAttribute('data-list-two')); - var data = { - labels: ["Total", "Federal", "Interstate", "State/Territory", "Tribal", "County", "City", "Special District", "School District", "Election Board"], - datasets: [ - { - label: labelOne, - backgroundColor: "rgba(255, 99, 132, 0.2)", - borderColor: "rgba(255, 99, 132, 1)", - borderWidth: 1, - data: listOne, + var data = { + labels: ["Total", "Federal", "Interstate", "State/Territory", "Tribal", "County", "City", "Special District", "School District", "Election Board"], + datasets: [ + { + label: labelOne, + backgroundColor: "rgba(255, 99, 132, 0.2)", + borderColor: "rgba(255, 99, 132, 1)", + borderWidth: 1, + data: listOne, + }, + { + label: labelTwo, + backgroundColor: "rgba(75, 192, 192, 0.2)", + borderColor: "rgba(75, 192, 192, 1)", + borderWidth: 1, + data: listTwo, + }, + ], + }; + + var options = { + responsive: true, + maintainAspectRatio: false, + plugins: { + legend: { + position: 'top', + }, + title: { + display: true, + text: title + } }, - { - label: labelTwo, - backgroundColor: "rgba(75, 192, 192, 0.2)", - borderColor: "rgba(75, 192, 192, 1)", - borderWidth: 1, - data: listTwo, + scales: { + y: { + beginAtZero: true, + }, }, - ], + }; + + new Chart(ctx, { + type: "bar", + data: data, + options: options, + }); + } + + function initComparativeColumnCharts() { + document.addEventListener("DOMContentLoaded", function () { + createComparativeColumnChart("myChart1", "Managed domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart2", "Unmanaged domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart3", "Deleted domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart4", "Ready domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart5", "Submitted requests", "Start Date", "End Date"); + createComparativeColumnChart("myChart6", "All requests", "Start Date", "End Date"); + }); }; - - var options = { - responsive: true, - maintainAspectRatio: false, - plugins: { - legend: { - position: 'top', - }, - title: { - display: true, - text: title - } - }, - scales: { - y: { - beginAtZero: true, - }, - }, - }; - - new Chart(ctx, { - type: "bar", - data: data, - options: options, - }); -} \ No newline at end of file + initComparativeColumnCharts(); +})(); \ No newline at end of file diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index b7a494aef..bb7be78f9 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -349,4 +349,36 @@ input.admin-confirm-button { .errors span.select2-selection { border: 1px solid var(--error-fg) !important; +} + +// Sticky submit bar for domain requests on desktop +@include at-media(desktop) { + .submit-row-wrapper { + position: fixed; + bottom: 0; + right: 0; + background: var(--darkened-bg); + border-top-left-radius: 6px; + transition: transform .2s ease-out; + .submit-row { + margin-bottom: 0; + } + } + + .submit-row-wrapper.collapsed { + // translate3d is more performant than translateY + // https://stackoverflow.com/questions/22111256/translate3d-vs-translate-performance + transform: translate3d(0, 45px, 0); + } + .submit-row-toggle{ + display: inline-block; + position: absolute; + top: -30px; + right: 0; + background: var(--darkened-bg); + } + + #submitRowToggle { + color: var( --body-fg); + } } \ No newline at end of file diff --git a/src/registrar/assets/sass/_theme/_base.scss b/src/registrar/assets/sass/_theme/_base.scss index 127db5589..dac94255a 100644 --- a/src/registrar/assets/sass/_theme/_base.scss +++ b/src/registrar/assets/sass/_theme/_base.scss @@ -117,6 +117,10 @@ abbr[title] { } } +.visible-desktop { + display: none; +} + @include at-media(desktop) { .float-right-desktop { float: right; @@ -124,33 +128,15 @@ abbr[title] { .float-left-desktop { float: left; } + .visible-desktop { + display: block; + } } .flex-end { align-items: flex-end; } -// Only apply this custom wrapping to desktop -@include at-media(desktop) { - .usa-tooltip__body { - width: 350px; - white-space: normal; - text-align: center; - } -} - -@include at-media(tablet) { - .usa-tooltip__body { - width: 250px !important; - white-space: normal !important; - text-align: center !important; - } -} - -@include at-media(mobile) { - .usa-tooltip__body { - width: 250px !important; - white-space: normal !important; - text-align: center !important; - } -} +.cursor-pointer { + cursor: pointer; +} \ No newline at end of file diff --git a/src/registrar/assets/sass/_theme/_buttons.scss b/src/registrar/assets/sass/_theme/_buttons.scss index ef8635b95..1f5047503 100644 --- a/src/registrar/assets/sass/_theme/_buttons.scss +++ b/src/registrar/assets/sass/_theme/_buttons.scss @@ -138,6 +138,13 @@ a.usa-button--unstyled:visited { } } +.usa-button--unstyled--white, +.usa-button--unstyled--white:hover, +.usa-button--unstyled--white:focus, +.usa-button--unstyled--white:active { + color: color('white'); +} + // Cancel button used on the // DNSSEC main page // We want to center this button on mobile diff --git a/src/registrar/assets/sass/_theme/_tooltips.scss b/src/registrar/assets/sass/_theme/_tooltips.scss new file mode 100644 index 000000000..01348e1b1 --- /dev/null +++ b/src/registrar/assets/sass/_theme/_tooltips.scss @@ -0,0 +1,26 @@ +@use "uswds-core" as *; + +// Only apply this custom wrapping to desktop +@include at-media(desktop) { + .usa-tooltip__body { + width: 350px; + white-space: normal; + text-align: center; + } +} + +@include at-media(tablet) { + .usa-tooltip__body { + width: 250px !important; + white-space: normal !important; + text-align: center !important; + } +} + +@include at-media(mobile) { + .usa-tooltip__body { + width: 250px !important; + white-space: normal !important; + text-align: center !important; + } +} diff --git a/src/registrar/assets/sass/_theme/styles.scss b/src/registrar/assets/sass/_theme/styles.scss index 0239199e7..942501110 100644 --- a/src/registrar/assets/sass/_theme/styles.scss +++ b/src/registrar/assets/sass/_theme/styles.scss @@ -13,6 +13,7 @@ @forward "lists"; @forward "buttons"; @forward "forms"; +@forward "tooltips"; @forward "fieldsets"; @forward "alerts"; @forward "tables"; diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 95392da1e..32aee7c71 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -92,5 +92,24 @@ -{{ block.super }} + +
+ + + + + +

+ Requested domain: {{ original.requested_domain.name }} +

+ {{ block.super }} +
+ + + {% endblock %} \ No newline at end of file diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index e295d2f7e..9d3bbe6e6 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -134,7 +134,7 @@ {{ invitation.status|title }}
- {% csrf_token %} + {% csrf_token %}
diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 8ed61d76c..c01106b27 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2,6 +2,7 @@ from datetime import date from django.test import TestCase, RequestFactory, Client, override_settings from django.contrib.admin.sites import AdminSite from contextlib import ExitStack +from api.tests.common import less_console_noise_decorator from django_webtest import WebTest # type: ignore from django.contrib import messages from django.urls import reverse @@ -1211,6 +1212,28 @@ class TestDomainRequestAdmin(MockEppLib): # Test that approved domain exists and equals requested domain self.assertEqual(domain_request.requested_domain.name, domain_request.approved_domain.name) + @less_console_noise_decorator + def test_sticky_submit_row_shows_requested_domain(self): + """Test that the change_form template contains a string indicative of the customization + of the sticky submit bar.""" + + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + self.client.force_login(self.superuser) + + # Create a sample domain request + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) + + # Create a mock request + request = self.client.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) + + # Since we're using client to mock the request, we can only test against + # non-interpolated values + expected_content = "Requested domain:" + + self.assertContains(request, expected_content) + def test_save_model_sets_restricted_status_on_user(self): with less_console_noise(): # make sure there is no user with this email diff --git a/src/registrar/tests/test_views_application.py b/src/registrar/tests/test_views_request.py similarity index 100% rename from src/registrar/tests/test_views_application.py rename to src/registrar/tests/test_views_request.py From 5afe85d5ce7c5f7f2dc8310621e7141a02f67e8e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 26 Mar 2024 14:52:17 -0400 Subject: [PATCH 02/45] Hide toggle button on mobile --- .../templates/django/admin/domain_request_change_form.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 32aee7c71..bfabd5124 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -95,7 +95,7 @@
- + -

+

Requested domain: {{ original.requested_domain.name }}

{{ block.super }} From d9c1ee82fb703ef6ff69aa3cdde92f10287d5eba Mon Sep 17 00:00:00 2001 From: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> Date: Fri, 29 Mar 2024 13:44:36 -0400 Subject: [PATCH 16/45] Update src/registrar/assets/sass/_theme/_admin.scss Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- src/registrar/assets/sass/_theme/_admin.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 53e887683..01b40a923 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -450,7 +450,7 @@ address.dja-address-contact-list { } #submitRowToggle { - color: var( --body-fg); + color: var(--body-fg); } .requested-domain-sticky { From 1668381a5c9f040428ef31cbfebd2a61f7f7fa2b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 13:58:49 -0400 Subject: [PATCH 17/45] layout tweak --- src/registrar/assets/js/get-gov-reports.js | 1 + src/registrar/assets/sass/_theme/_admin.scss | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/assets/js/get-gov-reports.js b/src/registrar/assets/js/get-gov-reports.js index 497b92443..92bba4a1f 100644 --- a/src/registrar/assets/js/get-gov-reports.js +++ b/src/registrar/assets/js/get-gov-reports.js @@ -125,5 +125,6 @@ createComparativeColumnChart("myChart6", "All requests", "Start Date", "End Date"); }); }; + initComparativeColumnCharts(); })(); diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 01b40a923..3fba9f5f1 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -428,6 +428,7 @@ address.dja-address-contact-list { position: fixed; bottom: 0; right: 0; + left: 350px; background: var(--darkened-bg); border-top-left-radius: 6px; transition: transform .2s ease-out; @@ -461,7 +462,7 @@ address.dja-address-contact-list { } } -@media screen and (max-width:1199px) { +@media screen and (max-width:1256px) { .submit-row { clear: both; } From 8119dc477bcc95441cbe7fa979e40271d47222b2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 14:04:41 -0400 Subject: [PATCH 18/45] layout tweak --- src/registrar/assets/sass/_theme/_admin.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 3fba9f5f1..88050bad4 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -428,7 +428,7 @@ address.dja-address-contact-list { position: fixed; bottom: 0; right: 0; - left: 350px; + left: 338px; background: var(--darkened-bg); border-top-left-radius: 6px; transition: transform .2s ease-out; From 1561f2c8b531af15ebd1244aec65a30b9e93b3ca Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 14:19:25 -0400 Subject: [PATCH 19/45] layout tweak --- src/registrar/assets/sass/_theme/_admin.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 88050bad4..a86612e73 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -466,4 +466,9 @@ address.dja-address-contact-list { .submit-row { clear: both; } + .submit-row-wrapper.collapsed { + // translate3d is more performant than translateY + // https://stackoverflow.com/questions/22111256/translate3d-vs-translate-performance + transform: translate3d(0, 88px, 0); + } } From 63953a2471619b97a2c1ab54df622fada0fdd394 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 14:40:27 -0400 Subject: [PATCH 20/45] tweak update_columns_with_domain_managers --- src/registrar/utility/csv_export.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 98e3786c5..5db51b539 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -165,13 +165,16 @@ def update_columns_with_domain_managers( dm_active = domain_info.domain.permissions.count() dm_invited = domain_info.domain.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).count() - if dm_active > max_dm_active or dm_invited > max_dm_invited: + if dm_active > max_dm_active: max_dm_active = max(dm_active, max_dm_active) + update_columns = True + + if dm_invited > max_dm_invited: max_dm_invited = max(dm_invited, max_dm_invited) - max_dm_total = max_dm_active + max_dm_invited update_columns = True if update_columns: + max_dm_total = max_dm_active + max_dm_invited for i in range(1, max_dm_total + 1): column_name = f"Domain manager {i}" column2_name = f"DM{i} status" From 8c8d7b6b63914234981b7a85200c547b7a3a4bcf Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 16:32:51 -0400 Subject: [PATCH 21/45] Fix logic, optimize analytics page load --- src/registrar/assets/js/get-gov-reports.js | 4 +- src/registrar/templates/admin/analytics.html | 4 +- src/registrar/utility/csv_export.py | 146 +++++++------------ src/registrar/views/admin_views.py | 52 +++---- 4 files changed, 84 insertions(+), 122 deletions(-) diff --git a/src/registrar/assets/js/get-gov-reports.js b/src/registrar/assets/js/get-gov-reports.js index da0781411..92bba4a1f 100644 --- a/src/registrar/assets/js/get-gov-reports.js +++ b/src/registrar/assets/js/get-gov-reports.js @@ -117,8 +117,8 @@ function initComparativeColumnCharts() { document.addEventListener("DOMContentLoaded", function () { - // createComparativeColumnChart("myChart1", "Managed domains", "Start Date", "End Date"); - // createComparativeColumnChart("myChart2", "Unmanaged domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart1", "Managed domains", "Start Date", "End Date"); + createComparativeColumnChart("myChart2", "Unmanaged domains", "Start Date", "End Date"); createComparativeColumnChart("myChart3", "Deleted domains", "Start Date", "End Date"); createComparativeColumnChart("myChart4", "Ready domains", "Start Date", "End Date"); createComparativeColumnChart("myChart5", "Submitted requests", "Start Date", "End Date"); diff --git a/src/registrar/templates/admin/analytics.html b/src/registrar/templates/admin/analytics.html index 6812c919d..e73f22ec5 100644 --- a/src/registrar/templates/admin/analytics.html +++ b/src/registrar/templates/admin/analytics.html @@ -113,7 +113,7 @@ - +
diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 5db51b539..0521a71be 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1,4 +1,3 @@ -from collections import Counter import csv import logging from datetime import datetime @@ -8,7 +7,7 @@ from registrar.models.domain_request import DomainRequest from registrar.models.domain_information import DomainInformation from django.utils import timezone from django.core.paginator import Paginator -from django.db.models import F, Value, CharField +from django.db.models import F, Value, CharField, Q, Count from django.db.models.functions import Concat, Coalesce from registrar.models.public_contact import PublicContact @@ -54,7 +53,7 @@ def get_domain_infos(filter_condition, sort_fields): return domain_infos_cleaned -def parse_domain_row(columns, domain_info: DomainInformation, security_emails_dict=None, get_domain_managers=False): +def parse_domain_row(columns, domain_info: DomainInformation, security_emails_dict=None, get_domain_managers=False, invites_with_invited_status=None): """Given a set of columns, generate a new row from cleaned column data""" # Domain should never be none when parsing this information @@ -108,7 +107,7 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di # Get lists of emails for active and invited domain managers dm_active_emails = [dm.user.email for dm in domain.permissions.all()] dm_invited_emails = [ - invite.email for invite in domain.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED) + invite.email for invite in invites_with_invited_status.filter(domain=domain) ] # Set up the "matching headers" + row field data for email and status @@ -149,32 +148,25 @@ def _get_security_emails(sec_contact_ids): def update_columns_with_domain_managers( - domain_info, update_columns, columns, max_dm_active, max_dm_invited, max_dm_total + domain_info,invites_with_invited_status, update_columns, columns, max_dm_total ): """Helper function that works with 'global' variables set in write_domains_csv Accepts: domain_info -> Domains to parse update_columns -> A control to make sure we only run the columns test and update when needed columns -> The header cells in the csv that's under construction - max_dm_active -> Starts at 0 and gets updated and passed again through this method - max_dm_invited -> Starts at 0 and gets updated and passed again through this method max_dm_total -> Starts at 0 and gets updated and passed again through this method Returns: Updated update_columns, columns, max_dm_active, max_dm_invited, max_dm_total""" dm_active = domain_info.domain.permissions.count() - dm_invited = domain_info.domain.invitations.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).count() + dm_invited = invites_with_invited_status.filter(domain=domain_info.domain).count() - if dm_active > max_dm_active: - max_dm_active = max(dm_active, max_dm_active) - update_columns = True - - if dm_invited > max_dm_invited: - max_dm_invited = max(dm_invited, max_dm_invited) + if dm_active + dm_invited > max_dm_total: + max_dm_total = dm_active + dm_invited update_columns = True if update_columns: - max_dm_total = max_dm_active + max_dm_invited for i in range(1, max_dm_total + 1): column_name = f"Domain manager {i}" column2_name = f"DM{i} status" @@ -183,7 +175,7 @@ def update_columns_with_domain_managers( columns.append(column2_name) update_columns = False - return update_columns, columns, max_dm_active, max_dm_invited, max_dm_total + return update_columns, columns, max_dm_total def write_domains_csv( @@ -213,10 +205,18 @@ def write_domains_csv( # We get the number of domain managers (DMs) an the domain # that has the most DMs so we can set the header row appropriately - max_dm_active = 0 - max_dm_invited = 0 + max_dm_total = 0 update_columns = False + invites_with_invited_status=None + + if get_domain_managers: + invites_with_invited_status = DomainInvitation.objects.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).prefetch_related("domain") + + # zander = DomainInformation.objects.filter(**filter_condition).annotate(invitations_count=Count('invitation', filter=Q(invitation__status='invited'))).values_list('domain_name', 'invitations_count') + # logger.info(f'zander {zander}') + # zander_dict = dict(zander) + # logger.info(f'zander_dict {zander_dict}') # This var will live outside of the nested for loops to aggregate # the data from those loops @@ -229,14 +229,14 @@ def write_domains_csv( # Get max number of domain managers if get_domain_managers: - update_columns, columns, max_dm_active, max_dm_invited, max_dm_total = ( + update_columns, columns, max_dm_total = ( update_columns_with_domain_managers( - domain_info, update_columns, columns, max_dm_active, max_dm_invited, max_dm_total + domain_info,invites_with_invited_status, update_columns, columns, max_dm_total ) ) try: - row = parse_domain_row(columns, domain_info, security_emails_dict, get_domain_managers) + row = parse_domain_row(columns, domain_info, security_emails_dict, get_domain_managers, invites_with_invited_status) rows.append(row) except ValueError: # This should not happen. If it does, just skip this row. @@ -518,58 +518,23 @@ def get_sliced_domains(filter_condition, distinct=False): when a domain has more that one manager. """ - # Round trip 1: Get distinct domain names based on filter condition - domains_count = DomainInformation.objects.filter(**filter_condition).distinct().count() - - # Round trip 2: Get counts for other slices - # This will require either 8 filterd and distinct DB round trips, - # or 2 DB round trips plus iteration on domain_permissions for each domain - if distinct: - generic_org_types_query = DomainInformation.objects.filter(**filter_condition).values_list( - "domain_id", "generic_org_type" - ) - # Initialize Counter to store counts for each generic_org_type - generic_org_type_counts = Counter() - - # Keep track of domains already counted - domains_counted = set() - - # Iterate over distinct domains - for domain_id, generic_org_type in generic_org_types_query: - # Check if the domain has already been counted - if domain_id in domains_counted: - continue - - # Get all permissions for the current domain - domain_permissions = DomainInformation.objects.filter(domain_id=domain_id, **filter_condition).values_list( - "domain__permissions", flat=True - ) - - # Check if the domain has multiple permissions - if len(domain_permissions) > 0: - # Mark the domain as counted - domains_counted.add(domain_id) - - # Increment the count for the corresponding generic_org_type - generic_org_type_counts[generic_org_type] += 1 - else: - generic_org_types_query = DomainInformation.objects.filter(**filter_condition).values_list( - "generic_org_type", flat=True - ) - generic_org_type_counts = Counter(generic_org_types_query) - - # Extract counts for each generic_org_type - federal = generic_org_type_counts.get(DomainRequest.OrganizationChoices.FEDERAL, 0) - interstate = generic_org_type_counts.get(DomainRequest.OrganizationChoices.INTERSTATE, 0) - state_or_territory = generic_org_type_counts.get(DomainRequest.OrganizationChoices.STATE_OR_TERRITORY, 0) - tribal = generic_org_type_counts.get(DomainRequest.OrganizationChoices.TRIBAL, 0) - county = generic_org_type_counts.get(DomainRequest.OrganizationChoices.COUNTY, 0) - city = generic_org_type_counts.get(DomainRequest.OrganizationChoices.CITY, 0) - special_district = generic_org_type_counts.get(DomainRequest.OrganizationChoices.SPECIAL_DISTRICT, 0) - school_district = generic_org_type_counts.get(DomainRequest.OrganizationChoices.SCHOOL_DISTRICT, 0) - - # Round trip 3 - election_board = DomainInformation.objects.filter(is_election_board=True, **filter_condition).distinct().count() + domains = DomainInformation.objects.all().filter(**filter_condition).distinct() + domains_count = domains.count() + federal = domains.filter(generic_org_type=DomainRequest.OrganizationChoices.FEDERAL).distinct().count() + interstate = domains.filter(generic_org_type=DomainRequest.OrganizationChoices.INTERSTATE).count() + state_or_territory = ( + domains.filter(generic_org_type=DomainRequest.OrganizationChoices.STATE_OR_TERRITORY).distinct().count() + ) + tribal = domains.filter(generic_org_type=DomainRequest.OrganizationChoices.TRIBAL).distinct().count() + county = domains.filter(generic_org_type=DomainRequest.OrganizationChoices.COUNTY).distinct().count() + city = domains.filter(generic_org_type=DomainRequest.OrganizationChoices.CITY).distinct().count() + special_district = ( + domains.filter(generic_org_type=DomainRequest.OrganizationChoices.SPECIAL_DISTRICT).distinct().count() + ) + school_district = ( + domains.filter(generic_org_type=DomainRequest.OrganizationChoices.SCHOOL_DISTRICT).distinct().count() + ) + election_board = domains.filter(is_election_board=True).distinct().count() return [ domains_count, @@ -588,26 +553,23 @@ def get_sliced_domains(filter_condition, distinct=False): def get_sliced_requests(filter_condition): """Get filtered requests counts sliced by org type and election office.""" - # Round trip 1: Get distinct requests based on filter condition - requests_count = DomainRequest.objects.filter(**filter_condition).distinct().count() - - # Round trip 2: Get counts for other slices - generic_org_types_query = DomainRequest.objects.filter(**filter_condition).values_list( - "generic_org_type", flat=True + requests = DomainRequest.objects.all().filter(**filter_condition).distinct() + requests_count = requests.count() + federal = requests.filter(generic_org_type=DomainRequest.OrganizationChoices.FEDERAL).distinct().count() + interstate = requests.filter(generic_org_type=DomainRequest.OrganizationChoices.INTERSTATE).distinct().count() + state_or_territory = ( + requests.filter(generic_org_type=DomainRequest.OrganizationChoices.STATE_OR_TERRITORY).distinct().count() ) - generic_org_type_counts = Counter(generic_org_types_query) - - federal = generic_org_type_counts.get(DomainRequest.OrganizationChoices.FEDERAL, 0) - interstate = generic_org_type_counts.get(DomainRequest.OrganizationChoices.INTERSTATE, 0) - state_or_territory = generic_org_type_counts.get(DomainRequest.OrganizationChoices.STATE_OR_TERRITORY, 0) - tribal = generic_org_type_counts.get(DomainRequest.OrganizationChoices.TRIBAL, 0) - county = generic_org_type_counts.get(DomainRequest.OrganizationChoices.COUNTY, 0) - city = generic_org_type_counts.get(DomainRequest.OrganizationChoices.CITY, 0) - special_district = generic_org_type_counts.get(DomainRequest.OrganizationChoices.SPECIAL_DISTRICT, 0) - school_district = generic_org_type_counts.get(DomainRequest.OrganizationChoices.SCHOOL_DISTRICT, 0) - - # Round trip 3 - election_board = DomainRequest.objects.filter(is_election_board=True, **filter_condition).distinct().count() + tribal = requests.filter(generic_org_type=DomainRequest.OrganizationChoices.TRIBAL).distinct().count() + county = requests.filter(generic_org_type=DomainRequest.OrganizationChoices.COUNTY).distinct().count() + city = requests.filter(generic_org_type=DomainRequest.OrganizationChoices.CITY).distinct().count() + special_district = ( + requests.filter(generic_org_type=DomainRequest.OrganizationChoices.SPECIAL_DISTRICT).distinct().count() + ) + school_district = ( + requests.filter(generic_org_type=DomainRequest.OrganizationChoices.SCHOOL_DISTRICT).distinct().count() + ) + election_board = requests.filter(is_election_board=True).distinct().count() return [ requests_count, diff --git a/src/registrar/views/admin_views.py b/src/registrar/views/admin_views.py index e33dea407..eba8423ed 100644 --- a/src/registrar/views/admin_views.py +++ b/src/registrar/views/admin_views.py @@ -41,29 +41,29 @@ class AnalyticsView(View): start_date_formatted = csv_export.format_start_date(start_date) end_date_formatted = csv_export.format_end_date(end_date) - # filter_managed_domains_start_date = { - # "domain__permissions__isnull": False, - # "domain__first_ready__lte": start_date_formatted, - # } - # filter_managed_domains_end_date = { - # "domain__permissions__isnull": False, - # "domain__first_ready__lte": end_date_formatted, - # } - # managed_domains_sliced_at_start_date = csv_export.get_sliced_domains(filter_managed_domains_start_date, True) - # managed_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_managed_domains_end_date, True) + filter_managed_domains_start_date = { + "domain__permissions__isnull": False, + "domain__first_ready__lte": start_date_formatted, + } + filter_managed_domains_end_date = { + "domain__permissions__isnull": False, + "domain__first_ready__lte": end_date_formatted, + } + managed_domains_sliced_at_start_date = csv_export.get_sliced_domains(filter_managed_domains_start_date, True) + managed_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_managed_domains_end_date, True) - # filter_unmanaged_domains_start_date = { - # "domain__permissions__isnull": True, - # "domain__first_ready__lte": start_date_formatted, - # } - # filter_unmanaged_domains_end_date = { - # "domain__permissions__isnull": True, - # "domain__first_ready__lte": end_date_formatted, - # } - # unmanaged_domains_sliced_at_start_date = csv_export.get_sliced_domains( - # filter_unmanaged_domains_start_date, True - # ) - # unmanaged_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_unmanaged_domains_end_date, True) + filter_unmanaged_domains_start_date = { + "domain__permissions__isnull": True, + "domain__first_ready__lte": start_date_formatted, + } + filter_unmanaged_domains_end_date = { + "domain__permissions__isnull": True, + "domain__first_ready__lte": end_date_formatted, + } + unmanaged_domains_sliced_at_start_date = csv_export.get_sliced_domains( + filter_unmanaged_domains_start_date, True + ) + unmanaged_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_unmanaged_domains_end_date, True) filter_ready_domains_start_date = { "domain__state__in": [models.Domain.State.READY], @@ -120,10 +120,10 @@ class AnalyticsView(View): last_30_days_applications=last_30_days_applications.count(), last_30_days_approved_applications=last_30_days_approved_applications.count(), average_application_approval_time_last_30_days=avg_approval_time_display, - # managed_domains_sliced_at_start_date=managed_domains_sliced_at_start_date, - # unmanaged_domains_sliced_at_start_date=unmanaged_domains_sliced_at_start_date, - # managed_domains_sliced_at_end_date=managed_domains_sliced_at_end_date, - # unmanaged_domains_sliced_at_end_date=unmanaged_domains_sliced_at_end_date, + managed_domains_sliced_at_start_date=managed_domains_sliced_at_start_date, + unmanaged_domains_sliced_at_start_date=unmanaged_domains_sliced_at_start_date, + managed_domains_sliced_at_end_date=managed_domains_sliced_at_end_date, + unmanaged_domains_sliced_at_end_date=unmanaged_domains_sliced_at_end_date, ready_domains_sliced_at_start_date=ready_domains_sliced_at_start_date, deleted_domains_sliced_at_start_date=deleted_domains_sliced_at_start_date, ready_domains_sliced_at_end_date=ready_domains_sliced_at_end_date, From 25a46983db810e30f50c3af5b1b86bca0f008ee8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 29 Mar 2024 16:41:00 -0400 Subject: [PATCH 22/45] change breakpoint for sticky nav --- src/registrar/assets/sass/_theme/_admin.scss | 2 +- .../templates/django/admin/domain_request_change_form.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index a86612e73..29c60aa1e 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -423,7 +423,7 @@ address.dja-address-contact-list { } // Sticky submit bar for domain requests on desktop -@include at-media(desktop) { +@media screen and (min-width:768px) { .submit-row-wrapper { position: fixed; bottom: 0; diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 0dedd4751..449a69062 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -114,7 +114,7 @@ -

+

Requested domain: {{ original.requested_domain.name }}

{{ block.super }} From e59c3613ac998b98eac469e8b1953dda48d4f7bf Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Mon, 1 Apr 2024 13:01:45 -0700 Subject: [PATCH 23/45] Add analyst or superuser check for deleting user role --- src/registrar/views/utility/mixins.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index aa0c9cd6b..c7083ce48 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -307,7 +307,12 @@ class UserDeleteDomainRolePermission(PermissionsLoginMixin): domain=domain_pk, domain__permissions__user=self.request.user, ).exists() - if not has_delete_permission: + + user_is_analyst_or_superuser = self.request.user.has_perm( + "registrar.analyst_access_permission" + ) or self.request.user.has_perm("registrar.full_access_permission") + + if not (has_delete_permission or user_is_analyst_or_superuser): return False # Check if more than one manager exists on the domain. From d1e8c2c4e8a6130b23bfa854e51ca29d6af38a8e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 1 Apr 2024 19:57:47 -0400 Subject: [PATCH 24/45] Tweak design for certain screen sizes for analysts only --- src/registrar/assets/js/get-gov-admin.js | 4 +- src/registrar/assets/sass/_theme/_admin.scss | 39 ++++++++++++------- .../admin/domain_request_change_form.html | 3 +- src/registrar/templatetags/custom_filters.py | 5 +++ 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 60f4bbbbb..9a92542b1 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -422,7 +422,7 @@ function enableRelatedWidgetButtons(changeLink, deleteLink, viewLink, elementPk, // Add event listener to toggle the class and update content on click toggleButton.addEventListener('click', function() { // Toggle the 'collapsed' class on the bar - submitRowWrapper.classList.toggle('collapsed'); + submitRowWrapper.classList.toggle('submit-row-wrapper--collapsed'); // Get a reference to the span element inside the button const spanElement = this.querySelector('span'); @@ -458,7 +458,7 @@ function enableRelatedWidgetButtons(changeLink, deleteLink, viewLink, elementPk, entries.forEach(entry => { if (entry.isIntersecting) { // Refresh reference to submit row wrapper and check if it's collapsed - if (document.querySelector('.submit-row-wrapper').classList.contains('collapsed')) { + if (document.querySelector('.submit-row-wrapper').classList.contains('submit-row-wrapper--collapsed')) { toggleButton.click(); } } diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 29c60aa1e..ddcc6acd6 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -422,6 +422,15 @@ address.dja-address-contact-list { border: 1px solid var(--error-fg) !important; } +@mixin submit-row-wrapper--collapsed-one-line(){ + &.submit-row-wrapper--collapsed { + transform: translate3d(0, 42px, 0); + } + .submit-row { + clear: none; + } + } + // Sticky submit bar for domain requests on desktop @media screen and (min-width:768px) { .submit-row-wrapper { @@ -436,11 +445,16 @@ address.dja-address-contact-list { margin-bottom: 0; } } - - .submit-row-wrapper.collapsed { + .submit-row-wrapper--collapsed { // translate3d is more performant than translateY // https://stackoverflow.com/questions/22111256/translate3d-vs-translate-performance - transform: translate3d(0, 42px, 0); + transform: translate3d(0, 88px, 0); + } + .submit-row-wrapper--collapsed-one-line { + @include submit-row-wrapper--collapsed-one-line(); + } + .submit-row { + clear: both; } .submit-row-toggle{ display: inline-block; @@ -449,11 +463,9 @@ address.dja-address-contact-list { right: 0; background: var(--darkened-bg); } - #submitRowToggle { color: var(--body-fg); } - .requested-domain-sticky { max-width: 325px; overflow: hidden; @@ -462,13 +474,14 @@ address.dja-address-contact-list { } } -@media screen and (max-width:1256px) { - .submit-row { - clear: both; - } - .submit-row-wrapper.collapsed { - // translate3d is more performant than translateY - // https://stackoverflow.com/questions/22111256/translate3d-vs-translate-performance - transform: translate3d(0, 88px, 0); +@media screen and (min-width:935px) { + .submit-row-wrapper--analyst-view { + @include submit-row-wrapper--collapsed-one-line(); + } +} + +@media screen and (min-width:1256px) { + .submit-row-wrapper { + @include submit-row-wrapper--collapsed-one-line(); } } diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 449a69062..17d4fdef9 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -1,4 +1,5 @@ {% extends 'admin/change_form.html' %} +{% load custom_filters %} {% load i18n static %} {% block field_sets %} @@ -103,7 +104,7 @@
-
+
+{# submit-row-wrapper--analyst-view is a class that manages layout on certain screens for analysts only #}
diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 37361375a..d6625081d 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1213,9 +1213,11 @@ class TestDomainRequestAdmin(MockEppLib): self.assertEqual(domain_request.requested_domain.name, domain_request.approved_domain.name) @less_console_noise_decorator - def test_sticky_submit_row_shows_requested_domain(self): - """Test that the change_form template contains a string indicative of the customization - of the sticky submit bar.""" + def test_sticky_submit_row(self): + """Test that the change_form template contains strings indicative of the customization + of the sticky submit bar. + + Also test that it does NOT contain a CSS class meant for analysts only when logged in as superuser.""" # make sure there is no user with this email EMAIL = "mayor@igorville.gov" @@ -1233,6 +1235,35 @@ class TestDomainRequestAdmin(MockEppLib): expected_content = "Requested domain:" expected_content2 = '' expected_content3 = '
' + not_expected_content = "submit-row-wrapper--analyst-view>" + self.assertContains(request, expected_content) + self.assertContains(request, expected_content2) + self.assertContains(request, expected_content3) + self.assertNotContains(request, not_expected_content) + + @less_console_noise_decorator + def test_sticky_submit_row_has_extra_class_for_analysts(self): + """Test that the change_form template contains strings indicative of the customization + of the sticky submit bar. + + Also test that it DOES contain a CSS class meant for analysts only when logged in as analyst.""" + + # make sure there is no user with this email + EMAIL = "mayor@igorville.gov" + User.objects.filter(email=EMAIL).delete() + self.client.force_login(self.staffuser) + + # Create a sample domain request + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) + + # Create a mock request + request = self.client.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) + + # Since we're using client to mock the request, we can only test against + # non-interpolated values + expected_content = "Requested domain:" + expected_content2 = '' + expected_content3 = '
' self.assertContains(request, expected_content) self.assertContains(request, expected_content2) self.assertContains(request, expected_content3) From 45c047dc8a5cf32a888d620dde33036b55b40df9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 1 Apr 2024 21:35:29 -0400 Subject: [PATCH 26/45] optimize by removing filters from inside iterations --- src/registrar/utility/csv_export.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 0521a71be..d4485e11f 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -107,7 +107,7 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di # Get lists of emails for active and invited domain managers dm_active_emails = [dm.user.email for dm in domain.permissions.all()] dm_invited_emails = [ - invite.email for invite in invites_with_invited_status.filter(domain=domain) + invite.email for invite in invites_with_invited_status if invite.domain_id == domain_info.domain_id ] # Set up the "matching headers" + row field data for email and status @@ -160,7 +160,7 @@ def update_columns_with_domain_managers( Updated update_columns, columns, max_dm_active, max_dm_invited, max_dm_total""" dm_active = domain_info.domain.permissions.count() - dm_invited = invites_with_invited_status.filter(domain=domain_info.domain).count() + dm_invited = sum(1 for invite in invites_with_invited_status if invite.domain_id == domain_info.domain_id) if dm_active + dm_invited > max_dm_total: max_dm_total = dm_active + dm_invited From c885d1dfab0e76263c25a74058707646871903c7 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Mon, 1 Apr 2024 18:36:45 -0700 Subject: [PATCH 27/45] Remove extraneous br --- src/registrar/templates/domain_request_status.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/domain_request_status.html b/src/registrar/templates/domain_request_status.html index 5b6ee8368..a594698dd 100644 --- a/src/registrar/templates/domain_request_status.html +++ b/src/registrar/templates/domain_request_status.html @@ -41,7 +41,7 @@

-

Last updated: {{DomainRequest.updated_at|date:"F j, Y"}}

+

Last updated: {{DomainRequest.updated_at|date:"F j, Y"}}

{% include "includes/domain_request.html" %}

Withdraw request From dba2dfb6c2034668fc658f461262ba4f5a6c1c89 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 1 Apr 2024 21:46:06 -0400 Subject: [PATCH 28/45] Revert last experiment --- src/registrar/utility/csv_export.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index d4485e11f..0521a71be 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -107,7 +107,7 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di # Get lists of emails for active and invited domain managers dm_active_emails = [dm.user.email for dm in domain.permissions.all()] dm_invited_emails = [ - invite.email for invite in invites_with_invited_status if invite.domain_id == domain_info.domain_id + invite.email for invite in invites_with_invited_status.filter(domain=domain) ] # Set up the "matching headers" + row field data for email and status @@ -160,7 +160,7 @@ def update_columns_with_domain_managers( Updated update_columns, columns, max_dm_active, max_dm_invited, max_dm_total""" dm_active = domain_info.domain.permissions.count() - dm_invited = sum(1 for invite in invites_with_invited_status if invite.domain_id == domain_info.domain_id) + dm_invited = invites_with_invited_status.filter(domain=domain_info.domain).count() if dm_active + dm_invited > max_dm_total: max_dm_total = dm_active + dm_invited From 98dfa95a5b149da019f3cc888340e6537bd4ed60 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 2 Apr 2024 07:51:51 -0700 Subject: [PATCH 29/45] Fix spacing --- src/registrar/templates/domain_request_status.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/domain_request_status.html b/src/registrar/templates/domain_request_status.html index a594698dd..d3c1eab6d 100644 --- a/src/registrar/templates/domain_request_status.html +++ b/src/registrar/templates/domain_request_status.html @@ -41,7 +41,7 @@


-

Last updated: {{DomainRequest.updated_at|date:"F j, Y"}}

+

Last updated: {{DomainRequest.updated_at|date:"F j, Y"}}

{% include "includes/domain_request.html" %}

Withdraw request From 410440ee709b26e086f3c85b80c04a6ba1c5bb63 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 2 Apr 2024 12:39:29 -0400 Subject: [PATCH 30/45] Optimize by using prebuilt dicts --- src/registrar/utility/csv_export.py | 139 ++++++++++++++++++---------- 1 file changed, 88 insertions(+), 51 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 0521a71be..d8638d0d9 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -11,6 +11,8 @@ from django.db.models import F, Value, CharField, Q, Count from django.db.models.functions import Concat, Coalesce from registrar.models.public_contact import PublicContact +from registrar.models.user_domain_role import UserDomainRole +from registrar.models.utility.generic_helper import Timer from registrar.utility.enums import DefaultEmail logger = logging.getLogger(__name__) @@ -33,7 +35,6 @@ def get_domain_infos(filter_condition, sort_fields): """ domain_infos = ( DomainInformation.objects.select_related("domain", "authorizing_official") - .prefetch_related("domain__permissions", "domain__invitations") .filter(**filter_condition) .order_by(*sort_fields) .distinct() @@ -53,7 +54,7 @@ def get_domain_infos(filter_condition, sort_fields): return domain_infos_cleaned -def parse_domain_row(columns, domain_info: DomainInformation, security_emails_dict=None, get_domain_managers=False, invites_with_invited_status=None): +def parse_domain_row(columns, domain_info: DomainInformation, security_emails_dict=None, get_domain_managers=False, domain_invitation_emails=None, domain_permissions_emails=None): """Given a set of columns, generate a new row from cleaned column data""" # Domain should never be none when parsing this information @@ -105,10 +106,9 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di if get_domain_managers: # Get lists of emails for active and invited domain managers - dm_active_emails = [dm.user.email for dm in domain.permissions.all()] - dm_invited_emails = [ - invite.email for invite in invites_with_invited_status.filter(domain=domain) - ] + + dm_active_emails = domain_permissions_emails.get(domain_info.domain.name, []) + dm_invited_emails = domain_invitation_emails.get(domain_info.domain.name, []) # Set up the "matching headers" + row field data for email and status i = 0 # Declare i outside of the loop to avoid a reference before assignment in the second loop @@ -148,7 +148,7 @@ def _get_security_emails(sec_contact_ids): def update_columns_with_domain_managers( - domain_info,invites_with_invited_status, update_columns, columns, max_dm_total + domain_info,domain_invitation_emails, domain_permissions_emails, update_columns, columns, max_dm_total ): """Helper function that works with 'global' variables set in write_domains_csv Accepts: @@ -159,8 +159,25 @@ def update_columns_with_domain_managers( Returns: Updated update_columns, columns, max_dm_active, max_dm_invited, max_dm_total""" - dm_active = domain_info.domain.permissions.count() - dm_invited = invites_with_invited_status.filter(domain=domain_info.domain).count() + dm_active = 0 + dm_invited = 0 + try: + # logger.info(f'domain_invitation_emails {domain_invitation_emails[domain_info.domain.name]}') + + # Get the list of invitation emails for the domain name if it exists, otherwise, return an empty list + invitation_emails = domain_invitation_emails.get(domain_info.domain.name, []) + # Count the number of invitation emails + dm_invited = len(invitation_emails) + except KeyError: + pass + + try: + active_emails = domain_permissions_emails.get(domain_info.domain.name, []) + # Count the number of invitation emails + dm_active = len(active_emails) + except KeyError: + pass + if dm_active + dm_invited > max_dm_total: max_dm_total = dm_active + dm_invited @@ -193,61 +210,80 @@ def write_domains_csv( should_write_header: Conditional bc export_data_domain_growth_to_csv calls write_body twice """ - all_domain_infos = get_domain_infos(filter_condition, sort_fields) + with Timer(): + all_domain_infos = get_domain_infos(filter_condition, sort_fields) - # Store all security emails to avoid epp calls or excessive filters - sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) + # Store all security emails to avoid epp calls or excessive filters + sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) - security_emails_dict = _get_security_emails(sec_contact_ids) + security_emails_dict = _get_security_emails(sec_contact_ids) - # Reduce the memory overhead when performing the write operation - paginator = Paginator(all_domain_infos, 1000) + # Reduce the memory overhead when performing the write operation + paginator = Paginator(all_domain_infos, 1000) - # We get the number of domain managers (DMs) an the domain - # that has the most DMs so we can set the header row appropriately + # We get the number of domain managers (DMs) an the domain + # that has the most DMs so we can set the header row appropriately - max_dm_total = 0 - update_columns = False - invites_with_invited_status=None + max_dm_total = 0 + update_columns = False + + invites_with_invited_status=None + domain_invitation_emails = {} + domain_permissions_emails = {} - if get_domain_managers: - invites_with_invited_status = DomainInvitation.objects.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).prefetch_related("domain") + if get_domain_managers: + invites_with_invited_status = DomainInvitation.objects.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).select_related("domain") - # zander = DomainInformation.objects.filter(**filter_condition).annotate(invitations_count=Count('invitation', filter=Q(invitation__status='invited'))).values_list('domain_name', 'invitations_count') - # logger.info(f'zander {zander}') - # zander_dict = dict(zander) - # logger.info(f'zander_dict {zander_dict}') + # Iterate through each domain invitation and populate the dictionary + for invite in invites_with_invited_status: + domain_name = invite.domain.name + email = invite.email + if domain_name not in domain_invitation_emails: + domain_invitation_emails[domain_name] = [] + domain_invitation_emails[domain_name].append(email) - # This var will live outside of the nested for loops to aggregate - # the data from those loops - total_body_rows = [] + domain_permissions = UserDomainRole.objects.all() - for page_num in paginator.page_range: - rows = [] - page = paginator.page(page_num) - for domain_info in page.object_list: + # Iterate through each domain invitation and populate the dictionary + for permission in domain_permissions: + domain_name = permission.domain.name + email = permission.user.email + if domain_name not in domain_permissions_emails: + domain_permissions_emails[domain_name] = [] + domain_permissions_emails[domain_name].append(email) - # Get max number of domain managers - if get_domain_managers: - update_columns, columns, max_dm_total = ( - update_columns_with_domain_managers( - domain_info,invites_with_invited_status, update_columns, columns, max_dm_total + # logger.info(f'domain_invitation_emails {domain_invitation_emails}') + + # This var will live outside of the nested for loops to aggregate + # the data from those loops + total_body_rows = [] + + for page_num in paginator.page_range: + rows = [] + page = paginator.page(page_num) + for domain_info in page.object_list: + + # Get max number of domain managers + if get_domain_managers: + update_columns, columns, max_dm_total = ( + update_columns_with_domain_managers( + domain_info, domain_invitation_emails,domain_permissions_emails, update_columns, columns, max_dm_total + ) ) - ) - try: - row = parse_domain_row(columns, domain_info, security_emails_dict, get_domain_managers, invites_with_invited_status) - rows.append(row) - except ValueError: - # This should not happen. If it does, just skip this row. - # It indicates that DomainInformation.domain is None. - logger.error("csv_export -> Error when parsing row, domain was None") - continue - total_body_rows.extend(rows) + try: + row = parse_domain_row(columns, domain_info, security_emails_dict, get_domain_managers, domain_invitation_emails,domain_permissions_emails) + rows.append(row) + except ValueError: + # This should not happen. If it does, just skip this row. + # It indicates that DomainInformation.domain is None. + logger.error("csv_export -> Error when parsing row, domain was None") + continue + total_body_rows.extend(rows) - if should_write_header: - write_header(writer, columns) - writer.writerows(total_body_rows) + if should_write_header: + write_header(writer, columns) + writer.writerows(total_body_rows) def get_requests(filter_condition, sort_fields): @@ -360,6 +396,7 @@ def export_data_type_to_csv(csv_file): Domain.State.READY, Domain.State.DNS_NEEDED, Domain.State.ON_HOLD, + Domain.State.UNKNOWN, ], } write_domains_csv( From 045140091b11254b389eb0329fb70b342d2cedf3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 2 Apr 2024 12:44:00 -0400 Subject: [PATCH 31/45] do not pull unknown domains --- src/registrar/utility/csv_export.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index d8638d0d9..913fe95db 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -396,7 +396,6 @@ def export_data_type_to_csv(csv_file): Domain.State.READY, Domain.State.DNS_NEEDED, Domain.State.ON_HOLD, - Domain.State.UNKNOWN, ], } write_domains_csv( From 03622204ce5216bbf49653c9e156e5f701ccae11 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 2 Apr 2024 13:13:43 -0400 Subject: [PATCH 32/45] Hide sticky requested domain in mobile --- src/registrar/assets/sass/_theme/_admin.scss | 10 ++++++++++ .../django/admin/domain_request_change_form.html | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index a537fc872..f0cc7fafb 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -476,6 +476,16 @@ address.dja-address-contact-list { } } +.visible-768 { + display: none; +} + +@media screen and (min-width:768px) { + .visible-768 { + display: block; + } +} + @media screen and (min-width:935px) { // Analyst only class .submit-row-wrapper--analyst-view { diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index be66c7c81..3b4fa7283 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -116,7 +116,7 @@ -

+

Requested domain: {{ original.requested_domain.name }}

{{ block.super }} From d33cc1c8ccf51d8b7d0c7365ba3c990463ec2a87 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 12:17:59 -0600 Subject: [PATCH 33/45] Remove perms --- .../migrations/0081_create_groups_v10.py | 37 +++++++++++++++++++ src/registrar/models/user_group.py | 5 --- 2 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 src/registrar/migrations/0081_create_groups_v10.py diff --git a/src/registrar/migrations/0081_create_groups_v10.py b/src/registrar/migrations/0081_create_groups_v10.py new file mode 100644 index 000000000..d65b6dbd2 --- /dev/null +++ b/src/registrar/migrations/0081_create_groups_v10.py @@ -0,0 +1,37 @@ +# This migration creates the create_full_access_group and create_cisa_analyst_group groups +# It is dependent on 0079 (which populates federal agencies) +# If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS +# in the user_group model then: +# [NOT RECOMMENDED] +# step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions +# step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups +# step 3: fake run the latest migration in the migrations list +# [RECOMMENDED] +# Alternatively: +# step 1: duplicate the migration that loads data +# step 2: docker-compose exec app ./manage.py migrate + +from django.db import migrations +from registrar.models import UserGroup +from typing import Any + + +# For linting: RunPython expects a function reference, +# so let's give it one +def create_groups(apps, schema_editor) -> Any: + UserGroup.create_cisa_analyst_group(apps, schema_editor) + UserGroup.create_full_access_group(apps, schema_editor) + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0080_create_groups_v09"), + ] + + operations = [ + migrations.RunPython( + create_groups, + reverse_code=migrations.RunPython.noop, + atomic=True, + ), + ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index 2aa2f642e..e8636a462 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -26,11 +26,6 @@ class UserGroup(Group): "model": "contact", "permissions": ["change_contact"], }, - { - "app_label": "registrar", - "model": "domaininformation", - "permissions": ["change_domaininformation"], - }, { "app_label": "registrar", "model": "domainrequest", From b942d348928cde4feb7c796f3a386cac5a31561f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 12:58:17 -0600 Subject: [PATCH 34/45] Add has change perm --- src/registrar/admin.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5137300c7..e04509776 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1408,6 +1408,11 @@ class DomainInformationInline(admin.StackedInline): "submitter", ] + def has_change_permission(self, request, obj=None): + if request.user.has_perm("registrar.analyst_access_permission"): + return True + return super().has_change_permission(request, obj) + def formfield_for_manytomany(self, db_field, request, **kwargs): """customize the behavior of formfields with manytomany relationships. the customized behavior includes sorting of objects in lists as well as customizing helper text""" From 2150873ec1de8677dc0a8122d907945ead0eade7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 13:00:46 -0600 Subject: [PATCH 35/45] Update admin.py --- src/registrar/admin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e04509776..d16f5e315 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1409,6 +1409,8 @@ class DomainInformationInline(admin.StackedInline): ] def has_change_permission(self, request, obj=None): + """Custom has_change_permission override so that we can specify that + analysts can edit this through this inline, but not through the model normally""" if request.user.has_perm("registrar.analyst_access_permission"): return True return super().has_change_permission(request, obj) From b12669d7a5b52e61fac6feb9ea8a77a12da24049 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 14:27:58 -0600 Subject: [PATCH 36/45] Fix unit tests --- src/registrar/tests/test_admin.py | 8 ++++---- src/registrar/tests/test_migrations.py | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 7c0c81db4..dcffce6f8 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1960,8 +1960,8 @@ class TestDomainInformationAdmin(TestCase): # Get the other contact other_contact = domain_info.other_contacts.all().first() - p = "userpass" - self.client.login(username="staffuser", password=p) + p = "adminpass" + self.client.login(username="superuser", password=p) response = self.client.get( "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), @@ -2005,8 +2005,8 @@ class TestDomainInformationAdmin(TestCase): domain_request.approve() domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get() - p = "userpass" - self.client.login(username="staffuser", password=p) + p = "adminpass" + self.client.login(username="superuser", password=p) response = self.client.get( "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), follow=True, diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index bf3b09d0d..add65105a 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -34,7 +34,6 @@ class TestGroups(TestCase): "view_logentry", "change_contact", "view_domain", - "change_domaininformation", "add_domaininvitation", "view_domaininvitation", "change_domainrequest", From 70bd79516abc00ef875aec3013eff9b93bbcb8ec Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 2 Apr 2024 15:38:12 -0600 Subject: [PATCH 37/45] Use sender --- src/registrar/signals.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/registrar/signals.py b/src/registrar/signals.py index ad287219d..d106f974c 100644 --- a/src/registrar/signals.py +++ b/src/registrar/signals.py @@ -11,7 +11,7 @@ logger = logging.getLogger(__name__) @receiver(pre_save, sender=DomainRequest) @receiver(pre_save, sender=DomainInformation) -def create_or_update_organization_type(sender, instance, **kwargs): +def create_or_update_organization_type(sender: DomainRequest | DomainInformation, instance, **kwargs): """The organization_type field on DomainRequest and DomainInformation is consituted from the generic_org_type and is_election_board fields. To keep the organization_type field up to date, we need to update it before save based off of those field @@ -62,15 +62,7 @@ def create_or_update_organization_type(sender, instance, **kwargs): # == Init variables == # # Instance is already in the database, fetch its current state - if isinstance(instance, DomainRequest): - current_instance = DomainRequest.objects.get(id=instance.id) - elif isinstance(instance, DomainInformation): - current_instance = DomainInformation.objects.get(id=instance.id) - else: - # This should never occur. But it never hurts to have this check anyway. - raise ValueError( - "create_or_update_organization_type() -> instance was not DomainRequest or DomainInformation" - ) + current_instance = sender.objects.get(id=instance.id) # Check the new and old values generic_org_type_changed = instance.generic_org_type != current_instance.generic_org_type From c5e6295a8a5c25f480c0465b2dd328224463cde1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 2 Apr 2024 20:54:43 -0400 Subject: [PATCH 38/45] refactor wip --- src/registrar/tests/test_reports.py | 39 +++-- src/registrar/utility/csv_export.py | 247 +++++++++++++++------------- src/registrar/views/admin_views.py | 10 +- 3 files changed, 162 insertions(+), 134 deletions(-) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index cd882c4f8..b4861560f 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -243,7 +243,12 @@ class ExportDataTest(MockDb, MockEppLib): self.maxDiff = None # Call the export functions write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, + columns, + sort_fields, + filter_condition, + should_get_domain_managers=False, + should_write_header=True, ) # Reset the CSV file's position to the beginning @@ -305,7 +310,12 @@ class ExportDataTest(MockDb, MockEppLib): } # Call the export functions write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, + columns, + sort_fields, + filter_condition, + should_get_domain_managers=False, + should_write_header=True, ) # Reset the CSV file's position to the beginning csv_file.seek(0) @@ -358,7 +368,12 @@ class ExportDataTest(MockDb, MockEppLib): } # Call the export functions write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, + columns, + sort_fields, + filter_condition, + should_get_domain_managers=False, + should_write_header=True, ) # Reset the CSV file's position to the beginning csv_file.seek(0) @@ -438,7 +453,7 @@ class ExportDataTest(MockDb, MockEppLib): columns, sort_fields, filter_condition, - get_domain_managers=False, + should_get_domain_managers=False, should_write_header=True, ) write_domains_csv( @@ -446,7 +461,7 @@ class ExportDataTest(MockDb, MockEppLib): columns, sort_fields_for_deleted_domains, filter_conditions_for_deleted_domains, - get_domain_managers=False, + should_get_domain_managers=False, should_write_header=False, ) # Reset the CSV file's position to the beginning @@ -514,7 +529,12 @@ class ExportDataTest(MockDb, MockEppLib): self.maxDiff = None # Call the export functions write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=True, should_write_header=True + writer, + columns, + sort_fields, + filter_condition, + should_get_domain_managers=True, + should_write_header=True, ) # Reset the CSV file's position to the beginning @@ -697,13 +717,8 @@ class HelperFunctions(MockDb): "domain__first_ready__lte": self.end_date, } # Test with distinct - managed_domains_sliced_at_end_date = get_sliced_domains(filter_condition, True) - expected_content = [3, 2, 1, 0, 0, 0, 0, 0, 0, 2] - self.assertEqual(managed_domains_sliced_at_end_date, expected_content) - - # Test without distinct managed_domains_sliced_at_end_date = get_sliced_domains(filter_condition) - expected_content = [3, 4, 1, 0, 0, 0, 0, 0, 0, 2] + expected_content = [3, 2, 1, 0, 0, 0, 0, 0, 0, 2] self.assertEqual(managed_domains_sliced_at_end_date, expected_content) def test_get_sliced_requests(self): diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 913fe95db..faad25b28 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -7,7 +7,7 @@ from registrar.models.domain_request import DomainRequest from registrar.models.domain_information import DomainInformation from django.utils import timezone from django.core.paginator import Paginator -from django.db.models import F, Value, CharField, Q, Count +from django.db.models import F, Value, CharField from django.db.models.functions import Concat, Coalesce from registrar.models.public_contact import PublicContact @@ -54,7 +54,14 @@ def get_domain_infos(filter_condition, sort_fields): return domain_infos_cleaned -def parse_domain_row(columns, domain_info: DomainInformation, security_emails_dict=None, get_domain_managers=False, domain_invitation_emails=None, domain_permissions_emails=None): +def parse_domain_row( + columns, + domain_info: DomainInformation, + dict_security_emails_dict=None, + should_get_domain_managers=False, + dict_domain_invitations_with_invited_status=None, + dict_user_domain_roles=None, +): """Given a set of columns, generate a new row from cleaned column data""" # Domain should never be none when parsing this information @@ -66,8 +73,8 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di # Grab the security email from a preset dictionary. # If nothing exists in the dictionary, grab from .contacts. - if security_emails_dict is not None and domain.name in security_emails_dict: - _email = security_emails_dict.get(domain.name) + if dict_security_emails_dict is not None and domain.name in dict_security_emails_dict: + _email = dict_security_emails_dict.get(domain.name) security_email = _email if _email is not None else " " else: # If the dictionary doesn't contain that data, lets filter for it manually. @@ -104,20 +111,20 @@ def parse_domain_row(columns, domain_info: DomainInformation, security_emails_di "Deleted": domain.deleted, } - if get_domain_managers: + if should_get_domain_managers: # Get lists of emails for active and invited domain managers - dm_active_emails = domain_permissions_emails.get(domain_info.domain.name, []) - dm_invited_emails = domain_invitation_emails.get(domain_info.domain.name, []) + dms_active_emails = dict_user_domain_roles.get(domain_info.domain.name, []) + dms_invited_emails = dict_domain_invitations_with_invited_status.get(domain_info.domain.name, []) # Set up the "matching headers" + row field data for email and status i = 0 # Declare i outside of the loop to avoid a reference before assignment in the second loop - for i, dm_email in enumerate(dm_active_emails, start=1): + for i, dm_email in enumerate(dms_active_emails, start=1): FIELDS[f"Domain manager {i}"] = dm_email FIELDS[f"DM{i} status"] = "R" # Continue enumeration from where we left off and add data for invited domain managers - for j, dm_email in enumerate(dm_invited_emails, start=i + 1): + for j, dm_email in enumerate(dms_invited_emails, start=i + 1): FIELDS[f"Domain manager {j}"] = dm_email FIELDS[f"DM{j} status"] = "I" @@ -129,7 +136,7 @@ def _get_security_emails(sec_contact_ids): """ Retrieve security contact emails for the given security contact IDs. """ - security_emails_dict = {} + dict_security_emails_dict = {} public_contacts = ( PublicContact.objects.only("email", "domain__name") .select_related("domain") @@ -139,144 +146,152 @@ def _get_security_emails(sec_contact_ids): # Populate a dictionary of domain names and their security contacts for contact in public_contacts: domain: Domain = contact.domain - if domain is not None and domain.name not in security_emails_dict: - security_emails_dict[domain.name] = contact.email + if domain is not None and domain.name not in dict_security_emails_dict: + dict_security_emails_dict[domain.name] = contact.email else: logger.warning("csv_export -> Domain was none for PublicContact") - return security_emails_dict + return dict_security_emails_dict + + +def count_domain_managers(domain_name, dict_domain_invitations_with_invited_status, dict_user_domain_roles): + """Count active and invited domain managers""" + dms_active = len(dict_user_domain_roles.get(domain_name, [])) + dms_invited = len(dict_domain_invitations_with_invited_status.get(domain_name, [])) + return dms_active, dms_invited + + +def update_columns(columns, dms_total, should_update_columns): + """Update columns if necessary""" + if should_update_columns: + for i in range(1, dms_total + 1): + email_column_header = f"Domain manager {i}" + status_column_header = f"DM{i} status" + if email_column_header not in columns: + columns.append(email_column_header) + columns.append(status_column_header) + should_update_columns = False + return columns, should_update_columns, dms_total def update_columns_with_domain_managers( - domain_info,domain_invitation_emails, domain_permissions_emails, update_columns, columns, max_dm_total + columns, + domain_info, + should_update_columns, + dms_total, + dict_domain_invitations_with_invited_status, + dict_user_domain_roles, ): - """Helper function that works with 'global' variables set in write_domains_csv - Accepts: - domain_info -> Domains to parse - update_columns -> A control to make sure we only run the columns test and update when needed - columns -> The header cells in the csv that's under construction - max_dm_total -> Starts at 0 and gets updated and passed again through this method - Returns: - Updated update_columns, columns, max_dm_active, max_dm_invited, max_dm_total""" + """Helper function to update columns with domain manager information""" - dm_active = 0 - dm_invited = 0 - try: - # logger.info(f'domain_invitation_emails {domain_invitation_emails[domain_info.domain.name]}') - - # Get the list of invitation emails for the domain name if it exists, otherwise, return an empty list - invitation_emails = domain_invitation_emails.get(domain_info.domain.name, []) - # Count the number of invitation emails - dm_invited = len(invitation_emails) - except KeyError: - pass + domain_name = domain_info.domain.name try: - active_emails = domain_permissions_emails.get(domain_info.domain.name, []) - # Count the number of invitation emails - dm_active = len(active_emails) - except KeyError: - pass - + dms_active, dms_invited = count_domain_managers( + domain_name, dict_domain_invitations_with_invited_status, dict_user_domain_roles + ) - if dm_active + dm_invited > max_dm_total: - max_dm_total = dm_active + dm_invited - update_columns = True + if dms_active + dms_invited > dms_total: + dms_total = dms_active + dms_invited + should_update_columns = True - if update_columns: - for i in range(1, max_dm_total + 1): - column_name = f"Domain manager {i}" - column2_name = f"DM{i} status" - if column_name not in columns: - columns.append(column_name) - columns.append(column2_name) - update_columns = False + except Exception as err: + logger.error(f"Exception while parsing domain managers for reports: {err}") - return update_columns, columns, max_dm_total + return update_columns(columns, dms_total, should_update_columns) +def build_dictionaries_for_domain_managers(dict_user_domain_roles, dict_domain_invitations_with_invited_status): + """Helper function that builds dicts for invited users and active domain + managers. We do so to avoid filtering within loops.""" + + user_domain_roles = None + user_domain_roles = UserDomainRole.objects.all() + + # Iterate through each user domain role and populate the dictionary + for user_domain_role in user_domain_roles: + domain_name = user_domain_role.domain.name + email = user_domain_role.user.email + if domain_name not in dict_user_domain_roles: + dict_user_domain_roles[domain_name] = [] + dict_user_domain_roles[domain_name].append(email) + + domain_invitations_with_invited_status = None + domain_invitations_with_invited_status = DomainInvitation.objects.filter( + status=DomainInvitation.DomainInvitationStatus.INVITED + ).select_related("domain") + + # Iterate through each domain invitation and populate the dictionary + for invite in domain_invitations_with_invited_status: + domain_name = invite.domain.name + email = invite.email + if domain_name not in dict_domain_invitations_with_invited_status: + dict_domain_invitations_with_invited_status[domain_name] = [] + dict_domain_invitations_with_invited_status[domain_name].append(email) + + return dict_user_domain_roles, dict_domain_invitations_with_invited_status + def write_domains_csv( writer, columns, sort_fields, filter_condition, - get_domain_managers=False, + should_get_domain_managers=False, should_write_header=True, ): """ Receives params from the parent methods and outputs a CSV with filtered and sorted domains. Works with write_header as long as the same writer object is passed. - get_domain_managers: Conditional bc we only use domain manager info for export_data_full_to_csv + should_get_domain_managers: Conditional bc we only use domain manager info for export_data_full_to_csv should_write_header: Conditional bc export_data_domain_growth_to_csv calls write_body twice """ with Timer(): + # Retrieve domain information and all sec emails all_domain_infos = get_domain_infos(filter_condition, sort_fields) - - # Store all security emails to avoid epp calls or excessive filters sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) - - security_emails_dict = _get_security_emails(sec_contact_ids) - - # Reduce the memory overhead when performing the write operation + dict_security_emails_dict = _get_security_emails(sec_contact_ids) paginator = Paginator(all_domain_infos, 1000) - # We get the number of domain managers (DMs) an the domain - # that has the most DMs so we can set the header row appropriately - - max_dm_total = 0 - update_columns = False - - invites_with_invited_status=None - domain_invitation_emails = {} - domain_permissions_emails = {} - - if get_domain_managers: - invites_with_invited_status = DomainInvitation.objects.filter(status=DomainInvitation.DomainInvitationStatus.INVITED).select_related("domain") - - # Iterate through each domain invitation and populate the dictionary - for invite in invites_with_invited_status: - domain_name = invite.domain.name - email = invite.email - if domain_name not in domain_invitation_emails: - domain_invitation_emails[domain_name] = [] - domain_invitation_emails[domain_name].append(email) - - domain_permissions = UserDomainRole.objects.all() - - # Iterate through each domain invitation and populate the dictionary - for permission in domain_permissions: - domain_name = permission.domain.name - email = permission.user.email - if domain_name not in domain_permissions_emails: - domain_permissions_emails[domain_name] = [] - domain_permissions_emails[domain_name].append(email) - - # logger.info(f'domain_invitation_emails {domain_invitation_emails}') - - # This var will live outside of the nested for loops to aggregate - # the data from those loops + # Initialize variables + dms_total = 0 + should_update_columns = False total_body_rows = [] + dict_user_domain_roles = {} + dict_domain_invitations_with_invited_status = {} + # Build dictionaries if necessary + if should_get_domain_managers: + dict_user_domain_roles, dict_domain_invitations_with_invited_status = build_dictionaries_for_domain_managers( + dict_user_domain_roles, dict_domain_invitations_with_invited_status + ) + + # Process domain information for page_num in paginator.page_range: rows = [] page = paginator.page(page_num) for domain_info in page.object_list: - - # Get max number of domain managers - if get_domain_managers: - update_columns, columns, max_dm_total = ( - update_columns_with_domain_managers( - domain_info, domain_invitation_emails,domain_permissions_emails, update_columns, columns, max_dm_total - ) + if should_get_domain_managers: + columns, dms_total, should_update_columns = update_columns_with_domain_managers( + columns, + domain_info, + should_update_columns, + dms_total, + dict_domain_invitations_with_invited_status, + dict_user_domain_roles, ) try: - row = parse_domain_row(columns, domain_info, security_emails_dict, get_domain_managers, domain_invitation_emails,domain_permissions_emails) + row = parse_domain_row( + columns, + domain_info, + dict_security_emails_dict, + should_get_domain_managers, + dict_domain_invitations_with_invited_status, + dict_user_domain_roles, + ) rows.append(row) except ValueError: - # This should not happen. If it does, just skip this row. - # It indicates that DomainInformation.domain is None. logger.error("csv_export -> Error when parsing row, domain was None") continue total_body_rows.extend(rows) @@ -399,7 +414,7 @@ def export_data_type_to_csv(csv_file): ], } write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=True, should_write_header=True + writer, columns, sort_fields, filter_condition, should_get_domain_managers=True, should_write_header=True ) @@ -432,7 +447,7 @@ def export_data_full_to_csv(csv_file): ], } write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) @@ -466,7 +481,7 @@ def export_data_federal_to_csv(csv_file): ], } write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) @@ -536,19 +551,19 @@ def export_data_domain_growth_to_csv(csv_file, start_date, end_date): } write_domains_csv( - writer, columns, sort_fields, filter_condition, get_domain_managers=False, should_write_header=True + writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) write_domains_csv( writer, columns, sort_fields_for_deleted_domains, filter_condition_for_deleted_domains, - get_domain_managers=False, + should_get_domain_managers=False, should_write_header=False, ) -def get_sliced_domains(filter_condition, distinct=False): +def get_sliced_domains(filter_condition): """Get filtered domains counts sliced by org type and election office. Pass distinct=True when filtering by permissions so we do not to count multiples when a domain has more that one manager. @@ -639,7 +654,7 @@ def export_data_managed_domains_to_csv(csv_file, start_date, end_date): "domain__permissions__isnull": False, "domain__first_ready__lte": start_date_formatted, } - managed_domains_sliced_at_start_date = get_sliced_domains(filter_managed_domains_start_date, True) + managed_domains_sliced_at_start_date = get_sliced_domains(filter_managed_domains_start_date) writer.writerow(["MANAGED DOMAINS COUNTS AT START DATE"]) writer.writerow( @@ -663,7 +678,7 @@ def export_data_managed_domains_to_csv(csv_file, start_date, end_date): "domain__permissions__isnull": False, "domain__first_ready__lte": end_date_formatted, } - managed_domains_sliced_at_end_date = get_sliced_domains(filter_managed_domains_end_date, True) + managed_domains_sliced_at_end_date = get_sliced_domains(filter_managed_domains_end_date) writer.writerow(["MANAGED DOMAINS COUNTS AT END DATE"]) writer.writerow( @@ -688,7 +703,7 @@ def export_data_managed_domains_to_csv(csv_file, start_date, end_date): columns, sort_fields, filter_managed_domains_end_date, - get_domain_managers=True, + should_get_domain_managers=True, should_write_header=True, ) @@ -712,7 +727,7 @@ def export_data_unmanaged_domains_to_csv(csv_file, start_date, end_date): "domain__permissions__isnull": True, "domain__first_ready__lte": start_date_formatted, } - unmanaged_domains_sliced_at_start_date = get_sliced_domains(filter_unmanaged_domains_start_date, True) + unmanaged_domains_sliced_at_start_date = get_sliced_domains(filter_unmanaged_domains_start_date) writer.writerow(["UNMANAGED DOMAINS AT START DATE"]) writer.writerow( @@ -736,7 +751,7 @@ def export_data_unmanaged_domains_to_csv(csv_file, start_date, end_date): "domain__permissions__isnull": True, "domain__first_ready__lte": end_date_formatted, } - unmanaged_domains_sliced_at_end_date = get_sliced_domains(filter_unmanaged_domains_end_date, True) + unmanaged_domains_sliced_at_end_date = get_sliced_domains(filter_unmanaged_domains_end_date) writer.writerow(["UNMANAGED DOMAINS AT END DATE"]) writer.writerow( @@ -761,7 +776,7 @@ def export_data_unmanaged_domains_to_csv(csv_file, start_date, end_date): columns, sort_fields, filter_unmanaged_domains_end_date, - get_domain_managers=False, + should_get_domain_managers=False, should_write_header=True, ) diff --git a/src/registrar/views/admin_views.py b/src/registrar/views/admin_views.py index eba8423ed..01a8157f9 100644 --- a/src/registrar/views/admin_views.py +++ b/src/registrar/views/admin_views.py @@ -49,8 +49,8 @@ class AnalyticsView(View): "domain__permissions__isnull": False, "domain__first_ready__lte": end_date_formatted, } - managed_domains_sliced_at_start_date = csv_export.get_sliced_domains(filter_managed_domains_start_date, True) - managed_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_managed_domains_end_date, True) + managed_domains_sliced_at_start_date = csv_export.get_sliced_domains(filter_managed_domains_start_date) + managed_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_managed_domains_end_date) filter_unmanaged_domains_start_date = { "domain__permissions__isnull": True, @@ -60,10 +60,8 @@ class AnalyticsView(View): "domain__permissions__isnull": True, "domain__first_ready__lte": end_date_formatted, } - unmanaged_domains_sliced_at_start_date = csv_export.get_sliced_domains( - filter_unmanaged_domains_start_date, True - ) - unmanaged_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_unmanaged_domains_end_date, True) + unmanaged_domains_sliced_at_start_date = csv_export.get_sliced_domains(filter_unmanaged_domains_start_date) + unmanaged_domains_sliced_at_end_date = csv_export.get_sliced_domains(filter_unmanaged_domains_end_date) filter_ready_domains_start_date = { "domain__state__in": [models.Domain.State.READY], From 77d1158c1b34b8c40bd139009bb5c7784fad4149 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 2 Apr 2024 21:49:42 -0400 Subject: [PATCH 39/45] Clean up --- src/registrar/tests/test_reports.py | 20 ++--- src/registrar/utility/csv_export.py | 123 ++++++++++++++-------------- 2 files changed, 71 insertions(+), 72 deletions(-) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index b4861560f..b34f3d920 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -9,10 +9,10 @@ from registrar.utility.csv_export import ( export_data_unmanaged_domains_to_csv, get_sliced_domains, get_sliced_requests, - write_domains_csv, + write_csv_for_domains, get_default_start_date, get_default_end_date, - write_requests_csv, + write_csv_for_requests, ) from django.core.management import call_command @@ -242,7 +242,7 @@ class ExportDataTest(MockDb, MockEppLib): } self.maxDiff = None # Call the export functions - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -273,7 +273,7 @@ class ExportDataTest(MockDb, MockEppLib): expected_content = expected_content.replace(",,", "").replace(",", "").replace(" ", "").strip() self.assertEqual(csv_content, expected_content) - def test_write_domains_csv(self): + def test_write_csv_for_domains(self): """Test that write_body returns the existing domain, test that sort by domain name works, test that filter works""" @@ -309,7 +309,7 @@ class ExportDataTest(MockDb, MockEppLib): ], } # Call the export functions - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -367,7 +367,7 @@ class ExportDataTest(MockDb, MockEppLib): ], } # Call the export functions - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -448,7 +448,7 @@ class ExportDataTest(MockDb, MockEppLib): } # Call the export functions - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -456,7 +456,7 @@ class ExportDataTest(MockDb, MockEppLib): should_get_domain_managers=False, should_write_header=True, ) - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields_for_deleted_domains, @@ -528,7 +528,7 @@ class ExportDataTest(MockDb, MockEppLib): } self.maxDiff = None # Call the export functions - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -673,7 +673,7 @@ class ExportDataTest(MockDb, MockEppLib): "submission_date__lte": self.end_date, "submission_date__gte": self.start_date, } - write_requests_csv(writer, columns, sort_fields, filter_condition, should_write_header=True) + write_csv_for_requests(writer, columns, sort_fields, filter_condition, should_write_header=True) # Reset the CSV file's position to the beginning csv_file.seek(0) # Read the content into a variable diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index faad25b28..7605d5bfe 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -12,7 +12,6 @@ from django.db.models.functions import Concat, Coalesce from registrar.models.public_contact import PublicContact from registrar.models.user_domain_role import UserDomainRole -from registrar.models.utility.generic_helper import Timer from registrar.utility.enums import DefaultEmail logger = logging.getLogger(__name__) @@ -54,7 +53,7 @@ def get_domain_infos(filter_condition, sort_fields): return domain_infos_cleaned -def parse_domain_row( +def parse_row_for_domain( columns, domain_info: DomainInformation, dict_security_emails_dict=None, @@ -231,7 +230,8 @@ def build_dictionaries_for_domain_managers(dict_user_domain_roles, dict_domain_i return dict_user_domain_roles, dict_domain_invitations_with_invited_status -def write_domains_csv( + +def write_csv_for_domains( writer, columns, sort_fields, @@ -246,59 +246,58 @@ def write_domains_csv( should_write_header: Conditional bc export_data_domain_growth_to_csv calls write_body twice """ - with Timer(): - # Retrieve domain information and all sec emails - all_domain_infos = get_domain_infos(filter_condition, sort_fields) - sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) - dict_security_emails_dict = _get_security_emails(sec_contact_ids) - paginator = Paginator(all_domain_infos, 1000) + # Retrieve domain information and all sec emails + all_domain_infos = get_domain_infos(filter_condition, sort_fields) + sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) + dict_security_emails_dict = _get_security_emails(sec_contact_ids) + paginator = Paginator(all_domain_infos, 1000) - # Initialize variables - dms_total = 0 - should_update_columns = False - total_body_rows = [] - dict_user_domain_roles = {} - dict_domain_invitations_with_invited_status = {} + # Initialize variables + dms_total = 0 + should_update_columns = False + total_body_rows = [] + dict_user_domain_roles = {} + dict_domain_invitations_with_invited_status = {} - # Build dictionaries if necessary - if should_get_domain_managers: - dict_user_domain_roles, dict_domain_invitations_with_invited_status = build_dictionaries_for_domain_managers( - dict_user_domain_roles, dict_domain_invitations_with_invited_status - ) + # Build dictionaries if necessary + if should_get_domain_managers: + dict_user_domain_roles, dict_domain_invitations_with_invited_status = build_dictionaries_for_domain_managers( + dict_user_domain_roles, dict_domain_invitations_with_invited_status + ) - # Process domain information - for page_num in paginator.page_range: - rows = [] - page = paginator.page(page_num) - for domain_info in page.object_list: - if should_get_domain_managers: - columns, dms_total, should_update_columns = update_columns_with_domain_managers( - columns, - domain_info, - should_update_columns, - dms_total, - dict_domain_invitations_with_invited_status, - dict_user_domain_roles, - ) + # Process domain information + for page_num in paginator.page_range: + rows = [] + page = paginator.page(page_num) + for domain_info in page.object_list: + if should_get_domain_managers: + columns, dms_total, should_update_columns = update_columns_with_domain_managers( + columns, + domain_info, + should_update_columns, + dms_total, + dict_domain_invitations_with_invited_status, + dict_user_domain_roles, + ) - try: - row = parse_domain_row( - columns, - domain_info, - dict_security_emails_dict, - should_get_domain_managers, - dict_domain_invitations_with_invited_status, - dict_user_domain_roles, - ) - rows.append(row) - except ValueError: - logger.error("csv_export -> Error when parsing row, domain was None") - continue - total_body_rows.extend(rows) + try: + row = parse_row_for_domain( + columns, + domain_info, + dict_security_emails_dict, + should_get_domain_managers, + dict_domain_invitations_with_invited_status, + dict_user_domain_roles, + ) + rows.append(row) + except ValueError: + logger.error("csv_export -> Error when parsing row, domain was None") + continue + total_body_rows.extend(rows) - if should_write_header: - write_header(writer, columns) - writer.writerows(total_body_rows) + if should_write_header: + write_header(writer, columns) + writer.writerows(total_body_rows) def get_requests(filter_condition, sort_fields): @@ -312,7 +311,7 @@ def get_requests(filter_condition, sort_fields): return requests -def parse_request_row(columns, request: DomainRequest): +def parse_row_for_requests(columns, request: DomainRequest): """Given a set of columns, generate a new row from cleaned column data""" requested_domain_name = "No requested domain" @@ -344,7 +343,7 @@ def parse_request_row(columns, request: DomainRequest): return row -def write_requests_csv( +def write_csv_for_requests( writer, columns, sort_fields, @@ -365,7 +364,7 @@ def write_requests_csv( rows = [] for request in page.object_list: try: - row = parse_request_row(columns, request) + row = parse_row_for_requests(columns, request) rows.append(row) except ValueError: # This should not happen. If it does, just skip this row. @@ -413,7 +412,7 @@ def export_data_type_to_csv(csv_file): Domain.State.ON_HOLD, ], } - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, filter_condition, should_get_domain_managers=True, should_write_header=True ) @@ -446,7 +445,7 @@ def export_data_full_to_csv(csv_file): Domain.State.ON_HOLD, ], } - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) @@ -480,7 +479,7 @@ def export_data_federal_to_csv(csv_file): Domain.State.ON_HOLD, ], } - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) @@ -550,10 +549,10 @@ def export_data_domain_growth_to_csv(csv_file, start_date, end_date): "domain__deleted__gte": start_date_formatted, } - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, filter_condition, should_get_domain_managers=False, should_write_header=True ) - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields_for_deleted_domains, @@ -698,7 +697,7 @@ def export_data_managed_domains_to_csv(csv_file, start_date, end_date): writer.writerow(managed_domains_sliced_at_end_date) writer.writerow([]) - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -771,7 +770,7 @@ def export_data_unmanaged_domains_to_csv(csv_file, start_date, end_date): writer.writerow(unmanaged_domains_sliced_at_end_date) writer.writerow([]) - write_domains_csv( + write_csv_for_domains( writer, columns, sort_fields, @@ -807,4 +806,4 @@ def export_data_requests_growth_to_csv(csv_file, start_date, end_date): "submission_date__gte": start_date_formatted, } - write_requests_csv(writer, columns, sort_fields, filter_condition, should_write_header=True) + write_csv_for_requests(writer, columns, sort_fields, filter_condition, should_write_header=True) From c1527a08af6348c582bc6ee3b35dcf5b1f5fbac4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 3 Apr 2024 06:59:08 -0400 Subject: [PATCH 40/45] added end of file newline --- .../templates/django/admin/multiple_choice_list_filter.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/multiple_choice_list_filter.html b/src/registrar/templates/django/admin/multiple_choice_list_filter.html index 409695240..66643f4ec 100644 --- a/src/registrar/templates/django/admin/multiple_choice_list_filter.html +++ b/src/registrar/templates/django/admin/multiple_choice_list_filter.html @@ -34,4 +34,4 @@ {% endif %} {% endfor %} - \ No newline at end of file + From 9d8d90e45a1f6bf418c9c35cac268601f5b130cf Mon Sep 17 00:00:00 2001 From: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> Date: Wed, 3 Apr 2024 11:26:23 -0400 Subject: [PATCH 41/45] Update src/registrar/utility/csv_export.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- src/registrar/utility/csv_export.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 7605d5bfe..01eef295a 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -204,7 +204,6 @@ def build_dictionaries_for_domain_managers(dict_user_domain_roles, dict_domain_i """Helper function that builds dicts for invited users and active domain managers. We do so to avoid filtering within loops.""" - user_domain_roles = None user_domain_roles = UserDomainRole.objects.all() # Iterate through each user domain role and populate the dictionary From 602f07f2194629a3fa2130d1d2c5b7224587eb26 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 3 Apr 2024 11:31:46 -0400 Subject: [PATCH 42/45] var name correction --- src/registrar/utility/csv_export.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 01eef295a..949b0adcd 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -56,7 +56,7 @@ def get_domain_infos(filter_condition, sort_fields): def parse_row_for_domain( columns, domain_info: DomainInformation, - dict_security_emails_dict=None, + dict_security_emails=None, should_get_domain_managers=False, dict_domain_invitations_with_invited_status=None, dict_user_domain_roles=None, @@ -72,8 +72,8 @@ def parse_row_for_domain( # Grab the security email from a preset dictionary. # If nothing exists in the dictionary, grab from .contacts. - if dict_security_emails_dict is not None and domain.name in dict_security_emails_dict: - _email = dict_security_emails_dict.get(domain.name) + if dict_security_emails is not None and domain.name in dict_security_emails: + _email = dict_security_emails.get(domain.name) security_email = _email if _email is not None else " " else: # If the dictionary doesn't contain that data, lets filter for it manually. @@ -135,7 +135,7 @@ def _get_security_emails(sec_contact_ids): """ Retrieve security contact emails for the given security contact IDs. """ - dict_security_emails_dict = {} + dict_security_emails = {} public_contacts = ( PublicContact.objects.only("email", "domain__name") .select_related("domain") @@ -145,12 +145,12 @@ def _get_security_emails(sec_contact_ids): # Populate a dictionary of domain names and their security contacts for contact in public_contacts: domain: Domain = contact.domain - if domain is not None and domain.name not in dict_security_emails_dict: - dict_security_emails_dict[domain.name] = contact.email + if domain is not None and domain.name not in dict_security_emails: + dict_security_emails[domain.name] = contact.email else: logger.warning("csv_export -> Domain was none for PublicContact") - return dict_security_emails_dict + return dict_security_emails def count_domain_managers(domain_name, dict_domain_invitations_with_invited_status, dict_user_domain_roles): @@ -248,7 +248,7 @@ def write_csv_for_domains( # Retrieve domain information and all sec emails all_domain_infos = get_domain_infos(filter_condition, sort_fields) sec_contact_ids = all_domain_infos.values_list("domain__security_contact_registry_id", flat=True) - dict_security_emails_dict = _get_security_emails(sec_contact_ids) + dict_security_emails = _get_security_emails(sec_contact_ids) paginator = Paginator(all_domain_infos, 1000) # Initialize variables @@ -283,7 +283,7 @@ def write_csv_for_domains( row = parse_row_for_domain( columns, domain_info, - dict_security_emails_dict, + dict_security_emails, should_get_domain_managers, dict_domain_invitations_with_invited_status, dict_user_domain_roles, From dd7b0fc0bc81bbe04fc9979ca9671e535da98b81 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:49:54 -0600 Subject: [PATCH 43/45] Unit tests --- src/registrar/tests/test_admin.py | 81 +++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index dcffce6f8..8e83dedde 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -129,6 +129,49 @@ class TestDomainAdmin(MockEppLib, WebTest): ) mock_add_message.assert_has_calls([expected_call], 1) + @less_console_noise_decorator + def test_analyst_can_see_inline_domain_information_in_domain_change_form(self): + """Tests if the analyst can still see the inline domain information form""" + + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + _domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + + # Creates a Domain and DomainInformation object + _domain_request.approve() + + domain_information = DomainInformation.objects.filter(domain_request=_domain_request).get() + domain_information.organization_name = "MonkeySeeMonkeyDo" + domain_information.save() + + # We use filter here rather than just domain_information.domain just to get the latest data. + domain = Domain.objects.filter(domain=domain_information.domain) + + p = "userpass" + self.client.login(username="staffuser", password=p) + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.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) + + # Check for one of the inline headers + expected_header = ".gov domain" + self.assertContains(response, expected_header) + + # Test for data. We only need to test one since its all interconnected. + expected_organization_name = "MonkeySeeMonkeyDo" + self.assertContains(response, expected_organization_name) + @patch("registrar.admin.DomainAdmin._get_current_date", return_value=date(2024, 1, 1)) def test_extend_expiration_date_button_epp(self, mock_date_today): """ @@ -1982,6 +2025,44 @@ class TestDomainInformationAdmin(TestCase): expected_url = "Testy Tester" self.assertContains(response, expected_url) + @less_console_noise_decorator + def test_analyst_cant_access_domain_information(self): + """Ensures that analysts can't directly access the DomainInformation page through /admin""" + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + domain_request.approve() + domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get() + + p = "userpass" + self.client.login(username="staffuser", password=p) + response = self.client.get( + "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk), + follow=True, + ) + + # Make sure that we're denied access + self.assertEqual(response.status_code, 403) + + # To make sure that its not a fluke, swap to an admin user + # and try to access the same page. This should succeed. + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domaininformation/{}/change/".format(domain_info.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_info.domain.name) + @less_console_noise_decorator def test_contact_fields_have_detail_table(self): """Tests if the contact fields have the detail table which displays title, email, and phone""" From 7ac203913e7126f152dba6621bb4f974629296a5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:53:58 -0600 Subject: [PATCH 44/45] Update test_admin.py --- src/registrar/tests/test_admin.py | 46 +++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 8e83dedde..49bd1f299 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -131,7 +131,7 @@ class TestDomainAdmin(MockEppLib, WebTest): @less_console_noise_decorator def test_analyst_can_see_inline_domain_information_in_domain_change_form(self): - """Tests if the analyst can still see the inline domain information form""" + """Tests if an analyst can still see the inline domain information form""" # Create fake creator _creator = User.objects.create( @@ -151,7 +151,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain=domain_information.domain) + domain = Domain.objects.filter(domain_info=domain_information) p = "userpass" self.client.login(username="staffuser", password=p) @@ -172,6 +172,48 @@ class TestDomainAdmin(MockEppLib, WebTest): expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name) + @less_console_noise_decorator + def test_admin_can_see_inline_domain_information_in_domain_change_form(self): + """Tests if an admin can still see the inline domain information form""" + # Create fake creator + _creator = User.objects.create( + username="MrMeoward", + first_name="Meoward", + last_name="Jones", + ) + + # Create a fake domain request + _domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) + + # Creates a Domain and DomainInformation object + _domain_request.approve() + + domain_information = DomainInformation.objects.filter(domain_request=_domain_request).get() + domain_information.organization_name = "MonkeySeeMonkeyDo" + domain_information.save() + + # We use filter here rather than just domain_information.domain just to get the latest data. + domain = Domain.objects.filter(domain_info=domain_information) + + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.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) + + # Check for one of the inline headers + expected_header = ".gov domain" + self.assertContains(response, expected_header) + + # Test for data. We only need to test one since its all interconnected. + expected_organization_name = "MonkeySeeMonkeyDo" + self.assertContains(response, expected_organization_name) + @patch("registrar.admin.DomainAdmin._get_current_date", return_value=date(2024, 1, 1)) def test_extend_expiration_date_button_epp(self, mock_date_today): """ From c595405709bee9d556aa595bdcd6281bcbea4200 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:57:51 -0600 Subject: [PATCH 45/45] Fix unit tests --- src/registrar/tests/test_admin.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49bd1f299..f3c854aa0 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -151,7 +151,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain_info=domain_information) + domain = Domain.objects.filter(domain_info=domain_information).get() p = "userpass" self.client.login(username="staffuser", password=p) @@ -164,10 +164,6 @@ class TestDomainAdmin(MockEppLib, WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - # Check for one of the inline headers - expected_header = ".gov domain" - self.assertContains(response, expected_header) - # Test for data. We only need to test one since its all interconnected. expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name) @@ -193,7 +189,7 @@ class TestDomainAdmin(MockEppLib, WebTest): domain_information.save() # We use filter here rather than just domain_information.domain just to get the latest data. - domain = Domain.objects.filter(domain_info=domain_information) + domain = Domain.objects.filter(domain_info=domain_information).get() p = "adminpass" self.client.login(username="superuser", password=p) @@ -206,10 +202,6 @@ class TestDomainAdmin(MockEppLib, WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - # Check for one of the inline headers - expected_header = ".gov domain" - self.assertContains(response, expected_header) - # Test for data. We only need to test one since its all interconnected. expected_organization_name = "MonkeySeeMonkeyDo" self.assertContains(response, expected_organization_name)