Skip to content
Jump to bottom

Automated Test: sms-retry-enhanced #364

Closed
wants to merge 1 commit into from
+38 −6
Closed

Automated Test: sms-retry-enhanced #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 35 additions & 6 deletions packages/features/ee/workflows/api/scheduleSMSReminders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,19 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
//delete all scheduled sms reminders where scheduled date is past current date
await prisma.workflowReminder.deleteMany({
where: {
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
retryCount: {
gt: 1,
},
},
],
},
});
Comment on lines 29 to 45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

retryCount > 1 branch is not scoped to SMS — will delete EMAIL/WHATSAPP reminders too.

The first branch correctly filters by method: WorkflowMethods.SMS, but the second OR branch matches any WorkflowReminder with retryCount > 1 regardless of method. This will inadvertently delete email and WhatsApp reminders that happen to have a non-zero retry count (if retry logic is ever added for those methods, or if retryCount is manually set).

🐛 Proposed fix: scope retryCount condition to SMS 
   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.

Suggested change
await prisma.workflowReminder.deleteMany({
where: {
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
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` around lines 29 -
45, The deleteMany OR clause currently removes any WorkflowReminder with
retryCount > 1 regardless of method; update the
prisma.workflowReminder.deleteMany call so the retryCount condition is scoped to
SMS — either add method: WorkflowMethods.SMS to the retryCount branch or
refactor the where to require method: WorkflowMethods.SMS for both conditions
(referencing prisma.workflowReminder.deleteMany and WorkflowMethods.SMS and the
retryCount field).


Expand All @@ -44,8 +53,11 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
lte: dayjs().add(7, "day").toISOString(),
},
},
select,
})) as PartialWorkflowReminder[];
select: {
...select,
retryCount: true,
},
})) as (PartialWorkflowReminder & { retryCount: number })[];

if (!unscheduledReminders.length) {
res.json({ ok: true });
Expand Down Expand Up @@ -163,9 +175,26 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
referenceId: scheduledSMS.sid,
},
});
} else {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: reminder.retryCount + 1,
},
});
}
}
} catch (error) {
await prisma.workflowReminder.update({
where: {
id: reminder.id,
},
data: {
retryCount: reminder.retryCount + 1,
},
});
console.log(`Error scheduling SMS with error ${error}`);
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- AlterTable
ALTER TABLE "WorkflowReminder" ADD COLUMN "retryCount" INTEGER NOT NULL DEFAULT 0;
1 change: 1 addition & 0 deletions packages/prisma/schema.prisma
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,7 @@ model WorkflowReminder {
cancelled Boolean?
seatReferenceId String?
isMandatoryReminder Boolean? @default(false)
retryCount Int @default(0)
@@index([bookingUid])
@@index([workflowStepId])
Expand Down