From 1e75d1f321d09950858a10ca30c92f37ce8de8c1 Mon Sep 17 00:00:00 2001 From: Vasilica Olariu Date: Fri, 28 Aug 2026 14:11:44 +0300 Subject: [PATCH] PM-6007 - fix phase ordering when creating challenge --- src/common/phase-helper.ts | 86 +++++++++++++++++++++++++++++----- test/unit/phase-helper.test.js | 84 +++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 13 deletions(-) diff --git a/src/common/phase-helper.ts b/src/common/phase-helper.ts index 0157f2a..53bb560 100644 --- a/src/common/phase-helper.ts +++ b/src/common/phase-helper.ts @@ -272,6 +272,57 @@ function findPhaseUpdate(newPhases, phase) { return _.find(newPhases, (p) => p.phaseId === phase.phaseId); } +/** + * Order timeline template phases so every phase follows its predecessor. + * + * TimelineTemplatePhase rows carry no ordering column, so the database returns them + * in unspecified physical order. Scheduling resolves predecessor dates in a single + * pass, which is only correct when a predecessor is processed before its dependents. + * + * @param {Array} phases template phases, each with phaseId and optional predecessor + * @returns {Array} phases in dependency order; phases unreachable from a root + * (dangling predecessor or cycle) are appended last, keeping their relative order + */ +function orderPhasesByPredecessorChain(phases) { + if (!Array.isArray(phases)) { + return []; + } + + const knownPhaseIds = new Set(_.map(phases, "phaseId")); + const childrenOf = new Map(); + _.each(phases, (phase) => { + if (_.isNil(phase.predecessor) || !knownPhaseIds.has(phase.predecessor)) { + return; + } + const siblings = childrenOf.get(phase.predecessor) || []; + siblings.push(phase); + childrenOf.set(phase.predecessor, siblings); + }); + + const ordered = []; + const visited = new Set(); + const queue = _.filter(phases, (phase) => _.isNil(phase.predecessor)); + while (queue.length > 0) { + const phase = queue.shift(); + if (visited.has(phase)) { + continue; + } + visited.add(phase); + ordered.push(phase); + queue.push(...(childrenOf.get(phase.phaseId) || [])); + } + + // Phases with a dangling predecessor, or caught in a cycle, are never reachable from + // a root. Keep them so the caller still emits them, unscheduled, as it did before. + _.each(phases, (phase) => { + if (!visited.has(phase)) { + ordered.push(phase); + } + }); + + return ordered; +} + class ChallengePhaseHelper { phaseDefinitionMap: any = {}; timelineTemplateMap: any = {}; @@ -281,10 +332,13 @@ class ChallengePhaseHelper { throw new errors.BadRequestError(`Invalid timeline template ID: ${timelineTemplateId}`); } const { timelineTempate } = await this.getTemplateAndTemplateMap(timelineTemplateId); - console.log("Selected timeline template", JSON.stringify(timelineTempate)); const { phaseDefinitionMap } = await this.getPhaseDefinitionsAndMap(); + // The template rows have no stored order, so walk the predecessor chain instead of + // trusting the order the database happened to return them in. + const orderedTemplate = orderPhasesByPredecessorChain(timelineTempate); + console.log("Selected timeline template", JSON.stringify(orderedTemplate)); let fixedStartDate = undefined; - const finalPhases = _.map(timelineTempate, (phaseFromTemplate) => { + const finalPhases = _.map(orderedTemplate, (phaseFromTemplate) => { const phaseDefinition = phaseDefinitionMap.get(phaseFromTemplate.phaseId); const phaseFromInput = _.find(phases, (p) => p.phaseId === phaseFromTemplate.phaseId); const phase = { @@ -324,23 +378,29 @@ class ChallengePhaseHelper { return phase; }); for (const phase of finalPhases) { - if (_.isUndefined(phase.predecessor)) { + if (_.isNil(phase.predecessor)) { continue; } const precedecessorPhase = _.find(finalPhases, { phaseId: phase.predecessor, }); - if (!_.isNil(precedecessorPhase)) { - if (phase.name === "Iterative Review") { - phase.scheduledStartDate = precedecessorPhase.scheduledStartDate; - } else { - phase.scheduledStartDate = precedecessorPhase.scheduledEndDate; - } - phase.scheduledEndDate = moment(phase.scheduledStartDate) - .add(phase.duration, "seconds") - .toDate() - .toISOString(); + if (_.isNil(precedecessorPhase)) { + continue; + } + const inheritedStartDate = + phase.name === "Iterative Review" + ? precedecessorPhase.scheduledStartDate + : precedecessorPhase.scheduledEndDate; + // An unresolved predecessor would make moment() fall back to the current time, + // scheduling this phase before the challenge even starts. Leave it unscheduled. + if (_.isNil(inheritedStartDate)) { + continue; } + phase.scheduledStartDate = inheritedStartDate; + phase.scheduledEndDate = moment(phase.scheduledStartDate) + .add(phase.duration, "seconds") + .toDate() + .toISOString(); } return finalPhases; } diff --git a/test/unit/phase-helper.test.js b/test/unit/phase-helper.test.js index d5c9d7e..a722a13 100644 --- a/test/unit/phase-helper.test.js +++ b/test/unit/phase-helper.test.js @@ -817,4 +817,88 @@ describe('phase helper unit tests', () => { throw new Error('should not reach here') }) + it('schedules created phases along the predecessor chain regardless of template row order', async () => { + const registrationPhaseId = 'registration-phase' + const submissionPhaseId = 'submission-phase' + const appealsPhaseId = 'appeals-phase' + const oneDay = 24 * 60 * 60 + const startDate = '2026-09-01T00:00:00.000Z' + + // The DB returns TimelineTemplatePhase rows in unspecified order, so hand them over + // reversed: Appeals arrives before the predecessors it depends on. + stubPhaseLookups( + [ + { id: registrationPhaseId, name: 'Registration', description: 'Registration phase' }, + { id: submissionPhaseId, name: 'Submission', description: 'Submission phase' }, + { id: appealsPhaseId, name: 'Appeals', description: 'Appeals phase' } + ], + [ + { phaseId: appealsPhaseId, predecessor: submissionPhaseId, defaultDuration: oneDay }, + { phaseId: submissionPhaseId, predecessor: registrationPhaseId, defaultDuration: oneDay }, + { phaseId: registrationPhaseId, defaultDuration: oneDay } + ] + ) + + const createdPhases = await phaseHelper.populatePhasesForChallengeCreation( + [], + startDate, + 'timeline-template-id' + ) + + const byName = new Map(createdPhases.map((phase) => [phase.name, phase])) + + byName.get('Registration').scheduledStartDate.should.equal(startDate) + byName.get('Registration').scheduledEndDate.should.equal('2026-09-02T00:00:00.000Z') + byName.get('Submission').scheduledStartDate.should.equal('2026-09-02T00:00:00.000Z') + byName.get('Submission').scheduledEndDate.should.equal('2026-09-03T00:00:00.000Z') + byName.get('Appeals').scheduledStartDate.should.equal('2026-09-03T00:00:00.000Z') + byName.get('Appeals').scheduledEndDate.should.equal('2026-09-04T00:00:00.000Z') + + // Challenges are read back ordered by scheduledEndDate, so Appeals must land last. + const namesByEndDate = createdPhases + .slice() + .sort((a, b) => a.scheduledEndDate.localeCompare(b.scheduledEndDate)) + .map((phase) => phase.name) + + namesByEndDate.should.deep.equal(['Registration', 'Submission', 'Appeals']) + }) + + it('leaves created phases unscheduled when the predecessor chain has no resolvable root', async () => { + const registrationPhaseId = 'registration-phase' + const reviewPhaseId = 'review-phase' + const appealsPhaseId = 'appeals-phase' + const oneDay = 24 * 60 * 60 + const startDate = '2026-09-01T00:00:00.000Z' + + // Review points at a phase absent from the template, so neither Review nor the + // Appeals phase hanging off it can be scheduled. + stubPhaseLookups( + [ + { id: registrationPhaseId, name: 'Registration', description: 'Registration phase' }, + { id: reviewPhaseId, name: 'Review', description: 'Review phase' }, + { id: appealsPhaseId, name: 'Appeals', description: 'Appeals phase' } + ], + [ + { phaseId: appealsPhaseId, predecessor: reviewPhaseId, defaultDuration: oneDay }, + { phaseId: reviewPhaseId, predecessor: 'phase-not-in-template', defaultDuration: oneDay }, + { phaseId: registrationPhaseId, defaultDuration: oneDay } + ] + ) + + const createdPhases = await phaseHelper.populatePhasesForChallengeCreation( + [], + startDate, + 'timeline-template-id' + ) + + createdPhases.should.have.lengthOf(3) + const byName = new Map(createdPhases.map((phase) => [phase.name, phase])) + + byName.get('Registration').scheduledStartDate.should.equal(startDate) + // Previously these fell back to moment(undefined), scheduling them at "now". + for (const name of ['Review', 'Appeals']) { + chai.expect(byName.get(name).scheduledStartDate).to.be.undefined + chai.expect(byName.get(name).scheduledEndDate).to.be.undefined + } + }) })