Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| try { | ||
| amount = parseFloat(amountStr) | ||
| } catch (error) { | ||
| return amountStr |
There was a problem hiding this comment.
[correctness]
Returning amountStr in the catch block of formatCurrency may lead to inconsistent data types being returned (string instead of formatted currency). Consider returning a default formatted value or handling this case differently.
|
|
||
| if (status === 'ON_HOLD') { | ||
| if (!payment.paymentStatus?.payoutSetupComplete) { | ||
| status = 'On Hold (Payment Provider)' |
There was a problem hiding this comment.
[💡 readability]
The logic for determining the status when status === 'ON_HOLD' is complex and could be refactored for clarity. Consider extracting this logic into a separate function to improve readability.
| // awaiting sequentially to preserve order and server load control | ||
| // errors for individual items are caught and reported | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await editPayment(updates) |
There was a problem hiding this comment.
[performance]
Using await inside a for...of loop can lead to sequential execution, which might be inefficient if the operations can be performed concurrently. Consider using Promise.all to handle multiple promises concurrently if order is not important.
| if (updateObj.grossAmount !== undefined) updates.paymentAmount = updateObj.grossAmount | ||
| } | ||
|
|
||
| toast.success('Updating payment', { position: toast.POSITION.BOTTOM_RIGHT }) |
There was a problem hiding this comment.
[correctness]
Displaying a success toast before the actual update operation might mislead users if the update fails. Consider moving the success toast to after the editPayment call succeeds.
| try { | ||
| const updateMessage = await editPayment(updates) | ||
| toast.success(updateMessage, { position: toast.POSITION.BOTTOM_RIGHT }) | ||
| } catch (err:any) { |
There was a problem hiding this comment.
[💡 maintainability]
The error handling in updatePayment could be improved by providing more context about the error. Consider logging the error object or providing additional details to help with debugging.
| setPagination(response.pagination) | ||
| } catch (apiError) { | ||
| console.error('Failed to fetch points:', apiError) | ||
| if (apiError instanceof AxiosError && apiError?.response?.status === 403) { |
There was a problem hiding this comment.
[correctness]
Consider handling other HTTP status codes that might be relevant, such as 404 for not found or 500 for server errors, to provide more specific error messages to the user.
| try { | ||
| const updateMessage = await editPayment(updates) | ||
| toast.success(updateMessage, { position: toast.POSITION.BOTTOM_RIGHT }) | ||
| } catch (err:any) { |
There was a problem hiding this comment.
[correctness]
The catch block should handle specific error types. Consider checking if err is an instance of AxiosError and handle accordingly. This will ensure that you are not relying on the presence of a message property which might not exist for all error types.
| title: 'Edit Points', | ||
| }) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [handleValueUpdated, editState, fetchPoints]) |
There was a problem hiding this comment.
[❗❗ correctness]
The dependency array for useCallback should include all variables used within the callback to ensure the function is updated when its dependencies change. Consider adding editState and fetchPoints to the dependency array.
| 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 is called multiple times. Consider memoizing this function using useMemo to avoid unnecessary recalculations.
| /* 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 can lead to functions that are difficult to understand and maintain. Consider refactoring complex functions instead of disabling this 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.
[💡 readability]
The conditional logic for formatting grossAmountNumber could be extracted into a separate function to improve readability and maintainability.
|
|
||
| th { | ||
| top: 0; | ||
| background-color: white !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make it difficult to override styles later and should be avoided unless absolutely necessary. Consider refactoring to avoid its use.
| .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 future compatibility.
| @@ -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 every render. Consider using class methods or memoized callbacks 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 reduces type safety and can make the code harder to understand and maintain. Consider specifying return types explicitly.
| </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 points to avoid potential issues with React's reconciliation process.
| {point.description} | ||
| </div> | ||
| </td> | ||
| <td className='body-small'>{point.createDate}</td> |
There was a problem hiding this comment.
[💡 readability]
Consider formatting point.createDate to a more human-readable format if it's not already. This can improve the user experience.
| </div> | ||
| </td> | ||
| <td className='body-small'>{point.createDate}</td> | ||
| <td className='body-small'>{point.grossAmountNumber}</td> |
There was a problem hiding this comment.
[correctness]
Ensure that point.grossAmountNumber is formatted correctly for display, especially if it represents a currency or large number.
| /> | ||
| {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 for better 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.
[correctness]
The default value for type is set to WinningsType.PAYMENT. Ensure that this default aligns with the intended behavior of the function, as it might lead to unexpected results if a different type is expected by default.
| // 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]
Consider validating the type parameter to ensure it is a valid WinningsType. This can prevent potential issues if an invalid type is passed to the function.
1 participant
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3262
What's in this PR?