eduolalo / moment-business-days

This is a momentJS plugin that allows you to use only business days (Monday to Friday)
MIT License
240 stars 67 forks source link

businessAdd doesn't work correctly all the time #86

Closed baermathias closed 1 year ago

baermathias commented 4 years ago

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())
mcdado commented 4 years ago

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.

baermathias commented 4 years ago

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 commented 4 years ago

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

dadiyanidhi commented 4 years ago

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.

baermathias commented 4 years ago

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?

mcdado commented 3 years ago

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.

gfb107 commented 1 year ago

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;
}
mcdado commented 1 year ago

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.

gfb107 commented 1 year ago

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.