postnl/postnl-magento1-End-of-life

v1.15.7 Next Tuesday issue in latest version for buspakje

seansan opened this issue · 7 comments

seansan commented

Sorry but do think this is another issue?

Code comments states Letter box parcels cannot be delivered on mondays or tuesdays.

Then a little further in the code one check for day of the week 0 or 1 (is this not Sunday and MOnday?)

And then a little further one sets the delivery date to the next Tuesday

Should the comment maybe mean Letter box parcels cannot be delivered on Sundays and Mondays.

/**
 * Check if the current confirm and delivery dates are valid for letter box parcel shipments. If not, modify these
 * dates accordingly.
 *
 * @return $this
 */
protected function _checkBuspakjeDates()
{
    /**
     * Get the current delivery date.
     */
    $deliveryDate = $this->getDeliveryDate();
    $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $deliveryDate, new DateTimeZone('UTC'));

    /**
     * Letter box parcels cannot be delivered on mondays or tuesdays.
     */
    if ($deliveryDate->format('N') === '0' || $deliveryDate->format('N') == '1') {
        /** @var TIG_PostNL_Helper_Date $helper */
        $helper = $this->getHelper('date');

        /**
         * Modify the delivery date to the next tuesday.
         */
        $deliveryDate->modify('next tuesday ' . $deliveryDate->format('H:i:s'));

        $this->setDeliveryDate($deliveryDate->format('Y-m-d H:i:s'));

        /**
         * Also modify the confirm date accordingly.
         */
        $confirmDate = $helper->getShippingDateFromDeliveryDate($deliveryDate, $this->getStoreId());

        $this->setConfirmDate($confirmDate->format('Y-m-d H:i:s'));
    }

    /**
     * Letter box parcels have no expected delivery times.
     */
    $this->setExpectedDeliveryTimeStart(null)
         ->setExpectedDeliveryTimeEnd(null);

    return $this;
}
tig-dennisvanderhammen commented

Hi Sean,

Thank you for pointing this out. After going through Github issue #34, I noticed this as well.

You are right, the PHP docs should be "cannot be delivered on Sunday or Monday". I have since changed it in our private repository.

seansan commented
tig-dennisvanderhammen commented

Hi Sean,

Yes, I've found two seperate locations where the monday and tuesday is mentioned, and fixed them both.

seansan commented
tig-dennisvanderhammen commented

The deliveryDate is not only used for the buspakje check, there are many places where the getDeliveryDate() method is used. We don't want unexpected behaviour in other locations, that's why I decided to fix it in this way. Although having a boolean in the setter would not be a bad option either.

As for your question regarding to moving the date to Tuesday, this actually happens in a different part of the extension. Your initially reported error most likely originates from the method _extractProductCodeForType() in the Model/Core/Shipment.php.

Here we do a check, "elseif ($codes['is_buspakje'] == '-1'.) { $isBuspakje = $this->_getIsBuspakje(); }"
This might return a false. Later in the code you'll see the check if ($isBuspakje).

If we return back to the elseif-statement, we'll also see another if-statement, "if ($codes['is_buspakje'] == '1') { $isBupakje = true; }

In the later if ($isBuspakje) statement, you'll see a reference to the method $this->_checkBuspakjeDates(). In this method the deliveryDate actually does get changed to Tuesday.

While I did create a fix for the initial issue, when I found this I've already opened it for discussion internally, whether this is still intended behaviour.

For future comments, could we please use issue 34 to keep communication with their respective issue?

seansan commented
tig-dennisvanderhammen commented

I ment #34, I've since changed my comment :)