Skip to content

fix: make cancellation reason mandatory when cancelledBy param is set to host email#23764

Merged
anikdhabal merged 6 commits intomainfrom
make-cancellationReason-mandatory
Sep 12, 2025
Merged

fix: make cancellation reason mandatory when cancelledBy param is set to host email#23764
anikdhabal merged 6 commits intomainfrom
make-cancellationReason-mandatory

Conversation

@anikdhabal
Copy link
Contributor

fix: make cancellation reason mandatory when cancelledBy param is set to host email

@anikdhabal anikdhabal requested a review from a team as a code owner September 11, 2025 07:47
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 11, 2025

Walkthrough

Adds a derived boolean isCancellationUserHost (true when props.isHost or organizer email matches current user) in apps/web/components/booking/CancelBooking.tsx and uses it wherever host-specific behavior was previously gated by props.isHost (cancellation reason requirement, label selection, and host info banner). Improves API error handling during cancel to parse a JSON response body and prefer data.message on non-2xx responses. In packages/features/bookings/lib/handleCancelBooking.ts the host-detection is similarly expanded to consider bookingToDelete.userId == userId or bookingToDelete.user.email == cancelledBy, and tests were updated to require a cancellationReason when cancellation is initiated by the host (including new tests asserting rejection when missing). No exported/public signatures changed; core cancellation flow and UI enablement remain tied to the host-missing-reason and no-show acknowledgment flags.

Possibly related PRs

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely and accurately summarizes the primary change: requiring a cancellation reason when the cancelledBy parameter is the host's email, and it aligns with the modified logic in CancelBooking.tsx and handleCancelBooking.ts as described in the PR summary.
Description Check ✅ Passed The PR description directly matches the changeset and restates the title, clearly communicating the intent to make a cancellation reason mandatory when cancelledBy is set to the host email.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch make-cancellationReason-mandatory

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot requested a review from a team September 11, 2025 07:47
@keithwillcode keithwillcode added the core area: core, team members only label Sep 11, 2025
@dosubot dosubot bot added bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working labels Sep 11, 2025
Copy link
Member

@alishaz-polymath alishaz-polymath left a comment

Choose a reason for hiding this comment

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

NIT

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: 2

🧹 Nitpick comments (2)
packages/features/bookings/lib/handleCancelBooking.ts (1)

123-128: Verify platform bypass is intended; consider server-side parity with internal-note requirement.

  • Skipping reason when platformClientId exists may contradict “mandatory when host cancels.” Confirm product intent.
  • UI also enforces selecting an internal note preset for hosts; server currently doesn’t. Consider validating internalNote server-side when presets are configured to keep parity.
apps/web/components/booking/CancelBooking.tsx (1)

163-166: Use the unified host flag everywhere (no‑show fee gating, labels, presets) for consistency.

Currently, organizer-by-email is treated as host for reason requirement but not for:

  • No-show fee exemption
  • “Host” label/message
  • Internal note presets visibility

Outside this hunk, update:

  • In autoChargeNoShowFee(): if (isCancellationUserIsHost) return false;
  • Presets visibility (Line 193): isCancellationUserIsHost && props.internalNotePresets.length > 0
  • Host label (Line 217): isCancellationUserIsHost ? t("cancellation_reason_host") : t("cancellation_reason")
  • Info note (Line 229): condition on isCancellationUserIsHost
  • No-show acknowledgment (Line 167): !isCancellationUserIsHost && cancellationNoShowFeeWarning && !acknowledgeCancellationNoShowFee

Example (illustrative, not a diff):

const isCancellationUserIsHost = /* moved above */;

const cancellationNoShowFeeNotAcknowledged =
  !isCancellationUserIsHost && cancellationNoShowFeeWarning && !acknowledgeCancellationNoShowFee;

// ...
{isCancellationUserIsHost && props.internalNotePresets.length > 0 && (/* presets */)}
<Label>{isCancellationUserIsHost ? t("cancellation_reason_host") : t("cancellation_reason")}</Label>
{isCancellationUserIsHost ? (<div>/* info */</div>) : null}

function autoChargeNoShowFee() {
  if (isCancellationUserIsHost) return false;
  // ...
}
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0a7e573 and e543823.

📒 Files selected for processing (2)
  • apps/web/components/booking/CancelBooking.tsx (1 hunks)
  • packages/features/bookings/lib/handleCancelBooking.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Always use t() for text localization in frontend code; direct text embedding should trigger a warning

Files:

  • apps/web/components/booking/CancelBooking.tsx
**/*.{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:

  • apps/web/components/booking/CancelBooking.tsx
  • packages/features/bookings/lib/handleCancelBooking.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • apps/web/components/booking/CancelBooking.tsx
  • packages/features/bookings/lib/handleCancelBooking.ts
**/*.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/handleCancelBooking.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Install dependencies / Yarn install & cache

@github-actions github-actions bot marked this pull request as draft September 11, 2025 07:57
@hariombalhara
Copy link
Member

hariombalhara commented Sep 11, 2025

image In text it says it is optional but has become required, so it could be misleading and people might complain

this is how we show when user is logged in as organizer, I believe we should show the same.
image

Copy link
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Left a comment

@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 11, 2025
@vercel
Copy link

vercel bot commented Sep 11, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Sep 11, 2025 3:49pm
cal-eu Ignored Ignored Sep 11, 2025 3:49pm

@hariombalhara
Copy link
Member

hariombalhara commented Sep 11, 2025

One more thing:
I know this is an existing issue but we should show the real useful message here instead of this 400 msg. I intentionally creatd a scenario to replicate this message.
image

@anikdhabal anikdhabal marked this pull request as ready for review September 11, 2025 08:42
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: 2

♻️ Duplicate comments (1)
apps/web/components/booking/CancelBooking.tsx (1)

160-162: Normalize emails when detecting host to avoid false negatives

Compare trimmed, lowercased emails.

-  const isCancellationUserHost =
-    props.isHost || bookingCancelledEventProps.organizer.email === currentUserEmail;
+  const normalizedOrganizer = bookingCancelledEventProps.organizer?.email?.trim().toLowerCase();
+  const normalizedCurrent = currentUserEmail?.trim().toLowerCase();
+  const isCancellationUserHost = props.isHost || normalizedOrganizer === normalizedCurrent;
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts (1)

515-582: Make assertion resilient to punctuation; prevent brittle failure

Error text often varies by a trailing period. Prefer a regex match.

-    ).rejects.toThrow("Cancellation reason is required when you are the host");
+    ).rejects.toThrow(/Cancellation reason is required when you are the host\.?$/);

Also, consider standardizing spelling in test titles to either “canceling” or “cancelling” for consistency.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e543823 and f514b2f.

📒 Files selected for processing (3)
  • apps/web/components/booking/CancelBooking.tsx (4 hunks)
  • packages/features/bookings/lib/handleCancelBooking.ts (1 hunks)
  • packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/features/bookings/lib/handleCancelBooking.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/handleCancelBooking/test/handleCancelBooking.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/handleCancelBooking/test/handleCancelBooking.test.ts
  • apps/web/components/booking/CancelBooking.tsx
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts
  • apps/web/components/booking/CancelBooking.tsx
**/*.tsx

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Always use t() for text localization in frontend code; direct text embedding should trigger a warning

Files:

  • apps/web/components/booking/CancelBooking.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.

Applied to files:

  • packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts (2)
packages/platform/libraries/index.ts (1)
  • handleCancelBooking (71-71)
apps/web/test/utils/bookingScenario/bookingScenario.ts (7)
  • getBooker (2220-2234)
  • getOrganizer (1520-1576)
  • TestData (1239-1511)
  • getGoogleCalendarCredential (1192-1200)
  • getDate (1093-1140)
  • createBookingScenario (978-1009)
  • getScenarioData (1578-1664)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Tests / Unit
  • GitHub Check: Linters / lint
  • GitHub Check: Type check / check-types
🔇 Additional comments (6)
packages/features/bookings/lib/handleCancelBooking/test/handleCancelBooking.test.ts (4)

135-136: Good: host payloads now include cancellationReason

Aligns tests with new requirement for host-initiated cancellations.


265-266: Good: refund path updated to include cancellationReason

Keeps host-cancel flow consistent across scenarios.


689-689: Good: fee-exempt organizer path includes cancellationReason

Matches the new contract.


821-821: Good: team-admin path includes cancellationReason

Consistent with host requirement.

apps/web/components/booking/CancelBooking.tsx (2)

217-217: LGTM: host-specific label

Correctly localizes label by host context.


228-235: LGTM: host-only info banner gating

Now matches the broadened host detection.

Copy link
Member

@alishaz-polymath alishaz-polymath left a comment

Choose a reason for hiding this comment

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

LGTM

@anikdhabal anikdhabal dismissed hariombalhara’s stale review September 11, 2025 13:35

comment addressed, pls check

@anikdhabal anikdhabal enabled auto-merge (squash) September 11, 2025 15:18
@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2025

E2E results are ready!

@anikdhabal anikdhabal disabled auto-merge September 11, 2025 15:48
@anikdhabal anikdhabal enabled auto-merge (squash) September 11, 2025 15:48
@anikdhabal anikdhabal merged commit a62c74f into main Sep 12, 2025
40 checks passed
@anikdhabal anikdhabal deleted the make-cancellationReason-mandatory branch September 12, 2025 17:28
@coderabbitai coderabbitai bot mentioned this pull request Oct 11, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working core area: core, team members only ready-for-e2e size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments