fix(braintree): harden error propagation and document payment provider - #42
Open
SGFGOV wants to merge 8 commits into
Open
fix(braintree): harden error propagation and document payment provider#42SGFGOV wants to merge 8 commits into
SGFGOV wants to merge 8 commits into
Conversation
…into fix/multiple-refunds
…rrors Propagate MedusaError types through authorize/account-holder/webhook paths, preserve sync errors when orphan void fails, and align refund unit tests with the braintreeRefund history array shape. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Braintree provider now uses shared error handling, staged refund processing, normalized refund history, and explicit webhook validation behavior. Tests cover authorization failures, missing transaction IDs, refund history, webhook parsing, and legacy refund data. ChangesBraintree provider behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BraintreeBase
participant BraintreeGateway
participant BraintreeTransaction
participant PaymentSessionData
BraintreeBase->>BraintreeTransaction: load transaction context
BraintreeBase->>BraintreeGateway: settle, void, or refund transaction
BraintreeGateway-->>BraintreeBase: return transaction result
BraintreeBase->>PaymentSessionData: append braintreeRefund history entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 4
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-import.ts (1)
144-157: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDistinguish missing transactions from gateway failures.
The catch block converts all
transaction.findrejections toNOT_FOUND. Network timeouts, TLS failures, and authentication failures against the Braintree API are not missing transactions. Operators then see "Braintree transaction not found" for a transient outage and may treat the import as invalid instead of retrying.The Braintree SDK error object includes a
typeproperty set to"notFoundError"for missing resources. Check this property to apply the correct error type to other failures.♻️ Proposed change
if (MedusaError.isMedusaError(error)) throw error; + const isNotFound = error instanceof Error && (error as any).type === 'notFoundError'; throw new MedusaError( - MedusaError.Types.NOT_FOUND, - `Braintree transaction not found: ${session.transactionId}`, + isNotFound ? MedusaError.Types.NOT_FOUND : MedusaError.Types.UNEXPECTED_STATE, + isNotFound + ? `Braintree transaction not found: ${session.transactionId}` + : `Failed to look up Braintree transaction ${session.transactionId}`, );Verify the exact error object structure returned by the transaction.find method for the SDK version in use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts` around lines 144 - 157, Update the catch block in the transaction.find handling within initiatePayment so only Braintree errors whose type is exactly "notFoundError" are converted to MedusaError.Types.NOT_FOUND. Preserve existing MedusaError propagation, and rethrow other SDK failures—including network, TLS, and authentication errors—without relabeling them as missing transactions; verify the actual transaction.find error shape for the configured SDK version.
🧹 Nitpick comments (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.ts (1)
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for history accumulation.
Each assertion reads only the latest entry. No test seeds an existing
braintreeRefundarray ininput.data. The append behavior inbuildRefundPaymentOutputis the main change in this shape, and it is untested.Add a case that seeds one prior entry, runs a refund, and asserts the resulting array has two entries in order.
Also applies to: 282-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.ts` around lines 231 - 233, Extend the tests around buildRefundPaymentOutput to seed input.data with one existing braintreeRefund entry, execute the refund flow, and assert the resulting array contains both entries in their original order followed by the new refund entry. Keep the existing latest-entry assertions and add coverage for accumulation rather than replacement.plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts (1)
644-654: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce duplicate error logging in the authorize catch block.
The catch block logs twice.
rethrowGatewayErrorthen callslogErrorDetailagain, andbuildBraintreeErrorlogs a third time for non-Medusa errors. A single authorize failure produces up to four log records with the same cause.Consider removing the local
logErrorDetailcall and keeping only the contextuallogger.error, becauserethrowGatewayErroralready receives the same context.♻️ Proposed change
} catch (error) { - this.logErrorDetail('authorizePayment', error, { - amount: (input.data as { amount?: number })?.amount, - currency_code: (input.data as { currency_code?: string })?.currency_code, - }); this.logger.error(`Error authorizing transaction: ${(error as Error).message}`, error as Error); this.rethrowGatewayError(error, 'authorize payment', {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts` around lines 644 - 654, Remove the local logErrorDetail call from the authorizePayment catch block, leaving the contextual logger.error and rethrowGatewayError invocation intact; rethrowGatewayError already receives the same authorization context and handles the downstream error logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts`:
- Around line 180-192: Update the JSDoc for validateString so its `@returns`
description accurately states that the function returns the original validated
string without trimming it; leave the implementation unchanged.
- Around line 1455-1456: Update the sessionId extraction in the transaction
notification handler to safely handle paymentData.customFields being absent,
returning an empty session ID when no custom fields exist while preserving the
existing value when present. Ensure transaction_settled and
transaction_settlement_declined notifications for unrelated transactions do not
throw during this lookup.
- Around line 938-942: Update retrieveOrVoidSale to validate the captured
transactionId before calling retrieveTransaction; when it is missing, fail with
an accurate missing-sale-transaction error rather than dereferencing
saleResponse.transaction. Preserve the existing retrieval flow for valid
transaction IDs.
- Around line 1104-1118: Update buildRefundPaymentOutput to validate
input.data?.braintreeRefund with Array.isArray() before assigning prior, falling
back to an empty array for legacy non-array values so [...prior, entry] remains
iterable.
---
Outside diff comments:
In
`@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts`:
- Around line 144-157: Update the catch block in the transaction.find handling
within initiatePayment so only Braintree errors whose type is exactly
"notFoundError" are converted to MedusaError.Types.NOT_FOUND. Preserve existing
MedusaError propagation, and rethrow other SDK failures—including network, TLS,
and authentication errors—without relabeling them as missing transactions;
verify the actual transaction.find error shape for the configured SDK version.
---
Nitpick comments:
In
`@plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.ts`:
- Around line 231-233: Extend the tests around buildRefundPaymentOutput to seed
input.data with one existing braintreeRefund entry, execute the refund flow, and
assert the resulting array contains both entries in their original order
followed by the new refund entry. Keep the existing latest-entry assertions and
add coverage for accumulation rather than replacement.
In
`@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts`:
- Around line 644-654: Remove the local logErrorDetail call from the
authorizePayment catch block, leaving the contextual logger.error and
rethrowGatewayError invocation intact; rethrowGatewayError already receives the
same authorization context and handles the downstream error logging.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8a8cbb3-f0f2-49a0-af2f-5f0f5cd0af99
📒 Files selected for processing (4)
.gitignoreplugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.tsplugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.tsplugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-import.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
The main merge left half-merged refundPayment logic inside resolveRefundAction/executeRefundAction and broke the class syntax. Co-authored-by: Cursor <cursoragent@cursor.com>
Guard missing sale transaction ids, legacy non-array braintreeRefund history, and absent webhook customFields; clarify validateString JSDoc. Co-authored-by: Cursor <cursoragent@cursor.com>
![currybot-lc[bot]](https://avatars.githubusercontent.com/in/2831021?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
- Ticket: No ticket referenced — hardens Braintree error propagation, refund history, orphan-sale cleanup, webhook validation, and provider documentation
- Related: Root and Braintree provider guidance consulted; no applicable RAG or meeting decisions found
- DevAgent plan: N/A
- Scope:
⚠️ issues found
🔍 Gap Analysis
- Authorization failures now preserve
PAYMENT_AUTHORIZATION_ERROR, webhook parse failures propagate, account-holder methods preserve typed errors, and refund tests reflect thedata.braintreeRefundhistory-array contract. - The orphan-sale cleanup preserves the original synchronization error when
transaction.voidrejects, but does not handle Braintree’s normal resolved failure-result shape. - The added tests cover authorization typing and webhook parse propagation, but omit the stated orphan-void error-precedence case and do not exercise a resolved
{ success: false }cleanup response.
🧪 Code Quality
- Medium — A failed orphan-sale void can still be silently discarded. At
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts:954, the cleanup awaitstransaction.voidbut never checkssuccessor declined status. Braintree commonly reports gateway failures as resolved result objects, so{ success: false, ... }bypasses the catch, produces no failure log, and is indistinguishable from successful cleanup. Validate the result with the existing failure classifier while logging—without throwing over the original synchronization error.
🔧 Simplicity Audit
- No framework reimplementation, unjustified indirection, or disproportionate complexity found. The extracted authorization, refund, and webhook helpers each isolate a distinct lifecycle responsibility and include point-of-use rationale.
- Simplicity deduction: -0.0.
🌟 Highlights
- Authorization validation and processor declines now retain the Medusa error type required by cart completion.
- Refund handling cleanly separates status resolution, gateway execution, and history construction while preserving the original transaction.
- Webhook signature failures are no longer converted into ordinary payment actions, and the new test verifies propagation.
📊 Merge Confidence: 8.6/10
- ✅ The main error-typing, webhook, refund-history, and documentation goals are implemented coherently.
⚠️ +0.5 for inspecting and logging resolved unsuccessful orphan-void results while preserving the original sync error.⚠️ +0.3 for tests covering both rejected and resolved-unsuccessful orphan cleanup, including error precedence.- 🔧 Simplicity deductions applied: -0.0 total.
Log resolved unsuccessful orphan-void Results without overriding the sync error, drop duplicate authorize logging, and cover refund history accumulation plus orphan-void failure precedence. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@currybot-lc please review again, all your observations have been addressed |
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
- Ticket: No ticket referenced — document Braintree lifecycle behavior, preserve typed Medusa errors, propagate webhook failures, maintain refund history, and safely handle orphan-sale cleanup failures.
- Related: Repository guidance requires Medusa ≥2.5, Node ≥20, and Yarn 4.6; Braintree README documents refund routing and debug logging. No relevant ticket, RAG, meeting, root review, agent, or cursor guidance was found.
- DevAgent plan: N/A
- Scope: ✅ matches requirements
🔍 Gap Analysis
- Prior finding 1 — resolved-unsuccessful orphan cleanup: ✅ resolved.
retrieveOrVoidSalenow inspects the resolved void result withisBraintreeFailureResponse, records failure details, and logs the unsuccessful cleanup without replacing the synchronization error (plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts:950). - Prior finding 2 — missing orphan-cleanup coverage: ✅ resolved. Tests now cover both a rejected void and a resolved
{ success: false }response, assert the cleanup attempt and failure log, and verify that the original synchronization failure remains the surfaced error (plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.ts:638,:667). - Refund-history coverage now verifies that a new refund is appended to the existing
data.braintreeRefundarray without discarding its prior entry (plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.ts:236). - Documentation, typed-error preservation in authorize/account-holder paths, webhook parse propagation, and the
references/ignore rule: No change since last review; they remain aligned with the stated requirements. - No scope deviations or missing requirement-level pieces found.
🧪 Code Quality
- No new correctness, typing, security, lifecycle, or coverage defects found in the commits since the prior review.
- The cleanup path correctly distinguishes thrown failures from resolved unsuccessful Braintree results and preserves original-error precedence.
- GitHub exposes no authoritative test check. The PR reports 30 targeted tests passing, but the three manual integration checks remain unchecked; this review therefore does not treat CI as passing and did not rerun tests locally.
🔧 Simplicity Audit
- No
[VIOLATION]or[MISSING JUSTIFICATION]findings. The added result inspection reuses existing Braintree response helpers, and the two explicit failure-mode tests are proportional to the lifecycle risk. - Total simplicity deduction: -0
🌟 Highlights
- Reuses
isBraintreeFailureResponseandgetBraintreeErrorMessageinstead of introducing a second cleanup-error interpretation path. - Keeps orphan cleanup best-effort while reliably surfacing the original payment-session synchronization error.
- Adds focused regression coverage for both promise rejection and Braintree’s resolved-failure result contract.
📊 Merge Confidence: 8.9/10
- ✅ What's solid: Both prior findings are resolved, the implementation meets the stated requirements, tests directly exercise original-error precedence, and no new code-quality or simplicity issues were found.
⚠️ What would raise the score: +0.1 for an authoritative targeted test check; +0.2 for completing the outstanding manual Braintree integration checks.- 🔧 Simplicity deductions applied: -0 total
Contributor
There was a problem hiding this comment.
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)
945-968: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve typed synchronization errors after orphan cleanup.
If
retrieveTransaction(transactionId)throws aMedusaError, line 968 replaces it with a new error frombuildBraintreeError. This loses the original error type and code after the void attempt completes.Re-throw an existing
MedusaErrorbefore wrapping unknown errors.Proposed fix
} catch (voidError) { this.logErrorDetail('void orphan sale after sync failure', voidError, { transactionId }); this.logger.error( `Failed to void orphan Braintree transaction ${transactionId} after sync failure`, voidError instanceof Error ? voidError : undefined, ); } + if (MedusaError.isMedusaError(error)) throw error; throw buildBraintreeError(error, 'sync payment session', this.logger, { transactionId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts` around lines 945 - 968, Update the catch block around retrieveTransaction in the synchronization flow to rethrow the original error after the orphan-void cleanup when it is already a MedusaError; only pass unknown errors to buildBraintreeError. Preserve the existing cleanup behavior and use the existing MedusaError type or detection utility available in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts`:
- Around line 945-968: Update the catch block around retrieveTransaction in the
synchronization flow to rethrow the original error after the orphan-void cleanup
when it is already a MedusaError; only pass unknown errors to
buildBraintreeError. Preserve the existing cleanup behavior and use the existing
MedusaError type or detection utility available in this module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c97154ce-9cf0-4d09-b4f3-a1d4de8a5a37
📒 Files selected for processing (2)
plugins/braintree-payment/src/providers/payment-braintree/src/core/__tests__/braintree-base.spec.tsplugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Contributor
Author
|
@currybot-lc the authorative test is already on the 360trainng repo as part of the full checkout flows |
|
Got it—thanks. I’ll treat the full checkout flows in the 360Training repo as the authoritative integration coverage for this provider. My review note was specifically that GitHub doesn’t expose a linked authoritative check on this PR, not that the coverage is absent. If those flows are green for this change, that closes the remaining test-confidence caveat. |
|
🍛 |
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
2 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
braintree-base.ts: webhook parse failures now throw; authorize/account-holder paths preserve typedMedusaErrorviarethrowGatewayError/MedusaError.isMedusaError; orphan-sale void failures no longer replace the original sync error.braintreeRefundhistory-array shape and add coverage for webhook parse propagation.references/clones in.gitignore.Test plan
npm test -- --testPathPattern='braintree-base|braintree-import'inplugins/braintree-payment(30 passed)PAYMENT_AUTHORIZATION_ERRORreaches the cart completion flowFAILED)data.braintreeRefundhistory entries (type+transaction)Summary by CodeRabbit
references/from version control.