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

Does businessDiff treat Sat and Sun as if it's Monday? #74

Closed anton6 closed 5 years ago

anton6 commented 5 years ago

I want to show some records to the user if businessDiff is <= 2 days

It works fine if current date is Monday and Tuesday, but for Wednesday I got a surprising result.

If I have some records with timestamp Saturday and Sunday and today is Wednesday it shows business diff as 2 days (same as if the records are from Monday).

I was expecting that the difference between Saturday/Sunday and Wednesday to be 3 (i.e. treating Saturday and Sunday as Friday).

Can someone confirm if this is expected behaviour for businessDiff and how I can achieve my expected behaviour with businessDiff? i.e. not to display records from Sat/Sun if today is Wednesday.

mcdado commented 5 years ago

Can you share your setup? Specifically when you run updateLocale to set business days and any other possibile holiday. Can you also please share the piece of code you use to calculate the diff? That would be very helpful to diagnose the issue. :)

anton6 commented 5 years ago

Thanks for quick response. Really appreciate your time with this package.

These are my dependencies:

"moment": "^2.22.2", "moment-business-days": "^1.1.3",

I am actually not running updateLocale at all, as I only care about Saturday and Sunday at this stage. I assume that for Saturday and Sunday I don't need to do this step. Or do I?

So I am only doing something like this to recreate the issue:

import moment from 'moment-business-days';

describe('moment-business-days', () => {
  context('businessDiff', () => {
    it('should return correct diff for Wednesday vs Sunday', () => {
      // Mon Jan 01 2018 18:00:00 GMT+0000 (Greenwich Mean Time)
      const SUNDAY = moment('2018-01-07T18:00:00Z');
      const NEXT_WEDNESDAY = moment('2018-01-10T18:00:00Z');
      const businessDiff = NEXT_WEDNESDAY.businessDiff(SUNDAY);
      expect(businessDiff).to.equal(3);
    });
  });
});

1 failing

1) moment-business-days businessDiff should return correct diff for Wednesday vs Sunday:

  AssertionError: expected 2 to equal 3
  + expected - actual

  -2
  +3

Should the difference be 2 or 3 for the above test? I was expecting the diff to be 3, because at least in my case I only want to show records for Monday and Tuesday, but because of the above result it also grabs the weekend records.

mcdado commented 5 years ago

So the negative value is expected since the change in https://github.com/kalmecak/moment-business-days/pull/54

The way businessDiff works is that it counts the working days between two dates. So if you have a date on Wednesday and a date on Sunday, you would count 2 because you have two working days between: Tuesday and Monday. Same for Saturday, the working days are between these days are 2.

Instead if you would subtract 3 days from Wednesday, if would end up on Friday. This is a difference between businessDiff on one side and businessAdd and businessSubtract on the other.