Validate affiliate conversion ids before mutation queries#459
Conversation
Greptile SummaryThis PR tightens
Confidence Score: 4/5Safe to merge — the validation logic is correct and the existing conversion flows are unchanged. The core guard and trim logic in route.ts is solid. The test additions cover the stated regression cases, but the whitespace-only string path (a newly rejected case) and DELETE’s success/auth/status branches are untested, leaving some branches dark. route.test.ts — the DELETE describe block and the missing whitespace-only assertion would benefit from a bit more coverage before shipping. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant PUT/DELETE Handler
participant affiliate_offers
participant affiliate_conversions
Client->>PUT/DELETE Handler: Request with conversion_id
PUT/DELETE Handler->>affiliate_offers: SELECT (auth/ownership check)
affiliate_offers-->>PUT/DELETE Handler: offer row
PUT/DELETE Handler->>PUT/DELETE Handler: isNonEmptyString(conversion_id)?
alt non-string or whitespace-only
PUT/DELETE Handler-->>Client: 400 conversion_id is required
else valid string
PUT/DELETE Handler->>PUT/DELETE Handler: conversionId = conversion_id.trim()
PUT/DELETE Handler->>affiliate_conversions: UPDATE/SELECT+DELETE .eq("id", conversionId)
affiliate_conversions-->>PUT/DELETE Handler: result
PUT/DELETE Handler-->>Client: 200 ok / 400 / 404
end
Reviews (1): Last reviewed commit: "Validate affiliate conversion ids" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("rejects non-string conversion_id before updating conversions", async () => { | ||
| mockGetAuthContext.mockResolvedValue({ | ||
| user: { id: "user-seller", authMethod: "session" }, | ||
| }); | ||
|
|
||
| mockFrom.mockImplementation((table: string) => { | ||
| if (table === "affiliate_offers") { | ||
| return chainable({ | ||
| id: "offer-1", | ||
| seller_id: "user-seller", | ||
| commission_rate: 0.2, | ||
| commission_type: "percentage", | ||
| commission_flat_sats: 0, | ||
| }); | ||
| } | ||
| return chainable([]); | ||
| }); | ||
|
|
||
| const res = await PUT( | ||
| makePutRequest("offer-1", { | ||
| conversion_id: { id: "conv-1" }, | ||
| note: "paid elsewhere", | ||
| }), | ||
| makeParams("offer-1") | ||
| ); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| await expect(res.json()).resolves.toEqual({ error: "conversion_id is required" }); | ||
| expect(mockFrom).not.toHaveBeenCalledWith("affiliate_conversions"); | ||
| }); |
There was a problem hiding this comment.
conversion_id not tested
The new isNonEmptyString guard rejects whitespace-only strings (e.g., " "), which is a behavior change from the old !conversion_id check — a whitespace string is truthy and previously would have been forwarded to Supabase. Neither the PUT nor the DELETE new tests cover this path, so that specific validation branch has no regression coverage.
| describe("DELETE /api/affiliates/offers/[id]/conversions", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("rejects non-string conversion_id before querying conversions", async () => { | ||
| mockGetAuthContext.mockResolvedValue({ | ||
| user: { id: "user-seller", authMethod: "session" }, | ||
| }); | ||
|
|
||
| mockFrom.mockImplementation((table: string) => { | ||
| if (table === "affiliate_offers") { | ||
| return chainable({ | ||
| id: "offer-1", | ||
| seller_id: "user-seller", | ||
| }); | ||
| } | ||
| return chainable([]); | ||
| }); | ||
|
|
||
| const res = await DELETE( | ||
| makeDeleteRequest("offer-1", { | ||
| conversion_id: ["conv-1"], | ||
| }), | ||
| makeParams("offer-1") | ||
| ); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| await expect(res.json()).resolves.toEqual({ error: "conversion_id is required" }); | ||
| expect(mockFrom).not.toHaveBeenCalledWith("affiliate_conversions"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new describe("DELETE ...") block adds a single rejection test. The DELETE handler also guards against unauthorized users, non-existent conversions (404), and deleting paid conversions — none of which are covered. A regression on any of those branches 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 #459. Verification:
uGig invoice evidence has been sent for this PR. |
2 participants
Closes #458.
Summary
conversion_idto be a non-empty string for affiliate conversion PUT and DELETE mutations.affiliate_conversions.Verification
corepack pnpm vitest run 'src/app/api/affiliates/offers/[id]/conversions/route.test.ts'corepack pnpm tsc --noEmit