-
Notifications
You must be signed in to change notification settings - Fork 3
Fix an issue with F2Fs not closing and appeals not opening immediately after review #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import type { | |
| IPhase, | ||
| IChallengeReviewer, | ||
| } from '../../challenge/interfaces/challenge.interface'; | ||
| import type { ChallengeCompletionService } from './challenge-completion.service'; | ||
| import { | ||
| ITERATIVE_REVIEW_PHASE_NAME, | ||
| REGISTRATION_PHASE_NAME, | ||
|
|
@@ -105,6 +106,7 @@ describe('First2FinishService', () => { | |
| let reviewService: jest.Mocked<ReviewService>; | ||
| let resourcesService: jest.Mocked<ResourcesService>; | ||
| let configService: jest.Mocked<ConfigService>; | ||
| let challengeCompletionService: jest.Mocked<ChallengeCompletionService>; | ||
| let service: First2FinishService; | ||
|
|
||
| beforeEach(() => { | ||
|
|
@@ -132,6 +134,7 @@ describe('First2FinishService', () => { | |
|
|
||
| resourcesService = { | ||
| getReviewerResources: jest.fn(), | ||
| getMemberHandleMap: jest.fn(), | ||
| } as unknown as jest.Mocked<ResourcesService>; | ||
|
|
||
| configService = { | ||
|
|
@@ -146,16 +149,22 @@ describe('First2FinishService', () => { | |
| }), | ||
| } as unknown as jest.Mocked<ConfigService>; | ||
|
|
||
| challengeCompletionService = { | ||
| completeChallengeWithWinners: jest.fn(), | ||
| } as unknown as jest.Mocked<ChallengeCompletionService>; | ||
|
|
||
| service = new First2FinishService( | ||
| challengeApiService, | ||
| schedulerService, | ||
| reviewService, | ||
| resourcesService, | ||
| configService, | ||
| challengeCompletionService, | ||
| ); | ||
|
|
||
| reviewService.getReviewerSubmissionPairs.mockResolvedValue(new Set()); | ||
| reviewService.getPendingReviewCount.mockResolvedValue(0); | ||
| resourcesService.getMemberHandleMap.mockResolvedValue(new Map()); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
|
|
@@ -491,5 +500,73 @@ describe('First2FinishService', () => { | |
| state: 'END', | ||
| }), | ||
| ); | ||
|
|
||
| expect(challengeCompletionService.completeChallengeWithWinners).toHaveBeenCalledWith( | ||
| challenge.id, | ||
| [ | ||
| { | ||
| handle: 'submitter', | ||
| placement: 1, | ||
| userId: 4001, | ||
| }, | ||
| ], | ||
| { reason: 'iterative-review-pass' }, | ||
| ); | ||
| }); | ||
|
|
||
| it('uses member handle map when completing after a passing iterative review', async () => { | ||
| const iterativePhase = buildIterativePhase({ | ||
| id: 'iter-phase', | ||
| isOpen: true, | ||
| actualEndDate: null, | ||
| }); | ||
|
|
||
| const challenge = buildChallenge({ | ||
| phases: [iterativePhase], | ||
| reviewers: [buildReviewer()], | ||
| }); | ||
|
|
||
| resourcesService.getMemberHandleMap.mockResolvedValue( | ||
| new Map([['4001', 'resolvedHandle']]), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| ); | ||
| reviewService.getScorecardPassingScore.mockResolvedValue(80); | ||
|
|
||
| await service.handleIterativeReviewCompletion( | ||
| challenge, | ||
| iterativePhase, | ||
| { | ||
| score: 90, | ||
| scorecardId: 'iterative-scorecard', | ||
| resourceId: 'resource-1', | ||
| submissionId: 'submission-1', | ||
| phaseId: iterativePhase.id, | ||
| }, | ||
| { | ||
| reviewId: 'review-1', | ||
| challengeId: challenge.id, | ||
| submissionId: 'submission-1', | ||
| phaseId: iterativePhase.id, | ||
| scorecardId: 'iterative-scorecard', | ||
| reviewerResourceId: 'resource-1', | ||
| reviewerHandle: 'iterativeReviewer', | ||
| reviewerMemberId: '2001', | ||
| submitterHandle: 'submitter', | ||
| submitterMemberId: '4001', | ||
| completedAt: iso(), | ||
| initialScore: 90, | ||
| }, | ||
| ); | ||
|
|
||
| expect(challengeCompletionService.completeChallengeWithWinners).toHaveBeenCalledWith( | ||
| challenge.id, | ||
| [ | ||
| { | ||
| handle: 'resolvedHandle', | ||
| placement: 1, | ||
| userId: 4001, | ||
| }, | ||
| ], | ||
| { reason: 'iterative-review-pass' }, | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ import { | |
| } from '../constants/challenge.constants'; | ||
| import { isActiveStatus } from '../utils/config.utils'; | ||
| import { selectScorecardId } from '../utils/reviewer.utils'; | ||
| import { ChallengeCompletionService } from './challenge-completion.service'; | ||
|
|
||
| @Injectable() | ||
| export class First2FinishService { | ||
|
|
@@ -46,6 +47,7 @@ export class First2FinishService { | |
| private readonly reviewService: ReviewService, | ||
| private readonly resourcesService: ResourcesService, | ||
| private readonly configService: ConfigService, | ||
| private readonly challengeCompletionService: ChallengeCompletionService, | ||
| ) { | ||
| this.iterativeRoles = PHASE_ROLE_MAP[ITERATIVE_REVIEW_PHASE_NAME] ?? [ | ||
| 'Iterative Reviewer', | ||
|
|
@@ -189,6 +191,8 @@ export class First2FinishService { | |
| projectStatus: challenge.status, | ||
| }); | ||
| } | ||
|
|
||
| await this.completeFirst2FinishChallenge(challenge, payload); | ||
| } else { | ||
| this.logger.log( | ||
| `Iterative review failed for submission ${payload.submissionId} on challenge ${challenge.id} (score ${finalScore}, passing ${passingScore}).`, | ||
|
|
@@ -199,6 +203,57 @@ export class First2FinishService { | |
| } | ||
| } | ||
|
|
||
| private async completeFirst2FinishChallenge( | ||
| challenge: IChallenge, | ||
| payload: ReviewCompletedPayload, | ||
| ): Promise<void> { | ||
| if (!isActiveStatus(challenge.status)) { | ||
| this.logger.debug( | ||
| `Skipping completion for challenge ${challenge.id}; status ${challenge.status ?? 'UNKNOWN'} is not ACTIVE.`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const memberIdRaw = payload.submitterMemberId ?? ''; | ||
| const numericMemberId = Number(memberIdRaw); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
|
|
||
| if (!Number.isFinite(numericMemberId)) { | ||
| this.logger.warn( | ||
| `Unable to complete challenge ${challenge.id} after passing iterative review; submitterMemberId is invalid (${payload.submitterMemberId}).`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| let handle = payload.submitterHandle?.trim(); | ||
| try { | ||
| const handleMap = await this.resourcesService.getMemberHandleMap( | ||
| challenge.id, | ||
| [String(memberIdRaw)], | ||
| ); | ||
| handle = handleMap.get(String(memberIdRaw)) ?? handle; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [💡 |
||
| } catch (error) { | ||
| const err = error as Error; | ||
| this.logger.warn( | ||
| `Failed to resolve handle for member ${memberIdRaw} on challenge ${challenge.id}; using provided payload handle.`, | ||
| err.stack, | ||
| ); | ||
| } | ||
|
|
||
| await this.challengeCompletionService.completeChallengeWithWinners( | ||
| challenge.id, | ||
| [ | ||
| { | ||
| userId: numericMemberId, | ||
| handle: handle && handle.length ? handle : String(memberIdRaw), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [💡 |
||
| placement: 1, | ||
| }, | ||
| ], | ||
| { | ||
| reason: 'iterative-review-pass', | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| async handleIterativePhaseClosed(challengeId: string): Promise<void> { | ||
| try { | ||
| await this.prepareNextIterativeReview(challengeId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -837,6 +837,48 @@ export class SchedulerService implements OnModuleInit, OnModuleDestroy { | |
|
|
||
| let skipPhaseChain = false; | ||
| let skipFinalization = false; | ||
| let appealsOpenedImmediately = false; | ||
|
|
||
| if (operation === 'close' && isReviewPhase && result.next?.phases?.length) { | ||
| const appealsSuccessors = result.next.phases.filter( | ||
| (phase) => | ||
| this.isAppealsPhaseName(phase.name) || | ||
| this.isAppealsResponsePhaseName(phase.name), | ||
| ); | ||
|
|
||
| if (appealsSuccessors.length > 0) { | ||
| try { | ||
| if (this.phaseChainCallback) { | ||
| this.logger.log( | ||
| `[APPEALS FAST-TRACK] Closing Review for challenge ${data.challengeId}; opening ${appealsSuccessors.length} appeals-related phase(s) immediately.`, | ||
| ); | ||
|
|
||
| const callbackResult = this.phaseChainCallback( | ||
| data.challengeId, | ||
| data.projectId, | ||
| data.projectStatus ?? ChallengeStatusEnum.ACTIVE, | ||
| appealsSuccessors, | ||
| ); | ||
|
|
||
| if (callbackResult instanceof Promise) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [💡 |
||
| await callbackResult; | ||
| } | ||
|
|
||
| appealsOpenedImmediately = true; | ||
| } else { | ||
| this.logger.warn( | ||
| `[APPEALS FAST-TRACK] Phase chain callback not set; unable to auto-open appeals for challenge ${data.challengeId}.`, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| const err = error as Error; | ||
| this.logger.error( | ||
| `[APPEALS FAST-TRACK] Failed to fast-open appeals for challenge ${data.challengeId}: ${err.message}`, | ||
| err.stack, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (operation === 'close' && isReviewPhase) { | ||
| this.reviewCloseRetryAttempts.delete( | ||
|
|
@@ -1058,16 +1100,31 @@ export class SchedulerService implements OnModuleInit, OnModuleDestroy { | |
| result.next.phases && | ||
| result.next.phases.length > 0 | ||
| ) { | ||
| const phasesToOpen = appealsOpenedImmediately | ||
| ? result.next.phases.filter( | ||
| (phase) => | ||
| !this.isAppealsPhaseName(phase.name) && | ||
| !this.isAppealsResponsePhaseName(phase.name), | ||
| ) | ||
| : result.next.phases; | ||
|
|
||
| if (!phasesToOpen.length) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [💡 |
||
| this.logger.log( | ||
| `[PHASE CHAIN] All successor appeals phases already opened for challenge ${data.challengeId}; skipping additional phase chaining.`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if (this.phaseChainCallback) { | ||
| this.logger.log( | ||
| `[PHASE CHAIN] Triggering phase chain callback for challenge ${data.challengeId} with ${result.next.phases.length} next phases`, | ||
| `[PHASE CHAIN] Triggering phase chain callback for challenge ${data.challengeId} with ${phasesToOpen.length} next phases`, | ||
| ); | ||
| const callbackResult = this.phaseChainCallback( | ||
| data.challengeId, | ||
| data.projectId, | ||
| data.projectStatus, | ||
| result.next.phases, | ||
| phasesToOpen, | ||
| ); | ||
|
|
||
| // Handle both sync and async callbacks | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[⚠️
correctness]The use of
voidto ignore the promise returned bygenerateChallengePaymentscould lead to unhandled errors if the promise is rejected. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.