-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 no show webhook #15502
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:
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
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.
Self review done
await webhookReceiver.waitForRequestCount(1); | ||
const [request] = webhookReceiver.requestList; | ||
const body = request.body; | ||
// remove dynamic properties that differs depending on where you run the tests | ||
const dynamic = "[redacted/dynamic]"; | ||
// @ts-expect-error we are modifying the object | ||
body.createdAt = dynamic; | ||
expect(body).toMatchObject({ | ||
triggerEvent: "BOOKING_NO_SHOW_UPDATED", | ||
createdAt: "[redacted/dynamic]", | ||
payload: { | ||
message: "x_marked_as_no_show", | ||
attendees: [{ email: "first@cal.com", noShow: true, utcOffset: null }], | ||
bookingUid: bookingWhereFirstUserIsOrganizer?.uid, | ||
}, | ||
}); | ||
webhookReceiver.close(); |
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.
Decided to add the test in here instead of webhooks to avoid repeating the whole logic.
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.
Extracted this reused logic into a fixture.
apps/web/playwright/lib/fixtures.ts
Outdated
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.
New webhooks fixture
apps/web/playwright/webhook.e2e.ts
Outdated
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.
This file is mostly refactor using the new fixture.
|
||
const log = logger.getSubLogger({ prefix: ["[WebhookService] "] }); | ||
|
||
/** This is a WIP. With minimal methods until the API matures and stabilizes */ |
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.
This is a POC to simplify handling webhooks in future code.
const booking = await prisma.booking.findUnique({ | ||
where: { uid: bookingUid }, | ||
select: { | ||
eventType: { | ||
select: { | ||
id: true, | ||
teamId: true, | ||
userId: true, | ||
}, | ||
}, | ||
}, | ||
}); | ||
const orgId = await getOrgIdFromMemberOrTeamId({ | ||
memberId: booking?.eventType?.userId, | ||
teamId: booking?.eventType?.teamId, | ||
}); |
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.
These are required payloads for Webhooks service. Would like to simplify even more.
"success" | ||
); | ||
onSuccess: async (data) => { | ||
showToast(t(data.message, { x: name || email }), "success"); |
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.
We get the message from server instead
export type WebhookDataType = CalendarEvent & | ||
// BookingNoShowUpdatedPayload & // This breaks all other webhooks |
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.
To be fixed
// @ts-expect-error payload is too booking specific, we need to refactor this | ||
await webhooks.sendPayload({ ...payload, bookingUid }); |
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.
Added explainer here.
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.
@zomars E2E test "on submitting user form, triggers user webhook" is failing locally because of createReceiver (in createWebhookPageFixture).
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.
Fixed the test. Re approving the PR
* WIP Signed-off-by: zomars <zomars@me.com> * WIP Signed-off-by: zomars <zomars@me.com> * WIP Signed-off-by: zomars <zomars@me.com> * Type fixes * Update webhook.e2e.ts * Update noShow.handler.ts * Log failed results * Updates tests * Show generic error on frontend. * test: add basic webhook service test * fix: webhook end to end test * fix: type error * fix: test --------- Signed-off-by: zomars <zomars@me.com> Co-authored-by: sean-brydon <sean@cal.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com> Co-authored-by: Udit Takkar <udit222001@gmail.com>
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?