-
Couldn't load subscription status.
- Fork 10.9k
feat: Edit booking before confirmation and add optional attendee pending emails #24691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Edit booking before confirmation and add optional attendee pending emails #24691
Conversation
…ing emails Signed-off-by: Parship Chowdhury <[email protected]>
|
@Parship12 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 8 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/emails/email-manager.ts">
<violation number="1" location="packages/emails/email-manager.ts:444">
Adding this early return also skips the attendee SMS, so the new "Disable pending booking emails" toggle stops all pending notifications, not just the email. Please gate only the email send while keeping the SMS dispatch intact.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
packages/emails/email-manager.ts
Outdated
| eventTypeMetadata?: EventTypeMetadata | ||
| ) => { | ||
| if (eventTypeDisableAttendeeEmail(eventTypeMetadata)) return; | ||
| if (eventTypeMetadata?.disableStandardEmails?.request?.attendee) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this early return also skips the attendee SMS, so the new "Disable pending booking emails" toggle stops all pending notifications, not just the email. Please gate only the email send while keeping the SMS dispatch intact.
Prompt for AI agents
Address the following comment on packages/emails/email-manager.ts at line 444:
<comment>Adding this early return also skips the attendee SMS, so the new "Disable pending booking emails" toggle stops all pending notifications, not just the email. Please gate only the email send while keeping the SMS dispatch intact.</comment>
<file context>
@@ -441,6 +441,7 @@ const _sendAttendeeRequestEmailAndSMS = async (
eventTypeMetadata?: EventTypeMetadata
) => {
if (eventTypeDisableAttendeeEmail(eventTypeMetadata)) return;
+ if (eventTypeMetadata?.disableStandardEmails?.request?.attendee) return;
const calendarEvent = formatCalEvent(calEvent);
</file context>
✅ Addressed in 51cf524
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cubic-dev-ai How it will effect the SMS dispatch? Can you please describe the issue properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Parship12 Of course. The early return statement you've added causes the _sendAttendeeRequestEmailAndSMS function to exit before the SMS sending logic is executed.
I suggested a change because the new toggle is specifically for disabling emails, but your implementation also disables SMS notifications for pending bookings.
Here's the relevant part of the function:
// packages/emails/email-manager.ts:446
const _sendAttendeeRequestEmailAndSMS = async (
// ...
) => {
if (eventTypeDisableAttendeeEmail(eventTypeMetadata)) return;
// Your new line causes the function to exit here
if (eventTypeMetadata?.disableStandardEmails?.request?.attendee) return;
const calendarEvent = formatCalEvent(calEvent);
// This sends the email
await sendEmail(() => new AttendeeRequestEmail(calendarEvent, attendee));
// This SMS logic is never reached if the new toggle is on
const eventRequestSms = new EventRequestSMS(calendarEvent);
await eventRequestSms.sendSMSToAttendee(attendee);
};To fix this, you can wrap just the email-sending line with the conditional check, ensuring the SMS is always sent:
const _sendAttendeeRequestEmailAndSMS = async (
calEvent: CalendarEvent,
attendee: Person,
eventTypeMetadata?: EventTypeMetadata
) => {
if (eventTypeDisableAttendeeEmail(eventTypeMetadata)) return;
const calendarEvent = formatCalEvent(calEvent);
if (!eventTypeMetadata?.disableStandardEmails?.request?.attendee) {
await sendEmail(() => new AttendeeRequestEmail(calendarEvent, attendee));
}
const eventRequestSms = new EventRequestSMS(calendarEvent);
await eventRequestSms.sendSMSToAttendee(attendee);
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Parship12 nice, making it draft until you address this
Signed-off-by: Parship12 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 8 files
What does this PR do?
Before changes (3 emails):
After changes (2 emails):
Two new features:
Add guests to pending booking:
Test the New Toggle
Please watch the below screen recordings for better understanding of the newly added features
Video Demo (if applicable):
Screen.Recording.2025-10-25.19273211.mp4
Screen.Recording.2025-10-25.200338.mp4
Mandatory Tasks (DO NOT REMOVE)
Summary by cubic
Enable editing of pending bookings before confirmation and add a setting to disable attendee pending emails, reducing unnecessary notifications. Addresses Linear CAL-6622 by making pending emails optional and shifting notifications to confirmation.
New Features
Migration