Repository navigation
feat: ability to add guests via app.cal.com/bookings - #14740
Conversation
|
@anikdhabal is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
github-actions
Bot
added
booking-page
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 Eighty-five Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored. |
dosubot
Bot
added
the
bookings
Graphite Automations"Add community label" took an action on this PR • (04/29/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (04/29/24)1 reviewer was added to this PR based on Keith Williams's automation. |
ibex088
left a comment
ibex088
left a comment
There was a problem hiding this comment.
Hi @anikdhabal,
Great job! I have a few recommendations, let me know what you think:
-
– I'm not sure this is the right email to send to the person we just invited.
-
In this video (https://www.loom.com/share/a265d095174149fb9d551227f3dd42ee?sid=5bad974f-6453-49e4-bd36-1fbe9504701e), I noticed that the first email includes the cal-video link, but the subsequent emails do not.
Hello @SomayChauhan , thanks for the review. Can you explain the first recommendation? |
https://www.loom.com/share/5978b488df724862ad8f340a651172de?sid=1f96fb9f-3844-4183-8aeb-698953e04b3e for the ones that were already were part of the event, |
|
I've made changes, and the second recommendation is working fine for me with any other conference app. I think it is a Cal Videos internal error. |
Thanks for pointing this out. I’ve fixed it. |
E2E results are ready! |
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Can we update the existing calendar events of the booking to add the new attendee?
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 15 files. 0 high-severity issues found. Verdict: approved.
packages/emails/src/templates/OrganizerAddGuestsEmail.tsx (low)
- Reviewed packages/emails/src/templates/OrganizerAddGuestsEmail.tsx — looks good
packages/emails/email-manager.ts (low)
- Reviewed packages/emails/email-manager.ts — looks good
apps/web/components/booking/BookingListItem.tsx (low)
- Reviewed apps/web/components/booking/BookingListItem.tsx — looks good
packages/emails/templates/attendee-add-guests-email.ts (low)
- Reviewed packages/emails/templates/attendee-add-guests-email.ts — looks good
packages/emails/src/templates/AttendeeAddGuestsEmail.tsx (low)
- Reviewed packages/emails/src/templates/AttendeeAddGuestsEmail.tsx — looks good
apps/web/public/static/locales/en/common.json (low)
- Reviewed apps/web/public/static/locales/en/common.json — looks good
packages/trpc/server/routers/viewer/bookings/addGuests.schema.ts (low)
- Reviewed packages/trpc/server/routers/viewer/bookings/addGuests.schema.ts — looks good
packages/emails/src/templates/index.ts (low)
- Reviewed packages/emails/src/templates/index.ts — looks good
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (low)
- Reviewed packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts — looks good
packages/ui/form/MultiEmail.tsx (low)
- Reviewed packages/ui/form/MultiEmail.tsx — looks good
packages/emails/templates/organizer-add-guests-email.ts (low)
- Reviewed packages/emails/templates/organizer-add-guests-email.ts — looks good
packages/trpc/server/routers/viewer/bookings/_router.tsx (low)
- Reviewed packages/trpc/server/routers/viewer/bookings/_router.tsx — looks good
packages/ui/form/MultiEmailLazy.tsx (low)
- Reviewed packages/ui/form/MultiEmailLazy.tsx — looks good
packages/ui/index.tsx (medium)
- Reviewed packages/ui/index.tsx — looks good
apps/web/components/dialog/AddGuestsDialog.tsx (low)
- Reviewed apps/web/components/dialog/AddGuestsDialog.tsx — looks good
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Add additional guests to a booking
Problem
This PR (fixes #14691, #15663) adds the ability for organizers, team admins/owners, and bookers (attendees) to add additional guests to an existing booking after it has been created. It introduces a new tRPC mutation viewer.bookings.addGuests, a UI dialog, a reusable MultiEmail input component, new email templates, and calendar-event attendee updates.
Solution Reviewed
The implementation adds: (1) a addGuests.handler.ts tRPC mutation that fetches the booking, checks authorization (organizer / team admin-or-owner / attendee), filters guests against existing attendees and a BLACKLISTED_GUEST_EMAILS env var, creates new Attendee rows via createMany, rebuilds a CalendarEvent, calls EventManager.updateCalendarAttendees for calendar sync, and sends notification emails via sendAddGuestsEmails; (2) an AddGuestsDialog with client-side zod email+uniqueness validation; (3) a lazy-loaded MultiEmail UI component; (4) organizer/attendee email templates that subclass the existing scheduled-email classes.
Summary
The feature is well-structured and follows existing patterns (lazy handler cache, email template hierarchy, dialog wiring). However, there are three blocking issues: an authorization logic bug (&& instead of ||), no server-side cap on guest count (DoS/abuse vector), and calendar updates using the requester's credentials instead of the organizer's (silently breaking the core feature for non-organizers). Several non-blocking issues around email case-insensitivity, missing booking-status validation, lack of transactional consistency, and error-handling gaps should also be addressed.
Files Reviewed
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts— deeply reviewedpackages/trpc/server/routers/viewer/bookings/addGuests.schema.ts— deeply reviewedpackages/trpc/server/routers/viewer/bookings/_router.tsx— deeply reviewedapps/web/components/dialog/AddGuestsDialog.tsx— deeply reviewedpackages/ui/form/MultiEmail.tsx— deeply reviewedpackages/ui/form/MultiEmailLazy.tsx— lightly reviewed (matches repo Lazy pattern)packages/ui/index.tsx— lightly reviewed (export wiring)packages/emails/email-manager.ts— deeply reviewedpackages/emails/templates/organizer-add-guests-email.ts— deeply reviewedpackages/emails/templates/attendee-add-guests-email.ts— deeply reviewedpackages/emails/src/templates/OrganizerAddGuestsEmail.tsx— lightly reviewedpackages/emails/src/templates/AttendeeAddGuestsEmail.tsx— lightly reviewedpackages/emails/src/templates/index.ts— lightly reviewed (export wiring)apps/web/components/booking/BookingListItem.tsx— lightly reviewed (dialog wiring)apps/web/public/static/locales/en/common.json— lightly reviewed (locale keys)
Verification
- Typecheck — skipped: full
turbo run type-checkon the monorepo is infeasible in this context. Boundary contracts (constructor signatures,updateCalendarAttendees/generateIcsStringsignatures, i18next key presence, package exports) were verified manually by the integration pass. - i18next keys — all 9 new/referenced keys confirmed present in
common.jsonexceptyou_do_not_have_permission(missing — see inline finding). - Constructor signatures —
OrganizerAddGuestsEmail({ calEvent, teamMember? })andAttendeeAddGuestsEmail(calEvent, attendee)match parent class constructors.
Verdict
Recommend changes before merge — three blocking issues (authorization logic error, missing guest-count cap, wrong credentials for calendar update) must be fixed for the feature to work correctly and safely. The non-blocking issues should be addressed in the same PR or a fast follow.
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); | ||
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && |
There was a problem hiding this comment.
🔴 Blocking — Authorization logic bug (&& vs ||)
isTeamAdminOrOwner uses && between isTeamAdmin() and isTeamOwner(). Per the implementations in packages/lib/server/queries/teams/index.ts, isTeamAdmin returns truthy for membership role ADMIN or OWNER, while isTeamOwner returns truthy only for OWNER. The conjunction is therefore true only when the user is an OWNER — a team ADMIN gets truthy && falsy = false and is denied with FORBIDDEN.
This contradicts the PR's stated intent ("team admins/owners" per the discussion on this PR) and the variable name isTeamAdminOrOwner. A pure team admin cannot add guests.
Fix: use || (or just call isTeamAdmin(), which already covers both ADMIN and OWNER roles).
|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), |
There was a problem hiding this comment.
🔴 Blocking — No server-side cap on guest count (DoS / mass-email abuse)
guests: z.array(z.string().email()) has no .max(). Any authorized caller — which per the handler includes any authenticated user who is merely an attendee of the booking — can submit thousands of emails in a single request. Each address creates an Attendee row (the Attendee model has no @@unique([bookingId, email]) constraint), triggers EventManager.updateCalendarAttendees calendar API calls, and sends outbound emails to every existing attendee plus the organizer/team.
There is no rate limiting or bound. This is an abuse vector for mass-email spam and DB/calendar API exhaustion.
Fix: add a reasonable .max() (e.g. .max(25) or a configurable limit) to the schema, consistent with other booking guest limits in the codebase.
| }; | ||
| } | ||
|
|
||
| const credentials = await getUsersCredentials(ctx.user); |
There was a problem hiding this comment.
🔴 Blocking — Calendar update uses the requester's credentials, not the organizer's
getUsersCredentials(ctx.user) fetches credentials for the acting user, then new EventManager({ ...user, credentials }) is built from that user. But the booking's calendar references (Google/Outlook event UIDs) belong to the organizer. Inside updateAllCalendarEvents, credentials are matched by reference.credentialId against this.calendarCredentials — the requester's credentials.
When a non-organizer (an attendee, or a team admin/owner) adds guests: the requester either has no credential of that type (the update silently no-ops) or has their own (the calendar API call targets an event UID that doesn't exist in the requester's calendar and fails, caught/logged). Net effect: Google/Outlook calendar event attendees are NOT updated when a non-organizer adds guests — contradicting the PR's stated behavior.
Fix: use the organizer's credentials (the organizer is already fetched above) to build the EventManager, or restrict calendar updates to the organizer path.
|
|
||
| const uniqueGuests = guests.filter( | ||
| (guest) => | ||
| !booking.attendees.some((attendee) => guest === attendee.email) && |
There was a problem hiding this comment.
🟡 Non-blocking — Case-sensitive email comparisons throughout the filter
Both the dedup check (guest === attendee.email, line 76) and the blacklist check (blacklistedGuestEmails.includes(guest), line 77) compare email strings verbatim. The blacklist entries are lowercased (line 71) but guest is not lowercased before comparison, so Spam@Example.com bypasses a spam@example.com blacklist entry. Similarly, a guest differing only in case from an existing attendee passes the dedup filter and is added as a duplicate Attendee row.
The repo's canonical pattern (e.g. handleNewBooking.ts, checkIfBookerEmailIsBlocked.ts) lowercases both sides. All email comparisons here (dedup, blacklist, and the attendee authorization check on line 52) should be case-insensitive.
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); |
There was a problem hiding this comment.
🟡 Non-blocking — sendAddGuestsEmails receives the original unfiltered guests array
Line 168 passes guests (the raw input) to sendAddGuestsEmails(evt, guests), not uniqueGuests. Inside sendAddGuestsEmails (email-manager.ts:541), newGuests.includes(attendee.email) classifies which attendees are new guests — using the dirty, un-deduped, non-lowercased list. This can misclassify attendees (e.g. a case-variant of an existing attendee that was actually filtered out by uniqueGuests would still match includes and get the wrong email template).
Fix: pass uniqueGuests (or a lowercased version of it) instead of guests.
| credentials: [...credentials], | ||
| }); | ||
|
|
||
| await eventManager.updateCalendarAttendees(evt, booking); |
There was a problem hiding this comment.
🟡 Non-blocking — Calendar update failures are not surfaced
updateCalendarAttendees returns void but internally updateAllCalendarEvents returns EventResult[] with success flags. When the calendar update silently fails (e.g. no matching credentials — see the credentials finding at line 158; booking.references.length === 0; or the internal catch block at EventManager.ts:921 that returns all results with success: false), the handler proceeds to send emails and returns { message: "Guests added" } as if successful.
editLocation.handler.ts inspects results with results.every((res) => !res.success) before proceeding. Consider surfacing calendar sync failures to the caller (e.g. a partial-success return or a warning toast).
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { | ||
| console.log("Error sending AddGuestsEmails"); |
There was a problem hiding this comment.
🟡 Non-blocking — Swallowed email errors and misleading success state
catch (err) { console.log("Error sending AddGuestsEmails"); } binds err but never logs it — only a static string is printed. The mutation returns { message: "Guests added" } regardless, so the client shows a success toast (showToast(t("guests_added"), "success")) even if zero guest emails were actually sent. Partial email delivery failures are invisible to both the user and operators.
Additionally, console.log is used instead of the repo's structured logger (logger from @calcom/lib/logger, used in editLocation.handler.ts and requestReschedule.handler.ts).
| : [], | ||
| seatsPerTimeSlot: booking.eventType?.seatsPerTimeSlot, | ||
| seatsShowAttendees: booking.eventType?.seatsShowAttendees, | ||
| }; |
There was a problem hiding this comment.
🟡 Non-blocking — evt.team is never set, so team-member notification emails are never sent
The CalendarEvent object built here (lines 124–147) never includes a team field, and formatCalEvent (email-manager.ts:526) does not derive one. Consequently, in sendAddGuestsEmails (email-manager.ts:531), calendarEvent.team?.members is always undefined and the team-member notification loop is unreachable — team members never receive the OrganizerAddGuestsEmail for team event-type bookings. The PR adds a team-member notification path that is dead given this evt construction.
Fix: populate evt.team from booking.eventType.team (with members) before calling sendAddGuestsEmails, or fetch team members in the handler.
| utils.viewer.bookings.invalidate(); | ||
| }, | ||
| onError: (err) => { | ||
| const message = `${err.data?.code}: ${t(err.message)}`; |
There was a problem hiding this comment.
🟡 Non-blocking — t(err.message) translates a raw error string; fallback is dead code
const message = \${err.data?.code}: ${t(err.message)}`treats the raw tRPC errormessageas a translation key. For errors like"booking_not_found"this works (the key exists), but for"you_do_not_have_permission"the key is **missing** fromcommon.json— i18next returns the raw key string, so the user seesFORBIDDEN: you_do_not_have_permission. The tRPC error code (FORBIDDEN, BAD_REQUEST`) is also leaked to the end user.
Additionally, showToast(message || t("unable_to_add_guests"), "error") — message is always truthy (it's a template string, even if empty), so the t("unable_to_add_guests") fallback is dead code.
Fix: use a safe mapping from known error codes/messages to user-facing translation keys, and keep the fallback reachable.
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
||
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { | ||
| throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" }); |
There was a problem hiding this comment.
🟡 Non-blocking — you_do_not_have_permission is not a locale key
The handler throws TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" }), but you_do_not_have_permission does not exist in apps/web/public/static/locales/en/common.json (confirmed via grep). The dialog's onError calls t(err.message) which returns the raw key string to the user. Either add the key to the locale file or use an existing permission-related key.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Add additional guests to a booking
Problem
This PR (fixes #14691, #15663) lets organizers, team admins/owners, and attendees add additional guests to an existing booking after creation. It adds a viewer.bookings.addGuests tRPC mutation, an AddGuestsDialog, a reusable MultiEmail input, organizer/attendee email templates, and calendar-event attendee sync.
Solution Reviewed
addGuests.handler.ts loads the booking, authorizes via organizer / team-admin-or-owner / attendee checks, dedups guests against existing attendees and a BLACKLISTED_GUEST_EMAILS env var, creates Attendee rows via createMany, builds a CalendarEvent, calls EventManager.updateCalendarAttendees, and sends notification emails via sendAddGuestsEmails. The UI wires a dialog from BookingListItem using a lazy-loaded MultiEmail component; the emails reuse the existing OrganizerScheduledEmail/AttendeeScheduledEmail classes via thin subclasses.
Summary
The feature is well-structured and follows existing patterns (lazy handler cache, email-template hierarchy, dialog wiring), but it has three blocking issues: an authorization logic bug (&& instead of || that denies team admins), no server-side cap on guest count (abuse/DoS), and a case-sensitive blacklist bypass. Several non-blocking issues around wrong-email-template routing, email case-insensitivity, missing team-member notifications, a missing i18n key, swallowed errors, and a React-key bug should also be addressed.
Files Reviewed
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts— deeply reviewedpackages/trpc/server/routers/viewer/bookings/addGuests.schema.ts— deeply reviewedpackages/trpc/server/routers/viewer/bookings/_router.tsx— lightly reviewed (handler wiring)apps/web/components/dialog/AddGuestsDialog.tsx— deeply reviewedpackages/ui/form/MultiEmail.tsx— deeply reviewedpackages/ui/form/MultiEmailLazy.tsx— lightly reviewed (matches repo Lazy pattern)packages/ui/index.tsx— lightly reviewed (export wiring)packages/emails/email-manager.ts— deeply reviewedpackages/emails/templates/organizer-add-guests-email.ts— deeply reviewedpackages/emails/templates/attendee-add-guests-email.ts— deeply reviewedpackages/emails/src/templates/OrganizerAddGuestsEmail.tsx— lightly reviewedpackages/emails/src/templates/AttendeeAddGuestsEmail.tsx— lightly reviewedpackages/emails/src/templates/index.ts— lightly reviewed (exports + renderEmail names verified correct)apps/web/components/booking/BookingListItem.tsx— lightly reviewed (dialog wiring)apps/web/public/static/locales/en/common.json— lightly reviewed (locale keys)
Verification
- i18n keys — all new/referenced keys confirmed present in
common.jsonexceptyou_do_not_have_permission(missing — see inline finding). - Constructor signatures —
OrganizerAddGuestsEmail({ calEvent, teamMember })andAttendeeAddGuestsEmail(calEvent, attendee)match parent constructors;renderEmailnames match exports inpackages/emails/src/templates/index.ts. - Authorization semantics — verified against
isTeamAdmin/isTeamOwnersource inpackages/lib/server/queries/teams/index.ts. EventManager.updateAllCalendarEvents— confirmed it falls back to fetching the calendar credential from the DB byreference.credentialId, so the calendar update resolves the organizer's credential regardless of requester.- Typecheck/lint/tests — skipped: full
turborun on the monorepo is infeasible here. No new tests were added for the handler/schema/dialog (test gap noted below).
Issues Found
See inline comments for the 3 blocking and 6 non-blocking findings. Additional non-inline notes:
- Attendee-driven calendar mutation (design check): Any booking attendee can trigger
EventManager.updateCalendarAttendees, which (via the DB credential fallback) updates the organizer's connected calendar event with new attendees. This may be intended to keep the shared event in sync, but it lets a non-owner mutate the organizer's external calendar. Consider scoping calendar updates to the organizer/team-admin-or-owner path, or constructing theEventManagerfrom the organizer's credentials rather thanctx.user, so the privilege model is explicit. - Transactional consistency: The
prisma.booking.update(createMany) commits before the calendar update and email send.updateCalendarAttendeesruns outside the try/catch, so a calendar error throws after the DB already has the new attendees — leaving a partial state with no rollback. Consider running this in a transaction or making the calendar/email steps best-effort with a clearly surfaced partial-failure result. - Test coverage: No unit or e2e tests were added for the new
addGuestshandler, schema, or dialog. Given this is a mutating, authorization-sensitive endpoint, at least handler-level tests (authorized organizer / denied non-attendee / dedup / blacklist) would materially de-risk it.
Verdict
Recommend changes before merge — the three blocking issues (authorization && bug, missing guest-count cap, blacklist case bypass) must be fixed for the feature to work correctly and safely; the non-blocking issues should be addressed in this PR or a fast follow.
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && | ||
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); |
There was a problem hiding this comment.
🔴 Authorization bug: && should be ||. isTeamAdmin already returns truthy for ADMIN or OWNER, while isTeamOwner is true only for OWNER (see packages/lib/server/queries/teams/index.ts). Combining them with && makes isTeamAdminOrOwner true only for owners, so a team ADMIN (not also owner) is denied with FORBIDDEN unless they separately qualify as organizer/attendee — silently stripping admins of an intended capability.
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) ||
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), |
There was a problem hiding this comment.
🔴 No server-side cap on the guests array (abuse/DoS). z.array(z.string().email()) has no .max(), and the server schema has no uniqueness refine (only the client dialog does). For each guest the handler inserts an Attendee row, fans out emails to the organizer + every existing attendee (+ team members), and makes external calendar API calls. Any authorized user — including a plain booking attendee — can submit thousands of emails in one request (DB write / email-blast / calendar-quota exhaustion). Client-side validation is trivially bypassed.
guests: z.array(z.string().email()).min(1).max(10),Consider also enforcing a total-attendee cap per booking before createMany.
| const uniqueGuests = guests.filter( | ||
| (guest) => | ||
| !booking.attendees.some((attendee) => guest === attendee.email) && | ||
| !blacklistedGuestEmails.includes(guest) |
There was a problem hiding this comment.
🔴 Blacklist bypass via case. blacklistedGuestEmails is built with .toLowerCase(), but guest is compared without lowercasing: !blacklistedGuestEmails.includes(guest). zod's .email() does not normalize case, so Bad@Example.com slips past a blacklisted bad@example.com, defeating the control via trivial case variation. Lowercase guest before all comparisons (and store consistently):
const guestLc = guest.toLowerCase();
return !booking.attendees.some((a) => a.email.toLowerCase() === guestLc) && !blacklistedGuestEmails.includes(guestLc);| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); |
There was a problem hiding this comment.
Passing raw guests (not uniqueGuests) sends the wrong email template. newGuests here is the unfiltered request input. In email-manager.ts, newGuests.includes(attendee.email) selects AttendeeScheduledEmail ("You're booked") vs AttendeeAddGuestsEmail. An existing attendee whose email was redundantly included in the request will receive the full "You're booked" email again. Pass the filtered list, and compare case-insensitively:
await sendAddGuestsEmails(evt, uniqueGuests);|
|
||
| const isOrganizer = booking.userId === user.id; | ||
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); |
There was a problem hiding this comment.
Case-sensitive email comparisons. attendee.email === user.email (here) and guest === attendee.email (line 76) are exact matches. Emails are effectively case-insensitive; a session/guest email with different casing is wrongly denied (FORBIDDEN) or not deduped, producing duplicate Attendee rows for the same person. Compare lowercased values:
const isAttendee = !!booking.attendees.find((a) => a.email.toLowerCase() === user.email.toLowerCase());| const emailsToSend: Promise<unknown>[] = []; | ||
| emailsToSend.push(sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent }))); | ||
|
|
||
| if (calendarEvent.team?.members) { |
There was a problem hiding this comment.
Team members are never notified — evt has no team. This branch is gated on calendarEvent.team?.members, but the CalendarEvent built in addGuests.handler.ts (~line 124) never sets a team property, and formatCalEvent doesn't synthesize one. So for team/managed event-type bookings, co-organizers/team admins get no "guests added" email, unlike the organizer/attendee emails that are sent. Populate evt.team (name + members) from booking.eventType.team when present, or pass team members to sendAddGuestsEmails explicitly.
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
||
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { | ||
| throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" }); |
There was a problem hiding this comment.
Missing i18n key. This FORBIDDEN branch throws message: "you_do_not_have_permission", and AddGuestsDialog.onError renders t(err.message). That key does not exist in apps/web/public/static/locales/en/common.json (verified), so i18next returns the raw key and the user sees FORBIDDEN: you_do_not_have_permission. Add the key to common.json (and the Crowdin source), or reuse an existing permission-denied key.
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { | ||
| console.log("Error sending AddGuestsEmails"); |
There was a problem hiding this comment.
Email-send failure is silently swallowed. catch (err) { console.log("Error sending AddGuestsEmails"); } discards err, uses console.log instead of the repo logger, and the mutation still returns success — so delivery/templating failures are invisible and unretryable. Note also that updateCalendarAttendees (line 165) runs outside this try/catch, so a calendar error throws after the DB createMany already committed (partial state, no rollback). Log with the structured logger and surface partial failure:
} catch (err) {
logger.error("Error sending AddGuestsEmails", { bookingId, err });
}| </label> | ||
| <ul> | ||
| {value.map((field, index) => ( | ||
| <li key={index}> |
There was a problem hiding this comment.
key={index} breaks input state on middle-item removal. Rows are keyed by array index while removal uses splice(index, 1). Removing a middle row shifts the indices, so React reuses the wrong DOM node for each subsequent row — typed text/focus/selection jumps to the wrong field. Use a stable id per entry (e.g. a counter or crypto.randomUUID() when pushing) as the key instead of the array index.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Add additional guests to a booking
Problem
This PR (fixes #14691, #15663) lets organizers, team admins/owners, and attendees add additional guests to an existing booking after creation. It adds a viewer.bookings.addGuests tRPC mutation, an AddGuestsDialog, a reusable MultiEmail input, organizer/attendee email templates, and calendar-event attendee sync.
Solution Reviewed
addGuests.handler.ts loads the booking, authorizes via organizer / team-admin-or-owner / attendee checks, dedups guests against existing attendees and a BLACKLISTED_GUEST_EMAILS env var, creates Attendee rows via createMany, builds a CalendarEvent, calls EventManager.updateCalendarAttendees, and sends notification emails via sendAddGuestsEmails. The UI adds a menu item to BookingListItem and a dialog with a MultiEmail input.
Summary
The feature is well-structured and follows existing handler/email conventions, but the authorization check has a logic error (&& instead of ||) that breaks the team-admin path, and sendAddGuestsEmails is called with the raw input instead of the filtered list, sending the wrong email to existing attendees. Several data-integrity and error-handling gaps should also be addressed before merge.
Files Reviewed
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts— deeply reviewed (new, high-risk: auth + data mutation)packages/trpc/server/routers/viewer/bookings/addGuests.schema.ts— deeply reviewed (new, trust boundary)packages/trpc/server/routers/viewer/bookings/_router.tsx— lightly reviewed (router wiring, follows existing pattern)apps/web/components/dialog/AddGuestsDialog.tsx— deeply reviewed (new, user-facing logic)packages/ui/form/MultiEmail.tsx— deeply reviewed (new, reusable component)packages/ui/form/MultiEmailLazy.tsx— lightly reviewed (6-line lazy wrapper)packages/ui/index.tsx— lightly reviewed (one export line)packages/emails/email-manager.ts— deeply reviewed (email routing logic)packages/emails/src/templates/AttendeeAddGuestsEmail.tsx— lightly reviewed (thin wrapper)packages/emails/src/templates/OrganizerAddGuestsEmail.tsx— lightly reviewed (thin wrapper)packages/emails/src/templates/index.ts— lightly reviewed (two exports)packages/emails/templates/attendee-add-guests-email.ts— lightly reviewed (email payload)packages/emails/templates/organizer-add-guests-email.ts— lightly reviewed (email payload)apps/web/components/booking/BookingListItem.tsx— lightly reviewed (menu item + dialog mount)apps/web/public/static/locales/en/common.json— lightly reviewed (translation keys)
Verification
tsc --noEmit -p packages/trpc/tsconfig.json— passed (no type errors in addGuests files)isTeamAdmin/isTeamOwnersource inspected (lib/server/queries/teams/index.ts:264-292) — confirmsisTeamAdminalready matches ADMIN or OWNERAttendeeprisma model inspected (schema.prisma:526-539) — confirms NO@@unique([bookingId, email]), only indexes- Translation keys checked against
common.json—you_do_not_have_permissionis absent; all other keys exist - No test files exist for
addGuestsor any booking handler (test coverage gap, noted below)
Issues Found
See inline comments. 2 blocking, 9 non-blocking, 1 suggestion.
Verdict
Recommend changes before merge — the && auth logic and the sendAddGuestsEmails(evt, guests) call are blocking correctness bugs that break the feature for the documented use cases.
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); | ||
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && |
There was a problem hiding this comment.
Blocking — authorization logic error. isTeamAdminOrOwner uses &&, but isTeamAdmin already returns truthy for both ADMIN and OWNER (lib/server/queries/teams/index.ts:266-270: OR: [{role:"ADMIN"},{role:"OWNER"}]), while isTeamOwner only matches OWNER. So isTeamAdmin && isTeamOwner reduces to owner-only — a team ADMIN (non-owner) is denied the ability to add guests, contradicting the variable name and the documented "team admins/owners" intent. This fails closed (over-restrictive, not a bypass), but the feature is broken for admins.
// isTeamAdmin already covers ADMIN+OWNER; use || or just isTeamAdmin:
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) ||
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); |
There was a problem hiding this comment.
Blocking — wrong email sent to existing attendees. sendAddGuestsEmails(evt, guests) passes the raw input guests, not uniqueGuests (computed at line 74). In email-manager.ts:541-547, newGuests.includes(attendee.email) decides whether each attendee gets AttendeeScheduledEmail (a full "your event has been scheduled" confirmation) or AttendeeAddGuestsEmail. Because the raw array still contains emails filtered out as already-existing attendees, an existing attendee who was re-listed is misrouted to AttendeeScheduledEmail and receives a fresh booking confirmation instead of the "guests added" notification.
// Pass the actually-added guests, not the raw input:
await sendAddGuestsEmails(evt, uniqueGuests);| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | ||
| : []; | ||
|
|
||
| const uniqueGuests = guests.filter( |
There was a problem hiding this comment.
Blacklist bypass via case-sensitivity. blacklistedGuestEmails entries are .toLowerCase()'d (line 71), but the comparison blacklistedGuestEmails.includes(guest) uses the un-normalized guest. So Bad@Evil.com passes z.string().email() validation and evades a blacklisted bad@evil.com. The same case-sensitivity affects the existing-attendee dedup at line 76 (guest === attendee.email), so a case-variant of an existing attendee is inserted as a duplicate. Normalize the guest email before both comparisons.
const uniqueGuests = guests
.map((g) => g.trim().toLowerCase())
.filter(
(guest, i, arr) =>
arr.indexOf(guest) === i &&
!booking.attendees.some((a) => guest === a.email.toLowerCase()) &&
!blacklistedGuestEmails.includes(guest)
);|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), |
There was a problem hiding this comment.
No server-side uniqueness or length cap. The schema is z.array(z.string().email()) with no .refine for uniqueness and no .max(). The handler's uniqueGuests only dedups against existing attendees, not within the input array — so a direct tRPC call with ["a@x.com","a@x.com"] inserts two identical Attendee rows (the model has no @@unique([bookingId, email]), so the DB doesn't reject them). The frontend enforces uniqueness, but the server is the trust boundary. Add a uniqueness refinement and a cap to bound createMany / email fan-out.
export const ZAddGuestsInputSchema = z.object({
bookingId: z.number(),
guests: z
.array(z.string().email())
.max(25, "too_many_guests")
.refine(
(emails) => new Set(emails.map((e) => e.toLowerCase())).size === emails.length,
{ message: "emails_must_be_unique_valid" }
),
});|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { |
There was a problem hiding this comment.
Swallowed error + misleading success state. The catch (err) binds err but never uses it — only console.log("Error sending AddGuestsEmails") runs, and the handler still returns { message: "Guests added" }. By this point the DB rows are committed and calendar attendees updated, so the user sees a success toast while notification emails silently failed (a real failure mode: transient SMTP errors, rejected addresses). The failure is undebuggable because err is never logged. Use the codebase's structured logger (@calcom/lib/logger) and consider surfacing partial success.
import logger from "@calcom/lib/logger";
// ...
try {
await sendAddGuestsEmails(evt, uniqueGuests);
} catch (err) {
logger.error("Error sending AddGuestsEmails", { bookingId, err });
}| throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" }); | ||
| } | ||
|
|
||
| const organizer = await prisma.user.findFirstOrThrow({ |
There was a problem hiding this comment.
Raw Prisma error leak on missing/null organizer. prisma.user.findFirstOrThrow({ where: { id: booking.userId || 0 } }) throws a raw PrismaClientKnownRequestError (P2025) when no user matches — e.g. booking.userId is null (the User? relation is nullable) or the organizer user was deleted. Unlike the explicit TRPCError at line 44, this raw Prisma error surfaces as an unhandled INTERNAL_SERVER_ERROR (500). The query is also redundant — booking.user is already eagerly loaded at the top findFirst. Use the loaded relation or findFirst + explicit guard.
if (!booking.user) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
const organizer = {
name: booking.user.name,
email: booking.userPrimaryEmail ?? booking.user.email,
timeZone: booking.user.timeZone,
locale: booking.user.locale,
};| onSuccess: async () => { | ||
| showToast(t("guests_added"), "success"); | ||
| setIsOpenDialog(false); | ||
| setMultiEmailValue([""]); |
There was a problem hiding this comment.
Stale validation error persists across dialog open/close. isInvalidEmail is set to true on validation failure but never reset — not in onSuccess, not on close, not at the start of handleAdd. Since the dialog is always mounted in BookingListItem, a failed validation leaves the red "Emails must be unique and valid" banner visible when the user reopens the dialog on a fresh empty field. Reset it on success and before re-validating.
onSuccess: async () => {
showToast(t("guests_added"), "success");
setIsOpenDialog(false);
setMultiEmailValue([""]);
setIsInvalidEmail(false); // add this
utils.viewer.bookings.invalidate();
},
// and at the top of handleAdd:
const handleAdd = () => {
setIsInvalidEmail(false);
// ...
};| addOnSuffix={ | ||
| !readOnly ? ( | ||
| <Tooltip content="Remove email"> | ||
| <button className="m-1" type="button"> |
There was a problem hiding this comment.
a11y: remove-email button has no accessible name. The icon-only <button> inside the Tooltip has no aria-label or sr-only text, so screen readers announce only "button". The PasswordField in the same package handles this correctly with <span className="sr-only">{textLabel}</span>. Add an accessible name.
<button className="m-1" type="button" aria-label={t("remove_email")}>
<Icon name="x" width={12} className="text-default" />
</button>(Also add a remove_email key to common.json.)
|
|
||
| const guestsFullDetails = uniqueGuests.map((guest) => { | ||
| return { | ||
| name: "", |
There was a problem hiding this comment.
New guests get name: "" with no fallback. Downstream the email to header becomes <email> (empty display name) and the ICS ATTENDEE gets an empty CN=. This is inconsistent with the codebase convention — handleNewBooking.ts:816 uses fullName || "Nameless", and this same handler uses organizer.name ?? "Nameless" at line 132. Derive a fallback from the email local-part.
const guestsFullDetails = uniqueGuests.map((guest) => ({
name: guest.split("@")[0] || "Nameless",
email: guest,
timeZone: organizer.timeZone,
locale: organizer.locale,
}));| const { user } = ctx; | ||
| const { bookingId, guests } = input; | ||
|
|
||
| const booking = await prisma.booking.findFirst({ |
There was a problem hiding this comment.
No booking-status guard. The findFirst (line 26) has no status filter and the only guard is !booking. So a direct tRPC call with a CANCELLED/REJECTED/AWAITING_HOST booking ID passes authorization, inserts new attendees, fires "guests added" / "scheduled" notification emails to new guests for an event that no longer exists, and calls updateCalendarAttendees on a cancelled calendar event. (editLocation.handler also omits a status check, so the omission is convention-consistent — but the misleading-email consequence is specific to addGuests.) Restrict to live bookings:
const booking = await prisma.booking.findFirst({
where: {
id: bookingId,
status: { in: [BookingStatus.ACCEPTED, BookingStatus.PENDING] },
},
// ...
});
|
8 participants
What does this PR do?
Fixes #14691
Fixes #15663
Bookings._.Cal.com.and.6.more.pages.-.Personal.-.Microsoft_.Edge.2024-04-30.03-39-02.mp4