fix: braintree refund #26
Conversation
WalkthroughBumps the braintree-payment package version and refactors the Braintree provider and import flows: adds input validators and centralized error helper, standardizes transaction naming and numeric formatting, strengthens null/undefined guards, reworks refund/void logic using MathBN, adds a public logger to BraintreeImport, and adds a two-decimal formatting utility. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant BraintreeImport
participant BraintreeBase
participant BraintreeSDK as Braintree SDK
participant Logger
rect rgb(240,248,255)
note over Client,BraintreeSDK: Refund/void flow (status-based)
Client->>BraintreeImport: refundPayment(session, amount, context)
BraintreeImport->>BraintreeBase: validate inputs (validateString/MathBN)
BraintreeBase-->>BraintreeImport: ok or throws (buildBraintreeError)
BraintreeImport->>Logger: log start
BraintreeImport->>BraintreeSDK: get transaction by id
BraintreeSDK-->>BraintreeImport: transaction(status,id)
alt shouldVoid (authorized/submitted_for_settlement)
BraintreeImport->>BraintreeSDK: void(transaction.id)
BraintreeSDK-->>BraintreeImport: voidResult(success, transaction)
alt success
BraintreeImport-->>Client: { transaction, action: "void" }
else error
BraintreeImport->>Logger: log error
BraintreeImport->>BraintreeBase: buildBraintreeError("void", ...)
BraintreeBase-->>Client: throws
end
else shouldRefund (settled/settling)
BraintreeImport->>BraintreeBase: validate amount (MathBN + formatToTwoDecimalString)
BraintreeImport->>BraintreeSDK: refund(transaction.id, amount)
BraintreeSDK-->>BraintreeImport: refundResult(success, transaction)
alt success
BraintreeImport-->>Client: { transaction, action: "refund" }
else error
BraintreeImport->>BraintreeBase: buildBraintreeError("refund", ...)
BraintreeBase-->>Client: throws
end
else unsupported
BraintreeImport->>BraintreeBase: buildBraintreeError("refund/void unsupported", ...)
BraintreeBase-->>Client: throws
end
end
sequenceDiagram
autonumber
participant Caller
participant BraintreeBase
participant BraintreeSDK as Braintree SDK
participant Logger
rect rgb(245,245,255)
note over Caller,BraintreeSDK: Standardized operations with validation + centralized errors
Caller->>BraintreeBase: initiate/authorize/capture/cancel/delete/refund/webhook
BraintreeBase->>BraintreeBase: validateString/validateOptionalString
BraintreeBase->>BraintreeSDK: perform operation
BraintreeSDK-->>BraintreeBase: result or error
alt success
BraintreeBase-->>Caller: normalized { transaction, ... }
else error
BraintreeBase->>Logger: log error via buildBraintreeError
BraintreeBase-->>Caller: throws standardized MedusaError (INVALID_DATA / NOT_FOUND / etc.)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Comment |
| const refundAmountBN = MathBN.convert(input.amount, 2); | ||
| const refundAmount = refundAmountBN.toNumber(); |
There was a problem hiding this comment.
@SGFGOV this was the issue. Medusa was sending in a weird BigNumberJs format that braintree couldn't understand.
….0.13-beta.10 and adjust process order error handling
…-beta.11 and adjust LMS error handling
|
@CodeRabbit please review |
![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 (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (2)
383-409: Fix missing-id guard and check void success result in cancelPayment.Calling find/void with "undefined" id and not checking .success can misreport success.
- const sessionData = await this.parsePaymentSessionData(input.data ?? {}); - const transaction = await this.retrieveTransaction(sessionData.transaction?.id as string); - - if (!transaction) return {}; + const sessionData = await this.parsePaymentSessionData(input.data ?? {}); + const txnId = sessionData.transaction?.id; + if (!txnId) { + throw new MedusaError(MedusaError.Types.INVALID_ARGUMENT, 'Braintree transaction id is required'); + } + const transaction = await this.retrieveTransaction(txnId); + + if (!transaction) return {}; @@ - const updatedTransaction = await this.gateway.transaction.void(transaction.id as string); - - if (updatedTransaction) { - const updated = await this.retrieveTransaction(transaction.id); + const voidResult = await this.gateway.transaction.void(transaction.id as string); + if (!voidResult.success) { + throw new MedusaError( + MedusaError.Types.PAYMENT_AUTHORIZATION_ERROR, + `Failed to void transaction ${transaction.id}: ${voidResult.message}` + ); + } + const updated = await this.retrieveTransaction(transaction.id); return { data: { ...input.data, transaction: updated, }, }; - } - - throw new MedusaError(MedusaError.Types.NOT_FOUND, `No payments found for transaction ${transaction.id}`); +
81-88: Avoid unsafe cast; make transaction optional in session data.Setting transaction: transaction as Transaction inserts undefined and violates the interface.
export interface BraintreePaymentSessionData { client_token: string; - transaction: Transaction; + transaction?: Transaction; amount: number; currency_code: string; payment_method_nonce?: string; account_holder?: PaymentAccountHolderDTO; }And only include transaction when present:
- const dataToSave: BraintreePaymentSessionData = { - transaction: transaction as Transaction, + const dataToSave: BraintreePaymentSessionData = { client_token: token, payment_method_nonce: data?.payment_method_nonce as string, amount: Number(input.amount), currency_code: input.currency_code, account_holder: input.context?.account_holder, }; + if (transaction) { + (dataToSave as BraintreePaymentSessionData).transaction = transaction; + }Also applies to: 578-586
🧹 Nitpick comments (7)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (5)
214-228: Harden amount formatting against NaN/invalid inputs.padEnd on parts[1] will throw if toFixed yields "NaN". Guard for finiteness early.
Apply:
- private formatToTwoDecimalString(amount: number|string): string { - if (typeof amount !== 'string') { - amount = amount.toString(); - } - - const num = Number.parseFloat(amount); - const rounded = Math.round(num * 100) / 100; - - // Use toFixed but handle any precision issues - const fixed = rounded.toFixed(2); - - // Ensure it's exactly 2 decimal places - const parts = fixed.split('.'); - return `${parts[0]}.${parts[1].padEnd(2, '0').substring(0, 2)}`; - } + private formatToTwoDecimalString(amount: number | string): string { + const num = typeof amount === 'string' ? Number.parseFloat(amount) : amount; + if (!Number.isFinite(num)) { + throw new MedusaError(MedusaError.Types.INVALID_DATA, 'Amount must be a finite number'); + } + const rounded = Math.round(num * 100) / 100; + return rounded.toFixed(2); + }
169-192: Coerce legacy string amounts to numbers to avoid breaking persisted sessions.z.number() will reject string amounts saved by older versions. Coerce safely.
- amount: z.number(), + amount: z.coerce.number(),Please confirm if any historical sessions may have string amounts; if yes, the above prevents INVALID_ARGUMENT on reads.
376-379: Use centralized error handler and correct operation name.Message says “capture” inside authorizePayment and loses original error context.
- } catch (error) { - this.logger.error(`Error capturing transaction: ${error.message}`, error); - throw new MedusaError(MedusaError.Types.INVALID_DATA, `Failed to capture transaction`); - } + } catch (error) { + handleBraintreeError(error, 'authorize transaction', this.logger); + }
720-724: Currency scale: verify MathBN.convert scale=2 for zero-decimal currencies.JPY, KRW, etc. have 0 decimals. Using 2 may misconvert amounts.
If available, derive scale from sessionData.currency_code and pass it to MathBN.convert. Otherwise, document/guard for zero-decimal currencies.
55-56: Remove unused import.TransactionOrchestrator is imported but unused.
-import { TransactionOrchestrator } from '@medusajs/framework/orchestration';plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (2)
107-110: Deduplicate money formatting with base provider.formatToTwoDecimalString duplicates logic from base; consider extracting a shared util to avoid drift.
166-170: Currency scale: verify MathBN.convert scale=2.Zero‑decimal currencies will be mis-scaled.
If feasible, derive decimal scale from the currency or session context; otherwise validate supported currencies for this provider.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
plugins/braintree-payment/package.json(1 hunks)plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts(15 hunks)plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (1)
BraintreeConstructorArgs(57-60)plugins/braintree-payment/src/providers/payment-braintree/src/types/index.ts (1)
BraintreeOptions(3-12)
🔇 Additional comments (5)
plugins/braintree-payment/package.json (1)
3-3: Version bump looks good.No issues spotted with publishing scripts or engine/peer ranges.
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (1)
96-108: Input guards LGTM.validateString/validateOptionalString are clear and consistent with MedusaError usage.
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (3)
56-63: Logger wiring LGTM.Constructor pulls logger from container; aligns with base provider patterns.
194-207: Void path looks correct and checks .success; good error logging.Consider also accepting 'submitted_for_settlement' and 'settlement_pending' consistently with base.ts for symmetry.
218-236: Refund path correctness.Checks status, formats amount, validates response; aligns with Braintree API constraints.
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Show resolved
Hide resolved
|
@SGFGOV: I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Outdated
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Outdated
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Outdated
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Outdated
Show resolved
Hide resolved
…-beta.13 and adjust processOrder error handling
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (1)
385-412: Replace type assertion with explicit validation.Line 387 uses a type assertion
as stringwhich bypasses type safety. IfsessionData.transaction?.idis undefined, this will passundefinedtoretrieveTransaction, causing a runtime error.Use the
validateStringhelper consistently:Apply this diff:
async cancelPayment(input: CancelPaymentInput): Promise<CancelPaymentOutput> { const sessionData = await this.parsePaymentSessionData(input.data ?? {}); - const transaction = await this.retrieveTransaction(sessionData.transaction?.id as string); + const txId = validateString(sessionData.transaction?.id, 'Braintree transaction ID'); + const transaction = await this.retrieveTransaction(txId); if (!transaction) return {};
♻️ Duplicate comments (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (2)
214-231: Duplicate comment: same as braintree-import.ts lines 107-124.This implementation has the same issues noted for
braintree-import.ts:
- Code duplication between files
- Unnecessary complexity after
toFixed(2)per past review comment at line 227See the review comment on
braintree-import.tslines 107-124 for the recommended fix.
717-795: Add transaction ID guard and remove TEST_FORCE_SETTLED block
- Validate
sessionData.transaction?.id(throwINVALID_ARGUMENTif missing) before callingretrieveTransaction.- Remove the
if (process.env.TEST_FORCE_SETTLED === 'true') { … }block to avoid test-only behavior in production.- (Optional) Add
this.logger.errorbefore throwing on failed void for consistent logging.- let transaction = await this.retrieveTransaction(sessionData.transaction?.id as string); + const txnId = sessionData.transaction?.id; + if (!txnId) { + throw new MedusaError(MedusaError.Types.INVALID_ARGUMENT, 'Braintree transaction id is required to refund'); + } + let transaction = await this.retrieveTransaction(txnId); - if(process.env.TEST_FORCE_SETTLED === 'true') { - shouldVoid = false; - await this.gateway.testing.settle(transaction.id); - transaction = await this.retrieveTransaction(transaction.id); - }
🧹 Nitpick comments (3)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (1)
240-266: Ensure consistent error handling in refund path.The refund path (lines 251-257) should follow the same error handling pattern as the void path. Currently:
- Void path: logs error then throws (lines 224-228)
- Refund path: directly throws without logging (lines 253-257)
Standardize by adding a logger.error call before throwing in the refund failure case, similar to the void path.
Apply this diff:
const refundResponse = await this.gateway.transaction.refund(transaction.id, refundAmountDecimal); -if (!refundResponse.success) +if (!refundResponse.success) { + this.logger.error(`Failed to refund braintree transaction ${transaction.id}: ${refundResponse.message}`); throw new MedusaError( MedusaError.Types.INVALID_DATA, `Failed to create Braintree refund: ${refundResponse.message}`, ); +}plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (2)
297-343: Improve variable naming to avoid shadowing.The use of
_transactionat lines 315 and 330 shadows the outertransactionvariable and adds confusion. Use more descriptive names likeupdatedTransactionorretrievedTransactionto clarify intent.Apply this diff:
const captureResult = await this.gateway.transaction.submitForSettlement(id, toPay); if (captureResult.success) { - const _transaction = await this.retrieveTransaction(transaction.id); + const updatedTransaction = await this.retrieveTransaction(transaction.id); return { data: { ...input.data, - transaction: _transaction, + transaction: updatedTransaction, }, }; }And similarly at lines 330-336.
568-594: Clarify undefined transaction initialization.Lines 571-582 declare
transactionbut never assign it, then cast itas Transactionwhen adding todataToSave. This is confusing because:
- The variable is declared but unused before assignment
- The type assertion suggests it's defined when it's actually undefined
Consider either:
- Remove the declaration and explicitly set
transaction: undefinedin dataToSave- Add a comment explaining why transaction starts as undefined in initiate phase
Apply this diff:
async initiatePayment(input: InitiatePaymentInput): Promise<InitiatePaymentOutput> { const data = this.validateInitiatePaymentData(input.data ?? {}); - let transaction: Transaction | undefined; const token = await this.getValidClientToken(input.context?.customer?.id, input.context?.account_holder); const paymentSessionId = validateString(input.context?.idempotency_key, 'Payment session ID'); if (!token) { throw new MedusaError(MedusaError.Types.INVALID_ARGUMENT, 'Failed to generate client token'); } const dataToSave: BraintreePaymentSessionData = { - transaction: transaction as Transaction, + transaction: undefined as any, // Transaction created during authorization client_token: token,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
plugins/braintree-payment/package.json(1 hunks)plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts(15 hunks)plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/braintree-payment/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (1)
BraintreeConstructorArgs(57-60)plugins/braintree-payment/src/providers/payment-braintree/src/types/index.ts (1)
BraintreeOptions(3-12)
🔇 Additional comments (7)
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts (1)
1-8: LGTM: Logger integration follows framework patterns.The addition of the logger property and its initialization from the container using
ContainerRegistrationKeys.LOGGERis consistent with the framework architecture and aligns with the changes inbraintree-base.ts.Also applies to: 38-38, 56-56, 62-62
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (6)
96-117: LGTM: Centralized error handling improves consistency.The
validateString,validateOptionalString, andbuildBraintreeErrorhelpers are well-designed utilities that improve error handling consistency across the codebase. The error helper properly preserves error context in logging while providing clean error messages to callers.
414-516: LGTM: Method rename improves consistency.Renaming from
getBraintreeTransactionCreateRequestBodytogetTransactionCreateRequestBodyreduces redundancy since the class is alreadyBraintreeBase. The implementation properly handles field length constraints and validates input data.
596-636: LGTM: Proper error handling with cleanup logic.The error handling in
createTransactionproperly:
- Uses validation helpers consistently
- Cleans up failed transactions by voiding them
- Uses
buildBraintreeErrorfor standardized error reportingThis aligns with the improvements suggested in past review comments.
638-664: LGTM: Consistent error handling pattern.The use of
buildBraintreeErrorat line 653 follows the established pattern for standardized error handling. The logic correctly handles both cases (with and without existing transaction).
835-892: LGTM: Consistent error handling in account operations.The use of
buildBraintreeErrorat lines 866 and 889 properly standardizes error handling for account holder operations, addressing the past review comments about using centralized error handling instead of scattered throws.
894-943: LGTM: Webhook handling is clean and concise.The webhook logging at line 897 uses a boolean check (
!!webhookData.data) which is appropriate for logging whether data is present without exposing sensitive information.
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Outdated
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts
Outdated
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts
Show resolved
Hide resolved
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts
Show resolved
Hide resolved
…-beta.14 and refactor amount formatting logic
|
@dwene please review and release new version of plugin then i a can update the medusa core in a tiny PR :) |
Summary by CodeRabbit
Bug Fixes
Refactor
Chores