Conversation
PM-3080 disable delete for submission if run not complete
PM-3080 Fix delete submission
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| const showDeleteButton = status !== CHALLENGE_STATUS.COMPLETED |
There was a problem hiding this comment.
[maintainability]
The showDeleteButton logic is duplicated in the JSX rendering. Consider refactoring this logic into a helper function to improve maintainability and reduce redundancy.
| <DeleteIcon /> | ||
| </button> | ||
| {showDeleteButton && ( | ||
| isWorkflowRunComplete ? ( |
There was a problem hiding this comment.
[💡 readability]
The conditional rendering of the delete button based on isWorkflowRunComplete is nested deeply, which can make it harder to read. Consider extracting this logic into a separate component or function to improve readability.
| // submissionPhaseStartDate will be the start date of | ||
| // the current submission/checkpoint or empty string if any other phase | ||
|
|
||
| const TERMINAL_STATUSES = [ |
There was a problem hiding this comment.
[💡 performance]
Consider moving TERMINAL_STATUSES outside of the loop to avoid redefining it on each iteration. This will improve performance slightly by not recreating the array multiple times.
|
|
||
| const hasRuns = workflowRunsForSubmission && workflowRunsForSubmission.length > 0; | ||
|
|
||
| const isWorkflowRunComplete = !hasRuns |
There was a problem hiding this comment.
[💡 readability]
The logic for isWorkflowRunComplete could be simplified by using !workflowRunsForSubmission || workflowRunsForSubmission.every(run => TERMINAL_STATUSES.includes(run.status));. This reduces the need for the hasRuns variable and makes the code more concise.
| return res.json(); | ||
| const data = await res.json(); | ||
|
|
||
| const opportunities = Array.isArray(data) ? data : []; |
There was a problem hiding this comment.
[maintainability]
The check Array.isArray(data) ? data : [] assumes that data should always be an array. If the API response structure changes or if there's an error in the response format, this could lead to silent failures. Consider adding logging or error handling to capture unexpected data structures.
|
|
||
| return { | ||
| ...opportunityData.result.content, | ||
| ...withEstimatedReviewerPayments(opportunityData.result.content), |
There was a problem hiding this comment.
[correctness]
The function withEstimatedReviewerPayments is applied directly to opportunityData.result.content. Ensure that opportunityData.result.content is always in the expected format and contains the necessary fields for withEstimatedReviewerPayments to function correctly. Consider adding validation or error handling to prevent runtime errors.
| const incremental = _.toNumber(incrementalPayment); | ||
| const submissions = _.toNumber(estimatedSubmissions); | ||
|
|
||
| if (_.isNaN(base) || _.isNaN(submissions)) { |
There was a problem hiding this comment.
[correctness]
Consider handling the case where basePayment is NaN more explicitly. Returning null might lead to unexpected behavior if the caller does not handle null values properly.
| estimatedSubmissions, | ||
| ); | ||
|
|
||
| if (!_.isNumber(estimatedPayment)) { |
There was a problem hiding this comment.
[💡 correctness]
The check !_.isNumber(estimatedPayment) could be more explicit by checking for null or NaN directly, as calculateEstimatedReviewerPayment only returns null or a number.
| ) => { | ||
| if (!opportunity) return opportunity; | ||
|
|
||
| const estimatedPayment = calculateEstimatedReviewerPayment( |
There was a problem hiding this comment.
[correctness]
Using _.get with a default value of _.get(opportunity, 'payments[0].payment') assumes that payments is an array and has at least one element. Consider adding a check to ensure payments is a non-empty array to avoid potential runtime errors.
2 participants
https://topcoder.atlassian.net/browse/PM-3080
https://topcoder.atlassian.net/browse/PS-476