Lite: Fix number field validation appearing when min/max values are empty by shervElmi · Pull Request #2664 · Strategy11/formidable-forms

shervElmi · 2025-12-17T13:05:51Z

Pro PR

https://github.com/Strategy11/formidable-pro/pull/6137

Summary by CodeRabbit

New Features
- Added validation for range and numeric field settings in the form builder: invalid configs (e.g., min ≥ max, non-positive or oversized step) now show clear error messages and highlight affected fields.
- Improved field-type/ID detection to make validations more reliable.
- Validation utilities are now exposed so other admin tools can trigger or reuse these checks.

… range and step settings

…nge event handler

…export getRangeSettingsDefaults

coderabbitai · 2025-12-17T16:16:08Z

Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: db5f5cd6-6b5d-4896-b8a6-08acf8a95400

📥 Commits

Reviewing files that changed from the base of the PR and between 382a5bc and 6573fc5.

📒 Files selected for processing (2)

js/formidable_admin.js
js/src/admin/settings/validateRangeSettings.js

📝 Walkthrough

Walkthrough

Adds client-side validation for range and step settings: a generic validator that shows errors in the info modal and restores focus, range/step-specific checks (min < max, step bounds), builder wiring to run validations on change, and a public API under frmAdminBuild.settings.validate.

Changes

Range Settings Validation

Layer / File(s)	Summary
Generic Field Validation Framework `js/src/admin/settings/validateField.js`	`validateField(field, getError)` runs a validation callback, shows the info modal on error, toggles the `frm_invalid_field` CSS class, and restores focus after modal dismissal via `focusFieldOnModalDismiss`.
Range and Step Validators `js/src/admin/settings/validateRangeSettings.js`	`getRangeSettingsDefaults` derives numeric defaults with `frm_range_settings_defaults` filter support. `validateNumberRangeSetting` enforces min < max for number-range fields. `validateStepSetting` enforces step > 0 and that step ≤ (max - min). `validateRangeSettings` runs both and applies `frm_validate_range_settings`.
Builder Integration and Public API `js/src/admin/admin.js`	Imports validation utilities, invokes `validateRangeSettings(target)` in `handleBuilderChangeEvent`, and exposes validation functions under `frmAdminBuild.settings.validate` for external use.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 New validators jump and play,

min and max kept in their place,
steps counted true, not astray,
modals blink and return my gaze,
builder hums with validation grace.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change—adding validation utilities to fix number field validation appearing when min/max values are empty, which is reflected throughout the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix-number-validation-popup

⚔️ Resolve merge conflicts

Resolve merge conflict in branch fix-number-validation-popup

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (3)

js/src/admin/admin.js (2)

8458-8463: Builder change handler now conditionally runs range/step validation

Hooking validateNumberRangeSetting(target) and validateStepSetting(target) from handleBuilderChangeEvent behind ! frmGlobal.proIsConnected looks safe: both helpers guard on the element’s context (.frm-number-range / .frm-step) and will no-op for unrelated changes, so the extra work on the builder change stream is minimal while avoiding duplicate validation when Pro is connected.

If you want to tighten this further later, you could short‑circuit on target.closest('.frm-number-range, .frm-step') before calling either validator to avoid two function calls for clearly unrelated inputs.

11170-11180: Public frmAdminBuild.settings.validate API looks good; fill in @since

Exposing validateField, getRangeSettingsDefaults, validateNumberRangeSetting, and validateStepSetting under frmAdminBuild.settings.validate is a reasonable and coherent namespace, and aligns with how other helpers are surfaced from this module. The only nit is the JSDoc @since x.x placeholder above it; consider replacing this with the actual release version so downstream consumers know when this API became available.
js/src/admin/settings/validateRangeSettings.js (1)
51-120: Consider extracting common field lookup pattern.

Both validateNumberRangeSetting and validateStepSetting share a similar pattern for retrieving singleSettings, fieldId, and related input elements. Consider extracting this into a shared helper function to reduce duplication.

Example refactoring:
/**
 * Gets common field validation context.
 */
function getFieldContext( field, containerClass ) {
	if ( ! field.closest( containerClass ) ) {
		return null;
	}
	
	const singleSettings = field.closest( '.frm-single-settings' );
	const fieldId = getFieldId( singleSettings );
	
	return fieldId ? { singleSettings, fieldId } : null;
}

export function validateNumberRangeSetting( field ) {
	const context = getFieldContext( field, '.frm-number-range' );
	if ( ! context ) {
		return;
	}
	
	const { singleSettings, fieldId } = context;
	// ... rest of validation
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d6fde5 and 43eed37.

⛔ Files ignored due to path filters (1)

js/formidable_admin.js.map is excluded by !**/*.map, !**/*.map

📒 Files selected for processing (5)

classes/helpers/FrmAppHelper.php (1 hunks)
js/src/admin/admin.js (3 hunks)
js/src/admin/settings/utils.js (1 hunks)
js/src/admin/settings/validateField.js (1 hunks)
js/src/admin/settings/validateRangeSettings.js (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

js/src/admin/admin.js (1)

js/src/admin/settings/validateRangeSettings.js (2)

validateNumberRangeSetting (51-79)

validateStepSetting (88-120)

js/src/admin/settings/validateRangeSettings.js (1)

js/src/admin/settings/validateField.js (1)

validateField (11-21)

🔇 Additional comments (5)

classes/helpers/FrmAppHelper.php (1)

3868-3868: LGTM. The proIsConnected addition correctly exposes the Pro connection status to frontend validation logic. The implementation gates number field range and step validation to run only when Pro is disconnected, with proper handling of empty min/max values via default values. The validation logic is sound and works correctly in both connected and disconnected states.

js/src/admin/admin.js (1)

4-8: New validation imports are consistent and correctly scoped

The new require calls for validateField, getRangeSettingsDefaults, validateNumberRangeSetting, and validateStepSetting match the existing CommonJS pattern in this file and are only used where needed; no issues from a bundling or scoping perspective.

js/src/admin/settings/utils.js (1)

11-30: LGTM! Good use of optional chaining.

Both utility functions properly handle null/undefined inputs using optional chaining, making them safe to call in various contexts without explicit null checks by the caller.

js/src/admin/settings/validateRangeSettings.js (2)

72-78: LGTM! Correctly handles empty values.

The validation logic properly addresses the PR objective by using fallback defaults (0 for min, 9999999 for max) when fields are empty. This ensures validation doesn't trigger incorrectly when min/max values are not provided, while still catching actual invalid entries.

104-119: LGTM! Step validation logic is sound.

The validation correctly ensures step is positive and doesn't exceed the maximum value, while gracefully handling empty inputs using defaults.

deepsource-io · 2026-06-05T20:14:57Z

DeepSource Code Review

We reviewed changes in b7cd07c...b3ae6ed on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Jun 5, 2026 8:32p.m.	Review ↗
JavaScript		Jun 5, 2026 8:32p.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.

deepsource-io · 2026-06-05T20:15:27Z

+	return validateField( field, () => {
+		const { minNum, maxNum } = getRangeSettingsDefaults( singleSettings );
+
+		return parseFloat( minValueInput.value || minNum ) >= parseFloat( maxValueInput.value || maxNum )
+			? __( 'Minimum value cannot be greater than or equal to maximum value.', 'formidable' )
+			: '';
+	} );


Function 'validateNumberRangeSetting' expected no return value

Any code paths that do not have explicit returns will return undefined. It is recommended to replace any implicit dead-ends that return undefined with a return null statement.

deepsource-io · 2026-06-05T20:15:27Z

+	return validateField( field, () => {
+		const { step, maxNum } = getRangeSettingsDefaults( singleSettings );
+		const stepInputValue = parseFloat( stepInput.value || step );
+		if ( stepInputValue <= 0 ) {
+			return __( 'Step value must be greater than 0.', 'formidable' );
+		}
+
+		const maxValueInput = document.querySelector( `[name="field_options[maxnum_${ fieldId }]"]` );
+		if ( ! maxValueInput ) {
+			return '';
+		}
+
+		return stepInputValue > parseFloat( maxValueInput.value || maxNum )
+			? __( 'Step value must be less than maximum value.', 'formidable' )
+			: '';
+	} );


Function 'validateStepSetting' expected no return value

Any code paths that do not have explicit returns will return undefined. It is recommended to replace any implicit dead-ends that return undefined with a return null statement.

deepsource-io · 2026-06-05T20:28:07Z

+	return validateField( field, () => {
+		const { step, minNum, maxNum } = getRangeSettingsDefaults( singleSettings );
+		const stepInputValue = parseFloat( stepInput.value || step );
+		if ( stepInputValue <= 0 ) {
+			return __( 'Step value must be greater than 0.', 'formidable' );
+		}
+
+		const maxValueInput = document.querySelector( `[name="field_options[maxnum_${ fieldId }]"]` );
+		if ( ! maxValueInput ) {
+			return '';
+		}
+
+		const minValueInput = document.querySelector( `[name="field_options[minnum_${ fieldId }]"]` );
+		const minValue = parseFloat( minValueInput?.value || minNum );
+		const maxValue = parseFloat( maxValueInput.value || maxNum );
+
+		return stepInputValue > maxValue - minValue
+			? __( 'Step value cannot be greater than the difference between the minimum and maximum values.', 'formidable' )
+			: '';
+	} );


Function 'validateStepSetting' expected no return value

Any code paths that do not have explicit returns will return undefined. It is recommended to replace any implicit dead-ends that return undefined with a return null statement.

Crabcyborg

I made several changes.

This no longer requires a JS var to determine if Pro is connected.

I also made some changes in Pro. When Lite wasn't up to date, the errors were not appearing.

And the step validation was weird. I updated it to check for the difference between max and min.

shervElmi added 5 commits December 17, 2025 15:13

Add proIsConnected flag to global admin JavaScript strings

97c02b5

Add utility functions to get field ID and type from settings elements

f3ce3a8

Add validateField utility function for field validation and UI feedback

18de437

Add validateRangeSettings module with validation functions for number…

f92f1a5

… range and step settings

Add validation for number range and step settings in form builder cha…

0df7555

…nge event handler

shervElmi self-assigned this Dec 17, 2025

Add formidable_admin.js file

d0944d6

shervElmi mentioned this pull request Dec 17, 2025

Lite: Fix number field validation appearing when min/max values are empty #2647

Closed

shervElmi added 2 commits December 17, 2025 18:49

Reorganize validation functions into settings.validate namespace and …

bf245ed

…export getRangeSettingsDefaults

Add formidable_admin.js file

43eed37

shervElmi marked this pull request as ready for review December 17, 2025 16:12

shervElmi added run analysis run tests labels Dec 17, 2025

shervElmi requested a review from Crabcyborg December 17, 2025 16:12

coderabbitai Bot reviewed Dec 17, 2025

View reviewed changes

Comment thread js/src/admin/settings/validateField.js

Comment thread js/src/admin/settings/validateRangeSettings.js

Crabcyborg added this to the 6.28 milestone Jan 5, 2026

Crabcyborg modified the milestones: 6.28, 6.29 Feb 12, 2026

Crabcyborg modified the milestones: 6.29, 6.30 Mar 9, 2026

Crabcyborg modified the milestones: 6.30 (Pricing Fields), 6.31 (PayPal Commerce) Apr 15, 2026

Crabcyborg modified the milestones: 6.31 (Form Action Refresh, PayPal Commerce), 6.32 May 26, 2026

Update validation logic so it is easier for Pro to extend

382a5bc

deepsource-io Bot reviewed Jun 5, 2026

View reviewed changes

Improve step validation

6573fc5

deepsource-io Bot reviewed Jun 5, 2026

View reviewed changes

Merged master

b3ae6ed

Crabcyborg approved these changes Jun 5, 2026

View reviewed changes

Crabcyborg merged commit f1aebec into master Jun 5, 2026
19 of 22 checks passed

Crabcyborg deleted the fix-number-validation-popup branch June 5, 2026 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lite: Fix number field validation appearing when min/max values are empty#2664

Lite: Fix number field validation appearing when min/max values are empty#2664
Crabcyborg merged 11 commits into
masterfrom
fix-number-validation-popup

shervElmi commented Dec 17, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 17, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

deepsource-io Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

deepsource-io Bot Jun 5, 2026

Uh oh!

deepsource-io Bot Jun 5, 2026

Uh oh!

deepsource-io Bot Jun 5, 2026

Uh oh!

Crabcyborg left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shervElmi commented Dec 17, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pro PR

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deepsource-io Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

deepsource-io Bot Jun 5, 2026

Choose a reason for hiding this comment

Function 'validateNumberRangeSetting' expected no return value

Uh oh!

deepsource-io Bot Jun 5, 2026

Choose a reason for hiding this comment

Function 'validateStepSetting' expected no return value

Uh oh!

deepsource-io Bot Jun 5, 2026

Choose a reason for hiding this comment

Function 'validateStepSetting' expected no return value

Uh oh!

Crabcyborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shervElmi commented Dec 17, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 17, 2025 •

edited

Loading

deepsource-io Bot commented Jun 5, 2026 •

edited

Loading