Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds seller-driven milestone status transitions to the escrow module: a new PATCH controller endpoint, DTO for validation, service logic with transactional updates and guards, extended MilestoneStatus enum and entity, a PostgreSQL enum migration, and unit tests validating access control, transitions, idempotency, and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Seller as Seller (auth)
participant API as EscrowController
participant SVC as EscrowService
participant DB as DB (Escrow & Milestone)
Seller->>API: PATCH /escrows/:escrowId/milestones/:milestoneId/status {status}
note right of API: JwtAuthGuard + RolesGuard(Seller)
API->>SVC: changeMilestoneStatus(escrowId, milestoneId, sellerId, status)
rect rgb(242,248,255)
SVC->>DB: Begin transaction
SVC->>DB: Load Escrow, Milestone
SVC-->>SVC: Validate ownership, linkage, allowed status, progression
alt invalid access/transition
SVC-->>API: throw Forbidden/BadRequest
else valid
SVC->>DB: Update Milestone.status (+timestamps)
SVC->>DB: Commit
SVC-->>API: Updated Milestone
end
end
API-->>Seller: 200 OK (Milestone)
note over Seller,API: Buyer approval flow remains separate (approve endpoint)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (6)
src/app.module.ts (1)
69-71: Consider autoLoadEntities to reduce manual maintenanceManually listing entities scales poorly. Optionally enable autoLoadEntities in TypeOrmModule.forRoot to avoid updating this list whenever new entities are added.
src/modules/escrows/dto/approve-milestone.dto.ts (1)
3-7: Avoid placeholder fields or document them for API consumersIf
typeisn’t used yet, consider removing it to keep the contract lean, or document/enum the intended values to prevent ambiguity in clients and validation.src/modules/escrows/dto/update-milestone-status.dto.ts (1)
5-8: Restrict DTO to seller-allowed statuses only
@IsEnum(MilestoneStatus)accepts all enum values, includingpendingandapproved. Constrain to READY | IN_PROGRESS | DELIVERED so bad requests fail at validation, not in service logic.Apply:
-import { IsEnum } from 'class-validator'; +import { IsIn } from 'class-validator'; import { MilestoneStatus } from '../entities/milestone.entity'; // Seller-changeable statuses (not including APPROVED which is buyer action) export class UpdateMilestoneStatusDto { - @IsEnum(MilestoneStatus, { message: 'Invalid milestone status' }) - status: MilestoneStatus; // Expect READY | IN_PROGRESS | DELIVERED + @IsIn([MilestoneStatus.READY, MilestoneStatus.IN_PROGRESS, MilestoneStatus.DELIVERED], { + message: 'Invalid milestone status', + }) + status: MilestoneStatus; // ready | in_progress | delivered }Based on learnings
test/escrow.e2e-spec.ts (3)
44-53: E2E uses AppModule DB config; prefer isolated test DBBootstrapping AppModule will use real DATABASE_URL. Consider a test module with an in-memory or containerized DB (drop schema per test) to avoid external dependencies and flakiness.
132-138: Idempotency status code choiceSecond approval returns 400 here; 409 Conflict or 200 (idempotent OK) could be clearer. Align with API conventions.
33-41: Add tests for seller status transitionsPlease add e2e/specs for PATCH /escrows/:escrowId/milestones/:milestoneId/status covering linear transitions and rejections (e.g., skipping steps, moving backward, after approved).
I can scaffold concise tests for ready → in_progress → delivered and invalid transitions. Want me to add them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.husky/post-checkout(1 hunks).husky/post-commit(1 hunks).husky/post-merge(1 hunks).husky/pre-push(1 hunks)src/app.module.ts(3 hunks)src/modules/escrows/controllers/escrow.controller.ts(1 hunks)src/modules/escrows/dto/approve-milestone.dto.ts(1 hunks)src/modules/escrows/dto/update-milestone-status.dto.ts(1 hunks)src/modules/escrows/entities/escrow.entity.ts(1 hunks)src/modules/escrows/entities/milestone.entity.ts(1 hunks)src/modules/escrows/escrows.module.ts(1 hunks)src/modules/escrows/migrations/1752190000000-ExtendMilestoneStatusEnum.ts(1 hunks)src/modules/escrows/services/escrow.service.spec.ts(1 hunks)src/modules/escrows/services/escrow.service.ts(1 hunks)test/escrow.e2e-spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/modules/escrows/entities/milestone.entity.ts (1)
src/modules/escrows/entities/escrow.entity.ts (1)
Entity(12-53)
src/modules/escrows/entities/escrow.entity.ts (1)
src/modules/escrows/entities/milestone.entity.ts (1)
Entity(12-48)
src/modules/escrows/escrows.module.ts (1)
src/app.module.ts (1)
Module(43-92)
src/modules/escrows/controllers/escrow.controller.ts (3)
src/modules/wishlist/controller/wishlist.controller.ts (1)
UseGuards(16-47)src/modules/wishlist/common/types/auth-request.type.ts (1)
AuthRequest(4-14)src/modules/escrows/dto/update-milestone-status.dto.ts (1)
UpdateMilestoneStatusDto(5-8)
🔇 Additional comments (5)
src/modules/escrows/escrows.module.ts (1)
8-14: Module wiring looks correctImports, providers, controller, and export are properly set up.
src/app.module.ts (2)
39-41: EscrowsModule integration imports look goodNew entities and module imports are correctly added.
89-89: EscrowsModule added to imports is fineNo concerns here.
test/escrow.e2e-spec.ts (2)
115-123: No changes needed for response shape assertions
Global ResponseInterceptor and manual controller responses already wrap payloads as{ success: boolean, data: … }, so assertingres.body.successandres.body.data.statusis correct.
99-109: No changes needed:/auth/registersets atokencookie
TheregisterWithWallethandler callssetToken(res, …), which sets a cookie namedtoken, so your test’s extraction ofauthTokenBuyer/authTokenSellerfromset-cookieis valid.
| #!/bin/sh | ||
| command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } | ||
| git lfs pre-push "$@" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore Husky helper sourcing before running commands
All Husky-managed hooks should source . "$(dirname "$0")/_/husky.sh" right after the shebang. Without it we lose Husky’s guardrails (honoring HUSKY=0, PATH adjustments, consistent error handling), so these hooks will ignore standard skip mechanisms and can surprise CI/dev workflows. Please add the sourcing line here (and mirror it on the other new hook scripts in this PR).
#!/bin/sh
+. "$(dirname "$0")/_/husky.sh"
+
command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; }
git lfs pre-push "$@"📝 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.
| #!/bin/sh | |
| command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } | |
| git lfs pre-push "$@" | |
| #!/bin/sh | |
| . "$(dirname "$0")/_/husky.sh" | |
| command -v git-lfs >/dev/null 2>&1 || { printf >&2 "\n%s\n\n" "This repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks')."; exit 2; } | |
| git lfs pre-push "$@" |
🤖 Prompt for AI Agents
In .husky/pre-push around lines 1 to 3, the hook is missing Husky's helper
sourcing line immediately after the shebang; add the line . "$(dirname
"$0")/_/husky.sh" right after #!/bin/sh so the hook honors HUSKY=0, PATH
adjustments and Husky error handling before running the existing git-lfs checks,
and apply the same insertion to the other new hook scripts in this PR.
| @Entity('escrows') | ||
| @Check('"totalAmount" >= 0') | ||
| export class Escrow { | ||
| @PrimaryGeneratedColumn('uuid') | ||
| id: string; | ||
|
|
||
| @Column({ type: 'uuid', name: 'offer_id' }) | ||
| offerId: string; | ||
|
|
||
| @ManyToOne(() => Offer, { nullable: false }) | ||
| @JoinColumn({ name: 'offer_id' }) | ||
| offer: Offer; | ||
|
|
||
| @Column({ name: 'buyer_id' }) | ||
| buyerId: number; | ||
|
|
||
| @ManyToOne(() => User, { nullable: false }) | ||
| @JoinColumn({ name: 'buyer_id' }) | ||
| buyer: User; | ||
|
|
||
| @Column({ name: 'seller_id' }) | ||
| sellerId: number; | ||
|
|
||
| @ManyToOne(() => User, { nullable: false }) | ||
| @JoinColumn({ name: 'seller_id' }) | ||
| seller: User; | ||
|
|
||
| @Column({ type: 'decimal', precision: 12, scale: 2, name: 'total_amount' }) | ||
| totalAmount: number; |
There was a problem hiding this comment.
Fix the CHECK constraint column name
The CHECK constraint references "totalAmount", but the actual column is persisted as "total_amount" (per name: 'total_amount'). As written, TypeORM will emit a constraint against a non-existent column and the migration will fail. Update the constraint to reference the snake_case column name so the table can be created safely.
-@Check('"totalAmount" >= 0')
+@Check('"total_amount" >= 0')🤖 Prompt for AI Agents
In src/modules/escrows/entities/escrow.entity.ts around lines 12 to 40, the
CHECK constraint references "totalAmount" while the actual DB column is named
"total_amount"; update the Check decorator to use the snake_case column name
(e.g., @Check('"total_amount" >= 0')) so the constraint targets the persisted
column and the migration will succeed.
| if (nextOrder < currentOrder) { | ||
| throw new BadRequestException('Cannot move milestone status backwards'); | ||
| } | ||
| if (currentOrder === nextOrder) { | ||
| return milestone; // idempotent | ||
| } | ||
|
|
||
| milestone.status = nextStatus; | ||
| await manager.save(milestone); |
There was a problem hiding this comment.
Sequential milestone transitions are not enforced
Line 91 allows any forward jump (e.g., PENDING → DELIVERED) because the only guard is nextOrder < currentOrder. This breaks the PR requirement of linear transitions (pending → ready → in_progress → delivered) and lets sellers bypass intermediate states. Please block forward skips by ensuring the next status is at most one step ahead.
- if (nextOrder < currentOrder) {
- throw new BadRequestException('Cannot move milestone status backwards');
- }
+ if (nextOrder < currentOrder) {
+ throw new BadRequestException('Cannot move milestone status backwards');
+ }
+ if (nextOrder - currentOrder > 1) {
+ throw new BadRequestException('Milestone status must advance one step at a time');
+ }📝 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.
| if (nextOrder < currentOrder) { | |
| throw new BadRequestException('Cannot move milestone status backwards'); | |
| } | |
| if (currentOrder === nextOrder) { | |
| return milestone; // idempotent | |
| } | |
| milestone.status = nextStatus; | |
| await manager.save(milestone); | |
| if (nextOrder < currentOrder) { | |
| throw new BadRequestException('Cannot move milestone status backwards'); | |
| } | |
| if (nextOrder - currentOrder > 1) { | |
| throw new BadRequestException('Milestone status must advance one step at a time'); | |
| } | |
| if (currentOrder === nextOrder) { | |
| return milestone; // idempotent | |
| } | |
| milestone.status = nextStatus; | |
| await manager.save(milestone); |
🤖 Prompt for AI Agents
In src/modules/escrows/services/escrow.service.ts around lines 91 to 99, the
current check only forbids backward moves but permits skipping ahead multiple
steps; change the validation to forbid forward skips by throwing when nextOrder
> currentOrder + 1 (while keeping the existing backward check and idempotent
early return), so only the immediate next status is allowed before setting
milestone.status = nextStatus and saving via manager.save(milestone).
| const br = brRepo.create({ | ||
| title: 'Test Request', | ||
| description: 'Need something', | ||
| budgetMin: 10, | ||
| budgetMax: 100, | ||
| categoryId: 1, | ||
| userId: buyer.id, | ||
| status: 'open', | ||
| }); | ||
| await brRepo.save(br); |
There was a problem hiding this comment.
Hardcoded foreign key values risk FK violations
categoryId: 1 assumes an existing category. Seed or create required FK rows within the test to avoid failures in clean environments.
🤖 Prompt for AI Agents
In test/escrow.e2e-spec.ts around lines 63 to 72 you hardcode categoryId: 1
which risks FK violations in clean DBs; instead create (or fetch) the required
category row in the test and use its id for the business request: use the
category repository to create and save a Category entity (or seed a minimal
category), then assign categoryId to the savedCategory.id (or the fetched id)
before saving the BR so the test does not depend on implicit seeded data.
2 participants
🚀 StarShop Pull Request - Issue #160
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Adds seller milestone workflow:
MilestoneStatusenum: pending, ready, in_progress, delivered, approved.UpdateMilestoneStatusDto.changeMilestoneStatus(seller-only, linear forward transitions, blocked if approved).PATCH /escrows/:escrowId/milestones/:milestoneId/statusBody:
{ "status": "ready" | "in_progress" | "delivered" }Future: add tests for status transitions & authorization, integrate indexer/event emission.
📸 Evidence (A photo is required as evidence)
Example request:
PATCH /api/v1/escrows/<<escrowId>>/milestones/<<milestoneId>>/statusBody:
Example success response (wrapped by interceptor):
⏰ Time spent breakdown
Total: ~40m
🌌 Comments
Tests not added yet—suggest adding unit tests for:
Indexer hook placeholder can be added inside service after status change.
Summary by CodeRabbit