Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ module.exports = {
// TODO: change this to localhost
RESOURCE_ROLES_API_URL:
process.env.RESOURCE_ROLES_API_URL || "http://api.topcoder-dev.com/v5/resource-roles",
GROUPS_API_URL: process.env.GROUPS_API_URL || "http://localhost:4000/v5/groups",
GROUPS_API_URL: process.env.GROUPS_API_URL || "http://localhost:4000/v6/groups",

Choose a reason for hiding this comment

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

[❗❗ correctness]
The change from /v5/groups to /v6/groups in the GROUPS_API_URL suggests an API version update. Ensure that the new version is backward compatible or that all necessary changes are made to accommodate any breaking changes in the new API version.

PROJECTS_API_URL: process.env.PROJECTS_API_URL || "http://localhost:4000/v5/projects",
TERMS_API_URL: process.env.TERMS_API_URL || "http://localhost:4000/v5/terms",
CUSTOMER_PAYMENTS_URL:
Expand Down
56 changes: 54 additions & 2 deletions src/services/ChallengeService.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ const REVIEW_PHASE_NAMES = Object.freeze([
"approval",
]);
const REVIEW_PHASE_NAME_SET = new Set(REVIEW_PHASE_NAMES);
const REQUIRED_REVIEW_PHASE_NAME_SET = new Set([...REVIEW_PHASE_NAMES, "iterative review"]);

/**
* Enrich skills data with full details from standardized skills API.
Expand Down Expand Up @@ -2356,6 +2357,8 @@ async function updateChallenge(currentUser, challengeId, data, options = {}) {
validateTask(currentUser, challenge, data, challengeResources);
const taskCompletionInfo = prepareTaskCompletionData(challenge, challengeResources, data);

const isStatusChangingToActive =
data.status === ChallengeStatusEnum.ACTIVE && challenge.status !== ChallengeStatusEnum.ACTIVE;
let sendActivationEmail = false;
let sendSubmittedEmail = false;
let sendCompletedEmail = false;
Expand Down Expand Up @@ -2459,8 +2462,6 @@ async function updateChallenge(currentUser, challengeId, data, options = {}) {

/* END self-service stuffs */

const isStatusChangingToActive =
data.status === ChallengeStatusEnum.ACTIVE && challenge.status !== ChallengeStatusEnum.ACTIVE;
let isChallengeBeingActivated = isStatusChangingToActive;
let isChallengeBeingCancelled = false;
if (data.status) {
Expand Down Expand Up @@ -2806,6 +2807,57 @@ async function updateChallenge(currentUser, challengeId, data, options = {}) {
});
}

if (
isStatusChangingToActive &&
(challenge.status === ChallengeStatusEnum.NEW || challenge.status === ChallengeStatusEnum.DRAFT)
) {
const effectiveReviewers = Array.isArray(data.reviewers)
? data.reviewers
: Array.isArray(challenge.reviewers)
? challenge.reviewers
: [];
const reviewerPhaseIds = new Set(
effectiveReviewers
.filter((reviewer) => reviewer && reviewer.phaseId)
.map((reviewer) => String(reviewer.phaseId))
);

if (reviewerPhaseIds.size === 0) {
throw new errors.BadRequestError(
"Cannot activate a challenge without at least one reviewer configured"
);
}

const normalizePhaseName = (name) => String(name || "").trim().toLowerCase();
const effectivePhases =
(Array.isArray(phasesForUpdate) && phasesForUpdate.length > 0
? phasesForUpdate
: challenge.phases) || [];

const missingPhaseNames = new Set();
for (const phase of effectivePhases) {
if (!phase) {
continue;
}
const normalizedName = normalizePhaseName(phase.name);
if (!REQUIRED_REVIEW_PHASE_NAME_SET.has(normalizedName)) {
continue;
}
const phaseId = _.get(phase, "phaseId");
if (!phaseId || !reviewerPhaseIds.has(String(phaseId))) {
missingPhaseNames.add(phase.name || "Unknown phase");
}
}

if (missingPhaseNames.size > 0) {
throw new errors.BadRequestError(
`Cannot activate challenge; missing reviewers for phase(s): ${Array.from(
missingPhaseNames
).join(", ")}`
);
}
}

// convert data to prisma models
const updateData = prismaHelper.convertChallengeSchemaToPrisma(
currentUser,
Expand Down
247 changes: 247 additions & 0 deletions test/unit/ChallengeService.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,152 @@ describe('challenge service unit tests', () => {
})

describe('update challenge tests', () => {
const ensureSkipTimelineTemplate = async () => {
const templateId = config.SKIP_PROJECT_ID_BY_TIMLINE_TEMPLATE_ID

Choose a reason for hiding this comment

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

[⚠️ correctness]
The variable templateId is assigned using config.SKIP_PROJECT_ID_BY_TIMLINE_TEMPLATE_ID. Ensure that this configuration value is correctly set and validated before use to prevent potential runtime errors.

let template = await prisma.timelineTemplate.findUnique({ where: { id: templateId } })
if (!template) {
template = await prisma.timelineTemplate.create({
data: {
id: templateId,
name: `skip-template-${templateId}`,
description: 'Template used to bypass project requirements in activation tests',
isActive: true,
createdBy: 'activation-test',
updatedBy: 'activation-test'
}
})
}

const existingPhases = await prisma.timelineTemplatePhase.findMany({
where: { timelineTemplateId: templateId }
})
const existingPhaseIds = new Set(existingPhases.map(p => p.phaseId))

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of new Set(existingPhases.map(p => p.phaseId)) is efficient for checking existence, but ensure that data.phase.id and data.phase2.id are defined and valid before this check to avoid potential errors.

const phaseRows = []
if (!existingPhaseIds.has(data.phase.id)) {
phaseRows.push({
timelineTemplateId: templateId,
phaseId: data.phase.id,
defaultDuration: 1000,
createdBy: 'activation-test',
updatedBy: 'activation-test'
})
}
if (!existingPhaseIds.has(data.phase2.id)) {
phaseRows.push({
timelineTemplateId: templateId,
phaseId: data.phase2.id,
predecessor: data.phase.id,
defaultDuration: 1000,
createdBy: 'activation-test',
updatedBy: 'activation-test'
})
}
if (phaseRows.length > 0) {
await prisma.timelineTemplatePhase.createMany({ data: phaseRows })
}
return templateId
}

const createChallengeWithRequiredReviewPhases = async () => {
await ensureSkipTimelineTemplate()

Choose a reason for hiding this comment

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

[⚠️ correctness]
The function createChallengeWithRequiredReviewPhases assumes that data.challenge.typeId and data.challenge.trackId are available. Ensure these values are properly initialized and validated to prevent potential issues.


const phaseNames = ['Screening', 'Review']
const createdPhaseIds = []
const phaseRecords = []

for (const phaseName of phaseNames) {
let phaseRecord = await prisma.phase.findFirst({ where: { name: phaseName } })
if (!phaseRecord) {
phaseRecord = await prisma.phase.create({
data: {
id: uuid(),
name: phaseName,
description: `${phaseName} phase`,
isOpen: false,
duration: 86400,
createdBy: 'activation-test',
updatedBy: 'activation-test'
}
})
createdPhaseIds.push(phaseRecord.id)
}
phaseRecords.push(phaseRecord)
}

const challenge = await prisma.challenge.create({
data: {
id: uuid(),
name: `Activation coverage ${Date.now()}`,
description: 'Activation coverage test',
typeId: data.challenge.typeId,
trackId: data.challenge.trackId,
timelineTemplateId: config.SKIP_PROJECT_ID_BY_TIMLINE_TEMPLATE_ID,
startDate: new Date(),
status: ChallengeStatusEnum.DRAFT,
tags: [],
groups: [],
createdBy: 'activation-test',
updatedBy: 'activation-test'
}
})

const challengePhaseIds = []
await prisma.challengePhase.createMany({
data: phaseRecords.map((phase) => {
const cpId = uuid()
challengePhaseIds.push(cpId)
return {
id: cpId,
challengeId: challenge.id,
phaseId: phase.id,
name: phase.name,
duration: phase.duration || 86400,
isOpen: false,
createdBy: 'activation-test',
updatedBy: 'activation-test'
}
})
})

return { challenge, phaseRecords, createdPhaseIds, challengePhaseIds }
}

const cleanupChallengeWithRequiredReviewPhases = async ({
challenge,
createdPhaseIds = [],
challengePhaseIds = []
}) => {
if (challengePhaseIds.length > 0) {
await prisma.challengePhase.deleteMany({ where: { id: { in: challengePhaseIds } } })
}
if (challenge) {
await prisma.challenge.delete({ where: { id: challenge.id } })
}
if (createdPhaseIds.length > 0) {
await prisma.phase.deleteMany({ where: { id: { in: createdPhaseIds } } })
}
}

const createActivationChallenge = async (status = ChallengeStatusEnum.NEW) => {

Choose a reason for hiding this comment

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

[⚠️ correctness]
The function createActivationChallenge uses data.challenge.typeId and data.challenge.trackId. Ensure these values are initialized and validated to prevent potential issues.

const timelineTemplateId = await ensureSkipTimelineTemplate()
return prisma.challenge.create({
data: {
id: uuid(),
name: `Activation reviewer check ${Date.now()}`,
description: 'activation reviewer check',
typeId: data.challenge.typeId,
trackId: data.challenge.trackId,
timelineTemplateId,
startDate: new Date(),
status,
tags: [],
groups: [],
createdBy: 'activation-test',
updatedBy: 'activation-test'
}
})
}

it('update challenge successfully 1', async () => {
const challengeData = testChallengeData
const result = await service.updateChallenge({ isMachine: true, sub: 'sub3', userId: 22838965 }, id, {
Expand Down Expand Up @@ -1128,6 +1274,107 @@ describe('challenge service unit tests', () => {
throw new Error('should not reach here')
})

it('update challenge - prevent activating without reviewers', async () => {
const activationChallenge = await createActivationChallenge(ChallengeStatusEnum.DRAFT)
try {
await service.updateChallenge({ isMachine: true, sub: 'sub-activate' }, activationChallenge.id, {
status: ChallengeStatusEnum.ACTIVE
})
} catch (e) {
should.equal(e.message.indexOf('reviewer configured') >= 0, true)

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The error message check e.message.indexOf('reviewer configured') >= 0 is fragile and could break if the error message changes. Consider using a more robust error handling strategy.

return
} finally {
await prisma.challenge.delete({ where: { id: activationChallenge.id } })
}
throw new Error('should not reach here')
})

it('update challenge - allow activating with reviewers provided', async () => {
const activationChallenge = await createActivationChallenge()
try {
const updated = await service.updateChallenge(
{ isMachine: true, sub: 'sub-activate', userId: 22838965 },
activationChallenge.id,
{
status: ChallengeStatusEnum.ACTIVE,
reviewers: [
{
phaseId: data.phase.id,
scorecardId: 'activation-scorecard',
isMemberReview: false,
aiWorkflowId: 'workflow-123'
}
]
}
)
should.equal(updated.status, ChallengeStatusEnum.ACTIVE)
should.exist(updated.reviewers)
should.equal(updated.reviewers.length, 1)
should.equal(updated.reviewers[0].scorecardId, 'activation-scorecard')
} finally {
await prisma.challenge.delete({ where: { id: activationChallenge.id } })
}
})

it('update challenge - enforce reviewers for required phases', async () => {
const setup = await createChallengeWithRequiredReviewPhases()
try {
await service.updateChallenge(
{ isMachine: true, sub: 'sub-activate', userId: 22838965 },
setup.challenge.id,
{
status: ChallengeStatusEnum.ACTIVE,
reviewers: [
{
phaseId: setup.phaseRecords[0].id,
scorecardId: 'screening-scorecard',
isMemberReview: false,
aiWorkflowId: 'workflow-screening'
}
]
}
)
} catch (e) {
should.equal(e.message.indexOf('missing reviewers for phase(s): Review') >= 0, true)

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The error message check e.message.indexOf('missing reviewers for phase(s): Review') >= 0 is fragile and could break if the error message changes. Consider using a more robust error handling strategy.

return
} finally {
await cleanupChallengeWithRequiredReviewPhases(setup)
}
throw new Error('should not reach here')
})

it('update challenge - allow activation when required phases have reviewers', async () => {
const setup = await createChallengeWithRequiredReviewPhases()
try {
const updated = await service.updateChallenge(
{ isMachine: true, sub: 'sub-activate', userId: 22838965 },
setup.challenge.id,
{
status: ChallengeStatusEnum.ACTIVE,
reviewers: [
{
phaseId: setup.phaseRecords[0].id,
scorecardId: 'screening-scorecard',
isMemberReview: false,
aiWorkflowId: 'workflow-screening'
},
{
phaseId: setup.phaseRecords[1].id,
scorecardId: 'review-scorecard',
isMemberReview: false,
aiWorkflowId: 'workflow-review'
}
]
}
)
should.equal(updated.status, ChallengeStatusEnum.ACTIVE)
should.exist(updated.reviewers)
should.equal(updated.reviewers.length, 2)
} finally {
await cleanupChallengeWithRequiredReviewPhases(setup)
}
})

it('update challenge - set winners with non-completed Active status', async () => {
try {
await service.updateChallenge({ isMachine: true, sub: 'sub3' }, id, {
Expand Down
Loading