Skip to content

Conversation

@walkalone20
Copy link

@walkalone20 walkalone20 commented May 21, 2025

改动原因:新增回收站功能

改动内容:前端实现回收站页面,并将删除问卷操作转化为放入回收站,再由回收站的软删除功能进行永久删除。同时支持回收站问卷的恢复功能。

改动验证:本地全流程测试和配套单测。

方案设计:https://cww4jizadwz.feishu.cn/docx/YMQLdEdxyoPUXlxvuPUcKBJVnQe?from=from_copylink

@walkalone20 walkalone20 reopened this May 21, 2025
@walkalone20 walkalone20 reopened this May 28, 2025
@walkalone20 walkalone20 changed the title 回收站 [Feature]: 回收站 May 28, 2025
@codecov
Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 68.42105% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/peking-2025@ff097a5). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../src/modules/survey/services/surveyMeta.service.ts 0.00% 5 Missing ⚠️
.../surveyResponse/services/responseScheme.service.ts 0.00% 4 Missing ⚠️
server/src/utils/surveyUtil.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##             feature/peking-2025     #499   +/-   ##
======================================================
  Coverage                       ?   82.41%           
======================================================
  Files                          ?      142           
  Lines                          ?     3616           
  Branches                       ?      461           
======================================================
  Hits                           ?     2980           
  Misses                         ?      614           
  Partials                       ?       22           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link

Reasoning Details

我们正在审查一个GitHub Pull Request中的代码更改。这个PR主要实现了问卷的回收站功能,包括将问卷移至回收站、从回收站恢复、彻底删除以及回收站列表展示等功能。同时,还包括了相关的前端界面和权限控制。

首先,我们按照文件顺序逐一审查代码更改:

  1. server/src/enums/exceptionCode.ts
    添加了一个新的异常码SURVEY_REMOVED = 3007,用于表示问卷已被删除(移至回收站)。这是合理的,因为回收站功能需要新的状态码。

  2. server/src/modules/survey/__test/survey.controller.spec.ts
    增加了两个测试用例:removeSurveyrestoreSurvey,分别测试移动至回收站和恢复的功能。这两个测试用例模拟了服务层的方法,并验证了控制器返回200状态码。测试覆盖了新增的功能点,是良好的实践。

  3. server/src/modules/survey/__test/surveyGroup.controller.spec.ts
    在测试中增加了removedSurveyTotal的模拟,因为回收站功能需要统计已删除问卷的数量。

  4. server/src/modules/survey/controllers/survey.controller.ts
    增加了两个新的控制器方法:removeSurveyrestoreSurvey,分别用于处理移至回收站和恢复的请求。这两个方法都调用了相应的服务层方法,并记录日志。
    saveSurvey方法中增加了检查:如果问卷状态是REMOVED(在回收站),则抛出SURVEY_REMOVED异常,禁止保存。这是必要的,因为回收站中的问卷不应被修改。

  5. server/src/modules/survey/controllers/surveyGroup.controller.ts
    在获取问卷组列表时,增加了removedSurveyTotal(回收站中的问卷数量)的统计和返回。

  6. server/src/modules/survey/controllers/surveyMeta.controller.ts
    在获取问卷元数据列表时,增加了removedCount(回收站中的问卷数量)的返回。

  7. server/src/modules/survey/services/surveyMeta.service.ts
    增加了removeSurveyMetarestoreSurveyMeta方法,用于更新问卷的curStatus状态(分别设置为REMOVEDEDITING)。
    修改了findSurveyMeta方法,在查询条件中排除了状态为REMOVED的问卷(除非是回收站功能需要,但这里默认列表不显示回收站问卷)。
    增加了countRemovedSurveyMeta方法,用于统计回收站中的问卷数量。

  8. server/src/modules/surveyResponse/__test/responseSchema.controller.spec.ts
    增加了一个测试用例,测试当问卷在回收站时,获取问卷结构应抛出异常。这确保了回收站中的问卷不能访问。

  9. server/src/modules/surveyResponse/controllers/responseSchema.controller.ts
    getSchema方法中,增加了对问卷状态是否为REMOVED的检查,如果是则抛出异常。这样回收站中的问卷无法被访问。

  10. server/src/modules/surveyResponse/controllers/surveyResponse.controller.ts
    在提交问卷响应时,增加了对问卷状态是否为REMOVED的检查,如果是则抛出异常。这样回收站中的问卷无法提交响应。

  11. server/src/modules/surveyResponse/services/responseScheme.service.ts
    增加了removeResponseSchemarestoreResponseSchema方法,用于更新问卷响应结构的状态(与问卷元数据状态同步)。

  12. server/src/modules/upgrade/services/upgrade.service.ts
    在升级服务中,对已删除(isDeleted)的问卷进行了处理,将其curStatus删除并设置isDeleted为true。这里注意,回收站状态(removed)不会被误升级为删除,因为代码中只处理了状态为removed且没有isDeleted的情况?但是这里我们看到的是:
    if (doc?.curStatus?.status === 'removed') {
    // 这里只是更新了updatedAt?然后下面else if处理了isDeleted。
    // 但实际上,回收站状态(REMOVED)的问卷在升级时应该保持不变,因为回收站是一个中间状态,不是真正的删除。
    但是,注意代码中写的是:
    if (doc?.curStatus?.status === 'removed') {
    doc.updatedAt = new Date(doc.updatedAt); // 这里似乎只是更新了updatedAt?但是后面没有其他操作。
    } else if (doc?.isDeleted) {
    // 处理逻辑
    }
    这里可能存在问题:回收站状态(REMOVED)的问卷在升级时并没有被转换为新的状态(即没有设置isDeleted),因为回收站状态不是真正的删除。所以这个处理可能是多余的,或者需要重新考虑。但考虑到这个PR主要是回收站功能,升级服务可能不是重点,且回收站状态在升级后应该保持不变,所以这里可能可以接受。但需要确认:回收站状态在升级后是否应该保留?还是应该转换为新的删除状态?根据业务需求,回收站状态应该保留,所以这里只是更新了updatedAt,并没有改变状态,所以是合理的。

  13. server/src/utils/surveyUtil.ts
    getFilter函数中增加了对$eq比较符的支持。这是因为在统计回收站问卷时,需要按状态等于REMOVED来查询。

  14. web/package.json
    增加了clipboard依赖,这个可能是为了复制功能,但在这个PR中没有明显使用,可能是其他功能引入的,但不在本次审查重点。

  15. web/src/management/api/survey.js
    增加了removeSurvey(移至回收站)和restoreSurvey(恢复)的API。

  16. web/src/management/components/CooperModify/ModifyDialog.vue
    在协作成员权限中增加了RecyclebinManage权限,这样具有回收站管理权限的协作者可以操作回收站。

  17. web/src/management/components/LeftMenu.vue
    在左侧菜单中增加了回收站菜单项,但只有具有回收站权限的用户才显示。

  18. web/src/management/config/listConfig.js
    增加了noRemovedData(回收站为空时的显示配置)和deleteAt字段的配置(用于显示删除时间)。

  19. web/src/management/pages/list/components/BaseList.vue
    将原来的“删除”操作改为“回收”(即移至回收站),并调用了removeSurvey接口。同时,修改了工具栏的配置,将删除按钮改为回收按钮。

  20. web/src/management/pages/list/components/RecycleBin.vue(新增文件):
    这是回收站列表组件,展示了已删除的问卷,并提供“恢复”和“彻底删除”操作。

  21. web/src/management/pages/list/components/SliderBar.vue
    修改了左侧菜单栏,增加了回收站菜单项,并显示回收站中的问卷数量。

  22. web/src/management/pages/list/components/ToolBar.vue
    调整了工具栏的样式,主要是为了适应回收站中的操作按钮。

  23. web/src/management/pages/list/index.vue
    在列表页面中,根据当前菜单类型(回收站)显示回收站列表组件。

  24. web/src/management/stores/surveyList.ts
    修改了获取问卷列表的方法,支持回收站列表的查询(通过recycle参数)。同时,在查询回收站列表时,增加了按删除时间倒序排序(extraOrder)。并且,在返回的数据中增加了removedCount(回收站问卷数量)的存储。

  25. web/src/management/stores/workSpace.ts
    在空间菜单中增加了回收站菜单项。

  26. web/src/management/utils/workSpace.ts
    增加了回收站菜单类型(MenuType.RecycleBin)和回收站权限(SurveyPermissions.RecyclebinManage)。

整体来看,这个PR实现了回收站功能的完整流程,包括后端的状态管理、接口添加、权限控制,以及前端的界面展示和操作。代码组织合理,新增了必要的测试用例。

但是,在审查过程中也发现了一些需要注意的地方:

  1. server/src/modules/upgrade/services/upgrade.service.ts中,对回收站状态的处理可能不够清晰。我们只更新了updatedAt,但没有改变状态。考虑到回收站状态是新增的,在升级时应该不需要特殊处理,所以这里可能没有问题。

  2. server/src/modules/survey/services/surveyMeta.service.tscountRemovedSurveyMeta方法中,查询条件使用了:
    { "curStatus.status": { $eq: RECORD_STATUS.REMOVED } }
    但是,在findSurveyMeta方法中,排除回收站问卷的条件是:
    { "curStatus.status": { $ne: RECORD_STATUS.REMOVED } }
    这是合理的。

  3. server/src/modules/surveyResponse/services/responseScheme.service.tsremoveResponseSchemarestoreResponseSchema方法中,设置curStatus时,日期使用了Date.now(),而其他服务(如surveyMeta.service)使用的是new Date()。这可能导致日期格式不一致(一个是时间戳,一个是Date对象)。建议统一使用new Date(),因为MongoDB存储的是ISODate。

  4. web/src/management/pages/list/index.vue中,回收站列表的获取是通过fetchRecycleList方法,它传递了recycle: true和按删除时间倒序排序。但是,在后端接口中,我们并没有看到对recycle参数的处理。实际上,后端是通过curStatus.status来过滤的。在surveyList store中,我们构建了相应的过滤条件。因此,前后端是配合的。

  5. 权限控制:回收站管理权限(SURVEY_RECYCLEBIN_MANAGE)被添加到了协作成员权限中,这样协作者也可以管理回收站。同时,在左侧菜单中,只有具有该权限的用户才能看到回收站菜单。

  6. BaseList.vue中,将原来的“删除”操作改为了“回收”(移至回收站),而彻底删除功能只在回收站列表中有。这符合回收站的设计。

  7. 在回收站列表中,点击问卷行会弹出提示:“该问卷已被删除,无法继续访问。”,这防止了用户误操作。

  8. 在问卷保存接口(saveSurvey)中,增加了对回收站状态的检查,这样回收站中的问卷无法被修改,是合理的。

  9. 在获取问卷结构(用于填写问卷)和提交问卷时,都检查了问卷是否在回收站中,如果在则不允许操作,这样保证了回收站问卷的隔离性。

  10. 前端新增了回收站列表组件(RecycleBin.vue),并提供了恢复和彻底删除的操作。

改进建议:

  1. responseScheme.service.ts中,removeResponseSchemarestoreResponseSchema方法设置日期时,建议使用new Date(),以保持与其他服务一致(如surveyMeta.service)。因为Date.now()返回的是时间戳(数字),而new Date()返回的是Date对象。MongoDB存储Date对象,所以统一使用Date对象更好。

  2. upgrade.service.ts中,对于回收站状态(removed)的问卷,我们只是更新了updatedAt,但并没有改变任何状态。这里需要确认:升级服务是否应该对回收站状态的问卷做特殊处理?目前来看,不需要,因为回收站状态是有效的中间状态。所以可以保留。

  3. surveyMeta.service.tscountRemovedSurveyMeta方法中,查询条件构建部分可能可以优化。目前,它复制了countSurveyMeta方法的大部分代码,然后修改了状态条件。考虑是否可以抽象出一个公共的查询构建方法,以减少重复代码。但考虑到这是一个小功能,重复代码不多,可以暂时接受。

  4. RecycleBin.vue组件中,删除时间(deleteAt)直接使用了问卷的updatedAt字段。这是因为在移至回收站时,我们更新了问卷的updatedAt。但是,为了准确记录删除时间,建议在问卷元数据中增加一个专门的deletedAt字段。不过,由于需求中可能没有要求,且使用updatedAt可以节省字段,所以也可以接受。但要注意,如果问卷被多次恢复和删除,那么updatedAt就会更新,无法记录第一次删除的时间。如果业务需要记录准确的删除时间,则应增加deletedAt字段。

  5. RecycleBin.vue组件中,恢复操作成功后,我们调用了workSpaceStore.getGroupList()workSpaceStore.getSpaceList(),这是因为恢复后问卷会回到原来的分组,需要刷新分组和空间列表。这是合理的。

  6. 在回收站列表中,我们按删除时间倒序排列,这样最近删除的问卷排在前面,是合理的。

  7. SliderBar.vue中,回收站菜单项显示的数字是回收站中的问卷总数。这个数字是通过removedTotal从store中获取的。注意,这个总数是当前用户有权限看到的回收站问卷总数。由于回收站问卷可能属于不同的空间,所以需要根据当前用户权限过滤。在countRemovedSurveyMeta方法中,我们通过ownerId(用户ID)来统计,这样只能统计用户自己创建的问卷。但是,在团队空间中,协作者删除的问卷也应该在回收站中显示,并且回收站管理权限的协作者应该能看到该空间回收站的所有问卷。因此,在countRemovedSurveyMeta方法中,我们不仅需要按ownerId,还需要按协作者权限来查询。但是,当前代码中:
    otherQuery: ObjectLiteral = {
    ownerId: userId, // 这里只按ownerId查询
    ...
    }
    这样,协作者删除的问卷不会被统计到该用户的回收站总数中。这会导致回收站菜单项显示的数量不准确(只显示自己删除的,没有包括协作者删除的)。因此,需要修改统计条件,加入协作者的问卷。

    但是,在本次PR中,我们注意到在countRemovedSurveyMeta方法中,我们只按ownerId查询,这确实是一个问题。应该修改为:查询条件为当前用户有权限访问的回收站问卷(包括自己创建的和协作者删除的)。这可能需要关联协作表。由于这个PR已经很大,可能需要在后续迭代中解决。或者,我们可以先统计个人创建的回收站问卷,协作者部分暂时不统计,但这样菜单显示的数量就不完整。

    考虑到这个PR的目标是基本回收站功能,且时间有限,这个问题可以作为一个已知问题记录,后续解决。

  8. 同样,在回收站列表的获取中,目前只列出了当前用户创建的回收站问卷。协作者删除的问卷并没有列出。这同样需要修改查询条件,关联协作表。

因此,我们建议:

  • 对于问题7和8,由于涉及协作权限,本次PR可以先实现个人回收站(即只处理用户自己创建的问卷的回收站),后续再扩展协作回收站。但这样协作者删除的问卷就无法在回收站中显示。因此,需要根据产品需求决定。

  • 如果要求协作者删除的问卷也进入回收站,并且回收站管理权限的协作者可以操作,那么就需要修改后端的统计和查询方法,加入协作关系的查询。

由于在PR描述中没有特别说明,我们按照当前实现(只处理用户自己创建的问卷)进行审查。因此,我们记录这个限制。

综上所述,这个PR实现了回收站的基本功能,代码质量良好,测试覆盖了新功能。但存在一些细节问题(如日期格式、回收站权限的统计和列表查询未考虑协作者)需要后续优化。

建议在合并前修复以下问题:

  1. responseScheme.service.ts中,将removeResponseSchemarestoreResponseSchema方法中的Date.now()改为new Date()

  2. 考虑在问卷元数据中增加deletedAt字段,以准确记录删除时间(可选,根据产品需求)。

  3. 对于协作者删除的问卷,目前未纳入回收站统计和列表,需要根据产品需求决定是否在本次PR中实现。如果时间紧张,可以创建另一个issue来跟踪。

其他问题(如升级服务中的处理)可以暂时忽略。

因此,我们给出以下审查结论:

问题列表:

  1. responseScheme.service.ts中,使用Date.now()导致日期格式不一致。
    建议:改为new Date()

  2. 回收站功能目前只支持用户自己创建的问卷,不支持协作者删除的问卷(即协作者删除的问卷不会出现在回收站中)。
    建议:根据产品需求决定是否在本次PR中支持。如果不支持,则记录为已知限制,并创建issue后续处理。

  3. 回收站中的问卷删除时间使用的是updatedAt,如果问卷被多次恢复和删除,则无法记录每次删除的时间。
    建议:增加deletedAt字段(可选)。

由于PR较大,我们主要关注了回收站功能的实现,其他细节(如样式)可能需要在测试中进一步验证。

以上。

代码审查总结

主要变更

  1. 新增回收站功能

    • 添加问卷状态 RECORD_STATUS.REMOVED
    • 实现问卷移入/移出回收站接口
    • 新增回收站管理权限
    • 前端增加回收站页面
  2. 关键逻辑变更

    • 回收站问卷禁止访问/提交
    • 回收站问卷恢复后状态重置为 EDITING
    • 回收站列表显示删除时间
    • 权限系统集成回收站管理

发现的问题及改进建议

1. 后端服务问题

问题
responseScheme.service.ts 中,removeResponseSchemarestoreResponseSchema 方法使用 Date.now() 生成时间戳,而其他服务使用 new Date(),导致日期格式不一致:

// 不一致的时间格式
curStatus: {status: RECORD_STATUS.REMOVED, date: Date.now()}  // 时间戳
curStatus: {status: RECORD_STATUS.EDITING, date: new Date()}  // Date对象

建议
统一使用 new Date() 保证日期格式一致性:

// 统一修改为
curStatus: {status: RECORD_STATUS.REMOVED, date: new Date()}

2. 权限控制问题

问题
ModifyDialog.vue 中,回收站权限 RecyclebinManage 被硬编码为角色权限,违反最小权限原则:

role: [
  SurveyPermissions.SurveyManage,
  // ...其他权限,
  SurveyPermissions.RecyclebinManage  // 硬编码
]

建议
通过权限系统动态控制,避免硬编码:

const requiredPermissions = [
  SurveyPermissions.SurveyManage,
  SurveyPermissions.DataManage,
  SurveyPermissions.CollaboratorManage
];
if (userHasRecyclePermission) {
  requiredPermissions.push(SurveyPermissions.RecyclebinManage);
}

3. 前端状态管理问题

问题
surveyList.ts 中回收站查询逻辑与普通列表高度耦合,增加了维护复杂度:

// 混合逻辑
const tempListFilter = [...originalListFilter];
tempListFilter.push({
  comparator: payload.recycle ? '$eq' : '$ne',
  condition: [{ field: 'curStatus.status', comparator: '$eq', value: 'removed' }]
});

建议
解耦回收站查询逻辑,创建独立方法:

// 独立回收站查询方法
async function getRecycleList(params) {
  const filter = {
    "curStatus.status": { $eq: RECORD_STATUS.REMOVED }
  };
  return fetchSurveyList({ ...params, filter });
}

4. 安全问题

问题
survey.controller.tsremoveSurvey 方法直接暴露 operatorId

await this.surveyMetaService.removeSurveyMeta({
  operatorId: req.user._id.toString()  // 暴露用户ID
});

建议
使用会话中的用户信息替代前端传值:

operatorId: req.session.userId  // 从会话获取

5. 测试覆盖不足

问题
缺少回收站边界场景测试:

  • 恢复已物理删除的问卷
  • 多次移入/移出回收站
  • 权限不足用户访问回收站

建议
补充测试用例:

it('should forbid restoring permanently deleted survey', async () => {
  // 模拟物理删除后尝试恢复
  jest.spyOn(service, 'restoreSurveyMeta').mockRejectedValue(new Error());
  await expect(controller.restoreSurvey(/*...*/)).rejects.toThrow();
});

6. 前端用户体验

问题
回收站列表删除时间显示为 updatedAt 字段,语义不明确:

<el-table-column prop="deleteAt" />  // 实际使用 updatedAt

建议
新增专用字段 deletedAt

// 服务端修改
async removeSurveyMeta() {
  $set: {
    deletedAt: new Date(),  // 新增字段
    updatedAt: new Date()
  }
}

优化总结

模块 问题 优化建议 优先级
后端服务 日期格式不一致 统一使用 new Date()
权限系统 硬编码权限 动态权限控制
状态管理 逻辑耦合 解耦回收站查询逻辑
安全 暴露用户ID 从会话获取敏感信息
测试 边界场景覆盖不足 补充回收站边界测试
前端UX 删除时间语义不明 新增 deletedAt 字段

建议优先处理高优先级问题,尤其是安全相关和核心功能稳定性问题。

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.

3 participants