Repository navigation
[Feat/#589] "세부 과제 다중 담당자 할당 가능" - #593
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough세부 작업의 담당자 저장 구조를 Changes다중 담당자 세부 작업
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TaskController
participant TaskService
participant TaskRepository
participant AlarmService
Client->>TaskController: assigneeIds 전송
TaskController->>TaskService: setSubTaskAssignee 호출
TaskService->>TaskRepository: 담당자 매핑 교체
TaskService->>AlarmService: 담당자별 알림 생성
TaskService-->>TaskController: assignees 배열 반환
TaskController-->>Client: 갱신된 세부 작업 응답
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/validators/task.schema.js (1)
176-183: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBoth assignee fields optional means an empty body clears all assignees.
With
assigneeIdandassigneeIdsboth optional,PATCH .../assigneewith{}validates andsetSubTaskAssigneeresolves to an empty list, replacing all existing assignees with none (src/services/task.service.js lines 762-802). Consider requiring at least one of the two fields so accidental empty payloads can't wipe assignments.♻️ Suggested guard
- assigneeIds: z.array(z.coerce.number().int().positive()).optional(), - }), + assigneeIds: z.array(z.coerce.number().int().positive()).optional(), + }).refine( + (body) => body.assigneeId !== undefined || body.assigneeIds !== undefined, + { error: "assigneeIds 또는 assigneeId 중 하나는 필수입니다." }, + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/task.schema.js` around lines 176 - 183, Update the assignee validation schema containing assigneeId and assigneeIds to require at least one field, rejecting an empty object before setSubTaskAssignee can resolve it to an empty assignment list. Preserve the existing coercion and validation rules when either field is provided.src/services/alarm.service.js (2)
277-320: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
isAlarmis persisted before the assignee check — inconsistent state on failure.
updateSubtaskAlarmStatus(line 277-280) writesisAlarmto the DB, and only afterward (line 294-300) does the code validate that assignees exist, throwingSUBTASK_NO_ASSIGNEEif not. SincesubTask.subTaskAssigneesis already fetched pre-update, this validation can (and should) run before the persistence call. As written, a request to enable alarms on an unassigned subtask fails the API call but leavesisAlarm = truepermanently persisted with zero alarms created — a state that cannot be self-corrected by retrying the same request.Separately,
deleteAlarmsBySubTaskIdsfollowed by a per-assignee loop ofcreateSubTaskAlarmcalls (lines 302-320) is not wrapped in a transaction. If any iteration fails partway, some assignees end up with alarms and others don't, with no rollback.🐛 Proposed fix: validate assignees before persisting the status change
- const updatedSubTask = await this.alarmRepository.updateSubtaskAlarmStatus( - subTaskId, - isAlarm, - ); - - if (!updatedSubTask) { - throw new NotFoundError( - "SUBTASK_UPDATE_FAILED", - "세부과제 업데이트에 실패했습니다.", - ); - } - - const assignments = subTask.subTaskAssignees || []; - const notifyAssignments = assignments.filter( - (assignment) => assignment.member?.user?.id != null, - ); - - if (updatedSubTask.isAlarm === true) { - if (notifyAssignments.length === 0) { - throw new BadRequestError( - "SUBTASK_NO_ASSIGNEE", - "담당자가 지정된 세부과제만 알림을 켤 수 있습니다.", - ); - } - + const assignments = subTask.subTaskAssignees || []; + const notifyAssignments = assignments.filter( + (assignment) => assignment.member?.user?.id != null, + ); + + if (isAlarm === true && notifyAssignments.length === 0) { + throw new BadRequestError( + "SUBTASK_NO_ASSIGNEE", + "담당자가 지정된 세부과제만 알림을 켤 수 있습니다.", + ); + } + + const updatedSubTask = await this.alarmRepository.updateSubtaskAlarmStatus( + subTaskId, + isAlarm, + ); + + if (!updatedSubTask) { + throw new NotFoundError( + "SUBTASK_UPDATE_FAILED", + "세부과제 업데이트에 실패했습니다.", + ); + } + + if (updatedSubTask.isAlarm === true) { await this.alarmRepository.deleteAlarmsBySubTaskIds([updatedSubTask.id]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/alarm.service.js` around lines 277 - 320, Move the SUBTASK_NO_ASSIGNEE validation using subTask.subTaskAssignees ahead of updateSubtaskAlarmStatus so enabling alarms cannot persist an invalid status. Keep the existing notification creation flow, but execute the status update, deleteAlarmsBySubTaskIds, and createSubTaskAlarm operations within the service’s existing transaction mechanism so partial alarm creation rolls back on failure.
302-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrap the alarm rebuild in a transaction. Pass a shared
txthroughdeleteAlarmsBySubTaskIdsand eachcreateSubTaskAlarmcall here; a failure mid-loop leaves the alarm set partially updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/alarm.service.js` around lines 302 - 320, Wrap the alarm deletion and rebuild sequence in a shared transaction, passing its tx context to deleteAlarmsBySubTaskIds and every createSubTaskAlarm call in the notifyAssignments loop. Ensure the transaction commits only after all alarm creations succeed and rolls back on any failure, preventing a partially updated alarm set.src/services/task.service.js (1)
716-721: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThrown errors are missing
reason.These handler-bound errors set
statusCode/errorCodebut noreason, so the global middleware has no message field to serialize. Prefer the existingBadRequestError/NotFoundErrorclasses, which populate all required fields.As per coding guidelines, "Thrown errors handled by global middleware must include
statusCode,errorCode,reason, and optionaldata."Also applies to: 771-778
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 716 - 721, Replace the manually constructed errors in the missing-user checks near the current block and the additional occurrence around the referenced later branch with the existing NotFoundError or BadRequestError class, preserving the 404/USER_NOT_FOUND semantics and Korean message. Ensure the thrown handler-bound error exposes statusCode, errorCode, and reason through the shared error class.Source: Coding guidelines
🧹 Nitpick comments (5)
src/services/member.service.js (1)
77-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the join-table cleanup behind the repository layer.
The service reaches into
tx.subTaskAssigneedirectly; ataskRepository.deleteSubTaskAssigneesByMemberId(memberId, tx)keeps data access in the repository layer consistent with the rest of this transaction (alarmRepository,taskRepository).As per coding guidelines, "Use repositories as the data-access abstraction layer" and "Follow the Controller → Service → Repository flow for new API behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/member.service.js` around lines 77 - 79, Replace the direct tx.subTaskAssignee.deleteMany call in the member service with taskRepository.deleteSubTaskAssigneesByMemberId(requestingMember.id, tx), and add or update that repository method to perform the join-table cleanup while preserving the transaction context.Source: Coding guidelines
src/repositories/task.repository.js (1)
299-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated assignee include graph.
The same
subTaskAssignees → member → user → alarmSettinginclude is duplicated infindSubTaskById,createSubTask, andupdateSubTask(and partially infindTaskDetail). A shared constant reduces drift risk.♻️ Suggested extraction
const SUBTASK_ASSIGNEES_INCLUDE = { subTaskAssignees: { include: { member: { include: { user: { include: { alarmSetting: true } } } }, }, }, };Then use
include: SUBTASK_ASSIGNEES_INCLUDEin each query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/repositories/task.repository.js` around lines 299 - 348, Extract the repeated subTaskAssignees → member → user → alarmSetting include graph into a shared SUBTASK_ASSIGNEES_INCLUDE constant, then replace the duplicated include objects in findSubTaskById, createSubTask, and updateSubTask, and reuse it where applicable in findTaskDetail.src/controllers/task.controller.js (1)
286-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHTTP path skips the shared assignee validation used by the socket path.
The socket handler runs
taskHelper.buildUpdateSubTaskAssigneeParamsbefore calling the service, while this controller forwards rawreq.bodyvalues. Reusing the helper keeps validation/normalization identical across transports and keeps the growing positional argument list (subTaskId, assigneeId, assigneeIds, userId) constructed in one place.♻️ Suggested change
- const { assigneeId, assigneeIds } = req.body; - const userId = req.user.id; - - const result = await taskService.setSubTaskAssignee( - subTaskId, - assigneeId, - assigneeIds, - userId, - ); + const { assigneeId, assigneeIds } = req.body ?? {}; + const params = taskHelper.buildUpdateSubTaskAssigneeParams({ + userId: req.user.id, + subTaskId, + assigneeId, + assigneeIds, + }); + + const result = await taskService.setSubTaskAssignee( + params.subTaskId, + params.assigneeId, + params.assigneeIds, + params.userId, + );Verify
taskHelperis imported in this controller before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/task.controller.js` around lines 286 - 306, Update setSubTaskAssignee in the task controller to call taskHelper.buildUpdateSubTaskAssigneeParams with the request values before invoking taskService.setSubTaskAssignee. Reuse the helper’s validated and normalized output, including the positional arguments and userId, so the HTTP path matches the socket handler; verify taskHelper is imported.src/services/task.service.js (2)
1300-1318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssignee normalization here duplicates the resolver's logic.
Lines 1308-1314 repeat the same
Number→NaN/> 0filter → dedupe pipeline present inresolveSubTaskAssigneeMemberIds,setSubTaskAssignee, andtask.helper.js. Extracting a sharednormalizeUserIds(input)helper would remove four copies.Also consider returning the updated
assigneesarray inresults.push(...)(line 1376) for parity withsetSubTaskAssignee/createSingleSubTask, since clients now expect assignee arrays after an update.Also applies to: 1337-1374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 1300 - 1318, Extract the repeated assignee ID conversion, positive-number filtering, and deduplication into a shared normalizeUserIds(input) helper, then reuse it in resolveSubTaskAssigneeMemberIds, setSubTaskAssignee, task.helper.js, and this update flow instead of maintaining separate pipelines. In the results.push(...) update response, include the updated assignees array to match setSubTaskAssignee and createSingleSubTask.
50-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused single-assignee wrapper
resolveSubTaskAssigneeMemberIdonly forwards toresolveSubTaskAssigneeMemberIds, and the codebase only references the array-based helper. Dropping the wrapper would trim dead API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 50 - 63, Remove the unused resolveSubTaskAssigneeMemberId method from the task service, leaving resolveSubTaskAssigneeMemberIds as the sole subtask assignee resolution API. Verify no remaining references require updating, and do not alter the array-based helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prisma/schema.prisma`:
- Around line 266-277: Add a database migration for the SubTaskAssignee schema
change: backfill one SubTaskAssignee row for each existing SubTask.assignee_id,
preserving the associated sub-task and member, then remove the obsolete
SubTask.assignee_id column and its related constraint/index. Ensure the
migration handles existing assignment data before dropping the column.
In `@src/helpers/task.helper.js`:
- Around line 52-56: Update the assigneeIds normalization in the relevant task
helper to validate every provided entry and throw INVALID_ASSIGNEE_ID when any
value is non-numeric or not greater than zero, matching the existing assigneeId
handling. Do not filter invalid entries into an empty array; only produce
normalizedAssigneeIds when the entire input is valid.
In `@src/repositories/task.repository.js`:
- Around line 375-383: Make replaceSubTaskAssignees atomic when using its
default prisma client by wrapping the deleteMany and createMany operations in a
transaction, or require callers to provide tx and enforce that contract.
Preserve the empty-array behavior, and deduplicate memberIds before createMany
to prevent duplicate member assignments from causing a constraint failure.
In `@src/services/task.service.js`:
- Around line 65-112: Deduplicate the normalized IDs in
resolveSubTaskAssigneeMemberIds before resolving members, preserving the
existing numeric and positive-ID filtering. Rename the local memberIds
collection to reflect that it contains member objects, and continue returning
each resolved member’s id so createSubTask, replaceSubTaskAssignees,
registerTask, and createSingleSubTask receive unique assignee IDs.
- Around line 693-704: Guard the assignee update flow before constructing or
applying uniqueAssigneeUserIds: when both assigneeId and assigneeIds are omitted
or undefined, preserve the existing assignments and do not call
replaceSubTaskAssignees. Continue allowing explicit assigneeIds: [] or
assigneeId: null to clear assignments, and ensure this behavior applies to both
request and socket payloads handled by the surrounding assignee-update logic.
- Around line 1223-1231: Update the flow around the newSubTask handling to avoid
returning early when subTaskAssignees exists but isAlarm is false. Skip only the
alarm-specific processing in that case, then use the existing final return block
to map subTaskAssignees into the assignees payload instead of returning an empty
array.
- Around line 1320-1335: Hoist the member lookup out of the subtask loop, fetch
it once using the transaction client `tx.member.findMany({ where: { taskId }
})`, and reuse the result when calling `resolveSubTaskAssigneeMemberIds` for
each subtask. Remove the per-subtask global `prisma.member.findMany` call while
preserving the existing assignment-change behavior.
---
Outside diff comments:
In `@src/services/alarm.service.js`:
- Around line 277-320: Move the SUBTASK_NO_ASSIGNEE validation using
subTask.subTaskAssignees ahead of updateSubtaskAlarmStatus so enabling alarms
cannot persist an invalid status. Keep the existing notification creation flow,
but execute the status update, deleteAlarmsBySubTaskIds, and createSubTaskAlarm
operations within the service’s existing transaction mechanism so partial alarm
creation rolls back on failure.
- Around line 302-320: Wrap the alarm deletion and rebuild sequence in a shared
transaction, passing its tx context to deleteAlarmsBySubTaskIds and every
createSubTaskAlarm call in the notifyAssignments loop. Ensure the transaction
commits only after all alarm creations succeed and rolls back on any failure,
preventing a partially updated alarm set.
In `@src/services/task.service.js`:
- Around line 716-721: Replace the manually constructed errors in the
missing-user checks near the current block and the additional occurrence around
the referenced later branch with the existing NotFoundError or BadRequestError
class, preserving the 404/USER_NOT_FOUND semantics and Korean message. Ensure
the thrown handler-bound error exposes statusCode, errorCode, and reason through
the shared error class.
In `@src/validators/task.schema.js`:
- Around line 176-183: Update the assignee validation schema containing
assigneeId and assigneeIds to require at least one field, rejecting an empty
object before setSubTaskAssignee can resolve it to an empty assignment list.
Preserve the existing coercion and validation rules when either field is
provided.
---
Nitpick comments:
In `@src/controllers/task.controller.js`:
- Around line 286-306: Update setSubTaskAssignee in the task controller to call
taskHelper.buildUpdateSubTaskAssigneeParams with the request values before
invoking taskService.setSubTaskAssignee. Reuse the helper’s validated and
normalized output, including the positional arguments and userId, so the HTTP
path matches the socket handler; verify taskHelper is imported.
In `@src/repositories/task.repository.js`:
- Around line 299-348: Extract the repeated subTaskAssignees → member → user →
alarmSetting include graph into a shared SUBTASK_ASSIGNEES_INCLUDE constant,
then replace the duplicated include objects in findSubTaskById, createSubTask,
and updateSubTask, and reuse it where applicable in findTaskDetail.
In `@src/services/member.service.js`:
- Around line 77-79: Replace the direct tx.subTaskAssignee.deleteMany call in
the member service with
taskRepository.deleteSubTaskAssigneesByMemberId(requestingMember.id, tx), and
add or update that repository method to perform the join-table cleanup while
preserving the transaction context.
In `@src/services/task.service.js`:
- Around line 1300-1318: Extract the repeated assignee ID conversion,
positive-number filtering, and deduplication into a shared
normalizeUserIds(input) helper, then reuse it in
resolveSubTaskAssigneeMemberIds, setSubTaskAssignee, task.helper.js, and this
update flow instead of maintaining separate pipelines. In the results.push(...)
update response, include the updated assignees array to match setSubTaskAssignee
and createSingleSubTask.
- Around line 50-63: Remove the unused resolveSubTaskAssigneeMemberId method
from the task service, leaving resolveSubTaskAssigneeMemberIds as the sole
subtask assignee resolution API. Verify no remaining references require
updating, and do not alter the array-based helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e275346-9e50-4eac-940f-1b918d3be5a8
📒 Files selected for processing (13)
prisma/schema.prismaprisma/seed.jssrc/controllers/task.controller.jssrc/dtos/task.dto.jssrc/helpers/task.helper.jssrc/repositories/alarm.repository.jssrc/repositories/task.repository.jssrc/services/alarm.service.jssrc/services/member.service.jssrc/services/task.service.jssrc/socket/handlers/task.handler.jssrc/swagger/swagger.ymlsrc/validators/task.schema.js
💤 Files with no reviewable changes (1)
- src/repositories/alarm.repository.js
| model SubTaskAssignee { | ||
| id Int @id @default(autoincrement()) @map("sub_task_assignee_id") | ||
| subTaskId Int @map("sub_task_id") | ||
| memberId Int @map("member_id") | ||
| createdAt DateTime @default(now()) @map("created_at") @db.Timestamp | ||
|
|
||
| subTask SubTask @relation(fields: [subTaskId], references: [id], onDelete: Cascade) | ||
| member Member @relation(fields: [memberId], references: [id], onDelete: Cascade) | ||
|
|
||
| @@unique([subTaskId, memberId]) | ||
| @@map("SubTaskAssignee") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd . prisma/migrations --type d | sort | tail -20
rg -n -i 'sub_task_assignee|assignee_id' prisma/migrations || echo "no migration references found"Repository: check-task/backend
Length of output: 762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrations =="
fd . prisma/migrations --type d | sort
echo
echo "== schema references =="
rg -n -i 'model SubTaskAssignee|assignee_id|sub_task_assignee' prisma/schema.prisma prisma/migrations || true
echo
echo "== migration sql files =="
fd migration.sql prisma/migrations -t f | sort | xargs -r -I{} sh -c 'echo "--- {}"; rg -n -i "assignee_id|sub_task_assignee|SubTaskAssignee" "{}" || true'Repository: check-task/backend
Length of output: 1590
Add a migration to backfill SubTaskAssignee and drop SubTask.assignee_id Existing migrations only define assignee_id in the initial schema, so deploying this schema change without a data-migration step would lose assignment data on existing databases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@prisma/schema.prisma` around lines 266 - 277, Add a database migration for
the SubTaskAssignee schema change: backfill one SubTaskAssignee row for each
existing SubTask.assignee_id, preserving the associated sub-task and member,
then remove the obsolete SubTask.assignee_id column and its related
constraint/index. Ensure the migration handles existing assignment data before
dropping the column.
- 마이그레이션 추가 - 유효하지 않은 값에 대한 예외처리 보완
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
src/swagger/swagger.yml (2)
2585-2628: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
assigneeIds.items에minimum: 1을 추가하십시오.구현은 각 값을 양의 정수로 검증합니다. OpenAPI 명세는 현재 모든 정수를 허용합니다.
assigneeIds와 빈 배열은 선택 사항으로 유지하십시오. 응답의sub_task_id와assignees구조는 컨트롤러 반환값과 일치합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/swagger/swagger.yml` around lines 2585 - 2628, Update the assigneeIds item schema in the sub-task assignee endpoint to add minimum: 1, matching the positive-integer validation. Keep assigneeIds optional, allow empty arrays, and preserve the existing response structure.
6055-6066: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win요청별
state와nonce를 생성하고 콜백에서 검증하십시오.
/apple은local또는prod를 그대로state로 전달합니다./apple/callback은state를 리다이렉트 대상 선택에만 사용하며, 세션·Redis 값·nonce와 비교하지 않습니다. 따라서 공격자가 자신의 Apple 콜백을 피해자 브라우저에 제출하면 피해자를 공격자 계정으로 로그인시킬 수 있습니다.환경 값은 서버 측 저장소에 보관하고, 요청별 예측 불가능한
state를 생성하십시오. 콜백에서state를 일회성으로 검증한 뒤 로그인 처리를 수행하십시오. Applenonce도 생성하고 ID 토큰의nonce와 검증하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/swagger/swagger.yml` around lines 6055 - 6066, Update the Apple OAuth flow documented around the /apple and /apple/callback endpoints to generate a cryptographically unpredictable, request-specific state and nonce, store the environment mapping server-side, and include them in the authorization request. In the callback, validate and consume state exactly once before selecting the redirect environment or completing login, and verify the Apple ID token nonce against the stored request nonce; do not use the client-provided environment value directly.src/services/task.service.js (8)
772-776: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win404 오류 객체에
reason을 포함하십시오.라인 773의
Error는statusCode와errorCode만 설정합니다. 전역 오류 미들웨어는reason이 없는 오류를 계약대로 직렬화할 수 없습니다.NotFoundError를 사용하거나reason을 설정하십시오.As per coding guidelines, “Thrown errors handled by global middleware must include
statusCode,errorCode,reason, and optionaldata.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 772 - 776, Update the missing-user error in the task service flow to include the required reason field, or replace the generic Error with the established NotFoundError type. Preserve the existing 404 statusCode, USER_NOT_FOUND errorCode, and Korean message while ensuring the thrown error satisfies the global middleware contract.Source: Coding guidelines
1288-1306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win새 세부 과제의 다중 담당자 알림을 생성하십시오.
newSubTask.assigneeId와newSubTask.assignee는 조인 관계 기반 반환값에 없습니다. 이 조건은 거짓이 되어 알림이 생성되지 않습니다. 조건이 참이 되더라도assignee변수가 선언되지 않아 실패합니다.
newSubTask.subTaskAssignees를 순회하고 각 담당자별로 알림을 생성하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 1288 - 1306, Update the alarm creation block for newSubTask to iterate over newSubTask.subTaskAssignees instead of relying on newSubTask.assigneeId, newSubTask.assignee, or the undeclared assignee variable. For each assigned user, calculate the alarm using that user’s alarm settings and create a separate alarm with the corresponding assignee identifier, while preserving the existing isAlarm and endDate requirements.
749-753: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win유효하지 않은
assigneeIds값을 담당자 해제로 처리하지 마십시오.현재 필터는
["invalid"]를[]로 바꿉니다. 이후replaceSubTaskAssignees(subTaskId, [])가 실행되어 기존 담당자를 모두 삭제합니다. PR 계약에서 담당자 해제는 명시적인assigneeIds: []에만 허용해야 합니다.배열의 각 값이 양의 정수인지 검증하십시오. 하나라도 유효하지 않으면
BadRequestError를 발생시키십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 749 - 753, Update the assigneeIds parsing near parsedAssigneeUserIds to validate every array element as a positive integer; if any value is invalid, throw BadRequestError instead of filtering it out. Preserve replacement with an empty list only when the caller explicitly provides assigneeIds: [].
879-910: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win정의되지 않은 단일 담당자 변수를 제거하십시오.
nextAssigneeMemberId,memberIdChanged,newAssignments는 이 메서드에서 선언되지 않았습니다. 담당자 변경 요청은 라인 881에서 즉시ReferenceError를 발생시켜 트랜잭션을 롤백합니다.
memberIdsChanged를 사용하십시오. 변경된 경우updatedTask.subTaskAssignees를 순회하여 새 담당자 전체의 알림을 생성하십시오.수정 방향
- if ( - nextAssigneeMemberId != null && - updatedTask.isAlarm && - updatedTask.endDate && - memberIdChanged - ) { - const newMember = await tx.member.findFirst({ - where: { id: nextAssigneeMemberId, taskId: task.id }, - include: { user: { include: { alarmSetting: true } } }, - }); - - for (const assignment of newAssignments) { + if ( + memberIdsChanged && + updatedTask.isAlarm && + updatedTask.endDate + ) { + for (const assignment of updatedTask.subTaskAssignees) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 879 - 910, Remove the undefined nextAssigneeMemberId, memberIdChanged, and newAssignments references in the alarm creation block. Use the declared memberIdsChanged condition, then iterate updatedTask.subTaskAssignees to create alarms for every newly assigned member while preserving the existing endDate and alarm-setting calculations.
1427-1457: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win관련 필드가 변경될 때만 기존 알림을 교체하십시오.
현재 흐름은 담당자 변경이나 알림 비활성화일 때만 기존 알림을 삭제합니다. 그러나 이후 블록은 제목 또는 상태만 수정해도 새 알림을 생성합니다. 기존 알림이 남아 있으므로 담당자별 중복 알림이 생성되거나 고유 제약 오류가 발생합니다.
endDate,isAlarm, 또는 담당자가 변경된 경우에만 알림을 재계산하십시오. 재생성 전에는 해당 세부 과제의 기존 알림을 삭제하거나 사용자별 upsert를 사용하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 1427 - 1457, 업데이트 흐름에서 assignmentChangeRequested 및 알림 생성 블록을 수정하여 endDate, isAlarm 또는 assigneeId가 변경된 경우에만 alarmDate를 재계산하고 알림을 교체하십시오. 재생성 전에 해당 subTaskId의 기존 알림을 삭제하거나 사용자별 upsert를 사용해 중복 및 고유 제약 오류를 방지하고, 제목이나 상태만 변경된 경우 기존 알림과 생성 흐름을 건드리지 마십시오.
1431-1458: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win중복 선언과 담당자 관계 기반 알림 처리를 수정하십시오.
const alarmHours중복 선언과 정의되지 않은assignee참조로 서비스를 로드할 수 없습니다.SubTask에는assigneeId필드가 없으므로 해당 조건을 제거하십시오.updatedSubTask.subTaskAssignees를 순회하여 각 담당자에게 알림을 생성하십시오.updateSubTask가 필요한 조인 관계를 이미 반환하므로 재조회는 필요하지 않습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 1431 - 1458, In the alarm-processing block of updateSubTask, remove the duplicate alarmHours declaration and the undefined assignee reference. Do not gate processing on updatedSubTask.assigneeId; instead iterate updatedSubTask.subTaskAssignees and create an alarm for each assignee using the joined data already returned by updateSubTask, without re-querying the SubTask.
605-701: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
updateSubTaskDeadline에서assignee관계를 제거하십시오.
SubTask모델에는assignee와assigneeId가 없고subTaskAssignees만 있습니다. 현재include.assignee는 Prisma 쿼리를 실패시킵니다.subTaskAssignees.member.user.alarmSetting을 조회하고, 기존 알림이 없으면 각 담당자에게createSubTaskAlarm을 호출하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 605 - 701, updateSubTaskDeadline의 기존 assignee 조회를 제거하고 subTaskAssignees.member.user.alarmSetting 관계를 포함하도록 Prisma 조회를 변경하십시오. 알림 생성 조건을 단일 assignee 기준이 아닌 각 subTaskAssignees 담당자 기준으로 처리하고, 기존 알림이 없을 때 각 담당자에게 createSubTaskAlarm을 호출하십시오.
281-287: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win다중 담당자 관계로 알림 생성 조건을 변경하십시오.
createSubTask는subTaskAssignees를 반환합니다. 라인 285의subTask.assigneeId와 라인 286의subTask.assignee는 이 결과에 없습니다. 따라서 새 세부 과제의 담당자 알림이 생성되지 않습니다.
subTask.subTaskAssignees를 순회하고, 각assignment.member.user의taskAlarm설정으로 알림을 생성하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task.service.js` around lines 281 - 287, Update the notification condition and creation flow around createSubTask to use subTask.subTaskAssignees instead of subTask.assigneeId and subTask.assignee. Iterate each assignment, use assignment.member.user.taskAlarm to determine eligibility, and create the corresponding alarm for every assigned user while preserving the existing endDate and isAlarm requirements.src/services/alarm.service.js (2)
289-300: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win담당자 검증을 상태 저장 전에 수행하십시오.
현재 코드는 먼저
isAlarm을true로 저장합니다. 그 후 담당자가 없으면SUBTASK_NO_ASSIGNEE오류를 반환합니다. 요청은 실패하지만 세부 과제는 알림 활성화 상태로 남습니다.
notifyAssignments검증을updateSubtaskAlarmStatus호출 전에 수행하십시오. 또는 상태 변경과 알림 처리를 하나의 트랜잭션으로 처리하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/alarm.service.js` around lines 289 - 300, Move the notifyAssignments validation for updatedSubTask.isAlarm === true before the updateSubtaskAlarmStatus call, so SUBTASK_NO_ASSIGNEE is thrown before persisting the alarm state. Preserve the existing validation condition and error response while ensuring failed requests cannot leave the subtask enabled.
314-334: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win기존 세부 과제 알림을 사용자별로 갱신하거나 교체하십시오.
existingAlarm을 조회하지만 결과를 사용하지 않습니다.isAlarm: true요청이 이미 활성화된 세부 과제에 다시 들어오면 모든 담당자에 대한createSubTaskAlarm이 다시 실행됩니다. 이 동작은 중복 알림을 만들거나 고유 제약 오류를 반환합니다.담당자별 알림을 upsert하십시오. 대안으로, 재생성 전에
deleteAlarmsBySubTaskIds([updatedSubTask.id])를 호출하고 모든 담당자 알림을 다시 생성하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/alarm.service.js` around lines 314 - 334, Update the alarm handling around existingAlarm and the notifyAssignments loop so existing subtask alarms are not duplicated when isAlarm remains enabled. Prefer upserting each assignee’s alarm using its existing record, or delete all alarms for updatedSubTask.id via deleteAlarmsBySubTaskIds before recreating them, while preserving per-assignee alarm dates and recipients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/alarm.service.js`:
- Around line 289-300: Move the notifyAssignments validation for
updatedSubTask.isAlarm === true before the updateSubtaskAlarmStatus call, so
SUBTASK_NO_ASSIGNEE is thrown before persisting the alarm state. Preserve the
existing validation condition and error response while ensuring failed requests
cannot leave the subtask enabled.
- Around line 314-334: Update the alarm handling around existingAlarm and the
notifyAssignments loop so existing subtask alarms are not duplicated when
isAlarm remains enabled. Prefer upserting each assignee’s alarm using its
existing record, or delete all alarms for updatedSubTask.id via
deleteAlarmsBySubTaskIds before recreating them, while preserving per-assignee
alarm dates and recipients.
In `@src/services/task.service.js`:
- Around line 772-776: Update the missing-user error in the task service flow to
include the required reason field, or replace the generic Error with the
established NotFoundError type. Preserve the existing 404 statusCode,
USER_NOT_FOUND errorCode, and Korean message while ensuring the thrown error
satisfies the global middleware contract.
- Around line 1288-1306: Update the alarm creation block for newSubTask to
iterate over newSubTask.subTaskAssignees instead of relying on
newSubTask.assigneeId, newSubTask.assignee, or the undeclared assignee variable.
For each assigned user, calculate the alarm using that user’s alarm settings and
create a separate alarm with the corresponding assignee identifier, while
preserving the existing isAlarm and endDate requirements.
- Around line 749-753: Update the assigneeIds parsing near parsedAssigneeUserIds
to validate every array element as a positive integer; if any value is invalid,
throw BadRequestError instead of filtering it out. Preserve replacement with an
empty list only when the caller explicitly provides assigneeIds: [].
- Around line 879-910: Remove the undefined nextAssigneeMemberId,
memberIdChanged, and newAssignments references in the alarm creation block. Use
the declared memberIdsChanged condition, then iterate
updatedTask.subTaskAssignees to create alarms for every newly assigned member
while preserving the existing endDate and alarm-setting calculations.
- Around line 1427-1457: 업데이트 흐름에서 assignmentChangeRequested 및 알림 생성 블록을 수정하여
endDate, isAlarm 또는 assigneeId가 변경된 경우에만 alarmDate를 재계산하고 알림을 교체하십시오. 재생성 전에 해당
subTaskId의 기존 알림을 삭제하거나 사용자별 upsert를 사용해 중복 및 고유 제약 오류를 방지하고, 제목이나 상태만 변경된 경우 기존
알림과 생성 흐름을 건드리지 마십시오.
- Around line 1431-1458: In the alarm-processing block of updateSubTask, remove
the duplicate alarmHours declaration and the undefined assignee reference. Do
not gate processing on updatedSubTask.assigneeId; instead iterate
updatedSubTask.subTaskAssignees and create an alarm for each assignee using the
joined data already returned by updateSubTask, without re-querying the SubTask.
- Around line 605-701: updateSubTaskDeadline의 기존 assignee 조회를 제거하고
subTaskAssignees.member.user.alarmSetting 관계를 포함하도록 Prisma 조회를 변경하십시오. 알림 생성 조건을
단일 assignee 기준이 아닌 각 subTaskAssignees 담당자 기준으로 처리하고, 기존 알림이 없을 때 각 담당자에게
createSubTaskAlarm을 호출하십시오.
- Around line 281-287: Update the notification condition and creation flow
around createSubTask to use subTask.subTaskAssignees instead of
subTask.assigneeId and subTask.assignee. Iterate each assignment, use
assignment.member.user.taskAlarm to determine eligibility, and create the
corresponding alarm for every assigned user while preserving the existing
endDate and isAlarm requirements.
In `@src/swagger/swagger.yml`:
- Around line 2585-2628: Update the assigneeIds item schema in the sub-task
assignee endpoint to add minimum: 1, matching the positive-integer validation.
Keep assigneeIds optional, allow empty arrays, and preserve the existing
response structure.
- Around line 6055-6066: Update the Apple OAuth flow documented around the
/apple and /apple/callback endpoints to generate a cryptographically
unpredictable, request-specific state and nonce, store the environment mapping
server-side, and include them in the authorization request. In the callback,
validate and consume state exactly once before selecting the redirect
environment or completing login, and verify the Apple ID token nonce against the
stored request nonce; do not use the client-provided environment value directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48c33c52-3177-4d91-b7f6-cca9cc2d7b08
📒 Files selected for processing (9)
prisma/schema.prismasrc/controllers/task.controller.jssrc/dtos/task.dto.jssrc/helpers/task.helper.jssrc/repositories/task.repository.jssrc/services/alarm.service.jssrc/services/task.service.jssrc/swagger/swagger.ymlsrc/validators/task.schema.js
🚧 Files skipped from review as they are similar to previous changes (6)
- src/controllers/task.controller.js
- src/helpers/task.helper.js
- src/validators/task.schema.js
- src/dtos/task.dto.js
- prisma/schema.prisma
- src/repositories/task.repository.js
…issue-589 # Conflicts: # src/services/task.service.js
merge 시 develop 쪽 변경이 다중 담당자 리팩토링 이전 코드로 일부 로직을 덮어써서 생긴 문제 수정: - task.service.js updateSubTaskDeadline: 존재하지 않는 Prisma 관계 `assignee`를 include하여 호출 시마다 예외가 발생하던 문제를 subTaskAssignees 기반 다중 담당자 순회로 수정 - alarm.service.js updateSubtaskAlarmStatus: 사용되지 않는 단일 담당자 잔재 코드(assignee 참조, 미사용 쿼리) 제거 및 알림 재생성 전 기존 알림 삭제 로직 복원(중복 알림 생성 방지) - 관련 유닛 테스트 mock을 다중 담당자 구조에 맞게 갱신
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
prisma/migrations/20260809040000_add_sub_task_assignee/migration.sql (1)
26-30: 🩺 Stability & Availability | 🔵 Trivial무중단 배포에서는
assignee_id를 즉시 삭제하지 마십시오.Line 30은 백필 직후 열을 삭제합니다. 이전 애플리케이션 인스턴스가 배포 중에
"SubTask"."assignee_id"를 읽거나 쓰면 SQL 오류가 발생합니다. 무중단 배포를 사용하면 expand/contract 배포로 분리하거나, 마이그레이션 전에 이전 인스턴스가 모두 종료되는지 확인하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/migrations/20260809040000_add_sub_task_assignee/migration.sql` around lines 26 - 30, Update the migration around the SubTask assignee_id backfill and DROP COLUMN so zero-downtime deployments remain compatible: do not remove assignee_id immediately after backfilling while older application instances may still read or write it. Use an expand/contract sequence that defers dropping the column and foreign key until all old instances are retired, or explicitly enforce a deployment process that stops them before this destructive migration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/task.helper.js`:
- Around line 68-76: normalizedAssigneeIds를 생성하는 assigneeIds.map 검증을 수정해 담당자 ID가
양의 안전한 정수만 허용되도록 하십시오. 문자열은 10진 정수 형식만 통과시키고, true 같은 비문자열 값과 소수·Infinity·안전 범위를
벗어난 값은 Number 변환 전에 거부하십시오. 유효성 검증 실패 시 기존 BadRequestError를 유지하고, 통과한 값만 parsed로
반환하십시오.
---
Nitpick comments:
In `@prisma/migrations/20260809040000_add_sub_task_assignee/migration.sql`:
- Around line 26-30: Update the migration around the SubTask assignee_id
backfill and DROP COLUMN so zero-downtime deployments remain compatible: do not
remove assignee_id immediately after backfilling while older application
instances may still read or write it. Use an expand/contract sequence that
defers dropping the column and foreign key until all old instances are retired,
or explicitly enforce a deployment process that stops them before this
destructive migration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 165ed2f5-6455-4522-8367-1d0df5ba592c
📒 Files selected for processing (6)
prisma/migrations/20260809040000_add_sub_task_assignee/migration.sqlsrc/helpers/task.helper.jssrc/repositories/task.repository.jssrc/services/alarm.service.jssrc/services/task.service.jstests/unit/subtask-enddate-nullable.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/alarm.service.js
- src/repositories/task.repository.js
- src/services/task.service.js
📌 관련 이슈번호
📌 PR 유형
어떤 변경 사항이 있나요?
📌 PR 요약
세부 과제 담당자를 단일(1명)에서 다중(배열)으로 할당할 수 있도록 수정.
📌 작업 세부 내용
assigneeId(단일)에서assigneeIds(배열,userId기준)로 변경assigneeIds에 빈 배열([]) 전달 시 담당자가 0명으로 처리📸 스크린샷 (선택)
🔗 참고 자료
Summary by CodeRabbit
새로운 기능
개선 사항