PM-4318 move to next phase when manually closing#80
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const endChanged = !datesAreSame(successor.scheduledEndDate, desiredEndDate); | ||
|
|
||
| if (startChanged || endChanged) { | ||
| successorForQueue = await tx.challengePhase.update({ |
There was a problem hiding this comment.
[performance]
The update operation inside the loop could lead to performance issues if there are many successor phases. Consider batching updates or using a more efficient mechanism to reduce the number of database transactions.
| await recalculateDependentPhaseDates(tx, challengeId, updatedPhase, currentUserId); | ||
| } | ||
| } | ||
| if (isClosingPhase && !_.isNil(originalScheduledEndDate) && !_.isNil(updatedPhase.actualEndDate)) { |
There was a problem hiding this comment.
[❗❗ correctness]
The logic for calculating scheduledEndTime and actualEndTime should handle potential NaN values more explicitly to avoid unexpected behavior if dates are malformed or missing.
| data.challenge.id, | ||
| data.challengePhase2Id, | ||
| { | ||
| isOpen: false |
There was a problem hiding this comment.
[correctness]
Consider adding validation to ensure that the actualEndDate is set before closing the phase. This will prevent potential issues if the phase is closed without an end date, which could lead to incorrect scheduling of successor phases.
|
|
||
| actualEndMs.should.be.at.least(before.getTime()) | ||
| actualEndMs.should.be.at.most(after.getTime()) | ||
| successorStartMs.should.equal(actualEndMs) |
There was a problem hiding this comment.
[💡 design]
The test assumes that the successor phase's start date should exactly match the actualEndDate of the closed phase. Ensure that this behavior is intended and documented, as it might not be obvious to all developers.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…if there are pending escalation requests
…-close-review-when-escalations-pending PM-4363 - do not allow review phase to be clsoed, appeals to be open …
| `Found AI review configuration ${aiReviewConfig.id} for challenge ${challengeId}` | ||
| ); | ||
|
|
||
| const reviewPrisma = getReviewClient(); |
There was a problem hiding this comment.
[performance]
Consider using a single instance of the review client for the entire service to avoid creating multiple connections, which can lead to resource exhaustion.
|
|
||
| // Check if this is the Appeals phase | ||
| const normalizedPhaseName = normalizePhaseName(phaseName); | ||
| if (normalizedPhaseName === "appeals") { |
There was a problem hiding this comment.
[correctness]
The check for pending escalation requests is performed only when opening the Appeals phase. Ensure that this logic aligns with the business requirements, as it might be necessary to check for pending escalations in other phases as well.
| } | ||
| if ( | ||
| String(closingPhaseName || "").toLowerCase() === "appeals response" && | ||
| normalizedClosingPhaseName === "review" && |
There was a problem hiding this comment.
[correctness]
The check for pending escalation requests when closing the Review phase is similar to the check when opening the Appeals phase. Ensure consistency in how these checks are applied across different phases.
…thub.com:topcoder-platform/challenge-api-v6 into PM-4318_move-to-next-phase-when-manually-closing
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| id: challengePhase.id, | ||
| }, | ||
| }); | ||
| let scheduleExtended = false; |
There was a problem hiding this comment.
[💡 readability]
The variable scheduleExtended is declared with let and then reassigned. Consider using const for the initial declaration and reassigning only if necessary to improve code clarity and prevent accidental reassignment.
| await recalculateDependentPhaseDates(tx, challengeId, updatedPhase, currentUserId); | ||
| } | ||
| } | ||
| if (isClosingPhase && !_.isNil(originalScheduledEndDate) && !_.isNil(updatedPhase.actualEndDate)) { |
There was a problem hiding this comment.
[💡 readability]
The logic for determining shiftBaselineScheduledEndDate could be simplified by directly assigning the value based on the condition, which would enhance readability.
2 participants
https://topcoder.atlassian.net/browse/PM-4318 - When submission phase is manually closed, the AI screening phase doesn't get opened even if all AI reviews are completed
Make sure phases are recalculated and autopilot is advancing phases when user intervenes in timeline
Challenge Phase Scheduling Improvements:
shiftDependentPhaseDates, toChallengePhaseService.jsthat shifts the scheduled start and end dates of all successor phases when a phase is closed with an actual end date different from its scheduled end date. This ensures phase timelines remain consistent when phases are closed early or late.partiallyUpdateChallengePhaseto callshiftDependentPhaseDateswhen closing a phase, applying the time delta to all dependent phases.AI Screening Phase Closure Logic Refactor:
extractSubmissionId,ensureChallengeHasAiReviewers, andensureAIScreeningCanBeClosedfromChallengePhaseService.jstoChallengeService.jsfor better separation of concerns and to avoid circular dependencies.