Skip to content

fix: add required property on input#3934

Merged
emrysal merged 31 commits intocalcom:mainfrom
Udit-takkar:fix/custom-input
Aug 29, 2022
Merged

fix: add required property on input#3934
emrysal merged 31 commits intocalcom:mainfrom
Udit-takkar:fix/custom-input

Conversation

@Udit-takkar
Copy link
Copy Markdown
Contributor

@Udit-takkar Udit-takkar commented Aug 23, 2022

Fixes: #3918
Also added checks in /api/book if required customInputs are missing.

Loom Video:- https://www.loom.com/share/01ff8da8e2764a1691f23572a37344a6

@vercel
Copy link
Copy Markdown

vercel Bot commented Aug 23, 2022

@Udit-takkar is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@Udit-takkar Udit-takkar marked this pull request as draft August 23, 2022 21:24
@Udit-takkar Udit-takkar marked this pull request as ready for review August 23, 2022 22:47
@PeerRich PeerRich requested a review from hariombalhara August 24, 2022 08:09
@PeerRich
Copy link
Copy Markdown
Member

awesome work! super fast.👏 @hariombalhara can you review and approve this?

@PeerRich PeerRich requested a review from zlwaterfield August 24, 2022 08:17
Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also have a look at using zod in the book/event endpoint; could be helpful

Comment thread apps/web/pages/api/book/event.ts Outdated
Comment thread apps/web/pages/api/book/event.ts Outdated
@Udit-takkar
Copy link
Copy Markdown
Contributor Author

@emrysal where exactly can zod be helpful in 'book/event endpoint'. i think it is already being used at the start

 extendedBookingCreateBody.parse(req.body);

Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this

Comment thread apps/web/pages/api/book/event.ts Outdated
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment that needs to be addressed before the merge.

Also, there is a cleaner way to do it using zod, which allows schema validation to work for frontend as well as backend with a single definition. See zodResolver usage in BookingPage.tsx. I wouldn't consider this a blocker for merge but it is good to have.

Thanks again for your contribution @Udit-takkar

@PeerRich PeerRich added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label Aug 25, 2022
@Udit-takkar Udit-takkar requested review from emrysal and removed request for zlwaterfield August 29, 2022 09:13
@emrysal emrysal dismissed hariombalhara’s stale review August 29, 2022 12:14

Zod changes are done & required behaviour is as intended on checkboxes (e.g. I agree to the terms & conditions)

@emrysal emrysal enabled auto-merge (squash) August 29, 2022 12:15
Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent PR - thanks for the contribution and follow ups! 👍

@emrysal emrysal merged commit 00c00a9 into calcom:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

♻️ autoupdate tells kodiak to keep this branch up-to-date

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Required additional inputs are not actually required

4 participants