Conversation
PM-4070 Display profile completeness to Admin and TMs too
PM-4067 Redesigned profile skills UI
PM-3921 Rephrase open to work subtext
PM-3837 Fix task payments on review UI
PM-3838 Set the redirect target before awaiting
…flows PM-4127 re-trigger ai workflow run
…iggers ai workflow
…details-for-run PM-4146 #time 30m automatically re-fetch run details
PM-4070 Change completeness text for admin
PM-3957 Add payment setup guide
PM-4078 Remove submitter's email from Submission tab in review UI
…files-report PM-4158 - add completed profiles report page in customer portal
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| authRequired: true, | ||
| element: <Rewrite to={talentSearchRouteId} />, | ||
| element: <Rewrite to={profileCompletionRouteId} />, |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the default route from talentSearchRouteId to profileCompletionRouteId may have unintended consequences if the profile completion process is not intended to be the default entry point. Ensure this change aligns with the intended user flow and business requirements.
| export function getTabsConfig(userRoles: string[], isAnonymous: boolean, isUnprivilegedUser: boolean): TabsNavItem[] { | ||
|
|
||
| const tabs: TabsNavItem[] = [ | ||
| ...(!isUnprivilegedUser ? [{ |
There was a problem hiding this comment.
[security]
The addition of profileCompletionRouteId to the tabs configuration should be verified to ensure that this route is intended to be accessible to all non-unprivileged users. Consider any potential security implications if this route should be restricted to certain user roles.
| @@ -0,0 +1,189 @@ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
There was a problem hiding this comment.
[performance]
Disabling react/jsx-no-bind can lead to performance issues as it allows inline functions in JSX, which can cause unnecessary re-renders. Consider using useCallback to memoize the onChange handler for InputSelect.
| userId?: number | string | ||
| } | ||
|
|
||
| function normalizeToList(raw: any): any[] { |
There was a problem hiding this comment.
[maintainability]
The normalizeToList function uses any type for its parameter and return type, which can lead to potential type safety issues. Consider defining a more specific type for the input and output to improve type safety.
|
|
||
| async function fetchCompletedProfiles(): Promise<CompletedProfile[]> { | ||
| const response = await xhrGetAsync<CompletedProfile[]>( | ||
| `${EnvironmentConfig.REPORTS_API}/topcoder/completed-profiles`, |
There was a problem hiding this comment.
[❗❗ security]
The URL for fetching completed profiles is constructed using string interpolation. Ensure that EnvironmentConfig.REPORTS_API is properly validated and sanitized to prevent potential security vulnerabilities such as injection attacks.
| </thead> | ||
| <tbody> | ||
| {displayedRows.map(profile => ( | ||
| <tr key={profile.userId || profile.handle}> |
There was a problem hiding this comment.
[❗❗ correctness]
Using profile.userId || profile.handle as a key can lead to potential key collisions if userId is undefined or not unique. Ensure that userId is always defined and unique, or use a combination of fields that guarantees uniqueness.
| @@ -0,0 +1 @@ | |||
| export * from './ProfileCompletionPage' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports and make it harder to track what is being exported from the module. Consider explicitly exporting only the necessary components or functions to improve maintainability and clarity.
| authRequired: true, | ||
| element: <ProfileCompletionPage />, | ||
| id: 'profile-completion-page', | ||
| route: '', |
There was a problem hiding this comment.
[correctness]
The route property is set to an empty string. Ensure this is intentional and that the routing logic correctly handles this case. If not, consider specifying a valid route path.
| export const customerPortalProfileCompletionRoutes = [ | ||
| { | ||
| children: [...profileCompletionChildRoutes], | ||
| element: getRoutesContainer(profileCompletionChildRoutes), |
There was a problem hiding this comment.
[correctness]
The element property is using getRoutesContainer(profileCompletionChildRoutes). Verify that getRoutesContainer correctly handles the profileCompletionChildRoutes array and returns a valid React element. If getRoutesContainer is expected to return a component, ensure it is properly tested.
| iconToLeft | ||
| disabled={!!shouldNavigateTo.current} | ||
| onClick={async function onClick() { | ||
| const url |
There was a problem hiding this comment.
[💡 readability]
The assignment of url could be simplified by using template literals directly without breaking the line. This would improve readability.
|
|
||
| const canDownload: boolean = canDownloadProfile(props.authProfile, props.profile) | ||
|
|
||
| const isAdminOrTM = props.authProfile?.roles?.includes(UserRole.administrator) |
There was a problem hiding this comment.
[maintainability]
Consider using a utility function to determine if a user has a specific role. This can improve maintainability and reduce duplication if role checks are needed elsewhere in the codebase.
|
|
||
| const isSelf = props.authProfile?.handle === props.profile.handle | ||
|
|
||
| const canSeeProfileCompleteness = isSelf || isAdminOrTM |
There was a problem hiding this comment.
[correctness]
The variable canSeeProfileCompleteness is derived from isSelf and isAdminOrTM. Ensure that the logic for determining these variables is correct and consistent with the application's authorization requirements.
| const isCustomer = props.authProfile.roles.some(r => r.indexOf(' Customer') > -1) | ||
|
|
||
| const hideCompletenessMeter = isLoading || isCompleted || isCustomer | ||
| const hideCompletenessMeter = isLoading || isCompleted |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the isCustomer check from hideCompletenessMeter may change the behavior for customer roles. Ensure this change is intentional and that customers should now see the completeness meter.
| </small> | ||
| </div> | ||
| )} | ||
| {props.isAdminOrTM && !props.isSelf |
There was a problem hiding this comment.
[design]
The logic for displaying the message to admins or team members (isAdminOrTM) when !isSelf is correct, but consider if there are any other roles that should also see this message. Ensure that the role logic aligns with business requirements.
|
|
||
| const handleRerun = useCallback(async (): Promise<void> => { | ||
| const runId = props.run?.id | ||
| if (!runId || runId === '-1') { |
There was a problem hiding this comment.
[maintainability]
The check for runId === '-1' seems to be a magic value. Consider defining a constant for this value to improve maintainability and readability.
| const score: number | undefined = props.showScore ? (props.score ?? props.run?.score) : undefined | ||
|
|
||
| const Wrapper: FC<PropsWithChildren> = useCallback(({ children }: PropsWithChildren) => { | ||
| if (!isAdmin || displayStatus === 'pending' || !props.run?.id || props.run?.id === '-1') { |
There was a problem hiding this comment.
[maintainability]
The condition props.run?.id === '-1' is repeated here. Consider extracting this logic into a helper function or constant to avoid duplication and improve maintainability.
| await mutate(getAiWorkflowRunsCacheKey(props.submissionId)) | ||
| } else { | ||
| await mutate( | ||
| (key: unknown) => typeof key === 'string' && key.includes('/workflows/runs?submissionId='), |
There was a problem hiding this comment.
[correctness]
The mutate function uses a dynamic key filter which could potentially lead to unexpected cache updates if the key pattern changes. Ensure that this pattern is well-documented and tested.
| .toUpperCase()) | ||
|
|
||
| // Determine which prize set types we should look for | ||
| const desiredPrizeSetTypes = normalizedRoles.map(role => ROLE_TO_PRIZESET_TYPE[role] ?? role) |
There was a problem hiding this comment.
[correctness]
The use of ROLE_TO_PRIZESET_TYPE[role] ?? role implies that if a role is not found in ROLE_TO_PRIZESET_TYPE, the role itself is used as the prize set type. This could lead to unexpected behavior if roles are not properly validated or if there are typos. Consider explicitly handling unknown roles to avoid potential issues.
| }, [fetchError]) | ||
|
|
||
| const uniqueRuns = uniqBy(orderBy(runs, 'completedAt', 'desc'), 'workflow.id') | ||
| const uniqueRuns = uniqBy(orderBy(runs, ['startedAt', 'completedAt'], ['desc', 'desc']), 'workflow.id') |
There was a problem hiding this comment.
[correctness]
The orderBy function now sorts by both startedAt and completedAt in descending order. Ensure that this change in sorting logic aligns with the intended behavior, as it may affect the order of uniqueRuns.
| } | ||
|
|
||
| export const retriggerAiWorkflowRun = async (workflowRunId: string): Promise<AiWorkflowRun> => ( | ||
| xhrPostAsync<RetriggerAiWorkflowRunRequest, AiWorkflowRun>( |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the retriggerAiWorkflowRun function to manage potential failures from the xhrPostAsync call, ensuring robustness and providing feedback to the caller.
| case 'RETURNED': | ||
| return 'Returned' | ||
| default: | ||
| return status.replaceAll('_', ' ') |
There was a problem hiding this comment.
[maintainability]
The replaceAll method is not supported in all environments. Consider using replace with a global regex for broader compatibility.
| } | ||
| } | ||
|
|
||
| const formatCurrency = (amountStr: string, currency: string): string => { |
There was a problem hiding this comment.
[correctness]
The formatCurrency function does not handle invalid currency codes gracefully. Consider adding validation for the currency parameter to prevent potential runtime errors.
| })) | ||
| }, []) | ||
|
|
||
| const convertToWinnings = useCallback( |
There was a problem hiding this comment.
[maintainability]
The convertToWinnings function uses replaceAll, which is not supported in all environments. Consider using replace with a global regex for broader compatibility.
| setIsLoading(false) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [convertToWinnings, filters, pagination.currentPage, pagination.pageSize]) |
There was a problem hiding this comment.
[correctness]
The fetchWinnings function does not handle pagination changes correctly if the total number of pages changes. Consider resetting the current page to 1 when filters change to avoid potential out-of-bounds errors.
| return confirmFlow?.content | ||
| }, [confirmFlow]) | ||
|
|
||
| // eslint-disable-next-line complexity |
There was a problem hiding this comment.
[💡 readability]
The updatePayment function uses a complex conditional structure to determine paymentStatus. Consider refactoring this logic into a separate function for improved readability and maintainability.
| const [bulkOpen, setBulkOpen] = React.useState(false) | ||
| const [bulkAuditNote, setBulkAuditNote] = React.useState('') | ||
|
|
||
| const onBulkApprove = useCallback(async (auditNote: string) => { |
There was a problem hiding this comment.
[performance]
The onBulkApprove function performs sequential API calls, which can be slow. Consider using Promise.all for concurrent execution if order is not important and server load can handle it.
| setSelectedPayments({}) | ||
| await fetchWinnings() | ||
| }, [selectedPayments, fetchWinnings]) | ||
| const [paymentsCollapsed, setPaymentsCollapsed] = useState(false) |
There was a problem hiding this comment.
[maintainability]
Consider using useReducer instead of multiple useState hooks for managing related state variables like paymentsCollapsed and pointsCollapsed. This can improve maintainability by centralizing state logic.
| @@ -0,0 +1,414 @@ | |||
| /* eslint-disable max-len */ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
There was a problem hiding this comment.
[performance]
Disabling react/jsx-no-bind can lead to performance issues due to unnecessary re-renders. Consider using useCallback or moving the function definitions outside the render method.
| }) | ||
|
|
||
| const convertToPoints = useCallback( | ||
| async (pointsData: WinningDetail[], handleMap: Map<number, string>): Promise<ReadonlyArray<Winning>> => pointsData.map(point => { |
There was a problem hiding this comment.
[💡 correctness]
The convertToPoints function is defined as an async function but does not use await inside. Consider removing async if no asynchronous operations are performed.
| setPointsWinnings(pointsData) | ||
| setPagination(response.pagination) | ||
| } catch (apiError) { | ||
| console.error('Failed to fetch points:', apiError) |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more specific error message or logging mechanism to capture additional details about the apiError for better debugging.
| title: 'Edit Points', | ||
| }) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [handleValueUpdated, editState, fetchPoints]) |
There was a problem hiding this comment.
[maintainability]
Avoid disabling eslint rules unless absolutely necessary. Consider refactoring the dependencies of useCallback to include all necessary variables.
| props.profile.roles.includes('Payment Admin') | ||
| || props.profile.roles.includes('Payment BA Admin') | ||
| || props.profile.roles.includes('Payment Editor') | ||
| ) |
There was a problem hiding this comment.
[💡 performance]
The isEditingAllowed function could be optimized by using a Set for roles to improve lookup performance, especially if the list of roles grows.
| interface PaymentEditFormProps { | ||
| payment: Winning | ||
| canSave?: (canSave: boolean) => void | ||
| isPointsEdit?: boolean |
There was a problem hiding this comment.
[💡 maintainability]
Consider documenting the purpose and implications of the isPointsEdit prop in the component's documentation or prop types. This will help future developers understand its role and how it affects the component's behavior.
| <InputText | ||
| name='grossPayment' | ||
| label='Gross Payment' | ||
| label={props.isPointsEdit ? 'Points amount' : 'Gross Payment'} |
There was a problem hiding this comment.
[correctness]
Ensure that the grossAmount value is correctly formatted as a number when isPointsEdit is true, as the label and placeholder suggest a different context (points vs. payment). Verify that the grossAmount validation logic still applies correctly in this context.
| }} | ||
| /> | ||
| {props.payment.status.toUpperCase() !== 'PAID' && ( | ||
| {!props.isPointsEdit && ( |
There was a problem hiding this comment.
[design]
The conditional rendering of InputSelect based on isPointsEdit could lead to unexpected behavior if the paymentStatus is required in some contexts. Ensure that this condition aligns with the business logic and does not inadvertently hide necessary fields.
| /* eslint-disable unicorn/no-null */ | ||
| /* eslint-disable max-len */ | ||
| /* eslint-disable react/jsx-no-bind */ | ||
| /* eslint-disable complexity */ |
There was a problem hiding this comment.
[maintainability]
Disabling the complexity rule may hide potential issues with overly complex functions. Consider refactoring the code to reduce complexity instead of disabling the rule.
| <span className={styles.label}>{props.isPoints ? 'Points' : 'Payment'}</span> | ||
| <p className={styles.value}> | ||
| {props.payment.grossAmountNumber.toLocaleString(undefined, { | ||
| {props.isPoints ? `${props.payment.grossAmountNumber}` : props.payment.grossAmountNumber.toLocaleString(undefined, { |
There was a problem hiding this comment.
[💡 correctness]
The use of toLocaleString for formatting currency is correct, but when isPoints is true, the number is converted to a string without any formatting. Ensure that this behavior is intentional and that no formatting is required for points.
|
|
||
| th { | ||
| top: 0; | ||
| background-color: white !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make future maintenance difficult as it overrides all other styles. Consider refactoring to avoid using !important if possible.
| .descriptionCell { | ||
| max-width: 300px; | ||
| white-space: normal; | ||
| word-wrap: break-word; |
There was a problem hiding this comment.
[maintainability]
The word-wrap property is deprecated. Consider using overflow-wrap: break-word; for better compatibility and future-proofing.
| @@ -0,0 +1,121 @@ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
There was a problem hiding this comment.
[performance]
Disabling react/jsx-no-bind can lead to performance issues due to the creation of new function instances on each render. Consider using class methods or memoized functions instead.
| @@ -0,0 +1,121 @@ | |||
| /* eslint-disable react/jsx-no-bind */ | |||
| /* eslint-disable @typescript-eslint/explicit-function-return-type */ | |||
There was a problem hiding this comment.
[💡 maintainability]
Disabling @typescript-eslint/explicit-function-return-type can reduce type safety and make the code harder to understand. Consider specifying return types for better maintainability and clarity.
| </thead> | ||
| <tbody> | ||
| {props.points.map(point => ( | ||
| <tr key={point.id}> |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that point.id is unique across all Winning items to avoid potential issues with React's reconciliation process.
| /> | ||
| {props.currentPage > 3 && <span>...</span>} | ||
| <div className={styles.pageNumbers}> | ||
| {Array.from(Array(props.numPages) |
There was a problem hiding this comment.
[maintainability]
The pagination logic could be extracted into a separate function or component to improve readability and maintainability.
| export async function exportSearchResults(filters: Record<string, string[]>): Promise<Blob> { | ||
| export async function exportSearchResults( | ||
| filters: Record<string, string[]>, | ||
| type: WinningsType = WinningsType.PAYMENT, |
There was a problem hiding this comment.
[💡 design]
The type parameter in exportSearchResults defaults to WinningsType.PAYMENT, which is fine, but ensure that this default aligns with the expected behavior of the function. If the function is called without specifying a type, it will always export payments. Verify if this is the intended default behavior.
| // eslint-disable-next-line max-len | ||
| export async function getPayments(limit: number, offset: number, filters: Record<string, string[]>): Promise<{ | ||
| export async function fetchWinnings( | ||
| type: WinningsType, |
There was a problem hiding this comment.
[correctness]
The fetchWinnings function now requires a type parameter, which is passed directly from getPayments and getPoints. Ensure that all calls to fetchWinnings correctly specify the type to avoid unexpected behavior.
|
|
||
| if (response.status === 'error') { | ||
| throw new Error('Error fetching payments') | ||
| throw new Error('Error fetching winnings') |
There was a problem hiding this comment.
[💡 maintainability]
The error message in fetchWinnings was changed from 'Error fetching payments' to 'Error fetching winnings'. Ensure that this change is reflected in any documentation or logging that relies on specific error messages.
| icon={IconOutline.ExternalLinkIcon} | ||
| target='_blank' | ||
| rel='noopener noreferrer' | ||
| to={`${EnvironmentConfig.TOPCODER_URL}/thrive/articles/payment-policies-and-instructions`} |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more descriptive constant name instead of TOPCODER_URL to clarify the purpose of this URL, such as TOPCODER_ARTICLES_URL, to improve maintainability.
| export * from './default.env' | ||
|
|
||
| export const TC_FINANCE_API = 'http://localhost:3009/v6/finance' | ||
| export const REPORTS_API = 'http://localhost:3013/v6/reports' |
There was a problem hiding this comment.
[security]
The REPORTS_API is using an HTTP protocol. Consider using HTTPS to ensure secure communication, especially if this configuration might be used in environments beyond local development.
| {categories.map((categoryName: string) => { | ||
| const skills = props.groupedSkillsByCategory[categoryName] ?? [] | ||
|
|
||
| const sortedSkills = [...skills].sort( |
There was a problem hiding this comment.
[💡 correctness]
Sorting skills by name using localeCompare is generally correct, but consider specifying a locale explicitly for consistency across different environments.
| const total = sortedSkills.length | ||
| const extraCount = Math.max(0, total - DEFAULT_VISIBLE) | ||
|
|
||
| const visibleSkills = isExpanded ? sortedSkills : sortedSkills.slice(0, DEFAULT_VISIBLE) |
There was a problem hiding this comment.
[💡 readability]
The visibleSkills logic is correct, but consider using a more descriptive variable name like displayedSkills to clarify its purpose.
| type='button' | ||
| className={styles.moreLessButton} | ||
| data-category={categoryName} | ||
| onClick={toggleCategory} |
There was a problem hiding this comment.
[security]
Using data-category to store the category name is effective, but ensure that the category names do not contain characters that could cause issues in HTML attributes.
| <div className={styles.title}>{props.header}</div> | ||
| <div className={styles.badgeCount}> | ||
| {props.children && props.children.length} | ||
| {props.children && props.count} |
There was a problem hiding this comment.
[correctness]
Using props.count directly without validating it could lead to incorrect badge counts if count is not provided or is undefined. Consider providing a default value or a fallback to ensure the badge count is always a valid number.
…omer-portal PM-4170 - remove PM from authorized user roles in Customer portal
| rolesRequired: [ | ||
| UserRole.administrator, | ||
| UserRole.projectManager, | ||
| UserRole.talentManager, |
There was a problem hiding this comment.
[❗❗ security]
The removal of UserRole.projectManager from the rolesRequired array could potentially impact access control. Ensure that this change aligns with the intended access policy and that project managers are no longer expected to have access to this route.
3 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?