Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,29 @@ export const TabContentScreening: FC<Props> = (props: Props) => {
const filteredScreening = useMemo<Screening[]>(
() => {
const baseRows = props.screening ?? []
// Defensive filtering: exclude any entries that don't belong to Screening phase
const phaseValidatedRows = baseRows.filter(row => {
if (!row.reviewId) {

Choose a reason for hiding this comment

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

[❗❗ correctness]
Consider checking for row.phaseName before calling toLowerCase() and trim(). If row.phaseName is null or undefined, this could lead to a runtime error.

return true
}

const normalizedPhaseName = row.phaseName
?.toLowerCase()
.trim()

return normalizedPhaseName === 'screening'
})
const canSeeAll = isPrivilegedRole || hasReviewerRole

if (isChallengeCompleted && !canSeeAll && !hasPassedScreeningThreshold) {
return []
}

if (canSeeAll || (isChallengeCompleted && hasPassedScreeningThreshold)) {
return baseRows
return phaseValidatedRows
}

return baseRows.filter(row => {
return phaseValidatedRows.filter(row => {
if (row.myReviewResourceId
&& (screenerResourceIds.has(row.myReviewResourceId)
|| reviewerResourceIds.has(row.myReviewResourceId))) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ export const TableRegistration: FC<Props> = (props: Props) => {
[myRoles],
)

const hasManagerRole = useMemo(
() => (myRoles ?? [])

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider using toLowerCase().trim() consistently across all role checks for uniformity and to prevent potential issues with leading/trailing spaces.

.some(role => role
?.trim()
.toLowerCase() === 'manager'),
[myRoles],
)

const isAdmin = useMemo(
() => loginUserInfo?.roles?.some(
role => typeof role === 'string'
Expand All @@ -66,7 +74,7 @@ export const TableRegistration: FC<Props> = (props: Props) => {
[loginUserInfo?.roles],
)

const shouldDisplayEmail = hasCopilotRole || isAdmin
const shouldDisplayEmail = hasCopilotRole || hasManagerRole || isAdmin

const columns = useMemo<TableColumn<BackendResource>[]>(
() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ import {
import { ChallengeDetailContext, ReviewAppContext } from '../../contexts'
import { updateReview } from '../../services'
import { ConfirmModal } from '../ConfirmModal'
import { useSubmissionDownloadAccess } from '../../hooks'
import { useRole, useSubmissionDownloadAccess } from '../../hooks'
import type { UseSubmissionDownloadAccessResult } from '../../hooks/useSubmissionDownloadAccess'
import type { useRoleProps } from '../../hooks/useRole'

import styles from './TableSubmissionScreening.module.scss'

Expand Down Expand Up @@ -424,48 +425,79 @@ const isScreeningReviewInProgress = (entry: Screening): boolean => (
|| isInProgressStatus(entry.myReviewStatus)
)

/**
* Creates columns for displaying screening review data.
*
* Note: This function assumes that the input data has been validated
* to contain only Screening phase reviews. Defensive filtering is
* performed in TabContentScreening to ensure phase data isolation.
*
* @param config - Configuration for scorecard access and score masking
* @returns Array of table columns for screening data
*/
const createScreeningColumns = ({
canViewScorecard,
shouldMaskScore,
}: ScreeningColumnConfig): TableColumn<Screening>[] => [
{
label: 'Screener',
propertyName: 'screenerHandle',
renderer: (data: Screening) => (data.screener?.memberHandle ? (
<a
href={getHandleUrl(data.screener)}
target='_blank'
rel='noreferrer'
style={{
color: data.screener?.handleColor,
}}
onClick={function onClick() {
window.open(
getHandleUrl(data.screener),
'_blank',
)
}}
>
{data.screener?.memberHandle ?? ''}
</a>
) : (
<span
style={{
color: data.screener?.handleColor,
}}
>
{data.screener?.memberHandle ?? ''}
</span>
)),
propertyName: 'screener',
renderer: (data: Screening) => {
const normalizedPhaseName = data.phaseName
?.toLowerCase()
.trim()

if (normalizedPhaseName && normalizedPhaseName !== 'screening') {

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The normalizedPhaseName check ensures that only 'screening' phase data is processed. However, the logic for handling non-'screening' phases is simply returning an empty <span />. Consider logging or handling this case explicitly to avoid silent failures or unexpected behavior.

return <span />
}

// Display the screener handle from the Screening phase review
// (Review phase data is filtered out in TabContentScreening)
return data.screener?.memberHandle ? (
<a
href={getHandleUrl(data.screener)}
target='_blank'
rel='noreferrer'
style={{
color: data.screener?.handleColor,
}}
onClick={function onClick() {
window.open(
getHandleUrl(data.screener),
'_blank',
)
}}
>
{data.screener?.memberHandle ?? ''}
</a>
) : (
<span
style={{
color: data.screener?.handleColor,
}}
>
{data.screener?.memberHandle ?? ''}
</span>
)
},
type: 'element',
},
{
label: 'Screening Score',
propertyName: 'score',
renderer: (data: Screening) => {
const normalizedPhaseName = data.phaseName

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The normalizedPhaseName check is repeated in multiple places. Consider refactoring this logic into a helper function to improve maintainability and reduce code duplication.

?.toLowerCase()
.trim()
// Link to the Screening phase scorecard
// (Review phase scorecards are filtered out upstream)
const maskScore = shouldMaskScore(data)
const scoreValue = maskScore ? '--' : (data.score ?? '-')

if (normalizedPhaseName && normalizedPhaseName !== 'screening') {
return <span>{scoreValue}</span>
}

if (!data.reviewId || maskScore) {
return <span>{scoreValue}</span>
}
Expand Down Expand Up @@ -631,6 +663,7 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
getRestrictionMessageForMember,
currentMemberId,
}: UseSubmissionDownloadAccessResult = useSubmissionDownloadAccess()
const { hasReviewerRole }: useRoleProps = useRole()

const normalisedRoles = useMemo(
() => (myRoles ?? []).map(role => role.toLowerCase()),
Expand Down Expand Up @@ -971,7 +1004,7 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
return false
}

if (canViewAllScorecards) {
if (canViewAllScorecards || hasReviewerRole) {

Choose a reason for hiding this comment

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

[💡 readability]
The condition canViewAllScorecards || hasReviewerRole is used multiple times. Consider extracting this logic into a descriptive function or variable to improve readability and maintainability.

return true
}

Expand All @@ -996,6 +1029,7 @@ export const TableSubmissionScreening: FC<Props> = (props: Props) => {
},
[
canViewAllScorecards,
hasReviewerRole,
currentMemberId,
myResourceIds,
],
Expand Down
1 change: 1 addition & 0 deletions src/apps/review/src/lib/hooks/useFetchScreeningReview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1149,6 +1149,7 @@ export function useFetchScreeningReview(): useFetchScreeningReviewProps {
myReviewId: myAssignment?.id,
myReviewResourceId: myAssignment?.resourceId,
myReviewStatus: myAssignment?.status ?? undefined,
phaseName: matchedReview?.phaseName ?? undefined,
result,
reviewId: matchedReview?.id,
reviewPhaseId: resolveReviewPhaseId(matchedReview),
Expand Down
5 changes: 5 additions & 0 deletions src/apps/review/src/lib/models/Screening.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ export interface Screening {
* Submission type (e.g. CONTEST_SUBMISSION, CHECKPOINT_SUBMISSION).
*/
type?: string
/**
* The phase name of the associated review (e.g., 'Screening', 'Review').
* Used for defensive filtering to ensure phase data isolation.
*/
phaseName?: string
}

/**
Expand Down
13 changes: 9 additions & 4 deletions src/apps/review/src/lib/utils/reviewMatching.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,14 @@ function resolvePhaseOrTypeMatch({
return true
}

debugLog('reviewMatchesPhase.phaseNameFallback', {
debugLog('reviewMatchesPhase.phaseNameMismatchRejected', {
normalizedPhaseName,
normalizedReviewPhaseName,
reviewId: review.id,
reviewPhaseName: truncateForLog(reviewPhaseName ?? ''),
})

return false
}

if (normalizedReviewTypeAlpha && reviewType && normalizedPhaseNameForReviewType) {
Expand Down Expand Up @@ -351,14 +353,15 @@ const doesReviewMatchScorecard = (
}

/**
* Determines whether a review matches the supplied phase context by evaluating scorecard,
* phase identifier, phase name, type, and metadata criteria in priority order.
* Determines whether a review matches the supplied phase context. Reviews that include an explicit
* phase name must match the target phase name exactly; otherwise scorecard, phase identifier, type,
* and metadata criteria are evaluated in order.
*
* @param review - Review to evaluate.
* @param scorecardId - Scorecard identifier associated with the target phase.
* @param phaseIds - Set of phase identifiers collected for the target phase.
* @param phaseName - Optional phase name for matching when available.
* @returns True when the review satisfies any of the matching criteria.
* @returns True when the review satisfies the matching criteria with phase names taking precedence.
*/
export function reviewMatchesPhase(
review: BackendReview | undefined,
Expand Down Expand Up @@ -428,6 +431,8 @@ export function reviewMatchesPhase(
normalizedReviewPhaseName,
reviewId: review.id,
})

return false
}

const reviewTypeId = getNormalizedLowerCase(review.typeId ?? undefined)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1045,13 +1045,20 @@ export const ChallengeDetailsPage: FC<Props> = (props: Props) => {
}),
[myResources],
)
const hasManagerRole = useMemo(
() => (myResources ?? [])
.some(resource => (resource.roleName ?? '')
.trim()
.toLowerCase() === 'manager'),
[myResources],
)
const isAdmin = useMemo(
() => loginUserInfo?.roles?.some(
role => typeof role === 'string' && role.toLowerCase() === 'administrator',
) ?? false,
[loginUserInfo?.roles],
)
const shouldShowResourcesSection = hasCopilotRole || isAdmin
const shouldShowResourcesSection = hasCopilotRole || hasManagerRole || isAdmin

Choose a reason for hiding this comment

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

[⚠️ correctness]
The addition of hasManagerRole to shouldShowResourcesSection seems to align with the PR's intent to adjust visibility based on roles. Ensure that this change is consistent with the business logic and requirements for role-based access.

const canManagePhases = useMemo(
() => !isPastReviewDetail && (hasCopilotRole || isAdmin),
[hasCopilotRole, isAdmin, isPastReviewDetail],
Expand Down