Conversation
…fied-skills-tooltip PM-3717 - enhance the verified skill tooltip
Check if member has activity details for tooltip
…ngagement-payments PM-3698 bulk update engagement payments
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| city, | ||
| associatedSkills, | ||
| }: any = work | ||
| const normalizedCompanyName: string = _.trim(companyName || company || '') |
There was a problem hiding this comment.
[correctness]
Using _.trim on companyName || company || '' is safe, but consider handling cases where both companyName and company are non-empty but different. This could lead to unexpected results if the data is inconsistent.
| associatedSkills: Array.isArray(associatedSkills) ? associatedSkills : undefined, | ||
| cityName: city, | ||
| companyName: companyName || '', | ||
| company: normalizedCompanyName || undefined, |
There was a problem hiding this comment.
[correctness]
The property company is being set to normalizedCompanyName || undefined. If normalizedCompanyName is an empty string, this will result in company being undefined. Ensure this behavior is intentional and aligns with the expected data structure.
|
|
||
| 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 updates 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 bottlenecks as each request waits for the previous one to complete. Consider using Promise.all to execute requests concurrently if order is not important.
| 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 could lead to issues if the user enters only whitespace. Consider trimming the input before validation.
| size='lg' | ||
| /> | ||
| )} | ||
| {props.selectedCount && props.selectedCount > 0 && ( |
There was a problem hiding this comment.
[💡 readability]
The condition props.selectedCount && props.selectedCount > 0 is redundant. props.selectedCount > 0 is sufficient since it inherently checks for truthiness.
| className={styles.bulkButton} | ||
| label={`${props.selectedCount > 1 ? 'Bulk ' : ''}Approve (${props.selectedCount})`} | ||
| size='lg' | ||
| onClick={() => props.onBulkClick?.()} |
There was a problem hiding this comment.
[correctness]
Consider providing a default value or handling the case where onBulkClick is not defined, to avoid potential runtime errors if the function is expected to be called.
| } | ||
| }, [props.selectedPayments]) | ||
|
|
||
| // Only rows with this status are selectable for bulk actions |
There was a problem hiding this comment.
[💡 maintainability]
Consider extracting the selectableStatus string into a constant or configuration file to avoid magic strings and improve maintainability.
| type='checkbox' | ||
| aria-label='Select All' | ||
| checked={allVisibleSelected} | ||
| ref={el => { if (el) el.indeterminate = someVisibleSelected }} |
There was a problem hiding this comment.
[correctness]
Using ref to set indeterminate state directly on the DOM element can lead to unexpected behavior if the component re-renders. Consider using a useEffect hook to manage the indeterminate state based on someVisibleSelected.
| const someVisibleSelected = visibleSelectablePayments.some(p => selectedPayments[p.id]) && !allVisibleSelected | ||
|
|
||
| const onToggleSelectAll = (checked: boolean) => { | ||
| if (checked) { |
There was a problem hiding this comment.
[correctness]
The onToggleSelectAll function directly mutates the selectedPayments state, which can lead to bugs if the state is updated elsewhere asynchronously. Consider using a functional state update to ensure consistency.
| certification?: UserSkillActivity | ||
| course?: UserSkillActivity | ||
| challenge?: UserSkillActivity | ||
| challenge?: { |
There was a problem hiding this comment.
[maintainability]
Changing challenge from UserSkillActivity to an object with dynamic keys of type UserSkillActivity increases flexibility but may introduce issues if not handled properly. Ensure that all parts of the codebase that interact with challenge are updated to handle this new structure. Consider if this change impacts serialization/deserialization or any external API contracts.
| return 'Loading...' | ||
| } | ||
|
|
||
| if (!skillDetails.lastUsedDate && !skillDetails.activity) { |
There was a problem hiding this comment.
[correctness]
Returning an empty string when both lastUsedDate and activity are missing might lead to unexpected UI behavior. Consider providing a more informative fallback or handling this case explicitly in the UI.
| <strong>Last used:</strong> | ||
| <span>{format(new Date(skillDetails.lastUsedDate), 'MMM dd, yyyy HH:mm')}</span> | ||
| <strong>Last activity:</strong> | ||
| <span>{format(new Date(skillDetails.lastUsedDate), 'MMM dd, yyyy')}</span> |
There was a problem hiding this comment.
[❗❗ correctness]
The format function is called with skillDetails.lastUsedDate without checking if it's a valid date. If lastUsedDate is undefined or invalid, this could throw an error. Consider validating lastUsedDate before formatting.
| ))} | ||
| </li> | ||
| )} | ||
| {skillDetails.activity.challenge && Object.keys(skillDetails.activity.challenge) |
There was a problem hiding this comment.
[❗❗ correctness]
Using Object.keys(skillDetails.activity.challenge) assumes challenge is an object. Ensure challenge is always an object or handle cases where it might not be.
…-approver-fixes PM-3698 - engagement approver fixes
| // 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]
The editPayment function is awaited inside a loop. This can lead to performance issues if the number of payments is large, as each request waits for the previous one to complete. Consider using Promise.all to execute these requests concurrently, if the order of execution is not critical.
| } | ||
| } | ||
|
|
||
| if (successfullyUpdated === ids.length) { |
There was a problem hiding this comment.
[💡 usability]
The success message is only shown if all payments are successfully updated. Consider providing feedback for partial success as well, to inform the user how many payments were successfully updated even if some failed.
| size='lg' | ||
| /> | ||
| )} | ||
| {!!props.selectedCount && props.selectedCount > 0 && ( |
There was a problem hiding this comment.
[💡 readability]
The use of double negation !!props.selectedCount is redundant here since props.selectedCount > 0 already ensures that props.selectedCount is truthy. Consider removing !!props.selectedCount for clarity.
3 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3698
https://topcoder.atlassian.net/browse/PM-3717
https://topcoder.atlassian.net/browse/PM-2651
What's in this PR?