magento-lts icon indicating copy to clipboard operation
magento-lts copied to clipboard

Invalid qty_shipped for shipments created via API

Open luigifab opened this issue 2 years ago • 11 comments

Preconditions

With latest commit at 1.9.4.x

Create a bundle product with one associated product with qty 2:

image

Go to your frontend, and create some orders with this product.

Results

1/ The way where all is working: open the first order, and ship the order. Check your database, in sales_flat_shipment_item:

image

And in sales_flat_order_item:

image

Here, all qty are good: 2.


2/ Now ship another order via API:

$proxy = new SoapClient('http://.../api.php/soap/?wsdl', ['trace' => 1, 'user_agent' => 'Local/Test']);
$sessionId = $proxy->login('covid', 'autotest2');
var_dump($proxy->call($sessionId, 'sales_order_shipment.create', '3400000040'));

Check your database, in sales_flat_shipment_item:

image

And in sales_flat_order_item:

image

Here, qty are wrong: 1 instead 2.

Extra

If I'm not wrong, you can count number of rows impacted in your production database with:

SELECT created_at, order_id, qty_ordered, qty_shipped, sku
FROM sales_flat_order_item
WHERE qty_shipped > 0 AND qty_ordered != qty_shipped
ORDER BY item_id ASC
LIMIT 5;

luigifab avatar Jul 12 '22 09:07 luigifab

I started some debugging:

diff --git a/app/code/core/Mage/Sales/Model/Order/Shipment/Item.php b/app/code/core/Mage/Sales/Model/Order/Shipment/Item.php
index 7c6fc5c3c..cb4876091 100644
--- a/app/code/core/Mage/Sales/Model/Order/Shipment/Item.php
+++ b/app/code/core/Mage/Sales/Model/Order/Shipment/Item.php
@@ -137,6 +137,8 @@ class Mage_Sales_Model_Order_Shipment_Item extends Mage_Core_Model_Abstract
      */
     public function setQty($qty)
     {
+//Mage::log(debug_backtrace(!DEBUG_BACKTRACE_PROVIDE_OBJECT|DEBUG_BACKTRACE_IGNORE_ARGS,2));
+Mage::log($this->getSku().' setQty '.$qty);
         if ($this->getOrderItem()->getIsQtyDecimal()) {
             $qty = (float) $qty;
         } else {
@@ -162,7 +164,7 @@ class Mage_Sales_Model_Order_Shipment_Item extends Mage_Core_Model_Abstract
      * @return $this
      */
     public function register()
-    {
+    {exit;
         $this->getOrderItem()->setQtyShipped(
             $this->getOrderItem()->getQtyShipped()+$this->getQty()
         );
diff --git a/app/code/core/Mage/Sales/Model/Service/Order.php b/app/code/core/Mage/Sales/Model/Service/Order.php
index c875b630e..db0a083fe 100644
--- a/app/code/core/Mage/Sales/Model/Service/Order.php
+++ b/app/code/core/Mage/Sales/Model/Service/Order.php
@@ -168,6 +168,7 @@ class Mage_Sales_Model_Service_Order
 
             if ($orderItem->isDummy(true)) {
                 $qty = 0;
+Mage::log($item->getSku().' isDummy '.print_r($qtys, true));
                 if (isset($qtys[$orderItem->getParentItemId()])) {
                     $productOptions = $orderItem->getProductOptions();
                     if (isset($productOptions['bundle_selection_attributes'])) {

When you try to ship an order from backend:

2022-07-12T09:38:10+00:00 DEBUG (7): Simple setQty 1
2022-07-12T09:38:10+00:00 DEBUG (7): jklkj setQty 1
2022-07-12T09:38:10+00:00 DEBUG (7): Simple-Associé isDummy Array
(
    [39] => 1
    [40] => 1
)
2022-07-12T09:38:10+00:00 DEBUG (7): Simple-Associé setQty 2

And from API:

2022-07-12T09:37:04+00:00 DEBUG (7): Simple setQty 1
2022-07-12T09:37:04+00:00 DEBUG (7): jklkj setQty 1
2022-07-12T09:37:04+00:00 DEBUG (7): Simple-Associé isDummy Array
(
)

2022-07-12T09:37:04+00:00 DEBUG (7): Simple-Associé setQty 1

Not sure, but it seems that because $qtys is empty... since 11 years ago !?

luigifab avatar Jul 12 '22 09:07 luigifab

More than likely there have been other bugs for a long time. Are those particular cases that we rarely meet. In general such issues are discovered by the mid-level users who find them. As long as they were never reported by someone the Magento team did not take every line of code to check it periodically. Last night I found in an XML file information about Shockwave, Adobe Flash Player, that we did not remove yet. There were also some references to LTS csv files that were recently deleted. You are best able to solve this issue when you have a solution do not forget to do a PR.

addison74 avatar Jul 12 '22 09:07 addison74

I opened a discussion a while ago that I explained a similar issue with bundle items refund, and I think the same issue is happening with shipment qty.

elidrissidev avatar Jul 12 '22 09:07 elidrissidev

There is a similar problem with qty_invoiced of bundle items... qty_invoiced = qty_ordered for partial invoice. I think that nothing works.

The qty_canceled also not working (= 0) 🥇

luigifab avatar Jul 12 '22 12:07 luigifab

It all has to do with the price type and the isDummy method's result I think, such a weird logic.

elidrissidev avatar Jul 12 '22 13:07 elidrissidev

Yes! There is no logic to calculate the qty.

https://github.com/OpenMage/magento-lts/blob/24c8721621e5371a231a678c4b2430cff1aedb97/app/code/core/Mage/Sales/Model/Order/Item.php#L326-L338

https://github.com/OpenMage/magento-lts/blob/24c8721621e5371a231a678c4b2430cff1aedb97/app/code/core/Mage/Sales/Model/Order/Item.php#L354-L369

https://github.com/OpenMage/magento-lts/blob/24c8721621e5371a231a678c4b2430cff1aedb97/app/code/core/Mage/Sales/Model/Order/Item.php#L371-L382

https://github.com/OpenMage/magento-lts/blob/24c8721621e5371a231a678c4b2430cff1aedb97/app/code/core/Mage/Sales/Model/Order/Item.php#L403-L418

https://github.com/OpenMage/magento-lts/blob/24c8721621e5371a231a678c4b2430cff1aedb97/app/code/core/Mage/Sales/Model/Order/Item.php#L420-L432

luigifab avatar Jul 12 '22 13:07 luigifab

Before any changes, for configurable products, in sales_flat_shipment_item, when an order is shipped from API there are 2 lines (one for the configurable item, and another one for the simple item), but when an order is shipped from backend there is 1 line (only one for the configurable item).

So, who says the truth?

shipment from api api-simple-before

shipment from backend backend-simple-before

luigifab avatar Jul 19 '22 11:07 luigifab

I had issues calculating some reports in the past and I guess those miscounts were coming from this bug that I didn't notice!

I guess I'd say the frontend tells the truth (with the 2 lines) since on most shops frontend orders are the vast majority.

fballiano avatar Jul 19 '22 12:07 fballiano

I'm sad for that, this will be more easier to drop the second line to fix previous data!

luigifab avatar Jul 19 '22 14:07 luigifab

I agree on the sadness cause it would be nice to drop half of the rows of those tables but, from what I've seen, a lot of people have built code that relies on those rows (myself included) so if we remove those it could be tricky.

I've also notices that not all the "numeric columns" (I'm talking about the various prices, discounts etcc) of the sales_flat_*_item have values, some columns are filled on the configurable, some on the simple... which makes a sanitization even trickier.

But, I mean, if we find a way to drop half records of a table that may have milions... it would be a great achievement.

fballiano avatar Jul 20 '22 08:07 fballiano

Fixed by 2395.

We are not using stock from OpenMage side, but we are synchronizing stock data from orders to another tool. So I suppose this is better than nothing.

luigifab avatar Jun 08 '23 10:06 luigifab