Conversation
What was broken: Checkpoint winner selection could include submissions with the minimum possible checkpoint review score (for example 0.00), which then generated checkpoint prize payments for failing submissions. Root cause: The checkpoint winner query only compared review scores against the configured passing threshold, but did not explicitly exclude the scorecard minimum score floor, so floor-score submissions could still qualify. What was changed: Updated ReviewService.getTopCheckpointReviewScores SQL filter to require checkpoint scores to be greater than the scorecard minScore in addition to meeting the existing passing-threshold condition. Any added/updated tests: Updated the getTopCheckpointReviewScores unit test to assert the SQL now includes the strict greater-than minScore eligibility check.
…pproval phase What was broken - Changing the Approver after Approval had started could leave pending approval reviews bound to the previous resource, so the new approver had no actionable link in Review app. Root cause (if identifiable) - Reassignment only targeted phases where was true and only matched exact-case role/phase names; in affected updates, Approval could be current via while was false, so reassignment was skipped. What was changed - Normalized review-role matching to be case-insensitive. - Updated approver reassignment logic to treat Approval as active when either the phase is open or Approval appears in . - Kept reassignment scoped to Approval phases and retained existing pending-review status filters. Any added/updated tests - Added in to cover the regression.
What was broken - Challenges that reached COMPLETED before autopilot finalization processing could skip finance payment generation, resulting in missing winnings/payments. Root cause - finalizeChallenge returned immediately for non-ACTIVE challenges and never invoked FinanceApiService for already-payable statuses. What was changed - Updated ChallengeCompletionService.finalizeChallenge to trigger finance payment generation when status is already COMPLETED or CANCELLED_FAILED_REVIEW before returning. - Preserved existing finalization skip behavior for other non-ACTIVE statuses. Any added/updated tests - Added a unit test asserting finance payment generation is triggered when finalizeChallenge is called for an already COMPLETED challenge.
What was broken: Challenges that reached payable terminal statuses via challenge update events (for example from new WM flows) could skip winnings generation, leaving Review payments empty. Root cause: PhaseScheduleManager.handleChallengeUpdate returned early for all non-ACTIVE statuses and never triggered finance generation in those paths. What was changed: - Injected FinanceApiService into PhaseScheduleManager. - Added payable-status detection for COMPLETED and CANCELLED_FAILED_REVIEW. - Trigger finance generation when a challenge update transitions into a payable status. - Updated challenge-status cache before non-ACTIVE early returns to avoid duplicate finance triggers on repeated updates. - Added the same finance trigger when immediate overdue-phase processing refreshes a challenge into a payable non-ACTIVE status. Any added/updated tests: - Added phase-schedule-manager.service.spec.ts with coverage for: - one-time trigger on transition to COMPLETED, - no trigger for non-payable non-ACTIVE statuses, - trigger when ACTIVE becomes COMPLETED after immediate phase closure refresh.
| jest.mock('../../kafka/kafka.service', () => ({ | ||
| KafkaService: jest.fn().mockImplementation(() => ({})), | ||
| })); | ||
| /* eslint-disable @typescript-eslint/unbound-method */ |
There was a problem hiding this comment.
[maintainability]
Disabling @typescript-eslint/unbound-method can lead to issues with this context in methods. Consider refactoring the code to avoid the need for this rule, ensuring methods are bound correctly.
| @@ -428,7 +576,7 @@ describe('AutopilotService - handleSubmissionNotificationAggregate', () => { | |||
| }); | |||
|
|
|||
| describe('handleReviewCompleted (approval phase)', () => { | |||
There was a problem hiding this comment.
[💡 design]
The buildApprovalPhase and buildFinalFixPhase functions now accept an overrides parameter, which is a good practice for flexibility. Ensure that all necessary properties are correctly overridden and that defaults are sensible to prevent unexpected behavior.
| }); | ||
|
|
||
| it('closes the approval phase without creating a follow-up when the score meets the minimum', async () => { | ||
| it('closes the approval phase and closes any open Final Fix phases when the score meets the minimum', async () => { |
There was a problem hiding this comment.
[correctness]
The test case description was updated to include closing any open Final Fix phases. Ensure that the logic in the implementation correctly handles all edge cases where multiple phases might be open simultaneously.
| approvalTemplate: IPhase, | ||
| submissionId: string, | ||
| ): Promise<void> { | ||
| if (this.approvalResubmissionLocks.has(challengeId)) { |
There was a problem hiding this comment.
[maintainability]
The use of approvalResubmissionLocks to prevent duplicate approval submissions is a good approach, but it could lead to potential memory leaks if challenges are not properly removed from the set. Consider implementing a mechanism to ensure challenges are removed from approvalResubmissionLocks in all exit paths, including error scenarios.
| nextPhase.phaseId ?? undefined, | ||
| ) ?? approvalTemplate; | ||
|
|
||
| if (!latestApprovalTemplate.phaseId) { |
There was a problem hiding this comment.
[💡 maintainability]
The check for latestApprovalTemplate.phaseId is necessary, but the logic could be simplified by ensuring that approvalTemplate is always valid before this point. Consider refactoring to validate approvalTemplate earlier to avoid redundant checks.
| ), | ||
| ); | ||
|
|
||
| if (!uniqueResources.length) { |
There was a problem hiding this comment.
[💡 readability]
The warning log for no approver resources found is useful, but it might be beneficial to include more context, such as the expected roles or phase name, to aid in debugging.
| this.logger.log( | ||
| `Challenge ${challengeId} is already ${challenge.status}; ensuring finance payments are generated.`, | ||
| ); | ||
| void this.financeApiService.generateChallengePayments(challengeId); |
There was a problem hiding this comment.
[correctness]
The use of void here suppresses any potential errors from generateChallengePayments. Consider handling errors explicitly to ensure that any issues with payment generation are logged or managed appropriately.
| }; | ||
|
|
||
| financeApiService = { | ||
| generateChallengePayments: jest.fn().mockResolvedValue(true), |
There was a problem hiding this comment.
[correctness]
Using mockResolvedValue(true) for generateChallengePayments could lead to false positives in tests if the actual implementation returns different values. Consider using a more realistic mock return value that matches the expected behavior of the real function.
| service = new PhaseScheduleManager( | ||
| { | ||
| setPhaseChainCallback: jest.fn(), | ||
| } as any, |
There was a problem hiding this comment.
[maintainability]
The use of as any for type casting can hide potential type errors and reduce type safety. Consider defining proper interfaces or types for the dependencies being injected into PhaseScheduleManager to leverage TypeScript's type checking.
| {} as any, | ||
| {} as any, | ||
| { | ||
| get: jest.fn().mockReturnValue(undefined), |
There was a problem hiding this comment.
[correctness]
The mock for get returns undefined, which might not reflect the actual behavior of the service being mocked. Ensure that this mock accurately represents the expected behavior to avoid misleading test results.
| get: jest.fn().mockReturnValue(undefined), | ||
| } as any, | ||
| { | ||
| logAction: jest.fn(), |
There was a problem hiding this comment.
[💡 correctness]
The mock for logAction does not specify any behavior. If this method is expected to perform specific actions, consider defining its behavior in the mock to ensure comprehensive test coverage.
|
|
||
| if ( | ||
| previousStatus && | ||
| previousStatus.toUpperCase() === currentStatus.toUpperCase() |
There was a problem hiding this comment.
[correctness]
The comparison previousStatus.toUpperCase() === currentStatus.toUpperCase() is used to check if the status has changed. However, this could lead to unnecessary calls to generateChallengePayments if the status strings differ only in case. Consider storing statuses in a consistent case format (e.g., all uppercase) when caching or retrieving them to avoid this issue.
| this.logger.log( | ||
| `[FINANCE] Challenge ${challengeId} transitioned to ${currentStatus}; triggering payment generation.`, | ||
| ); | ||
| void this.financeApiService.generateChallengePayments(challengeId); |
There was a problem hiding this comment.
[❗❗ correctness]
The call to this.financeApiService.generateChallengePayments(challengeId) is prefixed with void, which suppresses any unhandled promise rejections. If this method can fail, consider handling the promise rejection to avoid potential silent failures.
| createdBy: 'system', | ||
| }; | ||
|
|
||
| resourcesService.getResourceById.mockResolvedValue({ |
There was a problem hiding this comment.
[maintainability]
The use of as unknown as any for type assertions can lead to potential runtime errors by bypassing TypeScript's type checking. Consider defining a proper interface or type for the mocked response to ensure type safety.
| roleId: 'role-approver', | ||
| } as unknown as any); | ||
|
|
||
| challengeApiService.getChallengeById.mockResolvedValue({ |
There was a problem hiding this comment.
[maintainability]
The use of as unknown as any for type assertions can lead to potential runtime errors by bypassing TypeScript's type checking. Consider defining a proper interface or type for the mocked response to ensure type safety.
| reviewers: [], | ||
| } as unknown as any); | ||
|
|
||
| reviewService.reassignPendingReviewsToResource.mockResolvedValue(1); |
There was a problem hiding this comment.
[correctness]
The use of mockResolvedValue(1) for reassignPendingReviewsToResource suggests that the function returns a number. Ensure that this mock accurately reflects the actual return type of the function to prevent potential type mismatches.
|
|
||
| private isApproverRole(roleName?: string | null): boolean { | ||
| const normalizedRoleName = this.normalizeName(roleName); | ||
| return normalizedRoleName === 'approver'; |
There was a problem hiding this comment.
[maintainability]
The isApproverRole method currently checks if the normalized role name is exactly 'approver'. Consider using a set of approver role names similar to normalizedReviewRoleNames to allow for more flexibility and future-proofing in case additional approver roles are introduced.
| let appealsOpenedImmediately = false; | ||
|
|
||
| if (data.skipPhaseChain) { | ||
| skipPhaseChain = true; |
There was a problem hiding this comment.
[💡 readability]
The use of optional chaining this.logger.debug?.() is unnecessary here since this.logger is instantiated as a Logger object in the constructor and should always be defined. Consider removing the optional chaining to simplify the code.
| 'Topgear Task Post Mortem', | ||
| ); | ||
| } catch (err) { | ||
| } catch { |
There was a problem hiding this comment.
[maintainability]
The catch block is empty, which might lead to silent failures. If the intention is to ignore the error, consider logging it at a lower level (e.g., debug) to aid in troubleshooting if needed.
| durationSeconds: number, | ||
| ): Promise<IPhase> { | ||
| const finalFixPhaseType = await this.prisma.phase.findFirst({ | ||
| where: { name: FINAL_FIX_PHASE_NAME }, |
There was a problem hiding this comment.
[performance]
Consider adding an index on the name column of the phase table if it is not already indexed. This could improve the performance of the findFirst query, especially if the table contains a large number of rows.
| AND GREATEST( | ||
| COALESCE(r."finalScore", 0), | ||
| COALESCE(r."initialScore", 0) | ||
| ) > COALESCE(sc."minScore", 0) |
There was a problem hiding this comment.
[maintainability]
The condition GREATEST(COALESCE(r."finalScore", 0), COALESCE(r."initialScore", 0)) > COALESCE(sc."minScore", 0) seems redundant given the previous condition GREATEST(COALESCE(r."finalScore", 0), COALESCE(r."initialScore", 0)) >= COALESCE(sc."minimumPassingScore", sc."minScore", 50). If minScore is always less than or equal to minimumPassingScore, this condition may not be necessary. Consider reviewing the logic to ensure both conditions are needed.
| }; | ||
|
|
||
| service = new SyncService( | ||
| autopilotService as any, |
There was a problem hiding this comment.
[maintainability]
Using as any for type casting can lead to runtime errors and makes the code less type-safe. Consider defining proper interfaces for autopilotService, challengeApiService, and schedulerService to ensure type safety.
| } | ||
|
|
||
| if (status === 'COMPLETED') { | ||
| throw new Error('temporary db issue'); |
There was a problem hiding this comment.
[💡 maintainability]
Throwing a generic error message like 'temporary db issue' can make debugging difficult. Consider using more descriptive error messages or logging additional context to aid in troubleshooting.
| */ | ||
| private isWithinPayableLookback(challenge: IChallenge, now: Date): boolean { | ||
| const updatedAtMs = new Date(challenge.updated).getTime(); | ||
| if (!Number.isFinite(updatedAtMs)) { |
There was a problem hiding this comment.
[💡 readability]
The check Number.isFinite(updatedAtMs) is used to ensure updatedAtMs is a valid number. However, since new Date(challenge.updated).getTime() will return NaN if the date is invalid, it might be more explicit to check for NaN directly using isNaN(updatedAtMs) for clarity.
| challenges = await this.challengeApiService.getAllActiveChallenges({ | ||
| status, | ||
| page: 1, | ||
| perPage: SyncService.PAYABLE_CHALLENGE_PAGE_SIZE, |
There was a problem hiding this comment.
[correctness]
Consider implementing pagination handling for getAllActiveChallenges. If the number of challenges exceeds PAYABLE_CHALLENGE_PAGE_SIZE, only the first page will be processed, potentially missing other challenges.
No description provided.