feat(core, dashboard): Support pluggable refund destinations#4594
feat(core, dashboard): Support pluggable refund destinations#4594oliverstreissi wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Dashboard Preview: https://admin-dashboard-51vnluwnj-vendure.vercel.app |
The fix for modified order lines not appearing in the refund dialog needs further investigation. Reverting to keep the PR clean.
| * The returned {@link CreateRefundResult} determines the refund state | ||
| * and any associated transaction ID or metadata. | ||
| */ | ||
| createRefund( |
There was a problem hiding this comment.
Will this not lead to duplication of logic between eg the StoreCreditPaymentMethodHandler and the StoreCreditRefundDestination?
Why do we not delegate this logic to the existing payment method handler?
…r RefundDestinationStrategy Relates to #4563
… method [skip ci]
|
There was a problem hiding this comment.
Dumping some notes from Claude here for when we revisit.
Architectural Review
Thanks for the solid work on this — the feature is clearly needed, and the strategy interface, e2e tests, and dashboard UI refactor are all well done. However, there are some architectural concerns worth discussing before merge.
1. Payment coupling for non-payment destinations
This is the main one. Every Refund is forced to reference a Payment, even when the destination (store credit, gift card) has nothing to do with the original payment method. The dashboard grabs firstRefundablePaymentId just to satisfy the FK — making Refund.payment semantically meaningless for destination refunds.
Both createRefund() and isAvailable() take a Payment parameter that's irrelevant for non-payment destinations. This will cause confusion for anyone querying refund data downstream.
Suggestion: Make Refund.payment nullable for destination-based refunds, and remove the Payment parameter from the strategy interface (pass Order only). If payment context is genuinely needed for some destinations, it could be optional.
2. No database migration
The PR adds @Column({ nullable: true, type: 'varchar' }) destination to the Refund entity but includes no migration file. This column won't exist until someone generates and runs a migration.
3. isAvailable inconsistency between query and mutation
getRefundDestinations() checks availability against ALL refundable payments (any match = available):
for (const payment of refundablePayments) {
if (await strategy.isAvailable(ctx, order, payment)) {
available = true;
break;
}
}But createRefund() checks against the specific paymentId the dashboard sent — which for destinations is just firstRefundablePaymentId. This means the query could report a destination as available (because payment B passes), but the mutation rejects it (because it checks payment A).
4. Non-atomic split refunds
Multi-target refunds send sequential mutations from the dashboard. If the first succeeds and the second fails, you get a partial refund with no rollback. Only the first target gets refundLines, so subsequent Refund entities have no line references — which will cause issues in reporting and reconciliation.
Suggestion: Consider a single-mutation API that accepts multiple targets, so the server can handle this atomically.
5. Hardcoded constant duplication
// Must match DEFAULT_REFUND_DESTINATION_CODE from @vendure/common
const DEFAULT_REFUND_DESTINATION_CODE = 'default';The CJS/ESM issue is understood, but this will go stale silently. Could the refundDestinations query mark which destination is default, avoiding the need for the dashboard to know the constant?
Minor items
- No validation for duplicate strategy codes at startup
getRefundDestinationslives onOrderServicebut deals with payment/refund concerns —PaymentServicemight be a more natural home- ~60 lines of unrelated formatting changes (import reordering, ternary indentation) make the diff noisier than needed
2 participants
Description
Add
RefundDestinationStrategyinterface allowing plugins to define alternative refund destinations (store credit, gift cards, etc.) instead of only refunding to the original payment method.Backend:
RefundDestinationStrategyinterface withisAvailable/createRefundmethodsrefundDestinationsquery to discover available destinations per orderdestinationfield onRefundOrderInputandRefundentityPaymentService.createRefundthrough destination strategy when specifieddefaultcodeDashboard:
refundFragmentand dead utility functionsFixes #4563
Breaking changes
None. Omitting
destinationinRefundOrderInputpreserves existing behavior.Screenshots
Screen.Recording.2026-03-31.at.10.21.26.mov
Checklist
📌 Always:
👍 Most of the time: