Requested PR changes

Removed skip on test_domain_security_email_form, fixed common.py, extended contact_error.py, fixed domain_detail not showing the right data, small cleanup of domain.py

src/registrar/models/domain.py

@@ -14,7 +14,7 @@ from epplibwrapper import (
     RegistryError,
     ErrorCode,
 )
-from registrar.models.utility.contact_error import ContactError
+from registrar.models.utility.contact_error import ContactError, ContactErrorCodes
 
 from .utility.domain_field import DomainField
 from .utility.domain_helper import DomainHelper
@@ -676,10 +676,10 @@ class Domain(TimeStampedModel, DomainHelper):
             return None
 
         if contact_type is None:
-            raise ContactError("contact_type is None")
+            raise ContactError(code=ContactErrorCodes.CONTACT_TYPE_NONE)
 
         if contact_id is None:
-            raise ContactError("contact_id is None")
+            raise ContactError(code=ContactErrorCodes.CONTACT_ID_NONE)
 
         # Since contact_id is registry_id,
         # check that its the right length
@@ -727,13 +727,25 @@ class Domain(TimeStampedModel, DomainHelper):
     def _convert_streets_to_dict(self, streets):
         """
         Converts EPPLibs street representation
-        to PublicContacts
+        to PublicContacts.
+
+        Args:
+            streets (Sequence[str]): Streets from EPPLib.
+
+        Returns:
+            dict: {
+                "street1": str or "",
+
+                "street2": str or None,
+
+                "street3": str or None,
+            }
 
         EPPLib returns 'street' as an sequence of strings.
         Meanwhile, PublicContact has this split into three
         seperate properties: street1, street2, street3.
 
-        Handles this disparity
+        Handles this disparity.
         """
         # 'zips' two lists together.
         # For instance, (('street1', 'some_value_here'),
@@ -1117,26 +1129,27 @@ class Domain(TimeStampedModel, DomainHelper):
 
     def _fetch_contacts(self, contact_data):
         """Fetch contact info."""
-        contacts = []
+        choices = PublicContact.ContactTypeChoices
+        # We expect that all these fields get populated,
+        # so we can create these early, rather than waiting.
+        contacts_dict = {
+            choices.ADMINISTRATIVE: None,
+            choices.SECURITY: None,
+            choices.TECHNICAL: None,
+        }
         for domainContact in contact_data:
             req = commands.InfoContact(id=domainContact.contact)
             data = registry.send(req, cleaned=True).res_data[0]
-            contact = {
+
-                "id": domainContact.contact,
+            # Map the object we recieved from EPP to a PublicContact
-                "type": domainContact.type,
+            mapped_object = self.map_epp_contact_to_public_contact(
-                "auth_info": getattr(data, "auth_info", ...),
+                data, domainContact.contact, domainContact.type
-                "cr_date": getattr(data, "cr_date", ...),
+            )
-                "disclose": getattr(data, "disclose", ...),
+
-                "email": getattr(data, "email", ...),
+            # Find/create it in the DB
-                "fax": getattr(data, "fax", ...),
+            in_db = self._get_or_create_public_contact(mapped_object)
-                "postal_info": getattr(data, "postal_info", ...),
+            contacts_dict[in_db.contact_type] = in_db.registry_id
-                "statuses": getattr(data, "statuses", ...),
+        return contacts_dict
-                "tr_date": getattr(data, "tr_date", ...),
-                "up_date": getattr(data, "up_date", ...),
-                "voice": getattr(data, "voice", ...),
-            }
-            contacts.append({k: v for k, v in contact.items() if v is not ...})
-        return contacts
 
     def _get_or_create_contact(self, contact: PublicContact):
         """Try to fetch info about a contact. Create it if it does not exist."""
@@ -1224,27 +1237,7 @@ class Domain(TimeStampedModel, DomainHelper):
                 and isinstance(cleaned["_contacts"], list)
                 and len(cleaned["_contacts"]) > 0
             ):
-                choices = PublicContact.ContactTypeChoices
+                cleaned["contacts"] = self._fetch_contacts(cleaned["_contacts"])
-                # We expect that all these fields get populated,
-                # so we can create these early, rather than waiting.
-                cleaned["contacts"] = {
-                    choices.ADMINISTRATIVE: None,
-                    choices.SECURITY: None,
-                    choices.TECHNICAL: None,
-                }
-                for domainContact in cleaned["_contacts"]:
-                    req = commands.InfoContact(id=domainContact.contact)
-                    data = registry.send(req, cleaned=True).res_data[0]
-
-                    # Map the object we recieved from EPP to a PublicContact
-                    mapped_object = self.map_epp_contact_to_public_contact(
-                        data, domainContact.contact, domainContact.type
-                    )
-
-                    # Find/create it in the DB
-                    in_db = self._get_or_create_public_contact(mapped_object)
-                    cleaned["contacts"][in_db.contact_type] = in_db.registry_id
-
                 # We're only getting contacts, so retain the old
                 # hosts that existed in cache (if they existed)
                 # and pass them along.

src/registrar/models/utility/contact_error.py

@@ -1,2 +1,47 @@
+from enum import IntEnum
+
+
+class ContactErrorCodes(IntEnum):
+    """
+    Used in the ContactError class for
+    error mapping.
+
+    Overview of contact error codes:
+        - 2000 CONTACT_TYPE_NONE
+        - 2001 CONTACT_ID_NONE
+        - 2002 CONTACT_ID_INVALID_LENGTH
+        - 2003 CONTACT_INVALID_TYPE
+    """
+
+    CONTACT_TYPE_NONE = 2000
+    CONTACT_ID_NONE = 2001
+    CONTACT_ID_INVALID_LENGTH = 2002
+    CONTACT_INVALID_TYPE = 2003
+
+
 class ContactError(Exception):
-    ...
+    """
+    Overview of contact error codes:
+        - 2000 CONTACT_TYPE_NONE
+        - 2001 CONTACT_ID_NONE
+        - 2002 CONTACT_ID_INVALID_LENGTH
+        - 2003 CONTACT_INVALID_TYPE
+    """
+    # For linter
+    _contact_id_error = "contact_id has an invalid length. Cannot exceed 16 characters."
+    _contact_invalid_error = "Contact must be of type InfoContactResultData"
+    _error_mapping = {
+        ContactErrorCodes.CONTACT_TYPE_NONE: "contact_type is None",
+        ContactErrorCodes.CONTACT_ID_NONE: "contact_id is None",
+        ContactErrorCodes.CONTACT_ID_INVALID_LENGTH: _contact_id_error,
+        ContactErrorCodes.CONTACT_INVALID_TYPE: _contact_invalid_error,
+    }
+
+    def __init__(self, *args, code=None, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.code = code
+        if self.code in self._error_mapping:
+            self.message = self._error_mapping.get(self.code)
+
+    def __str__(self):
+        return f"{self.message}"

src/registrar/templates/domain_detail.html

@@ -46,7 +46,7 @@
     {% include "includes/summary_item.html" with title='Your contact information' value=request.user.contact contact='true' edit_link=url %}
 
     {% url 'domain-security-email' pk=domain.id as url %}
-    {% include "includes/summary_item.html" with title='Security email' value=domain.security_email edit_link=url %}
+    {% include "includes/summary_item.html" with title='Security email' value=domain.get_security_email() edit_link=url %}
 
     {% url 'domain-users' pk=domain.id as url %}
     {% include "includes/summary_item.html" with title='User management' users='true' list=True value=domain.permissions.all edit_link=url %}

src/registrar/tests/common.py

@@ -606,7 +606,7 @@ class MockEppLib(TestCase):
             return fake
 
     mockDataInfoDomain = fakedEppObject(
-        "lastPw",
+        "fakePw",
         cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35),
         contacts=[
             common.DomainContact(

src/registrar/tests/test_views.py

@@ -1466,7 +1466,6 @@ class TestDomainDetail(TestWithDomainPermissions, WebTest, MockEppLib):
         )
         self.assertContains(page, "Domain security email")
 
-    @skip("Ticket 912 needs to fix this one")
     def test_domain_security_email_form(self):
         """Adding a security email works.
         Uses self.app WebTest because we need to interact with forms.