-
Notifications
You must be signed in to change notification settings - Fork 8
Add endpoint to get the review progress of a challenge #39
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
typeof challengeId !== 'string' || | ||
challengeId.trim() === '' | ||
) { | ||
throw new Error('Invalid challengeId parameter'); |
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 specific exception class like BadRequestException
instead of throwing a generic Error
for invalid challengeId
to provide more context and consistency with other error handling.
error, | ||
); | ||
|
||
if (error.message === 'Invalid challengeId parameter') { |
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 here could be improved by using specific exception classes instead of re-throwing a generic Error
. For example, use BadRequestException
or NotFoundException
directly when catching specific errors.
|
||
// Handle Resource API errors based on HTTP status codes | ||
if (error.message === 'Cannot get data from Resource API.') { | ||
const statusCode = (error as Error & { statusCode?: number }) |
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 statusCode
is undefined before comparing it to avoid potential runtime errors if statusCode
is not present in the error object.
} | ||
} | ||
|
||
if (error.message && error.message.includes('not found')) { |
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 check error.message.includes('not found')
might be too broad and could potentially catch unintended errors. Consider refining the condition or using a more specific error handling mechanism.
description: 'Total number of reviewers for the challenge', | ||
example: 2, | ||
}) | ||
@IsNumber() |
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 validation decorator such as @IsInt()
to ensure that totalReviewers
is an integer, as fractional reviewers do not make sense.
description: 'Total number of submissions for the challenge', | ||
example: 4, | ||
}) | ||
@IsNumber() |
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 validation decorator such as @IsInt()
to ensure that totalSubmissions
is an integer, as fractional submissions do not make sense.
description: 'Total number of submitted reviews', | ||
example: 6, | ||
}) | ||
@IsNumber() |
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 validation decorator such as @IsInt()
to ensure that totalSubmittedReviews
is an integer, as fractional reviews do not make sense.
example: 75.0, | ||
}) | ||
@IsNumber() | ||
progressPercentage: number; |
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 validation decorator such as @IsPositive()
to ensure that progressPercentage
is a positive number, as negative progress does not make sense.
@@ -79,7 +79,10 @@ export class ResourceApiService { | |||
} catch (e) { | |||
if (e instanceof AxiosError) { | |||
this.logger.error(`Http Error: ${e.message}`, e.response?.data); | |||
throw new Error('Cannot get data from Resource API.'); | |||
const error = new Error('Cannot get data from Resource API.'); | |||
(error as any).statusCode = e.response?.status; |
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 specific type instead of any
for the error
object to improve type safety and maintainability.
throw new Error('Cannot get data from Resource API.'); | ||
const error = new Error('Cannot get data from Resource API.'); | ||
(error as any).statusCode = e.response?.status; | ||
(error as any).originalMessage = e.response?.data?.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.
Consider using a more specific type instead of any
for the error
object to improve type safety and maintainability.
http://www.topcoder.com/challenge-details/30377831/?type=develop