-
Notifications
You must be signed in to change notification settings - Fork 64
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
Do not process orders automatically #272
Conversation
AFAIK, if payment action is set to Authorize, you won't get pinged via PP IPN. The order status is updated by IPN handler, so I think it should remains in on-hold. |
Ok, i just tested this with master and apparently it updates the status :/. Testing your branch. |
@bor0 I think the culprit of this is sandbox check on IPN handler. PayPal standard in core has the same check. This won't affect live transaction. I'm not sure the reasoning behind that. @mikejolley did you remember why we need to set payment status to completed (in IPN handler) when sandbox is enabled? The change introduced in this PR will affect transaction when payment action is set to Sale (authorize and capture). So, i suggest we either remove the sandbox check or add another check based on payment action setting. @yukikatayama @mdmoore does the issue happens on live mode? I imagine if that's the case it will generate lots of tickets. |
@gedex A longstanding issue in PayPal IPN was that it was not possible to get 'completed' status back - only pending. So to work around this and test payments for real we added the test_ipn check to workaround it. It only affects sandbox and should not affect live. It would be better to not work around it in other places and just accept that in testing with Sandbox, all transactions are completed. |
Thanks for confirming @mikejolley! Waiting for @yukikatayama / @mdmoore to confirm that it only happens on sandbox before closing it as wont fix. |
It would have to be both core change (for PayPal) and change in this extension (PPEC), right? Or did you mean changing some other part? |
@bor0 Just the core test_ipn check |
@mikejolley but we have that check both in So just confirming it needs both changes on the extension and core? |
Ah yes. I didn't realise PPEC had IPN too. |
@gedex / @mikejolley another round of review please? (Along with woocommerce/woocommerce#15402) |
Fixes #260.