fix: initialize customer DNS management modal#1338
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR improves DNS record management by normalizing backend responses and refactoring the frontend to use secure, localized configuration. A new backend helper normalizes records across all provider paths; the frontend wiring registers and configures the script with AJAX URLs and nonces; and the JavaScript is refactored to use these config-aware helpers throughout record loading, editing, deleting, and bulk operations. ChangesDNS Record Management Frontend-Backend Integration
Sequence DiagramsequenceDiagram
participant UI as Domain_Mapping_Element
participant JS as dns-management.js
participant AJAX as WordPress AJAX
participant Backend as DNS_Record_Manager
UI->>JS: register & localize script with wu_dns_config
Note over JS: window.wu_dns_config now available
JS->>JS: loadRecords() via wubox:load listener
JS->>JS: getConfig() & validate nonce
JS->>AJAX: POST wu_get_dns_records_for_domain
AJAX->>Backend: route to ajax_get_records()
Backend->>Backend: normalize_records_for_response()
Backend->>AJAX: return normalized records
AJAX->>JS: success response
JS->>JS: renderRecords() with normalized data
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/WP_Ultimo/UI/Domain_Mapping_Element_Test.php (1)
20-42: 🏗️ Heavy liftAssert runtime registration state instead of raw source strings.
These assertions only prove that specific substrings exist in the files. They can still pass if
register_scripts()stops wiring the handle correctly, localizes the wrong payload, or moves equivalent logic into another helper. ExercisingDomain_Mapping_Element::register_scripts()and asserting against thewu-dns-managemententry in$wp_scriptswould protect the actual contract this PR depends on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/UI/Domain_Mapping_Element_Test.php` around lines 20 - 42, Replace raw source string assertions with an integration-style test that calls Domain_Mapping_Element::register_scripts() and asserts the runtime registration state: invoke Domain_Mapping_Element::register_scripts(), then inspect the global $wp_scripts (and $wp_styles if needed) to find an entry with handle 'wu-dns-management' and assert its existence, that it is enqueued (or registered) and that the localized data created via wp_localize_script and inline script added via wp_add_inline_script were attached to that handle (check registered script's extra data or localized vars and inline data). Also assert that admin_url('admin-ajax.php') was included in the localized payload (or in the localized var key 'ajaxurl') and that the script is ultimately enqueued via wp_enqueue_script('wu-dns-management') by verifying the enqueue state on the 'wu-dns-management' handle.tests/WP_Ultimo/Managers/DNS_Record_Manager_Normalization_Test.php (1)
8-17: ⚡ Quick winAlign the test namespace with the source namespace.
This file sits under
tests/WP_Ultimo/Managers/but declaresWP_Ultimo\Tests\Managers. The repo convention for this path is to mirror the source namespace, so this should trackWP_Ultimo\Managersinstead of introducing a separateTestssegment.As per coding guidelines "test namespace must mirror source structure (e.g., WP_Ultimo\Checkout\Cart_Test tests WP_Ultimo\Checkout\Cart)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/Managers/DNS_Record_Manager_Normalization_Test.php` around lines 8 - 17, The test class namespace is incorrect: change the namespace declaration from "WP_Ultimo\Tests\Managers" to mirror the source namespace "WP_Ultimo\Managers" so the test tracks the corresponding source code; update the top of the file where the namespace is declared for the class DNS_Record_Manager_Normalization_Test to use "WP_Ultimo\Managers" (keep use statements and class name DNS_Record_Manager_Normalization_Test unchanged).inc/ui/class-domain-mapping-element.php (1)
830-838: ⚡ Quick winReuse the manager sanitizer instead of duplicating the record-shaping rules.
The arrays on Lines 830-838 and 952-959 duplicate
DNS_Record_Manager::sanitize_record_data()ininc/managers/class-dns-record-manager.php. That makes the customer modal path drift-prone: a future field/default change can update the AJAX handlers but leave these form handlers behind. Please route both paths through one shared sanitizer.Also applies to: 952-959
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/ui/class-domain-mapping-element.php` around lines 830 - 838, Replace the duplicated inline sanitization in inc/ui/class-domain-mapping-element.php with a call to the centralized sanitizer DNS_Record_Manager::sanitize_record_data() so both the modal/form path and the AJAX path use the same logic; specifically, stop building the $record array manually in the blocks around the existing array (the one creating 'type','name','content','ttl','priority','proxied') and instead pass the incoming raw record data through DNS_Record_Manager::sanitize_record_data($record) (ensuring you provide the same defaults if the sanitizer requires them), and make the identical change for the other duplicated block noted (around lines 952-959) so both places reuse the single sanitizer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@inc/ui/class-domain-mapping-element.php`:
- Around line 830-838: Replace the duplicated inline sanitization in
inc/ui/class-domain-mapping-element.php with a call to the centralized sanitizer
DNS_Record_Manager::sanitize_record_data() so both the modal/form path and the
AJAX path use the same logic; specifically, stop building the $record array
manually in the blocks around the existing array (the one creating
'type','name','content','ttl','priority','proxied') and instead pass the
incoming raw record data through
DNS_Record_Manager::sanitize_record_data($record) (ensuring you provide the same
defaults if the sanitizer requires them), and make the identical change for the
other duplicated block noted (around lines 952-959) so both places reuse the
single sanitizer.
In `@tests/WP_Ultimo/Managers/DNS_Record_Manager_Normalization_Test.php`:
- Around line 8-17: The test class namespace is incorrect: change the namespace
declaration from "WP_Ultimo\Tests\Managers" to mirror the source namespace
"WP_Ultimo\Managers" so the test tracks the corresponding source code; update
the top of the file where the namespace is declared for the class
DNS_Record_Manager_Normalization_Test to use "WP_Ultimo\Managers" (keep use
statements and class name DNS_Record_Manager_Normalization_Test unchanged).
In `@tests/WP_Ultimo/UI/Domain_Mapping_Element_Test.php`:
- Around line 20-42: Replace raw source string assertions with an
integration-style test that calls Domain_Mapping_Element::register_scripts() and
asserts the runtime registration state: invoke
Domain_Mapping_Element::register_scripts(), then inspect the global $wp_scripts
(and $wp_styles if needed) to find an entry with handle 'wu-dns-management' and
assert its existence, that it is enqueued (or registered) and that the localized
data created via wp_localize_script and inline script added via
wp_add_inline_script were attached to that handle (check registered script's
extra data or localized vars and inline data). Also assert that
admin_url('admin-ajax.php') was included in the localized payload (or in the
localized var key 'ajaxurl') and that the script is ultimately enqueued via
wp_enqueue_script('wu-dns-management') by verifying the enqueue state on the
'wu-dns-management' handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c560774-2a52-427b-b05a-b5febe7f22e5
📒 Files selected for processing (5)
assets/js/dns-management.jsinc/managers/class-dns-record-manager.phpinc/ui/class-domain-mapping-element.phptests/WP_Ultimo/Managers/DNS_Record_Manager_Normalization_Test.phptests/WP_Ultimo/UI/Domain_Mapping_Element_Test.php
Collaborator
Author
Summary
Verification
Merged via PR #1338 to main. aidevops.sh v3.20.11 spent 13m on this as a headless bash routine. |
superdav42
added
the
review-feedback-scanned
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Domain_Mapping_Element::register_scripts()souser_manage_dns_recordshas config on front-end shortcode pages.wubox:loadevent while preserving the legacywubox-loadevent path used by the current minified asset.id,name,content,ttl).Verification
vendor/bin/phpcs inc/managers/class-dns-record-manager.php tests/WP_Ultimo/Managers/DNS_Record_Manager_Normalization_Test.php inc/ui/class-domain-mapping-element.php tests/WP_Ultimo/UI/Domain_Mapping_Element_Test.phpvendor/bin/phpunit --no-coverage --filter 'Domain_Mapping_Element_Test|DNS_Record_Manager_Normalization_Test'node_modules/.bin/eslint --resolve-plugins-relative-to /home/dave/Git/ultimate-multisite /home/dave/Git/ultimate-multisite-user-manage-dns/assets/js/dns-management.js --quietdns-management.min.js, openeduser_manage_dns_recordsvia wubox, Vue mounted,loadingVisible: false, and the modal displayed read-only DNS records instead of spinning forever.Summary by CodeRabbit
Bug Fixes
Tests