djaodjin / djaodjin-saas

Django application for software-as-service and subscription businesses
Other
564 stars 124 forks source link

Always call processor API when loading Charge receipts #132

Closed smirolo closed 5 years ago

smirolo commented 6 years ago

The view ChargeReceiptView will call the API endpoint ChargeResourceView that in turn will call on the processor to retrieve the state of a Charge if it is "in process". This was intended to cut down on calls to the processor. As it turns out, in case the webhook for some reason does not update the state of a Charge from DONE to DISPUTED, we will get a 500 error when a manager attempts to refund a disputed charge (i.e. "cannot refund a disputed charge" error).

The API endpoint thus must always call the processor to retrieve the actual state of a Charge and disable refund buttons when the charge has been disputed.

To reproduce issue:

  1. Run testsite
  2. Browse to pricing page (aka /pricing/)
  3. Click on a plan
  4. Login as a subscriber (ex: xia) (redirects to cart)
  5. Enter Stripe test credit card 4000000000000259
  6. Note the Charge processor_key
  7. Login as a manager (ex: donny)
  8. Browse to the "Funds" dashboard page, click on the Charge link
  9. On the Charge receipt page, click the "Refund" button
  10. Enter a refund amount and submit
knivets commented 6 years ago

I'm not really sure what needs to be done here.

First of all, you said that this old logic was used to cut down on call to the processor, but every time a Charge is retrieved, a call to Stripe is being made.

Next, charge.retrieve return value is not being used anywhere. It doesn't even return a Stripe stripe.Charge representation, it basically returns its argument -- models.Charge.

I assume that "API endpoint ChargeResourceView that in turn will call on the processor to retrieve the state of a Charge if it is "in process" refers to this portion of code. Which means that removing this block will make this function do nothing, because when this function is called from charge.retrieve, event_type is always None, and all subsequent statements rely on it not being None.

So, the question is should I modify models.Charge state every time charge.retrieve is called or should this method always return stripe.Charge?

smirolo commented 6 years ago

So yes indeed a call to the Stripe API is being made on every Charge.retrieve (so much for caching).

When the stripe.Charge.status switch from pending to succeeded or failed, we set the event_type accordingly to call charge.payment_successful() and charge.failed() respectively. These create Transaction in the ledger so they must only be called once (i.e. on the state transition).

The issue is when stripe.Charge.status switch from succeeded to disputed (which apparently means stripe.Charge.dispute is not null) nothing is done but charge.dispute_created() should be called. Idem when the dispute is lost or won, charge.dispute_lost() or charge.dispute_won() should be called respectively - only once of course, not on every retrieve since it creates Transaction in the ledger.