-
Notifications
You must be signed in to change notification settings - Fork 21
[PROD] - Payment Status UI fixes & logs #1358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…cache PM-3091 - clear review cache
PM-3089 failure run
fix(PM-3079): removed border bottom to last row in ai table
fix(PM-3137): mutation keys
…dmin-ui-to-show-onholdadmin PS-469 - update wallet admin ui to show the onhold admin dropdown
fix(PM-3073): UI in ai review table
| width: 100%; | ||
| border-collapse: collapse; | ||
|
|
||
| &.reviewsTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The nested &.reviewsTable selector is redundant since it is already within the .reviewsTable block. Consider removing the nested selector to simplify the code.
|
|
||
| .scoreCol { | ||
| text-align: right; | ||
| text-align: left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[readability]
Changing the text alignment of .scoreCol from right to left could impact the layout or readability of the table, especially if the column contains numerical data. Verify that this change aligns with the intended design and does not negatively affect the user interface.
| await mutate(`${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}]`) | ||
| setShowReply(false) | ||
| }, [workflowId, workflowRun?.id, feedback?.id]) | ||
| }, [workflowId, workflowRun?.id, workflowRun?.status, feedback?.id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The dependency array for useCallback now includes workflowRun?.status. Ensure that this change is intentional and that onSubmitReply should indeed be re-created when workflowRun?.status changes. If not, this could lead to unnecessary re-renders.
| ${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}] | ||
| `) | ||
| // eslint-disable-next-line max-len | ||
| await mutate(`${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}]`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The mutate function is called with a URL that includes a query parameter with a potentially undefined value (workflowRun?.status). Consider adding a check to ensure workflowRun?.status is defined before constructing the URL to avoid potential issues with malformed URLs.
| ${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}] | ||
| `) | ||
| // eslint-disable-next-line max-len | ||
| await mutate(`${EnvironmentConfig.API.V6}/workflows/${workflowId}/runs/${workflowRun?.id}/items?[${workflowRun?.status}]`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Similar to line 40, ensure that workflowRun?.status is defined before using it in the URL to prevent potential issues with malformed URLs.
| ), | ||
| } | ||
| reset(newFormData) | ||
| const newFormData = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The removal of the check if (reviewItems?.length) before processing reviewItems could lead to unnecessary processing when reviewItems is an empty array. Consider re-adding the check to avoid unnecessary operations.
| challengeInfo, | ||
| }: ChallengeDetailContextModel = useChallengeDetailsContext() | ||
| const navigate = useAppNavigate() | ||
| const workflowRunIsFailed = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 performance]
The use of includes with an array containing a single element is unnecessary. Consider using a direct comparison: workflowRun?.status === AiWorkflowRunStatusEnum.FAILURE.
| status = 'Owed' | ||
| } | ||
|
|
||
| setConfirmFlow({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The removal of the status normalization logic might lead to inconsistencies in how payment statuses are handled. Ensure that the PaymentEditForm component correctly handles all possible status values without this normalization.
| 'On Hold (Payment Provider)', | ||
| ].includes(props.payment.status) | ||
|
|
||
| return [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The use of props.payment.status directly in the options array could lead to inconsistencies if the status string changes elsewhere in the code. Consider using a constant or an enumeration for payment statuses to ensure consistency and reduce the risk of typos.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PS-469
https://topcoder.atlassian.net/browse/PM-3089
https://topcoder.atlassian.net/browse/PM-3091
https://topcoder.atlassian.net/browse/PM-3079
https://topcoder.atlassian.net/browse/PM-3137
https://topcoder.atlassian.net/browse/PM-3073
What's in this PR?