Comprehensive workflow reminder management for booking lifecycle events#6
Comprehensive workflow reminder management for booking lifecycle events#6everettbu wants to merge 1 commit into
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>
WalkthroughThis set of changes updates the handling of workflow reminders for bookings and workflows. Instead of deleting reminders from the database immediately, scheduled reminders are now cancelled via external APIs or marked as cancelled in the database. New logic is added for cancelling email reminders close to their scheduled time. The Prisma schema and migration are updated to support a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DB
participant EmailAPI
participant SMSAPI
User->>App: Cancel booking or reschedule
App->>DB: Fetch workflow reminders
App->>EmailAPI: Cancel scheduled email (if referenceId)
App->>SMSAPI: Cancel scheduled SMS (if referenceId)
App->>DB: Mark reminder as cancelled or delete (based on type)
App->>DB: Delete attendees, booking references
sequenceDiagram
participant CronJob
participant DB
participant EmailAPI
CronJob->>DB: Find reminders with cancelled=true, scheduledDate < 1h
loop For each reminder
CronJob->>EmailAPI: Cancel scheduled send (using referenceId)
CronJob->>DB: Delete workflowReminder record
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/features/bookings/lib/handleCancelBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(4 hunks)packages/features/ee/workflows/api/scheduleEmailReminders.ts(2 hunks)packages/features/ee/workflows/components/WorkflowStepContainer.tsx(1 hunks)packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(1 hunks)packages/features/ee/workflows/lib/reminders/smsReminderManager.ts(1 hunks)packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/trpc/server/routers/viewer/bookings.tsx(1 hunks)packages/trpc/server/routers/viewer/workflows.tsx(6 hunks)
🔇 Additional comments (19)
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql (1)
1-2: LGTM! Clean and safe migration.The migration correctly adds a nullable boolean column for the cancelled state without affecting existing data.
packages/prisma/schema.prisma (1)
644-644: LGTM! Schema change aligns with migration.The nullable boolean field correctly supports the new cancellation state management for workflow reminders.
packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
177-190: LGTM! Improved separation of concerns.The updated function signature and logic properly separate external API cancellation (conditional on
referenceId) from database deletion (usingreminderId). This makes the cancellation process more robust and handles edge cases where external references might not exist.packages/features/bookings/lib/handleNewBooking.ts (3)
1-41: LGTM! Import updates align with function signature changes.The imports correctly include
WorkflowMethodsand the updated reminder deletion functions that now accept bothidandreferenceIdparameters.
772-772: LGTM! Necessary relation added for reminder cancellation.Adding
workflowReminders: trueto the query is required to access reminder data for the new cancellation logic.
964-975: LGTM! Proper reminder cancellation for rescheduled bookings.The new logic correctly:
- Iterates over workflow reminders from the original booking
- Calls the appropriate deletion function based on reminder method
- Passes both
idandreferenceIdas required by the updated function signatures- Handles errors gracefully without disrupting the rescheduling process
The placement before existing rescheduling logic ensures reminders are cancelled before creating new ones.
packages/features/ee/workflows/api/scheduleEmailReminders.ts (1)
10-10: LGTM! Necessary import for new cancellation logic.The additional imports are required for the new reminder cancellation functionality.
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (1)
390-461: LGTM! Clean UI refactor for phone verification workflow.The conditional rendering logic has been well-simplified to provide clearer states for phone number verification. The refactor eliminates duplication and provides a more intuitive user experience with explicit verified/unverified states.
packages/trpc/server/routers/viewer/bookings.tsx (2)
484-484: Accurate comment update for reschedule context.The comment change from "delete scheduled jobs of cancelled bookings" to "delete scheduled jobs of previous booking" better reflects the reschedule scenario where we're handling the previous booking's scheduled jobs.
487-494: Improved workflow reminder cancellation approach.The changes align well with the comprehensive workflow reminder management approach:
- Removed conditional checks for
scheduledandreferenceIdto ensure all reminders are processed- Now calling deletion functions with both
idandreferenceIdparameters as required by the updated API- This supports the new pattern of cancelling scheduled reminders via external APIs rather than immediate database deletion
The approach provides better reliability by ensuring all workflow reminders are properly cancelled during reschedule operations.
packages/features/bookings/lib/handleCancelBooking.ts (2)
484-493: Consistent workflow reminder cancellation implementation.The changes properly implement the new workflow reminder management approach:
- Removed conditional checks to ensure all reminders are processed consistently
- Updated to call deletion functions with both
idandreferenceIdparameters- Eliminates immediate database deletion in favor of external API cancellation
This aligns well with the broader changes across the codebase for comprehensive workflow reminder management.
495-495: Simplified batch operations after reminder management changes.The removal of
remindersToDeletefrom the prismaPromises array correctly reflects that workflow reminders are no longer immediately deleted from the database during cancellation. The batch now focuses only on attendee and booking reference cleanup, which is appropriate for the new reminder cancellation approach.packages/trpc/server/routers/viewer/workflows.tsx (6)
2-2: LGTM! Import organization improved.The reordering of Prisma-related imports improves code organization by grouping related imports together.
Also applies to: 9-9
211-217: LGTM! Updated to use new cancellation strategy.The code now properly cancels workflow reminders using the updated deletion functions with both reminder ID and reference ID parameters, aligning with the new cancellation approach instead of direct database deletion.
375-382: LGTM! Consistent cancellation logic for disabled event types.The reminder cancellation logic for event types that are no longer active follows the same pattern as other cancellation scenarios, properly passing both reminder ID and reference ID to the cancellation functions.
517-525: LGTM! Proper immediate cancellation for deleted steps.The reminder cancellation logic for deleted workflow steps correctly uses immediate cancellation (immediateDelete=true) for email reminders, which is appropriate when steps are permanently removed.
572-580: LGTM! Appropriate cancellation strategy for edited steps.The reminder cancellation logic for edited workflow steps uses the database marking approach (without immediateDelete), which is suitable since new reminders will be created for the updated step configuration.
904-943: LGTM! Formatting improvements in commented test code.The formatting changes in the commented test action code improve consistency and readability, even though this code is currently disabled.
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-236: deleteScheduledEmailReminder Implementation ApprovedThe function cleanly handles all three cancellation scenarios:
- No referenceId: removes the reminder directly from the database
- immediateDelete=true: issues a Sendgrid cancellation request
- Default: flags the reminder as cancelled for deferred cleanup
Verification confirmed that the
WorkflowRemindermodel inpackages/prisma/schema.prismaincludes the nullablecancelledBoolean field:model WorkflowReminder { id Int @id @default(autoincrement()) … cancelled Boolean? }No further changes are required—approving these updates.
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | ||
| const remindersToCancel = await prisma.workflowReminder.findMany({ | ||
| where: { | ||
| cancelled: true, | ||
| scheduledDate: { | ||
| lte: dayjs().add(1, "hour").toISOString(), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| 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); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for referenceId before API cancellation.
The cancellation logic is well-structured, but there's a potential issue: reminder.referenceId could be null, but it's used directly as batch_id in the SendGrid API call without validation.
Apply this diff to add proper null checking:
for (const reminder of remindersToCancel) {
- await client.request({
- url: "/v3/user/scheduled_sends",
- method: "POST",
- body: {
- batch_id: reminder.referenceId,
- status: "cancel",
- },
- });
+ if (reminder.referenceId) {
+ await client.request({
+ url: "/v3/user/scheduled_sends",
+ method: "POST",
+ body: {
+ batch_id: reminder.referenceId,
+ status: "cancel",
+ },
+ });
+ }This ensures we only attempt API cancellation when a referenceId exists, which aligns with the pattern used in the SMS reminder manager.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | |
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); | |
| 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); | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } | |
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | |
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); | |
| try { | |
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| for (const reminder of remindersToCancel) { | |
| if (reminder.referenceId) { | |
| 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); | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } |
🤖 Prompt for AI Agents
In packages/features/ee/workflows/api/scheduleEmailReminders.ts between lines 43
and 77, the code uses reminder.referenceId directly in the API call without
checking if it is null. To fix this, add a null check for reminder.referenceId
before making the API request to cancel the scheduled send. Only proceed with
the cancellation API call and deletion if referenceId is not null, ensuring the
code does not attempt to cancel with an invalid batch_id.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 6
Summary by CodeRabbit
New Features
Improvements
Bug Fixes