Skip to content

Commit f1b0941

Browse files
fix: address AI code review feedback
- Wrap dispute creation in database transaction for atomicity - Change evidenceLinks to evidence_links (snake_case) in API schema - Update all test cases to use new field name - Ensure both insert and update operations rollback on failure Addresses review comments on PR #108
1 parent 0b242d5 commit f1b0941

2 files changed

Lines changed: 72 additions & 58 deletions

File tree

packages/api/src/__tests__/submissions.test.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ describe('POST /api/submissions/:id/dispute', () => {
280280
Authorization: 'Bearer invalid.token',
281281
'Content-Type': 'application/json',
282282
},
283-
body: JSON.stringify({ reason: 'Test dispute', evidenceLinks: [] }),
283+
body: JSON.stringify({ reason: 'Test dispute', evidence_links: [] }),
284284
});
285285

286286
expect(res.status).toBe(401);
@@ -306,7 +306,7 @@ describe('POST /api/submissions/:id/dispute', () => {
306306
Authorization: 'Bearer valid.token',
307307
'Content-Type': 'application/json',
308308
},
309-
body: JSON.stringify({ evidenceLinks: [] }),
309+
body: JSON.stringify({ evidence_links: [] }),
310310
});
311311

312312
expect(res.status).toBe(400);
@@ -324,7 +324,7 @@ describe('POST /api/submissions/:id/dispute', () => {
324324
Authorization: 'Bearer valid.token',
325325
'Content-Type': 'application/json',
326326
},
327-
body: JSON.stringify({ reason: 'This is unfair!', evidenceLinks: [] }),
327+
body: JSON.stringify({ reason: 'This is unfair!', evidence_links: [] }),
328328
});
329329

330330
expect(res.status).toBe(404);
@@ -349,7 +349,7 @@ describe('POST /api/submissions/:id/dispute', () => {
349349
Authorization: 'Bearer valid.token',
350350
'Content-Type': 'application/json',
351351
},
352-
body: JSON.stringify({ reason: 'This is unfair!', evidenceLinks: [] }),
352+
body: JSON.stringify({ reason: 'This is unfair!', evidence_links: [] }),
353353
});
354354

355355
expect(res.status).toBe(400);
@@ -391,7 +391,7 @@ describe('POST /api/submissions/:id/dispute', () => {
391391
Authorization: 'Bearer valid.token',
392392
'Content-Type': 'application/json',
393393
},
394-
body: JSON.stringify({ reason: 'This is unfair!', evidenceLinks: [] }),
394+
body: JSON.stringify({ reason: 'This is unfair!', evidence_links: [] }),
395395
});
396396

397397
expect(res.status).toBe(409);
@@ -410,7 +410,7 @@ describe('POST /api/submissions/:id/dispute', () => {
410410
id: 'd-new',
411411
submissionId: '123e4567-e89b-12d3-a456-426614174000',
412412
reason: 'The code meets all requirements',
413-
evidenceLinks: ['https://example.com/proof'],
413+
evidence_links: ['https://example.com/proof'],
414414
status: 'open',
415415
createdAt: new Date(),
416416
};
@@ -455,7 +455,7 @@ describe('POST /api/submissions/:id/dispute', () => {
455455
},
456456
body: JSON.stringify({
457457
reason: 'The code meets all requirements',
458-
evidenceLinks: ['https://example.com/proof'],
458+
evidence_links: ['https://example.com/proof'],
459459
}),
460460
});
461461

packages/api/src/routes/submissions.ts

Lines changed: 65 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ submissionsRouter.get(
112112

113113
const disputeSchema = z.object({
114114
reason: z.string().min(1).max(2000),
115-
evidenceLinks: z.array(z.string().url()).optional().default([]),
115+
evidence_links: z.array(z.string().url()).optional().default([]),
116116
});
117117

118118
/**
@@ -131,61 +131,75 @@ submissionsRouter.post(
131131
}
132132

133133
const { id } = c.req.valid('param');
134-
const { reason, evidenceLinks } = c.req.valid('json');
135-
136-
// Check if submission exists and belongs to user
137-
const [submission] = await db
138-
.select()
139-
.from(submissions)
140-
.where(and(
141-
eq(submissions.id, id),
142-
eq(submissions.developerId, user.id)
143-
));
144-
145-
if (!submission) {
146-
return c.json({ error: 'Submission not found' }, 404);
147-
}
134+
const { reason, evidence_links } = c.req.valid('json');
135+
136+
// Use transaction to ensure atomicity
137+
const result = await db.transaction(async (tx) => {
138+
// Check if submission exists and belongs to user
139+
const [submission] = await tx
140+
.select()
141+
.from(submissions)
142+
.where(and(
143+
eq(submissions.id, id),
144+
eq(submissions.developerId, user.id)
145+
));
146+
147+
if (!submission) {
148+
return { error: 'Submission not found', status: 404 };
149+
}
148150

149-
// Validate submission was rejected
150-
if (submission.status !== 'rejected') {
151-
return c.json({
152-
error: 'Only rejected submissions can be disputed',
153-
currentStatus: submission.status
154-
}, 400);
155-
}
151+
// Validate submission was rejected
152+
if (submission.status !== 'rejected') {
153+
return {
154+
error: 'Only rejected submissions can be disputed',
155+
currentStatus: submission.status,
156+
status: 400
157+
};
158+
}
156159

157-
// Check if dispute already exists
158-
const [existingDispute] = await db
159-
.select()
160-
.from(disputes)
161-
.where(eq(disputes.submissionId, id));
162-
163-
if (existingDispute) {
164-
return c.json({
165-
error: 'Dispute already exists for this submission',
166-
disputeId: existingDispute.id
167-
}, 409);
168-
}
160+
// Check if dispute already exists
161+
const [existingDispute] = await tx
162+
.select()
163+
.from(disputes)
164+
.where(eq(disputes.submissionId, id));
165+
166+
if (existingDispute) {
167+
return {
168+
error: 'Dispute already exists for this submission',
169+
disputeId: existingDispute.id,
170+
status: 409
171+
};
172+
}
169173

170-
// Create dispute record
171-
const [dispute] = await db
172-
.insert(disputes)
173-
.values({
174-
submissionId: id,
175-
reason,
176-
evidenceLinks,
177-
status: 'open',
178-
})
179-
.returning();
180-
181-
// Update submission status to disputed
182-
await db
183-
.update(submissions)
184-
.set({ status: 'disputed' })
185-
.where(eq(submissions.id, id));
174+
// Create dispute record
175+
const [dispute] = await tx
176+
.insert(disputes)
177+
.values({
178+
submissionId: id,
179+
reason,
180+
evidenceLinks: evidence_links,
181+
status: 'open',
182+
})
183+
.returning();
184+
185+
// Update submission status to disputed
186+
await tx
187+
.update(submissions)
188+
.set({ status: 'disputed' })
189+
.where(eq(submissions.id, id));
190+
191+
return { dispute, status: 201 };
192+
});
193+
194+
if (result.error) {
195+
return c.json(
196+
{ error: result.error, disputeId: result.disputeId, currentStatus: result.currentStatus },
197+
result.status as 400 | 404 | 409
198+
);
199+
}
186200

187201
return c.json({
188-
data: dispute,
202+
data: result.dispute,
189203
message: 'Dispute opened successfully',
190204
}, 201);
191205
}

0 commit comments

Comments
 (0)