Skip to content

Conversation

kart1ka
Copy link
Contributor

@kart1ka kart1ka commented Jun 4, 2025

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox - N/A
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Run yarn test packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts

Summary by cubic

Added integration tests for no-show webhooks to verify task creation, payloads, and webhook triggering logic. These tests ensure correct handling of host and guest no-show scenarios.

@kart1ka kart1ka requested a review from a team as a code owner June 4, 2025 08:03
Copy link

vercel bot commented Jun 4, 2025

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

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Jun 4, 2025
@graphite-app graphite-app bot requested a review from a team June 4, 2025 08:03
@github-actions github-actions bot added automated-tests area: unit tests, e2e tests, playwright enterprise area: enterprise, audit log, organisation, SAML, SSO Medium priority Created by Linear-GitHub Sync webhooks area: webhooks, callback, webhook payload labels Jun 4, 2025
@dosubot dosubot bot added this to the v5.4 milestone Jun 4, 2025
Copy link

graphite-app bot commented Jun 4, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (06/04/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (06/04/25)

1 label was added to this PR based on Keith Williams's automation.

Copy link
Contributor

github-actions bot commented Jun 9, 2025

E2E results are ready!

@keithwillcode keithwillcode added the community-interns The team responsible for reviewing, testing and shipping low/medium community PRs label Jun 11, 2025
Copy link
Contributor

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

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

Looks great! Left a question for you

@github-actions github-actions bot marked this pull request as draft June 13, 2025 22:48
@dosubot dosubot bot modified the milestones: v5.4, v5.5 Jun 17, 2025
Copy link
Contributor

github-actions bot commented Jul 2, 2025

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Jul 2, 2025
@kart1ka kart1ka marked this pull request as ready for review July 4, 2025 17:08
@kart1ka kart1ka requested a review from hbjORbj July 4, 2025 17:09
@dosubot dosubot bot added this to the v5.5 milestone Jul 4, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic reviewed 1 file and found no issues. Review PR in cubic.dev.

@github-actions github-actions bot removed the Stale label Jul 5, 2025
@anikdhabal anikdhabal enabled auto-merge (squash) July 9, 2025 15:28
Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

If you are going through the test requirements: -
We need to test the following scenarios:

  1. When scheduling a webhook expect the correct Tasker job to be created
  2. On another test, make sure the task payload is correct
  3. Given a Task, ensure the task handler runs properly with the correct payload
  4. Make sure a Task is deleted when the webhook is deleted

Please make sure to fix the test name and make it specific to these scenarios only.

Also, is this the correct place to add the integration test? Does this test file run when executing: yarn test -- --integrationTestsOnly?

I would suggest taking some help from Devin, if you found any difficulties. Devin is so good with test

@github-actions github-actions bot marked this pull request as draft July 11, 2025 16:54
auto-merge was automatically disabled July 11, 2025 16:54

Pull request was converted to draft

@kart1ka kart1ka marked this pull request as ready for review July 14, 2025 05:28
@kart1ka
Copy link
Contributor Author

kart1ka commented Jul 14, 2025

If you are going through the test requirements: - We need to test the following scenarios:

  1. When scheduling a webhook expect the correct Tasker job to be created
  2. On another test, make sure the task payload is correct
  3. Given a Task, ensure the task handler runs properly with the correct payload
  4. Make sure a Task is deleted when the webhook is deleted

Please make sure to fix the test name and make it specific to these scenarios only.

Also, is this the correct place to add the integration test? Does this test file run when executing: yarn test -- --integrationTestsOnly?

I would suggest taking some help from Devin, if you found any difficulties. Devin is so good with test

I have changed the names of the tests as per the request. And Yes, the test file runs when executing yarn test -- --integrationTestsOnly.

@kart1ka kart1ka requested a review from anikdhabal July 14, 2025 05:29
@kart1ka kart1ka dismissed anikdhabal’s stale review July 14, 2025 05:29

stale. pls re-review

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

cubic found 3 issues across 1 file. Review them in cubic.dev

React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.

@anikdhabal
Copy link
Contributor

@kart1ka pls address the cubic reviews. Also where is this test:- Make sure a Task is deleted when the webhook is deleted?

@kart1ka
Copy link
Contributor Author

kart1ka commented Jul 14, 2025

@kart1ka pls address the cubic reviews. Also where is this test:- Make sure a Task is deleted when the webhook is deleted?

@anikdhabal I have addressed the cubic comments. I haven’t added the test for checking if a Task is deleted when a webhook is removed, since that logic hasn’t been implemented yet. I’ve opened a separate PR to fix it. Ref - #21345. I have mentioned the same in the description as well.

@CarinaWolli CarinaWolli modified the milestones: v5.5, v5.6 Jul 16, 2025
@dosubot dosubot bot modified the milestone: v5.6 Jul 16, 2025
Copy link
Contributor

coderabbitai bot commented Aug 4, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

A new integration test suite was introduced in packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts. This suite sets up test data including a user and two webhooks, then verifies the scheduling and triggering of no-show webhook tasks for bookings. The tests assert that tasks are created in the task queue with correct payloads, check that the task handler processes the payload as expected, and confirm that the booking record is updated accordingly. External dependencies are mocked, and clean-up routines are included to remove test data after execution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Scheduling a webhook creates the correct Tasker job (#17100, CAL-4515)
Task payload is correct when scheduled (#17100, CAL-4515)
Task handler runs properly with the correct payload (#17100, CAL-4515)
Task is deleted when the webhook is deleted (#17100, CAL-4515) No test present to verify deletion of a Task when its associated webhook is deleted.

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes found.

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb36497 and 179b2c4.

📒 Files selected for processing (1)
  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts

📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: .env.example:354-357
Timestamp: 2025-07-18T08:49:18.779Z
Learning: For E2E integration tests that require real third-party service credentials (like Outlook calendar), it's acceptable to temporarily include actual test account credentials in .env.example during feature development and validation, provided there's a clear plan to replace them with placeholder values before final release. Test credentials should be for dedicated test tenants/accounts, not production systems.
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.

Applied to files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
📚 Learning: applies to **/*.ts : for prisma queries, only select data you need; never use `include`, always use ...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`

Applied to files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.

Applied to files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
📚 Learning: in the insights routing funnel component (packages/features/insights/components/routingfunnel.tsx), ...
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.

Applied to files:

  • packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Install dependencies / Yarn install & cache

Comment on lines +294 to +298
const updatedBooking = await prisma.booking.findUnique({
where: {
id: testBookingId,
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use select to fetch only required fields

According to our Prisma query guidelines, always use select to fetch only the data you need. Since you only need the noShowHost field for the assertion, specify it explicitly.

    const updatedBooking = await prisma.booking.findUnique({
      where: {
        id: testBookingId,
      },
+     select: {
+       noShowHost: true,
+     },
    });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const updatedBooking = await prisma.booking.findUnique({
where: {
id: testBookingId,
},
});
const updatedBooking = await prisma.booking.findUnique({
where: {
id: testBookingId,
},
select: {
noShowHost: true,
},
});
🤖 Prompt for AI Agents
In
packages/features/bookings/lib/handleNewBooking/scheduleNoShowTriggers.integration-test.ts
around lines 294 to 298, the Prisma query fetches the entire booking record but
only the noShowHost field is needed. Modify the findUnique call to include a
select clause that explicitly requests only the noShowHost field to optimize the
query and follow Prisma guidelines.

@emrysal emrysal enabled auto-merge (squash) August 5, 2025 00:22
Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Approved, thank you for your contribution @kart1ka - appreciated as always.

@emrysal emrysal merged commit a61148c into calcom:main Aug 5, 2025
34 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests area: unit tests, e2e tests, playwright community Created by Linear-GitHub Sync community-interns The team responsible for reviewing, testing and shipping low/medium community PRs enterprise area: enterprise, audit log, organisation, SAML, SSO Medium priority Created by Linear-GitHub Sync ready-for-e2e webhooks area: webhooks, callback, webhook payload
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-4515] Integration tests for no-show webhooks
7 participants