Review Opportunities #6
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| M2M_AUTH_DOMAIN="topcoder-dev.auth0.com" | ||
| M2M_AUTH_AUDIENCE="https://m2m.topcoder-dev.com/" | ||
| M2M_AUTH_PROXY_SEREVR_URL= |
There was a problem hiding this comment.
The variable M2M_AUTH_PROXY_SEREVR_URL seems to have a typo in its name. It should likely be M2M_AUTH_PROXY_SERVER_URL.
There was a problem hiding this comment.
@billsedison - Let's fix this thanks.
| res.json({}) | ||
| }) | ||
|
|
||
| const m2mToken = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL3RvcGNvZGVyLWRldi5hdXRoMC5jb20vIiwic3ViIjoiakdJZjJwZDNmNDRCMWpxdk9haTMwQklLVFphbllCZlVAY2xpZW50cyIsImF1ZCI6Imh0dHBzOi8vbTJtLnRvcGNvZGVyLWRldi5jb20vIiwiaWF0IjoxNzQ4MDk5NDk4LCJleHAiOjE4NDgxODU4OTgsInNjb3BlIjoid3JpdGU6YnVzX2FwaSxhbGw6Y2hhbGxlbmdlcyIsImd0eSI6ImNsaWVudC1jcmVkZW50aWFscyIsImF6cCI6ImpHSWYycGQzZjQ0QjFqcXZPYWkzMEJJS1RaYW5ZQmZVIn0.h3ksdsdJm5USGF1VgROrpkTtStmCzv5ZA6y8bd8AnGY'; |
There was a problem hiding this comment.
Storing sensitive information such as tokens directly in the code is not secure. Consider using environment variables or a secure vault to manage sensitive data.
There was a problem hiding this comment.
@billsedison - We can't put these into any code, even if they are long since expired, due to security headaches they cause us in Wipro scans. Let's remove this and make it an env variable.
| // Event bus | ||
| app.post('/eventBus', (req, res) => { | ||
| logger.info(`Event Bus received message: ${JSON.stringify(req.body)}`); | ||
| res.statusCode = 200; |
There was a problem hiding this comment.
Instead of setting res.statusCode = 200;, use res.status(200); for consistency and readability.
| app.get('/members', (req, res) => { | ||
| logger.info(`Member API receives params: ${JSON.stringify(req.query)}`) | ||
| let userIdStr = req.query.userIds | ||
| userIdStr = userIdStr.replaceAll('[', '').replaceAll(']', '') |
There was a problem hiding this comment.
Using replaceAll might not be supported in all environments. Consider using a more compatible method such as replace with a regular expression.
| // directly challenge details | ||
| const id = req.params.id | ||
| logger.info(`Getting challenge with id ${id}`) | ||
| if (id === '11111111-2222-3333-9999-444444444444') { |
There was a problem hiding this comment.
Consider using res.status(404).json({}); for setting the status and sending the response in a single chain for better readability.
| '@types/[email protected]': | ||
| resolution: {integrity: sha512-N4LZ2xG7DatVqhCZzOGb1Yi5lMbXSZcmdLDe9EzSndPV2HpWYWzRbaerl2n27irrm94EPpprqa8KpskPT085+A==} | ||
|
|
||
| '@types/[email protected]': | ||
| resolution: {integrity: sha512-3xhRnjJPkULekpSzgtoNYYcTWgEZkp4myc+Saevii5JPnHNvHMRlBSHDbs7Bh1iPPoVTERHEZXyhyLbMEsExsA==} | ||
|
|
||
| '@types/[email protected]': | ||
| resolution: {integrity: sha512-iJbM7nsyBgnxCrCe7VjWIi4nyyhlaKUl7jxeHDpK+KXk3sYrUZViMkgFv9qSZmxDleB8dfpQR9gK5MGNyM/M6w==} | ||
| deprecated: This is a stub types definition. express-unless provides its own type definitions, so you do not need this installed. |
There was a problem hiding this comment.
The entry for @types/[email protected] indicates that it is deprecated and not needed because express-unless provides its own type definitions. Consider removing this dependency to avoid unnecessary installations and potential conflicts.
| @@ -1585,6 +1652,11 @@ packages: | |||
| [email protected]: | |||
| resolution: {integrity: sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ==} | |||
|
|
|||
| [email protected]: | |||
| resolution: {integrity: sha512-0tECWShh6wUysgucJcBAoYegf3JJoZWibxdqhTm7OHPeT42qdjkZ29QCMcKwbgU1kiH+auSIasNRXMLWXafXig==} | |||
| engines: {'0': node >=0.10.0} | |||
There was a problem hiding this comment.
The engines field uses an unconventional format with a numeric key '0'. Typically, this field should directly specify the engine requirements without a numeric key. Consider using engines: {node: '>=0.10.0'} for consistency and clarity.
| @@ -2246,6 +2369,10 @@ packages: | |||
| engines: {node: 20 || >=22} | |||
| hasBin: true | |||
|
|
|||
| [email protected]: | |||
There was a problem hiding this comment.
The addition of [email protected] is using a deprecated version. Consider updating to a supported version of the glob package to avoid potential issues with unsupported versions.
| @@ -2639,13 +2780,20 @@ packages: | |||
| [email protected]: | |||
| resolution: {integrity: sha512-5dgndWOriYSm5cnYaJNhalLNDKOqFwyDB/rr1E9ZsGciGvKPs8R2xYGCacuf3z6K1YKDz182fd+fY3cn3pMqXQ==} | |||
|
|
|||
| [email protected]: | |||
There was a problem hiding this comment.
The addition of [email protected] alongside [email protected] could lead to potential conflicts or inconsistencies. Consider verifying if both versions are necessary and if not, consolidate to a single version to avoid dependency issues.
| @@ -2926,6 +3106,9 @@ packages: | |||
| [email protected]: | |||
| resolution: {integrity: sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w==} | |||
|
|
|||
| [email protected]: | |||
There was a problem hiding this comment.
The package [email protected] seems to be added, but there is already a similar package [email protected] present. Please verify if both packages are necessary or if this is a duplication error.
| @@ -3101,6 +3291,10 @@ packages: | |||
| resolution: {integrity: sha512-WuyALRjWPDGtt/wzJiadO5AXY+8hZ80hVpe6MyivgraREW751X3SbhRvG3eLKOYN+8VEvqLcf3wdnt44Z4S4SA==} | |||
| engines: {node: '>=10'} | |||
|
|
|||
| [email protected]: | |||
| resolution: {integrity: sha512-xx0kgFxSHWY9aG1109uv4w2b+JLwHseSowOWo1bzCTDBpUk3er2rZdtQ90mAjUYbkh6Hus9DAwWvmHsX5pHaIQ==} | |||
| engines: {node: ^8.10.0 || ^10.13.0 || >=11.10.1} | |||
There was a problem hiding this comment.
The engines field specifies compatibility with Node.js versions. Ensure that the specified versions align with the project's requirements and do not introduce compatibility issues with other dependencies.
| @@ -3181,10 +3378,19 @@ packages: | |||
| resolution: {integrity: sha512-U9nH88a3fc/ekCF1l0/UP1IosiuIjyTh7hBvXVMHYgVcfGvt897Xguj2UOLDeI5BG2m7/uwyaLVT6fbtCwTyzw==} | |||
| engines: {iojs: '>=1.0.0', node: '>=0.10.0'} | |||
|
|
|||
| [email protected]: | |||
| resolution: {integrity: sha512-J5xnxTyqaiw06JjMftq7L9ouA448dw/E7dKghkP9WpKNuwmARNNg+Gk8/u5ryb9N/Yo2+z3MCwuqFK/+qPOPfQ==} | |||
| deprecated: Rimraf versions prior to v4 are no longer supported | |||
There was a problem hiding this comment.
The deprecated field indicates that versions prior to v4 of Rimraf are no longer supported. Consider updating to a supported version to avoid potential issues.
| @@ -4777,6 +5048,11 @@ snapshots: | |||
|
|
|||
| '@types/[email protected]': {} | |||
|
|
|||
| '@types/[email protected]': | |||
| dependencies: | |||
| '@types/express': 5.0.0 | |||
There was a problem hiding this comment.
The version specified for @types/express is 5.0.0, which is a major version update. Ensure that this version is compatible with the rest of the codebase and other dependencies that rely on @types/express.
| @@ -6593,6 +7039,19 @@ snapshots: | |||
| optionalDependencies: | |||
| graceful-fs: 4.2.11 | |||
|
|
|||
| [email protected]: | |||
There was a problem hiding this comment.
Consider updating jsonwebtoken to the latest version if possible, as version 9.0.2 is already listed in the dependencies. This could help avoid potential security vulnerabilities or bugs present in older versions.
| [email protected]: | ||
| dependencies: | ||
| '@types/express-jwt': 0.0.42 | ||
| axios: 0.21.4([email protected]) |
There was a problem hiding this comment.
The dependency 'axios' is listed with a version and a nested dependency '[email protected]' in parentheses. This format is unusual and may cause confusion. Consider separating 'axios' and 'debug' into distinct entries if they are intended to be separate dependencies.
| @@ -6840,6 +7349,10 @@ snapshots: | |||
| dependencies: | |||
| wrappy: 1.0.2 | |||
|
|
|||
| [email protected]: | |||
There was a problem hiding this comment.
The package [email protected] seems to be added here. Please verify if this is the correct version and if it is necessary to include both [email protected] and [email protected] as they might be different packages or versions of the same package. Ensure there is no conflict or redundancy.
| @@ -7115,13 +7663,15 @@ snapshots: | |||
| dependencies: | |||
| semver: 7.7.1 | |||
|
|
|||
| [email protected]: {} | |||
There was a problem hiding this comment.
The addition of [email protected] seems unnecessary if it is not being used anywhere in the project. Consider removing it to keep the dependencies clean.
| [email protected]: {} | ||
|
|
||
| [email protected]: {} | ||
|
|
||
| [email protected]: | ||
| dependencies: | ||
| debug: 4.3.6 | ||
| debug: 4.4.0 |
There was a problem hiding this comment.
The update of debug from version 4.3.6 to 4.4.0 should be verified for compatibility with the rest of the codebase. Ensure that there are no breaking changes that could affect the functionality.
| "id" VARCHAR(14) NOT NULL DEFAULT nanoid(), | ||
| "userId" TEXT NOT NULL, | ||
| "handle" TEXT NOT NULL, | ||
| "opportunityId" TEXT NOT NULL, |
There was a problem hiding this comment.
The column opportunityId is defined as TEXT NOT NULL, but it references the id column in the reviewOpportunity table, which is defined as VARCHAR(14). Consider using the same data type (VARCHAR(14)) for consistency and to avoid potential issues with data integrity.
There was a problem hiding this comment.
@billsedison - Should we fix this?
| "incrementalPayment" DOUBLE PRECISION NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "createdBy" TEXT NOT NULL, | ||
| "updatedAt" TIMESTAMP(3) NOT NULL, |
There was a problem hiding this comment.
Consider adding a default value for the updatedAt column to ensure it is automatically set when a new record is created.
There was a problem hiding this comment.
@billsedison - Should we set a default for this?
| "status" "ReviewApplicationStatus" NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "createdBy" TEXT NOT NULL, | ||
| "updatedAt" TIMESTAMP(3) NOT NULL, |
There was a problem hiding this comment.
Consider adding a default value for the updatedAt column to ensure it is automatically set when a new record is created.
There was a problem hiding this comment.
@billsedison - Should we set a default for this?
|
|
||
| @@unique([challengeId, type]) | ||
|
|
||
| @@index([id]) // Index for direct ID lookups |
There was a problem hiding this comment.
The index on id is redundant because id is already a primary key, which is indexed by default. Consider removing this index.
|
|
||
| @@unique([opportunityId, userId, role]) | ||
|
|
||
| @@index([id]) // Index for direct ID lookups |
There was a problem hiding this comment.
The index on id is redundant because id is already a primary key, which is indexed by default. Consider removing this index.
| @@ -1,22 +1,32 @@ | |||
| import { Module } from '@nestjs/common'; | |||
| import { HttpModule } from '@nestjs/axios'; | |||
There was a problem hiding this comment.
Consider checking if HttpModule is actually used within this module. If it's not being utilized, importing it might be unnecessary and could be removed to keep the module clean and efficient.
|
|
||
| @Module({ | ||
| imports: [GlobalProvidersModule], | ||
| imports: [HttpModule, GlobalProvidersModule], |
There was a problem hiding this comment.
Suggestion:
Consider verifying if the addition of HttpModule to the imports is necessary for the new controllers or services. If it is not required, it might be better to avoid adding it to keep the module dependencies minimal.
| controllers: [ | ||
| HealthCheckController, | ||
| ScorecardController, | ||
| AppealController, | ||
| ContactRequestsController, | ||
| ReviewController, | ||
| ProjectResultController, | ||
| ReviewOpportunityController, | ||
| ReviewApplicationController, | ||
| ReviewHistoryController |
There was a problem hiding this comment.
Consider verifying if ReviewHistoryController is correctly implemented and necessary for the current module. Ensure it aligns with the module's responsibilities.
| ], | ||
| providers: [], | ||
| providers: [ReviewOpportunityService, ReviewApplicationService, ChallengeApiService], |
There was a problem hiding this comment.
Ensure that ReviewOpportunityService, ReviewApplicationService, and ChallengeApiService are properly implemented and necessary for this module. Verify that they are being used effectively within the module's context.
| async getByUserId(@Req() req: Request, @Param('userId') userId: string) { | ||
| // Check user permission. Only admin and user himself can access | ||
| const authUser: JwtUser = req['user'] as JwtUser; | ||
| if (authUser.userId !== userId && !isAdmin(authUser)) { |
There was a problem hiding this comment.
Consider using a constant or enum for the error message to maintain consistency and ease of maintenance.
| @ApiOperation({ | ||
| summary: 'Get applications by opportunity ID', | ||
| description: | ||
| 'All users should be able to see full list. Including anonymous.', |
There was a problem hiding this comment.
The description mentions that all users, including anonymous, should be able to see the full list. Consider adding appropriate authentication or authorization checks if needed to ensure this behavior aligns with security requirements.
| const handle = authUser.handle as string; | ||
| // make sure review opportunity exists | ||
| const opportunity = await this.prisma.reviewOpportunity.findUnique({ | ||
| where: { id: dto.opportunityId} |
There was a problem hiding this comment.
Consider adding a semicolon at the end of the object in the findUnique method for consistency with the rest of the code.
| * @param entityList review application entity list | ||
| * @param status application status | ||
| */ | ||
| private async sendEmails(entityList, status: ReviewApplicationStatus) { |
There was a problem hiding this comment.
Specify the type for the entityList parameter to improve type safety and readability.
| * @param entity prisma entity | ||
| * @returns response dto | ||
| */ | ||
| private buildResponse(entity): ReviewApplicationResponseDto { |
There was a problem hiding this comment.
Specify the type for the entity parameter to improve type safety and readability.
| }) | ||
| @ApiResponse({ status: 400, description: 'Bad Request' }) | ||
| @ApiResponse({ status: 401, description: 'Unauthorized' }) | ||
| @ApiResponse({ status: 403, description: 'Unauthorized' }) |
There was a problem hiding this comment.
The description for the 403 status code should be 'Forbidden' instead of 'Unauthorized' to accurately reflect the nature of the error.
There was a problem hiding this comment.
@billsedison - Let's fix this, thanks.
| @ApiBearerAuth() | ||
| @Roles(UserRole.Reviewer, UserRole.Admin) | ||
| @Get('/:userId') | ||
| async getHistory(@Req() req: Request, @Param('userId') userId: string, @Query('range') range: number): Promise<ResponseDto<ReviewApplicationResponseDto[]>> { |
There was a problem hiding this comment.
The range parameter should be validated to ensure it is a positive number. Consider adding validation logic to handle cases where range might be negative or not a number.
| @Get('/:userId') | ||
| async getHistory(@Req() req: Request, @Param('userId') userId: string, @Query('range') range: number): Promise<ResponseDto<ReviewApplicationResponseDto[]>> { | ||
| // Check user permission | ||
| const authUser: JwtUser = req['user'] as JwtUser; |
There was a problem hiding this comment.
The type assertion as JwtUser could potentially lead to runtime errors if req['user'] is not of type JwtUser. Consider adding a type guard or validation to ensure req['user'] is indeed a JwtUser.
| required: false | ||
| }) | ||
| @ApiQuery({ | ||
| name: 'limit', |
There was a problem hiding this comment.
The query parameter name 'limit' is duplicated. Consider renaming the second 'limit' parameter to 'offset' to avoid confusion and ensure correct functionality.
There was a problem hiding this comment.
@billsedison - This should be fixed, thanks.
| type: UpdateReviewOpportunityDto, | ||
| }) | ||
| @ApiResponse({ | ||
| status: 201, |
There was a problem hiding this comment.
The HTTP status code for a successful update operation is typically 200, not 201. Consider changing the status code to 200 to align with RESTful conventions.
| } | ||
| // sort list | ||
| responseList = [...responseList].sort((a, b) => { | ||
| return dto.sortOrder === 'asc' ? (a[dto.sortBy] - b[dto.sortBy]) : (a[dto.sortBy] - b[dto.sortBy]); |
There was a problem hiding this comment.
The sorting logic is incorrect. The comparison function should return a positive number, zero, or a negative number, but currently, it always returns zero because the same expression is used for both 'asc' and 'desc'. Consider using (a[dto.sortBy] - b[dto.sortBy]) for 'asc' and (b[dto.sortBy] - a[dto.sortBy]) for 'desc'.
There was a problem hiding this comment.
@billsedison - Let's double check this thanks.
| * @returns response list | ||
| */ | ||
| private async assembleList( | ||
| entityList, |
There was a problem hiding this comment.
The entityList parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entityList: ReviewOpportunityEntity[].
| * @param entity prisma entity | ||
| * @returns response dto | ||
| */ | ||
| private async assembleResult(entity): Promise<ReviewOpportunityResponseDto> { |
There was a problem hiding this comment.
The entity parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entity: ReviewOpportunityEntity.
| * @returns response dto | ||
| */ | ||
| private buildResponse( | ||
| entity, |
There was a problem hiding this comment.
The entity parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entity: ReviewOpportunityEntity.
| ): ResponseDto<string> { | ||
| const ret = new ResponseDto<string>(); | ||
| const result = new ResultDto<string>(); | ||
| result.success = true; |
There was a problem hiding this comment.
The success property is set to true in FailResponse, which seems incorrect for an error response. Consider setting it to false to accurately represent a failure.
There was a problem hiding this comment.
@billsedison - Let's fix this thanks.
| @ApiProperty({ | ||
| description: 'Review Application Role', | ||
| }) | ||
| role: string; |
There was a problem hiding this comment.
Consider using the ReviewApplicationRole type for the role property instead of string to ensure type safety and consistency with the CreateReviewApplicationDto class.
There was a problem hiding this comment.
@billsedison - Seems like a good suggestion here thanks.
| example: '50.0', | ||
| }) | ||
| @IsNumber() | ||
| incrementalPayment: number; |
There was a problem hiding this comment.
Consider adding @IsPositive() to ensure incrementalPayment is a positive number, similar to basePayment.
There was a problem hiding this comment.
@billsedison - Let's fix this thanks.
| @ApiProperty({ | ||
| description: 'Review opportunity id', | ||
| }) | ||
| id: string; |
There was a problem hiding this comment.
Consider adding @IsUUID() to validate that id is a valid UUID, similar to challengeId.
There was a problem hiding this comment.
@billsedison - Let's fix this thanks.
| description: 'Review application role id', | ||
| example: 8, | ||
| }) | ||
| roleId: number; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsNumber() and @IsPositive() to ensure roleId is a positive number.
| 'Review payment. Should be base payment if there is 1 submission.', | ||
| example: 180.0, | ||
| }) | ||
| payment: number; |
There was a problem hiding this comment.
Consider adding validation decorators such as @IsNumber() and @IsPositive() to ensure payment is a positive number.
There was a problem hiding this comment.
@billsedison - Seems like a reasonable suggestion thanks.
| url: process.env.M2M_AUTH_URL ?? 'http://localhost:4000/oauth/token', | ||
| domain: process.env.M2M_AUTH_DOMAIN ?? 'topcoder-dev.auth0.com', | ||
| audience: process.env.M2M_AUTH_AUDIENCE ?? 'https://m2m.topcoder-dev.com/', | ||
| proxyUrl: process.env.M2M_AUTH_PROXY_SEREVR_URL, |
There was a problem hiding this comment.
Typo in the environment variable name: M2M_AUTH_PROXY_SEREVR_URL should be M2M_AUTH_PROXY_SERVER_URL.
There was a problem hiding this comment.
@billsedison - Let's fix this thanks.
| Admin = 'Admin', | ||
| Copilot = 'Copilot', | ||
| Reviewer = 'Reviewer', | ||
| Admin = 'administrator', |
There was a problem hiding this comment.
Consider using consistent casing for enum values. The value for 'Admin' has been changed to lowercase 'administrator', but 'Submitter' remains capitalized. For consistency, you might want to change 'Submitter' to 'submitter' or revert 'administrator' to 'Admin'.
| Reviewer = 'Reviewer', | ||
| Admin = 'administrator', | ||
| Copilot = 'copilot', | ||
| Reviewer = 'reviewer', | ||
| Submitter = 'Submitter', |
There was a problem hiding this comment.
The addition of 'User = 'Topcoder User'' introduces a new role. Ensure that this new role is handled appropriately in the rest of the application, including any access control logic.
| const token = await this.m2mService.getM2MToken(); | ||
| // Send request to challenge api | ||
| const url = CommonConfig.apis.challengeApiUrl + challengeId; | ||
|
|
There was a problem hiding this comment.
Consider using a template literal for constructing the URL for better readability and maintainability: const url = ${CommonConfig.apis.challengeApiUrl}${challengeId};
| } catch (e) { | ||
| if (e instanceof AxiosError) { | ||
| this.logger.error(`Http Error: ${e.message}`, e.response?.data); | ||
| throw new Error('Cannot get data from Challenge API.'); |
There was a problem hiding this comment.
Consider including the status code in the error message for more detailed logging: this.logger.error(Http Error: ${e.message}, Status Code: ${e.response?.status}, e.response?.data);
| class EventBusMessage<T> { | ||
| topic: string; | ||
| originator: string; | ||
| 'mime-type': string = 'application/json'; |
There was a problem hiding this comment.
Consider using camelCase for the 'mime-type' property to maintain consistency with JavaScript naming conventions.
| // build event bus message | ||
| const msg = new EventBusMessage<EventBusSendEmailPayload>(); | ||
| msg.topic = 'external.action.email'; | ||
| // TODO: Maybe we should update this value. |
There was a problem hiding this comment.
The 'originator' value is marked with a TODO comment. Ensure that this value is updated if necessary before merging.
| throw new Error(`Event bus status code: ${response.status}`); | ||
| } | ||
| } catch (e) { | ||
| this.logger.error(`Event bus failed with error: ${e.message}`); |
There was a problem hiding this comment.
Consider logging the entire error object instead of just 'e.message' to capture more detailed error information for debugging purposes.
| @@ -1,15 +1,21 @@ | |||
| import { Global, Module } from '@nestjs/common'; | |||
| import { APP_GUARD } from '@nestjs/core'; | |||
| import { HttpModule } from '@nestjs/axios'; | |||
There was a problem hiding this comment.
Consider checking if HttpModule is necessary for all consumers of this global module. If not, it might be better to import it only in the specific modules that require it to avoid unnecessary dependencies.
| import { PrismaService } from './prisma.service'; | ||
| import { TokenRolesGuard } from '../../guards/tokenRoles.guard'; | ||
| import { JwtService } from './jwt.service'; | ||
| import { LoggerService } from './logger.service'; | ||
| import { PrismaErrorService } from './prisma-error.service'; | ||
| import { M2MService } from './m2m.service'; |
There was a problem hiding this comment.
Ensure that M2MService is intended to be a global provider. If it is only used in specific contexts, consider importing it in those specific modules instead.
| import { PrismaService } from './prisma.service'; | ||
| import { TokenRolesGuard } from '../../guards/tokenRoles.guard'; | ||
| import { JwtService } from './jwt.service'; | ||
| import { LoggerService } from './logger.service'; | ||
| import { PrismaErrorService } from './prisma-error.service'; | ||
| import { M2MService } from './m2m.service'; | ||
| import { ChallengeApiService } from './challenge.service'; |
There was a problem hiding this comment.
Verify if ChallengeApiService needs to be globally available. If its usage is limited to certain modules, it might be more efficient to import it only where needed.
| import { PrismaService } from './prisma.service'; | ||
| import { TokenRolesGuard } from '../../guards/tokenRoles.guard'; | ||
| import { JwtService } from './jwt.service'; | ||
| import { LoggerService } from './logger.service'; | ||
| import { PrismaErrorService } from './prisma-error.service'; | ||
| import { M2MService } from './m2m.service'; | ||
| import { ChallengeApiService } from './challenge.service'; | ||
| import { EventBusService } from './eventBus.service'; |
There was a problem hiding this comment.
Check if EventBusService is required globally. If not, it could be imported in the specific modules that utilize it to reduce the global scope.
| import { M2MService } from './m2m.service'; | ||
| import { ChallengeApiService } from './challenge.service'; | ||
| import { EventBusService } from './eventBus.service'; | ||
| import { MemberService } from './member.service'; |
There was a problem hiding this comment.
Consider whether MemberService should be a global provider. If its application is limited to certain areas, importing it in those specific modules might be more appropriate.
| roles?: UserRole[]; | ||
| scopes?: string[]; | ||
| isMachine: boolean; |
There was a problem hiding this comment.
The isMachine property is added to the JwtUser interface but is not marked as optional. Consider whether this property should be optional like userId, handle, roles, and scopes.
| } | ||
|
|
||
| export const isAdmin = (user: JwtUser): boolean => { | ||
| return user.isMachine || (user.roles ?? []).includes(UserRole.Admin); |
There was a problem hiding this comment.
The isAdmin function uses the isMachine property to determine if a user is an admin. Ensure that all instances of JwtUser are updated to include this new property, as it is currently not optional.
| } | ||
|
|
||
| // Check if it's a test M2M token | ||
| if (TEST_M2M_TOKENS[token]) { | ||
| const rawScopes = TEST_M2M_TOKENS[token]; | ||
| const scopes = this.expandScopes(rawScopes); | ||
| return { scopes }; | ||
| return { scopes, isMachine: false }; |
There was a problem hiding this comment.
The isMachine property is set to false for test M2M tokens. Consider whether this should be true to accurately reflect that these are machine-to-machine tokens.
| @@ -118,21 +128,29 @@ export class JwtService implements OnModuleInit { | |||
| throw new UnauthorizedException('Invalid token'); | |||
| } | |||
|
|
|||
| const user: JwtUser = {}; | |||
| const user: JwtUser = {isMachine: false}; | |||
There was a problem hiding this comment.
The initialization of user with isMachine: false is a good default, but it might be clearer to explicitly set user.isMachine = false in the else block to ensure clarity in the logic flow.
| user.userId = decodedToken.sub; | ||
| user.isMachine = true; |
There was a problem hiding this comment.
Consider adding a check to ensure decodedToken.sub is defined before assigning it to user.userId to prevent potential runtime errors.
| user.userId = decodedToken[key] as string; | ||
| } | ||
| if (key.endsWith('roles')) { | ||
| user.roles = decodedToken[key] as UserRole[] |
There was a problem hiding this comment.
Ensure that decodedToken[key] is of type UserRole[] before assigning it to user.roles to avoid potential type errors.
| */ | ||
| @Injectable() | ||
| export class M2MService { | ||
| private readonly m2m; |
There was a problem hiding this comment.
Consider specifying the type for the m2m property for better type safety and clarity.
| }); | ||
| } | ||
|
|
||
| async getM2MToken() { |
There was a problem hiding this comment.
Consider adding error handling for the getM2MToken method to manage potential exceptions from the getMachineToken call.
| // construct URL of member API. Eg, https://api.topcoder-dev.com/v5/members?fields=email,userId&userIds=[123456] | ||
| const url = | ||
| CommonConfig.apis.memberApiUrl + | ||
| `?fields=email,userId&userIds=[${userIds.join(',')}]`; |
There was a problem hiding this comment.
The URL construction assumes that userIds is always a valid array of strings. Consider adding validation to ensure userIds is not empty or null before constructing the URL.
| }), | ||
| ); | ||
| const infoList = plainToInstance(MemberInfo, response.data); | ||
| await Promise.all(infoList.map((e) => validateOrReject(e))); |
There was a problem hiding this comment.
The validateOrReject function can throw an error, which is caught in the catch block. However, it might be useful to log which specific MemberInfo instance failed validation for better debugging.
| if (e instanceof AxiosError) { | ||
| this.logger.error( | ||
| `Can't get member info: ${e.message}`, | ||
| e.response?.data, |
There was a problem hiding this comment.
Consider logging more details from e.response?.data to provide more context in the error logs, which can be helpful for debugging issues related to the API response.
No description provided.