-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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: booking with phone number #14461
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
Current Playwright Test Results Summary✅ 318 Passing - ❌ 3 Failing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 05/21/2024 03:16:11pm UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: eb72e57 Started: 05/21/2024 03:12:32pm UTC ❌ Failures📄 apps/web/playwright/profile.e2e.ts • 1 FailureTest Case Results
📄 apps/web/playwright/managedBooking/advancedOptions.e2e.ts • 1 FailureTest Case Results
📄 apps/web/playwright/manage-booking-questions.e2e.ts • 1 FailureTest Case Results
|
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Stripe integration When event is paid and confirmed Payment should confirm pending payment booking
Retry 1 • Initial Attempt |
4.55% (10)10 / 220 runsfailed over last 7 days |
5.45% (12)12 / 220 runsflaked over last 7 days |
📄 apps/web/playwright/signup.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Signup Flow Test Email verification sent if enabled
Retry 1 • Initial Attempt |
0.81% (2)2 / 247 runsfailed over last 7 days |
28.74% (71)71 / 247 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 9 Flakes
Top 1 Common Error Messages
|
9 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Popup Tests should open embed iframe on click - Configured with light theme
Retry 1 • Initial Attempt |
2.55% (6)6 / 235 runsfailed over last 7 days |
60.43% (142)142 / 235 runsflaked over last 7 days |
Popup Tests should be able to reschedule
Retry 1 • Initial Attempt |
-163.22% (-142)-142 / 87 runsfailed over last 7 days |
163.22% (142)142 / 87 runsflaked over last 7 days |
Popup Tests should open Routing Forms embed on click
Retry 1 • Initial Attempt |
-163.22% (-142)-142 / 87 runsfailed over last 7 days |
163.22% (142)142 / 87 runsflaked over last 7 days |
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe according to system theme when no theme is configured through Embed API
Retry 1 • Initial Attempt |
-159.77% (-139)-139 / 87 runsfailed over last 7 days |
160.92% (140)140 / 87 runsflaked over last 7 days |
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe according to system theme when configured with 'auto' theme using Embed API
Retry 1 • Initial Attempt |
-161.63% (-139)-139 / 86 runsfailed over last 7 days |
161.63% (139)139 / 86 runsflaked over last 7 days |
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe(Booker Profile Page) with dark theme when configured with dark theme using Embed API
Retry 1 • Initial Attempt |
-161.63% (-139)-139 / 86 runsfailed over last 7 days |
161.63% (139)139 / 86 runsflaked over last 7 days |
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe(Event Booking Page) with dark theme when configured with dark theme using Embed API
Retry 1 • Initial Attempt |
-161.63% (-139)-139 / 86 runsfailed over last 7 days |
161.63% (139)139 / 86 runsflaked over last 7 days |
Popup Tests prendered embed should be loaded and apply the config given to it
Retry 1 • Initial Attempt |
-161.63% (-139)-139 / 86 runsfailed over last 7 days |
161.63% (139)139 / 86 runsflaked over last 7 days |
Popup Tests should open on clicking child element
Retry 1 • Initial Attempt |
-159.30% (-137)-137 / 86 runsfailed over last 7 days |
159.30% (137)137 / 86 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/namespacing.e2e.ts • 4 Flakes
Top 1 Common Error Messages
|
4 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Namespacing Inline Embed Double install Embed Snippet with inline embed using a namespace
Retry 1 • Initial Attempt |
0.43% (1)1 / 234 runfailed over last 7 days |
61.54% (144)144 / 234 runsflaked over last 7 days |
Namespacing Inline Embed Add inline embed using a namespace without reload
Retry 1 • Initial Attempt |
0% (0)0 / 234 runsfailed over last 7 days |
63.68% (149)149 / 234 runsflaked over last 7 days |
Namespacing Inline Embed Double install Embed Snippet with inline embed without a namespace(i.e. default namespace)
Retry 1 • Initial Attempt |
0% (0)0 / 234 runsfailed over last 7 days |
64.53% (151)151 / 234 runsflaked over last 7 days |
Namespacing Different namespaces can have different init configs
Retry 1 • Initial Attempt |
0% (0)0 / 233 runsfailed over last 7 days |
61.37% (143)143 / 233 runsflaked over last 7 days |
📄 apps/web/playwright/login.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
user can login & logout succesfully -- future login flow user & logout using dashboard
Retry 1 • Initial Attempt |
4.18% (10)10 / 239 runsfailed over last 7 days |
35.56% (85)85 / 239 runsflaked over last 7 days |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
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.
Overall nice work so far @Udit-takkar. I only have 2 asks to increase the confidence for merging.
- Since this is a significant change of the booking process. Can we at least add a couple of tests cases for our E2E booking flow tests?
- Since email is not required anymore I feel like this is introducing a lot of nesting and overall it's becoming harder to track these fields. What's the rationale behind the DB design? What other alternatives were considered and why was this the winner?
@@ -64,7 +64,7 @@ export function getMockPaymentService() { | |||
paymentData: Payment | |||
): Promise<void> { | |||
// TODO: App implementing PaymentService is supposed to send email by itself at the moment. | |||
await sendAwaitingPaymentEmail({ | |||
await sendAwaitingPaymentEmailAndSMS({ |
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.
Instead of keep adding to the name. Let's rename it to something more generic that infers handling more than one type of notification.
await sendAwaitingPaymentEmailAndSMS({ | |
await sendAwaitingPaymentNotification({ |
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.
I have to create a single notification manager class in follow up PR #15415 (comment)
@@ -62,11 +63,11 @@ export default class ExchangeCalendarService implements Calendar { | |||
appointment.Location = event.location || ""; | |||
appointment.Body = new MessageBody(event.description || ""); | |||
event.attendees.forEach((attendee: Person) => { | |||
appointment.RequiredAttendees.Add(new Attendee(attendee.email)); | |||
appointment.RequiredAttendees.Add(new Attendee(attendee.email ?? BOOKED_WITH_SMS_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.
Why is the BOOKED_WITH_SMS_EMAIL necessary?
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.
email is required field here to create an event and BOOKED_WITH_SMS would also help us know that it was booked with phone number.
...ages/features/bookings/lib/handleNewBooking/test/team-bookings/collective-scheduling.test.ts
Outdated
Show resolved
Hide resolved
Yeah sure. I already added few unit tests. is there any specific scenario that i should add?
The changes in DB schema are:- I made email an optional field now and added a new field phoneNumber in Attendee model with a constraint that make sure atleast one of the two exist
BOOKED_WITH_SMS is used as a placeholder email address when it's not present as a lot of our codebase depend upon bookerEmail and this is a very big change. Few things like creating a calendar event would also require email of attendee. Eventually I have to completely remove BOOKED_WITH_SMS after some time on production. This feature is only limited to org team event types for now and we need it fast for cal ai feature which some customers are waiting for. |
Booking with only phone required. Booking with both phone and email required. |
Done. |
What does this PR do?
Fixes #14534
Follow up: #15415
How to create event type with only phone number?
Turn the toggle on for attendee phone number
Added 4 Unit Tests
Added 2 E2E tests
Type of change