moment-business-days icon indicating copy to clipboard operation
moment-business-days copied to clipboard

businessAdd doesn't work correctly all the time

Open baermathias opened this issue 4 years ago • 6 comments

The aim of this application is always to indicate the 16th business day of each month (means taking into account working days and public holidays). Some of the dates are correct and some are not, here is my actual result and what is the target if the dates were calculates correctly:

Month  Actual  Target  Correct
Jan    22      22      yes
Feb    24      21      no
March  23      20      no
April  22      22      yes
May    22      22      yes
June   25      26      no
July   22      22      yes
Aug    21      24      no

This is the code I use (it is also available as jsfiddle):

myHolidays = [
  { holidayDate: "2020-06-01", description: "holiday 1" },
  { holidayDate: "2020-06-02", description: "holiday 2" },
  { holidayDate: "2020-06-03", description: "holiday 3" },  
  { holidayDate: "2020-06-06", description: "weekend saturday" },
  { holidayDate: "2020-06-07", description: "weekend sunday" },
  { holidayDate: "2020-06-11", description: "holiday 6" },
  { holidayDate: "2020-06-13", description: "weekend saturday" },
  { holidayDate: "2020-06-14", description: "weekend sunday" },
  { holidayDate: "2020-06-20", description: "weekend saturday" },
  { holidayDate: "2020-06-21", description: "weekend sunday" },
  { holidayDate: "2020-06-27", description: "weekend saturday" },
  { holidayDate: "2020-06-28", description: "weekend sunday" },
];

moment.updateLocale('de', {
   holidays: myHolidays.map(i => i.holidayDate),
   holidayFormat: 'YYYY-MM-DD'
});

var startDate = moment("2020-01-01").startOf('month')
var endDate = moment("2020-12-01")
var eachMonthWith16 = [];

function generate16InEachMonth() {
  let monthsList = []
  // loop by months
  while (endDate > startDate) {
    monthsList.push(new Date(moment(this.startDate).businessAdd(15, 'days')))
    startDate = startDate.add(1, "month").startOf('month')
  }
  return monthsList
}

console.log(generate16InEachMonth())

baermathias avatar Jul 21 '20 08:07 baermathias

At a quick glance I'm guessing this has to do with months starting on a weekend or holiday. The counting should start from the first working day, right? For example in June 2020 should be on the 4th, then continue 5th, 8th and so on... i would need to step through a debugger but I suspect the first step is also counted.

mcdado avatar Jul 21 '20 09:07 mcdado

Correct, it should start counting from the 4th in the June example. I have visualized the days:

The red marked days are holidays and the pink marked day should be the 16th business day of June, which is the 26th June taking in account the given holidays.

Unbenannt

If I use businessAdd(16, 'days'), then June is calculated correctly, but you get problems with other months, e.g. July should then be on 22, but is then 23.

baermathias avatar Jul 22 '20 06:07 baermathias

@mcdado Could you already find out what causes this issue?

baermathias avatar Jul 29 '20 07:07 baermathias

I created a demo sandbox . for my case the june's date is coming wrong. rest works fine. Although i know why it happened as i added 15 businessdays instead of 16.

dadiyanidhi avatar Aug 25 '20 12:08 dadiyanidhi

The reason the code wouldn't work for all months is probably that the first day of the month is counted as a business day by default! This is a tricky thing and should be mentioned in the docs, so developers can handle it e.g. like this: var daysToAdd = (startDate.isBusinessDay()) ? 15 : 16 Or it should work differently by default. What do you think?

baermathias avatar Sep 08 '20 07:09 baermathias

Sorry for answering only now. Could you please specify a runnable test with expected results? So it is clearer what should happen and what actually happens, and solve the problem.

mcdado avatar Oct 10 '20 02:10 mcdado

I know this is old, but IMO the bug is in the application, not the library. You need to make sure you're adding 15 business days to the first business day of the month, rather than to the first day of the month.

startDate.businessAdd( 15 ), should (and does) return the 15th business day AFTER startDate, it does not matter that startDate is a business day or not.

function generate16InEachMonth() {
  let monthsList = [];
  // loop by months
  while (endDate > startDate) {
    const firstDay = moment( startDate );
    if ( !firstDay.isBusinessDay()) {
        firstDay.nextBusinessDay();
    }
    monthsList.push( new Date( firstDay.businessAdd( 15 )));
    startDate.add( 1, "month" );
  }
  return monthsList;
}

gfb107 avatar Dec 19 '22 18:12 gfb107

I know this is old, but IMO the bug is in the application, not the library. You need to make sure you're adding 15 business days to the first business day of the month, rather than to the first day of the month.

Thank you for your feedback, if I understand correctly you feel that this is an expected behaviour from moment-business-days, and one should be aware of it? In that case I could add some gotcha disclaimer in the README.

mcdado avatar Dec 20 '22 10:12 mcdado

Yes, I feel this is expected behavior. If you want to add something to the README, that is fine, but I don't see the need.

I think of calling businessAdd( 15 ) as equivalent to calling nextBusinessDay() 15 times . nextBusinessDay() will always return a value at least 1 day after the original moment. It will never return the original moment. It does not (and should not) matter if the original moment is a business day or not.

So businessAdd(15) also does not (and should not) behave differently when the original moment is business day or not.

gfb107 avatar Dec 20 '22 14:12 gfb107