Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createRefund not taking promotions into account #2393

Closed
jacobfrantz1 opened this issue Sep 12, 2023 · 8 comments
Closed

createRefund not taking promotions into account #2393

jacobfrantz1 opened this issue Sep 12, 2023 · 8 comments
Assignees
Labels
next minor Candidate for the next minor release type: bug 🐛 Something isn't working @vendure/core

Comments

@jacobfrantz1
Copy link
Contributor

jacobfrantz1 commented Sep 12, 2023

Describe the bug
When canceling and refunding a order that has promotions applied. The refundOrderLinesTotal doesn't account for promotions. The payment is properly refunded, but there is an error as it looks for another payment to cover the total, and the refund on vendure's side fails.

From the logs:

error 9/7/23, 6:58 PM - [Vendure Server] {
  "message": "INTERNAL_SERVER_ERROR: Could not find a Payment to refund",
  "variables": {}
}
Error: Could not find a Payment to refund
    at PaymentService.createRefund (/home/app/node_modules/@vendure/core/dist/service/services/payment.service.js:253:23)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async OrderService.refundOrder (/home/app/node_modules/@vendure/core/dist/service/services/order.service.js:1088:16)
debug 9/7/23, 6:58 PM - [Vendure Server] Error: Could not find a Payment to refund
    at PaymentService.createRefund (/home/app/node_modules/@vendure/core/dist/service/services/payment.service.js:253:23)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async OrderService.refundOrder (/home/app/node_modules/@vendure/core/dist/service/services/order.service.js:1088:16)

To Reproduce
Steps to reproduce the behavior:
1.) Cancel and Refund order that has promotions applied.
2.) See Error "Could not find a Payment to refund"

Expected behavior
The problem may be wide spread, as order line prices are not calculated properly on placed orders.

The problem is here:

private getUnitAdjustmentsTotal(type?: AdjustmentType): number {
if (!this.adjustments || this.quantity === 0) {
return 0;
}

and here:
if (!this.adjustments || this.quantity === 0) {
return 0;
}

These function exits early as quantity is 0, but orderPlacedQuantity is not 0.

Environment (please complete the following information):

  • @vendure/core version: 2.0.6
  • Nodejs version: 16.20.1
  • Database (mysql/postgres etc): postgres 14
@jacobfrantz1 jacobfrantz1 added the type: bug 🐛 Something isn't working label Sep 12, 2023
@seminarian
Copy link
Contributor

Oeh, that's a nasty one! :(

@michaelbromley
Copy link
Member

michaelbromley commented Sep 26, 2023

I'm going to re-think the way that refunds work. Right now it is both too complex and too restrictive.

  • Refund amounts are internally calculated from order lines, shipping and optional adjustment. This is OK in the simplest case, but with e.g. multiple payment methods it becomes very unpredictable and hard to reason about.
  • There's a tight coupling between modification to the order contents and refund amount. Again, this can be a good thing in the simple case, but it becomes onerous in many real-world situations.

I think a better design would allow:

  1. Finer control over which payments will get refunded.
  2. A manual override on the refund amounts. Sometimes stuff just happens where you need to refund a payment and it might not precisely correspond to a particular cancellation.
  3. A re-think of the UI flow for cancellation & refund. Perhaps more of a wizard-like step-by-step flow would be better. Or - allow refunds to be made directly against the payment card, and separate it entirely from the item cancellation.

@jacobfrantz1
Copy link
Contributor Author

Something to consider that we encounter is when an order is changed, when need to refund the difference to them, the entire payment can't be considered refunded because if the customer wants to cancel later, they need refunded the part that wasn't refunded previously. I would like the redesign of refunds to allow for partial refunds against a payment.

Something else I want to implement, but don't have a good way to currently, is to have a portion of the order be non-refundable, as we sell custom made-to-order products, and calculating and adding an adjustment every time someone cancels is onerous.

@michaelbromley
Copy link
Member

@jacobfrantz1 thanks for the input. I definitely intend to allow partial refunds (as long as the underlying payment provider allows it, which is mostly the case).

Right now the big issue for me is that the way Refunds are designed, they have to be related to a particular part of the order that is getting refunded (e.g. an order line item, or shipping). I'm not sure if this is useful or not. I'm wondering whether it would make more sense to just simplify refunds and allow the store operator to decide how much to refund from each payment method. Of course we can calculate what we think the refund should be based on e.g. cancelled items, but beyond that maybe just let the store operator have total control and not have Vendure do its own calculations on what it thinks should be an allowable refund in the backend logic as it does currently.

Any thoughts on that?

@jacobfrantz1
Copy link
Contributor Author

jacobfrantz1 commented Sep 30, 2023

The only use case I can think of for linking refunds to order line items is for statistics, which isn't super important.

I think how the refund amount is determined should be a configurable strategy, though the store operator should still be able to change it to what they want. That would help my use case, though I'm not sure if enough people would find that useful to justify it.

@michaelbromley michaelbromley added the next minor Candidate for the next minor release label Dec 15, 2023
@martijnvdbrug
Copy link
Collaborator

I agree that separating refunds from cancellation gives us a lot more freedom on how to do refunds. It also feels a more intuitive for me. We could then just manually refund $10, with a manual note like Broken product, no return, correct?

As you already said, you can still have the admin UI do a suggestion for a refund when you cancel an order item.

❓ What happens with the order when you refund $10, but don't cancel any items? E.g:

  1. An order is placed with 3 items, with a total of $100
  2. A manual refund is created for $10
  3. The order still has 3 items

Will the order total still be $100, or will it be $90? I'm thinking about this from an invoice perspective, where the invoice should reflect the actual amount paid. (So, $90 should be on a re-created invoice after refund)

@michaelbromley
Copy link
Member

Related:

@michaelbromley
Copy link
Member

@martijnvdbrug good question. I'm not sure, I'd need to research how other systems handle this.

Here's a video that show's Shopify's refund process - it is very simple and follows the idea I outline above where the system will calculate the refund amount based on the selected products being refunded, but also just allows the amount to be set manually.

https://www.youtube.com/watch?v=Tk0DZd4MSCU&t=64s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next minor Candidate for the next minor release type: bug 🐛 Something isn't working @vendure/core
Projects
Status: Done
Development

No branches or pull requests

4 participants