Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const [, winners] = challengeApiService.completeChallenge.mock.calls[0]; | ||
| expect(winners).toHaveLength(2); | ||
| expect(winners).toHaveLength(3); |
There was a problem hiding this comment.
[correctness]
The change from expect(winners).toHaveLength(2); to expect(winners).toHaveLength(3); suggests that the logic now allows for more winners than placement prizes. Ensure that this change aligns with the business requirements and that there is no unintended impact on prize distribution.
| expect(winners.map((winner) => winner.userId)).toEqual([101, 102]); | ||
| expect(winners.map((winner) => winner.placement)).toEqual([1, 2]); | ||
| expect(winners).toHaveLength(3); | ||
| expect(winners.map((winner) => winner.userId)).toEqual([101, 101, 102]); |
There was a problem hiding this comment.
[correctness]
The expectation expect(winners.map((winner) => winner.userId)).toEqual([101, 101, 102]); implies that a single user can have multiple placements. Verify that this behavior is intended and that it does not violate any business rules regarding unique placements per user.
| 'Topcoder Post Mortem', | ||
| ); | ||
| } catch (_) { | ||
| } catch { |
There was a problem hiding this comment.
[💡 maintainability]
The change from catch (_) to catch removes the unused variable _, which is a good practice. However, ensure that the logging inside the reviewService is comprehensive enough to capture all necessary error details, as this catch block no longer handles any specific error.
| if (timeA === timeB) { | ||
| return 0; | ||
| const timeDiff = | ||
| this.getSubmissionTimestamp(a.submittedDate) - |
There was a problem hiding this comment.
[correctness]
The method getSubmissionTimestamp is used to calculate the time difference between submissions. Ensure that the logic for handling null dates (returning 0) aligns with the intended sorting behavior, as this could affect the order of submissions with null dates.
| } | ||
| const winners = [...placementWinners, ...passedReviewWinners]; | ||
|
|
||
| await this.challengeApiService.completeChallenge(challengeId, winners); |
There was a problem hiding this comment.
[correctness]
The completeChallenge method is called with both placementWinners and passedReviewWinners combined into winners. Ensure that the downstream logic in completeChallenge correctly handles both types of winners, as this could affect the challenge completion process.
| REVIEW_PHASE_NAMES.has(data.phaseTypeName); | ||
|
|
||
| if (operation === 'open' && isReviewPhase) { | ||
| const canOpenNow = |
There was a problem hiding this comment.
[❗❗ correctness]
The ensureAssignmentsOrSchedule method is called with an async callback that immediately calls advancePhase. This could potentially lead to a recursive call if advancePhase ends up calling this block again. Ensure that there is a termination condition or a mechanism to prevent infinite recursion.
| }, | ||
| ); | ||
|
|
||
| if (!canOpenNow) { |
There was a problem hiding this comment.
[💡 maintainability]
The return statement here exits the function if canOpenNow is false, but it might be more informative to log why the phase cannot be opened. Consider adding a log statement before returning.
| { | ||
| userId: 456, | ||
| handle: 'runner-up', | ||
| placement: 1, |
There was a problem hiding this comment.
[❗❗ correctness]
The placement value for the 'runner-up' is set to 1, which is the same as the 'winner'. This might be a mistake if placements are supposed to be unique. Verify if this is intentional.
| where: { | ||
| challengeId, | ||
| type: { | ||
| in: [PrizeSetTypeEnum.PLACEMENT, PrizeSetTypeEnum.PASSED_REVIEW], |
There was a problem hiding this comment.
[correctness]
Consider verifying that PrizeSetTypeEnum.PASSED_REVIEW is a valid type for the context of this operation. If this type is not expected to be used here, it could lead to unintended deletions.
| placement: winner.placement, | ||
| type: PrizeSetTypeEnum.PLACEMENT, | ||
| type: | ||
| winner.type === PrizeSetTypeEnum.PASSED_REVIEW |
There was a problem hiding this comment.
[correctness]
The conditional logic here assumes that winner.type can only be PrizeSetTypeEnum.PASSED_REVIEW or defaults to PrizeSetTypeEnum.PLACEMENT. Ensure that winner.type is validated before this point to prevent unexpected behavior if other types are introduced.
| handle: string; | ||
| placement: number; | ||
| type?: string; | ||
| type?: PrizeSetTypeEnum; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the type of type from string to PrizeSetTypeEnum is a breaking change if this interface is used in contexts expecting a string. Ensure that all usages of IChallengeWinner are updated accordingly to handle PrizeSetTypeEnum values. If backward compatibility is needed, consider providing a migration path or versioning the API.
1 participant
https://topcoder.atlassian.net/browse/PM-3902