diff --git a/src/app/(rep)/rep/outbox.test.ts b/src/app/(rep)/rep/outbox.test.ts index 04fdb3e..519f229 100644 --- a/src/app/(rep)/rep/outbox.test.ts +++ b/src/app/(rep)/rep/outbox.test.ts @@ -121,6 +121,22 @@ describe("flushOutbox", () => { expect(readOutbox()[0].lastError).toMatch(/Failed to fetch/); }); + it("keeps a check-in queued when the server accepted the request but wrote nothing", async () => { + // Live-reproduced bug: recordVisits can create 0 rows (a rejected + // position_source value, or any batch where every item fails) — + // the route now answers 502 rather than 200 for that case. This + // proves the outbox's existing 5xx-retry path actually catches it, + // so a fully-failed write is retried instead of being treated as + // delivered and discarded. + enqueue(CHECK_IN); + const fetchImpl = vi.fn(async () => response(502)) as unknown as typeof fetch; + + const result = await flushOutbox("tok", fetchImpl); + + expect(result).toMatchObject({ sent: 0, remaining: 1 }); + expect(readOutbox()).toHaveLength(1); + }); + it("keeps an entry on a 5xx and counts the attempt", async () => { enqueue(CHECK_IN); const fetchImpl = vi.fn(async () => response(503)) as unknown as typeof fetch; diff --git a/src/app/api/v1/visits/route.test.ts b/src/app/api/v1/visits/route.test.ts new file mode 100644 index 0000000..cdd9e29 --- /dev/null +++ b/src/app/api/v1/visits/route.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +const getBrowserSessionUser = vi.fn(); +const recordVisits = vi.fn(); + +vi.mock("@/lib/server/browser-session", () => ({ + getBrowserSessionUser: () => getBrowserSessionUser(), +})); + +vi.mock("@/lib/server/visits-service", () => ({ + recordVisits: (args: unknown) => recordVisits(args), +})); + +import { POST } from "./route"; + +function request(body: unknown, token = "tok"): NextRequest { + return new NextRequest("http://localhost:4001/api/v1/visits", { + method: "POST", + headers: { "Content-Type": "application/json", Authorization: `Bearer ${token}` }, + body: JSON.stringify(body), + }); +} + +const VALID_BODY = { + lat: -26.4, + lon: 27.4, + positionSource: "phone", + visits: [{ customerId: "c1" }], +}; + +beforeEach(() => { + vi.clearAllMocks(); + getBrowserSessionUser.mockResolvedValue({ id: "u-tiaan", user_type: ["REP"] }); +}); + +describe("POST /api/v1/visits", () => { + it("scopes the write to the session's own rep id, not anything the client sends", async () => { + recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] }); + + await POST(request({ ...VALID_BODY, repId: "someone-else" })); + + expect(recordVisits).toHaveBeenCalledWith( + expect.objectContaining({ repId: "u-tiaan" }) + ); + }); + + it("returns 502, not 200, when every visit in the batch failed to write", async () => { + // This is the failure this route exists to prevent: recordVisits + // can legitimately create 0 rows (e.g. a PB schema mismatch on + // position_source, reproduced live before this fix), and a 200 in + // that case tells the offline outbox the entry was delivered — it + // gets discarded and the visit is lost with no trace. + recordVisits.mockResolvedValue({ + created: 0, + pinned: 1, + visitIds: [], + errors: [{ customerId: "c1", message: "Invalid value phone." }], + }); + + const res = await POST(request(VALID_BODY)); + + expect(res.status).toBe(502); + const json = await res.json(); + expect(json.errors[0].message).toMatch(/Invalid value phone/); + }); + + it("returns 200 when at least one visit was written, even alongside partial failures", async () => { + recordVisits.mockResolvedValue({ + created: 1, + pinned: 0, + visitIds: ["v1"], + errors: [{ customerId: "c2", message: "stale customer id" }], + }); + + const res = await POST(request({ ...VALID_BODY, visits: [{ customerId: "c1" }, { customerId: "c2" }] })); + + expect(res.status).toBe(200); + }); + + it("defaults an unrecognised positionSource to phone", async () => { + recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] }); + + await POST(request({ ...VALID_BODY, positionSource: "carrier-pigeon" })); + + expect(recordVisits).toHaveBeenCalledWith( + expect.objectContaining({ positionSource: "phone" }) + ); + }); + + it("rejects a request with no visits before calling the service", async () => { + const res = await POST(request({ ...VALID_BODY, visits: [] })); + expect(res.status).toBe(400); + expect(recordVisits).not.toHaveBeenCalled(); + }); + + it("rejects non-numeric coordinates", async () => { + const res = await POST(request({ ...VALID_BODY, lat: "not a number" })); + expect(res.status).toBe(400); + expect(recordVisits).not.toHaveBeenCalled(); + }); + + it("rejects an unauthenticated caller", async () => { + getBrowserSessionUser.mockRejectedValue( + Object.assign(new Error("Not authenticated."), { status: 401 }) + ); + + const res = await POST(request(VALID_BODY)); + expect(res.status).toBe(401); + expect(recordVisits).not.toHaveBeenCalled(); + }); + + it("drops a visit entry with a blank customerId rather than passing it through", async () => { + recordVisits.mockResolvedValue({ created: 1, pinned: 0, visitIds: ["v1"], errors: [] }); + + await POST(request({ ...VALID_BODY, visits: [{ customerId: "" }, { customerId: "c1" }] })); + + expect(recordVisits).toHaveBeenCalledWith( + expect.objectContaining({ visits: [{ customerId: "c1", notes: undefined, pinCustomer: false }] }) + ); + }); +}); diff --git a/src/app/api/v1/visits/route.ts b/src/app/api/v1/visits/route.ts index bc403de..8f06cfe 100644 --- a/src/app/api/v1/visits/route.ts +++ b/src/app/api/v1/visits/route.ts @@ -106,6 +106,17 @@ export async function POST(req: NextRequest) { visits, }); + // Nothing written is a failed submission, not a successful empty + // one — a 200 here would tell the outbox the entry was delivered + // and it would discard it, silently losing every visit in the + // batch. 502 (not a 4xx) so the outbox's "permanent failure, drop + // it" rule doesn't apply: this is very likely transient or a + // config issue (see the errors array), and it should keep retrying + // until it either succeeds or someone looks at the message. + if (result.created === 0) { + return NextResponse.json(result, { status: 502 }); + } + return NextResponse.json(result); } catch (error) { const message = error instanceof Error ? error.message : "Failed to record visits"; diff --git a/src/lib/server/visits-service.test.ts b/src/lib/server/visits-service.test.ts index c7b4235..a24cb87 100644 --- a/src/lib/server/visits-service.test.ts +++ b/src/lib/server/visits-service.test.ts @@ -188,6 +188,39 @@ describe("recordVisits", () => { expect(pb.store.rep_visits[0].customer).toBe("c2"); }); + it("reports the failure reason rather than swallowing it", async () => { + // Live-verified failure mode: a `rep_visits.position_source` value + // PocketBase hasn't been given yet fails every item in the batch + // identically. A bare `created: 0` with no explanation looks + // indistinguishable from "nothing to do" — the actual cause has to + // survive the catch. + const pb = fakePb({ customers: [{ id: "c1" }] as Row[] }); + pb.failOn("rep_visits", "c1"); + + const result = await recordVisits({ ...base, visits: [{ customerId: "c1" }] }, pb); + + expect(result.errors).toEqual([{ customerId: "c1", message: "create failed" }]); + }); + + it("reports one error per failed customer in a mixed batch", async () => { + const pb = fakePb({ customers: [{ id: "c1" }, { id: "c2" }] as Row[] }); + pb.failOn("rep_visits", "c1"); + + const result = await recordVisits( + { ...base, visits: [{ customerId: "c1" }, { customerId: "c2" }] }, + pb + ); + + expect(result.errors).toEqual([{ customerId: "c1", message: "create failed" }]); + expect(result.created).toBe(1); + }); + + it("returns no errors on a clean run", async () => { + const pb = fakePb({ customers: [{ id: "c1" }] as Row[] }); + const result = await recordVisits({ ...base, visits: [{ customerId: "c1" }] }, pb); + expect(result.errors).toEqual([]); + }); + it("updates lastvisiteddate on the customer", async () => { const pb = fakePb({ customers: [{ id: "c1" }] as Row[] }); diff --git a/src/lib/server/visits-service.ts b/src/lib/server/visits-service.ts index a099ac1..3746b20 100644 --- a/src/lib/server/visits-service.ts +++ b/src/lib/server/visits-service.ts @@ -86,6 +86,14 @@ export interface RecordVisitsResult { created: number; pinned: number; visitIds: string[]; + /** + * One entry per customer whose visit row failed to write, with the + * underlying PocketBase message. Never swallowed silently — a schema + * mismatch (e.g. an unapproved `position_source` value) fails every + * item in the batch identically, and the caller needs to see why + * rather than receive a bare `created: 0`. + */ + errors: { customerId: string; message: string }[]; } /** @@ -158,7 +166,7 @@ export async function recordVisits( } const visitedAt = args.visitedAt ?? new Date().toISOString(); - const result: RecordVisitsResult = { created: 0, pinned: 0, visitIds: [] }; + const result: RecordVisitsResult = { created: 0, pinned: 0, visitIds: [], errors: [] }; for (const visit of args.visits) { if (!visit.customerId) continue; @@ -191,7 +199,16 @@ export async function recordVisits( }); result.created++; result.visitIds.push((row as { id: string }).id); - } catch { + } catch (error) { + // Caught here, not thrown, so one stale customer id in a batch + // doesn't cost the rest of the visits — but the message is kept + // rather than discarded, so a *systemic* failure (every item in + // the batch hitting the same PB validation error) is visible to + // the caller instead of looking like a clean, empty success. + result.errors.push({ + customerId: visit.customerId, + message: error instanceof Error ? error.message : "Failed to record visit", + }); continue; }