[MI-1340] fix: refunds and minor refactors for type safety - #41
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Braintree provider now stores the gateway returned by initialization, updates option type checks, accumulates refund records in ChangesBraintree provider update
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
plugins/braintree-payment/package.jsonplugins/braintree-payment/src/providers/payment-braintree/src/core/braintree-base.ts
|
One remaining compatibility case: this fixes repeat refunds only when 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 Could we self-heal this in
Braintree exposes It would also be good to add regression coverage for:
|
There was a problem hiding this comment.
🍛 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.braintreeRefundin favor ofdata.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 under0.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
braintreeRefundtobraintreeRefundsis 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
transactionobject instead ofvoidedTransaction, 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 BraintreeOptionsindexing 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.isArraysafely 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.
Suggested changeGiven that partial refunds are not in production, I would keep this PR narrowly focused on MI-1340 and preserve the existing successful-refund contract:
The success branches should remain equivalent to: and: 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. |
Summary by CodeRabbit