From 5cd81f70cd602e6a72137b8134a65b3060187ea5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 21 Sep 2023 20:41:05 +0000 Subject: [PATCH 01/21] Bump cryptography from 41.0.3 to 41.0.4 in /src Bumps [cryptography](https://github.com/pyca/cryptography) from 41.0.3 to 41.0.4. - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](https://github.com/pyca/cryptography/compare/41.0.3...41.0.4) --- updated-dependencies: - dependency-name: cryptography dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- src/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/requirements.txt b/src/requirements.txt index 52ded59fc..a5972c4dc 100644 --- a/src/requirements.txt +++ b/src/requirements.txt @@ -7,7 +7,7 @@ certifi==2023.7.22 ; python_version >= '3.6' cfenv==0.5.3 cffi==1.15.1 charset-normalizer==3.1.0 ; python_full_version >= '3.7.0' -cryptography==41.0.3 ; python_version >= '3.7' +cryptography==41.0.4 ; python_version >= '3.7' defusedxml==0.7.1 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4' dj-database-url==2.0.0 dj-email-url==1.0.6 From a3905625ccfab6d4f8ff86078f8aa16a067b1b3a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 21 Sep 2023 17:01:01 -0400 Subject: [PATCH 02/21] Revise fetch cache to fetch only what's needed, revise unit tests --- src/registrar/models/domain.py | 119 +++++++++++----------- src/registrar/tests/test_models_domain.py | 21 ++-- 2 files changed, 74 insertions(+), 66 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 13405d9bb..23dc69a7c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -960,75 +960,38 @@ class Domain(TimeStampedModel, DomainHelper): # statuses can just be a list no need to keep the epp object if "statuses" in cleaned.keys(): cleaned["statuses"] = [status.state for status in cleaned["statuses"]] + + # Capture and store old hosts and contacts from cache if they exist + old_cache_hosts = self._cache.get("hosts") + old_cache_contacts = self._cache.get("contacts") + # get contact info, if there are any if ( - # fetch_contacts and - "_contacts" in cleaned + fetch_contacts + and "_contacts" in cleaned and isinstance(cleaned["_contacts"], list) and len(cleaned["_contacts"]) ): - cleaned["contacts"] = [] - for domainContact in cleaned["_contacts"]: - # we do not use _get_or_create_* because we expect the object we - # just asked the registry for still exists -- - # if not, that's a problem - - # TODO- discuss-should we check if contact is in public contacts - # and add it if not- this is really to keep in mine the transisiton - req = commands.InfoContact(id=domainContact.contact) - data = registry.send(req, cleaned=True).res_data[0] - - # extract properties from response - # (Ellipsis is used to mean "null") - # convert this to use PublicContactInstead - contact = { - "id": domainContact.contact, - "type": domainContact.type, - "auth_info": getattr(data, "auth_info", ...), - "cr_date": getattr(data, "cr_date", ...), - "disclose": getattr(data, "disclose", ...), - "email": getattr(data, "email", ...), - "fax": getattr(data, "fax", ...), - "postal_info": getattr(data, "postal_info", ...), - "statuses": getattr(data, "statuses", ...), - "tr_date": getattr(data, "tr_date", ...), - "up_date": getattr(data, "up_date", ...), - "voice": getattr(data, "voice", ...), - } - - cleaned["contacts"].append( - {k: v for k, v in contact.items() if v is not ...} - ) + cleaned["contacts"] = self._fetch_contacts(cleaned["_contacts"]) + # We're only getting contacts, so retain the old + # hosts that existed in cache (if they existed) + # and pass them along. + if old_cache_hosts is not None: + cleaned["hosts"] = old_cache_hosts # get nameserver info, if there are any if ( - # fetch_hosts and - "_hosts" in cleaned + fetch_hosts + and "_hosts" in cleaned and isinstance(cleaned["_hosts"], list) and len(cleaned["_hosts"]) ): - # TODO- add elif in cache set it to be the old cache value - # no point in removing - cleaned["hosts"] = [] - for name in cleaned["_hosts"]: - # we do not use _get_or_create_* because we expect the object we - # just asked the registry for still exists -- - # if not, that's a problem - req = commands.InfoHost(name=name) - data = registry.send(req, cleaned=True).res_data[0] - # extract properties from response - # (Ellipsis is used to mean "null") - host = { - "name": name, - "addrs": getattr(data, "addrs", ...), - "cr_date": getattr(data, "cr_date", ...), - "statuses": getattr(data, "statuses", ...), - "tr_date": getattr(data, "tr_date", ...), - "up_date": getattr(data, "up_date", ...), - } - cleaned["hosts"].append( - {k: v for k, v in host.items() if v is not ...} - ) + cleaned["hosts"] = self._fetch_hosts(cleaned["_hosts"]) + # We're only getting hosts, so retain the old + # contacts that existed in cache (if they existed) + # and pass them along. + if old_cache_contacts is not None: + cleaned["contacts"] = old_cache_contacts # replace the prior cache with new data self._cache = cleaned @@ -1036,6 +999,46 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error(e) + def _fetch_contacts(self, contact_data): + """Fetch contact info.""" + contacts = [] + for domainContact in contact_data: + req = commands.InfoContact(id=domainContact.contact) + data = registry.send(req, cleaned=True).res_data[0] + contact = { + "id": domainContact.contact, + "type": domainContact.type, + "auth_info": getattr(data, "auth_info", ...), + "cr_date": getattr(data, "cr_date", ...), + "disclose": getattr(data, "disclose", ...), + "email": getattr(data, "email", ...), + "fax": getattr(data, "fax", ...), + "postal_info": getattr(data, "postal_info", ...), + "statuses": getattr(data, "statuses", ...), + "tr_date": getattr(data, "tr_date", ...), + "up_date": getattr(data, "up_date", ...), + "voice": getattr(data, "voice", ...), + } + contacts.append({k: v for k, v in contact.items() if v is not ...}) + return contacts + + def _fetch_hosts(self, host_data): + """Fetch host info.""" + hosts = [] + for name in host_data: + req = commands.InfoHost(name=name) + data = registry.send(req, cleaned=True).res_data[0] + host = { + "name": name, + "addrs": getattr(data, "addrs", ...), + "cr_date": getattr(data, "cr_date", ...), + "statuses": getattr(data, "statuses", ...), + "tr_date": getattr(data, "tr_date", ...), + "up_date": getattr(data, "up_date", ...), + } + hosts.append({k: v for k, v in host.items() if v is not ...}) + return hosts + def _invalidate_cache(self): """Remove cache data when updates are made.""" self._cache = {} diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index d35b0ba96..3280ba100 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -49,8 +49,6 @@ class TestDomainCache(MockEppLib): commands.InfoDomain(name="igorville.gov", auth_info=None), cleaned=True, ), - call(commands.InfoContact(id="123", auth_info=None), cleaned=True), - call(commands.InfoHost(name="fake.host.com"), cleaned=True), ], any_order=False, # Ensure calls are in the specified order ) @@ -72,8 +70,6 @@ class TestDomainCache(MockEppLib): call( commands.InfoDomain(name="igorville.gov", auth_info=None), cleaned=True ), - call(commands.InfoContact(id="123", auth_info=None), cleaned=True), - call(commands.InfoHost(name="fake.host.com"), cleaned=True), ] self.mockedSendFunction.assert_has_calls(expectedCalls) @@ -108,6 +104,19 @@ class TestDomainCache(MockEppLib): # 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 host + domain._get_property("hosts") + 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 tearDown(self) -> None: Domain.objects.all().delete() @@ -168,8 +177,6 @@ class TestDomainCreation(MockEppLib): commands.InfoDomain(name="beef-tongue.gov", auth_info=None), cleaned=True, ), - call(commands.InfoContact(id="123", auth_info=None), cleaned=True), - call(commands.InfoHost(name="fake.host.com"), cleaned=True), ], any_order=False, # Ensure calls are in the specified order ) @@ -219,8 +226,6 @@ class TestDomainStatuses(MockEppLib): commands.InfoDomain(name="chicken-liver.gov", auth_info=None), cleaned=True, ), - call(commands.InfoContact(id="123", auth_info=None), cleaned=True), - call(commands.InfoHost(name="fake.host.com"), cleaned=True), ], any_order=False, # Ensure calls are in the specified order ) From c6baf9c98bcc999e12de5c8ec3bbba044544f82f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 27 Sep 2023 14:45:33 -0600 Subject: [PATCH 03/21] Preliminary changes --- src/registrar/admin.py | 43 ++++++- src/registrar/models/domain.py | 47 +++++--- src/registrar/models/domain_application.py | 8 +- src/registrar/tests/common.py | 7 ++ src/registrar/tests/test_models_domain.py | 128 ++++++++++++++++++--- 5 files changed, 197 insertions(+), 36 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e99e767bd..6ec27a085 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -6,11 +6,12 @@ from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.contenttypes.models import ContentType from django.http.response import HttpResponseRedirect from django.urls import reverse +from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models.utility.admin_sort_fields import AdminSortFields from . import models from auditlog.models import LogEntry # type: ignore from auditlog.admin import LogEntryAdmin # type: ignore - +from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) @@ -717,10 +718,46 @@ class DomainAdmin(ListHeaderAdmin): def do_delete_domain(self, request, obj): try: - obj.deleted() + obj.deletedInEpp() obj.save() - except Exception as err: + except RegistryError as err: + if err.is_connection_error(): + self.message_user( + request, + "Error connecting to the registry", + messages.ERROR, + ) + elif err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: + self.message_user( + request, + "Error deleting this Domain: " + f"Cannot delete Domain when in status {obj.status}", + messages.ERROR, + ) + elif err.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + self.message_user( + request, + "Error deleting this Domain: " + f" This subdomain is being used as a hostname on another domain", + messages.ERROR, + ) + elif err.code: + self.message_user( + request, + f"Error deleting this Domain: {err}", + messages.ERROR, + ) + else: + # all other type error messages, display the error + self.message_user(request, err, messages.ERROR) + except ValueError as err: self.message_user(request, err, messages.ERROR) + except TransitionNotAllowed + self.message_user( + request, + f"Error deleting this Domain: {err}", + messages.ERROR, + ) else: self.message_user( request, diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2c7f8703c..827d26073 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -609,11 +609,6 @@ class Domain(TimeStampedModel, DomainHelper): """ return self.state == self.State.READY - def delete_request(self): - """Delete from host. Possibly a duplicate of _delete_host?""" - # TODO fix in ticket #901 - pass - def transfer(self): """Going somewhere. Not implemented.""" raise NotImplementedError() @@ -658,7 +653,8 @@ class Domain(TimeStampedModel, DomainHelper): """This domain should be deleted from the registry may raises RegistryError, should be caught or handled correctly by caller""" request = commands.DeleteDomain(name=self.name) - registry.send(request) + response = registry.send(request, cleaned=True) + return response def __str__(self) -> str: return self.name @@ -773,6 +769,8 @@ class Domain(TimeStampedModel, DomainHelper): self.addAllDefaults() + + def addAllDefaults(self): security_contact = self.get_default_security_contact() security_contact.save() @@ -805,15 +803,34 @@ class Domain(TimeStampedModel, DomainHelper): # TODO -on the client hold ticket any additional error handling here @transition(field="state", source=State.ON_HOLD, target=State.DELETED) - def deleted(self): - """domain is deleted in epp but is saved in our database""" - # TODO Domains may not be deleted if: - # a child host is being used by - # another .gov domains. The host must be first removed - # and/or renamed before the parent domain may be deleted. - logger.info("pendingCreate()-> inside pending create") - self._delete_domain() - # TODO - delete ticket any additional error handling here + def deletedInEpp(self): + """domain is deleted in epp but is saved in our database. + Returns the request_code""" + valid_delete_states = [ + self.State.ON_HOLD, + self.State.DNS_NEEDED + ] + # Check that the domain contacts a valid status + if (self.state not in valid_delete_states): + raise ValueError( + f"Invalid domain state of {self.state}. Cannot delete." + ) + + try: + logger.info("deletedInEpp()-> inside _delete_domain") + self._delete_domain() + except RegistryError as err: + logger.error( + f"Could not delete domain. Registry returned error: {err}" + ) + raise err + except Exception as err: + logger.error( + f"Could not delete domain. An unspecified error occured: {err}" + ) + raise err + else: + self._invalidate_cache() @transition( field="state", diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 7df51baf4..c9d32d978 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -598,7 +598,11 @@ class DomainApplication(TimeStampedModel): "emails/domain_request_withdrawn.txt", "emails/domain_request_withdrawn_subject.txt", ) - + + # TODO + #def delete(self, *args, **kwargs): + #super().delete(*args, **kwargs) + @transition( field="status", source=[IN_REVIEW, APPROVED], @@ -612,7 +616,7 @@ class DomainApplication(TimeStampedModel): (will cascade), and send an email notification.""" if self.status == self.APPROVED: - self.approved_domain.delete_request() + self.approved_domain.deletedInEpp() self.approved_domain.delete() self.approved_domain = None diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index fe41647f9..9fc85b76b 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -604,6 +604,13 @@ class MockEppLib(TestCase): # use this for when a contact is being updated # sets the second send() to fail raise RegistryError(code=ErrorCode.OBJECT_EXISTS) + elif ( + isinstance(_request, commands.DeleteDomain) + and getattr(_request, "name", None) == "fail.gov" + ): + raise RegistryError( + code=ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION + ) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 54045bb32..842f2e116 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -940,39 +940,95 @@ class TestAnalystLock(TestCase): raise -class TestAnalystDelete(TestCase): +class TestAnalystDelete(MockEppLib): """Rule: Analysts may delete a domain""" - def setUp(self): - """ - Background: - Given the analyst is logged in - And a domain exists in the registry - """ - pass + """ + Background: + Given the analyst is logged in + And a domain exists in the registry + """ + super().setUp() + self.domain, _ = Domain.objects.get_or_create( + name="fake.gov", state=Domain.State.READY + ) + self.domain_on_hold, _ = Domain.objects.get_or_create( + name="fake-on-hold.gov", state=Domain.State.ON_HOLD + ) + + def tearDown(self): + Domain.objects.all().delete() + super().tearDown() - @skip("not implemented yet") def test_analyst_deletes_domain(self): """ Scenario: Analyst permanently deletes a domain - When `domain.delete()` is called + When `domain.deletedInEpp()` is called Then `commands.DeleteDomain` is sent to the registry And `state` is set to `DELETED` """ - raise + # Put the domain in client hold + self.domain.place_client_hold() + # Delete it... + self.domain.deletedInEpp() + 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) - @skip("not implemented yet") def test_analyst_deletes_domain_idempotent(self): """ Scenario: Analyst tries to delete an already deleted domain Given `state` is already `DELETED` - When `domain.delete()` is called + When `domain.deletedInEpp()` is called Then `commands.DeleteDomain` is sent to the registry And Domain returns normally (without error) """ - raise + # Put the domain in client hold + self.domain.place_client_hold() + # Delete it... + self.domain.deletedInEpp() + 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) + + # Delete it again - monitoring for errors + try: + self.domain.deletedInEpp() + except Exception as err: + self.fail("deletedInEpp() threw an error") + raise err + + 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) - @skip("not implemented yet") def test_deletion_is_unsuccessful(self): """ Scenario: Domain deletion is unsuccessful @@ -980,4 +1036,44 @@ class TestAnalystDelete(TestCase): Then a user-friendly error message is returned for displaying on the web And `state` is not set to `DELETED` """ - raise + domain, _ = Domain.objects.get_or_create( + name="fail.gov", state=Domain.State.ON_HOLD + ) + # Put the domain in client hold + domain.place_client_hold() + # Delete it... + + with self.assertRaises(RegistryError) as err: + domain.deletedInEpp() + self.assertTrue( + err.is_client_error() + and err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION + ) + # TODO - check UI for error + # Domain itself should not be deleted + self.assertNotEqual(domain, None) + # State should not have changed + self.assertEqual(domain.state, Domain.State.ON_HOLD) + + @skip("not implemented yet") + def test_deletion_ready_fsm_failure(self): + """ + Scenario: Domain deletion is unsuccessful due to FSM rules + Given state is 'ready' + When `domain.deletedInEpp()` is called + Then a user-friendly error message is returned for displaying on the web + And `state` is not set to `DELETED` + """ + self.domain.deletedInEpp() + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.DeleteDomain(name="fake.gov", auth_info=None), + cleaned=True, + ) + ] + ) + # Domain should not be deleted + self.assertNotEqual(self.domain, None) + # Domain should have the right state + self.assertEqual(self.domain.state, "DELETED") From 59b095beab06df382b31069413a1a9b077ccf289 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 28 Sep 2023 13:53:21 -0600 Subject: [PATCH 04/21] Commit for #901 --- src/registrar/admin.py | 63 ++++---- src/registrar/models/domain.py | 27 ++-- .../django/admin/domain_change_form.html | 4 +- src/registrar/templates/home.html | 3 + src/registrar/tests/common.py | 11 +- src/registrar/tests/test_admin.py | 145 ++++++++++++++++++ src/registrar/tests/test_models_domain.py | 88 ++++------- 7 files changed, 226 insertions(+), 115 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6ec27a085..415289e42 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -7,6 +7,7 @@ from django.contrib.contenttypes.models import ContentType from django.http.response import HttpResponseRedirect from django.urls import reverse from epplibwrapper.errors import ErrorCode, RegistryError +from registrar.models.domain import Domain from registrar.models.utility.admin_sort_fields import AdminSortFields from . import models from auditlog.models import LogEntry # type: ignore @@ -717,51 +718,47 @@ class DomainAdmin(ListHeaderAdmin): return super().response_change(request, obj) def do_delete_domain(self, request, obj): + if not isinstance(obj, Domain): + # Could be problematic if the type is similar, + # but not the same (same field/func names) so we err out. + # We do not want to accidentally delete records. + raise ValueError("Object is not of type Domain") try: obj.deletedInEpp() obj.save() except RegistryError as err: - if err.is_connection_error(): - self.message_user( - request, - "Error connecting to the registry", - messages.ERROR, - ) - elif err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: - self.message_user( - request, - "Error deleting this Domain: " + # Human-readable mappings of ErrorCodes. Can be expanded. + error_messages = { + ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: f"Cannot delete Domain when in status {obj.status}", - messages.ERROR, - ) - elif err.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + "This subdomain is being used as a hostname on another domain" + } + + message = "Cannot connect to the registry" + if not err.is_connection_error(): + # If nothing is found, will default to returned err + message = error_messages.get(err.code, err) + self.message_user(request, f"Error deleting this Domain: {message}", messages.ERROR) + except TransitionNotAllowed as err: + if obj.state == Domain.State.DELETED: self.message_user( request, - "Error deleting this Domain: " - f" This subdomain is being used as a hostname on another domain", - messages.ERROR, - ) - elif err.code: - self.message_user( - request, - f"Error deleting this Domain: {err}", - messages.ERROR, + f"This domain is already deleted", + messages.INFO, ) else: - # all other type error messages, display the error - self.message_user(request, err, messages.ERROR) - except ValueError as err: - self.message_user(request, err, messages.ERROR) - except TransitionNotAllowed - self.message_user( - request, - f"Error deleting this Domain: {err}", - messages.ERROR, - ) + self.message_user( + request, + "Error deleting this Domain: " + f"Can't switch from state '{obj.state}' to 'deleted'" + , + messages.ERROR, + ) else: self.message_user( request, - ("Domain %s Should now be deleted " ". Thanks!") % obj.name, + ("Domain %s has been deleted. Thanks!") % obj.name, ) return HttpResponseRedirect(".") diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 827d26073..e264d4aa9 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2,7 +2,7 @@ import logging from datetime import date from string import digits -from django_fsm import FSMField, transition # type: ignore +from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore from django.db import models @@ -802,20 +802,14 @@ class Domain(TimeStampedModel, DomainHelper): self._remove_client_hold() # TODO -on the client hold ticket any additional error handling here - @transition(field="state", source=State.ON_HOLD, target=State.DELETED) + @transition(field="state", source=[State.ON_HOLD, State.DNS_NEEDED], target=State.DELETED) def deletedInEpp(self): - """domain is deleted in epp but is saved in our database. - Returns the request_code""" - valid_delete_states = [ - self.State.ON_HOLD, - self.State.DNS_NEEDED - ] - # Check that the domain contacts a valid status - if (self.state not in valid_delete_states): - raise ValueError( - f"Invalid domain state of {self.state}. Cannot delete." - ) - + """Domain is deleted in epp but is saved in our database. + Error handling should be provided by the caller.""" + # While we want to log errors, we want to preserve + # that information when this function is called. + # Human-readable errors are introduced at the admin.py level, + # as doing everything here would reduce reliablity. try: logger.info("deletedInEpp()-> inside _delete_domain") self._delete_domain() @@ -824,6 +818,11 @@ class Domain(TimeStampedModel, DomainHelper): f"Could not delete domain. Registry returned error: {err}" ) raise err + except TransitionNotAllowed as err: + logger.error( + "Could not delete domain. FSM failure: {err}" + ) + raise err except Exception as err: logger.error( f"Could not delete domain. An unspecified error occured: {err}" diff --git a/src/registrar/templates/django/admin/domain_change_form.html b/src/registrar/templates/django/admin/domain_change_form.html index 1b8b90930..ca44aa03c 100644 --- a/src/registrar/templates/django/admin/domain_change_form.html +++ b/src/registrar/templates/django/admin/domain_change_form.html @@ -8,9 +8,9 @@ {% block field_sets %}
- {% if original.state == original.State.READY %} + {% if original.state == original.State.READY%} - {% elif original.state == original.State.ON_HOLD %} + {% elif original.state == original.State.ON_HOLD%} {% endif %} diff --git a/src/registrar/templates/home.html b/src/registrar/templates/home.html index db3fab886..737437b74 100644 --- a/src/registrar/templates/home.html +++ b/src/registrar/templates/home.html @@ -40,6 +40,9 @@ {{ domain.name }} {{ domain.created_time|date }} + {% comment %} Should this be domain.status? + {{ domain.status|title }} + {% endcomment %} {{ domain.application_status|title }} diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 9fc85b76b..10c387099 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -606,11 +606,14 @@ class MockEppLib(TestCase): raise RegistryError(code=ErrorCode.OBJECT_EXISTS) elif ( isinstance(_request, commands.DeleteDomain) - and getattr(_request, "name", None) == "fail.gov" + and getattr(_request, "name", None) == "failDelete.gov" ): - raise RegistryError( - code=ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION - ) + name = getattr(_request, "name", None) + fake_nameserver = "ns1.failDelete.gov" + if name in fake_nameserver: + raise RegistryError( + code=ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION + ) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9ff9ce451..ce47edc27 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -49,6 +49,7 @@ class TestDomainAdmin(MockEppLib): self.client = Client(HTTP_HOST="localhost:8080") self.superuser = create_superuser() self.staffuser = create_user() + self.factory = RequestFactory() super().setUp() def test_place_and_remove_hold(self): @@ -87,6 +88,150 @@ class TestDomainAdmin(MockEppLib): self.assertContains(response, "Place hold") self.assertNotContains(response, "Remove hold") + def test_deletion_is_successful(self): + """ + Scenario: Domain deletion is unsuccessful + When the domain is deleted + Then a user-friendly success message is returned for displaying on the web + And `state` is et to `DELETED` + """ + domain = create_ready_domain() + # Put in client hold + domain.place_client_hold() + self.client.login(username="staffuser", password="userpass") + + # Ensure everything is displaying correctly + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.pk), + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + self.assertContains(response, "EPP Delete Domain") + + # Test the info dialog + request = self.factory.post( + "/admin/registrar/domain/{}/change/".format(domain.pk), + {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + follow=True, + ) + request.user = self.client + + 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!", + extra_tags='', + fail_silently=False + ) + + self.assertEqual(domain.state, Domain.State.DELETED) + + def test_deletion_ready_fsm_failure(self): + """ + Scenario: Domain deletion is unsuccessful + When an error is returned from epplibwrapper + Then a user-friendly error message is returned for displaying on the web + And `state` is not set to `DELETED` + """ + domain = create_ready_domain() + self.client.login(username="staffuser", password="userpass") + + # Ensure everything is displaying correctly + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.pk), + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + self.assertContains(response, "EPP Delete Domain") + + # Test the error + request = self.factory.post( + "/admin/registrar/domain/{}/change/".format(domain.pk), + {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + follow=True, + ) + request.user = self.client + + 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.ERROR, + "Error deleting this Domain: Can't switch from state 'ready' to 'deleted'", + extra_tags='', + fail_silently=False + ) + + self.assertEqual(domain.state, Domain.State.READY) + + def test_analyst_deletes_domain_idempotent(self): + """ + Scenario: Analyst tries to delete an already deleted domain + Given `state` is already `DELETED` + When `domain.deletedInEpp()` is called + Then `commands.DeleteDomain` is sent to the registry + And Domain returns normally without an error dialog + """ + domain = create_ready_domain() + # Put in client hold + domain.place_client_hold() + self.client.login(username="staffuser", password="userpass") + + # Ensure everything is displaying correctly + response = self.client.get( + "/admin/registrar/domain/{}/change/".format(domain.pk), + follow=True, + ) + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain.name) + self.assertContains(response, "EPP Delete Domain") + + # Test the info dialog + request = self.factory.post( + "/admin/registrar/domain/{}/change/".format(domain.pk), + {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + follow=True, + ) + 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!", + extra_tags='', + fail_silently=False + ) + + self.assertEqual(domain.state, Domain.State.DELETED) + + # Try to delete it again + # Test the info dialog + request = self.factory.post( + "/admin/registrar/domain/{}/change/".format(domain.pk), + {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + follow=True, + ) + request.user = self.client + + 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, + "This domain is already deleted", + extra_tags='', + fail_silently=False + ) + + self.assertEqual(domain.state, Domain.State.DELETED) + @skip("Waiting on epp lib to implement") def test_place_and_remove_hold_epp(self): raise diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 842f2e116..6054202f5 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -16,7 +16,7 @@ from registrar.models.draft_domain import DraftDomain from registrar.models.public_contact import PublicContact from registrar.models.user import User from .common import MockEppLib - +from django_fsm import TransitionNotAllowed # type: ignore from epplibwrapper import ( commands, common, @@ -983,97 +983,61 @@ class TestAnalystDelete(MockEppLib): self.assertNotEqual(self.domain, None) # Domain should have the right state self.assertEqual(self.domain.state, Domain.State.DELETED) - - def test_analyst_deletes_domain_idempotent(self): - """ - Scenario: Analyst tries to delete an already deleted domain - Given `state` is already `DELETED` - When `domain.deletedInEpp()` is called - Then `commands.DeleteDomain` is sent to the registry - And Domain returns normally (without error) - """ - # Put the domain in client hold - self.domain.place_client_hold() - # Delete it... - self.domain.deletedInEpp() - 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) - - # Delete it again - monitoring for errors - try: - self.domain.deletedInEpp() - except Exception as err: - self.fail("deletedInEpp() threw an error") - raise err - - 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) + # Cache should be invalidated + self.assertEqual(self.domain._cache, {}) def test_deletion_is_unsuccessful(self): """ Scenario: Domain deletion is unsuccessful When an error is returned from epplibwrapper - Then a user-friendly error message is returned for displaying on the web + Then a client error is returned of code 2305 And `state` is not set to `DELETED` """ + # Desired domain domain, _ = Domain.objects.get_or_create( - name="fail.gov", state=Domain.State.ON_HOLD + name="failDelete.gov", state=Domain.State.ON_HOLD ) # Put the domain in client hold domain.place_client_hold() - # Delete it... + # Delete it... with self.assertRaises(RegistryError) as err: domain.deletedInEpp() self.assertTrue( err.is_client_error() - and err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION + and err.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION ) + self.mockedSendFunction.assert_has_calls( + [ + call( + commands.DeleteDomain(name="failDelete.gov"), + cleaned=True, + ) + ] + ) # TODO - check UI for error # Domain itself should not be deleted self.assertNotEqual(domain, None) # State should not have changed self.assertEqual(domain.state, Domain.State.ON_HOLD) - @skip("not implemented yet") def test_deletion_ready_fsm_failure(self): """ Scenario: Domain deletion is unsuccessful due to FSM rules Given state is 'ready' When `domain.deletedInEpp()` is called - Then a user-friendly error message is returned for displaying on the web + and domain is of `state` is `READY` + Then an FSM error is returned And `state` is not set to `DELETED` """ - self.domain.deletedInEpp() - self.mockedSendFunction.assert_has_calls( - [ - call( - commands.DeleteDomain(name="fake.gov", auth_info=None), - cleaned=True, - ) - ] - ) + self.assertEqual(self.domain.state, Domain.State.READY) + with self.assertRaises(TransitionNotAllowed) as err: + self.domain.deletedInEpp() + self.assertTrue( + err.is_client_error() + and err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION + ) # Domain should not be deleted self.assertNotEqual(self.domain, None) # Domain should have the right state - self.assertEqual(self.domain.state, "DELETED") + self.assertEqual(self.domain.state, Domain.State.READY) From 27166549149c23a8361b93e1e4debd20a94400e7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 28 Sep 2023 13:58:39 -0600 Subject: [PATCH 05/21] Stashed lint changes --- src/registrar/admin.py | 16 +++++++---- src/registrar/models/domain.py | 14 ++++----- src/registrar/models/domain_application.py | 6 +--- src/registrar/tests/test_admin.py | 29 ++++++++++--------- src/registrar/tests/test_models_domain.py | 33 +++++++++++----------- 5 files changed, 48 insertions(+), 50 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 415289e42..2d04fc7e5 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -12,7 +12,7 @@ from registrar.models.utility.admin_sort_fields import AdminSortFields from . import models from auditlog.models import LogEntry # type: ignore from auditlog.admin import LogEntryAdmin # type: ignore -from django_fsm import TransitionNotAllowed # type: ignore +from django_fsm import TransitionNotAllowed # type: ignore logger = logging.getLogger(__name__) @@ -729,9 +729,9 @@ class DomainAdmin(ListHeaderAdmin): except RegistryError as err: # Human-readable mappings of ErrorCodes. Can be expanded. error_messages = { - ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: + ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: f"Cannot delete Domain when in status {obj.status}", - ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: "This subdomain is being used as a hostname on another domain" } @@ -739,12 +739,16 @@ class DomainAdmin(ListHeaderAdmin): if not err.is_connection_error(): # If nothing is found, will default to returned err message = error_messages.get(err.code, err) - self.message_user(request, f"Error deleting this Domain: {message}", messages.ERROR) - except TransitionNotAllowed as err: + self.message_user( + request, + f"Error deleting this Domain: {message}", + messages.ERROR + ) + except TransitionNotAllowed: if obj.state == Domain.State.DELETED: self.message_user( request, - f"This domain is already deleted", + "This domain is already deleted", messages.INFO, ) else: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index e264d4aa9..98b4120d5 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -769,8 +769,6 @@ class Domain(TimeStampedModel, DomainHelper): self.addAllDefaults() - - def addAllDefaults(self): security_contact = self.get_default_security_contact() security_contact.save() @@ -802,7 +800,9 @@ class Domain(TimeStampedModel, DomainHelper): self._remove_client_hold() # TODO -on the client hold ticket any additional error handling here - @transition(field="state", source=[State.ON_HOLD, State.DNS_NEEDED], target=State.DELETED) + @transition( + field="state", source=[State.ON_HOLD, State.DNS_NEEDED], target=State.DELETED + ) def deletedInEpp(self): """Domain is deleted in epp but is saved in our database. Error handling should be provided by the caller.""" @@ -814,14 +814,10 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("deletedInEpp()-> inside _delete_domain") self._delete_domain() except RegistryError as err: - logger.error( - f"Could not delete domain. Registry returned error: {err}" - ) + logger.error(f"Could not delete domain. Registry returned error: {err}") raise err except TransitionNotAllowed as err: - logger.error( - "Could not delete domain. FSM failure: {err}" - ) + logger.error("Could not delete domain. FSM failure: {err}") raise err except Exception as err: logger.error( diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index c9d32d978..89c643368 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -598,11 +598,7 @@ class DomainApplication(TimeStampedModel): "emails/domain_request_withdrawn.txt", "emails/domain_request_withdrawn_subject.txt", ) - - # TODO - #def delete(self, *args, **kwargs): - #super().delete(*args, **kwargs) - + @transition( field="status", source=[IN_REVIEW, APPROVED], diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index ce47edc27..c52af5628 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -117,14 +117,14 @@ class TestDomainAdmin(MockEppLib): ) request.user = self.client - with patch('django.contrib.messages.add_message') as mock_add_message: + 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!", - extra_tags='', - fail_silently=False + extra_tags="", + fail_silently=False, ) self.assertEqual(domain.state, Domain.State.DELETED) @@ -156,18 +156,19 @@ class TestDomainAdmin(MockEppLib): ) request.user = self.client - with patch('django.contrib.messages.add_message') as mock_add_message: + 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.ERROR, - "Error deleting this Domain: Can't switch from state 'ready' to 'deleted'", - extra_tags='', - fail_silently=False + "Error deleting this Domain: " + "Can't switch from state 'ready' to 'deleted'", + extra_tags="", + fail_silently=False, ) self.assertEqual(domain.state, Domain.State.READY) - + def test_analyst_deletes_domain_idempotent(self): """ Scenario: Analyst tries to delete an already deleted domain @@ -199,14 +200,14 @@ class TestDomainAdmin(MockEppLib): request.user = self.client # Delete it once - with patch('django.contrib.messages.add_message') as mock_add_message: + 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!", - extra_tags='', - fail_silently=False + extra_tags="", + fail_silently=False, ) self.assertEqual(domain.state, Domain.State.DELETED) @@ -220,14 +221,14 @@ class TestDomainAdmin(MockEppLib): ) request.user = self.client - with patch('django.contrib.messages.add_message') as mock_add_message: + 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, "This domain is already deleted", - extra_tags='', - fail_silently=False + extra_tags="", + fail_silently=False, ) self.assertEqual(domain.state, Domain.State.DELETED) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 6054202f5..6be79848d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -16,7 +16,7 @@ from registrar.models.draft_domain import DraftDomain from registrar.models.public_contact import PublicContact from registrar.models.user import User from .common import MockEppLib -from django_fsm import TransitionNotAllowed # type: ignore +from django_fsm import TransitionNotAllowed # type: ignore from epplibwrapper import ( commands, common, @@ -942,19 +942,20 @@ class TestAnalystLock(TestCase): class TestAnalystDelete(MockEppLib): """Rule: Analysts may delete a domain""" + def setUp(self): - """ - Background: - Given the analyst is logged in - And a domain exists in the registry - """ - super().setUp() - self.domain, _ = Domain.objects.get_or_create( - name="fake.gov", state=Domain.State.READY - ) - self.domain_on_hold, _ = Domain.objects.get_or_create( - name="fake-on-hold.gov", state=Domain.State.ON_HOLD - ) + """ + Background: + Given the analyst is logged in + And a domain exists in the registry + """ + super().setUp() + self.domain, _ = Domain.objects.get_or_create( + name="fake.gov", state=Domain.State.READY + ) + self.domain_on_hold, _ = Domain.objects.get_or_create( + name="fake-on-hold.gov", state=Domain.State.ON_HOLD + ) def tearDown(self): Domain.objects.all().delete() @@ -995,7 +996,7 @@ class TestAnalystDelete(MockEppLib): """ # Desired domain domain, _ = Domain.objects.get_or_create( - name="failDelete.gov", state=Domain.State.ON_HOLD + name="failDelete.gov", state=Domain.State.ON_HOLD ) # Put the domain in client hold domain.place_client_hold() @@ -1004,7 +1005,7 @@ class TestAnalystDelete(MockEppLib): with self.assertRaises(RegistryError) as err: domain.deletedInEpp() self.assertTrue( - err.is_client_error() + err.is_client_error() and err.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION ) self.mockedSendFunction.assert_has_calls( @@ -1034,7 +1035,7 @@ class TestAnalystDelete(MockEppLib): with self.assertRaises(TransitionNotAllowed) as err: self.domain.deletedInEpp() self.assertTrue( - err.is_client_error() + err.is_client_error() and err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION ) # Domain should not be deleted From 9a9c814269100a6a1dff3e09083a3457c6d50e4a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 28 Sep 2023 14:06:21 -0600 Subject: [PATCH 06/21] Clarification --- src/registrar/tests/test_models_domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 6be79848d..45f3ff271 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -990,7 +990,7 @@ class TestAnalystDelete(MockEppLib): def test_deletion_is_unsuccessful(self): """ Scenario: Domain deletion is unsuccessful - When an error is returned from epplibwrapper + When a subdomain exists Then a client error is returned of code 2305 And `state` is not set to `DELETED` """ From b7a5e0e763e72199a0d08f8761c40bdd63f1cded Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 28 Sep 2023 14:16:19 -0600 Subject: [PATCH 07/21] Linter mc linterton --- src/registrar/models/domain.py | 3 +-- .../templates/django/admin/domain_change_form.html | 4 ++-- src/registrar/tests/test_admin.py | 9 ++++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 98b4120d5..b5f2814fc 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -653,8 +653,7 @@ class Domain(TimeStampedModel, DomainHelper): """This domain should be deleted from the registry may raises RegistryError, should be caught or handled correctly by caller""" request = commands.DeleteDomain(name=self.name) - response = registry.send(request, cleaned=True) - return response + registry.send(request, cleaned=True) def __str__(self) -> str: return self.name diff --git a/src/registrar/templates/django/admin/domain_change_form.html b/src/registrar/templates/django/admin/domain_change_form.html index ca44aa03c..1b8b90930 100644 --- a/src/registrar/templates/django/admin/domain_change_form.html +++ b/src/registrar/templates/django/admin/domain_change_form.html @@ -8,9 +8,9 @@ {% block field_sets %}
- {% if original.state == original.State.READY%} + {% if original.state == original.State.READY %} - {% elif original.state == original.State.ON_HOLD%} + {% elif original.state == original.State.ON_HOLD %} {% endif %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index c52af5628..31fa8fbcb 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -98,7 +98,8 @@ class TestDomainAdmin(MockEppLib): domain = create_ready_domain() # Put in client hold domain.place_client_hold() - self.client.login(username="staffuser", password="userpass") + p = "userpass" + self.client.login(username="staffuser", password=p) # Ensure everything is displaying correctly response = self.client.get( @@ -137,7 +138,8 @@ class TestDomainAdmin(MockEppLib): And `state` is not set to `DELETED` """ domain = create_ready_domain() - self.client.login(username="staffuser", password="userpass") + p = "userpass" + self.client.login(username="staffuser", password=p) # Ensure everything is displaying correctly response = self.client.get( @@ -180,7 +182,8 @@ class TestDomainAdmin(MockEppLib): domain = create_ready_domain() # Put in client hold domain.place_client_hold() - self.client.login(username="staffuser", password="userpass") + p = "userpass" + self.client.login(username="staffuser", password=p) # Ensure everything is displaying correctly response = self.client.get( From 82c03907df1b631c362ac4e4c74713eb66769bd6 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 08:16:04 -0600 Subject: [PATCH 08/21] Added Daves suggestion - generic exception handling --- src/registrar/admin.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 2d04fc7e5..b4657a70c 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -732,7 +732,7 @@ class DomainAdmin(ListHeaderAdmin): ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: f"Cannot delete Domain when in status {obj.status}", ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: - "This subdomain is being used as a hostname on another domain" + "This subdomain is being used as a hostname on another domain", } message = "Cannot connect to the registry" @@ -740,9 +740,7 @@ class DomainAdmin(ListHeaderAdmin): # If nothing is found, will default to returned err message = error_messages.get(err.code, err) self.message_user( - request, - f"Error deleting this Domain: {message}", - messages.ERROR + request, f"Error deleting this Domain: {message}", messages.ERROR ) except TransitionNotAllowed: if obj.state == Domain.State.DELETED: @@ -755,15 +753,21 @@ class DomainAdmin(ListHeaderAdmin): self.message_user( request, "Error deleting this Domain: " - f"Can't switch from state '{obj.state}' to 'deleted'" - , + f"Can't switch from state '{obj.state}' to 'deleted'", messages.ERROR, ) + except Exception: + self.message_user( + request, + "Could not delete: An unspecified error occured", + messages.ERROR, + ) else: self.message_user( request, ("Domain %s has been deleted. Thanks!") % obj.name, ) + return HttpResponseRedirect(".") def do_get_status(self, request, obj): From ffcc77de4e71f29c7680b3ecd5f2448008144f7f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 29 Sep 2023 10:30:36 -0400 Subject: [PATCH 09/21] added TestDomainAvailable to test_models_domain.py --- src/epplibwrapper/__init__.py | 2 + src/registrar/tests/test_models_domain.py | 112 +++++++++++++++++++++- 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/src/epplibwrapper/__init__.py b/src/epplibwrapper/__init__.py index b306dbd0e..65de3ec05 100644 --- a/src/epplibwrapper/__init__.py +++ b/src/epplibwrapper/__init__.py @@ -45,6 +45,7 @@ try: from .client import CLIENT, commands from .errors import RegistryError, ErrorCode from epplib.models import common + from epplib import responses except ImportError: pass @@ -52,6 +53,7 @@ __all__ = [ "CLIENT", "commands", "common", + "responses", "ErrorCode", "RegistryError", ] diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 54045bb32..9c4e18203 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -5,7 +5,7 @@ This file tests the various ways in which the registrar interacts with the regis """ from django.test import TestCase from django.db.utils import IntegrityError -from unittest.mock import patch, call +from unittest.mock import MagicMock, patch, call import datetime from registrar.models import Domain @@ -20,6 +20,7 @@ from .common import MockEppLib from epplibwrapper import ( commands, common, + responses, RegistryError, ErrorCode, ) @@ -263,6 +264,115 @@ class TestDomainStatuses(MockEppLib): super().tearDown() +class TestDomainAvailable(MockEppLib): + """Test Domain.available""" + + # No SetUp or tearDown necessary for these tests + + def test_domain_available(self): + """ + Scenario: Testing whether an available domain is available + Should return True + + Mock response to mimic EPP Response + Validate CheckDomain command is called + Validate response given mock + """ + def side_effect(_request, cleaned): + return MagicMock( + res_data=[ + responses.check.CheckDomainResultData(name='available.gov', avail=True, reason=None) + ], + ) + + patcher = patch("registrar.models.domain.registry.send") + mocked_send = patcher.start() + mocked_send.side_effect = side_effect + + available = Domain.available("available.gov") + mocked_send.assert_has_calls( + [ + call( + commands.CheckDomain( + [ + "available.gov" + ], + ), + cleaned=True, + ) + ] + ) + self.assertTrue(available) + patcher.stop() + + def test_domain_unavailable(self): + """ + Scenario: Testing whether an unavailable domain is available + Should return False + + Mock response to mimic EPP Response + Validate CheckDomain command is called + Validate response given mock + """ + def side_effect(_request, cleaned): + return MagicMock( + res_data=[ + responses.check.CheckDomainResultData( + name='unavailable.gov', + avail=False, + reason="In Use" + ) + ], + ) + + patcher = patch("registrar.models.domain.registry.send") + mocked_send = patcher.start() + mocked_send.side_effect = side_effect + + available = Domain.available("unavailable.gov") + mocked_send.assert_has_calls( + [ + call( + commands.CheckDomain( + [ + "unavailable.gov" + ], + ), + cleaned=True, + ) + ] + ) + self.assertFalse(available) + patcher.stop() + + def test_domain_available_with_value_error(self): + """ + Scenario: Testing whether an invalid domain is available + Should throw ValueError + + Validate ValueError is raised + """ + with self.assertRaises(ValueError): + Domain.available("invalid-string") + + def test_domain_available_unsuccessful(self): + """ + Scenario: Testing behavior when registry raises a RegistryError + + Validate RegistryError is raised + """ + def side_effect(_request, cleaned): + raise RegistryError(code=ErrorCode.COMMAND_SYNTAX_ERROR) + + patcher = patch("registrar.models.domain.registry.send") + mocked_send = patcher.start() + mocked_send.side_effect = side_effect + + with self.assertRaises(RegistryError) as err: + Domain.available("raises-error.gov") + patcher.stop() + + class TestRegistrantContacts(MockEppLib): """Rule: Registrants may modify their WHOIS data""" From 6231ab8145cd9e4a065f0a341d38e35730fe035b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 29 Sep 2023 10:40:37 -0400 Subject: [PATCH 10/21] formmatted for lint --- src/registrar/tests/test_models_domain.py | 25 +++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 9c4e18203..53f286d47 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -278,10 +278,13 @@ class TestDomainAvailable(MockEppLib): Validate CheckDomain command is called Validate response given mock """ + def side_effect(_request, cleaned): return MagicMock( res_data=[ - responses.check.CheckDomainResultData(name='available.gov', avail=True, reason=None) + responses.check.CheckDomainResultData( + name="available.gov", avail=True, reason=None + ) ], ) @@ -294,9 +297,7 @@ class TestDomainAvailable(MockEppLib): [ call( commands.CheckDomain( - [ - "available.gov" - ], + ["available.gov"], ), cleaned=True, ) @@ -314,13 +315,12 @@ class TestDomainAvailable(MockEppLib): Validate CheckDomain command is called Validate response given mock """ + def side_effect(_request, cleaned): return MagicMock( res_data=[ responses.check.CheckDomainResultData( - name='unavailable.gov', - avail=False, - reason="In Use" + name="unavailable.gov", avail=False, reason="In Use" ) ], ) @@ -334,9 +334,7 @@ class TestDomainAvailable(MockEppLib): [ call( commands.CheckDomain( - [ - "unavailable.gov" - ], + ["unavailable.gov"], ), cleaned=True, ) @@ -358,17 +356,18 @@ class TestDomainAvailable(MockEppLib): def test_domain_available_unsuccessful(self): """ Scenario: Testing behavior when registry raises a RegistryError - + Validate RegistryError is raised """ + def side_effect(_request, cleaned): raise RegistryError(code=ErrorCode.COMMAND_SYNTAX_ERROR) - + patcher = patch("registrar.models.domain.registry.send") mocked_send = patcher.start() mocked_send.side_effect = side_effect - with self.assertRaises(RegistryError) as err: + with self.assertRaises(RegistryError): Domain.available("raises-error.gov") patcher.stop() From e94f9dc51b0e7fbfad2f0a8bb11810f3ae6b1a61 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 08:45:25 -0600 Subject: [PATCH 11/21] Linter + test case --- src/registrar/admin.py | 8 ++++---- src/registrar/models/domain_application.py | 12 +++++++++--- src/registrar/tests/test_admin.py | 4 +++- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index b4657a70c..d20020ba3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -13,6 +13,7 @@ from . import models from auditlog.models import LogEntry # type: ignore from auditlog.admin import LogEntryAdmin # type: ignore from django_fsm import TransitionNotAllowed # type: ignore + logger = logging.getLogger(__name__) @@ -729,10 +730,9 @@ class DomainAdmin(ListHeaderAdmin): except RegistryError as err: # Human-readable mappings of ErrorCodes. Can be expanded. error_messages = { - ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: - f"Cannot delete Domain when in status {obj.status}", - ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: - "This subdomain is being used as a hostname on another domain", + # noqa on these items as black wants to reformat to an invalid length + ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: f"Cannot delete Domain when in status {obj.status}", # noqa + ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: "This subdomain is being used as a hostname on another domain", # noqa } message = "Cannot connect to the registry" diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 89c643368..68429d381 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -6,6 +6,7 @@ import logging from django.apps import apps from django.db import models from django_fsm import FSMField, transition # type: ignore +from registrar.models.domain import Domain from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError @@ -610,9 +611,11 @@ class DomainApplication(TimeStampedModel): As side effects this will delete the domain and domain_information (will cascade), and send an email notification.""" - if self.status == self.APPROVED: - self.approved_domain.deletedInEpp() + domain_state = self.approved_domain.state + # Only reject if it exists on EPP + if domain_state != Domain.State.UNKNOWN: + self.approved_domain.deletedInEpp() self.approved_domain.delete() self.approved_domain = None @@ -638,7 +641,10 @@ class DomainApplication(TimeStampedModel): and domain_information (will cascade) when they exist.""" if self.status == self.APPROVED: - self.approved_domain.delete_request() + domain_state = self.approved_domain.state + # Only reject if it exists on EPP + if domain_state != Domain.State.UNKNOWN: + self.approved_domain.deletedInEpp() self.approved_domain.delete() self.approved_domain = None diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 31fa8fbcb..7cd530123 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -287,8 +287,9 @@ class TestDomainApplicationAdminForm(TestCase): ) -class TestDomainApplicationAdmin(TestCase): +class TestDomainApplicationAdmin(MockEppLib): def setUp(self): + super().setUp() self.site = AdminSite() self.factory = RequestFactory() self.admin = DomainApplicationAdmin( @@ -839,6 +840,7 @@ class TestDomainApplicationAdmin(TestCase): domain_information.refresh_from_db() def tearDown(self): + super().tearDown() Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() From 0314220713c4a0f4090943843d9303aa51e14ab8 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 10:32:39 -0600 Subject: [PATCH 12/21] Linter --- src/registrar/admin.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d20020ba3..2e4f61991 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -728,11 +728,14 @@ class DomainAdmin(ListHeaderAdmin): obj.deletedInEpp() obj.save() except RegistryError as err: + # To get past the linter.. + l1 = f"Cannot delete Domain when in status {obj.status}" + l2 = "This subdomain is being used as a hostname on another domain" # 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: f"Cannot delete Domain when in status {obj.status}", # noqa - ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: "This subdomain is being used as a hostname on another domain", # noqa + ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: l1, + ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: l2, } message = "Cannot connect to the registry" From 96279729d05e433d6044344c4835a983a7c8b52e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 11:22:47 -0600 Subject: [PATCH 13/21] Update domain.py --- src/registrar/views/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index a4498146a..d28e7ccce 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -372,6 +372,7 @@ class DomainAddUserView(DomainPermissionView, FormMixin): requested_email = form.cleaned_data["email"] # look up a user with that email try: + l# Will remove - to push to my sandbox requested_user = User.objects.get(email=requested_email) except User.DoesNotExist: # no matching user, go make an invitation From 596f7144286ecdae51a8571a7309ac89cf450ae9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 11:25:32 -0600 Subject: [PATCH 14/21] Correction --- src/registrar/views/domain.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index d28e7ccce..a4498146a 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -372,7 +372,6 @@ class DomainAddUserView(DomainPermissionView, FormMixin): requested_email = form.cleaned_data["email"] # look up a user with that email try: - l# Will remove - to push to my sandbox requested_user = User.objects.get(email=requested_email) except User.DoesNotExist: # no matching user, go make an invitation From 228d37ef3983aac94fe8866d13f8a5b4a902a81f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 14:41:52 -0600 Subject: [PATCH 15/21] Requested changes --- src/registrar/admin.py | 13 +++++++------ .../templates/django/admin/domain_change_form.html | 2 +- src/registrar/tests/test_admin.py | 14 +++++++------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 2e4f61991..19669605f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -728,14 +728,14 @@ class DomainAdmin(ListHeaderAdmin): obj.deletedInEpp() obj.save() except RegistryError as err: - # To get past the linter.. - l1 = f"Cannot delete Domain when in status {obj.status}" - l2 = "This subdomain is being used as a hostname on another domain" + # Using variables to get past the linter + message1 = f"Cannot delete Domain when in status {obj.status}" + message2 = "This subdomain is being used as a hostname on another domain" # 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: l1, - ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: l2, + ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: message1, + ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: message2, } message = "Cannot connect to the registry" @@ -756,7 +756,8 @@ class DomainAdmin(ListHeaderAdmin): self.message_user( request, "Error deleting this Domain: " - f"Can't switch from state '{obj.state}' to 'deleted'", + f"Can't switch from state '{obj.state}' to 'deleted'" + ", must be either 'dns_needed' or 'on_hold'", messages.ERROR, ) except Exception: diff --git a/src/registrar/templates/django/admin/domain_change_form.html b/src/registrar/templates/django/admin/domain_change_form.html index 1b8b90930..cc6261af0 100644 --- a/src/registrar/templates/django/admin/domain_change_form.html +++ b/src/registrar/templates/django/admin/domain_change_form.html @@ -15,7 +15,7 @@ {% endif %} - +
{{ block.super }} {% endblock %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 7cd530123..c12a2d2a6 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -108,12 +108,12 @@ class TestDomainAdmin(MockEppLib): ) self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - self.assertContains(response, "EPP Delete Domain") + self.assertContains(response, "Delete Domain in Registry") # Test the info dialog request = self.factory.post( "/admin/registrar/domain/{}/change/".format(domain.pk), - {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + {"_delete_domain": "Delete Domain in Registry", "name": domain.name}, follow=True, ) request.user = self.client @@ -148,12 +148,12 @@ class TestDomainAdmin(MockEppLib): ) self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - self.assertContains(response, "EPP Delete Domain") + self.assertContains(response, "Delete Domain in Registry") # Test the error request = self.factory.post( "/admin/registrar/domain/{}/change/".format(domain.pk), - {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + {"_delete_domain": "Delete Domain in Registry", "name": domain.name}, follow=True, ) request.user = self.client @@ -192,12 +192,12 @@ class TestDomainAdmin(MockEppLib): ) self.assertEqual(response.status_code, 200) self.assertContains(response, domain.name) - self.assertContains(response, "EPP Delete Domain") + self.assertContains(response, "Delete Domain in Registry") # Test the info dialog request = self.factory.post( "/admin/registrar/domain/{}/change/".format(domain.pk), - {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + {"_delete_domain": "Delete Domain in Registry", "name": domain.name}, follow=True, ) request.user = self.client @@ -219,7 +219,7 @@ class TestDomainAdmin(MockEppLib): # Test the info dialog request = self.factory.post( "/admin/registrar/domain/{}/change/".format(domain.pk), - {"_delete_domain": "Epp Delete Domain", "name": domain.name}, + {"_delete_domain": "Delete Domain in Registry", "name": domain.name}, follow=True, ) request.user = self.client From d5b557d443ad1efe956cd6fe10b5a38bb7cd0180 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 14:46:13 -0600 Subject: [PATCH 16/21] Fix test --- src/registrar/tests/test_admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index c12a2d2a6..def475536 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -164,7 +164,8 @@ class TestDomainAdmin(MockEppLib): request, messages.ERROR, "Error deleting this Domain: " - "Can't switch from state 'ready' to 'deleted'", + "Can't switch from state 'ready' to 'deleted'" + ", must be either 'dns_needed' or 'on_hold'", extra_tags="", fail_silently=False, ) From 0d2a89de01f94674dbab11a530c42807ddc72292 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 14:54:21 -0600 Subject: [PATCH 17/21] Fix typo --- 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 19669605f..5976f0370 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -729,7 +729,7 @@ class DomainAdmin(ListHeaderAdmin): obj.save() except RegistryError as err: # Using variables to get past the linter - message1 = f"Cannot delete Domain when in status {obj.status}" + message1 = f"Cannot delete Domain when in state {obj.state}" message2 = "This subdomain is being used as a hostname on another domain" # Human-readable mappings of ErrorCodes. Can be expanded. error_messages = { From f2184e5c37766396e78484cb84137e16af0c7647 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Sun, 1 Oct 2023 17:08:33 -0600 Subject: [PATCH 18/21] Code cleanup / do not display on DELETED --- .../templates/django/admin/domain_change_form.html | 2 ++ src/registrar/tests/test_models_domain.py | 7 +++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/django/admin/domain_change_form.html b/src/registrar/templates/django/admin/domain_change_form.html index cc6261af0..ac26fc922 100644 --- a/src/registrar/templates/django/admin/domain_change_form.html +++ b/src/registrar/templates/django/admin/domain_change_form.html @@ -15,7 +15,9 @@ {% endif %} + {% if original.state != original.State.DELETED %} + {% endif %}
{{ block.super }} {% endblock %} diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 45f3ff271..56190da13 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -980,10 +980,13 @@ class TestAnalystDelete(MockEppLib): ) ] ) + # 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) + # Cache should be invalidated self.assertEqual(self.domain._cache, {}) @@ -1001,7 +1004,7 @@ class TestAnalystDelete(MockEppLib): # Put the domain in client hold domain.place_client_hold() - # Delete it... + # Delete it with self.assertRaises(RegistryError) as err: domain.deletedInEpp() self.assertTrue( @@ -1016,7 +1019,7 @@ class TestAnalystDelete(MockEppLib): ) ] ) - # TODO - check UI for error + # Domain itself should not be deleted self.assertNotEqual(domain, None) # State should not have changed From cf0b342bbb4afe798d975f58258abdd7b4f55cf5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 2 Oct 2023 12:45:46 -0600 Subject: [PATCH 19/21] Message instead of value err --- src/registrar/admin.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5976f0370..a806d7dc7 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -721,9 +721,13 @@ class DomainAdmin(ListHeaderAdmin): def do_delete_domain(self, request, obj): if not isinstance(obj, Domain): # Could be problematic if the type is similar, - # but not the same (same field/func names) so we err out. + # but not the same (same field/func names). # We do not want to accidentally delete records. - raise ValueError("Object is not of type Domain") + self.message_user( + request, "Object is not of type Domain", messages.ERROR + ) + return + try: obj.deletedInEpp() obj.save() From bb66043e59f567e31d56f81747d8d3e545384809 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 2 Oct 2023 12:56:22 -0600 Subject: [PATCH 20/21] Reformat --- src/registrar/admin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a806d7dc7..275f67bb3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -723,9 +723,7 @@ class DomainAdmin(ListHeaderAdmin): # Could be problematic if the type is similar, # but not the same (same field/func names). # We do not want to accidentally delete records. - self.message_user( - request, "Object is not of type Domain", messages.ERROR - ) + self.message_user(request, "Object is not of type Domain", messages.ERROR) return try: From 57115d8b48aa3b5f7c30d5579adf08266727a569 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Mon, 2 Oct 2023 12:06:24 -0700 Subject: [PATCH 21/21] Add Erin Song analyst and admin data --- src/registrar/fixtures.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index a4e75dd2e..521d632d6 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -82,6 +82,11 @@ class UserFixture: "first_name": "Nicolle", "last_name": "LeClair", }, + { + "username": "24840450-bf47-4d89-8aa9-c612fe68f9da", + "first_name": "Erin", + "last_name": "Song", + }, ] STAFF = [ @@ -134,6 +139,12 @@ class UserFixture: "last_name": "LeClair-Analyst", "email": "nicolle.leclair@ecstech.com", }, + { + "username": "378d0bc4-d5a7-461b-bd84-3ae6f6864af9", + "first_name": "Erin-Analyst", + "last_name": "Song-Analyst", + "email": "erin.song+1@gsa.gov", + }, ] STAFF_PERMISSIONS = [