Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ exports[`Matches shallow shapshot 1`] = `
"id": "test-challenge",
}
}
isWorkflowRunComplete={true}
onDelete={[Function]}
onDownload={[Function]}
onOpenDownloadArtifactsModal={[Function]}
Expand Down
44 changes: 33 additions & 11 deletions src/shared/components/SubmissionManagement/Submission/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export default function Submission(props) {
onOpenRatingsListModal,
status,
allowDelete,
isWorkflowRunComplete,
} = props;
const formatDate = date => moment(+new Date(date)).format('MMM DD, YYYY hh:mm A');
const onDownloadSubmission = onDownload.bind(1, submissionObject.id);
Expand All @@ -66,6 +67,10 @@ export default function Submission(props) {
}
}

const showDeleteButton = status !== CHALLENGE_STATUS.COMPLETED
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
The showDeleteButton logic is duplicated in the JSX rendering. Consider refactoring this logic into a helper function to improve maintainability and reduce redundancy.

&& track === COMPETITION_TRACKS.DES
&& safeForDownloadCheck === true;

return (
<tr styleName="submission-row">
<td styleName="id-col">
Expand Down Expand Up @@ -146,17 +151,33 @@ export default function Submission(props) {
onClick={() => onDownload(submissionObject.id)}
><DownloadIcon /></button>
*/ }
{status !== CHALLENGE_STATUS.COMPLETED
&& track === COMPETITION_TRACKS.DES
&& safeForDownloadCheck === true && (
<button
styleName="delete-icon"
onClick={() => onDelete(submissionObject.id)}
disabled={!allowDelete}
type="button"
>
<DeleteIcon />
</button>
{showDeleteButton && (
isWorkflowRunComplete ? (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

<button
styleName="delete-icon"
onClick={() => onDelete(submissionObject.id)}
type="button"
disabled={!allowDelete}
>
<DeleteIcon />
</button>
) : (
// Disabled delete button with tooltip when workflow run is pending
<Tooltip content={() => (
<div styleName="tooltip-content">
You can delete this submission only after the review is complete.
</div>
)}
>
<button
styleName="delete-icon"
disabled
type="button"
>
<DeleteIcon />
</button>
</Tooltip>
)
)
}
{ !isTopCrowdChallenge
Expand Down Expand Up @@ -217,4 +238,5 @@ Submission.propTypes = {
allowDelete: PT.bool.isRequired,
onOpenDownloadArtifactsModal: PT.func,
onOpenRatingsListModal: PT.func,
isWorkflowRunComplete: PT.bool.isRequired,
};
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,29 @@ export default function SubmissionsTable(props) {
submissionObjects.forEach((subObject) => {
// submissionPhaseStartDate will be the start date of
// the current submission/checkpoint or empty string if any other phase

const TERMINAL_STATUSES = [
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

'COMPLETED',
'FAILURE',
'CANCELLED',
'SUCCESS',
];

const workflowRunsForSubmission = submissionWorkflowRuns
&& submissionWorkflowRuns[subObject.id]
? submissionWorkflowRuns[subObject.id]
: null;

const hasRuns = workflowRunsForSubmission && workflowRunsForSubmission.length > 0;

const isWorkflowRunComplete = !hasRuns
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

? true
: workflowRunsForSubmission.every(run => TERMINAL_STATUSES.includes(run.status));

const allowDelete = submissionPhaseStartDate
&& moment(subObject.submissionDate).isAfter(submissionPhaseStartDate);


const submission = (
<Submission
challenge={challenge}
Expand All @@ -91,13 +111,10 @@ export default function SubmissionsTable(props) {
allowDelete={allowDelete}
onOpenDownloadArtifactsModal={onOpenDownloadArtifactsModal}
onOpenRatingsListModal={onOpenRatingsListModal}
isWorkflowRunComplete={isWorkflowRunComplete}
/>
);
submissionsWithDetails.push(submission);
const workflowRunsForSubmission = submissionWorkflowRuns
&& submissionWorkflowRuns[subObject.id]
? submissionWorkflowRuns[subObject.id]
: null;

const submissionDetail = (
<tr key={subObject.id} styleName="submission-row">
Expand Down
8 changes: 6 additions & 2 deletions src/shared/services/reviewOpportunities.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { config } from 'topcoder-react-utils';
import { withEstimatedReviewerPayments } from 'utils/reviewOpportunities';

const v6ApiUrl = config.API.V6;

Expand All @@ -22,7 +23,10 @@ export default async function getReviewOpportunities(page, pageSize) {
throw new Error(res.statusText);
}

return res.json();
const data = await res.json();

const opportunities = Array.isArray(data) ? data : [];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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 opportunities.map(opportunity => withEstimatedReviewerPayments(opportunity));
}

/**
Expand Down Expand Up @@ -51,7 +55,7 @@ export async function getDetails(challengeId, opportunityId) {
const challengeData = await challengeRes.json();

return {
...opportunityData.result.content,
...withEstimatedReviewerPayments(opportunityData.result.content),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

challenge: challengeData,
};
} catch (err) {
Expand Down
55 changes: 53 additions & 2 deletions src/shared/utils/reviewOpportunities.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,54 @@ import _ from 'lodash';
import moment from 'moment';
import { REVIEW_OPPORTUNITY_TYPES } from './tc';

export const DEFAULT_ESTIMATED_SUBMISSIONS = 2;

export const calculateEstimatedReviewerPayment = (
basePayment,
incrementalPayment,
estimatedSubmissions = DEFAULT_ESTIMATED_SUBMISSIONS,
) => {
const base = _.toNumber(basePayment);
const incremental = _.toNumber(incrementalPayment);
const submissions = _.toNumber(estimatedSubmissions);

if (_.isNaN(base) || _.isNaN(submissions)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

return null;
}

const incrementalValue = _.isNaN(incremental) ? 0 : incremental;
return base + (submissions * incrementalValue);
};

export const withEstimatedReviewerPayments = (
opportunity,
estimatedSubmissions = DEFAULT_ESTIMATED_SUBMISSIONS,
) => {
if (!opportunity) return opportunity;

const estimatedPayment = calculateEstimatedReviewerPayment(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

_.get(opportunity, 'basePayment', _.get(opportunity, 'payments[0].payment')),
_.get(opportunity, 'incrementalPayment', 0),
estimatedSubmissions,
);

if (!_.isNumber(estimatedPayment)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 correctness]
The check !_.isNumber(estimatedPayment) could be more explicit by checking for null or NaN directly, as calculateEstimatedReviewerPayment only returns null or a number.

return opportunity;
}

const payments = Array.isArray(opportunity.payments)
? opportunity.payments.map(payment => ({
...payment,
payment: estimatedPayment,
}))
: opportunity.payments;

return {
...opportunity,
payments,
};
};

/**
* Infers open positions using review opportunity details and organizes them by role
*
Expand All @@ -14,7 +62,10 @@ import { REVIEW_OPPORTUNITY_TYPES } from './tc';
export const openPositionsByRole = (details) => {
if (!details.payments) return [];

const roleCount = details.payments.length;
const detailsWithPayments = withEstimatedReviewerPayments(details);
const payments = detailsWithPayments.payments || [];

const roleCount = payments.length;

let approved;
if (details.applications && details.openPositions === 1 && roleCount === 2) {
Expand All @@ -28,7 +79,7 @@ export const openPositionsByRole = (details) => {
return details.openPositions / roleCount;
};

return details.payments.map(({ role, roleId, payment }) => ({
return payments.map(({ role, roleId, payment }) => ({
role,
roleId,
payment,
Expand Down
Loading