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

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

Open
stefanw opened this issue Mar 24, 2022 · 5 comments · Fixed by PetrDlouhy/django-payments-payu#9 or #412
Labels

Comments

@stefanw
Copy link

stefanw commented Mar 24, 2022

b1dc16c 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:

payment.attrs.payer_info = executed_payment["payer"]["payer_info"]
if self._capture:
payment.captured_amount = payment.total
payment.change_status(PaymentStatus.CONFIRMED)
else:
payment.change_status(PaymentStatus.PREAUTH)
return redirect(success_url)

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
Copy link
Member

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. 🤔

@WhyNotHugo WhyNotHugo added the bug label Mar 24, 2022
@blayzen-w
Copy link

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 pushed a commit that referenced this issue Apr 18, 2022
@WhyNotHugo WhyNotHugo pinned this issue May 17, 2022
WhyNotHugo pushed a commit that referenced this issue May 21, 2022
WhyNotHugo pushed a commit that referenced this issue May 21, 2022
@WhyNotHugo
Copy link
Member

WhyNotHugo commented May 21, 2022

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: #318

onoy42 pushed a commit to onoy42/django-payments that referenced this issue Jun 22, 2023
radekholy24 added a commit to radekholy24/django-payments that referenced this issue Apr 30, 2024
radekholy24 added a commit to radekholy24/django-payments-payu that referenced this issue May 13, 2024
radekholy24 added a commit to radekholy24/django-payments that referenced this issue May 13, 2024
radekholy24 added a commit to radekholy24/django-payments that referenced this issue May 14, 2024
radekholy24 added a commit to radekholy24/django-payments that referenced this issue May 14, 2024
radekholy24 added a commit to radekholy24/django-payments that referenced this issue May 14, 2024
radekholy24 added a commit to radekholy24/django-payments that referenced this issue May 16, 2024
@radekholy24
Copy link
Contributor

radekholy24 commented May 16, 2024

@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: #412 Similar work needs to be done on multiple places though but this particular part prevents us from performing proper refunds.

@PetrDlouhy PetrDlouhy reopened this May 16, 2024
WhyNotHugo pushed a commit that referenced this issue Jun 11, 2024
@radekholy24
Copy link
Contributor

@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?

@WhyNotHugo WhyNotHugo reopened this Jun 12, 2024
onoy42 added a commit to onoy42/django-payments that referenced this issue Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants