Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

forms: generate & use yup validation schemas #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

forms: generate & use yup validation schemas #206

wants to merge 1 commit into from

Conversation

mirekys
Copy link

@mirekys mirekys commented May 15, 2024 •
edited

Description

  • Fixes Assertion error when creating banner from admin panel聽invenio-banners#26 by fixing form validation of required dropdown fields.

  • Some required SUI inputs cannot be validated natively by browser (e.g. Dropdown). It is possible to submit a form with a dropdown input marked as required, having undefined value.

  • This PR introduces an autogenerated Yup form validation schemas to address the issue.

  • Validation schema is generated based on resource's marshmallow schema.

  • Currently it supports only field type (numeric, string,...) and required validation rules, more rules could be implemented in mapValidationRules function.

Checklist

Ticks in all boxes and 馃煝 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub鈥檚 Terms of Service including that:

  1. You license your contribution under the same terms as the current repository鈥檚 license.
  2. You agree that you have the right to license your contribution under the current repository鈥檚 license.

@mirekys
forms: generate & use yup validation schemas
30591ad 
* Fixes invenio-banners#26 by fixing form validation of required dropdown fields.

* Some required SUI inputs cannot be validated natively in browser
  (e.g. Dropdown). This commit introduces autogenerated Yup form
  validation schema to address the issue.
@kpsherva
Copy link
Contributor

kpsherva commented Jun 12, 2024 •
edited

Thank you for trying to fix the issue!
I just have a general comment, I think the fix should be done in invenio banners, we tried to avoid frontend validation of the administration forms, in favour of the marshmallow validation, mostly because there might be many customized fields, and it would get very complex. I think we are simply missing default value in the Marshmallow schema for the field, since the whole form builder is based on marshmallow schemas.

@mirekys
Copy link
Author

mirekys commented Jun 13, 2024

Hi @kpsherva, thanks for the comment, I initially tried to solve it purely using marshmallow schema (ensuring it has a default value there & is flagged as required).

But for some form input fields (even more so, if there would be many complex custom fields), setting & honoring required (or things like min/max length) attribute on the field wouldn't work. In invenio banners case, it was the Form.Dropdown component, that doesn't forward the required attribute to its corresponding <input> tag (so the native browser validation won't work), while other simpler required inputs were still properly validated by FE.

If we decide to drop FE validation completely, it would mean the BE services for every form needs to expect & gracefully handle even completely invalid submitted data & respond with schema validation errors feedback that could possibly be feeded into formik to be reflected on FE side. I can see that as a viable option too, but currently, BE just crashes there, not raising any ValidationError.

@mirekys
Copy link
Author

mirekys commented Jun 13, 2024

mostly because there might be many customized fields

By customized fields you mean a planned support for Custom fields like it is in records deposit form?

@kpsherva
Copy link
Contributor

mostly because there might be many customized fields

By customized fields you mean a planned support for Custom fields like it is in records deposit form?

Sorry, I meant complex type fields, nesting etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assertion error when creating banner from admin panel
2 participants
@mirekys @kpsherva