QA fixes for reviewers seeing screenings even if they aren't a screener #1278
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const baseRows = props.screening ?? [] | ||
| // Defensive filtering: exclude any entries that don't belong to Screening phase | ||
| const phaseValidatedRows = baseRows.filter(row => { | ||
| if (!row.reviewId) { |
There was a problem hiding this comment.
[❗❗ 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.
| ) | ||
|
|
||
| const hasManagerRole = useMemo( | ||
| () => (myRoles ?? []) |
There was a problem hiding this comment.
[correctness]
Consider using toLowerCase().trim() consistently across all role checks for uniformity and to prevent potential issues with leading/trailing spaces.
| ?.toLowerCase() | ||
| .trim() | ||
|
|
||
| if (normalizedPhaseName && normalizedPhaseName !== 'screening') { |
There was a problem hiding this comment.
[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.
| label: 'Screening Score', | ||
| propertyName: 'score', | ||
| renderer: (data: Screening) => { | ||
| const normalizedPhaseName = data.phaseName |
There was a problem hiding this comment.
[maintainability]
The normalizedPhaseName check is repeated in multiple places. Consider refactoring this logic into a helper function to improve maintainability and reduce code duplication.
| } | ||
|
|
||
| if (canViewAllScorecards) { | ||
| if (canViewAllScorecards || hasReviewerRole) { |
There was a problem hiding this comment.
[💡 readability]
The condition canViewAllScorecards || hasReviewerRole is used multiple times. Consider extracting this logic into a descriptive function or variable to improve readability and maintainability.
| [loginUserInfo?.roles], | ||
| ) | ||
| const shouldShowResourcesSection = hasCopilotRole || isAdmin | ||
| const shouldShowResourcesSection = hasCopilotRole || hasManagerRole || isAdmin |
There was a problem hiding this comment.
[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.
https://topcoder.atlassian.net/browse/PM-2476