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

[WIP] feat: improve form validation #919

Merged

Conversation

akashrajum7
Copy link
Contributor

@akashrajum7 akashrajum7 commented Aug 31, 2024

Summary of change

This PR aims to improve the form field validation in emailpassword to allow for optional form fields to be truly optional and not have to send empty values. It looks like the rest of the recipes use the UserMetadata recipe to store extra information.

Related issues

Test Plan

Added a test case to check the functionality of optional fields missing in the payload does not result in an error

Documentation changes

Not sure which section of the docs talks about the behaviour of optional fields currently, can update there once this implementation is completed

Checklist for important updates

  • Changelog has been updated
  • Changes to the version if needed
    • In package.json
    • In package-lock.json
    • In lib/ts/version.ts
  • Had run npm run build-pretty
  • Had installed and ran the pre-commit hook
  • Issue this PR against the latest non-released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

  • Get clarity on what it means to "improve error message" for missing form fields

Copy link

netlify bot commented Aug 31, 2024

Deploy Preview for astounding-pegasus-21c111 canceled.

Name Link
🔨 Latest commit 7c4b0c7
🔍 Latest deploy log https://app.netlify.com/sites/astounding-pegasus-21c111/deploys/66d32fd9930e1d0008a7ead2

Copy link

netlify bot commented Aug 31, 2024

Deploy Preview for precious-marshmallow-968a81 canceled.

Name Link
🔨 Latest commit 7c4b0c7
🔍 Latest deploy log https://app.netlify.com/sites/precious-marshmallow-968a81/deploys/66d32fd95947bb0008b6a0f5

@akashrajum7
Copy link
Contributor Author

akashrajum7 commented Aug 31, 2024

@rishabhpoddar - Need your inputs here:

  1. What does it mean by "improve error message"? An example would be super helpful.
  2. Does this approach of validating optional fields only when present sound right to you?

Thanks.

@rishabhpoddar rishabhpoddar changed the base branch from master to 20.0 September 2, 2024 06:01
@rishabhpoddar rishabhpoddar merged commit 38b49b7 into supertokens:20.0 Sep 2, 2024
14 of 24 checks passed
@rishabhpoddar
Copy link
Member

Please feel free to add yourself to the readme of the supertokens-core repo

@rishabhpoddar
Copy link
Member

@akashrajum7
Copy link
Contributor Author

Hey @akashrajum7 there are two tests that are failing due to the changes made here: https://app.circleci.com/pipelines/github/supertokens/supertokens-node/5985/workflows/cfe50d90-2d11-4f6d-bf95-180345c1235c/jobs/3363

Last I ran this locally, everything went through fine, let me check again and open a new PR in case any changes are required, thanks.

@rishabhpoddar
Copy link
Member

@akashrajum7 we are doing the fix on our end now, since we have to do a release soon, and this is blocking our release.

@akashrajum7
Copy link
Contributor Author

@rishabhpoddar Sure, thanks. Would be nice if there was a pre-check on the test cases before the merge passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants