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.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughUpdates to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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!
classes/helpers/FrmAppHelper.php
Outdated
| * | ||
| * @return array | ||
| */ | ||
| public static function non_numeric_field_types() { |
There was a problem hiding this comment.
@shervElmi I feel like this would be a lot easier to manage if we just tracked numeric field types instead.
It's really just number, scale, star, slider, nps, product, quantity, total I think that should always be numbers, and even then range sliders don't really work in a calculation.
I believe radio, select, and checkbox all work as well, so we should show those.
And hidden fields.
Then possibly data and lookup, since their data sources could be numbers.
I also think we might want to still allow text and textarea, since they could still be numbers.
And it would be nice to add a filter as well, so this can be managed for custom field types and future fields in new add-ons without requiring future Lite updates. We could even add most of these in Pro / Surveys and eventually remove them from Lite. I like keeping this temporarily though for better compatibility with older Pro versions.
There was a problem hiding this comment.
@Crabcyborg, I have refactored this PR and the related Pro PR entirely based on your comments.
@lauramekaj1, I am requesting your review again since there have been major changes after your previous review.
Master PR behavior
Behavior after this PR and the related Pro PR
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
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