Skip to content

[MI-1340] fix: refunds and minor refactors for type safety - #41

Open
SGFGOV wants to merge 3 commits into
mainfrom
fix/refunds-2
Open

[MI-1340] fix: refunds and minor refactors for type safety#41
SGFGOV wants to merge 3 commits into
mainfrom
fix/refunds-2

Conversation

@SGFGOV

@SGFGOV SGFGOV commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Refund processing now returns the full refund history (including newly created refund entries) for both “void” and “refund” flows.
  • Bug Fixes
    • Improved Braintree gateway initialization to consistently expose the configured gateway for payment operations.
    • Enhanced runtime option validation to more accurately detect invalid configuration values.
  • Chores
    • Updated the Braintree payment integration version to 0.1.6-next.

@SGFGOV
SGFGOV requested review from dwene and lcmohsen July 28, 2026 00:42
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fc0fee49-8bee-40af-aae3-36bf1c4c8b13

📥 Commits

Reviewing files that changed from the base of the PR and between 7bb37b5 and d0d7937.

📒 Files selected for processing (1)
  • plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts

Walkthrough

The Braintree provider now stores the gateway returned by initialization, updates option type checks, accumulates refund records in braintreeRefunds, updates a refund error message, and changes the package version to 0.1.6-next.

Changes

Braintree provider update

Layer / File(s) Summary
Gateway initialization and option validation
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
The constructor stores the gateway returned by init, init returns the configured gateway, and option checks use keyof BraintreeOptions.
Refund history accumulation
plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
Void and direct refund responses append new refund records to prior braintreeRefunds data, use the existing transaction value, and update the “cannot be refunded” error text.
Package version update
plugins/braintree-payment/package.json
The package version changes from 0.1.5 to 0.1.6-next.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: dwene, lcmohsen

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main changes: refund handling updates and type-safety refactors.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/refunds-2

Comment @coderabbitai help to get the list of available commands.

@SGFGOV SGFGOV changed the title fix: refunds and minor refactors for type safety [MI-1340] fix: refunds and minor refactors for type safety Jul 28, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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`:
- Line 848: Normalize input.data?.braintreeRefunds to an array before spreading
it in both affected sites: braintree-base.ts lines 848-848 and 896-896. Update
the refund-history handling to validate the raw value with an array check and
fall back to an empty array for legacy or corrupted non-array payloads.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb265eea-90b3-4fa6-a5f7-c23efa2ca34f

📥 Commits

Reviewing files that changed from the base of the PR and between b782908 and 15487a2.

📒 Files selected for processing (2)
  • plugins/braintree-payment/package.json
  • plugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts

@currybot-lc

currybot-lc Bot commented Jul 28, 2026

Copy link
Copy Markdown

One remaining compatibility case: this fixes repeat refunds only when Payment.data.transaction still points to the original Braintree sale.

Orders that were already partially refunded under the old implementation have that field overwritten with the returned credit transaction. On their next refund, the provider will still retrieve the credit and reproduce Credit transactions cannot be refunded.

Could we self-heal this in refundPayment?

  1. Retrieve the transaction currently stored in Medusa.
  2. If it is a credit/refund and has refundedTransactionId, retrieve that original sale transaction.
  3. Use the original sale ID for the refund.
  4. Return/persist the original sale in data.transaction, while continuing to append the new entry to braintreeRefunds.

Braintree exposes refundedTransactionId on refund transactions, so this should repair legacy payment data on demand without requiring all old orders to be migrated up front.

It would also be good to add regression coverage for:

  • an old order that has never been refunded;
  • an order already partially refunded by the previous provider behavior; and
  • two consecutive partial refunds, asserting both gateway calls target the original sale ID.

@currybot-lc currybot-lc Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍛 Currybot Review

📋 Context

  • Ticket: MI-1340 — refund attempts for Braintree decline amounts 2005, 2014, and 2047 must fail in Medusa and be recorded as declined in Braintree.
  • Production constraint: There are no partial refunds in production today, so legacy partial-refund recovery is not a current compatibility requirement.
  • Related: No repository Cursor rules or DevAgent plan apply. The head has no GitHub check runs.
  • Scope: ⚠️ The diff primarily changes successful-refund persistence and does not itself change decline detection.

⚠️ Plan Quality

  • The refund-history/output-contract change is not required for the ticket’s decline-response acceptance criteria or for current full-refund-only production behavior. If this is intentionally laying groundwork for partial refunds, that should be a separately documented contract change with migration and tests.

🔍 Gap Analysis

  • Blocking — existing refund tests are now stale and will fail. The implementation removes data.braintreeRefund in favor of data.braintreeRefunds, but the existing provider suite still asserts the singular field in six successful void/refund cases (braintree-base.spec.ts:225, 248, 272, 316, 471, 491). Settled-refund entries also replace the prior transaction-shaped { id } value with { transactionId }. Update the contract and regression coverage before merging, including a full refund and a void.
  • MI-1340 linkage is unclear. The patch does not modify isBraintreeFailureResponse, decline status handling, or the failed-refund path. If the intended fix is publishing already-merged decline handling under 0.1.6-next, say that explicitly and keep this PR release-focused; otherwise the ticket behavior is not implemented by this diff.
  • The earlier legacy partial-refund self-healing concern is not blocking given the confirmed production state: there are no already-partially-refunded production orders to migrate.

🧪 Code Quality

  • Changing braintreeRefund to braintreeRefunds is a breaking provider-data contract in a patch-level prerelease. Even if no in-repo runtime consumer was found, downstream consumers may read the existing singular field. Preserve it during a transition or explicitly version and document the contract.
  • For current full refunds, the void path now persists the pre-void transaction object instead of voidedTransaction, and the refund path persists the original sale instead of the returned refund transaction. The former leaves stale status data immediately after a void; neither behavior change is needed for today’s full-refund-only flow.
  • The gateway return typing and keyof BraintreeOptions indexing are straightforward and type-safe.

🔧 Simplicity Audit

  • [VIOLATION] Refund-history accumulation and a plural output contract are added for a partial-refund lifecycle that is not in production and is outside MI-1340’s decline-handling scope. Preserve the existing full-refund output unless partial refunds are being implemented and tested as an explicit feature. -1.0
  • No unjustified abstraction layers were introduced.

🌟 Highlights

  • Array.isArray safely rejects malformed legacy history values instead of spreading them.
  • The non-refundable error now includes the transaction status, which will make support diagnostics clearer.
  • Keeping the original sale ID is useful groundwork for a future, explicitly scoped partial-refund implementation.

📊 Merge Confidence: 6.5/10

  • ✅ Small, readable change; gateway initialization and option-key typing are sound.
  • ⚠️ +1.5: restore/update the refund output contract and make the existing success-path tests pass.
  • ⚠️ +1.0: align the PR scope and description with MI-1340, including whether this is only a release/version vehicle for already-merged decline handling.
  • 🔧 Simplicity deductions applied: -1.0 total
  • Ready to merge: No.

@currybot-lc

currybot-lc Bot commented Jul 29, 2026

Copy link
Copy Markdown

Suggested change

Given that partial refunds are not in production, I would keep this PR narrowly focused on MI-1340 and preserve the existing successful-refund contract:

  • Restore the void result to transaction: voidedTransaction plus the singular braintreeRefund marker.
  • Restore the settled-refund result to transaction: refundTransaction plus braintreeRefund: refundTransaction.
  • Remove priorRefunds and braintreeRefunds; introduce a history model later as an explicitly scoped partial-refund feature with migration and compatibility coverage.
  • Keep the clearer non-refundable-status error and the type-safety improvements.
  • Add table-driven decline coverage for 2005, 2014, and 2047.
  • Clarify whether 0.1.6-next is only publishing decline handling that is already on main.

The success branches should remain equivalent to:

data: {
  ...input.data,
  transaction: voidedTransaction,
  braintreeRefund: {
    success: true,
    transactionId: voidedTransaction?.id,
    type: "void",
  },
}

and:

data: {
  ...input.data,
  transaction: refundTransaction,
  braintreeRefund: refundTransaction,
}

This keeps the current full-refund behavior stable and makes the six existing success-path assertions valid without prematurely introducing a partial-refund data model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant