From 0cfdab95eebe2323719ee7e8b0eb8d03aa8311c8 Mon Sep 17 00:00:00 2001 From: wangziqi Date: Thu, 9 Jul 2026 11:28:11 +0800 Subject: [PATCH] fix: add DTO validation, transactional imports, role-not-found handling, null role guard --- .../src/pages/IntegrationConfig/index.tsx | 399 +++--------------- apps/server/src/sync/dto/import-users.dto.ts | 30 ++ apps/server/src/sync/sync.controller.ts | 5 +- apps/server/src/sync/sync.service.spec.ts | 183 ++++---- apps/server/src/sync/sync.service.ts | 81 ++-- 5 files changed, 241 insertions(+), 457 deletions(-) create mode 100644 apps/server/src/sync/dto/import-users.dto.ts diff --git a/apps/admin/src/pages/IntegrationConfig/index.tsx b/apps/admin/src/pages/IntegrationConfig/index.tsx index 659b294..9c530f7 100644 --- a/apps/admin/src/pages/IntegrationConfig/index.tsx +++ b/apps/admin/src/pages/IntegrationConfig/index.tsx @@ -1,13 +1,10 @@ -import React, { useEffect, useState, useMemo } from 'react'; +import React, { useEffect, useMemo, useState } from 'react'; import { Card, Form, Input, Button, Space, message, Spin, Switch, Alert, Descriptions, Tag, - Tabs, Drawer, Tree, Checkbox, Select, TreeSelect, } from 'antd'; import { SaveOutlined, ApiOutlined, CheckCircleOutlined, CloseCircleOutlined, - SyncOutlined, ReloadOutlined, } from '@ant-design/icons'; -import type { DataNode } from 'antd/es/tree'; import api from '../../api'; interface DingTalkConfig { @@ -17,68 +14,11 @@ interface DingTalkConfig { startEnable: boolean; } -interface DingOrgTreeNodeExt { - id: number; - name: string; - parentId: number; - children: DingOrgTreeNodeExt[]; - users: Array<{ userid: string; name: string; mobile: string }>; -} - -interface RoleItem { - id: number; - name: string; - status: number; -} - -interface OrgTreeNodeRaw { - id: number; - name: string; - children?: OrgTreeNodeRaw[]; -} - -interface OrgTreeResponse { - success: boolean; - data: OrgTreeNodeRaw[]; -} - -interface OrgTreeWithUsersResponse { - success: boolean; - data: DingOrgTreeNodeExt[]; -} - -interface ImportUsersResponse { - teacherCount: number; - studentCount: number; - skipped: number; -} - -const UserTreeNode: React.FC<{ - u: { userid: string; name: string; mobile: string }; - isTeacher: boolean; - onToggle: () => void; - roleId: number | undefined; - defaultRoleId: number | null; - roles: Array<{ id: number; name: string }>; - onRoleChange: (roleId: number) => void; -}> = React.memo(({ u, isTeacher, onToggle, roleId, defaultRoleId, roles, onRoleChange }) => ( - - 老师 - {u.name} - {u.mobile && {u.mobile}} - {isTeacher && ( - - - - - - - - - - - - - - - - - - ), - }, - ...syncTabItems, - ]; - return ( @@ -426,7 +90,54 @@ const IntegrationConfigPage: React.FC = () => { {verified === false && } color="error">未连接} }> - + + {config && ( + + {config.corpId || '-'} + {config.agentId || '-'} + + + {config.startEnable ? '已启用' : '未启用'} + + + + )} + + + +
+ + + + + + + + + + + + + + + + +
+ ); }; diff --git a/apps/server/src/sync/dto/import-users.dto.ts b/apps/server/src/sync/dto/import-users.dto.ts new file mode 100644 index 0000000..1a137cd --- /dev/null +++ b/apps/server/src/sync/dto/import-users.dto.ts @@ -0,0 +1,30 @@ +import { + IsArray, + IsString, + IsNumber, + IsOptional, + ValidateNested, +} from 'class-validator'; +import { Type } from 'class-transformer'; + +export class ImportUserItemDto { + @IsString() + dingUserId: string; + + @IsString() + name: string; + + @IsString() + mobile: string; + + @IsOptional() + @IsNumber() + roleId: number | null; +} + +export class ImportUsersDto { + @IsArray() + @ValidateNested({ each: true }) + @Type(() => ImportUserItemDto) + users: ImportUserItemDto[]; +} diff --git a/apps/server/src/sync/sync.controller.ts b/apps/server/src/sync/sync.controller.ts index 1e2ce04..64d30e7 100644 --- a/apps/server/src/sync/sync.controller.ts +++ b/apps/server/src/sync/sync.controller.ts @@ -3,6 +3,7 @@ import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; import { RequirePermission } from '../auth/decorators/permission.decorator'; import { SyncService } from './sync.service'; import type { SyncPlatform } from '../entities/sync-log.entity'; +import { ImportUsersDto } from './dto/import-users.dto'; @UseGuards(JwtAuthGuard) @Controller('sync') @@ -48,13 +49,15 @@ export class SyncController { const tree = await this.syncService.getDingTalkOrgTreeWithUsers(rootId); return { success: true, data: tree }; } + /** 导入钉钉用户：老师分配角色，学生创建 Student */ @Post('dingtalk/import-users') @RequirePermission('sync:trigger') - async importDingTalkUsers(@Body() body: { users: Array<{ dingUserId: string; name: string; mobile: string; roleId: number | null }> }) { + async importDingTalkUsers(@Body() body: ImportUsersDto) { const result = await this.syncService.importDingTalkUsers(body.users); return { success: true, ...result }; } + @Get('logs') @RequirePermission('sync:read') async getLogs( diff --git a/apps/server/src/sync/sync.service.spec.ts b/apps/server/src/sync/sync.service.spec.ts index ab9f312..91531e9 100644 --- a/apps/server/src/sync/sync.service.spec.ts +++ b/apps/server/src/sync/sync.service.spec.ts @@ -1,6 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; import { SyncService, ImportUserDto } from './sync.service'; import { SyncLog, SyncState, UserDingMapping } from '../entities'; import { User } from '../entities/user.entity'; @@ -11,7 +11,24 @@ import { WeComService } from '../integration/wecom.service'; import { AttendanceImportService } from '../attendance/attendance-import.service'; import { ScheduleSyncService } from './schedule-sync.service'; -describe('SyncService — new methods', () => { +// ── EntityManager mock helpers ── + +interface ManagerMock { + create: jest.Mock; + save: jest.Mock; + findOne: jest.Mock; +} + +function mockManager(overrides: Partial = {}): ManagerMock { + return { + create: jest.fn(), + save: jest.fn(), + findOne: jest.fn(), + ...overrides, + }; +} + +describe('SyncService — importDingTalkUsers (transactional)', () => { let service: SyncService; let mappingRepo: jest.Mocked< @@ -20,10 +37,15 @@ describe('SyncService — new methods', () => { let userRepo: jest.Mocked, 'create' | 'save'>>; let studentRepo: jest.Mocked, 'create' | 'save'>>; let roleRepo: jest.Mocked, 'findOne'>>; + let dataSourceMock: jest.Mocked>; let dingTalkService: jest.Mocked>; + let mgr: ManagerMock; + beforeEach(async () => { + mgr = mockManager(); + mappingRepo = { findOne: jest.fn(), create: jest.fn(), @@ -45,6 +67,12 @@ describe('SyncService — new methods', () => { findOne: jest.fn(), }; + dataSourceMock = { + transaction: jest.fn().mockImplementation( + async (cb: (manager: ManagerMock) => Promise) => cb(mgr), + ), + }; + dingTalkService = { fetchOrgTreeWithUsers: jest.fn(), syncAll: jest.fn(), @@ -64,6 +92,7 @@ describe('SyncService — new methods', () => { { provide: getRepositoryToken(User), useValue: userRepo }, { provide: getRepositoryToken(Student), useValue: studentRepo }, { provide: getRepositoryToken(Role), useValue: roleRepo }, + { provide: DataSource, useValue: dataSourceMock }, { provide: DingTalkService, useValue: dingTalkService }, { provide: WeComService, useValue: mockWeComService }, { provide: AttendanceImportService, useValue: mockAttendanceImportService }, @@ -90,15 +119,13 @@ describe('SyncService — new methods', () => { it('imports teacher when roleId is a number (role found)', async () => { const mockRole = { id: 5, name: 'Teacher' } as Role; - roleRepo.findOne.mockResolvedValue(mockRole); + mgr.findOne.mockResolvedValue(mockRole); const mockUser = { id: 10 } as User; - userRepo.create.mockReturnValue(mockUser); - userRepo.save.mockResolvedValue(mockUser); + mgr.create.mockReturnValue(mockUser); + mgr.save.mockResolvedValue(mockUser); mappingRepo.findOne.mockResolvedValue(null); - mappingRepo.create.mockReturnValue({} as UserDingMapping); - mappingRepo.save.mockResolvedValue({} as UserDingMapping); const users: ImportUserDto[] = [ { dingUserId: 'user1', name: 'Zhang San', mobile: '13800001111', roleId: 5 }, @@ -109,27 +136,23 @@ describe('SyncService — new methods', () => { expect(result.teacherCount).toBe(1); expect(result.studentCount).toBe(0); expect(result.skipped).toBe(0); + expect(result.warnings).toEqual([]); - expect(roleRepo.findOne).toHaveBeenCalledWith({ where: { id: 5 } }); - expect(userRepo.save).toHaveBeenCalledWith( + expect(dataSourceMock.transaction).toHaveBeenCalledTimes(1); + expect(mgr.findOne).toHaveBeenCalledWith(Role, { where: { id: 5 } }); + expect(mgr.save).toHaveBeenCalledWith( expect.objectContaining({ roles: [mockRole] }), ); }); - it('imports teacher when roleId is a number but role not found (creates student as fallback)', async () => { - roleRepo.findOne.mockResolvedValue(null); + it('skips user when role not found (I1 fix — warns + skip, not silent teacher)', async () => { + mgr.findOne.mockResolvedValue(null); const mockUser = { id: 11 } as User; - userRepo.create.mockReturnValue(mockUser); - userRepo.save.mockResolvedValue(mockUser); - - const mockStudent = { id: 31 } as Student; - studentRepo.create.mockReturnValue(mockStudent); - studentRepo.save.mockResolvedValue(mockStudent); + mgr.create.mockReturnValue(mockUser); + mgr.save.mockResolvedValue(mockUser); mappingRepo.findOne.mockResolvedValue(null); - mappingRepo.create.mockReturnValue({} as UserDingMapping); - mappingRepo.save.mockResolvedValue({} as UserDingMapping); const users: ImportUserDto[] = [ { dingUserId: 'user2', name: 'Li Si', mobile: '13800002222', roleId: 999 }, @@ -137,35 +160,31 @@ describe('SyncService — new methods', () => { const result = await service.importDingTalkUsers(users); + // I1: role not found → skip with warning, NOT counted as teacher expect(result.teacherCount).toBe(0); - expect(result.studentCount).toBe(1); - expect(result.skipped).toBe(0); + expect(result.studentCount).toBe(0); + expect(result.skipped).toBe(1); + expect(result.warnings).toEqual([ + '角色 id=999 不存在，跳过用户 Li Si(user2)', + ]); - // User should be saved once (no second save for role assignment) - expect(userRepo.save).toHaveBeenCalledTimes(1); - // Student record should have been created as fallback - expect(studentRepo.create).toHaveBeenCalledWith( - expect.objectContaining({ - name: 'Li Si', - userId: 11, - status: 'active', - }), - ); - expect(studentRepo.save).toHaveBeenCalled(); + expect(dataSourceMock.transaction).toHaveBeenCalledTimes(1); }); it('imports student when roleId is null', async () => { const mockUser = { id: 20 } as User; - userRepo.create.mockReturnValue(mockUser); - userRepo.save.mockResolvedValue(mockUser); + mgr.create.mockReturnValueOnce(mockUser); + mgr.save.mockResolvedValueOnce(mockUser); const mockStudent = { id: 30 } as Student; - studentRepo.create.mockReturnValue(mockStudent); - studentRepo.save.mockResolvedValue(mockStudent); + mgr.create.mockReturnValueOnce(mockStudent); + mgr.save.mockResolvedValueOnce(mockStudent); + + // mapping create+save also calls create/save + mgr.create.mockReturnValueOnce({} as UserDingMapping); + mgr.save.mockResolvedValueOnce({} as UserDingMapping); mappingRepo.findOne.mockResolvedValue(null); - mappingRepo.create.mockReturnValue({} as UserDingMapping); - mappingRepo.save.mockResolvedValue({} as UserDingMapping); const users: ImportUserDto[] = [ { dingUserId: 'user3', name: 'Wang Wu', mobile: '', roleId: null }, @@ -176,15 +195,20 @@ describe('SyncService — new methods', () => { expect(result.studentCount).toBe(1); expect(result.teacherCount).toBe(0); expect(result.skipped).toBe(0); + expect(result.warnings).toEqual([]); - expect(studentRepo.create).toHaveBeenCalledWith( + // Verify Student was created via manager + const studentCreateCalls = mgr.create.mock.calls.filter( + ([entity]) => entity === Student, + ); + expect(studentCreateCalls.length).toBe(1); + expect(studentCreateCalls[0][1]).toEqual( expect.objectContaining({ name: 'Wang Wu', userId: 20, status: 'active', }), ); - expect(studentRepo.save).toHaveBeenCalled(); }); it('skips user when mapping already exists', async () => { @@ -199,25 +223,30 @@ describe('SyncService — new methods', () => { expect(result.skipped).toBe(1); expect(result.teacherCount).toBe(0); expect(result.studentCount).toBe(0); - expect(userRepo.create).not.toHaveBeenCalled(); + expect(result.warnings).toEqual([]); + expect(dataSourceMock.transaction).not.toHaveBeenCalled(); }); it('handles per-user errors gracefully — one failure does not block others', async () => { - // First user fails, second succeeds mappingRepo.findOne.mockResolvedValue(null); - mappingRepo.create.mockReturnValue({} as UserDingMapping); - mappingRepo.save.mockResolvedValue({} as UserDingMapping); - userRepo.create - .mockReturnValueOnce(new Error('DB error') as unknown as User) - .mockReturnValueOnce({ id: 40 } as User); - - userRepo.save - .mockRejectedValueOnce(new Error('DB error')) - .mockResolvedValueOnce({ id: 40 } as User); - - studentRepo.create.mockReturnValue({} as Student); - studentRepo.save.mockResolvedValue({} as Student); + // First transaction throws, second succeeds + dataSourceMock.transaction + .mockImplementationOnce(async () => { + throw new Error('DB error'); + }) + .mockImplementationOnce(async (cb) => { + const freshMgr = mockManager(); + const mockUser = { id: 40 } as User; + freshMgr.create.mockReturnValueOnce(mockUser); + freshMgr.save.mockResolvedValueOnce(mockUser); + const mockStudent = { id: 30 } as Student; + freshMgr.create.mockReturnValueOnce(mockStudent); + freshMgr.save.mockResolvedValueOnce(mockStudent); + freshMgr.create.mockReturnValueOnce({} as UserDingMapping); + freshMgr.save.mockResolvedValueOnce({} as UserDingMapping); + await cb(freshMgr); + }); const users: ImportUserDto[] = [ { dingUserId: 'fail', name: 'Fail User', mobile: '', roleId: null }, @@ -228,31 +257,41 @@ describe('SyncService — new methods', () => { expect(result.studentCount).toBe(1); expect(result.skipped).toBe(0); - - // The first user's mapping should not be saved, but the second's should - expect(mappingRepo.save).toHaveBeenCalledTimes(1); + expect(result.warnings).toEqual([]); + expect(dataSourceMock.transaction).toHaveBeenCalledTimes(2); }); it('counts mixed teacher/student/skipped correctly', async () => { const mockRole = { id: 1, name: 'Teacher Role' } as Role; - roleRepo.findOne.mockResolvedValue(mockRole); - const mockUser = { id: 50 } as User; - userRepo.create.mockReturnValue(mockUser); - userRepo.save.mockResolvedValue(mockUser); - - studentRepo.create.mockReturnValue({} as Student); - studentRepo.save.mockResolvedValue({} as Student); - - mappingRepo.create.mockReturnValue({} as UserDingMapping); - mappingRepo.save.mockResolvedValue({} as UserDingMapping); - - // First: skip (existing), second: teacher, third: student + // First: skip (existing) mappingRepo.findOne - .mockResolvedValueOnce({ id: 99 } as UserDingMapping) // skip - .mockResolvedValueOnce(null) // teacher + .mockResolvedValueOnce({ id: 99 } as UserDingMapping) + .mockResolvedValueOnce(null) // teacher .mockResolvedValueOnce(null); // student + // Teacher's manager operations + const teacherMgr = mockManager(); + teacherMgr.findOne.mockResolvedValue(mockRole); + const teacherUser = { id: 50 } as User; + teacherMgr.create.mockReturnValue(teacherUser); + teacherMgr.save.mockResolvedValue(teacherUser); + + // Student's manager operations + const studentMgr = mockManager(); + const studentUser = { id: 51 } as User; + const mockStudent = { id: 50 } as Student; + studentMgr.create.mockReturnValueOnce(studentUser); + studentMgr.save.mockResolvedValueOnce(studentUser); + studentMgr.create.mockReturnValueOnce(mockStudent); + studentMgr.save.mockResolvedValueOnce(mockStudent); + studentMgr.create.mockReturnValueOnce({} as UserDingMapping); + studentMgr.save.mockResolvedValueOnce({} as UserDingMapping); + + dataSourceMock.transaction + .mockImplementationOnce(async (cb) => cb(teacherMgr)) + .mockImplementationOnce(async (cb) => cb(studentMgr)); + const users: ImportUserDto[] = [ { dingUserId: 'skip', name: 'Skip', mobile: '138', roleId: null }, { dingUserId: 'teacher', name: 'Teacher', mobile: '139', roleId: 1 }, @@ -264,5 +303,7 @@ describe('SyncService — new methods', () => { expect(result.teacherCount).toBe(1); expect(result.studentCount).toBe(1); expect(result.skipped).toBe(1); + expect(result.warnings).toEqual([]); + expect(dataSourceMock.transaction).toHaveBeenCalledTimes(2); }); }); diff --git a/apps/server/src/sync/sync.service.ts b/apps/server/src/sync/sync.service.ts index 008b359..54a6422 100644 --- a/apps/server/src/sync/sync.service.ts +++ b/apps/server/src/sync/sync.service.ts @@ -1,6 +1,6 @@ import { Injectable, Logger } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; import { SyncLog, SyncState, UserDingMapping } from '../entities'; import type { SyncPlatform, SyncType, SyncStatus } from '../entities/sync-log.entity'; import { DingTalkService } from '../integration/dingtalk.service'; @@ -39,6 +39,7 @@ export class SyncService { private readonly weComService: WeComService, private readonly attendanceImportService: AttendanceImportService, private readonly scheduleSyncService: ScheduleSyncService, + private readonly dataSource: DataSource, ) {} // ── Scheduled sync disabled — use manual trigger via UI ── @@ -122,13 +123,14 @@ export class SyncService { teacherCount: number; studentCount: number; skipped: number; + warnings: string[]; }> { let teacherCount = 0; let studentCount = 0; let skipped = 0; + const warnings: string[] = []; for (const u of users) { - // 检查是否已存在映射 const existing = await this.mappingRepo.findOne({ where: { dingUserId: u.dingUserId }, }); @@ -138,65 +140,62 @@ export class SyncService { } try { - const username = `dd_${u.dingUserId}`; - const passwordHash = await bcrypt.hash('123456', 10); + await this.dataSource.transaction(async (manager) => { + const username = `dd_${u.dingUserId}`; + const passwordHash = await bcrypt.hash('123456', 10); - const user = this.userRepo.create({ - username, - name: u.name, - passwordHash, - isActive: true, - }); - await this.userRepo.save(user); + const user = manager.create(User, { + username, + name: u.name, + passwordHash, + isActive: true, + }); + await manager.save(user); - if (u.roleId != null) { - // 老师：分配角色 - const role = await this.roleRepo.findOne({ where: { id: u.roleId } }); - if (role) { - user.roles = [role]; - await this.userRepo.save(user); - teacherCount++; + if (u.roleId != null) { + const role = await manager.findOne(Role, { where: { id: u.roleId } }); + if (role) { + user.roles = [role]; + await manager.save(user); + teacherCount++; + } else { + const msg = `角色 id=${u.roleId} 不存在，跳过用户 ${u.name}(${u.dingUserId})`; + this.logger.warn(msg); + warnings.push(msg); + skipped++; + throw new Error('SKIP_USER'); + } } else { - this.logger.warn(`角色 id=${u.roleId} 不存在，用户 ${u.name} 转为学生`); - const student = this.studentRepo.create({ + const student = manager.create(Student, { name: u.name, phone: u.mobile || undefined, userId: user.id, status: 'active', }); - await this.studentRepo.save(student); + await manager.save(student); studentCount++; } - } else { - // 学生：创建 Student 记录 - const student = this.studentRepo.create({ - name: u.name, - phone: u.mobile || undefined, - userId: user.id, - status: 'active', - }); - await this.studentRepo.save(student); - studentCount++; - } - // 创建映射 - const mapping = this.mappingRepo.create({ - dingUserId: u.dingUserId, - userId: user.id, - dingName: u.name, - dingMobile: u.mobile, + const mapping = manager.create(UserDingMapping, { + dingUserId: u.dingUserId, + userId: user.id, + dingName: u.name, + dingMobile: u.mobile, + }); + await manager.save(mapping); }); - await this.mappingRepo.save(mapping); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - this.logger.error(`导入用户 ${u.name}(${u.dingUserId}) 失败: ${msg}`); + if (msg !== 'SKIP_USER') { + this.logger.error(`导入用户 ${u.name}(${u.dingUserId}) 失败: ${msg}`); + } } } this.logger.log( `钉钉用户导入完成: ${teacherCount} 位老师, ${studentCount} 位学生, ${skipped} 跳过`, ); - return { teacherCount, studentCount, skipped }; + return { teacherCount, studentCount, skipped, warnings }; } // ── 排班同步 ──