fix: resolve DvaService dependency in PaymentModule + frontend securi…#218
Merged
Conversation
…ty & a11y improvements - Add DvaModule import (forwardRef) to PaymentModule to fix Render deployment crash - Register LOGISTICS_QUEUE in PaymentModule BullMQ imports - Add runtime MIME/extension file-type validation in verification page uploads - Improve upload tile accessibility: aria-disabled, dynamic tabIndex, guarded handlers - Pass domain-specific emptyMessage to bank SearchableSelect
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces audit logging for admin operations, adds idempotency key support to order DTOs, implements deterministic idempotency key generation, refactors USSD payment flow to use centralized PaymentService, removes inventory reservation from direct-purchase OTP handling, and adds file-type validation with upload interaction guards to merchant verification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/backend/src/modules/admin/admin.service.ts (1)
526-532: Move audit log outside the transaction for consistency.The audit log call is inside the
$transactioncallback but usesthis.auditLog(which has its own Prisma client), not the transaction'stxclient. This means the audit write is not atomic with the user/profile changes—if the transaction fails to commit after the callback completes, the audit log persists while the actual changes roll back.All other audit log calls in this file (e.g.,
rejectMerchant,approveStaff,createSupplierAccount) are placed after the main operation completes, outside any transaction. For consistency and correctness, move this call outside:♻️ Suggested refactor
- return this.prisma.$transaction(async (tx) => { + const updatedUser = await this.prisma.$transaction(async (tx) => { // 1. Update the User role const updatedUser = await tx.user.update({ where: { id: userId }, data: { role: targetRole as any }, select: { id: true, email: true, firstName: true, lastName: true, role: true, }, }); // 2. Map logical role to integer access level const accessLevelText = targetRole === UserRole.SUPER_ADMIN ? "HIGH" : targetRole === UserRole.OPERATOR ? "MEDIUM" : "LOW"; // 3. Upsert the AdminProfile to attach the new staff schema await tx.adminProfile.upsert({ where: { userId: userId }, update: { accessLevel: accessLevelText, department: "Internal Operations", }, create: { userId: userId, accessLevel: accessLevelText, department: "Internal Operations", }, }); - await this.auditLog.log( - requestingAdminId, - "PROMOTE_ADMIN", - "User", - userId, - { targetRole }, - ); - this.logger.log( `User ${updatedUser.email} granted role ${targetRole} by admin ${requestingAdminId}`, ); return updatedUser; }); + + await this.auditLog.log( + requestingAdminId, + "PROMOTE_ADMIN", + "User", + userId, + { targetRole }, + ); + + return updatedUser; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/modules/admin/admin.service.ts` around lines 526 - 532, The audit log call for PROMOTE_ADMIN is currently invoked inside the Prisma $transaction callback and uses this.auditLog (its own Prisma client), so move the await this.auditLog.log(requestingAdminId, "PROMOTE_ADMIN", "User", userId, { targetRole }) out of the $transaction callback and place it immediately after the transaction completes successfully in the same method (the surrounding promote/admin update flow), ensuring it runs only after the tx commits; keep the same arguments and await the call, and handle/log any errors from this.auditLog.log as done by other methods like rejectMerchant/approveStaff/createSupplierAccount.apps/backend/src/modules/order/dto/checkout-cart.dto.ts (1)
37-39: ConstrainidempotencyKeyformat/length for safer request contracts.
@IsString()alone allows whitespace-only or very large values. Consider adding non-empty + max-length (and optional pattern) constraints.♻️ Proposed validation tightening
import { ArrayNotEmpty, IsArray, IsEnum, IsIn, IsNotEmpty, IsOptional, IsString, + MaxLength, + MinLength, IsUUID, IsObject, } from "class-validator"; @@ `@IsOptional`() `@IsString`() + `@MinLength`(8) + `@MaxLength`(128) idempotencyKey?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/modules/order/dto/checkout-cart.dto.ts` around lines 37 - 39, The idempotencyKey field currently only uses `@IsString`(), which allows empty/whitespace or arbitrarily long values; update the CheckoutCartDto's idempotencyKey to enforce a non-empty, bounded format by adding `@IsNotEmpty`() and a `@MaxLength` (e.g., 64 or 100) and optionally a `@Matches`(...) pattern to restrict to safe characters (alphanumeric and dashes/underscores) so validation fails for whitespace-only or overly long keys.apps/backend/test/stock-integrity.e2e-spec.ts (1)
10-13: Avoid leaking global prototype mutation across suites.Patching
BigInt.prototype.toJSONglobally without restore can cause cross-test side effects. Save/restore the original in setup/teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/test/stock-integrity.e2e-spec.ts` around lines 10 - 13, The test mutates BigInt.prototype.toJSON globally; save the original function (e.g., const originalToJSON = BigInt.prototype.toJSON) in setup, replace it for the test run, and restore it in teardown to avoid leaking across suites: set the patched implementation (BigInt.prototype.toJSON = function() { return this.toString(); }) in your beforeAll/beforeEach and restore BigInt.prototype.toJSON = originalToJSON in afterAll/afterEach so other tests are unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/modules/order/order.service.ts`:
- Around line 157-165: The deterministic idempotency key generation
(generateDeterministicKey) uses naive JSON.stringify so collection ordering
(e.g., cartItemIds) can change the hash; canonicalize collection fields by
sorting arrays (at least sort cartItemIds) or otherwise serializing payload in a
stable order before hashing, and update callers (where cartItemIds is passed at
lines around 444-452) to pass sorted arrays. Also wrap the transaction create
call (tx.order.create) in a try/catch that handles Prisma P2002
unique-constraint errors: on P2002 query and return the existing order (similar
to the Payment service findFirst pattern) instead of allowing the exception to
bubble up. Ensure idempotencyKey remains `@unique` on the Order model.
In `@apps/backend/src/modules/payment/payment.module.ts`:
- Line 21: The LogisticsProcessor decorated with `@Processor`(LOGISTICS_QUEUE) is
not being provided, so Nest won't instantiate it and queued jobs won't be
consumed; add LogisticsProcessor to a module's providers (or import the module
that provides it) and ensure the module includes BullModule.registerQueue({
name: LOGISTICS_QUEUE }) so the processor is instantiated; specifically locate
the module where BullModule.registerQueue({ name: LOGISTICS_QUEUE }) is called
(e.g., payment.module or queue.module) and add LogisticsProcessor to that
module's providers array or import the module that already provides
LogisticsProcessor.
In `@apps/web/src/app/`(dashboard)/merchant/verification/page.tsx:
- Around line 97-105: The file-type check currently allows files if either MIME
or extension is valid; change it so the extension must always be valid and the
MIME must be valid when present: require extValid to be true and, if file.type
is non-empty, require mimeValid as well (use ALLOWED_EXTENSIONS and
ALLOWED_MIME_TYPES to validate), otherwise show the same toast.error message;
update the conditional around file.name/file.type where ext, mimeValid, and
extValid are computed to enforce extValid && (file.type ? mimeValid : true).
---
Nitpick comments:
In `@apps/backend/src/modules/admin/admin.service.ts`:
- Around line 526-532: The audit log call for PROMOTE_ADMIN is currently invoked
inside the Prisma $transaction callback and uses this.auditLog (its own Prisma
client), so move the await this.auditLog.log(requestingAdminId, "PROMOTE_ADMIN",
"User", userId, { targetRole }) out of the $transaction callback and place it
immediately after the transaction completes successfully in the same method (the
surrounding promote/admin update flow), ensuring it runs only after the tx
commits; keep the same arguments and await the call, and handle/log any errors
from this.auditLog.log as done by other methods like
rejectMerchant/approveStaff/createSupplierAccount.
In `@apps/backend/src/modules/order/dto/checkout-cart.dto.ts`:
- Around line 37-39: The idempotencyKey field currently only uses `@IsString`(),
which allows empty/whitespace or arbitrarily long values; update the
CheckoutCartDto's idempotencyKey to enforce a non-empty, bounded format by
adding `@IsNotEmpty`() and a `@MaxLength` (e.g., 64 or 100) and optionally a
`@Matches`(...) pattern to restrict to safe characters (alphanumeric and
dashes/underscores) so validation fails for whitespace-only or overly long keys.
In `@apps/backend/test/stock-integrity.e2e-spec.ts`:
- Around line 10-13: The test mutates BigInt.prototype.toJSON globally; save the
original function (e.g., const originalToJSON = BigInt.prototype.toJSON) in
setup, replace it for the test run, and restore it in teardown to avoid leaking
across suites: set the patched implementation (BigInt.prototype.toJSON =
function() { return this.toString(); }) in your beforeAll/beforeEach and restore
BigInt.prototype.toJSON = originalToJSON in afterAll/afterEach so other tests
are unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ea41f38-5c42-48c1-8b63-435e389582fe
📒 Files selected for processing (10)
apps/backend/src/modules/admin/admin.service.tsapps/backend/src/modules/order/dto/checkout-cart.dto.tsapps/backend/src/modules/order/dto/create-direct-order.dto.tsapps/backend/src/modules/order/order.service.tsapps/backend/src/modules/payment/payment.module.tsapps/backend/src/modules/payment/payment.service.tsapps/backend/src/modules/ussd/ussd.service.tsapps/backend/test/stock-integrity.e2e-spec.tsapps/web/src/app/(dashboard)/merchant/settings/page.tsxapps/web/src/app/(dashboard)/merchant/verification/page.tsx
Comment on lines
+157
to
+165
| const idempotencyKey = | ||
| dto.idempotencyKey || | ||
| this.generateDeterministicKey("direct", buyerId, { | ||
| productId, | ||
| quantity, | ||
| deliveryAddress, | ||
| paymentMethod, | ||
| deliveryMethod, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect idempotency key schema constraints"
rg -n --type=prisma 'idempotencyKey|@@unique|@unique'
echo
echo "2) Inspect idempotency conflict handling in order/payment services"
rg -n --type=ts 'idempotencyKey|P2002|Unique constraint|findFirst\\(|findUnique\\(' apps/backend/src/modules/order apps/backend/src/modules/payment
echo
echo "3) Inspect deterministic key call sites for payload normalization"
rg -n --type=ts 'generateDeterministicKey\\(' apps/backend/src/modules/orderRepository: onerandomdevv/Swifta
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding Prisma schema file ==="
find . -name "*.prisma" -type f 2>/dev/null | head -20
echo -e "\n=== Checking for schema.prisma or similar ==="
find . -path "*prisma*schema*" -type f 2>/dev/null | head -20Repository: onerandomdevv/Swifta
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading order.service.ts to check file existence and size ==="
if [ -f "apps/backend/src/modules/order/order.service.ts" ]; then
wc -l "apps/backend/src/modules/order/order.service.ts"
else
echo "File not found, searching for order.service.ts"
find . -name "order.service.ts" -type f
fiRepository: onerandomdevv/Swifta
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Lines 157-165 of order.service.ts ==="
sed -n '157,165p' apps/backend/src/modules/order/order.service.ts
echo -e "\n=== Lines 444-452 of order.service.ts ==="
sed -n '444,452p' apps/backend/src/modules/order/order.service.ts
echo -e "\n=== Lines 1311-1319 of order.service.ts ==="
sed -n '1311,1319p' apps/backend/src/modules/order/order.service.tsRepository: onerandomdevv/Swifta
Length of output: 931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find generateDeterministicKey implementation ==="
rg -n 'generateDeterministicKey' apps/backend/src/modules/order --type=ts -A 20 | head -100
echo -e "\n=== Search for unique constraint on idempotencyKey in schema ==="
rg -n 'idempotencyKey' --type=ts --type=prisma apps/backend/srcRepository: onerandomdevv/Swifta
Length of output: 4583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for conflict handling patterns (P2002, try-catch) ==="
rg -n 'P2002|Unique constraint|findUnique|findFirst' apps/backend/src/modules/order apps/backend/src/modules/payment --type=ts -B 2 -A 5 | head -150Repository: onerandomdevv/Swifta
Length of output: 11471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading Prisma schema ==="
cat apps/backend/prisma/schema.prisma | grep -A 5 -B 5 'idempotencyKey' || echo "No idempotencyKey found in schema"Repository: onerandomdevv/Swifta
Length of output: 1546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check order model in Prisma schema for unique constraints ==="
rg -A 50 'model Order' apps/backend/prisma/schema.prisma | head -80Repository: onerandomdevv/Swifta
Length of output: 3983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for idempotencyKey usage in order creation (around lines 166-200) ==="
sed -n '166,250p' apps/backend/src/modules/order/order.service.ts
echo -e "\n=== Search for idempotencyKey in order creation (around lines 453-500) ==="
sed -n '453,500p' apps/backend/src/modules/order/order.service.tsRepository: onerandomdevv/Swifta
Length of output: 4346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for P2002 or conflict handling in order.service.ts ==="
rg -n 'P2002|catch|Unique constraint|idempotencyKey.*create' apps/backend/src/modules/order/order.service.ts -A 3 -B 3Repository: onerandomdevv/Swifta
Length of output: 3539
Deterministic key needs canonicalization + conflict handling for true idempotency.
The generateDeterministicKey implementation (lines 1311-1319) uses naive JSON.stringify without canonicalizing the payload. This causes an issue at lines 444-452 where cartItemIds (an array) is hashed directly—semantically identical checkouts with differently-ordered item IDs will generate different keys.
Additionally, the Order model has idempotencyKey @unique`` in the schema, but the order creation code (lines 168-227, 454-500) calls tx.order.create() with no try-catch. Duplicate requests with the same key will fail with P2002 (Unique constraint violation) instead of gracefully returning the existing order. The Payment service correctly handles this pattern with `findFirst` checks; order creation should follow the same approach.
Expected fixes:
- Sort
cartItemIdsbefore hashing (or canonicalize all collection fields) - Wrap order creation in try-catch to handle P2002 and return existing order
Also applies to: 444-452, 1311-1319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/modules/order/order.service.ts` around lines 157 - 165, The
deterministic idempotency key generation (generateDeterministicKey) uses naive
JSON.stringify so collection ordering (e.g., cartItemIds) can change the hash;
canonicalize collection fields by sorting arrays (at least sort cartItemIds) or
otherwise serializing payload in a stable order before hashing, and update
callers (where cartItemIds is passed at lines around 444-452) to pass sorted
arrays. Also wrap the transaction create call (tx.order.create) in a try/catch
that handles Prisma P2002 unique-constraint errors: on P2002 query and return
the existing order (similar to the Payment service findFirst pattern) instead of
allowing the exception to bubble up. Ensure idempotencyKey remains `@unique` on
the Order model.
| NotificationModule, | ||
| ConfigModule, | ||
| BullModule.registerQueue({ name: PAYOUT_QUEUE }), | ||
| BullModule.registerQueue({ name: LOGISTICS_QUEUE }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate logistics processor class/decorator"
rg -n --type=ts 'class\s+LogisticsProcessor|@Processor\(LOGISTICS_QUEUE'
echo
echo "2) Locate module provider registrations mentioning LogisticsProcessor"
rg -n --type=ts 'providers\s*:\s*\[[^]]*LogisticsProcessor|LogisticsProcessor'
echo
echo "3) Locate modules importing queue/logistics modules to ensure processor is in runtime graph"
rg -n --type=ts '@Module\(|imports\s*:\s*\['Repository: onerandomdevv/Swifta
Length of output: 4955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Check queue.module.ts for LogisticsProcessor registration ==="
cat -n apps/backend/src/queue/queue.module.ts
echo
echo "=== 2. Check payment.module.ts imports ==="
cat -n apps/backend/src/modules/payment/payment.module.ts
echo
echo "=== 3. Check logistics.processor.ts for `@Processor` decoration ==="
cat -n apps/backend/src/queue/logistics.processor.tsRepository: onerandomdevv/Swifta
Length of output: 6647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1. Search for all imports of LogisticsProcessor ==="
rg -n "LogisticsProcessor" --type=ts
echo
echo "=== 2. Check logistics.module.ts to see if processor should be registered there ==="
cat -n apps/backend/src/modules/logistics/logistics.module.tsRepository: onerandomdevv/Swifta
Length of output: 1281
Add LogisticsProcessor to a module's providers or import the module containing it.
LogisticsProcessor is defined with @Processor(LOGISTICS_QUEUE) but is never provided in any active module. While BullModule.registerQueue({ name: LOGISTICS_QUEUE }) enables producer injection, NestJS won't instantiate the processor without it being declared in a module's providers array. This means queued book-pickup jobs won't be consumed at runtime.
Import LogisticsProcessor in queue.module.ts (or whichever module manages processors) and add it to the providers array, or ensure this module is imported where the queue is used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/modules/payment/payment.module.ts` at line 21, The
LogisticsProcessor decorated with `@Processor`(LOGISTICS_QUEUE) is not being
provided, so Nest won't instantiate it and queued jobs won't be consumed; add
LogisticsProcessor to a module's providers (or import the module that provides
it) and ensure the module includes BullModule.registerQueue({ name:
LOGISTICS_QUEUE }) so the processor is instantiated; specifically locate the
module where BullModule.registerQueue({ name: LOGISTICS_QUEUE }) is called
(e.g., payment.module or queue.module) and add LogisticsProcessor to that
module's providers array or import the module that already provides
LogisticsProcessor.
Comment on lines
+97
to
+105
| // Runtime file-type validation (accept attribute alone is insufficient) | ||
| const ext = file.name.toLowerCase().slice(file.name.lastIndexOf(".")); | ||
| const mimeValid = ALLOWED_MIME_TYPES.includes(file.type); | ||
| const extValid = ALLOWED_EXTENSIONS.includes(ext); | ||
| if (!mimeValid && !extValid) { | ||
| return toast.error( | ||
| "Unsupported file type. Please upload a JPG, PNG, or PDF.", | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
File-type check is too permissive (&& allows spoofed files).
Current logic accepts files when either MIME or extension passes. A renamed file with .pdf can bypass this check. Require extension validity and, when MIME is present, require MIME validity too.
🔒 Proposed fix
- const mimeValid = ALLOWED_MIME_TYPES.includes(file.type);
- const extValid = ALLOWED_EXTENSIONS.includes(ext);
- if (!mimeValid && !extValid) {
+ const mimeValid = ALLOWED_MIME_TYPES.includes(file.type);
+ const extValid = ALLOWED_EXTENSIONS.includes(ext);
+ const hasMime = Boolean(file.type);
+ if (!extValid || (hasMime && !mimeValid)) {
return toast.error(
"Unsupported file type. Please upload a JPG, PNG, or PDF.",
);
}📝 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.
Suggested change
| // Runtime file-type validation (accept attribute alone is insufficient) | |
| const ext = file.name.toLowerCase().slice(file.name.lastIndexOf(".")); | |
| const mimeValid = ALLOWED_MIME_TYPES.includes(file.type); | |
| const extValid = ALLOWED_EXTENSIONS.includes(ext); | |
| if (!mimeValid && !extValid) { | |
| return toast.error( | |
| "Unsupported file type. Please upload a JPG, PNG, or PDF.", | |
| ); | |
| } | |
| // Runtime file-type validation (accept attribute alone is insufficient) | |
| const ext = file.name.toLowerCase().slice(file.name.lastIndexOf(".")); | |
| const mimeValid = ALLOWED_MIME_TYPES.includes(file.type); | |
| const extValid = ALLOWED_EXTENSIONS.includes(ext); | |
| const hasMime = Boolean(file.type); | |
| if (!extValid || (hasMime && !mimeValid)) { | |
| return toast.error( | |
| "Unsupported file type. Please upload a JPG, PNG, or PDF.", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/`(dashboard)/merchant/verification/page.tsx around lines 97
- 105, The file-type check currently allows files if either MIME or extension is
valid; change it so the extension must always be valid and the MIME must be
valid when present: require extValid to be true and, if file.type is non-empty,
require mimeValid as well (use ALLOWED_EXTENSIONS and ALLOWED_MIME_TYPES to
validate), otherwise show the same toast.error message; update the conditional
around file.name/file.type where ext, mimeValid, and extValid are computed to
enforce extValid && (file.type ? mimeValid : true).
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
1 participant
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.
…ty & a11y improvements
Summary by CodeRabbit
New Features
Improvements