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 01/16] 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.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 @@ def response_change(self, request, obj): 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 @@ def is_active(self) -> bool: """ 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 @@ def _delete_domain(self): """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 @@ def pendingCreate(self): self.addAllDefaults() + + def addAllDefaults(self): security_contact = self.get_default_security_contact() security_contact.save() @@ -805,15 +803,34 @@ def revert_client_hold(self): # 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 @@ def withdraw(self): "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 @@ def reject(self): (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 @@ def mockSend(self, _request, cleaned): # 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 @@ def test_update_is_unsuccessful(self): 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 @@ def test_deletion_is_unsuccessful(self): 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 02/16] Commit for #901 --- src/registrar/admin.py | 57 ++++--- 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, 223 insertions(+), 112 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.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 @@ def response_change(self, request, obj): 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, + f"This domain is already deleted", + messages.INFO, ) - elif err.code: + else: self.message_user( request, - f"Error deleting this Domain: {err}", + "Error deleting this Domain: " + f"Can't switch from state '{obj.state}' to 'deleted'" + , 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, - ("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 @@ 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 @@ def revert_client_hold(self): 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 @@ def deletedInEpp(self): 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 @@

Domains

{{ 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 @@ def mockSend(self, _request, cleaned): 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 @@ def setUp(self): 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 @@ def test_place_and_remove_hold(self): 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.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 @@ def test_analyst_deletes_domain(self): 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 03/16] 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 . 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 @@ def do_delete_domain(self, request, obj): 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 @@ def do_delete_domain(self, request, obj): 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 @@ def pendingCreate(self): self.addAllDefaults() - - def addAllDefaults(self): security_contact = self.get_default_security_contact() security_contact.save() @@ -802,7 +800,9 @@ def revert_client_hold(self): 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 @@ def deletedInEpp(self): 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 @@ def withdraw(self): "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 @@ def test_deletion_is_successful(self): ) 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 @@ def test_deletion_ready_fsm_failure(self): ) 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 @@ def test_analyst_deletes_domain_idempotent(self): 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 @@ def test_analyst_deletes_domain_idempotent(self): ) 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.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 @@ def test_update_is_unsuccessful(self): 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 @@ def test_deletion_is_unsuccessful(self): """ # 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 @@ def test_deletion_is_unsuccessful(self): 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 @@ def test_deletion_ready_fsm_failure(self): 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 04/16] 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 @@ def test_analyst_deletes_domain(self): 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 05/16] 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 @@ def _delete_domain(self): """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 @@ def test_deletion_is_successful(self): 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 @@ def test_deletion_ready_fsm_failure(self): 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 @@ def test_analyst_deletes_domain_idempotent(self): 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 06/16] 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 @@ def do_delete_domain(self, request, obj): 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 @@ def do_delete_domain(self, request, obj): # 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 @@ def do_delete_domain(self, request, obj): 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 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 07/16] 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 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 @@ def do_delete_domain(self, request, obj): 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 @@ 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 @@ def reject(self): 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 @@ def reject_with_prejudice(self): 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 @@ def test_form_choices_when_ineligible(self): ) -class TestDomainApplicationAdmin(TestCase): +class TestDomainApplicationAdmin(MockEppLib): def setUp(self): + super().setUp() self.site = AdminSite() self.factory = RequestFactory() self.admin = DomainApplicationAdmin( @@ -839,6 +840,7 @@ def custom_is_active(self): 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 08/16] 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 @@ def do_delete_domain(self, request, obj): 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 09/16] 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 @@ def form_valid(self, form): 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 10/16] 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 @@ def form_valid(self, form): 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 11/16] 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 @@ def do_delete_domain(self, request, obj): 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 @@ def do_delete_domain(self, request, obj): 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 @@ def test_deletion_is_successful(self): ) 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 @@ def test_deletion_ready_fsm_failure(self): ) 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 @@ def test_analyst_deletes_domain_idempotent(self): ) 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 @@ def test_analyst_deletes_domain_idempotent(self): # 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 12/16] 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 @@ def test_deletion_ready_fsm_failure(self): 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 13/16] 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 @@ def do_delete_domain(self, request, obj): 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 14/16] 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 @@ def test_analyst_deletes_domain(self): ) ] ) + # 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 @@ def test_deletion_is_unsuccessful(self): # 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 @@ def test_deletion_is_unsuccessful(self): ) ] ) - # 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 15/16] 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 @@ def response_change(self, 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. + # 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 16/16] 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 @@ def do_delete_domain(self, request, obj): # 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: