Skip to content

Commit c90f16f

Browse files
jmgasperjmgasper
committed
Fix iterative review visibility for reviewer
1 parent 9f80f06 commit c90f16f

File tree

5 files changed

+168
-36
lines changed

5 files changed

+168
-36
lines changed

‎src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentIterativeReview.tsx‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
} from '../../../config/index.config'
1717
import { ChallengeDetailContext } from '../../contexts'
1818
import { hasSubmitterPassedThreshold } from '../../utils/reviewScoring'
19+
import { shouldIncludeInReviewPhase } from '../../utils/reviewPhaseGuards'
1920

2021
interface Props {
2122
reviews: SubmissionInfo[]
@@ -109,9 +110,22 @@ export const TabContentIterativeReview: FC<Props> = (props: Props) => {
109110

110111
const filteredRows = useMemo(() => {
111112
const phaseId = props.phaseIdFilter?.trim()
112-
if (!phaseId) return sourceRows
113-
return sourceRows.filter(s => s.review?.phaseId === phaseId)
114-
}, [sourceRows, props.phaseIdFilter])
113+
if (phaseId) {
114+
return sourceRows.filter(s => s.review?.phaseId === phaseId)
115+
}
116+
117+
if (!isPostMortemPhase) {
118+
const iterativeOnly = sourceRows.filter(submission => !shouldIncludeInReviewPhase(
119+
submission,
120+
challengeInfo?.phases,
121+
))
122+
if (iterativeOnly.length) {
123+
return iterativeOnly
124+
}
125+
}
126+
127+
return sourceRows
128+
}, [sourceRows, props.phaseIdFilter, isPostMortemPhase, challengeInfo?.phases])
115129

116130
const reviewRows = useMemo(() => {
117131
const map = new Map<string, SubmissionInfo>()

‎src/apps/review/src/lib/components/common/reviewResult.spec.ts‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
import { resolveSubmissionReviewResult } from './reviewResult'
1+
import type { ReviewInfo } from '../../models/ReviewInfo.model'
2+
23
import type { AggregatedReviewDetail, SubmissionRow } from './types'
4+
import { resolveSubmissionReviewResult } from './reviewResult'
35

46
const DEFAULT_PASSING_SCORE = 80
57

@@ -23,6 +25,20 @@ const buildSubmission = (
2325
}
2426
}
2527

28+
const buildReviewInfo = (
29+
overrides: Partial<ReviewInfo> = {},
30+
): ReviewInfo => ({
31+
committed: true,
32+
createdAt: new Date()
33+
.toISOString(),
34+
resourceId: overrides.resourceId ?? 'res-1',
35+
reviewItems: [],
36+
scorecardId: overrides.scorecardId ?? 'scorecard-1',
37+
updatedAt: new Date()
38+
.toISOString(),
39+
...overrides,
40+
})
41+
2642
describe('resolveSubmissionReviewResult', () => {
2743
it('returns undefined when any reviewer is still pending in a multi-review setup', () => {
2844
const submission = buildSubmission([
@@ -102,4 +118,50 @@ describe('resolveSubmissionReviewResult', () => {
102118
expect(result)
103119
.toBe('PASS')
104120
})
121+
122+
it('returns FAIL when metadata outcome is fail even if score passes threshold', () => {
123+
const submission = buildSubmission([
124+
{
125+
finalScore: 92,
126+
resourceId: 'res-1',
127+
reviewInfo: buildReviewInfo({
128+
metadata: {
129+
minimumScore: 1,
130+
outcome: 'fail',
131+
},
132+
}),
133+
status: 'COMPLETED',
134+
},
135+
], 92)
136+
137+
const result = resolveSubmissionReviewResult(submission, {
138+
defaultMinimumPassingScore: DEFAULT_PASSING_SCORE,
139+
})
140+
141+
expect(result)
142+
.toBe('FAIL')
143+
})
144+
145+
it('returns PASS when metadata outcome is pass even if score fails threshold', () => {
146+
const submission = buildSubmission([
147+
{
148+
finalScore: 40,
149+
resourceId: 'res-1',
150+
reviewInfo: buildReviewInfo({
151+
metadata: {
152+
minimumScore: 100,
153+
outcome: 'pass',
154+
},
155+
}),
156+
status: 'COMPLETED',
157+
},
158+
], 40)
159+
160+
const result = resolveSubmissionReviewResult(submission, {
161+
defaultMinimumPassingScore: DEFAULT_PASSING_SCORE,
162+
})
163+
164+
expect(result)
165+
.toBe('PASS')
166+
})
105167
})

‎src/apps/review/src/lib/components/common/reviewResult.ts‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -414,24 +414,24 @@ export function resolveSubmissionReviewResult(
414414
? (reviewScore >= minimumPassingScore ? 'PASS' : 'FAIL')
415415
: undefined
416416

417-
if (scoreOutcome === 'PASS') {
417+
if (submission.isPassingReview === true) {
418418
return 'PASS'
419419
}
420420

421-
if (
422-
scoreOutcome === 'FAIL'
423-
&& submission.isPassingReview !== true
424-
&& metadataOutcome !== 'PASS'
425-
) {
421+
if (submission.isPassingReview === false) {
426422
return 'FAIL'
427423
}
428424

429-
if (submission.isPassingReview === true) {
425+
if (metadataOutcome === 'FAIL') {
426+
return 'FAIL'
427+
}
428+
429+
if (scoreOutcome === 'PASS') {
430430
return 'PASS'
431431
}
432432

433-
if (submission.isPassingReview === false) {
434-
return 'FAIL'
433+
if (scoreOutcome === 'FAIL') {
434+
return metadataOutcome === 'PASS' ? 'PASS' : 'FAIL'
435435
}
436436

437437
if (metadataOutcome) {

‎src/apps/review/src/lib/hooks/useFetchScreeningReview.ts‎

Lines changed: 78 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import { normalizeReviewMetadata } from '../utils/metadataMatching'
3939
import { resolvePhaseMeta } from '../utils/phaseResolution'
4040
import { buildReviewForResource } from '../utils/reviewBuilding'
4141
import { resolveReviewPhaseId, reviewMatchesPhase } from '../utils/reviewMatching'
42+
import { shouldIncludeInReviewPhase } from '../utils/reviewPhaseGuards'
4243
import {
4344
buildResourceFromReviewHandle,
4445
determinePassFail,
@@ -75,6 +76,18 @@ const isCheckpointSubmission = (submission: BackendSubmission): boolean => (
7576
normalizeSubmissionType(submission?.type) === SUBMISSION_TYPE_CHECKPOINT
7677
)
7778

79+
const resolveCheckpointSubmissionScore = (
80+
review: BackendReview,
81+
submission: BackendSubmission,
82+
): number | undefined => {
83+
const reviewStatus = (review.status || '').toUpperCase()
84+
if (reviewStatus !== 'COMPLETED' && reviewStatus !== 'SUBMITTED') {
85+
return undefined
86+
}
87+
88+
return parseSubmissionScore(submission.screeningScore)
89+
}
90+
7891
export interface useFetchScreeningReviewProps {
7992
mappingReviewAppeal: MappingReviewAppeal // from review id to appeal info
8093
// screening data
@@ -491,6 +504,24 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
491504
const reviewScorecardId = reviewPhaseMeta.scorecardId
492505
const reviewPhaseIds = reviewPhaseMeta.phaseIds
493506

507+
const iterativeReviewPhaseMeta = useMemo(
508+
() => resolvePhaseMeta(
509+
'Iterative Review',
510+
challengeInfo?.phases,
511+
challengeInfo?.reviewers,
512+
challengeReviews,
513+
challengeLegacy?.reviewScorecardId,
514+
),
515+
[
516+
challengeInfo?.phases,
517+
challengeInfo?.reviewers,
518+
challengeReviews,
519+
challengeLegacy?.reviewScorecardId,
520+
],
521+
)
522+
const iterativeReviewScorecardId = iterativeReviewPhaseMeta.scorecardId
523+
const iterativeReviewPhaseIds = iterativeReviewPhaseMeta.phaseIds
524+
494525
const approvalPhaseMeta = useMemo(
495526
() => resolvePhaseMeta(
496527
'Approval',
@@ -1063,7 +1094,17 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
10631094
.find(r => (r.roleName || '').toLowerCase() === 'checkpoint reviewer')?.id
10641095

10651096
const checkpointReviewRows = checkpointSubmissions.reduce<Screening[]>((rows, item) => {
1066-
const matchedReview = checkpointReviewsBySubmission.get(item.id)
1097+
let matchedReview = checkpointReviewsBySubmission.get(item.id)
1098+
if (!matchedReview && item.reviewResourceMapping) {
1099+
matchedReview = Object.values(item.reviewResourceMapping)
1100+
.find(review => reviewMatchesPhase(
1101+
review,
1102+
checkpointReviewScorecardId,
1103+
checkpointReviewPhaseIds,
1104+
'Checkpoint Review',
1105+
))
1106+
}
1107+
10671108
if (!matchedReview) {
10681109
return rows
10691110
}
@@ -1072,12 +1113,9 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
10721113
let numericScore = getNumericScore(matchedReview)
10731114

10741115
if (numericScore === undefined) {
1075-
const reviewStatus = (matchedReview.status || '').toUpperCase()
1076-
if (reviewStatus === 'COMPLETED' || reviewStatus === 'SUBMITTED') {
1077-
const submissionScore = parseSubmissionScore(item.screeningScore)
1078-
if (submissionScore !== undefined) {
1079-
numericScore = submissionScore
1080-
}
1116+
const submissionScore = resolveCheckpointSubmissionScore(matchedReview, item)
1117+
if (submissionScore !== undefined) {
1118+
numericScore = submissionScore
10811119
}
10821120
}
10831121

@@ -1486,30 +1524,37 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
14861524
combinedReviewerIds.push(normalized)
14871525
}
14881526

1489-
reviewerIds.forEach(appendReviewerId)
1490-
forEach(challengeSubmission.review, reviewEntry => {
1491-
const matchesReviewPhase = reviewMatchesPhase(
1492-
reviewEntry,
1527+
const matchesReviewPhase = (candidate: BackendReview | undefined): boolean => (
1528+
reviewMatchesPhase(
1529+
candidate,
14931530
reviewScorecardId,
14941531
reviewPhaseIds,
14951532
'Review',
14961533
)
1497-
if (matchesReviewPhase) {
1534+
|| reviewMatchesPhase(
1535+
candidate,
1536+
iterativeReviewScorecardId,
1537+
iterativeReviewPhaseIds,
1538+
'Iterative Review',
1539+
)
1540+
)
1541+
1542+
reviewerIds.forEach(appendReviewerId)
1543+
forEach(challengeSubmission.review, reviewEntry => {
1544+
if (matchesReviewPhase(reviewEntry)) {
14981545
appendReviewerId(reviewEntry?.resourceId)
14991546
}
15001547
})
15011548

15021549
forEach(combinedReviewerIds, reviewerId => {
15031550
const matchingReview = challengeSubmission.review?.find(
1504-
r => r.resourceId === reviewerId && reviewMatchesPhase(
1505-
r,
1506-
reviewScorecardId,
1507-
reviewPhaseIds,
1508-
'Review',
1509-
),
1551+
r => r.resourceId === reviewerId && matchesReviewPhase(r),
15101552
)
1511-
const assignmentReview
1553+
const assignmentReviewCandidate
15121554
= reviewAssignmentsBySubmission[challengeSubmission.id]?.[reviewerId]
1555+
const assignmentReview = matchesReviewPhase(assignmentReviewCandidate)
1556+
? assignmentReviewCandidate
1557+
: undefined
15131558

15141559
const normalizedReviewForResource = buildReviewForResource({
15151560
assignmentReview,
@@ -1540,6 +1585,8 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
15401585
}, [
15411586
challengeReviewById,
15421587
contestSubmissions,
1588+
iterativeReviewPhaseIds,
1589+
iterativeReviewScorecardId,
15431590
resourceMemberIdMapping,
15441591
reviewerIds,
15451592
reviewAssignmentsBySubmission,
@@ -1688,11 +1735,19 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
16881735
return 0
16891736
}
16901737

1691-
const isDesignChallenge = challengeInfo?.track.name === DESIGN
1738+
const eligibleReviews = review.filter(submission => shouldIncludeInReviewPhase(
1739+
submission,
1740+
challengeInfo?.phases,
1741+
))
1742+
if (!eligibleReviews.length) {
1743+
return 0
1744+
}
1745+
1746+
const isDesignChallenge = challengeInfo?.track?.name === DESIGN
16921747

16931748
const filteredReviews = isDesignChallenge
1694-
? review
1695-
: review.filter(item => item.isLatest)
1749+
? eligibleReviews
1750+
: eligibleReviews.filter(item => item.isLatest)
16961751

16971752
if (!filteredReviews.length) {
16981753
return 0
@@ -1724,7 +1779,7 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
17241779
return Math.round(
17251780
(completedReviews.length * 100) / filteredReviews.length,
17261781
)
1727-
}, [review, challengeInfo])
1782+
}, [review, challengeInfo?.phases, challengeInfo?.track?.name])
17281783

17291784
useEffect(() => () => {
17301785
cancelLoadResourceAppeal()

‎src/apps/review/src/lib/utils/metadataMatching.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ export function isPhaseAllowedForReview(phaseName?: string | null): boolean {
181181
}
182182

183183
return normalizedAlpha === 'review'
184+
|| normalizedAlpha === 'iterativereview'
184185
|| normalizedAlpha === 'postmortem'
185186
|| normalizedAlpha === 'approval'
186187
}

0 commit comments

Comments
 (0)