#4422: dnsrecord edit form save [dg] by DaisyGuti · Pull Request #4746 · cisagov/manage.get.gov

DaisyGuti · 2026-03-03T15:57:57Z

Ticket

Resolves #4422

Changes

Added edit (update) flow to DomainDNSRecordsView.post and refactored for flake8
Started the refactored post into methods on DNSRecord and DNSHostService
added db logic need functions to the DnsHostService
Added RequestError on domain view to capture CF API request failures, In _handle_edit(), a RequestError (timeout, connection reset, etc.) is now caught, logged, and re-raised as an APIError so the outer post() handler picks it up
fixed the cancel btn on the edit form

Setup

Please Note: DNS waffle flag must be on AND domain being work with must be enrolled in DNS hosting

You will need to either add or have DNS records to edit so to do this you can click on manage from the admin for example or you can use this one ready for edit: https://getgov-dg.app.cloud.gov/domain/569/dns/records
Edit the record.
Click on save.
The form should close and you will see your edits saved. You can test this by going to a different part of the site and coming back to the record for example.

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

Follow the process for requesting a design review. If code is not user-facing, delete design reviewer checklist
Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Reviewed accessibility checklist using screen reader (such as NVDA with Chrome or Voiceover with Safari), ANDI, or WAVE:
- Tested general usability
- Page header structure
- Landmarks
- Links and buttons
Checked for errors or warnings with an a11y browser tool (such as ANDI or WAVE)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Verified code meets all checks above. Address any checks that are not satisfied
Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
Checked that all code is adequately covered by tests
Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior. Comment any found issues or broken flows.
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

References

Code review best practices

Screenshots

src/registrar/templates/domain_dns_record_edit_form.html

DaisyGuti · 2026-03-03T16:17:31Z

src/registrar/templates/domain_dns_record_form_content.html

When moving the swap to the outer div, it changed how Alpine tracked the dropdown state across htmx swaps. x-on:change handler was now on the form content which gets swapped by htmx - meaning after a swap the handler was on a fresh element that Alpine needed to reinitialize. The x-init was added to ensure Alpine resynced recordType after that reinitialization.

DaisyGuti · 2026-03-03T16:24:47Z

src/registrar/templates/domain_dns_record_form_response.html

Added the edit path:

edit submissions return just the updated display row rather than injecting a new row into the table

Replaced counter == 1 first-record with an explicit is_first_record boolean from the view

this was reset back to counter

ANDI FIX: Edit branch now OOB-swaps the data (via update_cells) instead of the whole row, the edit form row is always OOB-swapped (for errors or reset)

src/registrar/templates/domain_dns_records.html

kimallen

I worked through the server side code. I still need to look at the template/htmx work and test in the app, but I wanted to publish early comments.
I'm noticing a couple places where there's aberration from existing patterns (naming, where we put db query methods), so would be interested in knowing if there's strong reasons for changing those patterns.

src/registrar/services/dns_host_service.py

kimallen · 2026-03-03T21:34:30Z

src/registrar/services/cloudflare_service.py

-            logger.error(f"Error {e.response.status_code} while creating dns record: {e}")
-            raise
+            error_body = e.response.text
+            logger.error(f"Error {e.response.status_code} while creating dns record: {e}\nResponse body: {error_body}")


Nice- thanks for expanding what we log. This will help with error handling.

kimallen · 2026-03-03T21:38:18Z

src/registrar/services/dns_host_service.py

            raise
        logger.info("Successfully enrolled %s in DNS hosting", domain_name)
+
+    def get_dns_record(self, domain, record_pk: int) -> DnsRecord | None:


Since this is purely a lookup, I wonder if this would better fit to fall under the DnsRecord Model? Is there a reason to put it here instead of the model? (same question for all of these methods)

Ah yes that would fit better. I'll move them over.

src/registrar/services/dns_host_service.py

src/registrar/views/domain.py

kimallen · 2026-03-03T23:22:52Z

src/registrar/views/domain.py

        """Find an item by name in a list of dictionaries."""
        return next((item.get("id") for item in items if item.get("name") == name), None)

+    def _parse_dns_record_id(self, request) -> int | None:


Can you say more about why this is necessary, and why we can't just get the id directly from the request.POST?

The id is only present in the POST for edit submissions

src/registrar/templates/domain_dns_record_edit_form.html

erinysong · 2026-03-04T17:01:26Z

src/registrar/templates/domain_dns_record_edit_form.html

-<tr x-show="showFormId == {{ counter }}" class="hide-td-borders">
+
+{# Inline edit form row for a DNS record. `counter` is pk-based. #}
+<tr id="dnsrecord-edit-row-{{ counter }}" x-show="showFormId == {{ counter }}" class="hide-td-borders" {% if oob_swap %}hx-swap-oob="outerHTML"{% endif %}>


syntax/naming: The hide-td-borders class is from legacy code, but I just noticed we call this hide-td-borders but aren't we technically hiding the row's overall border? Wondering how we ended up with that since we're using the class on the tr itself too 🤔

I am not sure how to answer this. This was already part of the form just kept it.

LMK if this is something that we now want to remove.

@erinysong are you suggesting this should be named hide-tr-borders instead of hide-td-borders?

I was asking mostly to ask if anyone else found the class name misleading/confusing since my first thought would've been that we're using this class to hide the border for the entire and not all the individual cells

I am still unclear on the resolution for this. If I missed it please re-iterate.

As this was pre-existing, if we want to change it etc. we might best address that during a clean up of the styling or a final pass of the work (another ticket) vs this PR, which is scoped for the saving of the existing edit form.

Great conversation and question to bring up!

erinysong · 2026-03-04T17:48:35Z

src/registrar/templates/domain_dns_record_edit_form.html

-<tr x-show="showFormId == {{ counter }}" class="hide-td-borders">
+
+{# Inline edit form row for a DNS record. `counter` is pk-based. #}
+<tr id="dnsrecord-edit-row-{{ counter }}" x-show="showFormId == {{ counter }}" class="hide-td-borders" {% if oob_swap %}hx-swap-oob="outerHTML"{% endif %}>


If we're identifying the rows by the record pk, why don't we just use the record.pk directly? Since counter implies that we're incrementally counting the rows themselves

on that note, we used to use counter to identify our rows because initially we decided to label our rows by the order in which they appeared on the table to give allow the user to easily identify which row on the table they were on. I noticed in this PR we're removing that counter and the counter value is now the record pk. Can I hear more about why we're switching that value here?

I didn't realize the counter had any use other than identifying really if it was the first row. Having the idea of the row be the ID is a pretty standard pattern for tables like this, especially since we're not allowing users to move their rows or anything like that so we don't need to keep track of order. Is there a requirement that we need to keep the order? If so we if so, we should probably add that to the model. I don't think I've seen it in there yet. I'll have to look again...

hm yeah I've seen both incremental counters and the id of the data object be used. I think the incremental counters was an existing pattern from product decision to go incremental on form int identifiers for multiple portfolios because we didn't want to expose organization object id's. I'm not sure if we have that same level of concern for records so I'd defer to product if that's the case

OH I didn't realize it was a over-arching decision on the project. Yeah I can change it back to do a counter thing.

I think back then that decision was made specifically because we wanted to avoid exposing organization id's in our DOM, but I'm not sure if records would have that same scrutiny. Let's check in with product on Slack to see if that's the case! I can start a thread

Per latest, updating to remove the counter and go back to the id.

erinysong · 2026-03-04T19:25:58Z

src/registrar/templates/domain_dns_record_edit_form.html

-<tr x-show="showFormId == {{ counter }}" class="hide-td-borders">
+
+{# Inline edit form row for a DNS record. `counter` is pk-based. #}
+<tr id="dnsrecord-edit-row-{{ counter }}" x-show="showFormId == {{ counter }}" class="hide-td-borders" {% if oob_swap %}hx-swap-oob="outerHTML"{% endif %}>


Also overall table design question from looking through dev tools: Can I hear more about why we load a hidden for each DNS row's edit form for each DNS record row specifically? My intuition would've been we only append/remove the row when we click the edit/cancel button for that one record row. So I'd be interested in hearing why we're loading hidden edit forms for each row 😮

Since we only will ever have a couple of rows doing it this way the x-show avoids that extra round trip (network calls etc.) and the form with all its pre-filled field values is already in the DOM ready to display when the user clicks Edit.

If we had hundreds of records or large records that we wouldn't do this and I'd use something like the htmx-fetched form on demand. But we're such a small record set, it is definitely a value add, and so we can take advantage of that and have the ability to lessen the load on the application and network.

Got it and makes sense given we likely won't have too many records here. Thank you for the rundown!

src/registrar/templates/domain_dns_record_form_response.html

kimallen

Observations when testing live (these are more around the form behavior and validations, so could potentially go in a different ticket):

The validation alerts only show up at the top of the page, not within the edit form. Is that the desired behavior?
Once opening the Edit form, I expected tabbing to enter the form. However, the next tab order was the "other options" and then it goes to the form fields. It seemed a bit confusing (especially with SR), and wondering if we want focus to go from the Edit button to the form directly (question for design?).
When I edit record and make a required field empty, no alert shows up at the top of the page, and an html alert shows up instead.

We also seemed to have lost the fix that labeled the ids for each field uniquely per edit form. We now have the a11y error again where the labels can't find which field they are for. (so screen reader is not reading the label when focusing on an input)

abe-alam-ecs · 2026-03-04T21:53:40Z

src/registrar/views/domain.py

        to display corresponding values in the table rows"""
        for record in dns_records:
            data_dict = self.record_dict_for_initial_data(record)
-            record.form = DomainDNSRecordForm(initial=data_dict, prefix=f"edit_{record.id}")


I added this line to correct issues in WAVE/ANDI with duplicate ids. Was this accidentally changed?

Not accidentally. But I'll see about adding it back in / or something like it.

Update to not use the interal ids and use the counter

I'll be OOO the next couple days and don't want to block this if changes get made. Abe is also on point to review, so he can check on follow up of my comments. It will still need a design review, so that can serve as the second reviewer.

DaisyGuti · 2026-03-07T20:09:04Z

src/registrar/forms/domain.py

Changed ttl field from ChoiceField to TypedChoiceField(coerce=int) so the cleaned value is an integer, preventing a TypeError when the model's clean() method compares self.ttl < 60.

src/registrar/management/commands/delete_expired_domains_not_setup.py

DaisyGuti · 2026-03-08T15:03:36Z

src/registrar/models/dns/dns_record.py


        if errors:
            raise ValidationError(errors)
+


Refactored view post calls into here and dhshostservice where appropriate.

DaisyGuti · 2026-03-08T15:03:43Z

src/registrar/services/dns_host_service.py

Refactored view post calls into here and dhshostservice where appropriate.

DaisyGuti · 2026-03-08T16:07:55Z

Hello team, I believe I've address all your comments except one that is open/unresolved by @Erin Song regarding the class="hide-td-borders". Reset all back to counters and I reused that to uniquely id the inputs, thus leaving internal ids un-exposed per the other Slack thread and comments.

I went ahead and fixed the pickling errors as well on this PR bc it kept failing and there were real fixes needed especially in our area so those are in here too now.

FYSA: I will OOO/traveling this week Mon AM -Fri AM. But I'll be on AM of Tuesday and Thursday. Thank you again for the revs.

Cc: @Kim Allen [CTR] @Abraham Alam [CTR] @Erin Song (oh also I did send this over to design as well for this next rev round fysa)

DaisyGuti · 2026-03-19T07:03:35Z

src/registrar/views/domain.py


 logger = logging.getLogger(__name__)

-context_dns_record = ContextVar("context_dns_record", default=None)


DaisyGuti · 2026-03-19T07:11:38Z

src/registrar/views/domain.py

+
+        return is_first_record, record_id
+
+    def post(self, request, *args, **kwargs):


Separated out the create, edit and form check into functions called from the post to fix flake 8 as the post was too complex.

DaisyGuti · 2026-03-19T07:14:35Z

src/registrar/views/domain.py

+                "update_cells": is_edit and self.dns_record is not None,
            },
-            headers={"HX-TRIGGER": hx_trigger_events},
+            headers={"HX-Trigger-After-Settle": json.dumps({"messagesRefresh": "", "recordSubmitSuccess": ""})},


for htmx swaps and transitions to be complete, before messagesRefresh and recordSubmitSuccess fire.

DaisyGuti · 2026-03-19T09:18:43Z

src/registrar/tests/services/test_cloudflare_service.py

revert failure_cases to expect HTTPStatusError for methods that re-raise directly

add dns_record_failure_cases with APIError for create/update dns record methods that wrap the error

set mock_response.text to a real string so APIError message assertions pass

DaisyGuti · 2026-03-19T09:19:35Z

src/registrar/tests/services/test_mock_cloudflare_service.py

expect APIError (not HTTPStatusError) for DNS record error cases since CloudflareService wraps HTTP errors in APIError for those methods

DaisyGuti · 2026-03-19T09:25:06Z

src/registrar/tests/views/test_domain_dns_record_and_form.py

updated mock create_dns_record (the method the view actuallycalls)

fix get_x_zone_id_if_zone_exists to return a list for nameservers instead of True, when nameservers = True (boolean), Django's template [domain_dns_nameservers_table.html] tries to iterates over True, so this would raise a TypeError during template rendering.

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

@kimallen

…pup, 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

…d POST field names

kimallen · 2026-03-19T17:19:32Z

src/registrar/views/domain.py

+        - Otherwise attach a blank/initial form from the DB model.
+        - Uses the record id to generate unique field IDs across multiple forms on the page.
+        """
+        auto_id = f"id_edit_{record_obj.pk}_%s"


What does %s do in this string?

string substitution - replaces the %s with the name of the form field

kimallen

Thanks so much for all your work and patience on this @DaisyGuti !
LGTM!

erinysong

Great work Daisy :D Thank you for working through all the changes and discussions we had around this PR!

github-project-automation bot added this to .gov Product Board Mar 3, 2026

DaisyGuti moved this to 🎯 Ready in .gov Product Board Mar 3, 2026

DaisyGuti commented Mar 3, 2026

View reviewed changes

src/registrar/templates/domain_dns_record_edit_form.html Show resolved Hide resolved

DaisyGuti commented Mar 3, 2026

View reviewed changes

src/registrar/templates/domain_dns_records.html Show resolved Hide resolved

DaisyGuti changed the title ~~#4422: dnsrecord edit form save~~ #4422: dnsrecord edit form save [dg] Mar 3, 2026

kimallen assigned kimallen and abe-alam-ecs Mar 3, 2026

kimallen previously requested changes Mar 3, 2026

View reviewed changes

github-project-automation bot moved this from 🎯 Ready to 👀 In review in .gov Product Board Mar 3, 2026

erinysong self-assigned this Mar 3, 2026

abe-alam-ecs reviewed Mar 4, 2026

View reviewed changes

src/registrar/templates/domain_dns_record_edit_form.html Outdated Show resolved Hide resolved

erinysong reviewed Mar 4, 2026

View reviewed changes

src/registrar/templates/domain_dns_record_form_response.html Outdated Show resolved Hide resolved

kimallen reviewed Mar 4, 2026

View reviewed changes

abe-alam-ecs reviewed Mar 4, 2026

View reviewed changes

DaisyGuti commented Mar 7, 2026

View reviewed changes

DaisyGuti force-pushed the dg/4422-dnsrecord-edit-save branch from d91fc30 to 2dd63d1 Compare March 7, 2026 20:18

DaisyGuti mentioned this pull request Mar 8, 2026

Fix unpicklable MagicMock objects causing parallel test runner failures in CI #4752

Closed

DaisyGuti commented Mar 8, 2026

View reviewed changes

src/registrar/management/commands/delete_expired_domains_not_setup.py Show resolved Hide resolved

DaisyGuti commented Mar 8, 2026

View reviewed changes

src/registrar/services/dns_host_service.py

Copy link

Contributor Author

DaisyGuti Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored view post calls into here and dhshostservice where appropriate.

DaisyGuti added the design-review dev ticket needing design review label Mar 8, 2026

DaisyGuti requested review from abe-alam-ecs and erinysong March 8, 2026 15:52

DaisyGuti commented Mar 19, 2026

View reviewed changes

DaisyGuti force-pushed the dg/4422-dnsrecord-edit-save branch from c367e61 to f8cabaa Compare March 19, 2026 08:22

DaisyGuti commented Mar 19, 2026

View reviewed changes

DaisyGuti requested review from erinysong and kimallen March 19, 2026 09:31

DaisyGuti and others added 13 commits March 19, 2026 12:04

All working save.

a0d7b3b

Started refactor of the dns form post. Lint, flake fixes

ec4bc8d

Tests

0062335

Readding the empty text.

1c0d689

Apply suggestion from @kimallen

8d1a225

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

PR rev items cleaned up and lint fixes.

f1f828a

flake8 fixes and added descriptor to the dnshostSvc

fce8a3b

Update to swap the form on error correction.

7977e90

fix tests, added a test to verify the form close

04a38fb

Updated success message and edit btn ANDI fixes

76c07cd

Updated to use id vs counter, fixed cancel btn and form validation po…

7db7dad

…pup, 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

Fix pickling test failures: correct exception types, mock methods, an…

1d0867e

…d POST field names

lint fix

bd742fa

DaisyGuti force-pushed the dg/4422-dnsrecord-edit-save branch from 8320d9c to bd742fa Compare March 19, 2026 16:06

fixed flake 8 issues

e24f5dc

kimallen reviewed Mar 19, 2026

View reviewed changes

kimallen approved these changes Mar 19, 2026

View reviewed changes

erinysong approved these changes Mar 19, 2026

View reviewed changes

DaisyGuti merged commit 317a523 into main Mar 20, 2026
12 checks passed

DaisyGuti deleted the dg/4422-dnsrecord-edit-save branch March 20, 2026 15:55

github-project-automation bot moved this from 👀 In review to ✅ Done in .gov Product Board Mar 20, 2026


		logger = logging.getLogger(__name__)

		context_dns_record = ContextVar("context_dns_record", default=None)


		return is_first_record, record_id

		def post(self, request, args, *kwargs):

Conversation

DaisyGuti commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Changes

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

References

Screenshots

Uh oh!

Uh oh!

DaisyGuti Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kimallen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaisyGuti Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaisyGuti Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erinysong Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaisyGuti commented Mar 3, 2026 •

edited

Loading

DaisyGuti Mar 3, 2026 •

edited

Loading

DaisyGuti Mar 4, 2026 •

edited

Loading

DaisyGuti Mar 18, 2026 •

edited

Loading

erinysong Mar 4, 2026 •

edited

Loading

DaisyGuti Mar 4, 2026 •

edited

Loading

DaisyGuti Mar 4, 2026 •

edited

Loading

kimallen left a comment •

edited

Loading

DaisyGuti Mar 5, 2026 •

edited

Loading