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)