fix: use smsReminderNumber fallback for attendee phone in SMS reminders#27942
fix: use smsReminderNumber fallback for attendee phone in SMS reminders#27942anikdhabal merged 16 commits intomainfrom
Conversation
|
I reviewed this PR thoroughly. The overall approach is solid — mirroring the lazy email scheduling pattern for SMS via Tasker is the right fix. Here's my review: SummaryThe PR correctly identifies and fixes the root cause: The fix routes these reminders through the Tasker instead, which handles scheduling at any future date. Core changes look good
Issues found in
|
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/tasker/tasks/sendWorkflowSMS.ts">
<violation number="1" location="packages/features/tasker/tasks/sendWorkflowSMS.ts:89">
P2: Limit the profile query to only the needed fields (organizationId) instead of fetching the full row.</violation>
<violation number="2" location="packages/features/tasker/tasks/sendWorkflowSMS.ts:169">
P2: The opt-out message is being added to `bodyWithoutOptOut` while the SMS body uses the unmodified message. This causes SMS to miss the opt-out text when enabled and emails to include it. Swap the usage so SMS gets the opt-out version and email uses the plain message.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
|
I reviewed both Cubic AI violations and checked their confidence scores:
Neither issue meets the 9/10 confidence threshold required for automated fixes. These should be reviewed manually by a human reviewer if needed. |
Paragon: tests updated4 updated tests generated for this PR. Updated Tests
DetailsUpdated Tests
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts">
<violation number="1" location="packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts:188">
P2: `attendees: true` pulls all attendee columns. The SMS task only uses a handful of attendee fields (email/name/phone/timezone/locale), so this over-fetches data and increases exposure. Select only the fields actually used.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts
Outdated
Show resolved
Hide resolved
Devin AI is addressing Cubic AI's review feedbackA Devin session has been created to address the issues identified by Cubic AI. |
Pull request was converted to draft
|
Hey guys, I am the one who raised this problem since my SMS workflows were not working. Can I know when this can be solved? I am losing my pipeline because of this issue 😅 |
Tests Added by ParagonThe following test files have been added to this PR:
These tests were generated from an approved test proposal. Generated with Paragon |
Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
E2E results are ready! |
Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
CarinaWolli
left a comment
There was a problem hiding this comment.
In smsReminderManager.ts we are still scheduling sms without tasker
Pull request was converted to draft
…iles Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
…al branch state Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
…minders Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
…back Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>


Summary by cubic
Fixes recipient phone resolution for BEFORE_EVENT/AFTER_EVENT SMS and WhatsApp reminders. For SMS_ATTENDEE/WHATSAPP_ATTENDEE, use the seated attendee’s phone when a seat reference exists; otherwise prefer booking.smsReminderNumber, then fall back to the attendee’s phone (SMS_NUMBER/WHATSAPP_NUMBER still use the configured number).