Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/services/ChallengePhaseService.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ const PHASE_RESOURCE_ROLE_REQUIREMENTS = Object.freeze({
review: "Reviewer",
"checkpoint review": "Checkpoint Reviewer",
});
const SUBMISSION_PHASE_NAME_SET = new Set(["submission", "topgear submission"]);
const REGISTRATION_PHASE_NAME = "registration";

const normalizePhaseName = (name) => String(name || "").trim().toLowerCase();

async function hasPendingScorecardsForPhase(challengePhaseId) {
if (!config.REVIEW_DB_URL) {
Expand Down Expand Up @@ -561,7 +565,14 @@ async function partiallyUpdateChallengePhase(currentUser, challengeId, id, data)
return reopenedPhaseIdentifiers.has(String(phase.predecessor));
});

if (dependentOpenPhases.length === 0) {
const normalizedPhaseName = normalizePhaseName(phaseName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 performance]
The normalizePhaseName function is called twice for each phase in openPhases when checking hasSubmissionVariantOpen. Consider normalizing phase?.name once before the some method to improve performance slightly.

const hasSubmissionVariantOpen = openPhases.some((phase) =>
SUBMISSION_PHASE_NAME_SET.has(normalizePhaseName(phase?.name))
);
const allowRegistrationReopenWithoutExplicitDependency =
normalizedPhaseName === REGISTRATION_PHASE_NAME && hasSubmissionVariantOpen;

if (dependentOpenPhases.length === 0 && !allowRegistrationReopenWithoutExplicitDependency) {
throw new errors.ForbiddenError(
`Cannot reopen ${phaseName} because no currently open phase depends on it`
);
Expand Down
67 changes: 65 additions & 2 deletions test/unit/ChallengePhaseService.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ describe('challenge phase service unit tests', () => {
}
})

it('partially update challenge phase - cannot reopen when open phase is not a successor', async () => {
it('partially update challenge phase - can reopen registration when submission is open', async () => {
const startDate = new Date('2025-06-01T00:00:00.000Z')
const endDate = new Date('2025-06-02T00:00:00.000Z')

Expand All @@ -350,6 +350,60 @@ describe('challenge phase service unit tests', () => {
}
})

try {
const challengePhase = await service.partiallyUpdateChallengePhase(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The try block is missing a catch clause. If service.partiallyUpdateChallengePhase throws an error, it will be silently ignored, which could lead to undetected test failures.

authUser,
data.challenge.id,
data.challengePhase1Id,
{
isOpen: true
}
)
should.equal(challengePhase.id, data.challengePhase1Id)
should.equal(challengePhase.isOpen, true)
should.equal(challengePhase.actualEndDate, null)
} finally {
await prisma.challengePhase.update({
where: { id: data.challengePhase1Id },
data: {
isOpen: false,
actualStartDate: startDate,
actualEndDate: endDate
}
})
await prisma.challengePhase.update({
where: { id: data.challengePhase2Id },
data: {
isOpen: false,
predecessor: data.challengePhase1Id,
name: 'Submission'
}
})
}

})

it('partially update challenge phase - cannot reopen when open phase is not a successor or submission variant', async () => {
const startDate = new Date('2025-06-01T00:00:00.000Z')
const endDate = new Date('2025-06-02T00:00:00.000Z')

await prisma.challengePhase.update({
where: { id: data.challengePhase1Id },
data: {
isOpen: false,
actualStartDate: startDate,
actualEndDate: endDate
}
})
await prisma.challengePhase.update({
where: { id: data.challengePhase2Id },
data: {
isOpen: true,
predecessor: null,
name: 'Review'
}
})

try {
await service.partiallyUpdateChallengePhase(authUser, data.challenge.id, data.challengePhase1Id, {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The try block is missing a catch clause. If service.partiallyUpdateChallengePhase throws an error, it will be silently ignored, which could lead to undetected test failures.

isOpen: true
Expand All @@ -366,7 +420,16 @@ describe('challenge phase service unit tests', () => {
where: { id: data.challengePhase2Id },
data: {
isOpen: false,
predecessor: data.challengePhase1Id
predecessor: data.challengePhase1Id,
name: 'Submission'
}
})
await prisma.challengePhase.update({
where: { id: data.challengePhase1Id },
data: {
isOpen: false,
actualStartDate: startDate,
actualEndDate: endDate
}
})
}
Expand Down