Reject fractional testimonial ratings before database updates#473
Conversation
Greptile SummaryThis PR tightens testimonial rating validation by replacing the
Confidence Score: 5/5Safe to merge — the change is a small, targeted guard swap with no new database interactions or behavioral side-effects. Both handlers are updated consistently, Number.isInteger correctly rejects floats while implicitly handling non-number types, and two focused regression tests confirm the guard fires before any write reaches the database. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PATCH /api/testimonials/:id] --> B{Auth check}
B -- Unauthorized --> C[401]
B -- OK --> D[SELECT testimonial from DB]
D -- Not found --> E[404]
D -- Found --> F{rating provided?}
F -- No --> G{status provided?}
F -- Yes --> H{Number.isInteger AND 1–5?}
H -- No --> I[400 'Rating must be an integer from 1-5']
H -- Yes --> J[updateData.rating = rating]
J --> K[UPDATE testimonials in DB]
K --> L[200 testimonial]
G -- Invalid --> M[400]
G -- approved/rejected --> N[Permission check]
N -- No permission --> O[403]
N -- OK --> P[UPDATE status in DB]
P --> Q[200 testimonial]
Reviews (2): Last reviewed commit: "Validate testimonial creation ratings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("rejects fractional ratings before updating the testimonial", async () => { | ||
| const response = await PATCH(makeRequest(4.5), { | ||
| params: Promise.resolve({ id: "testimonial-1" }), | ||
| }); | ||
| const body = await response.json(); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(body.error).toBe("Rating must be an integer from 1-5"); | ||
| expect(mocks.mockUpdate).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test suite only exercises the fractional-rejection path. Complementary cases that would give stronger confidence in the guard are: a valid integer (e.g. 3) that should reach the database, and out-of-range integers (0, 6) that should also return 400. Without a passing happy-path case there is no test that confirms mockUpdate is called when a valid rating is supplied, so a regression that accidentally broadens the guard (e.g. rejecting all numbers) would go undetected.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
CI is green for PR #473. Verification:
uGig invoice evidence will be sent after the retry window expires. |
|
CI is green after expanding this PR to cover both testimonial creation and edits. Verification:
uGig invoice evidence has been sent for this PR. |
2 participants
Fixes #472.
Changes
Verification
corepack pnpm vitest run 'src/app/api/testimonials/[id]/route.test.ts' src/app/api/testimonials/route.rating.test.tscorepack pnpm tsc --noEmit