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.

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

fix(ui): Fix yup validation bugs #387

Merged

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Jul 23, 2024

Context

In PR #384, the yup package had been updated from 0.29.1 to 1.4.0. However, this huge upgrade had a lot of breaking changes (see the latest README.md vs the old one) and effectively broke most of the existing validation schemas in the Turing UI. This PR serves to fix those broken validation schemas - these changes can largely be grouped into the following:

  • The Schema.when function has been updated, such that when the builder object (one with the fields is, then, and sometimes otherwise defined) is passed as an argument, the then field accepts ONLY functions of the following signature (schema: Schema) => Schema instead of Schema previously.

    Existing schemas that do not follow this convention have been updated and turned into arrow functions.

    Example:

    // before
    some_field: yup.string().when("some_other_field", {
      is: "nop",
      then: yup.string().required("this is needed")
    }),
    
    // after
    some_field: yup.string().when("some_other_field", {
      is: "nop",
      then: (_) => yup.string().required("this is needed")
    }),
  • When using the same Schema.when function but passing a function directly as an argument instead of the builder object, the signature of the expected function has also been updated from (value, schema)=> Schema): Schema to (values: any[], schema) => Schema): Schema.

    Existing schemas that do not follow this convention now have been updated to reflect the new expected array:

    Example:

    // before
    some_field: yup.string().when("some_other_field", (some_other_field, schema) =>
      some_other_field ? yup.string().required("this is needed") : schema
    ),
    
    // after
    some_field: yup.string().when("some_other_field", ([some_other_field], schema) =>
      some_other_field ? yup.string().required("this is needed") : schema
    ),
  • The mixed schema has been severely nerfed and is now no longer the base class for all the other schemas, and as such no longer supports a lot of the other functions that we were previously calling. Schemas that have fields which use the mixed schema have been updated to use a schema (e.g. string) corresponding to its expected primitive data type.

Unrelated Update

This PR also bumps up the version of the caraml-dev/ui-lib library to the latest version.

@deadlycoconuts deadlycoconuts added the type: bug Something isn't working label Jul 23, 2024
@deadlycoconuts deadlycoconuts self-assigned this Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.26%. Comparing base (dd0b448) to head (e618957).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
- Coverage   62.27%   62.26%   -0.02%     
==========================================
  Files         124      124              
  Lines        9774     9774              
==========================================
- Hits         6087     6086       -1     
- Misses       2949     2950       +1     
  Partials      738      738              
Flag Coverage Δ
api-test 62.26% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deadlycoconuts added a commit to caraml-dev/merlin that referenced this pull request Jul 23, 2024
# Description
Similar to caraml-dev/turing#387, this PR to
fixes broken validation schemas caused by the upgrade of the `yup`
package in PR #591 - these changes can largely be grouped into the
following:

- The `Schema.when` function has been
[updated](https://github.com/jquense/yup?tab=readme-ov-file#schemawhenkeys-string--string-builder-object--values-any-schema--schema-schema),
such that when the builder object (one with the fields `is`, `then`, and
sometimes `otherwise` defined) is passed as an argument, the `then`
field accepts ONLY functions of the following signature `(schema:
Schema) => Schema` instead of `Schema` previously.

Existing schemas that do not follow this convention have been updated
and turned into arrow functions.
  
  Example:
  ```javascript
  // before
  some_field: yup.string().when("some_other_field", {
    is: "nop",
    then: yup.string().required("this is needed")
  }),

  // after
  some_field: yup.string().when("some_other_field", {
    is: "nop",
    then: (_) => yup.string().required("this is needed")
  }),
  ```
- When using the same `Schema.when` function but passing a function
directly as an argument instead of the builder object, the signature of
the expected function has also been updated from `(value, schema)=>
Schema): Schema` to `(values: any[], schema) => Schema): Schema`.

Existing schemas that do not follow this convention now have been
updated to reflect the new expected array:

  Example:
  ```javascript
  // before
some_field: yup.string().when("some_other_field", (some_other_field,
schema) =>
    some_other_field ? yup.string().required("this is needed") : schema
  ),

  // after
some_field: yup.string().when("some_other_field", ([some_other_field],
schema) =>
    some_other_field ? yup.string().required("this is needed") : schema
  ),
  ```
## Unrelated Update
This PR also bumps up the version of the `caraml-dev/ui-lib` library to
the latest version.

# Modifications
- UI bug fixes
- Update to `caraml-dev/ui-lib` library

# Tests
<!-- Besides the existing / updated automated tests, what specific
scenarios should be tested? Consider the backward compatibility of the
changes, whether corner cases are covered, etc. Please describe the
tests and check the ones that have been completed. Eg:
- [x] Deploying new and existing standard models
- [ ] Deploying PyFunc models
-->

# Checklist
- [x] Added PR label
- [ ] Added unit test, integration, and/or e2e tests
- [x] Tested locally
- [ ] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Release Notes
```release-note
NONE
```
@deadlycoconuts
Copy link
Contributor Author

Thanks for the quick approval! Merging this after the pipeline passed (I temporarily removed codecov for the API server since it's constantly failing the past PRs even though there are no API server related changes; the threshold doesn't seem to work either which is weird)! 🚀

@deadlycoconuts deadlycoconuts merged commit 1c83ad3 into caraml-dev:main Jul 23, 2024
12 checks passed
@deadlycoconuts deadlycoconuts deleted the fix_ui_deps_upgrade_bugs branch August 1, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants