SMS workflow reminder retry count tracking#4
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds retry tracking for SMS workflow reminders to handle delivery failures. The implementation adds a retryCount field to the WorkflowReminder model and includes logic to increment the count on failures and clean up reminders that have exceeded the retry threshold.
Changes:
- Added
retryCountfield to theWorkflowReminderdatabase model with a default value of 0 - Updated cleanup logic to delete reminders that have exceeded 1 retry attempt
- Implemented retry count incrementing for both SMS scheduling failures and catch-all error scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/prisma/schema.prisma | Adds retryCount integer field to WorkflowReminder model |
| packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql | Database migration to add the retryCount column |
| packages/features/ee/workflows/api/scheduleSMSReminders.ts | Implements retry logic including cleanup conditions, field selection, and retry count updates on failures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
The cleanup condition deletes reminders with retryCount > 1, which means reminders are only deleted after 2 failed attempts. However, this condition is in an OR clause with the scheduled date check, meaning reminders could be deleted immediately if their scheduledDate has passed, regardless of retry count. The retry count condition should be combined with the method and scheduledDate checks using AND logic to ensure proper retry behavior before cleanup.
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| retryCount: { | |
| gt: 1, | |
| }, |
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| data: { | ||
| retryCount: reminder.retryCount + 1, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The retry count increment logic is duplicated in both the else block (lines 179-186) and the catch block (lines 190-197). Consider extracting this into a helper function to reduce duplication and improve maintainability.
Test 9nn
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#9](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/9)*