Hide non-numeric fields for Math calculations - #2619
Conversation
Remove `isTextCalculationForBox` and `refreshCalcFieldListOnTypeChange` functions and replace with simpler `isMathCalcType` helper that directly checks the calc_type radio button value. Remove the change event listener for calc_type inputs as it's no longer needed.
Show a message "This form has no numeric fields to insert into the calculation." and hide the search box when a Math calculation type has no available numeric fields to display. Add JSDoc comment for the popCalcFields function.
Refactor the Math calculation type check to use the `isMathCalcType` helper function instead of `isTextCalculationForBox`. Simplify the conditional logic and inline the JSON parsing. Add JSDoc comment for the getExcludeArray function.
Cache the result of `isMathCalcType()` call in `popCalcFields()` to avoid redundant checks. Pass the cached value to `getExcludeArray()` function. Add optional chaining to `classList.contains()` check in `isMathCalcType()` for safer property access.
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCalculation field population now uses code-list metadata for exclusions and numeric type filtering. Math calculations accept numeric-compatible fields, while text calculations retain existing behavior. The bundled admin script includes the same filtering logic. ChangesCalculation field filtering
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)
3059-3123: Guard against missing.frm-searchelement inpopCalcFields
searchis obtained withbox.querySelector( '.frm-search' )and then used unconditionally. If the calc box is rendered without a search wrapper for any reason,searchwill benullandsearch.classList.add/removewill throw, breaking the calc modal.Consider guarding these calls:
- // If the calc type is math and there are no fields, hide search and show a message - const search = box.querySelector( '.frm-search' ); - if ( ! list.hasChildNodes() && isMathCalc ) { - search.classList.add( 'frm_hidden' ); + // If the calc type is math and there are no fields, hide search and show a message + const search = box.querySelector( '.frm-search' ); + if ( ! list.hasChildNodes() && isMathCalc ) { + if ( search ) { + search.classList.add( 'frm_hidden' ); + } list.appendChild( @@ - ); - } else { - search.classList.remove( 'frm_hidden' ); + ); + } else if ( search ) { + search.classList.remove( 'frm_hidden' ); }
🧹 Nitpick comments (2)
classes/helpers/FrmAppHelper.php (1)
5074-5097: Expose non‑numeric types via filter and finalize@sincebefore releaseThe helper cleanly centralizes the non‑numeric field type list and matches the PR intent (hide clearly text/attachment/AI style fields from math calcs). Two follow‑ups would make this more robust:
- Allow third‑party/custom field types to participate via a filter, e.g.:
- public static function non_numeric_field_types() { - return array( + public static function non_numeric_field_types() { + $types = array( 'text', 'textarea', 'email', 'url', 'name', 'phone', 'password', 'tag', 'address', 'rte', 'file', 'signature', 'ai', - ); + ); + + /** + * Filter the list of non-numeric field types used in math calculations. + * + * @since x.x + * + * @param array $types Non-numeric field type slugs. + */ + return apply_filters( 'frm_non_numeric_field_types', $types ); }
- Replace the
@since x.xplaceholder with the actual version number before merging so the public API is properly documented.js/src/admin/admin.js (1)
3134-3160: Harden JSON parsing fordata-exclude-non-numericingetExcludeArrayThe new math-calculation branch assumes
data-exclude-non-numericis always valid JSON. If the attribute is missing, empty, or malformed (e.g., from a PHP/template change),JSON.parsewill throw and break the calc field list.You could make this a bit more robust without changing behavior in the normal case:
- } else if ( isMathCalc ) { - const nonNumericTypes = codeList.getAttribute( 'data-exclude-non-numeric' ); - if ( nonNumericTypes ) { - exclude.push( ...JSON.parse( nonNumericTypes ) ); - } + } else if ( isMathCalc ) { + const nonNumericTypes = codeList.getAttribute( 'data-exclude-non-numeric' ); + if ( nonNumericTypes ) { + try { + const parsed = JSON.parse( nonNumericTypes ); + if ( Array.isArray( parsed ) ) { + exclude.push( ...parsed ); + } + } catch ( e ) { + // Silently ignore malformed data-exclude-non-numeric. + } + } }This keeps the new feature but avoids a hard failure if the data attribute ever diverges from the expected format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(2 hunks)js/src/admin/admin.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (2)
js/admin/dom.js (2)
search(328-405)__(4-4)js/admin/embed.js (1)
tag(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (1)
classes/helpers/FrmAppHelper.php (1)
3773-3784: Docblock alignment with implementation looks good
format_form_data( &$form )operates on an array of inputs, so documenting@param array $formis accurate and keeps the signature/comment in sync. No further changes needed here.
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@shervElmi I tested it and it's working as expected. Thank you!
Relocate the `non_numeric_field_types()` static method from `FrmAppHelper` to `FrmFieldsHelper` where field-type-related utility methods belong. Remove the method from FrmAppHelper as it's more appropriately scoped to field operations.
…udelist for numeric types Replace the exclude-based approach for filtering non-numeric fields in Math calculations with an allowlist approach. Move numeric field type filtering from `getExcludeArray()` to the main field loop in `popCalcFields()`. Add `getNumericFieldTypes()` helper function to retrieve allowed numeric types from `data-numeric-types` attribute. Simplify `getExcludeArray()` by removing the `isMathCalc` parameter
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@shervElmi I tested it and it's working as expected. Thank you!
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Aug 6, 2026 1:15p.m. | Review ↗ |
| JavaScript | |
Aug 6, 2026 1:15p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2619 +/- ##
=========================================
Coverage 26.30% 26.30%
+ Complexity 9467 9466 -1
=========================================
Files 155 155
Lines 31689 31689
=========================================
+ Hits 8335 8336 +1
+ Misses 23354 23353 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I removed the Pro logic from this branch in favour of using a new hook.
This should be good to go now.
🚀
3 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/6087
When building a Math calculation, non-numeric fields (Name, Email, URL, Address, Text, etc.) are now automatically hidden from the field list. These fields don't make sense in mathematical operations, so hiding them reduces confusion and prevents errors.
When switching to Text calculation type, all fields are shown again since text calculations can combine any field values.
Pro PR
https://github.com/Strategy11/formidable-pro/pull/6117
Summary by CodeRabbit