Review Opportunities #6

billsedison · 2025-06-03T13:55:20Z

No description provided.

github-actions · 2025-06-03T13:55:37Z

.env.sample

+M2M_AUTH_CLIENT_SECRET="ldzqVaVEbqhwjM5KtZ79sG8djZpAVK8Z7qieVcC3vRjI4NirgcinKSBpPwk6mYYP"
+M2M_AUTH_DOMAIN="topcoder-dev.auth0.com"
+M2M_AUTH_AUDIENCE="https://m2m.topcoder-dev.com/"
+M2M_AUTH_PROXY_SEREVR_URL=


The variable M2M_AUTH_PROXY_SEREVR_URL seems to have a typo in its name. It should likely be M2M_AUTH_PROXY_SERVER_URL.

@billsedison - Let's fix this thanks.

github-actions · 2025-06-03T13:55:42Z

mock/mock-api.js

+  res.json({})
+})
+
+const m2mToken = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL3RvcGNvZGVyLWRldi5hdXRoMC5jb20vIiwic3ViIjoiakdJZjJwZDNmNDRCMWpxdk9haTMwQklLVFphbllCZlVAY2xpZW50cyIsImF1ZCI6Imh0dHBzOi8vbTJtLnRvcGNvZGVyLWRldi5jb20vIiwiaWF0IjoxNzQ4MDk5NDk4LCJleHAiOjE4NDgxODU4OTgsInNjb3BlIjoid3JpdGU6YnVzX2FwaSxhbGw6Y2hhbGxlbmdlcyIsImd0eSI6ImNsaWVudC1jcmVkZW50aWFscyIsImF6cCI6ImpHSWYycGQzZjQ0QjFqcXZPYWkzMEJJS1RaYW5ZQmZVIn0.h3ksdsdJm5USGF1VgROrpkTtStmCzv5ZA6y8bd8AnGY';


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.

@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.

github-actions · 2025-06-03T13:55:42Z

mock/mock-api.js

+// Event bus
+app.post('/eventBus', (req, res) => {
+  logger.info(`Event Bus received message: ${JSON.stringify(req.body)}`);
+  res.statusCode = 200;


Instead of setting res.statusCode = 200;, use res.status(200); for consistency and readability.

github-actions · 2025-06-03T13:55:42Z

mock/mock-api.js

+app.get('/members', (req, res) => {
+  logger.info(`Member API receives params: ${JSON.stringify(req.query)}`)
+  let userIdStr = req.query.userIds
+  userIdStr = userIdStr.replaceAll('[', '').replaceAll(']', '')


Using replaceAll might not be supported in all environments. Consider using a more compatible method such as replace with a regular expression.

github-actions · 2025-06-03T13:55:42Z

mock/mock-api.js

+  // directly challenge details
+  const id = req.params.id
+  logger.info(`Getting challenge with id ${id}`)
+  if (id === '11111111-2222-3333-9999-444444444444') {


Consider using res.status(404).json({}); for setting the status and sending the response in a single chain for better readability.

github-actions · 2025-06-03T13:55:57Z

pnpm-lock.yaml


+  '@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.


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.

github-actions · 2025-06-03T13:56:07Z

pnpm-lock.yaml


+  [email protected]:
+    resolution: {integrity: sha512-0tECWShh6wUysgucJcBAoYegf3JJoZWibxdqhTm7OHPeT42qdjkZ29QCMcKwbgU1kiH+auSIasNRXMLWXafXig==}
+    engines: {'0': node >=0.10.0}


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.

github-actions · 2025-06-03T13:56:20Z

pnpm-lock.yaml

    engines: {node: 20 || >=22}
    hasBin: true

+  [email protected]:


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.

github-actions · 2025-06-03T13:56:27Z

pnpm-lock.yaml

  [email protected]:
    resolution: {integrity: sha512-5dgndWOriYSm5cnYaJNhalLNDKOqFwyDB/rr1E9ZsGciGvKPs8R2xYGCacuf3z6K1YKDz182fd+fY3cn3pMqXQ==}

+  [email protected]:


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.

github-actions · 2025-06-03T13:56:41Z

pnpm-lock.yaml

  [email protected]:
    resolution: {integrity: sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w==}

+  [email protected]:


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.

github-actions · 2025-06-03T13:56:46Z

pnpm-lock.yaml


+  [email protected]:
+    resolution: {integrity: sha512-xx0kgFxSHWY9aG1109uv4w2b+JLwHseSowOWo1bzCTDBpUk3er2rZdtQ90mAjUYbkh6Hus9DAwWvmHsX5pHaIQ==}
+    engines: {node: ^8.10.0 || ^10.13.0 || >=11.10.1}


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.

github-actions · 2025-06-03T13:56:50Z

pnpm-lock.yaml


+  [email protected]:
+    resolution: {integrity: sha512-J5xnxTyqaiw06JjMftq7L9ouA448dw/E7dKghkP9WpKNuwmARNNg+Gk8/u5ryb9N/Yo2+z3MCwuqFK/+qPOPfQ==}
+    deprecated: Rimraf versions prior to v4 are no longer supported


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.

github-actions · 2025-06-03T13:57:12Z

pnpm-lock.yaml


+  '@types/[email protected]':
+    dependencies:
+      '@types/express': 5.0.0


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.

github-actions · 2025-06-03T13:57:36Z

pnpm-lock.yaml

    optionalDependencies:
      graceful-fs: 4.2.11

+  [email protected]:


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.

github-actions · 2025-06-03T13:57:39Z

pnpm-lock.yaml

+  [email protected]:
+    dependencies:
+      '@types/express-jwt': 0.0.42
+      axios: 0.21.4([email protected])


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.

github-actions · 2025-06-03T13:57:51Z

pnpm-lock.yaml

    dependencies:
      wrappy: 1.0.2

+  [email protected]:


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.

github-actions · 2025-06-03T13:58:02Z

pnpm-lock.yaml

    dependencies:
      semver: 7.7.1

+  [email protected]: {}


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.

github-actions · 2025-06-03T13:58:02Z

pnpm-lock.yaml

  [email protected]:
    dependencies:
-      debug: 4.3.6
+      debug: 4.4.0


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.

github-actions · 2025-06-03T13:58:16Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "id" VARCHAR(14) NOT NULL DEFAULT nanoid(),
+    "userId" TEXT NOT NULL,
+    "handle" TEXT NOT NULL,
+    "opportunityId" TEXT NOT NULL,


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.

@billsedison - Should we fix this?

github-actions · 2025-06-03T13:58:16Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "incrementalPayment" DOUBLE PRECISION NOT NULL,
+    "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    "createdBy" TEXT NOT NULL,
+    "updatedAt" TIMESTAMP(3) NOT NULL,


Consider adding a default value for the updatedAt column to ensure it is automatically set when a new record is created.

@billsedison - Should we set a default for this?

github-actions · 2025-06-03T13:58:17Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "status" "ReviewApplicationStatus" NOT NULL,
+    "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    "createdBy" TEXT NOT NULL,
+    "updatedAt" TIMESTAMP(3) NOT NULL,


Consider adding a default value for the updatedAt column to ensure it is automatically set when a new record is created.

@billsedison - Should we set a default for this?

github-actions · 2025-06-03T13:58:20Z

prisma/schema.prisma

+
+  @@unique([challengeId, type])
+
+  @@index([id]) // Index for direct ID lookups


The index on id is redundant because id is already a primary key, which is indexed by default. Consider removing this index.

github-actions · 2025-06-03T13:58:20Z

prisma/schema.prisma

+
+  @@unique([opportunityId, userId, role])
+
+  @@index([id]) // Index for direct ID lookups


The index on id is redundant because id is already a primary key, which is indexed by default. Consider removing this index.

github-actions · 2025-06-03T13:58:23Z

src/api/api.module.ts

@@ -1,22 +1,32 @@
 import { Module } from '@nestjs/common';
+import { HttpModule } from '@nestjs/axios';


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.

github-actions · 2025-06-03T13:58:26Z

src/api/api.module.ts


 @Module({
-  imports: [GlobalProvidersModule],
+  imports: [HttpModule, GlobalProvidersModule],


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.

github-actions · 2025-06-03T13:58:30Z

src/api/api.module.ts

    ProjectResultController,
+    ReviewOpportunityController,
+    ReviewApplicationController,
+    ReviewHistoryController


Consider verifying if ReviewHistoryController is correctly implemented and necessary for the current module. Ensure it aligns with the module's responsibilities.

github-actions · 2025-06-03T13:58:30Z

src/api/api.module.ts

+    ReviewHistoryController
  ],
-  providers: [],
+  providers: [ReviewOpportunityService, ReviewApplicationService, ChallengeApiService],


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.

github-actions · 2025-06-03T13:58:33Z

src/api/review-application/reviewApplication.controller.ts

+  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)) {


Consider using a constant or enum for the error message to maintain consistency and ease of maintenance.

github-actions · 2025-06-03T13:58:33Z

src/api/review-application/reviewApplication.controller.ts

+  @ApiOperation({
+    summary: 'Get applications by opportunity ID',
+    description:
+      'All users should be able to see full list. Including anonymous.',


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.

github-actions · 2025-06-03T13:58:37Z

src/api/review-application/reviewApplication.service.ts

+    const handle = authUser.handle as string;
+    // make sure review opportunity exists
+    const opportunity = await this.prisma.reviewOpportunity.findUnique({
+      where: { id: dto.opportunityId}


Consider adding a semicolon at the end of the object in the findUnique method for consistency with the rest of the code.

github-actions · 2025-06-03T13:58:37Z

src/api/review-application/reviewApplication.service.ts

+   * @param entityList review application entity list
+   * @param status application status
+   */
+  private async sendEmails(entityList, status: ReviewApplicationStatus) {


Specify the type for the entityList parameter to improve type safety and readability.

github-actions · 2025-06-03T13:58:37Z

src/api/review-application/reviewApplication.service.ts

+   * @param entity prisma entity
+   * @returns response dto
+   */
+  private buildResponse(entity): ReviewApplicationResponseDto {


Specify the type for the entity parameter to improve type safety and readability.

github-actions · 2025-06-03T13:58:41Z

src/api/review-history/reviewHistory.controller.ts

+  })
+  @ApiResponse({ status: 400, description: 'Bad Request' })
+  @ApiResponse({ status: 401, description: 'Unauthorized' })
+  @ApiResponse({ status: 403, description: 'Unauthorized' })


The description for the 403 status code should be 'Forbidden' instead of 'Unauthorized' to accurately reflect the nature of the error.

@billsedison - Let's fix this, thanks.

github-actions · 2025-06-03T13:58:42Z

src/api/review-history/reviewHistory.controller.ts

+  @ApiBearerAuth()
+  @Roles(UserRole.Reviewer, UserRole.Admin)
+  @Get('/:userId')
+  async getHistory(@Req() req: Request, @Param('userId') userId: string, @Query('range') range: number): Promise<ResponseDto<ReviewApplicationResponseDto[]>> {


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.

github-actions · 2025-06-03T13:58:42Z

src/api/review-history/reviewHistory.controller.ts

+  @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;


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.

github-actions · 2025-06-03T13:58:51Z

src/api/review-opportunity/reviewOpportunity.controller.ts

+    required: false
+  })
+  @ApiQuery({
+    name: 'limit',


The query parameter name 'limit' is duplicated. Consider renaming the second 'limit' parameter to 'offset' to avoid confusion and ensure correct functionality.

@billsedison - This should be fixed, thanks.

github-actions · 2025-06-03T13:58:51Z

src/api/review-opportunity/reviewOpportunity.controller.ts

+    type: UpdateReviewOpportunityDto,
+  })
+  @ApiResponse({
+    status: 201,


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.

github-actions · 2025-06-03T13:58:56Z

src/api/review-opportunity/reviewOpportunity.service.ts

+    }
+    // sort list
+    responseList = [...responseList].sort((a, b) => {
+      return dto.sortOrder === 'asc' ? (a[dto.sortBy] - b[dto.sortBy]) : (a[dto.sortBy] - b[dto.sortBy]);


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'.

@billsedison - Let's double check this thanks.

github-actions · 2025-06-03T13:58:56Z

src/api/review-opportunity/reviewOpportunity.service.ts

+   * @returns response list
+   */
+  private async assembleList(
+    entityList,


The entityList parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entityList: ReviewOpportunityEntity[].

github-actions · 2025-06-03T13:58:56Z

src/api/review-opportunity/reviewOpportunity.service.ts

+   * @param entity prisma entity
+   * @returns response dto
+   */
+  private async assembleResult(entity): Promise<ReviewOpportunityResponseDto> {


The entity parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entity: ReviewOpportunityEntity.

github-actions · 2025-06-03T13:58:57Z

src/api/review-opportunity/reviewOpportunity.service.ts

+   * @returns response dto
+   */
+  private buildResponse(
+    entity,


The entity parameter should have a type annotation for better type safety and readability. Consider specifying the type, such as entity: ReviewOpportunityEntity.

github-actions · 2025-06-03T13:58:59Z

src/dto/common.dto.ts

+): ResponseDto<string> {
+  const ret = new ResponseDto<string>();
+  const result = new ResultDto<string>();
+  result.success = true;


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.

@billsedison - Let's fix this thanks.

github-actions · 2025-06-03T13:59:02Z

src/dto/reviewApplication.dto.ts

+  @ApiProperty({
+    description: 'Review Application Role',
+  })
+  role: string;


Consider using the ReviewApplicationRole type for the role property instead of string to ensure type safety and consistency with the CreateReviewApplicationDto class.

@billsedison - Seems like a good suggestion here thanks.

github-actions · 2025-06-03T13:59:06Z

src/dto/reviewOpportunity.dto.ts

+    example: '50.0',
+  })
+  @IsNumber()
+  incrementalPayment: number;


Consider adding @IsPositive() to ensure incrementalPayment is a positive number, similar to basePayment.

@billsedison - Let's fix this thanks.

github-actions · 2025-06-03T13:59:06Z

src/dto/reviewOpportunity.dto.ts

+  @ApiProperty({
+    description: 'Review opportunity id',
+  })
+  id: string;


Consider adding @IsUUID() to validate that id is a valid UUID, similar to challengeId.

@billsedison - Let's fix this thanks.

github-actions · 2025-06-03T13:59:06Z

src/dto/reviewOpportunity.dto.ts

+    description: 'Review application role id',
+    example: 8,
+  })
+  roleId: number;


Consider adding validation decorators such as @IsNumber() and @IsPositive() to ensure roleId is a positive number.

github-actions · 2025-06-03T13:59:06Z

src/dto/reviewOpportunity.dto.ts

+      'Review payment. Should be base payment if there is 1 submission.',
+    example: 180.0,
+  })
+  payment: number;


Consider adding validation decorators such as @IsNumber() and @IsPositive() to ensure payment is a positive number.

@billsedison - Seems like a reasonable suggestion thanks.

github-actions · 2025-06-03T13:59:22Z

src/shared/modules/global/eventBus.service.ts

+class EventBusMessage<T> {
+  topic: string;
+  originator: string;
+  'mime-type': string = 'application/json';


Consider using camelCase for the 'mime-type' property to maintain consistency with JavaScript naming conventions.

github-actions · 2025-06-03T13:59:22Z

src/shared/modules/global/eventBus.service.ts

+    // build event bus message
+    const msg = new EventBusMessage<EventBusSendEmailPayload>();
+    msg.topic = 'external.action.email';
+    // TODO: Maybe we should update this value.


The 'originator' value is marked with a TODO comment. Ensure that this value is updated if necessary before merging.

github-actions · 2025-06-03T13:59:22Z

src/shared/modules/global/eventBus.service.ts

+        throw new Error(`Event bus status code: ${response.status}`);
+      }
+    } catch (e) {
+      this.logger.error(`Event bus failed with error: ${e.message}`);


Consider logging the entire error object instead of just 'e.message' to capture more detailed error information for debugging purposes.

github-actions · 2025-06-03T13:59:27Z

src/shared/modules/global/globalProviders.module.ts

@@ -1,15 +1,21 @@
 import { Global, Module } from '@nestjs/common';
 import { APP_GUARD } from '@nestjs/core';
+import { HttpModule } from '@nestjs/axios';


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.

github-actions · 2025-06-03T13:59:27Z

src/shared/modules/global/globalProviders.module.ts

 import { JwtService } from './jwt.service';
 import { LoggerService } from './logger.service';
 import { PrismaErrorService } from './prisma-error.service';
+import { M2MService } from './m2m.service';


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.

github-actions · 2025-06-03T13:59:27Z

src/shared/modules/global/globalProviders.module.ts

 import { LoggerService } from './logger.service';
 import { PrismaErrorService } from './prisma-error.service';
+import { M2MService } from './m2m.service';
+import { ChallengeApiService } from './challenge.service';


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.

github-actions · 2025-06-03T13:59:28Z

src/shared/modules/global/globalProviders.module.ts

 import { PrismaErrorService } from './prisma-error.service';
+import { M2MService } from './m2m.service';
+import { ChallengeApiService } from './challenge.service';
+import { EventBusService } from './eventBus.service';


Check if EventBusService is required globally. If not, it could be imported in the specific modules that utilize it to reduce the global scope.

github-actions · 2025-06-03T13:59:28Z

src/shared/modules/global/globalProviders.module.ts

+import { M2MService } from './m2m.service';
+import { ChallengeApiService } from './challenge.service';
+import { EventBusService } from './eventBus.service';
+import { MemberService } from './member.service';


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.

github-actions · 2025-06-03T13:59:34Z

src/shared/modules/global/jwt.service.ts

+  handle?: string;
  roles?: UserRole[];
  scopes?: string[];
+  isMachine: boolean;


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.

github-actions · 2025-06-03T13:59:34Z

src/shared/modules/global/jwt.service.ts

+}
+
+export const isAdmin = (user: JwtUser): boolean => {
+  return user.isMachine || (user.roles ?? []).includes(UserRole.Admin);


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.

github-actions · 2025-06-03T13:59:47Z

src/shared/modules/global/jwt.service.ts

        const rawScopes = TEST_M2M_TOKENS[token];
        const scopes = this.expandScopes(rawScopes);
-        return { scopes };
+        return { scopes, isMachine: false };


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.

github-actions · 2025-06-03T13:59:52Z

src/shared/modules/global/jwt.service.ts

      }

-      const user: JwtUser = {};
+      const user: JwtUser = {isMachine: false};


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.

github-actions · 2025-06-03T13:59:52Z

src/shared/modules/global/jwt.service.ts

-      if (decodedToken.roles) {
-        user.roles = decodedToken.roles as UserRole[];
        user.userId = decodedToken.sub;
+        user.isMachine = true;


Consider adding a check to ensure decodedToken.sub is defined before assigning it to user.userId to prevent potential runtime errors.

github-actions · 2025-06-03T13:59:52Z

src/shared/modules/global/jwt.service.ts

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


Ensure that decodedToken[key] is of type UserRole[] before assigning it to user.roles to avoid potential type errors.

github-actions · 2025-06-03T13:59:56Z

src/shared/modules/global/m2m.service.ts

+ */
+@Injectable()
+export class M2MService {
+  private readonly m2m;


Consider specifying the type for the m2m property for better type safety and clarity.

github-actions · 2025-06-03T13:59:56Z

src/shared/modules/global/m2m.service.ts

+    });
+  }
+
+  async getM2MToken() {


Consider adding error handling for the getM2MToken method to manage potential exceptions from the getMachineToken call.

github-actions · 2025-06-03T14:00:00Z

src/shared/modules/global/member.service.ts

+    // 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(',')}]`;


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.

github-actions · 2025-06-03T14:00:00Z

src/shared/modules/global/member.service.ts

+        }),
+      );
+      const infoList = plainToInstance(MemberInfo, response.data);
+      await Promise.all(infoList.map((e) => validateOrReject(e)));


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.

github-actions · 2025-06-03T14:00:00Z

src/shared/modules/global/member.service.ts

+      if (e instanceof AxiosError) {
+        this.logger.error(
+          `Can't get member info: ${e.message}`,
+          e.response?.data,


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.

github-actions · 2025-06-22T15:15:41Z

.env.sample

@@ -1 +1,18 @@
-DATABASE_URL="postgresql://johndoe:randompassword@localhost:5432/mydb?schema=public"
+DATABASE_URL="postgresql://johndoe:randompassword@localhost:5432/mydb"


Consider removing the hardcoded credentials in the DATABASE_URL for security reasons. It's better to use placeholders like <username> and <password>.

github-actions · 2025-06-22T15:15:44Z

.env.sample

+M2M_AUTH_CLIENT_ID="jGIf2pd3f44B1jqvOai30BIKTZanYBfU"
+M2M_AUTH_CLIENT_SECRET="ldzqVaVEbqhwjM5KtZ79sG8djZpAVK8Z7qieVcC3vRjI4NirgcinKSBpPwk6mYYP"
+M2M_AUTH_DOMAIN="topcoder-dev.auth0.com"
+M2M_AUTH_AUDIENCE="https://m2m.topcoder-dev.com/"


Typo correction: The variable name M2M_AUTH_PROXY_SEREVR_URL was corrected to M2M_AUTH_PROXY_SERVER_URL. Ensure that this change is reflected in all parts of the codebase where this variable is used.

@billsedison - We shouldn't have any mock client ids, tokens, or secrets in the codebase, even though they aren't valid. We get pinged by Wipro security on those and they don't understand that these aren't usable.

github-actions · 2025-06-22T15:15:45Z

.env.sample

+M2M_AUTH_AUDIENCE="https://m2m.topcoder-dev.com/"
+M2M_AUTH_PROXY_SERVER_URL=
+# Mock API configs
+M2M_MOCK_TOKEN="eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL3RvcGNvZGVyLWRldi5hdXRoMC5jb20vIiwic3ViIjoiakdJZjJwZDNmNDRCMWpxdk9haTMwQklLVFphbllCZlVAY2xpZW50cyIsImF1ZCI6Imh0dHBzOi8vbTJtLnRvcGNvZGVyLWRldi5jb20vIiwiaWF0IjoxNzQ4MDk5NDk4LCJleHAiOjE4NDgxODU4OTgsInNjb3BlIjoid3JpdGU6YnVzX2FwaSxhbGw6Y2hhbGxlbmdlcyIsImd0eSI6ImNsaWVudC1jcmVkZW50aWFscyIsImF6cCI6ImpHSWYycGQzZjQ0QjFqcXZPYWkzMEJJS1RaYW5ZQmZVIn0.h3ksdsdJm5USGF1VgROrpkTtStmCzv5ZA6y8bd8AnGY"


Security concern: The M2M_MOCK_TOKEN contains a JWT token in the sample environment file. Consider whether this token should be included here, as it might expose sensitive information. If it's necessary for testing, ensure it's a non-sensitive mock token.

@billsedison - We shouldn't have any mock client ids, tokens, or secrets in the codebase, even though they aren't valid. We get pinged by Wipro security on those and they don't understand that these aren't usable.

github-actions · 2025-06-22T15:15:48Z

appStartUp.sh

 #!/bin/bash
 set -eo pipefail

 export DATABASE_URL=$(echo -e ${DATABASE_URL})


Using echo -e with variable substitution can lead to unexpected behavior if the variable contains escape sequences. Consider using printf for safer handling of variables.

github-actions · 2025-06-22T15:15:48Z

appStartUp.sh

+fi
+
 echo "Database - running migrations."
 if $RESET_DB; then


The condition $RESET_DB assumes that RESET_DB is a command or a boolean. Ensure that RESET_DB is properly initialized and is a boolean value to avoid unexpected behavior.

github-actions · 2025-06-22T15:15:51Z

mock/mock-api.js

+})
+
+// Use environment variable for M2M token instead of hardcoding
+const m2mToken = process.env.M2M_MOCK_TOKEN || 'dummy-token';


Using a hardcoded fallback token ('dummy-token') can be a security risk if it is accidentally used in production. Consider removing the fallback or ensuring it is only used in a safe development environment.

github-actions · 2025-06-22T15:15:53Z

prisma/migrate.ts


+// Get the schema name from environment variable or use 'public' as default
+const schema = process.env.POSTGRES_SCHEMA || 'public';
+console.log(`Using PostgreSQL schema: ${schema}`);


Using console.log for logging may not be suitable for production environments. Consider using a logging library that can handle different log levels and outputs.

github-actions · 2025-06-22T15:15:56Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "incrementalPayment" DOUBLE PRECISION NOT NULL,
+    "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    "createdBy" TEXT NOT NULL,
+    "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,


The addition of DEFAULT CURRENT_TIMESTAMP to the updatedAt column may not be appropriate if the intention is to track the last update time. This change will set the updatedAt field to the current timestamp upon record creation, which might not reflect the actual update time of the record. Consider whether this behavior aligns with the intended use of the updatedAt field.

github-actions · 2025-06-22T15:15:59Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "id" VARCHAR(14) NOT NULL DEFAULT nanoid(),
+    "userId" TEXT NOT NULL,
+    "handle" TEXT NOT NULL,
+    "opportunityId" VARCHAR(14) NOT NULL,


Consider ensuring that the change from TEXT to VARCHAR(14) for opportunityId aligns with the expected data length and constraints for this field. If opportunityId values can exceed 14 characters, this change might lead to data truncation issues.

github-actions · 2025-06-22T15:15:59Z

prisma/migrations/20250525042446_add_review_application/migration.sql

+    "status" "ReviewApplicationStatus" NOT NULL,
+    "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+    "createdBy" TEXT NOT NULL,
+    "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,


Adding a default value of CURRENT_TIMESTAMP to updatedAt might not be appropriate if the intention is to track the actual update time. Consider whether this default value should be removed or if an update trigger should be used to set updatedAt to the current timestamp upon record modification.

github-actions · 2025-06-22T15:16:04Z

src/api/review-opportunity/reviewOpportunity.controller.ts

+    required: false
+  })
+  @ApiQuery({
+    name: 'offset',


The parameter name has been changed from limit to offset. Ensure that this change is intentional and that the rest of the codebase, including any documentation or dependent services, is updated to reflect this change. If the parameter is meant to control pagination, verify that the logic handling this parameter is correctly adjusted to use offset instead of limit.

github-actions · 2025-06-22T15:16:11Z

src/dto/reviewOpportunity.dto.ts

+      'Review payment. Should be base payment if there is 1 submission.',
+    example: 180.0,
+  })
+  @IsNumber()


Consider specifying the @IsNumber() decorator with { allowNaN: false } to ensure that the payment is not NaN, which is a valid number in JavaScript but may not be desirable in this context.

github-actions · 2025-06-22T15:16:16Z

src/shared/modules/global/prisma.service.ts

    });

    this.logger = LoggerService.forRoot('PrismaService');
+    this.logger.log(`Using PostgreSQL schema: ${schema}`);


Consider using a more secure method to log the schema name, especially if it contains sensitive information. Logging sensitive information can lead to security vulnerabilities.

github-actions · 2025-06-22T15:26:23Z

src/dto/reviewOpportunity.dto.ts

+  })
+  @IsString()
+  @IsNotEmpty()
+  @IsUUID()


The @IsUUID() decorator already ensures that the value is a non-empty string that conforms to the UUID format. Therefore, the @IsString() and @IsNotEmpty() decorators are redundant and can be removed.

@billsedison - This one can be fixed

github-actions · 2025-06-23T03:01:27Z

src/dto/reviewOpportunity.dto.ts

+  @ApiProperty({
+    description: 'Challenge id',
+  })
+  @IsUUID()


Consider re-adding the @IsString() and @IsNotEmpty() decorators if they are necessary for validation. The @IsUUID() decorator ensures the format is a valid UUID, but it does not check for non-empty strings or enforce that the input is a string type. If challengeId should not be empty and must be a string, these decorators are important for validation.

github-actions · 2025-06-23T03:01:30Z

src/dto/reviewOpportunity.dto.ts

+  @ApiProperty({
+    description: 'Review opportunity id',
+  })
+  @IsUUID()


Consider re-adding the @IsString() and @IsNotEmpty() decorators if the id field is expected to be a non-empty string. While @IsUUID() ensures the format, it does not enforce non-emptiness or string type explicitly.

github-actions · 2025-06-23T03:06:34Z

src/dto/reviewOpportunity.dto.ts

+  @ApiProperty({
+    description: 'Challenge id',
+  })
+  @IsString()


The @IsString() decorator is unnecessary here because @IsUUID() already implies that the value should be a string. Consider removing @IsString() to avoid redundancy.

github-actions · 2025-06-23T03:06:36Z

src/dto/reviewOpportunity.dto.ts

+  @ApiProperty({
+    description: 'Review opportunity id',
+  })
+  @IsString()


The @IsString() decorator is redundant here since @IsUUID() already implies that the value is a string. Consider removing @IsString().

Review Opportunities

1e4e60b

billsedison requested review from jmgasper and kkartunov June 3, 2025 13:55

github-actions bot reviewed Jun 3, 2025

View reviewed changes

billsedison added 2 commits June 22, 2025 23:14

Mkke schema configurable

0729f07

Fix GitHub issues

2820bbc

github-actions bot reviewed Jun 22, 2025

View reviewed changes

Add IsUUID for review opportunity ID

e035dac

github-actions bot reviewed Jun 22, 2025

View reviewed changes

Remove redundant IsString IsNotEmpty and remove secrets in .env.sample

08b12ce

github-actions bot reviewed Jun 23, 2025

View reviewed changes

Add IsString and IsNotEmpty decorators back

aae1df9

github-actions bot reviewed Jun 23, 2025

View reviewed changes

jmgasper approved these changes Jun 23, 2025

View reviewed changes

billsedison merged commit 42a278d into develop Jun 23, 2025
1 check passed

kkartunov deleted the feat/review-opportunities branch August 6, 2025 05:14


		@@unique([challengeId, type])

		@@index([id]) // Index for direct ID lookups


		@@unique([opportunityId, userId, role])

		@@index([id]) // Index for direct ID lookups

		@@ -1,22 +1,32 @@
		import { Module } from '@nestjs/common';
		import { HttpModule } from '@nestjs/axios';

		@@ -1 +1,18 @@
		DATABASE_URL="postgresql://johndoe:randompassword@localhost:5432/mydb?schema=public"
		DATABASE_URL="postgresql://johndoe:randompassword@localhost:5432/mydb"

Review Opportunities #6

Review Opportunities #6

Uh oh!

Conversation

billsedison commented Jun 3, 2025

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 3, 2025