added test cases for place and revert client hold; allowed for idempotent updates

src/epplibwrapper/errors.py

@@ -69,7 +69,7 @@ class RegistryError(Exception):
 
     def is_session_error(self):
         return self.code is not None and (self.code >= 2501 and self.code <= 2502)
-    
+
     def is_server_error(self):
         return self.code is not None and (self.code >= 2400 and self.code <= 2500)
 

src/registrar/admin.py

@@ -317,7 +317,7 @@ class DomainAdmin(ListHeaderAdmin):
                 self.message_user(
                     request,
                     "Error placing the hold with the registry: {err}",
-                    messages.ERROR
+                    messages.ERROR,
                 )
             else:
                 # all other type error messages, display the error
@@ -344,7 +344,7 @@ class DomainAdmin(ListHeaderAdmin):
                 self.message_user(
                     request,
                     "Error removing the hold in the registry: {err}",
-                    messages.ERROR
+                    messages.ERROR,
                 )
             else:
                 # all other type error messages, display the error
@@ -798,131 +798,6 @@ class DomainInformationInline(admin.StackedInline):
         return DomainInformationAdmin.get_readonly_fields(self, request, obj=None)
 
 
-class DomainAdmin(ListHeaderAdmin):
-    """Custom domain admin class to add extra buttons."""
-
-    inlines = [DomainInformationInline]
-
-    # Columns
-    list_display = [
-        "name",
-        "organization_type",
-        "state",
-    ]
-
-    def organization_type(self, obj):
-        return obj.domain_info.organization_type
-
-    organization_type.admin_order_field = (  # type: ignore
-        "domain_info__organization_type"
-    )
-
-    # Filters
-    list_filter = ["domain_info__organization_type", "state"]
-
-    search_fields = ["name"]
-    search_help_text = "Search by domain name."
-    change_form_template = "django/admin/domain_change_form.html"
-    readonly_fields = ["state"]
-
-    def response_change(self, request, obj):
-        # Create dictionary of action functions
-        ACTION_FUNCTIONS = {
-            "_place_client_hold": self.do_place_client_hold,
-            "_remove_client_hold": self.do_remove_client_hold,
-            "_edit_domain": self.do_edit_domain,
-            "_delete_domain": self.do_delete_domain,
-            "_get_status": self.do_get_status,
-        }
-
-        # Check which action button was pressed and call the corresponding function
-        for action, function in ACTION_FUNCTIONS.items():
-            if action in request.POST:
-                return function(request, obj)
-
-        # If no matching action button is found, return the super method
-        return super().response_change(request, obj)
-
-    def do_delete_domain(self, request, obj):
-        try:
-            obj.deleted()
-            obj.save()
-        except Exception as err:
-            self.message_user(request, err, messages.ERROR)
-        else:
-            self.message_user(
-                request,
-                ("Domain %s Should now be deleted " ". Thanks!") % obj.name,
-            )
-        return HttpResponseRedirect(".")
-
-    def do_get_status(self, request, obj):
-        try:
-            statuses = obj.statuses
-        except Exception as err:
-            self.message_user(request, err, messages.ERROR)
-        else:
-            self.message_user(
-                request,
-                ("Domain statuses are %s" ". Thanks!") % statuses,
-            )
-        return HttpResponseRedirect(".")
-
-    def do_place_client_hold(self, request, obj):
-        try:
-            obj.place_client_hold()
-            obj.save()
-        except Exception as err:
-            self.message_user(request, err, messages.ERROR)
-        else:
-            self.message_user(
-                request,
-                (
-                    "%s is in client hold. This domain is no longer accessible on"
-                    " the public internet."
-                )
-                % obj.name,
-            )
-        return HttpResponseRedirect(".")
-
-    def do_remove_client_hold(self, request, obj):
-        try:
-            obj.revert_client_hold()
-            obj.save()
-        except Exception as err:
-            self.message_user(request, err, messages.ERROR)
-        else:
-            self.message_user(
-                request,
-                ("%s is ready. This domain is accessible on the public internet.")
-                % obj.name,
-            )
-        return HttpResponseRedirect(".")
-
-    def do_edit_domain(self, request, obj):
-        # We want to know, globally, when an edit action occurs
-        request.session["analyst_action"] = "edit"
-        # Restricts this action to this domain (pk) only
-        request.session["analyst_action_location"] = obj.id
-        return HttpResponseRedirect(reverse("domain", args=(obj.id,)))
-
-    def change_view(self, request, object_id):
-        # If the analyst was recently editing a domain page,
-        # delete any associated session values
-        if "analyst_action" in request.session:
-            del request.session["analyst_action"]
-            del request.session["analyst_action_location"]
-        return super().change_view(request, object_id)
-
-    def has_change_permission(self, request, obj=None):
-        # Fixes a bug wherein users which are only is_staff
-        # can access 'change' when GET,
-        # but cannot access this page when it is a request of type POST.
-        if request.user.is_staff:
-            return True
-        return super().has_change_permission(request, obj)
-
-
 class DraftDomainAdmin(ListHeaderAdmin):
     """Custom draft domain admin class."""
 

src/registrar/models/domain.py

@@ -639,9 +639,7 @@ class Domain(TimeStampedModel, DomainHelper):
             self._invalidate_cache()
         except RegistryError as err:
             # if registry error occurs, log the error, and raise it as well
-            logger.error(
+            logger.error(f"registry error placing client hold: {err}")
-                f"registry error placing client hold: {err}"
-            )
             raise (err)
 
     def _remove_client_hold(self):
@@ -653,9 +651,7 @@ class Domain(TimeStampedModel, DomainHelper):
             self._invalidate_cache()
         except RegistryError as err:
             # if registry error occurs, log the error, and raise it as well
-            logger.error(
+            logger.error(f"registry error removing client hold: {err}")
-                f"registry error removing client hold: {err}"
-            )
             raise (err)
 
     def _delete_domain(self):
@@ -789,7 +785,9 @@ class Domain(TimeStampedModel, DomainHelper):
         administrative_contact.domain = self
         administrative_contact.save()
 
-    @transition(field="state", source=State.READY, target=State.ON_HOLD)
+    @transition(
+        field="state", source=[State.READY, State.ON_HOLD], target=State.ON_HOLD
+    )
     def place_client_hold(self):
         """place a clienthold on a domain (no longer should resolve)"""
         # TODO - ensure all requirements for client hold are made here
@@ -798,7 +796,7 @@ class Domain(TimeStampedModel, DomainHelper):
         self._place_client_hold()
         # TODO -on the client hold ticket any additional error handling here
 
-    @transition(field="state", source=State.ON_HOLD, target=State.READY)
+    @transition(field="state", source=[State.READY, State.ON_HOLD], target=State.READY)
     def revert_client_hold(self):
         """undo a clienthold placed on a domain"""
 

src/registrar/tests/common.py

@@ -595,12 +595,6 @@ class MockEppLib(TestCase):
             return MagicMock(res_data=[self.mockDataInfoDomain])
         elif isinstance(_request, commands.InfoContact):
             return MagicMock(res_data=[self.mockDataInfoContact])
-        elif (
-            isinstance(_request, commands.UpdateDomain)
-            and getattr(_request, "name", "fake-on-hold.gov")
-            and getattr(_request, "add", [common.Status(state=Domain.Status.CLIENT_HOLD, description='', lang='en')])
-        ):
-            raise RegistryError(code=ErrorCode.)
         elif (
             isinstance(_request, commands.CreateContact)
             and getattr(_request, "id", None) == "fail"

src/registrar/tests/test_models_domain.py

@@ -20,6 +20,8 @@ from .common import MockEppLib
 from epplibwrapper import (
     commands,
     common,
+    RegistryError,
+    ErrorCode,
 )
 
 
@@ -714,40 +716,17 @@ class TestAnalystClientHold(MockEppLib):
         super().setUp()
         # for the tests, need a domain in the ready state
         self.domain, _ = Domain.objects.get_or_create(
-            name="fake.gov",
+            name="fake.gov", state=Domain.State.READY
-            state=Domain.State.READY
         )
         # for the tests, need a domain in the on_hold state
         self.domain_on_hold, _ = Domain.objects.get_or_create(
-            name="fake-on-hold.gov",
+            name="fake-on-hold.gov", state=Domain.State.ON_HOLD
-            state=Domain.State.ON_HOLD
         )
 
     def tearDown(self):
         Domain.objects.all().delete()
         super().tearDown()
 
-    # def test_get_status(self):
-    #     """Domain 'statuses' getter returns statuses by calling epp"""
-    #     domain, _ = Domain.objects.get_or_create(name="chicken-liver.gov")
-    #     # trigger getter
-    #     _ = domain.statuses
-    #     status_list = [status.state for status in self.mockDataInfoDomain.statuses]
-    #     self.assertEquals(domain._cache["statuses"], status_list)
-
-    #     # Called in _fetch_cache
-    #     self.mockedSendFunction.assert_has_calls(
-    #         [
-    #             call(
-    #                 commands.InfoDomain(name="chicken-liver.gov", auth_info=None),
-    #                 cleaned=True,
-    #             ),
-    #             call(commands.InfoContact(id="123", auth_info=None), cleaned=True),
-    #             call(commands.InfoHost(name="fake.host.com"), cleaned=True),
-    #         ],
-    #         any_order=False,  # Ensure calls are in the specified order
-    #     )
-
     def test_analyst_places_client_hold(self):
         """
         Scenario: Analyst takes a domain off the internet
@@ -760,7 +739,13 @@ class TestAnalystClientHold(MockEppLib):
                 call(
                     commands.UpdateDomain(
                         name="fake.gov",
-                        add=[common.Status(state=Domain.Status.CLIENT_HOLD, description='', lang='en')],
+                        add=[
+                            common.Status(
+                                state=Domain.Status.CLIENT_HOLD,
+                                description="",
+                                lang="en",
+                            )
+                        ],
                         nsset=None,
                         keyset=None,
                         registrant=None,
@@ -772,7 +757,6 @@ class TestAnalystClientHold(MockEppLib):
         )
         self.assertEquals(self.domain.state, Domain.State.ON_HOLD)
 
-    @skip("not implemented yet")
     def test_analyst_places_client_hold_idempotent(self):
         """
         Scenario: Analyst tries to place client hold twice
@@ -780,7 +764,29 @@ class TestAnalystClientHold(MockEppLib):
             When `domain.place_client_hold()` is called
             Then Domain returns normally (without error)
         """
-        raise
+        self.domain_on_hold.place_client_hold()
+        self.mockedSendFunction.assert_has_calls(
+            [
+                call(
+                    commands.UpdateDomain(
+                        name="fake-on-hold.gov",
+                        add=[
+                            common.Status(
+                                state=Domain.Status.CLIENT_HOLD,
+                                description="",
+                                lang="en",
+                            )
+                        ],
+                        nsset=None,
+                        keyset=None,
+                        registrant=None,
+                        auth_info=None,
+                    ),
+                    cleaned=True,
+                )
+            ]
+        )
+        self.assertEquals(self.domain_on_hold.state, Domain.State.ON_HOLD)
 
     def test_analyst_removes_client_hold(self):
         """
@@ -795,7 +801,13 @@ class TestAnalystClientHold(MockEppLib):
                 call(
                     commands.UpdateDomain(
                         name="fake-on-hold.gov",
-                        rem=[common.Status(state=Domain.Status.CLIENT_HOLD, description='', lang='en')],
+                        rem=[
+                            common.Status(
+                                state=Domain.Status.CLIENT_HOLD,
+                                description="",
+                                lang="en",
+                            )
+                        ],
                         nsset=None,
                         keyset=None,
                         registrant=None,
@@ -807,7 +819,6 @@ class TestAnalystClientHold(MockEppLib):
         )
         self.assertEquals(self.domain_on_hold.state, Domain.State.READY)
 
-    @skip("not implemented yet")
     def test_analyst_removes_client_hold_idempotent(self):
         """
         Scenario: Analyst tries to remove client hold twice
@@ -815,16 +826,54 @@ class TestAnalystClientHold(MockEppLib):
             When `domain.remove_client_hold()` is called
             Then Domain returns normally (without error)
         """
-        raise
+        self.domain.revert_client_hold()
+        self.mockedSendFunction.assert_has_calls(
+            [
+                call(
+                    commands.UpdateDomain(
+                        name="fake.gov",
+                        rem=[
+                            common.Status(
+                                state=Domain.Status.CLIENT_HOLD,
+                                description="",
+                                lang="en",
+                            )
+                        ],
+                        nsset=None,
+                        keyset=None,
+                        registrant=None,
+                        auth_info=None,
+                    ),
+                    cleaned=True,
+                )
+            ]
+        )
+        self.assertEquals(self.domain.state, Domain.State.READY)
 
-    @skip("not implemented yet")
     def test_update_is_unsuccessful(self):
         """
         Scenario: An update to place or remove client hold is unsuccessful
             When an error is returned from epplibwrapper
             Then a user-friendly error message is returned for displaying on the web
         """
-        raise
+
+        def side_effect(_request, cleaned):
+            raise RegistryError(code=ErrorCode.OBJECT_STATUS_PROHIBITS_OPERATION)
+
+        patcher = patch("registrar.models.domain.registry.send")
+        mocked_send = patcher.start()
+        mocked_send.side_effect = side_effect
+
+        # if RegistryError is raised, admin formats user-friendly
+        # error message if error is_client_error, is_session_error, or
+        # is_server_error; so test for those conditions
+        with self.assertRaises(RegistryError) as err:
+            self.domain.place_client_hold()
+            self.assertTrue(
+                err.is_client_error() or err.is_session_error() or err.is_server_error()
+            )
+
+        patcher.stop()
 
 
 class TestAnalystLock(TestCase):