fix(appointments): gate Manage Timings on commitment, and give group events a real Unschedule - #1083
Conversation
Manage Timings writes new times with no notice requirement and no acceptance, which is honest only while nobody else holds the time. The consultant menu offered it for any non-cancelled, non-past booking, so a consultation or subscription session a consultee had paid for could be moved out from under them. Adds `allowsManageTimings` next to `slotsAllowReschedule`: group events and anything unallocated keep the surface, a 1:1 with a confirmed slot loses it and gets the negotiated Reschedule instead. The two are now complements, so a booking never shows both. The page enforces the same rule after its ownership check, because the URL is linkable. Closes #1082
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
✅ Deploy Preview for familiarise ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesAppointment timing actions
Reschedule notification outcomes
Notification UI and response metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Consultant
participant ConsultantAppointmentsAdapter
participant useConsultantEventActions
participant RescheduleRoute
Consultant->>ConsultantAppointmentsAdapter: select Unschedule
ConsultantAppointmentsAdapter->>useConsultantEventActions: call handleUnschedule
useConsultantEventActions->>RescheduleRoute: post unschedule request
RescheduleRoute-->>useConsultantEventActions: return action result
sequenceDiagram
participant RescheduleRoute
participant rescheduleNotificationVariant
participant NotificationService
RescheduleRoute->>RescheduleRoute: capture releasedAt and proposedAt
RescheduleRoute->>rescheduleNotificationVariant: classify outcome
rescheduleNotificationVariant-->>RescheduleRoute: return outcome fields
RescheduleRoute->>NotificationService: send rescheduled payload
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
Gating Manage Timings on whether anyone has committed to a time also took Reschedule off webinars and classes. That was right — for a group event that route never opened a proposal, it only marked every slot tentative and handed the instance back to the allocate queue — but it removed a real capability along with the wrong label. Adds `allowsUnschedule` beside the other two predicates. It is orthogonal to them, not a third branch: a confirmed webinar offers Timings AND Unschedule, a 1:1 offers Reschedule and never Unschedule, and the "never both, never neither" property still holds within the Timings/Reschedule pair. The action routes at the existing reschedule endpoint rather than a parallel implementation, so the behaviour is unchanged and only its name, its gate and its confirm copy are new. Unschedule is emphatically not Cancel. The booking stays sold, attendees stay enrolled, no refund is issued and no earnings, ledger row or utilisation figure moves; the only thing withdrawn is the date. The confirm dialog says all of that plainly and points at Cancel booking for the other outcome, because for a webinar with thirty paid attendees the difference is a new date versus refunding all of them. Attendee notification was already correct on this path: the route's post-transaction fan-out adds every user connected to the event's slots (#624), and a group buyer is connected to every slot of every session, so enrolled attendees are told the date has been withdrawn. Part of #1082
…m to" The consultant's inbox rendered "rescheduled ... from to". The payload type declared `oldDateTime?` and `newDateTime?`, the reschedule route passed neither, and the template interpolated both anyway. Passing two values would not have fixed it, because the sentence is wrong for the common case. A plain release has no destination — that is the entire point of "Any time works": the slots go back to the consultant's queue and no new time exists yet. Only an auto-confirmed proposal actually moved anything, and between those two sits a third case the route already distinguishes in its own response message, a proposal that has been made and is waiting to be answered. So the payload now carries `outcome`, one of MOVED, PROPOSED or RELEASED, and the two arms are a union rather than optional fields: MOVED and PROPOSED cannot be constructed without both times, which makes the blank-blank payload a compile error rather than a rendering accident. This is the idiom the org workflows already use — `reminderStage` on the dunning notice and `kind` on the payout failure both drive their copy from one workflow id. `rescheduleNotificationVariant` in the policy module derives the variant, so the route stays a caller and the rule is unit-testable. The released time is captured inside the transaction because an auto-confirm deletes those slot rows and writes new ones — by the time the notification is assembled the time being given up exists nowhere else. The template itself lives in Novu's dashboard and cannot be changed from here. Until it branches on `outcome`, the moved case renders real times where it rendered blanks, and the released case still renders the wrong sentence. The Novu-side change is spelled out in the PR body. An audit of every optional field on every payload in lib/novu found the same shape once more: `SupportTicketPayload.respondedBy` is declared and never passed, so a template naming the responder rendered an empty attribution. It is passed now. Three more are declared and never passed but need a query or a loop change to supply, and are listed in the PR body rather than fixed here.
The notification panel covered the Friday and Saturday columns of the calendar at a normal laptop width. Two things put it there. It was Novu's own bundled popover, fixed at 400px on a bespoke z-index of 9999 with no collision handling; and its open state belonged to Novu, so clicking a notification routed the user to the page the notification was about and then stayed open on top of it. The panel a consultant saw over their calendar was usually the one that had just sent them there. Given children, `Inbox` drops to being a provider — `Bell` and `InboxContent` are the composition parts — so the panel becomes ours to place. It is now the repo's Radix popover from components/ui, which brings collision-aware placement, a viewport-capped size, Escape and outside-click dismissal, and the shared z-layer instead of a number picked to beat everything else on the page. Because the open state is ours, the panel closes before routing. It is also narrower: 22rem against the old 25rem, capped at the viewport on both axes, which leaves more of the grid legible while it is open. Presentational only. The subscriber, the ADR 23 scope tabs, the appearance variables and the click-to-route behaviour are unchanged; the two appearance keys that styled Novu's popover are dropped because that popover no longer renders, and `bellContainer` with them, since a custom `renderBell` has always bypassed the container it styled.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/appointments/slots.ts`:
- Around line 62-90: Update allowsManageTimings so CONSULTATION and SUBSCRIPTION
return false when any slot is confirmed, rather than checking only
slots[0]?.isTentative; preserve the existing empty-slot and WEBINAR/CLASS
behavior. Add coverage for slots ordered with a tentative or RESCHEDULED slot
before a later confirmed slot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a9c9082-9f15-4a0a-b3fb-0047174c5027
📒 Files selected for processing (12)
__tests__/booking-algorithm/manage-timings-affordance.test.ts__tests__/booking-algorithm/reschedule-proposals.test.tsapp/api/appointments/[appointmentId]/reschedule/route.tsapp/api/staff/support-tickets/[ticketId]/responses/route.tsapp/dashboard/consultant/[consultantId]/(features)/appointments/ConsultantAppointmentsAdapter.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/[appointmentId]/timings/page.tsxapp/dashboard/consultant/[consultantId]/(features)/appointments/components/useConsultantEventActions.tscomponents/appointments/UnscheduleConfirmationDialog.tsxcomponents/notifications/NotificationInbox.tsxlib/appointments/slots.tslib/booking/reschedule-proposals.tslib/novu/workflows.ts
A partial reschedule releases one session of a multi-session booking and leaves the rest confirmed. The released slot can sort first, so reading only `slots[0].isTentative` saw "tentative", concluded nobody had committed, and handed the consultant Manage Timings — the unilateral surface — for a booking whose later sessions the consultee still holds. The existing in-flight test happened to put the released slot second, which is why it passed. The new case puts it first, and fails against the old predicate. Part of #1082
#1067 rewrote slotTimes to take the looser SessionSlotLike so the join surfaces can pass planner rows and MeetingSlot; that signature wins. The three affordance helpers this branch adds are additive alongside it. SlotLike is no longer imported: its only use here was the slotTimes signature #1067 replaced, and an unused import fails Sonar. Co-authored-by: Cursor <cursoragent@cursor.com>
|




A consultee books a consultation, pays for it, and puts the time in their diary. The consultant then opens Manage Timings on that booking, picks a different slot, and saves. There is no notice period, no request, and nothing for the consultee to accept — the session simply moves, and they find out when their calendar changes. The same is true of a subscription session part-way through a programme.
The cause is one line in the wrong place, not a wrong policy. The
MANAGE_TIMINGSpicker setsminLeadHours: 0and its own comment explains why: the surface has no counterparty to give notice to. That is entirely true of what it was built for, which is a consultant deciding when their own webinar or class instance runs. The consultant appointments menu simply never made the distinction, and offered the action for every appointment in a non-cancelled, non-past bucket.This PR fixes that, and in doing so gives the consultant's menu three named actions where it previously had two, one of which was wearing the wrong label.
The model — three actions, three predicates
The deciding question for the first two is whether anyone has already committed to a time, not who owns the calendar.
Manage Timings stays wherever nobody has committed. A webinar or class instance keeps it unconditionally, including once its slots are confirmed: the organiser publishes a schedule and attendees buy into it, so there is no counterparty to negotiate with and asking thirty attendees to each accept a new time is not a coherent flow. An offering that has never been scheduled keeps it, since it has no
Appointmentrow at all and travels as anunscheduled-class-<id>/unscheduled-webinar-<id>synthetic id. A booking whose slots are still tentative keeps it too, because a tentative slot means the request is awaiting allocation and nothing has been placed yet.Reschedule replaces it wherever a counterparty holds a confirmed time. That means a consultation or a subscription session with allocated, non-tentative slots. Moving one of those is a two-party negotiation, so it goes through the proposal the consultee has to answer. It is 1:1 only.
Unschedule is new, and it is orthogonal to that pair rather than a third branch of it. It pulls a placed group event off the calendar and back into the allocate queue without cancelling it, so a confirmed webinar offers Manage Timings and Unschedule, while a 1:1 offers neither of those and gets Reschedule instead. The "never both, never neither" property from the original change still holds, unchanged, within the Timings/Reschedule pair.
Trials follow the same rule and land on the same side as a consultation. A trial is strictly one consultant and one consultee, and although the consultant is the one who allocates the time, the slot is created with
isTentative: falseand the consultee is notified of it immediately — so it is a commitment by exactly the test above. This changes nothing in practice: the menu already withheld Timings from trials because a trial VM carries noraw.appointment, andreadManageTimingsTargetalready returnednullfor aTRIALappointment type. The predicate now says so explicitly rather than leaving it to those two accidents.Unschedule is not Cancel, and a reviewer should not read it as one
This is the distinction the whole second half of the PR exists to draw. The two actions sit one menu row apart and undo wildly different amounts.
CANCELLED, terminalUnschedule means "this is still happening, I just do not know when yet." For a webinar with thirty paid attendees, that is the difference between moving the date and refunding ₹150,000. Nothing on this path touches money, earnings, the ledger or utilization, and the confirm dialog says so in as many words before naming Cancel booking as the action for the other outcome. It is deliberately not styled destructive, because a red button would read as "refund everyone".
Why the capability already existed
The first commit's removal of Reschedule from webinars and classes was correct, but it removed a real capability along with a wrong label. For a group event that route never opened a proposal — its own docstring says "For WEBINAR/CLASS: Marks all slots as tentative", and the auto-confirm block notes that "Only the two 1:1 kinds carry proposals". Marking every slot tentative and re-stamping the event
SCHEDULEDis an unschedule. It was an Unschedule wearing the Reschedule label.So
handleUnscheduleposts to the samePOST /api/appointments/[appointmentId]/reschedulewith no body, exactly as the group-event Reschedule menu item used to. There is no parallel implementation, no new route and no new transaction. What is new is the name, the predicate that decides when to offer it, and copy that tells the truth about what it does. Thetypequery param is deliberately omitted: the route derives the type from the DB and only compares when one is supplied, so omitting it cannot mismatch.Attendee notification — already correct, and confirmed
A sibling audit found that group-event cancellation notifies nobody (being fixed separately in #1081), so the same gap was expected here. It is not present. The reschedule route's post-transaction fan-out re-reads the appointment with
slotsOfAppointment: { select: { user: … } }and pushes every connected user into the recipient list before callingnotifyAppointmentRescheduled— the "FIX #624" block. Attendees are excluded only if they are the initiator.That is sufficient for classes as well as webinars, even though the notification is scoped to a single appointment while the release covers every session of the class. Group checkout never mints a per-buyer slot:
handleClassCheckoutconnects the buyer to every slot of every session appointment, andhandleWebinarCheckoutto every slot of the shared appointment. One appointment's slots therefore already carry the whole roster. No second recipient assembly was written.Why no new mechanism was needed for the 1:1 side
A reviewer will reasonably ask whether this should have shipped a reschedule-request feature. It should not, because the two-party protocol already exists and is already asymmetric in the right direction. A consultee proposing a time inside the consultant's published availability auto-confirms, since publishing availability is standing consent to be booked within it. A consultant proposing never auto-confirms, since a consultee merely being free at a time is not consent to be moved to it. The initiator may withdraw, the recipient may decline, and both decline and expiry drop the booking into the consultant's allocate queue, so nothing dead-ends. A consultant with a genuine emergency opens a proposal and the consultee accepts; if they never answer, expiry hands it back to the allocate queue; and cancellation with a refund remains available throughout. There is therefore no case that requires moving a committed session without agreement.
What changed
lib/appointments/slots.tsgainsallowsManageTimings(kind, slots)andallowsUnschedule(kind, slots), alongside theslotsAllowReschedulethe first is the complement of.allowsUnschedulereturns true only for aWEBINARorCLASSwith at least one placed (non-tentative) upcoming slot, which makes the action idempotent by construction rather than by a second guard — the release leaves every slot tentative, so an already-unscheduled event stops offering it and offers Manage Timings instead. The module also gainsupcomingSlots, the "still ahead of now, chronological" filter that the consultant adapter had inline, so the menu and the page decide on the same list: a finished session is not what "has someone committed to a time" is asking about, and without the filter a subscription with past sessions and unallocated future ones would read as committed.ConsultantAppointmentsAdapter.tsxgates the Timings item on the new predicate and adds its negation to the Reschedule item's condition, so those two are now mutually exclusive by construction rather than by coincidence. The one deliberate exception is a booking with a reschedule already in flight, where the released slot awaiting a new time is the open proposal — Reschedule would earn a 409 and Timings would write straight over the proposal the consultee is still answering, so neither is offered. There is a test naming that case. The adapter also gains the Unschedule item, gated on plan ownership (the samecanManageBookingLifecyclecheck Cancel and Reschedule use, so a collaborator who is not the HOST does not get it), a non-terminal status,isConfirmedStatus— which matches the route's own from-state ofSCHEDULED/IN_PROGRESSfor a group release — and the new predicate.components/appointments/UnscheduleConfirmationDialog.tsxis new: the confirm step described above.useConsultantEventActions.tsgainshandleUnschedule, the thin call onto the existing route, with a success toast that repeats the two things a consultant needs to be sure of — the event is back in their queue, and attendees stay enrolled and have been told.timings/page.tsxenforces the Manage Timings rule withnotFound()after the ownership check. The menu is not a control on its own: the URL is linkable and survives a refresh.generateMetadataalready gated the offering title onplanOwnerIds; that stays, and the counterparty gate joins it so a route that 404s never gets a titled tab either. Only the two 1:1 kinds need the extra slot read and only they pay for it, and it goes throughReact.cacheso metadata and body share one query. The slots are fetched in the page rather than widened intoManageTimingsTargetdeliberately, to stay clear of #1075, which is editing that file concurrently.Not in scope
Whether an attendee may refund out of a booking that has been unscheduled is an open question. If a consultant withdraws the date of a webinar someone paid for, it is arguable that the attendee should be able to take their money back rather than wait for a date that may not suit them. That is a consumer-protection decision rather than an engineering one, it has not been made, and nothing here builds toward either answer: there is no refund affordance on the unschedule path and no policy hook waiting for one. It needs a decision before this ships to consultants at scale.
The route's existing 24-hour floor applies to Unschedule unchanged, since it is the same route: an event starting within 24 hours cannot be unscheduled and the API returns a 400. The menu does not pre-empt that, exactly as it does not for Reschedule today, so the consultant meets it as an error rather than as a hidden action. Duplicating
MINIMUM_HOURS_BEFORE_RESCHEDULEinto the client to hide the item would put the policy in two places, which is worse.The proposal protocol, the auto-confirm asymmetry and the single round are all untouched, as is the
MANAGE_TIMINGSpolicy itself —minLeadHours: 0and "no counterparty" remain correct for the surface's real use. There is no schema change and no migration. The consultant's menu still offers a trial neither Timings nor Reschedule, because the reschedule API has noTRIALbranch and returns 403 for one; that gap predates this change. Group-event cancellation still notifies nobody; that is #1081, not this.Also in this PR — two bugs on the notification surface
Neither belongs to the Manage Timings story. Both were found while looking at it, this branch already owns the reschedule route that causes the first, and there are more open branches than is useful right now, so they ride here as two separate commits rather than as a third pull request.
A reschedule with no new time rendered "from to"
A reschedule notification reached the consultant's inbox with both of its times missing, reading "rescheduled … from to".
AppointmentRescheduledPayloaddeclaredoldDateTime?andnewDateTime?, the reschedule route passed neither, and the template interpolated both anyway.Passing two values would not have been the fix, because the sentence itself is wrong for the common case. A plain release has no destination — that is the entire point of "Any time works", where the released slots go back to the consultant's queue and no new time exists yet. Only an auto-confirmed proposal has actually moved anything. Between those two sits a third case the route already distinguishes in its own response message, which is a proposal that has been made and is still waiting for the other party to answer.
The payload now carries an
outcomeofMOVED,PROPOSEDorRELEASED, and its arms are a union rather than a bag of optional fields:MOVEDandPROPOSEDcannot be constructed without both times, so the blank-blank payload that produced the screenshot is a compile error rather than a rendering accident. One workflow id with a variant flag in the payload is the idiom the enterprise workflows already use, wherereminderStagedrives the dunning copy andkindseparates a rejected payout from a reversed one, so no second workflow id was minted.rescheduleNotificationVariantinlib/booking/reschedule-proposals.tsderives the variant, which keeps the route a caller and makes the rule unit-testable without a database. The released time is captured inside the transaction rather than read back afterwards, because an auto-confirm deletes those slot rows and writes new ones — by the time the notification is assembled, the time being given up exists nowhere else.What still has to change in Novu
The
appointment-rescheduledtemplate lives in the Novu dashboard and not in this repository, so this PR cannot fix the rendered sentence on its own. What it does is make the payload carry enough for the template to tell the three cases apart. The dashboard template has to branch on the discriminator:{% if payload.outcome == "MOVED" %} Your {{payload.appointmentType}} moved from {{payload.oldDateTime}} to {{payload.newDateTime}}. {% elsif payload.outcome == "PROPOSED" %} {{payload.consulteeName}} asked to move your {{payload.appointmentType}} from {{payload.oldDateTime}} to {{payload.newDateTime}}. {% else %} Your {{payload.appointmentType}} was released and is awaiting a new time. {% endif %}The
RELEASEDbranch must not referencenewDateTimeat all: the payload does not carry the key, and it is absent rather thanundefinedso that nothing serializes into the interpolation.oldDateTimeis present onRELEASEDtoo in every ordinary case and may be used as colour, but the branch must still read correctly without it, since a booking with no placed slots supplies none.Until that template change lands, the auto-confirmed case renders real times where it rendered two blanks, and the released case still renders the wrong sentence. Merging this alone does not close the bug.
The same shape elsewhere in
lib/novuA payload field that is declared optional and then passed by no caller is a pattern rather than a one-off, so every optional field on every payload in
lib/novu/workflows.tsandlib/novu/org-workflows.tswas checked against its call sites. Four fields are never passed by anyone.AppointmentRescheduledPayload.oldDateTimeandnewDateTimeare the bug above.SupportTicketPayload.respondedByis the same shape one subsystem over — a template naming the staff member who replied had nothing to name — and it is fixed here, since the responder is already loaded on the row the route just created.AppointmentPayload.dateTimeis missing fromnotifyAppointmentBookedin the payment webhook handler and from bothnotifyAppointmentCompletedcalls in the auto-complete sweep; neither is fixed here. The booked one needs a slot read added to a select that #734 deliberately trimmed, in a payments file another branch is currently editing, and the completed ones need the sweep's slot bookkeeping changed to keep the row rather than only its end time. The reminder path, which is the one where a missing date would be most obviously wrong, does pass it.Every other optional field is passed by at least one caller.
The Inbox panel sat on top of the calendar
The notification panel covered the Friday and Saturday columns of the slot grid entirely at a normal laptop width. Two things put it there. It was Novu's own bundled popover, fixed at 400px on a bespoke z-index of 9999 with no collision handling, and its open state belonged to Novu, so clicking a notification routed the user to the page that notification was about and then stayed open on top of it. The panel a consultant found over their calendar was usually the one that had just sent them there.
Given children, Novu's
Inboxdrops to being a provider and exposesBellandInboxContentas composition parts, which makes the panel ours to place. It is now the repo's Radix popover fromcomponents/ui/popover, so it gets collision-aware placement, a size capped against the viewport on both axes, Escape and outside-click dismissal, and the sharedz-[1200]layer instead of a number picked to beat everything else on the page. Because the open state is ours, the panel closes before routing. It is also narrower, at 22rem against the old 25rem, which leaves more of the grid legible while it is open.This is presentational and nothing about the notification data flow moved. The subscriber, the ADR 23 scope tabs, the appearance variables and the click-to-route behaviour are all unchanged. The two appearance keys that styled Novu's popover are dropped because that popover no longer renders, and
bellContainergoes with them, because a customrenderBellhas always bypassed the container it styled.Verification
tsc --noEmitis clean from a cold build (cache cleared, 8 GB heap).eslintis clean on all nine changed files. Prettier is clean on every changed file but three; the three the notification commits touch — the reschedule route, the staff support-response route and the Inbox component — were already Prettier-dirty ondevbefore this branch existed, and running--writeover them would reformat unrelated lines, so the added code is hand-formatted to match what Prettier would emit and the pre-existing drift is left where it was. Jest goes from 192 suites / 2166 tests on the untouched branch, to 193 / 2178 after the first commit, to 193 / 2185 after the second, and to 194 suites / 2199 tests here — all passing, no regressions. The extra suite is not one of ours: it arrived with thedevmerge that brought #1079 onto this branch. The nineteen new tests cover each case in the rule, the mutual exclusivity of Timings and Reschedule across all of them, the in-flight exception, the upcoming-slots filter, and for Unschedule: that a confirmed webinar and a confirmed class each offer Timings and Unschedule but not Reschedule, that a never-scheduled offering and an already-unscheduled event offer Timings but not Unschedule, that a part-released class still offers it while any session is placed, that a confirmed 1:1 offers Reschedule and neither other action, and that Unschedule never appears for any 1:1 kind whatever its slots look like.Four further tests cover the reschedule payload: an auto-confirmed proposal carries both times as
MOVED, an unanswered one carries the same pair asPROPOSED, a plain release carriesRELEASEDwith the released time and nonewDateTimekey at all, and a reschedule with no released time to report degrades toRELEASEDrather than half-filling the other sentence.One caveat on the Inbox change. The instruction for this work was no dev server, so the panel has been verified by type-checking, linting and reading Novu's composition API —
Inboxwith children mounts the provider only,Bellwith a customrenderBellrenders no button of its own so nothing nests inside the trigger, andInboxContentcarries the tabs through context — but not by opening it in a browser. It wants one look before merge.Closes #1082
Summary by CodeRabbit