Commit for #901

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():
+            # Human-readable mappings of ErrorCodes. Can be expanded.
-                self.message_user(
+            error_messages = {
-                    request,
+                ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION: 
-                    "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,
+                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,
+                    f"This domain is already deleted",
+                    messages.INFO,
                 )
-            elif err.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION:
+            else:
                 self.message_user(
                     request,
                     "Error deleting this Domain: "
-                    f" This subdomain is being used as a hostname on another domain",
+                    f"Can't switch from state '{obj.state}' to 'deleted'"
-                    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,
-                ("Domain %s Should now be deleted " ". Thanks!") % obj.name,
+                ("Domain %s has been deleted. Thanks!") % obj.name,
             )
         return HttpResponseRedirect(".")
 

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.
+        """Domain is deleted in epp but is saved in our database.
-        Returns the request_code"""
+        Error handling should be provided by the caller."""
-        valid_delete_states = [
+        # While we want to log errors, we want to preserve
-            self.State.ON_HOLD,
+        # that information when this function is called.
-            self.State.DNS_NEEDED
+        # Human-readable errors are introduced at the admin.py level,
-        ]
+        # as doing everything here would reduce reliablity.
-        # 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()
@@ -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}"

src/registrar/templates/django/admin/domain_change_form.html

@@ -8,9 +8,9 @@
 
 {% block field_sets %}
     <div class="submit-row">
-        {% if original.state == original.State.READY %}
+        {% if original.state == original.State.READY%}
           <input type="submit" value="Place hold" name="_place_client_hold">
-        {% elif original.state == original.State.ON_HOLD %}
+        {% elif original.state == original.State.ON_HOLD%}
           <input type="submit" value="Remove hold" name="_remove_client_hold">
         {% endif %}
         <input id="manageDomainSubmitButton" type="submit" value="Manage Domain" name="_edit_domain">

src/registrar/templates/home.html

@@ -40,6 +40,9 @@
             {{ domain.name }}
           </th>
           <td data-sort-value="{{ domain.created_time|date:"U" }}" data-label="Date created">{{ domain.created_time|date }}</td>
+          {% comment %} Should this be domain.status? 
+          <td data-label="Status">{{ domain.status|title }}</td>
+          {% endcomment %}
           <td data-label="Status">{{ domain.application_status|title }}</td>
           <td>
             <a href="{% url "domain" pk=domain.pk %}">

src/registrar/tests/common.py

@@ -606,10 +606,13 @@ 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"
         ):
+            name = getattr(_request, "name", None)
+            fake_nameserver = "ns1.failDelete.gov"
+            if name in fake_nameserver:
                 raise RegistryError(
-                code=ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION
+                    code=ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION
                 )
         return MagicMock(res_data=[self.mockDataInfoHosts])
 

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

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,71 +983,37 @@ class TestAnalystDelete(MockEppLib):
         self.assertNotEqual(self.domain, None)
         # Domain should have the right state
         self.assertEqual(self.domain.state, Domain.State.DELETED)
-
+        # Cache should be invalidated
-    def test_analyst_deletes_domain_idempotent(self):
+        self.assertEqual(self.domain._cache, {})
-        """
-        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)
 
     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
@@ -1055,25 +1021,23 @@ class TestAnalystDelete(MockEppLib):
         # 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.assertEqual(self.domain.state, Domain.State.READY)
+        with self.assertRaises(TransitionNotAllowed) as err:
             self.domain.deletedInEpp()
-        self.mockedSendFunction.assert_has_calls(
+            self.assertTrue(
-            [
+                err.is_client_error() 
-                call(
+                and err.code == ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION
-                    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")
+        self.assertEqual(self.domain.state, Domain.State.READY)