Skip to content

fix: Host schedule not updating upon updating/deleting default schedule #20750

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

anikdhabal
Copy link
Contributor

@anikdhabal anikdhabal commented Apr 17, 2025

Summary by mrge

Fixed host schedule synchronization to properly update when default schedules are changed, deleted, or updated.

Bug Fixes

  • Added setupHostDefaultSchedule function to ensure host schedules stay in sync with user default schedules
  • Updated delete and update handlers to call the new function when schedule changes occur

@graphite-app graphite-app bot requested a review from a team April 17, 2025 10:50
@keithwillcode keithwillcode added the core area: core, team members only label Apr 17, 2025
@dosubot dosubot bot added the 🐛 bug Something isn't working label Apr 17, 2025
Copy link

vercel bot commented Apr 17, 2025

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

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Apr 17, 2025 11:05am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Apr 17, 2025 11:05am

Copy link

@mrge-io mrge-io bot left a comment

Choose a reason for hiding this comment

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

mrge found 1 issue across 3 files. View it in mrge.io

Copy link

graphite-app bot commented Apr 17, 2025

Graphite Automations

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

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

Comment on lines +59 to +60
} else {
if (user.defaultScheduleId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if

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 good other than the nit I left

Keith wanted us to write unit tests for each server function we write, can you do that?

@github-actions github-actions bot marked this pull request as draft April 17, 2025 16:23
@hbjORbj hbjORbj marked this pull request as ready for review April 17, 2025 16:24
@hbjORbj hbjORbj enabled auto-merge (squash) April 17, 2025 16:24
@dosubot dosubot bot added the bookings area: bookings, availability, timezones, double booking label Apr 17, 2025
Copy link

@mrge-io mrge-io bot left a comment

Choose a reason for hiding this comment

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

mrge found 2 issues across 3 files. View them in mrge.io

@@ -45,6 +46,8 @@ export const deleteHandler = async ({ input, ctx }: DeleteOptions) => {
// to throw the error if there arent any other schedules
if (!scheduleToSetAsDefault) throw new TRPCError({ code: "BAD_REQUEST" });

await updateHostsWithNewDefaultSchedule(user.id, input.scheduleId, scheduleToSetAsDefault.id, prisma);
Copy link

Choose a reason for hiding this comment

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

Missing error handling around this async operation could lead to inconsistent state if it fails

@@ -45,6 +46,8 @@
// to throw the error if there arent any other schedules
if (!scheduleToSetAsDefault) throw new TRPCError({ code: "BAD_REQUEST" });

await updateHostsWithNewDefaultSchedule(user.id, input.scheduleId, scheduleToSetAsDefault.id, prisma);
Copy link

Choose a reason for hiding this comment

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

Consider using a database transaction for related database operations to ensure atomicity

scheduleId: number,
prisma: PrismaClient
) => {
return prisma.host.updateMany({
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to use prisma directly in trpc routers

We should create a repository function and use that here

@github-actions github-actions bot marked this pull request as draft April 21, 2025 15:00
auto-merge was automatically disabled April 21, 2025 15:00

Pull request was converted to draft

Copy link
Contributor

E2E results are ready!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants