Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove check for startTime < currentTime #3593

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

bytrangle
Copy link
Contributor

Description

This PR removes the check for task start time < current time in the parseScheduleDate function.

With this condition, if an user schedules a task with a time less than current time, the day of the task will be set to the next day.

} else if (start.date().getTime() < now.getTime()) {
        plannedAt = start.date().setDate(start.date().getDate() + 1);
}

There are two holes with this logic:

  • For example, the current time is 2024-10-17T14:37. If a user schedules the task as 2024-10-18T13:37, the date will be parsed as 2024-10-19T13:37 since 13:37 < 14:37
  • There is a test in short-syntax.spec.ts that uses this condition. The tested time is 13:37. It will fail if anyone runs the test after 13:37.
it('should parse scheduled date using local time zone when unspecified', () => {
      const t = {
        ...TASK,
        title: '@2024-10-12T13:37',
      };
      const plannedTimestamp = getPlannedDateTimestampFromShortSyntaxReturnValue(t);
      expect(checkIfCorrectDateAndTime(plannedTimestamp, 'saturday', 13, 37)).toBeTrue();
    });

There is no need to check for schedule time < current time anyway. The date parser is configured as forwardDate: true and the reference date is now (new Date()). So any input date will be parsed as forward into the future.

Issues Resolved

Relates to #3192

Check List

  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@johannesjo
Copy link
Owner

Thank you!

@johannesjo johannesjo merged commit 79c5aa5 into johannesjo:master Oct 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants