From 059585d3e195b8bb120333fa5136353fc687ddee Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 09:28:33 -0600
Subject: [PATCH 01/17] add try/catch and update model

---
 .../0090_alter_publiccontact_registry_id.py   | 24 +++++++++++++
 src/registrar/models/domain.py                | 36 ++++++++++---------
 src/registrar/models/public_contact.py        |  1 +
 3 files changed, 45 insertions(+), 16 deletions(-)
 create mode 100644 src/registrar/migrations/0090_alter_publiccontact_registry_id.py

diff --git a/src/registrar/migrations/0090_alter_publiccontact_registry_id.py b/src/registrar/migrations/0090_alter_publiccontact_registry_id.py
new file mode 100644
index 000000000..c8bdc1574
--- /dev/null
+++ b/src/registrar/migrations/0090_alter_publiccontact_registry_id.py
@@ -0,0 +1,24 @@
+# Generated by Django 4.2.10 on 2024-05-02 15:27
+
+from django.db import migrations, models
+import registrar.models.public_contact
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("registrar", "0089_user_verification_type"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="publiccontact",
+            name="registry_id",
+            field=models.CharField(
+                default=registrar.models.public_contact.get_id,
+                help_text="Auto generated ID to track this contact in the registry",
+                max_length=16,
+                unique=True,
+            ),
+        ),
+    ]
diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 7f53bb234..c49050d4a 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -7,7 +7,7 @@ from typing import Optional
 
 from django_fsm import FSMField, transition, TransitionNotAllowed  # type: ignore
 
-from django.db import models
+from django.db import IntegrityError, models
 from django.utils import timezone
 from typing import Any
 from registrar.models.host import Host
@@ -758,7 +758,7 @@ class Domain(TimeStampedModel, DomainHelper):
         so follow on additions will update the current registrant"""
 
         logger.info("making registrant contact")
-        self._set_singleton_contact(contact=contact, expectedType=contact.ContactTypeChoices.REGISTRANT)
+        self.try_set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.REGISTRANT)
 
     @Cache
     def administrative_contact(self) -> PublicContact | None:
@@ -769,10 +769,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @administrative_contact.setter  # type: ignore
     def administrative_contact(self, contact: PublicContact):
         logger.info("making admin contact")
-        if contact.contact_type != contact.ContactTypeChoices.ADMINISTRATIVE:
-            raise ValueError("Cannot set a registrant contact with a different contact type")
-        self._make_contact_in_registry(contact=contact)
-        self._update_domain_with_contact(contact, rem=False)
+        self.try_set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.ADMINISTRATIVE)
 
     def _update_epp_contact(self, contact: PublicContact):
         """Sends UpdateContact to update the actual contact object,
@@ -832,6 +829,16 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.error("Error changing to new registrant error code is %s, error is %s" % (e.code, e))
             # TODO-error handling better here?
 
+    def try_set_singleton_contact(self, contact: PublicContact, expected_type: str):  # noqa
+        """Runs the _set_singleton_contact function, while catching an IntegrityError
+        from the DB and handling it appropriately"""
+        try:
+            self._set_singleton_contact(contact=contact, expectedType=expected_type)
+        except IntegrityError as err:
+            # If this error occurs, it indicates that our DB tried to create 
+            # a duplicate PublicContact
+            logger.error(f"A contact with this registry ID already exists. Error: {err}")
+
     def _set_singleton_contact(self, contact: PublicContact, expectedType: str):  # noqa
         """Sets the contacts by adding them to the registry as new contacts,
         updates the contact if it is already in epp,
@@ -850,12 +857,13 @@ class Domain(TimeStampedModel, DomainHelper):
         # get publicContact objects that have the matching
         # domain and type but a different id
         # like in highlander we there can only be one
-        hasOtherContact = (
+        duplicate_contacts = (
             PublicContact.objects.exclude(registry_id=contact.registry_id)
             .filter(domain=self, contact_type=contact.contact_type)
-            .exists()
         )
 
+
+
         # if no record exists with this contact type
         # make contact in registry, duplicate and errors handled there
         errorCode = self._make_contact_in_registry(contact)
@@ -871,14 +879,10 @@ class Domain(TimeStampedModel, DomainHelper):
         logger.info("_set_singleton_contact()-> contact has been added to the registry")
 
         # if has conflicting contacts in our db remove them
-        if hasOtherContact:
+        if duplicate_contacts.exists():
             logger.info("_set_singleton_contact()-> updating domain, removing old contact")
 
-            existing_contact = (
-                PublicContact.objects.exclude(registry_id=contact.registry_id)
-                .filter(domain=self, contact_type=contact.contact_type)
-                .get()
-            )
+            existing_contact = duplicate_contacts.get()
 
             if isRegistrant:
                 # send update domain only for registant contacts
@@ -925,7 +929,7 @@ class Domain(TimeStampedModel, DomainHelper):
         from domain information (not domain request)
         and should have the security email from DomainRequest"""
         logger.info("making security contact in registry")
-        self._set_singleton_contact(contact, expectedType=contact.ContactTypeChoices.SECURITY)
+        self.try_set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.SECURITY)
 
     @Cache
     def technical_contact(self) -> PublicContact | None:
@@ -936,7 +940,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @technical_contact.setter  # type: ignore
     def technical_contact(self, contact: PublicContact):
         logger.info("making technical contact")
-        self._set_singleton_contact(contact, expectedType=contact.ContactTypeChoices.TECHNICAL)
+        self.try_set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.TECHNICAL)
 
     def is_active(self) -> bool:
         """Currently just returns if the state is created,
diff --git a/src/registrar/models/public_contact.py b/src/registrar/models/public_contact.py
index 00d065404..5b3679995 100644
--- a/src/registrar/models/public_contact.py
+++ b/src/registrar/models/public_contact.py
@@ -48,6 +48,7 @@ class PublicContact(TimeStampedModel):
         help_text="For which type of WHOIS contact",
     )
     registry_id = models.CharField(
+        unique=True,
         max_length=16,
         default=get_id,
         null=False,

From 7ba71f4f2c5abff8092adfbde7498b8b15d07f0a Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 09:46:07 -0600
Subject: [PATCH 02/17] Delete duplicate on get_or_create

---
 src/registrar/models/domain.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index c49050d4a..2dbe72560 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1966,10 +1966,11 @@ class Domain(TimeStampedModel, DomainHelper):
             domain=self,
         )
 
-        # Raise an error if we find duplicates.
-        # This should not occur
+        # If we find duplicates, log it and delete the newest one.
         if db_contact.count() > 1:
-            raise Exception(f"Multiple contacts found for {public_contact.contact_type}")
+            logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
+            newest_duplicate = db_contact.order_by('-created_at').first()
+            newest_duplicate.delete()
 
         # Save to DB if it doesn't exist already.
         if db_contact.count() == 0:
@@ -1981,16 +1982,14 @@ class Domain(TimeStampedModel, DomainHelper):
 
         existing_contact = db_contact.get()
 
-        # Does the item we're grabbing match
-        # what we have in our DB?
+        # Does the item we're grabbing match what we have in our DB?
         if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id:
             existing_contact.delete()
             public_contact.save()
             logger.warning("Requested PublicContact is out of sync " "with DB.")
             return public_contact
-        # If it already exists, we can
-        # assume that the DB instance was updated
-        # during set, so we should just use that.
+
+        # If it already exists, we can assume that the DB instance was updated during set, so we should just use that.
         return existing_contact
 
     def _registrant_to_public_contact(self, registry_id: str):

From a9509ebdcef177c162c45a011b5c3f7d7faaec1d Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 12:10:01 -0600
Subject: [PATCH 03/17] Update domain.py

---
 src/registrar/models/domain.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 2dbe72560..18a7cab91 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1969,6 +1969,7 @@ class Domain(TimeStampedModel, DomainHelper):
         # If we find duplicates, log it and delete the newest one.
         if db_contact.count() > 1:
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
+            # Q: Should we be deleting the newest or the oldest? Does it even matter?
             newest_duplicate = db_contact.order_by('-created_at').first()
             newest_duplicate.delete()
 

From 41089aadfdd83c7bf58863a1902e22cf85aa3e16 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 13:00:02 -0600
Subject: [PATCH 04/17] Cleanup

---
 src/registrar/models/domain.py | 65 ++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 18a7cab91..e8b52eb78 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -830,15 +830,68 @@ class Domain(TimeStampedModel, DomainHelper):
             # TODO-error handling better here?
 
     def try_set_singleton_contact(self, contact: PublicContact, expected_type: str):  # noqa
-        """Runs the _set_singleton_contact function, while catching an IntegrityError
-        from the DB and handling it appropriately"""
+        """
+        Attempts to set a singleton contact for a domain, 
+        handling potential IntegrityErrors due to duplicate entries.
+
+        This function wraps the `_set_singleton_contact` method, 
+        which is intended to assign a unique contact of a specified type to a domain.
+
+        If a duplicate is detected, all duplicate entries are deleted before retrying the operation.
+        
+        Args:
+            contact (PublicContact): The contact instance to set as a singleton.
+            expected_type (str): The expected type of the contact (e.g., 'technical', 'administrative').
+
+        Raises:
+            IntegrityError: Re-raises the IntegrityError if an unexpected number of duplicates are found,
+            or on the rerun of _set_singleton_contact.
+        """
         try:
             self._set_singleton_contact(contact=contact, expectedType=expected_type)
         except IntegrityError as err:
-            # If this error occurs, it indicates that our DB tried to create 
-            # a duplicate PublicContact
+            # If this error occurs, it indicates that our DB tried to create a duplicate PublicContact
             logger.error(f"A contact with this registry ID already exists. Error: {err}")
 
+            # Delete the duplicates and try again.
+            duplicates = PublicContact.objects.filter(
+                registry_id=contact.registry_id,
+                contact_type=contact.contact_type,
+                domain=self,
+            )
+
+            # If we find duplicates, log it and delete the newest one.
+            if duplicates.count() <= 1:
+                # Something weird happened - we got an integrity error with one or fewer records
+                # which should not be possible.
+                logger.error(
+                    "try_set_singleton_contact() -> "
+                    f"An IntegrityError was raised but {duplicates.count()} entries exist"
+                )
+                # Raise the error. This is a case in which it is appropriate to do so.
+                raise err
+            else:
+                logger.warning("try_set_singleton_contact() -> Duplicate contacts found. Deleting duplicate.")
+                self._delete_duplicates(duplicates)
+            
+            # Try to set the contact again. If we get an error at this point, then
+            # this indicates a very bad data issue in the DB (duplicate registry_ids on other domains).
+            # This requires patching the DB and epp with a script due to the epp <---> DB link.
+            # Trying to fix that here will just result in corrupt EPP records.
+            self._set_singleton_contact(contact=contact, expectedType=expected_type)
+
+    def _delete_duplicates(self, duplicates):
+        """Given a list of duplicates, delete all but the oldest one."""
+
+        # Q: Should we be deleting the newest or the oldest? Does it even matter?
+        oldest_duplicate = duplicates.order_by('created_at').first()
+
+        # Exclude the oldest entry
+        duplicates_to_delete = duplicates.exclude(id=oldest_duplicate.id)
+
+        # Delete all duplicates
+        duplicates_to_delete.delete()
+
     def _set_singleton_contact(self, contact: PublicContact, expectedType: str):  # noqa
         """Sets the contacts by adding them to the registry as new contacts,
         updates the contact if it is already in epp,
@@ -1969,9 +2022,7 @@ class Domain(TimeStampedModel, DomainHelper):
         # If we find duplicates, log it and delete the newest one.
         if db_contact.count() > 1:
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
-            # Q: Should we be deleting the newest or the oldest? Does it even matter?
-            newest_duplicate = db_contact.order_by('-created_at').first()
-            newest_duplicate.delete()
+            self._delete_duplicates(db_contact)
 
         # Save to DB if it doesn't exist already.
         if db_contact.count() == 0:

From a2540128e78cee6a0aae7e4fcffa11b6b9be6d76 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 15:17:49 -0600
Subject: [PATCH 05/17] Cleanup

---
 src/registrar/models/domain.py | 59 +++-------------------------------
 1 file changed, 4 insertions(+), 55 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index e8b52eb78..4f64b3089 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -758,7 +758,7 @@ class Domain(TimeStampedModel, DomainHelper):
         so follow on additions will update the current registrant"""
 
         logger.info("making registrant contact")
-        self.try_set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.REGISTRANT)
+        self._set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.REGISTRANT)
 
     @Cache
     def administrative_contact(self) -> PublicContact | None:
@@ -769,7 +769,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @administrative_contact.setter  # type: ignore
     def administrative_contact(self, contact: PublicContact):
         logger.info("making admin contact")
-        self.try_set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.ADMINISTRATIVE)
+        self._set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.ADMINISTRATIVE)
 
     def _update_epp_contact(self, contact: PublicContact):
         """Sends UpdateContact to update the actual contact object,
@@ -829,57 +829,6 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.error("Error changing to new registrant error code is %s, error is %s" % (e.code, e))
             # TODO-error handling better here?
 
-    def try_set_singleton_contact(self, contact: PublicContact, expected_type: str):  # noqa
-        """
-        Attempts to set a singleton contact for a domain, 
-        handling potential IntegrityErrors due to duplicate entries.
-
-        This function wraps the `_set_singleton_contact` method, 
-        which is intended to assign a unique contact of a specified type to a domain.
-
-        If a duplicate is detected, all duplicate entries are deleted before retrying the operation.
-        
-        Args:
-            contact (PublicContact): The contact instance to set as a singleton.
-            expected_type (str): The expected type of the contact (e.g., 'technical', 'administrative').
-
-        Raises:
-            IntegrityError: Re-raises the IntegrityError if an unexpected number of duplicates are found,
-            or on the rerun of _set_singleton_contact.
-        """
-        try:
-            self._set_singleton_contact(contact=contact, expectedType=expected_type)
-        except IntegrityError as err:
-            # If this error occurs, it indicates that our DB tried to create a duplicate PublicContact
-            logger.error(f"A contact with this registry ID already exists. Error: {err}")
-
-            # Delete the duplicates and try again.
-            duplicates = PublicContact.objects.filter(
-                registry_id=contact.registry_id,
-                contact_type=contact.contact_type,
-                domain=self,
-            )
-
-            # If we find duplicates, log it and delete the newest one.
-            if duplicates.count() <= 1:
-                # Something weird happened - we got an integrity error with one or fewer records
-                # which should not be possible.
-                logger.error(
-                    "try_set_singleton_contact() -> "
-                    f"An IntegrityError was raised but {duplicates.count()} entries exist"
-                )
-                # Raise the error. This is a case in which it is appropriate to do so.
-                raise err
-            else:
-                logger.warning("try_set_singleton_contact() -> Duplicate contacts found. Deleting duplicate.")
-                self._delete_duplicates(duplicates)
-            
-            # Try to set the contact again. If we get an error at this point, then
-            # this indicates a very bad data issue in the DB (duplicate registry_ids on other domains).
-            # This requires patching the DB and epp with a script due to the epp <---> DB link.
-            # Trying to fix that here will just result in corrupt EPP records.
-            self._set_singleton_contact(contact=contact, expectedType=expected_type)
-
     def _delete_duplicates(self, duplicates):
         """Given a list of duplicates, delete all but the oldest one."""
 
@@ -982,7 +931,7 @@ class Domain(TimeStampedModel, DomainHelper):
         from domain information (not domain request)
         and should have the security email from DomainRequest"""
         logger.info("making security contact in registry")
-        self.try_set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.SECURITY)
+        self._set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.SECURITY)
 
     @Cache
     def technical_contact(self) -> PublicContact | None:
@@ -993,7 +942,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @technical_contact.setter  # type: ignore
     def technical_contact(self, contact: PublicContact):
         logger.info("making technical contact")
-        self.try_set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.TECHNICAL)
+        self._set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.TECHNICAL)
 
     def is_active(self) -> bool:
         """Currently just returns if the state is created,

From 9058e0dc97955d1da59f4893c66395922350962d Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 15:24:30 -0600
Subject: [PATCH 06/17] Update domain.py

---
 src/registrar/models/domain.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 4f64b3089..a8004eb07 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -758,7 +758,7 @@ class Domain(TimeStampedModel, DomainHelper):
         so follow on additions will update the current registrant"""
 
         logger.info("making registrant contact")
-        self._set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.REGISTRANT)
+        self._set_singleton_contact(contact=contact, expectedType=contact.ContactTypeChoices.REGISTRANT)
 
     @Cache
     def administrative_contact(self) -> PublicContact | None:
@@ -769,7 +769,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @administrative_contact.setter  # type: ignore
     def administrative_contact(self, contact: PublicContact):
         logger.info("making admin contact")
-        self._set_singleton_contact(contact=contact, expected_type=contact.ContactTypeChoices.ADMINISTRATIVE)
+        self._set_singleton_contact(contact=contact, expectedType=contact.ContactTypeChoices.ADMINISTRATIVE)
 
     def _update_epp_contact(self, contact: PublicContact):
         """Sends UpdateContact to update the actual contact object,
@@ -931,7 +931,7 @@ class Domain(TimeStampedModel, DomainHelper):
         from domain information (not domain request)
         and should have the security email from DomainRequest"""
         logger.info("making security contact in registry")
-        self._set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.SECURITY)
+        self._set_singleton_contact(contact, expectedType=contact.ContactTypeChoices.SECURITY)
 
     @Cache
     def technical_contact(self) -> PublicContact | None:
@@ -942,7 +942,7 @@ class Domain(TimeStampedModel, DomainHelper):
     @technical_contact.setter  # type: ignore
     def technical_contact(self, contact: PublicContact):
         logger.info("making technical contact")
-        self._set_singleton_contact(contact, expected_type=contact.ContactTypeChoices.TECHNICAL)
+        self._set_singleton_contact(contact, expectedType=contact.ContactTypeChoices.TECHNICAL)
 
     def is_active(self) -> bool:
         """Currently just returns if the state is created,

From 5c78618ac0437fa50bf6ff80f96ee3a082bf34b8 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Thu, 2 May 2024 15:29:45 -0600
Subject: [PATCH 07/17] Cleanup

---
 src/registrar/models/domain.py | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index a8004eb07..6d6b628ad 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -829,18 +829,6 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.error("Error changing to new registrant error code is %s, error is %s" % (e.code, e))
             # TODO-error handling better here?
 
-    def _delete_duplicates(self, duplicates):
-        """Given a list of duplicates, delete all but the oldest one."""
-
-        # Q: Should we be deleting the newest or the oldest? Does it even matter?
-        oldest_duplicate = duplicates.order_by('created_at').first()
-
-        # Exclude the oldest entry
-        duplicates_to_delete = duplicates.exclude(id=oldest_duplicate.id)
-
-        # Delete all duplicates
-        duplicates_to_delete.delete()
-
     def _set_singleton_contact(self, contact: PublicContact, expectedType: str):  # noqa
         """Sets the contacts by adding them to the registry as new contacts,
         updates the contact if it is already in epp,
@@ -859,13 +847,10 @@ class Domain(TimeStampedModel, DomainHelper):
         # get publicContact objects that have the matching
         # domain and type but a different id
         # like in highlander we there can only be one
-        duplicate_contacts = (
-            PublicContact.objects.exclude(registry_id=contact.registry_id)
-            .filter(domain=self, contact_type=contact.contact_type)
+        duplicate_contacts = PublicContact.objects.exclude(registry_id=contact.registry_id).filter(
+            domain=self, contact_type=contact.contact_type
         )
 
-
-
         # if no record exists with this contact type
         # make contact in registry, duplicate and errors handled there
         errorCode = self._make_contact_in_registry(contact)
@@ -1971,7 +1956,15 @@ class Domain(TimeStampedModel, DomainHelper):
         # If we find duplicates, log it and delete the newest one.
         if db_contact.count() > 1:
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
-            self._delete_duplicates(db_contact)
+
+            # Q: Should we be deleting the newest or the oldest? Does it even matter?
+            oldest_duplicate = db_contact.order_by("created_at").first()
+
+            # Exclude the oldest entry
+            duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
+
+            # Delete all duplicates
+            duplicates_to_delete.delete()
 
         # Save to DB if it doesn't exist already.
         if db_contact.count() == 0:

From 92746e606180a4815388a904fd1039d339cafeda Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Fri, 3 May 2024 11:16:18 -0600
Subject: [PATCH 08/17] Add some logging, use unique_together rather than
 unique

---
 .../0090_alter_publiccontact_registry_id.py   | 24 -------------------
 ...090_alter_publiccontact_unique_together.py | 17 +++++++++++++
 src/registrar/models/domain.py                | 13 ++++++++--
 src/registrar/models/public_contact.py        | 10 +++++++-
 4 files changed, 37 insertions(+), 27 deletions(-)
 delete mode 100644 src/registrar/migrations/0090_alter_publiccontact_registry_id.py
 create mode 100644 src/registrar/migrations/0090_alter_publiccontact_unique_together.py

diff --git a/src/registrar/migrations/0090_alter_publiccontact_registry_id.py b/src/registrar/migrations/0090_alter_publiccontact_registry_id.py
deleted file mode 100644
index c8bdc1574..000000000
--- a/src/registrar/migrations/0090_alter_publiccontact_registry_id.py
+++ /dev/null
@@ -1,24 +0,0 @@
-# Generated by Django 4.2.10 on 2024-05-02 15:27
-
-from django.db import migrations, models
-import registrar.models.public_contact
-
-
-class Migration(migrations.Migration):
-
-    dependencies = [
-        ("registrar", "0089_user_verification_type"),
-    ]
-
-    operations = [
-        migrations.AlterField(
-            model_name="publiccontact",
-            name="registry_id",
-            field=models.CharField(
-                default=registrar.models.public_contact.get_id,
-                help_text="Auto generated ID to track this contact in the registry",
-                max_length=16,
-                unique=True,
-            ),
-        ),
-    ]
diff --git a/src/registrar/migrations/0090_alter_publiccontact_unique_together.py b/src/registrar/migrations/0090_alter_publiccontact_unique_together.py
new file mode 100644
index 000000000..a476bfa04
--- /dev/null
+++ b/src/registrar/migrations/0090_alter_publiccontact_unique_together.py
@@ -0,0 +1,17 @@
+# Generated by Django 4.2.10 on 2024-05-03 17:06
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("registrar", "0089_user_verification_type"),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name="publiccontact",
+            unique_together={("contact_type", "registry_id", "domain")},
+        ),
+    ]
diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 6d6b628ad..0719dd075 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1207,24 +1207,28 @@ class Domain(TimeStampedModel, DomainHelper):
 
     def get_default_security_contact(self):
         """Gets the default security contact."""
+        logger.info("get_default_security_contact() -> Adding default security contact")
         contact = PublicContact.get_default_security()
         contact.domain = self
         return contact
 
     def get_default_administrative_contact(self):
         """Gets the default administrative contact."""
+        logger.info("get_default_security_contact() -> Adding administrative security contact")
         contact = PublicContact.get_default_administrative()
         contact.domain = self
         return contact
 
     def get_default_technical_contact(self):
         """Gets the default technical contact."""
+        logger.info("get_default_security_contact() -> Adding technical security contact")
         contact = PublicContact.get_default_technical()
         contact.domain = self
         return contact
 
     def get_default_registrant_contact(self):
         """Gets the default registrant contact."""
+        logger.info("get_default_security_contact() -> Adding default registrant contact")
         contact = PublicContact.get_default_registrant()
         contact.domain = self
         return contact
@@ -1238,6 +1242,7 @@ class Domain(TimeStampedModel, DomainHelper):
         Returns:
             PublicContact | None
         """
+        logger.info(f"get_contact_in_keys() -> Grabbing a {contact_type} contact from cache")
         # Registrant doesn't exist as an array, and is of
         # a special data type, so we need to handle that.
         if contact_type == PublicContact.ContactTypeChoices.REGISTRANT:
@@ -1300,6 +1305,7 @@ class Domain(TimeStampedModel, DomainHelper):
                     logger.error(e.code)
                     raise e
                 if e.code == ErrorCode.OBJECT_DOES_NOT_EXIST and self.state == Domain.State.UNKNOWN:
+                    logger.info("_get_or_create_domain() -> Switching to dns_needed from unknown")
                     # avoid infinite loop
                     already_tried_to_create = True
                     self.dns_needed_from_unknown()
@@ -1310,6 +1316,7 @@ class Domain(TimeStampedModel, DomainHelper):
                     raise e
 
     def addRegistrant(self):
+        """Adds a default registrant contact"""
         registrant = PublicContact.get_default_registrant()
         registrant.domain = self
         registrant.save()  # calls the registrant_contact.setter
@@ -1337,6 +1344,8 @@ class Domain(TimeStampedModel, DomainHelper):
         self.addAllDefaults()
 
     def addAllDefaults(self):
+        """Adds default security, technical, and administrative contacts"""
+        logger.info("addAllDefaults() -> Adding default security, technical, and administrative contacts")
         security_contact = self.get_default_security_contact()
         security_contact.save()
 
@@ -1351,7 +1360,7 @@ class Domain(TimeStampedModel, DomainHelper):
         """place a clienthold on a domain (no longer should resolve)
         ignoreEPP (boolean) - set to true to by-pass EPP (used for transition domains)
         """
-        # TODO - ensure all requirements for client hold are made here
+
         # (check prohibited statuses)
         logger.info("clientHold()-> inside clientHold")
 
@@ -1359,7 +1368,6 @@ class Domain(TimeStampedModel, DomainHelper):
         # include this ignoreEPP flag
         if not ignoreEPP:
             self._place_client_hold()
-        # TODO -on the client hold ticket any additional error handling here
 
     @transition(field="state", source=[State.READY, State.ON_HOLD], target=State.READY)
     def revert_client_hold(self, ignoreEPP=False):
@@ -1561,6 +1569,7 @@ class Domain(TimeStampedModel, DomainHelper):
 
     def _get_or_create_contact(self, contact: PublicContact):
         """Try to fetch info about a contact. Create it if it does not exist."""
+        logger.info(f"_get_or_create_contact() -> Fetching contact info")
         try:
             return self._request_contact_info(contact)
         except RegistryError as e:
diff --git a/src/registrar/models/public_contact.py b/src/registrar/models/public_contact.py
index 5b3679995..b5dde3dd9 100644
--- a/src/registrar/models/public_contact.py
+++ b/src/registrar/models/public_contact.py
@@ -19,6 +19,15 @@ def get_id():
 class PublicContact(TimeStampedModel):
     """Contact information intended to be published in WHOIS."""
 
+    class Meta:
+        """Contains meta info about this class"""
+        # Creates a composite primary key with these fields.
+        # We can share the same registry id, but only if the contact type is
+        # different or if the domain is different. 
+        # For instance - we don't desire to have two admin contacts with the same id
+        # on the same domain.
+        unique_together = [("contact_type", "registry_id", "domain")]
+
     class ContactTypeChoices(models.TextChoices):
         """These are the types of contacts accepted by the registry."""
 
@@ -48,7 +57,6 @@ class PublicContact(TimeStampedModel):
         help_text="For which type of WHOIS contact",
     )
     registry_id = models.CharField(
-        unique=True,
         max_length=16,
         default=get_id,
         null=False,

From 8f606312101e02aac237491a6b5266df22087072 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Fri, 3 May 2024 12:12:17 -0600
Subject: [PATCH 09/17] Cleanup

---
 src/registrar/models/domain.py         | 4 ++--
 src/registrar/models/public_contact.py | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 0719dd075..a8953200d 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -7,7 +7,7 @@ from typing import Optional
 
 from django_fsm import FSMField, transition, TransitionNotAllowed  # type: ignore
 
-from django.db import IntegrityError, models
+from django.db import models
 from django.utils import timezone
 from typing import Any
 from registrar.models.host import Host
@@ -1967,7 +1967,7 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
 
             # Q: Should we be deleting the newest or the oldest? Does it even matter?
-            oldest_duplicate = db_contact.order_by("created_at").first()
+            oldest_duplicate = db_contact.order_by("created_at")
 
             # Exclude the oldest entry
             duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
diff --git a/src/registrar/models/public_contact.py b/src/registrar/models/public_contact.py
index b5dde3dd9..71ed07de5 100644
--- a/src/registrar/models/public_contact.py
+++ b/src/registrar/models/public_contact.py
@@ -21,9 +21,10 @@ class PublicContact(TimeStampedModel):
 
     class Meta:
         """Contains meta info about this class"""
+
         # Creates a composite primary key with these fields.
         # We can share the same registry id, but only if the contact type is
-        # different or if the domain is different. 
+        # different or if the domain is different.
         # For instance - we don't desire to have two admin contacts with the same id
         # on the same domain.
         unique_together = [("contact_type", "registry_id", "domain")]

From ec26f03b44d30b7b2019f16c5f0d0ad83ff65638 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Fri, 3 May 2024 12:21:30 -0600
Subject: [PATCH 10/17] Update domain.py

---
 src/registrar/models/domain.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index a8953200d..2bcc50292 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1967,7 +1967,7 @@ class Domain(TimeStampedModel, DomainHelper):
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
 
             # Q: Should we be deleting the newest or the oldest? Does it even matter?
-            oldest_duplicate = db_contact.order_by("created_at")
+            oldest_duplicate = db_contact.order_by("created_at").first()
 
             # Exclude the oldest entry
             duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)

From 9bd0d9e6b7c33c4d795494fbe21a9041396d010c Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Mon, 6 May 2024 08:09:51 -0600
Subject: [PATCH 11/17] Update domain.py

---
 src/registrar/models/domain.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 2bcc50292..a63abf364 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1569,7 +1569,7 @@ class Domain(TimeStampedModel, DomainHelper):
 
     def _get_or_create_contact(self, contact: PublicContact):
         """Try to fetch info about a contact. Create it if it does not exist."""
-        logger.info(f"_get_or_create_contact() -> Fetching contact info")
+        logger.info("_get_or_create_contact() -> Fetching contact info")
         try:
             return self._request_contact_info(contact)
         except RegistryError as e:

From 06c605a62fa5b24d4f66d0b0d1c6a5f5fb723356 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Mon, 6 May 2024 08:15:46 -0600
Subject: [PATCH 12/17] Overzealous linter

---
 src/registrar/models/domain.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index a63abf364..92c38b8d7 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1969,8 +1969,8 @@ class Domain(TimeStampedModel, DomainHelper):
             # Q: Should we be deleting the newest or the oldest? Does it even matter?
             oldest_duplicate = db_contact.order_by("created_at").first()
 
-            # Exclude the oldest entry
-            duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
+            # Exclude the oldest
+            duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)  # noqa
 
             # Delete all duplicates
             duplicates_to_delete.delete()

From c3db67bf1d50dbdbd91cae77a9d2c8b80602f698 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Mon, 6 May 2024 08:24:00 -0600
Subject: [PATCH 13/17] Add check for linter

---
 src/registrar/models/domain.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 92c38b8d7..359df1c27 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1969,8 +1969,13 @@ class Domain(TimeStampedModel, DomainHelper):
             # Q: Should we be deleting the newest or the oldest? Does it even matter?
             oldest_duplicate = db_contact.order_by("created_at").first()
 
-            # Exclude the oldest
-            duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)  # noqa
+            # The linter wants this check on the id, though in practice
+            # this should be otherwise impossible.
+            if hasattr(oldest_duplicate, "id"):
+                # Exclude the oldest
+                duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
+            else:
+                duplicates_to_delete = db_contact
 
             # Delete all duplicates
             duplicates_to_delete.delete()

From 86717945dec79312b4c8cf1db87c4a99b6e0b3ae Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Mon, 6 May 2024 08:31:46 -0600
Subject: [PATCH 14/17] Update domain.py

---
 src/registrar/models/domain.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index 359df1c27..f2640e8ec 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -1962,16 +1962,16 @@ class Domain(TimeStampedModel, DomainHelper):
             domain=self,
         )
 
-        # If we find duplicates, log it and delete the newest one.
+        # If we find duplicates, log it and delete the newest ones.
         if db_contact.count() > 1:
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
 
             # Q: Should we be deleting the newest or the oldest? Does it even matter?
             oldest_duplicate = db_contact.order_by("created_at").first()
 
-            # The linter wants this check on the id, though in practice
+            # The linter wants this check on the id / oldest_duplicate, though in practice
             # this should be otherwise impossible.
-            if hasattr(oldest_duplicate, "id"):
+            if oldest_duplicate is not None and hasattr(oldest_duplicate, "id"):
                 # Exclude the oldest
                 duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
             else:
@@ -1980,6 +1980,13 @@ class Domain(TimeStampedModel, DomainHelper):
             # Delete all duplicates
             duplicates_to_delete.delete()
 
+            # Do a second filter to grab the latest content
+            db_contact = PublicContact.objects.filter(
+                registry_id=public_contact.registry_id,
+                contact_type=public_contact.contact_type,
+                domain=self,
+            )
+
         # Save to DB if it doesn't exist already.
         if db_contact.count() == 0:
             # Doesn't run custom save logic, just saves to DB

From cb5d9d2368505f99549be8f27c8034aea8282b1f Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Tue, 7 May 2024 08:47:32 -0600
Subject: [PATCH 15/17] PR suggestions

---
 src/registrar/models/domain.py | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py
index f2640e8ec..4c9028bb4 100644
--- a/src/registrar/models/domain.py
+++ b/src/registrar/models/domain.py
@@ -846,7 +846,7 @@ class Domain(TimeStampedModel, DomainHelper):
 
         # get publicContact objects that have the matching
         # domain and type but a different id
-        # like in highlander we there can only be one
+        # like in highlander where there can only be one
         duplicate_contacts = PublicContact.objects.exclude(registry_id=contact.registry_id).filter(
             domain=self, contact_type=contact.contact_type
         )
@@ -1962,20 +1962,13 @@ class Domain(TimeStampedModel, DomainHelper):
             domain=self,
         )
 
-        # If we find duplicates, log it and delete the newest ones.
+        # If we find duplicates, log it and delete the oldest ones.
         if db_contact.count() > 1:
             logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.")
 
-            # Q: Should we be deleting the newest or the oldest? Does it even matter?
-            oldest_duplicate = db_contact.order_by("created_at").first()
+            newest_duplicate = db_contact.order_by("-created_at").first()
 
-            # The linter wants this check on the id / oldest_duplicate, though in practice
-            # this should be otherwise impossible.
-            if oldest_duplicate is not None and hasattr(oldest_duplicate, "id"):
-                # Exclude the oldest
-                duplicates_to_delete = db_contact.exclude(id=oldest_duplicate.id)
-            else:
-                duplicates_to_delete = db_contact
+            duplicates_to_delete = db_contact.exclude(id=newest_duplicate.id)  # type: ignore
 
             # Delete all duplicates
             duplicates_to_delete.delete()

From 185dabe72dee459198a45bdd16053a671420859d Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Tue, 7 May 2024 08:52:02 -0600
Subject: [PATCH 16/17] Fix migrations after merge with latest

---
 ...ogether.py => 0091_alter_publiccontact_unique_together.py} | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 rename src/registrar/migrations/{0090_alter_publiccontact_unique_together.py => 0091_alter_publiccontact_unique_together.py} (73%)

diff --git a/src/registrar/migrations/0090_alter_publiccontact_unique_together.py b/src/registrar/migrations/0091_alter_publiccontact_unique_together.py
similarity index 73%
rename from src/registrar/migrations/0090_alter_publiccontact_unique_together.py
rename to src/registrar/migrations/0091_alter_publiccontact_unique_together.py
index a476bfa04..ff5a18beb 100644
--- a/src/registrar/migrations/0090_alter_publiccontact_unique_together.py
+++ b/src/registrar/migrations/0091_alter_publiccontact_unique_together.py
@@ -1,4 +1,4 @@
-# Generated by Django 4.2.10 on 2024-05-03 17:06
+# Generated by Django 4.2.10 on 2024-05-07 14:51
 
 from django.db import migrations
 
@@ -6,7 +6,7 @@ from django.db import migrations
 class Migration(migrations.Migration):
 
     dependencies = [
-        ("registrar", "0089_user_verification_type"),
+        ("registrar", "0090_waffleflag"),
     ]
 
     operations = [

From 5f9edd78cbe59e30c8a7aa5a68e6a7639cf4ef59 Mon Sep 17 00:00:00 2001
From: zandercymatics <141044360+zandercymatics@users.noreply.github.com>
Date: Wed, 8 May 2024 11:35:25 -0600
Subject: [PATCH 17/17] Add migration

---
 ...ogether.py => 0093_alter_publiccontact_unique_together.py} | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 rename src/registrar/migrations/{0091_alter_publiccontact_unique_together.py => 0093_alter_publiccontact_unique_together.py} (65%)

diff --git a/src/registrar/migrations/0091_alter_publiccontact_unique_together.py b/src/registrar/migrations/0093_alter_publiccontact_unique_together.py
similarity index 65%
rename from src/registrar/migrations/0091_alter_publiccontact_unique_together.py
rename to src/registrar/migrations/0093_alter_publiccontact_unique_together.py
index ff5a18beb..08c71e122 100644
--- a/src/registrar/migrations/0091_alter_publiccontact_unique_together.py
+++ b/src/registrar/migrations/0093_alter_publiccontact_unique_together.py
@@ -1,4 +1,4 @@
-# Generated by Django 4.2.10 on 2024-05-07 14:51
+# Generated by Django 4.2.10 on 2024-05-08 17:35
 
 from django.db import migrations
 
@@ -6,7 +6,7 @@ from django.db import migrations
 class Migration(migrations.Migration):
 
     dependencies = [
-        ("registrar", "0090_waffleflag"),
+        ("registrar", "0092_rename_updated_federal_agency_domaininformation_federal_agency_and_more"),
     ]
 
     operations = [