Fixes that workflow reminders of cancelled and rescheduled bookings are still sent - #1
Fixes that workflow reminders of cancelled and rescheduled bookings are still sent#1linxia0415 wants to merge 8 commits into
Conversation
![code-hawk-sit[bot]](https://avatars.githubusercontent.com/in/3248189?s=60&v=4){kind=small}
| if (originalRescheduledBooking?.uid) { | ||
| try { | ||
| // cancel workflow reminders from previous rescheduled booking | ||
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { |
There was a problem hiding this comment.
🟡 AI 代码审查发现问题
📋 问题概述
handleNewBooking中对workflowReminders的遍历逻辑存在潜在扩展性问题。当前代码只处理了EMAIL和SMS两种方法类型，但如果未来添加新的WorkflowMethods类型（如PUSH_NOTIFICATION），这些类型的提醒将不会被取消，可能导致资源浪费或错误状态。
📍 问题详情
🟡 问题 1 | 严重程度: MEDIUM | 行号: 966
💬 详细说明：
- 如果将来添加新的提醒方法类型，这些提醒将不会被正确取消，可能导致系统中残留无效的计划提醒，占用不必要的资源。
📝 问题代码：
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);
}
});
💡 修复建议：
应该使用switch语句并包含default分支来处理未知的提醒方法类型，或者在else if分支后添加日志记录未知类型。
✅ 修复示例：
🔗 参考链接
无
| status: "cancel", | ||
| }, | ||
| }); | ||
| if (!referenceId) { |
There was a problem hiding this comment.
🔴 AI 代码审查发现问题
📋 问题概述
deleteScheduledEmailReminder函数在处理referenceId为null的情况时，直接删除数据库记录而不尝试取消外部API调度。这可能导致外部服务仍然按计划发送邮件，而系统认为提醒已被取消。
📍 问题详情
🔴 问题 1 | 严重程度: HIGH | 行号: 203
💬 详细说明：
- 如果referenceId为null但外部服务实际上已经安排了邮件发送，则无法取消该邮件，可能导致用户收到不应该收到的提醒邮件。
📝 问题代码：
if (!referenceId) {
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
return;
}
💡 修复建议：
应增加日志记录，说明为什么referenceId为null，并考虑是否需要其他机制来处理这种情况。
✅ 修复示例：
🔗 参考链接
无
| await client.request({ | ||
| url: `/v3/user/scheduled_sends/${referenceId}`, | ||
| method: "DELETE", | ||
| if (immediateDelete) { |
There was a problem hiding this comment.
🟡 AI 代码审查发现问题
📋 问题概述
当immediateDelete为true时，函数只取消外部API调度但不更新数据库状态。这违反了'先外部后内部'的安全顺序原则，如果后续数据库操作失败，会导致状态不一致。
📍 问题详情
🟡 问题 1 | 严重程度: MEDIUM | 行号: 213
💬 详细说明：
- 如果外部API取消成功但数据库更新失败，系统将处于不一致状态：外部服务已取消提醒，但数据库中仍保留记录，可能造成混乱。
📝 问题代码：
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
return;
}
💡 修复建议：
应先更新数据库状态为已取消，然后再取消外部API调度，确保即使外部调用失败，数据库状态也是正确的。
✅ 修复示例：
🔗 参考链接
无
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟡 AI 代码审查发现问题
📋 问题概述
错误处理不当，使用console.log而不是适当的错误处理。这可能导致错误信息丢失，难以调试问题。
📍 问题详情
🟡 问题 1 | 严重程度: MEDIUM | 行号: 234
💬 详细说明：
- 错误信息可能不会被正确记录或监控，使得生产环境中的问题难以排查。
📝 问题代码：
console.log(`Error canceling reminder with error ${error}`);
💡 修复建议：
应使用适当的日志记录机制，并可能需要抛出错误或返回错误状态以便调用方处理。
✅ 修复示例：
🔗 参考链接
无
| if (referenceId) { | ||
| await twilio.cancelSMS(referenceId); | ||
| } | ||
| await prisma.workflowReminder.delete({ |
There was a problem hiding this comment.
🔴 AI 代码审查发现问题
📋 问题概述
发现 2 个邻近问题（第 179–182 行）
📍 问题详情
🟡 问题 1 | 严重程度: MEDIUM | 行号: 179
💬 详细说明：
- 如果referenceId为null但外部服务实际上已经安排了短信发送，则无法取消该短信，可能导致用户收到不应该收到的提醒短信。
📝 问题代码：
if (referenceId) {
await twilio.cancelSMS(referenceId);
}
💡 修复建议：
应增加日志记录，说明为什么referenceId为null，并考虑是否需要其他机制来处理这种情况。
✅ 修复示例：
🔴 问题 2 | 严重程度: HIGH | 行号: 182
💬 详细说明：
- 如果Twilio取消成功但数据库删除失败，系统将保留一个已取消的提醒记录，可能导致未来的清理任务无法正确处理。
📝 问题代码：
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
💡 修复建议：
应考虑添加事务处理或补偿机制，确保两个操作要么都成功，要么都失败。
✅ 修复示例：
🔗 参考链接
无
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟡 AI 代码审查发现问题
📋 问题概述
错误处理不当，使用console.log而不是适当的错误处理。这可能导致错误信息丢失，难以调试问题。
📍 问题详情
🟡 问题 1 | 严重程度: MEDIUM | 行号: 188
💬 详细说明：
- 错误信息可能不会被正确记录或监控，使得生产环境中的问题难以排查。
📝 问题代码：
console.log(`Error canceling reminder with error ${error}`);
💡 修复建议：
应使用适当的日志记录机制，并可能需要抛出错误或返回错误状态以便调用方处理。
✅ 修复示例：
🔗 参考链接
无
1 participant
What does this PR do?
Fixes that workflow emails are still sent for cancelled and rescheduled bookings. The fix of PR #6991 was wrong and didn't work as expected.
The SendGrid endpoint
DELETE /v3/user/scheduled_sends/${referenceId}was not used correctly. This endpoint deletes a cancelled scheduled email fromscheduled_sendsby removing the 'cancel' status (so email will again be scheduled).The reason why cancelling our scheduled emails stopped working is that all cancerlled emails are saved in
scheduled_sendsuntil the scheduled date but the max. of pending cancellations is 100. Once we reached 100 pending cancellations, new cancellations failed and emails were still sent out.The solution implemented in this PR:
cancelledis added to theworkflowRemindermodelscheduleEmailRemindersis responsible for the final cancellation of the scheduled email (runs every 15 mins)Fixes #7225
Environment: Staging(main branch) / Production
Type of change