Skip to content
Jump to bottom

Automated Test: workflow-queue-enhanced #357

Closed
wants to merge 1 commit into from
+241 −203
Closed

Automated Test: workflow-queue-enhanced #357

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
25 changes: 6 additions & 19 deletions packages/features/bookings/lib/handleCancelBooking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import {
BookingStatus,
MembershipRole,
Prisma,
PrismaPromise,
WebhookTriggerEvents,
WorkflowMethods,
WorkflowReminder,
Expand Down Expand Up @@ -483,29 +481,18 @@ async function handler(req: NextApiRequest & { userId?: number }) {
cancelScheduledJobs(booking);
});

//Workflows - delete all reminders for bookings
const remindersToDelete: PrismaPromise<Prisma.BatchPayload>[] = [];
//Workflows - cancel all reminders for cancelled bookings
updatedBookings.forEach((booking) => {
booking.workflowReminders.forEach((reminder) => {
if (reminder.scheduled && reminder.referenceId) {
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.referenceId);
}
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
const reminderToDelete = prisma.workflowReminder.deleteMany({
where: {
id: reminder.id,
},
});
remindersToDelete.push(reminderToDelete);
});
});
Comment on lines +484 to 493
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

Same fire-and-forget pattern — reminder deletions are not awaited.

forEach doesn't await the async deleteScheduledEmailReminder/deleteScheduledSMSReminder calls, so these promises are not included in the Promise.all on line 497. The handler could respond with "Booking successfully cancelled" while reminder cleanup is still in-flight (or has silently failed).

If this is intentional best-effort cleanup, consider at minimum collecting the promises so failures are logged:

♻️ Proposed fix 
   //Workflows - cancel all reminders for cancelled bookings
+  const reminderDeletions: Promise<void>[] = [];
   updatedBookings.forEach((booking) => {
     booking.workflowReminders.forEach((reminder) => {
       if (reminder.method === WorkflowMethods.EMAIL) {
-        deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+        reminderDeletions.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId));
       } else if (reminder.method === WorkflowMethods.SMS) {
-        deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+        reminderDeletions.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId));
       }
     });
   });

-  const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
+  const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes, ...reminderDeletions];
🤖 Prompt for AI Agents 
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 484 -
493, The reminder deletions inside updatedBookings.forEach are being fired
without awaiting their async work; collect the promises returned by
deleteScheduledEmailReminder and deleteScheduledSMSReminder (from the
updatedBookings -> booking.workflowReminders loop) into an array (e.g.,
reminderDeletionPromises) and then await Promise.all(reminderDeletionPromises)
before finishing the handler so cleanup is awaited; also catch/log rejections
from Promise.all to surface failures (or use Promise.allSettled if you want
best-effort logging without failing the whole flow).


const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes].concat(
remindersToDelete
);
const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];

await Promise.all(prismaPromises.concat(apiDeletes));

Expand Down
28 changes: 26 additions & 2 deletions packages/features/bookings/lib/handleNewBooking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import type { App, Credential, EventTypeCustomInput, Prisma } from "@prisma/client";
import { BookingStatus, SchedulingType, WebhookTriggerEvents } from "@prisma/client";
import {
App,
BookingStatus,
Credential,
EventTypeCustomInput,
Prisma,
SchedulingType,
WebhookTriggerEvents,
WorkflowMethods,
} from "@prisma/client";
import async from "async";
import { isValidPhoneNumber } from "libphonenumber-js";
import { cloneDeep } from "lodash";
Expand Down Expand Up @@ -28,7 +36,9 @@ import {
sendScheduledEmails,
sendScheduledSeatsEmails,
} from "@calcom/emails";
import { deleteScheduledEmailReminder } from "@calcom/features/ee/workflows/lib/reminders/emailReminderManager";
import { scheduleWorkflowReminders } from "@calcom/features/ee/workflows/lib/reminders/reminderScheduler";
import { deleteScheduledSMSReminder } from "@calcom/features/ee/workflows/lib/reminders/smsReminderManager";
import getWebhooks from "@calcom/features/webhooks/lib/getWebhooks";
import { isPrismaObjOrUndefined, parseRecurringEvent } from "@calcom/lib";
import { getVideoCallUrl } from "@calcom/lib/CalEventParser";
Expand Down Expand Up @@ -759,6 +769,7 @@ async function handler(req: NextApiRequest & { userId?: number | undefined }) {
},
},
payment: true,
workflowReminders: true,
},
});
}
Expand Down Expand Up @@ -950,6 +961,19 @@ async function handler(req: NextApiRequest & { userId?: number | undefined }) {
let videoCallUrl;

if (originalRescheduledBooking?.uid) {
try {
// cancel workflow reminders from previous rescheduled booking
originalRescheduledBooking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});
} catch (error) {
log.error("Error while canceling scheduled workflow reminders", error);
}
Comment on lines +964 to +975
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

forEach with async callbacks — deletions are fire-and-forget and the catch block is ineffective.

Array.prototype.forEach does not await the returned promises from async callbacks. The try-catch here will never catch rejection errors from deleteScheduledEmailReminder or deleteScheduledSMSReminder — they become unhandled promise rejections.

If fire-and-forget is intentional, remove the misleading try-catch. If errors should be caught, use Promise.all (or Promise.allSettled):

🐛 Proposed fix (if errors should be caught) 
     try {
       // cancel workflow reminders from previous rescheduled booking
-      originalRescheduledBooking.workflowReminders.forEach((reminder) => {
+      await Promise.allSettled(originalRescheduledBooking.workflowReminders.map((reminder) => {
         if (reminder.method === WorkflowMethods.EMAIL) {
-          deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+          return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
         } else if (reminder.method === WorkflowMethods.SMS) {
-          deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+          return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
         }
-      });
+      }));
     } catch (error) {
       log.error("Error while canceling scheduled workflow reminders", error);
     }
📝 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
try {
// cancel workflow reminders from previous rescheduled booking
originalRescheduledBooking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});
} catch (error) {
log.error("Error while canceling scheduled workflow reminders", error);
}
try {
// cancel workflow reminders from previous rescheduled booking
await Promise.allSettled(originalRescheduledBooking.workflowReminders.map((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
}));
} catch (error) {
log.error("Error while canceling scheduled workflow reminders", error);
}
🤖 Prompt for AI Agents 
In `@packages/features/bookings/lib/handleNewBooking.ts` around lines 964 - 975,
The try-catch around originalRescheduledBooking.workflowReminders.forEach(...)
is ineffective because forEach doesn't await async calls; replace the
fire-and-forget loop with collecting promises (map over
originalRescheduledBooking.workflowReminders and push either
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) or
deleteScheduledSMSReminder(reminder.id, reminder.referenceId)) and then await
Promise.allSettled(...) (or Promise.all(...) if you want fail-fast) so errors
from deleteScheduledEmailReminder and deleteScheduledSMSReminder are observed
and can be logged; keep references to WorkflowMethods to choose the correct
deletion call and log failures from the settled results.


// Use EventManager to conditionally use all needed integrations.
const updateManager = await eventManager.reschedule(
evt,
Expand Down
37 changes: 37 additions & 0 deletions packages/features/ee/workflows/api/scheduleEmailReminders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { NextApiRequest, NextApiResponse } from "next";
import dayjs from "@calcom/dayjs";
import { defaultHandler } from "@calcom/lib/server";
import prisma from "@calcom/prisma";
import { Prisma, WorkflowReminder } from "@calcom/prisma/client";
import { bookingMetadataSchema } from "@calcom/prisma/zod-utils";

import customTemplate, { VariablesType } from "../lib/reminders/templates/customTemplate";
Expand Down Expand Up @@ -39,6 +40,42 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
},
});

//cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});
Comment on lines +44 to +51
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Missing method: WorkflowMethods.EMAIL filter — this may cancel SMS reminders too.

This cron is specifically for email reminders, but the cancellation query doesn't filter by method. It will pick up all cancelled reminders (including SMS) scheduled within the next hour, and attempt to cancel them via the SendGrid API (which will fail or be a no-op for SMS reminders). It also deletes them from the DB, preventing the SMS cron from processing them.

   const remindersToCancel = await prisma.workflowReminder.findMany({
     where: {
       cancelled: true,
+      method: WorkflowMethods.EMAIL,
       scheduledDate: {
         lte: dayjs().add(1, "hour").toISOString(),
       },
     },
   });
📝 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
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
method: WorkflowMethods.EMAIL,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 44
- 51, The cancellation query is missing a method filter and may include
non-email reminders; update the prisma.workflowReminder.findMany call (the
remindersToCancel query) to include where.method: WorkflowMethods.EMAIL so only
email reminders are selected, and add an import/reference to WorkflowMethods in
this file if it's not already present; keep the existing cancelled: true and
scheduledDate <= dayjs().add(1, "hour").toISOString() predicates.


try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];

for (const reminder of remindersToCancel) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});

const workflowReminderToDelete = prisma.workflowReminder.delete({
where: {
id: reminder.id,
},
});

workflowRemindersToDelete.push(workflowReminderToDelete);
}
await Promise.all(workflowRemindersToDelete);
} catch (error) {
console.log(`Error cancelling scheduled Emails: ${error}`);
}
Comment on lines +53 to +77
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

A single SendGrid failure aborts cancellation for all remaining reminders.

The await client.request(...) on line 57 is inside a for loop wrapped in a single try-catch. If the SendGrid call fails for one reminder, the exception propagates to the outer catch and all subsequent reminders in remindersToCancel are skipped (never cancelled, never deleted).

Move the try-catch inside the loop so each reminder is handled independently:

🐛 Proposed fix 
-  try {
-    const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
-
-    for (const reminder of remindersToCancel) {
+  const workflowRemindersToDelete: Promise<WorkflowReminder>[] = [];
+
+  for (const reminder of remindersToCancel) {
+    try {
       await client.request({
         url: "/v3/user/scheduled_sends",
         method: "POST",
         body: {
           batch_id: reminder.referenceId,
           status: "cancel",
         },
       });
 
       const workflowReminderToDelete = prisma.workflowReminder.delete({
         where: {
           id: reminder.id,
         },
       });
 
       workflowRemindersToDelete.push(workflowReminderToDelete);
+    } catch (error) {
+      console.log(`Error cancelling scheduled Email for reminder ${reminder.id}: ${error}`);
     }
-    await Promise.all(workflowRemindersToDelete);
-  } catch (error) {
-    console.log(`Error cancelling scheduled Emails: ${error}`);
   }
+  await Promise.all(workflowRemindersToDelete);
📝 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
try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
for (const reminder of remindersToCancel) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
const workflowReminderToDelete = prisma.workflowReminder.delete({
where: {
id: reminder.id,
},
});
workflowRemindersToDelete.push(workflowReminderToDelete);
}
await Promise.all(workflowRemindersToDelete);
} catch (error) {
console.log(`Error cancelling scheduled Emails: ${error}`);
}
const workflowRemindersToDelete: Promise<WorkflowReminder>[] = [];
for (const reminder of remindersToCancel) {
try {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
const workflowReminderToDelete = prisma.workflowReminder.delete({
where: {
id: reminder.id,
},
});
workflowRemindersToDelete.push(workflowReminderToDelete);
} catch (error) {
console.log(`Error cancelling scheduled Email for reminder ${reminder.id}: ${error}`);
}
}
await Promise.all(workflowRemindersToDelete);
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 53
- 77, The current try-catch around the entire loop means a single failing
client.request aborts processing of the rest of reminders; change the logic so
each reminder in remindersToCancel is processed inside its own try-catch: for
each reminder call client.request(...) inside a per-iteration try block,
handle/log errors for that reminder in the catch and continue to the next
reminder, and only create/push the prisma.workflowReminder.delete
(workflowRemindersToDelete) for reminders you intended to delete (e.g., after a
successful cancel or according to your desired failure policy) so that failures
for one reminder do not prevent others from being cancelled/deleted.


//find all unscheduled Email reminders
const unscheduledReminders = await prisma.workflowReminder.findMany({
where: {
Expand Down
126 changes: 60 additions & 66 deletions packages/features/ee/workflows/components/WorkflowStepContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,81 +387,75 @@ export default function WorkflowStepContainer(props: WorkflowStepProps) {
}}
/>
</div>
{(isPhoneNumberNeeded || isSenderIdNeeded) && (
{isPhoneNumberNeeded && (
<div className="mt-2 rounded-md bg-gray-50 p-4 pt-0">
{isPhoneNumberNeeded && (
<Label className="pt-4">{t("custom_phone_number")}</Label>
<div className="block sm:flex">
<PhoneInput<FormValues>
control={form.control}
name={`steps.${step.stepNumber - 1}.sendTo`}
placeholder={t("phone_number")}
id={`steps.${step.stepNumber - 1}.sendTo`}
className="min-w-fit sm:rounded-tl-md sm:rounded-bl-md sm:border-r-transparent"
required
onChange={() => {
const isAlreadyVerified = !!verifiedNumbers
?.concat([])
.find((number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`));
setNumberVerified(isAlreadyVerified);
}}
/>
<Button
color="secondary"
disabled={numberVerified || false}
className={classNames(
"-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ",
numberVerified ? "hidden" : "mt-3 sm:mt-0"
)}
onClick={() =>
sendVerificationCodeMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
})
}>
{t("send_code")}
</Button>
</div>

{form.formState.errors.steps &&
form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo && (
<p className="mt-1 text-xs text-red-500">
{form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo?.message || ""}
</p>
)}
{numberVerified ? (
<div className="mt-1">
<Badge variant="green">{t("number_verified")}</Badge>
</div>
) : (
<>
<Label className="pt-4">{t("custom_phone_number")}</Label>
<div className="block sm:flex">
<PhoneInput<FormValues>
control={form.control}
name={`steps.${step.stepNumber - 1}.sendTo`}
placeholder={t("phone_number")}
id={`steps.${step.stepNumber - 1}.sendTo`}
className="min-w-fit sm:rounded-tl-md sm:rounded-bl-md sm:border-r-transparent"
required
onChange={() => {
const isAlreadyVerified = !!verifiedNumbers
?.concat([])
.find(
(number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)
);
setNumberVerified(isAlreadyVerified);
<div className="mt-3 flex">
<TextField
className=" border-r-transparent"
placeholder="Verification code"
value={verificationCode}
onChange={(e) => {
setVerificationCode(e.target.value);
}}
required
/>
<Button
color="secondary"
disabled={numberVerified || false}
className={classNames(
"-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ",
numberVerified ? "hidden" : "mt-3 sm:mt-0"
)}
onClick={() =>
sendVerificationCodeMutation.mutate({
className="-ml-[3px] rounded-tl-none rounded-bl-none "
disabled={verifyPhoneNumberMutation.isLoading}
onClick={() => {
verifyPhoneNumberMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
})
}>
{t("send_code")}
code: verificationCode,
});
}}>
Verify
</Button>
Comment on lines +436 to 457
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Hardcoded English strings — missing i18n for "Verification code" and "Verify".

The placeholder on line 439 ("Verification code") and the button text on line 456 ("Verify") are hardcoded in English instead of using t("verification_code") and t("verify") respectively. Other strings in this component are properly localized.

                       <TextField
                         className=" border-r-transparent"
-                        placeholder="Verification code"
+                        placeholder={t("verification_code")}
                         value={verificationCode}
-                          Verify
+                          {t("verify")}
📝 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
<div className="mt-3 flex">
<TextField
className=" border-r-transparent"
placeholder="Verification code"
value={verificationCode}
onChange={(e) => {
setVerificationCode(e.target.value);
}}
required
/>
<Button
color="secondary"
disabled={numberVerified || false}
className={classNames(
"-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ",
numberVerified ? "hidden" : "mt-3 sm:mt-0"
)}
onClick={() =>
sendVerificationCodeMutation.mutate({
className="-ml-[3px] rounded-tl-none rounded-bl-none "
disabled={verifyPhoneNumberMutation.isLoading}
onClick={() => {
verifyPhoneNumberMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
})
}>
{t("send_code")}
code: verificationCode,
});
}}>
Verify
</Button>
<div className="mt-3 flex">
<TextField
className=" border-r-transparent"
placeholder={t("verification_code")}
value={verificationCode}
onChange={(e) => {
setVerificationCode(e.target.value);
}}
required
/>
<Button
color="secondary"
className="-ml-[3px] rounded-tl-none rounded-bl-none "
disabled={verifyPhoneNumberMutation.isLoading}
onClick={() => {
verifyPhoneNumberMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
code: verificationCode,
});
}}>
{t("verify")}
</Button>
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx` around
lines 436 - 457, Replace the two hardcoded English strings with i18n keys: use
the translation function for the TextField placeholder and the Button label in
the WorkflowStepContainer component—update the TextField placeholder from
"Verification code" to t("verification_code") and the Button text from "Verify"
to t("verify"); ensure the component imports/has access to t and leave the rest
of the logic around verificationCode, verifyPhoneNumberMutation,
form.getValues(...) and step.stepNumber unchanged.

</div>

{form.formState.errors.steps &&
form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo && (
<p className="mt-1 text-xs text-red-500">
{form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo?.message || ""}
</p>
)}
{numberVerified ? (
<div className="mt-1">
<Badge variant="green">{t("number_verified")}</Badge>
</div>
) : (
<>
<div className="mt-3 flex">
<TextField
className=" border-r-transparent"
placeholder="Verification code"
value={verificationCode}
onChange={(e) => {
setVerificationCode(e.target.value);
}}
required
/>
<Button
color="secondary"
className="-ml-[3px] rounded-tl-none rounded-bl-none "
disabled={verifyPhoneNumberMutation.isLoading}
onClick={() => {
verifyPhoneNumberMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
code: verificationCode,
});
}}>
Verify
</Button>
</div>
</>
)}
</>
)}
</div>
Expand Down
45 changes: 33 additions & 12 deletions packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,20 +194,41 @@ export const scheduleEmailReminder = async (
}
};

export const deleteScheduledEmailReminder = async (referenceId: string) => {
export const deleteScheduledEmailReminder = async (
reminderId: number,
referenceId: string | null,
immediateDelete?: boolean
) => {
try {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
if (!referenceId) {
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});

return;
}

await client.request({
url: `/v3/user/scheduled_sends/${referenceId}`,
method: "DELETE",
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
return;
}

await prisma.workflowReminder.update({
where: {
id: reminderId,
},
data: {
cancelled: true,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
Expand Down
11 changes: 9 additions & 2 deletions packages/features/ee/workflows/lib/reminders/smsReminderManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,16 @@ export const scheduleSMSReminder = async (
}
};

export const deleteScheduledSMSReminder = async (referenceId: string) => {
export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
try {
await twilio.cancelSMS(referenceId);
if (referenceId) {
await twilio.cancelSMS(referenceId);
}
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
}
Comment on lines +177 to 189
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

If twilio.cancelSMS throws, the DB record is never deleted.

Both the Twilio cancellation and the Prisma delete are in the same try block. If cancelSMS fails (e.g., the message was already delivered or Twilio is unreachable), the reminder record will persist in the database, potentially causing repeated cancellation attempts.

Consider either separating the error handling or proceeding to delete regardless:

♻️ Proposed fix 
 export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
   try {
     if (referenceId) {
-      await twilio.cancelSMS(referenceId);
+      try {
+        await twilio.cancelSMS(referenceId);
+      } catch (error) {
+        console.log(`Error canceling SMS via Twilio: ${error}`);
+      }
     }
     await prisma.workflowReminder.delete({
       where: {
         id: reminderId,
       },
     });
   } catch (error) {
     console.log(`Error canceling reminder with error ${error}`);
   }
 };
🤖 Prompt for AI Agents 
In `@packages/features/ee/workflows/lib/reminders/smsReminderManager.ts` around
lines 177 - 189, deleteScheduledSMSReminder currently wraps both
twilio.cancelSMS and prisma.workflowReminder.delete in the same try/catch so a
failure in cancelSMS prevents the DB delete; update deleteScheduledSMSReminder
to call twilio.cancelSMS inside its own try/catch (log the Twilio error but
swallow it) and then always call prisma.workflowReminder.delete in a separate
try/catch (or let it bubble) so the reminder record is removed regardless of
Twilio failures; reference the functions twilio.cancelSMS and
prisma.workflowReminder.delete when making the changes.

Expand Down
2 changes: 2 additions & 0 deletions packages/prisma/migrations/20230217230604_add_cancelled_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 "cancelled" BOOLEAN;
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 @@ -641,6 +641,7 @@ model WorkflowReminder {
scheduled Boolean
workflowStepId Int
workflowStep WorkflowStep @relation(fields: [workflowStepId], references: [id], onDelete: Cascade)
cancelled Boolean?
}

enum WorkflowTemplates {
Expand Down