-
Notifications
You must be signed in to change notification settings - Fork 0
syncing dev to master #19
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
PM-1110 - sfdc payment report
PM-1114 report ba fee
Pm 1110 sfdc payment report
fix typo in ba-fees report
Remove @apiquery from controllers to fix swagger docs
…report add exclude billing account filter for sfdc payment report
| exclude: string[]; | ||
| }; | ||
|
|
||
| export function multiValueArrayFilter(filterValue?: 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.
[correctness]
Consider adding a type guard to ensure that filterValue contains only strings. This will prevent runtime errors if non-string values are passed inadvertently.
| export function multiValueArrayFilter(filterValue?: string[]) { | ||
| return (filterValue ?? []).reduce<FiltersSplit>( | ||
| (acc, id) => { | ||
| if (id.startsWith("!")) { |
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.
[❗❗ correctness]
Using id.startsWith("!") assumes that id is always a string. If filterValue can contain non-string values, this will throw an error. Consider adding a check to ensure id is a string before calling startsWith.
| super.debug(this.formatMessages(messages)); | ||
| } | ||
|
|
||
| info(...messages: any[]): void { |
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.
[💡 design]
The info method is using super.log, which is the same as the log method. Consider whether this duplication is necessary or if a different logging level should be used.
| super.error(this.formatMessages(messages)); | ||
| } | ||
|
|
||
| private formatMessages(messages: any[]): 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.
[maintainability]
The formatMessages method uses any[] for the messages parameter. Consider using a more specific type if possible to improve type safety.
| return new Date(); // now | ||
| } | ||
|
|
||
| export function transformArray({ value }: { value: 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.
[❗❗ correctness]
The function transformArray expects value to be a string, but it checks if value is an array. This could lead to unexpected behavior if value is indeed a string, as the check will always return false and wrap the string in an array. Consider revising the type of value or the logic to ensure it handles the expected input correctly.
|
|
||
| enableShutdownHooks(app: INestApplication) { | ||
| process.on("beforeExit", () => { | ||
| app.close().catch(console.error); |
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.
[maintainability]
Consider logging the error explicitly instead of using console.error to ensure that the error is captured in the application's logging system. This will help in debugging and maintaining consistent logging practices.
| ERROR = "error", | ||
| } | ||
|
|
||
| export class Error { |
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.
[maintainability]
Consider renaming the Error class to something more specific like ApiError to avoid potential conflicts with the built-in Error object in JavaScript.
| @ApiProperty({ | ||
| description: "The error message", | ||
| }) | ||
| error: Error; |
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.
[❗❗ correctness]
The error property should be optional or nullable to accurately represent cases where there is no error. Consider using error?: Error or error: Error | null.
| @ApiResponse({ status: 200, description: "Service is healthy." }) | ||
| @ApiResponse({ status: 503, description: "Service is unhealthy." }) | ||
| check() { | ||
| return this.healthService.check(); |
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.
[correctness]
Consider adding error handling for the healthService.check() call to manage potential exceptions or errors that might occur during the health check process. This will improve the robustness of the endpoint.
|
|
||
| async check() { | ||
| // A simple, fast query to check if the database is reachable and responsive. | ||
| await this.db.query("SELECT 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.
[❗❗ correctness]
Consider adding error handling for the database query to ensure that any connection issues or query failures are properly managed and reported. This will improve the robustness of the health check functionality.
| ); | ||
| } | ||
| bootstrap(); | ||
| bootstrap().catch(console.error); |
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.
[maintainability]
Catching errors with console.error is not ideal for production environments as it may not provide sufficient error handling. Consider using a more robust error handling mechanism, such as logging the error to a monitoring service or providing a user-friendly error message.
| @ApiResponse({ | ||
| status: 200, | ||
| description: "Export successful.", | ||
| type: ResponseDto<PaymentsReportResponse>, |
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.
[correctness]
The type ResponseDto<PaymentsReportResponse> is used for the response of the getChallengeHistoryReport method. Ensure that PaymentsReportResponse is the correct type for this report, as it might be misleading if the report does not pertain to payments specifically.
| @ApiResponse({ | ||
| status: 200, | ||
| description: "Export successful.", | ||
| type: ResponseDto<PaymentsReportResponse>, |
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.
[correctness]
Similar to the previous comment, verify that PaymentsReportResponse is the appropriate type for the response of the getRegistrantsHistory method. If the report is not related to payments, consider defining a more suitable response type.
| return results; | ||
| } catch (e) { | ||
| this.logger.error(e); | ||
| } |
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.
[❗❗ correctness]
Consider rethrowing the error or returning a meaningful response in case of an exception. Currently, the function will return undefined if an error occurs, which might lead to unexpected behavior in the calling code.
| filters.completionDateTo, | ||
| ]); | ||
|
|
||
| return payments; |
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.
[❗❗ correctness]
The function does not handle exceptions. Consider adding a try-catch block similar to getSubmissionLinks to log errors and potentially handle them appropriately.
| ], | ||
| ); | ||
|
|
||
| return payments; |
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.
[❗❗ correctness]
The function does not handle exceptions. Consider adding a try-catch block similar to getSubmissionLinks to log errors and potentially handle them appropriately.
| ACTIVE = "ACTIVE", | ||
| COMPLETED = "COMPLETED", | ||
| DELETED = "DELETED", | ||
| CANCELLED = "CANCELLED", |
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.
[maintainability]
Consider consolidating the CANCELLED statuses into a more structured format, such as a nested enum or a separate object, to improve maintainability and readability. This will help manage the growing list of cancellation reasons more effectively.
| example: "2023-01-01T00:00:00.000Z", | ||
| }) | ||
| @IsDateString() | ||
| completionDateFrom?: Date; |
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.
[❗❗ correctness]
The completionDateFrom property is marked as required in the ApiProperty decorator, but it is defined as optional (?) in the TypeScript type. This inconsistency could lead to confusion or errors in API documentation and usage.
| challengeId: string; | ||
| challengeName: string; | ||
| groupNames: string[]; | ||
| challengeStatus: 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.
[maintainability]
The challengeStatus property in ChallengesReportResponseDto is typed as string, but it might be more appropriate to use the ChallengeStatus enum to ensure type safety and consistency with the ChallengesReportQueryDto.
| example: "2023-01-01T00:00:00.000Z", | ||
| }) | ||
| @IsDateString() | ||
| completionDateFrom?: Date; |
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.
[❗❗ correctness]
The completionDateFrom property is marked as required in the ApiProperty decorator, but the TypeScript type is optional (Date?). This inconsistency could lead to confusion or errors when validating the input. Consider aligning the required status in both the decorator and the TypeScript type.
| winnerHandle: string | null; | ||
| challengeCompletedDate: string | null; | ||
| registrantFinalScore?: number; | ||
| challengeStatus: 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.
[maintainability]
The challengeStatus field in ChallengeRegistrantsResponseDto is typed as string, but it seems like it should be of type ChallengeStatus to maintain consistency with the ChallengeRegistrantsQueryDto. Consider using the ChallengeStatus enum to ensure type safety and consistency.
| }) | ||
| @IsOptional() | ||
| @IsDateString() | ||
| @Transform(({ value }) => value || new Date().toISOString()) |
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.
[correctness]
Using new Date().toISOString() as a default value in the @Transform decorator could lead to unexpected behavior if the transformation is applied multiple times, as it will always use the current date and time. Consider whether this is the intended behavior or if a different defaulting strategy should be used.
| @ApiProperty({ | ||
| description: "Challenge ID", | ||
| }) | ||
| challengeId: 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.
[correctness]
The challengeId property is defined as a number, but there is no validation to ensure it is a number. Consider adding validation to ensure the correct type is always provided.
| description: | ||
| "The final score received by the registrant on that submission", | ||
| }) | ||
| registrantFinalScore: 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.
[correctness]
The registrantFinalScore property is defined as a number, but there is no validation to ensure it is a number. Consider adding validation to ensure the correct type is always provided.
| description: | ||
| "This denotes if the submission has passed. If the final score is greater than 98 then its considered pass", | ||
| }) | ||
| didSubmissionPass: boolean; |
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.
[correctness]
The logic for determining didSubmissionPass is not included here, but the description suggests a threshold of 98. Ensure that this logic is implemented consistently wherever this DTO is used, as discrepancies could lead to incorrect data representation.
| @ApiProperty({ | ||
| description: "Submission ID", | ||
| }) | ||
| submissionId: 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.
[correctness]
The submissionId property is defined as a number, but there is no validation to ensure it is a number. Consider adding validation to ensure the correct type is always provided.
| @ApiBearerAuth() | ||
| @ApiOperation({ | ||
| summary: "SFDC Payments report", | ||
| description: "", |
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.
[💡 readability]
Consider providing a detailed description for the API operation to improve the documentation clarity. This can help consumers of the API understand its purpose and usage better.
| @ApiBearerAuth() | ||
| @ApiOperation({ | ||
| summary: "Report of BA to fee / member payment", | ||
| description: "", |
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.
[💡 readability]
Consider providing a detailed description for the API operation to improve the documentation clarity. This can help consumers of the API understand its purpose and usage better.
| }) | ||
| @IsOptional() | ||
| @IsNumber() | ||
| @Transform(({ value }) => parseFloat(value)) |
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.
[correctness]
Using parseFloat in the @Transform decorator can lead to unexpected results if the input is not a valid number string. Consider validating the input format before transforming it to ensure it is a valid number.
| }) | ||
| @IsOptional() | ||
| @IsNumber() | ||
| @Transform(({ value }) => parseFloat(value)) |
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.
[correctness]
Using parseFloat in the @Transform decorator can lead to unexpected results if the input is not a valid number string. Consider validating the input format before transforming it to ensure it is a valid number.
| example: "2023-01-01T00:00:00.000Z", | ||
| }) | ||
| @IsDateString() | ||
| startDate?: Date; |
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.
[❗❗ correctness]
The startDate property in BaFeesReportQueryDto is marked as optional but does not have the @IsOptional() decorator. This could lead to validation issues if the property is not provided. Consider adding @IsOptional() to ensure consistent behavior.
| multiValueArrayFilter(filters.billingAccountIds); | ||
|
|
||
| const payments = await this.db.query<PaymentsReportResponse>(query, [ | ||
| billingAccountIds.length ? billingAccountIds : undefined, |
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.
[correctness]
Consider handling the case where billingAccountIds or excludeBillingAccountIds is an empty array more explicitly. Returning undefined might lead to unexpected behavior in the SQL query execution if not handled properly in the SQL script.
|
|
||
| const payments = await this.db.query<PaymentsReportResponse>(query, [ | ||
| billingAccountIds.length ? billingAccountIds : undefined, | ||
| excludeBillingAccountIds.length ? excludeBillingAccountIds : undefined, |
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.
[correctness]
Consider handling the case where excludeBillingAccountIds is an empty array more explicitly. Returning undefined might lead to unexpected behavior in the SQL query execution if not handled properly in the SQL script.
| challengeRegistrantHandle: string; | ||
|
|
||
| @ApiProperty({ description: "Email of the user" }) | ||
| userEmail: 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.
[correctness]
Consider validating the userEmail field to ensure it contains a valid email address format. This can help prevent invalid data from being processed.
| challengeId: number; | ||
| @ApiProperty({ description: "Did registrant submitted?" }) | ||
| didRegistrantSubmit: boolean; | ||
| @ApiProperty({ description: "Hanlde of the winner" }) |
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.
[💡 readability]
Typo in the description: 'Hanlde' should be 'Handle'.
| registrantHandle: string; | ||
| @ApiProperty({ description: "Challenge Id" }) | ||
| challengeId: number; | ||
| @ApiProperty({ description: "Did registrant submitted?" }) |
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.
[💡 readability]
The description 'Did registrant submitted?' is grammatically incorrect. Consider changing it to 'Did the registrant submit?' for clarity.
| @ApiProperty({ description: "Hanlde of the winner" }) | ||
| winnerHandle: string | null; | ||
| @ApiProperty({ description: "Challenge completion date" }) | ||
| challengeCompletionDate: string | 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.
[correctness]
Consider using a date type (e.g., Date) for challengeCompletionDate instead of string to ensure proper date handling and validation.
| @ApiProperty({ description: "Registrant email id" }) | ||
| registrantEmail: string | null; | ||
| @ApiProperty({ description: "Project scheduled end date" }) | ||
| projectScheduledEndDate: string | 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.
[correctness]
Consider using a date type (e.g., Date) for projectScheduledEndDate instead of string to ensure proper date handling and validation.
| startDate?: string; | ||
| endDate?: string; | ||
| }): Promise<ChallengeStatsByUserDto[]> { | ||
| const startDate = opts.startDate |
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.
[maintainability]
Consider using parseOptionalDate for opts.startDate and opts.endDate to ensure consistent date parsing and error handling across all methods.
| } | ||
|
|
||
| const query = this.sql.load("reports/topgear/challenge-stats-by-user.sql"); | ||
| return this.db.query<ChallengeStatsByUserDto>(query, [startDate, endDate]); |
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.
[❗❗ security]
Ensure that the SQL query in challenge-stats-by-user.sql is parameterized properly to prevent SQL injection, especially since the dates are passed as parameters.
| } | ||
|
|
||
| const query = this.sql.load("reports/topgear/payments.sql"); | ||
| // Postgres parameter placeholders: $1, $2 |
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.
[💡 style]
The comment about Postgres parameter placeholders seems unnecessary here as it's not providing additional context or clarity. Consider removing it unless it serves a specific purpose.
| throw new BadRequestException("start_date must be <= end_date"); | ||
| } | ||
|
|
||
| const query = this.sql.load("reports/topgear/challenge.sql"); |
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.
[❗❗ security]
Ensure that the SQL query in challenge.sql is parameterized properly to prevent SQL injection, especially since the dates are passed as parameters.
| throw new BadRequestException("start_date must be <= end_date"); | ||
| } | ||
|
|
||
| const query = this.sql.load("reports/topgear/cancelled-challenge.sql"); |
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.
[❗❗ security]
Ensure that the SQL query in cancelled-challenge.sql is parameterized properly to prevent SQL injection, especially since the dates are passed as parameters.
| }>(q); | ||
| return rows.map((row) => { | ||
| const countryName = | ||
| alpha3ToCountryName(row.country_code) ?? row.country_code ?? ""; |
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.
[correctness]
The function alpha3ToCountryName is used to convert country codes to names. Ensure that this function handles all possible edge cases, such as invalid or unexpected country codes, to prevent potential issues with incorrect data representation.
| alpha3ToCountryName(row.country_code) ?? row.country_code ?? ""; | ||
| return { | ||
| "country.country_name": countryName, | ||
| "user.count": Number(row["user.count"] ?? 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.
[correctness]
The conversion of user.count to a number using Number(row['user.count'] ?? 0) assumes that any non-numeric value should default to 0. Consider validating the input to ensure it is a numeric string before conversion to avoid silently handling unexpected data.
| return { | ||
| "country.country_name": countryName, | ||
| "user.count": Number(row["user.count"] ?? 0), | ||
| rank: row.rank !== null && row.rank !== undefined ? Number(row.rank) : 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.
[correctness]
The conversion of rank to a number using Number(row.rank) assumes that rank is always a valid numeric string when not null or undefined. Consider adding validation to ensure the input is a numeric string to prevent potential conversion errors.
| const q = this.sql.load( | ||
| "reports/statistics/development/development-countries-represented.sql", | ||
| ); | ||
| const rows = await this.db.query<{ |
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.
[correctness]
Consider adding error handling for the database query to manage potential exceptions or errors that might occur during the execution of the query. This will improve the robustness of the method.
| rank: number | string | null; | ||
| }>(q); | ||
| return rows.map((row) => { | ||
| const countryName = |
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.
[correctness]
The use of alpha3ToCountryName assumes that the country_code is always a valid alpha-3 code. Consider handling cases where alpha3ToCountryName might return undefined due to an invalid or unexpected country_code.
| async getTotalPrizes() { | ||
| const q = this.sql.load("reports/statistics/general/total-prizes.sql"); | ||
| const rows = await this.db.query<{ total: string }>(q); | ||
| const total = rows?.[0]?.total ? rows[0].total : "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.
[correctness]
Consider converting total to a number before returning it, similar to how count is handled in getMemberCount. This ensures consistent data types and prevents potential issues when the total is used in calculations.
| alpha3ToCountryName(row.country_code) ?? row.country_code ?? ""; | ||
| return { | ||
| "country.country_name": countryName, | ||
| "user.count": Number(row["user.count"] ?? 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.
[correctness]
Ensure that the conversion of user.count to a number is safe and expected. If row["user.count"] is not a valid number string, Number() will return NaN, which might not be the desired behavior.
| alpha3ToCountryName(row.country_code) ?? row.country_code ?? ""; | ||
| return { | ||
| "country.country_name": countryName, | ||
| "challenge_stats.count": Number(row["challenge_stats.count"] ?? 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.
[correctness]
Similar to the conversion in getCountriesRepresented, ensure that converting challenge_stats.count to a number is safe. If row["challenge_stats.count"] is not a valid number string, Number() will return NaN, which might not be the desired behavior.
|
|
||
| private loadJson<T = any>(fileName: string): T { | ||
| const fullPath = path.join(this.baseDir, fileName); | ||
| const raw = fs.readFileSync(fullPath, "utf-8"); |
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.
[❗❗ correctness]
Consider handling potential errors when reading the file with fs.readFileSync. If the file does not exist or there is a read error, it will throw an exception. Adding try-catch blocks or validating the file's existence before reading can improve robustness.
| private loadJson<T = any>(fileName: string): T { | ||
| const fullPath = path.join(this.baseDir, fileName); | ||
| const raw = fs.readFileSync(fullPath, "utf-8"); | ||
| return JSON.parse(raw) as T; |
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.
[❗❗ correctness]
Consider handling potential errors when parsing JSON with JSON.parse. If the file content is not valid JSON, it will throw an exception. Adding try-catch blocks or validating the JSON format before parsing can improve robustness.
| ) {} | ||
|
|
||
| async getWins() { | ||
| const q = this.sql.load("reports/statistics/qa/wins.sql"); |
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.
[correctness]
Consider adding error handling for the SQL query loading process to ensure that any issues with loading the SQL file are caught and handled appropriately.
|
|
||
| async getWins() { | ||
| const q = this.sql.load("reports/statistics/qa/wins.sql"); | ||
| return this.db.query(q); |
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.
[correctness]
Consider adding error handling for the database query execution to manage potential database errors and improve the robustness of the service.
|
|
||
| private loadJson<T = any>(fileName: string): T { | ||
| const fullPath = path.join(this.baseDir, fileName); | ||
| const raw = fs.readFileSync(fullPath, "utf-8"); |
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.
[❗❗ correctness]
Consider adding error handling for fs.readFileSync. If the file does not exist or cannot be read, it will throw an exception, which could crash the application. Implementing try-catch or using fs.promises with async/await could improve robustness.
| private loadJson<T = any>(fileName: string): T { | ||
| const fullPath = path.join(this.baseDir, fileName); | ||
| const raw = fs.readFileSync(fullPath, "utf-8"); | ||
| return JSON.parse(raw) as T; |
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.
[❗❗ correctness]
Consider adding error handling for JSON.parse. If the file content is not valid JSON, it will throw an exception, potentially crashing the application. Implementing try-catch around this operation could improve error resilience.
| export class SrmDataService { | ||
| private baseDir = path.resolve(process.cwd(), "data/statistics/srm"); | ||
|
|
||
| private loadJson<T = any>(fileName: string): T { |
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.
[maintainability]
Using a generic type T = any for loadJson can lead to type safety issues. Consider specifying a more precise type or interface for the expected JSON structure to improve type safety and maintainability.
| @ApiTags("Statistics") | ||
| @Controller("/statistics/development") | ||
| export class StatisticsDevelopmentController { | ||
| constructor(private readonly development: DevelopmentStatisticsService) {} |
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.
[💡 readability]
Consider renaming the development variable to developmentStatisticsService for improved clarity and maintainability. This will make it more apparent that this variable is an instance of DevelopmentStatisticsService.
| @ApiTags("Statistics") | ||
| @Controller("/statistics/general") | ||
| export class StatisticsGeneralController { | ||
| constructor(private readonly general: GeneralStatisticsService) {} |
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.
[💡 readability]
Consider renaming the general variable to something more descriptive, like generalStatisticsService, to improve clarity and maintainability.
| @ApiTags("Statistics") | ||
| @Controller("/statistics/qa") | ||
| export class StatisticsQaController { | ||
| constructor(private readonly qa: QaStatisticsService) {} |
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.
[💡 readability]
Consider renaming the qa parameter to something more descriptive like qaStatisticsService to improve readability and clarity.
| @Get("/wins") | ||
| @ApiOperation({ summary: "Quality Assurance challenge wins by member (desc)" }) | ||
| getQaWins() { | ||
| return this.qa.getWins(); |
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.
[correctness]
Ensure that getWins() method in QaStatisticsService handles potential errors or exceptions gracefully, especially if it involves database operations or external service calls.
| @Get("/srm/top-rated") | ||
| @ApiOperation({ summary: "Highest rated SRMs (static)" }) | ||
| getSrmTopRated() { | ||
| return this.srm.getTopRated(); |
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.
[correctness]
Consider adding error handling for the service call this.srm.getTopRated(). If the service fails, it might result in an unhandled exception and could impact the stability of the application.
| @Get("/srm/country-ratings") | ||
| @ApiOperation({ summary: "SRM country ratings (static)" }) | ||
| getSrmCountryRatings() { | ||
| return this.srm.getCountryRatings(); |
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.
[correctness]
Consider adding error handling for the service call this.srm.getCountryRatings(). This will help manage potential service failures gracefully.
| @Get("/srm/competitions-count") | ||
| @ApiOperation({ summary: "SRM number of competitions (static)" }) | ||
| getSrmCompetitionsCount() { | ||
| return this.srm.getCompetitionsCount(); |
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.
[correctness]
Consider adding error handling for the service call this.srm.getCompetitionsCount(). This will improve the robustness of the endpoint.
| @Get("/mm/top-rated") | ||
| @ApiOperation({ summary: "Highest rated Marathon Matches (static)" }) | ||
| getMmTopRated() { | ||
| return this.mm.getTopRated(); |
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.
[correctness]
Consider adding error handling for the service call this.mm.getTopRated(). This will ensure that any service errors are handled appropriately.
| @Get("/mm/country-ratings") | ||
| @ApiOperation({ summary: "Marathon Match country ratings (static)" }) | ||
| getMmCountryRatings() { | ||
| return this.mm.getCountryRatings(); |
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.
[correctness]
Consider adding error handling for the service call this.mm.getCountryRatings(). This will help prevent unhandled exceptions in case of service failures.
| @Get("/mm/top-10-finishes") | ||
| @ApiOperation({ summary: "Marathon Match Top 10 finishes (static)" }) | ||
| getMmTop10Finishes() { | ||
| return this.mm.getTop10Finishes(); |
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.
[correctness]
Consider adding error handling for the service call this.mm.getTop10Finishes(). This will improve the stability of the application by handling potential errors.
| @Get("/mm/competitions-count") | ||
| @ApiOperation({ summary: "Marathon Match number of competitions (static)" }) | ||
| getMmCompetitionsCount() { | ||
| return this.mm.getCompetitionsCount(); |
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.
[correctness]
Consider adding error handling for the service call this.mm.getCompetitionsCount(). This will help manage potential errors from the service call.
No description provided.