changed UpdateDomain on nameserver.setter to be called with all the added/removed hosts in one call

src/registrar/models/domain.py

@@ -366,7 +366,7 @@ class Domain(TimeStampedModel, DomainHelper):
             # get deleted values-which are values in previous nameserver list
             # but are not in the list of new host values
             if prevHost not in newHostDict:
-                deleted_values.append((prevHost, addrs))
+                deleted_values.append(prevHost)
             # if the host exists in both, check if the addresses changed
             else:
                 # TODO - host is being updated when previous was None+new is empty list
@@ -388,16 +388,6 @@ class Domain(TimeStampedModel, DomainHelper):
 
         return (deleted_values, updated_values, new_values, previousHostDict)
 
-    def _deleted_host_values(self, deleted_values) -> int:
-        successDeletedCount = 0
-
-        for hostTuple in deleted_values:
-            deleted_response_code = self._delete_host(hostTuple[0])
-            if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY:
-                successDeletedCount += 1
-
-        return successDeletedCount
-
     def _update_host_values(self, updated_values, oldNameservers):
         for hostTuple in updated_values:
             updated_response_code = self._update_host(
@@ -412,8 +402,15 @@ class Domain(TimeStampedModel, DomainHelper):
                     % (hostTuple[0], updated_response_code)
                 )
 
-    def _new_host_values(self, new_values) -> int:
+    def createNewHostList(self, new_values: dict) -> list:
-        successCreatedCount = 0
+        """convert the dictionary of new values to a list of HostObjSet
+        for use in the UpdateDomain epp message
+        Args:
+            new_values: dict(str,list)- dict of {nameserver:ips} to add to domain
+        Returns:
+            list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message
+        """
+        addToDomainList = []
         for key, value in new_values.items():
             createdCode = self._create_host(
                 host=key, addrs=value
@@ -422,18 +419,21 @@ class Domain(TimeStampedModel, DomainHelper):
                 createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY
                 or createdCode == ErrorCode.OBJECT_EXISTS
             ):
-                request = commands.UpdateDomain(
+                addToDomainList.append(epp.HostObjSet([key]))
-                    name=self.name, add=[epp.HostObjSet([key])]
+
-                )
+        return addToDomainList
-                try:
+
-                    registry.send(request, cleaned=True)
+    def createDeleteHostList(self, hostsToDelete: list[str]):
-                    successCreatedCount += 1
+        """
-                except RegistryError as e:
+        Args:
-                    logger.error(
+            hostsToDelete (list[str])- list of nameserver/host names to remove
-                        "Error adding nameserver, code was %s error was %s"
+        Returns:
-                        % (e.code, e)
+            list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message
-                    )
+        """
-        return successCreatedCount
+        deleteList = []
+        for nameserver in hostsToDelete:
+            deleteList.append(epp.HostObjSet([nameserver]))
+        return deleteList
 
     @Cache
     def dnssecdata(self) -> extensions.DNSSECExtension:
@@ -479,16 +479,23 @@ class Domain(TimeStampedModel, DomainHelper):
             oldNameservers,
         ) = self.getNameserverChanges(hosts=hosts)
 
-        successCreatedCount = self._new_host_values(new_values)
-        successDeletedCount = self._deleted_host_values(deleted_values)  # returns value
         _ = self._update_host_values(
             updated_values, oldNameservers
         )  # returns nothing, just need to be run and errors
-
+        addToDomainList = self.createNewHostList(new_values)
-        successTotalNameservers = (
+        deleteHostList = self.createDeleteHostList(deleted_values)
-            len(oldNameservers) - successDeletedCount + successCreatedCount
+        responseCode = self.addAndRemoveHostsFromDomain(
+            hostsToAdd=addToDomainList, hostsToDelete=deleteHostList
         )
 
+        # if unable to update domain raise error and stop
+        if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY:
+            raise NameserverError(code=nsErrorCodes.UNABLE_TO_UPDATE_DOMAIN)
+
+        successTotalNameservers = (
+            len(oldNameservers) - len(deleteHostList) + len(addToDomainList)
+        )
+        self._delete_hosts_if_not_used(hostsToDelete=deleted_values)
         if successTotalNameservers < 2:
             try:
                 self.dns_needed()
@@ -1459,36 +1466,64 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.info("_update_host()-> sending req as %s" % request)
             return response.code
         except RegistryError as e:
-            logger.error("Error _delete_host, code was %s error was %s" % (e.code, e))
+            logger.error("Error _update_host, code was %s error was %s" % (e.code, e))
             return e.code
 
-    def _delete_host(self, nameserver: str):
+    def addAndRemoveHostsFromDomain(
-        """Remove this host from the domain and delete the host object in registry,
+        self, hostsToAdd: list[str], hostsToDelete: list[str]
-        will only delete the host object, if it's not being used by another domain
+    ):
-        Performs two epp calls and can result in a RegistryError
+        """sends an UpdateDomain message to the registry with the hosts provided
         Args:
-            nameserver (str): nameserver or subdomain
+            hostsToDelete (list[epp.HostObjSet])- list of host objects to delete
-            ip_list (list[str]): the new list of ips, may be empty
+            hostsToAdd (list[epp.HostObjSet])- list of host objects to add
-            old_ip_list  (list[str]): the old ip list, may also be empty
-
         Returns:
-            errorCode (int): one of ErrorCode enum type values
+            response code (int)- RegistryErrorCode integer value
-
+            defaults to return COMMAND_COMPLETED_SUCCESSFULLY
+            if there is nothing to add or delete
         """
+
+        if hostsToAdd == [] and hostsToDelete == []:
+            return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY
+
         try:
             updateReq = commands.UpdateDomain(
-                name=self.name, rem=[epp.HostObjSet([nameserver])]
+                name=self.name, rem=hostsToDelete, add=hostsToAdd
             )
             response = registry.send(updateReq, cleaned=True)
 
-            logger.info("_delete_host()-> sending update domain req as %s" % updateReq)
-
-            deleteHostReq = commands.DeleteHost(name=nameserver)
-            response = registry.send(deleteHostReq, cleaned=True)
             logger.info(
-                "_delete_host()-> sending delete host req as %s" % deleteHostReq
+                "addAndRemoveHostsFromDomain()-> sending update domain req as %s"
+                % updateReq
             )
-            return response.code
+
+        except RegistryError as e:
+            logger.error(
+                "Error addAndRemoveHostsFromDomain, code was %s error was %s"
+                % (e.code, e)
+            )
+
+        return response.code
+
+    def _delete_hosts_if_not_used(self, hostsToDelete: list[str]):
+        """delete the host object in registry,
+        will only delete the host object, if it's not being used by another domain
+        Performs just the DeleteHost epp call
+        Supresses regstry error, as registry can disallow delete for various reasons
+        Args:
+            hostsToDelete (list[str])- list of nameserver/host names to remove
+        Returns:
+            None
+
+        """
+        try:
+            for nameserver in hostsToDelete:
+                deleteHostReq = commands.DeleteHost(name=nameserver)
+                registry.send(deleteHostReq, cleaned=True)
+                logger.info(
+                    "_delete_hosts_if_not_used()-> sending delete host req as %s"
+                    % deleteHostReq
+                )
+
         except RegistryError as e:
             if e.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION:
                 logger.info(
@@ -1497,9 +1532,9 @@ class Domain(TimeStampedModel, DomainHelper):
                 )
             else:
                 logger.error(
-                    "Error _delete_host, code was %s error was %s" % (e.code, e)
+                    "Error _delete_hosts_if_not_used, code was %s error was %s"
+                    % (e.code, e)
                 )
-                return e.code
 
     def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False):
         """Contact registry for info about a domain."""

src/registrar/tests/common.py

@@ -784,9 +784,12 @@ class MockEppLib(TestCase):
                 res_data=[self.mockDataHostChange],
                 code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY,
             )
-            # elif isinstance(_request, commands.UpdateHost):
+        elif isinstance(_request, commands.UpdateHost):
-            #     if getattr(_request, "name", None) == "ns1.failednameserver.gov":
+            return MagicMock(
-            #         raise RegistryError(code=ErrorCode.OBJECT_EXISTS)
+                res_data=[self.mockDataHostChange],
+                code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY,
+            )
+        elif isinstance(_request, commands.UpdateDomain):
             return MagicMock(
                 res_data=[self.mockDataHostChange],
                 code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY,

src/registrar/tests/test_models_domain.py

@@ -882,7 +882,7 @@ class TestRegistrantNameservers(MockEppLib):
             oldNameservers,
         ) = self.domain.getNameserverChanges(newChanges)
 
-        self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3.4"])])
+        self.assertEqual(deleted_values, ["ns2.example.com"])
         self.assertEqual(updated_values, [])
         self.assertEqual(new_values, {})
         self.assertEqual(
@@ -956,7 +956,9 @@ class TestRegistrantNameservers(MockEppLib):
         # when we create a host, we should've updated at the same time
         created_host = commands.CreateHost(nameserver)
         update_domain_with_created = commands.UpdateDomain(
-            name=self.domain.name, add=[common.HostObjSet([created_host.name])]
+            name=self.domain.name,
+            add=[common.HostObjSet([created_host.name])],
+            rem=[],
         )
 
         # checking if commands were sent (commands have to be sent in order)
@@ -986,13 +988,15 @@ class TestRegistrantNameservers(MockEppLib):
 
         # when you create a host, you also have to update at same time
         created_host1 = commands.CreateHost(self.nameserver1)
-        update_domain_with_created1 = commands.UpdateDomain(
-            name=self.domain.name, add=[common.HostObjSet([created_host1.name])]
-        )
-
         created_host2 = commands.CreateHost(self.nameserver2)
-        update_domain_with_created2 = commands.UpdateDomain(
+
-            name=self.domain.name, add=[common.HostObjSet([created_host2.name])]
+        update_domain_with_created = commands.UpdateDomain(
+            name=self.domain.name,
+            add=[
+                common.HostObjSet([created_host1.name]),
+                common.HostObjSet([created_host2.name]),
+            ],
+            rem=[],
         )
 
         infoDomain = commands.InfoDomain(name="my-nameserver.gov", auth_info=None)
@@ -1000,13 +1004,12 @@ class TestRegistrantNameservers(MockEppLib):
         expectedCalls = [
             call(infoDomain, cleaned=True),
             call(created_host1, cleaned=True),
-            call(update_domain_with_created1, cleaned=True),
             call(created_host2, cleaned=True),
-            call(update_domain_with_created2, cleaned=True),
+            call(update_domain_with_created, cleaned=True),
         ]
 
         self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True)
-        self.assertEqual(5, self.mockedSendFunction.call_count)
+        self.assertEqual(4, self.mockedSendFunction.call_count)
         # check that status is READY
         self.assertTrue(self.domain.is_active())
 
@@ -1116,24 +1119,15 @@ class TestRegistrantNameservers(MockEppLib):
             call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True),
             call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True),
             call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True),
-            call(
-                commands.UpdateDomain(
-                    name=self.domainWithThreeNS.name,
-                    add=[],
-                    rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])],
-                    nsset=None,
-                    keyset=None,
-                    registrant=None,
-                    auth_info=None,
-                ),
-                cleaned=True,
-            ),
             call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True),
             call(
                 commands.UpdateDomain(
                     name=self.domainWithThreeNS.name,
                     add=[],
-                    rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])],
+                    rem=[
+                        common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]),
+                        common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]),
+                    ],
                     nsset=None,
                     keyset=None,
                     registrant=None,
@@ -1157,7 +1151,6 @@ class TestRegistrantNameservers(MockEppLib):
             And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3
             And `commands.DeleteHost` is sent to delete #2 and #3
         """
-        print("in second failing test")
         self.domainWithThreeNS.nameservers = [
             (self.nameserver1,),
             ("ns1.cats-are-superior1.com",),
@@ -1172,35 +1165,11 @@ class TestRegistrantNameservers(MockEppLib):
             call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True),
             call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True),
             call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True),
-            call(
-                commands.UpdateDomain(
-                    name=self.domainWithThreeNS.name,
-                    add=[],
-                    rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])],
-                    nsset=None,
-                    keyset=None,
-                    registrant=None,
-                    auth_info=None,
-                ),
-                cleaned=True,
-            ),
             call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True),
             call(
                 commands.CreateHost(name="ns1.cats-are-superior1.com", addrs=[]),
                 cleaned=True,
             ),
-            call(
-                commands.UpdateDomain(
-                    name=self.domainWithThreeNS.name,
-                    add=[common.HostObjSet(hosts=["ns1.cats-are-superior1.com"])],
-                    rem=[],
-                    nsset=None,
-                    keyset=None,
-                    registrant=None,
-                    auth_info=None,
-                ),
-                cleaned=True,
-            ),
             call(
                 commands.CreateHost(name="ns1.cats-are-superior2.com", addrs=[]),
                 cleaned=True,
@@ -1208,8 +1177,14 @@ class TestRegistrantNameservers(MockEppLib):
             call(
                 commands.UpdateDomain(
                     name=self.domainWithThreeNS.name,
-                    add=[common.HostObjSet(hosts=["ns1.cats-are-superior2.com"])],
+                    add=[
-                    rem=[],
+                        common.HostObjSet(hosts=["ns1.cats-are-superior1.com"]),
+                        common.HostObjSet(hosts=["ns1.cats-are-superior2.com"]),
+                    ],
+                    rem=[
+                        common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]),
+                        common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]),
+                    ],
                     nsset=None,
                     keyset=None,
                     registrant=None,
@@ -1395,7 +1370,6 @@ class TestRegistrantNameservers(MockEppLib):
             domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])]
 
     def tearDown(self):
-        print("in tear down")
         Domain.objects.all().delete()
         return super().tearDown()
 

src/registrar/utility/errors.py

@@ -35,6 +35,7 @@ class NameserverErrorCodes(IntEnum):
     GLUE_RECORD_NOT_ALLOWED = 2
     INVALID_IP = 3
     TOO_MANY_HOSTS = 4
+    UNABLE_TO_UPDATE_DOMAIN = 5
 
 
 class NameserverError(Exception):
@@ -52,6 +53,10 @@ class NameserverError(Exception):
         NameserverErrorCodes.TOO_MANY_HOSTS: (
             "Too many hosts provided, you may not have more than " "13 nameservers."
         ),
+        NameserverErrorCodes.UNABLE_TO_UPDATE_DOMAIN: (
+            "Unable to update domain, changes were not applied."
+            "Check logs as a Registry Error is the likely cause"
+        ),
     }
 
     def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs):