feat(finance): add provider preference, disbursements, and build/test hardening#420
Conversation
… hardening Add organization-level fiat provider preference and wire it through company profile updates and fiat provider selection. Implement fiat disbursement API flow with validation and provider-aware service logic. Add/adjust tests for company profile, disbursement behavior, health checks, register payload validation, JWT expiry handling, and idempotency mocks. Harden runtime/build behavior by making DB initialization lazy, adding ioredis dependency, and wrapping invite accept search-params usage in Suspense. Improve API error backward compatibility and resilience in rate-limit/idempotency/test DB utilities.
There was a problem hiding this comment.
Pull request overview
Implements organization-level fiat provider preference (monnify vs flutterwave) and introduces a provider-aware fiat disbursement flow, alongside several build/runtime hardening changes and accompanying tests.
Changes:
- Add
provider_preferenceto organizations (Drizzle schema + SQL migration) and expose/update it via company profile API/service. - Introduce fiat disbursements (validation schema, service, and
POST /api/v1/finance/disbursementsroute) with provider selection. - Hardening updates across API error compatibility, DB initialization, idempotency cache behavior, and test stability.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/validations/finance.schema.ts | Adds request validation for creating disbursements. |
| src/server/validations/company.schema.ts | Adds validation for updating company profile, including provider preference. |
| src/server/utils/transaction-idempotency.ts | Adjusts idempotency DB backend to lazy-load schema. |
| src/server/utils/transaction-idempotency.spec.ts | Updates mocks to match idempotency cache schema usage. |
| src/server/utils/errors.ts | Extends AppError with a status field for compatibility. |
| src/server/utils/api-response.ts | Returns RFC7807 bodies with additional legacy fields (success, message). |
| src/server/test/db-utils.ts | Improves transactional mock DB defaults for emailVerifications. |
| src/server/services/team.service.ts | Implements expenses retrieval instead of returning an empty list. |
| src/server/services/rate-limit.service.ts | Makes rate-limit identifier extraction more defensive. |
| src/server/services/organization.service.ts | Extends Organization interface with providerPreference. |
| src/server/services/jwt.service.ts | Normalizes ms expirations for jose token creation. |
| src/server/services/invitation.service.spec.ts | Skips DB-dependent tests when TEST_DATABASE_URL is not set. |
| src/server/services/finance-wallet.service.spec.ts | Avoids throwing when DB tests are skipped; keeps safety check for non-test DBs. |
| src/server/services/fiat/index.ts | Adds provider factory helpers and organization-based provider resolution. |
| src/server/services/fiat-disbursement.service.ts | Adds disbursement orchestration and transaction persistence. |
| src/server/services/fiat-disbursement.service.spec.ts | Adds unit coverage for provider selection and persistence calls. |
| src/server/services/company.service.ts | Adds provider preference to profile and supports persisted updates via DB. |
| src/server/services/company.service.spec.ts | Adds unit tests for provider preference read/update behavior. |
| src/server/db/schema.ts | Adds fiat_provider enum and providerPreference column to organizations. |
| src/server/db/index.ts | Introduces lazy DB initialization and test DB URL resolution changes. |
| src/app/invite/accept/page.tsx | Wraps search-params usage in Suspense for Next.js build compatibility. |
| src/app/api/v1/health/route.test.ts | Updates health endpoint tests to reflect degraded/unhealthy behavior and DB ping mocking. |
| src/app/api/v1/finance/disbursements/route.ts | Adds disbursement API endpoint with auth + validation. |
| src/app/api/v1/company/profile/route.ts | Adds PUT handler to update company profile/provider preference with validation. |
| src/app/api/v1/auth/register/route.test.ts | Updates register tests to include required fields (password/agreement). |
| package.json | Adds ioredis dependency. |
| pnpm-lock.yaml | Locks ioredis and transitive dependencies. |
| next.config.ts | Marks ioredis as a server external package. |
| drizzle/migrations/0012_add_provider_preference_to_organizations.sql | Creates enum + adds provider_preference column with default. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| export const fiatProviderEnum = pgEnum("fiat_provider", [ | ||
| "monnify", | ||
| "flutterwave", | ||
| ]); |
There was a problem hiding this comment.
fiatProviderEnum is introduced, but fiatTransactions.provider remains a free-form varchar, which allows values outside the supported provider set. Consider switching the fiat_transactions.provider column to use fiatProviderEnum (with a migration) to keep provider values consistent across organization preference and transaction records.
| export async function POST(req: NextRequest) { | ||
| try { | ||
| const { userId } = await AuthUtils.authenticateRequestOrRefreshCookie(req); | ||
| const payload = await req.json(); |
There was a problem hiding this comment.
req.json() can throw a SyntaxError on malformed JSON, which currently falls through to the generic catch and returns a 500. Other routes in the codebase return 400 for malformed JSON; handle SyntaxError around req.json() and respond with a 400 "Malformed JSON" error to avoid misclassifying client errors as server errors.
| const payload = await req.json(); | |
| let payload: unknown; | |
| try { | |
| payload = await req.json(); | |
| } catch (error) { | |
| if (error instanceof SyntaxError) { | |
| return ApiResponse.error("Malformed JSON", 400); | |
| } | |
| throw error; | |
| } |
| const payload = await req.json(); | ||
|
|
There was a problem hiding this comment.
req.json() can throw on malformed JSON, but the PUT handler currently treats this as an internal error and returns 500. Wrap JSON parsing and return a 400 response for malformed JSON (consistent with other API routes like auth/forgot-password).
| const payload = await req.json(); | |
| let payload: unknown; | |
| try { | |
| payload = await req.json(); | |
| } catch (parseError) { | |
| // Handle malformed JSON bodies as a client error (400) | |
| console.error("[Update Company Profile Error] Malformed JSON body", parseError); | |
| return ApiResponse.error("Invalid request body", 400); | |
| } |
| const disbursement = await provider.disburse({ | ||
| amount: input.amount, | ||
| reference, | ||
| narration: input.narration, | ||
| destinationBankCode: input.destinationBankCode, | ||
| destinationAccountNumber: input.destinationAccountNumber, | ||
| destinationAccountName: input.destinationAccountName, | ||
| currency: input.currency, | ||
| }); | ||
|
|
||
| await db.insert(fiatTransactions).values({ | ||
| organizationId: organization.id, | ||
| amount: BigInt(input.amount), | ||
| type: "payout", | ||
| status: disbursement.status, | ||
| provider: providerPreference, | ||
| providerReference: disbursement.providerReference, | ||
| metadata: { | ||
| reference: disbursement.reference, | ||
| fee: disbursement.fee, | ||
| narration: input.narration, | ||
| }, |
There was a problem hiding this comment.
The service calls the external provider to initiate a payout before writing any record to fiat_transactions. If the subsequent DB insert fails, the payout may have been initiated without any persisted audit trail/reference in your system. Consider inserting a pending transaction first (or using a DB transaction/outbox pattern) and then updating it after the provider call, so failures remain traceable and retryable.
| const disbursement = await provider.disburse({ | |
| amount: input.amount, | |
| reference, | |
| narration: input.narration, | |
| destinationBankCode: input.destinationBankCode, | |
| destinationAccountNumber: input.destinationAccountNumber, | |
| destinationAccountName: input.destinationAccountName, | |
| currency: input.currency, | |
| }); | |
| await db.insert(fiatTransactions).values({ | |
| organizationId: organization.id, | |
| amount: BigInt(input.amount), | |
| type: "payout", | |
| status: disbursement.status, | |
| provider: providerPreference, | |
| providerReference: disbursement.providerReference, | |
| metadata: { | |
| reference: disbursement.reference, | |
| fee: disbursement.fee, | |
| narration: input.narration, | |
| }, | |
| const disbursement = await db.transaction(async (tx) => { | |
| // Insert a pending transaction record before initiating the external payout | |
| const [createdTransaction] = await tx | |
| .insert(fiatTransactions) | |
| .values({ | |
| organizationId: organization.id, | |
| amount: BigInt(input.amount), | |
| type: "payout", | |
| status: "pending", | |
| provider: providerPreference, | |
| providerReference: null, | |
| metadata: { | |
| reference, | |
| narration: input.narration, | |
| }, | |
| }) | |
| .returning({ id: fiatTransactions.id }); | |
| const payout = await provider.disburse({ | |
| amount: input.amount, | |
| reference, | |
| narration: input.narration, | |
| destinationBankCode: input.destinationBankCode, | |
| destinationAccountNumber: input.destinationAccountNumber, | |
| destinationAccountName: input.destinationAccountName, | |
| currency: input.currency, | |
| }); | |
| await tx | |
| .update(fiatTransactions) | |
| .set({ | |
| status: payout.status, | |
| provider: providerPreference, | |
| providerReference: payout.providerReference, | |
| metadata: { | |
| reference: payout.reference, | |
| fee: payout.fee, | |
| narration: input.narration, | |
| }, | |
| }) | |
| .where(eq(fiatTransactions.id, createdTransaction.id)); | |
| return payout; |
| name: data.name ?? org.name, | ||
| industry: data.industry ?? org.industry, | ||
| registrationNumber: data.registrationNumber ?? org.registrationNumber, | ||
| providerPreference: data.providerPreference ?? org.providerPreference, | ||
| registeredStreet: data.registered?.street ?? org.registeredStreet, | ||
| registeredCity: data.registered?.city ?? org.registeredCity, | ||
| registeredState: data.registered?.state ?? org.registeredState, | ||
| registeredPostalCode: | ||
| data.registered?.postalCode ?? org.registeredPostalCode, | ||
| registeredCountry: data.registered?.country ?? org.registeredCountry, | ||
| billingStreet: data.billing?.street ?? org.billingStreet, | ||
| billingCity: data.billing?.city ?? org.billingCity, | ||
| billingState: data.billing?.state ?? org.billingState, | ||
| billingPostalCode: data.billing?.postalCode ?? org.billingPostalCode, | ||
| billingCountry: data.billing?.country ?? org.billingCountry, |
There was a problem hiding this comment.
updateCompanyProfile uses nullish coalescing (??) when applying updates, which prevents callers from explicitly setting nullable fields to null (e.g., industry: null will be ignored and the old value kept). Use undefined checks instead so null is treated as a deliberate update while undefined means “no change” (apply this to top-level and nested address fields).
| name: data.name ?? org.name, | |
| industry: data.industry ?? org.industry, | |
| registrationNumber: data.registrationNumber ?? org.registrationNumber, | |
| providerPreference: data.providerPreference ?? org.providerPreference, | |
| registeredStreet: data.registered?.street ?? org.registeredStreet, | |
| registeredCity: data.registered?.city ?? org.registeredCity, | |
| registeredState: data.registered?.state ?? org.registeredState, | |
| registeredPostalCode: | |
| data.registered?.postalCode ?? org.registeredPostalCode, | |
| registeredCountry: data.registered?.country ?? org.registeredCountry, | |
| billingStreet: data.billing?.street ?? org.billingStreet, | |
| billingCity: data.billing?.city ?? org.billingCity, | |
| billingState: data.billing?.state ?? org.billingState, | |
| billingPostalCode: data.billing?.postalCode ?? org.billingPostalCode, | |
| billingCountry: data.billing?.country ?? org.billingCountry, | |
| name: data.name !== undefined ? data.name : org.name, | |
| industry: data.industry !== undefined ? data.industry : org.industry, | |
| registrationNumber: | |
| data.registrationNumber !== undefined | |
| ? data.registrationNumber | |
| : org.registrationNumber, | |
| providerPreference: | |
| data.providerPreference !== undefined | |
| ? data.providerPreference | |
| : org.providerPreference, | |
| registeredStreet: | |
| data.registered && data.registered.street !== undefined | |
| ? data.registered.street | |
| : org.registeredStreet, | |
| registeredCity: | |
| data.registered && data.registered.city !== undefined | |
| ? data.registered.city | |
| : org.registeredCity, | |
| registeredState: | |
| data.registered && data.registered.state !== undefined | |
| ? data.registered.state | |
| : org.registeredState, | |
| registeredPostalCode: | |
| data.registered && data.registered.postalCode !== undefined | |
| ? data.registered.postalCode | |
| : org.registeredPostalCode, | |
| registeredCountry: | |
| data.registered && data.registered.country !== undefined | |
| ? data.registered.country | |
| : org.registeredCountry, | |
| billingStreet: | |
| data.billing && data.billing.street !== undefined | |
| ? data.billing.street | |
| : org.billingStreet, | |
| billingCity: | |
| data.billing && data.billing.city !== undefined | |
| ? data.billing.city | |
| : org.billingCity, | |
| billingState: | |
| data.billing && data.billing.state !== undefined | |
| ? data.billing.state | |
| : org.billingState, | |
| billingPostalCode: | |
| data.billing && data.billing.postalCode !== undefined | |
| ? data.billing.postalCode | |
| : org.billingPostalCode, | |
| billingCountry: | |
| data.billing && data.billing.country !== undefined | |
| ? data.billing.country | |
| : org.billingCountry, |
|
|
||
| export type FiatProviderPreference = "monnify" | "flutterwave"; | ||
|
|
There was a problem hiding this comment.
FiatProviderPreference is redefined here while a same-named union type also exists in src/server/services/fiat/index.ts, and organization.service.ts has another inline union. This duplication can drift; prefer exporting a single canonical provider-preference type (e.g., from db/schema via the enum column type or from the fiat service) and reusing it across services and validations.
| export type FiatProviderPreference = "monnify" | "flutterwave"; | |
| import type { FiatProviderPreference } from "./fiat"; |
| let transactionCache: any; | ||
| try { | ||
| const schema = (await import("../db/schema")) as Record<string, any>; | ||
| transactionCache = schema.transactionCache; | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The DB backend does a dynamic import("../db/schema") on every has()/set() call and silently disables idempotency when the import fails. Consider caching the resolved transactionCache table reference in a static field (lazy init once) and logging/raising on unexpected import failures so idempotency isn’t inadvertently bypassed without visibility.
| if (process.env.NODE_ENV === "test") { | ||
| return ( | ||
| process.env.TEST_DATABASE_URL || | ||
| process.env.DATABASE_URL || | ||
| "postgres://postgres:postgres@127.0.0.1:5432/vestroll_test" | ||
| ); |
There was a problem hiding this comment.
In test mode, resolveDatabaseUrl() falls back to a hard-coded local connection string when neither TEST_DATABASE_URL nor DATABASE_URL is set. This can cause tests (or any code running with NODE_ENV=test) to unexpectedly connect to a developer machine’s local Postgres, masking missing env configuration and creating hard-to-debug failures. Prefer requiring an explicit test DB URL (or gating the fallback behind a dedicated opt-in env flag).
| if (process.env.NODE_ENV === "test") { | |
| return ( | |
| process.env.TEST_DATABASE_URL || | |
| process.env.DATABASE_URL || | |
| "postgres://postgres:postgres@127.0.0.1:5432/vestroll_test" | |
| ); | |
| const isTest = process.env.NODE_ENV === "test"; | |
| if (isTest) { | |
| if (process.env.TEST_DATABASE_URL) { | |
| return process.env.TEST_DATABASE_URL; | |
| } | |
| if (process.env.DATABASE_URL) { | |
| return process.env.DATABASE_URL; | |
| } | |
| if (process.env.ALLOW_LOCAL_TEST_DB === "true") { | |
| return "postgres://postgres:postgres@127.0.0.1:5432/vestroll_test"; | |
| } | |
| return undefined; |
codeZe-us
left a comment
codeZe-us
left a comment
There was a problem hiding this comment.
@Oluwaseyi89 this is okay
|
@Oluwaseyi89 please fix conflict in your branch |
|
@codeZe-us conflicts have been resolved. |
Pull Request
Summary
This PR implements the
provider_preferencefeature for organizations, allowing them to choose between 'monnify' and 'flutterwave' as their default fiat gateway. Beyond the core preference logic, this work includes the implementation of a new fiat disbursement service, API route hardening, and comprehensive test coverage across the finance and company domains.Type of Change
Linked Issues
Fixes #348
Key Changes
providerPreferenceto the organizations table via Drizzle migration (0012_add_provider_preference_to_organizations.sql).company.service.tsto support updating organization-level provider preferences.fiat-disbursement.service.tswith provider-aware logic to handle disbursements based on organization settings.POST /api/v1/finance/disbursementsfor handling fiat payouts.Suspensefor Next.js build compatibility.ioredisto dependencies and updatednext.config.ts.company.serviceandfiat-disbursement.service.Checklist
Screenshots / Recordings
N/A
Closes #348