-
Notifications
You must be signed in to change notification settings - Fork 4
Topcoder NestJS Autopilot Phase Transition Logic Optimization #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@IsString() | ||
operator: string; | ||
@IsEnum(AutopilotOperator) | ||
operator: AutopilotOperator | string; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 && |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) || |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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(): { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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}`, |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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}`, |
There was a problem hiding this comment.
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(() => ({ |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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[] = |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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' }); |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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.
No description provided.