Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
amanuelabiy
left a comment
amanuelabiy
left a comment
There was a problem hiding this comment.
Nice work on the core logic. The transaction is solid. We just need a few of these changes before merging.
Also for testing please add a test that calls visits.scan through createCaller (see routers/index.test.ts), including one that checks a call without a session is rejected with UNAUTHORIZED. Your existing tests can stay. Just pass the fake db in directly instead of mocking the module.
| import { COLLECTIONS } from "@repo/common"; | ||
| import type { Person } from "@repo/common"; | ||
|
|
||
| type ScanInput = { barcode: string; visitTypeId?: string }; |
There was a problem hiding this comment.
As per the task, the input schema should live in packages/common/src/schemas.ts as arktype schema, so its checked at runtime and shared with the frontend.
Remove this local type. And add something like this to packages/common/src/schemas.ts :
export const scanInput = type({
barcode: "string >= 1",
"visitTypeId?": "string",
});
export type ScanInput = typeof scanInput.infer;
| }; | ||
| } | ||
|
|
||
| export function handleBarcodeScan(input: ScanInput) { |
There was a problem hiding this comment.
Good logic but nothing is calling handleBarcodeScan, so the kiosk has no way to use it. Step 1 of the task is to expose it as a tRPC procedure called visits.scan. That's what lets the frontend call it over the network.
Please create packages/backend/src/routers/visits.ts:
export const visitsRouter = router({
scan: protectedProcedure
.input(scanInput)
.mutation(({ ctx, input }) => handleBarcodeScan(ctx.db, input)),
});
and mount it as visits: visitsRouter in routers/index.ts. Use .mutation rather than .query, since it writes data, and use protectedProcedure so only a signed-in kiosk can call it.
| } | ||
|
|
||
| export function handleBarcodeScan(input: ScanInput) { | ||
| const db = getFirestore(); |
There was a problem hiding this comment.
Take db: Firestore as a parameter instead of calling getFirestore() here and on lines 24 and 64. Pass ctx.db in from the procedure, like the other routers do. That also allows the tests to pass a fake db directly instead of mocking the whole fire-base-admin/firestore module.
|
|
||
| export function handleBarcodeScan(input: ScanInput) { | ||
| const db = getFirestore(); | ||
| const barcode = input.barcode.trim().toUpperCase(); //ToDo: swap for normalizeBarcode() |
There was a problem hiding this comment.
Once #2 merges, please update this branch from main and use normalizeBarcode().
Also write TODOs as //TODO:. So editors and searches can pick them up as this is the usual convention.
| ) { | ||
| const visitRef = getFirestore().collection(COLLECTIONS.visits).doc(); //new visit id | ||
|
|
||
| transaction.set(visitRef, { |
There was a problem hiding this comment.
This visit doesn't match the visit schema in common/src/visit.ts. I checked it on the emulator and it's missing personType, checkInMethod, endReason, flags, updatedAt and updatedBy.
Please add:
personType: person.personType,
checkInMethod: "barcode",
endReason: null,
flags: [],
updatedAt: now,
updatedBy: null,
It would also help to check the object with visit(...) before writing it, so a missing field fails right away.
| const checkedInAt = visitData.get("checkedInAt") as number; | ||
| const durationMs = now - checkedInAt; | ||
|
|
||
| transaction.update(visitRef, { checkedOutAt: now, durationMs: durationMs }); |
There was a problem hiding this comment.
Please set endReason: "checkout" and updatedAt: now here. This allows reports to tell the difference between a normal checkout and an auto-expired/admin-closed visit.
| transaction, | ||
| personRef, | ||
| person, | ||
| input.visitTypeId ?? null, |
There was a problem hiding this comment.
visitTypeId is optional in the input, but the visit schema requires a string, so this can write null. Please default to "member-hours" (the first of DEFAULT_VISIT_TYPES in catalog.ts) when none is given.
Pull Request
Description
Adds
handleBarcodeScaninpackages/backend/src/barcode-scan-handler.ts, which handles a member scanning their barcode at the kiosk and addsbarcode-scan-handler.test.tsusing an in-memory fake of Firestore.Type of Change
Related Issues (put task name here from notion)
Screenshots (If it is a front end feature screenshot is required)
Additional Notes
Current visit fields:
personId,visitTypeId,checkedInAt,checkedOutAt,durationMs.normalizeBarcode()isn't on main yet, so I usedtrim().toUpperCase()for now and will swap it in later.Tests use a fake Firestore, so they don't cover two scans at the same time.