From f10cfe2c9bd143029af27270807619eab41be630 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:45:08 -0600 Subject: [PATCH 001/103] 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 002/103] 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 003/103] 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 004/103] 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 005/103] 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 006/103] 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 007/103] 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 008/103] 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 009/103] 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 010/103] 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 011/103] 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 012/103] 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 013/103] 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 014/103] 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 015/103] 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 6f3888f57aa97c223bd67957bb55f53bb27439fc Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 15:27:31 -0500 Subject: [PATCH 016/103] expanded row --- .../assets/src/js/getgov/table-members.js | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 75a7c29ac..5e93ecab8 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -87,7 +87,7 @@ export class MembersTable extends BaseTable { // generate html blocks for domains and permissions for the member let domainsHTML = this.generateDomainsHTML(num_domains, member.domain_names, member.domain_urls, member.action_url); - let permissionsHTML = this.generatePermissionsHTML(member.permissions, customTableOptions.UserPortfolioPermissionChoices); + let permissionsHTML = this.generatePermissionsHTML(member.is_admin, member.permissions, customTableOptions.UserPortfolioPermissionChoices); // domainsHTML block and permissionsHTML block need to be wrapped with hide/show toggle, Expand let showMoreButton = ''; @@ -257,10 +257,11 @@ export class MembersTable extends BaseTable { let domainsHTML = ''; // Only generate HTML if the member has one or more assigned domains + + domainsHTML += "
"; + domainsHTML += "

Domains assigned

"; if (num_domains > 0) { - domainsHTML += "
"; - domainsHTML += "

Domains assigned

"; - domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; + domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; domainsHTML += "
    "; // Display up to 6 domains with their URLs @@ -269,13 +270,15 @@ export class MembersTable extends BaseTable { } domainsHTML += "
"; - - // If there are more than 6 domains, display a "View assigned domains" link - domainsHTML += `

View assigned domains

`; - - domainsHTML += "
"; + } else { + domainsHTML += `

This member is assigned to 0 domains.

`; } + // If there are more than 6 domains, display a "View assigned domains" link + domainsHTML += `

View domains assigned

`; + + domainsHTML += "
"; + return domainsHTML; } @@ -387,40 +390,51 @@ export class MembersTable extends BaseTable { * - If no relevant permissions are found, the function returns a message stating that the user has no additional permissions. * - The resulting HTML always includes a header "Additional permissions for this member" and appends the relevant permission descriptions. */ - generatePermissionsHTML(member_permissions, UserPortfolioPermissionChoices) { + generatePermissionsHTML(is_admin, member_permissions, UserPortfolioPermissionChoices) { let permissionsHTML = ''; // Define shared classes across elements for easier refactoring - let sharedParagraphClasses = "font-body-xs text-base-dark margin-top-1 p--blockquote"; + let sharedParagraphClasses = "font-body-xs text-base-darker margin-top-1 p--blockquote"; + + // Member access + if (is_admin) { + permissionsHTML += `

Member access: Admin

`; + } else { + permissionsHTML += `

Member access: Basic

`; + } // Check domain-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS)) { - permissionsHTML += `

Domains: Can view all organization domains. Can manage domains they are assigned to and edit information about the domain (including DNS settings).

`; + permissionsHTML += `

Domains: Viewer

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS)) { - permissionsHTML += `

Domains: Can manage domains they are assigned to and edit information about the domain (including DNS settings).

`; + permissionsHTML += `

Domains: Viewer, limited

`; } // Check request-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_REQUESTS)) { - permissionsHTML += `

Domain requests: Can view all organization domain requests. Can create domain requests and modify their own requests.

`; + permissionsHTML += `

Domain requests: Creator

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS)) { - permissionsHTML += `

Domain requests (view-only): Can view all organization domain requests. Can't create or modify any domain requests.

`; + permissionsHTML += `

Domain requests: Viewer

`; + } else { + permissionsHTML += `

Domain requests: No access

`; } // Check member-related permissions if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_MEMBERS)) { - permissionsHTML += `

Members: Can manage members including inviting new members, removing current members, and assigning domains to members.

`; + permissionsHTML += `

Members: Manager

`; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MEMBERS)) { - permissionsHTML += `

Members (view-only): Can view all organizational members. Can't manage any members.

`; + permissionsHTML += `

Members: Viewer

`; + } else { + permissionsHTML += `

Members: No access

`; } // If no specific permissions are assigned, display a message indicating no additional permissions if (!permissionsHTML) { - permissionsHTML += `

No additional permissions: There are no additional permissions for this member.

`; + permissionsHTML += `

No additional permissions: There are no additional permissions for this member.

`; } // Add a permissions header and wrap the entire output in a container - permissionsHTML = `

Additional permissions for this member

${permissionsHTML}
`; + permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; return permissionsHTML; } From e751388533a40176dea8285d783b1078f7747989 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 10 Feb 2025 15:58:14 -0500 Subject: [PATCH 017/103] 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 72a2f6305104119e6f9c138118254e92918aa51f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:18:03 -0500 Subject: [PATCH 018/103] Member profile page --- .../src/js/getgov/portfolio-member-page.js | 2 +- src/registrar/forms/portfolio.py | 2 +- .../includes/member_domain_management.html | 4 +-- .../includes/member_permissions_summary.html | 16 ++++++------ .../templates/includes/summary_item.html | 26 ++++++++++++------- src/registrar/templates/portfolio_member.html | 6 ++--- src/registrar/tests/test_views_portfolio.py | 2 +- 7 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index c96677ebc..95723fc7e 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -128,7 +128,7 @@ export function initAddNewMemberPageListeners() { }); } else { // for admin users, the permissions are always the same - appendPermissionInContainer('Domains', 'Viewer, all', permissionDetailsContainer); + appendPermissionInContainer('Domains', 'Viewer', permissionDetailsContainer); appendPermissionInContainer('Domain requests', 'Creator', permissionDetailsContainer); appendPermissionInContainer('Members', 'Manager', permissionDetailsContainer); } diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 2725224f1..76a5032da 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -127,7 +127,7 @@ class BasePortfolioMemberForm(forms.ModelForm): domain_permissions = forms.ChoiceField( choices=[ (UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS.value, "Viewer, limited"), - (UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value, "Viewer, all"), + (UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS.value, "Viewer"), ], widget=forms.RadioSelect, required=False, diff --git a/src/registrar/templates/includes/member_domain_management.html b/src/registrar/templates/includes/member_domain_management.html index 1e5b29994..2adc3f950 100644 --- a/src/registrar/templates/includes/member_domain_management.html +++ b/src/registrar/templates/includes/member_domain_management.html @@ -1,6 +1,6 @@ -

Assigned domains

{% if domain_count > 0 %} +

Domains assigned

{{domain_count}}

{% else %} -

This member does not manage any domains.{% if manage_button %} To assign this member a domain, click "Manage".{% endif %}

+

This member does not manage any domains.{% if manage_button %} To assign this member a domain, click "Edit".{% endif %}

{% endif %} diff --git a/src/registrar/templates/includes/member_permissions_summary.html b/src/registrar/templates/includes/member_permissions_summary.html index 3a91d16f6..8e3dad528 100644 --- a/src/registrar/templates/includes/member_permissions_summary.html +++ b/src/registrar/templates/includes/member_permissions_summary.html @@ -9,25 +9,25 @@

Domains

{% if member_has_view_all_domains_portfolio_permission %} -

Viewer, all

+

Viewer: Can view all domains for the organization

{% else %} -

Viewer, limited

+

Viewer, limited: Can view only the domains they manage

{% endif %}

Domain requests

{% if member_has_edit_request_portfolio_permission %} -

Creator

+

Creator: Can view all domain requests for the organization and create requests

{% elif member_has_view_all_requests_portfolio_permission %} -

Viewer

+

Viewer: Can view all domain requests for the organization

{% else %} -

No access

+

No access: Cannot view or create domain requests

{% endif %}

Members

{% if member_has_edit_members_portfolio_permission %} -

Manager

+

Manager: Can view and manage all member permissions

{% elif member_has_view_members_portfolio_permission %} -

Viewer

+

Viewer: Can view all members permissions

{% else %} -

No access

+

No access: Cannot view member permissions

{% endif %} \ No newline at end of file diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index d062a7b4e..a091e5ab7 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -134,18 +134,26 @@ {% endif %} - {% if editable and edit_link %} + {% if editable and edit_link or view_button %}
- - - {% if manage_button %}Manage{% elif view_button %}View{% else %}Edit{% endif %} {{ title }} - + > + + {% if manage_button %} + Manage + {% elif editable and edit_link %} + Edit + {% else %} + View + {% endif %} + {{ title|default:"Page" }} +
{% endif %} + diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 709582e71..477599d49 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -101,11 +101,11 @@ Organization member {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% endif %} - {% comment %}view_button is passed below as true in all cases. This is because manage_button logic will trump view_button logic; ie. if manage_button is true, view_button will never be looked at{% endcomment %} + {% comment %}view_button is passed below as true in all cases. This is because editable logic will trump view_button logic; ie. if editable is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain management' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=True manage_button=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% endif %} diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 097aa1879..7a664a8c6 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1063,7 +1063,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Invited") self.assertContains(response, portfolio_invitation.email) self.assertContains(response, "Admin") - self.assertContains(response, "Viewer, all") + self.assertContains(response, "Viewer") self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( From 891d0f3dbb11bb13d434adb03f47feca1003a894 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:21:17 -0500 Subject: [PATCH 019/103] domain assignments page --- src/registrar/templates/includes/member_domains_edit_table.html | 2 +- src/registrar/templates/includes/member_domains_table.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/includes/member_domains_edit_table.html b/src/registrar/templates/includes/member_domains_edit_table.html index 0b8ff005a..ac05dc14f 100644 --- a/src/registrar/templates/includes/member_domains_edit_table.html +++ b/src/registrar/templates/includes/member_domains_edit_table.html @@ -100,7 +100,7 @@ > From 77711894b3061e36ba63867f8c99d150b075304a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:29:40 -0500 Subject: [PATCH 021/103] Add a new member page --- src/registrar/templates/portfolio_members_add_new.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index 464eaefce..c3a648bdc 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -55,7 +55,7 @@

What level of access would you like to grant this member?

-

Select one *

+

Select the level of access for this member. *

{% with add_class="usa-radio__input--tile" add_legend_class="usa-sr-only" %} {% input_with_errors form.role %} @@ -88,7 +88,7 @@ aria-controls="invite-member-modal" data-open-modal >Trigger invite member modal - + From 586d83adc172a058e288c1bba7207123ee734cbe Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:44:57 -0500 Subject: [PATCH 022/103] Fix unit tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 7a664a8c6..7d56e1650 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -952,7 +952,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' ) # Assert buttons and links within the page are correct @@ -1067,7 +1067,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Manage"' + response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' ) # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present From 1d54fe6b43e1716a20c39ee269408b86c13437a8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 16:50:54 -0500 Subject: [PATCH 023/103] fix unit tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 7d56e1650..c00892eca 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -952,7 +952,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' + response, 'This member does not manage any domains.' ) # Assert buttons and links within the page are correct @@ -1067,7 +1067,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Creator") self.assertContains(response, "Manager") self.assertContains( - response, 'This member does not manage any domains. To assign this member a domain, click "Edit"' + response, 'This member does not manage any domains.' ) # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present From c44c66ae96eebbff82ac1883b7fb79c69f4daf1a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:01:43 -0500 Subject: [PATCH 024/103] wip --- src/registrar/assets/src/js/getgov/table-members.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 5e93ecab8..e876ce0f9 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -108,7 +108,7 @@ export class MembersTable extends BaseTable { `; - showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; + showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; showMoreRow.classList.add('show-more-content'); showMoreRow.classList.add('display-none'); showMoreRow.id = unique_id; @@ -258,7 +258,7 @@ export class MembersTable extends BaseTable { // Only generate HTML if the member has one or more assigned domains - domainsHTML += "
"; + domainsHTML += "
"; domainsHTML += "

Domains assigned

"; if (num_domains > 0) { domainsHTML += `

This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

`; @@ -275,7 +275,7 @@ export class MembersTable extends BaseTable { } // If there are more than 6 domains, display a "View assigned domains" link - domainsHTML += `

View domains assigned

`; + domainsHTML += `

View domain assignments

`; domainsHTML += "
"; @@ -434,7 +434,7 @@ export class MembersTable extends BaseTable { } // Add a permissions header and wrap the entire output in a container - permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; + permissionsHTML = `

Member access and permissions

${permissionsHTML}
`; return permissionsHTML; } From d7c76b6d20e612c661a413eace0f150ec25550bd Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:03:34 -0500 Subject: [PATCH 025/103] Domain assignments with s --- src/registrar/templates/portfolio_member.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 477599d49..46e0cced6 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -103,9 +103,9 @@ Organization member {% comment %}view_button is passed below as true in all cases. This is because editable logic will trump view_button logic; ie. if editable is true, view_button will never be looked at{% endcomment %} {% if portfolio_permission %} - {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignments' domain_mgmt=True value=portfolio_permission.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% elif portfolio_invitation %} - {% include "includes/summary_item.html" with title='Domain assignment' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Domain assignments' domain_mgmt=True value=portfolio_invitation.get_managed_domains_count edit_link=domains_url editable=has_edit_members_portfolio_permission view_button=True %} {% endif %}
From fe1780fbb921138e88a2faea6a734b5a2681f30c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:07:56 -0500 Subject: [PATCH 026/103] wip --- src/registrar/templates/portfolio_member.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 46e0cced6..b270e03fb 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -65,7 +65,7 @@ Organization member
{% csrf_token %}
- Last active: + Last active: {% if member and member.last_login %} {{ member.last_login }} {% elif portfolio_invitation %} @@ -75,7 +75,7 @@ Organization member {% endif %}
- Full name: + Full name: {% if member %} {% if member.first_name or member.last_name %} {{ member.get_formatted_name }} @@ -87,7 +87,7 @@ Organization member {% endif %}
- Title or organization role: + Title or organization role: {% if member and member.title %} {{ member.title }} {% else %} From 5216cb4a53ca9e8dc4e4b1aeaed01c60f12ff4ac Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:15:28 -0500 Subject: [PATCH 027/103] wip --- src/registrar/templates/portfolio_members_add_new.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index c3a648bdc..715ad53ab 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -67,7 +67,7 @@ {% include "includes/member_basic_permissions.html" %} -

Domain management

+

Domain assignments

After you invite this person to your organization, you can assign domain management permissions on their member profile.

From e031d04fa5cdb9559a318dcce806b412b35ced6a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 17:23:18 -0500 Subject: [PATCH 028/103] lint --- src/registrar/tests/test_views_portfolio.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c00892eca..e45f65f6f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -951,9 +951,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Admin") self.assertContains(response, "Creator") self.assertContains(response, "Manager") - self.assertContains( - response, 'This member does not manage any domains.' - ) + self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present @@ -1066,9 +1064,7 @@ class TestPortfolio(WebTest): self.assertContains(response, "Viewer") self.assertContains(response, "Creator") self.assertContains(response, "Manager") - self.assertContains( - response, 'This member does not manage any domains.' - ) + self.assertContains(response, "This member does not manage any domains.") # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present From 819fd35e591f2db7af19191e792d0c73e1a9780e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 18:04:17 -0500 Subject: [PATCH 029/103] remove last active on member page --- src/registrar/templates/portfolio_member.html | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index b270e03fb..b4b54291b 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -65,16 +65,6 @@ Organization member
{% csrf_token %}
- Last active: - {% if member and member.last_login %} - {{ member.last_login }} - {% elif portfolio_invitation %} - Invited - {% else %} - ⎯ - {% endif %} -
- Full name: {% if member %} {% if member.first_name or member.last_name %} From aa253a85dad9602c82e84786a4b3efb2c560c4d1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 10 Feb 2025 18:09:13 -0500 Subject: [PATCH 030/103] wip --- src/registrar/assets/src/js/getgov/table-members.js | 2 +- src/registrar/tests/test_views_portfolio.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index e876ce0f9..bb2d8c185 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -108,7 +108,7 @@ export class MembersTable extends BaseTable { `; - showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; + showMoreRow.innerHTML = `
${domainsHTML} ${permissionsHTML}
`; showMoreRow.classList.add('show-more-content'); showMoreRow.classList.add('display-none'); showMoreRow.id = unique_id; diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index e45f65f6f..052155bb0 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -956,7 +956,7 @@ class TestPortfolio(WebTest): # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertContains(response, "sprite.svg#edit") # test that Manage link is present self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator @@ -1068,7 +1068,7 @@ class TestPortfolio(WebTest): # Assert buttons and links within the page are correct self.assertContains(response, "wrapper-delete-action") # test that 3 dot is present self.assertContains(response, "sprite.svg#edit") # test that Edit link is present - self.assertContains(response, "sprite.svg#settings") # test that Manage link is present + self.assertContains(response, "sprite.svg#edit") # test that Manage link is present self.assertNotContains(response, "sprite.svg#visibility") # test that View link is not present @less_console_noise_decorator From 40d5828d3e14ecbb23de52ea9eb651ba9edddbef Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 11 Feb 2025 09:30:13 -0600 Subject: [PATCH 031/103] 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 8d0f50f491e778471979fc02cbac86bf595600d2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 11 Feb 2025 12:48:40 -0500 Subject: [PATCH 032/103] add member modal --- src/registrar/templates/portfolio_members_add_new.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index 715ad53ab..d58508ac2 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -104,7 +104,7 @@

Invite this member to the organization?

-

Member information and permissions

+

Member access and permissions

Email

@@ -123,7 +123,7 @@
  • -
  • - {% translate "View on site" %} +
    • {% else %} @@ -30,18 +30,18 @@ {% endif %}
  • - {% translate "History" %} +
    • {% if opts.model_name == 'domainrequest' %}
  • - +
    • {% endif %} diff --git a/src/registrar/templates/admin/change_list_object_tools.html b/src/registrar/templates/admin/change_list_object_tools.html index 9a046b4bb..5ba88aa3a 100644 --- a/src/registrar/templates/admin/change_list_object_tools.html +++ b/src/registrar/templates/admin/change_list_object_tools.html @@ -5,9 +5,9 @@ {% if has_add_permission %}

    {% url cl.opts|admin_urlname:'add' as add_url %} - +

    {% endif %} {% endblock %} \ No newline at end of file From 20ff3541b43706d609d4349efd444a83f84dbee7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 19 Feb 2025 13:09:59 -0700 Subject: [PATCH 087/103] fix aria warning --- .../templates/admin/search_form.html | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 src/registrar/templates/admin/search_form.html diff --git a/src/registrar/templates/admin/search_form.html b/src/registrar/templates/admin/search_form.html new file mode 100644 index 000000000..c5fcf31f8 --- /dev/null +++ b/src/registrar/templates/admin/search_form.html @@ -0,0 +1,26 @@ +{% comment %} This is an override of the django search bar to add better accessibility compliance. +There are no blocks defined here, so we had to copy the code. +https://github.com/django/django/blob/main/django/contrib/admin/templates/admin/search_form.html +{% endcomment %} +{% load i18n static %} +{% if cl.search_fields %} +
    +
    +{% comment %} .gov override - removed for="searchbar" {% endcomment %} + + + +{% if show_result_count %} + {% blocktranslate count counter=cl.result_count %}{{ counter }} result{% plural %}{{ counter }} results{% endblocktranslate %} ({% if cl.show_full_result_count %}{% blocktranslate with full_result_count=cl.full_result_count %}{{ full_result_count }} total{% endblocktranslate %}{% else %}{% translate "Show all" %}{% endif %}) +{% endif %} +{% for pair in cl.params.items %} + {% if pair.0 != search_var %}{% endif %} +{% endfor %} +
    +{% if cl.search_help_text %} +
    +{% comment %} .gov override - added for="searchbar" {% endcomment %} + +{% endif %} +
    +{% endif %} \ No newline at end of file From 2cfce34edebaefaefa199549de245e9358dbe5d1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 19 Feb 2025 13:24:15 -0700 Subject: [PATCH 088/103] Add aria-label for table sort buttons --- src/registrar/templates/admin/change_list_results.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/admin/change_list_results.html b/src/registrar/templates/admin/change_list_results.html index f55ac7197..c5be04133 100644 --- a/src/registrar/templates/admin/change_list_results.html +++ b/src/registrar/templates/admin/change_list_results.html @@ -34,9 +34,9 @@ Load our custom filters to extract info from the django generated markup. {% if header.sortable %} {% if header.sort_priority > 0 %}
    - + {% if num_sorted_fields > 1 %}{{ header.sort_priority }}{% endif %} - +
    {% endif %} {% endif %} From 3bfb75fa3bd5a5309f36fd08636ca47dd3d4efa9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 19 Feb 2025 13:27:35 -0700 Subject: [PATCH 089/103] Update src/registrar/assets/src/sass/_theme/_admin.scss --- src/registrar/assets/src/sass/_theme/_admin.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/src/sass/_theme/_admin.scss b/src/registrar/assets/src/sass/_theme/_admin.scss index f136fb098..bd55bbfcb 100644 --- a/src/registrar/assets/src/sass/_theme/_admin.scss +++ b/src/registrar/assets/src/sass/_theme/_admin.scss @@ -995,4 +995,4 @@ ul.add-list-reset { #result_list > tbody tr > th > a { text-decoration: underline; -} \ No newline at end of file +} From eb02869df82bf363c70aadc63dcb564376fd5ddd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 20 Feb 2025 08:57:09 -0700 Subject: [PATCH 090/103] lint --- src/registrar/assets/src/js/getgov-admin/button-utils.js | 1 - src/registrar/templatetags/custom_filters.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/registrar/assets/src/js/getgov-admin/button-utils.js b/src/registrar/assets/src/js/getgov-admin/button-utils.js index 79d9091d6..e3746d289 100644 --- a/src/registrar/assets/src/js/getgov-admin/button-utils.js +++ b/src/registrar/assets/src/js/getgov-admin/button-utils.js @@ -7,7 +7,6 @@ export function initButtonLinks() { button.addEventListener('click', function() { // Equivalent to button.getAttribute("data-href") const href = this.dataset.href; - console.log(`in loop: ${href}`) if (href) { window.location.href = href; } diff --git a/src/registrar/templatetags/custom_filters.py b/src/registrar/templatetags/custom_filters.py index cf2abf447..e02a29e73 100644 --- a/src/registrar/templatetags/custom_filters.py +++ b/src/registrar/templatetags/custom_filters.py @@ -32,7 +32,7 @@ def extract_a_text(value): text_only = re.sub(text_pattern, "", content) # Clean up any extra whitespace return text_only.strip() - + return "" From d887a7514a1b77d298252a560fe4e0959689e01f Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 20 Feb 2025 11:21:06 -0600 Subject: [PATCH 091/103] 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 092/103] 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 093/103] 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 094/103] 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 095/103] 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 096/103] 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 097/103] 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 %} From 3795e1405885a56823bb775463b3c860516bd71c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 20 Feb 2025 22:08:11 -0500 Subject: [PATCH 098/103] refactor summary_item --- .../src/js/getgov/portfolio-member-page.js | 2 +- .../assets/src/sass/_theme/_accordions.scss | 2 + .../templates/includes/summary_item.html | 41 ++++++++++--------- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 0510c875d..96961e5dc 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -100,7 +100,7 @@ export function initAddNewMemberPageListeners() { const permissionSections = document.querySelectorAll(`#${permission_details_div_id} > h3`); permissionSections.forEach(section => { - // Find the

    element text + // Find the

    element text, strip out the '*' const sectionTitle = section.textContent.trim().replace(/\*$/, "") + ": "; // Find the associated radio buttons container (next fieldset) diff --git a/src/registrar/assets/src/sass/_theme/_accordions.scss b/src/registrar/assets/src/sass/_theme/_accordions.scss index 803a4510a..86b8779ab 100644 --- a/src/registrar/assets/src/sass/_theme/_accordions.scss +++ b/src/registrar/assets/src/sass/_theme/_accordions.scss @@ -47,6 +47,8 @@ } .usa-accordion--more-actions .usa-accordion__content { + // We need to match the height of the trigger button + // to align the 'popup' underneath top: 20px; &.top-28px { top: 28px; diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index a091e5ab7..0ce7910bb 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -134,25 +134,28 @@ {% endif %} - {% if editable and edit_link or view_button %} -
    - - - {% if manage_button %} - Manage - {% elif editable and edit_link %} - Edit - {% else %} - View - {% endif %} - {{ title|default:"Page" }} - -
    + {% comment %}We have conditions where an edit_link is set but editable can be true or false{% endcomment %} + {% if edit_link %} + {% if manage_button or editable or view_button %} +
    + + + {% if manage_button %} + Manage + {% elif editable %} + Edit + {% else %} + View + {% endif %} + {{ title|default:"Page" }} + +
    + {% endif %} {% endif %} From 699db826d08d6bff191d8927c8c0213b6f869c8c Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Feb 2025 10:59:26 -0700 Subject: [PATCH 099/103] Move button to other row to fix tab issue --- .../assets/src/js/getgov/table-members.js | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 29d140185..26a11da09 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -69,6 +69,7 @@ export class MembersTable extends BaseTable { const kebabHTML = customTableOptions.hasAdditionalActions ? generateKebabHTML('remove-member', unique_id, cancelInvitationButton, `Expand for more options for ${member.name}`): ''; const row = document.createElement('tr'); + row.classList.add('hide-td-borders'); let admin_tagHTML = ``; if (member.is_admin) admin_tagHTML = `Admin` @@ -96,20 +97,26 @@ export class MembersTable extends BaseTable { `; - showMoreRow.innerHTML = `
    ${domainsHTML} ${permissionsHTML}
    `; - showMoreRow.classList.add('show-more-content'); - showMoreRow.classList.add('display-none'); + showMoreRow.innerHTML = ` + + ${showMoreButton} + + + `; showMoreRow.id = unique_id; } row.innerHTML = ` - - ${member.member_display} ${admin_tagHTML} ${showMoreButton} + + ${member.member_display} ${admin_tagHTML} - + ${last_active.display_value} - +
    Admin` // generate html blocks for domains and permissions for the member - let domainsHTML = this.generateDomainsHTML(num_domains, member.domain_names, member.domain_urls, member.action_url); - let permissionsHTML = this.generatePermissionsHTML(member.is_admin, member.permissions, customTableOptions.UserPortfolioPermissionChoices); + let domainsHTML = this.generateDomainsHTML(num_domains, member.domain_names, member.domain_urls, member.action_url, unique_id); + let permissionsHTML = this.generatePermissionsHTML(member.is_admin, member.permissions, customTableOptions.UserPortfolioPermissionChoices, unique_id); // domainsHTML block and permissionsHTML block need to be wrapped with hide/show toggle, Expand let showMoreButton = ''; @@ -243,16 +243,18 @@ export class MembersTable extends BaseTable { * @param {number} num_domains - The number of domains the member is assigned to. * @param {Array} domain_names - An array of domain names. * @param {Array} domain_urls - An array of corresponding domain URLs. + * @param {Array} unique_id - A unique row id. * @returns {string} - A string of HTML displaying the domains assigned to the member. */ - generateDomainsHTML(num_domains, domain_names, domain_urls, action_url) { + generateDomainsHTML(num_domains, domain_names, domain_urls, action_url, unique_id) { // Initialize an empty string for the HTML let domainsHTML = ''; // Only generate HTML if the member has one or more assigned domains domainsHTML += "
    "; - domainsHTML += "

    Domains assigned

    "; + domainsHTML += `

    Domains assigned

    `; + domainsHTML += `
    ` if (num_domains > 0) { domainsHTML += `

    This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

    `; domainsHTML += "
    " domainsHTML += "
    "; return domainsHTML; @@ -368,7 +370,7 @@ export class MembersTable extends BaseTable { * - VIEW_ALL_REQUESTS * - EDIT_MEMBERS * - VIEW_MEMBERS - * + * @param {String} unique_id * @returns {string} - A string of HTML representing the user's additional permissions. * If the user has no specific permissions, it returns a default message * indicating no additional permissions. @@ -383,51 +385,51 @@ export class MembersTable extends BaseTable { * - If no relevant permissions are found, the function returns a message stating that the user has no additional permissions. * - The resulting HTML always includes a header "Additional permissions for this member" and appends the relevant permission descriptions. */ - generatePermissionsHTML(is_admin, member_permissions, UserPortfolioPermissionChoices) { - let permissionsHTML = ''; - - // Define shared classes across elements for easier refactoring - let sharedParagraphClasses = "font-body-xs text-base-darker margin-top-1 p--blockquote"; - - // Member access - if (is_admin) { - permissionsHTML += `

    Member access: Admin

    `; - } else { - permissionsHTML += `

    Member access: Basic

    `; - } - - // Check domain-related permissions + generatePermissionsHTML(is_admin, member_permissions, UserPortfolioPermissionChoices, unique_id) { + // 1. Role + const memberAccessValue = is_admin ? "Admin" : "Basic"; + + // 2. Domain access + let domainValue = "No access"; if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS)) { - permissionsHTML += `

    Domains: Viewer

    `; + domainValue = "Viewer"; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS)) { - permissionsHTML += `

    Domains: Viewer, limited

    `; + domainValue = "Viewer, limited"; } - - // Check request-related permissions + + // 3. Request access + let requestValue = "No access"; if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_REQUESTS)) { - permissionsHTML += `

    Domain requests: Creator

    `; + requestValue = "Creator"; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS)) { - permissionsHTML += `

    Domain requests: Viewer

    `; - } else { - permissionsHTML += `

    Domain requests: No access

    `; + requestValue = "Viewer"; } - - // Check member-related permissions + + // 4. Member access + let memberValue = "No access"; if (member_permissions.includes(UserPortfolioPermissionChoices.EDIT_MEMBERS)) { - permissionsHTML += `

    Members: Manager

    `; + memberValue = "Manager"; } else if (member_permissions.includes(UserPortfolioPermissionChoices.VIEW_MEMBERS)) { - permissionsHTML += `

    Members: Viewer

    `; - } else { - permissionsHTML += `

    Members: No access

    `; + memberValue = "Viewer"; } - - // If no specific permissions are assigned, display a message indicating no additional permissions - if (!permissionsHTML) { - permissionsHTML += `

    No additional permissions: There are no additional permissions for this member.

    `; - } - - // Add a permissions header and wrap the entire output in a container - permissionsHTML = `

    Member access and permissions

    ${permissionsHTML}
    `; + + // Helper function for faster element refactoring + const createPermissionItem = (label, value) => { + return `

    ${label}: ${value}

    `; + }; + const permissionsHTML = ` +
    +

    + Member access and permissions +

    +
    + ${createPermissionItem("Member access", memberAccessValue)} + ${createPermissionItem("Domains", domainValue)} + ${createPermissionItem("Domain requests", requestValue)} + ${createPermissionItem("Members", memberValue)} +
    +
    + `; return permissionsHTML; } From 6f96e70002d5fa051b54a54869560cf118fd0df4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Feb 2025 09:26:57 -0700 Subject: [PATCH 102/103] Update table-members.js --- src/registrar/assets/src/js/getgov/table-members.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 28a73f013..44589ffa7 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -254,7 +254,7 @@ export class MembersTable extends BaseTable { domainsHTML += "
    "; domainsHTML += `

    Domains assigned

    `; - domainsHTML += `
    ` + domainsHTML += `
    ` if (num_domains > 0) { domainsHTML += `

    This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

    `; domainsHTML += "
      "; From d5c1ce0de07b1fefd6ba4b17b9e2b66bc344f800 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Feb 2025 09:31:33 -0700 Subject: [PATCH 103/103] Fix screenreader issue --- .../assets/src/js/getgov/table-members.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index 44589ffa7..a13894e95 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -257,14 +257,19 @@ export class MembersTable extends BaseTable { domainsHTML += `
      ` if (num_domains > 0) { domainsHTML += `

      This member is assigned to ${num_domains} domain${num_domains > 1 ? 's' : ''}:

      `; - domainsHTML += "
        "; + if (num_domains > 1) { + domainsHTML += "
          "; - // Display up to 6 domains with their URLs - for (let i = 0; i < num_domains && i < 6; i++) { - domainsHTML += `
        • ${domain_names[i]}
          • `; + // Display up to 6 domains with their URLs + for (let i = 0; i < num_domains && i < 6; i++) { + domainsHTML += `
        • ${domain_names[i]}
          • `; + } + + domainsHTML += "
        "; + } else { + // We don't display this in a list for better screenreader support, when only one item exists. + domainsHTML += `${domain_names[0]}`; } - - domainsHTML += "
      "; } else { domainsHTML += `

      This member is assigned to 0 domains.

      `; }