SMS workflow reminder retry count tracking#9
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>
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as SMS Reminder Scheduler
participant DB as Database
loop For each unscheduled reminder
Scheduler->>DB: Query reminder (includes retryCount)
alt Reminder scheduled successfully
Scheduler->>DB: Update reminder as scheduled
else Scheduling fails
Scheduler->>DB: Increment retryCount
alt retryCount > 1
Scheduler->>DB: Delete reminder
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ 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.
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![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 (3)
packages/features/ee/workflows/api/scheduleSMSReminders.ts(3 hunks)packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/features/ee/workflows/api/scheduleSMSReminders.ts (2)
packages/features/ee/workflows/lib/getWorkflowReminders.ts (2)
select(112-172)PartialWorkflowReminder(35-40)packages/prisma/index.ts (1)
prisma(39-39)
🔇 Additional comments (5)
packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql (1)
1-2: LGTM! Clean migration implementation.The migration correctly adds the
retryCountcolumn as a non-nullable integer with a sensible default value of 0, ensuring existing records are properly handled.packages/prisma/schema.prisma (1)
1000-1000: LGTM! Schema change is consistent with migration.The
retryCountfield definition properly matches the database migration with appropriate type and default value.packages/features/ee/workflows/api/scheduleSMSReminders.ts (3)
56-60: LGTM! Proper query extension for retryCount.The select query correctly includes the
retryCountfield and the type annotation properly extends the existing type.
178-187: LGTM! Proper retry count increment on scheduling failure.The logic correctly increments the retry count when SMS scheduling fails, maintaining proper failure tracking.
190-197: LGTM! Proper retry count increment on exceptions.The error handling correctly tracks retry attempts when exceptions occur, ensuring consistent failure tracking across different error scenarios.
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Critical logic error in deletion criteria.
The current OR condition will delete ANY workflow reminder (EMAIL, WHATSAPP, etc.) with retryCount > 1, not just SMS reminders. This could break other workflow types.
The logic should ensure both conditions apply only to SMS reminders:
await prisma.workflowReminder.deleteMany({
where: {
- OR: [
- {
- method: WorkflowMethods.SMS,
- scheduledDate: {
- lte: dayjs().toISOString(),
- },
- },
- {
- retryCount: {
- gt: 1,
- },
- },
- ],
+ method: WorkflowMethods.SMS,
+ OR: [
+ {
+ scheduledDate: {
+ lte: dayjs().toISOString(),
+ },
+ },
+ {
+ retryCount: {
+ gt: 1,
+ },
+ },
+ ],
},
});📝 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.
| OR: [ | |
| { | |
| method: WorkflowMethods.SMS, | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| await prisma.workflowReminder.deleteMany({ | |
| where: { | |
| method: WorkflowMethods.SMS, | |
| OR: [ | |
| { | |
| scheduledDate: { | |
| lte: dayjs().toISOString(), | |
| }, | |
| }, | |
| { | |
| retryCount: { | |
| gt: 1, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In packages/features/ee/workflows/api/scheduleSMSReminders.ts between lines 31
and 43, the OR condition incorrectly deletes any workflow with retryCount > 1
regardless of method type. To fix this, combine the conditions so that both
retryCount > 1 and method being SMS are checked together, ensuring only SMS
reminders meeting these criteria are deleted. Replace the separate OR conditions
with a single condition that includes method: WorkflowMethods.SMS along with the
retryCount or scheduledDate checks.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 9
Summary by CodeRabbit