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
6 changes: 5 additions & 1 deletion src/common/challenge-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const { hasAdminRole } = require("./role-helper");
const { ensureAcessibilityToModifiedGroups } = require("./group-helper");
const { ChallengeStatusEnum } = require("@prisma/client");

const SUBMISSION_PHASE_PRIORITY = ["Topcoder Submission", "Submission"];

Choose a reason for hiding this comment

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

[💡 maintainability]
Consider making SUBMISSION_PHASE_PRIORITY a constant outside of this file if it is used in multiple places across the codebase. This will improve maintainability by ensuring that changes to the priority list are centralized.


class ChallengeHelper {
/**
* @param {Object} challenge the challenge object
Expand Down Expand Up @@ -356,7 +358,9 @@ class ChallengeHelper {
enrichChallengeForResponse(challenge, track, type, options = {}) {
if (challenge.phases && challenge.phases.length > 0) {
const registrationPhase = _.find(challenge.phases, (p) => p.name === "Registration");
const submissionPhase = _.find(challenge.phases, (p) => p.name === "Submission");
const submissionPhase =
_.find(challenge.phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[0]) ||

Choose a reason for hiding this comment

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

[⚠️ performance]
The logic for finding the submissionPhase could be optimized by using a single _.find with a predicate that checks for both names in SUBMISSION_PHASE_PRIORITY. This reduces redundancy and improves performance slightly.

_.find(challenge.phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[1]);

// select last started open phase as current phase
_.forEach(challenge.phases, (p) => {
Expand Down
40 changes: 24 additions & 16 deletions src/common/prisma-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ const Decimal = require("decimal.js");
const constants = require("../../app-constants");
const { PrizeSetTypeEnum } = require("@prisma/client");
const { dedupeChallengeTerms } = require("./helper");

const SUBMISSION_PHASE_PRIORITY = ["Topcoder Submission", "Submission"];
/**
* Convert phases data to prisma model.
*
Expand All @@ -11,6 +13,7 @@ const { dedupeChallengeTerms } = require("./helper");
* @param {Object} auditFields createdBy and updatedBy
*/
function convertChallengePhaseSchema(challenge, result, auditFields) {
const phases = _.isArray(challenge.phases) ? challenge.phases : [];
// keep phase data
const phaseFields = [
"name",
Expand All @@ -25,24 +28,29 @@ function convertChallengePhaseSchema(challenge, result, auditFields) {
"challengeSource",
];
// current phase names
result.currentPhaseNames = _.map(
_.filter(challenge.phases, (p) => p.isOpen === true),
"name"
);
// get registration date and submission date
_.forEach(challenge.phases, (p) => {
if (p.name === "Registration") {
result.registrationStartDate = p.actualStartDate || p.scheduledStartDate;
result.registrationEndDate = p.actualEndDate || p.scheduledEndDate;
} else if (p.name === "Submission") {
result.submissionStartDate = p.actualStartDate || p.scheduledStartDate;
result.submissionEndDate = p.actualEndDate || p.scheduledEndDate;
}
});
result.currentPhaseNames = _.map(_.filter(phases, (p) => p.isOpen === true), "name");

const registrationPhase = _.find(phases, (p) => p.name === "Registration");
const submissionPhase =
_.find(phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[0]) ||

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of SUBMISSION_PHASE_PRIORITY array to find the submissionPhase is a good approach for flexibility. However, consider adding a check to ensure that at least one submission phase is found. If neither 'Topcoder Submission' nor 'Submission' phases exist, submissionPhase will be undefined, which might lead to unexpected behavior when accessing its properties later.

_.find(phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[1]);

if (registrationPhase) {
result.registrationStartDate =
registrationPhase.actualStartDate || registrationPhase.scheduledStartDate;
result.registrationEndDate =
registrationPhase.actualEndDate || registrationPhase.scheduledEndDate;
}
if (submissionPhase) {
result.submissionStartDate =
submissionPhase.actualStartDate || submissionPhase.scheduledStartDate;
result.submissionEndDate =
submissionPhase.actualEndDate || submissionPhase.scheduledEndDate;
}
// set phases array data
if (!_.isEmpty(challenge.phases)) {
if (!_.isEmpty(phases)) {
result.phases = {
create: _.map(challenge.phases, (p) => {
create: _.map(phases, (p) => {
const phaseData = {
phase: { connect: { id: p.phaseId } },
..._.pick(p, phaseFields),
Expand Down
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.

[💡 maintainability]
The variable normalizedPhaseName is declared but not used in the subsequent logic. Consider removing it if it's unnecessary or ensure it's used correctly if it was intended for a specific purpose.

const hasSubmissionVariantOpen = openPhases.some((phase) =>
SUBMISSION_PHASE_NAME_SET.has(normalizePhaseName(phase?.name))

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of optional chaining phase?.name is good for safety, but ensure that all code paths that rely on phase.name being non-null are correctly handled. If phase.name is null or undefined, normalizePhaseName will return an empty string, which may not be the intended behavior in all cases.

);
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.

[⚠️ maintainability]
Consider adding error handling for the partiallyUpdateChallengePhase call to ensure any exceptions are properly caught and logged. This will help in diagnosing issues if the update fails unexpectedly.

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({

Choose a reason for hiding this comment

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

[❗❗ correctness]
Ensure that the finally block correctly handles any exceptions that might occur during the prisma.challengePhase.update calls. If an error occurs here, it could leave the database in an inconsistent state.

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, {
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