Automated Test: sms-retry-enhanced

[admin-coderabbit]

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

• New Features

  • Added automatic retry functionality for SMS reminders, improving delivery reliability when initial attempts fail.

• Chores

  • Updated workflow reminder system to track retry attempts and optimize cleanup of outdated reminders.

[coderabbit-eval]

📝 Walkthrough

Walkthrough

This pull request introduces retry count tracking to SMS reminder scheduling. It adds a retryCount field to the WorkflowReminder model via schema update and database migration, and updates the scheduling logic to increment retry counts on failure, delete reminders based on retry attempts, and query reminders with retry information.

Changes

Cohort / File(s)	Summary
Database Schema packages/prisma/schema.prisma, packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql	Added retryCount integer field with default value 0 to WorkflowReminder model. Migration creates corresponding database column.
SMS Scheduling Logic packages/features/ee/workflows/api/scheduleSMSReminders.ts	Updated deletion logic to purge reminders with retryCount > 1 or past SMS reminders. Enhanced querying to select and cast retryCount. Added retry count increments on scheduling failures and error conditions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

When SMS reminders take their flight,
Some miss the mark, some hit just right,
With retryCount tracking every hop,
This fluffy logic won't ever stop!
The bravest workflows retry with grace! 📨🐰

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Automated Test: sms-retry-enhanced' is vague and generic. It uses a prefix ('Automated Test:') that doesn't describe the actual technical change being made, and while 'sms-retry-enhanced' references the feature branch, it doesn't clearly explain what the pull request implements or changes.	Use a more descriptive title that clearly explains the main technical change, such as 'Add retry count tracking to SMS reminders' or 'Implement SMS reminder retry mechanism'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sms-retry-enhanced

---

_{Comment @coderabbit eval help to get the list of available commands and usage tips.}

[coderabbit-eval]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/features/ee/workflows/api/scheduleSMSReminders.ts`:
- Around line 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).

🧹 Nitpick comments (1)

packages/features/ee/workflows/api/scheduleSMSReminders.ts (1)

189-198: DB update inside catch can throw unhandled, halting the loop for remaining reminders.

If prisma.workflowReminder.update at line 190 throws (e.g., DB connectivity issue, or the reminder was already deleted by the deleteMany above in a race), the exception propagates out of the for loop and the remaining unscheduled reminders won't be processed in this cron run.

Wrap the update in its own try-catch to keep the loop going:

♻️ Proposed fix

     } catch (error) {
+      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}`);
     }

[coderabbit-eval]

⚠️ 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).