Conversation
…emplateId and attach default ai config when creating a challenge
…v6 into PM-4062_ai-config
[DO NOT MERGE] PM-4062 default ai config
PM-4062 fix migration file
…ing phase after the submission phase
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| process.env.SUBMISSIONS_API_URL || "https://api.topcoder-dev.com/v5/submissions", | ||
| MEMBERS_API_URL: process.env.MEMBERS_API_URL || "https://api.topcoder-dev.com/v6/members", | ||
| REVIEW_SUMMATIONS_API_URL: process.env.REVIEW_SUMMATIONS_API_URL || "https://api.topcoder-dev.com/v6/reviewSummations", | ||
| REVIEWS_API_URL: process.env.REVIEWS_API_URL || "https://api.topcoder-dev.com", |
There was a problem hiding this comment.
[maintainability]
The REVIEWS_API_URL is set to https://api.topcoder-dev.com, which is inconsistent with the other API URLs that specify a version (e.g., /v5 or /v6). Consider adding a version to ensure consistency and avoid potential issues with future API changes.
| @@ -4,5 +4,6 @@ AUTH0_URL=https://topcoder-dev.auth0.com/oauth/token | |||
| AUTH0_AUDIENCE=https://m2m.topcoder-dev.com/ | |||
| DATABASE_URL="postgresql://topcoder:NUDvFEZzrey2x2og2!zn_kBzcaJ.fu_njAJcD*z2q@topcoder-services.ci8xwsszszsw.us-east-1.rds.amazonaws.com:5432/topcoder-services?schema=challenges" | |||
There was a problem hiding this comment.
[❗❗ security]
The DATABASE_URL contains hardcoded credentials, which is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
PM-3926 ai screening phase
| '2025-03-10T13:08:02.378Z', | ||
| 'topcoder user' | ||
| ) | ||
| ON CONFLICT DO NOTHING; |
There was a problem hiding this comment.
[correctness]
Using ON CONFLICT DO NOTHING can silently ignore conflicts without logging or handling them, which might lead to data inconsistencies or missed updates. Consider specifying a conflict target and handling the conflict appropriately to ensure data integrity.
| * @param {Function} logDebugMessage optional logging function | ||
| */ | ||
| async addAIScreeningPhaseForChallengeCreation(challenge, prisma, logDebugMessage = () => {}) { | ||
| if (!challenge || !challenge.phases || !Array.isArray(challenge.reviewers)) { |
There was a problem hiding this comment.
[maintainability]
The function addAIScreeningPhaseForChallengeCreation mutates the challenge object in-place. Consider returning a new object or using a more functional approach to avoid potential side effects, which can improve maintainability.
| ); | ||
|
|
||
| if (!submissionPhaseName) { | ||
| throw new errors.BadRequestError( |
There was a problem hiding this comment.
[💡 maintainability]
Throwing an error when no submission phase is found is correct, but consider logging this event before throwing to aid in debugging and tracing issues in production.
| ([_, phase]) => phase.name === "AI Screening" | ||
| ); | ||
|
|
||
| if (!aiScreeningPhaseDefEntry) { |
There was a problem hiding this comment.
[💡 maintainability]
The error message 'AI Screening phase definition not found in the system' could be more informative by including the context or identifier of the challenge to help with debugging.
|
|
||
| // Recalculate phase dates to keep timeline in sync | ||
| if (submissionPhase.scheduledEndDate) { | ||
| aiScreeningPhase.scheduledStartDate = submissionPhase.scheduledEndDate; |
There was a problem hiding this comment.
[💡 performance]
The use of moment for date manipulation is fine, but consider using native JavaScript Date methods or a lighter library like date-fns if moment's full functionality is not needed, to reduce bundle size.
| @@ -98,9 +100,27 @@ class ChallengePhaseHelper { | |||
| // to incorrectly push earlier phases forward. Sorting by template order prevents that. | |||
| const orderIndex = new Map(); | |||
There was a problem hiding this comment.
[💡 performance]
The variable orderIndex is created using new Map() but is populated using _.each. Consider using native JavaScript methods like forEach for consistency and potentially better performance.
| const submissionPhase = submissionPhaseName | ||
| ? _.find(challengePhases, (phase) => phase.name === submissionPhaseName) | ||
| : null; | ||
| const submissionOrderIndex = _.isNil(submissionPhase) |
There was a problem hiding this comment.
[💡 readability]
The use of _.isNil to check for null or undefined is correct, but consider using submissionPhase === null for clarity since submissionPhase is explicitly set to null when not found.
| return updatedPhase; | ||
| }); | ||
|
|
||
| const aiScreeningPhase = _.find(updatedPhases, (phase) => phase.name === "AI Screening"); |
There was a problem hiding this comment.
[correctness]
The logic for updating the predecessor of aiScreeningPhase and review phases assumes that updateSubmissionPhase is not null. Ensure that this logic is covered by tests to prevent potential issues if updateSubmissionPhase is unexpectedly null.
2 participants
No description provided.