Skip to content

Feat/v6 #1276

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 28, 2025
Merged

Feat/v6 #1276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0041f7
Tweaks for showing specific details to mixed roles
jmgasper Oct 28, 2025
bed5fdd
Fix showing Checkpoint Screening to Checkpoint Screener
jmgasper Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/apps/review/src/lib/components/common/TableColumnRenderers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,11 @@ export function renderReviewScoreCell(
.trim()
.toUpperCase()
: ''
const hasValidScore = typeof review.finalScore === 'number'
&& Number.isFinite(review.finalScore)
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 readability]
The use of typeof review.finalScore === 'number' && Number.isFinite(review.finalScore) is a good check for a valid score. However, consider using Number.isNaN(review.finalScore) to explicitly check for NaN values, which can sometimes be more readable and convey the intent more clearly.


if (!normalizedStatus) {
return true
return !hasValidScore
}

return normalizedStatus !== 'COMPLETED'
Expand Down Expand Up @@ -540,9 +543,9 @@ export function renderScoreCell(
}

const reviewInfo = reviewDetail.reviewInfo
const reviewId = reviewInfo?.id || reviewDetail.reviewId
const reviewId = reviewInfo?.id ?? reviewDetail.reviewId
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The change from || to ?? is appropriate here as it ensures that reviewDetail.reviewId is only used when reviewInfo?.id is null or undefined, which is the intended behavior for nullish coalescing.


if (!reviewInfo || !reviewId) {
if (!reviewId) {
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The removal of the !reviewInfo check may lead to potential issues if reviewInfo is null or undefined. Ensure that reviewInfo is always defined when this function is called, or consider adding a check to prevent possible runtime errors.

return (
<span className={styles.notReviewed}>
--
Expand Down
67 changes: 56 additions & 11 deletions src/apps/review/src/lib/hooks/useFetchChallengeRelativeDatas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,52 @@ import {
fetchAllResourceRoles,
fetchChallengeResouces,
} from '../services/resources.service'
import { BackendResourceRole, BackendSubmission, ChallengeRealtiveInfosMapping } from '../models'
import { fetchSubmissions } from '../services'
import { BackendResourceRole, BackendSubmission, ChallengeRealtiveInfosMapping } from '../models'

const FALLBACK_RESOURCE_ROLES: BackendResourceRole[] = [
{
fullReadAccess: false,
fullWriteAccess: false,
id: 'fd672cca-556e-4d16-b0a2-718218edd412',
isActive: true,
legacyId: 19,
name: 'Checkpoint Screener',
selfObtainable: false,
},
{
fullReadAccess: true,
fullWriteAccess: false,
id: '3970272b-85b4-48d8-8439-672b4f6031bd',
isActive: true,
legacyId: 20,
name: 'Checkpoint Reviewer',
selfObtainable: false,
},
]

const ensureResourceRoleMapping = (
mapping: { [key: string]: BackendResourceRole } | undefined,
): { [key: string]: BackendResourceRole } => {
const base = { ...(mapping ?? {}) }

FALLBACK_RESOURCE_ROLES.forEach(role => {
const existing = base[role.id]
if (!existing) {
base[role.id] = role
return
}

if (!existing.name) {
base[role.id] = {
...existing,
name: role.name,
}
}
})

return base
}

export interface useFetchChallengeRelativeDatasProps {
challengeRelativeInfosMapping: ChallengeRealtiveInfosMapping // from challenge id to list of my role
Expand Down Expand Up @@ -57,20 +101,21 @@ export function useFetchChallengeRelativeDatas(
setResourceRoleReviewer(find(results.data, {
name: 'Reviewer',
}))
setResourceRoleMapping(
reduce(
results.data,
(mappingResult, resourceRole: BackendResourceRole) => ({
...mappingResult,
[resourceRole.id]: resourceRole,
}),
{},
),
)
const mapping = ensureResourceRoleMapping(reduce(
results.data,
(mappingResult, resourceRole: BackendResourceRole) => ({
...mappingResult,
[resourceRole.id]: resourceRole,
}),
{},
))

setResourceRoleMapping(mapping)
setIsLoadingResourceRoles(false)
})
.catch(e => {
handleError(e)
setResourceRoleMapping(current => ensureResourceRoleMapping(current))
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
In the catch block, setResourceRoleMapping(current => ensureResourceRoleMapping(current)) is called. If current is undefined, ensureResourceRoleMapping will handle it, but it's worth considering if this behavior is intentional and if it might lead to unexpected results. Ensure that undefined is an acceptable input here.

setIsLoadingResourceRoles(false)
})
})
Expand Down
2 changes: 2 additions & 0 deletions src/apps/review/src/lib/models/ReviewResult.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { BackendReview } from './BackendReview.model'
* Review result info
*/
export interface ReviewResult {
id?: string
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The id field is added as optional in the ReviewResult interface. Ensure that the rest of the codebase handles cases where id might be undefined, especially if id is used as a unique identifier or key in collections.

appeals: AppealResult[]
createdAt: string | Date
createdAtString?: string // this field is calculated at frontend
Expand Down Expand Up @@ -111,6 +112,7 @@ export function convertBackendReviewToReviewResult(
appeals: [],
createdAt,
createdAtString,
id: data.id,
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The id field is directly assigned from data.id. Ensure that data.id is always defined and valid, or handle cases where it might be undefined to prevent potential issues in the UI or data processing.

phaseName,
resourceId: data.resourceId,
reviewDate,
Expand Down
12 changes: 7 additions & 5 deletions src/apps/review/src/lib/models/SubmissionInfo.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface SubmissionInfo {
memberId: string
userInfo?: BackendResource // this field is calculated at frontend
review?: ReviewInfo
reviewInfos?: ReviewInfo[]
reviews?: ReviewResult[]
/**
* Backend review type identifier (e.g. 'Post-Mortem Review').
Expand Down Expand Up @@ -109,6 +110,9 @@ export function convertBackendSubmissionToSubmissionInfo(
? isPassingReviewRaw
: undefined
const reviewEntries = Array.isArray(data.review) ? data.review : []
const reviewInfos = reviewEntries.map(convertBackendReviewToReviewInfo)
Copy link

@github-actions github-actions bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The reviewEntries variable is already checked to be an array, so the map operation on line 113 is safe. However, consider adding a check to ensure that convertBackendReviewToReviewInfo and convertBackendReviewToReviewResult handle any potential edge cases within their implementations, such as null or undefined values within reviewEntries.

const reviewResults = reviewEntries.map(convertBackendReviewToReviewResult)
const primaryReviewInfo = reviewInfos[0]
const primaryReview = reviewEntries[0]

return {
Expand All @@ -117,11 +121,9 @@ export function convertBackendSubmissionToSubmissionInfo(
isLatest: data.isLatest,
isPassingReview,
memberId: data.memberId,
review:
primaryReview
? convertBackendReviewToReviewInfo(primaryReview)
: undefined,
reviews: reviewEntries.map(convertBackendReviewToReviewResult),
review: primaryReviewInfo,
reviewInfos,
reviews: reviewResults,
reviewTypeId: primaryReview?.typeId ?? undefined,
submittedDate,
submittedDateString,
Expand Down
Loading