feat: add manual project completion validation#679
Conversation
|
@usmanliaqat0 is attempting to deploy a commit to the TryCatch 4 Match's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds conclusion-eligibility validation to the PATCH /api/team-project/user route and expands unit coverage for the new failure paths and stack-checking behavior. ChangesProject conclusion validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/team-project/user/route.ts (1)
124-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdmin allowance now unreachable for this route.
The pre-existing check at Lines 124-131 permits
ADMINrole to pass alongside the owner. But the new checks requirestatus === CONCLUIDO(Lines 133-140) and then strictlyexisting.ownerId === session.user.idwith no ADMIN exception (Lines 142-149). Since this route now only performs conclusion, an ADMIN can never successfully complete a PATCH here — they will always be rejected at Line 142-149 with "Apenas o owner pode concluir". This makes the ADMIN branch of the earlier check dead/misleading, and admins lose access they appear to have.Either explicitly allow ADMIN to conclude too, or remove the ADMIN exception from the earlier ownership check to avoid confusion.
Proposed fix (allow ADMIN to conclude)
- if (existing.ownerId !== session.user.id) { + if (existing.ownerId !== session.user.id && session.user.role !== 'ADMIN') { return buildResponse({ success: false, message: MESSAGES.AUTH.UNAUTHORIZED, status: 403, errors: ['Apenas o owner pode concluir este projeto.'], }); }Also applies to: 142-149
🤖 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/app/api/team-project/user/route.ts` around lines 124 - 131, The authorization flow in the user route is inconsistent because ADMIN is allowed in the earlier ownership check in the PATCH handler but then blocked later by the conclusion-only owner check. Update the logic in the route handler to make the intent consistent: either allow ADMIN to pass the `status === CONCLUIDO`/owner validation path as well, or remove the ADMIN exception from the initial check so `existing.ownerId` and `session.user.role` rules match. Use the `PATCH` handler’s ownership/status checks and the `buildResponse` unauthorized responses to keep the behavior clear.
🧹 Nitpick comments (3)
src/app/api/team-project/user/route.ts (2)
167-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueZero-stack case shares a misleading message with incomplete-stack case.
When
totalProjectStacks === 0, the error "Todas as stacks devem estar assumidas para concluir." is inaccurate — there are no stacks to assume at all. If a project with zero stacks should legitimately be blocked from conclusion, consider a distinct error message clarifying that a project needs at least one defined stack.🤖 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/app/api/team-project/user/route.ts` around lines 167 - 174, The validation in the team-project user route currently uses the same “all stacks must be assumed” message for both incomplete-stack and zero-stack projects. Update the conditional around the conclusion check in the route handler so the zero-stack case gets a distinct error message explaining that a project must have at least one defined stack before it can be concluded, while keeping the existing message for the incomplete-stack case.
160-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate stack-completeness logic vs.
checkProjectStatus.The stack counting and comparison here duplicates the same
projectStack.count/stackTaken.countlogic already implemented incheckProjectStatus(src/lib/check-project-status.ts, Lines 47-59). Consider extracting a shared helper (e.g.,getStackCompleteness(projectId)) to avoid divergence between the two implementations over time.🤖 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/app/api/team-project/user/route.ts` around lines 160 - 174, The stack-completeness check is duplicated in this route and in checkProjectStatus, so centralize the projectStack/stackTaken counting logic into a shared helper and have both call it. Extract the repeated comparison into a reusable function (for example, a completeness helper used by checkProjectStatus and the team-project user route) so the validation stays consistent and changes only need to be made in one place.src/tests/unit/api/team-project/team-project-user-route.test.ts (1)
61-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for ADMIN role attempting conclusion.
Given the ownership logic in route.ts now blocks non-owners (including ADMIN) from concluding (Lines 142-149 of route.ts), an explicit test asserting this behavior would help lock in the intended contract and surface the dead-code concern raised in route.ts.
🤖 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/tests/unit/api/team-project/team-project-user-route.test.ts` around lines 61 - 151, Add an explicit test in the PATCH /api/team-project/user suite to cover an ADMIN user attempting to conclude a project and verify it is rejected with 403. Reuse the existing authorizeUser, PATCH, and createRequest helpers, but call authorizeUser with an ADMIN user context and assert the response matches MESSAGES.AUTH.UNAUTHORIZED and that prisma.project.update is not called. Place it alongside the existing ownership and status validation tests so the route.ts ownership check remains covered.
🤖 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/app/api/team-project/user/route.ts`:
- Around line 160-176: The conclude-project flow in the user route has a TOCTOU
gap because the `projectStack.count`, `stackTaken.count`, and
`prisma.project.update` calls are separate, non-atomic operations. Update the
logic around the eligibility check and final update in the route handler so they
run inside a single `prisma.$transaction(...)`, using the same `projectId`-based
read conditions before performing the `project.update` to keep the
check-and-update atomic.
---
Outside diff comments:
In `@src/app/api/team-project/user/route.ts`:
- Around line 124-131: The authorization flow in the user route is inconsistent
because ADMIN is allowed in the earlier ownership check in the PATCH handler but
then blocked later by the conclusion-only owner check. Update the logic in the
route handler to make the intent consistent: either allow ADMIN to pass the
`status === CONCLUIDO`/owner validation path as well, or remove the ADMIN
exception from the initial check so `existing.ownerId` and `session.user.role`
rules match. Use the `PATCH` handler’s ownership/status checks and the
`buildResponse` unauthorized responses to keep the behavior clear.
---
Nitpick comments:
In `@src/app/api/team-project/user/route.ts`:
- Around line 167-174: The validation in the team-project user route currently
uses the same “all stacks must be assumed” message for both incomplete-stack and
zero-stack projects. Update the conditional around the conclusion check in the
route handler so the zero-stack case gets a distinct error message explaining
that a project must have at least one defined stack before it can be concluded,
while keeping the existing message for the incomplete-stack case.
- Around line 160-174: The stack-completeness check is duplicated in this route
and in checkProjectStatus, so centralize the projectStack/stackTaken counting
logic into a shared helper and have both call it. Extract the repeated
comparison into a reusable function (for example, a completeness helper used by
checkProjectStatus and the team-project user route) so the validation stays
consistent and changes only need to be made in one place.
In `@src/tests/unit/api/team-project/team-project-user-route.test.ts`:
- Around line 61-151: Add an explicit test in the PATCH /api/team-project/user
suite to cover an ADMIN user attempting to conclude a project and verify it is
rejected with 403. Reuse the existing authorizeUser, PATCH, and createRequest
helpers, but call authorizeUser with an ADMIN user context and assert the
response matches MESSAGES.AUTH.UNAUTHORIZED and that prisma.project.update is
not called. Place it alongside the existing ownership and status validation
tests so the route.ts ownership check remains covered.
🪄 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: cc8572b1-5f8b-4ebb-9451-153b450d23b4
📒 Files selected for processing (3)
sonar-project.propertiessrc/app/api/team-project/user/route.tssrc/tests/unit/api/team-project/team-project-user-route.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/unit/api/team-project/team-project-user-route.test.ts (1)
137-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for ADMIN-but-non-owner rejection branch.
The route (per
src/app/api/team-project/user/route.ts) has a distinct branch: after thestatus !== CONCLUIDOcheck passes, it re-checksexisting.ownerId !== session.user.idand returns 403 "Apenas o owner pode concluir este projeto" — this specifically blocks ADMINs (who bypass the first ownership check) from concluding projects they don't own. This branch appears untested; the only 403 test here (non-owner, non-admin) is caught by the earlier check.Consider adding a test where
authorizeUsersimulates an ADMIN role for a different user id, asserting the second unauthorized branch is hit.🤖 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/tests/unit/api/team-project/team-project-user-route.test.ts` around lines 137 - 157, Add a unit test for the ADMIN-but-non-owner rejection path in the team-project user route. In the existing PATCH/authorizeUser test suite for team-project-user-route, create a case where the session user has ADMIN role but a different user id than the project owner, so the first ownership gate is bypassed and the second `existing.ownerId !== session.user.id` branch in `route.ts` is exercised. Assert the response is 403 with the specific “Apenas o owner pode concluir este projeto” message, and verify `prisma.project.update` is not called.
🤖 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/tests/unit/api/team-project/team-project-user-route.test.ts`:
- Line 100: The test setup is using an unsafe any cast for
prisma.projectStack.count, which should match the typed mock approach used
nearby. Update the mock in team-project-user-route.test.ts to cast
projectStack.count as a jest.Mock (like stackTaken.count) before calling
mockResolvedValue, so the test stays typed and avoids the no-explicit-any lint
violation.
---
Nitpick comments:
In `@src/tests/unit/api/team-project/team-project-user-route.test.ts`:
- Around line 137-157: Add a unit test for the ADMIN-but-non-owner rejection
path in the team-project user route. In the existing PATCH/authorizeUser test
suite for team-project-user-route, create a case where the session user has
ADMIN role but a different user id than the project owner, so the first
ownership gate is bypassed and the second `existing.ownerId !== session.user.id`
branch in `route.ts` is exercised. Assert the response is 403 with the specific
“Apenas o owner pode concluir este projeto” message, and verify
`prisma.project.update` is not called.
🪄 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: d2e5ce27-a60c-427e-a3c9-f5fe4085d208
📒 Files selected for processing (2)
src/app/api/team-project/user/route.tssrc/tests/unit/api/team-project/team-project-user-route.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/api/team-project/user/route.ts
| authorizeUser(); | ||
|
|
||
| (prisma.project.findUnique as jest.Mock).mockResolvedValue(mockProject()); | ||
| (prisma.projectStack as any).count.mockResolvedValue(2); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid any cast; use typed mock like sibling line.
Line 101 already uses (prisma.stackTaken.count as jest.Mock); line 100 should follow the same pattern instead of as any, which also trips the no-explicit-any ESLint rule.
🔧 Proposed fix
- (prisma.projectStack as any).count.mockResolvedValue(2);
+ (prisma.projectStack.count as jest.Mock).mockResolvedValue(2);📝 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.
| (prisma.projectStack as any).count.mockResolvedValue(2); | |
| (prisma.projectStack.count as jest.Mock).mockResolvedValue(2); |
🧰 Tools
🪛 ESLint
[error] 100-100: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Actions: CI Pipeline / 4_Build and Prepare Environment.txt
[error] 100-100: ESLint (typescript-eslint/no-explicit-any): Unexpected any. Specify a different type.
🪛 GitHub Actions: CI Pipeline / Build and Prepare Environment
[error] 100-100: ESLint (typescript-eslint/no-explicit-any): Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any
🤖 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/tests/unit/api/team-project/team-project-user-route.test.ts` at line 100,
The test setup is using an unsafe any cast for prisma.projectStack.count, which
should match the typed mock approach used nearby. Update the mock in
team-project-user-route.test.ts to cast projectStack.count as a jest.Mock (like
stackTaken.count) before calling mockResolvedValue, so the test stays typed and
avoids the no-explicit-any lint violation.
Source: Linters/SAST tools
correção de lint
|
2 participants
Summary
CONCLUIDOstatus,EM_ANDAMENTOcurrent state, and all stacks taken before completion.Validation
npx jest src/tests/unit/api/team-project/team-project-user-route.test.ts --coverage --collectCoverageFrom='src/app/api/team-project/user/route.ts' --runInBandnpm run lintnpm testnpm run buildnpm run test:push/,/login,/dashboard/team-projects, andPATCH /api/team-project/userSummary by CodeRabbit
PATCHscenarios and added coverage forGETfailure paths, including authorization, validation, stack-completion, and error handling.