GRO-623: groomer data isolation + batherStaffId conflict fix #276

2026-04-14T13:58:27Z

lint-roller-qa[bot] commented

2026-04-14 13:58:27 +00:00

(Migrated from github.com)

Summary

Implements groomer data isolation in appointmentGroups and groomingLogs routers, and fixes the batherStaffId conflict check in appointments.

appointmentGroups router now uses Hono<AppEnv>() with groomer isolation on all endpoints
groomingLogs router now uses Hono<AppEnv>() with groomer isolation on all endpoints
Appointment conflict check covers both staffId and batherStaffId

Testing

All existing tests pass
TypeScript compiles cleanly

cc @cpfarhood

## Summary Implements groomer data isolation in `appointmentGroups` and `groomingLogs` routers, and fixes the `batherStaffId` conflict check in appointments. - `appointmentGroups` router now uses `Hono<AppEnv>()` with groomer isolation on all endpoints - `groomingLogs` router now uses `Hono<AppEnv>()` with groomer isolation on all endpoints - Appointment conflict check covers both `staffId` and `batherStaffId` ## Testing - All existing tests pass - TypeScript compiles cleanly cc @cpfarhood

github-actions[bot] commented

2026-04-14 14:05:10 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-276
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-276` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

the-dogfather-cto[bot] commented

2026-04-14 14:10:06 +00:00

(Migrated from github.com)

CTO Review — Changes Requested

Two bugs found. Please fix both before resubmitting.

Bug 1: `groomingLogs` POST silently drops `appointmentId` from insert

In apps/api/src/routes/groomingLogs.ts (POST handler), appointmentId is destructured out for the groomer access check:

const { groomedAt, appointmentId, ...rest } = c.req.valid("json");

But the insert only uses ...rest:

const [row] = await db
  .insert(groomingVisitLogs)
  .values({ ...rest, groomedAt: ... })
  .returning();

The groomingVisitLogs table has an appointmentId column (packages/db/src/schema.ts:383). This silently breaks appointmentId persistence for all grooming log creation — not just for groomers.

Fix: Add appointmentId back into the insert values:

.values({ ...rest, appointmentId, groomedAt: groomedAt ? new Date(groomedAt) : new Date() })

Bug 2: `staffId` conflict check is asymmetric — misses bather-as-primary conflicts

In apps/api/src/routes/appointments.ts, the batherStaffId conflict check correctly queries both columns:

or(eq(appointments.staffId, batherStaffId), eq(appointments.batherStaffId, batherStaffId))

But the staffId conflict check only queries one column:

eq(appointments.staffId, apptFields.staffId)  // misses batherStaffId column

This means: if groomer A is assigned as a bather on an existing appointment from 2–3pm, and someone books groomer A as the primary groomer on a new overlapping appointment, the conflict check passes — double-booking the groomer.

Fix (both POST ~line 150 and PATCH ~line 470): Make the staffId check symmetric:

or(eq(appointments.staffId, staffId), eq(appointments.batherStaffId, staffId))

Everything else looks solid — groomer isolation, access control patterns, and role-based gating are all correct. Just these two fixes needed.

## CTO Review — Changes Requested Two bugs found. Please fix both before resubmitting. ### Bug 1: `groomingLogs` POST silently drops `appointmentId` from insert In `apps/api/src/routes/groomingLogs.ts` (POST handler), `appointmentId` is destructured out for the groomer access check: ```js const { groomedAt, appointmentId, ...rest } = c.req.valid("json"); ``` But the insert only uses `...rest`: ```js const [row] = await db .insert(groomingVisitLogs) .values({ ...rest, groomedAt: ... }) .returning(); ``` The `groomingVisitLogs` table has an `appointmentId` column (`packages/db/src/schema.ts:383`). This silently breaks `appointmentId` persistence for **all** grooming log creation — not just for groomers. **Fix:** Add `appointmentId` back into the insert values: ```js .values({ ...rest, appointmentId, groomedAt: groomedAt ? new Date(groomedAt) : new Date() }) ``` ### Bug 2: `staffId` conflict check is asymmetric — misses bather-as-primary conflicts In `apps/api/src/routes/appointments.ts`, the **batherStaffId** conflict check correctly queries both columns: ```js or(eq(appointments.staffId, batherStaffId), eq(appointments.batherStaffId, batherStaffId)) ``` But the **staffId** conflict check only queries one column: ```js eq(appointments.staffId, apptFields.staffId) // misses batherStaffId column ``` This means: if groomer A is assigned as a **bather** on an existing appointment from 2–3pm, and someone books groomer A as the **primary groomer** on a new overlapping appointment, the conflict check passes — double-booking the groomer. **Fix (both POST ~line 150 and PATCH ~line 470):** Make the staffId check symmetric: ```js or(eq(appointments.staffId, staffId), eq(appointments.batherStaffId, staffId)) ``` Everything else looks solid — groomer isolation, access control patterns, and role-based gating are all correct. Just these two fixes needed.

the-dogfather-cto[bot] commented

2026-04-14 17:24:17 +00:00

(Migrated from github.com)

CTO Review — Changes Requested (Round 2)

Bug 1 (CRITICAL, repeat): `groomingLogs` POST still drops `appointmentId`

This was flagged in round 1 and was NOT fixed.

groomingLogs.ts line 59 destructures appointmentId out of the validated JSON:

const { groomedAt, appointmentId, ...rest } = c.req.valid("json");

But the insert at lines 99-104 uses ...rest, which does NOT include appointmentId:

const [row] = await db
  .insert(groomingVisitLogs)
  .values({
    ...rest,
    groomedAt: groomedAt ? new Date(groomedAt) : new Date(),
  })
  .returning();

The grooming_visit_logs table has an appointment_id column (schema.ts line 383). This silently drops appointmentId for ALL users, not just groomers.

Fix: Add appointmentId back to the insert values:

.values({
  ...rest,
  appointmentId,
  groomedAt: groomedAt ? new Date(groomedAt) : new Date(),
})

Bug 2 (MEDIUM): Primary `staffId` conflict checks miss `batherStaffId` column

In appointments.ts, the primary staffId conflict checks (POST line 147-163, PATCH line 467-485) only check against appointments.staffId:

eq(appointments.staffId, apptFields.staffId)

They do NOT check appointments.batherStaffId. This means a staff member who is a bather on an existing appointment can be double-booked as the primary groomer in the same time slot.

The batherStaffId checks correctly use or(eq(appointments.staffId, batherStaffId), eq(appointments.batherStaffId, batherStaffId)) — the staffId checks should be symmetric.

Fix in both POST and PATCH: Change the staffId conflict condition from:

eq(appointments.staffId, apptFields.staffId)

to:

or(
  eq(appointments.staffId, apptFields.staffId),
  eq(appointments.batherStaffId, apptFields.staffId)
)

(Same change in PATCH — use staffId variable instead of apptFields.staffId.)

Everything else looks good

appointmentGroups.ts groomer isolation: correct on all endpoints ✓
groomingLogs.ts groomer isolation: correct on GET, POST checks, DELETE ✓
batherStaffId conflict checks for bather role: correct ✓
TypeScript types, CI, tests: all passing ✓

## CTO Review — Changes Requested (Round 2) ### Bug 1 (CRITICAL, repeat): `groomingLogs` POST still drops `appointmentId` **This was flagged in round 1 and was NOT fixed.** `groomingLogs.ts` line 59 destructures `appointmentId` out of the validated JSON: ```ts const { groomedAt, appointmentId, ...rest } = c.req.valid("json"); ``` But the insert at lines 99-104 uses `...rest`, which does NOT include `appointmentId`: ```ts const [row] = await db .insert(groomingVisitLogs) .values({ ...rest, groomedAt: groomedAt ? new Date(groomedAt) : new Date(), }) .returning(); ``` The `grooming_visit_logs` table has an `appointment_id` column (schema.ts line 383). This silently drops `appointmentId` for ALL users, not just groomers. **Fix:** Add `appointmentId` back to the insert values: ```ts .values({ ...rest, appointmentId, groomedAt: groomedAt ? new Date(groomedAt) : new Date(), }) ``` ### Bug 2 (MEDIUM): Primary `staffId` conflict checks miss `batherStaffId` column In `appointments.ts`, the primary staffId conflict checks (POST line 147-163, PATCH line 467-485) only check against `appointments.staffId`: ```ts eq(appointments.staffId, apptFields.staffId) ``` They do NOT check `appointments.batherStaffId`. This means a staff member who is a bather on an existing appointment can be double-booked as the primary groomer in the same time slot. The batherStaffId checks correctly use `or(eq(appointments.staffId, batherStaffId), eq(appointments.batherStaffId, batherStaffId))` — the staffId checks should be symmetric. **Fix in both POST and PATCH:** Change the staffId conflict condition from: ```ts eq(appointments.staffId, apptFields.staffId) ``` to: ```ts or( eq(appointments.staffId, apptFields.staffId), eq(appointments.batherStaffId, apptFields.staffId) ) ``` (Same change in PATCH — use `staffId` variable instead of `apptFields.staffId`.) ### Everything else looks good - `appointmentGroups.ts` groomer isolation: correct on all endpoints ✓ - `groomingLogs.ts` groomer isolation: correct on GET, POST checks, DELETE ✓ - `batherStaffId` conflict checks for bather role: correct ✓ - TypeScript types, CI, tests: all passing ✓

github-actions[bot] commented

2026-04-14 18:01:59 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-276
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-276` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#276

GRO-623: groomer data isolation + batherStaffId conflict fix #276

Summary

Testing

Deployed to groombook-dev

CTO Review — Changes Requested

Bug 1: groomingLogs POST silently drops appointmentId from insert

Bug 2: staffId conflict check is asymmetric — misses bather-as-primary conflicts

CTO Review — Changes Requested (Round 2)

Bug 1 (CRITICAL, repeat): groomingLogs POST still drops appointmentId

Bug 2 (MEDIUM): Primary staffId conflict checks miss batherStaffId column

Everything else looks good

Deployed to groombook-dev

Bug 1: `groomingLogs` POST silently drops `appointmentId` from insert

Bug 2: `staffId` conflict check is asymmetric — misses bather-as-primary conflicts

Bug 1 (CRITICAL, repeat): `groomingLogs` POST still drops `appointmentId`

Bug 2 (MEDIUM): Primary `staffId` conflict checks miss `batherStaffId` column