v1.15.7 Next Tuesday issue in latest version for buspakje

[seansan]

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]

Hi Sean,

Thank you for pointing this out. After going through Github issue https://github.com/tig-nl/postnl-magento1/issues/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]

OK thanks

do know tje comment can be found in multiple places

On Tue, Dec 18, 2018 at 1:06 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

Thank you for pointing this out. After going through Github issue #34 https://github.com/tig-nl/postnl-magento1/issues/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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/36#issuecomment-448198742, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a0rZ7m3SeWaVBiywhv1C0tXIK5dGks5u6NpegaJpZM4ZWrJ_ .

[tig-dennisvanderhammen]

Hi Sean,

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

[seansan]

As advised in other ticket I would solve it in the getter

Or create a setter with logic for the next first Tuesday

Also: I am not sure you are the experts ... but is there not a logic error? Now you check for a buspakje date and if the expected delivery = SUN or MON then you return false .... should the solution not be to just: move the date to Tuesday?

I mean shipping on say saturday could result in a deliverydate of SUN or MON .... but now the logic reads: do not send it as buspakje

Whilst you do send it as buspakje, only the expected delivery date is not SUN or MON but rather TUE

On Tue, Dec 18, 2018 at 2:53 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/36#issuecomment-448228078, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a4rOfyGnEbnlIHAa8GTBn_ECsHBXks5u6PNrgaJpZM4ZWrJ_ .

[tig-dennisvanderhammen]

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]

OK thanks

I think the setter is the best location (and we solved it like this)

Issue #35 is about 2 different timezones not the samen issue as in this ticket

On Tue, Dec 18, 2018 at 3:48 PM Dennis van der Hammen < notifications@github.com> wrote:

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 35 to keep communication with their respective issue?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/36#issuecomment-448245208, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0ay8pF4QLlABn82rlGe0BnxBqW6BIks5u6QAogaJpZM4ZWrJ_ .

[tig-dennisvanderhammen]

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