feat: Add project frontend API with tests and fix backend project API #25
Conversation
This commit adds the project frontend API along with its corresponding tests. It also fixes an issue in the backend project API where the user ID should be a number. Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
WalkthroughThe changes in this pull request involve modifications across several files in the backend application. Key updates include the introduction of new modules, services, guards, and decorators to enhance role and menu management, as well as the addition of a new GraphQL resolver. The Changes
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (9)
backend/src/common/enums/role.enum.ts (1)
1-3: Add JSDoc documentation for the enum.While the implementation is correct, adding documentation would help other developers understand the purpose and usage of these roles.
+/** + * Default roles available in the system. + * These roles are initialized at application startup by InitRolesService. + */ export enum DefaultRoles { + /** Administrator role with full system access */ ADMIN = 'Admin', }backend/src/init/init.module.ts (1)
1-3: Enhance documentation with JSDoc format.While the comments are informative, consider using JSDoc format for better IDE integration and documentation generation.
-// This module is use for init operation like init roles, init permissions, init users, etc. -// This module is imported in app.module.ts -// @Author: Jackson Chen +/** + * @module InitModule + * @description Module responsible for initialization operations including: + * - Role initialization + * - Permission initialization + * - User initialization + * + * This module should be imported in app.module.ts + * @author Jackson Chen + */backend/src/app.module.ts (1)
42-42: Review User entity registration scope.The
TypeOrmModule.forFeature([User])is registered in the AppModule. This makes the User repository available in this module's context, but consider if this is the appropriate scope or if it should be moved to UserModule instead.Consider moving this registration to UserModule if the User repository is only needed there:
// In user.module.ts @Module({ imports: [ + TypeOrmModule.forFeature([User]), // other imports ], })backend/src/init/init-roles.service.ts (2)
4-4: Consider relocating the Role model to a more appropriate module.The Role model's location under
auth/rolesuggests it's tightly coupled with authentication. However, roles appear to be a core domain concept that might be used across different contexts. Consider moving it to a dedicatedrolesmodule or acore/domaindirectory for better separation of concerns.
1-56: Add comprehensive test coverage for initialization service.This service is critical for application bootstrapping and role management. Please ensure it has thorough test coverage including:
- Successful initialization scenarios
- Handling of duplicate roles
- Error cases and recovery
- Transaction rollback scenarios
Would you like me to help create a test suite for this service?
backend/src/guard/roles.guard.ts (2)
49-50: Use consistent error messages to prevent user enumerationThrowing
UnauthorizedException('User not found')may reveal to an attacker that a user does not exist in the system. For security reasons, consider providing a generic error message that does not disclose user existence.Apply this diff to generalize the error message:
- throw new UnauthorizedException('User not found'); + throw new UnauthorizedException('User is not authenticated');
65-65: Avoid logging sensitive error detailsLogging
error.messagemay expose sensitive information. It's safer to log a generic message or ensure that the error details are appropriate for logs.Apply this diff to log a generic error message:
- this.logger.error(`Role check failed: ${error.message}`); + this.logger.error('Role check failed');backend/src/auth/auth.service.ts (2)
120-122: Ensure accurate validation when checking for missing menusThe current check
if (menus.length !== menuIds.length)assumes that there are no duplicate IDs inmenuIds. If duplicates exist, this condition might not accurately detect missing menus. Consider removing duplicates frommenuIdsbefore querying or adjusting the validation logic to ensure all specified menus are found.You might preprocess
menuIdsto eliminate duplicates:+ const uniqueMenuIds = Array.from(new Set(menuIds)); - const menus = await this.menuRepository.findByIds(menuIds); + const menus = await this.menuRepository.findBy({ id: In(uniqueMenuIds) }); if (menus.length !== uniqueMenuIds.length) { throw new NotFoundException('Some menus were not found'); }
150-153: Avoid redundant try-catch blocks that rethrow errors without modificationIn several methods, you catch errors, log them, and then rethrow the same error. This pattern can be redundant because the error will propagate upwards regardless, and excessive logging might clutter the output.
Consider removing these try-catch blocks or enhance them to provide additional context or custom error handling. If logging is essential, ensure it's adding value beyond what's already available in the error stack.
Example adjustment:
- try { await this.roleRepository.save(role); Logger.log('Action successful'); return await this.roleRepository.findOne({ ... }); - } catch (error) { - Logger.error(`Failed to perform action: ${error.message}`); - throw error; - }Also applies to: 184-187, 233-235, 267-269
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- README.md (1 hunks)
- backend/src/app.module.ts (2 hunks)
- backend/src/app.resolver.ts (1 hunks)
- backend/src/app.service.ts (0 hunks)
- backend/src/auth/auth.service.ts (4 hunks)
- backend/src/auth/role/role.model.ts (2 hunks)
- backend/src/common/enums/role.enum.ts (1 hunks)
- backend/src/decorator/auth.decorator.ts (1 hunks)
- backend/src/decorator/menu.decorator.ts (1 hunks)
- backend/src/decorator/roles.decorator.ts (1 hunks)
- backend/src/guard/menu.guard.ts (1 hunks)
- backend/src/guard/roles.guard.ts (1 hunks)
- backend/src/init/init-roles.service.ts (1 hunks)
- backend/src/init/init.module.ts (1 hunks)
- backend/src/schema.gql (2 hunks)
- backend/src/user/user.service.ts (1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/app.service.ts
✅ Files skipped from review due to trivial changes (1)
- backend/src/user/user.service.ts
🧰 Additional context used
🪛 Markdownlint
README.md
7-7: null
Images should have alternate text (alt text)(MD045, no-alt-text)
🪛 GitHub Check: autofix
backend/src/app.module.ts
[warning] 13-13:
'RolesGuard' is defined but never used. Allowed unused vars must match /^_/u
[warning] 14-14:
'MenuGuard' is defined but never used. Allowed unused vars must match /^_/ubackend/src/app.resolver.ts
[warning] 1-1:
'Injectable' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (17)
backend/src/common/enums/role.enum.ts (1)
1-3: Consider adding more default roles for better role management.Having only an ADMIN role might lead to overprivileged accounts where users are given admin access when they only need basic permissions. Consider adding more granular roles like USER, GUEST, etc.
export enum DefaultRoles { ADMIN = 'Admin', + USER = 'User', + GUEST = 'Guest', }Let's check if there are any role-based checks in the codebase that might need updating:
backend/src/decorator/roles.decorator.ts (2)
1-4: LGTM! Basic implementation follows NestJS best practices.The decorator implementation correctly uses
SetMetadatafor role-based access control, following NestJS conventions.
3-4: Verify decorator integration and role string consistency.Let's ensure the decorator is properly integrated with the auth system and role strings are used consistently across the codebase.
✅ Verification successful
Let me gather more information about the auth integration and role implementation.
Role decorator is properly integrated and implemented
The verification shows that:
- The roles decorator is correctly integrated with the auth system through
RolesGuardandAuthdecorators- The guard implementation properly validates roles against the user's assigned roles
- There is a clear example of usage in
AppResolverwith the 'Admin' role- The decorator is part of a comprehensive auth system that includes menu-based permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check integration with auth system and role string usage # Check for auth guard implementation echo "Checking for auth guard implementation..." rg -l "RolesGuard" --type ts # Check for role string usage consistency echo "\nChecking role string usage..." rg -B 2 -A 2 "@Roles\(" --type ts # Check for potential role strings defined elsewhere echo "\nChecking for other role definitions..." rg "role.*=.*['\"]" --type tsLength of output: 1445
Script:
#!/bin/bash # Check the roles guard implementation echo "=== Roles Guard Implementation ===" cat backend/src/guard/roles.guard.ts # Check auth decorator implementation echo -e "\n=== Auth Decorator Implementation ===" cat backend/src/decorator/auth.decorator.ts # Check app module configuration echo -e "\n=== App Module Configuration ===" cat backend/src/app.module.ts # Look for actual usage of roles in resolvers echo -e "\n=== Resolver Usage ===" rg -A 5 "class.*Resolver" --type tsLength of output: 7093
backend/src/decorator/menu.decorator.ts (1)
1-4: LGTM! Clean and follows NestJS best practices.The implementation correctly uses
SetMetadatafor custom decorator creation and follows NestJS conventions.backend/src/app.resolver.ts (2)
9-11: Add tests for the resolver.The PR objectives mention tests, but no tests are provided for this resolver. Consider adding unit tests to verify:
- The query returns the expected string
- Role-based access control works correctly
- Error handling for unauthorized access
Would you like me to help generate test cases for this resolver?
9-11: Consider implementing meaningful response.The current implementation returns a static string which appears to be a placeholder. Consider implementing a more meaningful response based on your application's requirements.
Let's check if this is used in production code:
backend/src/init/init.module.ts (2)
4-7: LGTM! Imports are well-organized and necessary.The imports are correctly structured and include all required dependencies for the module's functionality.
9-13: Consider module initialization strategy and visibility.The module structure is correct, but consider the following suggestions:
- If
InitRolesServiceneeds to be used by other modules, consider exporting it:@Module({ imports: [TypeOrmModule.forFeature([Role])], providers: [InitRolesService], + exports: [InitRolesService], })
- Ensure there's a clear initialization trigger (e.g., using
onApplicationBootstraplifecycle hook in the service).Let's verify the initialization implementation:
backend/src/app.module.ts (2)
44-44: Verify AppResolver implementation.The
AppServicehas been replaced withAppResolver. Ensure that all previous AppService functionality has been properly migrated to the new resolver.Let's verify the migration:
✅ Verification successful
Migration from AppService to AppResolver is correct
The verification shows that:
- The original
AppServiceonly had a singlegetHello()method that returned "Hello World!"- The new
AppResolverimplements the same functionality withgetHello()method, with added GraphQL@Querydecorator and role-based access control via@RequireRoles('Admin')- No other functionality existed in
AppServicethat needed migrationThe migration appears to be complete and properly implemented, with additional security improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining AppService references rg "AppService" --type ts # Check AppResolver implementation ast-grep --pattern 'class AppResolver { $$$ }'Length of output: 420
Script:
#!/bin/bash # Let's check the git history to see what was removed/changed git log -p -- backend/src/app.service.ts # Also check for any test files that might show the previous functionality fd "app\.(spec|test)\.ts$" backend/srcLength of output: 1672
36-42: Verify InitModule initialization order.The
InitModuleis placed before other feature modules. Ensure this ordering is intentional, as it may affect the initialization sequence of your application.Let's check if InitModule has any dependencies on other modules:
✅ Verification successful
Let me try different commands to check the InitModule implementation and dependencies:
Let me check the InitModule's contents and its providers/imports to understand its dependencies:
Let me check the InitRolesService to understand what initialization it performs:
InitModule's placement is correct and necessary
The InitModule's placement at the start of the imports is intentional and correct because:
- It implements
OnApplicationBootstraplifecycle hook to initialize default roles- This initialization should happen before other modules that might depend on these roles (like AuthModule)
- The module has proper dependency setup, importing only what it needs (TypeOrmModule with Role entity)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check InitModule dependencies ast-grep --pattern 'class InitModule { $$$ }' # Check if other modules depend on InitModule rg -l "InitModule" --type tsLength of output: 420
Script:
#!/bin/bash # Search for InitModule implementation ast-grep --pattern 'class InitModule { $$$ }' # Find files importing InitModule rg "import.*InitModule" --type ts # Find files referencing InitModule rg "InitModule" -A 3 --type tsLength of output: 782
Script:
#!/bin/bash # Check the complete implementation of init.module.ts cat backend/src/init/init.module.ts # Check for any init services or controllers fd "init\." --type f --exec cat {}Length of output: 1032
Script:
#!/bin/bash # Check InitRolesService implementation cat backend/src/init/init-roles.service.ts # Check if Role model is used by other modules rg "TypeOrmModule\.forFeature\(\[.*Role.*\]\)" --type tsLength of output: 1825
backend/src/init/init-roles.service.ts (1)
43-43: Verify the security implications of empty menus array.Initializing roles with an empty menus array might grant unintended access if the application's authorization logic doesn't properly handle this case. Consider:
- Defining explicit default menu permissions for each role type
- Adding validation to ensure roles can't be created without specific menu permissions
✅ Verification successful
Let me gather more information about the authorization logic and menu permissions handling.
Empty menus array is safely handled by the authorization system
The MenuGuard properly validates menu access by checking if a user has the required menu permissions through
role.menus?.some((menu) => menu.path === requiredPath). An empty menus array will correctly result in denied access, as the guard will throw an UnauthorizedException when the user doesn't have the required menu permission.Key validations in place:
- MenuGuard explicitly checks menu permissions using role-menu relationships
- Empty menus array results in
hasMenuAccess = false- Proper error handling with UnauthorizedException
- Detailed logging of unauthorized access attempts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how empty menus array is handled in authorization logic rg -A 5 "menus\W*(=|:)" --type tsLength of output: 2701
Script:
#!/bin/bash # Check for menu permission validation and authorization checks rg -A 5 "checkPermission|hasPermission|canAccess|authorize" --type ts # Look for menu-related guards or interceptors ast-grep --pattern 'class $_ implements CanActivate { $$$ }'Length of output: 19150
backend/src/schema.gql (2)
Line range hint
1-124: Schema maintains good consistency and type safetyThe schema demonstrates good practices:
- Consistent use of ID scalar type for all identifiers
- Clear type definitions with appropriate nullability
- Well-structured input types and responses
69-69: Verify the impact of makingpathnon-nullableMaking the
pathfield non-nullable is a breaking change that could affect existing clients and data. Please ensure:
- No existing projects have null paths in the database
- All frontend components handle this field as required
- API clients are notified of this breaking change
#!/bin/bash # Search for frontend components that might be affected by this change rg -l 'project.*path' --type ts --type tsx # Search for any null checks on project.path that might need updating rg 'project\?.path|project\.path\s*===?\s*null' --type ts --type tsxbackend/src/decorator/auth.decorator.ts (1)
1-1: Add missing article 'a' in the comment for clarityIn line 1, adding 'a' before 'combination' improves readability.
[typo]
Apply this diff:
-// This is combination of RolesGuard, MenuGuard, roles.decorator.ts, menu.decorator.ts, auth.decorator.ts +// This is a combination of RolesGuard, MenuGuard, roles.decorator.ts, menu.decorator.ts, auth.decorator.tsbackend/src/auth/role/role.model.ts (3)
17-17: Verify database migration for UUID primary keysChanging the primary key generation strategy to
'uuid'for theidfield may affect existing database records and relations. Please ensure that database migrations are updated to handle this change, and that any references to theidfield are compatible with UUIDs.
21-21: Ensure uniqueness constraint onnamecolumn is properly handledAdding
unique: trueto thenamecolumn enforces a uniqueness constraint at the database level. If there are existing duplicate role names in the database, this change could cause migration failures or runtime errors. Please verify that the data complies with this constraint.
24-26: Expose thedescriptionfield in the GraphQL schemaThe
descriptionfield has been added to theRoleentity. Ensure that this field is intentionally exposed in the GraphQL API and that clients are aware of this new field. Update any relevant DTOs, input types, and documentation as needed.
| export const ROLES_KEY = 'roles'; | ||
| export const Roles = (...roles: string[]) => SetMetadata(ROLES_KEY, roles); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing type safety with an enum for roles.
The current implementation using string literals for roles could lead to typos and maintenance issues. Consider using an enum to ensure type safety and maintainability.
Here's a suggested implementation:
+export enum UserRole {
+ ADMIN = 'Admin',
+ USER = 'User',
+ // Add other roles as needed
+}
export const ROLES_KEY = 'roles';
-export const Roles = (...roles: string[]) => SetMetadata(ROLES_KEY, roles);
+export const Roles = (...roles: UserRole[]) => SetMetadata(ROLES_KEY, roles);Usage example:
@Roles(UserRole.ADMIN)
getHello() {
// ...
}| export const MENU_PATH_KEY = 'menuPath'; | ||
| export const Menu = (path: string) => SetMetadata(MENU_PATH_KEY, path); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing documentation and type safety.
While the implementation is correct, consider these improvements:
- Add JSDoc documentation explaining the decorator's purpose and usage
- Consider using a union type for valid menu paths to prevent typos
Here's a suggested enhancement:
+/** Valid menu paths in the application */
+export type MenuPath = '/dashboard' | '/users' | '/settings'; // Add your valid paths
+
+/**
+ * Decorator that marks a route handler with a menu path for authorization.
+ * Used in conjunction with MenuGuard to control access based on user permissions.
+ *
+ * @param path - The menu path required to access this route
+ * @example
+ * @Menu('/dashboard')
+ * @Get()
+ * getDashboard() {
+ * return this.dashboardService.getData();
+ * }
+ */
export const MENU_PATH_KEY = 'menuPath';
-export const Menu = (path: string) => SetMetadata(MENU_PATH_KEY, path);
+export const Menu = (path: MenuPath) => SetMetadata(MENU_PATH_KEY, path);Committable suggestion was skipped due to low confidence.
| @Resolver() | ||
| export class AppResolver { | ||
| @Query(() => String) | ||
| @RequireRoles('Admin') | ||
| getHello(): string { | ||
| return 'Hello World!'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using role enum instead of hard-coded string.
Using string literals for role names can lead to maintenance issues. Consider using an enum for role definitions to ensure type safety and maintainability.
Create a roles enum and use it in the decorator:
// src/common/enums/roles.enum.ts
export enum Roles {
ADMIN = 'Admin',
// Add other roles as needed
}
// In this file:
@RequireRoles(Roles.ADMIN)| import { Injectable } from '@nestjs/common'; | ||
| import { Query, Resolver } from '@nestjs/graphql'; | ||
| import { RequireRoles } from './decorator/auth.decorator'; | ||
|
|
There was a problem hiding this comment.
Remove unused Injectable import.
The Injectable decorator is not being used in this resolver class. The @Resolver() decorator already provides dependency injection capability in NestJS.
Apply this diff to remove the unused import:
-import { Injectable } from '@nestjs/common';
import { Query, Resolver } from '@nestjs/graphql';
import { RequireRoles } from './decorator/auth.decorator';📝 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.
| import { Injectable } from '@nestjs/common'; | |
| import { Query, Resolver } from '@nestjs/graphql'; | |
| import { RequireRoles } from './decorator/auth.decorator'; | |
| import { Query, Resolver } from '@nestjs/graphql'; | |
| import { RequireRoles } from './decorator/auth.decorator'; | |
🧰 Tools
🪛 GitHub Check: autofix
[warning] 1-1:
'Injectable' is defined but never used. Allowed unused vars must match /^_/u
| import { RolesGuard } from './guard/roles.guard'; | ||
| import { MenuGuard } from './guard/menu.guard'; |
There was a problem hiding this comment.
Remove unused imports or implement guards.
The RolesGuard and MenuGuard are imported but never used in this module. If these guards are intended for route protection, they need to be properly implemented using APP_GUARD provider.
Apply this diff to implement the guards globally:
import { RolesGuard } from './guard/roles.guard';
import { MenuGuard } from './guard/menu.guard';
import { User } from './user/user.model';
import { AppResolver } from './app.resolver';
+import { APP_GUARD } from '@nestjs/core';
@Module({
imports: [
// ... existing imports
],
- providers: [AppResolver],
+ providers: [
+ AppResolver,
+ {
+ provide: APP_GUARD,
+ useClass: RolesGuard,
+ },
+ {
+ provide: APP_GUARD,
+ useClass: MenuGuard,
+ },
+ ],
})Alternatively, if these guards are not needed, remove the unused imports.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: autofix
[warning] 13-13:
'RolesGuard' is defined but never used. Allowed unused vars must match /^_/u
[warning] 14-14:
'MenuGuard' is defined but never used. Allowed unused vars must match /^_/u
| const roleIndex = user.roles?.findIndex((role) => role.id === roleId); | ||
|
|
||
| if (roleIndex === -1) { | ||
| throw new NotFoundException( | ||
| `Role with ID "${roleId}" not found in user's roles`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle potential undefined user.roles to prevent runtime errors
In the removeRoleFromUser method, if user.roles is undefined or empty, calling findIndex on it could lead to unexpected behavior. Since user.roles?.findIndex returns undefined when user.roles is undefined, the comparison roleIndex === -1 will not catch this case.
Apply this diff to address the issue:
+ if (!user.roles || user.roles.length === 0) {
+ throw new NotFoundException(`No roles assigned to user "${user.username}"`);
+ }
const roleIndex = user.roles.findIndex((role) => role.id === roleId);
if (roleIndex === -1) {
throw new NotFoundException(
`Role with ID "${roleId}" not found in user's roles`,
);
}📝 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.
| const roleIndex = user.roles?.findIndex((role) => role.id === roleId); | |
| if (roleIndex === -1) { | |
| throw new NotFoundException( | |
| `Role with ID "${roleId}" not found in user's roles`, | |
| ); | |
| } | |
| if (!user.roles || user.roles.length === 0) { | |
| throw new NotFoundException(`No roles assigned to user "${user.username}"`); | |
| } | |
| const roleIndex = user.roles.findIndex((role) => role.id === roleId); | |
| if (roleIndex === -1) { | |
| throw new NotFoundException( | |
| `Role with ID "${roleId}" not found in user's roles`, | |
| ); | |
| } |
| const menuIndex = role.menus?.findIndex((menu) => menu.id === menuId); | ||
|
|
||
| if (menuIndex === -1) { | ||
| throw new NotFoundException( | ||
| `Menu with ID "${menuId}" not found in role "${role.name}"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle potential undefined role.menus to prevent runtime errors
In the removeMenuFromRole method, if role.menus is undefined or empty, calling findIndex on it could lead to unexpected behavior. Since role.menus?.findIndex returns undefined when role.menus is undefined, the comparison menuIndex === -1 will not catch this case.
Apply this diff to address the issue:
+ if (!role.menus || role.menus.length === 0) {
+ throw new NotFoundException(`No menus assigned to role "${role.name}"`);
+ }
const menuIndex = role.menus.findIndex((menu) => menu.id === menuId);
if (menuIndex === -1) {
throw new NotFoundException(
`Menu with ID "${menuId}" not found in role "${role.name}"`,
);
}📝 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.
| const menuIndex = role.menus?.findIndex((menu) => menu.id === menuId); | |
| if (menuIndex === -1) { | |
| throw new NotFoundException( | |
| `Menu with ID "${menuId}" not found in role "${role.name}"`, | |
| ); | |
| } | |
| if (!role.menus || role.menus.length === 0) { | |
| throw new NotFoundException(`No menus assigned to role "${role.name}"`); | |
| } | |
| const menuIndex = role.menus.findIndex((menu) => menu.id === menuId); | |
| if (menuIndex === -1) { | |
| throw new NotFoundException( | |
| `Menu with ID "${menuId}" not found in role "${role.name}"`, | |
| ); | |
| } |
| @Entity() | ||
| export class Role { |
There was a problem hiding this comment.
Potential loss of inherited properties from SystemBaseModel
By removing the inheritance from SystemBaseModel, the Role class may no longer have access to the properties and methods defined in SystemBaseModel. This could impact functionality, especially if SystemBaseModel includes common fields like createdAt, updatedAt, or other base properties.
|
|
||
| @ManyToMany(() => Menu) | ||
| @Field(() => [Menu], { nullable: true }) | ||
| @ManyToMany(() => Menu, { eager: true }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance implications of eager loading menus
Setting eager: true on the ManyToMany relationship with Menu will automatically load all related Menu entities whenever a Role is fetched. This can lead to performance overhead and increased response times, especially if a role is associated with many menus. Consider using lazy loading or fetching menus explicitly when needed.
| @ManyToMany(() => User, (user) => user.roles) | ||
| @JoinTable({ | ||
| name: 'user_roles', | ||
| joinColumn: { | ||
| name: 'role_id', | ||
| referencedColumnName: 'id', | ||
| }, | ||
| inverseJoinColumn: { | ||
| name: 'user_id', | ||
| referencedColumnName: 'id', | ||
| }, | ||
| }) | ||
| users?: User[]; |
There was a problem hiding this comment.
💡 Codebase verification
Remove @JoinTable decorator from Role entity as it's already defined in User entity
The @JoinTable decorator is currently defined on both sides of the ManyToMany relationship between User and Role entities. This should be defined only on one side to avoid conflicts. Since it's already properly configured in the User entity, you should remove the @JoinTable decorator and its configuration from the Role entity, keeping only the @ManyToMany decorator.
🔗 Analysis chain
Verify the ManyToMany relationship and @JoinTable usage with User
In a ManyToMany relationship, the @JoinTable decorator should be used on only one side of the relationship. Currently, the Role entity uses @JoinTable for the users relationship. Please verify that the User entity does not also declare @JoinTable for the roles relationship to prevent unintended behavior.
Run the following script to check the User entity for @JoinTable usage:
Let me gather more context about the User entity's relationship implementation to make a definitive assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'User' entity has a ManyToMany relationship with 'Role' and check for '@JoinTable'
# Find 'user.model.ts' files
fd 'user.model.ts' --type f --exclude 'node_modules' --hidden
# Check each file for '@ManyToMany' with 'Role' and '@JoinTable'
fd 'user.model.ts' --type f --exclude 'node_modules' --hidden --exec bash -c '
echo "Checking $1"
rg -A5 "@ManyToMany.*Role" "$1" | rg "@JoinTable"
' bash {}
Length of output: 309
Script:
#!/bin/bash
# Get the full context of the ManyToMany relationship in User entity
rg -B2 -A10 "@ManyToMany.*Role" backend/src/user/user.model.ts
Length of output: 318
#55) … (#25) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive setup for a Next.js application with TypeScript, including configuration files for ESLint, PostCSS, Tailwind CSS, and TypeScript. - Added a new layout and home page component with responsive design and optimized image handling. - Implemented a utility function for managing Tailwind CSS class names. - **Documentation** - Created a README file outlining project setup, development instructions, and deployment information. - **Chores** - Added `.gitignore` to maintain a clean repository by excluding unnecessary files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: ZHallen122 <[email protected]>
#5
This commit adds the project frontend API along with its corresponding tests. It also fixes an issue in the backend project API where the user ID should be a number.
Summary by CodeRabbit
Release Notes
New Features
getHelloGraphQL query for a simple greeting response.AuthService.MenuGuardandRolesGuardfor enhanced access control.InitRolesServicefor initializing default roles on application startup.Bug Fixes
README.mdfor better readability.Documentation
Chores
UserService.