Fix some issues with iterative review handling, as well as review opportunity creation #24
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| challengeId: string; // Changed to string to support UUIDs | ||
| preventFinalization?: boolean; | ||
| // When closing Iterative Review phases after a passing score, skip follow-up iterative processing | ||
| skipIterativePhaseRefresh?: boolean; |
There was a problem hiding this comment.
[maintainability]
Consider clarifying the purpose and impact of skipIterativePhaseRefresh. While the name suggests skipping a phase refresh, it might be beneficial to ensure this behavior is well-documented and understood by the team, especially if it affects the workflow significantly.
|
|
||
| let existing: any[] = []; | ||
| try { | ||
| existing = await this.reviewApiService.getReviewOpportunitiesByChallengeId( |
There was a problem hiding this comment.
[maintainability]
Consider handling specific error types when fetching existing review opportunities to provide more detailed error handling and potentially retry logic for transient errors.
|
|
||
| for (const reviewer of reviewers) { | ||
| const type = | ||
| reviewer.type?.toString().trim().toUpperCase() || 'REGULAR_REVIEW'; |
There was a problem hiding this comment.
[💡 maintainability]
The default type 'REGULAR_REVIEW' is hardcoded. Consider defining this as a constant or configuration to improve maintainability and avoid magic strings.
| phase.scheduledStartDate || phase.actualStartDate || null; | ||
| const duration = this.resolvePhaseDuration(phase); | ||
|
|
||
| if (!startDate || duration <= 0) { |
There was a problem hiding this comment.
[correctness]
The check for duration <= 0 could be improved by explicitly checking for duration === 0 or negative values separately, as they might indicate different issues.
| incrementalPayment = basePayment; | ||
| } | ||
|
|
||
| if (basePayment <= 0 || incrementalPayment <= 0) { |
There was a problem hiding this comment.
[maintainability]
The logic for computing payments results in skipping opportunity creation if either basePayment or incrementalPayment is non-positive. Consider logging more detailed information about the reviewer and challenge to aid in debugging why payments are non-positive.
| hasSubmitterResource: jest.fn().mockResolvedValue(true), | ||
| getResourcesByRoleNames: jest.fn().mockResolvedValue([]), | ||
| getReviewerResources: jest.fn().mockResolvedValue([]), | ||
| getReviewerResources: jest.fn().mockResolvedValue([{} as any]), |
There was a problem hiding this comment.
[maintainability]
Using {} as any for getReviewerResources return value could lead to runtime errors if the expected structure is not met. Consider defining a more specific type for the reviewer resources to ensure type safety.
| }); | ||
|
|
||
| challengeApiService.getPhaseDetails.mockResolvedValue(phaseDetails); | ||
| reviewService.getCompletedReviewCountForPhase.mockResolvedValue(0); |
There was a problem hiding this comment.
[correctness]
The test defers closing review phases when no reviews are completed relies on getCompletedReviewCountForPhase returning 0. Ensure that this mock accurately reflects the real-world scenario where no reviews are completed, as it could affect the test's validity.
|
|
||
| await scheduler.advancePhase(payload); | ||
|
|
||
| expect(first2FinishService.handleIterativePhaseClosed).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[correctness]
The test skips iterative submission refresh when instructed checks that handleIterativePhaseClosed is not called. Ensure that the logic for skipping the refresh is correctly implemented and that this test covers all relevant conditions.
| true, | ||
| ); | ||
|
|
||
| if (coverage.expected <= 0) { |
There was a problem hiding this comment.
[maintainability]
The check for coverage.expected <= 0 is repeated in two places (lines 588 and 611). Consider refactoring this logic into a separate method to improve maintainability and reduce code duplication.
| data.phaseId, | ||
| ); | ||
|
|
||
| if (completedReviews <= 0) { |
There was a problem hiding this comment.
[maintainability]
The condition completedReviews <= 0 is checked, but the logic for handling this case is very similar to the logic for coverage.expected <= 0. Consider consolidating these checks to avoid redundancy and improve maintainability.
| ) ?? []; | ||
|
|
||
| if ( | ||
| appealsSuccessors.length === 0 && |
There was a problem hiding this comment.
[💡 readability]
The logic for determining appealsSuccessors is complex and involves multiple conditions. Consider simplifying this logic or adding helper methods to make it easier to understand and maintain.
| if ( | ||
| operation === 'close' && | ||
| phaseName === ITERATIVE_REVIEW_PHASE_NAME && | ||
| !data.skipIterativePhaseRefresh |
There was a problem hiding this comment.
[❗❗ correctness]
The added condition !data.skipIterativePhaseRefresh is a critical change. Ensure that this flag is correctly set and documented elsewhere in the codebase to avoid unexpected behavior.
https://topcoder.atlassian.net/browse/PM-2312