From f10cfe2c9bd143029af27270807619eab41be630 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:45:08 -0600 Subject: [PATCH 01/28] delete ds data --- src/registrar/models/domain.py | 15 +++++++++- src/registrar/tests/test_models_domain.py | 36 +++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 0f0b3f112..134da7ead 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1053,13 +1053,15 @@ class Domain(TimeStampedModel, DomainHelper): note=f"Host {host.name} is in use by {host.domain}", ) + # set hosts to empty list so nameservers are deleted ( deleted_values, updated_values, new_values, oldNameservers, ) = self.getNameserverChanges(hosts=[]) - + + # update the hosts _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors addToDomainList, _ = self.createNewHostList(new_values) deleteHostList, _ = self.createDeleteHostList(deleted_values) @@ -1073,6 +1075,7 @@ class Domain(TimeStampedModel, DomainHelper): # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) + # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) for contact in contacts: @@ -1081,6 +1084,16 @@ class Domain(TimeStampedModel, DomainHelper): request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) + # delete ds data if it exists + if self.dnssecdata: + logger.debug("Deleting ds data for %s", self.name) + try: + self.dnssecdata = None + except RegistryError as e: + logger.error("Error deleting ds data for %s: %s", self.name, e) + e.note = "Error deleting ds data for %s" % self.name + raise e + logger.info("Deleting domain %s", self.name) request = commands.DeleteDomain(name=self.name) registry.send(request, cleaned=True) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 083725a55..5bb783fd0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2863,6 +2863,42 @@ class TestAnalystDelete(MockEppLib): # State should have changed self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) + def test_analyst_deletes_domain_with_ds_data(self): + """ + Scenario: Domain with DS data is deleted + When `domain.deletedInEpp()` is called + Then `commands.DeleteDomain` is sent to the registry + And `state` is set to `DELETED` + """ + # Create a domain with DS data + domain, _ = Domain.objects.get_or_create(name="dsdomain.gov", state=Domain.State.READY) + domain.dnssecdata = extensions.DNSSECExtension( + dsdata=[extensions.DSData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + keydata=[extensions.DNSSECKeyData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + ) + domain.save() + + # Delete the domain + domain.deletedInEpp() + domain.save() + + # Check that dsdata is None + self.assertEqual(domain.dnssecdata, None) + + # Check that the UpdateDomain command was sent to the registry with the correct extension + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.UpdateDomain(name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), + cleaned=True, + ), + ] + ) + + # Check that the domain was deleted + self.assertEqual(domain.state, Domain.State.DELETED) + + @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): """ From 59c22643316dd87b7ed7910eb62de07962f8e6fe Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:55:19 -0600 Subject: [PATCH 02/28] remove raise error on delete_hosts_if_not_used --- src/registrar/models/domain.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 134da7ead..44eb1cde0 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -750,11 +750,7 @@ class Domain(TimeStampedModel, DomainHelper): successTotalNameservers = len(oldNameservers) - deleteCount + addToDomainCount - try: - self._delete_hosts_if_not_used(hostsToDelete=deleted_values) - except Exception as e: - # we don't need this part to succeed in order to continue. - logger.error("Failed to delete nameserver hosts: %s", e) + self._delete_hosts_if_not_used(hostsToDelete=deleted_values) if successTotalNameservers < 2: try: @@ -1849,8 +1845,6 @@ class Domain(TimeStampedModel, DomainHelper): else: logger.error("Error _delete_hosts_if_not_used, code was %s error was %s" % (e.code, e)) - raise e - def _fix_unknown_state(self, cleaned): """ _fix_unknown_state: Calls _add_missing_contacts_if_unknown From 041ca70a8b9fa0dc8c1d924a1940278c39f6d005 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 08:04:32 -0500 Subject: [PATCH 03/28] portfolio removed email and associated tests --- .../templates/emails/portfolio_removal.txt | 24 ++++++ .../emails/portfolio_removal_subject.txt | 1 + src/registrar/tests/test_email_invitations.py | 74 +++++++++++++++++++ src/registrar/tests/test_views_portfolio.py | 58 +++++++++++++-- src/registrar/utility/email_invitations.py | 41 ++++++++++ src/registrar/views/portfolios.py | 18 +++-- 6 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 src/registrar/templates/emails/portfolio_removal.txt create mode 100644 src/registrar/templates/emails/portfolio_removal_subject.txt diff --git a/src/registrar/templates/emails/portfolio_removal.txt b/src/registrar/templates/emails/portfolio_removal.txt new file mode 100644 index 000000000..6de2190ae --- /dev/null +++ b/src/registrar/templates/emails/portfolio_removal.txt @@ -0,0 +1,24 @@ +{% autoescape off %}{# In a text file, we don't want to have HTML entities escaped #} +Hi,{% if requested_user and requested_user.first_name %} {{ requested_user.first_name }}.{% endif %} + +{{ requestor_email }} has removed you from {{ portfolio.organization_name }}. + +You can no longer view this organization or its related domains within the .gov registrar. + + +SOMETHING WRONG? +If you have questions or concerns, reach out to the person who removed you from the +organization, or reply to this email. + + +THANK YOU +.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. + +---------------------------------------------------------------- + +The .gov team +Contact us: +Learn about .gov + +The .gov registry is a part of the Cybersecurity and Infrastructure Security Agency (CISA) +{% endautoescape %} diff --git a/src/registrar/templates/emails/portfolio_removal_subject.txt b/src/registrar/templates/emails/portfolio_removal_subject.txt new file mode 100644 index 000000000..d60ef9859 --- /dev/null +++ b/src/registrar/templates/emails/portfolio_removal_subject.txt @@ -0,0 +1 @@ +You've been removed from a .gov organization \ No newline at end of file diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 20ac4a565..f07e2f2a7 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -16,6 +16,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) @@ -962,3 +963,76 @@ class TestSendPortfolioMemberPermissionUpdateEmail(unittest.TestCase): # Assertions mock_logger.warning.assert_not_called() # Function should fail before logging email failure + + +class TestSendPortfolioMemberPermissionRemoveEmail(unittest.TestCase): + """Unit tests for send_portfolio_member_permission_remove_email function.""" + + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations._get_requestor_email") + def test_send_email_success(self, mock_get_requestor_email, mock_send_email): + """Test that the email is sent successfully when there are no errors.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_get_requestor_email.assert_called_once_with(requestor, portfolio=permissions.portfolio) + mock_send_email.assert_called_once_with( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address="user@example.com", + context={ + "requested_user": permissions.user, + "portfolio": permissions.portfolio, + "requestor_email": "requestor@example.com", + }, + ) + self.assertTrue(result) + + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError("Email failed")) + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations.logger") + def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_send_email): + """Test that the function returns False and logs an error when email sending fails.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + permissions.user.email = "user@example.com" + permissions.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_called_once_with( + "Could not send email organization member removal notification to %s for portfolio: %s", + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) + self.assertFalse(result) + + @patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error")) + @patch("registrar.utility.email_invitations.logger") + def test_requestor_email_retrieval_failure(self, mock_logger, mock_get_requestor_email): + """Test that an exception in retrieving requestor email is logged.""" + # Mock data + requestor = MagicMock() + permissions = MagicMock(spec=UserPortfolioPermission) + + # Call function + with self.assertRaises(Exception): + send_portfolio_member_permission_remove_email(requestor, permissions) + + # Assertions + mock_logger.warning.assert_not_called() # Function should fail before logging email failure diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 65e0350ee..329e8e9f1 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1669,7 +1669,8 @@ class TestPortfolioMemberDeleteView(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_members_table_active_requests(self, send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_delete_view_members_table_active_requests(self, send_member_removal, send_removal_emails): """Error state w/ deleting a member with active request on Members Table""" # I'm a user UserPortfolioPermission.objects.get_or_create( @@ -1709,12 +1710,15 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is not called + send_member_removal.assert_not_called() @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_members_table_only_admin(self, send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_delete_view_members_table_only_admin(self, send_member_removal, send_removal_emails): """Error state w/ deleting a member that's the only admin on Members Table""" # I'm a user with admin permission @@ -1744,12 +1748,15 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is not called + send_member_removal.assert_not_called() @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_member_success(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_member_success(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND not only admin""" # I'm a user @@ -1774,6 +1781,9 @@ class TestPortfolioMemberDeleteView(WebTest): roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], ) + # Member removal email sent successfully + send_member_removal.return_value = True + # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( User, "is_only_admin_of_portfolio", return_value=False @@ -1796,12 +1806,23 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is not called # because member being removed is not an admin mock_send_removal_emails.assert_not_called() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() + + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_admin_success(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_admin_success(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND not only admin. Because admin, removal emails are sent.""" @@ -1828,6 +1849,7 @@ class TestPortfolioMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = True + send_member_removal.return_value = True # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( @@ -1850,6 +1872,8 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is called mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -1859,13 +1883,23 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") + def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, send_member_removal, mock_send_removal_emails): """Success state with deleting on Members Table page bc no active request AND - not only admin. Because admin, removal emails are sent, but fail to send.""" + not only admin. Because admin, removal emails are sent, but fail to send. + Email to removed member also fails to send.""" # I'm a user UserPortfolioPermission.objects.get_or_create( @@ -1890,6 +1924,7 @@ class TestPortfolioMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = False + send_member_removal.return_value = False # And set that the member has no active requests AND it's not the only admin with patch.object(User, "get_active_requests_count_in_portfolio", return_value=0), patch.object( @@ -1912,6 +1947,8 @@ class TestPortfolioMemberDeleteView(WebTest): # assert that send_portfolio_admin_removal_emails is called mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_member_permission_remove_email is called + send_member_removal.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -1920,6 +1957,15 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["email"], member_email) self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + + # Get the arguments passed to send_portfolio_member_permission_remove_email + _, called_kwargs = send_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["permissions"].user, upp.user) + self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) + @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index d206bf279..2ddf74cc0 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -268,6 +268,47 @@ def send_portfolio_member_permission_update_email(requestor, permissions: UserPo return True +def send_portfolio_member_permission_remove_email(requestor, permissions: UserPortfolioPermission): + """ + Sends an email notification to a portfolio member when their permissions are deleted. + + This function retrieves the requestor's email and sends a templated email to the affected user, + notifying them of the removal of their portfolio permissions. + + Args: + requestor (User): The user initiating the permission update. + permissions (UserPortfolioPermission): The updated permissions object containing the affected user + and the portfolio details. + + Returns: + bool: True if the email was sent successfully, False if an EmailSendingError occurred. + + Raises: + MissingEmailError: If the requestor has no email associated with their account. + """ + requestor_email = _get_requestor_email(requestor, portfolio=permissions.portfolio) + try: + send_templated_email( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address=permissions.user.email, + context={ + "requested_user": permissions.user, + "portfolio": permissions.portfolio, + "requestor_email": requestor_email, + }, + ) + except EmailSendingError: + logger.warning( + "Could not send email organization member removal notification to %s " "for portfolio: %s", + permissions.user.email, + permissions.portfolio.organization_name, + exc_info=True, + ) + return False + return True + + def send_portfolio_admin_addition_emails(email: str, requestor, portfolio: Portfolio): """ Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d63b5964e..3a1124898 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -20,6 +20,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) from registrar.utility.errors import MissingEmailError @@ -149,18 +150,23 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): messages.error(request, error_message) return redirect(reverse("member", kwargs={"pk": pk})) - # if member being removed is an admin - if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: - try: + try: + # if member being removed is an admin + if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: # attempt to send notification emails of the removal to other portfolio admins if not send_portfolio_admin_removal_emails( email=portfolio_member_permission.user.email, requestor=request.user, portfolio=portfolio_member_permission.portfolio, ): - messages.warning(self.request, "Could not send email notification to existing organization admins.") - except Exception as e: - self._handle_exceptions(e) + messages.warning(request, "Could not send email notification to existing organization admins.") + # send notification email to member being removed + if not send_portfolio_member_permission_remove_email( + requestor=request.user, permissions=portfolio_member_permission + ): + messages.warning(request, f"Could not send email notification to {member.email}") + except Exception as e: + self._handle_exceptions(e) # passed all error conditions portfolio_member_permission.delete() From a7f08aa5868005d27bfb688694b3a40d8656a4ba Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 6 Feb 2025 11:09:23 -0600 Subject: [PATCH 04/28] fix test --- src/registrar/tests/test_models_domain.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 5bb783fd0..973a5ad39 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2863,6 +2863,7 @@ class TestAnalystDelete(MockEppLib): # State should have changed self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) + # @less_console_noise def test_analyst_deletes_domain_with_ds_data(self): """ Scenario: Domain with DS data is deleted @@ -2872,9 +2873,10 @@ class TestAnalystDelete(MockEppLib): """ # Create a domain with DS data domain, _ = Domain.objects.get_or_create(name="dsdomain.gov", state=Domain.State.READY) + # set domain to be on hold + domain.place_client_hold() domain.dnssecdata = extensions.DNSSECExtension( - dsdata=[extensions.DSData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], - keydata=[extensions.DNSSECKeyData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + dsData=[extensions.DSData(keyTag=1, alg=1, digestType=1, digest="1234567890")], ) domain.save() @@ -2885,6 +2887,11 @@ class TestAnalystDelete(MockEppLib): # Check that dsdata is None self.assertEqual(domain.dnssecdata, None) + # Print out all calls from the mockedSendFunction + print("\nAll calls to mockedSendFunction:") + for call in self.mockedSendFunction.call_args_list: + print(f"- {call}") + # Check that the UpdateDomain command was sent to the registry with the correct extension self.mockedSendFunction.assert_has_calls( [ From e3e001fe0d0efd9d09abef7ee20a6492879775e6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 13:25:54 -0500 Subject: [PATCH 05/28] add static messages to admin confirmation messages --- src/registrar/admin.py | 2 ++ ...rtfolio_invitation_delete_confirmation.html | 17 +++++++++++++++++ ...rtfolio_permission_delete_confirmation.html | 12 ++++++++++++ src/registrar/tests/test_admin.py | 18 ++++++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html create mode 100644 src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 928ead442..d27a4849c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1326,6 +1326,7 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): search_help_text = "Search by first name, last name, email, or portfolio." change_form_template = "django/admin/user_portfolio_permission_change_form.html" + delete_confirmation_template = "django/admin/user_portfolio_permission_delete_confirmation.html" def get_roles(self, obj): readable_roles = obj.get_readable_roles() @@ -1631,6 +1632,7 @@ class PortfolioInvitationAdmin(BaseInvitationAdmin): autocomplete_fields = ["portfolio"] change_form_template = "django/admin/portfolio_invitation_change_form.html" + delete_confirmation_template = "django/admin/portfolio_invitation_delete_confirmation.html" # Select portfolio invitations to change -> Portfolio invitations def changelist_view(self, request, extra_context=None): diff --git a/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html new file mode 100644 index 000000000..5aa01c33f --- /dev/null +++ b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html @@ -0,0 +1,17 @@ +{% extends "admin/delete_confirmation.html" %} + +{% block content_subtitle %} +
+
+

+ If you cancel the portfolio invitation here, it won't trigger any emails. It also won't remove the user's + portfolio access if they already logged in. Go to the + + User Portfolio Permissions + + table if you want to remove the user from a portfolio. +

+
+
+ {{ block.super }} +{% endblock %} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html b/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html new file mode 100644 index 000000000..71c789a63 --- /dev/null +++ b/src/registrar/templates/django/admin/user_portfolio_permission_delete_confirmation.html @@ -0,0 +1,12 @@ +{% extends "admin/delete_confirmation.html" %} + +{% block content_subtitle %} +
+
+

+ If you remove someone from a portfolio here, it will not send any emails when you click "Save". +

+
+
+ {{ block.super }} +{% endblock %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 28a407036..e237f9509 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1549,6 +1549,24 @@ class TestPortfolioInvitationAdmin(TestCase): request, "Could not send email notification to existing organization admins." ) + @less_console_noise_decorator + def test_delete_confirmation_page_contains_static_message(self): + """Ensure the custom message appears in the delete confirmation page.""" + self.client.force_login(self.superuser) + # Create a test portfolio invitation + self.invitation = PortfolioInvitation.objects.create( + email="testuser@example.com", + portfolio=self.portfolio, + roles=["organization_member"] + ) + delete_url = reverse( + "admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk] + ) + response = self.client.get(delete_url) + + # Check if the response contains the expected static message + expected_message = "If you cancel the portfolio invitation here" + self.assertIn(expected_message, response.content.decode("utf-8")) class TestHostAdmin(TestCase): """Tests for the HostAdmin class as super user From 6d0b9d1a9e2f995c37fce5b0f27245534def08dd Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 13:39:04 -0500 Subject: [PATCH 06/28] last test for messages in dja --- src/registrar/tests/test_admin.py | 26 ++++++++++++++++----- src/registrar/tests/test_views_portfolio.py | 7 +++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index e237f9509..8e04899cd 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -55,6 +55,7 @@ from .common import ( MockDbForSharedTests, AuditedAdminMockData, completed_domain_request, + create_test_user, generic_domain_object, less_console_noise, mock_user, @@ -1079,6 +1080,7 @@ class TestUserPortfolioPermissionAdmin(TestCase): """Create a client object""" self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() + self.testuser = create_test_user() self.portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser) def tearDown(self): @@ -1111,6 +1113,21 @@ class TestUserPortfolioPermissionAdmin(TestCase): "If you add someone to a portfolio here, it will not trigger an invitation email.", ) + @less_console_noise_decorator + def test_delete_confirmation_page_contains_static_message(self): + """Ensure the custom message appears in the delete confirmation page.""" + self.client.force_login(self.superuser) + # Create a test portfolio permission + self.permission = UserPortfolioPermission.objects.create( + user=self.testuser, portfolio=self.portfolio, roles=["organization_member"] + ) + delete_url = reverse("admin:registrar_userportfoliopermission_delete", args=[self.permission.pk]) + response = self.client.get(delete_url) + + # Check if the response contains the expected static message + expected_message = "If you remove someone from a portfolio here, it will not send any emails" + self.assertIn(expected_message, response.content.decode("utf-8")) + class TestPortfolioInvitationAdmin(TestCase): """Tests for the PortfolioInvitationAdmin class as super user @@ -1555,19 +1572,16 @@ class TestPortfolioInvitationAdmin(TestCase): self.client.force_login(self.superuser) # Create a test portfolio invitation self.invitation = PortfolioInvitation.objects.create( - email="testuser@example.com", - portfolio=self.portfolio, - roles=["organization_member"] - ) - delete_url = reverse( - "admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk] + email="testuser@example.com", portfolio=self.portfolio, roles=["organization_member"] ) + delete_url = reverse("admin:registrar_portfolioinvitation_delete", args=[self.invitation.pk]) response = self.client.get(delete_url) # Check if the response contains the expected static message expected_message = "If you cancel the portfolio invitation here" self.assertIn(expected_message, response.content.decode("utf-8")) + class TestHostAdmin(TestCase): """Tests for the HostAdmin class as super user diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 329e8e9f1..76290ff3c 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1896,7 +1896,9 @@ class TestPortfolioMemberDeleteView(WebTest): @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") @patch("registrar.views.portfolios.send_portfolio_member_permission_remove_email") - def test_portfolio_member_table_delete_admin_success_removal_email_fail(self, send_member_removal, mock_send_removal_emails): + def test_portfolio_member_table_delete_admin_success_removal_email_fail( + self, send_member_removal, mock_send_removal_emails + ): """Success state with deleting on Members Table page bc no active request AND not only admin. Because admin, removal emails are sent, but fail to send. Email to removed member also fails to send.""" @@ -1957,7 +1959,7 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["email"], member_email) self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) - + # Get the arguments passed to send_portfolio_member_permission_remove_email _, called_kwargs = send_member_removal.call_args @@ -1966,7 +1968,6 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(called_kwargs["permissions"].user, upp.user) self.assertEqual(called_kwargs["permissions"].portfolio, upp.portfolio) - @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) From 9dd357c81f90bd7c0d16cd86ac22c87c841a2f2a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 15:02:59 -0500 Subject: [PATCH 07/28] refactored method to make more readable --- src/registrar/views/portfolios.py | 77 +++++++++++++++++++------------ 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 3a1124898..f85e8ebea 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -119,65 +119,82 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): """ portfolio_member_permission = get_object_or_404(UserPortfolioPermission, pk=pk) member = portfolio_member_permission.user + portfolio = portfolio_member_permission.portfolio + # Validate if the member can be removed + error_message = self._validate_member_removal(request, member, portfolio) + if error_message: + return self._handle_error_response(request, error_message, pk) + + # Attempt to send notification emails + self._send_removal_notifications(request, portfolio_member_permission) + + # Passed all error conditions, proceed with deletion + portfolio_member_permission.delete() + + # Return success response + return self._handle_success_response(request, member.email) + + def _validate_member_removal(self, request, member, portfolio): + """ + Check whether the member can be removed from the portfolio. + Returns an error message if removal is not allowed; otherwise, returns None. + """ active_requests_count = member.get_active_requests_count_in_portfolio(request) - support_url = "https://get.gov/contact/" - error_message = "" - if active_requests_count > 0: - # If they have any in progress requests - error_message = mark_safe( # nosec + return mark_safe( # nosec "This member can't be removed from the organization because they have an active domain request. " f"Please contact us to remove this member." ) - elif member.is_only_admin_of_portfolio(portfolio_member_permission.portfolio): - # If they are the last manager of a domain - error_message = ( + if member.is_only_admin_of_portfolio(portfolio): + return ( "There must be at least one admin in your organization. Give another member admin " "permissions, make sure they log into the registrar, and then remove this member." ) + return None - # From the Members Table page Else the Member Page - if error_message: - if request.headers.get("X-Requested-With") == "XMLHttpRequest": - return JsonResponse( - {"error": error_message}, - status=400, - ) - else: - messages.error(request, error_message) - return redirect(reverse("member", kwargs={"pk": pk})) + def _handle_error_response(self, request, error_message, pk): + """ + Return an error response (JSON or redirect with messages). + """ + if request.headers.get("X-Requested-With") == "XMLHttpRequest": + return JsonResponse({"error": error_message}, status=400) + messages.error(request, error_message) + return redirect(reverse("member", kwargs={"pk": pk})) + def _send_removal_notifications(self, request, portfolio_member_permission): + """ + Attempt to send notification emails about the member's removal. + """ try: - # if member being removed is an admin + # Notify other portfolio admins if removing an admin if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_member_permission.roles: - # attempt to send notification emails of the removal to other portfolio admins if not send_portfolio_admin_removal_emails( email=portfolio_member_permission.user.email, requestor=request.user, portfolio=portfolio_member_permission.portfolio, ): messages.warning(request, "Could not send email notification to existing organization admins.") - # send notification email to member being removed + + # Notify the member being removed if not send_portfolio_member_permission_remove_email( requestor=request.user, permissions=portfolio_member_permission ): - messages.warning(request, f"Could not send email notification to {member.email}") + messages.warning(request, f"Could not send email notification to {portfolio_member_permission.user.email}") except Exception as e: self._handle_exceptions(e) - # passed all error conditions - portfolio_member_permission.delete() - - # From the Members Table page Else the Member Page - success_message = f"You've removed {member.email} from the organization." + def _handle_success_response(self, request, member_email): + """ + Return a success response (JSON or redirect with messages). + """ + success_message = f"You've removed {member_email} from the organization." if request.headers.get("X-Requested-With") == "XMLHttpRequest": return JsonResponse({"success": success_message}, status=200) - else: - messages.success(request, success_message) - return redirect(reverse("members")) + messages.success(request, success_message) + return redirect(reverse("members")) def _handle_exceptions(self, exception): """Handle exceptions raised during the process.""" From 9c1e3bbc9419fe618c0100a03aebdbae49074e7a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 6 Feb 2025 17:36:04 -0500 Subject: [PATCH 08/28] lint --- src/registrar/views/portfolios.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index c0a0a442f..a52a757ec 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -182,7 +182,9 @@ class PortfolioMemberDeleteView(PortfolioMemberPermission, View): if not send_portfolio_member_permission_remove_email( requestor=request.user, permissions=portfolio_member_permission ): - messages.warning(request, f"Could not send email notification to {portfolio_member_permission.user.email}") + messages.warning( + request, f"Could not send email notification to {portfolio_member_permission.user.email}" + ) except Exception as e: self._handle_exceptions(e) From dea13ec27b58406a812db3464942a96523e461e9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 10:37:52 -0600 Subject: [PATCH 09/28] bug fixes for deletion --- src/registrar/models/domain.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 44eb1cde0..032cac8b4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1074,17 +1074,24 @@ class Domain(TimeStampedModel, DomainHelper): # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) + for contact in contacts: - if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: - self._update_domain_with_contact(contact, rem=True) - request = commands.DeleteContact(contact.registry_id) - registry.send(request, cleaned=True) + try: + if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: + self._update_domain_with_contact(contact, rem=True) + request = commands.DeleteContact(contact.registry_id) + registry.send(request, cleaned=True) + except RegistryError as e: + logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) # delete ds data if it exists if self.dnssecdata: logger.debug("Deleting ds data for %s", self.name) try: + # set and unset client hold to be able to change ds data + self._remove_client_hold() self.dnssecdata = None + self._place_client_hold() except RegistryError as e: logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name From d4a5e3e40081bd12d5b6d93e0d1f1147eb5483a1 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 10:45:21 -0600 Subject: [PATCH 10/28] fix deletion regex --- src/registrar/models/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 032cac8b4..4a3edcd4a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1040,7 +1040,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Deleting subdomains for %s", self.name) # check if any subdomains are in use by another domain - hosts = Host.objects.filter(name__regex=r".+{}".format(self.name)) + hosts = Host.objects.filter(name__regex=r".+\.{}".format(self.name)) for host in hosts: if host.domain != self: logger.error("Unable to delete host: %s is in use by another domain: %s", host.name, host.domain) From 826adc575355f26024a4954316c6e9af01800ef6 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:02:35 -0600 Subject: [PATCH 11/28] add more logging --- src/registrar/models/domain.py | 45 ++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4a3edcd4a..18a4442f4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1048,21 +1048,23 @@ class Domain(TimeStampedModel, DomainHelper): code=ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION, note=f"Host {host.name} is in use by {host.domain}", ) - - # set hosts to empty list so nameservers are deleted - ( - deleted_values, - updated_values, - new_values, - oldNameservers, - ) = self.getNameserverChanges(hosts=[]) - - # update the hosts - _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors - addToDomainList, _ = self.createNewHostList(new_values) - deleteHostList, _ = self.createDeleteHostList(deleted_values) - responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) - + try: + # set hosts to empty list so nameservers are deleted + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.getNameserverChanges(hosts=[]) + + # update the hosts + _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors + addToDomainList, _ = self.createNewHostList(new_values) + deleteHostList, _ = self.createDeleteHostList(deleted_values) + responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) + except RegistryError as e: + logger.error(f"Error trying to delete hosts from domain {self}: {e}") + raise e # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) @@ -1070,6 +1072,7 @@ class Domain(TimeStampedModel, DomainHelper): # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) + logger.info("Finished _delete_host_if_not_used inside _delete_domain()") # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) @@ -1083,6 +1086,8 @@ class Domain(TimeStampedModel, DomainHelper): registry.send(request, cleaned=True) except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) + + logger.info("Finished deleting contacts") # delete ds data if it exists if self.dnssecdata: @@ -1097,9 +1102,13 @@ class Domain(TimeStampedModel, DomainHelper): e.note = "Error deleting ds data for %s" % self.name raise e - logger.info("Deleting domain %s", self.name) - request = commands.DeleteDomain(name=self.name) - registry.send(request, cleaned=True) + try: + logger.info("Deleting domain %s", self.name) + request = commands.DeleteDomain(name=self.name) + registry.send(request, cleaned=True) + except RegistryError as e: + logger.error(f"Error deleting domain {self}: {e}") + raise e def __str__(self) -> str: return self.name From 68732c7895d2ca0217b4bd481bd0cfdf5a2bc4c8 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:22:06 -0600 Subject: [PATCH 12/28] add MOAR LOGS --- src/registrar/models/domain.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 18a4442f4..a77641872 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1068,6 +1068,8 @@ class Domain(TimeStampedModel, DomainHelper): # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) + + logger.info("Finished removing nameservers from domain") # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves @@ -1077,13 +1079,19 @@ class Domain(TimeStampedModel, DomainHelper): # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) + logger.info(f"retrieved contacts for domain: {contacts}") for contact in contacts: try: if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: - self._update_domain_with_contact(contact, rem=True) + logger.info(f"Deleting contact: {contact}") + try: + self._update_domain_with_contact(contact, rem=True) + except Exception as e: + logger.error(f"Error while updateing domain with contact: {contact}, e: {e}", exc_info=True) request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) + logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) @@ -1094,8 +1102,10 @@ class Domain(TimeStampedModel, DomainHelper): logger.debug("Deleting ds data for %s", self.name) try: # set and unset client hold to be able to change ds data + logger.info("removing client hold") self._remove_client_hold() self.dnssecdata = None + logger.info("placing client hold") self._place_client_hold() except RegistryError as e: logger.error("Error deleting ds data for %s: %s", self.name, e) From 0f50fd62e9c238f54f2bfeb509813576c921d2f9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 11:38:32 -0600 Subject: [PATCH 13/28] fix typo --- src/registrar/models/domain.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a77641872..adaa8703b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1088,12 +1088,12 @@ class Domain(TimeStampedModel, DomainHelper): try: self._update_domain_with_contact(contact, rem=True) except Exception as e: - logger.error(f"Error while updateing domain with contact: {contact}, e: {e}", exc_info=True) + logger.error(f"Error while updating domain with contact: {contact}, e: {e}", exc_info=True) request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: - logger.error(f"Error deleting contact: {contact}, {e}", exec_info=True) + logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) logger.info("Finished deleting contacts") From f08c2ff4ad39d9c97477b225490b3e8edf848d63 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 10 Feb 2025 10:30:50 -0600 Subject: [PATCH 14/28] linter fixes --- src/registrar/models/domain.py | 14 ++++++++------ src/registrar/tests/test_models_domain.py | 10 +++------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index adaa8703b..812cc3894 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1034,7 +1034,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.error(f"registry error removing client hold: {err}") raise (err) - def _delete_domain(self): + def _delete_domain(self): # noqa """This domain should be deleted from the registry may raises RegistryError, should be caught or handled correctly by caller""" @@ -1056,9 +1056,11 @@ class Domain(TimeStampedModel, DomainHelper): new_values, oldNameservers, ) = self.getNameserverChanges(hosts=[]) - + # update the hosts - _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors + _ = self._update_host_values( + updated_values, oldNameservers + ) # returns nothing, just need to be run and errors addToDomainList, _ = self.createNewHostList(new_values) deleteHostList, _ = self.createDeleteHostList(deleted_values) responseCode = self.addAndRemoveHostsFromDomain(hostsToAdd=addToDomainList, hostsToDelete=deleteHostList) @@ -1068,7 +1070,7 @@ class Domain(TimeStampedModel, DomainHelper): # if unable to update domain raise error and stop if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.BAD_DATA) - + logger.info("Finished removing nameservers from domain") # addAndRemoveHostsFromDomain removes the hosts from the domain object, @@ -1080,7 +1082,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) logger.info(f"retrieved contacts for domain: {contacts}") - + for contact in contacts: try: if contact.contact_type != PublicContact.ContactTypeChoices.REGISTRANT: @@ -1094,7 +1096,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.info(f"sent DeleteContact for {contact}") except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) - + logger.info("Finished deleting contacts") # delete ds data if it exists diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 973a5ad39..8bbd6d60f 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2887,16 +2887,13 @@ class TestAnalystDelete(MockEppLib): # Check that dsdata is None self.assertEqual(domain.dnssecdata, None) - # Print out all calls from the mockedSendFunction - print("\nAll calls to mockedSendFunction:") - for call in self.mockedSendFunction.call_args_list: - print(f"- {call}") - # Check that the UpdateDomain command was sent to the registry with the correct extension self.mockedSendFunction.assert_has_calls( [ call( - commands.UpdateDomain(name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), + commands.UpdateDomain( + name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None + ), cleaned=True, ), ] @@ -2904,7 +2901,6 @@ class TestAnalystDelete(MockEppLib): # Check that the domain was deleted self.assertEqual(domain.state, Domain.State.DELETED) - @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): From 78be172ee4fd3bf6744cffad46420573047d1dbb Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 10 Feb 2025 13:20:46 -0600 Subject: [PATCH 15/28] add retry mechanism to domain deletion --- src/registrar/models/domain.py | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 812cc3894..8366014a3 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,7 @@ from itertools import zip_longest import logging import ipaddress import re -from datetime import date, timedelta +from datetime import date, time, timedelta from typing import Optional from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -1114,12 +1114,34 @@ class Domain(TimeStampedModel, DomainHelper): e.note = "Error deleting ds data for %s" % self.name raise e + # Previous deletions have to complete before we can delete the domain + # This is a polling mechanism to ensure that the domain is deleted try: - logger.info("Deleting domain %s", self.name) - request = commands.DeleteDomain(name=self.name) - registry.send(request, cleaned=True) + logger.info("Attempting to delete domain %s", self.name) + delete_request = commands.DeleteDomain(name=self.name) + max_attempts = 5 # maximum number of retries + wait_interval = 1 # seconds to wait between attempts + + for attempt in range(max_attempts): + try: + registry.send(delete_request, cleaned=True) + logger.info("Domain %s deleted successfully.", self.name) + break # exit the loop on success + except RegistryError as e: + error = e + logger.warning( + "Attempt %d of %d: Domain deletion not possible yet: %s. Retrying in %d seconds.", + attempt + 1, + max_attempts, + e, + wait_interval, + ) + time.sleep(wait_interval) + else: + logger.error("Exceeded maximum attempts to delete domain %s", self.name) + raise error except RegistryError as e: - logger.error(f"Error deleting domain {self}: {e}") + logger.error("Error deleting domain %s after polling: %s", self.name, e) raise e def __str__(self) -> str: From e751388533a40176dea8285d783b1078f7747989 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 10 Feb 2025 15:58:14 -0500 Subject: [PATCH 16/28] better error handling in domain deletion --- src/registrar/admin.py | 4 ++-- src/registrar/models/domain.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7dbe7abb0..d22bff58f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3679,10 +3679,10 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ), messages.ERROR, ) - except Exception: + except Exception as e: self.message_user( request, - "Could not delete: An unspecified error occured", + f"Could not delete: An unspecified error occured: {e}", messages.ERROR, ) else: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8366014a3..23ffb2ad6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1097,7 +1097,7 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) - logger.info("Finished deleting contacts") + logger.info(f"Finished deleting contacts for {self.name}") # delete ds data if it exists if self.dnssecdata: From 40d5828d3e14ecbb23de52ea9eb651ba9edddbef Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 11 Feb 2025 09:30:13 -0600 Subject: [PATCH 17/28] check domain before deletion --- src/registrar/models/domain.py | 69 ++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8366014a3..d1bb8a0ae 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1076,7 +1076,7 @@ class Domain(TimeStampedModel, DomainHelper): # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) - logger.info("Finished _delete_host_if_not_used inside _delete_domain()") + logger.info("Finished _delete_hosts_if_not_used inside _delete_domain()") # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) @@ -1113,37 +1113,50 @@ class Domain(TimeStampedModel, DomainHelper): logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name raise e - - # Previous deletions have to complete before we can delete the domain - # This is a polling mechanism to ensure that the domain is deleted + + # check if the domain can be deleted + if not self._domain_can_be_deleted(): + raise RegistryError(code=ErrorCode.UNKNOWN, note="Domain cannot be deleted.") + + # delete the domain + request = commands.DeleteDomain(name=self.name) try: - logger.info("Attempting to delete domain %s", self.name) - delete_request = commands.DeleteDomain(name=self.name) - max_attempts = 5 # maximum number of retries - wait_interval = 1 # seconds to wait between attempts - - for attempt in range(max_attempts): - try: - registry.send(delete_request, cleaned=True) - logger.info("Domain %s deleted successfully.", self.name) - break # exit the loop on success - except RegistryError as e: - error = e - logger.warning( - "Attempt %d of %d: Domain deletion not possible yet: %s. Retrying in %d seconds.", - attempt + 1, - max_attempts, - e, - wait_interval, - ) - time.sleep(wait_interval) - else: - logger.error("Exceeded maximum attempts to delete domain %s", self.name) - raise error + registry.send(request, cleaned=True) + logger.info("Domain %s deleted successfully.", self.name) except RegistryError as e: - logger.error("Error deleting domain %s after polling: %s", self.name, e) + logger.error("Error deleting domain %s: %s", self.name, e) raise e + def _domain_can_be_deleted(self, max_attempts=5, wait_interval=2) -> bool: + """ + Polls the registry using InfoDomain calls to confirm that the domain can be deleted. + Returns True if the domain can be deleted, False otherwise. Includes a retry mechanism + using wait_interval and max_attempts, which may be necessary if subdomains and other + associated objects were only recently deleted as the registry may not be immediately updated. + """ + logger.info("Polling registry to confirm deletion pre-conditions for %s", self.name) + last_info_error = None + for attempt in range(max_attempts): + try: + info_response = registry.send(commands.InfoDomain(name=self.name), cleaned=True) + domain_info = info_response.res_data[0] + hosts_associated = getattr(domain_info, "hosts", None) + if hosts_associated is None or len(hosts_associated) == 0: + logger.info("InfoDomain reports no associated hosts for %s. Proceeding with deletion.", self.name) + return True + else: + logger.info("Attempt %d: Domain %s still has hosts: %s", attempt + 1, self.name, hosts_associated) + except RegistryError as info_e: + # If the domain is already gone, we can assume deletion already occurred. + if info_e.code == ErrorCode.OBJECT_DOES_NOT_EXIST: + logger.info("InfoDomain check indicates domain %s no longer exists.", self.name) + raise info_e + logger.warning("Attempt %d: Error during InfoDomain check: %s", attempt + 1, info_e) + time.sleep(wait_interval) + else: + logger.error("Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", self.name, last_info_error) + return False + def __str__(self) -> str: return self.name From 641f8885a40e15ea8f576c40abee11e5fe93eb80 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 11 Feb 2025 14:51:59 -0600 Subject: [PATCH 18/28] test and linter fixes --- src/registrar/models/domain.py | 23 +++-- src/registrar/tests/test_models_domain.py | 101 ++++++++++------------ 2 files changed, 61 insertions(+), 63 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 080997289..1f0d2dcf2 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,8 @@ from itertools import zip_longest import logging import ipaddress import re -from datetime import date, time, timedelta +import time +from datetime import date, timedelta from typing import Optional from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -161,12 +162,12 @@ class Domain(TimeStampedModel, DomainHelper): """Returns a help message for a desired state. If none is found, an empty string is returned""" help_texts = { # For now, unknown has the same message as DNS_NEEDED - cls.UNKNOWN: ("Before this domain can be used, " "you’ll need to add name server addresses."), - cls.DNS_NEEDED: ("Before this domain can be used, " "you’ll need to add name server addresses."), + cls.UNKNOWN: ("Before this domain can be used, " "you'll need to add name server addresses."), + cls.DNS_NEEDED: ("Before this domain can be used, " "you'll need to add name server addresses."), cls.READY: "This domain has name servers and is ready for use.", cls.ON_HOLD: ( "This domain is administratively paused, " - "so it can’t be edited and won’t resolve in DNS. " + "so it can't be edited and won't resolve in DNS. " "Contact help@get.gov for details." ), cls.DELETED: ("This domain has been removed and " "is no longer registered to your organization."), @@ -1113,11 +1114,11 @@ class Domain(TimeStampedModel, DomainHelper): logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name raise e - + # check if the domain can be deleted if not self._domain_can_be_deleted(): - raise RegistryError(code=ErrorCode.UNKNOWN, note="Domain cannot be deleted.") - + raise RegistryError(code=ErrorCode.COMMAND_FAILED, note="Associated objects were not cleared.") + # delete the domain request = commands.DeleteDomain(name=self.name) try: @@ -1131,7 +1132,7 @@ class Domain(TimeStampedModel, DomainHelper): """ Polls the registry using InfoDomain calls to confirm that the domain can be deleted. Returns True if the domain can be deleted, False otherwise. Includes a retry mechanism - using wait_interval and max_attempts, which may be necessary if subdomains and other + using wait_interval and max_attempts, which may be necessary if subdomains and other associated objects were only recently deleted as the registry may not be immediately updated. """ logger.info("Polling registry to confirm deletion pre-conditions for %s", self.name) @@ -1154,7 +1155,11 @@ class Domain(TimeStampedModel, DomainHelper): logger.warning("Attempt %d: Error during InfoDomain check: %s", attempt + 1, info_e) time.sleep(wait_interval) else: - logger.error("Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", self.name, last_info_error) + logger.error( + "Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", + self.name, + last_info_error, + ) return False def __str__(self) -> str: diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 8bbd6d60f..760ea4ee3 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -35,6 +35,7 @@ from epplibwrapper import ( from .common import MockEppLib, MockSESClient, less_console_noise import logging import boto3_mocking # type: ignore +import copy logger = logging.getLogger(__name__) @@ -2697,38 +2698,6 @@ class TestAnalystDelete(MockEppLib): Domain.objects.all().delete() super().tearDown() - @less_console_noise_decorator - def test_analyst_deletes_domain(self): - """ - Scenario: Analyst permanently deletes a domain - When `domain.deletedInEpp()` is called - Then `commands.DeleteDomain` is sent to the registry - And `state` is set to `DELETED` - - The deleted date is set. - """ - # Put the domain in client hold - self.domain.place_client_hold() - # Delete it... - self.domain.deletedInEpp() - self.domain.save() - self.mockedSendFunction.assert_has_calls( - [ - call( - commands.DeleteDomain(name="fake.gov"), - cleaned=True, - ) - ] - ) - # Domain itself should not be deleted - self.assertNotEqual(self.domain, None) - # Domain should have the right state - self.assertEqual(self.domain.state, Domain.State.DELETED) - # Domain should have a deleted - self.assertNotEqual(self.domain.deleted, None) - # Cache should be invalidated - self.assertEqual(self.domain._cache, {}) - @less_console_noise_decorator def test_deletion_is_unsuccessful(self): """ @@ -2756,18 +2725,44 @@ class TestAnalystDelete(MockEppLib): @less_console_noise_decorator def test_deletion_with_host_and_contacts(self): """ - Scenario: Domain with related Host and Contacts is Deleted - When a contact and host exists that is tied to this domain - Then all the needed commands are sent to the registry - And `state` is set to `DELETED` - """ - # Put the domain in client hold - self.domain_with_contacts.place_client_hold() - # Delete it - self.domain_with_contacts.deletedInEpp() - self.domain_with_contacts.save() + Scenario: Domain with related Host and Contacts is Deleted. + When a contact and host exists that is tied to this domain, + then all the needed commands are sent to the registry and + the domain's state is set to DELETED. - # Check that the host and contacts are deleted + This test now asserts only the commands that are actually issued + during the deletion process. + """ + # Put the domain in client hold. + self.domain_with_contacts.place_client_hold() + + # Invalidate the cache so that deletion fetches fresh data. + self.domain_with_contacts._invalidate_cache() + + # We'll use a mutable counter to simulate different responses if needed. + info_domain_call_count = [0] + + # TODO: This is a hack, we should refactor the MockEPPLib to be more flexible + def side_effect(request, cleaned=True): + # For an InfoDomain command for "freeman.gov", simulate behavior: + if isinstance(request, commands.InfoDomain) and request.name.lower() == "freeman.gov": + info_domain_call_count[0] += 1 + fake_info = copy.deepcopy(self.InfoDomainWithContacts) + # If this branch ever gets hit, you could vary response based on call count. + # But note: in our current deletion flow, InfoDomain may not be called. + if info_domain_call_count[0] == 1: + fake_info.hosts = ["fake.host.com"] + else: + fake_info.hosts = [] + return MagicMock(res_data=[fake_info]) + return self.mockedSendFunction(request, cleaned=cleaned) + + with patch("registrar.models.domain.registry.send", side_effect=side_effect): + self.domain_with_contacts.deletedInEpp() + self.domain_with_contacts.save() + + # Now assert the expected calls that we know occur. + # Note: we no longer assert a call to InfoDomain. self.mockedSendFunction.assert_has_calls( [ call( @@ -2782,14 +2777,10 @@ class TestAnalystDelete(MockEppLib): ), cleaned=True, ), - ] + ], ) self.mockedSendFunction.assert_has_calls( [ - call( - commands.InfoDomain(name="freeman.gov", auth_info=None), - cleaned=True, - ), call( commands.InfoHost(name="fake.host.com"), cleaned=True, @@ -2806,7 +2797,8 @@ class TestAnalystDelete(MockEppLib): ), cleaned=True, ), - ] + ], + any_order=True, ) self.mockedSendFunction.assert_has_calls( [ @@ -2857,13 +2849,10 @@ class TestAnalystDelete(MockEppLib): ), ], ) - - # Domain itself should not be deleted - self.assertNotEqual(self.domain_with_contacts, None) - # State should have changed + self.assertIsNotNone(self.domain_with_contacts) self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) - # @less_console_noise + @less_console_noise_decorator def test_analyst_deletes_domain_with_ds_data(self): """ Scenario: Domain with DS data is deleted @@ -2880,6 +2869,10 @@ class TestAnalystDelete(MockEppLib): ) domain.save() + # Mock the InfoDomain command data to return a domain with no hosts + # This is needed to simulate the domain being able to be deleted + self.mockDataInfoDomain.hosts = [] + # Delete the domain domain.deletedInEpp() domain.save() From f8a5b8e629c85e1a1d319fb79614159fb34fd4d1 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:44:53 -0600 Subject: [PATCH 19/28] Update admin.py --- src/registrar/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d22bff58f..0ca49563a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3679,10 +3679,10 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ), messages.ERROR, ) - except Exception as e: + except Exception: self.message_user( request, - f"Could not delete: An unspecified error occured: {e}", + f"Could not delete: An unspecified error occured.", messages.ERROR, ) else: From 0106ffbf0520ddbf5dc564bbba3a603e6f064dea Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:45:31 -0600 Subject: [PATCH 20/28] Update admin.py --- 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 0ca49563a..7dbe7abb0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3682,7 +3682,7 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): except Exception: self.message_user( request, - f"Could not delete: An unspecified error occured.", + "Could not delete: An unspecified error occured", messages.ERROR, ) else: From 1ca20dd421e05139c30214fa1ef258fc42a47b5d Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:47:46 -0600 Subject: [PATCH 21/28] Update domain.py --- src/registrar/models/domain.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 1f0d2dcf2..78bf7a36e 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -162,12 +162,12 @@ class Domain(TimeStampedModel, DomainHelper): """Returns a help message for a desired state. If none is found, an empty string is returned""" help_texts = { # For now, unknown has the same message as DNS_NEEDED - cls.UNKNOWN: ("Before this domain can be used, " "you'll need to add name server addresses."), - cls.DNS_NEEDED: ("Before this domain can be used, " "you'll need to add name server addresses."), + cls.UNKNOWN: ("Before this domain can be used, " "you’ll need to add name server addresses."), + cls.DNS_NEEDED: ("Before this domain can be used, " "you’ll need to add name server addresses."), cls.READY: "This domain has name servers and is ready for use.", cls.ON_HOLD: ( "This domain is administratively paused, " - "so it can't be edited and won't resolve in DNS. " + "so it can’t be edited and won’t resolve in DNS. " "Contact help@get.gov for details." ), cls.DELETED: ("This domain has been removed and " "is no longer registered to your organization."), From d887a7514a1b77d298252a560fe4e0959689e01f Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 20 Feb 2025 11:21:06 -0600 Subject: [PATCH 22/28] linter and test fixes --- src/registrar/admin.py | 2 + src/registrar/models/domain.py | 3 +- src/registrar/tests/test_admin.py | 2 +- src/registrar/tests/test_admin_domain.py | 11 +- src/registrar/tests/test_models_domain.py | 151 +++++++++++----------- 5 files changed, 88 insertions(+), 81 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 2d2b90a5f..b3dd76f4f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3738,11 +3738,13 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Using variables to get past the linter message1 = f"Cannot delete Domain when in state {obj.state}" message2 = f"This subdomain is being used as a hostname on another domain: {err.note}" + message3 = f"Command failed with note: {err.note}" # Human-readable mappings of ErrorCodes. Can be expanded. error_messages = { # noqa on these items as black wants to reformat to an invalid length ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: message1, ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: message2, + ErrorCode.COMMAND_FAILED: message3, } message = "Cannot connect to the registry" diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8f5f90d58..d3c0ed347 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1117,7 +1117,8 @@ class Domain(TimeStampedModel, DomainHelper): # check if the domain can be deleted if not self._domain_can_be_deleted(): - raise RegistryError(code=ErrorCode.COMMAND_FAILED, note="Associated objects were not cleared.") + note = "Domain has associated objects that prevent deletion." + raise RegistryError(code=ErrorCode.COMMAND_FAILED, note=note) # delete the domain request = commands.DeleteDomain(name=self.name) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index aadb85c66..8e7449f07 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -3816,7 +3816,7 @@ class TestTransferUser(WebTest): with self.assertRaises(User.DoesNotExist): self.user2.refresh_from_db() - # @less_console_noise_decorator + @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 diff --git a/src/registrar/tests/test_admin_domain.py b/src/registrar/tests/test_admin_domain.py index 9489c2e0f..76406bfad 100644 --- a/src/registrar/tests/test_admin_domain.py +++ b/src/registrar/tests/test_admin_domain.py @@ -178,7 +178,7 @@ class TestDomainAdminAsStaff(MockEppLib): Then a user-friendly success message is returned for displaying on the web And `state` is set to `DELETED` """ - domain = create_ready_domain() + domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.READY) # Put in client hold domain.place_client_hold() # Ensure everything is displaying correctly @@ -212,7 +212,7 @@ class TestDomainAdminAsStaff(MockEppLib): mock_add_message.assert_called_once_with( request, messages.INFO, - "Domain city.gov has been deleted. Thanks!", + "Domain my-nameserver.gov has been deleted. Thanks!", extra_tags="", fail_silently=False, ) @@ -266,7 +266,7 @@ class TestDomainAdminAsStaff(MockEppLib): mock_add_message.assert_called_once_with( request, messages.ERROR, - "Error deleting this Domain: This subdomain is being used as a hostname on another domain: ns1.sharedhost.com", # noqa + "Error deleting this Domain: Command failed with note: Domain has associated objects that prevent deletion.", # noqa extra_tags="", fail_silently=False, ) @@ -321,7 +321,7 @@ class TestDomainAdminAsStaff(MockEppLib): Then `commands.DeleteDomain` is sent to the registry And Domain returns normally without an error dialog """ - domain = create_ready_domain() + domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.READY) # Put in client hold domain.place_client_hold() # Ensure everything is displaying correctly @@ -340,12 +340,13 @@ class TestDomainAdminAsStaff(MockEppLib): ) request.user = self.client # Delete it once + with patch("django.contrib.messages.add_message") as mock_add_message: self.admin.do_delete_domain(request, domain) mock_add_message.assert_called_once_with( request, messages.INFO, - "Domain city.gov has been deleted. Thanks!", + "Domain my-nameserver.gov has been deleted. Thanks!", extra_tags="", fail_silently=False, ) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 760ea4ee3..f14218382 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -98,58 +98,59 @@ class TestDomainCache(MockEppLib): self.mockedSendFunction.assert_has_calls(expectedCalls) + # @less_console_noise_decorator def test_cache_nested_elements_not_subdomain(self): """Cache works correctly with the nested objects cache and hosts""" - with less_console_noise(): - domain, _ = Domain.objects.get_or_create(name="igorville.gov") - # The contact list will initially contain objects of type 'DomainContact' - # this is then transformed into PublicContact, and cache should NOT - # hold onto the DomainContact object - expectedUnfurledContactsList = [ - common.DomainContact(contact="123", type="security"), - ] - expectedContactsDict = { - PublicContact.ContactTypeChoices.ADMINISTRATIVE: "adminContact", - PublicContact.ContactTypeChoices.SECURITY: "securityContact", - PublicContact.ContactTypeChoices.TECHNICAL: "technicalContact", - } - expectedHostsDict = { - "name": self.mockDataInfoDomain.hosts[0], - "addrs": [], # should return empty bc fake.host.com is not a subdomain of igorville.gov - "cr_date": self.mockDataInfoHosts.cr_date, - } - # this can be changed when the getter for contacts is implemented - domain._get_property("contacts") + domain, _ = Domain.objects.get_or_create(name="igorville.gov") + # The contact list will initially contain objects of type 'DomainContact' + # this is then transformed into PublicContact, and cache should NOT + # hold onto the DomainContact object + expectedUnfurledContactsList = [ + common.DomainContact(contact="123", type="security"), + ] + expectedContactsDict = { + PublicContact.ContactTypeChoices.ADMINISTRATIVE: "adminContact", + PublicContact.ContactTypeChoices.SECURITY: "securityContact", + PublicContact.ContactTypeChoices.TECHNICAL: "technicalContact", + } + expectedHostsDict = { + "name": self.mockDataInfoDomain.hosts[0], + "addrs": [], # should return empty bc fake.host.com is not a subdomain of igorville.gov + "cr_date": self.mockDataInfoHosts.cr_date, + } - # check domain info is still correct and not overridden - self.assertEqual(domain._cache["auth_info"], self.mockDataInfoDomain.auth_info) - self.assertEqual(domain._cache["cr_date"], self.mockDataInfoDomain.cr_date) + # this can be changed when the getter for contacts is implemented + domain._get_property("contacts") - # check contacts - self.assertEqual(domain._cache["_contacts"], self.mockDataInfoDomain.contacts) - # The contact list should not contain what is sent by the registry by default, - # as _fetch_cache will transform the type to PublicContact - self.assertNotEqual(domain._cache["contacts"], expectedUnfurledContactsList) + # check domain info is still correct and not overridden + self.assertEqual(domain._cache["auth_info"], self.mockDataInfoDomain.auth_info) + self.assertEqual(domain._cache["cr_date"], self.mockDataInfoDomain.cr_date) - self.assertEqual(domain._cache["contacts"], expectedContactsDict) + # check contacts + self.assertEqual(domain._cache["_contacts"], self.mockDataInfoDomain.contacts) + # The contact list should not contain what is sent by the registry by default, + # as _fetch_cache will transform the type to PublicContact + self.assertNotEqual(domain._cache["contacts"], expectedUnfurledContactsList) - # get and check hosts is set correctly - domain._get_property("hosts") - self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) - self.assertEqual(domain._cache["contacts"], expectedContactsDict) - # invalidate cache - domain._cache = {} + self.assertEqual(domain._cache["contacts"], expectedContactsDict) - # get host - domain._get_property("hosts") - # Should return empty bc fake.host.com is not a subdomain of igorville.gov - self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) + # get and check hosts is set correctly + domain._get_property("hosts") + self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) + self.assertEqual(domain._cache["contacts"], expectedContactsDict) + # invalidate cache + domain._cache = {} - # get contacts - domain._get_property("contacts") - self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) - self.assertEqual(domain._cache["contacts"], expectedContactsDict) + # get host + domain._get_property("hosts") + # Should return empty bc fake.host.com is not a subdomain of igorville.gov + self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) + + # get contacts + domain._get_property("contacts") + self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) + self.assertEqual(domain._cache["contacts"], expectedContactsDict) def test_cache_nested_elements_is_subdomain(self): """Cache works correctly with the nested objects cache and hosts""" @@ -1248,6 +1249,13 @@ class TestRegistrantNameservers(MockEppLib): self.domainWithThreeNS, _ = Domain.objects.get_or_create( name="threenameserversDomain.gov", state=Domain.State.READY ) + + def tearDown(self): + PublicContact.objects.all().delete() + HostIP.objects.all().delete() + Host.objects.all().delete() + Domain.objects.all().delete() + super().tearDown() def test_get_nameserver_changes_success_deleted_vals(self): """Testing only deleting and no other changes""" @@ -1798,6 +1806,7 @@ class TestRegistrantNameservers(MockEppLib): mock_host_ip_get_or_create.assert_not_called() self.assertEqual(mock_host_ip_get_or_create.call_count, 0) + # @less_console_noise_decorator def test_nameservers_stored_on_fetch_cache_not_subdomain_with_ip(self): """ Scenario: Nameservers are stored in db when they are retrieved from fetch_cache. @@ -1809,21 +1818,20 @@ class TestRegistrantNameservers(MockEppLib): #3: Nameserver is not a subdomain, but it does have an IP address returned due to how we set up our defaults """ - with less_console_noise(): - domain, _ = Domain.objects.get_or_create(name="fake.gov", state=Domain.State.READY) + domain, _ = Domain.objects.get_or_create(name="freeman.gov", state=Domain.State.READY) + + with patch.object(Host.objects, "get_or_create") as mock_host_get_or_create, patch.object( + HostIP.objects, "get_or_create" + ) as mock_host_ip_get_or_create: + mock_host_get_or_create.return_value = (Host(domain=domain), True) + mock_host_ip_get_or_create.return_value = (HostIP(), True) - with patch.object(Host.objects, "get_or_create") as mock_host_get_or_create, patch.object( - HostIP.objects, "get_or_create" - ) as mock_host_ip_get_or_create: - mock_host_get_or_create.return_value = (Host(domain=domain), True) - mock_host_ip_get_or_create.return_value = (HostIP(), True) + # force fetch_cache to be called, which will return above documented mocked hosts + domain.nameservers - # force fetch_cache to be called, which will return above documented mocked hosts - domain.nameservers - - mock_host_get_or_create.assert_called_once_with(domain=domain, name="fake.host.com") - mock_host_ip_get_or_create.assert_not_called() - self.assertEqual(mock_host_ip_get_or_create.call_count, 0) + mock_host_get_or_create.assert_called_once_with(domain=domain, name="fake.host.com") + mock_host_ip_get_or_create.assert_not_called() + self.assertEqual(mock_host_ip_get_or_create.call_count, 0) def test_nameservers_stored_on_fetch_cache_not_subdomain_without_ip(self): """ @@ -1862,12 +1870,6 @@ class TestRegistrantNameservers(MockEppLib): with self.assertRaises(RegistryError): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] - def tearDown(self): - HostIP.objects.all().delete() - Host.objects.all().delete() - Domain.objects.all().delete() - return super().tearDown() - class TestNameserverValidation(TestCase): """Test the isValidDomain method which validates nameservers""" @@ -1948,8 +1950,6 @@ class TestRegistrantDNSSEC(MockEppLib): And a domain exists in the registry """ super().setUp() - # for the tests, need a domain in the unknown state - self.domain, _ = Domain.objects.get_or_create(name="fake.gov") def tearDown(self): PublicContact.objects.all().delete() @@ -2042,6 +2042,7 @@ class TestRegistrantDNSSEC(MockEppLib): self.assertEquals(dnssecdata_get.dsData, self.dnssecExtensionWithDsData.dsData) patcher.stop() + @less_console_noise_decorator def test_dnssec_is_idempotent(self): """ Scenario: Registrant adds DNS data twice, due to a UI glitch @@ -2058,7 +2059,6 @@ class TestRegistrantDNSSEC(MockEppLib): 5 - getter properly parses dnssecdata from InfoDomain response and sets to cache """ - # need to use a separate patcher and side_effect for this test, as # response from InfoDomain must be different for different iterations # of the same command @@ -2127,6 +2127,7 @@ class TestRegistrantDNSSEC(MockEppLib): self.assertEquals(dnssecdata_get.dsData, self.dnssecExtensionWithDsData.dsData) patcher.stop() + @less_console_noise_decorator def test_user_adds_dnssec_data_multiple_dsdata(self): """ Scenario: Registrant adds DNSSEC data with multiple DSData. @@ -2195,6 +2196,7 @@ class TestRegistrantDNSSEC(MockEppLib): self.assertEquals(dnssecdata_get.dsData, self.dnssecExtensionWithMultDsData.dsData) patcher.stop() + # @less_console_noise_decorator def test_user_removes_dnssec_data(self): """ Scenario: Registrant removes DNSSEC ds data. @@ -2220,28 +2222,27 @@ class TestRegistrantDNSSEC(MockEppLib): else: return MagicMock(res_data=[self.mockDataInfoHosts]) - with less_console_noise(): - patcher = patch("registrar.models.domain.registry.send") - mocked_send = patcher.start() + with patch("registrar.models.domain.registry.send") as mocked_send: mocked_send.side_effect = side_effect + domain, _ = Domain.objects.get_or_create(name="dnssec-dsdata.gov") - - # Initial setting of dnssec data + domain.dnssecdata = self.dnssecExtensionWithDsData - + # Check dsdata_last_change is updated domain = Domain.objects.get(name="dnssec-dsdata.gov") self.assertIsNotNone(domain.dsdata_last_change) - initial_change = domain.dsdata_last_change + # Invalidate the cache to force a fresh lookup + domain._invalidate_cache() + # Remove dnssec data domain.dnssecdata = self.dnssecExtensionRemovingDsData # Check that dsdata_last_change is updated again domain = Domain.objects.get(name="dnssec-dsdata.gov") self.assertIsNotNone(domain.dsdata_last_change) - self.assertNotEqual(domain.dsdata_last_change, initial_change) # get the DNS SEC extension added to the UpdateDomain command and @@ -2293,7 +2294,6 @@ class TestRegistrantDNSSEC(MockEppLib): ), ] ) - patcher.stop() def test_update_is_unsuccessful(self): """ @@ -2895,6 +2895,9 @@ class TestAnalystDelete(MockEppLib): # Check that the domain was deleted self.assertEqual(domain.state, Domain.State.DELETED) + # reset to avoid test pollution + self.mockDataInfoDomain.hosts = ['fake.host.com'] + @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): """ From 9b446df6ea725f0b79a2be5d0f3f6281d4d7d451 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 20 Feb 2025 11:32:53 -0600 Subject: [PATCH 23/28] linter fix --- src/registrar/tests/test_models_domain.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index f14218382..93072f93b 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -1249,7 +1249,7 @@ class TestRegistrantNameservers(MockEppLib): self.domainWithThreeNS, _ = Domain.objects.get_or_create( name="threenameserversDomain.gov", state=Domain.State.READY ) - + def tearDown(self): PublicContact.objects.all().delete() HostIP.objects.all().delete() @@ -1819,7 +1819,7 @@ class TestRegistrantNameservers(MockEppLib): due to how we set up our defaults """ domain, _ = Domain.objects.get_or_create(name="freeman.gov", state=Domain.State.READY) - + with patch.object(Host.objects, "get_or_create") as mock_host_get_or_create, patch.object( HostIP.objects, "get_or_create" ) as mock_host_ip_get_or_create: @@ -2059,6 +2059,7 @@ class TestRegistrantDNSSEC(MockEppLib): 5 - getter properly parses dnssecdata from InfoDomain response and sets to cache """ + # need to use a separate patcher and side_effect for this test, as # response from InfoDomain must be different for different iterations # of the same command @@ -2224,11 +2225,11 @@ class TestRegistrantDNSSEC(MockEppLib): with patch("registrar.models.domain.registry.send") as mocked_send: mocked_send.side_effect = side_effect - + domain, _ = Domain.objects.get_or_create(name="dnssec-dsdata.gov") - + domain.dnssecdata = self.dnssecExtensionWithDsData - + # Check dsdata_last_change is updated domain = Domain.objects.get(name="dnssec-dsdata.gov") self.assertIsNotNone(domain.dsdata_last_change) @@ -2896,7 +2897,7 @@ class TestAnalystDelete(MockEppLib): self.assertEqual(domain.state, Domain.State.DELETED) # reset to avoid test pollution - self.mockDataInfoDomain.hosts = ['fake.host.com'] + self.mockDataInfoDomain.hosts = ["fake.host.com"] @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): From 44d8b6e7946039e91fcab08b63fe6e4cc3808b92 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 20 Feb 2025 14:16:07 -0500 Subject: [PATCH 24/28] send emails to canceled invitations --- src/registrar/utility/email_invitations.py | 42 ++++++++++++++++++++++ src/registrar/views/portfolios.py | 5 +++ 2 files changed, 47 insertions(+) diff --git a/src/registrar/utility/email_invitations.py b/src/registrar/utility/email_invitations.py index 1394f7d48..08ebb4d86 100644 --- a/src/registrar/utility/email_invitations.py +++ b/src/registrar/utility/email_invitations.py @@ -2,6 +2,7 @@ from datetime import date from django.conf import settings from registrar.models import Domain, DomainInvitation, UserDomainRole from registrar.models.portfolio import Portfolio +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.errors import ( @@ -310,6 +311,47 @@ def send_portfolio_member_permission_remove_email(requestor, permissions: UserPo return True +def send_portfolio_invitation_remove_email(requestor, invitation: PortfolioInvitation): + """ + Sends an email notification to a portfolio invited member when their permissions are deleted. + + This function retrieves the requestor's email and sends a templated email to the affected email, + notifying them of the removal of their invited portfolio permissions. + + Args: + requestor (User): The user initiating the permission update. + invitation (PortfolioInvitation): The invitation object containing the affected user + and the portfolio details. + + Returns: + bool: True if the email was sent successfully, False if an EmailSendingError occurred. + + Raises: + MissingEmailError: If the requestor has no email associated with their account. + """ + requestor_email = _get_requestor_email(requestor, portfolio=invitation.portfolio) + try: + send_templated_email( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address=invitation.email, + context={ + "requested_user": None, + "portfolio": invitation.portfolio, + "requestor_email": requestor_email, + }, + ) + except EmailSendingError: + logger.warning( + "Could not send email organization member removal notification to %s " "for portfolio: %s", + invitation.email, + invitation.portfolio.organization_name, + exc_info=True, + ) + return False + return True + + def send_portfolio_admin_addition_emails(email: str, requestor, portfolio: Portfolio): """ Notifies all portfolio admins of the provided portfolio of a newly invited portfolio admin diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index a52a757ec..8687ad47c 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -20,6 +20,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_invitation_remove_email, send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) @@ -478,6 +479,10 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): email=portfolio_invitation.email, requestor=request.user, portfolio=portfolio_invitation.portfolio ): messages.warning(self.request, "Could not send email notification to existing organization admins.") + if not send_portfolio_invitation_remove_email(requestor=request.user, invitation=portfolio_invitation): + messages.warning( + request, f"Could not send email notification to {portfolio_invitation.email}" + ) except Exception as e: self._handle_exceptions(e) From 963f738d8979873ead1faa77b8374f7019e375a9 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 20 Feb 2025 15:28:10 -0500 Subject: [PATCH 25/28] updating tests --- src/registrar/tests/test_email_invitations.py | 75 +++++++++++++++++++ src/registrar/tests/test_views_portfolio.py | 17 ++++- src/registrar/views/portfolios.py | 18 ++--- 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/src/registrar/tests/test_email_invitations.py b/src/registrar/tests/test_email_invitations.py index 391fbd445..2331d35e8 100644 --- a/src/registrar/tests/test_email_invitations.py +++ b/src/registrar/tests/test_email_invitations.py @@ -3,6 +3,7 @@ from unittest.mock import patch, MagicMock from datetime import date from registrar.models.domain import Domain from registrar.models.portfolio import Portfolio +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user import User from registrar.models.user_domain_role import UserDomainRole from registrar.models.user_portfolio_permission import UserPortfolioPermission @@ -16,6 +17,7 @@ from registrar.utility.email_invitations import ( send_portfolio_admin_addition_emails, send_portfolio_admin_removal_emails, send_portfolio_invitation_email, + send_portfolio_invitation_remove_email, send_portfolio_member_permission_remove_email, send_portfolio_member_permission_update_email, ) @@ -1037,3 +1039,76 @@ class TestSendPortfolioMemberPermissionRemoveEmail(unittest.TestCase): # Assertions mock_logger.warning.assert_not_called() # Function should fail before logging email failure + + +class TestSendPortfolioInvitationRemoveEmail(unittest.TestCase): + """Unit tests for send_portfolio_invitation_remove_email function.""" + + @patch("registrar.utility.email_invitations.send_templated_email") + @patch("registrar.utility.email_invitations._get_requestor_email") + def test_send_email_success(self, mock_get_requestor_email, mock_send_email): + """Test that the email is sent successfully when there are no errors.""" + # Mock data + requestor = MagicMock() + invitation = MagicMock(spec=PortfolioInvitation) + invitation.email = "user@example.com" + invitation.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_invitation_remove_email(requestor, invitation) + + # Assertions + mock_get_requestor_email.assert_called_once_with(requestor, portfolio=invitation.portfolio) + mock_send_email.assert_called_once_with( + "emails/portfolio_removal.txt", + "emails/portfolio_removal_subject.txt", + to_address="user@example.com", + context={ + "requested_user": None, + "portfolio": invitation.portfolio, + "requestor_email": "requestor@example.com", + }, + ) + self.assertTrue(result) + + @patch("registrar.utility.email_invitations.send_templated_email", side_effect=EmailSendingError("Email failed")) + @patch("registrar.utility.email_invitations._get_requestor_email") + @patch("registrar.utility.email_invitations.logger") + def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_send_email): + """Test that the function returns False and logs an error when email sending fails.""" + # Mock data + requestor = MagicMock() + invitation = MagicMock(spec=PortfolioInvitation) + invitation.email = "user@example.com" + invitation.portfolio.organization_name = "Test Portfolio" + + mock_get_requestor_email.return_value = "requestor@example.com" + + # Call function + result = send_portfolio_invitation_remove_email(requestor, invitation) + + # Assertions + mock_logger.warning.assert_called_once_with( + "Could not send email organization member removal notification to %s for portfolio: %s", + invitation.email, + invitation.portfolio.organization_name, + exc_info=True, + ) + self.assertFalse(result) + + @patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error")) + @patch("registrar.utility.email_invitations.logger") + def test_requestor_email_retrieval_failure(self, mock_logger, mock_get_requestor_email): + """Test that an exception in retrieving requestor email is logged.""" + # Mock data + requestor = MagicMock() + invitation = MagicMock(spec=PortfolioInvitation) + + # Call function + with self.assertRaises(Exception): + send_portfolio_invitation_remove_email(requestor, invitation) + + # Assertions + mock_logger.warning.assert_not_called() # Function should fail before logging email failure diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 6194668c2..1952d7c12 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2092,7 +2092,8 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_manage_members_page_invitedmember(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_invitation_remove_email") + def test_portfolio_member_delete_view_manage_members_page_invitedmember(self, send_invited_member_removal, mock_send_removal_emails): """Success state w/ deleting invited member on Manage Members page should redirect back to Members Table""" # I'm a user @@ -2113,6 +2114,10 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], ) + + # Invited member removal email sent successfully + send_invited_member_removal.return_value = True + with patch("django.contrib.messages.success") as mock_success: self.client.force_login(self.user) response = self.client.post( @@ -2136,6 +2141,16 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): # assert send_portfolio_admin_removal_emails not called since invitation # is for a basic member mock_send_removal_emails.assert_not_called() + # assert that send_portfolio_invitation_remove_email is called + send_invited_member_removal.assert_called_once() + + # Get the arguments passed to send_portfolio_invitation_removal_email + _, called_kwargs = send_invited_member_removal.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["invitation"].email, invitation.email) + self.assertEqual(called_kwargs["invitation"].portfolio, invitation.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 8687ad47c..f53fe5f22 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -471,20 +471,20 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): """ portfolio_invitation = get_object_or_404(PortfolioInvitation, pk=pk) - # if invitation being removed is an admin - if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_invitation.roles: - try: + try: + # if invitation being removed is an admin + if UserPortfolioRoleChoices.ORGANIZATION_ADMIN in portfolio_invitation.roles: # attempt to send notification emails of the removal to portfolio admins if not send_portfolio_admin_removal_emails( email=portfolio_invitation.email, requestor=request.user, portfolio=portfolio_invitation.portfolio ): messages.warning(self.request, "Could not send email notification to existing organization admins.") - if not send_portfolio_invitation_remove_email(requestor=request.user, invitation=portfolio_invitation): - messages.warning( - request, f"Could not send email notification to {portfolio_invitation.email}" - ) - except Exception as e: - self._handle_exceptions(e) + if not send_portfolio_invitation_remove_email(requestor=request.user, invitation=portfolio_invitation): + messages.warning( + request, f"Could not send email notification to {portfolio_invitation.email}" + ) + except Exception as e: + self._handle_exceptions(e) portfolio_invitation.delete() From c05cb1ef378c7ac7577fe11a5fb5675ecac9426c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 20 Feb 2025 15:42:38 -0500 Subject: [PATCH 26/28] removed thank you message, finished tests, linted --- .../commands/disclose_security_emails.py | 2 +- .../commands/master_domain_migrations.py | 8 ++--- .../populate_domain_updated_federal_agency.py | 2 +- .../templates/emails/portfolio_removal.txt | 3 -- src/registrar/tests/test_views_portfolio.py | 36 +++++++++++++++++-- src/registrar/views/portfolios.py | 4 +-- src/registrar/views/user_profile.py | 3 +- 7 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/registrar/management/commands/disclose_security_emails.py b/src/registrar/management/commands/disclose_security_emails.py index 62989e4c0..62456747a 100644 --- a/src/registrar/management/commands/disclose_security_emails.py +++ b/src/registrar/management/commands/disclose_security_emails.py @@ -1,4 +1,4 @@ -"""" +""" " Converts all ready and DNS needed domains with a non-default public contact to disclose their public contact. Created for Issue#1535 to resolve disclose issue of domains with missing security emails. diff --git a/src/registrar/management/commands/master_domain_migrations.py b/src/registrar/management/commands/master_domain_migrations.py index 7f702e047..2907add06 100644 --- a/src/registrar/management/commands/master_domain_migrations.py +++ b/src/registrar/management/commands/master_domain_migrations.py @@ -1,8 +1,8 @@ """Data migration: - 1 - generates a report of data integrity across all - transition domain related tables - 2 - allows users to run all migration scripts for - transition domain data +1 - generates a report of data integrity across all +transition domain related tables +2 - allows users to run all migration scripts for +transition domain data """ import logging diff --git a/src/registrar/management/commands/populate_domain_updated_federal_agency.py b/src/registrar/management/commands/populate_domain_updated_federal_agency.py index dd8ceb3b2..3fbf2792c 100644 --- a/src/registrar/management/commands/populate_domain_updated_federal_agency.py +++ b/src/registrar/management/commands/populate_domain_updated_federal_agency.py @@ -1,4 +1,4 @@ -"""" +""" " Data migration: Renaming deprecated Federal Agencies to their new updated names ie (U.S. Peace Corps to Peace Corps) within Domain Information and Domain Requests diff --git a/src/registrar/templates/emails/portfolio_removal.txt b/src/registrar/templates/emails/portfolio_removal.txt index 6de2190ae..779f37546 100644 --- a/src/registrar/templates/emails/portfolio_removal.txt +++ b/src/registrar/templates/emails/portfolio_removal.txt @@ -11,9 +11,6 @@ If you have questions or concerns, reach out to the person who removed you from organization, or reply to this email. -THANK YOU -.Gov helps the public identify official, trusted information. Thank you for using a .gov domain. - ---------------------------------------------------------------- The .gov team diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 1952d7c12..0f98b7eea 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2093,7 +2093,9 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") @patch("registrar.views.portfolios.send_portfolio_invitation_remove_email") - def test_portfolio_member_delete_view_manage_members_page_invitedmember(self, send_invited_member_removal, mock_send_removal_emails): + def test_portfolio_member_delete_view_manage_members_page_invitedmember( + self, send_invited_member_removal, mock_send_removal_emails + ): """Success state w/ deleting invited member on Manage Members page should redirect back to Members Table""" # I'm a user @@ -2156,7 +2158,10 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_manage_members_page_invitedadmin(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_invitation_remove_email") + def test_portfolio_member_delete_view_manage_members_page_invitedadmin( + self, send_invited_member_email, mock_send_removal_emails + ): """Success state w/ deleting invited admin on Manage Members page should redirect back to Members Table""" # I'm a user @@ -2171,6 +2176,7 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = True + send_invited_member_email.return_value = True # Invite an admin under same portfolio invited_member_email = "invited_member@example.com" @@ -2202,6 +2208,8 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): # assert send_portfolio_admin_removal_emails is called since invitation # is for an admin mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_invitation_remove_email is called + send_invited_member_email.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -2211,11 +2219,22 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + # Get the arguments passed to send_portfolio_invitation_remove_email + _, called_kwargs = send_invited_member_email.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["invitation"].email, invitation.email) + self.assertEqual(called_kwargs["invitation"].portfolio, invitation.portfolio) + @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_members", active=True) @patch("registrar.views.portfolios.send_portfolio_admin_removal_emails") - def test_portfolio_member_delete_view_manage_members_page_invitedadmin_email_fails(self, mock_send_removal_emails): + @patch("registrar.views.portfolios.send_portfolio_invitation_remove_email") + def test_portfolio_member_delete_view_manage_members_page_invitedadmin_email_fails( + self, send_invited_member_email, mock_send_removal_emails + ): """Success state w/ deleting invited admin on Manage Members page should redirect back to Members Table""" # I'm a user @@ -2230,6 +2249,7 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): ) mock_send_removal_emails.return_value = False + send_invited_member_email.return_value = False # Invite an admin under same portfolio invited_member_email = "invited_member@example.com" @@ -2261,6 +2281,8 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): # assert send_portfolio_admin_removal_emails is called since invitation # is for an admin mock_send_removal_emails.assert_called_once() + # assert that send_portfolio_invitation_remove_email is called + send_invited_member_email.assert_called_once() # Get the arguments passed to send_portfolio_admin_addition_emails _, called_kwargs = mock_send_removal_emails.call_args @@ -2270,6 +2292,14 @@ class TestPortfolioInvitedMemberDeleteView(WebTest): self.assertEqual(called_kwargs["requestor"], self.user) self.assertEqual(called_kwargs["portfolio"], self.portfolio) + # Get the arguments passed to send_portfolio_invitation_remove_email + _, called_kwargs = send_invited_member_email.call_args + + # Assert the email content + self.assertEqual(called_kwargs["requestor"], self.user) + self.assertEqual(called_kwargs["invitation"].email, invitation.email) + self.assertEqual(called_kwargs["invitation"].portfolio, invitation.portfolio) + class TestPortfolioMemberDomainsView(TestWithUser, WebTest): @classmethod diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index f53fe5f22..07adb3688 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -480,9 +480,7 @@ class PortfolioInvitedMemberDeleteView(PortfolioMemberPermission, View): ): messages.warning(self.request, "Could not send email notification to existing organization admins.") if not send_portfolio_invitation_remove_email(requestor=request.user, invitation=portfolio_invitation): - messages.warning( - request, f"Could not send email notification to {portfolio_invitation.email}" - ) + messages.warning(request, f"Could not send email notification to {portfolio_invitation.email}") except Exception as e: self._handle_exceptions(e) diff --git a/src/registrar/views/user_profile.py b/src/registrar/views/user_profile.py index 2012d12ab..41584bc82 100644 --- a/src/registrar/views/user_profile.py +++ b/src/registrar/views/user_profile.py @@ -1,5 +1,4 @@ -"""Views for a User Profile. -""" +"""Views for a User Profile.""" import logging From b4f64f6d0538ce4966032723377f5a249b90d878 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 20 Feb 2025 16:02:02 -0500 Subject: [PATCH 27/28] endline --- .../django/admin/domain_invitation_delete_confirmation.html | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html index 215bf5ada..72d02e0ab 100644 --- a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html +++ b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html @@ -14,3 +14,4 @@ {{ block.super }} {% endblock %} + From 30dd3817273c53510dbe9be41e0d7272e7d2c7c0 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 20 Feb 2025 16:11:04 -0500 Subject: [PATCH 28/28] fixed endlines --- .../django/admin/domain_invitation_delete_confirmation.html | 1 - .../django/admin/portfolio_invitation_delete_confirmation.html | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html index 72d02e0ab..215bf5ada 100644 --- a/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html +++ b/src/registrar/templates/django/admin/domain_invitation_delete_confirmation.html @@ -14,4 +14,3 @@ {{ block.super }} {% endblock %} - diff --git a/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html index 5aa01c33f..932822766 100644 --- a/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html +++ b/src/registrar/templates/django/admin/portfolio_invitation_delete_confirmation.html @@ -14,4 +14,4 @@ {{ block.super }} -{% endblock %} \ No newline at end of file +{% endblock %}