Skip to content
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: booking_paid webhook and added new payment metadata #11093

Merged

Conversation

alannnc
Copy link
Contributor

@alannnc alannnc commented Sep 2, 2023

What does this PR do?

  • Implements webhook booking_paid
  • Looks like it was implement a while a go but wasn't working right.
  • Add payment data to webhook metadata
  • Fixes a bug when saving app.metadata with 2 payment app credentials installed

Fixes #10679

https://www.loom.com/share/c0a54a80063841d5947917934c8eee6c?sid=bf913ef1-1219-4c2d-aee1-f40f9ebd43b4

Requirement/Documentation

  • If there is a requirement document, please, share it here.
  • If there is ab UI/UX design document, please, share it here.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  • Are there environment variables that should be set? No
  • What are the minimal test data to have? Stripe installed and and event that uses it.
  • What is expected (happy path) to have (input and output)? Pay the booking and we should receive webhook for BOOKING_PAID
  • Any other important info that could help to test that PR

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@alannnc alannnc linked an issue Sep 2, 2023 that may be closed by this pull request
@linear
Copy link

linear bot commented Sep 2, 2023

CAL-2328 Add more data to booking_paid webhook

We can add { stripePaymentId, stripeCustomerId, providerId: "cal.com", userId, eventName, eventId }

@vercel
Copy link

vercel bot commented Sep 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ai ❌ Failed (Inspect) Sep 5, 2023 5:54am
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2023 5:54am
cal-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2023 5:54am
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2023 5:54am
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2023 5:54am
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2023 5:54am
qa ⬜️ Ignored (Inspect) Visit Preview Sep 5, 2023 5:54am

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

Thank you for following the naming conventions! 🙏

@github-actions github-actions bot added billing area: billing, stripe, payments, paypal, get paid Low priority Created by Linear-GitHub Sync webhooks area: webhooks, callback, webhook payload labels Sep 2, 2023
@zomars zomars added the core area: core, team members only label Sep 2, 2023
await Promise.all(promises);

if (paid) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a whole section when receiving paid flag here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about refactoring it out to a handlePaidEventConfirmation function

triggerEvent: WebhookTriggerEvents.BOOKING_PAID,
teamId: booking.eventType?.teamId,
});
const bookingWithPayment = await prisma.booking.findFirst({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gather all required information

bookerEmail: evt.attendees[0].email,
eventTitle: booking.eventType?.title,
externalId: paymentExternalId,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare new metadata for payment

);

// I don't need to await for this
Promise.all(bookingPaidSubscribers);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't await to complete this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do a Promise.all I guess, if we aren't waiting for the results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do a Promise.all I guess, if we aren't waiting for the results.

It's ok, we want them to start in parallel instead of cascading even if we don't wait for the response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But calls would go in parallel already due to line 380. We don't need the result of any of those promises. So we don't need line number 400 at all.

@alannnc alannnc marked this pull request as ready for review September 2, 2023 04:01
@alannnc alannnc requested review from a team and CarinaWolli September 2, 2023 04:01
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

📦 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! 🙌

@deploysentinel
Copy link

deploysentinel bot commented Sep 2, 2023

Current Playwright Test Results Summary

✅ 117 Passing - ⚠️ 3 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 09/05/2023 05:56:01am UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 0dc1fe8

Started: 09/05/2023 05:54:19am UTC

⚠️ Flakes

📄   apps/web/playwright/integrations-stripe.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Stripe integration Can book a paid booking
Retry 1Initial Attempt
6.45% (18) 18 / 279 runs
failed over last 7 days
9.32% (26) 26 / 279 runs
flaked over last 7 days

📄   apps/web/playwright/webhook.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
FORM_SUBMITTED on submitting user form, triggers user webhook
Retry 1Initial Attempt
0% (0) 0 / 3 runs
failed over last 7 days
33.33% (1) 1 / 3 run
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Popup Tests should be able to reschedule
Retry 1Initial Attempt
6.03% (17) 17 / 282 runs
failed over last 7 days
92.91% (262) 262 / 282 runs
flaked over last 7 days

View Detailed Build Results


@hariombalhara
Copy link
Member

I will be soon opening a PR to handle firing BOOKING_CREATED webhook and others for paid events as per #7120. I was going to add BOOKING_PAYMENT_SUCCESSFUL as well event which you already handled here as BOOKING_PAID

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to fix the issue. I feel this can be improved upon. But good enough for now. Nice job @alannnc

Ship it

@zomars zomars merged commit b81b221 into main Sep 6, 2023
@zomars zomars deleted the fix-add-more-data-to-booking-paid-webhook-10679-cal-2328 branch September 6, 2023 19:40
sean-brydon added a commit that referenced this pull request Sep 7, 2023
* Pass organization name & logo

* Overflow hidden

* Show org icon on public page

* Add org logo to large user avatars

* Clean up

* Add org name and logo to context

* Get org logo from /avatar.png endpoint

* Do not query for logo

* Remove name and logo from session middleware

* Type fix

* Set user onboarding org logo

* feat: organization avatar component (#10788)

Co-authored-by: sean-brydon <[email protected]>

* Type fixes

* Type fix

* Transition to org slug for organization avatar

* Address feedback

* Clean up

* Clean up

* Type fix

* fix: set avatar cache control (#11163)

* test: Integration tests for handleNewBooking (#11044)

Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: Joe Au-Yeung <[email protected]>

* fix: booking_paid webhook and added new payment metadata (#11093)

* app store improvements, logos, dark mode, added screenshots, fixed author names (#11164)

* fix: mobile event types and avatars (#11184)

* New Crowdin translations by Github Action

* fix: updateProfile metadata overwrite (#11188)

Co-authored-by: alannnc <[email protected]>

* New Crowdin translations by Github Action

---------

Co-authored-by: Sean Brydon <[email protected]>
Co-authored-by: sean-brydon <[email protected]>
Co-authored-by: Omar López <[email protected]>
Co-authored-by: Hariom Balhara <[email protected]>
Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: Leo Giovanetti <[email protected]>
Co-authored-by: Crowdin Bot <[email protected]>
ashwintelmore pushed a commit to ashwintelmore/cal.com that referenced this pull request Sep 19, 2023
* Pass organization name & logo

* Overflow hidden

* Show org icon on public page

* Add org logo to large user avatars

* Clean up

* Add org name and logo to context

* Get org logo from /avatar.png endpoint

* Do not query for logo

* Remove name and logo from session middleware

* Type fix

* Set user onboarding org logo

* feat: organization avatar component (calcom#10788)

Co-authored-by: sean-brydon <[email protected]>

* Type fixes

* Type fix

* Transition to org slug for organization avatar

* Address feedback

* Clean up

* Clean up

* Type fix

* fix: set avatar cache control (calcom#11163)

* test: Integration tests for handleNewBooking (calcom#11044)

Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: Joe Au-Yeung <[email protected]>

* fix: booking_paid webhook and added new payment metadata (calcom#11093)

* app store improvements, logos, dark mode, added screenshots, fixed author names (calcom#11164)

* fix: mobile event types and avatars (calcom#11184)

* New Crowdin translations by Github Action

* fix: updateProfile metadata overwrite (calcom#11188)

Co-authored-by: alannnc <[email protected]>

* New Crowdin translations by Github Action

---------

Co-authored-by: Sean Brydon <[email protected]>
Co-authored-by: sean-brydon <[email protected]>
Co-authored-by: Omar López <[email protected]>
Co-authored-by: Hariom Balhara <[email protected]>
Co-authored-by: Shivam Kalra <[email protected]>
Co-authored-by: alannnc <[email protected]>
Co-authored-by: Peer Richelsen <[email protected]>
Co-authored-by: Leo Giovanetti <[email protected]>
Co-authored-by: Crowdin Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
billing area: billing, stripe, payments, paypal, get paid core area: core, team members only Low priority Created by Linear-GitHub Sync webhooks area: webhooks, callback, webhook payload
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-2328] Add more data to booking_paid webhook
3 participants