Skip to content

Conversation

@kkartunov
Copy link
Contributor

@kkartunov kkartunov commented Dec 9, 2025

@kkartunov kkartunov requested a review from jmgasper December 9, 2025 07:46
branches:
only:
- develop
- security
Copy link

Choose a reason for hiding this comment

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

[⚠️ design]
The branch filter for build-dev now includes security. Ensure this change aligns with your workflow requirements, as it will trigger development builds for the security branch, which may not be intended.

if (_.isNil(successor.actualStartDate)) {
const desiredStartDate = new Date(currentEndDate);
const durationSeconds = Number(successor.duration);
if (!Number.isFinite(durationSeconds) || Number.isNaN(desiredStartDate.getTime())) {
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 for Number.isFinite(durationSeconds) is good, but consider also checking if durationSeconds is a positive number to avoid setting negative durations.

const durationSeconds = Number(successor.duration);
if (!Number.isFinite(durationSeconds) || Number.isNaN(desiredStartDate.getTime())) {
visited.add(successor.id);
queue.push(successorForQueue);
Copy link

Choose a reason for hiding this comment

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

[⚠️ performance]
The logic here adds the successor to the queue even if durationSeconds is not finite or desiredStartDate is invalid. This could lead to unnecessary iterations. Consider revising the logic to avoid adding such successors to the queue.

queue.push(successorForQueue);
continue;
}
const desiredEndDate = new Date(desiredStartDate.getTime() + durationSeconds * 1000);
Copy link

Choose a reason for hiding this comment

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

[💡 correctness]
The calculation of desiredEndDate assumes durationSeconds is in seconds. Ensure that this assumption is documented or validated elsewhere in the code.

const originalScheduledStartDate = challengePhase.scheduledStartDate;
const originalScheduledEndDate = challengePhase.scheduledEndDate;
const isReviewPhaseUpdate = isReviewPhase(data.name || challengePhase.name);
const shouldAttemptSuccessorRecalc =
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The condition isReviewPhaseUpdate && (data.duration || data.scheduledStartDate || data.scheduledEndDate) could be more explicit by checking for non-null values of data.duration, data.scheduledStartDate, and data.scheduledEndDate to avoid false positives when these fields are zero or empty strings.

});
if (shouldAttemptSuccessorRecalc) {
const scheduleChanged =
!datesAreSame(originalScheduledStartDate, updatedPhase.scheduledStartDate) ||
Copy link

Choose a reason for hiding this comment

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

[💡 correctness]
The datesAreSame function is used to check if dates have changed. Ensure that this function correctly handles edge cases such as different time zones or daylight saving time changes.

})

it('partially update challenge phase - recalculates successor schedules when review is extended', async function () {
this.timeout(50000)
Copy link

Choose a reason for hiding this comment

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

[💡 performance]
The test case sets a timeout of 50000ms, which is quite high. Consider reducing it if possible to avoid unnecessarily long test execution times.

})

try {
const extendedDuration = reviewDuration * 2
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The extendedDuration is calculated as reviewDuration * 2. Ensure that this multiplication does not exceed any maximum duration constraints that might be defined elsewhere in the system.

@kkartunov kkartunov changed the title Security fixes -> master [PROD] - Security fixes -> master Dec 9, 2025

const isConsistent = _.every(prizeSets, (prizeSet) =>
_.every(prizeSet.prizes, (prize) => prize.type === firstType)
const prizeTypes = _.uniq(

Choose a reason for hiding this comment

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

[💡 readability]
The use of _.flatMap and _.map to extract prize types is correct, but it could be simplified by using _.flatMap directly with a path argument: _.flatMap(prizeSets, 'prizes.type'). This would improve readability and maintainability.


if (!isConsistent) {
throw new errors.BadRequestError("All prizes must be of the same type");
if (prizeTypes.length === 0) {

Choose a reason for hiding this comment

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

[💡 style]
The check for prizeTypes.length === 0 is correct, but consider using _.isEmpty(prizeTypes) for consistency with the rest of the codebase, which uses lodash for similar checks.

_.get(project, "members", []),
(m) => _.toString(m.userId) === _.toString(currentUser.userId)
);
const role = _.toLower(_.get(member, "role", ""));

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of _.toLower on line 718 may lead to unexpected behavior if member.role is null or undefined. Consider providing a default value to avoid potential issues.

const role = _.toLower(_.get(member, "role", ""));
return PROJECT_WRITE_ACCESS_ROLES.has(role);
} catch (err) {
const status = _.get(err, "httpStatus") || _.get(err, "response.status");

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The error handling on line 721 could be improved by rethrowing the error after logging it. This ensures that upstream functions are aware of the failure and can handle it appropriately.

return new Date(dateA).getTime() === new Date(dateB).getTime();
}

function dateIsAfter(dateA, dateB) {
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The dateIsAfter function does not handle invalid date formats gracefully. Consider adding validation to ensure the input dates are valid before attempting to convert them to timestamps.


let successorForQueue = successor;
if (_.isNil(successor.actualStartDate)) {
const alignToPredecessorStart = normalizePhaseName(successor.name) === "iterative review";
Copy link

Choose a reason for hiding this comment

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

[💡 maintainability]
The logic for determining alignToPredecessorStart relies on the phase name being exactly 'iterative review'. Consider using a more flexible approach, such as checking against a set of phase names, to avoid potential issues with case sensitivity or minor name variations.

);
}
const originalScheduledEndDate = challengePhase.scheduledEndDate;
const shouldAttemptSuccessorRecalc = Boolean(
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The removal of isReviewPhaseUpdate and its associated logic may affect the intended behavior of shouldAttemptSuccessorRecalc. Ensure that the new logic correctly captures all scenarios where successor recalculation is necessary.

},
});
if (shouldAttemptSuccessorRecalc) {
const scheduleExtended = dateIsAfter(
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The scheduleExtended check only considers the scheduled end date. If the start date is also relevant for determining schedule changes, ensure it is included in the logic.

it('create challenge - invalid status', async () => {
const challengeData = _.clone(testChallengeData)
challengeData.status = ['Active']
challengeData.status = ['ACTIVE']
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The change from ['Active'] to ['ACTIVE'] does not address the underlying issue of the status being an array instead of a string. The test is expected to fail with the error message "status" must be a string. Consider updating the test data to use a string for the status.

})
} catch (e) {
should.equal(e.message.indexOf('Cannot change Completed challenge status to Active status') >= 0, true)
should.equal(e.message.indexOf('Cannot change COMPLETED challenge status to ACTIVE status') >= 0, true)
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The change from Completed to COMPLETED in the error message check ensures consistency with the status values used in the application. Ensure that the application logic and error messages are consistently using uppercase status values if this is the intended convention.

const { dedupeChallengeTerms } = require("./helper");

const SUBMISSION_PHASE_PRIORITY = ["Topcoder Submission", "Submission"];
const SUBMISSION_PHASE_PRIORITY = ["Topgear Submission", "Topcoder Submission", "Submission"];
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The addition of "Topgear Submission" to the SUBMISSION_PHASE_PRIORITY array changes the priority order for finding the submission phase. Ensure that this change is intentional and that it aligns with the business logic, as it may affect which phase is selected as the submission phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants