Skip to content
Jump to bottom

Automated Test: sms-retry-enhanced #380

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

Automated Test: sms-retry-enhanced #380

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 +31 to +43
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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if retryCount is used/incremented by other workflow method handlers (email, whatsapp)
rg -n "retryCount" --type=ts -C 3

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 4246

🏁 Script executed:

#!/bin/bash
# Search for other workflow reminder handlers
fd -e ts -path "*/workflows/*" "schedule.*Reminders" 

echo "---"

# Search for WorkflowReminder model/schema definition
fd -e ts -o -e prisma | xargs rg -l "WorkflowReminder" | head -20

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1793

🏁 Script executed:

#!/bin/bash
# Check if email and whatsapp handlers use retryCount
echo "=== WHATSAPP HANDLER ===" 
rg -n "retryCount" packages/features/ee/workflows/api/scheduleWhatsappReminders.ts

echo ""
echo "=== EMAIL HANDLER ===" 
rg -n "retryCount" packages/features/ee/workflows/api/scheduleEmailReminders.ts

echo ""
echo "=== MIGRATION FILE ===" 
cat packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 262

🏁 Script executed:

#!/bin/bash
# Get the full WHERE clause context to understand the delete operation
sed -n '25,50p' packages/features/ee/workflows/api/scheduleSMSReminders.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 657

🏁 Script executed:

#!/bin/bash
# Get more context around the delete operation including what happens after
sed -n '28,65p' packages/features/ee/workflows/api/scheduleSMSReminders.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 961

🏁 Script executed:

#!/bin/bash
# Check the catch block and surrounding code to verify the update error handling
sed -n '165,200p' packages/features/ee/workflows/api/scheduleSMSReminders.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 917

Add method filter to retryCount deletion condition to prevent unintended removal of non-SMS reminders.

The second OR branch deletes any WorkflowReminder with retryCount > 1, regardless of method. While currently only the SMS handler increments retryCount (keeping email/WhatsApp reminders at 0), this is a logic error that could cause issues if retry logic is added to other methods in the future.

Proposed fix 
       OR: [
         {
           method: WorkflowMethods.SMS,
           scheduledDate: {
             lte: dayjs().toISOString(),
           },
         },
         {
+          method: WorkflowMethods.SMS,
           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
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
retryCount: {
gt: 1,
},
},
],
OR: [
{
method: WorkflowMethods.SMS,
scheduledDate: {
lte: dayjs().toISOString(),
},
},
{
method: WorkflowMethods.SMS,
retryCount: {
gt: 1,
},
},
],
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 31 -
43, The OR branch currently deletes reminders with retryCount > 1 regardless of
method; update the condition so the second alternative also requires method ===
WorkflowMethods.SMS (i.e., make the OR array entry include both { method:
WorkflowMethods.SMS, retryCount: { gt: 1 } }) so only SMS reminders are matched
for the retryCount-based deletion; modify the OR clause around
WorkflowMethods.SMS, scheduledDate and retryCount to include this method filter.

},
});

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}`);
Comment on lines 189 to 198
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

The prisma.update in the catch block can throw and abort processing of remaining reminders.

If the retryCount update on Line 190 fails (e.g., DB connectivity issue), the error propagates out of the catch block and the for loop, skipping all subsequent unscheduled reminders and potentially returning a 500 instead of the 200 on Line 201.

Wrap this in a nested try-catch or use .catch() to ensure the loop continues.

🛡️ Proposed fix 
     } catch (error) {
-      await prisma.workflowReminder.update({
-        where: {
-          id: reminder.id,
-        },
-        data: {
-          retryCount: reminder.retryCount + 1,
-        },
-      });
+      try {
+        await prisma.workflowReminder.update({
+          where: {
+            id: reminder.id,
+          },
+          data: {
+            retryCount: reminder.retryCount + 1,
+          },
+        });
+      } catch (updateError) {
+        console.log(`Error updating retryCount for reminder ${reminder.id}: ${updateError}`);
+      }
       console.log(`Error scheduling SMS with error ${error}`);
     }
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts` around lines 189
- 198, The catch block that calls prisma.workflowReminder.update can itself
throw and abort the enclosing for loop; wrap the retryCount update in its own
try-catch (or append a .catch()) so failures to increment reminder.retryCount
are swallowed/logged and do not rethrow. Specifically, protect the
prisma.workflowReminder.update call that uses reminder.id and
reminder.retryCount inside scheduleSMSReminders.ts (inside the outer catch) with
a nested try/catch (or use prisma.workflowReminder.update(...).catch(...)) and
log the update error without throwing so the loop continues processing remaining
reminders.

}
}
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