search

jazzband / django-payments

Universal payment handling for Django.
https://django-payments.readthedocs.io
Other
1.07k stars 282 forks source link

Code relying on `payment.change_status` to save whole payment object fails #309

Open stefanw opened 2 years ago

stefanw commented 2 years ago

https://github.com/jazzband/django-payments/commit/b1dc16c8234993abdf506e358f867cdb7a1ed707 made change_status 'safer' by not saving the whole payment object but just some fields.

This backwards incompatible change got me a while ago and I fixed it in my own implementation, but it looks like code in this project also has not been adapted to this new behaviour.

One example is the Paypal provider:

https://github.com/jazzband/django-payments/blob/38dae99d5adb52127bc79fb977a4e7e20f5041ed/payments/paypal/__init__.py#L252-L258

payment.attrs and payment.captured_amount are NOT saved due to change_status only doing: self.save(update_fields=["status", "message"]). Or am I missing something?

WhyNotHugo commented 2 years ago

Right, this is a real issue.

Context for the change in change_status: sometimes users were redirected back, and while unrelated operations were happening to the payment object, a second provider notification came in, and the concurrent updates left the payment object in a borked state.

In other words, it prevents issues with concurrent editions of the Payment object, but the approach has not been ideal. I think locking the row while processing callbacks and alike would be best. Applications should also lock the row to edit it to avoid issues too.

Or maybe applications should avoid mutating Payment objects themselves as a rule. :thinking:

blayzen-w commented 2 years ago

The change to change_status also caused problems for us. We were relying on it to save the entire object so we didn't need to call save twice.

Adding this backwards incompatible change in the change log file would help other people with upgrading the version.

WhyNotHugo commented 2 years ago

I'm aiming to stabilise this again for the next release. I'll revert the offending commit.

I've also added some docs on good practices to avoid the issue that the original commit/change intended to address: https://github.com/jazzband/django-payments/pull/318

radekholy24 commented 6 months ago

@PetrDlouhy, I believe this got closed accidentally by merging my PR into your PayU provider. Can you reopen this?

BTW: Here, is my PR to this repo to fix one part of this issue: https://github.com/jazzband/django-payments/pull/412 Similar work needs to be done on multiple places though but this particular part prevents us from performing proper refunds.

radekholy24 commented 5 months ago

@WhyNotHugo, I believe this got closed accidentally by merging my PR. Similar work needs to be done in multiple places before this issue can be considered resolved. Can you reopen this?