Migration deploy of final code #18
Conversation
…point reviewer without an initial resource assigned to either.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| jobs: | ||
| trivy-scan: | ||
| name: Use Trivy | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
[maintainability]
Consider using a more stable version of Ubuntu, such as ubuntu-latest, to ensure compatibility and reduce maintenance overhead when newer versions are released.
| ignore-unfixed: true | ||
| format: "sarif" | ||
| output: "trivy-results.sarif" | ||
| severity: "CRITICAL,HIGH,UNKNOWN" |
There was a problem hiding this comment.
[💡 readability]
Including UNKNOWN in the severity list might result in false positives or less actionable results. Consider reviewing if this level is necessary for your use case.
| export const ITERATIVE_REVIEW_PHASE_NAME = 'Iterative Review'; | ||
| export const POST_MORTEM_PHASE_NAME = 'Post-Mortem'; | ||
| export const POST_MORTEM_PHASE_ALTERNATE_NAME = 'Post-mortem'; | ||
| export const POST_MORTEM_PHASE_NAMES = new Set<string>([ |
There was a problem hiding this comment.
[correctness]
The introduction of POST_MORTEM_PHASE_ALTERNATE_NAME and its inclusion in POST_MORTEM_PHASE_NAMES is a good approach for handling case sensitivity. However, ensure that all parts of the application that use phase names are aware of this change to prevent any unexpected behavior due to case differences.
| return PHASE_ROLE_MAP[phaseName] ?? DEFAULT_PHASE_ROLES; | ||
| } | ||
|
|
||
| export function isPostMortemPhaseName(phaseName: string): boolean { |
There was a problem hiding this comment.
[correctness]
The function isPostMortemPhaseName correctly checks for both 'Post-Mortem' and 'Post-mortem' due to the use of POST_MORTEM_PHASE_NAMES. Ensure that similar functions for other phase names are consistent in handling case sensitivity if applicable.
| createPendingReview: jest.fn(), | ||
| } as unknown as jest.Mocked<ReviewService>; | ||
|
|
||
| reviewService.createPendingReview.mockResolvedValue(false); |
There was a problem hiding this comment.
[correctness]
The createPendingReview method is mocked to resolve false by default, but in the test case for creating a follow-up approval phase, it is explicitly mocked to resolve true. Ensure that the default mock behavior aligns with the expected behavior in all test cases to avoid unexpected results.
| expect(phaseReviewService.handlePhaseOpened).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('creates a follow-up approval phase when the score is below the minimum', async () => { |
There was a problem hiding this comment.
[correctness]
In the test case for creating a follow-up approval phase, the getReviewerResources method is mocked to return a specific reviewer resource. Ensure that this mock data accurately reflects the expected state of the system to maintain test reliability.
| }; | ||
|
|
||
| reviewSummationApiService = { | ||
| finalizeSummations: jest.fn().mockResolvedValue(true), |
There was a problem hiding this comment.
[correctness]
The finalizeSummations method is mocked to always resolve to true. Consider verifying that this behavior is appropriate for the test scenarios, as it might mask potential issues with the actual implementation of finalizeSummations.
| return true; | ||
| } | ||
|
|
||
| await this.reviewSummationApiService.finalizeSummations(challengeId); |
There was a problem hiding this comment.
[correctness]
Consider handling potential errors from finalizeSummations(challengeId) to ensure robustness. If this operation fails, it might impact subsequent logic.
|
|
||
| if (!assigned) { | ||
| const submissionSnapshot = | ||
| await this.reviewService.getAllSubmissionIdsOrdered(challenge.id); |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the getAllSubmissionIdsOrdered call. If this method throws an error, it could prevent the proper logging and phase management that follows.
| const submissionSnapshot = | ||
| await this.reviewService.getAllSubmissionIdsOrdered(challenge.id); | ||
|
|
||
| if (!submissionSnapshot.length) { |
There was a problem hiding this comment.
[💡 maintainability]
The check if (!submissionSnapshot.length) is used twice consecutively with similar logic. Consider consolidating these checks to improve readability and maintainability.
| finalizeChallenge: jest.fn(), | ||
| } as unknown as jest.Mocked<ChallengeCompletionService>; | ||
| reviewSummationApiService = { | ||
| finalizeSummations: jest.fn().mockResolvedValue(true), |
There was a problem hiding this comment.
[correctness]
The mock implementation of finalizeSummations resolves to true, which might not accurately reflect the real behavior of the method. Consider ensuring the mock behavior aligns with the actual method's expected outcomes, especially if it can resolve to different values or throw errors.
| 1, | ||
| ); | ||
| expect(reviewSummationApiService.finalizeSummations).toHaveBeenCalledWith( | ||
| challenge.id, |
There was a problem hiding this comment.
[correctness]
The test checks if finalizeSummations is called with challenge.id, but it does not verify the outcome or side effects of this call. Consider adding assertions to ensure the method's effects are as expected, especially if it modifies state or interacts with other services.
| 1, | ||
| ); | ||
| expect(reviewSummationApiService.finalizeSummations).toHaveBeenCalledWith( | ||
| challenge.id, |
There was a problem hiding this comment.
[correctness]
Similar to the previous test, ensure that the effects of calling finalizeSummations are verified. This will help confirm that the method behaves correctly in different scenarios.
| SCREENING_PHASE_NAMES, | ||
| APPROVAL_PHASE_NAMES, | ||
| POST_MORTEM_PHASE_NAME, | ||
| isPostMortemPhaseName, |
There was a problem hiding this comment.
[❗❗ correctness]
The change from POST_MORTEM_PHASE_NAME to isPostMortemPhaseName alters the logic from a direct comparison to a function call. Ensure that isPostMortemPhaseName correctly handles all expected phase names and edge cases.
| let submissionIds: string[] = []; | ||
| if (isApprovalPhase) { | ||
| try { | ||
| await this.reviewSummationApiService.finalizeSummations(challengeId); |
There was a problem hiding this comment.
[performance]
The addition of await this.reviewSummationApiService.finalizeSummations(challengeId); introduces a new asynchronous operation. Ensure that this call is necessary and that its potential impact on performance and error handling is considered, especially if it affects the timing of subsequent operations.
| this.logger.log( | ||
| `[REVIEW OPPORTUNITIES] Detected transition to ACTIVE for new challenge ${challenge.id}; review opportunity creation pending.`, | ||
| ); | ||
| // TODO: createReviewOpportunitiesForChallenge(challengeDetails); |
There was a problem hiding this comment.
[❗❗ correctness]
The createReviewOpportunitiesForChallenge function is marked as TODO. Ensure this function is implemented before deploying to avoid missing functionality related to review opportunities.
| this.logger.log( | ||
| `[REVIEW OPPORTUNITIES] Detected transition to ACTIVE for updated challenge ${message.id}; review opportunity creation pending.`, | ||
| ); | ||
| // TODO: createReviewOpportunitiesForChallenge(challengeDetails); |
There was a problem hiding this comment.
[❗❗ correctness]
The createReviewOpportunitiesForChallenge function is marked as TODO. Ensure this function is implemented before deploying to avoid missing functionality related to review opportunities.
| challengeDetails = await this.challengeApiService.getChallengeById( | ||
| message.id, | ||
| ); | ||
| if (challengeDetails.status !== previousStatus) { |
There was a problem hiding this comment.
[maintainability]
Consider updating the cached status regardless of whether the immediate closures were processed. This ensures the cache is always in sync with the latest challenge details.
| ); | ||
| } | ||
|
|
||
| this.updateCachedStatus(message.id, challengeDetails.status); |
There was a problem hiding this comment.
[maintainability]
Ensure that updateCachedStatus is called consistently after challenge updates to maintain cache integrity.
| ], | ||
| } as unknown as any); | ||
|
|
||
| resourcesService.getReviewerResources.mockResolvedValue([ |
There was a problem hiding this comment.
[maintainability]
The use of as unknown as any for type casting is a potential source of errors and reduces type safety. Consider defining a proper interface or type for the expected return value of getReviewerResources to improve maintainability and type safety.
| ], | ||
| } as unknown as any); | ||
|
|
||
| resourcesService.getReviewerResources.mockResolvedValue([]); |
There was a problem hiding this comment.
[maintainability]
The use of as unknown as any for type casting is a potential source of errors and reduces type safety. Consider defining a proper interface or type for the expected return value of getReviewerResources to improve maintainability and type safety.
|
|
||
| for (const phase of reviewPhases) { | ||
| for (const phase of phases) { | ||
| const isReviewPhase = REVIEW_PHASE_NAMES.has(phase.name); |
There was a problem hiding this comment.
[💡 performance]
The variable isReviewPhase is used multiple times in the loop. Consider moving its declaration outside the loop to avoid repeated computation, which can slightly improve performance.
| } | ||
| } else { | ||
| try { | ||
| ready = await this.isScreeningPhaseReady(challenge, phase); |
There was a problem hiding this comment.
[correctness]
The method isScreeningPhaseReady is called without checking if phase.phaseId is defined. Although the method handles this case internally, it might be clearer and safer to check this condition before calling the method to avoid unnecessary processing.
| challenge: Awaited<ReturnType<ChallengeApiService['getChallengeById']>>, | ||
| phase: IPhase, | ||
| ): Promise<boolean> { | ||
| if (!phase.phaseId) { |
There was a problem hiding this comment.
[❗❗ correctness]
The warning message indicates that the screening phase is opened without reviewer validation if phase.phaseId is missing. This could lead to incorrect behavior if the phase should require validation. Consider revisiting this logic to ensure phases are correctly validated before opening.
| ); | ||
| return true; | ||
| } catch (error) { | ||
| const err = error as any; |
There was a problem hiding this comment.
[maintainability]
Catching errors as any can obscure the actual type of the error and make it harder to handle specific cases. Consider using a more specific error type or refining the error handling logic.
| }; | ||
|
|
||
| const response = await firstValueFrom( | ||
| this.httpService.post(url, undefined, axiosConfig), |
There was a problem hiding this comment.
[❗❗ correctness]
The firstValueFrom function is used to convert an Observable to a Promise. Ensure that the httpService.post method returns an Observable that completes, as firstValueFrom will hang indefinitely if the Observable does not complete.
| ); | ||
|
|
||
| const pendingReviewSpy = jest | ||
| .spyOn(scheduler as any, 'createPostMortemPendingReviews') |
There was a problem hiding this comment.
[maintainability]
Using as any to spy on private methods can lead to brittle tests and maintenance challenges. Consider refactoring the code to expose these methods in a testable way or using dependency injection to avoid the need for as any.
| ); | ||
|
|
||
| const pendingReviewSpy = jest | ||
| .spyOn(scheduler as any, 'createPostMortemPendingReviews') |
There was a problem hiding this comment.
[maintainability]
Using as any to spy on private methods can lead to brittle tests and maintenance challenges. Consider refactoring the code to expose these methods in a testable way or using dependency injection to avoid the need for as any.
| status: ChallengeStatusEnum.CANCELLED_ZERO_SUBMISSIONS, | ||
| } as unknown as IChallenge); | ||
|
|
||
| await (scheduler as any).handlePostMortemPhaseClosed(payload); |
There was a problem hiding this comment.
[maintainability]
Using as any to call private methods can lead to brittle tests and maintenance challenges. Consider refactoring the code to expose these methods in a testable way or using dependency injection to avoid the need for as any.
| ); | ||
| } | ||
| } else if (phaseName === POST_MORTEM_PHASE_NAME) { | ||
| } else if (isPostMortemPhaseName(phaseName)) { |
There was a problem hiding this comment.
[maintainability]
The change from phaseName === POST_MORTEM_PHASE_NAME to isPostMortemPhaseName(phaseName) improves maintainability by abstracting the phase name check into a function. Ensure that isPostMortemPhaseName is correctly implemented to handle all expected phase name variations.
|
|
||
| if ( | ||
| !skipFinalization && | ||
| !data.preventFinalization && |
There was a problem hiding this comment.
[❗❗ correctness]
The addition of !data.preventFinalization to the condition for attemptChallengeFinalization introduces a new flag that could affect the finalization logic. Ensure that this flag is correctly set and used throughout the application to avoid unintended behavior.
| ) { | ||
| await this.attemptChallengeFinalization(data.challengeId); | ||
| } else { | ||
| if (!skipFinalization && data.preventFinalization) { |
There was a problem hiding this comment.
[💡 maintainability]
The new debug log for preventFinalization provides useful information for tracing deferred finalization scenarios. Ensure that logging is appropriately configured to capture these debug messages in production environments.
|
|
||
| let postMortemPhase = | ||
| challenge.phases?.find((p) => p.name === POST_MORTEM_PHASE_NAME) ?? null; | ||
| challenge.phases?.find((p) => isPostMortemPhaseName(p.name)) ?? null; |
There was a problem hiding this comment.
[maintainability]
The change from challenge.phases?.find((p) => p.name === POST_MORTEM_PHASE_NAME) to challenge.phases?.find((p) => isPostMortemPhaseName(p.name)) improves maintainability by using a function to check phase names. Verify that isPostMortemPhaseName correctly identifies all post-mortem phase names.
| ? ChallengeStatusEnum.CANCELLED_ZERO_SUBMISSIONS | ||
| : ChallengeStatusEnum.CANCELLED_ZERO_REGISTRATIONS; | ||
|
|
||
| await this.challengeApiService.cancelChallenge( |
There was a problem hiding this comment.
[❗❗ correctness]
The introduction of cancelStatus based on hasSubmitter adds complexity to the cancellation logic. Ensure that the logic for determining hasSubmitter is reliable and consistent across different scenarios.
| : '[ZERO REGISTRATIONS]'; | ||
|
|
||
| const status = hasSubmitter | ||
| ? ChallengeStatusEnum.CANCELLED_ZERO_SUBMISSIONS |
There was a problem hiding this comment.
[❗❗ correctness]
The conditional logic for setting status based on hasSubmitter could lead to inconsistent challenge statuses if hasSubmitter is not accurately determined. Double-check the implementation of hasSubmitterResource to ensure it provides correct results.
| previousStatus?: string | null, | ||
| currentStatus?: string | null, | ||
| ): boolean { | ||
| const wasActive = isActiveStatus(previousStatus ?? undefined); |
There was a problem hiding this comment.
[💡 readability]
Using previousStatus ?? undefined is redundant because isActiveStatus already handles undefined values. Consider simplifying to isActiveStatus(previousStatus).
| currentStatus?: string | null, | ||
| ): boolean { | ||
| const wasActive = isActiveStatus(previousStatus ?? undefined); | ||
| const isActive = isActiveStatus(currentStatus ?? undefined); |
There was a problem hiding this comment.
[💡 readability]
Using currentStatus ?? undefined is redundant because isActiveStatus already handles undefined values. Consider simplifying to isActiveStatus(currentStatus).
|
|
||
| const postMortemPhaseType = await tx.phase.findUnique({ | ||
| where: { name: 'Post-Mortem' }, | ||
| const postMortemPhaseType = await tx.phase.findFirst({ |
There was a problem hiding this comment.
[correctness]
The change from findUnique to findFirst when querying for postMortemPhaseType could lead to unexpected results if there are multiple entries with names in POST_MORTEM_PHASE_NAMES. Ensure that the data model guarantees uniqueness or that this behavior is intended.
|
|
||
| const postMortemPhaseType = await tx.phase.findUnique({ | ||
| where: { name: 'Post-Mortem' }, | ||
| const postMortemPhaseType = await tx.phase.findFirst({ |
There was a problem hiding this comment.
[correctness]
Similar to the previous comment, changing from findUnique to findFirst could lead to unexpected results if there are multiple entries with names in POST_MORTEM_PHASE_NAMES. Verify that this change is intentional and that the data model supports this behavior.
| } | ||
|
|
||
| async createIterativeReviewPhase( | ||
| private async createContinuationPhase( |
There was a problem hiding this comment.
[💡 design]
The method createContinuationPhase is marked as private but is being used by other public methods like createIterativeReviewPhase and createApprovalPhase. Consider whether this method should be protected or public to reflect its usage.
| ? pollInterval | ||
| : DEFAULT_POLL_INTERVAL_MS, | ||
| summationApiUrl: (process.env.REVIEW_SUMMATION_API_URL || '').trim(), | ||
| summationApiTimeoutMs: 10000, |
There was a problem hiding this comment.
[maintainability]
The summationApiTimeoutMs is hardcoded to 10000. Consider using the parseNumber function to allow configuration via environment variables, similar to timeoutMs. This would improve maintainability and flexibility.
| .integer() | ||
| .positive() | ||
| .default(5 * 60 * 1000), | ||
| REVIEW_SUMMATION_API_URL: Joi.string(), |
There was a problem hiding this comment.
[correctness]
Consider adding a .uri() validation to REVIEW_SUMMATION_API_URL to ensure it is a valid URL. This will help catch configuration errors early.
| status?: string; | ||
| type?: string; | ||
| openPositions!: number; | ||
| startDate!: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more specific date type, such as Date, instead of string for startDate. This will help prevent errors related to date manipulation and ensure better type safety.
| type?: string; | ||
| openPositions!: number; | ||
| startDate!: string; | ||
| duration!: number; |
There was a problem hiding this comment.
[correctness]
Consider adding validation or constraints for duration to ensure it is a positive number. This will prevent potential logical errors when calculating time spans.
| openPositions!: number; | ||
| startDate!: string; | ||
| duration!: number; | ||
| basePayment!: number; |
There was a problem hiding this comment.
[correctness]
Consider adding validation or constraints for basePayment to ensure it is a non-negative number. This will prevent potential issues with payment calculations.
| startDate!: string; | ||
| duration!: number; | ||
| basePayment!: number; | ||
| incrementalPayment!: number; |
There was a problem hiding this comment.
[correctness]
Consider adding validation or constraints for incrementalPayment to ensure it is a non-negative number. This will prevent potential issues with payment calculations.
| ); | ||
| return response.data; | ||
| } catch (error) { | ||
| const err = error as any; |
There was a problem hiding this comment.
[maintainability]
Catching errors as any can obscure the actual error type and make it harder to handle specific cases. Consider using a more specific error type or refining the error handling logic.
| ); | ||
| return Array.isArray(response.data) ? response.data : []; | ||
| } catch (error) { | ||
| const err = error as any; |
There was a problem hiding this comment.
[maintainability]
Catching errors as any can obscure the actual error type and make it harder to handle specific cases. Consider using a more specific error type or refining the error handling logic.
| $transaction: jest | ||
| .fn() | ||
| .mockImplementation( | ||
| async (callback: (tx: { $executeRaw: typeof executeRawMock }) => Promise<void>) => { |
There was a problem hiding this comment.
[maintainability]
The $transaction mock implementation assumes that the callback will only use $executeRaw. If the actual implementation uses other methods, this test might not cover all cases. Consider expanding the mock to include other methods that might be used within a transaction.
| legacySubmissionId: 'legacy-1', | ||
| memberId: '123456', | ||
| submittedDate: '2024-10-21T10:00:00.000Z', | ||
| aggregateScore: '70', |
There was a problem hiding this comment.
[correctness]
The aggregateScore and minimumPassingScore are defined as strings in buildReviewSummationRow. Consider using numbers to avoid potential type-related issues when performing arithmetic operations or comparisons.
| legacySubmissionId: 'legacy-1', | ||
| memberId: '123456', | ||
| submittedDate: '2024-10-21T10:00:00.000Z', | ||
| aggregateScore: '95', |
There was a problem hiding this comment.
[correctness]
The aggregateScore is defined as a string in buildAggregationRow. Consider using a number to ensure consistency and avoid potential type-related issues.
| if (!summaries.length) { | ||
| summaries = await this.rebuildSummariesFromReviews(challengeId); | ||
| usedRebuild = summaries.length > 0; | ||
| } else if (summaries.every((summary) => !summary.isPassing)) { |
There was a problem hiding this comment.
[correctness]
The variable usedRebuild is set to true when summaries are rebuilt, but it is not reset to false if the recalculated summaries do not differ or have passing entries. Consider resetting usedRebuild to false before the else if block to ensure it accurately reflects whether a rebuild was used.
| AND rs."isFinal" = true | ||
| AND ( | ||
| s."type" IS NULL | ||
| OR UPPER((s."type")::text) = 'CONTEST_SUBMISSION' |
There was a problem hiding this comment.
[💡 maintainability]
The condition UPPER((s."type")::text) = 'CONTEST_SUBMISSION' is repeated multiple times in the SQL queries. Consider extracting this condition into a reusable constant or method to improve maintainability and reduce duplication.
|
|
||
| if ( | ||
| hasScore && | ||
| ReviewService.isFinalReviewType(reviewTypeName) |
There was a problem hiding this comment.
[💡 performance]
The method isFinalReviewType is called multiple times within the loop. Consider caching its result in a variable to avoid redundant calls and improve performance.
| let scorecardLegacyId = accumulator.primaryScorecardLegacyId; | ||
| let passingScore = accumulator.primaryPassingScore; | ||
|
|
||
| if (count === 0 && accumulator.fallbackCount > 0) { |
There was a problem hiding this comment.
[💡 maintainability]
The logic for determining total, count, scorecardId, scorecardLegacyId, and passingScore when primaryCount is zero and fallbackCount is greater than zero is repeated. Consider refactoring this logic into a helper function to improve readability and maintainability.
No description provided.