fix(booking): use the standard visibility rule in the booking pickers - #2792
Conversation
Follow-up to #2778, per review feedback on #2779. #2778 introduced a bespoke `scopeToCustodian` request param to decide which bookings the add-to-existing-booking pickers may show. That is not how booking visibility works anywhere else: the rule is `canSeeAllBookings`, resolved from the caller's role plus the workspace's `selfServiceCanSeeBookings` / `baseUserCanSeeBookings` overrides, and applied by `/bookings`, the command palette and the CSV export. - Remove `scopeToCustodian` from the filter schema, from `/api/model-filters` and from both dialog call sites. - Scope booking searches with `canSeeAllBookings` instead, matching the shape `command-palette.search.ts` already uses. - Extract `resolveCanSeeAllBookings` in roles.server so `requirePermission` and the endpoint share one definition rather than each computing the rule. - Switch `loadBookingsData` from `isSelfServiceOrBase` to `canSeeAllBookings`. Gating on the role alone meant the workspace override never reached these two dialogs, so a workspace could enable "self-service can see all bookings" and those users would still be restricted here while seeing the same bookings on the index. This makes the setting mean the same thing on every surface. Draft visibility is unchanged: `bookingDraftVisibilityClause` stays unconditional and is independent of this toggle.
🩺 React Doctor — webapp✅ No new findings on the files changed by this PR. Run locally with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9bcacbdcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughBooking visibility now derives from effective roles and organization settings. Booking queries apply separate read and write scopes. Asset and kit selectors remove request-controlled custodian filtering. Regression tests cover visibility, ownership, roles, workspace overrides, and draft conditions. ChangesBooking visibility rules
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BookingRoute
participant RolesServer
participant BookingService
participant BookingDatabase
BookingRoute->>RolesServer: Resolve role and canSeeAllBookings
RolesServer-->>BookingRoute: Return permission values
BookingRoute->>BookingService: Load bookings
BookingService->>BookingDatabase: Apply read and write scopes
BookingDatabase-->>BookingService: Return authorized bookings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)apps/webapp/app/modules/booking/service.server.tsFile contains syntax errors that prevent linting: Line 12458: Catch clause variable type annotation must be 'any' or 'unknown' if specified.; Line 12517: Catch clause variable type annotation must be 'any' or 'unknown' if specified. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webapp/app/routes/api+/model-filters.test.server.ts (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the now-unused
resolveCustodianScopemock.The loader in
model-filters.tsno longer imports or callsresolveCustodianScope. This mock and thebookingMocks.resolveCustodianScopespy are no longer exercised by any test in this file.Remove the stale mock so the test double matches the code paths actually under test.
♻️ Proposed cleanup
-const bookingMocks = vi.hoisted(() => ({ - resolveCustodianScope: vi.fn(), -})); - // why: service.server pulls in the whole booking domain (schedulers, emails); // the clause itself is pure, so a local equivalent keeps the test fast. vi.mock("~/modules/booking/service.server", () => ({ bookingDraftVisibilityClause: clause.buildDraftVisibility, - resolveCustodianScope: bookingMocks.resolveCustodianScope, }));🤖 Prompt for 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. In `@apps/webapp/app/routes/api`+/model-filters.test.server.ts around lines 79 - 88, Remove the unused bookingMocks.resolveCustodianScope spy and its resolveCustodianScope entry from the vi.mock setup for booking/service.server; retain only the bookingDraftVisibilityClause mock required by the tests.
🤖 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 `@apps/webapp/app/routes/api`+/model-filters.ts:
- Around line 187-202: Update the booking search scoping around
canSeeAllBookings to use resolveCustodianScope instead of directly adding {
custodianUserId: userId }. Preserve the unrestricted path when canSeeAllBookings
is true, and apply the shared scope to where.AND when it is false so search
matches loadBookingsData and the existing booking routes.
---
Nitpick comments:
In `@apps/webapp/app/routes/api`+/model-filters.test.server.ts:
- Around line 79-88: Remove the unused bookingMocks.resolveCustodianScope spy
and its resolveCustodianScope entry from the vi.mock setup for
booking/service.server; retain only the bookingDraftVisibilityClause mock
required by the tests.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c6d07a4f-110f-4830-98a1-86afc3ee8b99
📒 Files selected for processing (7)
apps/webapp/app/modules/booking/service.server.load-bookings-data.test.tsapps/webapp/app/modules/booking/service.server.tsapps/webapp/app/routes/_layout+/assets.$assetId.overview.add-to-existing-booking.tsxapps/webapp/app/routes/_layout+/kits.$kitId.assets.add-to-existing-booking.tsxapps/webapp/app/routes/api+/model-filters.test.server.tsapps/webapp/app/routes/api+/model-filters.tsapps/webapp/app/utils/roles.server.ts
|
@carlosvirreira i see this still has some unresolved feedback from the review agents. LMK if its ready for me to test/review. |
…epts Review follow-up on the booking picker visibility work. The loader that seeds each picker and the search endpoint it hands over to now apply the SAME two server-derived restrictions, so the list cannot change mid-search and cannot offer a booking the submit then rejects. - Add `bookingWriteScopeClause`, the query-side mirror of `validateBookingOwnership`. Deliberately independent of `canSeeAllBookings`: that workspace toggle governs READ visibility only, so gating a mutation-target picker on it offered SELF_SERVICE / BASE users bookings the action then 403s on. - Extract `custodianScopeClause` from `getBookings` and reuse it in `/api/model-filters`. The endpoint matched `custodianUserId` alone, so a booking custodied through a legacy team-member row appeared in the seeded list and vanished the moment the user typed. - Thread `role` into `loadBookingsData` and pass both restrictions to `getBookings` via `writableByUserId` / `writableByRole` — scalars rather than a where-input, so no call site can hand it a request-controlled predicate. Tests: an equivalence suite pinning the new clause against the gate it mirrors (5 booking fixtures x 4 roles, with an evaluator that throws rather than silently passing if the clause shape grows), plus a regression asserting that `scopeToCustodian=false&selectedValues=<other-booking>` changes nothing about the resulting where.
Two fail-open defaults flagged by the pre-commit security reviewer. - `bookingWriteScopeClause` now ALLOW-lists ADMIN / OWNER instead of deny-listing SELF_SERVICE / BASE. A role added to `OrganizationRoles` later lands in the restricted branch by default, so the picker under-offers — a visible gap — rather than offering rows no rule covered. - Collapse `getBookings`' `writableByUserId` / `writableByRole` into a single `writableBy` object. As two sibling params, setting only one silently skipped the restriction entirely; both halves are now required together or not at all. Adds a test for the unrecognised-role branch and an enum-exhaustiveness guard that fails the moment `OrganizationRoles` grows a member, so whoever adds one has to decide which side of the clause it belongs on.
|
Picking up the two items from CodeRabbit's review body that never became inline threads, so were never answered. 1. "Remove the now-unused It was correct when filed. That revision had dropped As of 8fa6b55 the endpoint resolves the full custodian scope again ( Worth noting the two findings pointed in opposite directions — dropping the mock would have cemented the bug the Major finding asked to fix. Fixing the Major one made the nitpick obsolete. 2. Docstring Coverage 40% (threshold 80%) — not claiming this one fixed; the check hasn't re-run against the current head. Every export added since carries full JSDoc ( Threads for the two inline findings on this review have been answered separately. |
2 participants
Follow-up to #2778. Implements @DonKoko's answer on #2779: booking visibility should follow the standard rule rather than anything bespoke.
What was wrong
#2778 shipped a
scopeToCustodianrequest param that the two "Add to existing booking" dialogs set to decide which bookings they may show. That is not how booking visibility works anywhere else in the app.The standard rule is
canSeeAllBookings, resolved inrequirePermissionfrom the caller's role plus the workspace overrides (selfServiceCanSeeBookings/baseUserCanSeeBookings)./bookings, the command palette and the CSV export all gate on it.api+/command-palette.search.tsis the closest precedent, a search endpoint applying it directly:Changes
scopeToCustodianfrom the filter schema, from/api/model-filters, and from both dialog call sites. The request no longer carries any visibility intent.canSeeAllBookings, same shape as the command palette.resolveCanSeeAllBookingsinroles.server.ts, used by bothrequirePermissionand the endpoint, so the rule has one definition instead of being recomputed per surface. A surface that computes its own rule is what makes a picker's list change the moment a user types.loadBookingsDatafromisSelfServiceOrBasetocanSeeAllBookings.On change 4
This is a behaviour change, not a refactor, so calling it out explicitly.
loadBookingsDatagated on the role alone, so the workspace override never reached these two dialogs. A workspace could enable "self-service users can see all bookings" and those users would still be restricted here, while seeing the very same bookings on/bookings, in the command palette and in exports. After this change the setting means the same thing on every surface.It also has to land with change 2 rather than after it. If the endpoint moves to
canSeeAllBookingswhile the loader keeps gating on the role, then in a workspace with the setting on the untyped list shows own bookings only and the typed list shows all — re-creating the loader-versus-search divergence #2778 exists to remove, in the opposite direction.Draft visibility
Unchanged.
bookingDraftVisibilityClausestays unconditional and is independent of this toggle: a DRAFT booking is only ever visible to its creator.Verification
Covered at both layers, since the observable here is which rows the query returns:
api+/model-filters.test.server.ts(14 tests) — restricted for SELF_SERVICE and BASE when the setting is off, unrestricted when the workspace enables it, never restricted for ADMIN / OWNER, and the draft clause present in every case.modules/booking/service.server.load-bookings-data.test.ts(new, 3 tests) — the loader applies the custodian restriction only whencanSeeAllBookingsis false, and never drops the draft clause. Asserts on the Prismawhere, following the convention inservice.server.get-bookings-permissions.test.ts.Full suite green: 313 files, 4048 tests.
turbo typecheckand ESLint clean.Summary by CodeRabbit
Booking Visibility
Booking Updates