kylekatarnls / business-time

Carbon mixin to handle business days and opening hours
MIT License
296 stars 14 forks source link

currentOrNextBusinessDay() & isBusinessDay() not working correctly when used on weekends despite opening hours specified for weekends #68

Closed ziming closed 5 months ago

ziming commented 5 months ago

Example code:


// $schedule object with opening hours specified for both saturday & sunday

$today = Carbon::today(); // Saturday

$schedule->currentOrNextBusinessDay($today); // return monday

$schedule->isOpenOn($today, 'saturday'); // true

$schedule->isBusinessDay($today); // false

$today->isSaturday() && $schedule->isOpenOn($today, 'saturday'); // true
kylekatarnls commented 5 months ago

Carbon::today() return Saturday 00:00 (midnight), is it open at Saturday midnight? If no, then it's working as intended.

If you still have an issue, please provide a $schedule example to reproduce it.

ziming commented 5 months ago

If Carbon::today() is weekday, this is not a problem. even if opening hours is say 9 am

// return monday even if there are opening hours on saturday or sunday
$schedule->currentOrNextBusinessDay($saturdayMidnight); 

// But if it is monday
$today = Carbon::today(); // monday 0:00
$schedule->currentOrNextBusinessDay($today); // return monday

The same for isBusinessDay(), if it is weekdays, it return true if there are opening hours later at 9 am, even though $today is monday midnight. But if it is weekends, it will return false even if business hours are specified for the weekends

ziming commented 5 months ago

U can give this $schedule example a try

[
            'monday' => ['10:00-12:00', '14:00-16:30'],
            'tuesday' => ['10:00-12:00', '14:00-16:30'],
            'wednesday' => ['10:00-12:00', '14:00-16:30'],
            'thursday' => ['10:00-12:00', '14:00-16:30'],
            'friday' => ['10:00-12:00', '14:00-16:30'],
            'saturday' => ['10:00-12:00'],
            'sunday' => ['14:00-16:30'], // u can try comment out sunday too
            'holidaysAreClosed' => true,
            'holidays' => [
                'region' => 'sg',
            ],
]
kylekatarnls commented 5 months ago

Hello, sorry I didn't understood the question, the method you're looking is currentOrNextBusinessOpen:

$schedule->currentOrNextBusinessOpen($saturdayMidnight)

The method currentOrNextBusinessDay is from the package business-day

Which only cares about weekday+holiday and is not aware of the weekly opening-hours defined.

I guess you discovered this method via the auto-completion of your IDE, but basically only the methods listed in the README are aware of the schedule: https://github.com/kylekatarnls/business-time?tab=readme-ov-file#isopenon

ziming commented 5 months ago

thank you, what about the business time equivalent of isBusinessDay() ?

ziming commented 5 months ago

$schedule->isOpenOn($date) give me error, it need a 2nd argument

https://github.com/kylekatarnls/business-time?tab=readme-ov-file#isopenon

kylekatarnls commented 5 months ago

At the moment it needs to be written:

$schedule->isOpenOn($today, $today->dayName);

(First parameter is actually ignored but it's a constraint on Schedule to have a Carbon (or CarbonImmutable) instance as first parameter for any method to be used as reference for the current "now" moment.

For isOpenOn and isClosedOn, we don't need a reference for "now", so I'll make it optional for the next version.

ziming commented 5 months ago

so if $today is a public holiday, isOpenOn will return true because that day name has opening hours?

kylekatarnls commented 5 months ago

Yes, isOpenOn only check the weekly schedule by day name when given a day name, isOpen that and exact date-and-time is open, so it will account for the holidays in the country/region you selected if you set holidaysAreClosed to true.

kylekatarnls commented 5 months ago

You also have getCurrentDayOpeningHours() that give you an array of all opening hours of a given day, and it will return an empty array in case of closed holiday.

ziming commented 5 months ago

Yes, isOpenOn only check the weekly schedule by day name

But on the business time readme.md I saw this

CleanShot 2024-03-26 at 23 09 05@2x

So I can't pass in a date for isOpenOn and have it check if that date is open or not (and taking into account of closed on holidays?)

Update: I just tried $schedule->isOpenOn('2024-03-29') === false

and I got error Method isHoliday does not exist.

thank you

kylekatarnls commented 5 months ago

Sorry, I fixed my answer isOpenOn() only checks the weekly schedule if given a day name, if given a year-month-day date, it will also check holidays and then it should basically match the results of getCurrentDayOpeningHours() !== []

kylekatarnls commented 5 months ago

I reproduced this error Method isHoliday does not exist, this is an issue with Schedule in particular, I will fix that.

ziming commented 5 months ago

Thank you. I look forward to the fix

kylekatarnls commented 5 months ago

It should now be fixed in 1.13.1

ziming commented 5 months ago

Hi, i went to test it again & I spotted a new issue.

> now()->addDays(2)->isBusinessDay(); // return false since it is good friday 29 march 2024

> $schedule->isOpenOn(now()->addDays(2)->format('Y-m-d')); // return true even though it is good friday 29 march 2024
kylekatarnls commented 5 months ago

I can't reproduce:

$schedule = Schedule::create([
    'monday' => ['10:00-12:00', '14:00-16:30'],
    'tuesday' => ['10:00-12:00', '14:00-16:30'],
    'wednesday' => ['10:00-12:00', '14:00-16:30'],
    'thursday' => ['10:00-12:00', '14:00-16:30'],
    'friday' => ['10:00-12:00', '14:00-16:30'],
    'saturday' => ['10:00-12:00'],
    'sunday' => ['14:00-16:30'], // u can try comment out sunday too
    'holidaysAreClosed' => true,
    'holidays' => [
        'region' => 'sg',
    ],
]);

var_dump($schedule->isOpenOn('2024-03-29'));

It's giving me false