From aea8539d66559f2b636c995a27421bdb33e7d692 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 14 Aug 2024 11:18:47 -0600 Subject: [PATCH 001/329] Initial draft logic for updating action needed email behavior --- src/registrar/admin.py | 2 +- src/registrar/assets/js/get-gov-admin.js | 60 +++++++++++++------ .../admin/includes/detail_table_fieldset.html | 29 ++++++--- 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ca4038d51..2cd391435 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -223,7 +223,7 @@ class DomainRequestAdminForm(forms.ModelForm): "other_contacts": NoAutocompleteFilteredSelectMultiple("other_contacts", False), } labels = { - "action_needed_reason_email": "Auto-generated email", + "action_needed_reason_email": "E-mail", } def __init__(self, *args, **kwargs): diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 04f5417b0..c8fd45bd0 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -353,7 +353,7 @@ function initializeWidgetOnList(list, parentId) { let rejectionReasonFormGroup = document.querySelector('.field-rejection_reason') // This is the "action needed reason" field let actionNeededReasonFormGroup = document.querySelector('.field-action_needed_reason'); - // This is the "auto-generated email" field + // This is the "Email" field let actionNeededReasonEmailFormGroup = document.querySelector('.field-action_needed_reason_email') if (rejectionReasonFormGroup && actionNeededReasonFormGroup && actionNeededReasonEmailFormGroup) { @@ -510,7 +510,10 @@ function initializeWidgetOnList(list, parentId) { // Since this is an iife, these vars will be removed from memory afterwards var actionNeededReasonDropdown = document.querySelector("#id_action_needed_reason"); var actionNeededEmail = document.querySelector("#id_action_needed_reason_email"); - var readonlyView = document.querySelector("#action-needed-reason-email-readonly"); + var helptext = document.querySelector("#action-needed-reason-email-placeholder-text") + var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly"); + var actionNeededEmailReadonlyTextarea = document.querySelector("#action-needed-reason-email-readonly-textarea") + var editEmailButton = document.querySelector("#action-needed-reason-email-edit-button") let emailWasSent = document.getElementById("action-needed-email-sent"); let actionNeededEmailData = document.getElementById('action-needed-emails-data').textContent; @@ -536,6 +539,21 @@ function initializeWidgetOnList(list, parentId) { updateActionNeededEmailDisplay(reason) }); + editEmailButton.addEventListener("click", function() { + let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; + + if (emailHasBeenSentBefore) { + // Show warning Modal + + } + else { + // Show editable view + showElement(actionNeededEmail.parentElement) + hideElement(actionNeededEmailReadonly) + hideElement(helptext) + } + }); + // Add a change listener to the action needed reason dropdown actionNeededReasonDropdown.addEventListener("change", function() { let reason = actionNeededReasonDropdown.value; @@ -543,6 +561,7 @@ function initializeWidgetOnList(list, parentId) { if (reason && emailBody) { // Replace the email content actionNeededEmail.value = emailBody; + actionNeededEmailReadonlyTextarea.value = emailBody; // Reset the session object on change since change refreshes the email content. if (oldDropdownValue !== actionNeededReasonDropdown.value || oldEmailValue !== actionNeededEmail.value) { @@ -562,27 +581,30 @@ function initializeWidgetOnList(list, parentId) { // If the email doesn't exist or if we're of reason "other", display that no email was sent. // Likewise, if we've sent this email before, we should just display the content. function updateActionNeededEmailDisplay(reason) { - let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; - let collapseableDiv = readonlyView.querySelector(".collapse--dgsimple"); - let showMoreButton = document.querySelector("#action_needed_reason_email__show_details"); - if ((reason && reason != "other") && !emailHasBeenSentBefore) { - showElement(actionNeededEmail.parentElement) - hideElement(readonlyView) - hideElement(showMoreButton) - } else { - if (!reason || reason === "other") { - collapseableDiv.innerHTML = reason ? "No email will be sent." : "-"; - hideElement(showMoreButton) - if (collapseableDiv.classList.contains("collapsed")) { - showMoreButton.click() - } - }else { - showElement(showMoreButton) + + console.info("REASON: "+reason) + + if (reason) { + if (reason === "other") { + helptext.innerHTML = "No email will be sent."; + hideElement(actionNeededEmail.parentElement) + hideElement(actionNeededEmailReadonly) + showElement(helptext) } + else { + // Always show readonly view to start + hideElement(actionNeededEmail.parentElement) + showElement(actionNeededEmailReadonly) + hideElement(helptext) + } + } else { + helptext.innerHTML = "Select an action needed reason to see email"; hideElement(actionNeededEmail.parentElement) - showElement(readonlyView) + hideElement(actionNeededEmailReadonly) + showElement(helptext) } } + })(); diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 067b69c07..bcf5fd770 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -145,16 +145,29 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% block field_other %} {% if field.field.name == "action_needed_reason_email" %} - + {% elif field.field.name == "display_admins" %} +
{{ field.contents|safe }}
+ {% elif field.field.name == "display_members" %} +
{{ display_members_summary }}
{% else %}
{{ field.contents }}
{% endif %} @@ -240,6 +244,13 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% endif %} {% endwith %} + {% elif field.field.name == "display_members" %} +
+ Details +
+ {{ field.contents|safe }} +
+
{% elif field.field.name == "state_territory" %}
diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 9d59aae42..a2d70611c 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/src/registrar/templates/django/admin/portfolio_change_form.html @@ -14,8 +14,7 @@ This is a placeholder for now. Disclaimer: - When extending the fieldset view - *make a new one* that extends from detail_table_fieldset. - For instance, "portfolio_fieldset.html". + When extending the fieldset view consider whether you need to make a new one that extends from detail_table_fieldset. detail_table_fieldset is used on multiple admin pages, so a change there can have unintended consequences. {% endcomment %} {% include "django/admin/includes/detail_table_fieldset.html" with original_object=original %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..7ef1a4a97 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ from registrar.models import ( from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_domain_role import UserDomainRole +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -2066,6 +2067,7 @@ class TestPortfolioAdmin(TestCase): DomainRequest.objects.all().delete() Domain.objects.all().delete() Portfolio.objects.all().delete() + User.objects.all().delete() @less_console_noise_decorator def test_created_on_display(self): @@ -2121,3 +2123,81 @@ class TestPortfolioAdmin(TestCase): self.assertIn("request1.gov", domain_requests) self.assertIn("request2.gov", domain_requests) self.assertIn('
    ', domain_requests) + + @less_console_noise_decorator + def test_portfolio_members_display(self): + """Tests the custom portfolio members field, admin and member sections""" + admin_user_1 = User.objects.create( + username="testuser1", + first_name="Gerald", + last_name="Meoward", + title="Captain", + email="meaoward@gov.gov", + portfolio=self.portfolio, + portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + admin_user_2 = User.objects.create( + username="testuser2", + first_name="Arnold", + last_name="Poopy", + title="Major", + email="poopy@gov.gov", + portfolio=self.portfolio, + portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + ) + + admin_user_3 = User.objects.create( + username="testuser3", + first_name="Mad", + last_name="Max", + title="Road warrior", + email="madmax@gov.gov", + portfolio=self.portfolio, + portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + ) + + admin_user_4 = User.objects.create( + username="testuser4", + first_name="Agent", + last_name="Smith", + title="Program", + email="thematrix@gov.gov", + portfolio=self.portfolio, + portfolio_additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + display_admins = self.admin.display_admins(self.portfolio) + + self.assertIn( + f'Gerald Meoward meaoward@gov.gov', + display_admins, + ) + self.assertIn("Captain", display_admins) + self.assertIn( + f'Arnold Poopy poopy@gov.gov', display_admins + ) + self.assertIn("Major", display_admins) + + display_members_summary = self.admin.display_members_summary(self.portfolio) + + self.assertIn( + f'Mad Max madmax@gov.gov', + display_members_summary, + ) + self.assertIn( + f'Agent Smith thematrix@gov.gov', + display_members_summary, + ) + + display_members = self.admin.display_members(self.portfolio) + + self.assertIn("Mad Max", display_members) + self.assertIn("Member", display_members) + self.assertIn("Road warrior", display_members) + self.assertIn("Agent Smith", display_members) + self.assertIn("Domain requestor", display_members) + self.assertIn("Program", display_members) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index f4e998fff..5b9d0ade3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1201,6 +1201,61 @@ class TestUser(TestCase): User.objects.all().delete() UserDomainRole.objects.all().delete() + @patch.object(User, "has_edit_suborganization", return_value=True) + def test_portfolio_role_summary_admin(self, mock_edit_suborganization): + # Test if the user is recognized as an Admin + self.assertEqual(self.user.portfolio_role_summary, ["Admin"]) + + @patch.multiple( + User, + has_view_all_domains_permission=lambda self: True, + has_domain_requests_portfolio_permission=lambda self: True, + has_edit_requests=lambda self: True, + ) + def test_portfolio_role_summary_view_only_admin_and_domain_requestor(self): + # Test if the user has both 'View-only admin' and 'Domain requestor' roles + self.assertEqual(self.user.portfolio_role_summary, ["View-only admin", "Domain requestor"]) + + @patch.multiple( + User, + has_view_all_domains_permission=lambda self: True, + has_domain_requests_portfolio_permission=lambda self: True, + ) + def test_portfolio_role_summary_view_only_admin(self): + # Test if the user is recognized as a View-only admin + self.assertEqual(self.user.portfolio_role_summary, ["View-only admin"]) + + @patch.multiple( + User, + has_base_portfolio_permission=lambda self: True, + has_edit_requests=lambda self: True, + has_domains_portfolio_permission=lambda self: True, + ) + def test_portfolio_role_summary_member_domain_requestor_domain_manager(self): + # Test if the user has 'Member', 'Domain requestor', and 'Domain manager' roles + self.assertEqual(self.user.portfolio_role_summary, ["Member", "Domain requestor", "Domain manager"]) + + @patch.multiple(User, has_base_portfolio_permission=lambda self: True, has_edit_requests=lambda self: True) + def test_portfolio_role_summary_member_domain_requestor(self): + # Test if the user has 'Member' and 'Domain requestor' roles + self.assertEqual(self.user.portfolio_role_summary, ["Member", "Domain requestor"]) + + @patch.multiple( + User, has_base_portfolio_permission=lambda self: True, has_domains_portfolio_permission=lambda self: True + ) + def test_portfolio_role_summary_member_domain_manager(self): + # Test if the user has 'Member' and 'Domain manager' roles + self.assertEqual(self.user.portfolio_role_summary, ["Member", "Domain manager"]) + + @patch.multiple(User, has_base_portfolio_permission=lambda self: True) + def test_portfolio_role_summary_member(self): + # Test if the user is recognized as a Member + self.assertEqual(self.user.portfolio_role_summary, ["Member"]) + + def test_portfolio_role_summary_empty(self): + # Test if the user has no roles + self.assertEqual(self.user.portfolio_role_summary, []) + @less_console_noise_decorator def test_check_transition_domains_without_domains_on_login(self): """A user's on_each_login callback does not check transition domains. From e82326346c5af4ac1b175418d21a229cc4634463 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 21 Aug 2024 12:43:38 -0400 Subject: [PATCH 033/329] clean up roles logic --- src/registrar/models/user.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a6026b63a..8584bc0a1 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -318,12 +318,12 @@ class User(AbstractUser): self.has_base_portfolio_permission() and self.has_edit_requests() and self.has_domains_portfolio_permission(), - ["Member", "Domain requestor", "Domain manager"], + ["Domain requestor", "Domain manager"], ), - (self.has_base_portfolio_permission() and self.has_edit_requests(), ["Member", "Domain requestor"]), + (self.has_base_portfolio_permission() and self.has_edit_requests(), ["Domain requestor"]), ( self.has_base_portfolio_permission() and self.has_domains_portfolio_permission(), - ["Member", "Domain manager"], + ["Domain manager"], ), (self.has_base_portfolio_permission(), ["Member"]), ] From 08c42a6e8d48bb117e494e4ae82095c403106521 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Wed, 21 Aug 2024 12:28:41 -0500 Subject: [PATCH 034/329] merge main and resolve migration conflicts --- ...sion_dates.py => 0119_add_domainrequest_submission_dates.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/registrar/migrations/{0118_add_domainrequest_submission_dates.py => 0119_add_domainrequest_submission_dates.py} (92%) diff --git a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py similarity index 92% rename from src/registrar/migrations/0118_add_domainrequest_submission_dates.py rename to src/registrar/migrations/0119_add_domainrequest_submission_dates.py index c971b2f9a..0b94e1257 100644 --- a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -6,7 +6,7 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ("registrar", "0117_alter_portfolioinvitation_portfolio_additional_permissions_and_more"), + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), ] operations = [ From 32ed84a5e15d07c820d4553fb4804ad1c1a06e71 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:43:40 -0600 Subject: [PATCH 035/329] Fix logic in clean --- src/registrar/models/user_portfolio_permission.py | 10 +++++++--- src/registrar/tests/test_models.py | 8 +------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 4582ffd08..a3460a651 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -97,15 +97,19 @@ class UserPortfolioPermission(TimeStampedModel): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() - if not flag_is_active(None, "multiple_portfolios") and self.pk is None: + # Check if a user is set without accessing the related object. + has_user = bool(self.user_id) + if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) - if self.portfolio is None and self._get_portfolio_permissions(): + # Check if portfolio is set without accessing the related object. + has_portfolio = bool(self.portfolio_id) + if not has_portfolio and self._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - if self.portfolio is not None and not self._get_portfolio_permissions(): + if has_portfolio and not self._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 33499eb91..8db11a0e7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1459,15 +1459,9 @@ class TestUser(TestCase): portfolio_permission.portfolio = None portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - # Create a new UserPortfolioPermission instance without a portfolio - invalid_permission = UserPortfolioPermission( - user=self.user, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - portfolio=None # This should trigger the validation error - ) # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - invalid_permission.clean() + portfolio_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." From bfc2693a12e61f8629777a31f4612cdf02901d7d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:12:50 -0600 Subject: [PATCH 036/329] Fix a few more tests --- src/registrar/tests/test_models.py | 61 +++++++++------------ src/registrar/tests/test_views_domain.py | 12 ++-- src/registrar/tests/test_views_portfolio.py | 9 +-- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8db11a0e7..995f2a89c 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,6 +30,7 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, + get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1369,60 +1370,50 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) - portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - portfolio_permission.save() - portfolio_permission.refresh_from_db() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] portfolio_permission.save() portfolio_permission.refresh_from_db() - self.user.refresh_from_db() - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index ab489c813..e457f1b66 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -317,6 +317,7 @@ class TestDomainDetail(TestDomainOverview): self.assertContains(detail_page, "Domain missing domain information") @less_console_noise_decorator + @override_flag("organization_feature", active=True) def test_domain_readonly_on_detail_page(self): """Test that a domain, which is part of a portfolio, but for which the user is not a domain manager, properly displays read only""" @@ -330,11 +331,13 @@ class TestDomainDetail(TestDomainOverview): phone="8003111234", title="test title", ) + domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") + DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + UserPortfolioPermission.objects.get_or_create( user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") - DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + user.refresh_from_db() self.client.force_login(user) detail_page = self.client.get(f"/domain/{domain.id}") # Check that alert message displays properly @@ -1577,9 +1580,10 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) + self.user.refresh_from_db() # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..039f6f13e 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -252,12 +252,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions - ) - self.user.save() - self.user.refresh_from_db() + portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + portfolio_permission.save() + portfolio_permission.refresh_from_db() portfolio_page = self.app.get(reverse("home")).follow() From aa872d6e41457a73fe13f64e9eb3b38502144bed Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:22:03 -0600 Subject: [PATCH 037/329] Fix last test and lint --- src/registrar/models/user.py | 4 +--- src/registrar/tests/test_admin.py | 1 - src/registrar/tests/test_models.py | 15 +++++++++++---- src/registrar/tests/test_reports.py | 9 ++++++++- src/registrar/tests/test_views_portfolio.py | 2 +- src/registrar/views/portfolios.py | 4 +--- src/registrar/views/utility/mixins.py | 4 ++-- 7 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 38e971a3b..6aed8195c 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -397,8 +397,6 @@ class User(AbstractUser): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" portfolio = request.session.get("portfolio") if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): - return DomainInformation.objects.filter(portfolio=portfolio).values_list( - "domain_id", flat=True - ) + return DomainInformation.objects.filter(portfolio=portfolio).values_list("domain_id", flat=True) else: return UserDomainRole.objects.filter(user=self).values_list("domain_id", flat=True) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49d2b4802..827742ef1 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1209,7 +1209,6 @@ class TestMyUserAdmin(MockDbForSharedTests): domain_deleted.delete() role.delete() - # TODO - should refactor def test_analyst_cannot_see_selects_for_portfolio_role_and_permissions_in_user_form(self): """Can only test for the presence of a base element. The multiselects and the h2->h3 conversion are all dynamically generated.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 995f2a89c..3a5405fe8 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,7 +30,6 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, - get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1151,7 +1150,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieval(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() @@ -1172,7 +1173,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) self.assertEqual(portfolio_role.portfolio.organization_name, "Hotel California") @@ -1380,7 +1383,11 @@ class TestUser(TestCase): self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, + user=self.user, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + ) # TODO - uncomment this when we just pass request to these functions # request = get_wsgi_request_object(self.client, self.user) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 1e169b51d..8bb15921d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -34,7 +34,14 @@ import boto3_mocking from registrar.utility.s3_bucket import S3ClientError, S3ClientErrorCodes # type: ignore from django.utils import timezone from api.tests.common import less_console_noise_decorator -from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, get_wsgi_request_object, less_console_noise, get_time_aware_date +from .common import ( + MockDbForSharedTests, + MockDbForIndividualTests, + MockEppLib, + get_wsgi_request_object, + less_console_noise, + get_time_aware_date, +) from waffle.testutils import override_flag diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 039f6f13e..1568391fc 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -285,7 +285,7 @@ class TestPortfolio(WebTest): # removing non-basic portfolio role, which should remove domains # and domain requests from nav - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] portfolio_permission.save() portfolio_permission.refresh_from_db() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 18285774b..1b5cabea7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -52,9 +52,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) portfolio = self.request.session.get("portfolio") - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( - portfolio - ) + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(portfolio) return context def get_object(self, queryset=None): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index c35804243..d1edfc46e 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -434,7 +434,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) @@ -450,5 +450,5 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) From f4647caded419d5ea41d0b336a5120c9cff47999 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:28:23 -0400 Subject: [PATCH 038/329] working on tests --- src/registrar/tests/test_views_portfolio.py | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..ffcc87823 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -14,6 +14,7 @@ from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .common import create_test_user from waffle.testutils import override_flag +from django.contrib.sessions.middleware import SessionMiddleware import logging @@ -364,3 +365,47 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + + @less_console_noise_decorator + def test_portfolio_in_session_when_multiple_portfolios_active(self): + """When multiple_portfolios flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file From 01f05c7fde2a9cd1b6f80a6b5e01cab476aef58c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:42:36 -0400 Subject: [PATCH 039/329] test registrar_middleware for multiple_portfolios --- src/registrar/tests/test_views_portfolio.py | 77 ++++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 0104eefb8..3667b1460 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -363,6 +363,69 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): + """When organization_feature flag is false and user has a portfolio, + the portfolio should be set to None in session. + This test also satisfies the conidition when multiple_portfolios flag + is false and user has a portfolio, so won't add a redundant test for that.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): + """When organization_feature flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" + self.client.force_login(self.user) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, @@ -386,15 +449,11 @@ class TestPortfolio(WebTest): assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." @less_console_noise_decorator - def test_portfolio_in_session_when_organization_feature_active(self): - """When organization_feature flag is true and user has a portfolio, - the portfolio should be set in session.""" + def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): + """When multiple_portfolios flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" self.client.force_login(self.user) - portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) - with override_flag("organization_feature", active=True): + with override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -405,4 +464,4 @@ class TestPortfolio(WebTest): # Check if the 'portfolio' session variable exists assert 'portfolio' in session, "Portfolio session variable should exist." # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file + self.assertIsNone(session['portfolio']) From 035b4206f3f67a829287a6f707464527738f0477 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:07:52 -0600 Subject: [PATCH 040/329] Add test + fix perms --- src/registrar/tests/test_models.py | 65 +++++++++++++++++++++++++++ src/registrar/views/utility/mixins.py | 10 +++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..22758d4f6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1187,6 +1187,71 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) +class TestUserPortfolioPermission(TestCase): + @less_console_noise_decorator + def setUp(self): + self.user, _ = User.objects.get_or_create(email="mayor@igorville.gov") + super().setUp() + + def tearDown(self): + super().tearDown() + Domain.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + UserPortfolioPermission.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=True) + def test_clean_on_multiple_portfolios_when_flag_active(self): + """Ensures that a user can create multiple portfolio permission objects when the flag is enabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # Clean should pass on both of these objects + try: + portfolio_permission.clean() + portfolio_permission_2.clean() + except ValidationError as error: + self.fail(f"Raised ValidationError unexpectedly: {error}") + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=False) + def test_clean_on_creates_multiple_portfolios(self): + """Ensures that a user cannot create multiple portfolio permission objects when the flag is disabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # This should work as intended + portfolio_permission.clean() + + # Test if the ValidationError is raised with the correct message + with self.assertRaises(ValidationError) as cm: + portfolio_permission_2.clean() + + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + + self.assertEqual( + cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + ) + + class TestUser(TestCase): """Test actions that occur on user login, test class method that controls how users get validated.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index d1edfc46e..643405262 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,10 +432,11 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domains_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -448,7 +449,8 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() From 9e04e9f5bac29b02730ab0112b562427a92ec803 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:08:40 -0600 Subject: [PATCH 041/329] typo --- src/registrar/views/utility/mixins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 643405262..6f0745f41 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,7 +432,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domains_portfolio_permission(portfolio): return False @@ -449,7 +449,7 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False From 4dadb715e84a2638356eed8687969bc61f4ebf4a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:03:28 -0600 Subject: [PATCH 042/329] Fix merge conflict (pt 1) --- src/registrar/tests/test_views_portfolio.py | 31 ++++++++++----------- src/registrar/views/portfolios.py | 14 ++++++---- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c87a2f3c8..f9501aa14 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user +from .common import create_test_user, get_wsgi_request_object from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -502,14 +502,12 @@ class TestPortfolio(WebTest): # A default organization member should not be able to see any domains self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - self.user.save() - self.user.refresh_from_db() - - self.assertFalse(self.user.has_domains_portfolio_permission()) + permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) response = self.app.get(reverse("no-portfolio-domains")) + self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "You aren’t managing any domains.") @@ -518,25 +516,24 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") + permission.delete() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 436d1b913..e317d5089 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -5,6 +5,7 @@ from django.urls import reverse from django.contrib import messages from registrar.forms.portfolio import PortfolioOrgAddressForm, PortfolioSeniorOfficialForm from registrar.models import Portfolio, User +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, @@ -55,14 +56,17 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): """Add additional context data to the template.""" # We can override the base class. This view only needs this item. context = {} - portfolio = self.request.user.portfolio if self.request and self.request.user else None + portfolio = self.request.session.get("portfolio") if portfolio: - context["portfolio_administrators"] = User.objects.filter( + admin_ids = UserPortfolioPermission.objects.filter( portfolio=portfolio, - portfolio_roles__overlap=[ + roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ], - ) + ] + ).values_list("user__id", flat=True) + + admin_users = User.objects.filter(id__in=admin_ids) + context["portfolio_administrators"] = admin_users return context From 0858a9663f11c1f4c4f5c1dd8d71f1a160944043 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:08:09 -0400 Subject: [PATCH 043/329] added more tests --- src/registrar/tests/test_models.py | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..e5ce01599 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1186,6 +1186,48 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(len(roles), 1) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations(self): + """Retrieve user portfolio invitations when there are multiple and multiple_options flag true.""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + with override_flag("multiple_portfolios", active=True): + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 2) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations_when_multiple_portfolios_inactive(self): + """Attempt to retrieve user portfolio invitations when there are multiple + but multiple_portfolios flag set to False""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 1) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + class TestUser(TestCase): """Test actions that occur on user login, From e866bcb2fbbf6d6dcd95e2456497d203cc81079f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:12:37 -0600 Subject: [PATCH 044/329] Fix test (still needs refinement) --- src/registrar/tests/test_views_portfolio.py | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..3a973b7c7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -500,19 +500,20 @@ class TestPortfolio(WebTest): if they do not have the right permissions. """ - # A default organization member should not be able to see any domains - self.app.set_user(self.user.username) permission, _ = UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) - response = self.app.get(reverse("no-portfolio-domains")) - self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + # A default organization member should not be able to see any domains + self.client.force_login(self.user) + response = self.client.get(reverse("home"), follow=True) + + self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren’t managing any domains.") + self.assertContains(response, "You aren't managing any domains.") # Test the domains page - this user should not have access - response = self.app.get(reverse("domains"), expect_errors=True) + response = self.client.get(reverse("domains")) self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions @@ -521,19 +522,19 @@ class TestPortfolio(WebTest): permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] permission.save() permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() From ff86edbc3538c57c99b1e770dbeb5035c6f7491f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:18:00 -0400 Subject: [PATCH 045/329] more updates to test and for linting --- src/registrar/tests/test_models.py | 13 +++++--- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/tests/test_views_portfolio.py | 36 +++++++++------------ src/registrar/views/portfolios.py | 2 +- 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 6c67aa31f..227548b61 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1202,7 +1202,9 @@ class TestPortfolioInvitations(TestCase): self.user.refresh_from_db() roles = UserPortfolioPermission.objects.filter(user=self.user) self.assertEqual(len(roles), 2) - updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create( + email=self.email, portfolio=self.portfolio + ) self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1244,7 +1246,7 @@ class TestUserPortfolioPermission(TestCase): Portfolio.objects.all().delete() User.objects.all().delete() UserDomainRole.objects.all().delete() - + @less_console_noise_decorator @override_flag("multiple_portfolios", active=True) def test_clean_on_multiple_portfolios_when_flag_active(self): @@ -1279,18 +1281,19 @@ class TestUserPortfolioPermission(TestCase): portfolio_permission_2 = UserPortfolioPermission( portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - + # This should work as intended portfolio_permission.clean() # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: portfolio_permission_2.clean() - + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) self.assertEqual( - cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + cm.exception.message, + "Only one portfolio permission is allowed per user when multiple portfolios are disabled.", ) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index e457f1b66..5ac24fd69 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -6,7 +6,7 @@ from django.urls import reverse from django.contrib.auth import get_user_model from waffle.testutils import override_flag from api.tests.common import less_console_noise_decorator -from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..277f0520a 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user, get_wsgi_request_object +from .common import create_test_user from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -396,9 +396,7 @@ class TestPortfolio(WebTest): the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -408,9 +406,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): @@ -420,9 +418,7 @@ class TestPortfolio(WebTest): is false and user has a portfolio, so won't add a redundant test for that.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -431,9 +427,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): @@ -449,19 +445,17 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) - + self.assertIsNone(session["portfolio"]) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -471,9 +465,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): @@ -489,9 +483,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index e317d5089..0232b50d7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -62,7 +62,7 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): portfolio=portfolio, roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ] + ], ).values_list("user__id", flat=True) admin_users = User.objects.filter(id__in=admin_ids) From 833b698bf28655a71e1eaf4ad209dc1b9973eac4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:52:02 -0400 Subject: [PATCH 046/329] adding request to a few flag_is_active instances --- src/registrar/models/user.py | 7 ++++++- src/registrar/models/user_portfolio_permission.py | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 6aed8195c..03d11e5bd 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -3,6 +3,7 @@ import logging from django.contrib.auth.models import AbstractUser from django.db import models from django.db.models import Q +from django.http import HttpRequest from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices @@ -358,8 +359,12 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): + # need to create a bogus request and assign user to it, in order to pass request + # to flag_is_active + request = HttpRequest() + request.user = self only_single_portfolio = ( - not flag_is_active(None, "multiple_portfolios") and self.get_first_portfolio() is None + not flag_is_active(request, "multiple_portfolios") and self.get_first_portfolio() is None ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index a3460a651..abbd20afb 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -1,5 +1,6 @@ from django.db import models from django.forms import ValidationError +from django.http import HttpRequest from waffle import flag_is_active from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel @@ -99,7 +100,10 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user + if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( From ee73893a20856b79d35a058bca0a9a1c06ccb192 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 21 Aug 2024 16:12:34 -0600 Subject: [PATCH 047/329] fix e-mail sent logic, fix text size in nested text area input --- src/registrar/assets/js/get-gov-admin.js | 91 ++++++++++--------- src/registrar/assets/sass/_theme/_admin.scss | 14 ++- .../admin/includes/detail_table_fieldset.html | 18 ++-- 3 files changed, 73 insertions(+), 50 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 23413ade1..d58bced42 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -509,32 +509,41 @@ function initializeWidgetOnList(list, parentId) { (function () { // Since this is an iife, these vars will be removed from memory afterwards var actionNeededReasonDropdown = document.querySelector("#id_action_needed_reason"); - var actionNeededEmail = document.querySelector("#id_action_needed_reason_email"); + + // Placeholder text (for certain "action needed" reasons that do not involve e=mails) var placeholderText = document.querySelector("#action-needed-reason-email-placeholder-text") - var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly"); + + // E-mail divs and textarea components + var actionNeededEmail = document.querySelector("#id_action_needed_reason_email") + var actionNeededEmailReadonly = document.querySelector("#action-needed-reason-email-readonly") var actionNeededEmailReadonlyTextarea = document.querySelector("#action-needed-reason-email-readonly-textarea") + + // Edit e-mail button (appears only in readonly view for e-mail text) var editEmailButton = document.querySelector("#action-needed-reason-email-edit-button") + // Edit e-mail modal (and its confirmation button) var actionNeededEmailAlreadySentModal = document.querySelector("#email-already-sent-modal") var confirmEditEmailButton = document.querySelector("#email-already-sent-modal_continue-editing-button") + // Headers and footers (which change depending on if the e-mail was sent or not) var actionNeededEmailHeader = document.querySelector("#action-needed-email-header") var actionNeededEmailHeaderOnSave = document.querySelector("#action-needed-email-header-email-sent") var actionNeededEmailFooter = document.querySelector("#action-needed-email-footer") let emailWasSent = document.getElementById("action-needed-email-sent"); + let lastSentEmailText = document.getElementById("action-needed-email-last-sent-text"); + // Get the list of e-mails associated with each action-needed dropdown value let emailData = document.getElementById('action-needed-emails-data'); if (!emailData) { return; } - let actionNeededEmailData = emailData.textContent; if(!actionNeededEmailData) { return; } - let actionNeededEmailsJson = JSON.parse(actionNeededEmailData); + const domainRequestId = actionNeededReasonDropdown ? document.querySelector("#domain_request_id").value : null const emailSentSessionVariableName = `actionNeededEmailSent-${domainRequestId}`; const oldDropdownValue = actionNeededReasonDropdown ? actionNeededReasonDropdown.value : null; @@ -544,22 +553,19 @@ function initializeWidgetOnList(list, parentId) { // Add a change listener to dom load document.addEventListener('DOMContentLoaded', function() { let reason = actionNeededReasonDropdown.value; - let emailBody = reason in actionNeededEmailsJson ? actionNeededEmailsJson[reason] : null; // Handle the session boolean (to enable/disable editing) if (emailWasSent && emailWasSent.value === "True") { // An email was sent out - store that information in a session variable addOrRemoveSessionBoolean(emailSentSessionVariableName, add=true); } - + // Show an editable email field or a readonly one - updateActionNeededEmailDisplay(reason, emailBody) + updateActionNeededEmailDisplay(reason) }); editEmailButton.addEventListener("click", function() { - let emailHasBeenSentBefore = sessionStorage.getItem(emailSentSessionVariableName) !== null; - - if (emailHasBeenSentBefore) { + if (checkEmailAlreadySent()) { // Show warning Modal setModalVisibility(actionNeededEmailAlreadySentModal, true) } @@ -590,25 +596,41 @@ function initializeWidgetOnList(list, parentId) { if (reason && emailBody) { // Reset the session object on change since change refreshes the email content. if (oldDropdownValue !== actionNeededReasonDropdown.value || oldEmailValue !== actionNeededEmail.value) { - let emailSent = sessionStorage.getItem(emailSentSessionVariableName) - if (emailSent !== null){ - addOrRemoveSessionBoolean(emailSentSessionVariableName, add=false) - } + // Replace the email content + actionNeededEmail.value = emailBody; + actionNeededEmailReadonlyTextarea.value = emailBody; + hideEmailAlreadySentView(); } } // Show either a preview of the email or some text describing no email will be sent - updateActionNeededEmailDisplay(reason, emailBody) + updateActionNeededEmailDisplay(reason) }); + } - const saveButton = document.querySelector('input[name=_save]'); - // The button does not exist if no fields are editable. - if (saveButton) { - saveButton.addEventListener('click', function(event) { - // Show readonly view with messaging to indicate email was sent - showEmailAlreadySentView() - }); - } + function checkEmailAlreadySent() + { + lastEmailSent = lastSentEmailText.value.replace(/\s+/g, '') + currentEmailInTextArea = actionNeededEmail.value.replace(/\s+/g, '') + console.log("old email value = " + lastEmailSent) + console.log("current email value = " + currentEmailInTextArea) + return lastEmailSent === currentEmailInTextArea + } + + // Shows a readonly preview of the email with updated messaging to indicate this email was sent + function showEmailAlreadySentView() + { + hideElement(actionNeededEmailHeader) + showElement(actionNeededEmailHeaderOnSave) + actionNeededEmailFooter.innerHTML = "This email has been sent to the creator of this request"; + } + + // Shows a readonly preview of the email with updated messaging to indicate this email was sent + function hideEmailAlreadySentView() + { + showElement(actionNeededEmailHeader) + hideElement(actionNeededEmailHeaderOnSave) + actionNeededEmailFooter.innerHTML = "This email will be sent to the creator of this request after saving"; } function setModalVisibility(modalToToggle, isVisible) { @@ -623,15 +645,7 @@ function initializeWidgetOnList(list, parentId) { // Shows either a preview of the email or some text describing no email will be sent. // If the email doesn't exist or if we're of reason "other", display that no email was sent. - function updateActionNeededEmailDisplay(reason, emailBody) { - - if (reason && emailBody) { - // Replace the email content - actionNeededEmail.value = emailBody; - actionNeededEmailReadonlyTextarea.value = emailBody; - } - - actionNeededEmailFooter.innerHTML = "This email will be sent to the creator of this request after saving"; + function updateActionNeededEmailDisplay(reason) { hideElement(actionNeededEmail.parentElement) if (reason) { @@ -642,6 +656,10 @@ function initializeWidgetOnList(list, parentId) { else { // Always show readonly view of email to start showEmail(canEdit=false) + if(checkEmailAlreadySent()) + { + showEmailAlreadySentView(); + } } } else { // Hide email preview and show this text instead @@ -649,15 +667,6 @@ function initializeWidgetOnList(list, parentId) { } } - // Shows a readonly preview of the email with updated messaging to indicate this email was sent - function showEmailAlreadySentView() - { - showEmail(canEdit=false) - hideElement(actionNeededEmailHeader) - showElement(actionNeededEmailHeaderOnSave) - actionNeededEmailFooter.innerHTML = "This email has been sent to the creator of this request"; - } - // Shows either a readonly view (canEdit=false) or editable view (canEdit=true) of the action needed email function showEmail(canEdit) { diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index db1741f97..f7d1e5788 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -66,6 +66,9 @@ html[data-theme="light"] { // --object-tools-fg: var(--button-fg); // --object-tools-bg: var(--close-button-bg); // --object-tools-hover-bg: var(--close-button-hover-bg); + + --summary-box-bg: #f1f1f1; + --summary-box-border: #d1d2d2; } // Fold dark theme settings into our main CSS @@ -104,6 +107,9 @@ html[data-theme="light"] { --close-button-bg: #333333; --close-button-hover-bg: #666666; + + --summary-box-bg: #121212; + --summary-box-border: #666666; } // Dark mode django (bug due to scss cascade) and USWDS tables @@ -857,10 +863,12 @@ div.dja__model-description{ } .usa-summary-box_admin { - border-color: #d1d2d2; - background-color: #f1f1f1; - max-width: fit-content !important; + color: var(--body-fg); + border-color: var(--summary-box-border); + background-color: var(--summary-box-bg); + min-width: fit-content; padding: .5rem; + border-radius: .25rem; } .text-faded { diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index bf4323002..087bacda6 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -150,7 +150,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) -
- {% else %} {{ field.field }} From 76fef5303b1e24460fb1a282fe7662b4c38f5df6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 18:41:14 -0400 Subject: [PATCH 048/329] removed pseudo apostrophe --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 01cd84743..42cf82fa4 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -504,7 +504,7 @@ class TestPortfolio(WebTest): self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren't managing any domains.") + self.assertContains(response, "You aren") # Test the domains page - this user should not have access response = self.client.get(reverse("domains")) From bef49a85c16de5fc9f2640a9c2ee3f1c37e25478 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 08:33:03 -0600 Subject: [PATCH 049/329] Cleanup --- .../models/user_portfolio_permission.py | 10 +++++----- src/registrar/tests/test_models.py | 16 ---------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index abbd20afb..c2cdc61ad 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -100,12 +100,12 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - # Have to create a bogus request to set the user and pass to flag_is_active - request = HttpRequest() - request.user = self.user - if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: + if self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) - if existing_permissions.exists(): + if not flag_is_active(request, "multiple_portfolios") and existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 227548b61..9f55fced1 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1483,10 +1483,6 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1499,10 +1495,6 @@ class TestUser(TestCase): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1513,10 +1505,6 @@ class TestUser(TestCase): portfolio_permission.save() portfolio_permission.refresh_from_db() - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1525,10 +1513,6 @@ class TestUser(TestCase): UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) From 1a4268dedd72b5cfedefd235af1d756497883a77 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 22 Aug 2024 09:46:07 -0500 Subject: [PATCH 050/329] add new script to update first ready values --- .../commands/update_first_ready_values.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 src/registrar/management/commands/update_first_ready_values.py diff --git a/src/registrar/management/commands/update_first_ready_values.py b/src/registrar/management/commands/update_first_ready_values.py new file mode 100644 index 000000000..9ee02109d --- /dev/null +++ b/src/registrar/management/commands/update_first_ready_values.py @@ -0,0 +1,28 @@ +import logging +from django.core.management import BaseCommand +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors +from registrar.models import Domain, TransitionDomain + +logger = logging.getLogger(__name__) + +class Command(BaseCommand, PopulateScriptTemplate): + help = "Loops through eachdomain object and populates the last_status_update and first_submitted_date" + + def handle(self, **kwargs): + """Loops through each valid Domain object and updates it's first_ready value if they are out of sync""" + filter_conditions="state__in=[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]" + self.mass_update_records(Domain, filter_conditions, ["first_ready"]) + + def update_record(self, record: Domain): + """Defines how we update the first_read field""" + # if these are out of sync, update the + if self.is_transition_domain(record) and record.first_ready != record.created_at: + record.first_ready = record.created_at + + logger.info( + f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" + ) + + # check if a transition domain object for this domain name exists + def is_transition_domain(record: Domain): + return TransitionDomain.objects.filter(domain_name=record.name).exists() \ No newline at end of file From fbfab28f3b2709c20de4356af18f2a6e468dcde1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 11:17:36 -0600 Subject: [PATCH 051/329] skeleton --- src/registrar/admin.py | 9 +++++ .../migrations/0119_allowedemails.py | 25 +++++++++++++ .../migrations/0120_create_groups_v16.py | 37 +++++++++++++++++++ src/registrar/models/__init__.py | 3 ++ src/registrar/models/allowed_emails.py | 20 ++++++++++ 5 files changed, 94 insertions(+) create mode 100644 src/registrar/migrations/0119_allowedemails.py create mode 100644 src/registrar/migrations/0120_create_groups_v16.py create mode 100644 src/registrar/models/allowed_emails.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..730e0fb20 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3169,6 +3169,14 @@ class SuborganizationAdmin(ListHeaderAdmin, ImportExportModelAdmin): extra_context = {"domain_requests": domain_requests, "domains": domains} return super().change_view(request, object_id, form_url, extra_context) +class AllowedEmailsAdmin(ListHeaderAdmin): + class Meta: + model = models.AllowedEmails + + list_display = ["email"] + search_fields = ["email"] + search_help_text = "Search by email." + ordering = ["email"] admin.site.unregister(LogEntry) # Unregister the default registration @@ -3197,6 +3205,7 @@ admin.site.register(models.Portfolio, PortfolioAdmin) admin.site.register(models.DomainGroup, DomainGroupAdmin) admin.site.register(models.Suborganization, SuborganizationAdmin) admin.site.register(models.SeniorOfficial, SeniorOfficialAdmin) +admin.site.register(models.AllowedEmails, AllowedEmailsAdmin) # Register our custom waffle implementations admin.site.register(models.WaffleFlag, WaffleFlagAdmin) diff --git a/src/registrar/migrations/0119_allowedemails.py b/src/registrar/migrations/0119_allowedemails.py new file mode 100644 index 000000000..ddfda75ca --- /dev/null +++ b/src/registrar/migrations/0119_allowedemails.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.10 on 2024-08-22 17:16 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), + ] + + operations = [ + migrations.CreateModel( + name="AllowedEmails", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ("email", models.EmailField(max_length=320, unique=True)), + ], + options={ + "abstract": False, + }, + ), + ] diff --git a/src/registrar/migrations/0120_create_groups_v16.py b/src/registrar/migrations/0120_create_groups_v16.py new file mode 100644 index 000000000..51330bc99 --- /dev/null +++ b/src/registrar/migrations/0120_create_groups_v16.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", "0119_allowedemails"), + ] + + operations = [ + migrations.RunPython( + create_groups, + reverse_code=migrations.RunPython.noop, + atomic=True, + ), + ] diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 1e0aad0b1..93723de7d 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -21,6 +21,7 @@ from .portfolio import Portfolio from .domain_group import DomainGroup from .suborganization import Suborganization from .senior_official import SeniorOfficial +from .allowed_emails import AllowedEmails __all__ = [ @@ -46,6 +47,7 @@ __all__ = [ "DomainGroup", "Suborganization", "SeniorOfficial", + "AllowedEmails", ] auditlog.register(Contact) @@ -70,3 +72,4 @@ auditlog.register(Portfolio) auditlog.register(DomainGroup) auditlog.register(Suborganization) auditlog.register(SeniorOfficial) +auditlog.register(AllowedEmails) diff --git a/src/registrar/models/allowed_emails.py b/src/registrar/models/allowed_emails.py new file mode 100644 index 000000000..e89c5904b --- /dev/null +++ b/src/registrar/models/allowed_emails.py @@ -0,0 +1,20 @@ +from django.db import models + +from .utility.time_stamped_model import TimeStampedModel + + +class AllowedEmails(TimeStampedModel): + """ + AllowedEmails is a whitelist for email addresses that we can send to + in non-production environments. + """ + + email = models.EmailField( + unique=True, + null=False, + blank=False, + max_length=320, + ) + + def __str__(self): + return str(self.email) From 42dec38b224dc15b0cbace67f88c2ee8e127af5d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 12:12:38 -0600 Subject: [PATCH 052/329] skeleton pt 2 --- src/registrar/admin.py | 6 +++--- .../{0119_allowedemails.py => 0119_allowedemail.py} | 2 +- src/registrar/migrations/0120_create_groups_v16.py | 2 +- src/registrar/models/__init__.py | 6 +++--- .../models/{allowed_emails.py => allowed_email.py} | 4 ++-- src/registrar/templates/admin/model_descriptions.html | 2 ++ .../includes/descriptions/allowed_email_description.html | 6 ++++++ 7 files changed, 18 insertions(+), 10 deletions(-) rename src/registrar/migrations/{0119_allowedemails.py => 0119_allowedemail.py} (95%) rename src/registrar/models/{allowed_emails.py => allowed_email.py} (74%) create mode 100644 src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 730e0fb20..9e5fb71aa 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3169,9 +3169,9 @@ class SuborganizationAdmin(ListHeaderAdmin, ImportExportModelAdmin): extra_context = {"domain_requests": domain_requests, "domains": domains} return super().change_view(request, object_id, form_url, extra_context) -class AllowedEmailsAdmin(ListHeaderAdmin): +class AllowedEmailAdmin(ListHeaderAdmin): class Meta: - model = models.AllowedEmails + model = models.AllowedEmail list_display = ["email"] search_fields = ["email"] @@ -3205,7 +3205,7 @@ admin.site.register(models.Portfolio, PortfolioAdmin) admin.site.register(models.DomainGroup, DomainGroupAdmin) admin.site.register(models.Suborganization, SuborganizationAdmin) admin.site.register(models.SeniorOfficial, SeniorOfficialAdmin) -admin.site.register(models.AllowedEmails, AllowedEmailsAdmin) +admin.site.register(models.AllowedEmail, AllowedEmailAdmin) # Register our custom waffle implementations admin.site.register(models.WaffleFlag, WaffleFlagAdmin) diff --git a/src/registrar/migrations/0119_allowedemails.py b/src/registrar/migrations/0119_allowedemail.py similarity index 95% rename from src/registrar/migrations/0119_allowedemails.py rename to src/registrar/migrations/0119_allowedemail.py index ddfda75ca..9d63d6973 100644 --- a/src/registrar/migrations/0119_allowedemails.py +++ b/src/registrar/migrations/0119_allowedemail.py @@ -11,7 +11,7 @@ class Migration(migrations.Migration): operations = [ migrations.CreateModel( - name="AllowedEmails", + name="AllowedEmail", fields=[ ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), ("created_at", models.DateTimeField(auto_now_add=True)), diff --git a/src/registrar/migrations/0120_create_groups_v16.py b/src/registrar/migrations/0120_create_groups_v16.py index 51330bc99..f08e20805 100644 --- a/src/registrar/migrations/0120_create_groups_v16.py +++ b/src/registrar/migrations/0120_create_groups_v16.py @@ -25,7 +25,7 @@ def create_groups(apps, schema_editor) -> Any: class Migration(migrations.Migration): dependencies = [ - ("registrar", "0119_allowedemails"), + ("registrar", "0119_allowedemail"), ] operations = [ diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 93723de7d..f525e690e 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -21,7 +21,7 @@ from .portfolio import Portfolio from .domain_group import DomainGroup from .suborganization import Suborganization from .senior_official import SeniorOfficial -from .allowed_emails import AllowedEmails +from .allowed_email import AllowedEmail __all__ = [ @@ -47,7 +47,7 @@ __all__ = [ "DomainGroup", "Suborganization", "SeniorOfficial", - "AllowedEmails", + "AllowedEmail", ] auditlog.register(Contact) @@ -72,4 +72,4 @@ auditlog.register(Portfolio) auditlog.register(DomainGroup) auditlog.register(Suborganization) auditlog.register(SeniorOfficial) -auditlog.register(AllowedEmails) +auditlog.register(AllowedEmail) diff --git a/src/registrar/models/allowed_emails.py b/src/registrar/models/allowed_email.py similarity index 74% rename from src/registrar/models/allowed_emails.py rename to src/registrar/models/allowed_email.py index e89c5904b..90105debe 100644 --- a/src/registrar/models/allowed_emails.py +++ b/src/registrar/models/allowed_email.py @@ -3,9 +3,9 @@ from django.db import models from .utility.time_stamped_model import TimeStampedModel -class AllowedEmails(TimeStampedModel): +class AllowedEmail(TimeStampedModel): """ - AllowedEmails is a whitelist for email addresses that we can send to + AllowedEmail is a whitelist for email addresses that we can send to in non-production environments. """ diff --git a/src/registrar/templates/admin/model_descriptions.html b/src/registrar/templates/admin/model_descriptions.html index 4b61e21bd..9f13245fe 100644 --- a/src/registrar/templates/admin/model_descriptions.html +++ b/src/registrar/templates/admin/model_descriptions.html @@ -32,6 +32,8 @@ {% include "django/admin/includes/descriptions/website_description.html" %} {% elif opts.model_name == 'portfolioinvitation' %} {% include "django/admin/includes/descriptions/portfolio_invitation_description.html" %} + {% elif opts.model_name == 'allowedemail' %} + {% include "django/admin/includes/descriptions/allowed_email_description.html" %} {% else %}

This table does not have a description yet.

{% endif %} diff --git a/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html b/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html new file mode 100644 index 000000000..4bac06437 --- /dev/null +++ b/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html @@ -0,0 +1,6 @@ +

This table is an email whitelist for non-production environments.

+

+ If an email is sent out and the email does not exist within this table (or is not a subset of it), + then no email will be sent. +

+

If this table is populated in a production environment, no change will occur as it will simply be ignored.

\ No newline at end of file From 056a3ecf36cd1d81461e3ec355c713183051d2ab Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 13:26:32 -0600 Subject: [PATCH 053/329] Add to fixtures and add whitelist logic --- src/registrar/fixtures_users.py | 38 +++++++++++++++++++++++++++ src/registrar/models/allowed_email.py | 31 +++++++++++++++++++++- src/registrar/utility/email.py | 25 +++++++++++++----- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 0fc203248..ecc30db91 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -6,6 +6,7 @@ from registrar.models import ( User, UserGroup, ) +from registrar.models.allowed_email import AllowedEmail fake = Faker() @@ -240,6 +241,11 @@ class UserFixture: }, ] + # Additional emails to add to the AllowedEmail whitelist + ADDITIONAL_ALLOWED_EMAILS = [ + "zander.adkinson@ecstech.com" + ] + def load_users(cls, users, group_name, are_superusers=False): logger.info(f"Going to load {len(users)} users in group {group_name}") for user_data in users: @@ -264,6 +270,34 @@ class UserFixture: logger.warning(e) logger.info(f"All users in group {group_name} loaded.") + def load_allowed_emails(cls, users, additional_emails): + """Populates a whitelist of allowed emails (as defined in this list)""" + logger.info(f"Going to load allowed emails for {len(users)} users") + if additional_emails: + logger.info(f"Going to load {len(additional_emails)} additional allowed emails") + + allowed_emails = [] + + # Load user emails + for user_data in users: + user_email = user_data.get("email") + if user_email and user_email not in allowed_emails: + allowed_emails.append(AllowedEmail(email=user_email)) + else: + first_name = user_data.get("first_name") + last_name = user_data.get("last_name") + logger.warning(f"Could not load email for {first_name} {last_name}: No email exists.") + + # Load additional emails + for email in additional_emails: + allowed_emails.append(AllowedEmail(email=email)) + + if allowed_emails: + AllowedEmail.objects.bulk_create(allowed_emails) + logger.info(f"Loaded {len(allowed_emails)} allowed emails") + else: + logger.info("No allowed emails to load") + @classmethod def load(cls): # Lumped under .atomic to ensure we don't make redundant DB calls. @@ -275,3 +309,7 @@ class UserFixture: with transaction.atomic(): cls.load_users(cls, cls.ADMINS, "full_access_group", are_superusers=True) cls.load_users(cls, cls.STAFF, "cisa_analysts_group") + + # Combine ADMINS and STAFF lists + all_users = cls.ADMINS + cls.STAFF + cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS) diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 90105debe..796f4b556 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -1,5 +1,6 @@ from django.db import models - +from django.db.models import Q +import re from .utility.time_stamped_model import TimeStampedModel @@ -16,5 +17,33 @@ class AllowedEmail(TimeStampedModel): max_length=320, ) + @classmethod + def is_allowed_email(cls, email): + """Given an email, check if this email exists within our AllowEmail whitelist""" + print(f"the email is: {email}") + if not email: + return False + + # Split the email into a local part and a domain part + local, domain = email.split('@') + + # Check if there's a '+' in the local part + if "+" in local: + base_local = local.split("+")[0] + base_email = f"{base_local}@{domain}" + allowed_emails = cls.objects.filter(email__iexact=base_email) + + # The string must start with the local, and the plus must be a digit + # and occur immediately after the local. The domain should still exist in the email. + pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + + # If the base email exists AND the email matches our expected regex, + # then we can let the email through. + return allowed_emails.exists() and re.match(pattern, email) + else: + # If no '+' exists in the email, just do an exact match + allowed_emails = cls.objects.filter(email__iexact=email) + return allowed_emails.exists() + def __str__(self): return str(self.email) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index e274893a2..8e40d4397 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -4,6 +4,7 @@ import boto3 import logging import textwrap from datetime import datetime +from django.apps import apps from django.conf import settings from django.template.loader import get_template from email.mime.application import MIMEApplication @@ -27,7 +28,7 @@ def send_templated_email( to_address: str, bcc_address="", context={}, - attachment_file: str = None, + attachment_file = None, wrap_email=False, ): """Send an email built from a template to one email address. @@ -39,9 +40,21 @@ def send_templated_email( Raises EmailSendingError if SES client could not be accessed """ - if flag_is_active(None, "disable_email_sending") and not settings.IS_PRODUCTION: # type: ignore - message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." - raise EmailSendingError(message) + + + if not settings.IS_PRODUCTION: # type: ignore + if flag_is_active(None, "disable_email_sending"): # type: ignore + message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." + raise EmailSendingError(message) + else: + # Raise an email sending error if these doesn't exist within our whitelist. + # If these emails don't exist, this function can handle that elsewhere. + AllowedEmail = apps.get_model('registrar', 'AllowedEmail') + message = "Could not send email. The email '{}' does not exist within the whitelist." + if to_address and not AllowedEmail.is_allowed_email(to_address): + raise EmailSendingError(message.format(to_address)) + if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): + raise EmailSendingError(message.format(bcc_address)) template = get_template(template_name) email_body = template.render(context=context) @@ -63,7 +76,7 @@ def send_templated_email( ) logger.info(f"An email was sent! Template name: {template_name} to {to_address}") except Exception as exc: - logger.debug("E-mail unable to send! Could not access the SES client.") + logger.debug("An email was unable to send! Could not access the SES client.") raise EmailSendingError("Could not access the SES client.") from exc destination = {"ToAddresses": [to_address]} @@ -71,7 +84,7 @@ def send_templated_email( destination["BccAddresses"] = [bcc_address] try: - if attachment_file is None: + if not attachment_file: # Wrap the email body to a maximum width of 80 characters per line. # Not all email clients support CSS to do this, and our .txt files require parsing. if wrap_email: From 48901a716d80f5f3bc5507d72a20100806bfc45c Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 14:30:14 -0500 Subject: [PATCH 054/329] rewrite script and extend terminal helper for more context --- ..._ready_values.py => update_first_ready.py} | 21 +++-- .../commands/utility/terminal_helper.py | 77 ++++++++++++++++--- 2 files changed, 75 insertions(+), 23 deletions(-) rename src/registrar/management/commands/{update_first_ready_values.py => update_first_ready.py} (56%) diff --git a/src/registrar/management/commands/update_first_ready_values.py b/src/registrar/management/commands/update_first_ready.py similarity index 56% rename from src/registrar/management/commands/update_first_ready_values.py rename to src/registrar/management/commands/update_first_ready.py index 9ee02109d..4803b70f8 100644 --- a/src/registrar/management/commands/update_first_ready_values.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -6,23 +6,22 @@ from registrar.models import Domain, TransitionDomain logger = logging.getLogger(__name__) class Command(BaseCommand, PopulateScriptTemplate): - help = "Loops through eachdomain object and populates the last_status_update and first_submitted_date" + help = "Loops through each domain object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each valid Domain object and updates it's first_ready value if they are out of sync""" - filter_conditions="state__in=[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]" - self.mass_update_records(Domain, filter_conditions, ["first_ready"]) + """Loops through each valid Domain object and updates it's first_ready value if it is out of sync""" + filter_conditions={"state__in":[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]} + self.mass_update_records(Domain, filter_conditions, ["first_ready"], verbose=True, custom_filter=self.should_update) def update_record(self, record: Domain): - """Defines how we update the first_read field""" - # if these are out of sync, update the - if self.is_transition_domain(record) and record.first_ready != record.created_at: - record.first_ready = record.created_at + """Defines how we update the first_ready field""" + # update the first_ready value based on the creation date. + record.first_ready = record.created_at logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" ) - # check if a transition domain object for this domain name exists - def is_transition_domain(record: Domain): - return TransitionDomain.objects.filter(domain_name=record.name).exists() \ No newline at end of file + # check if a transition domain object for this domain name exists, and if so whether + def should_update(self, record: Domain) -> bool: + return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at \ No newline at end of file diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 2c69e1080..13d58191a 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -2,6 +2,7 @@ import logging import sys from abc import ABC, abstractmethod from django.core.paginator import Paginator +from django.db.models import Model from typing import List from registrar.utility.enums import LogCode @@ -75,28 +76,74 @@ class PopulateScriptTemplate(ABC): run_summary_header = None @abstractmethod - def update_record(self, record): - """Defines how we update each field. Must be defined before using mass_update_records.""" + def update_record(self, record: Model): + """Defines how we update each field. + + raises: + NotImplementedError: If not defined before calling mass_update_records. + """ raise NotImplementedError - def mass_update_records(self, object_class, filter_conditions, fields_to_update, debug=True): + def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False, custom_filter=None): """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. - You must define update_record before you can use this function. + Parameters: + object_class: The Django model class that you want to perform the bulk update on. + This should be the actual class, not a string of the class name. + + filter_conditions: dictionary of valid Django Queryset filter conditions + (e.g. {'verification_type__isnull'=True}). + + fields_to_update: List of strings specifying which fields to update. + (e.g. ["first_ready_date", "last_submitted_date"]) + + debug: Whether to log script run summary in debug mode. + Default: True. + + verbose: Whether to print a detailed run summary *before* run confirmation. + Default: False. + + custom_filter: function taking in a single record and returning a boolean representing whether + the record should be included of the final record set. Used to allow for filters that can't be + represented by django queryset field lookups. Applied *after* filter_conditions. + + Raises: + NotImplementedError: If you do not define update_record before using this function. + TypeError: If custom_filter is not Callable. """ records = object_class.objects.filter(**filter_conditions) + + # apply custom filter + if custom_filter: + to_exclude_pks = [] + for record in records: + try: + if not custom_filter(record): + to_exclude_pks.append(record.pk) + except TypeError as e: + logger.error(f"Error applying custom filter: {e}") + sys.exit() + records=records.exclude(pk__in=to_exclude_pks) + readable_class_name = self.get_class_name(object_class) + # for use in the execution prompt. + proposed_changes=f"""==Proposed Changes== + Number of {readable_class_name} objects to change: {len(records)} + These fields will be updated on each record: {fields_to_update} + """ + + if verbose: + proposed_changes=f"""{proposed_changes} + These records will be updated: {list(records.all())} + """ + # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" - ==Proposed Changes== - Number of {readable_class_name} objects to change: {len(records)} - These fields will be updated on each record: {fields_to_update} - """, + prompt_message=proposed_changes, prompt_title=self.prompt_title, ) logger.info("Updating...") @@ -220,6 +267,9 @@ class TerminalHelper: an answer is required of the user). The "answer" return value is True for "yes" or False for "no". + + Raises: + ValueError: When "default" is not "yes", "no", or None. """ valid = {"yes": True, "y": True, "ye": True, "no": False, "n": False} if default is None: @@ -244,6 +294,7 @@ class TerminalHelper: @staticmethod def query_yes_no_exit(question: str, default="yes"): """Ask a yes/no question via raw_input() and return their answer. + Allows for answer "e" to exit. "question" is a string that is presented to the user. "default" is the presumed answer if the user just hits . @@ -251,6 +302,9 @@ class TerminalHelper: an answer is required of the user). The "answer" return value is True for "yes" or False for "no". + + Raises: + ValueError: When "default" is not "yes", "no", or None. """ valid = { "yes": True, @@ -317,9 +371,8 @@ class TerminalHelper: case _: logger.info(print_statement) - # TODO - "info_to_inspect" should be refactored to "prompt_message" @staticmethod - def prompt_for_execution(system_exit_on_terminate: bool, info_to_inspect: str, prompt_title: str) -> bool: + def prompt_for_execution(system_exit_on_terminate: bool, prompt_message: str, prompt_title: str) -> bool: """Create to reduce code complexity. Prompts the user to inspect the given string and asks if they wish to proceed. @@ -340,7 +393,7 @@ class TerminalHelper: ===================================================== *** IMPORTANT: VERIFY THE FOLLOWING LOOKS CORRECT *** - {info_to_inspect} + {prompt_message} {TerminalColors.FAIL} Proceed? (Y = proceed, N = {action_description_for_selecting_no}) {TerminalColors.ENDC}""" From 7fb186e24b32cc9582f9f4484bbfd60ce874e011 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 14:11:28 -0600 Subject: [PATCH 055/329] Display logic in admin --- src/registrar/admin.py | 26 ++++++++++++++++++++++++++ src/registrar/models/domain_request.py | 12 ++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9e5fb71aa..995a00fde 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -7,6 +7,7 @@ from django import forms from django.db.models import Value, CharField, Q from django.db.models.functions import Concat, Coalesce from django.http import HttpResponseRedirect +from django.conf import settings from django.shortcuts import redirect from django_fsm import get_available_FIELD_transitions, FSMField from registrar.models.domain_information import DomainInformation @@ -306,6 +307,7 @@ class DomainRequestAdminForm(forms.ModelForm): return cleaned_data + def _check_for_valid_rejection_reason(self, rejection_reason) -> bool: """ Checks if the rejection_reason field is not none. @@ -1914,6 +1916,19 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): else: obj.action_needed_reason_email = default_email + if obj.status in DomainRequest.get_statuses_that_send_emails(): + if not settings.IS_PRODUCTION: + profile_flag = flag_is_active(None, "profile_feature") + if profile_flag and hasattr(obj, "creator"): + recipient = obj.creator + elif not profile_flag and hasattr(obj, "submitter"): + recipient = obj.submitter + else + recipient = None + + if recipient and recipient.email: + self._check_for_valid_email(request, recipient.email) + # == Handle status == # if obj.status == original_obj.status: # If the status hasn't changed, let the base function take care of it @@ -1926,6 +1941,17 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): if should_save: return super().save_model(request, obj, form, change) + def _check_for_valid_email(self, request, email): + """Certain emails are whitelisted in non-production environments, + so we should display that information using this function. + + """ + + allowed = models.AllowedEmail.is_allowed_email(email) + error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + if not allowed: + messages.warning(request, error_message) + def _handle_status_change(self, request, obj, original_obj): """ Checks for various conditions when a status change is triggered. diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 966c880d7..812f8e582 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -577,6 +577,18 @@ class DomainRequest(TimeStampedModel): blank=True, ) + @classmethod + def get_statuses_that_send_emails(cls): + """Returns a list of statuses that send an email to the user""" + excluded_statuses = [ + cls.DomainRequestStatus.INELIGIBLE, + cls.DomainRequestStatus.IN_REVIEW + ] + return [ + status for status in cls.DomainRequestStatus + if status not in excluded_statuses + ] + def sync_organization_type(self): """ Updates the organization_type (without saving) to match From e59d4738e388ea2bad4a73ab99cdee68bbad475c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 16:44:25 -0400 Subject: [PATCH 056/329] filtering by portfolios --- src/registrar/admin.py | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..4be9c375d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -34,6 +34,7 @@ from django_fsm import TransitionNotAllowed # type: ignore from django.utils.safestring import mark_safe from django.utils.html import escape from django.contrib.auth.forms import UserChangeForm, UsernameField +from django.contrib.admin.views.main import ChangeList, IGNORED_PARAMS from django_admin_multiple_choice_list_filter.list_filters import MultipleChoiceListFilter from import_export import resources from import_export.admin import ImportExportModelAdmin @@ -45,6 +46,27 @@ from django.utils.translation import gettext_lazy as _ logger = logging.getLogger(__name__) +class CustomChangeListForPortfolioFiltering(ChangeList): + """CustomChangeList so that portfolio can be passed in a url, but not appear + in the list of filters on the right side of the page.""" + + def get_filters_params(self, params=None): + """ + Return all params except IGNORED_PARAMS. + """ + params = params or self.params + lookup_params = params.copy() # a dictionary of the query string + # Remove all the parameters that are globally and systematically + # ignored. + # Remove portfolio so that it does not error as an invalid + # filter parameter. + ignored_params = list(IGNORED_PARAMS) + ['portfolio'] + for ignored in ignored_params: + if ignored in lookup_params: + del lookup_params[ignored] + return lookup_params + + class FsmModelResource(resources.ModelResource): """ModelResource is extended to support importing of tables which have FSMFields. ModelResource is extended with the following changes @@ -644,6 +666,19 @@ class ListHeaderAdmin(AuditedAdmin, OrderableFieldsMixin): ) except models.User.DoesNotExist: pass + elif parameter_name == "portfolio": + # Retrieves the corresponding portfolio from Portfolio + id_value = request.GET.get(param) + try: + portfolio = models.Portfolio.objects.get(id=id_value) + filters.append( + { + "parameter_name": "portfolio", + "parameter_value": portfolio.organization_name, + } + ) + except models.Portfolio.DoesNotExist: + pass else: # For other parameter names, append a dictionary with the original # parameter_name and the corresponding parameter_value @@ -2235,6 +2270,23 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # objects rather than Contact objects. use_sort = db_field.name != "senior_official" return super().formfield_for_foreignkey(db_field, request, use_admin_sort_fields=use_sort, **kwargs) + + def get_queryset(self, request): + """Custom get_queryset to filter by portfolio if portfolio is in the + request params.""" + qs = super().get_queryset(request) + # Check if a 'portfolio' parameter is passed in the request + portfolio_id = request.GET.get('portfolio') + if portfolio_id: + # Further filter the queryset by the portfolio + qs = qs.filter(DomainRequest_info__portfolio=portfolio_id) + return qs + + def get_changelist(self, request, **kwargs): + """ + Return the ChangeList class for use on the changelist page. + """ + return CustomChangeListForPortfolioFiltering class TransitionDomainAdmin(ListHeaderAdmin): @@ -2688,6 +2740,23 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ): return True return super().has_change_permission(request, obj) + + def get_queryset(self, request): + """Custom get_queryset to filter by portfolio if portfolio is in the + request params.""" + qs = super().get_queryset(request) + # Check if a 'portfolio' parameter is passed in the request + portfolio_id = request.GET.get('portfolio') + if portfolio_id: + # Further filter the queryset by the portfolio + qs = qs.filter(domain_info__portfolio=portfolio_id) + return qs + + def get_changelist(self, request, **kwargs): + """ + Return the ChangeList class for use on the changelist page. + """ + return CustomChangeListForPortfolioFiltering class DraftDomainResource(resources.ModelResource): From 4026e9be8601ec63f9d1aa29a001ef6c3c3cd31d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 15:47:21 -0500 Subject: [PATCH 057/329] minor changes --- src/registrar/management/commands/update_first_ready.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 4803b70f8..aaf04e2a7 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -16,7 +16,7 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: Domain): """Defines how we update the first_ready field""" # update the first_ready value based on the creation date. - record.first_ready = record.created_at + record.first_ready = record.created_at.date() logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" @@ -24,4 +24,4 @@ class Command(BaseCommand, PopulateScriptTemplate): # check if a transition domain object for this domain name exists, and if so whether def should_update(self, record: Domain) -> bool: - return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at \ No newline at end of file + return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date() \ No newline at end of file From 6286ae96be749deffd0176eaa180f9f61a2365c6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 17:24:33 -0400 Subject: [PATCH 058/329] updated portfolio changeform view in admin --- src/registrar/admin.py | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4be9c375d..1e20523cc 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2279,7 +2279,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): portfolio_id = request.GET.get('portfolio') if portfolio_id: # Further filter the queryset by the portfolio - qs = qs.filter(DomainRequest_info__portfolio=portfolio_id) + qs = qs.filter(portfolio=portfolio_id) return qs def get_changelist(self, request, **kwargs): @@ -2942,7 +2942,7 @@ class PortfolioAdmin(ListHeaderAdmin): # "classes": ("collapse", "closed"), # "fields": ["administrators", "members"]} # ), - ("Portfolio domains", {"classes": ("collapse", "closed"), "fields": ["domains", "domain_requests"]}), + ("Portfolio domains", {"fields": ["domains", "domain_requests"]}), ("Type of organization", {"fields": ["organization_type", "federal_type"]}), ( "Organization name and mailing address", @@ -3022,21 +3022,30 @@ class PortfolioAdmin(ListHeaderAdmin): return self.get_field_links_as_list(queryset, "suborganization") suborganizations.short_description = "Suborganizations" # type: ignore - + def domains(self, obj: models.Portfolio): - """Returns a list of links for each related domain""" - queryset = obj.get_domains() - return self.get_field_links_as_list( - queryset, "domaininformation", link_info_attribute="get_state_display_of_domain" - ) + """Returns the count of domains with a link to view them in the admin.""" + domain_count = obj.get_domains().count() # Count the related domains + if domain_count > 0: + # Construct the URL to the admin page, filtered by portfolio + url = reverse("admin:registrar_domain_changelist") + f"?portfolio={obj.id}" + label = "Domain" if domain_count == 1 else "Domains" + # Create a clickable link with the domain count + return format_html('{} {}', url, domain_count, label) + return "No Domains" domains.short_description = "Domains" # type: ignore def domain_requests(self, obj: models.Portfolio): - """Returns a list of links for each related domain request""" - queryset = obj.get_domain_requests() - return self.get_field_links_as_list(queryset, "domainrequest", link_info_attribute="get_status_display") - + """Returns the count of domain requests with a link to view them in the admin.""" + domain_request_count = obj.get_domain_requests().count() # Count the related domain requests + if domain_request_count > 0: + # Construct the URL to the admin page, filtered by portfolio + url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}" + # Create a clickable link with the domain request count + return format_html('{} Domain Requests', url, domain_request_count) + return "No Domain Requests" + domain_requests.short_description = "Domain requests" # type: ignore # Creates select2 fields (with search bars) From 62ac4757ec1b0f41b05d87d4d5cd2e3baa503f40 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 16:25:24 -0500 Subject: [PATCH 059/329] review changes --- src/registrar/assets/js/get-gov.js | 4 +-- .../commands/populate_domain_request_dates.py | 36 +++++++++---------- ...0119_add_domainrequest_submission_dates.py | 2 +- src/registrar/models/domain_request.py | 8 ++--- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index f3b41eb51..70659b009 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1599,7 +1599,7 @@ document.addEventListener('DOMContentLoaded', function() { const domainName = request.requested_domain ? request.requested_domain : `New domain request
(${utcDateString(request.created_at)})`; const actionUrl = request.action_url; const actionLabel = request.action_label; - const submissionDate = request.submission_date ? new Date(request.submission_date).toLocaleDateString('en-US', options) : `Not submitted`; + const submissionDate = request.last_submitted_date ? new Date(request.last_submitted_date).toLocaleDateString('en-US', options) : `Not submitted`; // Even if the request is not deletable, we may need this empty string for the td if the deletable column is displayed let modalTrigger = ''; @@ -1699,7 +1699,7 @@ document.addEventListener('DOMContentLoaded', function() { ${domainName} - + ${submissionDate} diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 90fc06dcf..19d8e4f00 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -17,26 +17,26 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - try: - # Retrieve and order audit log entries by timestamp in descending order - audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") + + # Retrieve and order audit log entries by timestamp in descending order + audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") - # Loop through logs in descending order to find most recent status change - for log_entry in audit_log_entries: - if "status" in log_entry.changes: - record.last_status_update = log_entry.timestamp.date() - break - - # Loop through logs in ascending order to find first submission - for log_entry in audit_log_entries.reverse(): - if log_entry.changes_dict['status'](1) == 'Submitted': - record.first_submitted_date = log_entry.timestamp.date() + # Loop through logs in descending order to find most recent status change + for log_entry in audit_log_entries: + if 'status' in LogEntry.changes_dict: + record.last_status_update = log_entry.timestamp.date() + break - except ObjectDoesNotExist as e: - logger.error(f"Object with object_pk {record.pk} does not exist: {e}") - except Exception as e: - logger.error(f"An error occurred during update_record: {e}") + # Loop through logs in ascending order to find first submission + for log_entry in audit_log_entries.reverse(): + if log_entry.changes_dict['status'](1) == 'Submitted': + record.first_submitted_date = log_entry.timestamp.date() + break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.OKCYAN}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) + + def should_skip_record(self, record) -> bool: + # make sure the record had some kind of history + return LogEntry.objects.filter(object_pk=record.pk).exists() diff --git a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py index 0b94e1257..ea209626e 100644 --- a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -35,7 +35,7 @@ class Migration(migrations.Migration): field=models.DateField( blank=True, default=None, - help_text="Date of last status updated", + help_text="Date of the last status update", null=True, verbose_name="last updated on", ), diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 74d275d95..09f200793 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -563,7 +563,7 @@ class DomainRequest(TimeStampedModel): help_text="Acknowledged .gov acceptable use policy", ) - # initial submission date records when domain request was first submitted + # Records when the domain request was first submitted first_submitted_date = models.DateField( null=True, blank=True, @@ -572,7 +572,7 @@ class DomainRequest(TimeStampedModel): help_text="Date initially submitted", ) - # last submission date records when domain request was last submitted + # Records when domain request was last submitted last_submitted_date = models.DateField( null=True, blank=True, @@ -581,13 +581,13 @@ class DomainRequest(TimeStampedModel): help_text="Date last submitted", ) - # last status update records when domain request status was last updated by an admin or analyst + # Records when domain request status was last updated by an admin or analyst last_status_update = models.DateField( null=True, blank=True, default=None, verbose_name="last updated on", - help_text="Date of last status updated", + help_text="Date of the last status update", ) notes = models.TextField( null=True, From 4a104b6ff6afbbe1870f630205af5fb66f557611 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 17:25:52 -0400 Subject: [PATCH 060/329] formatted for linter --- src/registrar/admin.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 1e20523cc..a49536102 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -60,7 +60,7 @@ class CustomChangeListForPortfolioFiltering(ChangeList): # ignored. # Remove portfolio so that it does not error as an invalid # filter parameter. - ignored_params = list(IGNORED_PARAMS) + ['portfolio'] + ignored_params = list(IGNORED_PARAMS) + ["portfolio"] for ignored in ignored_params: if ignored in lookup_params: del lookup_params[ignored] @@ -2270,18 +2270,18 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # objects rather than Contact objects. use_sort = db_field.name != "senior_official" return super().formfield_for_foreignkey(db_field, request, use_admin_sort_fields=use_sort, **kwargs) - + def get_queryset(self, request): """Custom get_queryset to filter by portfolio if portfolio is in the request params.""" qs = super().get_queryset(request) # Check if a 'portfolio' parameter is passed in the request - portfolio_id = request.GET.get('portfolio') + portfolio_id = request.GET.get("portfolio") if portfolio_id: # Further filter the queryset by the portfolio qs = qs.filter(portfolio=portfolio_id) return qs - + def get_changelist(self, request, **kwargs): """ Return the ChangeList class for use on the changelist page. @@ -2740,18 +2740,18 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ): return True return super().has_change_permission(request, obj) - + def get_queryset(self, request): """Custom get_queryset to filter by portfolio if portfolio is in the request params.""" qs = super().get_queryset(request) # Check if a 'portfolio' parameter is passed in the request - portfolio_id = request.GET.get('portfolio') + portfolio_id = request.GET.get("portfolio") if portfolio_id: # Further filter the queryset by the portfolio qs = qs.filter(domain_info__portfolio=portfolio_id) return qs - + def get_changelist(self, request, **kwargs): """ Return the ChangeList class for use on the changelist page. @@ -3022,7 +3022,7 @@ class PortfolioAdmin(ListHeaderAdmin): return self.get_field_links_as_list(queryset, "suborganization") suborganizations.short_description = "Suborganizations" # type: ignore - + def domains(self, obj: models.Portfolio): """Returns the count of domains with a link to view them in the admin.""" domain_count = obj.get_domains().count() # Count the related domains @@ -3045,7 +3045,7 @@ class PortfolioAdmin(ListHeaderAdmin): # Create a clickable link with the domain request count return format_html('{} Domain Requests', url, domain_request_count) return "No Domain Requests" - + domain_requests.short_description = "Domain requests" # type: ignore # Creates select2 fields (with search bars) From 5da8119f86be3a8654cfabf915a4433556ad7326 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 19:02:55 -0400 Subject: [PATCH 061/329] adjusted code for correct version of django --- src/registrar/admin.py | 51 +++++++++++-------------------- src/registrar/tests/test_admin.py | 8 ++--- src/registrar/tests/test_api.py | 6 ++++ 3 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a49536102..65974b42b 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -34,7 +34,7 @@ from django_fsm import TransitionNotAllowed # type: ignore from django.utils.safestring import mark_safe from django.utils.html import escape from django.contrib.auth.forms import UserChangeForm, UsernameField -from django.contrib.admin.views.main import ChangeList, IGNORED_PARAMS +from django.contrib.admin.views.main import IGNORED_PARAMS from django_admin_multiple_choice_list_filter.list_filters import MultipleChoiceListFilter from import_export import resources from import_export.admin import ImportExportModelAdmin @@ -46,27 +46,6 @@ from django.utils.translation import gettext_lazy as _ logger = logging.getLogger(__name__) -class CustomChangeListForPortfolioFiltering(ChangeList): - """CustomChangeList so that portfolio can be passed in a url, but not appear - in the list of filters on the right side of the page.""" - - def get_filters_params(self, params=None): - """ - Return all params except IGNORED_PARAMS. - """ - params = params or self.params - lookup_params = params.copy() # a dictionary of the query string - # Remove all the parameters that are globally and systematically - # ignored. - # Remove portfolio so that it does not error as an invalid - # filter parameter. - ignored_params = list(IGNORED_PARAMS) + ["portfolio"] - for ignored in ignored_params: - if ignored in lookup_params: - del lookup_params[ignored] - return lookup_params - - class FsmModelResource(resources.ModelResource): """ModelResource is extended to support importing of tables which have FSMFields. ModelResource is extended with the following changes @@ -450,6 +429,22 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): return ordering + def get_filters_params(self, params=None): + """ + Return all params except IGNORED_PARAMS. + """ + params = params or self.params + lookup_params = params.copy() # a dictionary of the query string + # Remove all the parameters that are globally and systematically + # ignored. + # Remove portfolio so that it does not error as an invalid + # filter parameter. + ignored_params = list(IGNORED_PARAMS) + ["portfolio"] + for ignored in ignored_params: + if ignored in lookup_params: + del lookup_params[ignored] + return lookup_params + class CustomLogEntryAdmin(LogEntryAdmin): """Overwrite the generated LogEntry admin class""" @@ -2282,12 +2277,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): qs = qs.filter(portfolio=portfolio_id) return qs - def get_changelist(self, request, **kwargs): - """ - Return the ChangeList class for use on the changelist page. - """ - return CustomChangeListForPortfolioFiltering - class TransitionDomainAdmin(ListHeaderAdmin): """Custom transition domain admin class.""" @@ -2752,12 +2741,6 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): qs = qs.filter(domain_info__portfolio=portfolio_id) return qs - def get_changelist(self, request, **kwargs): - """ - Return the ChangeList class for use on the changelist page. - """ - return CustomChangeListForPortfolioFiltering - class DraftDomainResource(resources.ModelResource): """defines how each field in the referenced model should be mapped to the corresponding fields in the diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..d4404b564 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2107,9 +2107,7 @@ class TestPortfolioAdmin(TestCase): domain_2.save() domains = self.admin.domains(self.portfolio) - self.assertIn("domain1.gov", domains) - self.assertIn("domain2.gov", domains) - self.assertIn('
    ', domains) + self.assertIn("2 Domains", domains) @less_console_noise_decorator def test_domain_requests_display(self): @@ -2118,6 +2116,4 @@ class TestPortfolioAdmin(TestCase): completed_domain_request(name="request2.gov", portfolio=self.portfolio) domain_requests = self.admin.domain_requests(self.portfolio) - self.assertIn("request1.gov", domain_requests) - self.assertIn("request2.gov", domain_requests) - self.assertIn('
      ', domain_requests) + self.assertIn("2 Domain Requests", domain_requests) diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 0025bc902..de52ad3d6 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user +from api.tests.common import less_console_noise_decorator + class GetSeniorOfficialJsonTest(TestCase): def setUp(self): @@ -26,6 +28,7 @@ class GetSeniorOfficialJsonTest(TestCase): SeniorOfficial.objects.all().delete() FederalAgency.objects.all().delete() + @less_console_noise_decorator def test_get_senior_official_json_authenticated_superuser(self): """Test that a superuser can fetch the senior official information.""" p = "adminpass" @@ -38,6 +41,7 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(data["last_name"], "Doe") self.assertEqual(data["title"], "Director") + @less_console_noise_decorator def test_get_senior_official_json_authenticated_analyst(self): """Test that an analyst user can fetch the senior official's information.""" p = "userpass" @@ -50,6 +54,7 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(data["last_name"], "Doe") self.assertEqual(data["title"], "Director") + @less_console_noise_decorator def test_get_senior_official_json_unauthenticated(self): """Test that an unauthenticated user receives a 403 with an error message.""" p = "password" @@ -57,6 +62,7 @@ class GetSeniorOfficialJsonTest(TestCase): response = self.client.get(self.api_url, {"agency_name": "Test Agency"}) self.assertEqual(response.status_code, 302) + @less_console_noise_decorator def test_get_senior_official_json_not_found(self): """Test that a request for a non-existent agency returns a 404 with an error message.""" p = "adminpass" From 851e176f83209dd8e902ac79d1848650d37e2268 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 22 Aug 2024 21:45:00 -0400 Subject: [PATCH 062/329] added tests and comments --- src/registrar/admin.py | 10 ++++++-- src/registrar/tests/test_admin_domain.py | 28 +++++++++++++++++++++++ src/registrar/tests/test_admin_request.py | 25 ++++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 65974b42b..385c1e60e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -367,7 +367,9 @@ class DomainRequestAdminForm(forms.ModelForm): class MultiFieldSortableChangeList(admin.views.main.ChangeList): """ This class overrides the behavior of column sorting in django admin tables in order - to allow for multi field sorting on admin_order_field + to allow for multi field sorting on admin_order_field. It also overrides behavior + of getting the filter params to allow portfolio filters to be executed without + displaying on the right side of the ChangeList view. Usage: @@ -431,7 +433,11 @@ class MultiFieldSortableChangeList(admin.views.main.ChangeList): def get_filters_params(self, params=None): """ - Return all params except IGNORED_PARAMS. + Overrides the default behavior which gets filter_params, except + those in IGNORED_PARAMS. The override is to also include + portfolio in the overrides. This allows the portfolio filter + not to throw an error as a valid filter while not listing the + portfolio filter on the right side of the Change List view. """ params = params or self.params lookup_params = params.copy() # a dictionary of the query string diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index e156dd377..64d4ee77d 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/src/registrar/tests/test_admin_domain.py @@ -14,6 +14,7 @@ from registrar.models import ( DomainInformation, User, Host, + Portfolio, ) from .common import ( MockSESClient, @@ -359,6 +360,7 @@ class TestDomainAdminWithClient(TestCase): Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() + Portfolio.objects.all().delete() @classmethod def tearDownClass(self): @@ -452,6 +454,32 @@ class TestDomainAdminWithClient(TestCase): domain_request.delete() _creator.delete() + @less_console_noise_decorator + def test_domains_by_portfolio(self): + """ + Tests that domains display for a portfolio. + """ + + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) + # Create a fake domain request and domain + _domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio + ) + _domain_request.approve() + + domain = _domain_request.approved_domain + + self.client.force_login(self.superuser) + response = self.client.get( + "/admin/registrar/domain/?portfolio={}".format(portfolio.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + self.assertContains(response, portfolio.organization_name) + @less_console_noise_decorator def test_helper_text(self): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..04faf0504 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -22,6 +22,7 @@ from registrar.models import ( Contact, Website, SeniorOfficial, + Portfolio, ) from .common import ( MockSESClient, @@ -78,6 +79,7 @@ class TestDomainRequestAdmin(MockEppLib): Contact.objects.all().delete() Website.objects.all().delete() SeniorOfficial.objects.all().delete() + Portfolio.objects.all().delete() self.mock_client.EMAILS_SENT.clear() @classmethod @@ -263,6 +265,29 @@ class TestDomainRequestAdmin(MockEppLib): self.assertContains(response, domain_request.requested_domain.name) self.assertContains(response, "Show details") + @less_console_noise_decorator + def test_domain_requests_by_portfolio(self): + """ + Tests that domain_requests display for a portfolio. + """ + + portfolio, _ = Portfolio.objects.get_or_create(organization_name="Test Portfolio", creator=self.superuser) + # Create a fake domain request and domain + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, portfolio=portfolio + ) + + self.client.force_login(self.superuser) + response = self.client.get( + "/admin/registrar/domainrequest/?portfolio={}".format(portfolio.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + self.assertContains(response, portfolio.organization_name) + @less_console_noise_decorator def test_analyst_can_see_and_edit_alternative_domain(self): """Tests if an analyst can still see and edit the alternative domain field""" From c1378acd2c5ae5fa7dfa8034ed2cee1c6071cf7f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 10:46:22 -0600 Subject: [PATCH 063/329] minor fixes + add notes --- src/registrar/admin.py | 7 ++++- src/registrar/fixtures_users.py | 5 ++-- src/registrar/models/allowed_email.py | 5 +++- src/registrar/tests/test_models.py | 38 +++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 995a00fde..6fe7f9849 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1923,9 +1923,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): recipient = obj.creator elif not profile_flag and hasattr(obj, "submitter"): recipient = obj.submitter - else + else: recipient = None + # Displays a warning in admin when an email cannot be sent, + # Or a success message if it was. if recipient and recipient.email: self._check_for_valid_email(request, recipient.email) @@ -1949,8 +1951,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): allowed = models.AllowedEmail.is_allowed_email(email) error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + success_message = f"An email to '{email}' was sent!" if not allowed: messages.warning(request, error_message) + else: + messages.success(request, success_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index ecc30db91..5f57dc93b 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -241,7 +241,8 @@ class UserFixture: }, ] - # Additional emails to add to the AllowedEmail whitelist + # Additional emails to add to the AllowedEmail whitelist. + # The format should be as follows: ["email@igorville.gov", "email2@igorville.gov"] ADDITIONAL_ALLOWED_EMAILS = [ "zander.adkinson@ecstech.com" ] @@ -286,7 +287,7 @@ class UserFixture: else: first_name = user_data.get("first_name") last_name = user_data.get("last_name") - logger.warning(f"Could not load email for {first_name} {last_name}: No email exists.") + logger.warning(f"Could add email to whitelist for {first_name} {last_name}: No email exists.") # Load additional emails for email in additional_emails: diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 796f4b556..7cf3df277 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -31,7 +31,10 @@ class AllowedEmail(TimeStampedModel): if "+" in local: base_local = local.split("+")[0] base_email = f"{base_local}@{domain}" - allowed_emails = cls.objects.filter(email__iexact=base_email) + allowed_emails = cls.objects.filter( + Q(email__iexact=base_email) | + Q(email__iexact=email) + ) # The string must start with the local, and the plus must be a digit # and occur immediately after the local. The domain should still exist in the email. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index f4e998fff..8c7225841 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2320,3 +2320,41 @@ class TestPortfolio(TestCase): self.assertEqual(portfolio.urbanization, "test123") self.assertEqual(portfolio.state_territory, DomainRequest.StateTerritoryChoices.PUERTO_RICO) + + +class TestAllowedEmail(TestCase): + """Tests our allowed email whitelist""" + + @less_console_noise_decorator + def setUp(self): + self.email = "mayor@igorville.gov" + self.domain_name = "igorvilleInTransition.gov" + self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") + self.user, _ = User.objects.get_or_create(email=self.email) + + def tearDown(self): + super().tearDown() + Domain.objects.all().delete() + DomainInvitation.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + DraftDomain.objects.all().delete() + TransitionDomain.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + UserDomainRole.objects.all().delete() + + + # Test for a normal email defined in the whitelist + # Test for a normal email NOT defined in the whitelist + + # Test for a +1 email defined in the whitelist + # Test for a +1 email NOT defined in the whitelist + + # Test for a +1 email NOT defined in the whitelist, but the normal is defined + # Test for a +1 email defined in the whitelist, but the normal is NOT defined + # Test for a +1 email NOT defined in the whitelist and NOT defined in the normal + + # Test for an invalid email that contains a '+' + + # TODO: We need a small test for domain request admin \ No newline at end of file From 416633a3ce4392ab566ff52e4fae54a67b284ba4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:25:49 -0600 Subject: [PATCH 064/329] Model unit test --- src/registrar/fixtures_users.py | 2 + src/registrar/models/allowed_email.py | 36 ++++---- src/registrar/tests/test_models.py | 115 +++++++++++++++++++++----- 3 files changed, 115 insertions(+), 38 deletions(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 5f57dc93b..f50afec8f 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -48,6 +48,7 @@ class UserFixture: "username": "eb2214cd-fc0c-48c0-9dbd-bc4cd6820c74", "first_name": "Alysia", "last_name": "Broddrick", + "email": "abroddrick@truss.works", }, { "username": "8f8e7293-17f7-4716-889b-1990241cbd39", @@ -64,6 +65,7 @@ class UserFixture: "username": "83c2b6dd-20a2-4cac-bb40-e22a72d2955c", "first_name": "Cameron", "last_name": "Dixon", + "email": "cameron.dixon@cisa.dhs.gov", }, { "username": "0353607a-cbba-47d2-98d7-e83dcd5b90ea", diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 7cf3df277..7910caf48 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -20,33 +20,37 @@ class AllowedEmail(TimeStampedModel): @classmethod def is_allowed_email(cls, email): """Given an email, check if this email exists within our AllowEmail whitelist""" - print(f"the email is: {email}") + if not email: return False # Split the email into a local part and a domain part - local, domain = email.split('@') + local, domain = email.split("@") + + # If the email exists within the whitelist, then do nothing else. + email_exists = cls.objects.filter(email__iexact=email).exists() + if email_exists: + return True # Check if there's a '+' in the local part if "+" in local: base_local = local.split("+")[0] - base_email = f"{base_local}@{domain}" - allowed_emails = cls.objects.filter( - Q(email__iexact=base_email) | - Q(email__iexact=email) - ) + base_email_exists = cls.objects.filter( + Q(email__iexact=f"{base_local}@{domain}") + ).exists() - # The string must start with the local, and the plus must be a digit - # and occur immediately after the local. The domain should still exist in the email. + # Given an example email, such as "joe.smoe+1@igorville.com" + # The full regex statement will be: "^joe.smoe\\+\\d+@igorville.com$" pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' - - # If the base email exists AND the email matches our expected regex, - # then we can let the email through. - return allowed_emails.exists() and re.match(pattern, email) + return base_email_exists and re.match(pattern, email) else: - # If no '+' exists in the email, just do an exact match - allowed_emails = cls.objects.filter(email__iexact=email) - return allowed_emails.exists() + # Edge case, the +1 record exists but the base does not, + # and the record we are checking is the base record. + pattern = f'^{re.escape(local)}\\+\\d+@{re.escape(domain)}$' + plus_email_exists = cls.objects.filter( + Q(email__iregex=pattern) + ).exists() + return plus_email_exists def __str__(self): return str(self.email) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8c7225841..be5f6da9b 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -17,6 +17,7 @@ from registrar.models import ( DomainInvitation, UserDomainRole, FederalAgency, + AllowedEmail, ) import boto3_mocking @@ -2328,33 +2329,103 @@ class TestAllowedEmail(TestCase): @less_console_noise_decorator def setUp(self): self.email = "mayor@igorville.gov" - self.domain_name = "igorvilleInTransition.gov" - self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") - self.user, _ = User.objects.get_or_create(email=self.email) + self.email_2 = "cake@igorville.gov" + self.plus_email = "mayor+1@igorville.gov" + self.invalid_plus_email = "1+mayor@igorville.gov" def tearDown(self): super().tearDown() - Domain.objects.all().delete() - DomainInvitation.objects.all().delete() - DomainInformation.objects.all().delete() - DomainRequest.objects.all().delete() - DraftDomain.objects.all().delete() - TransitionDomain.objects.all().delete() - Portfolio.objects.all().delete() - User.objects.all().delete() - UserDomainRole.objects.all().delete() - + AllowedEmail.objects.all().delete() - # Test for a normal email defined in the whitelist - # Test for a normal email NOT defined in the whitelist + def test_email_in_whitelist(self): + """Test for a normal email defined in the whitelist""" + AllowedEmail.objects.create(email=self.email) + is_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(is_allowed) - # Test for a +1 email defined in the whitelist - # Test for a +1 email NOT defined in the whitelist + def test_email_not_in_whitelist(self): + """Test for a normal email NOT defined in the whitelist""" + # Check a email not in the list + is_allowed = AllowedEmail.is_allowed_email(self.email_2) + self.assertFalse(AllowedEmail.objects.filter(email=self.email_2).exists()) + self.assertFalse(is_allowed) - # Test for a +1 email NOT defined in the whitelist, but the normal is defined - # Test for a +1 email defined in the whitelist, but the normal is NOT defined - # Test for a +1 email NOT defined in the whitelist and NOT defined in the normal + def test_plus_email_in_whitelist(self): + """Test for a +1 email defined in the whitelist""" + AllowedEmail.objects.create(email=self.plus_email) + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) - # Test for an invalid email that contains a '+' + def test_plus_email_not_in_whitelist(self): + """Test for a +1 email not defined in the whitelist""" + # This email should not be allowed. + # Checks that we do more than just a regex check on the record. + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertFalse(plus_email_allowed) - # TODO: We need a small test for domain request admin \ No newline at end of file + def test_plus_email_not_in_whitelist_but_base_email_is(self): + """ + Test for a +1 email NOT defined in the whitelist, but the normal one is defined. + Example: + normal (in whitelist) - joe@igorville.com + +1 email (not in whitelist) - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.email) + base_email_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(base_email_allowed) + + # The plus email should also be allowed + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) + + # This email shouldn't exist in the DB + self.assertFalse(AllowedEmail.objects.filter(email=self.plus_email).exists()) + + def test_plus_email_in_whitelist_but_base_email_is_not(self): + """ + Test for a +1 email defined in the whitelist, but the normal is NOT defined. + Example: + normal (not in whitelist) - joe@igorville.com + +1 email (in whitelist) - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.plus_email) + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) + + # The base email should also be allowed + base_email_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(base_email_allowed) + + # This email shouldn't exist in the DB + self.assertFalse(AllowedEmail.objects.filter(email=self.email).exists()) + + def test_invalid_regex_for_plus_email(self): + """ + Test for an invalid email that contains a '+'. + This base email should still pass, but the regex rule should not. + + Our regex should only pass for emails that end with a '+' + Example: + Invalid email - 1+joe@igorville.com + Valid email: - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.invalid_plus_email) + invalid_plus_email = AllowedEmail.is_allowed_email(self.invalid_plus_email) + # We still expect that this will pass, it exists in the db + self.assertTrue(invalid_plus_email) + + # The base email SHOULD NOT pass, as it doesn't match our regex + base_email = AllowedEmail.is_allowed_email(self.email) + self.assertFalse(base_email) + + # For good measure, also check the other plus email + regular_plus_email = AllowedEmail.is_allowed_email(self.plus_email) + self.assertFalse(regular_plus_email) + + # TODO: We need a small test for domain request admin + # We also need a basic test in test_emails based off the mocked is_allowed_email value. + # This will be simpler + # def test_email_in_whitelist_in_prod(self): + # """Tests that the whitelist does nothing when we are in production""" + # allowed_email = AllowedEmail.objects.create(email=self.email) + # self.assertEqual(allowed_email.is_allowed_email(), True) \ No newline at end of file From d95be51ebf7fe39c5b19f47bdb02d2fcd59d6dd7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:30:20 -0600 Subject: [PATCH 065/329] Update test_models.py --- src/registrar/tests/test_models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index be5f6da9b..3a78fa6df 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -236,7 +236,7 @@ class TestDomainRequest(TestCase): self, domain_request, msg, action, expected_count, expected_content=None, expected_email="mayor@igorville.com" ): """Check if an email was sent after performing an action.""" - + email_allowed = AllowedEmail.objects.get_or_create(email=expected_email) with self.subTest(msg=msg, action=action): with boto3_mocking.clients.handler_for("sesv2", self.mock_client): # Perform the specified action @@ -255,6 +255,8 @@ class TestDomainRequest(TestCase): email_content = sent_emails[0]["kwargs"]["Content"]["Simple"]["Body"]["Text"]["Data"] self.assertIn(expected_content, email_content) + email_allowed.delete() + @override_flag("profile_feature", active=False) @less_console_noise_decorator def test_submit_from_started_sends_email(self): From 86f520c3f6833857a587962cea8dc1e9617fa997 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:48:36 -0600 Subject: [PATCH 066/329] Update test_models.py --- src/registrar/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a78fa6df..77f908cf3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -236,7 +236,7 @@ class TestDomainRequest(TestCase): self, domain_request, msg, action, expected_count, expected_content=None, expected_email="mayor@igorville.com" ): """Check if an email was sent after performing an action.""" - email_allowed = AllowedEmail.objects.get_or_create(email=expected_email) + email_allowed, _ = AllowedEmail.objects.get_or_create(email=expected_email) with self.subTest(msg=msg, action=action): with boto3_mocking.clients.handler_for("sesv2", self.mock_client): # Perform the specified action From 866204cb8dccb26308c5d350d460a9eb3eb700ef Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:34:33 -0600 Subject: [PATCH 067/329] fix tests --- src/registrar/admin.py | 3 --- src/registrar/tests/test_admin_request.py | 14 ++++++++++++- src/registrar/tests/test_emails.py | 24 +++++++++++++++++++++++ src/registrar/tests/test_views_domain.py | 3 +++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6fe7f9849..f80a2da9e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1951,11 +1951,8 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): allowed = models.AllowedEmail.is_allowed_email(email) error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." - success_message = f"An email to '{email}' was sent!" if not allowed: messages.warning(request, error_message) - else: - messages.success(request, success_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..9b911e415 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -22,6 +22,7 @@ from registrar.models import ( Contact, Website, SeniorOfficial, + AllowedEmail, ) from .common import ( MockSESClient, @@ -52,6 +53,10 @@ class TestDomainRequestAdmin(MockEppLib): tests have available staffuser, superuser, client, admin and test_helper """ + @classmethod + def tearDownClass(cls): + super().tearDownClass() + @classmethod def setUpClass(self): super().setUpClass() @@ -84,6 +89,7 @@ class TestDomainRequestAdmin(MockEppLib): def tearDownClass(self): super().tearDownClass() User.objects.all().delete() + AllowedEmail.objects.all().delete() @less_console_noise_decorator def test_domain_request_senior_official_is_alphabetically_sorted(self): @@ -597,7 +603,8 @@ class TestDomainRequestAdmin(MockEppLib): ): """Helper method for the email test cases. email_index is the index of the email in mock_client.""" - + allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) + allowed_bcc_email, _ = AllowedEmail.objects.get_or_create(email=bcc_email_address) with less_console_noise(): # Access the arguments passed to send_email call_args = self.mock_client.EMAILS_SENT @@ -624,6 +631,9 @@ class TestDomainRequestAdmin(MockEppLib): if bcc_email_address: bcc_email = kwargs["Destination"]["BccAddresses"][0] self.assertEqual(bcc_email, bcc_email_address) + + allowed_email.delete() + allowed_bcc_email.delete() @override_settings(IS_PRODUCTION=True) @less_console_noise_decorator @@ -1686,6 +1696,8 @@ class TestDomainRequestAdmin(MockEppLib): # Patch Domain.is_active and django.contrib.messages.error simultaneously stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) + stack.enter_context(patch.object(messages, "success")) domain_request.status = another_state diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 8cf707004..5fcf7c7df 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -7,6 +7,7 @@ from waffle.testutils import override_flag from registrar.utility import email from registrar.utility.email import send_templated_email from .common import completed_domain_request +from registrar.models import AllowedEmail from api.tests.common import less_console_noise_decorator from datetime import datetime @@ -14,9 +15,32 @@ import boto3_mocking # type: ignore class TestEmails(TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + allowed_emails = [ + AllowedEmail(email="doesnotexist@igorville.com"), + AllowedEmail(email="testy@town.com"), + AllowedEmail(email="mayor@igorville.gov"), + AllowedEmail(email="testy2@town.com"), + AllowedEmail(email="cisaRep@igorville.gov"), + AllowedEmail(email="sender@example.com"), + AllowedEmail(email="recipient@example.com"), + ] + AllowedEmail.objects.bulk_create(allowed_emails) + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + AllowedEmail.objects.all().delete() + def setUp(self): self.mock_client_class = MagicMock() self.mock_client = self.mock_client_class.return_value + + def tearDown(self): + super().tearDown() @boto3_mocking.patching @override_flag("disable_email_sending", active=True) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 3a90543a2..15c2ff92b 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -27,6 +27,7 @@ from registrar.models import ( Domain, DomainInformation, DomainInvitation, + AllowedEmail, Contact, PublicContact, Host, @@ -460,6 +461,7 @@ class TestDomainManagers(TestDomainOverview): """Inviting a non-existent user sends them an email.""" # make sure there is no user with this email email_address = "mayor@igorville.gov" + allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) User.objects.filter(email=email_address).delete() self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) @@ -479,6 +481,7 @@ class TestDomainManagers(TestDomainOverview): Destination={"ToAddresses": [email_address]}, Content=ANY, ) + allowed_email.delete() @boto3_mocking.patching @less_console_noise_decorator From 9f6d1324d9263a32d2c77e0ef7e58d45b36d28cd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:54:16 -0600 Subject: [PATCH 068/329] Fix tests not within helpers --- src/registrar/tests/test_admin_request.py | 15 ++++++++++----- src/registrar/tests/test_views_domain.py | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 9b911e415..27971fc7c 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -576,7 +576,8 @@ class TestDomainRequestAdmin(MockEppLib): ): """Helper method for the email test cases.""" - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), ExitStack() as stack: + stack.enter_context(patch.object(messages, "warning")) # Create a mock request request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) @@ -1155,6 +1156,7 @@ class TestDomainRequestAdmin(MockEppLib): with ExitStack() as stack: stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) domain_request.status = DomainRequest.DomainRequestStatus.REJECTED self.admin.save_model(request, domain_request, None, True) @@ -1183,6 +1185,7 @@ class TestDomainRequestAdmin(MockEppLib): with ExitStack() as stack: stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) domain_request.status = DomainRequest.DomainRequestStatus.REJECTED domain_request.rejection_reason = DomainRequest.RejectionReasons.CONTACTS_OR_ORGANIZATION_LEGITIMACY @@ -1234,11 +1237,13 @@ class TestDomainRequestAdmin(MockEppLib): request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - # Modify the domain request's property - domain_request.status = DomainRequest.DomainRequestStatus.APPROVED + with ExitStack() as stack: + stack.enter_context(patch.object(messages, "warning")) + # Modify the domain request's property + domain_request.status = DomainRequest.DomainRequestStatus.APPROVED - # Use the model admin's save_model method - self.admin.save_model(request, domain_request, form=None, change=True) + # Use the model admin's save_model method + self.admin.save_model(request, domain_request, form=None, change=True) # Test that approved domain exists and equals requested domain self.assertEqual(domain_request.requested_domain.name, domain_request.approved_domain.name) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 15c2ff92b..0db47fb8f 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -345,6 +345,22 @@ class TestDomainDetail(TestDomainOverview): class TestDomainManagers(TestDomainOverview): + @classmethod + def setUpClass(cls): + super().setUpClass() + allowed_emails = [ + AllowedEmail(email=""), + AllowedEmail(email="testy@town.com"), + AllowedEmail(email="mayor@igorville.gov"), + AllowedEmail(email="testy2@town.com"), + ] + AllowedEmail.objects.bulk_create(allowed_emails) + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + AllowedEmail.objects.all().delete() + def tearDown(self): """Ensure that the user has its original permissions""" super().tearDown() @@ -567,6 +583,7 @@ class TestDomainManagers(TestDomainOverview): """Inviting a user sends them an email, with email as the name.""" # Create a fake user object email_address = "mayor@igorville.gov" + allowed_email = AllowedEmail.objects.get_or_create(email=email_address) User.objects.get_or_create(email=email_address, username="fakeuser@fakeymail.com") # Make sure the user is staff From a7c801739d7ff19c379a6d5306e127eaa6725db1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 16:33:10 -0400 Subject: [PATCH 069/329] new view, transfer and delete logic, modal, combobox --- src/registrar/admin.py | 2 + src/registrar/assets/js/get-gov-admin.js | 62 +++-- src/registrar/assets/sass/_theme/_admin.scss | 56 ++++- src/registrar/config/settings.py | 3 +- src/registrar/config/urls.py | 2 + src/registrar/templates/admin/analytics.html | 20 +- .../templates/admin/transfer_user.html | 229 ++++++++++++++++++ .../django/admin/user_change_form.html | 15 ++ src/registrar/views/transfer_user.py | 158 ++++++++++++ 9 files changed, 492 insertions(+), 55 deletions(-) create mode 100644 src/registrar/templates/admin/transfer_user.html create mode 100644 src/registrar/views/transfer_user.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..4a34ac0f8 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -41,6 +41,8 @@ from django.core.exceptions import ObjectDoesNotExist from django.contrib.admin.widgets import FilteredSelectMultiple from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ +from django.shortcuts import get_object_or_404, render +from django.urls import path logger = logging.getLogger(__name__) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 01c93abf6..6c302836f 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -172,40 +172,39 @@ function addOrRemoveSessionBoolean(name, add){ ** To perform data operations on this - we need to use jQuery rather than vanilla js. */ (function (){ - let selector = django.jQuery("#id_investigator") - let assignSelfButton = document.querySelector("#investigator__assign_self"); - if (!selector || !assignSelfButton) { - return; - } - - let currentUserId = assignSelfButton.getAttribute("data-user-id"); - let currentUserName = assignSelfButton.getAttribute("data-user-name"); - if (!currentUserId || !currentUserName){ - console.error("Could not assign current user: no values found.") - return; - } - - // Hook a click listener to the "Assign to me" button. - // Logic borrowed from here: https://select2.org/programmatic-control/add-select-clear-items#create-if-not-exists - assignSelfButton.addEventListener("click", function() { - if (selector.find(`option[value='${currentUserId}']`).length) { - // Select the value that is associated with the current user. - selector.val(currentUserId).trigger("change"); - } else { - // Create a DOM Option that matches the desired user. Then append it and select it. - let userOption = new Option(currentUserName, currentUserId, true, true); - selector.append(userOption).trigger("change"); + if (document.getElementById("id_investigator") && django && django.jQuery) { + let selector = django.jQuery("#id_investigator") + let assignSelfButton = document.querySelector("#investigator__assign_self"); + if (!selector || !assignSelfButton) { + return; } - }); - // Listen to any change events, and hide the parent container if investigator has a value. - selector.on('change', function() { - // The parent container has display type flex. - assignSelfButton.parentElement.style.display = this.value === currentUserId ? "none" : "flex"; - }); - - + let currentUserId = assignSelfButton.getAttribute("data-user-id"); + let currentUserName = assignSelfButton.getAttribute("data-user-name"); + if (!currentUserId || !currentUserName){ + console.error("Could not assign current user: no values found.") + return; + } + // Hook a click listener to the "Assign to me" button. + // Logic borrowed from here: https://select2.org/programmatic-control/add-select-clear-items#create-if-not-exists + assignSelfButton.addEventListener("click", function() { + if (selector.find(`option[value='${currentUserId}']`).length) { + // Select the value that is associated with the current user. + selector.val(currentUserId).trigger("change"); + } else { + // Create a DOM Option that matches the desired user. Then append it and select it. + let userOption = new Option(currentUserName, currentUserId, true, true); + selector.append(userOption).trigger("change"); + } + }); + + // Listen to any change events, and hide the parent container if investigator has a value. + selector.on('change', function() { + // The parent container has display type flex. + assignSelfButton.parentElement.style.display = this.value === currentUserId ? "none" : "flex"; + }); + } })(); /** An IIFE for pages in DjangoAdmin that use a clipboard button @@ -215,7 +214,6 @@ function addOrRemoveSessionBoolean(name, add){ function copyToClipboardAndChangeIcon(button) { // Assuming the input is the previous sibling of the button let input = button.previousElementSibling; - let userId = input.getAttribute("user-id") // Copy input value to clipboard if (input) { navigator.clipboard.writeText(input.value).then(function() { diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 8ca6b5465..40aef1121 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -120,7 +120,7 @@ html[data-theme="light"] { body.dashboard, body.change-list, body.change-form, - .analytics { + .custom-admin-template, dt { color: var(--body-fg); } .usa-table td { @@ -149,7 +149,7 @@ html[data-theme="dark"] { body.dashboard, body.change-list, body.change-form, - .analytics { + .custom-admin-template, dt { color: var(--body-fg); } .usa-table td { @@ -160,7 +160,7 @@ html[data-theme="dark"] { // Remove when dark mode successfully applies to Django delete page. .delete-confirmation .content a:not(.button) { color: color('primary'); - } + } } @@ -364,14 +364,40 @@ input.admin-confirm-button { list-style-type: none; line-height: normal; } - .button { - display: inline-block; - padding: 10px 8px; - line-height: normal; - } - a.button:active, a.button:focus { - text-decoration: none; - } +} + +// This block resolves some of the issues we're seeing on buttons due to css +// conflicts between DJ and USWDS +a.button, +.usa-button { + display: inline-block; + padding: 10px 15px; + font-size: 14px; + line-height: 16.1px; + font-kerning: auto; + font-family: inherit; + font-weight: normal; +} +.button svg, +.button span, +.usa-button svg, +.usa-button span { + vertical-align: middle; +} +.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary) { + background: var(--button-bg); +} +.usa-button span { + font-size: 14px; +} +.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary):hover { + background: var(--button-hover-bg); +} +a.button:active, a.button:focus { + text-decoration: none; +} +.usa-modal { + font-family: inherit; } .module--custom { @@ -732,7 +758,7 @@ div.dja__model-description{ li { list-style-type: disc; - font-family: Source Sans Pro Web,Helvetica Neue,Helvetica,Roboto,Arial,sans-serif; + font-family: family('sans'); } a, a:link, a:visited { @@ -852,3 +878,9 @@ ul.add-list-reset { padding: 0 !important; margin: 0 !important; } + +// Fix the combobox when deployed outside admin (eg user transfer) +.submit-row .select2, +.submit-row .select2 span { + margin-top: 0; +} \ No newline at end of file diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 73aecad7a..d1f8d2384 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,13 +357,14 @@ CSP_FORM_ACTION = allowed_sources # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN CSP_DEFAULT_SRC = ("'self'",) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css"] +CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css", "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css"] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js", "https://www.ssa.gov", "https://ajax.googleapis.com", + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js" ] CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 59f52cd95..ee32930f1 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -24,6 +24,7 @@ from registrar.views.report_views import ( from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json +from registrar.views.transfer_user import TransferUserView from registrar.views.utility.api_views import get_senior_official_from_federal_agency_json from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 @@ -129,6 +130,7 @@ urlpatterns = [ AnalyticsView.as_view(), name="analytics", ), + path('admin/registrar/user//transfer/', TransferUserView.as_view(), name='transfer_user'), path( "admin/api/get-senior-official-from-federal-agency-json/", get_senior_official_from_federal_agency_json, diff --git a/src/registrar/templates/admin/analytics.html b/src/registrar/templates/admin/analytics.html index 13db3b60a..2729d883d 100644 --- a/src/registrar/templates/admin/analytics.html +++ b/src/registrar/templates/admin/analytics.html @@ -5,7 +5,7 @@ {% block content %} -
      +
      @@ -29,28 +29,28 @@
      +
      +
      + +
      +
      +{% endblock %} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/user_change_form.html b/src/registrar/templates/django/admin/user_change_form.html index 005d67aec..c0ddd8caf 100644 --- a/src/registrar/templates/django/admin/user_change_form.html +++ b/src/registrar/templates/django/admin/user_change_form.html @@ -1,6 +1,21 @@ {% extends 'django/admin/email_clipboard_change_form.html' %} {% load i18n static %} + +{% block field_sets %} +
      +
      + + Transfer data from old account + +
      +
      + + {% for fieldset in adminform %} + {% include "django/admin/includes/domain_fieldset.html" with state_help_message=state_help_message %} + {% endfor %} +{% endblock %} + {% block after_related_objects %}

      Associated requests and domains

      diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py new file mode 100644 index 000000000..5e367f04d --- /dev/null +++ b/src/registrar/views/transfer_user.py @@ -0,0 +1,158 @@ +import logging + +from django.shortcuts import render, get_object_or_404, redirect +from django.views import View +from registrar.models.domain import Domain +from registrar.models.domain_information import DomainInformation +from registrar.models.domain_request import DomainRequest +from registrar.models.portfolio import Portfolio +from registrar.models.user import User +from django.contrib.admin import site +from django.contrib import messages + +from registrar.models.user_domain_role import UserDomainRole +from registrar.models.verified_by_staff import VerifiedByStaff + +logger = logging.getLogger(__name__) + +class TransferUserView(View): + """Transfer user methods that set up the transfer_user template and handle the forms on it.""" + + JOINS = [ + (DomainRequest, 'creator'), + (DomainInformation, 'creator'), + (Portfolio, 'creator'), + (DomainRequest, 'investigator'), + (UserDomainRole, 'user'), + (VerifiedByStaff, 'requestor'), + ] + + USER_FIELDS = ['portfolio', 'portfolio_roles', 'portfolio_additional_permissions'] + + def get(self, request, user_id): + """current_user referes to the 'source' user where the button that redirects to this view was clicked. + other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. + + This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" + + current_user = get_object_or_404(User, pk=user_id) + other_users = User.objects.exclude(pk=user_id).order_by('first_name', 'last_name') # Exclude the current user from the dropdown + + # Get the default admin site context, needed for the sidenav + admin_context = site.each_context(request) + + context = { + 'current_user': current_user, + 'other_users': other_users, + 'logged_in_user': request.user, + **admin_context, # Include the admin context + 'current_user_domains': self.get_domains(current_user), + 'current_user_domain_requests': self.get_domain_requests(current_user) + } + + selected_user_id = request.GET.get('selected_user') + if selected_user_id: + selected_user = get_object_or_404(User, pk=selected_user_id) + context['selected_user'] = selected_user + context['selected_user_domains'] = self.get_domains(selected_user) + context['selected_user_domain_requests'] = self.get_domain_requests(selected_user) + + return render(request, 'admin/transfer_user.html', context) + + def post(self, request, user_id): + """This handles the transfer from selected_user to current_user then deletes selected_user. + + NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" + + current_user = get_object_or_404(User, pk=user_id) + selected_user_id = request.POST.get('selected_user') + selected_user = get_object_or_404(User, pk=selected_user_id) + + try: + change_logs = [] + + # Transfer specific fields + self.transfer_user_fields_and_log(selected_user, current_user, change_logs) + + # Perform the updates and log the changes + for model_class, field_name in self.JOINS: + self.update_joins_and_log(model_class, field_name, selected_user, current_user, change_logs) + + # Success message if any related objects were updated + if change_logs: + success_message = f"Data transferred successfully for the following objects: {change_logs}" + messages.success(request, success_message) + + selected_user.delete() + messages.success(request, f"Deleted {selected_user} {selected_user.username}") + + except Exception as e: + messages.error(request, f"An error occurred during the transfer: {e}") + + return redirect('admin:registrar_user_change', object_id=user_id) + + @classmethod + def update_joins_and_log(cls, model_class, field_name, old_user, new_user, change_logs): + """ + Helper function to update the user join fields for a given model and log the changes. + """ + + filter_kwargs = {field_name: old_user} + updated_objects = model_class.objects.filter(**filter_kwargs) + + for obj in updated_objects: + # Check for duplicate UserDomainRole before updating + if model_class == UserDomainRole: + if model_class.objects.filter(user=new_user, domain=obj.domain).exists(): + continue # Skip the update to avoid a duplicate + + # Update the field on the object and save it + setattr(obj, field_name, new_user) + obj.save() + + # Log the change + cls.log_change(obj, field_name, old_user, new_user, change_logs) + + @classmethod + def transfer_user_fields_and_log(cls, old_user, new_user, change_logs): + """ + Transfers portfolio fields from the old_user to the new_user. + Logs the changes for each transferred field. + + NOTE: This will be refactored in #2644 + """ + + for field in cls.USER_FIELDS: + old_value = getattr(old_user, field, None) + + if old_value: + setattr(new_user, field, old_value) + cls.log_change(new_user, field, old_value, old_value, change_logs) + + new_user.save() + + @classmethod + def log_change(cls, obj, field_name, old_value, new_value, change_logs): + """Logs the change for a specific field on an object""" + log_entry = f'Changed {field_name} from "{old_value}" to "{new_value}" on {obj}' + + logger.info(log_entry) + + # Collect the related object for the success message + change_logs.append(log_entry) + + @classmethod + def get_domains(cls, user): + """A simplified version of domains_json""" + user_domain_roles = UserDomainRole.objects.filter(user=user) + domain_ids = user_domain_roles.values_list("domain_id", flat=True) + domains = Domain.objects.filter(id__in=domain_ids) + + return domains + + @classmethod + def get_domain_requests(cls, user): + """A simplified version of domain_requests_json""" + domain_requests = DomainRequest.objects.filter(creator=user) + + return domain_requests \ No newline at end of file From 056df7151dbfc79462279904385824f1139a37ad Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:45:44 -0600 Subject: [PATCH 070/329] Fix remaining tests (hopefully) --- src/registrar/admin.py | 9 +++++---- src/registrar/fixtures_users.py | 6 ++---- src/registrar/models/allowed_email.py | 12 ++++-------- src/registrar/models/domain_request.py | 10 ++-------- src/registrar/tests/test_admin_request.py | 12 ++++++------ src/registrar/tests/test_emails.py | 6 +++--- src/registrar/tests/test_models.py | 6 +++--- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/utility/email.py | 6 ++---- 9 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f80a2da9e..48a5b61c3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -307,7 +307,6 @@ class DomainRequestAdminForm(forms.ModelForm): return cleaned_data - def _check_for_valid_rejection_reason(self, rejection_reason) -> bool: """ Checks if the rejection_reason field is not none. @@ -1923,7 +1922,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): recipient = obj.creator elif not profile_flag and hasattr(obj, "submitter"): recipient = obj.submitter - else: + else: recipient = None # Displays a warning in admin when an email cannot be sent, @@ -1944,9 +1943,9 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): return super().save_model(request, obj, form, change) def _check_for_valid_email(self, request, email): - """Certain emails are whitelisted in non-production environments, + """Certain emails are whitelisted in non-production environments, so we should display that information using this function. - + """ allowed = models.AllowedEmail.is_allowed_email(email) @@ -3197,6 +3196,7 @@ class SuborganizationAdmin(ListHeaderAdmin, ImportExportModelAdmin): extra_context = {"domain_requests": domain_requests, "domains": domains} return super().change_view(request, object_id, form_url, extra_context) + class AllowedEmailAdmin(ListHeaderAdmin): class Meta: model = models.AllowedEmail @@ -3206,6 +3206,7 @@ class AllowedEmailAdmin(ListHeaderAdmin): search_help_text = "Search by email." ordering = ["email"] + admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index f50afec8f..8476c72eb 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -245,9 +245,7 @@ class UserFixture: # Additional emails to add to the AllowedEmail whitelist. # The format should be as follows: ["email@igorville.gov", "email2@igorville.gov"] - ADDITIONAL_ALLOWED_EMAILS = [ - "zander.adkinson@ecstech.com" - ] + ADDITIONAL_ALLOWED_EMAILS = ["zander.adkinson@ecstech.com"] def load_users(cls, users, group_name, are_superusers=False): logger.info(f"Going to load {len(users)} users in group {group_name}") @@ -290,7 +288,7 @@ class UserFixture: first_name = user_data.get("first_name") last_name = user_data.get("last_name") logger.warning(f"Could add email to whitelist for {first_name} {last_name}: No email exists.") - + # Load additional emails for email in additional_emails: allowed_emails.append(AllowedEmail(email=email)) diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 7910caf48..6622bcc55 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -35,21 +35,17 @@ class AllowedEmail(TimeStampedModel): # Check if there's a '+' in the local part if "+" in local: base_local = local.split("+")[0] - base_email_exists = cls.objects.filter( - Q(email__iexact=f"{base_local}@{domain}") - ).exists() + base_email_exists = cls.objects.filter(Q(email__iexact=f"{base_local}@{domain}")).exists() # Given an example email, such as "joe.smoe+1@igorville.com" # The full regex statement will be: "^joe.smoe\\+\\d+@igorville.com$" - pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + pattern = f"^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$" return base_email_exists and re.match(pattern, email) else: # Edge case, the +1 record exists but the base does not, # and the record we are checking is the base record. - pattern = f'^{re.escape(local)}\\+\\d+@{re.escape(domain)}$' - plus_email_exists = cls.objects.filter( - Q(email__iregex=pattern) - ).exists() + pattern = f"^{re.escape(local)}\\+\\d+@{re.escape(domain)}$" + plus_email_exists = cls.objects.filter(Q(email__iregex=pattern)).exists() return plus_email_exists def __str__(self): diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 812f8e582..02457a539 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -580,14 +580,8 @@ class DomainRequest(TimeStampedModel): @classmethod def get_statuses_that_send_emails(cls): """Returns a list of statuses that send an email to the user""" - excluded_statuses = [ - cls.DomainRequestStatus.INELIGIBLE, - cls.DomainRequestStatus.IN_REVIEW - ] - return [ - status for status in cls.DomainRequestStatus - if status not in excluded_statuses - ] + excluded_statuses = [cls.DomainRequestStatus.INELIGIBLE, cls.DomainRequestStatus.IN_REVIEW] + return [status for status in cls.DomainRequestStatus if status not in excluded_statuses] def sync_organization_type(self): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 27971fc7c..d82826f33 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -56,7 +56,8 @@ class TestDomainRequestAdmin(MockEppLib): @classmethod def tearDownClass(cls): super().tearDownClass() - + AllowedEmail.objects.all.delete() + @classmethod def setUpClass(self): super().setUpClass() @@ -74,6 +75,8 @@ class TestDomainRequestAdmin(MockEppLib): model=DomainRequest, ) self.mock_client = MockSESClient() + allowed_emails = [AllowedEmail(email="mayor@igorville.gov"), AllowedEmail(email="help@get.gov")] + AllowedEmail.objects.bulk_create(allowed_emails) def tearDown(self): super().tearDown() @@ -604,8 +607,8 @@ class TestDomainRequestAdmin(MockEppLib): ): """Helper method for the email test cases. email_index is the index of the email in mock_client.""" - allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) - allowed_bcc_email, _ = AllowedEmail.objects.get_or_create(email=bcc_email_address) + AllowedEmail.objects.get_or_create(email=email_address) + AllowedEmail.objects.get_or_create(email=bcc_email_address) with less_console_noise(): # Access the arguments passed to send_email call_args = self.mock_client.EMAILS_SENT @@ -632,9 +635,6 @@ class TestDomainRequestAdmin(MockEppLib): if bcc_email_address: bcc_email = kwargs["Destination"]["BccAddresses"][0] self.assertEqual(bcc_email, bcc_email_address) - - allowed_email.delete() - allowed_bcc_email.delete() @override_settings(IS_PRODUCTION=True) @less_console_noise_decorator diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 5fcf7c7df..a98c16604 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -15,7 +15,7 @@ import boto3_mocking # type: ignore class TestEmails(TestCase): - + @classmethod def setUpClass(cls): super().setUpClass() @@ -29,7 +29,7 @@ class TestEmails(TestCase): AllowedEmail(email="recipient@example.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) - + @classmethod def tearDownClass(cls): super().tearDownClass() @@ -38,7 +38,7 @@ class TestEmails(TestCase): def setUp(self): self.mock_client_class = MagicMock() self.mock_client = self.mock_client_class.return_value - + def tearDown(self): super().tearDown() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 77f908cf3..8ee5dac3d 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2360,7 +2360,7 @@ class TestAllowedEmail(TestCase): def test_plus_email_not_in_whitelist(self): """Test for a +1 email not defined in the whitelist""" - # This email should not be allowed. + # This email should not be allowed. # Checks that we do more than just a regex check on the record. plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) self.assertFalse(plus_email_allowed) @@ -2426,8 +2426,8 @@ class TestAllowedEmail(TestCase): # TODO: We need a small test for domain request admin # We also need a basic test in test_emails based off the mocked is_allowed_email value. - # This will be simpler + # This will be simpler # def test_email_in_whitelist_in_prod(self): # """Tests that the whitelist does nothing when we are in production""" # allowed_email = AllowedEmail.objects.create(email=self.email) - # self.assertEqual(allowed_email.is_allowed_email(), True) \ No newline at end of file + # self.assertEqual(allowed_email.is_allowed_email(), True) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 0db47fb8f..0f8b59995 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -355,7 +355,7 @@ class TestDomainManagers(TestDomainOverview): AllowedEmail(email="testy2@town.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) - + @classmethod def tearDownClass(cls): super().tearDownClass() diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 8e40d4397..d77de3ed0 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -28,7 +28,7 @@ def send_templated_email( to_address: str, bcc_address="", context={}, - attachment_file = None, + attachment_file=None, wrap_email=False, ): """Send an email built from a template to one email address. @@ -40,8 +40,6 @@ def send_templated_email( Raises EmailSendingError if SES client could not be accessed """ - - if not settings.IS_PRODUCTION: # type: ignore if flag_is_active(None, "disable_email_sending"): # type: ignore message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." @@ -49,7 +47,7 @@ def send_templated_email( else: # Raise an email sending error if these doesn't exist within our whitelist. # If these emails don't exist, this function can handle that elsewhere. - AllowedEmail = apps.get_model('registrar', 'AllowedEmail') + AllowedEmail = apps.get_model("registrar", "AllowedEmail") message = "Could not send email. The email '{}' does not exist within the whitelist." if to_address and not AllowedEmail.is_allowed_email(to_address): raise EmailSendingError(message.format(to_address)) From e84f346e4bed7ff933dc8657eaee824e24cef71b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:57:49 -0600 Subject: [PATCH 071/329] linting --- src/registrar/admin.py | 37 +++++++++++------------ src/registrar/tests/test_admin_request.py | 5 --- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/utility/email.py | 29 ++++++++++-------- 4 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 48a5b61c3..0fe3a2c38 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1915,20 +1915,8 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): else: obj.action_needed_reason_email = default_email - if obj.status in DomainRequest.get_statuses_that_send_emails(): - if not settings.IS_PRODUCTION: - profile_flag = flag_is_active(None, "profile_feature") - if profile_flag and hasattr(obj, "creator"): - recipient = obj.creator - elif not profile_flag and hasattr(obj, "submitter"): - recipient = obj.submitter - else: - recipient = None - - # Displays a warning in admin when an email cannot be sent, - # Or a success message if it was. - if recipient and recipient.email: - self._check_for_valid_email(request, recipient.email) + if obj.status in DomainRequest.get_statuses_that_send_emails() and not settings.IS_PRODUCTION: + self._check_for_valid_email(request, obj) # == Handle status == # if obj.status == original_obj.status: @@ -1942,16 +1930,27 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): if should_save: return super().save_model(request, obj, form, change) - def _check_for_valid_email(self, request, email): + def _check_for_valid_email(self, request, obj): """Certain emails are whitelisted in non-production environments, so we should display that information using this function. """ + profile_flag = flag_is_active(request, "profile_feature") + if profile_flag and hasattr(obj, "creator"): + recipient = obj.creator + elif not profile_flag and hasattr(obj, "submitter"): + recipient = obj.submitter + else: + recipient = None - allowed = models.AllowedEmail.is_allowed_email(email) - error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." - if not allowed: - messages.warning(request, error_message) + # Displays a warning in admin when an email cannot be sent, + # Or a success message if it was. + if recipient and recipient.email: + email = recipient.email + allowed = models.AllowedEmail.is_allowed_email(email) + error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + if not allowed: + messages.warning(request, error_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index d82826f33..46b7c22f2 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -53,11 +53,6 @@ class TestDomainRequestAdmin(MockEppLib): tests have available staffuser, superuser, client, admin and test_helper """ - @classmethod - def tearDownClass(cls): - super().tearDownClass() - AllowedEmail.objects.all.delete() - @classmethod def setUpClass(self): super().setUpClass() diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 0f8b59995..ae3689703 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -583,7 +583,7 @@ class TestDomainManagers(TestDomainOverview): """Inviting a user sends them an email, with email as the name.""" # Create a fake user object email_address = "mayor@igorville.gov" - allowed_email = AllowedEmail.objects.get_or_create(email=email_address) + AllowedEmail.objects.get_or_create(email=email_address) User.objects.get_or_create(email=email_address, username="fakeuser@fakeymail.com") # Make sure the user is staff diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index d77de3ed0..533bd9e99 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -41,18 +41,7 @@ def send_templated_email( """ if not settings.IS_PRODUCTION: # type: ignore - if flag_is_active(None, "disable_email_sending"): # type: ignore - message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." - raise EmailSendingError(message) - else: - # Raise an email sending error if these doesn't exist within our whitelist. - # If these emails don't exist, this function can handle that elsewhere. - AllowedEmail = apps.get_model("registrar", "AllowedEmail") - message = "Could not send email. The email '{}' does not exist within the whitelist." - if to_address and not AllowedEmail.is_allowed_email(to_address): - raise EmailSendingError(message.format(to_address)) - if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): - raise EmailSendingError(message.format(bcc_address)) + _can_send_email(to_address, bcc_address) template = get_template(template_name) email_body = template.render(context=context) @@ -113,6 +102,22 @@ def send_templated_email( raise EmailSendingError("Could not send SES email.") from exc +def _can_send_email(to_address, bcc_address): + """Raises an error if we cannot send an error""" + if flag_is_active(None, "disable_email_sending"): # type: ignore + message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." + raise EmailSendingError(message) + else: + # Raise an email sending error if these doesn't exist within our whitelist. + # If these emails don't exist, this function can handle that elsewhere. + AllowedEmail = apps.get_model("registrar", "AllowedEmail") + message = "Could not send email. The email '{}' does not exist within the whitelist." + if to_address and not AllowedEmail.is_allowed_email(to_address): + raise EmailSendingError(message.format(to_address)) + if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): + raise EmailSendingError(message.format(bcc_address)) + + def wrap_text_and_preserve_paragraphs(text, width): """ Wraps text to `width` preserving newlines; splits on '\n', wraps segments, rejoins with '\n'. From 8a9971ac79bb08a934e269674a28993d5470b608 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:50:24 -0400 Subject: [PATCH 072/329] initial implementation --- src/registrar/assets/js/get-gov-admin.js | 51 +++++++++++++++++++ src/registrar/config/urls.py | 10 +++- src/registrar/models/portfolio.py | 16 ++++-- .../django/admin/portfolio_change_form.html | 2 + src/registrar/views/utility/api_views.py | 33 ++++++++++++ 5 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 01c93abf6..11e3cae69 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,27 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; + fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) + .then(response => { + const statusCode = response.status; + return response.json().then(data => ({ statusCode, data })); + }) + .then(({ statusCode, data }) => { + if (data.error) { + console.error("Error in AJAX call: " + data.error); + return; + } + + let federal_type = data.federal_type; + let portfolio_type = data.portfolio_type; + console.log("portfolio type: " + portfolio_type); + console.log("federal type: " + federal_type); + updateFederalType(data.federal_type); + updatePortfolioType(data.portfolio_type); + }) + .catch(error => console.error("Error fetching federal and portfolio types: ", error)); + let seniorOfficialApi = document.getElementById("senior_official_from_agency_json_url").value; fetch(`${seniorOfficialApi}?agency_name=${selectedText}`) .then(response => { @@ -879,6 +900,7 @@ function initializeWidgetOnList(list, parentId) { } }) .catch(error => console.error("Error fetching senior official: ", error)); + } function handleStateTerritoryChange(stateTerritory, urbanizationField) { @@ -890,6 +912,35 @@ function initializeWidgetOnList(list, parentId) { } } + function updatePortfolioType(portfolioType) { + console.log("attempting to update portfolioType"); + // Find the div with class 'field-portfolio_type' + const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); + if (portfolioTypeDiv) { + console.log("found portfoliotype"); + // Find the nested div with class 'readonly' inside 'field-portfolio_type' + const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + console.log("found readonly div"); + // Update the text content of the readonly div + readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; + } + } + } + + function updateFederalType(federalType) { + // Find the div with class 'field-federal_type' + const federalTypeDiv = document.querySelector('.field-federal_type'); + if (federalTypeDiv) { + // Find the nested div with class 'readonly' inside 'field-federal_type' + const readonlyDiv = federalTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + // Update the text content of the readonly div + readonlyDiv.textContent = federalType !== null ? federalType : '-'; + } + } + } + function updateContactInfo(data) { if (!contactList) return; diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 413449896..5e58cf740 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -24,7 +24,10 @@ from registrar.views.report_views import ( from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json -from registrar.views.utility.api_views import get_senior_official_from_federal_agency_json +from registrar.views.utility.api_views import ( + get_senior_official_from_federal_agency_json, + get_federal_and_portfolio_types_from_federal_agency_json +) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 from api.views import available, get_current_federal, get_current_full @@ -139,6 +142,11 @@ urlpatterns = [ get_senior_official_from_federal_agency_json, name="get-senior-official-from-federal-agency-json", ), + path( + "admin/api/get-federal-and-portfolio-types-from-federal-agency-json/", + get_federal_and_portfolio_types_from_federal_agency_json, + name="get-federal-and-portfolio-types-from-federal-agency-json", + ), path("admin/", admin.site.urls), path( "reports/export_data_type_user/", diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index 0f9904c31..b9b0d4a22 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -131,9 +131,13 @@ class Portfolio(TimeStampedModel): Returns a combination of organization_type / federal_type, seperated by ' - '. If no federal_type is found, we just return the org type. """ - org_type_label = self.OrganizationChoices.get_org_label(self.organization_type) - agency_type_label = BranchChoices.get_branch_label(self.federal_type) - if self.organization_type == self.OrganizationChoices.FEDERAL and agency_type_label: + return self.get_portfolio_type(self.organization_type, self.federal_type) + + @classmethod + def get_portfolio_type(cls, organization_type, federal_type): + org_type_label = cls.OrganizationChoices.get_org_label(organization_type) + agency_type_label = BranchChoices.get_branch_label(federal_type) + if organization_type == cls.OrganizationChoices.FEDERAL and agency_type_label: return " - ".join([org_type_label, agency_type_label]) else: return org_type_label @@ -141,8 +145,12 @@ class Portfolio(TimeStampedModel): @property def federal_type(self): """Returns the federal_type value on the underlying federal_agency field""" - return self.federal_agency.federal_type if self.federal_agency else None + return self.get_federal_type(self.federal_agency) + @classmethod + def get_federal_type(cls, federal_agency): + return federal_agency.federal_type if federal_agency else None + # == Getters for domains == # def get_domains(self): """Returns all DomainInformations associated with this portfolio""" diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 9d59aae42..4eb941340 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/src/registrar/templates/django/admin/portfolio_change_form.html @@ -5,6 +5,8 @@ {% comment %} Stores the json endpoint in a url for easier access {% endcomment %} {% url 'get-senior-official-from-federal-agency-json' as url %} + {% url 'get-federal-and-portfolio-types-from-federal-agency-json' as url %} + {{ block.super }} {% endblock content %} diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index 2c9414d1d..e67a1974c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -5,6 +5,9 @@ from registrar.models import FederalAgency, SeniorOfficial from django.contrib.admin.views.decorators import staff_member_required from django.contrib.auth.decorators import login_required +from registrar.models.portfolio import Portfolio +from registrar.utility.constants import BranchChoices + logger = logging.getLogger(__name__) @@ -34,3 +37,33 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) + +@login_required +@staff_member_required +def get_federal_and_portfolio_types_from_federal_agency_json(request): + """Returns specific portfolio information as a JSON. Request must have + both agency_name and organization_type.""" + + # This API is only accessible to admins and analysts + superuser_perm = request.user.has_perm("registrar.full_access_permission") + analyst_perm = request.user.has_perm("registrar.analyst_access_permission") + if not request.user.is_authenticated or not any([analyst_perm, superuser_perm]): + return JsonResponse({"error": "You do not have access to this resource"}, status=403) + + federal_type = None + portfolio_type = None + + agency_name = request.GET.get("agency_name") + organization_type = request.GET.get("organization_type") + agency = FederalAgency.objects.filter(agency=agency_name).first() + if agency: + federal_type = Portfolio.get_federal_type(agency) + portfolio_type = Portfolio.get_portfolio_type(organization_type, federal_type) + federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" + + response_data = { + 'portfolio_type': portfolio_type, + 'federal_type': federal_type, + } + + return JsonResponse(response_data) \ No newline at end of file From ef7739dc556b9e9c7b5c046934e4ee44a5bf733e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:52:57 -0400 Subject: [PATCH 073/329] lint --- src/registrar/config/urls.py | 2 +- src/registrar/models/portfolio.py | 2 +- src/registrar/views/utility/api_views.py | 11 ++++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 5e58cf740..19fa99809 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -26,7 +26,7 @@ from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json from registrar.views.utility.api_views import ( get_senior_official_from_federal_agency_json, - get_federal_and_portfolio_types_from_federal_agency_json + get_federal_and_portfolio_types_from_federal_agency_json, ) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index b9b0d4a22..fadcf8cac 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -150,7 +150,7 @@ class Portfolio(TimeStampedModel): @classmethod def get_federal_type(cls, federal_agency): return federal_agency.federal_type if federal_agency else None - + # == Getters for domains == # def get_domains(self): """Returns all DomainInformations associated with this portfolio""" diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index e67a1974c..97eb7e86c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -37,7 +37,8 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) - + + @login_required @staff_member_required def get_federal_and_portfolio_types_from_federal_agency_json(request): @@ -62,8 +63,8 @@ def get_federal_and_portfolio_types_from_federal_agency_json(request): federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" response_data = { - 'portfolio_type': portfolio_type, - 'federal_type': federal_type, + "portfolio_type": portfolio_type, + "federal_type": federal_type, } - - return JsonResponse(response_data) \ No newline at end of file + + return JsonResponse(response_data) From 31285da3956043a0a924ef4ea2a30d785a6a1c1a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 18:16:01 -0400 Subject: [PATCH 074/329] added test code --- src/registrar/tests/test_api.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 0025bc902..bd96f3e24 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user +from api.tests.common import less_console_noise_decorator + class GetSeniorOfficialJsonTest(TestCase): def setUp(self): @@ -65,3 +67,32 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(response.status_code, 404) data = response.json() self.assertEqual(data["error"], "Senior Official not found") + + +class GetFederalPortfolioTypeJsonTest(TestCase): + def setUp(self): + self.client = Client() + p = "password" + self.user = get_user_model().objects.create_user(username="testuser", password=p) + + self.superuser = create_superuser() + self.analyst_user = create_user() + + self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type="judicial") + + self.api_url = reverse("get-federal-and-portfolio-types-from-federal-agency-json") + + def tearDown(self): + User.objects.all().delete() + FederalAgency.objects.all().delete() + + @less_console_noise_decorator + def test_get_federal_and_portfolio_types_json_authenticated_superuser(self): + """Test that a superuser can fetch the federal and portfolio types.""" + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["federal_type"], "Judicial") + self.assertEqual(data["portfolio_type"], "Federal - Judicial") From c1df03d831c48c1d4cbb25afe2fa806ee687da5c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:15:42 -0400 Subject: [PATCH 075/329] test and lint --- src/registrar/admin.py | 2 - src/registrar/config/settings.py | 8 +- src/registrar/config/urls.py | 2 +- src/registrar/tests/test_admin.py | 225 ++++++++++++++++++++++++++- src/registrar/views/__init__.py | 1 + src/registrar/views/transfer_user.py | 82 +++++----- 6 files changed, 274 insertions(+), 46 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4a34ac0f8..3ad5e3ea0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -41,8 +41,6 @@ from django.core.exceptions import ObjectDoesNotExist from django.contrib.admin.widgets import FilteredSelectMultiple from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ -from django.shortcuts import get_object_or_404, render -from django.urls import path logger = logging.getLogger(__name__) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index d1f8d2384..7965424bc 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,14 +357,18 @@ CSP_FORM_ACTION = allowed_sources # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN CSP_DEFAULT_SRC = ("'self'",) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css", "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css"] +CSP_STYLE_SRC = [ + "'self'", + "https://www.ssa.gov/accessibility/andi/andi.css", + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css", +] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js", "https://www.ssa.gov", "https://ajax.googleapis.com", - "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js" + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js", ] CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index ee32930f1..0a8e00350 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -130,7 +130,7 @@ urlpatterns = [ AnalyticsView.as_view(), name="analytics", ), - path('admin/registrar/user//transfer/', TransferUserView.as_view(), name='transfer_user'), + path("admin/registrar/user//transfer/", TransferUserView.as_view(), name="transfer_user"), path( "admin/api/get-senior-official-from-federal-agency-json/", get_senior_official_from_federal_agency_json, diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..f051325a6 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ from registrar.models import ( from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_domain_role import UserDomainRole +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -60,7 +61,8 @@ from .common import ( ) from django.contrib.sessions.backends.db import SessionStore from django.contrib.auth import get_user_model -from unittest.mock import patch, Mock +from unittest.mock import ANY, patch, Mock +from django_webtest import WebTest # type: ignore import logging @@ -2121,3 +2123,224 @@ class TestPortfolioAdmin(TestCase): self.assertIn("request1.gov", domain_requests) self.assertIn("request2.gov", domain_requests) self.assertIn('
        ', domain_requests) + + +class TestTransferUser(WebTest): + """User transfer custom admin page""" + + # csrf checks do not work well with WebTest. + # We disable them here. + csrf_checks = False + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.site = AdminSite() + cls.superuser = create_superuser() + cls.admin = PortfolioAdmin(model=Portfolio, admin_site=cls.site) + cls.factory = RequestFactory() + + def setUp(self): + self.app.set_user(self.superuser) + self.user1, _ = User.objects.get_or_create( + username="madmax", first_name="Max", last_name="Rokatanski", title="Road warrior" + ) + self.user2, _ = User.objects.get_or_create( + username="furiosa", first_name="Furiosa", last_name="Jabassa", title="Imperator" + ) + + def tearDown(self): + Suborganization.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + Domain.objects.all().delete() + Portfolio.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + def test_transfer_user_shows_current_and_selected_user_information(self): + """Assert we pull the current user info and display it on the transfer page""" + completed_domain_request(user=self.user1, name="wasteland.gov") + domain_request = completed_domain_request( + user=self.user1, name="citadel.gov", status=DomainRequest.DomainRequestStatus.SUBMITTED + ) + domain_request.status = DomainRequest.DomainRequestStatus.APPROVED + domain_request.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + self.assertContains(user_transfer_page, "madmax") + self.assertContains(user_transfer_page, "Max") + self.assertContains(user_transfer_page, "Rokatanski") + self.assertContains(user_transfer_page, "Road warrior") + self.assertContains(user_transfer_page, "wasteland.gov") + self.assertContains(user_transfer_page, "citadel.gov") + + select_form = user_transfer_page.forms[0] + select_form["selected_user"] = str(self.user2.id) + preview_result = select_form.submit() + + self.assertContains(preview_result, "furiosa") + self.assertContains(preview_result, "Furiosa") + self.assertContains(preview_result, "Jabassa") + self.assertContains(preview_result, "Imperator") + + @less_console_noise_decorator + def test_transfer_user_transfers_portfolio_roles_and_permissions_and_portfolio_creator(self): + """Assert that a portfolio gets copied over + NOTE: Should be revised for #2644""" + portfolio = Portfolio.objects.create(organization_name="Citadel", creator=self.user2) + self.user2.portfolio = portfolio + self.user2.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + self.user2.portfolio_additional_permissions = [ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ] + self.user2.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + + self.user1.refresh_from_db() + portfolio.refresh_from_db() + + self.assertEquals(portfolio.creator, self.user1) + self.assertEquals(self.user1.portfolio, portfolio) + self.assertEquals(self.user1.portfolio_roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + self.assertEquals( + self.user1.portfolio_additional_permissions, + [UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO], + ) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_request_creator_and_investigator(self): + """Assert that domain request fields get transferred""" + domain_request = completed_domain_request(user=self.user2, name="wasteland.gov", investigator=self.user2) + + self.assertEquals(domain_request.creator, self.user2) + self.assertEquals(domain_request.investigator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_request.refresh_from_db() + + self.assertEquals(domain_request.creator, self.user1) + self.assertEquals(domain_request.investigator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_information_creator(self): + """Assert that domain fields get transferred""" + domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user2) + + self.assertEquals(domain_information.creator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_information.refresh_from_db() + + self.assertEquals(domain_information.creator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_role(self): + """Assert that user domain role get transferred""" + domain_1, _ = Domain.objects.get_or_create(name="chrome.gov", state=Domain.State.READY) + domain_2, _ = Domain.objects.get_or_create(name="v8.gov", state=Domain.State.READY) + user_domain_role1, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_1, role=UserDomainRole.Roles.MANAGER + ) + user_domain_role2, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_2, role=UserDomainRole.Roles.MANAGER + ) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + user_domain_role1.refresh_from_db() + user_domain_role2.refresh_from_db() + + self.assertEquals(user_domain_role1.user, self.user1) + self.assertEquals(user_domain_role2.user, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_verified_by_staff_requestor(self): + """Assert that verified by staff creator gets transferred""" + vip, _ = VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + vip.refresh_from_db() + + self.assertEquals(vip.requestor, self.user1) + + @less_console_noise_decorator + def test_transfer_user_deletes_old_user(self): + """Assert that the slected user gets deleted""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + # Refresh user2 from the database and check if it still exists + with self.assertRaises(User.DoesNotExist): + self.user2.refresh_from_db() + + @less_console_noise_decorator + def test_transfer_user_throws_transfer_and_delete_success_messages(self): + """Test that success messages for data transfer and user deletion are displayed.""" + # Ensure the setup for VerifiedByStaff + VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + with patch("django.contrib.messages.success") as mock_success_message: + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + after_submit = submit_form.submit().follow() + + self.assertContains(after_submit, "

        Change user

        ") + + mock_success_message.assert_any_call( + ANY, + ( + "Data transferred successfully for the following objects: ['Changed requestor " + + 'from "Furiosa Jabassa " to "Max Rokatanski " on immortan.joe@citadel.com\']' + ), + ) + + mock_success_message.assert_any_call(ANY, f"Deleted {self.user2} {self.user2.username}") + + @less_console_noise_decorator + def test_transfer_user_throws_error_message(self): + """Test that an error message is thrown if the transfer fails.""" + with patch( + "registrar.views.TransferUserView.transfer_user_fields_and_log", side_effect=Exception("Simulated Error") + ): + with patch("django.contrib.messages.error") as mock_error: + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit().follow() + + # Assert that the error message was called with the correct argument + mock_error.assert_called_once_with(ANY, "An error occurred during the transfer: Simulated Error") + + @less_console_noise_decorator + def test_transfer_user_modal(self): + """Assert modal on page""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + self.assertContains(user_transfer_page, "But you know what you're doing, right?") diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py index f6e87dd07..2b830d958 100644 --- a/src/registrar/views/__init__.py +++ b/src/registrar/views/__init__.py @@ -19,3 +19,4 @@ from .user_profile import UserProfileView, FinishProfileSetupView from .health import * from .index import * from .portfolios import * +from .transfer_user import TransferUserView diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index 5e367f04d..cc901d5ab 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -15,57 +15,60 @@ from registrar.models.verified_by_staff import VerifiedByStaff logger = logging.getLogger(__name__) + class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" JOINS = [ - (DomainRequest, 'creator'), - (DomainInformation, 'creator'), - (Portfolio, 'creator'), - (DomainRequest, 'investigator'), - (UserDomainRole, 'user'), - (VerifiedByStaff, 'requestor'), + (DomainRequest, "creator"), + (DomainInformation, "creator"), + (Portfolio, "creator"), + (DomainRequest, "investigator"), + (UserDomainRole, "user"), + (VerifiedByStaff, "requestor"), ] - USER_FIELDS = ['portfolio', 'portfolio_roles', 'portfolio_additional_permissions'] + USER_FIELDS = ["portfolio", "portfolio_roles", "portfolio_additional_permissions"] def get(self, request, user_id): """current_user referes to the 'source' user where the button that redirects to this view was clicked. other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. - + This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" current_user = get_object_or_404(User, pk=user_id) - other_users = User.objects.exclude(pk=user_id).order_by('first_name', 'last_name') # Exclude the current user from the dropdown + other_users = User.objects.exclude(pk=user_id).order_by( + "first_name", "last_name" + ) # Exclude the current user from the dropdown # Get the default admin site context, needed for the sidenav admin_context = site.each_context(request) context = { - 'current_user': current_user, - 'other_users': other_users, - 'logged_in_user': request.user, + "current_user": current_user, + "other_users": other_users, + "logged_in_user": request.user, **admin_context, # Include the admin context - 'current_user_domains': self.get_domains(current_user), - 'current_user_domain_requests': self.get_domain_requests(current_user) + "current_user_domains": self.get_domains(current_user), + "current_user_domain_requests": self.get_domain_requests(current_user), } - selected_user_id = request.GET.get('selected_user') + selected_user_id = request.GET.get("selected_user") if selected_user_id: selected_user = get_object_or_404(User, pk=selected_user_id) - context['selected_user'] = selected_user - context['selected_user_domains'] = self.get_domains(selected_user) - context['selected_user_domain_requests'] = self.get_domain_requests(selected_user) + context["selected_user"] = selected_user + context["selected_user_domains"] = self.get_domains(selected_user) + context["selected_user_domain_requests"] = self.get_domain_requests(selected_user) - return render(request, 'admin/transfer_user.html', context) + return render(request, "admin/transfer_user.html", context) def post(self, request, user_id): """This handles the transfer from selected_user to current_user then deletes selected_user. - + NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" current_user = get_object_or_404(User, pk=user_id) - selected_user_id = request.POST.get('selected_user') + selected_user_id = request.POST.get("selected_user") selected_user = get_object_or_404(User, pk=selected_user_id) try: @@ -89,53 +92,52 @@ class TransferUserView(View): except Exception as e: messages.error(request, f"An error occurred during the transfer: {e}") - return redirect('admin:registrar_user_change', object_id=user_id) + return redirect("admin:registrar_user_change", object_id=user_id) @classmethod - def update_joins_and_log(cls, model_class, field_name, old_user, new_user, change_logs): + def update_joins_and_log(cls, model_class, field_name, selected_user, current_user, change_logs): """ Helper function to update the user join fields for a given model and log the changes. """ - filter_kwargs = {field_name: old_user} + filter_kwargs = {field_name: selected_user} updated_objects = model_class.objects.filter(**filter_kwargs) for obj in updated_objects: # Check for duplicate UserDomainRole before updating if model_class == UserDomainRole: - if model_class.objects.filter(user=new_user, domain=obj.domain).exists(): + if model_class.objects.filter(user=current_user, domain=obj.domain).exists(): continue # Skip the update to avoid a duplicate # Update the field on the object and save it - setattr(obj, field_name, new_user) + setattr(obj, field_name, current_user) obj.save() # Log the change - cls.log_change(obj, field_name, old_user, new_user, change_logs) + cls.log_change(obj, field_name, selected_user, current_user, change_logs) @classmethod - def transfer_user_fields_and_log(cls, old_user, new_user, change_logs): + def transfer_user_fields_and_log(cls, selected_user, current_user, change_logs): """ - Transfers portfolio fields from the old_user to the new_user. + Transfers portfolio fields from the selected_user to the current_user. Logs the changes for each transferred field. NOTE: This will be refactored in #2644 """ - for field in cls.USER_FIELDS: - old_value = getattr(old_user, field, None) + field_value = getattr(selected_user, field, None) - if old_value: - setattr(new_user, field, old_value) - cls.log_change(new_user, field, old_value, old_value, change_logs) + if field_value: + setattr(current_user, field, field_value) + cls.log_change(current_user, field, field_value, field_value, change_logs) - new_user.save() + current_user.save() @classmethod - def log_change(cls, obj, field_name, old_value, new_value, change_logs): + def log_change(cls, obj, field_name, field_value, new_value, change_logs): """Logs the change for a specific field on an object""" - log_entry = f'Changed {field_name} from "{old_value}" to "{new_value}" on {obj}' - + log_entry = f'Changed {field_name} from "{field_value}" to "{new_value}" on {obj}' + logger.info(log_entry) # Collect the related object for the success message @@ -149,10 +151,10 @@ class TransferUserView(View): domains = Domain.objects.filter(id__in=domain_ids) return domains - + @classmethod def get_domain_requests(cls, user): """A simplified version of domain_requests_json""" domain_requests = DomainRequest.objects.filter(creator=user) - return domain_requests \ No newline at end of file + return domain_requests From 778ad4de6f331ef919d42c30ac1c38a740353f99 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:39:36 -0400 Subject: [PATCH 076/329] accessibility fix --- src/registrar/assets/sass/_theme/_admin.scss | 5 ++++- src/registrar/templates/admin/transfer_user.html | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 40aef1121..b948e2934 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -883,4 +883,7 @@ ul.add-list-reset { .submit-row .select2, .submit-row .select2 span { margin-top: 0; -} \ No newline at end of file +} +.transfer-user-selector .select2-selection__placeholder { + color: #3d4551!important; +} diff --git a/src/registrar/templates/admin/transfer_user.html b/src/registrar/templates/admin/transfer_user.html index a73106979..eee1d1140 100644 --- a/src/registrar/templates/admin/transfer_user.html +++ b/src/registrar/templates/admin/transfer_user.html @@ -38,7 +38,7 @@
        -
        +
        From 8ed76a747514af8ce80fb68d20ab405a6838b80c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 22:46:54 -0400 Subject: [PATCH 079/329] A bit of cleanup --- .../templates/django/admin/includes/detail_table_fieldset.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 7819e3aff..8cfb31064 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -232,7 +232,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
      -
From 69a3db5f59af1a8eb83b40643d6b4ff3e985babb Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 22:54:47 -0400 Subject: [PATCH 080/329] accessibility fix --- .../templates/django/admin/includes/detail_table_fieldset.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 8cfb31064..3b4047d39 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -232,7 +232,8 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
-
From 4e0416e514da83748418851d05e64635b712cb9d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:13:14 -0400 Subject: [PATCH 081/329] lint --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e13353fe6..a5c7a16d3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2975,7 +2975,7 @@ class PortfolioAdmin(ListHeaderAdmin): Will be used in the after_help_text block.""" members = [user for user in obj.user.all() if "Admin" not in user.portfolio_role_summary] if not members: - return '' + return "" member_details = ( "" From 875dc1d467ce94d1b49c2629d9f890bcd74cc79f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:31:23 -0400 Subject: [PATCH 082/329] css fixes --- src/registrar/assets/sass/_theme/_admin.scss | 25 +++++++++---------- src/registrar/templates/admin/analytics.html | 18 ++++++------- .../templates/admin/transfer_user.html | 6 ++--- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index b948e2934..e6d3cf6fc 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -369,7 +369,7 @@ input.admin-confirm-button { // This block resolves some of the issues we're seeing on buttons due to css // conflicts between DJ and USWDS a.button, -.usa-button { +.usa-button--dja { display: inline-block; padding: 10px 15px; font-size: 14px; @@ -380,17 +380,17 @@ a.button, } .button svg, .button span, -.usa-button svg, -.usa-button span { +.usa-button--dja svg, +.usa-button--dja span { vertical-align: middle; } -.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary) { +.usa-button--dja:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary) { background: var(--button-bg); } -.usa-button span { +.usa-button--dja span { font-size: 14px; } -.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary):hover { +.usa-button--dja:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary):hover { background: var(--button-hover-bg); } a.button:active, a.button:focus { @@ -399,6 +399,12 @@ a.button:active, a.button:focus { .usa-modal { font-family: inherit; } +// Targets the DJA buttom with a nested icon +button .usa-icon, +.button .usa-icon, +.button--clipboard .usa-icon { + vertical-align: middle; +} .module--custom { a { @@ -491,13 +497,6 @@ address.dja-address-contact-list { color: var(--link-fg); } -// Targets the DJA buttom with a nested icon -button .usa-icon, -.button .usa-icon, -.button--clipboard .usa-icon { - vertical-align: middle; -} - .errors span.select2-selection { border: 1px solid var(--error-fg) !important; } diff --git a/src/registrar/templates/admin/analytics.html b/src/registrar/templates/admin/analytics.html index 2729d883d..7c1a09c78 100644 --- a/src/registrar/templates/admin/analytics.html +++ b/src/registrar/templates/admin/analytics.html @@ -29,28 +29,28 @@
  • -
  • -
  • -
  • -
  • -
    • {% endif %} From 7ba93d05a2bd7a3b76d571d96c2383b8a0238c4e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:50:40 -0400 Subject: [PATCH 083/329] add sample kwarg for url test --- src/registrar/tests/test_url_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 3a045498a..284ec7638 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -25,6 +25,7 @@ SAMPLE_KWARGS = { "domain": "whitehouse.gov", "user_pk": "1", "portfolio_id": "1", + "user_id": "1", } # Our test suite will ignore some namespaces. From ec6e5873a4ace4b5e6da09b6f848d13911bf37b8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:56:55 -0400 Subject: [PATCH 084/329] zap exclusion --- src/zap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zap.conf b/src/zap.conf index c97897aeb..c25406796 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -72,6 +72,7 @@ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ +10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 04157ce404e8087608fdf509e51176b38abca980 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Sat, 24 Aug 2024 00:02:52 -0400 Subject: [PATCH 085/329] zap exclusion --- src/zap.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index c25406796..dd9ae1565 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -72,7 +72,7 @@ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ -10038 OUTOFSCOPE http://app:8080/transfer/ +10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 6091ff76c3ad71aaaa130842d5cdbdf0c24dd21c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 11:42:10 -0400 Subject: [PATCH 086/329] cleanup --- src/registrar/assets/js/get-gov-admin.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 11e3cae69..98a2395db 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,8 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + // Determine if any changes are necessary to the display of portfolio type or federal type + // based on changes to the Federal Agency let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) .then(response => { @@ -851,8 +853,6 @@ function initializeWidgetOnList(list, parentId) { let federal_type = data.federal_type; let portfolio_type = data.portfolio_type; - console.log("portfolio type: " + portfolio_type); - console.log("federal type: " + federal_type); updateFederalType(data.federal_type); updatePortfolioType(data.portfolio_type); }) @@ -912,22 +912,25 @@ function initializeWidgetOnList(list, parentId) { } } + /** + * Dynamically update the portfolio type text in the dom to portfolioType + */ function updatePortfolioType(portfolioType) { - console.log("attempting to update portfolioType"); // Find the div with class 'field-portfolio_type' const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); if (portfolioTypeDiv) { - console.log("found portfoliotype"); // Find the nested div with class 'readonly' inside 'field-portfolio_type' const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); if (readonlyDiv) { - console.log("found readonly div"); // Update the text content of the readonly div readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; } } } + /** + * Dynamically update the federal type text in the dom to federalType + */ function updateFederalType(federalType) { // Find the div with class 'field-federal_type' const federalTypeDiv = document.querySelector('.field-federal_type'); From 497837b09882731c121adbba88110d6155253e66 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 26 Aug 2024 11:18:43 -0500 Subject: [PATCH 087/329] Update src/registrar/management/commands/populate_domain_request_dates.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- .../management/commands/populate_domain_request_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 19d8e4f00..fe471fc9d 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -34,7 +34,7 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) def should_skip_record(self, record) -> bool: From f7b2e38bba21c91cc46b39da26464800cdf72ac9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Mon, 26 Aug 2024 11:54:08 -0500 Subject: [PATCH 088/329] linter fixes --- .../commands/populate_domain_request_dates.py | 11 +++++++---- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/test_views_requests_json.py | 6 ++++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index fe471fc9d..e20e35909 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -3,7 +3,6 @@ from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import DomainRequest from auditlog.models import LogEntry -from django.core.exceptions import ObjectDoesNotExist logger = logging.getLogger(__name__) @@ -12,12 +11,13 @@ class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" + """Loops through each DomainRequest object and populates + its last_status_update and first_submitted_date values""" self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - + # Retrieve and order audit log entries by timestamp in descending order audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") @@ -34,7 +34,10 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"""{TerminalColors.OKCYAN}Updating {record} => + first submitted date: {record.first_submitted_date}, + last status update: {record.last_status_update}{TerminalColors.ENDC} + """ ) def should_skip_record(self, record) -> bool: diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 09f200793..7ee80e43a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -823,7 +823,7 @@ class DomainRequest(TimeStampedModel): if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - # if the domain has not been submitted before this must be the first time + # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index c140b7e44..fd44bdc31 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,7 +287,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -306,7 +307,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json From f1c8f52456f43c951a26271793997ef322484e7e Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 26 Aug 2024 11:23:02 -0700 Subject: [PATCH 089/329] Remove refs to submitter in domain request --- src/registrar/models/domain_request.py | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 966c880d7..acd41053b 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -339,13 +339,12 @@ class DomainRequest(TimeStampedModel): help_text="The suborganization that this domain request is included under", ) - # This is the domain request user who created this domain request. The contact - # information that they gave is in the `submitter` field + # This is the domain request user who created this domain request. creator = models.ForeignKey( "registrar.User", on_delete=models.PROTECT, related_name="domain_requests_created", - help_text="Person who submitted the domain request; will not receive email updates", + help_text="Person who submitted the domain request. Will receive email updates.", ) investigator = models.ForeignKey( @@ -483,17 +482,6 @@ class DomainRequest(TimeStampedModel): help_text="Other domain names the creator provided for consideration", ) - # This is the contact information provided by the domain requestor. The - # user who created the domain request is in the `creator` field. - submitter = models.ForeignKey( - "registrar.Contact", - null=True, - blank=True, - related_name="submitted_domain_requests", - on_delete=models.PROTECT, - help_text='Person listed under "your contact information" in the request form; will receive email updates', - ) - purpose = models.TextField( null=True, blank=True, @@ -715,9 +703,6 @@ class DomainRequest(TimeStampedModel): contact information. If there is not creator information, then do nothing. - If the waffle flag "profile_feature" is active, then this email will be sent to the - domain request creator rather than the submitter - Optional args: bcc_address: str -> the address to bcc to @@ -732,7 +717,7 @@ class DomainRequest(TimeStampedModel): custom_email_content: str -> Renders an email with the content of this string as its body text. """ - recipient = self.creator if flag_is_active(None, "profile_feature") else self.submitter + recipient = self.creator if recipient is None or recipient.email is None: logger.warning( f"Cannot send {new_status} email, no creator email address for domain request with pk: {self.pk}." @@ -1170,9 +1155,6 @@ class DomainRequest(TimeStampedModel): def _is_purpose_complete(self): return self.purpose is not None - def _is_submitter_complete(self): - return self.submitter is not None - def _has_other_contacts_and_filled(self): # Other Contacts Radio button is Yes and if all required fields are filled return ( @@ -1227,8 +1209,6 @@ class DomainRequest(TimeStampedModel): and self._is_senior_official_complete() and self._is_requested_domain_complete() and self._is_purpose_complete() - # NOTE: This flag leaves submitter as empty (request wont submit) hence set to True - and (self._is_submitter_complete() if not has_profile_feature_flag else True) and self._is_other_contacts_complete() and self._is_additional_details_complete() and self._is_policy_acknowledgement_complete() From 2f2b2fe10ffc238d91148cc25281c4355c53eb32 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 26 Aug 2024 11:46:15 -0700 Subject: [PATCH 090/329] Remove submitter from models --- src/registrar/admin.py | 29 ++++------------ src/registrar/fixtures_domain_requests.py | 7 ---- src/registrar/forms/domain_request_wizard.py | 8 ++--- ...ve_domaininformation_submitter_and_more.py | 33 +++++++++++++++++++ src/registrar/models/domain_information.py | 14 +------- .../admin/includes/detail_table_fieldset.html | 8 ++--- .../templates/domain_request_review.html | 4 +-- .../templates/domain_request_status.html | 4 +-- src/registrar/views/domain_request.py | 12 +++---- 9 files changed, 59 insertions(+), 60 deletions(-) create mode 100644 src/registrar/migrations/0119_remove_domaininformation_submitter_and_more.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..636a9fa5c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -505,7 +505,6 @@ class AdminSortFields: sort_mapping = { # == Contact == # "other_contacts": (Contact, _name_sort), - "submitter": (Contact, _name_sort), # == Senior Official == # "senior_official": (SeniorOfficial, _name_sort), # == User == # @@ -1390,13 +1389,9 @@ class DomainInformationAdmin(ListHeaderAdmin, ImportExportModelAdmin): "domain", "generic_org_type", "created_at", - "submitter", ] - orderable_fk_fields = [ - ("domain", "name"), - ("submitter", ["first_name", "last_name"]), - ] + orderable_fk_fields = [("domain", "name")] # Filters list_filter = ["generic_org_type"] @@ -1408,7 +1403,7 @@ class DomainInformationAdmin(ListHeaderAdmin, ImportExportModelAdmin): search_help_text = "Search by domain." fieldsets = [ - (None, {"fields": ["portfolio", "sub_organization", "creator", "submitter", "domain_request", "notes"]}), + (None, {"fields": ["portfolio", "sub_organization", "creator", "domain_request", "notes"]}), (".gov domain", {"fields": ["domain"]}), ("Contacts", {"fields": ["senior_official", "other_contacts", "no_other_contacts_rationale"]}), ("Background info", {"fields": ["anything_else"]}), @@ -1472,7 +1467,6 @@ class DomainInformationAdmin(ListHeaderAdmin, ImportExportModelAdmin): "more_organization_information", "domain", "domain_request", - "submitter", "no_other_contacts_rationale", "anything_else", "is_policy_acknowledged", @@ -1487,7 +1481,6 @@ class DomainInformationAdmin(ListHeaderAdmin, ImportExportModelAdmin): "domain_request", "senior_official", "domain", - "submitter", "portfolio", "sub_organization", ] @@ -1658,13 +1651,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "custom_election_board", "city", "state_territory", - "submitter", "investigator", ] orderable_fk_fields = [ ("requested_domain", "name"), - ("submitter", ["first_name", "last_name"]), ("investigator", ["first_name", "last_name"]), ] @@ -1694,11 +1685,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Search search_fields = [ "requested_domain__name", - "submitter__email", - "submitter__first_name", - "submitter__last_name", + "creator__email", + "creator__first_name", + "creator__last_name", ] - search_help_text = "Search by domain or submitter." + search_help_text = "Search by domain or creator." fieldsets = [ ( @@ -1714,7 +1705,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "action_needed_reason_email", "investigator", "creator", - "submitter", "approved_domain", "notes", ] @@ -1802,7 +1792,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "approved_domain", "alternative_domains", "purpose", - "submitter", "no_other_contacts_rationale", "anything_else", "is_policy_acknowledged", @@ -1813,7 +1802,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): autocomplete_fields = [ "approved_domain", "requested_domain", - "submitter", "creator", "senior_official", "investigator", @@ -2150,10 +2138,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): if not action_needed_reason or action_needed_reason == DomainRequest.ActionNeededReasons.OTHER: return None - if flag_is_active(None, "profile_feature"): # type: ignore - recipient = domain_request.creator - else: - recipient = domain_request.submitter + recipient = domain_request.creator # Return the context of the rendered views context = {"domain_request": domain_request, "recipient": recipient} diff --git a/src/registrar/fixtures_domain_requests.py b/src/registrar/fixtures_domain_requests.py index 50f611474..fad8f3f8a 100644 --- a/src/registrar/fixtures_domain_requests.py +++ b/src/registrar/fixtures_domain_requests.py @@ -37,7 +37,6 @@ class DomainRequestFixture: # "anything_else": None, # "is_policy_acknowledged": None, # "senior_official": None, - # "submitter": None, # "other_contacts": [], # "current_websites": [], # "alternative_domains": [], @@ -123,12 +122,6 @@ class DomainRequestFixture: else: da.senior_official = Contact.objects.create(**cls.fake_contact()) - if not da.submitter: - if "submitter" in app and app["submitter"] is not None: - da.submitter, _ = Contact.objects.get_or_create(**app["submitter"]) - else: - da.submitter = Contact.objects.create(**cls.fake_contact()) - if not da.requested_domain: if "requested_domain" in app and app["requested_domain"] is not None: da.requested_domain, _ = DraftDomain.objects.get_or_create(name=app["requested_domain"]) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index d97dd0de7..764436bb2 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -387,12 +387,12 @@ class PurposeForm(RegistrarForm): class YourContactForm(RegistrarForm): - JOIN = "submitter" + JOIN = "creator" def to_database(self, obj): if not self.is_valid(): return - contact = getattr(obj, "submitter", None) + contact = getattr(obj, "creator", None) if contact is not None and not contact.has_more_than_one_join("submitted_domain_requests"): # if contact exists in the database and is not joined to other entities super().to_database(contact) @@ -401,12 +401,12 @@ class YourContactForm(RegistrarForm): # in either case, create a new contact and update it contact = Contact() super().to_database(contact) - obj.submitter = contact + obj.creator = contact obj.save() @classmethod def from_database(cls, obj): - contact = getattr(obj, "submitter", None) + contact = getattr(obj, "creator", None) return super().from_database(contact) first_name = forms.CharField( diff --git a/src/registrar/migrations/0119_remove_domaininformation_submitter_and_more.py b/src/registrar/migrations/0119_remove_domaininformation_submitter_and_more.py new file mode 100644 index 000000000..d438d6744 --- /dev/null +++ b/src/registrar/migrations/0119_remove_domaininformation_submitter_and_more.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.10 on 2024-08-26 18:45 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), + ] + + operations = [ + migrations.RemoveField( + model_name="domaininformation", + name="submitter", + ), + migrations.RemoveField( + model_name="domainrequest", + name="submitter", + ), + migrations.AlterField( + model_name="domainrequest", + name="creator", + field=models.ForeignKey( + help_text="Person who submitted the domain request. Will receive email updates.", + on_delete=django.db.models.deletion.PROTECT, + related_name="domain_requests_created", + to=settings.AUTH_USER_MODEL, + ), + ), + ] diff --git a/src/registrar/models/domain_information.py b/src/registrar/models/domain_information.py index 774dba897..03b8cc047 100644 --- a/src/registrar/models/domain_information.py +++ b/src/registrar/models/domain_information.py @@ -48,8 +48,7 @@ class DomainInformation(TimeStampedModel): null=True, ) - # This is the domain request user who created this domain request. The contact - # information that they gave is in the `submitter` field + # This is the domain request user who created this domain request. creator = models.ForeignKey( "registrar.User", on_delete=models.PROTECT, @@ -197,17 +196,6 @@ class DomainInformation(TimeStampedModel): related_name="domain_info", ) - # This is the contact information provided by the domain requestor. The - # user who created the domain request is in the `creator` field. - submitter = models.ForeignKey( - "registrar.Contact", - null=True, - blank=True, - related_name="submitted_domain_requests_information", - on_delete=models.PROTECT, - help_text='Person listed under "your contact information" in the request form', - ) - purpose = models.TextField( null=True, blank=True, diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 683f33117..1c81e30d6 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -70,7 +70,7 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html)
NameTitleEmail