Update Distribution Functionality - #19
Merged
Idrhas merged 7 commits intoMar 26, 2026
Merged
Conversation
|
@DioChuks Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
📝 WalkthroughWalkthroughAdds a PATCH /distributions/:id endpoint: new validation and DTO types, a service method to apply partial updates (normalize addresses/symbol, recompute totalUsdAmount, handle metadata), a controller handler mapping not-found vs internal errors, and route registration with param/body validation middleware. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Controller as Controller
participant Service as DistributionService
participant Repo as Repository
participant DB as Database
Client->>Router: PATCH /distributions/:id + body
Router->>Controller: invoke updateDistribution(req, res)
Controller->>Service: updateDistribution(id, validatedData)
Service->>Repo: findOne(id)
Repo->>DB: SELECT distribution WHERE id=...
DB-->>Repo: distribution record / null
Repo-->>Service: entity / null
alt entity found
Service->>Service: normalize fields, recompute totalUsdAmount, process metadata
Service->>Repo: save(updatedEntity)
Repo->>DB: UPDATE distribution
DB-->>Repo: saved record
Repo-->>Service: saved entity
Service-->>Controller: DistributionResponseDto
Controller-->>Client: 200 { data: ..., success: true, message: "Distribution updated" }
else not found
Service-->>Controller: throws "Distribution not found"
Controller-->>Client: 404 { data: null, success: false, message: "Distribution not found" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/v1/distribution/distribution.controller.ts`:
- Around line 53-60: The current error handling builds errorResponse using the
raw error.message for all failures, which can leak internals; change the logic
so that when computing message you only expose the actual error.message for the
known "Distribution not found" case (status 404) and use a generic "Internal
server error" message for all other errors (status 500); keep the existing
status calculation (status) and res.status(status).json(errorResponse) but set
errorResponse.message conditionally and optionally log the full error via your
logger (e.g., console.error or processLogger.error) before sending the generic
500 response.
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 30-43: The update flow currently assigns updateData directly and
skips the normalization performed in prepareDistributionData; ensure the same
normalization is applied before saving by either running updateData through
prepareDistributionData or explicitly normalizing mutable fields (e.g., call
prepareDistributionData(updateData) or apply updateData.tokenAddress =
updateData.tokenAddress?.toLowerCase() and updateData.symbol =
updateData.symbol?.toUpperCase()) prior to computing totalUsdAmount, setting
metadata, and calling this.distributionRepository.save(distribution); keep
references to calculateTotalUsdAmount, processMetadata, and
Object.assign(distribution, updatedFields) intact.
In `@src/components/v1/distribution/distrubtion.routes.ts`:
- Line 12: The route currently only validates the request body via
policyMiddleware(updateDistributionSchema) and lets req.params.id bypass
validation; add a params schema (e.g., updateDistributionParamsSchema as
suggested) that validates id as a UUID and ensure the route applies params
validation middleware before controller execution (either by calling
policyMiddleware with the params schema or updating policyMiddleware to
accept/validate params), so distributionRouter.patch("/distributions/:id",
policyMiddleware(updateDistributionParamsSchema),
policyMiddleware(updateDistributionSchema), updateDistribution) (or equivalent
ordering) enforces id validation before updateDistribution runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4751273b-34c6-4a05-a8c2-ba0fa5d0a6bf
📒 Files selected for processing (5)
src/components/v1/distribution/distribution.controller.tssrc/components/v1/distribution/distribution.dto.tssrc/components/v1/distribution/distribution.service.tssrc/components/v1/distribution/distribution.validation.tssrc/components/v1/distribution/distrubtion.routes.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…huks/Fundable-Backend into feat/add-patch-distribution
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/v1/distribution/distribution.service.ts (1)
42-46: Consider using explicit!== undefinedchecks for consistency.The truthy check works correctly for valid decimal strings but differs from the explicit
!== undefinedpattern used in lines 32-38. Using explicit checks would be more defensive and consistent.♻️ Suggested change for consistency
- if (updateData.totalAmount || updateData.usdRate) { + if (updateData.totalAmount !== undefined || updateData.usdRate !== undefined) { const totalAmount = updateData.totalAmount ?? distribution.totalAmount const usdRate = updateData.usdRate ?? distribution.usdRate updatedFields.totalUsdAmount = this.calculateTotalUsdAmount(totalAmount, usdRate) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/distribution/distribution.service.ts` around lines 42 - 46, The conditional currently uses truthy checks for updateData.totalAmount and updateData.usdRate which is inconsistent with the explicit !== undefined pattern used earlier; change the if to explicitly check (updateData.totalAmount !== undefined || updateData.usdRate !== undefined) so calculateTotalUsdAmount is invoked when either field is present (even if zero or an empty-string-like value), keeping the existing assignment logic that computes totalAmount = updateData.totalAmount ?? distribution.totalAmount and usdRate = updateData.usdRate ?? distribution.usdRate and sets updatedFields.totalUsdAmount via this.calculateTotalUsdAmount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 48-50: The validation schema for the metadata field must allow
null to match the DistributionResponseDto type and the service must handle null
assignments explicitly: update the metadata schema (where the DTO/schema is
defined) from z.record(z.any()).optional() to
z.record(z.any()).nullable().optional(), and in distribution.service.ts update
the logic around updateData.metadata so that if updateData.metadata === null you
set updatedFields.metadata = null, else if it is defined call
this.processMetadata(updateData.metadata) as currently done; reference
DistributionResponseDto, the metadata validation schema, updatedFields, and
processMetadata to locate the changes.
---
Nitpick comments:
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 42-46: The conditional currently uses truthy checks for
updateData.totalAmount and updateData.usdRate which is inconsistent with the
explicit !== undefined pattern used earlier; change the if to explicitly check
(updateData.totalAmount !== undefined || updateData.usdRate !== undefined) so
calculateTotalUsdAmount is invoked when either field is present (even if zero or
an empty-string-like value), keeping the existing assignment logic that computes
totalAmount = updateData.totalAmount ?? distribution.totalAmount and usdRate =
updateData.usdRate ?? distribution.usdRate and sets updatedFields.totalUsdAmount
via this.calculateTotalUsdAmount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b04514b0-aa5a-409b-adf8-8928f7e893da
📒 Files selected for processing (1)
src/components/v1/distribution/distribution.service.ts
Contributor
|
Please fix the merge conflict |
Contributor
Author
|
on it |
Contributor
Author
|
@Utilitycoder done |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 42-46: The update path recomputes USD totals via
calculateTotalUsdAmount using Number.parseFloat which is unsafe for
decimal(65,30) values; change calculateTotalUsdAmount to perform decimal-safe
arithmetic (e.g., use Decimal from decimal.js or Big from big.js), accept
decimal string inputs for totalAmount and usdRate, multiply using the library,
and return a string (or a value matching DB decimal storage) to avoid precision
loss; update the caller in the PATCH/update flow (where updateData.totalAmount/
usdRate and updatedFields.totalUsdAmount are set) to pass strings/unmodified
decimal values through calculateTotalUsdAmount and ensure package.json includes
the chosen decimal library.
🪄 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
Run ID: 405f2950-9e7b-40f5-bafc-05778ac4c53b
📒 Files selected for processing (4)
src/components/v1/distribution/distribution.controller.tssrc/components/v1/distribution/distribution.service.tssrc/components/v1/distribution/distribution.validation.tssrc/components/v1/distribution/distrubtion.routes.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/v1/distribution/distrubtion.routes.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/v1/distribution/distribution.controller.ts
- src/components/v1/distribution/distribution.validation.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 24-32: The updateDistribution flow does an unprotected
read-modify-write which can cause lost updates; fix by either (A) using a
database transaction and obtaining a row lock when reading the entity (e.g., in
updateDistribution call use a transactional EntityManager and
distributionRepository.findOne with a pessimistic write lock) before applying
updateData and saving, or (B) add optimistic concurrency control to
DistributionEntity (add a `@VersionColumn`) and rely on optimistic locking during
repository.save to surface conflicts so callers can retry; update
updateDistribution to use one of these approaches (reference updateDistribution,
distributionRepository.findOne, DistributionEntity, UpdateDistributionDto).
🪄 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
Run ID: 632daab2-1f2e-4f05-9866-e59e7270d550
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/components/v1/distribution/distribution.service.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
Comment on lines
+24
to
+32
| async updateDistribution(id: string, updateData: UpdateDistributionDto): Promise<DistributionResponseDto> { | ||
| try { | ||
| const distribution = await this.distributionRepository.findOne({ where: { id } }) | ||
| if (!distribution) { | ||
| throw new Error("Distribution not found") | ||
| } | ||
|
|
||
| const updatedFields: Partial<DistributionEntity> = { ...updateData } | ||
|
|
Contributor
There was a problem hiding this comment.
Prevent lost updates in concurrent PATCH requests.
Line 26 + Line 55-Line 56 currently do a read-modify-write without optimistic/pessimistic concurrency control. Two overlapping updates to the same distribution can overwrite each other unintentionally.
Consider protecting this flow with row locking in a transaction or optimistic versioning (@VersionColumn) so concurrent PATCH calls cannot silently clobber fields.
Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/v1/distribution/distribution.service.ts` around lines 24 - 32,
The updateDistribution flow does an unprotected read-modify-write which can
cause lost updates; fix by either (A) using a database transaction and obtaining
a row lock when reading the entity (e.g., in updateDistribution call use a
transactional EntityManager and distributionRepository.findOne with a
pessimistic write lock) before applying updateData and saving, or (B) add
optimistic concurrency control to DistributionEntity (add a `@VersionColumn`) and
rely on optimistic locking during repository.save to surface conflicts so
callers can retry; update updateDistribution to use one of these approaches
(reference updateDistribution, distributionRepository.findOne,
DistributionEntity, UpdateDistributionDto).
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I have implemented the ability to partially update an existing distribution by ID via a
PATCHrequest.Changes Made
1. Validation and DTOs
updateDistributionSchemain distribution.validation.ts usingcreateDistributionSchema.partial().2. Service Layer
totalUsdAmountiftotalAmountorusdRateare changed.3. Controller Layer
4. Routing Layer
PATCH /distributions/:idin distrubtion.routes.ts.5. Casing Fix
./Distribution.service->./distribution.service).Verification Results
Code Review
Zodvalidation ensures type safety and data integrity.Manual Verification
The following files were updated:
Closes #3
Summary by CodeRabbit
New Features
Behavior Changes
Validation
Bug Fixes