-
Notifications
You must be signed in to change notification settings - Fork 10.2k
fix: Hosts not able to cancel/reschedule the event when Disable Rescheduling/Cancelling is enabled #22281
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
base: main
Are you sure you want to change the base?
fix: Hosts not able to cancel/reschedule the event when Disable Rescheduling/Cancelling is enabled #22281
Conversation
…eduling/Cancelling is enabled
@asadath1395 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/07/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/07/25)1 label was added to this PR based on Keith Williams's automation. |
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.
cubic found 2 issues across 6 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
const [user] = users.get(); | ||
await user.apiLogin(); | ||
|
||
await page.goto(`/booking/${bookingId}`); |
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.
Rule violated: E2E Tests Best Practices
Missing expect(page).toHaveURL() after navigation. According to the E2E Best Practices guideline, tests must assert the final URL immediately after page.goto to fail fast on unexpected redirects.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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.
Along with the host, Team/Org Admin and Owner should also be able to reschedule or cancel the booking.
Hi @asadath1395, Left a comment. There is a type check failing, pls address that as well. |
…able-rescheduling-cancelling
…e disabled in event type
…com/asadath1395/cal.com into fix/disable-rescheduling-cancelling
…able-rescheduling-cancelling
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.
cubic found 2 issues across 9 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
} | ||
} | ||
|
||
const isHostOrOwner = !!userIsHost || !!userIsOwnerOfEventType || !!hasTeamOrOrgPermissions; |
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.
The permission-calculation logic (userIsHost / userIsOwnerOfEventType / hasTeamOrOrgPermissions) is duplicated later in the same function, causing two additional database calls (isTeamAdmin
, isOrganisationAdmin
) per request. This hurts performance and maintainability; consider extracting a helper or computing once and re-using the result.
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.
Removed
} | ||
} | ||
} | ||
const isHostOrOwner = |
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.
Authorization relies on eventData.hosts which is limited to the first 3 hosts (because of take: 3
in the Prisma query). If an event has more than three hosts, legitimate hosts outside this slice will fail the check and be redirected, effectively blocking them from rescheduling/cancelling their own event.
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.
Nice catch, fixed it by querying it separately
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.
cubic found 2 issues across 9 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
…er is the host and fix type error
@kart1ka Thanks for flagging this. I have fixed it and added tests too. Please test this again and let me know if you find anything |
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.
if (isDisabledRescheduling) { | ||
// Check if user is a host or owner of the event type | ||
const userId = session?.user?.id; | ||
const userIsHost = booking?.eventType?.hosts?.find((host) => host.user.id === userId); |
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.
It does not make sense to let all the hosts cancel/reschedule the booking that do not belong to them.
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.
This is how we should check for permission:
cal.com/apps/web/modules/bookings/views/bookings-single-view.getServerSideProps.tsx
Line 158 in 6410687
const checkIfUserIsHost = (userId?: number | null) => { |
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.
@kart1ka I am not sure which one is correct, i can see both implementations in code, check https://github.com/calcom/cal.com/blob/main/packages/features/bookings/lib/handleInternalNote.ts#L23-L25, https://github.com/calcom/cal.com/blob/main/apps/web/lib/video/%5Buid%5D/getServerSideProps.ts#L81-L104 and the one you tagged above
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.
pls use the reference I have mentioned.
if (booking?.eventType?.disableRescheduling) { | ||
// Check if user is a host or owner of the event type | ||
const userId = session?.user?.id; | ||
const userIsHost = booking?.eventType?.hosts?.find((host) => host.userId === userId); |
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.
same here
if (userId && !isHostOrOwner) { | ||
const hostCheck = await prisma.host.findFirst({ | ||
where: { | ||
userId, | ||
eventTypeId: eventData.id, | ||
}, | ||
}); | ||
isHostOrOwner = !!hostCheck; | ||
} |
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.
same issue here
const isTeamAdminResult = await isTeamAdmin(userId, booking.eventType.team.id); | ||
if (isTeamAdminResult) { | ||
hasTeamOrOrgPermissions = true; | ||
} else if (booking.eventType.team.parentId) { | ||
const isOrgAdminResult = await isOrganisationAdmin(userId, booking.eventType.team.parentId); | ||
if (isOrgAdminResult) { | ||
hasTeamOrOrgPermissions = true; | ||
} |
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.
Since, we are checking for team or org admin/owner in many place, I would prefer to abstract this check into its own function and then reuse that in all the places we are performing this check.
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.
Done
@@ -106,7 +108,28 @@ async function handler(input: CancelBookingInput) { | |||
throw new HttpError({ statusCode: 400, message: "User not found" }); | |||
} | |||
|
|||
if (bookingToDelete.eventType?.disableCancelling) { | |||
// Check if user is a host or owner of the event type | |||
const userIsHost = bookingToDelete.eventType?.hosts?.find((host) => host.user.id === userId); |
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.
same issue here as well.
Note Other AI code review bot(s) detectedCodeRabbit 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. Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This PR is being marked as stale due to inactivity. |
@asadath1395 Are you working on this PR? |
@kart1ka Yes, i have addressed most of the comments, have couple more to fix. Will find some time this week to address those. |
What does this PR do?
Hosts not able to cancel/reschedule the event when Disable Rescheduling/Cancelling is enabled
Visual Demo (For contributors especially)
Before
https://www.loom.com/share/bf768bc270e04c48b37ec8d9379a8a0a?sid=c4fa7153-8794-47ef-8320-f42252c880b3
After
https://www.loom.com/share/5b130406f3364ec591764672e8a9660a?sid=6c02726f-84c4-4862-b08b-aed83d956c4f
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Check the videos above