-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hidden field can never be required. Skip that validation #716
The head ref may contain hidden characters: "1a2a6-can\u2019t-be-hidden-&-required"
Conversation
WalkthroughThis pull request updates the backend validation logic in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (1)
297-303
: Existing logic conflicts with the new ruleI notice that the current logic handling for multiple actions in
onActionInput()
already attempts to prevent the contradictory scenario of having both "hide-block" and "require-answer" actions together, which aligns with the new rule. However, it's implemented as an either-or choice rather than completely preventing "require-answer" when "hide-block" is selected.Since the backend now enforces that hidden fields cannot be required, it might be better to update the UI to disable the "Require answer" option entirely when "Hide Block" is selected.
You could consider modifying the
actionOptions
computed property to dynamically exclude the "Require answer" option when "hide-block" is in the selected actions:actionOptions() { // Existing code... if (this.field.hidden) { return [ { name: "Show Block", value: "show-block" }, - { name: "Require answer", value: "require-answer" }, ] } else if (this.field.disabled) { // Existing code... } else { + const actions = [ + { name: "Hide Block", value: "hide-block" }, + { name: "Disable Block", value: "disable-block" }, + ]; + + // Only add "Require answer" option if "hide-block" is not selected + if (!this.logic.actions.includes("hide-block")) { + actions.push(this.field.required + ? { name: "Make it optional", value: "make-it-optional" } + : { name: "Require answer", value: "require-answer" } + ); + } + + return actions; - return [ - { name: "Hide Block", value: "hide-block" }, - { name: "Disable Block", value: "disable-block" }, - this.field.required - ? { name: "Make it optional", value: "make-it-optional" } - : { - name: "Require answer", - value: "require-answer", - }, - ] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/app/Http/Requests/AnswerFormRequest.php
(1 hunks)client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (2)
api/app/Http/Requests/AnswerFormRequest.php (1)
90-90
: Logical improvement: Skip validation for hidden fieldsThe addition of the condition
(!isset($property['hidden']) || !$property['hidden'])
properly ensures that hidden fields will never be marked as required, preventing a paradoxical situation where users would be required to fill out fields they can't see.This change correctly implements the described PR objective and handles both cases where the 'hidden' property is explicitly set to true or not set at all.
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (1)
60-66
: Good UX enhancement: Clear notification about hidden field behaviorThe added alert provides users with clear information about the constraint that hidden fields cannot be required. This aligns perfectly with the backend validation logic change and helps prevent user confusion.
The styling choices (yellow color, information icon, subtle variant) appropriately convey this as informational guidance rather than an error or warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check other PR
- Replace UAlert with plain text description - Update action description text - Remove help text from select input - Simplify note about hidden field requirements
- Implement test case for fields hidden by logic conditions - Update AnswerFormRequest to use new FormLogicPropertyResolver method for hidden field check - Ensure required fields are skipped when hidden by form logic
Done |
Summary by CodeRabbit
Form Validation Improvements
User Interface
Tests