Skip to content

Conversation

billsedison
Copy link
Collaborator

No description provided.

@IsString()
operator: string;
@IsEnum(AutopilotOperator)
operator: AutopilotOperator | string;

Choose a reason for hiding this comment

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

Consider removing the | string union type if AutopilotOperator already includes all possible string values for operator. This will ensure type safety and prevent any unexpected values.

@IsString()
operator: string;
@IsEnum(AutopilotOperator)
operator: AutopilotOperator | string;

Choose a reason for hiding this comment

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

Consider removing the | string union type if operator should strictly be an AutopilotOperator. Allowing both AutopilotOperator and string could lead to potential type inconsistencies.

@IsString()
operator: string;
@IsEnum(AutopilotOperator)
operator: AutopilotOperator | string;

Choose a reason for hiding this comment

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

The type for operator has been changed to AutopilotOperator | string. Consider whether allowing both AutopilotOperator and string is necessary. If AutopilotOperator is an enum, it might be beneficial to restrict the type to just AutopilotOperator to ensure type safety and prevent invalid values.

phaseTypeName: string;
state: 'START' | 'END';
operator: string;
operator: AutopilotOperator | string; // Allow both enum and string for flexibility

Choose a reason for hiding this comment

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

Consider documenting the AutopilotOperator enum to ensure clarity on what values are expected when using the enum type. This will help maintain consistency and prevent errors when using string values that may not match the enum.

challengeId: string; // Changed to string to support UUIDs
status: string;
operator: string;
operator: AutopilotOperator | string; // Allow both enum and string for flexibility

Choose a reason for hiding this comment

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

Consider documenting the AutopilotOperator enum to ensure clarity on what values are expected when using this type. This will help maintain consistency and prevent errors when using the operator field.

export interface CommandPayload {
command: string;
operator: string;
operator: AutopilotOperator | string; // Allow both enum and string for flexibility

Choose a reason for hiding this comment

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

Consider documenting the AutopilotOperator enum to clarify its possible values and usage, ensuring that developers understand the flexibility intended by allowing both enum and string types for operator. This will help maintain consistency and prevent potential errors when using the operator field.

private readonly challengeApiService: ChallengeApiService,
) {}
) {
// Set up the phase chain callback to handle next phase opening and scheduling

Choose a reason for hiding this comment

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

Consider adding error handling within the phase chain callback to ensure that any issues during the execution of openAndScheduleNextPhases are properly managed and logged.

projectStatus: string,
nextPhases: IPhase[],
) => {
void this.openAndScheduleNextPhases(

Choose a reason for hiding this comment

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

The use of void here suppresses any potential errors or return values from openAndScheduleNextPhases. Ensure that this is intentional and that any necessary error handling or logging is performed within the method itself.

);
}
if (message.state === 'START') {
// Advance the phase (open it) using the scheduler service

Choose a reason for hiding this comment

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

Consider using a named function instead of an immediately invoked function expression (IIFE) for better readability and maintainability.

})();
} else if (message.state === 'END') {
// Advance the phase (close it) using the scheduler service
void (async () => {

Choose a reason for hiding this comment

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

Similar to the START state, consider using a named function instead of an IIFE for better readability and maintainability.

// Advance the phase (open it) using the scheduler service
void (async () => {
try {
await this.schedulerService.advancePhase(message);

Choose a reason for hiding this comment

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

The use of void before the async function is unconventional and may be confusing. Consider removing it unless it serves a specific purpose.

} else if (message.state === 'END') {
// Advance the phase (close it) using the scheduler service
void (async () => {
try {

Choose a reason for hiding this comment

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

The use of void before the async function is unconventional and may be confusing. Consider removing it unless it serves a specific purpose.

};
this.schedulePhaseTransition(phaseData);
// Find the next phase that should be scheduled (similar to PhaseAdvancer logic)
const nextPhase = this.findNextPhaseToSchedule(challengeDetails.phases);

Choose a reason for hiding this comment

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

Consider renaming findNextPhaseToSchedule to getNextPhaseToSchedule for consistency with common naming conventions for functions that return a value.


// Determine if we should schedule for start or end based on phase state
const now = new Date();
const shouldOpen =

Choose a reason for hiding this comment

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

The variable shouldOpen is used to determine the phase state, but it might be clearer to rename it to shouldStartPhase to explicitly indicate the action being checked.

!nextPhase.actualEndDate &&
new Date(nextPhase.scheduledStartDate) <= now;

const scheduleDate = shouldOpen

Choose a reason for hiding this comment

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

Consider adding validation for nextPhase.scheduledStartDate and nextPhase.scheduledEndDate to ensure they are valid date objects before using them in comparisons or assignments.

};

this.schedulePhaseTransition(phaseData);
this.logger.log(

Choose a reason for hiding this comment

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

The log message could be more informative by including the state variable to indicate whether the phase was scheduled to start or end.

state: 'END',
};
// Find the next phase that should be scheduled (similar to PhaseAdvancer logic)
const nextPhase = this.findNextPhaseToSchedule(challengeDetails.phases);

Choose a reason for hiding this comment

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

The method findNextPhaseToSchedule is introduced here, but its implementation is not visible in the diff. Ensure that this method is correctly implemented and handles edge cases such as empty phase lists or phases without scheduled dates.

// Determine if we should schedule for start or end based on phase state
const now = new Date();
const shouldOpen =
!nextPhase.isOpen &&

Choose a reason for hiding this comment

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

The logic for determining shouldOpen could be improved by explicitly checking if nextPhase.scheduledStartDate is defined before comparing it with now. This would prevent potential runtime errors if scheduledStartDate is undefined.

!nextPhase.actualEndDate &&
new Date(nextPhase.scheduledStartDate) <= now;

const scheduleDate = shouldOpen

Choose a reason for hiding this comment

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

Consider adding validation to ensure nextPhase.scheduledStartDate and nextPhase.scheduledEndDate are valid date strings before using them in date comparisons or assignments. This will help avoid unexpected behavior if the date strings are malformed.

const state = shouldOpen ? 'START' : 'END';

if (!scheduleDate) {
this.logger.warn(

Choose a reason for hiding this comment

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

The warning message assumes that nextPhase.id and message.challengeId are always defined. Consider adding checks to ensure these values are not undefined before logging them to prevent misleading log entries.

* Find the next phase that should be scheduled based on current phase state.
* Similar logic to PhaseAdvancer.js - only schedule the next phase that should advance.
*/
private findNextPhaseToSchedule(phases: IPhase[]): IPhase | null {

Choose a reason for hiding this comment

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

Consider renaming the method findNextPhaseToSchedule to something more descriptive like determinePhaseTransition to better reflect its purpose of finding the next phase to transition based on the current state.


// First, check for phases that should be open but aren't
const phasesToOpen = phases.filter((phase) => {
if (phase.isOpen || phase.actualEndDate) {

Choose a reason for hiding this comment

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

The condition if (phase.isOpen || phase.actualEndDate) could be simplified by using a single return statement: return !(phase.isOpen || phase.actualEndDate);.

return true; // No predecessor, ready to start
}

const predecessor = phases.find((p) => p.phaseId === phase.predecessor);

Choose a reason for hiding this comment

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

Consider adding a null check for predecessor before accessing predecessor.actualEndDate to prevent potential runtime errors.


// Step 2: Schedule the phase for closure (use updated phase data from API response)
const updatedPhase =
openResult.updatedPhases?.find((p) => p.id === nextPhase.id) ||

Choose a reason for hiding this comment

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

The logic for finding updatedPhase could be simplified by using optional chaining: openResult.updatedPhases?.find((p) => p.id === nextPhase.id) ?? nextPhase;.


// Check if this phase is already scheduled to avoid duplicates
const phaseKey = `${challengeId}:${nextPhase.id}`;
if (this.activeSchedules.has(phaseKey)) {

Choose a reason for hiding this comment

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

Consider using a more descriptive variable name than phaseKey, such as phaseScheduleKey, to clarify its purpose.

* @deprecated Use openAndScheduleNextPhases instead
* Schedule next phases in the transition chain
*/
scheduleNextPhases(

Choose a reason for hiding this comment

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

The scheduleNextPhases method is marked as deprecated, but it still contains logic. Consider removing the logic entirely or refactoring it to ensure it doesn't execute inadvertently.

/**
* Get detailed information about scheduled transitions for debugging
*/
getScheduledTransitionsDetails(): {

Choose a reason for hiding this comment

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

Consider adding a return type annotation for the method getScheduledTransitionsDetails to improve type safety and readability.

} {
const activeSchedules = this.getActiveSchedules();
const scheduledJobs =
this.schedulerService.getAllScheduledTransitionsWithData();

Choose a reason for hiding this comment

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

Ensure that getAllScheduledTransitionsWithData returns a Map<string, PhaseTransitionPayload> as expected. If the return type is different, it might cause runtime errors.

() => {
void (async () => {
try {
// Before triggering the event, check if the phase still needs the transition

Choose a reason for hiding this comment

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

Consider handling potential errors from getPhaseDetails more explicitly. If the API call fails due to network issues or other reasons, it might not return null or undefined, leading to unhandled exceptions.

}

// Check if the phase is in the expected state for the transition
if (phaseData.state === 'END' && !phaseDetails.isOpen) {

Choose a reason for hiding this comment

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

The condition phaseData.state === 'END' && !phaseDetails.isOpen assumes that phaseDetails.isOpen is a reliable indicator of the phase's current state. Ensure that isOpen accurately reflects the phase's status to avoid incorrect logic execution.

return;
}

if (phaseData.state === 'START' && phaseDetails.isOpen) {

Choose a reason for hiding this comment

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

The condition phaseData.state === 'START' && phaseDetails.isOpen assumes that phaseDetails.isOpen is a reliable indicator of the phase's current state. Ensure that isOpen accurately reflects the phase's status to avoid incorrect logic execution.

}

public async triggerKafkaEvent(data: PhaseTransitionPayload) {
// Validate phase state before sending the event

Choose a reason for hiding this comment

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

Consider adding error handling for the getPhaseDetails method to manage potential exceptions that could arise during the API call. This will ensure robustness in case of network issues or unexpected API responses.

...data,
state: 'END',
operator: 'system-scheduler',
state: data.state || 'END', // Default to END if not specified

Choose a reason for hiding this comment

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

The logic for defaulting the state to 'END' if not specified might lead to unintended behavior if the data.state is null or undefined. Consider explicitly checking for these cases to ensure the default is applied correctly.

await this.kafkaService.produce(KAFKA_TOPICS.PHASE_TRANSITION, message);
this.logger.log(
`Successfully sent transition event for challenge ${data.challengeId}, phase ${data.phaseId}`,
`Successfully sent ${payload.state} transition event for challenge ${data.challengeId}, phase ${data.phaseId}`,

Choose a reason for hiding this comment

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

Consider using template literals for better readability and consistency in string interpolation. The current implementation uses a mix of template literals and string concatenation, which can be confusing. For example: Successfully sent ${payload.state} transition event for challenge ${data.challengeId}, phase ${data.phaseId}.

);

// Check current phase state to determine the correct operation
const phaseDetails = await this.challengeApiService.getPhaseDetails(

Choose a reason for hiding this comment

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

Consider handling the case where getPhaseDetails might throw an error instead of returning null. This can help prevent unexpected behavior if the API call fails.

} else if (data.state === 'END') {
operation = 'close';
} else {
// Fallback: determine based on current phase state

Choose a reason for hiding this comment

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

The fallback logic assumes that if the state is neither 'START' nor 'END', it will determine the operation based on the current phase state. Ensure that this logic aligns with the intended behavior, as it might lead to unexpected operations if the state is not clearly defined.

);

// Handle both sync and async callbacks
if (callbackResult instanceof Promise) {

Choose a reason for hiding this comment

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

Consider adding a timeout or error handling mechanism for the asynchronous callback to prevent potential hanging operations if the promise does not resolve.

): Promise<PhaseAdvanceResponseDto> {
const body = { phaseId, operation };
// Get the phase name from the phase ID - the API expects phase name, not phase ID
const phaseName = await this.getPhaseTypeName(challengeId, phaseId);

Choose a reason for hiding this comment

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

Consider handling the case where getPhaseTypeName might throw an error or reject the promise, which could lead to unhandled exceptions.

const phaseName = await this.getPhaseTypeName(challengeId, phaseId);
if (!phaseName || phaseName === 'Unknown') {
this.logger.error(
`Cannot advance phase: Phase with ID ${phaseId} not found in challenge ${challengeId}`,

Choose a reason for hiding this comment

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

The error message could be enhanced by including more context, such as the operation being attempted, to aid in debugging.

method: 'POST',
url: `${this.challengeApiUrl}/challenges/${challengeId}/advance-phase`,
body,
headers: await this.getAuthHeader().catch(() => ({

Choose a reason for hiding this comment

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

The error handling for getAuthHeader() should be improved. Instead of catching the error and returning a static object with an error message, consider logging the error for better traceability and debugging. Additionally, ensure that the rest of the application can handle the absence of a valid auth header gracefully.

state: PhaseState;

@IsString()
@IsEnum(AutopilotOperator)

Choose a reason for hiding this comment

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

Consider removing | string from the type of operator if AutopilotOperator is intended to be a strict enumeration. Allowing both AutopilotOperator and string could lead to inconsistencies and make it harder to enforce valid values.

status: string;

@IsString()
@IsEnum(AutopilotOperator)

Choose a reason for hiding this comment

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

Consider removing the | string type from the operator property if AutopilotOperator is meant to strictly define the allowed values. Allowing both AutopilotOperator and string could lead to inconsistencies in the expected values.

command: string;

@IsString()
@IsEnum(AutopilotOperator)

Choose a reason for hiding this comment

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

The change from @IsString() to @IsEnum(AutopilotOperator) suggests that operator should be validated as an enum type. However, the type AutopilotOperator | string allows for string values that might not be part of the enum. Consider ensuring that the validation logic aligns with the intended type, possibly by handling string values separately if they are expected.

} from 'class-validator';
import { KafkaMessageTemplate } from './kafka.template';
import { KAFKA_TOPICS } from '../constants/topics';
import { AutopilotOperator } from '../../autopilot/interfaces/autopilot.interface';

Choose a reason for hiding this comment

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

The import statement for AutopilotOperator is added, but it is not used in the current diff. Ensure that this import is necessary and utilized in the code to avoid unused imports.

state: 'START' | 'END';

@IsString()
@IsEnum(AutopilotOperator)

Choose a reason for hiding this comment

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

Consider removing the string type from the operator property if AutopilotOperator already encompasses all valid string values. This will ensure type safety and prevent invalid strings from being assigned to operator.

status: string;

@IsString()
@IsEnum(AutopilotOperator)

Choose a reason for hiding this comment

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

Consider ensuring that the operator field is consistently typed. Allowing both AutopilotOperator and string could lead to type inconsistencies. If string is necessary, ensure that it is validated or converted appropriately to maintain type safety.

@IsEnum(AutopilotOperator)
@IsNotEmpty()
operator: string;
operator: AutopilotOperator | string;

Choose a reason for hiding this comment

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

The type for operator has been changed to AutopilotOperator | string. If AutopilotOperator is an enum, consider whether allowing a string type is necessary, as it could bypass the enum validation. If the flexibility is required, ensure that the logic handling this field can manage both types effectively.

this.logger.log('Starting recovery process...');

try {
// const activeChallenges: IChallenge[] =

Choose a reason for hiding this comment

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

The commented-out line of code should be removed if it is no longer needed. Leaving commented-out code can lead to confusion and clutter in the codebase.

const phaseEndDate = phase.scheduledEndDate;
if (!phaseEndDate) {
// Process all phases that need to be scheduled or triggered
const phasesToProcess = this.findAllPhasesToProcess(challenge.phases);

Choose a reason for hiding this comment

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

Consider renaming the method findAllPhasesToProcess to something more descriptive that indicates what criteria are used to determine which phases need processing.

let scheduleDate: string;
let state: 'START' | 'END';

if (action === 'start') {

Choose a reason for hiding this comment

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

The variable state is being set based on the action value. Consider using an enum or constant for 'START' and 'END' to avoid hardcoding these values multiple times.


if (endTime <= now) {
const scheduleTime = new Date(scheduleDate).getTime();
if (scheduleTime <= currentTime.getTime()) {

Choose a reason for hiding this comment

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

The comparison scheduleTime <= currentTime.getTime() might lead to unexpected behavior if the times are exactly equal. Consider whether this is the intended logic or if a strict inequality should be used.

* Similar logic to AutopilotService but also handles overdue phases.
* @deprecated Use findAllPhasesToProcess instead
*/
private findNextPhaseToProcess(phases: IPhase[]): IPhase | null {

Choose a reason for hiding this comment

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

The findNextPhaseToProcess method is marked as deprecated, but it is still present in the code. Consider removing this method if it is no longer needed, or ensure that it is properly documented and tested if it must remain for backward compatibility.

}
}

/**

Choose a reason for hiding this comment

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

The findAllPhasesToProcess method could benefit from additional validation checks, such as ensuring that the phases array is not null or undefined before processing. This would prevent potential runtime errors.

// Check predecessor requirements
if (phase.predecessor) {
const predecessor = phases.find(
(p) => p.phaseId === phase.predecessor,

Choose a reason for hiding this comment

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

In the findAllPhasesToProcess method, when checking for a predecessor phase, consider handling the case where phase.predecessor is defined but the corresponding phase is not found in the phases array. This could prevent potential issues if the data is inconsistent.

if (startTime <= now) {
// Check predecessor requirements
if (phase.predecessor) {
const predecessor = challenge.phases.find(

Choose a reason for hiding this comment

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

Consider using optional chaining for accessing phase.predecessor to avoid potential runtime errors if phase is undefined or null.

const predecessor = challenge.phases.find(
(p) => p.phaseId === phase.predecessor,
);
if (!predecessor || !predecessor.actualEndDate) {

Choose a reason for hiding this comment

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

The check for predecessor and predecessor.actualEndDate could be simplified using optional chaining and nullish coalescing to improve readability.

}
}
// Should start now or soon
scheduleDate = phase.scheduledStartDate;

Choose a reason for hiding this comment

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

Consider using toISOString() when assigning scheduleDate to ensure consistent date format.

}
} else if (phase.isOpen) {
// Phase is currently open, schedule for end
scheduleDate = phase.scheduledEndDate;

Choose a reason for hiding this comment

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

Ensure scheduleDate is properly formatted before assignment to avoid potential issues with date parsing.

continue;
}

if (!scheduleDate) continue;

Choose a reason for hiding this comment

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

The condition if (!scheduleDate) continue; is redundant since scheduleDate is always assigned before this line. Consider removing it to simplify the code.

state: 'END',
operator: 'system-sync',
state,
operator: AutopilotOperator.SYSTEM_SYNC,

Choose a reason for hiding this comment

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

Consider using a constant or enum for AutopilotOperator.SYSTEM_SYNC if it is used in multiple places to ensure consistency and ease of maintenance.

new Date(phaseEndDate).getTime()
(new Date(scheduledJob.date).getTime() !==
new Date(scheduleDate).getTime() ||
scheduledJob.state !== state)

Choose a reason for hiding this comment

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

Ensure that the comparison logic for scheduledJob.state !== state is correct and that state is always defined and has the expected value. This could potentially lead to unexpected behavior if state is undefined or null.

# Start the application using ts-node-dev
echo "Starting application in development mode..."
npm run start:dev
mkdir -p logs

Choose a reason for hiding this comment

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

Consider checking if the logs directory already exists before attempting to create it with mkdir -p logs. This can help avoid unnecessary operations.

echo "Starting application in development mode..."
npm run start:dev
mkdir -p logs
TIMESTAMP=$(date +"%Y%m%d_%H%M%S")

Choose a reason for hiding this comment

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

Ensure that the date command format is compatible across different operating systems, as some may not support the +"%Y%m%d_%H%M%S" format. Consider using a more universally compatible format or handling OS-specific differences.

npm run start:dev
mkdir -p logs
TIMESTAMP=$(date +"%Y%m%d_%H%M%S")
npm run start:dev 2>&1 | tee logs/app_${TIMESTAMP}.log

Choose a reason for hiding this comment

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

Redirecting both stdout and stderr to a log file is useful for debugging, but ensure that sensitive information is not inadvertently logged. Review the application output for any sensitive data that should be excluded from logs.

phases: [
{
id: 'p1a2b3c4-d5e6-f7a8-b9c0-d1e2f3a4b5c6',
phaseId: 'p1a2b3c4-d5e6-f7a8-b9c0-d1e2f3a4b5c6',

Choose a reason for hiding this comment

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

The addition of phaseId seems redundant as it duplicates the id field. Consider whether both fields are necessary or if one can be removed to avoid redundancy.

phaseId: 'p1a2b3c4-d5e6-f7a8-b9c0-d1e2f3a4b5c6',
name: 'Registration',
isOpen: true,
scheduledStartDate: mockPastPhaseDate,

Choose a reason for hiding this comment

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

The isOpen property is added but not explained in the context of the diff. Ensure that its purpose is clear and that it is used consistently throughout the codebase.

scheduledStartDate: mockPastPhaseDate,
scheduledEndDate: mockFuturePhaseDate1,
actualStartDate: mockPastPhaseDate,
actualEndDate: null,

Choose a reason for hiding this comment

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

The actualEndDate is set to null. If this is intentional, ensure that there is logic elsewhere to handle null values appropriately. Otherwise, consider initializing it with a meaningful default value.

phaseId: 'p2a3b4c5-d6e7-f8a9-b0c1-d2e3f4a5b6c7',
name: 'Submission',
isOpen: false,
scheduledStartDate: mockFuturePhaseDate1,

Choose a reason for hiding this comment

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

The isOpen property is added but not explained in the context of the diff. Ensure that its purpose is clear and that it is used consistently throughout the codebase.

isOpen: false,
scheduledStartDate: mockFuturePhaseDate1,
scheduledEndDate: mockFuturePhaseDate2,
actualStartDate: null,

Choose a reason for hiding this comment

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

The actualStartDate is set to null. If this is intentional, ensure that there is logic elsewhere to handle null values appropriately. Otherwise, consider initializing it with a meaningful default value.

scheduledStartDate: mockFuturePhaseDate1,
scheduledEndDate: mockFuturePhaseDate2,
actualStartDate: null,
actualEndDate: null,

Choose a reason for hiding this comment

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

The actualEndDate is set to null. If this is intentional, ensure that there is logic elsewhere to handle null values appropriately. Otherwise, consider initializing it with a meaningful default value.

scheduledEndDate: mockFuturePhaseDate2,
actualStartDate: null,
actualEndDate: null,
predecessor: 'p1a2b3c4-d5e6-f7a8-b9c0-d1e2f3a4b5c6',

Choose a reason for hiding this comment

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

The predecessor field is added, which implies a dependency between phases. Ensure that this logic is correctly implemented and tested to handle phase transitions based on predecessors.

{
...mockChallenge.phases[0],
scheduledEndDate: mockPastPhaseDate,
isOpen: true, // This phase is overdue but still open

Choose a reason for hiding this comment

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

Consider verifying if the isOpen property is necessary for the logic optimization. If the phase is overdue, it might be redundant to mark it as open unless it serves a specific purpose in the transition logic.

.useValue({
getAllActiveChallenges: mockGetAllActiveChallenges,
getChallenge: mockGetChallenge,
getChallengeById: mockGetActiveChallenge, // Add the new method to the mock

Choose a reason for hiding this comment

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

The comment // Add the new method to the mock is unnecessary and should be removed as it does not contribute to the functionality of the code.

getChallenge: mockGetChallenge,
getChallengeById: mockGetActiveChallenge, // Add the new method to the mock
advancePhase: jest.fn().mockResolvedValue({ success: true, message: 'Phase advanced' }),
getPhaseDetails: jest.fn().mockImplementation((challengeId, phaseId) => {

Choose a reason for hiding this comment

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

Consider handling the case where mockChallenge or mockChallenge.phases is undefined to prevent potential runtime errors.

scheduledEndDate: mockFuturePhaseDate1,
});
}),
advancePhase: jest.fn().mockImplementation((challengeId, phaseId, operation) => {

Choose a reason for hiding this comment

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

The advancePhase function could benefit from additional validation to ensure that challengeId and phaseId are valid and exist within the mock data before proceeding with the operation.

]
});
}
return Promise.resolve({ success: false, message: 'Unknown operation' });

Choose a reason for hiding this comment

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

The return statement for unknown operations could include more detailed information about why the operation is unknown, such as logging the received operation value for debugging purposes.


describe('Challenge Creation and Scheduling', () => {
it('should schedule phases when a challenge.notification.create event is received', async () => {
it('should schedule only the next phase when a challenge.notification.create event is received', async () => {

Choose a reason for hiding this comment

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

The test description has been updated to reflect that only the next phase should be scheduled. Ensure that the test logic is also updated to verify this behavior accurately. Check that the test setup and assertions align with the new requirement of scheduling only the next phase.

projectId: mockChallenge.projectId,
status: 'ACTIVE',
operator: 'system',
operator: AutopilotOperator.SYSTEM,

Choose a reason for hiding this comment

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

Consider verifying that AutopilotOperator.SYSTEM is correctly imported and defined elsewhere in the code to ensure that it is being used correctly here.

);
expect(scheduleSpy).toHaveBeenCalledTimes(2);
// Should only schedule 1 phase (the next one), not all phases
expect(scheduleSpy).toHaveBeenCalledTimes(1);

Choose a reason for hiding this comment

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

The comment // Should only schedule 1 phase (the next one), not all phases is redundant as the test assertion already conveys this information. Consider removing the comment.

expect(scheduleSpy).toHaveBeenCalledWith(
expect.objectContaining({
phaseId: mockChallenge.phases[0].id,
phaseId: mockChallenge.phases[0].id, // Should schedule the currently open phase

Choose a reason for hiding this comment

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

The comment // Should schedule the currently open phase is redundant as the test assertion already conveys this information. Consider removing the comment.

...mockChallenge.phases[0],
isOpen: false,
actualStartDate: mockPastPhaseDate,
actualEndDate: mockPastPhaseDate, // This phase has ended

Choose a reason for hiding this comment

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

The comment // This phase has ended is redundant as the properties actualStartDate and actualEndDate already convey this information. Consider removing the comment.

...mockChallenge.phases[1],
isOpen: false,
actualStartDate: null,
actualEndDate: null, // This phase is ready to start

Choose a reason for hiding this comment

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

The comment // This phase is ready to start is redundant as the properties actualStartDate and actualEndDate already convey this information. Consider removing the comment.

expect(scheduleSpy).toHaveBeenCalledTimes(1);
expect(scheduleSpy).toHaveBeenCalledWith(
expect.objectContaining({
phaseId: challengeWithNoOpenPhases.phases[1].id, // Should schedule the next ready phase

Choose a reason for hiding this comment

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

The comment // Should schedule the next ready phase is redundant as the test assertion already conveys this information. Consider removing the comment.

projectId: updatedChallenge.projectId,
status: 'ACTIVE',
operator: 'system',
operator: AutopilotOperator.SYSTEM,

Choose a reason for hiding this comment

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

Consider importing AutopilotOperator at the top of the file if it is not already imported, to ensure that it is defined and available for use here.

phaseTypeName: 'Registration',
state: 'END',
operator: 'system',
operator: AutopilotOperator.SYSTEM,

Choose a reason for hiding this comment

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

Consider ensuring that AutopilotOperator.SYSTEM is correctly imported and defined in the codebase. This change assumes that AutopilotOperator is an existing object or enum with a SYSTEM property. Verify that this is the case to prevent runtime errors.

challengeId: mockChallenge.id,
phaseId: mockChallenge.phases[0].id,
operator: 'admin',
operator: AutopilotOperator.ADMIN,

Choose a reason for hiding this comment

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

Consider verifying that AutopilotOperator.ADMIN is correctly imported and defined. Ensure that it aligns with the expected values and types used in the rest of the codebase.


describe('Recovery Service', () => {
it('should schedule future phases and immediately trigger overdue phases on bootstrap', async () => {
it('should immediately trigger overdue phases on bootstrap', async () => {

Choose a reason for hiding this comment

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

The test description has been changed to remove 'schedule future phases'. Ensure that the test implementation reflects this change and that there are no assertions or logic related to scheduling future phases, as it might lead to confusion or incorrect test behavior.

expect(challengeApiService.getAllActiveChallenges).toHaveBeenCalled();
expect(scheduleSpy).toHaveBeenCalledTimes(1);
// Should only trigger the overdue phase, not schedule future ones
expect(scheduleSpy).toHaveBeenCalledTimes(0);

Choose a reason for hiding this comment

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

The comment here suggests that scheduleSpy should not be called, but the test description and logic should ensure that this is the expected behavior. Verify that the test description aligns with the logic being tested.

);
});

it('should schedule future phases when no overdue phases exist', async () => {

Choose a reason for hiding this comment

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

The test description mentions scheduling future phases when no overdue phases exist. Ensure that the logic correctly handles scenarios where multiple future phases might be present and that only the appropriate phases are scheduled.

expect(triggerEventSpy).toHaveBeenCalledWith(
});

it('should open and schedule next phases in the chain when a phase completes', async () => {

Choose a reason for hiding this comment

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

The test description should clarify the conditions under which phases are opened and scheduled. Ensure that the test logic accurately reflects the intended phase transition behavior.

);
});

it('should handle complete phase chain flow from challenge creation to phase completion', async () => {

Choose a reason for hiding this comment

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

The test aims to handle the complete phase chain flow. Ensure that all edge cases are covered, such as handling phases with varying durations and dependencies.

);
});

it('should open phases before scheduling them for closure', async () => {

Choose a reason for hiding this comment

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

The test checks the order of phase operations. Ensure that the logic accounts for any asynchronous behavior that might affect the order of operations.

});
});

it('should not try to close phases that are already closed', async () => {

Choose a reason for hiding this comment

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

The test ensures that closed phases are not closed again. Verify that the logic correctly identifies and skips phases that are already closed.

expect(mockAdvancePhase).not.toHaveBeenCalled();
});

it('should not try to open phases that are already open', async () => {

Choose a reason for hiding this comment

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

The test ensures that open phases are not opened again. Verify that the logic correctly identifies and skips phases that are already open.

@jmgasper jmgasper merged commit 5a297e3 into develop Sep 15, 2025
1 check passed
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.

2 participants