Skip to content

Conversation

billsedison
Copy link
Collaborator

@billsedison billsedison commented Jul 31, 2025

http://www.topcoder.com/challenge-details/30377549/?type=develop

  • Fix TypeScript Lint Errors
  • Remove /api Prefix from All Endpoints

@billsedison billsedison requested a review from kkartunov July 31, 2025 15:36

// Global submission map to store submission information.
let submissionMap: Record<string, Record<string, string>> = {};
const submissionMap: Record<string, Record<string, string>> = {};

Choose a reason for hiding this comment

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

Consider reviewing other variables in the file to ensure consistent use of const where appropriate. Changing let to const is a good practice when the variable is not reassigned, but consistency across similar variables can improve readability and maintainability.

* Read submission data from resource_xxx.json, upload_xxx.json and submission_xxx.json.
*/
async function initSubmissionMap() {
function initSubmissionMap() {

Choose a reason for hiding this comment

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

The function initSubmissionMap was changed from async to a regular function. Ensure that there are no asynchronous operations within this function that require the use of await. If there are, the function should remain async to handle promises correctly.

console.log(`Reading submission data from ${f}`);
const jsonData = readJson(filePath)['submission'];
for (let d of jsonData) {
for (const d of jsonData) {

Choose a reason for hiding this comment

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

Consider using const for the variable jsonData on line 315 since its value is not reassigned.

const jsonData = readJson(filePath)['upload'];
for (let d of jsonData) {
if (d['upload_status_id'] === '1' && d['upload_type_id'] === '1' && d['resource_id']) {
for (const d of jsonData) {

Choose a reason for hiding this comment

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

Consider using const instead of let for d since it is not reassigned within the loop.

) {
// get submission info
uploadCount += 1
uploadCount += 1;

Choose a reason for hiding this comment

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

Ensure consistent use of semicolons throughout the code. A semicolon was added here, so verify other parts of the code for consistency.

}
}
console.log(`Read resource count: ${resourceCount}, submission resource count: ${validResourceCount}`);
console.log(

Choose a reason for hiding this comment

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

The console.log statement has been split into multiple lines. Consider keeping it on a single line for better readability unless it exceeds the maximum line length.

// init resource-submission data
console.log('Starting resource/submission import...');
await initSubmissionMap();
initSubmissionMap();

Choose a reason for hiding this comment

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

The await keyword has been removed from the initSubmissionMap() call. If initSubmissionMap() is an asynchronous function and its completion is necessary before proceeding, consider adding await back to ensure the function completes before moving on to the next steps.

code: errorResponse.code
throw new NotFoundException({
message: `Appeal with ID ${appealId} was not found`,
code: errorResponse.code,

Choose a reason for hiding this comment

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

Trailing comma is unnecessary in the object literal. Consider removing it for consistency with the rest of the code style.

code: errorResponse.code
throw new NotFoundException({
message: `Appeal with ID ${appealId} was not found`,
code: errorResponse.code,

Choose a reason for hiding this comment

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

Consider removing the trailing comma after errorResponse.code to adhere to standard JavaScript object syntax.

const whereClause: any = {};
if (resourceId) whereClause.resourceId = resourceId;
if (challengeId) whereClause.challengeId = challengeId;
if (reviewId) whereClause.appealId = reviewId;

Choose a reason for hiding this comment

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

The condition if (reviewId) whereClause.appealId = reviewId; seems to be setting appealId instead of reviewId. Verify if this is intentional or if it should be whereClause.reviewId = reviewId;.

where: whereClause,
skip,
take: perPage,
}),

Choose a reason for hiding this comment

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

The indentation of the closing bracket on line 345 should match the opening bracket on line 326 for better readability.

this.prisma.appealResponse.count({
where: whereClause,
})
}),

Choose a reason for hiding this comment

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

The indentation of the closing bracket on line 348 should match the opening bracket on line 346 for consistency.

@ApiTags('Contact Requests')
@ApiBearerAuth()
@Controller('/api')
@Controller('/')

Choose a reason for hiding this comment

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

Changing the controller path from /api to / might affect the routing of the application. Ensure that this change is intentional and that all routes are correctly updated to reflect this new path.


@ApiTags('Healthcheck')
@Controller('/api')
@Controller('/')

Choose a reason for hiding this comment

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

The change from @Controller('/api') to @Controller('/') alters the base path for the health check endpoint. Ensure that this change aligns with the intended routing structure and does not inadvertently affect other endpoints or the application's routing logic.

@ApiTags('ProjectResult')
@ApiBearerAuth()
@Controller('/api')
@Controller('/')

Choose a reason for hiding this comment

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

The change from @Controller('/api') to @Controller('/') alters the base path for the controller. Ensure that this change is intentional and that all routes are updated accordingly in the application to prevent routing issues.

where: { challengeId },
skip,
take: perPage,
}),

Choose a reason for hiding this comment

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

Consider aligning the indentation of the this.prisma.challengeResult.count call with the surrounding code for better readability.

this.prisma.challengeResult.count({
where: { challengeId },
})
}),

Choose a reason for hiding this comment

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

Ensure consistent use of trailing commas in multi-line array and object literals for better diff readability and to adhere to common linting rules.


@ApiTags('Review Application')
@Controller('/api/review-applications')
@Controller('/review-applications')

Choose a reason for hiding this comment

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

The path change from /api/review-applications to /review-applications might affect existing API consumers. Ensure that this change is intentional and that any necessary updates to documentation or client code are made to reflect this change.

},
});
if (existing && existing.length > 0) {
throw new ConflictException('Reviewer has submitted application before.');

Choose a reason for hiding this comment

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

Consider using double quotes for the string in ConflictException to maintain consistency with the other exception messages.

}
}
gte: beginDate,
},

Choose a reason for hiding this comment

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

Trailing comma added here. Ensure this is consistent with the project's linting rules. If the rules require trailing commas, this is correct; otherwise, it should be removed.

*/
private async sendEmails(entityList, status: ReviewApplicationStatus) {
private async sendEmails(
entityList: any[],

Choose a reason for hiding this comment

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

Consider specifying a more precise type for entityList instead of using any[] to improve type safety.

const challengeId = entityList[0].opportunity.challengeId;
// get member id list
const userIds = entityList.map(e => e.userId);
const userIds: string[] = entityList.map((e: any) => e.userId as string);

Choose a reason for hiding this comment

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

The type assertion as string is used here. Ensure that userId is always a string to avoid runtime errors.

const payload:EventBusSendEmailPayload = new EventBusSendEmailPayload();
for (const entity of entityList) {
const payload: EventBusSendEmailPayload = new EventBusSendEmailPayload();
payload.sendgrid_template_id = sendgridTemplateId;

Choose a reason for hiding this comment

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

Consider checking if userEmailMap.get(entity.userId) returns a valid email before adding it to payload.recipients to avoid sending emails to undefined addresses.


@ApiTags('Review History')
@Controller('/api/review-history')
@Controller('/review-history')

Choose a reason for hiding this comment

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

The change from '/api/review-history' to '/review-history' in the @Controller decorator modifies the endpoint path. Ensure this change is intentional and aligns with the application's routing requirements.

if (authUser.userId !== userId && !isAdmin(authUser)) {
throw new ForbiddenException('You cannot check this user\'s review history')
throw new ForbiddenException(
"You cannot check this user's review history",

Choose a reason for hiding this comment

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

Consider using single quotes for consistency, as the rest of the code uses single quotes for strings.


@ApiTags('Review Opportunity')
@Controller('/api/review-opportunities')
@Controller('/review-opportunities')

Choose a reason for hiding this comment

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

The change in the path from /api/review-opportunities to /review-opportunities might affect the routing if other parts of the application or external clients are expecting the original path. Ensure that this change is intentional and that all necessary updates are made to accommodate this new path.

} catch (e) {
// challenge doesn't exist. Return 400
this.logger.error('Can\'t get challenge:', e)
this.logger.error("Can't get challenge:", e);

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 with modern JavaScript practices: this.logger.error(\Can't get challenge: ${e}`);`

if (existing && existing.length > 0) {
throw new ConflictException('Review opportunity exists for challenge and type');
throw new ConflictException(
'Review opportunity exists for challenge and type',

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 with modern JavaScript practices: throw new ConflictException(\Review opportunity exists for challenge and type`);`

*/
private async assembleList(
entityList,
entityList: any[],

Choose a reason for hiding this comment

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

Consider specifying a more precise type for entityList instead of using any[]. This will improve type safety and maintainability.

// get challenge id and remove duplicated
const challengeIdList = [...new Set(entityList.map((e) => e.challengeId))];
const challengeIdList: string[] = [
...new Set(entityList.map((e: any) => e.challengeId as string)),

Choose a reason for hiding this comment

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

The use of any for the map function parameter e could be replaced with a more specific type to enhance type safety.

const challengeList =
await this.challengeService.getChallenges(challengeIdList);
// build challenge id -> challenge data map
const challengeMap = new Map();

Choose a reason for hiding this comment

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

Consider specifying the type for challengeMap to improve type safety and readability.

*/
private buildResponse(
entity,
entity: any,

Choose a reason for hiding this comment

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

Consider specifying a more precise type instead of any for the entity parameter to improve type safety and maintainability.

}));
}

Choose a reason for hiding this comment

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

Unnecessary whitespace change. Consider reverting to maintain consistency with surrounding code.

@ApiTags('Reviews')
@ApiBearerAuth()
@Controller('/api/reviews')
@Controller('/reviews')

Choose a reason for hiding this comment

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

The change in the path from /api/reviews to /reviews may affect existing API clients that rely on the previous endpoint. Ensure that this change is intentional and that any necessary updates to client applications or documentation are made to reflect this new path.

},
});

Choose a reason for hiding this comment

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

Remove unnecessary whitespace at the end of the line.


reviews = scorecards.flatMap((d) => d.reviews);

Choose a reason for hiding this comment

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

Remove unnecessary whitespace at the end of the line.


const submissionIds = challengeResults.map(c => c.submissionId);
const submissionIds = challengeResults.map((c) => c.submissionId);

Choose a reason for hiding this comment

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

Remove the unnecessary space after the opening parenthesis in the arrow function parameter to maintain consistent formatting.

include: {
reviewItemComments: true,
},
},

Choose a reason for hiding this comment

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

Unnecessary whitespace change. Consider reverting to maintain consistency with the surrounding code.

},
});

Choose a reason for hiding this comment

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

Unnecessary whitespace change. Consider reverting to maintain consistency with the surrounding code.

@ApiTags('Scorecard')
@ApiBearerAuth()
@Controller('/api/scorecards')
@Controller('/scorecards')

Choose a reason for hiding this comment

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

The change from /api/scorecards to /scorecards might affect existing API clients expecting the previous path. Ensure that this change is intentional and update any relevant documentation or client code accordingly.

})
@IsNumber()
@IsPositive()
incrementalPayment: number;

Choose a reason for hiding this comment

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

The OmitType function call is missing a closing parenthesis. Ensure that the syntax is correct and all parentheses are properly closed.

// Intercept response to log it
const originalSend = res.send;
res.send = function(body) {
res.send = function (body: any): Response {

Choose a reason for hiding this comment

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

Consider specifying a more precise type for body instead of using any to improve type safety.

return originalSend.call(this, body);

return originalSend.call(this, body) as Response;

Choose a reason for hiding this comment

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

The type assertion as Response might be unnecessary if the originalSend method already returns a Response type. Consider verifying the return type of originalSend to ensure this type assertion is needed.

--data '{"client_id":"your-client-id","client_secret":"your-client-secret","audience":"https://m2m.topcoder-dev.com/","grant_type":"client_credentials"}'
`)
`,

Choose a reason for hiding this comment

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

The trailing comma after the template literal might not be necessary here. Ensure that this is intentional and does not affect the functionality.

process.on('unhandledRejection', (reason, promise) => {
logger.error(`Unhandled Promise Rejection at: ${promise}`, reason as string);
process.on('unhandledRejection', (reason) => {
logger.error('Unhandled Promise Rejection', reason as string);

Choose a reason for hiding this comment

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

Consider providing more context in the log message for unhandled promise rejections. Including the promise information might be helpful for debugging.

ignoreExpiration: process.env.NODE_ENV !== 'production',
},
};
};

Choose a reason for hiding this comment

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

Ensure there is a newline at the end of the file to comply with POSIX standards and avoid potential issues with version control systems.

],
'm2m-token-project-result': [
Scope.AllProjectResult
Scope.AllScorecard,

Choose a reason for hiding this comment

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

Consider adding a trailing comma after Scope.AllScorecard for consistency with the rest of the array elements.

return { scopes, isMachine: false };
}

let decodedToken: any;

Choose a reason for hiding this comment

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

Remove the unused variable decodedToken if it is not used elsewhere in the code.

decodedToken = verify(token, signingKey, verifyOptions);

} catch (error) {
console.error('JWT verification failed:', error);

Choose a reason for hiding this comment

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

Avoid using console.error for logging errors in production code. Consider using a logging library that can be configured for different environments.

decodedToken = decode(token);
}

Choose a reason for hiding this comment

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

Remove unnecessary whitespace change.

}
if (key.endsWith('roles')) {
user.roles = decodedToken[key] as UserRole[]
user.roles = decodedToken[key] as UserRole[];

Choose a reason for hiding this comment

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

Ensure consistent use of semicolons throughout the codebase. The semicolon added here is correct, but verify that similar statements in the file also follow this convention.

try {
logMessage = JSON.stringify(message);
} catch (e) {
} catch {

Choose a reason for hiding this comment

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

Consider specifying the error variable in the catch block for better error handling and debugging. For example, use catch (error) { instead of just catch {.

? error.meta.target.join(', ')
: typeof error.meta.target === 'string'
? error.meta.target
: JSON.stringify(error.meta.target)

Choose a reason for hiding this comment

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

Consider using JSON.stringify only when error.meta.target is an object, as using it on non-object types might lead to unexpected results. Ensure that error.meta.target is not a primitive type before applying JSON.stringify.

await this.$connect();
this.logger.log('Prisma connected successfully');

Choose a reason for hiding this comment

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

Remove the unnecessary blank line to maintain code consistency and adhere to linting rules.

@kkartunov kkartunov merged commit 8d33cec into develop Aug 1, 2025
1 check passed
@kkartunov kkartunov deleted the fix/lint-error-path branch August 6, 2025 05:14
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