wakirin / Litepicker

Date range picker - lightweight, no dependencies
MIT License
899 stars 132 forks source link

With specific conditions, rangeIsLocked() will wrongly return that it's locked, while it's not #242

Open er314 opened 3 years ago

er314 commented 3 years ago

Describe the bug Under some specific conditions, rangeIsLocked() will wrongly return that it's locked, while it's not (identified on v2.0.11)

To Reproduce For example, let's say that : dateRange = [['2021-04-06','2021-04-08']] lockDays = [['2021-04-08','2021-04-10']] lockDaysInclusivity = "(]"

Expected behavior In the above example, rangeIsLocked() should return that the dateRange is NOT locked : with regards to the lockDays, the day '2021-04-08' is actually not locked, due to the inclusivity settings "(".

Actual behavior In the above example, rangeIsLocked() returns that the dateRange is locked.

Additional context Below is my understanding of what the problem is, and a proposed solution. Basically, rangeIsLocked() is doing these calls :

    lockDayStart.isBetween(day1, day2)
    lockDayEnd.isBetween(day1, day2)

But it should be doing the reverse :

    day1.isBetween(lockDayStart, lockDayEnd)
    day2.isBetween(lockDayStart, lockDayEnd)

Note that by comparison, dateIsLocked() is doing the correct call, in the "normal logic" :

    day.isBetween(lockDayStart, lockDayEnd)

I understand the rationale for using this "reverse logic" in rangeIsLocked() ; but nevertheless, this has a flaw. Why does the "reverse logic" vs "normal logic" order matter ? Because when lockDaysInclusivity is not default, the result obtained via "reverse logic" is NOT the same as the result obtained via "normal logic". Because lockDaysInclusivity is ONLY meant to be applied on lockDays ; not on the input date range !

Back to the example : dateRange = [['2021-04-06','2021-04-08']] lockDays = [['2021-04-08','2021-04-10']] lockDaysInclusivity = "(]"

. The "normal logic" is comparing the dateRange towards the lockDays -> result = is not locked : the day '2021-04-08' is a start date with regards to the lockDays, therefore it is actually NOT locked, due to the inclusivity settings "(". -> this is the expected result

. But the "reverse logic" is comparing the lockDays towards the dateRange -> result = is locked : the day '2021-04-08' is an end date with regards to the dateRange, therefore it is locked, due to the inclusivity setting "]". -> this is a wrong result, because the inclusivity setting is ONLY meant to be applied on lockDays.

Conclusion : Method rangeIsLocked() shall be fixed.

2 possibilities :

  1. Add a new method isBetweenReverse() , to be called by rangeIsLocked, in place of isBetween() ; this new method would implement the "reverse logic" down to the isBetween processing. -> I'm not convinced by this possibility or,
  2. Update rangeIsLocked() code, in order to make it behave in the "normal logic". -> note that by "removing" the "reverse logic", we must also change the triggering condition, so that it obbeys to following algorithm :
    isLocked IF
        startDate isBetween lockDays
            or
        endDate isBetween lockDays
            or
        startDate !isBetween lockdays AND endDate !isBetween lockDays AND startDate isBefore lockDays[end] AND endDate isAfter lockDays[start]  // = the date range is a superset of the lockDays

    -> here is the updated code I come up with :

export function rangeIsLocked(days: DateTime[], options): boolean {
  let isLocked = false;

  if (options.lockDays.length) {
    let lockDayStart; let lockDayEnd; let isStartInBetween; let isEndInBetween;
    isLocked = options.lockDays
      .filter((d) => {
        if (d instanceof Array) {
          lockDayStart = d[0];
          lockDayEnd = d[1];
    } else {
          lockDayStart = d;
          lockDayEnd = d;           
    }
        isStartInBetween = days[0].isBetween(lockDayStart, lockDayEnd, options.lockDaysInclusivity);
        isEndInBetween = days[1].isBetween(lockDayStart, lockDayEnd, options.lockDaysInclusivity);
        return isStartInBetween
            || isEndInBetween
            || (!isStartInBetween && !isEndInBetween && days[0].isBefore(lockDayEnd) && days[1].isAfter(lockDayStart));
      }).length;
  }

  if (!isLocked && typeof options.lockDaysFilter === 'function') {
    isLocked = options.lockDaysFilter.call(this, days[0].clone(), days[1].clone(), days);
  }

  return isLocked;
}

I don't have expertise / "the full picture" on litepicker, so this code proposal may have problems. That being said, it works very well within my testing cases - see #233 / latest comment for the full context.