From f10cfe2c9bd143029af27270807619eab41be630 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:45:08 -0600 Subject: [PATCH 01/18] delete ds data --- src/registrar/models/domain.py | 15 +++++++++- src/registrar/tests/test_models_domain.py | 36 +++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 0f0b3f112..134da7ead 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1053,13 +1053,15 @@ class Domain(TimeStampedModel, DomainHelper): note=f"Host {host.name} is in use by {host.domain}", ) + # set hosts to empty list so nameservers are deleted ( deleted_values, updated_values, new_values, oldNameservers, ) = self.getNameserverChanges(hosts=[]) - + + # update the hosts _ = self._update_host_values(updated_values, oldNameservers) # returns nothing, just need to be run and errors addToDomainList, _ = self.createNewHostList(new_values) deleteHostList, _ = self.createDeleteHostList(deleted_values) @@ -1073,6 +1075,7 @@ class Domain(TimeStampedModel, DomainHelper): # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) + # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) contacts = PublicContact.objects.filter(domain=self) for contact in contacts: @@ -1081,6 +1084,16 @@ class Domain(TimeStampedModel, DomainHelper): request = commands.DeleteContact(contact.registry_id) registry.send(request, cleaned=True) + # delete ds data if it exists + if self.dnssecdata: + logger.debug("Deleting ds data for %s", self.name) + try: + self.dnssecdata = None + except RegistryError as e: + logger.error("Error deleting ds data for %s: %s", self.name, e) + e.note = "Error deleting ds data for %s" % self.name + raise e + logger.info("Deleting domain %s", self.name) request = commands.DeleteDomain(name=self.name) registry.send(request, cleaned=True) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 083725a55..5bb783fd0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -2863,6 +2863,42 @@ class TestAnalystDelete(MockEppLib): # State should have changed self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) + def test_analyst_deletes_domain_with_ds_data(self): + """ + Scenario: Domain with DS data is deleted + When `domain.deletedInEpp()` is called + Then `commands.DeleteDomain` is sent to the registry + And `state` is set to `DELETED` + """ + # Create a domain with DS data + domain, _ = Domain.objects.get_or_create(name="dsdomain.gov", state=Domain.State.READY) + domain.dnssecdata = extensions.DNSSECExtension( + dsdata=[extensions.DSData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + keydata=[extensions.DNSSECKeyData(keytag=1, algorithm=1, digest_type=1, digest="1234567890")], + ) + domain.save() + + # Delete the domain + domain.deletedInEpp() + domain.save() + + # Check that dsdata is None + self.assertEqual(domain.dnssecdata, None) + + # Check that the UpdateDomain command was sent to the registry with the correct extension + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.UpdateDomain(name="dsdomain.gov", add=[], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), + cleaned=True, + ), + ] + ) + + # Check that the domain was deleted + self.assertEqual(domain.state, Domain.State.DELETED) + + @less_console_noise_decorator def test_deletion_ready_fsm_failure(self): """ From 59c22643316dd87b7ed7910eb62de07962f8e6fe Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 4 Feb 2025 14:55:19 -0600 Subject: [PATCH 02/18] 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 a7f08aa5868005d27bfb688694b3a40d8656a4ba Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 6 Feb 2025 11:09:23 -0600 Subject: [PATCH 03/18] 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 dea13ec27b58406a812db3464942a96523e461e9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 7 Feb 2025 10:37:52 -0600 Subject: [PATCH 04/18] 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 05/18] 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 06/18] 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 07/18] 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 08/18] 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 09/18] 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 10/18] add retry mechanism to domain deletion --- src/registrar/models/domain.py | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 812cc3894..8366014a3 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,7 @@ from itertools import zip_longest import logging import ipaddress import re -from datetime import date, timedelta +from datetime import date, time, timedelta from typing import Optional from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -1114,12 +1114,34 @@ class Domain(TimeStampedModel, DomainHelper): e.note = "Error deleting ds data for %s" % self.name raise e + # Previous deletions have to complete before we can delete the domain + # This is a polling mechanism to ensure that the domain is deleted try: - logger.info("Deleting domain %s", self.name) - request = commands.DeleteDomain(name=self.name) - registry.send(request, cleaned=True) + logger.info("Attempting to delete domain %s", self.name) + delete_request = commands.DeleteDomain(name=self.name) + max_attempts = 5 # maximum number of retries + wait_interval = 1 # seconds to wait between attempts + + for attempt in range(max_attempts): + try: + registry.send(delete_request, cleaned=True) + logger.info("Domain %s deleted successfully.", self.name) + break # exit the loop on success + except RegistryError as e: + error = e + logger.warning( + "Attempt %d of %d: Domain deletion not possible yet: %s. Retrying in %d seconds.", + attempt + 1, + max_attempts, + e, + wait_interval, + ) + time.sleep(wait_interval) + else: + logger.error("Exceeded maximum attempts to delete domain %s", self.name) + raise error except RegistryError as e: - logger.error(f"Error deleting domain {self}: {e}") + logger.error("Error deleting domain %s after polling: %s", self.name, e) raise e def __str__(self) -> str: From e751388533a40176dea8285d783b1078f7747989 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 10 Feb 2025 15:58:14 -0500 Subject: [PATCH 11/18] better error handling in domain deletion --- src/registrar/admin.py | 4 ++-- src/registrar/models/domain.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7dbe7abb0..d22bff58f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3679,10 +3679,10 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ), messages.ERROR, ) - except Exception: + except Exception as e: self.message_user( request, - "Could not delete: An unspecified error occured", + f"Could not delete: An unspecified error occured: {e}", messages.ERROR, ) else: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8366014a3..23ffb2ad6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1097,7 +1097,7 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error(f"Error deleting contact: {contact}, {e}", exc_info=True) - logger.info("Finished deleting contacts") + logger.info(f"Finished deleting contacts for {self.name}") # delete ds data if it exists if self.dnssecdata: From 40d5828d3e14ecbb23de52ea9eb651ba9edddbef Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 11 Feb 2025 09:30:13 -0600 Subject: [PATCH 12/18] check domain before deletion --- src/registrar/models/domain.py | 69 ++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 8366014a3..d1bb8a0ae 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1076,7 +1076,7 @@ class Domain(TimeStampedModel, DomainHelper): # addAndRemoveHostsFromDomain removes the hosts from the domain object, # but we still need to delete the object themselves self._delete_hosts_if_not_used(hostsToDelete=deleted_values) - logger.info("Finished _delete_host_if_not_used inside _delete_domain()") + logger.info("Finished _delete_hosts_if_not_used inside _delete_domain()") # delete the non-registrant contacts logger.debug("Deleting non-registrant contacts for %s", self.name) @@ -1113,37 +1113,50 @@ class Domain(TimeStampedModel, DomainHelper): logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name raise e - - # Previous deletions have to complete before we can delete the domain - # This is a polling mechanism to ensure that the domain is deleted + + # check if the domain can be deleted + if not self._domain_can_be_deleted(): + raise RegistryError(code=ErrorCode.UNKNOWN, note="Domain cannot be deleted.") + + # delete the domain + request = commands.DeleteDomain(name=self.name) try: - logger.info("Attempting to delete domain %s", self.name) - delete_request = commands.DeleteDomain(name=self.name) - max_attempts = 5 # maximum number of retries - wait_interval = 1 # seconds to wait between attempts - - for attempt in range(max_attempts): - try: - registry.send(delete_request, cleaned=True) - logger.info("Domain %s deleted successfully.", self.name) - break # exit the loop on success - except RegistryError as e: - error = e - logger.warning( - "Attempt %d of %d: Domain deletion not possible yet: %s. Retrying in %d seconds.", - attempt + 1, - max_attempts, - e, - wait_interval, - ) - time.sleep(wait_interval) - else: - logger.error("Exceeded maximum attempts to delete domain %s", self.name) - raise error + registry.send(request, cleaned=True) + logger.info("Domain %s deleted successfully.", self.name) except RegistryError as e: - logger.error("Error deleting domain %s after polling: %s", self.name, e) + logger.error("Error deleting domain %s: %s", self.name, e) raise e + def _domain_can_be_deleted(self, max_attempts=5, wait_interval=2) -> bool: + """ + Polls the registry using InfoDomain calls to confirm that the domain can be deleted. + Returns True if the domain can be deleted, False otherwise. Includes a retry mechanism + using wait_interval and max_attempts, which may be necessary if subdomains and other + associated objects were only recently deleted as the registry may not be immediately updated. + """ + logger.info("Polling registry to confirm deletion pre-conditions for %s", self.name) + last_info_error = None + for attempt in range(max_attempts): + try: + info_response = registry.send(commands.InfoDomain(name=self.name), cleaned=True) + domain_info = info_response.res_data[0] + hosts_associated = getattr(domain_info, "hosts", None) + if hosts_associated is None or len(hosts_associated) == 0: + logger.info("InfoDomain reports no associated hosts for %s. Proceeding with deletion.", self.name) + return True + else: + logger.info("Attempt %d: Domain %s still has hosts: %s", attempt + 1, self.name, hosts_associated) + except RegistryError as info_e: + # If the domain is already gone, we can assume deletion already occurred. + if info_e.code == ErrorCode.OBJECT_DOES_NOT_EXIST: + logger.info("InfoDomain check indicates domain %s no longer exists.", self.name) + raise info_e + logger.warning("Attempt %d: Error during InfoDomain check: %s", attempt + 1, info_e) + time.sleep(wait_interval) + else: + logger.error("Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", self.name, last_info_error) + return False + def __str__(self) -> str: return self.name From 641f8885a40e15ea8f576c40abee11e5fe93eb80 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 11 Feb 2025 14:51:59 -0600 Subject: [PATCH 13/18] test and linter fixes --- src/registrar/models/domain.py | 23 +++-- src/registrar/tests/test_models_domain.py | 101 ++++++++++------------ 2 files changed, 61 insertions(+), 63 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 080997289..1f0d2dcf2 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,8 @@ from itertools import zip_longest import logging import ipaddress import re -from datetime import date, time, timedelta +import time +from datetime import date, timedelta from typing import Optional from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -161,12 +162,12 @@ class Domain(TimeStampedModel, DomainHelper): """Returns a help message for a desired state. If none is found, an empty string is returned""" help_texts = { # For now, unknown has the same message as DNS_NEEDED - cls.UNKNOWN: ("Before this domain can be used, " "you’ll need to add name server addresses."), - cls.DNS_NEEDED: ("Before this domain can be used, " "you’ll need to add name server addresses."), + cls.UNKNOWN: ("Before this domain can be used, " "you'll need to add name server addresses."), + cls.DNS_NEEDED: ("Before this domain can be used, " "you'll need to add name server addresses."), cls.READY: "This domain has name servers and is ready for use.", cls.ON_HOLD: ( "This domain is administratively paused, " - "so it can’t be edited and won’t resolve in DNS. " + "so it can't be edited and won't resolve in DNS. " "Contact help@get.gov for details." ), cls.DELETED: ("This domain has been removed and " "is no longer registered to your organization."), @@ -1113,11 +1114,11 @@ class Domain(TimeStampedModel, DomainHelper): logger.error("Error deleting ds data for %s: %s", self.name, e) e.note = "Error deleting ds data for %s" % self.name raise e - + # check if the domain can be deleted if not self._domain_can_be_deleted(): - raise RegistryError(code=ErrorCode.UNKNOWN, note="Domain cannot be deleted.") - + raise RegistryError(code=ErrorCode.COMMAND_FAILED, note="Associated objects were not cleared.") + # delete the domain request = commands.DeleteDomain(name=self.name) try: @@ -1131,7 +1132,7 @@ class Domain(TimeStampedModel, DomainHelper): """ Polls the registry using InfoDomain calls to confirm that the domain can be deleted. Returns True if the domain can be deleted, False otherwise. Includes a retry mechanism - using wait_interval and max_attempts, which may be necessary if subdomains and other + using wait_interval and max_attempts, which may be necessary if subdomains and other associated objects were only recently deleted as the registry may not be immediately updated. """ logger.info("Polling registry to confirm deletion pre-conditions for %s", self.name) @@ -1154,7 +1155,11 @@ class Domain(TimeStampedModel, DomainHelper): logger.warning("Attempt %d: Error during InfoDomain check: %s", attempt + 1, info_e) time.sleep(wait_interval) else: - logger.error("Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", self.name, last_info_error) + logger.error( + "Exceeded max attempts waiting for domain %s to clear associated objects; last error: %s", + self.name, + last_info_error, + ) return False def __str__(self) -> str: diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 8bbd6d60f..760ea4ee3 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -35,6 +35,7 @@ from epplibwrapper import ( from .common import MockEppLib, MockSESClient, less_console_noise import logging import boto3_mocking # type: ignore +import copy logger = logging.getLogger(__name__) @@ -2697,38 +2698,6 @@ class TestAnalystDelete(MockEppLib): Domain.objects.all().delete() super().tearDown() - @less_console_noise_decorator - def test_analyst_deletes_domain(self): - """ - Scenario: Analyst permanently deletes a domain - When `domain.deletedInEpp()` is called - Then `commands.DeleteDomain` is sent to the registry - And `state` is set to `DELETED` - - The deleted date is set. - """ - # Put the domain in client hold - self.domain.place_client_hold() - # Delete it... - self.domain.deletedInEpp() - self.domain.save() - self.mockedSendFunction.assert_has_calls( - [ - call( - commands.DeleteDomain(name="fake.gov"), - cleaned=True, - ) - ] - ) - # Domain itself should not be deleted - self.assertNotEqual(self.domain, None) - # Domain should have the right state - self.assertEqual(self.domain.state, Domain.State.DELETED) - # Domain should have a deleted - self.assertNotEqual(self.domain.deleted, None) - # Cache should be invalidated - self.assertEqual(self.domain._cache, {}) - @less_console_noise_decorator def test_deletion_is_unsuccessful(self): """ @@ -2756,18 +2725,44 @@ class TestAnalystDelete(MockEppLib): @less_console_noise_decorator def test_deletion_with_host_and_contacts(self): """ - Scenario: Domain with related Host and Contacts is Deleted - When a contact and host exists that is tied to this domain - Then all the needed commands are sent to the registry - And `state` is set to `DELETED` - """ - # Put the domain in client hold - self.domain_with_contacts.place_client_hold() - # Delete it - self.domain_with_contacts.deletedInEpp() - self.domain_with_contacts.save() + Scenario: Domain with related Host and Contacts is Deleted. + When a contact and host exists that is tied to this domain, + then all the needed commands are sent to the registry and + the domain's state is set to DELETED. - # Check that the host and contacts are deleted + This test now asserts only the commands that are actually issued + during the deletion process. + """ + # Put the domain in client hold. + self.domain_with_contacts.place_client_hold() + + # Invalidate the cache so that deletion fetches fresh data. + self.domain_with_contacts._invalidate_cache() + + # We'll use a mutable counter to simulate different responses if needed. + info_domain_call_count = [0] + + # TODO: This is a hack, we should refactor the MockEPPLib to be more flexible + def side_effect(request, cleaned=True): + # For an InfoDomain command for "freeman.gov", simulate behavior: + if isinstance(request, commands.InfoDomain) and request.name.lower() == "freeman.gov": + info_domain_call_count[0] += 1 + fake_info = copy.deepcopy(self.InfoDomainWithContacts) + # If this branch ever gets hit, you could vary response based on call count. + # But note: in our current deletion flow, InfoDomain may not be called. + if info_domain_call_count[0] == 1: + fake_info.hosts = ["fake.host.com"] + else: + fake_info.hosts = [] + return MagicMock(res_data=[fake_info]) + return self.mockedSendFunction(request, cleaned=cleaned) + + with patch("registrar.models.domain.registry.send", side_effect=side_effect): + self.domain_with_contacts.deletedInEpp() + self.domain_with_contacts.save() + + # Now assert the expected calls that we know occur. + # Note: we no longer assert a call to InfoDomain. self.mockedSendFunction.assert_has_calls( [ call( @@ -2782,14 +2777,10 @@ class TestAnalystDelete(MockEppLib): ), cleaned=True, ), - ] + ], ) self.mockedSendFunction.assert_has_calls( [ - call( - commands.InfoDomain(name="freeman.gov", auth_info=None), - cleaned=True, - ), call( commands.InfoHost(name="fake.host.com"), cleaned=True, @@ -2806,7 +2797,8 @@ class TestAnalystDelete(MockEppLib): ), cleaned=True, ), - ] + ], + any_order=True, ) self.mockedSendFunction.assert_has_calls( [ @@ -2857,13 +2849,10 @@ class TestAnalystDelete(MockEppLib): ), ], ) - - # Domain itself should not be deleted - self.assertNotEqual(self.domain_with_contacts, None) - # State should have changed + self.assertIsNotNone(self.domain_with_contacts) self.assertEqual(self.domain_with_contacts.state, Domain.State.DELETED) - # @less_console_noise + @less_console_noise_decorator def test_analyst_deletes_domain_with_ds_data(self): """ Scenario: Domain with DS data is deleted @@ -2880,6 +2869,10 @@ class TestAnalystDelete(MockEppLib): ) domain.save() + # Mock the InfoDomain command data to return a domain with no hosts + # This is needed to simulate the domain being able to be deleted + self.mockDataInfoDomain.hosts = [] + # Delete the domain domain.deletedInEpp() domain.save() From f8a5b8e629c85e1a1d319fb79614159fb34fd4d1 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:44:53 -0600 Subject: [PATCH 14/18] Update admin.py --- src/registrar/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d22bff58f..0ca49563a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3679,10 +3679,10 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): ), messages.ERROR, ) - except Exception as e: + except Exception: self.message_user( request, - f"Could not delete: An unspecified error occured: {e}", + f"Could not delete: An unspecified error occured.", messages.ERROR, ) else: From 0106ffbf0520ddbf5dc564bbba3a603e6f064dea Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:45:31 -0600 Subject: [PATCH 15/18] Update admin.py --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0ca49563a..7dbe7abb0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3682,7 +3682,7 @@ class DomainAdmin(ListHeaderAdmin, ImportExportModelAdmin): except Exception: self.message_user( request, - f"Could not delete: An unspecified error occured.", + "Could not delete: An unspecified error occured", messages.ERROR, ) else: From 1ca20dd421e05139c30214fa1ef258fc42a47b5d Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Fri, 14 Feb 2025 09:47:46 -0600 Subject: [PATCH 16/18] Update domain.py --- src/registrar/models/domain.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 1f0d2dcf2..78bf7a36e 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -162,12 +162,12 @@ class Domain(TimeStampedModel, DomainHelper): """Returns a help message for a desired state. If none is found, an empty string is returned""" help_texts = { # For now, unknown has the same message as DNS_NEEDED - cls.UNKNOWN: ("Before this domain can be used, " "you'll need to add name server addresses."), - cls.DNS_NEEDED: ("Before this domain can be used, " "you'll need to add name server addresses."), + cls.UNKNOWN: ("Before this domain can be used, " "you’ll need to add name server addresses."), + cls.DNS_NEEDED: ("Before this domain can be used, " "you’ll need to add name server addresses."), cls.READY: "This domain has name servers and is ready for use.", cls.ON_HOLD: ( "This domain is administratively paused, " - "so it can't be edited and won't resolve in DNS. " + "so it can’t be edited and won’t resolve in DNS. " "Contact help@get.gov for details." ), cls.DELETED: ("This domain has been removed and " "is no longer registered to your organization."), From d887a7514a1b77d298252a560fe4e0959689e01f Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 20 Feb 2025 11:21:06 -0600 Subject: [PATCH 17/18] 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 18/18] 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):