Reviews #17
Reviews #17
Conversation
…s have been responded to
…ike registration and submission
…dle a no-registration challenge by moving to state CANCELLED_ZERO_REGISTRANTS
…and add some logging around payment creation to figure out why it's failing with a 403
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| source: ChallengeApiService.name, | ||
| details: { | ||
| submissionPhaseId, | ||
| predecessorPhaseId, |
There was a problem hiding this comment.
The variable predecessorPhaseId is introduced here, replacing submissionPhaseId. Ensure that predecessorPhaseId is correctly defined and initialized elsewhere in the code, as this change may affect the logic depending on how these identifiers are used.
| submissionPhaseId, | ||
| predecessorPhaseId, | ||
| durationHours, | ||
| preserveFuturePhases: true, |
There was a problem hiding this comment.
The addition of preserveFuturePhases: true should be verified against the business logic requirements. Ensure that this change aligns with the intended functionality and does not inadvertently alter the expected behavior of the phase handling.
| predecessorPhaseId, | ||
| durationHours, | ||
| preserveFuturePhases: true, | ||
| openImmediately, |
There was a problem hiding this comment.
The variable openImmediately is added here. Verify that this variable is properly defined and initialized elsewhere in the code, and ensure that its inclusion does not conflict with existing logic or introduce unintended side effects.
| export interface IChallengePrize { | ||
| type: string; | ||
| value: number; | ||
| description: string | null; |
There was a problem hiding this comment.
Consider using undefined instead of null for the description property to maintain consistency with TypeScript's handling of optional properties.
|
|
||
| export interface IChallengePrizeSet { | ||
| type: PrizeSetTypeEnum; | ||
| description: string | null; |
There was a problem hiding this comment.
Consider using undefined instead of null for the description property to maintain consistency with TypeScript's handling of optional properties.
| createdBy: string; | ||
| updatedBy: string; | ||
| metadata: any[]; | ||
| metadata: Record<string, string>; |
There was a problem hiding this comment.
Consider specifying a more precise type for the discussions array instead of using any[]. This will improve type safety and make the code easier to understand.
| createdBy: string; | ||
| updatedBy: string; | ||
| metadata: any[]; | ||
| metadata: Record<string, string>; |
There was a problem hiding this comment.
Consider specifying a more precise type for the events array instead of using any[]. This will improve type safety and make the code easier to understand.
| events: any[]; | ||
| prizeSets: any[]; | ||
| prizeSets: IChallengePrizeSet[]; | ||
| terms: any[]; |
There was a problem hiding this comment.
Consider specifying a more precise type for the terms array instead of using any[]. This will improve type safety and make the code easier to understand.
| prizeSets: any[]; | ||
| prizeSets: IChallengePrizeSet[]; | ||
| terms: any[]; | ||
| skills: any[]; |
There was a problem hiding this comment.
Consider specifying a more precise type for the skills array instead of using any[]. This will improve type safety and make the code easier to understand.
| prizeSets: IChallengePrizeSet[]; | ||
| terms: any[]; | ||
| skills: any[]; | ||
| attachments: any[]; |
There was a problem hiding this comment.
Consider specifying a more precise type for the attachments array instead of using any[]. This will improve type safety and make the code easier to understand.
| postMortemScorecardId: process.env.POST_MORTEM_SCORECARD_ID || null, | ||
| topgearPostMortemScorecardId: | ||
| process.env.TOPGEAR_POST_MORTEM_SCORECARD_ID || null, | ||
| // Optional default scorecard to use for First2Finish iterative reviews |
There was a problem hiding this comment.
Consider removing the comment as it is not necessary to explain the code. The variable name iterativeReviewScorecardId is self-explanatory.
| ['Appeals Response'], | ||
| ), | ||
| phaseNotificationSendgridTemplateId: | ||
| process.env.PHASE_NOTIFICATION_SENDGRID_TEMPLATE || null, |
There was a problem hiding this comment.
Consider renaming the environment variable PHASE_NOTIFICATION_SENDGRID_TEMPLATE to PHASE_NOTIFICATION_SENDGRID_TEMPLATE_ID for consistency with the property name phaseNotificationSendgridTemplateId.
| } | ||
|
|
||
| export default registerAs('bus', () => ({ | ||
| url: process.env.BUS_API_URL, |
There was a problem hiding this comment.
Consider adding validation to ensure process.env.BUS_API_URL is a valid URL format. This will help prevent potential runtime errors if the environment variable is incorrectly set.
| export default registerAs('bus', () => ({ | ||
| url: process.env.BUS_API_URL, | ||
| timeoutMs: parseNumber(process.env.BUS_API_TIMEOUT_MS, 10000), | ||
| originator: process.env.BUS_API_ORIGINATOR || 'autopilot-service', |
There was a problem hiding this comment.
Consider making the default value for originator configurable through a constant or configuration file, rather than hardcoding it as 'autopilot-service'. This will make it easier to change in the future if needed.
| } | ||
|
|
||
| export default registerAs('finance', () => ({ | ||
| baseUrl: process.env.FINANCE_API_URL || '', |
There was a problem hiding this comment.
Consider validating the process.env.FINANCE_API_URL to ensure it is a valid URL format before using it as baseUrl. This can prevent potential runtime errors if the environment variable is incorrectly set.
|
|
||
| export default registerAs('finance', () => ({ | ||
| baseUrl: process.env.FINANCE_API_URL || '', | ||
| timeoutMs: parseNumber(process.env.FINANCE_API_TIMEOUT_MS, 15000), |
There was a problem hiding this comment.
The parseNumber function currently only checks if the parsed number is finite and greater than zero. Consider adding additional validation to ensure the number is within a reasonable range for a timeout value, as extremely high values might not be practical.
| .positive() | ||
| .default(24), | ||
| // Optional default scorecard to use for First2Finish iterative reviews | ||
| ITERATIVE_REVIEW_SCORECARD_ID: Joi.string().optional().allow(null, ''), |
There was a problem hiding this comment.
Consider specifying a regex pattern for ITERATIVE_REVIEW_SCORECARD_ID to ensure it matches expected formats, if applicable.
| ITERATIVE_REVIEW_SCORECARD_ID: Joi.string().optional().allow(null, ''), | ||
| APPEALS_PHASE_NAMES: Joi.string().default('Appeals'), | ||
| APPEALS_RESPONSE_PHASE_NAMES: Joi.string().default('Appeals Response'), | ||
| PHASE_NOTIFICATION_SENDGRID_TEMPLATE: Joi.string().optional(), |
There was a problem hiding this comment.
It might be beneficial to set a default value for PHASE_NOTIFICATION_SENDGRID_TEMPLATE to ensure consistent behavior if the value is not provided.
| is: 'test', | ||
| then: Joi.optional().default('https://test-api.topcoder.com'), | ||
| otherwise: Joi.required(), | ||
| }), |
There was a problem hiding this comment.
The AUTH0_PROXY_SEREVR_URL key seems to have a typo. It should likely be AUTH0_PROXY_SERVER_URL.
| FINANCE_API_URL: Joi.string().uri().optional(), | ||
| FINANCE_API_TIMEOUT_MS: Joi.number().integer().positive().default(15000), | ||
|
|
||
| // Sync Service Configuration |
There was a problem hiding this comment.
The comment // Default sync cadence set to every 3 minutes is not aligned with the previous comment style. Consider maintaining consistency in comment style.
| private readonly auth0Service: Auth0Service, | ||
| private readonly dbLogger: AutopilotDbLoggerService, | ||
| ) { | ||
| this.baseUrl = (this.configService.get<string>('finance.baseUrl') || '').trim(); |
There was a problem hiding this comment.
Consider handling the case where finance.baseUrl is not a valid URL format. Currently, it only checks if it's an empty string after trimming.
| private readonly dbLogger: AutopilotDbLoggerService, | ||
| ) { | ||
| this.baseUrl = (this.configService.get<string>('finance.baseUrl') || '').trim(); | ||
| this.timeoutMs = this.configService.get<number>('finance.timeoutMs') ?? 15000; |
There was a problem hiding this comment.
The default timeout value of 15000 ms might be too high for some use cases. Consider making this configurable or reviewing if this is the appropriate default value.
| } | ||
|
|
||
| let token: string | undefined; | ||
| try { |
There was a problem hiding this comment.
Consider adding error handling for the getAccessToken method in case it fails to retrieve a token, which would prevent the subsequent HTTP request from being sent.
| } catch (error) { | ||
| const err = error as any; | ||
| const message = err?.message || 'Unknown error'; | ||
| const status = err?.response?.status; |
There was a problem hiding this comment.
The error handling could be improved by checking for specific error types or conditions, rather than relying on generic error properties like message. This would allow for more precise logging and handling.
| ); | ||
|
|
||
| if (!databaseUrl) { | ||
| Logger.warn( |
There was a problem hiding this comment.
Consider using this.logger.warn instead of Logger.warn to maintain consistency in logging and to ensure that the logger instance specific to this service is used.
| try { | ||
| if (memberIds.length) { | ||
| const idList = Prisma.join( | ||
| memberIds.map((id) => Prisma.sql`${BigInt(id)}`), |
There was a problem hiding this comment.
Using BigInt for converting id might cause issues if the id is not a valid number. Consider adding validation or error handling for non-numeric id values before this conversion.
|
|
||
| if (handles.length) { | ||
| const handleList = Prisma.join( | ||
| handles.map((handle) => Prisma.sql`${handle}`), |
There was a problem hiding this comment.
Ensure that handle values do not contain SQL injection vulnerabilities. Although Prisma.sql is used, it's important to validate or sanitize inputs where possible.
| ); | ||
|
|
||
| for (const row of rows) { | ||
| if (!row.userId || !row.email) { |
There was a problem hiding this comment.
The check if (!row.userId || !row.email) might skip valid userId entries if email is null. Consider whether email being null should indeed skip the entry or if it should be handled differently.
| ); | ||
|
|
||
| for (const row of rows) { | ||
| if (!row.handleLower || !row.email) { |
There was a problem hiding this comment.
Similar to line 79, consider whether handleLower entries with null email should be skipped or handled differently.
| return []; | ||
| } | ||
|
|
||
| const query = Prisma.sql` |
There was a problem hiding this comment.
Consider using parameterized queries or prepared statements to prevent SQL injection vulnerabilities. Directly embedding variables into SQL queries can be risky.
|
|
||
| return recipients; | ||
| } catch (error) { | ||
| const err = error as Error; |
There was a problem hiding this comment.
Instead of casting the error to Error, consider checking if the error is an instance of Error before accessing err.message. This ensures that the code handles unexpected error types gracefully.
| }; | ||
|
|
||
| service = new ReviewService( | ||
| prismaMock as unknown as any, |
There was a problem hiding this comment.
The type casting to any for prismaMock and dbLoggerMock can be avoided by defining more specific types for these mocks. Consider defining interfaces or types that match the expected structure of these objects.
| it('returns winners from review summations when available', async () => { | ||
| prismaMock.$queryRaw.mockResolvedValueOnce([ | ||
| { | ||
| memberId: ' 123 ', |
There was a problem hiding this comment.
The memberId value contains leading and trailing spaces. Consider trimming the memberId values before using them in comparisons or assertions to avoid unexpected behavior.
|
|
||
| const summariesSpy = jest | ||
| .spyOn(service, 'generateReviewSummaries') | ||
| .mockResolvedValue([ |
There was a problem hiding this comment.
The generateReviewSummaries method is being mocked here. Ensure that this method is tested separately to verify its behavior, as mocking it here means its internal logic is not being tested in this test suite.
| prismaMock.$queryRaw.mockResolvedValueOnce([ | ||
| { id: 'failed-1' }, | ||
| { id: 'failed-2' }, | ||
| { id: 'failed-1' }, |
There was a problem hiding this comment.
The mockResolvedValueOnce method is returning duplicate id values. Ensure that the test case accounts for duplicates if they are expected, or adjust the mock data to better reflect realistic scenarios.
| AND ( | ||
| s."type" IS NULL | ||
| OR upper(s."type") = 'CONTEST_SUBMISSION' | ||
| OR UPPER((s."type")::text) = 'CONTEST_SUBMISSION' |
There was a problem hiding this comment.
Casting s."type" to text using ::text is unnecessary if s."type" is already a text type. Ensure that this cast is required for the operation.
| }); | ||
| return []; | ||
| } | ||
| const winnersFromSummations = |
There was a problem hiding this comment.
The variable winnersFromSummations is assigned but not used directly. Consider using it directly or refactoring the assignment to winners for clarity.
| aggregateScore: number; | ||
| }> = []; | ||
| let winners = winnersFromSummations; | ||
| let dataSource: 'reviewSummation' | 'reviewSummaries' = 'reviewSummation'; |
There was a problem hiding this comment.
The variable dataSource is initialized but not used in the current context. Ensure it is necessary or remove it if not used.
| } | ||
|
|
||
| const aggregateScore = | ||
| typeof row.aggregateScore === 'string' |
There was a problem hiding this comment.
Consider adding a check to ensure that row.aggregateScore is a valid number before attempting to convert it with Number(row.aggregateScore). This will prevent potential issues if row.aggregateScore contains non-numeric strings.
| ): Promise< | ||
| Array<{ memberId: string; submissionId: string; aggregateScore: number }> | ||
| > { | ||
| const summaries = await this.generateReviewSummaries(challengeId); |
There was a problem hiding this comment.
The method generateReviewSummaries is called here, but it's not clear from the context if it handles errors or exceptions. Consider adding error handling to ensure that the function behaves correctly if generateReviewSummaries fails.
|
|
||
| const seenMembers = new Set<string>(); | ||
| const sortedSummaries = summaries | ||
| .filter((summary) => summary.isPassing) |
There was a problem hiding this comment.
The filter condition summary.isPassing assumes that isPassing is a boolean property. Ensure that all summaries have this property defined to avoid potential runtime errors.
| s."memberId", | ||
| CASE | ||
| WHEN ROW_NUMBER() OVER ( | ||
| PARTITION BY COALESCE(s."memberId", s."id") |
There was a problem hiding this comment.
The PARTITION BY clause uses COALESCE(s."memberId", s."id"), which might not be necessary if memberId is always present. Consider reviewing the logic to ensure it is needed.
| AND (s."status" = 'ACTIVE' OR s."status" IS NULL) | ||
| AND ( | ||
| s."type" IS NULL | ||
| OR UPPER((s."type")::text) = 'CONTEST_SUBMISSION' |
There was a problem hiding this comment.
The condition OR UPPER((s."type")::text) = 'CONTEST_SUBMISSION' assumes the type column is always in a consistent format. Ensure that the data is normalized or that this condition accounts for all possible variations.
| SELECT "id" | ||
| FROM ${ReviewService.SUBMISSION_TABLE} | ||
| WHERE "challengeId" = ${challengeId} | ||
| AND ("status" = 'ACTIVE' OR "status" IS NULL) |
There was a problem hiding this comment.
The condition AND ("status" = 'ACTIVE' OR "status" IS NULL) is repeated in multiple queries. Consider refactoring this logic into a reusable method or constant to improve maintainability.
| AND GREATEST( | ||
| COALESCE(r."finalScore", 0), | ||
| COALESCE(r."initialScore", 0) | ||
| ) >= COALESCE(sc."minimumPassingScore", sc."minScore", 50) |
There was a problem hiding this comment.
The use of GREATEST(COALESCE(r."finalScore", 0), COALESCE(r."initialScore", 0)) assumes that a score of 0 is appropriate when scores are missing. Ensure this logic aligns with business rules.
| new Set( | ||
| screeningScorecardIds | ||
| .map((id) => id?.trim()) | ||
| .filter((id): id is string => Boolean(id)), |
There was a problem hiding this comment.
The filter filter((id): id is string => Boolean(id)) is used to ensure non-empty strings. Consider using filter(Boolean) directly for simplicity.
| AND GREATEST( | ||
| COALESCE(r."finalScore", 0), | ||
| COALESCE(r."initialScore", 0) | ||
| ) < COALESCE(sc."minimumPassingScore", sc."minScore", 50) |
There was a problem hiding this comment.
The default value 50 in COALESCE(sc."minimumPassingScore", sc."minScore", 50) should be verified to ensure it aligns with the intended business logic.
|
|
||
| async getReviewerSubmissionPairs( | ||
| challengeId: string, | ||
| ): Promise<Set<string>> { |
There was a problem hiding this comment.
Consider specifying a more descriptive return type instead of Set<string>, such as Set<ReviewerSubmissionPair>, to improve code readability and maintainability.
| return new Set<string>(); | ||
| } | ||
|
|
||
| const query = Prisma.sql` |
There was a problem hiding this comment.
Ensure that the SQL query is safe from SQL injection attacks. Consider using parameterized queries or ORM methods to handle dynamic values.
| const result = new Set<string>(); | ||
|
|
||
| for (const record of records) { | ||
| if (!record.submissionId || !record.resourceId) { |
There was a problem hiding this comment.
The check for record.submissionId and record.resourceId being falsy might not be necessary if the database schema ensures these fields are always populated. Verify if this check can be removed to simplify the code.
| continue; | ||
| } | ||
|
|
||
| result.add(this.composeKey(record.resourceId, record.submissionId)); |
There was a problem hiding this comment.
The method composeKey is used here, but it's not clear what it does or if it handles edge cases correctly. Ensure that composeKey is well-tested and handles all potential input scenarios.
| OR UPPER((existing."status")::text) NOT IN ('COMPLETED', 'NO_REVIEW') | ||
| ) | ||
| ) | ||
| RETURNING "id" |
There was a problem hiding this comment.
The RETURNING "id" clause is added here. Ensure that the returned id is used appropriately in the subsequent code. If it's not needed, consider removing it to avoid unnecessary operations.
| SELECT pg_advisory_xact_lock(${lockId}) | ||
| `); | ||
|
|
||
| const insertedReviews = await tx.$queryRaw< |
There was a problem hiding this comment.
Consider specifying the type for insert to ensure type safety and clarity. This will help in understanding what kind of data is expected to be inserted.
|
|
||
| const existingPendingReviews = await tx.$queryRaw< | ||
| Array<{ id: string }> | ||
| >(Prisma.sql` |
There was a problem hiding this comment.
The SQL query in this block is quite complex. Ensure that all fields and conditions are necessary and that the query is optimized for performance, especially since it involves ordering and limiting results.
| AND existing."submissionId" IS NOT DISTINCT FROM ${submissionId} | ||
| AND existing."scorecardId" IS NOT DISTINCT FROM ${scorecardId} | ||
| AND ( | ||
| existing."status" IS NULL |
There was a problem hiding this comment.
The condition UPPER((existing."status")::text) NOT IN ('COMPLETED', 'NO_REVIEW') could potentially be simplified or optimized. Consider whether using a different approach, such as a status enum or a more efficient query structure, could improve performance.
| resourceId, | ||
| submissionId, | ||
| phaseId, | ||
| scorecardId, |
There was a problem hiding this comment.
Consider verifying if scorecardId is always available and correctly formatted before logging it to prevent potential logging of undefined or incorrect values.
| phaseId, | ||
| scorecardId, | ||
| error: err.message, | ||
| errorStack: err.stack ?? null, |
There was a problem hiding this comment.
Ensure that logging err.stack does not expose sensitive information that could be exploited. Consider sanitizing or limiting the stack trace details if necessary.
| } | ||
|
|
||
| async getScorecardIdByName(name: string): Promise<string | null> { | ||
| if (!name) { |
There was a problem hiding this comment.
Consider adding validation for the name parameter to ensure it meets expected criteria before proceeding with the query.
| return null; | ||
| } | ||
|
|
||
| const query = Prisma.sql` |
There was a problem hiding this comment.
Using raw SQL queries can expose the application to SQL injection risks. Ensure that the name parameter is properly sanitized or consider using parameterized queries.
|
|
||
| const id = record?.id ?? null; | ||
|
|
||
| void this.dbLogger.logAction('review.getScorecardIdByName', { |
There was a problem hiding this comment.
The logAction method is called with void, which suppresses any returned promise errors. Consider handling potential errors from logAction to ensure logging failures are captured.
| return id; | ||
| } catch (error) { | ||
| const err = error as Error; | ||
| void this.dbLogger.logAction('review.getScorecardIdByName', { |
There was a problem hiding this comment.
Similar to the success logging, the error logging uses void. Consider handling potential errors from logAction to ensure logging failures are captured.
| } | ||
|
|
||
| async getTotalAppealCount(challengeId: string): Promise<number> { | ||
| const query = Prisma.sql` |
There was a problem hiding this comment.
Consider using parameterized queries to prevent SQL injection vulnerabilities. The current implementation interpolates variables directly into the SQL string, which can be risky if any part of the input is not properly sanitized.
|
|
||
| try { | ||
| const [record] = await this.prisma.$queryRaw<AppealCountRecord[]>(query); | ||
| const rawCount = Number(record?.count ?? 0); |
There was a problem hiding this comment.
The use of Number(record?.count ?? 0) is correct for handling potential undefined values, but ensure that the database always returns a valid integer for count. If there is a possibility of receiving unexpected types, further validation might be necessary.
| deletePendingReviewsForResource: jest.fn().mockResolvedValue(0), | ||
| createPendingReview: jest.fn().mockResolvedValue(true), | ||
| getActiveSubmissionCount: jest.fn().mockResolvedValue(1), | ||
| getActiveContestSubmissions: jest |
There was a problem hiding this comment.
The getActiveContestSubmissions mock is returning an empty array. Ensure that this is the intended behavior for the test case. If the function is expected to return data in some scenarios, consider adding test cases to cover those scenarios.
| getActiveContestSubmissions: jest | ||
| .fn() | ||
| .mockResolvedValue([] as ActiveContestSubmission[]), | ||
| getActiveContestSubmissionIds: jest.fn().mockResolvedValue([]), |
There was a problem hiding this comment.
The getActiveContestSubmissionIds mock is returning an empty array. Verify if this is the expected return value for the test case. If there are cases where this function should return a non-empty array, consider adding tests for those cases.
| getActiveContestSubmissionIds: jest.fn().mockResolvedValue([]), | ||
| getAllSubmissionIdsOrdered: jest.fn().mockResolvedValue([]), | ||
| getExistingReviewPairs: jest.fn().mockResolvedValue(new Set()), | ||
| getReviewerSubmissionPairs: jest.fn().mockResolvedValue(new Set()), |
There was a problem hiding this comment.
The getReviewerSubmissionPairs mock is returning an empty set. Confirm if this is the expected outcome for the test. If there are situations where this function should return a non-empty set, consider adding tests to cover those situations.
| challengeWithZeroSubmissions, | ||
| ); | ||
| reviewServiceMockFns.getActiveSubmissionCount.mockResolvedValueOnce(0); | ||
| reviewServiceMockFns.getActiveContestSubmissionIds.mockResolvedValueOnce( |
There was a problem hiding this comment.
The function getActiveContestSubmissionIds is being used here instead of getActiveSubmissionCount. Ensure that this change is intentional and that the logic elsewhere in the code is compatible with this change, as it may affect how submissions are processed.
| new Set(), | ||
| ); | ||
| reviewServiceMockFns.getAllSubmissionIdsOrdered | ||
| .mockResolvedValueOnce(['submission-1', 'submission-2']) |
There was a problem hiding this comment.
The method mockResolvedValueOnce is called twice with the same array ['submission-1', 'submission-2']. If the intention is to mock different responses for consecutive calls, consider using mockResolvedValueOnce with different values or use mockResolvedValue for repeated values.
| .mockResolvedValueOnce(['submission-1', 'submission-2']); | ||
| reviewServiceMockFns.getExistingReviewPairs | ||
| .mockResolvedValueOnce(new Set()) | ||
| .mockResolvedValueOnce(new Set()); |
There was a problem hiding this comment.
The method mockResolvedValueOnce is called twice with the same value new Set(). If the intention is to mock different responses for consecutive calls, consider using mockResolvedValueOnce with different values or use mockResolvedValue for repeated values.
| .mockResolvedValueOnce(new Set()); | ||
| reviewServiceMockFns.getReviewerSubmissionPairs | ||
| .mockResolvedValueOnce(new Set(['iter-resource:submission-1'])) | ||
| .mockResolvedValueOnce(new Set(['iter-resource:submission-1'])); |
There was a problem hiding this comment.
The method mockResolvedValueOnce is called twice with the same set new Set(['iter-resource:submission-1']). If the intention is to mock different responses for consecutive calls, consider using mockResolvedValueOnce with different values or use mockResolvedValue for repeated values.
|
|
||
| expect(reviewServiceMockFns.createPendingReview).toHaveBeenCalledWith( | ||
| 'submission-1', | ||
| 'submission-2', |
There was a problem hiding this comment.
The change from 'submission-1' to 'submission-2' should be verified against the requirements or specifications to ensure it aligns with the intended functionality. If this change is intentional, ensure that all related tests and documentation are updated accordingly.
| schedulerAdvanceSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('should skip previously reviewed submissions when selecting the next iterative review', async () => { |
There was a problem hiding this comment.
The test case description 'should skip previously reviewed submissions when selecting the next iterative review' could be misleading. Consider clarifying the expected behavior in the test case name to better reflect the logic being tested.
|
|
||
| mockChallengeApiService.getChallengeById | ||
| .mockResolvedValueOnce(challengeWithOpenPhase) | ||
| .mockResolvedValueOnce(challengeAfterClose); |
There was a problem hiding this comment.
The use of mockResolvedValueOnce for mockChallengeApiService.getChallengeById could be improved by verifying that the mock is called with the expected arguments. This ensures the mock behaves as intended and the test is more robust.
| initialScore: 30, | ||
| } as ReviewCompletedPayload); | ||
|
|
||
| expect(reviewServiceMockFns.createPendingReview).toHaveBeenCalledWith( |
There was a problem hiding this comment.
Consider adding assertions to verify the state or behavior before calling autopilotService.handleReviewCompleted. This can help ensure the test setup is correct and the function is being tested under the right conditions.
| initialScore: 30, | ||
| } as ReviewCompletedPayload); | ||
|
|
||
| expect(reviewServiceMockFns.createPendingReview).toHaveBeenCalledWith( |
There was a problem hiding this comment.
The test could benefit from additional assertions to verify the state or side effects after autopilotService.handleReviewCompleted is called, beyond just checking the call to createPendingReview. This would make the test more comprehensive.
| topgearChallenge, | ||
| ); | ||
| reviewServiceMockFns.getActiveSubmissionCount.mockResolvedValueOnce(0); | ||
| reviewServiceMockFns.getActiveContestSubmissionIds.mockResolvedValueOnce( |
There was a problem hiding this comment.
The function getActiveContestSubmissionIds is being used here, but it is not clear if this change is intentional or if it replaces the previous getActiveSubmissionCount. Ensure that this change aligns with the intended functionality.
| expect(reviewServiceMockFns.createPendingReview).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('keeps Topgear submission phase open when scheduled via system-new-challenge operator', async () => { |
There was a problem hiding this comment.
Consider adding a test case to verify the behavior when the system-new-challenge operator schedules a phase that should not remain open. This will ensure comprehensive test coverage for different scenarios.
| phaseTypeName: submissionPhase.name, | ||
| state: 'END', | ||
| operator: AutopilotOperator.SYSTEM_SCHEDULER, | ||
| operator: AutopilotOperator.SYSTEM_NEW_CHALLENGE, |
There was a problem hiding this comment.
The operator value has been changed from AutopilotOperator.SYSTEM_SCHEDULER to AutopilotOperator.SYSTEM_NEW_CHALLENGE. Ensure that this change aligns with the intended logic and that all related functionality is updated accordingly.
1 participant
Lots of QA fixes and updates