PM-3698 bulk update engagement payments #1461
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| toast.success('Starting bulk approve', { position: toast.POSITION.BOTTOM_RIGHT }) | ||
|
|
||
| for (const id of ids) { |
There was a problem hiding this comment.
[maintainability]
Using any for the updates type can lead to runtime errors and makes the code harder to maintain. Consider defining a specific type for updates to ensure type safety.
| // 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 | ||
| const msg = await editPayment(updates) |
There was a problem hiding this comment.
[performance]
Using await inside a loop can lead to performance issues as each iteration waits for the previous one to complete. Consider using Promise.all to handle these promises concurrently if order is not important.
| const msg = await editPayment(updates) | ||
| toast.success(msg, { position: toast.POSITION.BOTTOM_RIGHT }) | ||
| } catch (err:any) { | ||
| if (err?.message) { |
There was a problem hiding this comment.
[💡 maintainability]
Catching errors with any type can obscure the actual error structure. Consider using a more specific error type or checking for known properties to improve error handling.
| onConfirm={function onConfirm() { | ||
| onBulkApprove(bulkAuditNote) | ||
| }} | ||
| canSave={bulkAuditNote.trim().length > 0} |
There was a problem hiding this comment.
[💡 correctness]
The canSave condition relies on bulkAuditNote.trim().length > 0, which might not be sufficient for validating the input. Consider adding more validation logic if necessary.
| size='lg' | ||
| /> | ||
| )} | ||
| {props.selectedCount && props.selectedCount > 0 && ( |
There was a problem hiding this comment.
[💡 readability]
The condition props.selectedCount && props.selectedCount > 0 can be simplified to props.selectedCount > 0 since selectedCount being a number will inherently evaluate to false if it is 0 or undefined.
| } | ||
| }, [props.selectedPayments]) | ||
|
|
||
| // Only rows with this status are selectable for bulk actions |
There was a problem hiding this comment.
[maintainability]
The selectableStatus variable is hardcoded as 'On Hold (Admin)'. Consider making this configurable or documenting its significance to ensure maintainability if the status values change in the future.
| const someVisibleSelected = visibleSelectablePayments.some(p => selectedPayments[p.id]) && !allVisibleSelected | ||
|
|
||
| const onToggleSelectAll = (checked: boolean) => { | ||
| if (checked) { |
There was a problem hiding this comment.
[💡 style]
In onToggleSelectAll, the next object is created and populated using forEach. Consider using reduce to directly construct the object, which can be more concise and functional.
| type='checkbox' | ||
| aria-label='Select All' | ||
| checked={allVisibleSelected} | ||
| ref={el => { if (el) el.indeterminate = someVisibleSelected }} |
There was a problem hiding this comment.
[design]
Setting the indeterminate property directly on the DOM element in the ref callback is a side effect that could be better handled in a useEffect hook to ensure consistency and avoid unexpected behavior.
2 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3698 - rontend Implementation for Engagement Payment Approver & Bulk Approval
What's in this PR?