-
Notifications
You must be signed in to change notification settings - Fork 12
Add guest management functionality to existing bookings #10
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: guest-management-base
Are you sure you want to change the base?
Conversation
* feat: ability to add guests via app.cal.com/bookings * fix: some update * fix: minor issue * fix: final update * update * update * add requested changes * fix type error * small update * final update * fix type error * fix location * update calender event --------- Co-authored-by: Somay Chauhan <[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.
Greptile Summary
This PR implements guest management functionality for existing bookings, allowing users to add additional attendees after a booking has been created. The implementation introduces a complete end-to-end feature spanning frontend UI, backend API, and email notifications.
Frontend Changes:
- Adds an "Add Guests" action to the booking list item menu that opens a modal dialog
- Creates a new
AddGuestsDialogcomponent with proper form validation using Zod schemas - Introduces a reusable
MultiEmailcomponent (with lazy loading) for managing multiple email addresses - Integrates with the existing TRPC mutation system and provides appropriate loading states and error handling
Backend API:
- Adds a new
addGuestsTRPC endpoint in the bookings router with comprehensive authorization checks - Implements permission validation ensuring only organizers, attendees, or team admins/owners can add guests
- Includes duplicate prevention and blacklist filtering for guest emails
- Integrates with calendar systems to update external calendar events when guests are added
Email System:
- Creates specialized email templates for both organizers (
OrganizerAddGuestsEmail) and attendees (AttendeeAddGuestsEmail) - Implements a new
sendAddGuestsEmailsfunction that differentiates between newly added guests (who receive scheduled emails) and existing attendees (who receive guest addition notifications) - Email templates reuse existing scheduled email components with customized titles and subjects for the guest addition context
The implementation follows Cal.com's established architectural patterns including lazy-loaded TRPC handlers, component composition for email templates, and the existing UI component library structure. The feature integrates seamlessly with existing systems like database operations, calendar synchronization, and internationalization.
Confidence score: 2/5
- This PR has significant security and logic issues that make it unsafe to merge without major revisions
- Critical permission validation logic uses AND instead of OR operators, potentially breaking authorization checks completely
- Multiple areas lack proper error handling, input validation, and could cause runtime failures
- Files that need more attention:
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts,packages/emails/templates/organizer-add-guests-email.ts,packages/ui/form/MultiEmail.tsx
14 files reviewed, 7 comments
| input, | ||
| }); | ||
| }), | ||
| addGuests: authedProcedure.input(ZAddGuestsInputSchema).mutation(async ({ input, ctx }) => { |
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.
logic: Using authedProcedure instead of bookingsProcedure means this endpoint bypasses the existing booking authorization middleware in util.ts. This creates inconsistent security patterns compared to other booking operations like editLocation.
| import { z } from "zod"; | ||
|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), |
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.
style: Consider using .int().positive() for more explicit booking ID validation
| replyTo: [this.calEvent.organizer.email, ...this.calEvent.attendees.map(({ email }) => email)], | ||
| subject: `${this.t("guests_added_event_type_subject", { | ||
| eventType: this.calEvent.type, | ||
| name: this.calEvent.attendees[0].name, |
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.
logic: Unsafe array access - calEvent.attendees[0] will throw if attendees array is empty. Add validation or use optional chaining.
| }); | ||
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); |
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.
style: Consider using an empty array [] instead of [""] as the initial state to be more semantically correct and avoid the validation issue mentioned above
| const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => { | ||
| const uniqueEmails = new Set(emails); | ||
| return uniqueEmails.size === emails.length; | ||
| }); |
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.
style: The Zod schema is duplicated from the backend schema file. Consider importing the schema from packages/trpc/server/routers/viewer/bookings/addGuests.schema.ts to maintain single source of truth
|
|
||
| function MultiEmail({ value, readOnly, label, setValue, placeholder }: MultiEmailProps) { | ||
| const { t } = useLocale(); | ||
| value = value || []; |
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.
logic: Mutating props directly can cause unexpected behavior. Use a derived state or default value instead.
| value = value || []; | |
| const emails = value || []; |
| {value.map((field, index) => ( | ||
| <li key={index}> |
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.
logic: Using array index as React key can cause rendering issues when items are reordered. Consider using a unique ID for each email entry.
| color="minimal" | ||
| variant="button" | ||
| StartIcon="user-plus" |
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.
syntax: The variant="button" prop may not be valid for Button component based on the component's interface.
| const blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | ||
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | ||
| : []; |
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.
logic: Case sensitivity issue: blacklisted emails are converted to lowercase but guest emails aren't, potentially allowing blacklisted emails through
| const blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | |
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | |
| : []; | |
| const uniqueGuests = guests.filter( | |
| (guest) => | |
| !booking.attendees.some((attendee) => guest === attendee.email) && | |
| !blacklistedGuestEmails.includes(guest.toLowerCase()) | |
| ); |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
This PR is being marked as stale due to inactivity. |
Test 10