[PROD] - Jan 26 r1 #1391
[PROD] - Jan 26 r1 #1391
Conversation
…ich are failed screening
…ression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Potential fix for code scanning alert no. 63: Inefficient regular expression
fix(PM-3216, PM-2573): dont show edit manager comment to reviewer and filter reviews which are failed screening
fix(PM-2666): Filtered submissions in checkpoint review tab based on screening result
…show own submissions (PM-3255)
fix(PM-3254): check for failed submissions instead of passing submission
fix(PM-2666): Reviews call doesn't triggered for submitters
Leave tracker
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,37 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[❗❗ correctness]
The import statement uses a relative path that starts with @libs, which might not resolve correctly depending on the build configuration. Ensure that the build system supports this alias or consider using a relative path.
| @import '@libs/ui/styles/includes'; | ||
|
|
||
| :root { | ||
| --LeaveColor: #4caf50; |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name for --LeaveColor to clarify its purpose, such as --primary-button-bg-color, since it is used for the .primaryButton background.
| // App-specific global styles | ||
| } | ||
|
|
||
| .primaryButton { |
There was a problem hiding this comment.
[maintainability]
The .primaryButton class uses a hardcoded color for text (white). Consider using a CSS variable for the text color to maintain consistency and flexibility in theming.
| ] | ||
|
|
||
| export const getMonthDates = (year: number, month: number): Date[] => { | ||
| const monthDate = new Date(year, month, 1) |
There was a problem hiding this comment.
[correctness]
Consider using Date.UTC(year, month, 1) to construct the monthDate to avoid potential issues with time zones that could affect date calculations.
| } | ||
|
|
||
| export const getStatusColor = (status: LeaveStatus): string => ( | ||
| statusColorMap[status] ?? statusColorMap[LeaveStatus.AVAILABLE] |
There was a problem hiding this comment.
[💡 maintainability]
The fallback for statusColorMap[status] is statusColorMap[LeaveStatus.AVAILABLE], which is correct. However, ensure that LeaveStatus.AVAILABLE is always present in statusColorMap to avoid potential issues if the map is modified in the future.
| @@ -0,0 +1 @@ | |||
| export * from './calendar.utils' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintentional exports and make it harder to track what is being exported from this module. Consider explicitly exporting only the necessary functions or variables to improve maintainability and clarity.
| font-weight: 600; | ||
| } | ||
|
|
||
| @media (max-width: 720px) { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more descriptive class name for .actions to better convey its purpose, especially since it is used in a media query. This can improve maintainability by making the code more self-explanatory.
| endOfMonth(currentDate), | ||
| ) | ||
| } catch { | ||
| setActionError('Unable to load leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context when an error occurs.
| setSelectedDates(new Set()) | ||
| await loadCurrentMonth() | ||
| } catch { | ||
| setActionError('Unable to update leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context when an error occurs.
| setSelectedDates(new Set()) | ||
| await loadCurrentMonth() | ||
| } catch { | ||
| setActionError('Unable to update leave dates. Please try again.') |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details in the catch block to aid in debugging and provide more context when an error occurs.
| .error { | ||
| margin-top: 12px; | ||
| padding: 12px 16px; | ||
| background: #fff5f5; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable for the background color #fff5f5 to improve maintainability and consistency across the application. This will make it easier to update the color scheme in the future if needed.
| margin-top: 12px; | ||
| padding: 12px 16px; | ||
| background: #fff5f5; | ||
| border: 1px solid #fda4af; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable for the border color #fda4af to improve maintainability and consistency across the application. This will make it easier to update the color scheme in the future if needed.
| background: #fff5f5; | ||
| border: 1px solid #fda4af; | ||
| border-radius: 8px; | ||
| color: #b91c1c; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable for the text color #b91c1c to improve maintainability and consistency across the application. This will make it easier to update the color scheme in the future if needed.
| startOfMonth(currentDate), | ||
| endOfMonth(currentDate), | ||
| ) | ||
| } catch { |
There was a problem hiding this comment.
[maintainability]
Catching all exceptions without logging or handling specific error types can make debugging difficult. Consider logging the error or handling specific exceptions to provide more context.
| setCurrentDate(prev => addMonths(prev, 1)) | ||
| }, []) | ||
|
|
||
| const errorMessage = useMemo(() => { |
There was a problem hiding this comment.
[💡 performance]
Using useMemo for errorMessage might be unnecessary here since the computation is simple and not expensive. Consider removing useMemo to simplify the code unless performance profiling indicates a need.
| return | ||
| } | ||
|
|
||
| const normalizedResult = (entry.result || '').toUpperCase() |
There was a problem hiding this comment.
[maintainability]
Consider using a constant or enum for the result strings 'PASS' and 'NO PASS' to avoid magic strings and potential typos.
| ], | ||
| ) | ||
|
|
||
| const filterCheckpointReviewByScreeningResult = useCallback( |
There was a problem hiding this comment.
[💡 performance]
The use of useCallback here is appropriate, but ensure that filterCheckpointReviewByScreeningResult is not being unnecessarily recreated if checkpointScreeningOutcome changes frequently, as this could impact performance.
| interface Props { | ||
| aiReviewers?: { aiWorkflowId: string }[] | ||
| selectedTab: string | ||
| screeningOutcome: { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using ReadonlySet<string> instead of Set<string> for failingSubmissionIds and passingSubmissionIds if these sets are not meant to be modified. This can help prevent accidental mutations and improve code safety.
| }) | ||
| return nf.format(n) | ||
| } catch (err) { | ||
| // Fallback for any non-ISO currency codes |
There was a problem hiding this comment.
[💡 maintainability]
Catching a generic error with catch (err) without handling or logging it could make debugging difficult if an error occurs. Consider logging the error or providing more context.
|
|
||
| const prizeCurrency = (prize as any)?.type | ||
| if ((String(prizeCurrency || '')) | ||
| .toUpperCase() !== 'POINT') { |
There was a problem hiding this comment.
[correctness]
The check for prizeCurrency being 'POINT' is case-insensitive, but the logic assumes that any non-'POINT' currency should return undefined. Consider handling other currency types or documenting why only 'POINT' is valid.
| }) | ||
| return nf.format(n) | ||
|
|
||
| const normalizedCurrency = (currency || 'USD').toUpperCase() |
There was a problem hiding this comment.
[correctness]
Consider using a more robust method to validate currency codes before using them in Intl.NumberFormat. This could prevent unexpected behavior if an invalid currency code is passed, even though there is a try-catch block.
| style: 'currency', | ||
| }) | ||
| return nf.format(n) | ||
| } catch (err) { |
There was a problem hiding this comment.
[💡 maintainability]
The catch block for handling non-ISO currency codes is a good fallback, but it might be beneficial to log the error for debugging purposes. This would help in identifying and fixing issues with currency code handling.
| <MarkdownReview value={comment} /> | ||
| </div> | ||
| {isManagerEdit && ( | ||
| {isManagerEdit && canAddManagerComment && ( |
There was a problem hiding this comment.
[correctness]
The addition of canAddManagerComment to the condition is a good step towards ensuring that the button is only shown when it is appropriate. However, ensure that canAddManagerComment is correctly initialized and updated in the context to avoid potential issues where the button might not appear when it should.
| aiReviewers?: { aiWorkflowId: string }[] | ||
| className?: string | ||
| datas: SubmissionInfo[] | ||
| screeningOutcome: { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using ReadonlySet<string> for failingSubmissionIds and passingSubmissionIds to prevent accidental modifications to these sets, as they appear to be used for filtering logic.
| ) | ||
|
|
||
| const filterScreeningPassedReviews = useCallback( | ||
| (submissions: SubmissionInfo[]): SubmissionInfo[] => submissions.filter( |
There was a problem hiding this comment.
[correctness]
The filterScreeningPassedReviews function uses submission.id ?? '', which implies that submission.id might be undefined. Ensure that submission.id is always defined or handle the case where it is undefined appropriately.
| }: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions(challengeId) | ||
| }: useFetchChallengeSubmissionsProps = useFetchChallengeSubmissions( | ||
| challengeId, | ||
| submissionViewer, |
There was a problem hiding this comment.
[performance]
Passing submissionViewer to useFetchChallengeSubmissions introduces a dependency on the loginUserInfo object. Ensure that this dependency is necessary and that useFetchChallengeSubmissions is designed to handle changes in submissionViewer correctly. If submissionViewer is not expected to change frequently, consider whether this could lead to unnecessary re-fetching of submissions.
| } | ||
|
|
||
| export interface ChallengeSubmissionsViewer { | ||
| roles?: Array<string | undefined | null> |
There was a problem hiding this comment.
[💡 maintainability]
The roles and tokenRoles arrays allow undefined and null values, which are then filtered out. Consider ensuring these arrays contain only strings at their source to avoid unnecessary filtering.
| const normalizedTokenRoles = useMemo<string[]>( | ||
| () => (viewer?.tokenRoles ?? []) | ||
| .map(role => (typeof role === 'string' ? role.toLowerCase() | ||
| .trim() : '')) |
There was a problem hiding this comment.
[correctness]
The normalizedTokenRoles logic assumes role is a string before trimming and converting to lowercase. Consider adding a check to ensure role is a string to prevent potential runtime errors.
| [normalizedRoles], | ||
| ) | ||
| const isProjectManager = useMemo<boolean>( | ||
| () => normalizedTokenRoles.some( |
There was a problem hiding this comment.
[💡 design]
The logic for determining isProjectManager checks both normalizedTokenRoles and normalizedRoles. Ensure that the roles are consistently defined across different parts of the application to avoid discrepancies.
| [normalizedRoles, normalizedTokenRoles], | ||
| ) | ||
| const canViewAllSubmissions = useMemo<boolean>( | ||
| () => (viewer ? ( |
There was a problem hiding this comment.
[❗❗ security]
The canViewAllSubmissions logic defaults to true if viewer is not provided. This could lead to unintended access if viewer is expected to be defined. Consider explicitly handling the case where viewer is undefined.
| const normalizedDeletedIds = new Set<string>() | ||
| const normalizedDeletedLegacyIds = new Set<string>() | ||
| const activeSubmissions: BackendSubmission[] = [] | ||
| const shouldRestrictToCurrentMember = Boolean( |
There was a problem hiding this comment.
[security]
The shouldRestrictToCurrentMember variable is determined by hasSubmitterRole and !canViewAllSubmissions. Ensure that the logic for these conditions aligns with the intended access control policies.
|
|
||
| const visibleSubmissions = shouldRestrictToCurrentMember | ||
| ? activeSubmissions.filter(submission => (viewerMemberId | ||
| ? `${submission?.memberId ?? ''}` === viewerMemberId |
There was a problem hiding this comment.
[💡 readability]
The comparison ${submission?.memberId ?? ''} === viewerMemberId could be simplified by ensuring viewerMemberId is always a string, avoiding the need for the template literal.
| isValidating: isValidatingChallengeReviews, | ||
| }: SWRResponse<BackendReview[], Error> = useSWR<BackendReview[], Error>( | ||
| `reviewBaseUrl/reviews/${challengeId}/${reviewerKey}`, | ||
| challengeId && (reviewerIds.length || shouldForceReviewFetch) |
There was a problem hiding this comment.
[performance]
The change from using isPaused to a conditional URL in the useSWR hook is a notable alteration. Ensure that this change does not inadvertently cause unnecessary fetches when challengeId is undefined or when there are no reviewerIds and shouldForceReviewFetch is false. This could potentially lead to performance issues or unexpected behavior.
| const normalizedRoles = [ | ||
| ...myRoles.map(role => role.toLowerCase()), | ||
| ...(isTopcoderAdmin ? ['admin'] : []), | ||
| ...(reviewerLikeResourceIds.size > 0 ? ['reviewer'] : []), |
There was a problem hiding this comment.
[correctness]
The addition of reviewerLikeResourceIds.size > 0 ? ['reviewer'] : [] to normalizedRoles changes the logic to include a 'Reviewer' role if any reviewer-like resources exist. Ensure this change aligns with the intended business logic, as it may affect role determination.
| [pmResourceIds], | ||
| ) | ||
|
|
||
| const hasReviewerRole = useMemo( |
There was a problem hiding this comment.
[correctness]
The hasReviewerRole logic now checks reviewerLikeResourceIds.size > 0, which broadens the criteria for having a reviewer role. Verify that this change is intentional and aligns with the requirements, as it could impact access control or permissions.
|
|
||
| const totalSeconds = Math.ceil(totalSecondsFloat) | ||
| const currentEndMoment = (() => { | ||
| const endSource = extendTarget.actualEndDate ?? extendTarget.scheduledEndDate |
There was a problem hiding this comment.
[performance]
The use of moment for date manipulation is generally discouraged in favor of more modern libraries like date-fns or luxon, which offer better performance and a smaller bundle size. Consider refactoring to use a more modern library if possible.
| } | ||
|
|
||
| const currentDuration = extendTarget.duration | ||
| if (typeof currentDuration === 'number' && Number.isFinite(currentDuration)) { |
There was a problem hiding this comment.
[💡 readability]
The check for typeof currentDuration === 'number' && Number.isFinite(currentDuration) is good for ensuring valid numbers, but consider adding a comment or refactoring to make the intention clearer, as this pattern might not be immediately obvious to all developers.
| { | ||
| duration: totalSeconds, | ||
| isOpen: true, | ||
| scheduledEndDate: endMoment.toISOString(), |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the duration property from the payload in handlePhaseUpdate might affect other parts of the system that rely on this data. Ensure that this change is intentional and that any dependent logic is updated accordingly.
| { prefix: '/v6/resources', target: 'http://localhost:3004' }, | ||
| { prefix: '/v6/resource-roles', target: 'http://localhost:3004' }, | ||
|
|
||
| { prefix: '/v6/leave', target: 'http://localhost:3011' }, |
There was a problem hiding this comment.
[❗❗ security]
The new service override for /v6/leave is added with a target of http://localhost:3011. Ensure that the service running on port 3011 is correctly configured and secured, especially if it handles sensitive data. This is important for maintaining security and correctness.
…isiblity-for-approvers Fix submissions visibility for approvers & checkpoint-reviewers
| resources, | ||
| }: ChallengeDetailContextModel = useContext(ChallengeDetailContext) | ||
| const { actionChallengeRole, isPrivilegedRole }: useRoleProps = useRole() | ||
| const { actionChallengeRole, isPrivilegedRole, hasApproverRole }: useRoleProps = useRole() |
There was a problem hiding this comment.
[correctness]
The addition of hasApproverRole to the destructured object from useRole() suggests that this role is now being used in the component. Ensure that useRole() is updated to correctly provide this property, and verify that its usage is consistent with the intended role logic throughout the application.
| const validReviewPhaseSubmissions = baseReviews.filter(hasReviewPhaseReview) | ||
|
|
||
| if (isPrivilegedRole || (isChallengeCompleted && hasPassedReviewThreshold)) { | ||
| if (isPrivilegedRole || hasApproverRole || (isChallengeCompleted && hasPassedReviewThreshold)) { |
There was a problem hiding this comment.
[design]
Including hasApproverRole in the conditional check broadens the access to validReviewPhaseSubmissions. Ensure that this change aligns with the business logic and that users with the Approver role are intended to have this access.
PM-3352 Validate skills during onboarding
| const hasValidSkills = (): boolean => { | ||
| const principalCount = principalSkills.length | ||
| const additionalCount = additionalSkills.length | ||
| return principalCount > 0 && additionalCount > 0 |
There was a problem hiding this comment.
[correctness]
The hasValidSkills function requires both principalSkills and additionalSkills to have at least one skill. Consider whether this logic aligns with the intended validation requirements. If either can be empty, this may need adjustment.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2611
https://topcoder.atlassian.net/browse/PM-3216
https://topcoder.atlassian.net/browse/PM-2573
https://topcoder.atlassian.net/browse/PM-2666
http://topcoder.atlassian.net/browse/PM-2664
What's in this PR?