Updated to use id vs counter, fixed cancel btn and form validation popup, removed contextvar and refactored post() for flake8 fix, removed silencer

Apply suggestion from @kimallen

Co-authored-by: Kim Allen <kim@truss.works>

cleanup

more cleanup

Author: DaisyGuti

src/registrar/models/dns/dns_record.py

@@ -88,21 +88,6 @@ def get_ordered_for_zone(cls, dns_zone: "DnsZone"):
         """Return all records for a zone ordered by pk (matches counter assignment order)."""
         return cls.objects.filter(dns_zone=dns_zone).order_by("pk")
 
-    @classmethod
-    def get_for_domain_by_counter(cls, domain: Domain, counter: int) -> "DnsRecord | None":
-        """Return the DnsRecord at the given 1st position for the domain's zone.
-
-        Uses the same ordering as the DNS records table so the counter matches
-        what was rendered on the GET request.
-        """
-        dns_zone = DnsZone.objects.filter(domain=domain).first()
-        if not dns_zone:
-            return None
-        try:
-            return cls.get_ordered_for_zone(dns_zone)[counter - 1]
-        except (IndexError, AssertionError):
-            return None
-
     @classmethod
     def zone_has_records(cls, domain: Domain) -> bool:
         """Return whether a domain's DNS zone has any existing records."""

src/registrar/services/dns_host_service.py

@@ -199,13 +199,13 @@ def update_dns_record(self, x_zone_id: str, record_id: int, form_record_data: di
 
         x_record_id = dns_record.get_active_x_record_id()
         if not x_record_id:
-            raise ValueError("This DNS record is missing a vendor id and cannot be updated.")
+            raise ValueError("This DNS record is missing an external record id and cannot be updated.")
 
-        self.update_and_save_record(x_zone_id, x_record_id, form_record_data)
+        self.update_and_save_dns_record(x_zone_id, x_record_id, form_record_data)
         return dns_record
 
-    def update_and_save_record(self, x_zone_id, x_record_id, form_record_data) -> dict:
-        """Calls update method of vendor service to update a DNS record"""
+    def update_and_save_dns_record(self, x_zone_id, x_record_id, form_record_data) -> dict:
+        """Push updated record data to the vendor and persist the changes in the local database."""
         # Update record in vendor service
         try:
             vendor_record_data = self.dns_vendor_service.update_dns_record(x_zone_id, x_record_id, form_record_data)

src/registrar/templates/domain_dns_record_edit_form.html

@@ -1,20 +1,21 @@
 {% load static field_helpers url_helpers %}
 
-{# Inline edit form row for a DNS record. `counter` is a sequential index, not the DB pk. #}
-<tr id="dnsrecord-edit-row-{{ counter }}" x-show="showFormId == {{ counter }}" class="hide-td-borders" {% if oob_swap %}hx-swap-oob="outerHTML"{% endif %}>
+{# Inline edit form row for a DNS record. `record_id` is the record pk used for DOM targeting. #}
+<tr id="dnsrecord-edit-row-{{ record_id }}" x-show="showFormId == {{ record_id }}" class="hide-td-borders" {% if oob_swap %}hx-swap-oob="outerHTML"{% endif %}>
     <td colspan="5" class="no-padding">
         <div class="border radius-lg border-base-lighter padding-3">
             <form
+                id="dnsrecord-edit-form-{{ record_id }}"
                 method="post"
                 class="usa-form usa-form--extra-large"
+                novalidate
                 hx-post="{% url 'domain-dns-records' domain.pk %}"
                 hx-swap="none"
             >
                 <h2 class="margin-top-0">Edit record</h2>
                 {% csrf_token %}
 
-                {# The counter identifies the row for both DOM targeting and DB lookup (positional index). #}
-                <input type="hidden" name="counter" value="{{ counter }}" />
+                <input type="hidden" name="id" value="{{ record_id }}" />
                 <input type="hidden" name="type" value="{{ dns_record.type }}" />
 
                 {% with add_group_class="usa-form-group--unstyled-error" %}
@@ -33,8 +34,12 @@ <h2 class="margin-top-0">Edit record</h2>
                 </div>
                 {% input_with_errors dns_record.form.comment %}
                 <button
-                    type="reset"
+                    type="button"
                     class="usa-button usa-button--outline"
+                    hx-get="{% url 'domain-dns-records' domain.pk %}"
+                    hx-select="#dnsrecord-edit-form-{{ record_id }}"
+                    hx-target="#dnsrecord-edit-form-{{ record_id }}"
+                    hx-swap="outerHTML"
                     x-on:click="showFormId = null"
                 >
                     Cancel

src/registrar/templates/domain_dns_record_form_content.html

@@ -5,7 +5,7 @@ <h2>Add record</h2>
 
 <div
     class="grid-row grid-gap"
-    x-init="recordType = $el.querySelector('#id_type')?.value || recordType"
+    x-init="recordType = ($el.querySelector('#id_type') || {}).value || recordType"
     x-on:change="if ($event.target && $event.target.id === 'id_type') { recordType = $event.target.value }"
 >
     <div class="grid-col-3">

src/registrar/templates/domain_dns_record_form_response.html

@@ -3,13 +3,13 @@
         {# OOB-swap individual cells to preserve the action column (Edit button) DOM for ANDI #}
         <template>
         {% if update_cells %}
-            <td id="dns-type-{{ counter }}" data-label="Type" hx-swap-oob="outerHTML">{{ dns_record.type }}</td>
-            <td id="dns-name-{{ counter }}" data-label="Name" hx-swap-oob="outerHTML">{{ dns_record.name }}</td>
-            <td id="dns-content-{{ counter }}" data-label="TTL" hx-swap-oob="outerHTML">{{ dns_record.content }}</td>
-            <td id="dns-ttl-{{ counter }}" data-label="Type" hx-swap-oob="outerHTML">{{ dns_record.ttl }}</td>
+            <td id="dns-type-{{ record_id }}" data-label="Type" hx-swap-oob="outerHTML">{{ dns_record.type }}</td>
+            <td id="dns-name-{{ record_id }}" data-label="Name" hx-swap-oob="outerHTML">{{ dns_record.name }}</td>
+            <td id="dns-content-{{ record_id }}" data-label="TTL" hx-swap-oob="outerHTML">{{ dns_record.content }}</td>
+            <td id="dns-ttl-{{ record_id }}" data-label="Type" hx-swap-oob="outerHTML">{{ dns_record.ttl }}</td>
         {% endif %}
         {# Always OOB swap the edit form row so errors are shown/cleared #}
-        {% include "domain_dns_record_edit_form.html" with counter=counter dns_record=dns_record oob_swap=True %}
+        {% include "domain_dns_record_edit_form.html" with record_id=record_id dns_record=dns_record oob_swap=True %}
         </template>
     {% else %}
         <template>
@@ -26,8 +26,8 @@
             {% endif %}
 
             <tbody id="dnsrecords-table-body" hx-swap-oob="afterbegin:#dnsrecords-table-body">
-                {% include "domain_dns_record_row.html" with counter=counter dns_record=dns_record %}
-                {% include "domain_dns_record_edit_form.html" with counter=counter dns_record=dns_record %}
+                {% include "domain_dns_record_row.html" with record_id=record_id dns_record=dns_record %}
+                {% include "domain_dns_record_edit_form.html" with record_id=record_id dns_record=dns_record %}
             </tbody>
         </template>
 

src/registrar/templates/domain_dns_record_row.html

@@ -1,24 +1,24 @@
-<tr id="dnsrecord-row-{{counter}}" :class="showFormId == {{ counter }} ? 'hide-td-borders' : ''">
-    <td id="dns-type-{{ counter }}" data-label="Type">
+<tr id="dnsrecord-row-{{record_id}}" :class="showFormId == {{ record_id }} ? 'hide-td-borders' : ''">
+    <td id="dns-type-{{ record_id }}" data-label="Type">
         {{dns_record.type}}
     </td>
-    <td id="dns-name-{{ counter }}" data-label="Name">
+    <td id="dns-name-{{ record_id }}" data-label="Name">
         {{dns_record.name}}
     </td>
-    <td id="dns-content-{{ counter }}" data-label="TTL">
+    <td id="dns-content-{{ record_id }}" data-label="TTL">
         {{dns_record.content}}
     </td>
-    <td id="dns-ttl-{{ counter }}" data-label="Type">
+    <td id="dns-ttl-{{ record_id }}" data-label="Type">
         {{dns_record.ttl}}
     </td>
     <td class="padding-right-0" data-label="Action">
         <div class="tablet:display-flex tablet:flex-row">
             <button type="button"
             class="usa-button usa-button--unstyled margin-right-2 margin-top-0"
-            x-on:click="showFormId = (showFormId === {{ counter }} ? null : {{ counter }})"
+            x-on:click="showFormId = (showFormId === {{ record_id }} ? null : {{ record_id }})"
             aria-expanded="false"
-            :aria-expanded="showFormId == {{ counter }} ? 'true' : 'false'"
-            aria-controls="dnsrecord-edit-row-{{ counter }}"
+            :aria-expanded="showFormId == {{ record_id }} ? 'true' : 'false'"
+            aria-controls="dnsrecord-edit-row-{{ record_id }}"
             aria-label="Edit {{ dns_record.name }}"
             >
                 <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24">
@@ -33,14 +33,14 @@
                         focusable="false" 
                         role="img"
                         width="8">
-                        <use x-show="showFormId != {{ counter }}" xlink:href="/public/img/sprite.svg#expand_more" ></use>
-                        <use x-show="showFormId == {{ counter }}" xlink:href="/public/img/sprite.svg#expand_less" ></use>
+                        <use x-show="showFormId != {{ record_id }}" xlink:href="/public/img/sprite.svg#expand_more" ></use>
+                        <use x-show="showFormId == {{ record_id }}" xlink:href="/public/img/sprite.svg#expand_less" ></use>
                     </svg>
 
             </button>
             <a
                 role="button"
-                id="button-trigger-delete-dnsrecord-{{ counter }}"
+                id="button-trigger-delete-dnsrecord-{{ record_id }}"
                 class="usa-button usa-button--unstyled text-underline margin-top-2 line-height-sans-5 text-secondary visible-mobile-flex"
                 >
                 <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24">
@@ -55,15 +55,15 @@
                     type="button"
                     class="usa-button usa-button--unstyled usa-button--with-icon usa-accordion__button usa-button--more-actions margin-top-2px"
                     aria-expanded="false"
-                    aria-controls="more-actions-dnsrecord-{{ counter }}"
+                    aria-controls="more-actions-dnsrecord-{{ record_id }}"
                     aria-label="More Actions for DNS record"
                 >
                     <svg class="usa-icon" aria-hidden="true" focusable="false" role="img" width="24">
                     <use xlink:href="/public/img/sprite.svg#more_vert"></use>
                     </svg>
                 </button>
                 </div>
-                <div id="more-actions-dnsrecord-{{ counter }}" class="usa-accordion__content usa-prose shadow-1 left-auto right-neg-1" hidden>
+                <div id="more-actions-dnsrecord-{{ record_id }}" class="usa-accordion__content usa-prose shadow-1 left-auto right-neg-1" hidden>
                 <h2>More options</h2>
                 <button
                     type="button"

src/registrar/templates/domain_dns_records_table.html

@@ -17,11 +17,9 @@
     </thead>
 
     <tbody id="dnsrecords-table-body">
-        {% if dns_records %}
-            {% for dns_record in dns_records %}
-                {% include "domain_dns_record_row.html" with counter=dns_record.counter dns_record=dns_record %}
-                {% include "domain_dns_record_edit_form.html" with counter=dns_record.counter dns_record=dns_record %}
-            {% endfor %}
-        {% endif %}
+        {% for dns_record in dns_records %}
+            {% include "domain_dns_record_row.html" with record_id=dns_record.id dns_record=dns_record %}
+            {% include "domain_dns_record_edit_form.html" with record_id=dns_record.id dns_record=dns_record %}
+        {% endfor %}
     </tbody>
 </table>

src/registrar/tests/services/test_dns_host_service.py

@@ -232,9 +232,10 @@ def test_dns_setup_failure_from_create_cf_zone(self, mock_create_cf_account, moc
         # mock_create_cf_zone.assert_called_once_with(zone_name, account_id) not sure why this fails: 0 calls
         self.assertIn("DNS setup failed to create zone", str(context.exception))
 
+    @patch("registrar.models.dns.dns_record.DnsRecord.get_by_x_record_id")
     @patch("registrar.models.dns.dns_record.DnsRecord.create_from_vendor_data")
     @patch("registrar.services.dns_host_service.CloudflareService.create_dns_record")
-    def test_create_cf_record_success(self, mock_create_dns_record, _):
+    def test_create_cf_record_success(self, mock_create_dns_record, _, mock_get_by_x_record_id):
         zone_id = "1234"
         record_data = {
             "type": "A",
@@ -245,11 +246,14 @@ def test_create_cf_record_success(self, mock_create_dns_record, _):
             "created_on": "2024-01-02T03:04:05Z",
         }
 
+        mock_dns_record = Mock()
+        mock_dns_record.name = "test.gov"
         mock_create_dns_record.return_value = {"result": {"id": zone_id, **record_data}}
+        mock_get_by_x_record_id.return_value = mock_dns_record
 
-        response = self.service.create_and_save_record(zone_id, record_data)
-        self.assertEqual(response["result"]["id"], zone_id)
-        self.assertEqual(response["result"]["name"], "test.gov")
+        response = self.service.create_dns_record(zone_id, record_data)
+        self.assertEqual(response.name, "test.gov")
+        mock_get_by_x_record_id.assert_called_once_with(zone_id)
 
     @patch("registrar.services.dns_host_service.CloudflareService.create_dns_record")
     def test_create_cf_record_failure(self, mock_create_dns_record):
@@ -260,7 +264,7 @@ def test_create_cf_record_failure(self, mock_create_dns_record):
         mock_create_dns_record.side_effect = APIError("Bad request: missing name")
 
         with self.assertRaises(APIError) as context:
-            self.service.create_and_save_record(zone_id, record_data)
+            self.service.create_dns_record(zone_id, record_data)
         self.assertIn("Bad request: missing name", str(context.exception))
 
     def test_update_account_dns_settings_success(self):

src/registrar/tests/test_views_domain.py

@@ -3555,7 +3555,7 @@ def test_edit_dns_record_save_updates_record(self):
         response = self.client.post(
             reverse("domain-dns-records", kwargs={"domain_pk": self.domain.id}),
             data={
-                "id": dns_record.pk,
+                "id": dns_record.id,
                 "type": dns_record.type,
                 "name": "api",
                 "content": "203.0.113.15",
@@ -3625,7 +3625,7 @@ def test_edit_dns_record_save_returns_400_without_active_vendor_id(self):
         response = self.client.post(
             reverse("domain-dns-records", kwargs={"domain_pk": self.domain.id}),
             data={
-                "id": dns_record.pk,
+                "id": dns_record.id,
                 "type": dns_record.type,
                 "name": "api",
                 "content": "203.0.113.25",

src/registrar/tests/views/test_domain_dns_record_and_form.py

@@ -73,7 +73,7 @@ def test_get_renders_page_and_form_fields_success(self):
 
     @override_flag("dns_hosting", active=True)
     @less_console_noise_decorator
-    def test_post_valid_forms_create_records_success(self):
+    def test_post_valid_forms_create_dns_records_success(self):
         for data in self.RECORD_TEST_CASES:
             with self.subTest(record_type=data["type"]):
                 mock_record = {