Fix for screening button not showing #1275
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| .pendingScore { | ||
| color: var(--GrayFontColor); |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a CSS variable for $blue-120 to maintain consistency with the usage of var(--GrayFontColor). This can improve maintainability by centralizing color definitions.
| const hasNumericScore = data.score && data.score !== 'Pending' | ||
| const reviewId = data.reviewId | ||
| const canViewScorecard = hasNumericScore && reviewId | ||
| const scoreLabel = data.score ?? 'Pending' |
There was a problem hiding this comment.
[correctness]
The scoreLabel is set to 'Pending' when data.score is null or undefined. Consider checking if data.score is explicitly 'Pending' before setting the scoreLabel to 'Pending' to avoid potential confusion between a null score and an actual 'Pending' status.
| const hasNumericScore = data.score && data.score !== 'Pending' | ||
| const reviewId = data.reviewId | ||
| const canViewScorecard = hasNumericScore && reviewId | ||
| const scoreLabel = data.score ?? 'Pending' |
There was a problem hiding this comment.
[correctness]
The scoreLabel is set to 'Pending' when data.score is null or undefined. Consider checking if data.score is explicitly 'Pending' before setting the scoreLabel to 'Pending' to avoid potential confusion between a null score and an actual 'Pending' status.
| pushReview(submissionReviewMap.get(submission.id)) | ||
| } | ||
|
|
||
| if (Array.isArray(globalReviews) && globalReviews.length) { |
There was a problem hiding this comment.
[💡 maintainability]
The check Array.isArray(globalReviews) && globalReviews.length is repeated multiple times in the code. Consider extracting this logic into a helper function to improve maintainability and reduce duplication.
| submission: BackendSubmission, | ||
| ): BackendReview | undefined => { | ||
| if (!reviews.length) { | ||
| return findFallbackReview(submission, phaseLabel, scorecardId, phaseIds) |
There was a problem hiding this comment.
[correctness]
In selectBestReview, the fallback logic is triggered when reviews.length is zero. Ensure that the fallback review logic correctly handles cases where submission.review might not contain relevant reviews, as this could lead to unexpected results.
| reviewerResourceId, | ||
| }: ReviewerDisplayArgs): BackendResource => { | ||
| if (reviewerResourceId) { | ||
| const resourceMatch = (resources ?? []).find(resource => resource.id === reviewerResourceId) |
There was a problem hiding this comment.
[💡 maintainability]
In resolveCheckpointReviewerDisplay, the logic to find a resource match is repeated. Consider refactoring this logic into a separate function to improve code clarity and reduce redundancy.
| if (!review) { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 performance]
The function matchesMyAssignment uses a nested function normalizeResourceId. Consider moving normalizeResourceId outside of matchesMyAssignment if it is used elsewhere, to avoid redefining it multiple times.
| if (fallbackCheckpointReviewer) { | ||
| return fallbackCheckpointReviewer | ||
| } | ||
| const reviewsToRender: Array<BackendReview | undefined> = candidateReviews.length |
There was a problem hiding this comment.
[correctness]
The variable reviewsToRender is assigned an array with a potential undefined value. Ensure that downstream functions handle undefined values correctly to avoid runtime errors.
| const matchedRole = ['Submitter', 'Copilot', 'Manager', 'Admin', 'Reviewer'].find( | ||
| item => normalizedRoles.some(role => role.includes(item.toLowerCase())), | ||
| ) as ChallengeRole | undefined | ||
| const rolePriority: ChallengeRole[] = [ |
There was a problem hiding this comment.
[❗❗ correctness]
The change in role priority order may affect the logic of role matching. Ensure that this new order aligns with the intended business logic, as it prioritizes 'Admin' over 'Submitter', which could lead to different outcomes in role determination.
|
|
||
| if (normalizedReviewPhaseName) { | ||
| return enforceExactPhaseNameMatch({ | ||
| const matches = enforceExactPhaseNameMatch({ |
There was a problem hiding this comment.
[💡 readability]
The variable matches is only used once and immediately returned. Consider returning the result of enforceExactPhaseNameMatch directly to simplify the code.
| ? findMetadataPhaseMatch( | ||
| review.metadata, | ||
| if (phaseBasedMatch === false && normalizedPhaseName) { | ||
| debugLog('reviewMatchesPhase.phaseBasedMatchFailed', { |
There was a problem hiding this comment.
[correctness]
The debug log message 'reviewMatchesPhase.phaseBasedMatchFailed' is only logged when phaseBasedMatch is false and normalizedPhaseName is truthy. Ensure this condition is intentional and covers all necessary cases.
| ].filter(key => key in value) as Array<keyof typeof value> | ||
|
|
||
| for (const key of candidateKeys) { | ||
| const detected = detectReviewPhaseType(value[key]) |
There was a problem hiding this comment.
[correctness]
The function detectReviewPhaseType is called with value[key] without checking if value[key] is defined. This could lead to unexpected behavior if value[key] is undefined. Consider adding a check before calling the function.
| const normalizedScorecardId = reviewInfo?.scorecardId ? `${reviewInfo.scorecardId}` : undefined | ||
|
|
||
| return detectReviewTypeFromReviewerConfig( | ||
| const metadataDerived = detectReviewTypeFromMetadata(reviewInfo?.metadata) |
There was a problem hiding this comment.
[💡 correctness]
The variable metadataDerived is assigned the result of detectReviewTypeFromMetadata, which may return undefined. Ensure that this behavior is intended and that the subsequent logic correctly handles undefined values.
|
|
||
| return detectReviewTypeFromReviewerConfig( | ||
| const metadataDerived = detectReviewTypeFromMetadata(reviewInfo?.metadata) | ||
| const phaseDerived = detectReviewTypeFromPhases( |
There was a problem hiding this comment.
[correctness]
The function detectReviewTypeFromPhases is called with challengeInfo?.phases cast to ChallengePhaseSummary[]. Ensure that challengeInfo?.phases is indeed of this type to avoid runtime errors.
| const metadataDerived = detectReviewTypeFromMetadata(reviewInfo?.metadata) | ||
| const phaseDerived = detectReviewTypeFromPhases( | ||
| challengeInfo?.phases as ChallengePhaseSummary[], | ||
| reviewInfo?.phaseId, |
There was a problem hiding this comment.
[correctness]
The function detectReviewTypeFromPhases is called with reviewInfo?.phaseId without checking if reviewInfo is defined. This could lead to unexpected behavior if reviewInfo is undefined. Consider adding a check before calling the function.
| reviewInfo?.phaseId, | ||
| ) | ||
| const reviewerDerived = detectReviewTypeFromReviewerConfig( | ||
| reviewerConfigs as ReviewerConfig[], |
There was a problem hiding this comment.
[correctness]
The function detectReviewTypeFromReviewerConfig is called with reviewerConfigs cast to ReviewerConfig[]. Ensure that reviewerConfigs is indeed of this type to avoid runtime errors.
| reviewInfo?.phaseId, | ||
| ) | ||
| || detectReviewPhaseType(scorecardInfo?.name) | ||
| const scorecardDerived = detectReviewPhaseType(scorecardInfo?.name) |
There was a problem hiding this comment.
[correctness]
The function detectReviewPhaseType is called with scorecardInfo?.name without checking if scorecardInfo is defined. This could lead to unexpected behavior if scorecardInfo is undefined. Consider adding a check before calling the function.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2607
What's in this PR?
Fixes for when we display the scorecards to users of mixed resource roles