-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036
base: main
Are you sure you want to change the base?
Conversation
@hussamkhatib is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
Graphite Automations"Add community label" took an action on this PR • (05/14/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (05/14/24)1 reviewer was added to this PR based on Keith Williams's automation. |
📦 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! 🙌 |
packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts
Outdated
Show resolved
Hide resolved
The unit tests are failling |
d8cac17
to
9f45c2e
Compare
@@ -295,8 +295,6 @@ export function expectWebhookToHaveBeenCalledWith( | |||
} | |||
if (data.payload.responses !== undefined) | |||
expect(parsedBody.payload.responses).toEqual(expect.objectContaining(data.payload.responses)); | |||
const { responses: _1, metadata: _2, ...remainingPayload } = data.payload; | |||
expect(parsedBody.payload).toEqual(expect.objectContaining(remainingPayload)); |
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.
Compare the 2 snippets at the end of the Issue for why this is not needed.
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’ve added some comments. I’m also concerned that these changes will break existing workflows for users, as fields won't be accessible anymore as they used to
@@ -295,8 +295,6 @@ export function expectWebhookToHaveBeenCalledWith( | |||
} | |||
if (data.payload.responses !== undefined) | |||
expect(parsedBody.payload.responses).toEqual(expect.objectContaining(data.payload.responses)); | |||
const { responses: _1, metadata: _2, ...remainingPayload } = data.payload; | |||
expect(parsedBody.payload).toEqual(expect.objectContaining(remainingPayload)); |
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.
let's fix the test instead of deleting it, this function is also used for other tests were it passes
}; | ||
|
||
const parsedPayload = jsonParse(job.payload); | ||
|
||
const body = JSON.stringify({ |
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 think we should be able to use sendPayload
that would handle everything including headers, body, secret and the fetch request
To make sure we don't break existing workflows for users, we decided to keep all fields as they are right now but additionally add the missing fields |
Sounds good @CarinaWolli, So could I go ahead without using |
Yes we can do that for now 👍 |
14acdc8
to
a00bede
Compare
a00bede
to
4053f66
Compare
Can anyone help with the unit tests ? |
Wasn't able to make changes to your branch. This should fix the test:
|
Great Thanks. |
const body = JSON.stringify({ | ||
...(job.payload && parsedPayload), | ||
triggerEvent: parsedPayload.triggerEvent, | ||
payload: parsedPayload, |
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.
payload still has different fields than for the Booking created
webhook and as it's described in the docs
We need to make sure to already save the correct payload on creation of WebhookScheduledTriggers
. We are sending different data than for the other triggers
Hey @hussamkhatib, are you still trying to work on that? Let us know if you need any help 🙏 |
What does this PR do?
The PR now provides the proper body in the webhook payload for scheduled Triggers (MEETING_STARTED & MEETING_ENDED)
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Follow steps to setup webhooks in your applciation from the docs
Adding logs to print (
req.body.payload, req.body
)Trigger the MEETING_START or MEETING_ENDED event
You should find
req.body.payload
will beundefined
andreq.body
having the contents of payload which isn't the format mentioned in the docs, neither do other events have.What are the minimal test data to have?
Booking a event creates the necessary records in the database, for ease of testing set buffer time to book to 0 and booking interval to 1 min, so that MEETING_STARTED & MEETING_ENDED will be trigerred faster.
Checklist