Comprehensive workflow reminder management for booking lifecycle events

[hussam789]

User description

PR #5

---

PR Type

Bug fix, Enhancement

---

Description

• Fix workflow reminders being sent for cancelled/rescheduled bookings

• Implement deferred cancellation of scheduled email reminders via cancelled flag

• Update reminder deletion functions to handle both immediate and deferred cancellation

• Simplify reminder cleanup logic by removing batch promise accumulation

• Add workflowReminders to booking queries for proper reminder management

---

Diagram Walkthrough

flowchart LR
  A["Booking Cancelled/Rescheduled"] --> B["Call deleteScheduled Reminder Functions"]
  B --> C{Immediate Delete?}
  C -->|Yes| D["Cancel via SendGrid API"]
  C -->|No| E["Mark as cancelled in DB"]
  E --> F["Email Reminder Scheduler Picks Up"]
  F --> G["Cancel via SendGrid API"]
  G --> H["Delete from DB"]

Loading

File Walkthrough

	Relevant files
Bug fix	4 files handleCancelBooking.ts Remove batch promise accumulation for reminder deletion    +6/-19    handleNewBooking.ts Add workflow reminder cancellation on booking reschedule  +26/-2    bookings.tsx Simplify reminder cancellation on booking reschedule          +6/-19    workflows.tsx Update reminder deletion calls with new function signatures +61/-83
Enhancement	3 files scheduleEmailReminders.ts Implement deferred email reminder cancellation logic          +37/-0    emailReminderManager.ts Add deferred cancellation and immediate delete modes          +33/-12  smsReminderManager.ts Update SMS reminder deletion with database cleanup              +9/-2
Formatting	1 files WorkflowStepContainer.tsx Simplify phone number verification UI logic                            +60/-66
Database migration	1 files migration.sql Add cancelled boolean column to WorkflowReminder                  +2/-0
Schema changes	1 files schema.prisma Add optional cancelled field to WorkflowReminder model      +1/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unreliable cancellation handling Description: Errors during SendGrid cancellation or DB deletion are only logged to console without retries or alerting, risking orphaned scheduled emails that could still be sent to users. scheduleEmailReminders.ts [53-77] Referred Code try { const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; for (const reminder of remindersToCancel) { await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id: reminder.referenceId, status: "cancel", }, }); const workflowReminderToDelete = prisma.workflowReminder.delete({ where: { id: reminder.id, }, }); workflowRemindersToDelete.push(workflowReminderToDelete); } ... (clipped 4 lines)
	Deferred cancel risk Description: Deferred email cancellation sets a cancelled flag without verifying SendGrid batch_id validity; if scheduler fails or is delayed beyond one hour window, emails may still be sent despite cancellation intent. emailReminderManager.ts [197-235] Referred Code export const deleteScheduledEmailReminder = async ( reminderId: number, referenceId: string | null, immediateDelete?: boolean ) => { try { if (!referenceId) { await prisma.workflowReminder.delete({ where: { id: reminderId, }, }); return; } if (immediateDelete) { await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { ... (clipped 18 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing auditing: New code cancels and deletes workflow reminders but does not log audit entries capturing who performed the action and outcome, making reconstruction harder. Referred Code try { const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; for (const reminder of remindersToCancel) { await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id: reminder.referenceId, status: "cancel", }, }); const workflowReminderToDelete = prisma.workflowReminder.delete({ where: { id: reminder.id, }, }); workflowRemindersToDelete.push(workflowReminderToDelete); } ... (clipped 4 lines)
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Weak error handling: Catch blocks use console logging without actionable context or retries, and cancellation loops lack per-item failure handling which may cause partial failures to go unnoticed. Referred Code try { const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; for (const reminder of remindersToCancel) { await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id: reminder.referenceId, status: "cancel", }, }); const workflowReminderToDelete = prisma.workflowReminder.delete({ where: { id: reminder.id, }, }); workflowRemindersToDelete.push(workflowReminderToDelete); } ... (clipped 4 lines)
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Console exposure: Using console.log in error paths may expose internal error details in environments where console output is user-visible or insufficiently protected. Referred Code } catch (error) { console.log(`Error cancelling scheduled Emails: ${error}`); }
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured logs: Error logging via console lacks structured format and redaction guarantees, risking inconsistent observability and potential sensitive data exposure if included in error objects. Referred Code } catch (error) { console.log(`Error cancelling scheduled Emails: ${error}`); }

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Unify the reminder cancellation strategy The PR inconsistently applies immediate and deferred cancellation for email reminders across different scenarios. A single, unified cancellation strategy should be adopted for all similar events to ensure predictable behavior. Examples: packages/features/bookings/lib/handleNewBooking.ts [968] deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); packages/trpc/server/routers/viewer/bookings.tsx [490] deleteScheduledEmailReminder(reminder.id, reminder.referenceId); Solution Walkthrough: Before: // In handleNewBooking.ts (rescheduling) originalBooking.reminders.forEach(reminder => { // Immediate cancellation for email deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); }); // In viewer/bookings.tsx (rescheduling) bookingToReschedule.reminders.forEach(reminder => { // Deferred cancellation for email deleteScheduledEmailReminder(reminder.id, reminder.referenceId); }); // In handleCancelBooking.ts (cancellation) booking.reminders.forEach(reminder => { // Deferred cancellation for email deleteScheduledEmailReminder(reminder.id, reminder.referenceId); }); After: // A unified strategy should be chosen. For example, using deferred cancellation everywhere: // In handleNewBooking.ts (rescheduling) originalBooking.reminders.forEach(reminder => { // Use deferred cancellation consistently deleteScheduledEmailReminder(reminder.id, reminder.referenceId); }); // In viewer/bookings.tsx (rescheduling) bookingToReschedule.reminders.forEach(reminder => { // Use deferred cancellation consistently deleteScheduledEmailReminder(reminder.id, reminder.referenceId); }); // In handleCancelBooking.ts (cancellation) booking.reminders.forEach(reminder => { // Use deferred cancellation consistently deleteScheduledEmailReminder(reminder.id, reminder.referenceId); }); Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design inconsistency in the new reminder cancellation logic, where both immediate and deferred strategies are used for similar events like rescheduling, which could lead to unpredictable behavior.	High
Possible issue	Delete reminder record after cancellation In deleteScheduledEmailReminder, add a database deletion for the WorkflowReminder record within the immediateDelete block to prevent leaving orphaned data. packages/features/ee/workflows/lib/reminders/emailReminderManager.ts [213-223] if (immediateDelete) { await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id: referenceId, status: "cancel", }, }); + await prisma.workflowReminder.delete({ + where: { + id: reminderId, + }, + }); return; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug where an orphaned WorkflowReminder record is left in the database when immediateDelete is true, and the proposed fix resolves this data integrity issue.	Medium
General	Improve performance by parallelizing cancellations Refactor the reminder cancellation logic to execute SendGrid API calls in parallel using Promise.all and then delete all corresponding database records in a single deleteMany operation for better performance. packages/features/ee/workflows/api/scheduleEmailReminders.ts [53-77] try { - const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; + const cancelPromises = remindersToCancel + .filter((reminder) => reminder.referenceId) + .map((reminder) => + client.request({ + url: "/v3/user/scheduled_sends", + method: "POST", + body: { + batch_id: reminder.referenceId, + status: "cancel", + }, + }) + ); - for (const reminder of remindersToCancel) { - await client.request({ - url: "/v3/user/scheduled_sends", - method: "POST", - body: { - batch_id: reminder.referenceId, - status: "cancel", + await Promise.all(cancelPromises); + + await prisma.workflowReminder.deleteMany({ + where: { + id: { + in: remindersToCancel.map((reminder) => reminder.id), }, - }); - - const workflowReminderToDelete = prisma.workflowReminder.delete({ - where: { - id: reminder.id, - }, - }); - - workflowRemindersToDelete.push(workflowReminderToDelete); - } - await Promise.all(workflowRemindersToDelete); + }, + }); } catch (error) { console.log(`Error cancelling scheduled Emails: ${error}`); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a performance bottleneck by replacing sequential API calls with parallel execution using Promise.all, and also improves database efficiency by using a single deleteMany operation.	Medium
	Handle async operations in parallel Refactor the reminder cancellation logic to use Promise.all for handling asynchronous deleteScheduled... calls concurrently, ensuring all operations are properly awaited and handled. packages/features/bookings/lib/handleNewBooking.ts [964-975] try { // cancel workflow reminders from previous rescheduled booking - originalRescheduledBooking.workflowReminders.forEach((reminder) => { + const cancellationPromises = originalRescheduledBooking.workflowReminders.map((reminder) => { if (reminder.method === WorkflowMethods.EMAIL) { - deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); + return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); } else if (reminder.method === WorkflowMethods.SMS) { - deleteScheduledSMSReminder(reminder.id, reminder.referenceId); + return deleteScheduledSMSReminder(reminder.id, reminder.referenceId); } + return Promise.resolve(); }); + await Promise.all(cancellationPromises); } catch (error) { log.error("Error while canceling scheduled workflow reminders", error); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that using forEach with async functions is problematic and proposes a more robust solution using Promise.all to handle asynchronous operations concurrently and improve error handling.	Medium
More