Buspakje calculation seems to double child items

[edwinljacobs]

Submitting issues through Github

Make sure you are using the latest version: https://tig.nl/postnl-magento-extensies/

Issues with outdated version will be rejected.

• [ x] I've verified and I assure that I'm running the latest version of the TIG PostNL Magento extension. (version 1.15.7)

What is the purpose of your issue?

• [ x] Bug report (encountered problems with the TIG PostNL Magento extension)

We encountered a problem in the buspakje calculation process in particular methods
TIG_PostNL_Model_Core_Shipment _getIsBuspakje (line 4904) and TIG_PostNL_Helper_Data fitsAsBuspakje (line 1378)

We found that for orders with composite order items (one order item being the child of the other order item) the wrong order items are given to the method fitsAsBuspakje

At line 4925 in TIG_PostNL_Model_Core_Shipment::_getIsBuspakje we find the following code:

        $orderItems = array();
        foreach ($shipmentItems as $item) {
            $orderItem = $item->getOrderItem();

            if ($orderItem->getChildrenItems()) {
                $orderItems = array_merge($orderItems, $orderItem->getChildrenItems());
                continue;
            }

            $orderItems[] = $orderItem;
        }

        if (!$this->getHelper()->fitsAsBuspakje($orderItems)) {
            return false;
        }

The issue being that if the first shipment item contains the parent order item then its childItem gets added after which the second shipment item (which contains the child order item) will add the same order item to $orderItems.

In our case we have a product which fits 3 times in a "buspakje". The order consists of two order items one parent item with qty 1 and one child item with qty 2 (which is the underlying simple). Looking at the code above we end up with the scenario that $orderItems contains two orderItems namely the same order item is added to $orderItems twice. The method fitsAsBuspakje now thinks there are 4 simples and returns false.

Furthermore the method fitsAsBuspakje in version 1.15.7
contains the following code:

if ($item instanceof Mage_Sales_Model_Order_Item) {
                $qty = $item->getQtyOrdered();
                if (empty($qty) && $item->getParentItemId()) {
                    $qty = $item->getParentItem()->getQtyOrdered();
                }

which differs from version 1.15.4:

foreach ($items as $item) {
            /**
             * Get either the qty ordered or the qty shipped, depending on whether this is an order or a shipment item.
             */
            if ($item instanceof Mage_Sales_Model_Order_Item) {
                if ($item->getParentItemId()) {
                    $qty = $item->getParentItem()->getQtyOrdered();
                } else {
                    $qty = $item->getQtyOrdered();
                }

This difference is not just semantics but functional. So in version 1.15.4 this resulted in $qty being 1 but since the item is double we pass this code twice and the calculation thinks there are 2 simples in the order. In version 1.15.7 $qty is 2, but again the order item is double so the calculation thinks that there are 4 simples in the order which is incorrect.

Despite that 1.15.4 produces correct results I feel that it is not really correct to loop over the same order item twice. The proposed fix: dont double the order items when deciding which order items should be passed to the fitsAsBuspakje method

$shipmentItems = $this->getShipment()
            ->getItemsCollection();

        /**
         * @var Mage_Sales_Model_Order_Shipment_Item $item
         */
        $orderItems = array();
        foreach ($shipmentItems as $item) {
            $orderItem = $item->getOrderItem();

            if ($childrenOrderItems = $orderItem->getChildrenItems()) {
                /** @var Mage_Sales_Model_Order_Item $childItem */
                foreach ($childrenOrderItems as $childItem) {
                    $orderItems[$childItem->getId()] = $childItem;
                }
                continue;
            }

            $orderItems[$orderItem->getId()] = $orderItem;
        }

In this case $orderItems has unique values.

My test case is an order with a composite product type consisting of 2 simples (the same simples).
After the shipment has been created I tested with the following code

<?php
require_once __DIR__ . '/../app/Mage.php';
Mage::app('admin');

$shipment = Mage::getModel('sales/order_shipment')->load(3);
$postnlShipment = Mage::getModel('postnl_core/shipment')->setShipment($shipment)->save();

If I am doing something wrong or there is something wrong with my assumptions let me know and I will close the issue.

With kind regards

[tig-dennisvanderhammen]

Hi @edwinljacobs,

To test this issue, within the sample data, I've changed the "Plaid Cotton Shirt" and every child of it to a buspakje count of 3. I've added 2 red plaid shirts and 1 grey plaid shirt to my cart.

There has indeed been some changes with how buspakje is calculated. You state that the code loops over the order items twice, although in the PostNL/Helper/Data.php on line 1440 there is a check to see if a product type is a simple product. This makes sure in this method that the configurable's quantity is not included in the calculation.
When I place a debug point under the foreach loop, the totalQty equals 3, thus the totalQuantityRatio equals 1 (which would mean it fits in a buspakje).

I do notice the code checking if the products fits as a buspakje several times while placing an order, but every time the values start at zero so this doesn't seem to be an issue.

Whenever I create a shipment based on the previously created order, within the _getIsBuspakje, the orderItem->getChildrenItems() only returns one for each shipmentItem, resulting in sending 2 simple products towards fitsAsBuspakje(). These also have a totalQuantityRatio of 1 (or 3 totalQty).

Am I doing something different from you trying to reproduce your issue?

[tig-dennisvanderhammen]

Hi @edwinljacobs,

Are you still having issues with the buspakje calculations?

[edwinljacobs]

Hi, sorry for the late / non existing response.

I fixed the issue in an extending plugin and since that project contains a lot of customer specific code it could very well be that the issue originates there. Since no one else responded to this issue ill close this for now.

With kind regards