buckaroo-it / Magento2

Repository containing the Magento2 plugin by Buckaroo
MIT License
28 stars 35 forks source link

Orders state + status incorrectly updated to Pending (from Processing) #827

Closed Skullsneeze closed 6 months ago

Skullsneeze commented 1 year ago

For certain orders we are seeing the state and status being updated from processing to pending after a successful payment has been pushed.

I have not been able to find a direct cause, but based on the debug logs I do see the following:

1. Start of order push is logged

Buckaroo\Magento2\Model\Push::receivePush|1|array (...)

2. The Push is successful and the current order state + status is logged:

Buckaroo\Magento2\Model\Push::receivePush|2|array (
  'message' => 'Success',
  'status' => 'BUCKAROO_MAGENTO2_STATUSCODE_SUCCESS',
  'code' => '190',
) [] []
Buckaroo\Magento2\Model\Push::canUpdateOrderStatus|1|array (
  0 => 'new',
  1 => 'pending',
) [] []

3. Order state + status is updated:

Buckaroo\Magento2\Model\Push::updateOrderStatus|0|array (
  0 => 'processing',
  1 => 'processing',
  2 => 'Payment status : <strong>Success</strong><br/>Total amount of € 108,45 has been paid',
) [] []

4. Final debug log from the Push is logged (indicating success):

Buckaroo\Magento2\Model\Push::receivePush|6|

5. (This is where the issue begins I believe) CommandInterface plugin is called:

Buckaroo\Magento2\Plugin\CommandInterface::aroundExecute|1|array (
  0 => 'buckaroo_magento2_ideal',
  1 => 'order',
) [] []

6. The order state + status is updated to Pending:

Buckaroo\Magento2\Plugin\CommandInterface::updateOrderStateAndStatus|5|'pending'

It seems that steps 5 and 6 are not occurring for all orders, but I can't find any logic as to why that would happen. I also don't see the logic in forcing the state and status here, so would also be interested in knowing why this is happening.

Buckaroo-Rens commented 1 year ago

@LucianTuriacArnia Can you have a look at this?

LucianTuriacArnia commented 1 year ago

Yes, I am looking into this issue. I will provide updates as I find them.

LucianTuriacArnia commented 1 year ago

We have implemented the CommandInterface Plugin to set the status of nearly all payment methods to 'pending' until we process the push. This ensures that the amount has been captured. Below is a comparison of an order placed with a credit card: with the plugin, the status is 'New/Pending', and without the plugin, it's 'Processing/Processing'. image

The issue arises randomly because sometimes the push arrives either before or simultaneously with the transaction response. For certain payment methods, we delay processing the push until we receive the transaction response. However, for iDEAL, we cannot apply the same logic.

My solution is to skip setting the status in CommandInterface if the order has total_paid = grand_total.

Total Paid is set during the Push, and the order is saved afterward. However, we still need to check if the object from CommandInterface contains the updated values.

I created the next PR: https://github.com/buckaroo-it/Magento2/pull/831/files

boldhedgehog commented 1 year ago

Hi @LucianTuriacArnia ,

I would suggest to not compare total due and total paid like this, because they can be float or string values, and it is risky to compare float values like this. It's safer to check that the difference is is less than a threshold:

if ($order->getGrandTotal() - $order->getTotalPaid() < 0.001) {

LucianTuriacArnia commented 1 year ago

Hi @boldhedgehog ,

Thank you, you are right, I will update the code.

boldhedgehog commented 1 year ago

@LucianTuriacArnia , One more thing: it is a known Magento issue caused by the same race condition, when 2 requests are updating one order at the same moment, the orders gets invoiced, the status is changed to Processing, the items are updated etc, but the total due and total paid are not updated, so this check will not work. Example, this order is paid and the status of the order is Processing:

image

LucianTuriacArnia commented 1 year ago

@boldhedgehog on the Push processor we saved the order and we update the total_paid app/code/Buckaroo/Magento2/Model/Push.php:1285 and CommandInterface::updateOrderStateAndStatus is called after image I don't think it is the same case, but I will check.

boldhedgehog commented 1 year ago

@LucianTuriacArnia this is another issue, indeed. My point was that the check of total paid might now always work.

LucianTuriacArnia commented 1 year ago

@Skullsneeze I propose implementing the fix using the following steps:

  1. Apply the fix to your environment: https://github.com/buckaroo-it/Magento2/pull/831/files

  2. If the issue persists, please send us the logs.

  3. Uncomment line 137: $order = $order->loadByIncrementId($order->getIncrementId()); to load the order from the database.

  4. If the issue persists after the above change, send us the logs again.

  5. If the problem continues, also apply this fix to ensure total_paid is updated in the database on Push: https://github.com/buckaroo-it/Magento2/pull/832

stefanfr commented 1 year ago

@Skullsneeze Please do not apply this "patch" this will ruin you order taxes. The patch prevents data from being saved in the sales_order_tax and sales_order_tax_item tables

LucianTuriacArnia commented 1 year ago

@stefanfr Thanks for bringing this to our attention. We've tested the system with and without the patch. So far, we haven't encountered any issues with the saving of taxes in the sales_order_tax and sales_order_tax_item tables as you mentioned.

To assist you better, could you please provide more details about the specific payment method you tested and share your tax configuration settings? This will help us narrow down the issue and address it more effectively.

Additionally, I'll be sharing an example of the test scenario we performed. Taxes.pdf

boldhedgehog commented 1 year ago

@LucianTuriacArnia we have this patch working on Production for a while with no such issues mentioned by @stefanfr

I suppose, he mean this part:

  1. Uncomment line 137: $order = $order->loadByIncrementId($order->getIncrementId()); to load the order from the database.
LucianTuriacArnia commented 1 year ago

Thank you @boldhedgehog @stefanfr , loading order using repository will fix the issue $order = $this->orderRepository->get($order->getId());

Due to limited feedback on the first version, I haven't had the opportunity to thoroughly analyze the commented code. Your insights on this issue are valuable, and I appreciate your collaboration.

https://github.com/buckaroo-it/Magento2/pull/846

LucianTuriacArnia commented 6 months ago

This issue was resolved with the following pull request: https://github.com/buckaroo-it/Magento2/pull/859