From d887a7514a1b77d298252a560fe4e0959689e01f Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 20 Feb 2025 11:21:06 -0600 Subject: [PATCH] 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): """