Feat/nfc payload endpoint#189
Conversation
|
|
||
| let username: string; | ||
|
|
||
| // Get username from user profile |
There was a problem hiding this comment.
Error handling around the DB logic should be improved here.
There was a problem hiding this comment.
Wrapped the user fetch in try/catch with request.log.error logging and returns 500 with a clear error message if DB fails.
| // GET /api/nfc/payload?card=<cardId> — returns payload for a specific card | ||
| app.get('/payload', async (request: FastifyRequest<{ Querystring: { card?: string } }>, reply: FastifyReply) => { | ||
| const userId = (request.user as any).id; | ||
| const { card: cardId } = request.query; |
There was a problem hiding this comment.
Could we validate the query params with Zod here it can be more robust.
There was a problem hiding this comment.
Added a Zod schema nfcQuerySchema with safeParse to validate query params returns 400 with flattened error details if invalid.
|
|
||
| if (cardId) { | ||
| // Validate card belongs to authenticated user | ||
| const card = await app.prisma.card.findUnique({ |
There was a problem hiding this comment.
Could error handling can be improved here?
There was a problem hiding this comment.
Wrapped card fetch in its own try/catch returns 404 if card not found, 403 if ownership mismatch, and 500 with request.log.error logging if DB fails.
| }); | ||
| } | ||
|
|
||
| return reply.send({ |
There was a problem hiding this comment.
Typed response schema is missing here?
There was a problem hiding this comment.
Adding a typed response interface now will push the fix shortly!
There was a problem hiding this comment.
Please add typed response here.
|
|
||
| return reply.send({ | ||
| type: 'URI', | ||
| payload: `https://devcard.dev/${username}`, |
There was a problem hiding this comment.
Typed response schema missing here?
|
@Midoriya-w Your PR have merge conflicts. |
|
Good catch! Adding a typed response interface now. |
|
Hey @Harxhit! Resolved the merge conflicts in app.ts kept both nfcRoutes and eventRoutes registrations. Also addressed all review comments. Could you re-review when you get a chance? |
|
Added NfcPayloadResponse type and updated both reply.send calls to reply.send() for full type safety. |
| @@ -1,4 +1,4 @@ | |||
| import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; | |||
| Dimport type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; | |||
There was a problem hiding this comment.
Fixed, removed the accidental D from the import statement.
|
@Midoriya-w Could you please add proofs for test in the PR descriptions. |
Test Evidence"Endpoint: GET /api/nfc/payload"
"Endpoint: GET /api/nfc/payload?card="
|
packages/shared/src/tests/cards.test.ts Add proofs for these in the PR description. |
|
Hey @Harxhit! Here are the test proofs from packages/shared/src/tests/cards.test.ts: -passes with valid platforms
diffCardPlatforms tests (3 cases):
All 10 test cases cover the acceptance criteria edge cases |
Please attach terminal screenshots to the description |
|
Added terminal screenshots to the PR description as requested. All shared package tests are passing successfully. |
| where: { id: userId }, | ||
| select: { username: true }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Catch position should be here
There was a problem hiding this comment.
Refactored the code to narrow the try/catch scope as suggested. Thanks for the review!
| const card = await app.prisma.card.findUnique({ | ||
| where: { id: cardId }, | ||
| select: { userId: true }, | ||
| }); |
There was a problem hiding this comment.
Catch position should be here.
There was a problem hiding this comment.
Updated this section as well to keep the try/catch scoped only around the database call. Thanks for catching that.
|
Hey @ShantKhatri! Harxhit has reviewed and confirmed the changes look good. Could you do the final PA review when you get a chance? |
|
@ShantKhatri Looks good to me your final review please. |

Closes #35
Changes: