Skip to content
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

Changes of user id in academic year #102

Open
wants to merge 1 commit into
base: release-1.0.0
Choose a base branch
from

Conversation

poojakarma
Copy link
Contributor

@poojakarma poojakarma commented Nov 25, 2024

Summary by CodeRabbit

  • New Features

    • Enhanced academic year management by integrating user ID handling for creation and retrieval processes.
    • Introduced a validation function for user ID checks to ensure valid input.
  • Bug Fixes

    • Improved error handling with detailed logging for user-related actions and errors.
  • Documentation

    • Added new response messages for user ID validation errors.

Copy link

coderabbitai bot commented Nov 25, 2024

Walkthrough

The changes involve updates to multiple files related to the AcademicyearsController and associated services, introducing a userId parameter across various methods for enhanced user identification. The AcademicyearsController has modified method signatures to include userId and request handling, while the AcademicYearDto class has had validation decorators commented out for certain properties. The service locator and PostgreSQL adapter interfaces have been updated to accept the userId parameter in their method signatures. Additionally, new utility functions for user ID validation and response messages have been added.

Changes

File Path Change Summary
src/academicyears/academicyears.controller.ts - Added Request and Query imports.
- Commented out @UseGuards(JwtAuthGuard) and @ApiBasicAuth("access-token").
- Updated method signatures to include userId and request handling.
src/academicyears/dto/academicyears-create.dto.ts - Commented out @ApiProperty and @IsUUID decorators for createdBy and updatedBy properties.
src/adapters/academicyearsservicelocater.ts - Updated method signatures to include userId for createAcademicYear, getAcademicYearList, and getAcademicYearById.
src/adapters/postgres/academicyears-adapter.ts - Updated method signatures to include userId for createAcademicYear, getAcademicYearList, and getAcademicYearById.
- Enhanced error logging with userId.
src/common/utils/custom-validation.ts - Added checkValidUserId function for validating user IDs.
src/common/utils/response.messages.ts - Added new response messages: PROVIDE_ONE_USERID_IN_QUERY and USERID_INVALID.

Sequence Diagram(s)

sequenceDiagram
    participant Controller
    participant ServiceLocator
    participant Adapter
    participant Logger
    participant Validator

    Controller->>Validator: checkValidUserId(userId)
    Validator-->>Controller: Valid userId or error

    Controller->>ServiceLocator: createAcademicYear(academicYearDto, userId)
    ServiceLocator->>Adapter: createAcademicYear(academicYearDto, userId)
    Adapter->>Logger: Log error if any
    Adapter-->>ServiceLocator: Return result
    ServiceLocator-->>Controller: Return result
Loading
sequenceDiagram
    participant Controller
    participant ServiceLocator
    participant Adapter
    participant Logger

    Controller->>ServiceLocator: getAcademicYearList(userId)
    ServiceLocator->>Adapter: getAcademicYearList(userId)
    Adapter->>Logger: Log error if any
    Adapter-->>ServiceLocator: Return result
    ServiceLocator-->>Controller: Return result
Loading

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (9)
src/common/utils/custom-validation.ts (3)

4-4: Remove or uncomment the unused import.

The commented import of ERROR_MESSAGES suggests incomplete refactoring. Either remove it if it's no longer needed or uncomment if it will be used in future implementations.

-// import { ERROR_MESSAGES } from "./constants.util";

6-14: Consider refactoring for better maintainability and type safety.

The function can be made more robust and maintainable with a few improvements:

  1. Combine the validation checks
  2. Add JSDoc documentation
  3. Use type guards for better type safety
+/**
+ * Validates if the provided value is a valid UUID string
+ * @param userId - The user ID to validate
+ * @throws {BadRequestException} If the user ID is missing or invalid
+ * @returns The validated user ID
+ */
 export const checkValidUserId = (userId: string | unknown): string => {
-    if (typeof userId !== 'string') {
-        throw new BadRequestException(API_RESPONSES.PROVIDE_ONE_USERID_IN_QUERY);
-    }
-    if (!userId || !isUUID(userId)) {
-        throw new BadRequestException(API_RESPONSES.USERID_INVALID);
-    }
-    return userId;
+    if (typeof userId !== 'string' || !userId) {
+        throw new BadRequestException(API_RESPONSES.PROVIDE_ONE_USERID_IN_QUERY);
+    }
+    
+    if (!isUUID(userId)) {
+        throw new BadRequestException(API_RESPONSES.USERID_INVALID);
+    }
+    
+    return userId;
 };
🧰 Tools
🪛 eslint (1.23.1)

[error] 7-7: Replace ··if·(typeof·userId·!==·'string' with if·(typeof·userId·!==·"string"

(prettier/prettier)


[error] 8-8: Delete ····

(prettier/prettier)


[error] 9-9: Delete ··

(prettier/prettier)


[error] 10-10: Delete ··

(prettier/prettier)


[error] 11-11: Delete ····

(prettier/prettier)


[error] 12-12: Delete ··

(prettier/prettier)


[error] 13-13: Delete ··

(prettier/prettier)


[error] 14-14: Insert

(prettier/prettier)


1-14: Ensure consistent formatting.

The code has some formatting inconsistencies. Consider running Prettier to automatically fix these issues:

  • Inconsistent indentation
  • Single vs double quotes
  • Missing newline at end of file
🧰 Tools
🪛 eslint (1.23.1)

[error] 7-7: Replace ··if·(typeof·userId·!==·'string' with if·(typeof·userId·!==·"string"

(prettier/prettier)


[error] 8-8: Delete ····

(prettier/prettier)


[error] 9-9: Delete ··

(prettier/prettier)


[error] 10-10: Delete ··

(prettier/prettier)


[error] 11-11: Delete ····

(prettier/prettier)


[error] 12-12: Delete ··

(prettier/prettier)


[error] 13-13: Delete ··

(prettier/prettier)


[error] 14-14: Insert

(prettier/prettier)

src/adapters/academicyearsservicelocater.ts (1)

Line range hint 1-21: Consider improving return types

The interface methods use Promise<any> or implicit any return types. Consider defining specific return types for better type safety.

Example improvement:

interface AcademicYearResponse {
  id: string;
  // Add other relevant fields
}

// Then use it in method signatures:
createAcademicYear(...): Promise<AcademicYearResponse>;
getAcademicYearList(...): Promise<AcademicYearResponse[]>;
getAcademicYearById(...): Promise<AcademicYearResponse>;
src/academicyears/dto/academicyears-create.dto.ts (1)

Line range hint 19-23: Add validation decorators for remaining properties.

The session, tenantId, and isActive properties lack validation decorators. This inconsistency could lead to data validation issues.

Add appropriate validation decorators:

   endDate: string;

+  @ApiProperty({ description: "Active status of academic year", example: true })
+  @IsBoolean()
   isActive?: boolean;

+  @ApiProperty({ description: "Academic session", example: "2023-24" })
+  @IsString()
+  @IsNotEmpty()
   session: string;

+  @ApiProperty({ description: "Tenant ID", example: "tenant-123" })
+  @IsString()
+  @IsNotEmpty()
   tenantId: string;
src/academicyears/academicyears.controller.ts (3)

62-65: Consider extracting userId from JWT token instead of query parameters

While the validation is good, extracting userId from query parameters is not ideal when dealing with authenticated requests. Consider:

  1. Extracting it from the JWT token payload
  2. Using NestJS's user decorator pattern

Example implementation:

// Create a custom decorator
const GetUser = createParamDecorator(
  (data: unknown, ctx: ExecutionContext) => {
    const request = ctx.switchToHttp().getRequest();
    return request.user?.id;
  },
);

// Use in controller
async createAcademicYears(
  @Body() academicyearsService: AcademicYearDto,
  @Res() response: Response,
  @GetUser() userId: string,
  @Headers() headers
)

80-80: Fix quote style consistency

Replace single quotes with double quotes for consistency with the codebase style:

-@Query('userId') userId: string | null
+@Query("userId") userId: string | null

Also applies to: 105-105

🧰 Tools
🪛 eslint (1.23.1)

[error] 80-80: Replace 'userId' with "userId"

(prettier/prettier)


userId validation should be consistent across all controller methods

The codebase shows that checkValidUserId utility is specifically designed to validate userId with proper error handling, throwing BadRequestException for invalid cases. This validation should be applied consistently across all methods in the AcademicyearsController that accept userId.

  • Add checkValidUserId validation in getAcademicYearList method before using the userId parameter
  • Add checkValidUserId validation in getAcademicYearById method before using the userId parameter
🔗 Analysis chain

Line range hint 62-110: Verify consistent error handling across methods

The createAcademicYears method includes userId validation, but similar validation is missing in getAcademicYearList and getAcademicYearById. Let's verify the error handling implementation across the codebase:


Let me gather more information about the error handling patterns in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for checkValidUserId usage patterns
rg "checkValidUserId" -A 5

# Search for similar controller methods handling userId
ast-grep --pattern '@Query("userId") $userId: $_'

Length of output: 1626


Script:

#!/bin/bash
# Search for userId validation patterns in controllers
rg "userId.*isUUID|isUUID.*userId" -A 3

# Search for BadRequestException usage with userId
rg "BadRequestException.*userId" -A 2

# Check API_RESPONSES related to userId
rg "API_RESPONSES\.[A-Z_]*USER[A-Z_]*"

Length of output: 5582

🧰 Tools
🪛 eslint (1.23.1)

[error] 43-43: Delete ·

(prettier/prettier)


[error] 80-80: Replace 'userId' with "userId"

(prettier/prettier)

src/adapters/postgres/academicyears-adapter.ts (1)

Line range hint 179-184: Add tenant isolation to getAcademicYearById

The method should include tenantId in both its signature and query to maintain proper data isolation.

- async getAcademicYearById(id, userId, response) {
+ async getAcademicYearById(id: string, userId: string, tenantId: string, response: Response) {
  const apiId = APIID.ACADEMICYEAR_GET;
  try {
-   const academicYearResult = await this.academicYearRespository.findOne({ where: { id: id } });
+   const academicYearResult = await this.academicYearRespository.findOne({
+     where: { 
+       id: id,
+       tenantId: tenantId
+     }
+   });
🧰 Tools
🪛 eslint (1.23.1)

[error] 182-182: Replace ·where:·{·id:·id·} with ⏎········where:·{·id:·id·},⏎·····

(prettier/prettier)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e8618e2 and f45322f.

📒 Files selected for processing (6)
  • src/academicyears/academicyears.controller.ts (4 hunks)
  • src/academicyears/dto/academicyears-create.dto.ts (1 hunks)
  • src/adapters/academicyearsservicelocater.ts (1 hunks)
  • src/adapters/postgres/academicyears-adapter.ts (9 hunks)
  • src/common/utils/custom-validation.ts (1 hunks)
  • src/common/utils/response.messages.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/academicyears/academicyears.controller.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/academicyears/dto/academicyears-create.dto.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/adapters/academicyearsservicelocater.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/adapters/postgres/academicyears-adapter.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/common/utils/custom-validation.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/common/utils/response.messages.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
🪛 eslint (1.23.1)
src/academicyears/academicyears.controller.ts

[error] 43-43: Delete ·

(prettier/prettier)


[error] 80-80: Replace 'userId' with "userId"

(prettier/prettier)


[error] 105-105: Replace 'userId' with "userId"

(prettier/prettier)

src/adapters/postgres/academicyears-adapter.ts

[error] 43-43: Replace ·where:·{·tenantId:·tenantId·}·}) with ⏎········where:·{·tenantId:·tenantId·},⏎······});

(prettier/prettier)


[error] 45-45: Replace API_RESPONSES.TENANT_NOTFOUND,·API_RESPONSES.NOT_FOUND,·apiId,·userId with ⏎··········API_RESPONSES.TENANT_NOTFOUND,⏎··········API_RESPONSES.NOT_FOUND,⏎··········apiId,⏎··········userId⏎········

(prettier/prettier)


[error] 61-61: Replace API_RESPONSES.ACADEMICYEAR_EXIST,·API_RESPONSES.ACADEMICYEAR_YEAR,·apiId,·userId with ⏎··········API_RESPONSES.ACADEMICYEAR_EXIST,⏎··········API_RESPONSES.ACADEMICYEAR_YEAR,⏎··········apiId,⏎··········userId⏎········

(prettier/prettier)


[error] 94-94: Replace errorMessage,·API_RESPONSES.INTERNAL_SERVER_ERROR,·apiId,·userId with ⏎········errorMessage,⏎········API_RESPONSES.INTERNAL_SERVER_ERROR,⏎········apiId,⏎········userId⏎······

(prettier/prettier)


[error] 168-168: Replace errorMessage,·API_RESPONSES.INTERNAL_SERVER_ERROR,·apiId,·userId with ⏎········errorMessage,⏎········API_RESPONSES.INTERNAL_SERVER_ERROR,⏎········apiId,⏎········userId⏎······

(prettier/prettier)


[error] 201-201: Replace errorMessage,·API_RESPONSES.INTERNAL_SERVER_ERROR,·apiId,·userId with ⏎········errorMessage,⏎········API_RESPONSES.INTERNAL_SERVER_ERROR,⏎········apiId,⏎········userId⏎······

(prettier/prettier)

src/common/utils/custom-validation.ts

[error] 7-7: Replace ··if·(typeof·userId·!==·'string' with if·(typeof·userId·!==·"string"

(prettier/prettier)


[error] 8-8: Delete ····

(prettier/prettier)


[error] 9-9: Delete ··

(prettier/prettier)


[error] 10-10: Delete ··

(prettier/prettier)


[error] 11-11: Delete ····

(prettier/prettier)


[error] 12-12: Delete ··

(prettier/prettier)


[error] 13-13: Delete ··

(prettier/prettier)


[error] 14-14: Insert

(prettier/prettier)

src/common/utils/response.messages.ts

[error] 158-158: Insert ,

(prettier/prettier)

🔇 Additional comments (3)
src/adapters/postgres/academicyears-adapter.ts (3)

13-13: LGTM: LoggerUtil import added correctly

The addition of LoggerUtil follows the NestJS best practices for consistent error logging across the application.


43-45: ⚠️ Potential issue

Use TypeORM parameterized query for tenant lookup

The current tenant lookup is vulnerable to SQL injection. Use TypeORM's built-in parameter binding.

- const tenantExist = await this.tenantRepository.findOne({ where: { tenantId: tenantId } })
+ const tenantExist = await this.tenantRepository
+   .createQueryBuilder('tenant')
+   .where('tenant.tenantId = :tenantId', { tenantId })
+   .getOne();

Likely invalid or redundant comment.

🧰 Tools
🪛 eslint (1.23.1)

[error] 43-43: Replace ·where:·{·tenantId:·tenantId·}·}) with ⏎········where:·{·tenantId:·tenantId·},⏎······});

(prettier/prettier)


[error] 45-45: Replace API_RESPONSES.TENANT_NOTFOUND,·API_RESPONSES.NOT_FOUND,·apiId,·userId with ⏎··········API_RESPONSES.TENANT_NOTFOUND,⏎··········API_RESPONSES.NOT_FOUND,⏎··········apiId,⏎··········userId⏎········

(prettier/prettier)


40-41: Validate userId before assignment

The userId should be validated before being assigned to createdBy and updatedBy fields.

import { API_RESPONSES } from "./response.messages";
// import { ERROR_MESSAGES } from "./constants.util";

export const checkValidUserId = (userId: any): string => {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety by avoiding any type.

Using any type bypasses TypeScript's type checking benefits. Consider using a more specific type for better type safety.

-export const checkValidUserId = (userId: any): string => {
+export const checkValidUserId = (userId: string | unknown): string => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const checkValidUserId = (userId: any): string => {
export const checkValidUserId = (userId: string | unknown): string => {

Comment on lines +8 to 11
userId,
tenantId,
response: Response
): Promise<any>;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type definitions for userId parameter

The newly added userId parameter lacks type definition. This could lead to type-related issues at runtime.

Apply this diff to add proper typing:

  createAcademicYear(
    academicYearDto: AcademicYearDto,
-   userId,
+   userId: string,
    tenantId,
    response: Response
  ): Promise<any>;
  getActiveAcademicYear(academicYearId: string, tenantId: string);
  getAcademicYearList(
    academicYearSearchDto: AcademicYearSearchDto,
-   userId,
+   userId: string,
    tenantId,
    response: Response
  );

Also applies to: 15-18

tenantId,
response: Response
);
getAcademicYearById(id, response: Response);
getAcademicYearById(id, userId, response: Response);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix inconsistencies in getAcademicYearById method

The method has several issues:

  1. Missing type definitions for parameters
  2. Inconsistent parameter ordering compared to other methods (tenantId is missing)
  3. Missing return type

Apply this diff to fix the issues:

-  getAcademicYearById(id, userId, response: Response);
+  getAcademicYearById(
+    id: string,
+    userId: string,
+    tenantId: string,
+    response: Response
+  ): Promise<any>;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
getAcademicYearById(id, userId, response: Response);
getAcademicYearById(
id: string,
userId: string,
tenantId: string,
response: Response
): Promise<any>;

Comment on lines +26 to +31
// @ApiProperty({
// type: String,
// description: "createdBy",
// example: "eff008a8-2573-466d-b877-fddf6a4fc13e",
// })
// @IsUUID("4", { message: "createdBy must be a valid UUID" })
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restore validation decorators for UUID fields.

Removing validation decorators for createdBy and updatedBy could lead to data integrity issues. Even if user ID validation is handled at the controller level, maintaining DTO-level validation provides an additional safety layer and follows NestJS best practices.

Restore the decorators with this diff:

-  // @ApiProperty({
-  //   type: String,
-  //   description: "createdBy",
-  //   example: "eff008a8-2573-466d-b877-fddf6a4fc13e",
-  // })
-  // @IsUUID("4", { message: "createdBy must be a valid UUID" })
+  @ApiProperty({
+    type: String,
+    description: "createdBy",
+    example: "eff008a8-2573-466d-b877-fddf6a4fc13e",
+  })
+  @IsUUID("4", { message: "createdBy must be a valid UUID" })
   createdBy: string;

-  // @ApiProperty({
-  //   type: String,
-  //   description: "updatedBy",
-  //   example: "eff008a8-2573-466d-b877-fddf6a4fc13e",
-  // })
-  // @IsUUID("4", { message: "updatedBy must be a valid UUID" })
+  @ApiProperty({
+    type: String,
+    description: "updatedBy",
+    example: "eff008a8-2573-466d-b877-fddf6a4fc13e",
+  })
+  @IsUUID("4", { message: "updatedBy must be a valid UUID" })
   updatedBy: string;

Also applies to: 34-39

Comment on lines +79 to 89
@Headers() headers,
@Query('userId') userId: string | null
) {
const tenantId = headers["tenantid"];
if (!tenantId || !isUUID(tenantId)) {
throw new BadRequestException(API_RESPONSES.TENANTID_VALIDATION);
}
const result = await this.academicYearAdapter
.buildAcademicYears()
.getAcademicYearList(academicYearSearchDto, tenantId, response);
.getAcademicYearList(academicYearSearchDto, userId, tenantId, response);
return response.status(result.statusCode).json(result);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider making userId a required parameter

Both methods accept userId as nullable (string | null), but the implementation suggests it's a required field for proper functionality. Consider:

  1. Making it non-nullable
  2. Adding validation similar to the create method
-@Query("userId") userId: string | null
+@Query("userId") userId: string

// Add validation
+const userId: string = checkValidUserId(userId);

Also applies to: 104-110

🧰 Tools
🪛 eslint (1.23.1)

[error] 80-80: Replace 'userId' with "userId"

(prettier/prettier)


@ApiTags("Academicyears")
@Controller("academicyears")
@UseGuards(JwtAuthGuard)
// @UseGuards(JwtAuthGuard)
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Security Issue: Inconsistent Authentication Pattern Detected

The commented-out @UseGuards(JwtAuthGuard) in academicyears.controller.ts deviates from the established authentication pattern in the codebase. Evidence shows that:

  • Other controllers consistently use @UseGuards(JwtAuthGuard) at the class level for protection
  • Related controllers like course-planner, user, cohort, and rbac all implement the guard
  • The guard is properly imported but deliberately commented out in this controller

This change leaves all academic year endpoints unprotected, creating a security vulnerability. The authentication guard should be restored to maintain consistent security across the application.

🔗 Analysis chain

Security Concern: Authentication guards are commented out

The removal of @UseGuards(JwtAuthGuard) and @ApiBasicAuth decorators without replacement leaves the endpoints unprotected. This is a critical security issue that could expose sensitive academic year data.

Let's verify if other controllers maintain authentication:

Also applies to: 47-47, 71-71, 94-94

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other controllers to verify authentication pattern
ast-grep --pattern 'class $_ {
  $$$
  @UseGuards($$$)
  $$$
}'

Length of output: 65


Script:

#!/bin/bash
# Let's try a different approach to search for authentication patterns
# First, let's search for UseGuards decorator usage
rg "@UseGuards" -A 2

# Also search for any authentication-related imports
rg "import.*Guard" 

# And search for any authentication-related configurations
rg "authentication|auth" --type ts

Length of output: 10883

@@ -23,6 +25,7 @@ export class PostgresAcademicYearService

public async createAcademicYear(
academicYearDto: AcademicYearDto,
userId,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type definition for userId parameter

The userId parameter should have a proper type definition for better type safety.

- userId,
+ userId: string,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
userId,
userId: string,

Comment on lines +134 to 136
userId,
tenantId,
response: Response
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add pagination support for academic year list

The method should implement pagination to handle large result sets efficiently.

async getAcademicYearList(
  academicYearSearchDto: AcademicYearSearchDto,
  userId: string,
  tenantId: string,
  response: Response,
+ page: number = 1,
+ limit: number = 10
) {

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +157 to +158
PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
USERID_INVALID: "Invalid UUID"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix syntax error: Add missing comma.

Add a comma after the last property to maintain valid object syntax.

  PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
-  USERID_INVALID: "Invalid UUID"
+  USERID_INVALID: "Invalid UUID",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
USERID_INVALID: "Invalid UUID"
PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
USERID_INVALID: "Invalid UUID",
🧰 Tools
🪛 eslint (1.23.1)

[error] 158-158: Insert ,

(prettier/prettier)


🛠️ Refactor suggestion

Maintain consistency with existing UUID validation messages.

The new messages should follow the same pattern as existing UUID validation messages in the codebase. Consider:

  1. USERID_INVALID overlaps with UUID_VALIDATION and should be consolidated
  2. Consider using template literals for dynamic values like other UUID messages
-  PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
-  USERID_INVALID: "Invalid UUID",
+  USERID_REQUIRED: "UserId is required in query parameters",
+  USERID_VALIDATION: (userId) => `Invalid userId format: ${userId} must be a valid UUID`,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PROVIDE_ONE_USERID_IN_QUERY: "Please provide userId in query param",
USERID_INVALID: "Invalid UUID"
USERID_REQUIRED: "UserId is required in query parameters",
USERID_VALIDATION: (userId) => `Invalid userId format: ${userId} must be a valid UUID`
🧰 Tools
🪛 eslint (1.23.1)

[error] 158-158: Insert ,

(prettier/prettier)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant