-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
feat: support phone based booking api #17635
Conversation
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (11/14/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (01/08/25)1 reviewer was added to this PR based on Keith Williams's automation. |
packages/platform/types/bookings/2024-08-13/inputs/create-booking.input.ts
Outdated
Show resolved
Hide resolved
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 PR looks good but the problem I see is that right now we can make a booking using attendee phone number for event types that don't have attendee phone number booking field. Just like we have in this PR RequireEmailOrPhone
we should have the same logic in event type.
The 4.5 release blog states
To enable this (phone based bookings), go to your event type and enable 'Phone' under booking questions, mark it as required, and you can then turn off 'Email'.
So i think we should follow same logic and at least allow enabling phone based event types (event type without email but with phone) and allow an event type to have either phone or email booking field in files I described in the thread.
However, to not block users who do have event types with phone I guess we could merge this after tests are added to unblock those users, but a follow up for the event types would be needed. Do you think you would have time for that?
packages/platform/types/bookings/2024-08-13/inputs/create-booking.input.ts
Outdated
Show resolved
Hide resolved
packages/platform/types/bookings/2024-08-13/inputs/create-booking.input.ts
Outdated
Show resolved
Hide resolved
packages/platform/types/bookings/2024-08-13/inputs/create-booking.input.ts
Show resolved
Hide resolved
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
This PR is being marked as stale due to inactivity. |
This PR is being marked as stale due to inactivity. |
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/team-bookings.e2e-spec.ts
Show resolved
Hide resolved
apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/team-bookings.e2e-spec.ts
Outdated
Show resolved
Hide resolved
const body: UpdateTeamEventTypeInput_2024_06_14 = { | ||
bookingFields: [ | ||
{ | ||
type: "email", |
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.
Like before - shouldn't we also omit email booking field here?
packages/platform/types/bookings/2024-08-13/inputs/create-booking.input.ts
Outdated
Show resolved
Hide resolved
@IsBoolean() | ||
@IsOptional() | ||
@DocsProperty() | ||
required = true; |
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.
Why does it have to be required if we can have phone based bookings aka without email?
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.
In the request payload of the PR description there is only attendee phone number.
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 was no required field defined for email i just set the default value true if it hasn't been passed as false or empty
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ |
E2E results are ready! |
* chore: save progress * feat: support phone based booking using API v2 * chore: change name of the variable * fix: update event types and booking fields for improved validation and new properties - Updated imports in input and output event types services to use the correct library version. - Added new `avatarUrl` and `successRedirectUrl` properties to the Swagger documentation for user and booking schemas. - Enhanced booking fields validation by changing the `required` property to a boolean type. - Improved descriptions and examples in the OpenAPI specifications for better clarity. - Refactored booking fields input and output classes to include new properties and ensure proper validation. * fix: booking with phone number * test: add event type update test * test: add test for creating booking * chore: update unit test * fix: tests * fix: add turbo * chore: remove emial * refactor: make email optional and move it to service * fix: test * chore: bump platform-libraries * fix: e2e test --------- Co-authored-by: supalarry <laurisskraucis@gmail.com>
* chore: save progress * feat: support phone based booking using API v2 * chore: change name of the variable * fix: update event types and booking fields for improved validation and new properties - Updated imports in input and output event types services to use the correct library version. - Added new `avatarUrl` and `successRedirectUrl` properties to the Swagger documentation for user and booking schemas. - Enhanced booking fields validation by changing the `required` property to a boolean type. - Improved descriptions and examples in the OpenAPI specifications for better clarity. - Refactored booking fields input and output classes to include new properties and ensure proper validation. * fix: booking with phone number * test: add event type update test * test: add test for creating booking * chore: update unit test * fix: tests * fix: add turbo * chore: remove emial * refactor: make email optional and move it to service * fix: test * chore: bump platform-libraries * fix: e2e test --------- Co-authored-by: supalarry <laurisskraucis@gmail.com>
* chore: save progress * feat: support phone based booking using API v2 * chore: change name of the variable * fix: update event types and booking fields for improved validation and new properties - Updated imports in input and output event types services to use the correct library version. - Added new `avatarUrl` and `successRedirectUrl` properties to the Swagger documentation for user and booking schemas. - Enhanced booking fields validation by changing the `required` property to a boolean type. - Improved descriptions and examples in the OpenAPI specifications for better clarity. - Refactored booking fields input and output classes to include new properties and ensure proper validation. * fix: booking with phone number * test: add event type update test * test: add test for creating booking * chore: update unit test * fix: tests * fix: add turbo * chore: remove emial * refactor: make email optional and move it to service * fix: test * chore: bump platform-libraries * fix: e2e test --------- Co-authored-by: supalarry <laurisskraucis@gmail.com>
What does this PR do?
https://app.campsite.com/cal/posts/muplmwz5gjpg
request payload:-
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?