-
Notifications
You must be signed in to change notification settings - Fork 10
T2591 extend usage of form field validator #230
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
base: 14.0-MyCompassion2.0
Are you sure you want to change the base?
T2591 extend usage of form field validator #230
Conversation
…er in my2_new_sponsorship_wizard.js
…s/form_field_validator.js
…er in my2_new_sponsorship_wizard.js
…s/form_field_validator.js
…sership-form' into T2586-phone-number-check-in-sponsership-form # Conflicts: # theme_compassion_2025/templates/components/FormField.xml
…-sponsership-form T2586 phone number check in sponsership form
Summary of ChangesHello @EliasKeller, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the client-side form validation system by introducing a new, reusable Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a reusable FormFieldValidator widget and refactors the new sponsorship wizard to use it, which is a great step towards centralizing validation logic and improving code maintainability. The implementation of the new widget is solid, but I've identified a critical issue regarding how data- attributes are accessed, which would prevent the validator from working as intended. I've also noted a minor inefficiency with event handling. Once these points are addressed, this will be an excellent enhancement.
… for sponsorship-wizard
|
/gemini-review |
|
It looks like you tried to invoke a command, but there was a typo. The correct command for a code review is |
|
/gemini review |
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.
Code Review
This pull request introduces a centralized client-side validation system for form fields, which is a significant improvement for code maintainability and UI consistency. The changes are well-structured, including a new Odoo widget for validation, updated CSS for error states, and refactoring of existing forms and JavaScript to use the new system. My review focuses on ensuring the new validation logic is applied consistently and correctly across all components. I've identified a couple of high-priority issues related to inconsistent handling of required fields and a missing attribute that impacts accessibility. I've also included a medium-severity suggestion to improve code clarity. Addressing these points will make the new validation system more robust and reliable.
| <input | ||
| type="email" | ||
| name="email" | ||
| class="form-control" | ||
| t-att-value="wizard.email" | ||
| data-validate-type="email" | ||
| required="" | ||
| /> |
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.
The id attribute is missing from this email input field. The parent FormFieldComponent defines a label with for="email", which requires the input to have a matching id for accessibility (e.g., for screen readers) and for the common UX pattern of clicking the label to focus the input field.
<input
id="email"
type="email"
name="email"
class="form-control"
t-att-value="wizard.email"
data-validate-type="email"
required=""
/>
| data-is-required="true" | ||
| data-validate-type="phone" |
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.
This phone input uses data-is-required="true" to mark it as a required field, but the new validator in form_field_validator.js only checks for the standard required attribute/property. This means the field will not be validated as required, which seems to contradict the intention here. To ensure this field is validated correctly, you should either replace data-is-required="true" with required="" or update the validator to also check for the data-is-required attribute (a suggestion for this has been made on form_field_validator.js).
| this.$el.data("widget", this); | ||
|
|
||
| this.$input = this.$("input, select"); | ||
| this.isRequired = this.$input.prop("required"); |
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.
There's an inconsistency in how required fields are marked across the codebase. Some use the standard required attribute, while others use data-is-required="true" (e.g., the phone field in my2_new_sponsorship_wizard.xml and the SelectComponent). However, the validator currently only checks for the required property.
This is also hinted at by the TODO comment in Select.xml which suggests removing t-att-required. To unify the behavior and support both methods, I suggest updating the isRequired check to consider both the property and the data attribute. This will ensure all fields intended to be required are validated correctly.
| this.isRequired = this.$input.prop("required"); | |
| this.isRequired = this.$input.prop("required") || this.$input.data("is-required"); |
| const clearErrors = () => { | ||
| let isValid = true; | ||
|
|
||
| $(form) | ||
| .find(".form-field-component:visible") | ||
| .each(function () { | ||
| const fieldWidget = $(this).data("widget"); | ||
| fieldWidget.clearError(); | ||
| }); | ||
|
|
||
| return isValid; | ||
| }; |
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.
The clearErrors function initializes isValid to true and returns it, but this variable is never modified, and its returned value is not used in the calling code. This makes the code slightly misleading. Additionally, fieldWidget.clearError() could throw an error if fieldWidget is undefined. To improve clarity, remove the unused isValid variable and add a guard clause to ensure fieldWidget exists before calling methods on it.
const clearErrors = () => {
$(form)
.find(".form-field-component:visible")
.each(function () {
const fieldWidget = $(this).data("widget");
if (fieldWidget) {
fieldWidget.clearError();
}
});
};
Description
This PR introduces a centralized client-side validation system for form fields and rolls it out across all forms using the theme components.
It evolved out of the previous validation PR and unifies validation behavior and UI states across the theme.
User Settings:
New Sponsership (Step 1):
New Sponsership (Step 2):
And all the other fields and forms....
What was implemented
Centralized all validation logic into
theme_compassion_2025/static/src/utils/form_field_validator.jsValidation is now applied everywhere where:
theme_compassion_2025.FormFieldComponentortheme_compassion_2025.SelectComponentis used
Validation runs on user interaction:
blurfor inputschangefor selectsFields expose their validation widget via
.data("widget")so validation can also be triggered externally if needed
How validation is enabled
Validation is active when a field has:
requiredand/ordata-validate-typeExample:
Supported validation types
requiredemailphoneUX behavior
is-invalidclassSelectComponentwrapper)Extending validation
Validation can be extended easily by adding a new entry to
validationConfiginform_field_validator.js.Each validation type only needs:
regexdefaultErrorMessageThe new validation can then be used via:
Notes