fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036

hussamkhatib · 2024-05-14T21:49:36Z

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)

I have self-reviewed the code (A decent size PR without self-review might be rejected)
I have added a Docs issue here if this PR makes changes that would require a documentation change
I have added or modified automated tests that prove my fix is effective or that my feature works (PRs might be rejected if logical changes are not properly tested)

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 be undefined and req.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

I haven't checked if my changes generate no new warnings

vercel · 2024-05-14T21:49:41Z

@hussamkhatib is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

github-actions · 2024-05-14T21:49:48Z

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

graphite-app · 2024-05-14T21:50:11Z

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.

github-actions · 2024-05-14T21:57:53Z

📦 Next.js Bundle Analysis for @calcom/web

This 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

Udit-takkar · 2024-05-15T07:06:15Z

The unit tests are failling

hussamkhatib · 2024-05-15T13:01:00Z

apps/web/test/utils/bookingScenario/expects.ts

@@ -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));


Compare the 2 snippets at the end of the Issue for why this is not needed.

CarinaWolli

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

CarinaWolli · 2024-05-15T14:21:51Z

apps/web/test/utils/bookingScenario/expects.ts

@@ -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));


let's fix the test instead of deleting it, this function is also used for other tests were it passes

CarinaWolli · 2024-05-15T14:22:51Z

packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts

    };

+    const parsedPayload = jsonParse(job.payload);
+
+    const body = JSON.stringify({


I think we should be able to use sendPayload that would handle everything including headers, body, secret and the fetch request

CarinaWolli · 2024-05-16T20:09:40Z

I’m also concerned that these changes will break existing workflows for users, as fields won't be accessible anymore as they used to

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 payload and createdAt

hussamkhatib · 2024-05-16T20:26:45Z

I’m also concerned that these changes will break existing workflows for users, as fields won't be accessible anymore as they used to

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 payload and createdAt

Sounds good @CarinaWolli, So could I go ahead without using sendPayload as using that won't allow to handle existing workflow for users

CarinaWolli · 2024-05-17T13:20:14Z

Sounds good @CarinaWolli, So could I go ahead without using sendPayload as using that won't allow to handle existing workflow for users

Yes we can do that for now 👍

hussamkhatib · 2024-05-21T12:17:14Z

Can anyone help with the unit tests ?

CarinaWolli · 2024-05-21T14:35:25Z

Can anyone help with the unit tests ?

Wasn't able to make changes to your branch. This should fix the test:

test(`should trigger if current date is after startAfter`, async () => {
  const now = dayjs();
  const payload = { triggerEvent: "MEETING_ENDED" };
  await prismock.webhookScheduledTriggers.createMany({
    data: [
      {
        id: 1,
        subscriberUrl: "https://example.com",
        startAfter: now.add(5, "minute").toDate(),
        payload: JSON.stringify(payload),
      },
      {
        id: 2,
        subscriberUrl: "https://example.com/test",
        startAfter: now.subtract(5, "minute").toDate(),
        payload: JSON.stringify(payload),
      },
    ],
  });
  await handleWebhookScheduledTriggers(prismock);

  expectWebhookToHaveBeenCalledWith("https://example.com/test", { triggerEvent: "MEETING_ENDED", payload });
  expect(() =>
    expectWebhookToHaveBeenCalledWith("https://example.com", { triggerEvent: "MEETING_ENDED", payload })
  ).toThrow("Webhook not sent to https://example.com for MEETING_ENDED. All webhooks: []");
});

hussamkhatib · 2024-05-21T17:03:24Z

Great Thanks.

CarinaWolli · 2024-05-22T13:56:04Z

packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts

+    const body = JSON.stringify({
+      ...(job.payload && parsedPayload),
+      triggerEvent: parsedPayload.triggerEvent,
+      payload: parsedPayload,


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

CarinaWolli · 2024-05-31T13:42:41Z

Hey @hussamkhatib, are you still trying to work on that? Let us know if you need any help 🙏

graphite-app bot added the community Created by Linear-GitHub Sync label May 14, 2024

graphite-app bot requested a review from a team May 14, 2024 21:49

github-actions bot added docs area: docs, documentation, cal.com/docs webhooks area: webhooks, callback, webhook payload labels May 14, 2024

dosubot bot added the 🐛 bug Something isn't working label May 14, 2024

hussamkhatib changed the title ~~fix body payload of MEETING_STARTED & MEETING_ENDED webhook~~ fix: body payload of MEETING_STARTED & MEETING_ENDED webhook May 14, 2024

Udit-takkar reviewed May 15, 2024

View reviewed changes

packages/features/webhooks/lib/handleWebhookScheduledTriggers.ts Outdated Show resolved Hide resolved

Udit-takkar added this to the v4.1 milestone May 15, 2024

Udit-takkar requested a review from CarinaWolli May 15, 2024 07:03

hussamkhatib force-pushed the fix-scheduled-webhooks branch from d8cac17 to 9f45c2e Compare May 15, 2024 11:37

hussamkhatib commented May 15, 2024

View reviewed changes

hussamkhatib requested a review from Udit-takkar May 15, 2024 13:01

CarinaWolli requested changes May 15, 2024

View reviewed changes

keithwillcode modified the milestones: v4.1, v4.2 May 15, 2024

dosubot bot modified the milestone: v4.2 May 15, 2024

CarinaWolli mentioned this pull request May 16, 2024

[CAL-3752] Unreliable Triggering and Payload Inconsistencies in Calcom Webhooks #15078

Open

hussamkhatib force-pushed the fix-scheduled-webhooks branch from 14acdc8 to a00bede Compare May 17, 2024 19:30

github-actions bot added High priority Created by Linear-GitHub Sync Urgent Created by Linear-GitHub Sync labels May 17, 2024

fix body payload of MEETING_STARTED & MEETING_ENDED webhook

b097a49

revert back headers

4053f66

hussamkhatib force-pushed the fix-scheduled-webhooks branch from a00bede to 4053f66 Compare May 21, 2024 11:24

fix unit tests

4cf2b98

hussamkhatib requested a review from CarinaWolli May 21, 2024 17:03

CarinaWolli requested changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036

fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036

hussamkhatib commented May 14, 2024 •

edited by CarinaWolli

vercel bot commented May 14, 2024

github-actions bot commented May 14, 2024 •

edited

graphite-app bot commented May 14, 2024

github-actions bot commented May 14, 2024 •

edited

Udit-takkar commented May 15, 2024

hussamkhatib May 15, 2024

CarinaWolli left a comment

CarinaWolli May 15, 2024

CarinaWolli May 15, 2024

CarinaWolli commented May 16, 2024

hussamkhatib commented May 16, 2024

CarinaWolli commented May 17, 2024

hussamkhatib commented May 21, 2024

CarinaWolli commented May 21, 2024

hussamkhatib commented May 21, 2024 •

edited

CarinaWolli May 22, 2024

CarinaWolli commented May 31, 2024

fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036

Are you sure you want to change the base?

fix: body payload of MEETING_STARTED & MEETING_ENDED webhook #15036

Conversation

hussamkhatib commented May 14, 2024 • edited by CarinaWolli

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

How should this be tested?

Checklist

vercel bot commented May 14, 2024

github-actions bot commented May 14, 2024 • edited

graphite-app bot commented May 14, 2024

Graphite Automations

github-actions bot commented May 14, 2024 • edited

📦 Next.js Bundle Analysis for @calcom/web

Udit-takkar commented May 15, 2024

hussamkhatib May 15, 2024

Choose a reason for hiding this comment

CarinaWolli left a comment

Choose a reason for hiding this comment

CarinaWolli May 15, 2024

Choose a reason for hiding this comment

CarinaWolli May 15, 2024

Choose a reason for hiding this comment

CarinaWolli commented May 16, 2024

hussamkhatib commented May 16, 2024

CarinaWolli commented May 17, 2024

hussamkhatib commented May 21, 2024

CarinaWolli commented May 21, 2024

hussamkhatib commented May 21, 2024 • edited

CarinaWolli May 22, 2024

Choose a reason for hiding this comment

CarinaWolli commented May 31, 2024

hussamkhatib commented May 14, 2024 •

edited by CarinaWolli

github-actions bot commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

hussamkhatib commented May 21, 2024 •

edited