Merge branch 'za/1852-user-contact-info-inline' into za/1848-copy-contact-email-to-clipboard

src/registrar/admin.py

@@ -911,6 +911,7 @@ class DomainInformationAdmin(ListHeaderAdmin):
         "no_other_contacts_rationale",
         "anything_else",
         "is_policy_acknowledged",
+        "other_contacts",
     ]
 
     # For each filter_horizontal, init in admin js extendFilterHorizontalWidgets
@@ -928,6 +929,8 @@ class DomainInformationAdmin(ListHeaderAdmin):
     # Table ordering
     ordering = ["domain__name"]
 
+    change_form_template = "django/admin/domain_information_change_form.html"
+
     def get_readonly_fields(self, request, obj=None):
         """Set the read-only state on form elements.
         We have 1 conditions that determine which fields are read-only:

src/registrar/templates/admin/fieldset.html

@@ -16,34 +16,49 @@ https://github.com/django/django/blob/main/django/contrib/admin/templates/admin/
         {% endif %}
     {% endblock fieldset_description %}
 
-    {% block fieldset_lines %}
+    {% for line in fieldset %}
-        {% for line in fieldset %}
+        <div class="form-row{% if line.fields|length == 1 and line.errors %} errors{% endif %}{% if not line.has_visible_field %} hidden{% endif %}{% for field in line %}{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% endfor %}">
-            <div class="form-row{% if line.fields|length == 1 and line.errors %} errors{% endif %}{% if not line.has_visible_field %} hidden{% endif %}{% for field in line %}{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% endfor %}">
+            {% if line.fields|length == 1 %}{{ line.errors }}{% else %}<div class="flex-container form-multiline">{% endif %}
-                {% if line.fields|length == 1 %}{{ line.errors }}{% else %}<div class="flex-container form-multiline">{% endif %}
+            {% for field in line %}
-                {% for field in line %}
+                <div>
-                    <div>
+                    {% if not line.fields|length == 1 and not field.is_readonly %}{{ field.errors }}{% endif %}
-                        {% if not line.fields|length == 1 and not field.is_readonly %}{{ field.errors }}{% endif %}
+                        <div class="flex-container{% if not line.fields|length == 1 %} fieldBox{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% if not field.is_readonly and field.errors %} errors{% endif %}{% if field.field.is_hidden %} hidden{% endif %}{% elif field.is_checkbox %} checkbox-row{% endif %}">
-                            <div class="flex-container{% if not line.fields|length == 1 %} fieldBox{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% if not field.is_readonly and field.errors %} errors{% endif %}{% if field.field.is_hidden %} hidden{% endif %}{% elif field.is_checkbox %} checkbox-row{% endif %}">
+                            {% if field.is_checkbox %}
-                                {% if field.is_checkbox %}
+                                {% block field_checkbox %}
-                                    {{ field.field }}{{ field.label_tag }}
+                                {{ field.field }}{{ field.label_tag }}
+                                {% endblock field_checkbox%}
+                            {% else %}
+                                {{ field.label_tag }}
+                                {% if field.is_readonly %}
+                                    {% block field_readonly %}
+                                    <div class="readonly">{{ field.contents }}</div>
+                                    {% endblock field_readonly%}
                                 {% else %}
-                                    {{ field.label_tag }}
+                                    {% block field_other %}
-                                    {% if field.is_readonly %}
+                                    {{ field.field }}
-                                        <div class="readonly">{{ field.contents }}</div>
+                                    {% endblock field_other%}
-                                    {% else %}
-                                        {{ field.field }}
-                                    {% endif %}
                                 {% endif %}
-                            </div>
+                            {% endif %}
-                        {% if field.field.help_text %}
+                        </div>
-                            <div class="help"{% if field.field.id_for_label %} id="{{ field.field.id_for_label }}_helptext"{% endif %}>
+                    
-                                <div>{{ field.field.help_text|safe }}</div>
+                    {% block before_help_text %}
-                            </div>
+                        {# For templating purposes #}
-                        {% endif %}
+                    {% endblock before_help_text %}
-                    </div>
+
-                {% endfor %}
+                    {% if field.field.help_text %}
-                {% if not line.fields|length == 1 %}</div>{% endif %}
+                        {% block help_text %}
-            </div>
+                        <div class="help"{% if field.field.id_for_label %} id="{{ field.field.id_for_label }}_helptext"{% endif %}>
-        {% endfor %}
+                            <div>{{ field.field.help_text|safe }}</div>
-    {% endblock fieldset_lines %}
+                        </div>
+                        {% endblock help_text %}
+                    {% endif %}
+
+                    {% block after_help_text %}
+                        {# For templating purposes #}
+                    {% endblock after_help_text %}
+                </div>
+            {% endfor %}
+            {% if not line.fields|length == 1 %}</div>{% endif %}
+        </div>
+    {% endfor %}
 </fieldset>

src/registrar/templates/django/admin/domain_information_change_form.html

@@ -0,0 +1,8 @@
+{% extends 'admin/change_form.html' %}
+{% load i18n static %}
+
+{% block field_sets %}
+    {% for fieldset in adminform %}
+        {% include "django/admin/includes/domain_information_fieldset.html" %}
+    {% endfor %}
+{% endblock %}

src/registrar/templates/django/admin/includes/detail_table_fieldset.html

@@ -0,0 +1,61 @@
+{% extends "admin/fieldset.html" %}
+{% load static url_helpers %}
+
+{% comment %}
+This is using a custom implementation fieldset.html (see admin/fieldset.html)
+{% endcomment %}
+{% block field_readonly %}
+    {% if field.field.name == "other_contacts" %}
+    <div class="readonly">
+        {% for contact in field.contents|split:", " %}
+            <a href="#" class="other-contact__{{forloop.counter}}">{{ contact }}</a>{% if not forloop.last %}, {% endif %}
+        {% endfor %}
+    </div>
+    {% elif field.field.name == "current_websites" %}
+    {% comment %}
+    The "website" model is essentially just a text field.
+    It is not useful to be redirected to the object definition,
+    rather it is more useful in this scenario to be redirected to the
+    actual website (as its just a plaintext string otherwise).
+
+    This ONLY applies to analysts. For superusers, its business as usual.
+    {% endcomment %}
+    {% for website in field.contents|split:", " %}
+        <a href="{{ website }}" class="padding-top-1 current-website__{{forloop.counter}}">{{ website }}</a>{% if not forloop.last %}, {% endif %}
+    {% endfor %}
+    {% else %}
+    <div class="readonly">{{ field.contents }}</div>
+    {% endif %}
+{% endblock field_readonly %}
+
+{% block after_help_text %}
+    {% if field.field.name == "creator" %}
+        {% include "django/admin/includes/contact_detail_table.html" with user=original.creator field_name="creator" %}
+    {% elif field.field.name == "submitter" %}
+        {% include "django/admin/includes/contact_detail_table.html" with user=original.submitter field_name="submitter" %}
+    {% elif field.field.name == "authorizing_official" %}
+        {% include "django/admin/includes/contact_detail_table.html" with user=original.authorizing_official field_name="authorizing_official" %}
+    {% elif field.field.name == "other_contacts" and original.other_contacts.all %}
+        <details class="margin-top-1 dja-detail-table">
+            <summary class="padding-1 dja-details-summary">Details</summary>
+            <div class="grid-container padding-left-0 padding-right-0 dja-details-contents">
+                <table>
+                    <tbody>
+                        {% for contact in original.other_contacts.all %}
+                        {% comment %}
+                        Since we can't get the id from field, we can embed this information here.
+                        Then we can link these two fields using javascript.
+                        {% endcomment %}
+                        <tr data-contact-id="{{ contact.id }}" data-contact-url="{% url 'admin:registrar_contact_change' contact.id %}">
+                            <th scope="row">{{contact.first_name}} {{contact.last_name}}</th>
+                            <td>{{ contact.title }}</td>
+                            <td>{{ contact.email }}</td>
+                            <td>{{ contact.phone }}</td>
+                        </tr>
+                        {% endfor %}
+                    </tbody>
+                </table>
+            </div>
+        </details>
+    {% endif %}
+{% endblock after_help_text %}

src/registrar/templates/django/admin/includes/domain_information_fieldset.html

@@ -0,0 +1,2 @@
+{% extends "django/admin/includes/detail_table_fieldset.html" %}
+{# Stubbed file for future expansion #}

src/registrar/templates/django/admin/includes/domain_request_fieldset.html

@@ -1,101 +1,2 @@
-{% extends "admin/fieldset.html" %}
+{% extends "django/admin/includes/detail_table_fieldset.html" %}
-{% load static url_helpers %}
+{# Stubbed file for future expansion #}
-{% comment %}
-This is using a custom implementation fieldset.html (see admin/fieldset.html)
-{% endcomment %}
-{% block fieldset_lines %}
-{% for line in fieldset %}
-    <div class="form-row{% if line.fields|length == 1 and line.errors %} errors{% endif %}{% if not line.has_visible_field %} hidden{% endif %}{% for field in line %}{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% endfor %}">
-        {% if line.fields|length == 1 %}{{ line.errors }}{% else %}<div class="flex-container form-multiline">{% endif %}
-        {% for field in line %}
-            <div>
-                {% if not line.fields|length == 1 and not field.is_readonly %}{{ field.errors }}{% endif %}
-                    <div class="flex-container{% if not line.fields|length == 1 %} fieldBox{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% if not field.is_readonly and field.errors %} errors{% endif %}{% if field.field.is_hidden %} hidden{% endif %}{% elif field.is_checkbox %} checkbox-row{% endif %}">
-                        {% if field.is_checkbox %}
-                            {{ field.field }}{{ field.label_tag }}
-                        {% else %}
-                            {{ field.label_tag }}
-                            {% if field.is_readonly %}
-                                {% if field.field.name == "other_contacts" %}
-                                    <div class="readonly">
-                                        {% for contact in field.contents|split:", " %}
-                                            <a href="#" class="other-contact__{{forloop.counter}}">{{ contact }}</a>{% if not forloop.last %}, {% endif %}
-                                        {% endfor %}
-                                    </div>
-                                {% elif field.field.name == "current_websites" %}
-                                    {% comment %}
-                                    The "website" model is essentially just a text field.
-                                    It is not useful to be redirected to the object definition,
-                                    rather it is more useful in this scenario to be redirected to the
-                                    actual website (as its just a plaintext string otherwise).
-
-                                    This ONLY applies to analysts. For superusers, its business as usual.
-                                    {% endcomment %}
-                                    {% for website in field.contents|split:", " %}
-                                        <a href="{{ website }}" class="padding-top-1 current-website__{{forloop.counter}}">{{ website }}</a>{% if not forloop.last %}, {% endif %}
-                                    {% endfor %}
-                                {% else %}
-                                    <div class="readonly">{{ field.contents }}</div>
-                                {% endif %}
-                            {% else %}
-                                {{ field.field }}
-                            {% endif %}
-                        {% endif %}
-                    </div>
-                {% if field.field.name == "creator" %}
-                    {% include "django/admin/includes/domain_request_detail_table.html" with user=original.creator field_name="creator" %}
-                {% elif field.field.name == "submitter" %}
-                    {% include "django/admin/includes/domain_request_detail_table.html" with user=original.submitter field_name="submitter" %}
-                {% elif field.field.name == "authorizing_official" %}
-                    {% include "django/admin/includes/domain_request_detail_table.html" with user=original.authorizing_official field_name="authorizing_official" %}
-                {% elif field.field.name == "other_contacts" %}
-                    <details class="margin-top-1 dja-detail-table">
-                        <summary class="padding-1 dja-details-summary">Details</summary>
-                        <div class="grid-container padding-left-0 padding-right-0 dja-details-contents">
-                            <table>
-                                <tbody>
-                                    {% for contact in original.other_contacts.all %}
-                                    {% comment %}
-                                    Since we can't get the id from field, we can embed this information here.
-                                    Then we can link these two fields using javascript.
-                                    {% endcomment %}
-                                    <tr data-contact-id="{{ contact.id }}" data-contact-url="{% url 'admin:registrar_contact_change' contact.id %}">
-                                        <th scope="row">{{contact.first_name}} {{contact.last_name}}</th>
-                                        <td>{{ contact.title }}</td>
-                                        <td>{{ contact.email }}</td>
-                                        <td>{{ contact.phone }}</td>
-                                        {# Copy button for the email #}
-                                        <td class="font-size-sm">
-                                            <input class="dja-clipboard-input" type="hidden" value="{{ contact.email }}">
-                                            <button 
-                                            class="usa-button usa-button--unstyled usa-button__icon usa-button__clipboard text-no-underline" 
-                                            type="button"
-                                            >
-                                                <svg 
-                                                class="usa-icon" 
-                                                >
-                                                    <use aria-hidden="true" xlink:href="{%static 'img/sprite.svg'%}#content_copy"></use>
-                                                </svg>
-                                                Copy email
-                                            </button>
-                                        </td>
-                                    </tr>
-                                    {% endfor %}
-                                </tbody>
-                            </table>
-                        </div>
-                    </details>
-                {% endif %}
-
-                {% if field.field.help_text %}
-                    <div class="help"{% if field.field.id_for_label %} id="{{ field.field.id_for_label }}_helptext"{% endif %}>
-                        <div>{{ field.field.help_text|safe }}</div>
-                    </div>
-                {% endif %}
-            </div>
-        {% endfor %}
-        {% if not line.fields|length == 1 %}</div>{% endif %}
-    </div>
-{% endfor %}
-
-{% endblock fieldset_lines %}

src/registrar/tests/common.py

@@ -116,6 +116,31 @@ class GenericTestHelper(TestCase):
         self.url = url
         self.client = client
 
+    def assert_response_contains_distinct_values(self, response, expected_values):
+        """
+        Asserts that each specified value appears exactly once in the response.
+
+        This method iterates over a list of tuples, where each tuple contains a field name
+        and its expected value. It then performs an assertContains check for each value,
+        ensuring that each value appears exactly once in the response.
+
+        Parameters:
+        - response: The HttpResponse object to inspect.
+        - expected_values: A list of tuples, where each tuple contains:
+            - field: The name of the field (used for subTest identification).
+            - value: The expected value to check for in the response.
+
+        Example usage:
+        expected_values = [
+            ("title", "Treat inspector</td>"),
+            ("email", "meoward.jones@igorville.gov</td>"),
+        ]
+        self.assert_response_contains_distinct_values(response, expected_values)
+        """
+        for field, value in expected_values:
+            with self.subTest(field=field, expected_value=value):
+                self.assertContains(response, value, count=1)
+
     def assert_table_sorted(self, o_index, sort_fields):
         """
         This helper function validates the sorting functionality of a Django Admin table view.

src/registrar/tests/test_admin.py

@@ -1247,31 +1247,6 @@ class TestDomainRequestAdmin(MockEppLib):
         expected_url = '<a href="city.com" class="padding-top-1 current-website__1">city.com</a>'
         self.assertContains(response, expected_url)
 
-    def assert_response_contains_distinct_values(self, response, expected_values):
-        """
-        Asserts that each specified value appears exactly once in the response.
-
-        This method iterates over a list of tuples, where each tuple contains a field name
-        and its expected value. It then performs an assertContains check for each value,
-        ensuring that each value appears exactly once in the response.
-
-        Parameters:
-        - response: The HttpResponse object to inspect.
-        - expected_values: A list of tuples, where each tuple contains:
-            - field: The name of the field (used for subTest identification).
-            - value: The expected value to check for in the response.
-
-        Example usage:
-        expected_values = [
-            ("title", "Treat inspector</td>"),
-            ("email", "meoward.jones@igorville.gov</td>"),
-        ]
-        self.assert_response_contains_distinct_values(response, expected_values)
-        """
-        for field, value in expected_values:
-            with self.subTest(field=field, expected_value=value):
-                self.assertContains(response, value, count=1)
-
     @less_console_noise_decorator
     def test_contact_fields_have_detail_table(self):
         """Tests if the contact fields have the detail table which displays title, email, and phone"""
@@ -1319,7 +1294,7 @@ class TestDomainRequestAdmin(MockEppLib):
             ("email_copy_button_input", f'<input class="dja-clipboard-input" type="hidden" value="{expected_email}"'),
             ("phone", "(555) 123 12345</td>"),
         ]
-        self.assert_response_contains_distinct_values(response, expected_creator_fields)
+        self.test_helper.assert_response_contains_distinct_values(response, expected_creator_fields)
 
         # Check for the field itself
         self.assertContains(response, "Meoward Jones")
@@ -1333,7 +1308,7 @@ class TestDomainRequestAdmin(MockEppLib):
             ("email_copy_button_input", f'<input class="dja-clipboard-input" type="hidden" value="{expected_email}"'),
             ("phone", "(555) 555 5556</td>"),
         ]
-        self.assert_response_contains_distinct_values(response, expected_submitter_fields)
+        self.test_helper.assert_response_contains_distinct_values(response, expected_submitter_fields)
         self.assertContains(response, "Testy2 Tester2")
 
         # == Check for the authorizing_official == #
@@ -1345,7 +1320,7 @@ class TestDomainRequestAdmin(MockEppLib):
             ("email_copy_button_input", f'<input class="dja-clipboard-input" type="hidden" value="{expected_email}"'),
             ("phone", "(555) 555 5555</td>"),
         ]
-        self.assert_response_contains_distinct_values(response, expected_ao_fields)
+        self.test_helper.assert_response_contains_distinct_values(response, expected_ao_fields)
 
         # count=4 because the underlying domain has two users with this name.
         # The dropdown has 3 of these.
@@ -1372,7 +1347,7 @@ class TestDomainRequestAdmin(MockEppLib):
             ("email_copy_button_input", f'<input class="dja-clipboard-input" type="hidden" value="{expected_email}"'),
             ("phone", "(555) 555 5557</td>"),
         ]
-        self.assert_response_contains_distinct_values(response, expected_other_employees_fields)
+        self.test_helper.assert_response_contains_distinct_values(response, expected_other_employees_fields)
 
         # count=1 as only one should exist in a table
         self.assertContains(response, "Testy Tester</th>", count=1)
@@ -1972,6 +1947,139 @@ class TestDomainInformationAdmin(TestCase):
         Contact.objects.all().delete()
         User.objects.all().delete()
 
+    @less_console_noise_decorator
+    def test_other_contacts_has_readonly_link(self):
+        """Tests if the readonly other_contacts field has links"""
+
+        # Create a fake domain request and domain
+        domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW)
+        domain_request.approve()
+        domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get()
+
+        # Get the other contact
+        other_contact = domain_info.other_contacts.all().first()
+
+        p = "userpass"
+        self.client.login(username="staffuser", password=p)
+        response = self.client.get(
+            "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk),
+            follow=True,
+        )
+
+        # Make sure the page loaded, and that we're on the right page
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, domain_info.domain.name)
+
+        # Check that the page contains the url we expect
+        expected_href = reverse("admin:registrar_contact_change", args=[other_contact.id])
+        self.assertContains(response, expected_href)
+
+        # Check that the page contains the link we expect.
+        # Since the url is dynamic (populated by JS), we can test for its existence
+        # by checking for the end tag.
+        expected_url = "Testy Tester</a>"
+        self.assertContains(response, expected_url)
+
+    @less_console_noise_decorator
+    def test_contact_fields_have_detail_table(self):
+        """Tests if the contact fields have the detail table which displays title, email, and phone"""
+
+        # Create fake creator
+        _creator = User.objects.create(
+            username="MrMeoward",
+            first_name="Meoward",
+            last_name="Jones",
+        )
+
+        # Due to the relation between User <==> Contact,
+        # the underlying contact has to be modified this way.
+        _creator.contact.email = "meoward.jones@igorville.gov"
+        _creator.contact.phone = "(555) 123 12345"
+        _creator.contact.title = "Treat inspector"
+        _creator.contact.save()
+
+        # Create a fake domain request
+        domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator)
+        domain_request.approve()
+        domain_info = DomainInformation.objects.filter(domain=domain_request.approved_domain).get()
+
+        p = "userpass"
+        self.client.login(username="staffuser", password=p)
+        response = self.client.get(
+            "/admin/registrar/domaininformation/{}/change/".format(domain_info.pk),
+            follow=True,
+        )
+
+        # Make sure the page loaded, and that we're on the right page
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, domain_info.domain.name)
+
+        # Check that the modal has the right content
+        # Check for the header
+
+        # == Check for the creator == #
+
+        # Check for the right title, email, and phone number in the response.
+        # We only need to check for the end tag
+        # (Otherwise this test will fail if we change classes, etc)
+        expected_creator_fields = [
+            # Field, expected value
+            ("title", "Treat inspector</td>"),
+            ("email", "meoward.jones@igorville.gov</td>"),
+            ("phone", "(555) 123 12345</td>"),
+        ]
+        self.test_helper.assert_response_contains_distinct_values(response, expected_creator_fields)
+
+        # Check for the field itself
+        self.assertContains(response, "Meoward Jones")
+
+        # == Check for the submitter == #
+        expected_submitter_fields = [
+            # Field, expected value
+            ("title", "Admin Tester</td>"),
+            ("email", "mayor@igorville.gov</td>"),
+            ("phone", "(555) 555 5556</td>"),
+        ]
+        self.test_helper.assert_response_contains_distinct_values(response, expected_submitter_fields)
+        self.assertContains(response, "Testy2 Tester2")
+
+        # == Check for the authorizing_official == #
+        expected_ao_fields = [
+            # Field, expected value
+            ("title", "Chief Tester</td>"),
+            ("email", "testy@town.com</td>"),
+            ("phone", "(555) 555 5555</td>"),
+        ]
+        self.test_helper.assert_response_contains_distinct_values(response, expected_ao_fields)
+
+        # count=4 because the underlying domain has two users with this name.
+        # The dropdown has 3 of these.
+        self.assertContains(response, "Testy Tester", count=4)
+
+        # Check for table titles. We only need to check for the end tag
+        # (Otherwise this test will fail if we change classes, etc)
+
+        # Title. Count=3 because this table appears on three records.
+        self.assertContains(response, "Title</th>", count=3)
+
+        # Email. Count=3 because this table appears on three records.
+        self.assertContains(response, "Email</th>", count=3)
+
+        # Phone. Count=3 because this table appears on three records.
+        self.assertContains(response, "Phone</th>", count=3)
+
+        # == Test the other_employees field == #
+        expected_other_employees_fields = [
+            # Field, expected value
+            ("title", "Another Tester</td>"),
+            ("email", "testy2@town.com</td>"),
+            ("phone", "(555) 555 5557</td>"),
+        ]
+        self.test_helper.assert_response_contains_distinct_values(response, expected_other_employees_fields)
+
+        # count=1 as only one should exist in a table
+        self.assertContains(response, "Testy Tester</th>", count=1)
+
     def test_readonly_fields_for_analyst(self):
         """Ensures that analysts have their permissions setup correctly"""
         with less_console_noise():
@@ -1990,6 +2098,7 @@ class TestDomainInformationAdmin(TestCase):
                 "no_other_contacts_rationale",
                 "anything_else",
                 "is_policy_acknowledged",
+                "other_contacts",
             ]
 
             self.assertEqual(readonly_fields, expected_fields)