Allow reopening of registration phase and submission start / end date fix #50
Conversation
Allow for Topgear Submission type phase when calculating start / end dates (PS-444)
Allow reopening of registration phase
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const { ensureAcessibilityToModifiedGroups } = require("./group-helper"); | ||
| const { ChallengeStatusEnum } = require("@prisma/client"); | ||
|
|
||
| const SUBMISSION_PHASE_PRIORITY = ["Topcoder Submission", "Submission"]; |
There was a problem hiding this comment.
[💡 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.
| 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]) || |
There was a problem hiding this comment.
[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.
|
|
||
| const registrationPhase = _.find(phases, (p) => p.name === "Registration"); | ||
| const submissionPhase = | ||
| _.find(phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[0]) || |
There was a problem hiding this comment.
[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.
| }); | ||
|
|
||
| if (dependentOpenPhases.length === 0) { | ||
| const normalizedPhaseName = normalizePhaseName(phaseName); |
There was a problem hiding this comment.
[💡 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.
| if (dependentOpenPhases.length === 0) { | ||
| const normalizedPhaseName = normalizePhaseName(phaseName); | ||
| const hasSubmissionVariantOpen = openPhases.some((phase) => | ||
| SUBMISSION_PHASE_NAME_SET.has(normalizePhaseName(phase?.name)) |
There was a problem hiding this comment.
[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.
| }) | ||
|
|
||
| try { | ||
| const challengePhase = await service.partiallyUpdateChallengePhase( |
There was a problem hiding this comment.
[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.
| should.equal(challengePhase.isOpen, true) | ||
| should.equal(challengePhase.actualEndDate, null) | ||
| } finally { | ||
| await prisma.challengePhase.update({ |
There was a problem hiding this comment.
[❗❗ 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.
https://topcoder.atlassian.net/browse/PS-444
https://topcoder.atlassian.net/browse/PS-445