Address feedback from Neil

src/registrar/models/domain.py

@@ -294,11 +294,12 @@ class Domain(TimeStampedModel, DomainHelper):
         return newDict
 
     def isSubdomain(self, nameserver):
-        return nameserver.find(self.name) != -1
+        return self.name in nameserver
 
     def checkHostIPCombo(self, nameserver: str, ip: list):
         if self.isSubdomain(nameserver) and (ip is None or ip == []):
-            raise ValueError("Nameserver %s needs to have an ip address" % nameserver)
+            raise ValueError("Nameserver %s needs to have an "
+                "ip address because it is a subdomain" % nameserver)
         elif not self.isSubdomain(nameserver) and (ip is not None and ip != []):
             raise ValueError(
                 "Nameserver %s cannot be linked "
@@ -306,15 +307,15 @@ class Domain(TimeStampedModel, DomainHelper):
             )
         return None
 
-    def getNameserverChanges(self, hosts: list[tuple[str]]):
+    def getNameserverChanges(self, hosts: list[tuple[str]]) -> tuple[list, list, dict, list]:
         """
         calls self.nameserver, it should pull from cache but may result
         in an epp call
         returns tuple of four values as follows:
-        deleted_values:
+        deleted_values: list
-        updated_values:
+        updated_values: list
         new_values: dict
-        oldNameservers:"""
+        oldNameservers: list"""
         oldNameservers = self.nameservers
 
         previousHostDict = self._convert_list_to_dict(oldNameservers)
@@ -417,7 +418,7 @@ class Domain(TimeStampedModel, DomainHelper):
             oldNameservers,
         ) = self.getNameserverChanges(hosts=hosts)
 
-        # TODO-848: Fix name here
+        # TODO-848: Fix name
         successTotalNameservers = self._loop_through(
             deleted_values, updated_values, new_values, oldNameservers
         )
@@ -1103,9 +1104,9 @@ class Domain(TimeStampedModel, DomainHelper):
                 and len(old_ip_list) != 0
             ):
                 return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY
-
+                
-            added_ip_list = set(ip_list) - set(old_ip_list)
+            added_ip_list = set(ip_list).difference(old_ip_list)
-            removed_ip_list = set(old_ip_list) - set(ip_list)
+            removed_ip_list = set(old_ip_list).difference(ip_list)
 
             request = commands.UpdateHost(
                 name=nameserver,

src/registrar/tests/common.py

@@ -631,8 +631,10 @@ class MockEppLib(TestCase):
             "ns2.nameserverwithip.gov",
             "ns3.nameserverwithip.gov",
         ],
+        addrs=["1.2.3", "2.3.4"],
     )
 
+    # TODO-848: Fix naming 
     extendedValues = False
 
     # TODO-848: Rename later - was getting complex err

src/registrar/tests/test_models_domain.py

@@ -544,15 +544,64 @@ class TestRegistrantNameservers(MockEppLib):
             name="my-nameserver.gov", state=Domain.State.DNS_NEEDED
         )
 
-    def test_get_nameserver_changes(self):
+    def test_get_nameserver_changes_success_deleted_vals(self):
+        # Testing only deleting and no other changes
         self.domain._cache["hosts"] = [
             {"name": "ns1.example.com", "addrs": None},
             {"name": "ns2.example.com", "addrs": ["1.2.3"]},
-            {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]},
         ]
         newChanges = [
             ("ns1.example.com",),
+        ]
+        (
+            deleted_values,
+            updated_values,
+            new_values,
+            oldNameservers,
+        ) = self.domain.getNameserverChanges(newChanges)
+
+        self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])])
+        self.assertEqual(updated_values, [])
+        self.assertEqual(new_values, {})
+        self.assertEqual(
+            oldNameservers,
+            {
+            'ns1.example.com': None, 'ns2.example.com': ['1.2.3']
+            },
+        )
+
+    def test_get_nameserver_changes_success_updated_vals(self):
+        # Testing only updating no other changes
+        self.domain._cache["hosts"] = [
+            {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]},
+        ]
+        newChanges = [
             ("ns3.my-nameserver.gov", ["1.2.4"]),
+        ]
+        (
+            deleted_values,
+            updated_values,
+            new_values,
+            oldNameservers,
+        ) = self.domain.getNameserverChanges(newChanges)
+
+        self.assertEqual(deleted_values, [])
+        self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])])
+        self.assertEqual(new_values, {})
+        self.assertEqual(
+            oldNameservers,
+            {
+            "ns3.my-nameserver.gov": ["1.2.3"]
+            },
+        )
+
+    def test_get_nameserver_changes_success_new_vals(self):
+        # Testing only creating no other changes
+        self.domain._cache["hosts"] = [
+            {"name": "ns1.example.com", "addrs": None},
+        ]
+        newChanges = [
+            ("ns1.example.com",),
             ("ns4.example.com",),
         ]
         (
@@ -561,15 +610,19 @@ class TestRegistrantNameservers(MockEppLib):
             new_values,
             oldNameservers,
         ) = self.domain.getNameserverChanges(newChanges)
-        self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])])
+
-        self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])])
+
+        print("deleted vals is ", deleted_values)
+        print("updated vals is ", updated_values)
+        print("new vals is ", new_values)
+        print("old nameserver is ", oldNameservers)
+        self.assertEqual(deleted_values, [])
+        self.assertEqual(updated_values, [])
         self.assertEqual(new_values, {"ns4.example.com": None})
         self.assertEqual(
             oldNameservers,
             {
                 "ns1.example.com": None,
-                "ns2.example.com": ["1.2.3"],
-                "ns3.my-nameserver.gov": ["1.2.3"],
             },
         )
 
@@ -601,7 +654,8 @@ class TestRegistrantNameservers(MockEppLib):
 
         self.mockedSendFunction.assert_has_calls(expectedCalls)
 
-        # check that status is still NOT READY
+        # check that status is still NOT READY 
+        # as you have less than 2 nameservers
         self.assertFalse(self.domain.is_active())
 
     def test_user_adds_two_nameservers(self):