Handle refunds where there is more than one invoice on the same day #198
Conversation
…ne with the same date
| invoices: List[Invoice], | ||
| itemsByInvoiceId: Map[String, List[InvoiceItem]], | ||
| ): (String, Invoice, List[InvoiceItem]) = { | ||
| joinInvoiceWithInvoiceItemsOnInvoiceIdKey(invoices, itemsByInvoiceId).iterator | ||
| .filter({ case (_, invoice, _) => invoice.Status == "Posted" }) | ||
| .filter({ case (_, invoice, _) => invoice.Amount > 0 }) | ||
| .filter({ case (_, invoice, _) => invoice.Amount >= refund }) |
There was a problem hiding this comment.
Maybe this should be invoice.PaymentAmount instead?
There was a problem hiding this comment.
good question, if they are in payment failure, (or the invoice is already adjusted or paid from a credit balance) then this might not work out well
There was a problem hiding this comment.
Actually existing adjustments and payments are fetched and checked later on in the program logic:
So it's probably ok, I wouldn't be surprised if there's a way to break it though!
There was a problem hiding this comment.
yeah but by that time we have already settled on a specific invoice which we fail if it doesn't have a payment
https://github.com/guardian/invoicing-api/blob/38fca38faa79761035975920e3a3ebdbd8687c4f/src/main/scala/com/gu/invoicing/refund/Program.scala#L49C27-L49C46
It doesn't look like we check if it's in payment failure, we would try to refund against a failed payment perhaps
the adjustment fetching is just so we can distribute further adjustments by the look
| val (invoiceId, invoice, items) = decideRelevantInvoice(refund, invoices, itemsByInvoiceId) tap { | ||
| case (_, invoice, _) => | ||
| s"$invoice should be posted and have an amount >= the refund value" assert ( | ||
| invoice.Amount >= refund && invoice.Status == "Posted" |
There was a problem hiding this comment.
I guess this is some kind of postcondition based programming? as it's asserting the logic in the function anyway.
There was a problem hiding this comment.
Yes it's slightly odd, I think it's just to concentrate all the error handling in one place and ensure that it's consistent and readable. I actually quite like it as an approach.
There was a problem hiding this comment.
found an interesting doc, which includes a section on that https://github.com/guardian/invoicing-api/blob/main/adr/lightweight-architecture.md#testing-in-production-via-preconditions-program-postconditions-instead-of-mocked-test
There was a problem hiding this comment.
it's going to work better in this case, but I think there are bigger issues in how we're using this functionality. I think we should probably model this on the auto cancellation process - ideally find the exact invoice for the cancelled period, and only refund the payment on that.
What does this change?
The invoicing-api currently has an issue with refunding subscriptions where there is more than one invoice on the same day.
This can happen during a product switch, for instance, if I take out an annual recurring contribution for £30 then decide that actually I want supporter plus so that I can access the app and immediately switch to annual S+ at a cost of £95/year. In Zuora my subscription will now have two associated invoices dated today one for £30 and one for £65 (30 + 65 = total cost of the S+ sub).
If I then cancel within 14 days we will attempt a refund of the £65 paid at switch using the invoicing-api and that will pick the most recent positive posted invoice on the account to carry out the refund on.
However the 'most recent' check is only to a day level so which of the invoices it chooses is undefined - in practice it seems to be the first one which in this example is the wrong one and does not have enough money on it to enable the refund.
This PR changes the behaviour which picks the appropriate invoice to run the refund against by ensuring that the invoice which we pick has an amount at least equal to the amount we are trying to refund.