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

feat: Add project frontend API with tests and fix backend project API #25

Merged
merged 2 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ Still on progress

CodeFox
LOGO
![](./assets/WechatIMG1000.svg)
![](./assets/WechatIMG1000.svg)
23 changes: 13 additions & 10 deletions backend/src/app.module.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import { ApolloDriver, ApolloDriverConfig } from '@nestjs/apollo';
import { Module } from '@nestjs/common';
import { AppService } from './app.service';
import { ConfigModule } from '@nestjs/config';
import { GraphQLModule } from '@nestjs/graphql';
import { ApolloDriver, ApolloDriverConfig } from '@nestjs/apollo';
import { join } from 'path';
import { TypeOrmModule } from '@nestjs/typeorm';
import { UserModule } from './user/user.module';
import { User } from './user/user.model';
import { join } from 'path';
import { AuthModule } from './auth/auth.module';
import { ChatModule } from './chat/chat.module';
import { ProjectModule } from './project/project.module';
import { TokenModule } from './token/token.module';
import { ConfigModule, ConfigService } from '@nestjs/config';
import { JwtModule } from '@nestjs/jwt';
import { JwtCacheService } from './auth/jwt-cache.service';
import { ChatModule } from './chat/chat.module';
import { UserModule } from './user/user.module';
import { InitModule } from './init/init.module';
import { RolesGuard } from './guard/roles.guard';

Check warning on line 13 in backend/src/app.module.ts

View workflow job for this annotation

GitHub Actions / autofix

'RolesGuard' is defined but never used. Allowed unused vars must match /^_/u
import { MenuGuard } from './guard/menu.guard';

Check warning on line 14 in backend/src/app.module.ts

View workflow job for this annotation

GitHub Actions / autofix

'MenuGuard' is defined but never used. Allowed unused vars must match /^_/u
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

import { User } from './user/user.model';
import { AppResolver } from './app.resolver';

@Module({
imports: [
Expand All @@ -32,12 +33,14 @@
logging: true,
entities: [__dirname + '/**/*.model{.ts,.js}'],
}),
InitModule,
UserModule,
AuthModule,
ProjectModule,
TokenModule,
ChatModule,
TypeOrmModule.forFeature([User]),
],
providers: [AppService],
providers: [AppResolver],
})
export class AppModule {}
12 changes: 12 additions & 0 deletions backend/src/app.resolver.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { Injectable } from '@nestjs/common';

Check warning on line 1 in backend/src/app.resolver.ts

View workflow job for this annotation

GitHub Actions / autofix

'Injectable' is defined but never used. Allowed unused vars must match /^_/u
import { Query, Resolver } from '@nestjs/graphql';
import { RequireRoles } from './decorator/auth.decorator';

Comment on lines +1 to +4
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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

@Resolver()
export class AppResolver {
@Query(() => String)
@RequireRoles('Admin')
getHello(): string {
return 'Hello World!';
}
}
Comment on lines +5 to +12
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 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)

8 changes: 0 additions & 8 deletions backend/src/app.service.ts

This file was deleted.

247 changes: 246 additions & 1 deletion backend/src/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
ConflictException,
Injectable,
Logger,
NotFoundException,
UnauthorizedException,
} from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
Expand All @@ -11,9 +12,11 @@
import { LoginUserInput } from 'src/user/dto/login-user.input';
import { RegisterUserInput } from 'src/user/dto/register-user.input';
import { User } from 'src/user/user.model';
import { Repository } from 'typeorm';
import { In, Repository } from 'typeorm';
import { CheckTokenInput } from './dto/check-token.input';
import { JwtCacheService } from 'src/auth/jwt-cache.service';
import { Menu } from './menu/menu.model';
import { Role } from './role/role.model';

@Injectable()
export class AuthService {
Expand All @@ -23,6 +26,10 @@
private jwtService: JwtService,
private jwtCacheService: JwtCacheService,
private configService: ConfigService,
@InjectRepository(Menu)
private menuRepository: Repository<Menu>,
@InjectRepository(Role)
private roleRepository: Repository<Role>,
) {}

async register(registerUserInput: RegisterUserInput): Promise<User> {
Expand Down Expand Up @@ -77,7 +84,7 @@
await this.jwtService.verifyAsync(params.token);
return this.jwtCacheService.isTokenStored(params.token);
} catch (error) {
console.log(error);

Check warning on line 87 in backend/src/auth/auth.service.ts

View workflow job for this annotation

GitHub Actions / autofix

Unexpected console statement
return false;
}
}
Expand All @@ -85,7 +92,7 @@
Logger.log('logout token', token);
try {
await this.jwtService.verifyAsync(token);
} catch (error) {

Check warning on line 95 in backend/src/auth/auth.service.ts

View workflow job for this annotation

GitHub Actions / autofix

'error' is defined but never used
return false;
}

Expand All @@ -95,4 +102,242 @@
this.jwtCacheService.removeToken(token);
return true;
}

async assignMenusToRole(roleId: string, menuIds: string[]): Promise<Role> {
// Find the role with existing menus
const role = await this.roleRepository.findOne({
where: { id: roleId },
relations: ['menus'],
});

if (!role) {
throw new NotFoundException(`Role with ID "${roleId}" not found`);
}

// Find all menus
const menus = await this.menuRepository.findByIds(menuIds);

if (menus.length !== menuIds.length) {
throw new NotFoundException('Some menus were not found');
}
Comment on lines +118 to +122
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use consistent repository methods for querying entities

In assignMenusToRole, you're using findByIds to retrieve menus. For consistency and to accommodate future updates of TypeORM, consider using findBy with the In operator, as used in other parts of the code.

Apply this diff to update the code:

-        const menus = await this.menuRepository.findByIds(menuIds);
+        const menus = await this.menuRepository.findBy({ id: In(menuIds) });

Committable suggestion was skipped due to low confidence.


if (!role.menus) {
role.menus = [];
}

const newMenus = menus.filter(
(menu) => !role.menus.some((existingMenu) => existingMenu.id === menu.id),
);

if (newMenus.length === 0) {
throw new ConflictException(
'All specified menus are already assigned to this role',
);
}

role.menus.push(...newMenus);

try {
await this.roleRepository.save(role);
Logger.log(
`${newMenus.length} menus assigned to role ${role.name} successfully`,
);

return await this.roleRepository.findOne({
where: { id: roleId },
relations: ['menus'],
});
} catch (error) {
Logger.error(`Failed to assign menus to role: ${error.message}`);
throw error;
}
}

async removeMenuFromRole(roleId: string, menuId: string): Promise<Role> {
const role = await this.roleRepository.findOne({
where: { id: roleId },
relations: ['menus'],
});

if (!role) {
throw new NotFoundException(`Role with ID "${roleId}" not found`);
}

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}"`,
);
}
Comment on lines +166 to +172
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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}"`,
);
}


role.menus.splice(menuIndex, 1);

try {
await this.roleRepository.save(role);
Logger.log(`Menu removed from role ${role.name} successfully`);

return await this.roleRepository.findOne({
where: { id: roleId },
relations: ['menus'],
});
} catch (error) {
Logger.error(`Failed to remove menu from role: ${error.message}`);
throw error;
}
}

async assignRoles(userId: string, roleIds: string[]): Promise<User> {
const user = await this.userRepository.findOne({
where: { id: userId },
relations: ['roles'],
});

if (!user) {
throw new NotFoundException(`User with ID "${userId}" not found`);
}

const roles = await this.roleRepository.findBy({ id: In(roleIds) });

if (roles.length !== roleIds.length) {
throw new NotFoundException('Some roles were not found');
}

if (!user.roles) {
user.roles = [];
}

const newRoles = roles.filter(
(role) => !user.roles.some((existingRole) => existingRole.id === role.id),
);

if (newRoles.length === 0) {
throw new ConflictException(
'All specified roles are already assigned to this user',
);
}

user.roles.push(...newRoles);

try {
await this.userRepository.save(user);
Logger.log(
`${newRoles.length} roles assigned to user ${user.username} successfully`,
);

return await this.userRepository.findOne({
where: { id: userId },
relations: ['roles'],
});
} catch (error) {
Logger.error(`Failed to assign roles to user: ${error.message}`);
throw error;
}
}

async removeRoleFromUser(userId: string, roleId: string): Promise<User> {
const user = await this.userRepository.findOne({
where: { id: userId },
relations: ['roles'],
});

if (!user) {
throw new NotFoundException(`User with ID "${userId}" not found`);
}

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`,
);
}
Comment on lines +248 to +254
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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`,
);
}


user.roles.splice(roleIndex, 1);

try {
await this.userRepository.save(user);
Logger.log(`Role removed from user ${user.username} successfully`);

return await this.userRepository.findOne({
where: { id: userId },
relations: ['roles'],
});
} catch (error) {
Logger.error(`Failed to remove role from user: ${error.message}`);
throw error;
}
}

async getUserRolesAndMenus(userId: string): Promise<{
roles: Role[];
menus: Menu[];
}> {
const user = await this.userRepository.findOne({
where: { id: userId },
relations: ['roles', 'roles.menus'],
});

if (!user) {
throw new NotFoundException(`User with ID "${userId}" not found`);
}

const userRoles = user.roles || [];

// Get unique menus across all roles
const userMenus = Array.from(
new Map(
userRoles
.flatMap((role) => role.menus || [])
.map((menu) => [menu.id, menu]),
).values(),
);
Comment on lines +288 to +294
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify the aggregation of unique menus

In getUserRolesAndMenus and getUserMenus, you're using a Map to collect unique menus. This can be simplified for better readability by using a Set to automatically handle uniqueness.

Refactor the code to simplify the aggregation:

         // Get unique menus across all roles
-        const userMenus = Array.from(
-          new Map(
-            userRoles
-              .flatMap((role) => role.menus || [])
-              .map((menu) => [menu.id, menu]),
-          ).values(),
-        );
+        const userMenus = Array.from(
+          new Set(
+            userRoles.flatMap((role) => role.menus || [])
+          )
+        );

Also applies to: 331-337


return {
roles: userRoles,
menus: userMenus,
};
}

async getUserRoles(userId: string): Promise<{
roles: Role[];
}> {
const user = await this.userRepository.findOne({
where: { id: userId },
relations: ['roles'],
});

if (!user) {
throw new NotFoundException(`User with ID "${userId}" not found`);
}

return {
roles: user.roles || [],
};
}

async getUserMenus(userId: string): Promise<{
menus: Menu[];
}> {
const user = await this.userRepository.findOne({
where: { id: userId },
relations: ['roles', 'roles.menus'],
});

if (!user) {
throw new NotFoundException(`User with ID "${userId}" not found`);
}

const userMenus = Array.from(
new Map(
(user.roles || [])
.flatMap((role) => role.menus || [])
.map((menu) => [menu.id, menu]),
).values(),
);

return {
menus: userMenus,
};
}
}
Loading
Loading