fix: respect hideCalendarNotes setting during booking reschedule#25945
fix: respect hideCalendarNotes setting during booking reschedule#25945pedroccastro merged 14 commits intomainfrom
hideCalendarNotes setting during booking reschedule#25945Conversation
Remove description reset lines that were overriding the hideCalendarNotes setting after calendar events were created/updated. The CalendarEventBuilder already properly sets hideCalendarNotes, and the CalendarManager respects this flag by replacing additionalNotes with 'Notes have been hidden by the organizer'. The description reset was incorrectly restoring the original notes, causing them to become visible again in ICS files after rescheduling. This fix removes three description reset lines: - After eventManager.reschedule() in the reschedule flow - After the reschedule video call details extraction - After eventManager.create() in the new booking flow Co-Authored-By: rajiv@cal.com <sahalrajiv6900@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Ryukemeister
changed the title
hideCalendarNotes setting during booking reschedule
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/bookings/lib/service/RegularBookingService.ts">
<violation number="1" location="packages/features/bookings/lib/service/RegularBookingService.ts:2050">
P1: This reset at line 2050 happens BEFORE `eventManager.reschedule()`, which means the calendar events will be created with the original description (notes visible) rather than the hidden notes. According to the PR description, the fix should remove resets that override hidden notes, not add a new one before the reschedule call. Additionally, there's still a reset at line 2071 after the reschedule flow that should likely be removed per the PR goals.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| "add_input": "Add an Input", | ||
| "disable_notes": "Hide notes in calendar", | ||
| "disable_notes_description": "For privacy reasons, additional inputs and notes will be hidden in the calendar entry. They will still be sent to your email. <0>Learn more</0>", | ||
| "notes_hidden_by_organizer": "Notes have been hidden by the organizer", |
There was a problem hiding this comment.
this is not being anywhere right now, its for later
| !shouldSkipAttendeeEmailWithSettings(eventTypeMetadata, organizationSettings, EmailType.RESCHEDULED) | ||
| ) { | ||
| emailsAndSMSToSend.push(sendEmail(() => new AttendeeRescheduledEmail(calendarEvent, person))); | ||
| emailsAndSMSToSend.push( |
There was a problem hiding this comment.
this is not a breaking change or change in logic, it was simply missing for the rescheduling logic. if you checkout line 395 of email manager here we were already implementing this when create a new booking, it was simply missing when rescheduling
| () => | ||
| new AttendeeRescheduledEmail( | ||
| { | ||
| ...calendarEvent, |
There was a problem hiding this comment.
same comment as above, missing logic
| ): Promise<EventResult<NewCalendarEventType>> => { | ||
| const formattedEvent = formatCalEvent(rawCalEvent); | ||
|
|
||
| if (formattedEvent.hideCalendarNotes) { |
There was a problem hiding this comment.
this is again not a logic change or breaking change, it was simply missing for rescheduling logic. if you checkout line 308 of the same function here its already present for create booking, it was simply missing for rescheduling
pedroccastro
left a comment
pedroccastro
left a comment
There was a problem hiding this comment.
Hey @Ryukemeister, nice work on this! The core fix looks solid 👍
One thing: sendRescheduledSeatEmailAndSMS (line ~380) doesn't have the hideCalendarNotes handling yet, but the other reschedule functions do. Should we add the same pattern there for consistency?
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
| } | ||
| // This gets overridden when updating the event - to check if notes have been hidden or not. We just reset this back | ||
| // to the default description when we are sending the emails. | ||
| evt.description = eventType.description; |
There was a problem hiding this comment.
why not eventType.description ?? evt.description like before?
There was a problem hiding this comment.
because evt.description comes the description property in the Booking table and that is where we store the additional notes (even tho the name of the field is description). so say if there's an event type with no description in that eventType.description will be undefined and hence it will fallback to evt.description which is the additional notes. as a result we will end up having additional notes twice.
hey Pedro, nice catch. ideally we should add it there as well, let me check and see if i've missed it. thanks! |
|
hey @pedroccastro, implemented the feedback that you gave. should be good now, thanks! |
pedroccastro
left a comment
pedroccastro
left a comment
There was a problem hiding this comment.
Hey @Ryukemeister! The changes look solid 👍
The updateEvent() change mirrors the existing pattern in createEvent() and the email handling covers all three reschedule functions
For a potential follow-up:
sendAddGuestsEmails / sendAddGuestsEmailsAndSMS don't have the hideCalendarNotes check for new guests. Not blocking, but flagging as they affect the same feature in other flows
pedroccastro
deleted the
devin/fix-hide-calendar-notes-reschedule-1765911722
branch
3 participants
What does this PR do?
Fixes an issue where the
hideCalendarNotessetting was not being respected during booking rescheduling in API v2. When rescheduling an event withhideCalendarNotes: true, the notes were incorrectly becoming visible again in the ICS file.Root cause: After the
CalendarManagerproperly hides notes by replacingadditionalNoteswith "Notes have been hidden by the organizer", the code was resettingevt.descriptionback toeventType.description, which contains the original notes. This reset was happening after calendar events were already created/updated.Fix: Remove the three description reset lines that were overriding the hidden notes:
eventManager.reschedule())eventManager.create())The removed code comments claimed the reset was "for sending emails", but email templates use
evt.additionalNotes, notevt.description, so this reset was unnecessary and harmful.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
hideCalendarNotes: trueHuman Review Checklist
additionalNotesand notdescriptionfor rendering notesevt.descriptionbeing reset after calendar event creationhideCalendarNotes+ reschedule scenarioLink to Devin run: https://app.devin.ai/sessions/186c0904faad49b8a2944d2f71fea937
Requested by: rajiv@cal.com (@Ryukemeister)