Skip to content

Commit aa36b05

Browse files
jmgasperjmgasper
committed
PM-5758: enforce design submission limits atomically
What was broken The submission API accepted concurrent requests beyond a finite Design limit, finite counts above one were reduced to latest-only review behavior, and review creation could advance submissions that had not passed the preceding screening phase. Root cause Submission limits were enforced only by client-side read-before-write checks. Review eligibility used a boolean latest flag instead of the configured count, and pending and direct review paths lacked positive screening-pass gates. What was changed Added transaction-scoped advisory locking around same-member, same-type count and create operations for finite Design limits. Added a shared current-and-legacy metadata policy, exact-type submission ranks, latest-X screening and review eligibility, standard Screening-to-Review and Checkpoint Screening-to-Checkpoint Review pass gates, and a direct standard Review pass check. Any added/updated tests Added metadata policy parity tests and expanded submission and review service coverage for atomic limits, concurrent final-slot requests, checkpoint and contest isolation, legacy metadata aliases, latest-X ranks, Development latest-only behavior, and passing, failed, or missing screening results.
1 parent 7a93489 commit aa36b05

6 files changed

Lines changed: 1694 additions & 258 deletions

File tree

‎src/api/review/review.service.spec.ts‎

Lines changed: 225 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { JwtUser } from 'src/shared/modules/global/jwt.service';
99
import { ChallengeStatus } from 'src/shared/enums/challengeStatus.enum';
1010
import { UserRole } from 'src/shared/enums/userRole.enum';
1111
import { CommonConfig } from 'src/shared/config/common.config';
12+
import { ScorecardType, SubmissionType } from '@prisma/client';
1213

1314
describe('ReviewService.createReview authorization checks', () => {
1415
const prismaMock = {
@@ -163,7 +164,8 @@ describe('ReviewService.createReview authorization checks', () => {
163164
id: 'submission-1',
164165
challengeId: 'challenge-1',
165166
memberId: null,
166-
isLatest: true,
167+
type: SubmissionType.CONTEST_SUBMISSION,
168+
submissionRank: BigInt(1),
167169
},
168170
]);
169171
prismaMock.reviewType.findUnique.mockResolvedValue({
@@ -212,7 +214,8 @@ describe('ReviewService.createReview authorization checks', () => {
212214
id: 'submission-older',
213215
challengeId: 'challenge-1',
214216
memberId: 'member-1',
215-
isLatest: false,
217+
type: SubmissionType.CONTEST_SUBMISSION,
218+
submissionRank: BigInt(2),
216219
},
217220
]);
218221

@@ -231,7 +234,8 @@ describe('ReviewService.createReview authorization checks', () => {
231234
id: submissionId,
232235
challengeId: 'challenge-1',
233236
memberId: 'member-1',
234-
isLatest: true,
237+
type: SubmissionType.CONTEST_SUBMISSION,
238+
submissionRank: BigInt(1),
235239
},
236240
]);
237241

@@ -257,6 +261,216 @@ describe('ReviewService.createReview authorization checks', () => {
257261
}
258262
});
259263

264+
it('allows a standard Review after the submission passes Screening', async () => {
265+
const submissionId = 'submission-screening-passed';
266+
prismaMock.$queryRaw
267+
.mockResolvedValueOnce([
268+
{
269+
id: submissionId,
270+
challengeId: 'challenge-1',
271+
memberId: 'member-1',
272+
type: SubmissionType.CONTEST_SUBMISSION,
273+
submissionRank: BigInt(1),
274+
},
275+
])
276+
.mockResolvedValueOnce([{ passed: true }]);
277+
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
278+
...baseChallengeDetail,
279+
phases: [
280+
{ id: 'phase-screening', name: 'Screening', isOpen: false },
281+
...baseChallengeDetail.phases,
282+
],
283+
});
284+
285+
const request = buildReviewRequest({ submissionId });
286+
const reviewCreateResult = buildReviewModel(request);
287+
prismaMock.review.create.mockResolvedValue(reviewCreateResult);
288+
prismaMock.review.update.mockResolvedValue(reviewCreateResult);
289+
const computeSpy = jest
290+
.spyOn(service as any, 'computeScoresFromItems')
291+
.mockResolvedValue({ initialScore: null, finalScore: null });
292+
293+
try {
294+
await expect(
295+
service.createReview(baseAuthUser, request),
296+
).resolves.toMatchObject({ submissionId });
297+
} finally {
298+
computeSpy.mockRestore();
299+
}
300+
301+
const screeningQuery = prismaMock.$queryRaw.mock.calls[1][0] as {
302+
strings: string[];
303+
values: unknown[];
304+
};
305+
expect(screeningQuery.strings.join(' ')).toContain('sc."type" =');
306+
expect(screeningQuery.values).toContain(ScorecardType.SCREENING);
307+
});
308+
309+
it.each([
310+
['failed', [{ passed: false }]],
311+
['missing', []],
312+
])(
313+
'rejects a standard Review when the Screening result is %s',
314+
async (_description, screeningRows) => {
315+
const submissionId = 'submission-screening-not-passed';
316+
prismaMock.$queryRaw
317+
.mockResolvedValueOnce([
318+
{
319+
id: submissionId,
320+
challengeId: 'challenge-1',
321+
memberId: 'member-1',
322+
type: SubmissionType.CONTEST_SUBMISSION,
323+
submissionRank: BigInt(1),
324+
},
325+
])
326+
.mockResolvedValueOnce(screeningRows);
327+
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
328+
...baseChallengeDetail,
329+
phases: [
330+
{ id: 'phase-screening', name: 'Screening', isOpen: false },
331+
...baseChallengeDetail.phases,
332+
],
333+
});
334+
335+
await expect(
336+
service.createReview(
337+
baseAuthUser,
338+
buildReviewRequest({ submissionId }),
339+
),
340+
).rejects.toMatchObject({
341+
response: expect.objectContaining({
342+
code: 'SUBMISSION_SCREENING_NOT_PASSED',
343+
details: expect.objectContaining({
344+
submissionId,
345+
requiredScorecardType: ScorecardType.SCREENING,
346+
}),
347+
}),
348+
});
349+
expect(prismaMock.review.create).not.toHaveBeenCalled();
350+
},
351+
);
352+
353+
it('allows the second-newest submission for a Design challenge limited to two', async () => {
354+
const submissionId = 'submission-second-newest';
355+
prismaMock.$queryRaw.mockResolvedValue([
356+
{
357+
id: submissionId,
358+
challengeId: 'challenge-1',
359+
memberId: 'member-1',
360+
type: SubmissionType.CONTEST_SUBMISSION,
361+
submissionRank: BigInt(2),
362+
},
363+
]);
364+
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
365+
...baseChallengeDetail,
366+
track: 'Design',
367+
metadata: {
368+
submissionLimit: JSON.stringify({
369+
maximum: '2',
370+
limit: 'yes',
371+
unlimited: 'no',
372+
}),
373+
},
374+
});
375+
376+
const request = buildReviewRequest({ submissionId });
377+
const reviewCreateResult = buildReviewModel(request);
378+
prismaMock.review.create.mockResolvedValue(reviewCreateResult);
379+
prismaMock.review.update.mockResolvedValue(reviewCreateResult);
380+
const computeSpy = jest
381+
.spyOn(service as any, 'computeScoresFromItems')
382+
.mockResolvedValue({ initialScore: null, finalScore: null });
383+
384+
try {
385+
await expect(
386+
service.createReview(baseAuthUser, request),
387+
).resolves.toMatchObject({ submissionId });
388+
} finally {
389+
computeSpy.mockRestore();
390+
}
391+
392+
const rankSql = prismaMock.$queryRaw.mock.calls[0][0] as {
393+
strings: string[];
394+
};
395+
const rankSqlText = rankSql.strings.join(' ');
396+
expect(rankSqlText).toContain(
397+
'PARTITION BY COALESCE(s."memberId", s."id"), s."type"',
398+
);
399+
expect(rankSqlText).toContain('s."type" IS NOT DISTINCT FROM');
400+
expect(rankSqlText).toContain(
401+
's."status" IS NULL OR s."status" <> \'DELETED\'',
402+
);
403+
});
404+
405+
it('rejects a Design submission outside a finite latest-X limit', async () => {
406+
prismaMock.$queryRaw.mockResolvedValue([
407+
{
408+
id: 'submission-third-newest',
409+
challengeId: 'challenge-1',
410+
memberId: 'member-1',
411+
type: SubmissionType.CONTEST_SUBMISSION,
412+
submissionRank: BigInt(3),
413+
},
414+
]);
415+
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
416+
...baseChallengeDetail,
417+
track: 'Design',
418+
metadata: {
419+
submissionLimit: JSON.stringify({
420+
count: '2',
421+
limit: 'true',
422+
unlimited: 'false',
423+
}),
424+
},
425+
});
426+
427+
await expect(
428+
service.createReview(
429+
baseAuthUser,
430+
buildReviewRequest({ submissionId: 'submission-third-newest' }),
431+
),
432+
).rejects.toMatchObject({
433+
response: expect.objectContaining({
434+
code: 'SUBMISSION_NOT_LATEST',
435+
details: expect.objectContaining({
436+
submissionRank: 3,
437+
submissionLimit: 2,
438+
}),
439+
}),
440+
});
441+
});
442+
443+
it('keeps Development challenges latest-only even with unlimited metadata', async () => {
444+
prismaMock.$queryRaw.mockResolvedValue([
445+
{
446+
id: 'development-older',
447+
challengeId: 'challenge-1',
448+
memberId: 'member-1',
449+
type: SubmissionType.CONTEST_SUBMISSION,
450+
submissionRank: BigInt(2),
451+
},
452+
]);
453+
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
454+
...baseChallengeDetail,
455+
metadata: {
456+
submissionLimit: JSON.stringify({
457+
count: '',
458+
limit: 'false',
459+
unlimited: 'true',
460+
}),
461+
},
462+
});
463+
464+
await expect(
465+
service.createReview(
466+
baseAuthUser,
467+
buildReviewRequest({ submissionId: 'development-older' }),
468+
),
469+
).rejects.toMatchObject({
470+
response: expect.objectContaining({ code: 'SUBMISSION_NOT_LATEST' }),
471+
});
472+
});
473+
260474
it('allows Post-Mortem review creation without submission when phase is open', async () => {
261475
const postMortemChallenge = {
262476
...baseChallengeDetail,
@@ -333,12 +547,14 @@ describe('ReviewService.createReview authorization checks', () => {
333547
id: submissionId,
334548
challengeId: 'challenge-1',
335549
memberId: 'member-1',
336-
isLatest: false,
550+
type: SubmissionType.CONTEST_SUBMISSION,
551+
submissionRank: BigInt(2),
337552
},
338553
]);
339554

340555
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
341556
...baseChallengeDetail,
557+
track: 'Design',
342558
metadata: {
343559
submissionLimit: { unlimited: 'TRUE' },
344560
},
@@ -373,12 +589,14 @@ describe('ReviewService.createReview authorization checks', () => {
373589
id: submissionId,
374590
challengeId: 'challenge-1',
375591
memberId: 'member-1',
376-
isLatest: false,
592+
type: SubmissionType.CONTEST_SUBMISSION,
593+
submissionRank: BigInt(2),
377594
},
378595
]);
379596

380597
challengeApiServiceMock.getChallengeDetail.mockResolvedValue({
381598
...baseChallengeDetail,
599+
track: 'Design',
382600
metadata: {
383601
submissionLimit: { unlimited: true },
384602
},
@@ -413,7 +631,8 @@ describe('ReviewService.createReview authorization checks', () => {
413631
id: submissionId,
414632
challengeId: 'challenge-1',
415633
memberId: null,
416-
isLatest: false,
634+
type: SubmissionType.CONTEST_SUBMISSION,
635+
submissionRank: BigInt(2),
417636
},
418637
]);
419638

0 commit comments

Comments
 (0)