Comprehensive workflow reminder management for booking lifecycle events#6
Comprehensive workflow reminder management for booking lifecycle events#6everettbu wants to merge 1 commit intoworkflow-queue-basefrom
Conversation
…re still sent (#7232) * small UI fix * fix cancelling scheduled emails * improve comments * delete reminders for rescheduled bookings * add migration file * cancel rescheduled bookings immediately * remove immediate delete for request reschedule --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive workflow reminder management system for booking lifecycle events, introducing a significant architectural change from immediate deletion to soft-deletion patterns for workflow reminders. The core enhancement involves adding a cancelled boolean field to the WorkflowReminder database schema and refactoring reminder management across multiple components.
The key changes include:
-
Database Schema Evolution: A new migration adds an optional
cancelledfield to theWorkflowRemindertable, enabling soft-deletion patterns that preserve audit trails while supporting proper cleanup of external services. -
Enhanced Reminder Managers: Both email and SMS reminder managers (
emailReminderManager.tsandsmsReminderManager.ts) have been updated to accept reminder IDs and support more sophisticated cancellation strategies. The email manager now supports both immediate deletion and soft-deletion via thecancelledfield, while the SMS manager ensures database cleanup occurs regardless of external API success. -
Centralized Cancellation Logic: The workflow reminder cancellation logic has been consolidated across multiple files (
handleNewBooking.ts,handleCancelBooking.ts,bookings.tsx,workflows.tsx) to use the updated reminder manager functions, removing manual database operations and complex batch processing. -
Cron-based Cleanup System: A new automated cleanup mechanism in
scheduleEmailReminders.tsprocesses reminders marked as cancelled within a 1-hour window, cancelling them via SendGrid API and removing them from the database.
This refactoring centralizes reminder management logic, improves data consistency, and provides better handling of external service integrations (SendGrid for emails, Twilio for SMS) while maintaining comprehensive audit trails of workflow reminder lifecycle events.
Confidence score: 2/5
• This PR has significant implementation issues that could cause runtime errors and performance problems
• Critical problems include incorrect Prisma typing, potential race conditions with forEach async operations, and missing error handling that could break booking flows
• Files needing attention: packages/features/ee/workflows/api/scheduleEmailReminders.ts, packages/trpc/server/routers/viewer/workflows.tsx, packages/features/ee/workflows/components/WorkflowStepContainer.tsx
10 files reviewed, 7 comments
| }); | ||
|
|
||
| try { | ||
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; |
There was a problem hiding this comment.
syntax: Incorrect type definition. Should be Promise<WorkflowReminder>[] instead of the Prisma client type
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| const workflowRemindersToDelete: Promise<WorkflowReminder>[] = []; |
| for (const reminder of remindersToCancel) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
logic: Missing null check for reminder.referenceId - this will cause SendGrid API errors if referenceId is null
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
style: The reminder cancellation logic should await the deletion operations to ensure they complete before proceeding with the reschedule
| //cancel all workflow reminders from steps that were edited | ||
| remindersToUpdate.forEach(async (reminder) => { | ||
| if (reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| await ctx.prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
logic: Using forEach with async functions can cause race conditions and doesn't wait for completion. Consider using Promise.all() with map() or a for...of loop to ensure proper async handling.
| //cancel all workflow reminders from steps that were edited | |
| remindersToUpdate.forEach(async (reminder) => { | |
| if (reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| await ctx.prisma.workflowReminder.deleteMany({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| }); | |
| //cancel all workflow reminders from steps that were edited | |
| await Promise.all(remindersToUpdate.map(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| })); |
| scheduledReminders.forEach((reminder) => { | ||
| if (reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
logic: The forEach calls to delete reminders are not awaited, which could lead to the workflow being deleted before all reminders are properly cancelled. This could leave orphaned scheduled reminders.
| scheduledReminders.forEach((reminder) => { | |
| if (reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); | |
| await Promise.all(scheduledReminders.map(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| })); |
| if (immediateDelete) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
logic: When immediateDelete is true, only the SendGrid cancellation happens but the database record remains. This could leave orphaned records if the SendGrid cancellation succeeds but the calling code expects the database record to be cleaned up.
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN; | |||
There was a problem hiding this comment.
style: Consider adding a default value (likely FALSE) to ensure consistent behavior for existing and new records
|
This PR is being marked as stale due to inactivity. |
Review Summary🏷️ Draft Comments (30)
🔍 Comments beyond diff scope (3)
|
3 participants
Test 6