AgendaPro Dev
d796538fd9
- Rebrand all user-facing text to AgendaMax - Fix sidebar scrolling: card container no longer overflows html (flex h-full min-h-0) - Fix calendar toolbar hover: active buttons keep readable contrast - Sticky calendar toolbar (month/week/day always visible on scroll) - Seed guarantees 2-4 appointments per day for current week (Mon-Sat)
60 lines
6.7 KiB
Markdown
60 lines
6.7 KiB
Markdown
# Task 6 — backend `appointments.ts` conflict guard + autoAssign fallback
|
||
|
||
## Files changed
|
||
- `server/routes/appointments.ts`
|
||
- Added import: `import { autoAssign, isAvailable, runInTransaction, parseWorkingHours } from "../lib/scheduling.ts";`
|
||
- Rewrote the `POST /` handler tail (from the old `// Resolve employee` comment through the handler's closing `})`) so that:
|
||
- `start`/`end`/`startMs`/`endMs`/`finalStatus`/`price` are computed first (outside the tx).
|
||
- The employee resolution + guard + INSERT + completed→ticket + `joinAppointment` all run inside `runInTransaction(db, …)`.
|
||
- Employee resolution keeps the same priority chain (`employee_id` from body → `employee.role` self-assign → `autoAssign` fallback). The old `LIMIT 1` lookup over `employee_services` is removed; when no employee is resolved, `autoAssign(db, {…})` picks the best available specialist.
|
||
- Before the INSERT, `isAvailable(db, empId, startMs, endMs)` is checked; on conflict it throws `{status:409, error:"Ese horario ya está ocupado para el especialista"}`.
|
||
- The catch translates any `{status,error}` throw into `err(res, status, error)` and rethrows everything else.
|
||
- `GET /`, `GET /:id`, `PATCH /:id`, `POST /:id/complete`, `DELETE /:id`, `GET /:id/cancellation-policy` are **untouched**. `joinAppointment` and `computeCommission` are untouched.
|
||
|
||
## Deviations from the plan's literal text
|
||
1. **Replaced a wider window than "lines 129-167".** The plan said to replace from `const start = new Date(start_at);` (line 129) through the handler end, but the verbatim block it provided *re-resolves* `empId` inside the transaction. If I had kept the original outer `empId` resolution (lines 116-127, including the `LIMIT 1` lookup and the early `return err(res,400,…)`), the outer code would (a) shadow the inner `empId`, (b) leave the new `autoAssign` path as dead code, and (c) directly contradict the task summary ("when still no employee, call `autoAssign(...)` instead of the old `LIMIT 1` lookup"). So I replaced the outer block too. This matches the verbatim block's own comment ("Resolver empleado (igual que antes, pero usando autoAssign si ninguno)") and the task summary.
|
||
2. **`businessId: Number(req.user!.business_id)` instead of `businessId: req.user!.business_id`.** `AuthedRequest.user.business_id` is typed `number | null` (from `shared/types.ts`), but `AutoAssignCtx.businessId` requires `number`. `Number(...)` is safe because `authRequired` guarantees an authenticated user with a business. This is a literal-text bug in the plan; without the coercion `tsc` errors with TS2322.
|
||
|
||
## Verification
|
||
|
||
### `npm run typecheck`
|
||
**PASS — 0 errors.** (After the `Number(...)` coercion; raw plan text produced `TS2322: Type 'number | null' is not assignable to type 'number'`.)
|
||
|
||
### `npm run test:unit`
|
||
**PASS — 15/15.** `scheduling.test.ts` unchanged and green.
|
||
|
||
### `npm run test:e2e`
|
||
**FAIL — but NOT a code bug; it is the seed/test-data concern the task anticipated.**
|
||
|
||
- The failure is at `e2e-test.mjs:46` ("create appointment"): `POST /api/appointments { service_id:1, client_id:1, start_at:"2026-09-15T11:00:00Z" }` returns **400** `{"error":"No hay empleado asignado a este servicio"}` (i.e. `autoAssign` returned `null`), which makes line 47 throw a `TypeError` reading `.id` off `undefined`.
|
||
- The other 16 checks up to that point all PASS (login, /me, business, employees+stats, services, clients+stats, appointments joined, dashboard ×9, tickets).
|
||
|
||
**Root cause (verified):** timezone mismatch between the e2e fixture and the new working-hours enforcement.
|
||
- Server TZ: `America/Mexico_City` (UTC−6).
|
||
- Fixture slot `2026-09-15T11:00:00Z` = **05:00 local** — before the business's 09:00 opening (Mon–Fri 09:00–20:00).
|
||
- `autoAssign` → `getWorkingHoursForDate` builds the day window in local time (09:00–20:00 local) and filters every candidate whose slot doesn't fit; at 05:00 local nobody qualifies → `autoAssign` returns `null` → handler throws `{status:400, "No hay empleado asignado a este servicio"}`.
|
||
- The legacy `LIMIT 1` lookup never checked working hours, so this exact fixture used to pass; the new code enforces the plan's hard constraint.
|
||
|
||
This is the "seed/test-data concern, not a code defect" case the task description calls out (it anticipated a 409, but the same principle applies to the 400 from the stricter autoAssign). The fix is data-side — either run the server in UTC, or update `e2e-test.mjs` to use in-hours ISO slots (the plan's own Task 7 `booking-e2e.mjs` already does this by fetching slots from `/slots`). **The guard was not weakened.**
|
||
|
||
### Conflict smoke (temp `.mjs`, created/verified/deleted)
|
||
Using an in-hours slot `2027-02-01T17:00:00Z` (= 11:00 local):
|
||
- Create #1 (`service_id:1`, owner, no `employee_id`): **201**, `autoAssign` placed it on `employee_id:1`.
|
||
- Create #2 (same `employee_id:1` + same slot): **409** `{"error":"Ese horario ya está ocupado para el especialista"}`.
|
||
- Create #3 (same `employee_id:1` + 2 h later, non-overlapping): **201**.
|
||
|
||
All three behave exactly as required.
|
||
|
||
## Self-review
|
||
- **Guard inside tx?** Yes — `isAvailable(db, empId, startMs, endMs)` runs inside `runInTransaction(db, …)`, before the INSERT. Throwing inside rolls back via the helper's ROLLBACK path.
|
||
- **autoAssign fallback when no employee?** Yes — after the body's `employee_id` and the employee-role self-assign, if `!empId`, the handler loads `businesses.working_hours`, parses it, and calls `autoAssign(db, {businessId, serviceId, serviceName, serviceCategory, startMs, endMs, bizWh})`. `empId = aa?.employeeId ?? null`; if still null, throws `{status:400,…}`.
|
||
- **Employee-role self-assign preserved?** Yes — `if (!empId && req.user!.role === "employee" && req.user!.employee_id) empId = req.user!.employee_id;` is intact.
|
||
- **Owner with no employee_id → autoAssign (not LIMIT 1)?** Yes — the old `SELECT … FROM employee_services … LIMIT 1` block is removed.
|
||
- **completed→ticket still works?** Yes — the `if (finalStatus === "completed") { INSERT INTO tickets … }` block is preserved verbatim inside the tx, keyed on the freshly `RETURNING *`-ed `inserted.id`.
|
||
- **Price override preserved?** Yes — `price = price_override !== undefined ? Number(price_override) : service.price` computed before the tx and passed into the INSERT.
|
||
- **Client resolution preserved?** Yes — untouched above the replaced block (existing `client_id` or `new_client` create).
|
||
- **`joinAppointment` on result?** Yes — called on `inserted` inside the tx, then returned; `res.status(201).json({ appointment: r })` outside.
|
||
- **Other endpoints untouched?** Yes — GET/PATCH/DELETE/complete/cancellation-policy are byte-identical to before.
|
||
- **Response on success?** Yes — `res.status(201).json({ appointment: r })` unchanged.
|
||
- **No `git commit` run?** Correct — no commit, no stage.
|