Fix template validation failing when addon product is selected#347
Merged
superdav42 merged 2 commits intomainfrom Feb 27, 2026
Merged
Fix template validation failing when addon product is selected#347superdav42 merged 2 commits intomainfrom
superdav42 merged 2 commits intomainfrom
Conversation
…ckout When merging product limitations during checkout, addon products with site_templates.limit = null would overwrite the plan's template list in additive mode, causing get_available_site_templates() to return an empty array. This made the validation rule reject all template selections with "The selected template is not available for this product." Two fixes: - merge_recursive now skips null values in additive mode, consistent with existing behavior for 0 and '' (unlimited) values - get_empty() now returns proper default module states in to_array(), so non-plan products only store actual differences from defaults Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors Limitations merging to prevent nulls from overwriting existing additive values, consolidates merge assignment, batch-initializes default module states, adds tests for these behaviors, adds a void return type to a PSR logger setter, and merges caller query params into the local query in AJAX model search. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/WP_Ultimo/Objects/Limitations_Test.php (2)
819-820: Consider using string keys in assertions for consistency.The
limitarray uses string keys ('2','3'), andget_available_site_templates()returns$site_idwhich preserves the string type. Depending on PHPUnit version and strict mode settings, comparing integer2against string'2'may behave unexpectedly.🔧 Suggested fix
- $this->assertContains(2, $available, 'Template 2 should still be available after merging addon with null limit'); - $this->assertContains(3, $available, 'Template 3 should still be available after merging addon with null limit'); + $this->assertContains('2', $available, 'Template 2 should still be available after merging addon with null limit'); + $this->assertContains('3', $available, 'Template 3 should still be available after merging addon with null limit');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/Objects/Limitations_Test.php` around lines 819 - 820, The assertions use integer literals (2, 3) while the `limit` array and `get_available_site_templates()` preserve string keys, which can cause mismatch; update the two assertions in Limitations_Test (the lines calling `$this->assertContains(2, $available, ...)` and `$this->assertContains(3, $available, ...)`) to assert against string keys ('2' and '3') so the types match the returned `$site_id` strings from `get_available_site_templates()`.
907-908: Same type consideration as earlier test.Similar to the earlier test, consider using string keys (
'5','7') in assertions to match the array key types in the limit configuration.🔧 Suggested fix
- $this->assertContains(5, $available, 'Template 5 should be available'); - $this->assertContains(7, $available, 'Template 7 should be available'); + $this->assertContains('5', $available, 'Template 5 should be available'); + $this->assertContains('7', $available, 'Template 7 should be available');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/Objects/Limitations_Test.php` around lines 907 - 908, The test is asserting numeric keys but the limit configuration uses string keys; update the assertions that check $available to use string keys ('5' and '7') instead of integers so the types match (i.e., change the two calls using $this->assertContains(5, $available, ...) and $this->assertContains(7, $available, ...) to $this->assertContains('5', $available, ...) and $this->assertContains('7', $available, ...)); alternatively you can replace them with $this->assertArrayHasKey('5', $available, ...) and $this->assertArrayHasKey('7', $available, ...) to explicitly assert keys exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/WP_Ultimo/Objects/Limitations_Test.php`:
- Around line 819-820: The assertions use integer literals (2, 3) while the
`limit` array and `get_available_site_templates()` preserve string keys, which
can cause mismatch; update the two assertions in Limitations_Test (the lines
calling `$this->assertContains(2, $available, ...)` and
`$this->assertContains(3, $available, ...)`) to assert against string keys ('2'
and '3') so the types match the returned `$site_id` strings from
`get_available_site_templates()`.
- Around line 907-908: The test is asserting numeric keys but the limit
configuration uses string keys; update the assertions that check $available to
use string keys ('5' and '7') instead of integers so the types match (i.e.,
change the two calls using $this->assertContains(5, $available, ...) and
$this->assertContains(7, $available, ...) to $this->assertContains('5',
$available, ...) and $this->assertContains('7', $available, ...)); alternatively
you can replace them with $this->assertArrayHasKey('5', $available, ...) and
$this->assertArrayHasKey('7', $available, ...) to explicitly assert keys exist.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
inc/objects/class-limitations.phppatches/mpdf-mpdf-psr-log-aware-trait-void-return.patchtests/WP_Ultimo/Objects/Limitations_Test.php
The $query variable was built from exclude/include params but never merged with $args['query'] which contains the search term. This caused server-side search filtering to silently fail (selectize.js client-side filtering masked it). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔨 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: |
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
merge_recursivein additive mode allowednullvalues from addon products'site_templates.limitto overwrite the plan's template list, causingget_available_site_templates()to return an empty arraymerge_recursivenow skips null values in additive mode (consistent with existing 0/'' unlimited handling)get_empty()now returns proper default states into_array(), so non-plan products only store actual differences from defaults rather than all modulesTest plan
get_empty()->to_array()returning proper default states🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests