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

[shervElmi]

Pro PR

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

[coderabbitai]

Walkthrough

This PR adds form field validation utilities for numeric range settings and step values, integrating Pro connection status into the admin frontend. It introduces conditional validation logic triggered when Pro is not connected, along with utility functions for field identification and type detection.

Changes

Cohort / File(s)	Change Summary
Backend Configuration classes/helpers/FrmAppHelper.php	Added proIsConnected entry to global JavaScript localization object, exposing Pro connection status to the frontend via self::pro_is_connected().
Frontend Validation Utilities js/src/admin/settings/validateField.js, js/src/admin/settings/validateRangeSettings.js, js/src/admin/settings/utils.js	Added field validation module with validateField function that displays error modals and manages UI feedback. Added range validation utilities: getRangeSettingsDefaults, validateNumberRangeSetting, validateStepSetting. Added utility functions getFieldId and getFieldType for field identification and type detection.
Admin Module Integration js/src/admin/admin.js	Enhanced handleBuilderChangeEvent to conditionally run validateNumberRangeSetting and validateStepSetting when proIsConnected is false. Extended public frmAdminBuild API with new settings.validate property exposing validation functions.

Sequence Diagram

sequenceDiagram
    participant User
    participant Admin as Admin UI
    participant Event as Builder<br/>Change Event
    participant Validator as Validation<br/>Logic
    participant Modal as Modal/UI
    participant i18n as WordPress<br/>i18n

    User->>Admin: Modify field range settings
    Admin->>Event: Trigger handleBuilderChangeEvent
    Event->>Validator: Check frmGlobal.proIsConnected
    alt Pro NOT Connected
        Validator->>Validator: validateNumberRangeSetting(field)
        Validator->>Validator: Locate min/max inputs<br/>via fieldId
        Validator->>Validator: getRangeSettingsDefaults()
        Validator->>Validator: Validate min < max
        alt Validation Error
            Validator->>i18n: Get error message
            Validator->>Modal: Show error via<br/>validateField()
            Modal->>Admin: Display info modal<br/>+ CSS class
        else Valid
            Modal->>Admin: Remove error class
        end
        Validator->>Validator: validateStepSetting(field)
        Validator->>Validator: Validate step rules<br/>(step > 0, step ≤ max)
        alt Step Error
            Validator->>i18n: Get error message
            Validator->>Modal: Show error
            Modal->>Admin: Update UI feedback
        end
    end
    Event-->>Admin: Change handling complete

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Conditional validation logic in admin.js: Verify correct guard conditions and integration with existing change handlers
• DOM selector logic in validateRangeSettings.js: Ensure robust field/input location based on fieldId and class name patterns
• Modal/UI feedback mechanism in validateField.js: Confirm proper modal display and CSS class management for error states
• Cross-module dependencies: Validate correct imports and function signatures across new utility modules

Suggested labels

run analysis, run tests, action: needs qa

Suggested reviewers

• Crabcyborg
• garretlaxton
• truongwp

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: fixing number field validation to not appear when min/max values are empty.
Description check	✅ Passed	The description references a corresponding Pro PR and relates to the changeset's purpose of fixing number field validation issues.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests 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

• X
• Mastodon
• Reddit
• LinkedIn

_{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.