Skip to content

SANC-94: Remap bookings.agency_id to organization.id#72

Open
jason-duong4509 wants to merge 4 commits intomainfrom
SANC-94-Remap-bookings.agency_id-to-organization.id
Open

SANC-94: Remap bookings.agency_id to organization.id#72
jason-duong4509 wants to merge 4 commits intomainfrom
SANC-94-Remap-bookings.agency_id-to-organization.id

Conversation

@jason-duong4509
Copy link
Copy Markdown
Contributor

@jason-duong4509 jason-duong4509 commented Apr 2, 2026

Remapped the bookings.agency_id column to be a foreign key of organization.id rather than user.id since the old foreign key caused discrepancy issues between implementation and design. Also made changes to various files within the repository to ensure that any affected code was fixed.

In addition to this change, a bug was fixed during the process of propagating the schema changes. Before, agency members could only view bookings that they created. Now, they can see bookings created by their agency.

Summary by CodeRabbit

  • Refactor
    • Updated agency association logic to use organization-based references instead of user-based identifiers. This improves data consistency and organizational structure handling throughout booking creation and access workflows.

- Made schema changes in the bookings table so that bookings.agency_id
  references the right column (organization.id not user.id)
  - Propagated the schema changes to affected files
- Fixed a bug where agency users could not see bookings made by their
  agency
…s.agency_id-to-organization.id

# Conflicts:
#	src/server/api/routers/trip.ts
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
salvationarmy Ready Ready Preview, Comment Apr 6, 2026 9:05pm

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 600f7176-3791-492a-92bf-46015fec2cd3

📥 Commits

Reviewing files that changed from the base of the PR and between da5db92 and b94e5bb.

📒 Files selected for processing (3)
  • src/app/debug/bookings/page.tsx
  • src/server/api/routers/bookings.ts
  • src/server/api/routers/trip.ts
✅ Files skipped from review due to trivial changes (1)
  • src/app/debug/bookings/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/server/api/routers/trip.ts
  • src/server/api/routers/bookings.ts

📝 Walkthrough

Walkthrough

This PR refactors the booking system's agency semantics by reassociating agencyId from individual users to organizations. The changes propagate across the database schema relationships, API routers, session-based authorization checks, and seed data generation to enforce this organizational model throughout the system.

Changes

Cohort / File(s) Summary
Database Schema Relations
src/server/db/booking-schema.ts, src/server/db/auth-schema.ts
Updated booking-to-agency relationships: agencyId foreign key now references organization instead of user, and moved agencyBookings relation from userRelations to organizationRelations.
API Authorization & Access Logic
src/server/api/routers/bookings.ts
Refactored authorization and filtering to use session.session.activeOrganizationId instead of session.user.id for agency access validation; added BAD_REQUEST error handling when activeOrganizationId is missing; updated getCurrentUser query to return agencyId.
Trip Creation Mutation
src/server/api/routers/trip.ts
Added validation requiring session.session.activeOrganizationId presence; changed booking insertion to source agencyId from activeOrganizationId instead of user.id.
Debug UI & Documentation
src/app/debug/bookings/page.tsx
Updated inline documentation and form field logic to reflect agencyId semantics change from user.id to organization.id.
Data Seeding
drizzle/seed.ts
Refactored seedBookings() to derive agencyId from organization membership via the member table instead of directly from agencyUser.id; added membership validation with conditional skipping.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • burtonjong
  • themaxboucher
  • JustinTan-1
  • Yemyam
  • Lujarios
  • tanvimahal
  • wesleylui

Poem

🐰 Hopping through schema with organizational grace,
From user to org, we've found the right place!
Relations now point where the membership flows,
Agency bookings in better repose. 🏢✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the main objective: remapping bookings.agency_id from user.id to organization.id, which is the core schema and codebase change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch SANC-94-Remap-bookings.agency_id-to-organization.id

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@drizzle/seed.ts`:
- Around line 343-349: Replace the ad-hoc membership lookup with a single helper
(e.g., getSingleUserMembership(userId)) that centralizes logic used at the three
places (the userOrg lookups around the current diff). Implement the helper to
query all memberships for the user (select from member where member.userId =
userId), then: return null if none; if exactly one, return it; if more than one,
either throw an explicit error (fail fast) or apply a deterministic selector
(for example orderBy(member.orgId) and pick the first) — pick one policy and
document it in the helper. Replace each occurrence (the current
db.select(...).limit(1).then(r => r[0]) calls) with calls to this helper so
behavior is consistent and deterministic across the file.

In `@src/server/api/routers/bookings.ts`:
- Around line 252-253: The code currently uses
ctx.session.session.activeOrganizationId ?? "" to populate agencyId which
creates an ambiguous empty-string sentinel; replace these fallbacks with
explicit null-handling: in mutation handlers (where you create/update bookings)
check ctx.session.session.activeOrganizationId and if missing throw a clear
TRPCError (bad_request/unauthorized) instead of using "", and in query/debug
paths return null or omit the agencyId field rather than "" so FK operations are
never passed an empty string; update the agencyId assignments and the three
other occurrences noted (the other agencyId assignments at the locations
referenced) to perform this guard and use TRPCError for mutations.

In `@src/server/api/routers/trip.ts`:
- Line 35: The code uses ctx.session.session.activeOrganizationId ?? "" when
setting agencyId which masks missing orgs and can cause FK errors; update the
router handler (the function setting agencyId in src/server/api/routers/trip.ts)
to explicitly read const activeOrg = ctx.session.session.activeOrganizationId
and if (!activeOrg) throw new TRPCError({ code: "BAD_REQUEST", message: "No
active organization set" }) (or "FORBIDDEN" if preferred) instead of falling
back to an empty string, then use agencyId: activeOrg in the payload.

In `@src/server/db/booking-schema.ts`:
- Around line 54-57: The relationName "agencyBookings" is defined on the wrong
side of the relation: in booking-schema.ts the agency field uses relationName:
"agencyBookings" with bookings.agencyId referencing organization.id, but the
reverse many(...) was placed in userRelations; move the reverse relation
declaration agencyBookings: many(bookings, { relationName: "agencyBookings" })
out of userRelations in auth-schema.ts and add it under organizationRelations
instead so the relation is owned by organization and matches bookings.agencyId
-> organization.id.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 16dae3c5-c6e1-44f9-bf20-cae08feb3d1d

📥 Commits

Reviewing files that changed from the base of the PR and between a9f337f and 3835bd3.

📒 Files selected for processing (5)
  • drizzle/seed.ts
  • src/app/debug/bookings/page.tsx
  • src/server/api/routers/bookings.ts
  • src/server/api/routers/trip.ts
  • src/server/db/booking-schema.ts

- Moved relation declaration from userRelations to organizationRelations
  since the bookings.agency_id FK moved from users to organizations
- Added error handling for when a user does not belong to an
  organization
Copy link
Copy Markdown
Contributor

@promatty promatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few easy fixes. also, you are now requiring activeOrganizationId. because the admin and driver calendars call getAll, is there assurance that admins and drivers will always have an activeOrganizationId? ie. we dont want to fail to load bookings into their calendars

@jason-duong4509
Copy link
Copy Markdown
Contributor Author

a few easy fixes. also, you are now requiring activeOrganizationId. because the admin and driver calendars call getAll, is there assurance that admins and drivers will always have an activeOrganizationId? ie. we dont want to fail to load bookings into their calendars

There is assurance, in the form of a database hook which sets a user's activeOrganizationId on login (in src\lib\auth.ts, lines 118-136). In theory, all users belong to exactly 1 organization at all times (except the admin account, where the seeded user has multiple). A user should always have an assigned organization in the DB on login

- Some error messages were spelt wrong. They have been corrected
- A comment was outdated due to changes made in this ticket. That has
  been corrected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants