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

Order processor should not re-calculate order totals after payment #705

Closed
christopherbolt opened this issue Jan 2, 2019 · 4 comments
Closed

Comments

@christopherbolt
Copy link
Contributor

Currently the order totals are processed before redirecting to the gateway and then again afterwards.
They should not be processed again afterwards because this causes a significant issue with some offsite payment gateways such as Payment Express PxPay which notify the site of the successful payment in the background, so not part of the current user session. Consider the scenario where the site uses the SilverShop/discounts SpecificPricingExtension which allows specific prices top be set for different user groups: currently when the gateway notifies the site of a successful payment the order totals get re-calculated again but because this notification is not part of the user session there is no logged in user so the user specific discounts don't get factored in and so the order item prices and totals are wrong and the order is left with an amount outstanding.
There shouldn't be any need to recalculate the order totals again at this point as they were already calculated correctly before redirecting to the gateway.

@wilr
Copy link
Contributor

wilr commented Jan 4, 2019

Makes sense. PR welcome if you can track down where the total is recalculated post gateway redirect. Order::Total() should be used rather than calculating.

@christopherbolt
Copy link
Contributor Author

christopherbolt commented Aug 7, 2019

I've finally had a chance to look at this.
The solution is to simple; just remove all calls to $this->order->calculate() from SilverShop\Checkout\OrderProcessor.php
This is found in two functions placeorder and completePayment
Lines 220 and 288

Removing calls to $this->order->calculate() from the order processor should have no impact on functionality because the order totals have already been calculated at this point.
I will make a PR as soon as I have time

@christopherbolt
Copy link
Contributor Author

Have created a pull request: #720

@wilr
Copy link
Contributor

wilr commented Aug 7, 2019

Merged.

@wilr wilr closed this as completed Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants