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

fix: Improve next-date calculation for monthly and yearly recurring tasks #1197

Merged
merged 31 commits into from
Oct 8, 2022

Conversation

schemar
Copy link
Collaborator

@schemar schemar commented Sep 30, 2022

When a recurrence rule states "every month" and the next month does not have the day of the month that the task is scheduled, that month is skipped and the next valid month is used instead.

For example: When I mark a task that recurs "every month" done on January 31, the next occurrence will be scheduled on March 31, since February does not have 31 days. This is in line with the rrule specification, but not what users expect. This can be triggered by accident, for example when the task is based on the done date and the done date is on the 31st.

@claremacrae architected a fix that I implemented with this change.

To fix the issue, the number of expected skipped months is parsed (defaults to one). While the actualy number of skipped months is higher, the base date is adjusted one day into the past. In practice, this means that the example from above would no schedule the recurring task on the 28th of February (or the 29th for a leap year).

The same applies for yearly recurrences, for example based on the 29th of February.

⚠️ This is currently still a proof of concept and is still missing:

edited to turn the list in to checkboxes..

  • Documentation in code
  • End user documentation to explain the behavior
  • Extensive testing to ensure this change does not introduce regressions

Fixes #614, #1087

@schemar schemar added type: bug Something isn't working scope: recurrence Anything to do with recurring/repeating tasks labels Sep 30, 2022
@schemar schemar requested a review from claremacrae September 30, 2022 10:40
When a recurrence rule states "every month" and the next month does not
have the day of the month that the task is scheduled, that month is
skipped and the next valid month is used instead.

For example: When I mark a task that recurs "every month" done on
January 31, the next occurrence will be scheduled on March 31, since
February does not have 31 days. This is in line with the rrule
specification, but not what users expect. This can be triggered by
accident, for example when the task is based on the done date and the
done date is on the 31st.

@claremacrae architected a fix that I implemented with this change.

To fix the issue, the number of expected skipped months is parsed
(defaults to one).
While the actualy number of skipped months is higher, the base date is
adjusted one day into the past. In practice, this means that the example
from above would no schedule the recurring task on the 28th of
February (or the 29th for a leap year).

The same applies for yearly recurrences, for example based on the 29th
of February.

This is a proof of concept and is still missing:

1. Documentation in code
2. End user documentation to explain the behavior
3. Extensive testing to ensure this change does not introduce regressions

Fixes #614, #1087
@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

Thank you @claremacrae for coming up with this fix and letting me know how it would work! Please check whether the implementation matches your ideas.

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

One thing that's not ideal is the change to the after instance (using the existing Moment methods). It is not transparent at all, that after changes through the various function invocations.

@claremacrae
Copy link
Collaborator

Extensive testing to ensure this change does not introduce regressions

By intentionally breaking the recurrence code, I found that in addition to tests/Recurrence.test.ts, there are existing tests of recurring in:

  • tests/Commands/ToggleDone.test.ts- it's actually testing the cursor position after completing recurring tasks

it('should complete a recurring task', () => {
testToggleLine(
'- [ ] I am a recurring task| 🔁 every day 📅 2022-09-04',
`- [ ] I am a recurring task 🔁 every day 📅 2022-09-05
- [x] I am a recurring task| 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
);
// With a trailing space at the end of the initial line, which is deleted
// when the task lines are regenerated, the cursor moves one character to the left:
testToggleLine(
'- [ ] I am a recurring task| 🔁 every day 📅 2022-09-04 ',
`- [ ] I am a recurring task 🔁 every day 📅 2022-09-05
- [x] I am a recurring tas|k 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
);
});

  • tests/Task.test.ts - lots of them:

type RecurrenceCase = {
interval: string;
due?: string;
scheduled?: string;
start?: string;
today?: string;
nextDue?: string;
nextScheduled?: string;
nextStart?: string;
};
const recurrenceCases: Array<RecurrenceCase> = [
{
interval: 'every 7 days',
due: '2021-02-21',
nextDue: '2021-02-28',
},
{
interval: 'every 7 days when done',
due: '2021-02-21',
today: '2021-02-18',
nextDue: '2021-02-25',
},
{
interval: 'every 7 days when done',
due: '2021-02-21',
today: '2021-02-22',
nextDue: '2021-03-01',
},
{
interval: 'every day',
due: '2021-02-21',
nextDue: '2021-02-22',
},
{
interval: 'every day when done',
due: '2021-02-21',
today: '2022-01-04',
nextDue: '2022-01-05',
},
{
interval: 'every 4 weeks',
due: '2021-10-15',
nextDue: '2021-11-12',
},
{
interval: 'every 4 weeks',
due: '2021-10-12',
nextDue: '2021-11-09',
},
{
interval: 'every 4 weeks',
due: '2022-10-12',
nextDue: '2022-11-09',
},
{
interval: 'every 4 weeks',
due: '2033-10-12',
nextDue: '2033-11-09',
},
{
interval: 'every 3 weeks when done',
due: '2022-01-10',
today: '2021-01-14',
nextDue: '2021-02-04',
},
{
interval: 'every month',
due: '2021-10-15',
nextDue: '2021-11-15',
},
{
interval: 'every month',
due: '2021-10-18',
nextDue: '2021-11-18',
},
{
interval: 'every month when done',
due: '2021-10-18',
today: '2021-10-16',
nextDue: '2021-11-16',
},
{
interval: 'every month when done',
due: '2021-10-18',
today: '2021-10-24',
nextDue: '2021-11-24',
},
{
interval: 'every month on the 2nd Wednesday',
due: '2021-09-08',
nextDue: '2021-10-13',
},
{
interval: 'every month on the 2nd Wednesday when done',
due: '2021-09-08',
today: '2021-09-15',
nextDue: '2021-10-13',
},
{
interval: 'every month on the 2nd Wednesday when done',
due: '2021-09-08',
today: '2021-10-12',
nextDue: '2021-10-13',
},
{
interval: 'every month on the 2nd Wednesday when done',
due: '2021-09-08',
today: '2021-10-13',
nextDue: '2021-11-10',
},
{
interval: 'every 3 months on the 3rd Thursday',
due: '2021-04-15',
nextDue: '2021-07-15',
},
{
interval: 'every 3 months on the 3rd Thursday',
due: '2021-08-19',
nextDue: '2021-11-18',
},
{
interval: 'every 3 months on the 3rd Thursday',
due: '2021-09-16',
nextDue: '2021-12-16',
},
{
interval: 'every week on Monday, Wednesday, Friday',
due: '2022-01-24',
nextDue: '2022-01-26',
},
{
interval: 'every week on Monday, Wednesday, Friday when done',
due: '2022-01-24',
today: '2022-01-25',
nextDue: '2022-01-26',
},
{
interval: 'every week on Monday, Wednesday, Friday when done',
due: '2022-01-24',
today: '2022-01-26',
nextDue: '2022-01-28',
},
{
interval: 'every 5 days',
start: '2021-10-10',
nextStart: '2021-10-15',
},
{
interval: 'every 5 days',
scheduled: '2021-10-10',
nextScheduled: '2021-10-15',
},
{
interval: 'every 5 days',
start: '2021-10-10',
due: '2021-10-11',
nextStart: '2021-10-15',
nextDue: '2021-10-16',
},
{
interval: 'every 5 days',
scheduled: '2021-10-10',
due: '2021-10-11',
nextScheduled: '2021-10-15',
nextDue: '2021-10-16',
},
{
interval: 'every 5 days',
start: '2021-10-09',
scheduled: '2021-10-10',
due: '2021-10-11',
nextStart: '2021-10-14',
nextScheduled: '2021-10-15',
nextDue: '2021-10-16',
},
{
interval: 'every 10 days when done',
start: '2021-10-05',
scheduled: '2021-10-07',
due: '2021-10-09',
today: '2021-10-04',
nextStart: '2021-10-10',
nextScheduled: '2021-10-12',
nextDue: '2021-10-14',
},
{
interval: 'every 10 days when done',
start: '2021-10-05',
scheduled: '2021-10-07',
due: '2021-10-09',
today: '2021-10-06',
nextStart: '2021-10-12',
nextScheduled: '2021-10-14',
nextDue: '2021-10-16',
},
{
interval: 'every 10 days when done',
start: '2021-10-05',
scheduled: '2021-10-07',
due: '2021-10-09',
today: '2021-10-08',
nextStart: '2021-10-14',
nextScheduled: '2021-10-16',
nextDue: '2021-10-18',
},
{
interval: 'every 10 days when done',
start: '2021-10-05',
scheduled: '2021-10-07',
due: '2021-10-09',
today: '2021-10-10',
nextStart: '2021-10-16',
nextScheduled: '2021-10-18',
nextDue: '2021-10-20',
},
];
test.concurrent.each<RecurrenceCase>(recurrenceCases)(
'recurs correctly (%j)',
({ interval, due, scheduled, start, today, nextDue, nextScheduled, nextStart }) => {
const todaySpy = jest.spyOn(Date, 'now').mockReturnValue(moment(today).valueOf());
const line = [
'- [ ] I am task',
`🔁 ${interval}`,
!!scheduled && `⏳ ${scheduled}`,
!!due && `📅 ${due}`,
!!start && `🛫 ${start}`,
]
.filter(Boolean)
.join(' ');
const task = fromLine({
line,
});
const nextTask: Task = task!.toggle()[0];
expect({
nextDue: nextTask.dueDate?.format('YYYY-MM-DD'),
nextScheduled: nextTask.scheduledDate?.format('YYYY-MM-DD'),
nextStart: nextTask.startDate?.format('YYYY-MM-DD'),
}).toMatchObject({
nextDue,
nextScheduled,
nextStart,
});
expect(nextTask.recurrence?.toText()).toBe(interval);
todaySpy.mockClear();
},
);
it('supports recurrence rule after a due date', () => {
// Arrange
const line = '- [ ] this is a task 🗓 2021-09-12 🔁 every day';
// Act
const task = fromLine({
line,
});
// Assert
expect(task).not.toBeNull();
expect(task!.dueDate).not.toBeNull();
expect(task!.dueDate!.isSame(moment('2021-09-12', 'YYYY-MM-DD'))).toStrictEqual(true);
const nextTask: Task = task!.toggle()[0];
expect({
nextDue: nextTask.dueDate?.format('YYYY-MM-DD'),
nextScheduled: nextTask.scheduledDate?.format('YYYY-MM-DD'),
nextStart: nextTask.startDate?.format('YYYY-MM-DD'),
}).toMatchObject({
nextDue: '2021-09-13',
nextScheduled: undefined,
nextStart: undefined,
});
});
});

@claremacrae
Copy link
Collaborator

One thing that's not ideal is the change to the after instance (using the existing Moment methods). It is not transparent at all, that after changes through the various function invocations.

I see what you mean.

Perhaps rename it to something like currentAfter? I thought of nextAfter but that's misleading too...

@claremacrae
Copy link
Collaborator

@schemar I grabbed my tests from #614 and added them to Task.test.ts, updating the comments.

They all pass, which is great.
Feel free to add them, or I can...

diff --git a/tests/Task.test.ts b/tests/Task.test.ts
index 9566b09..822260f 100644
--- a/tests/Task.test.ts
+++ b/tests/Task.test.ts
@@ -650,6 +650,26 @@ describe('toggle done', () => {
             nextScheduled: '2021-10-18',
             nextDue: '2021-10-20',
         },
+        {
+            // every month - due 31 March, and so 31 April would not exist: it used to skip forward 2 months
+            interval: 'every month',
+            due: '2021-03-31',
+            nextDue: '2021-04-30',
+        },
+        {
+            // every month - due 29 January, and so 29 February would not exist: it used to skip forward 2 months
+            interval: 'every month',
+            due: '2021-01-29',
+            nextDue: '2021-02-28',
+        },
+
+        // Testing yearly repetition around leap days
+        {
+            // yearly, due on leap day 29th February: it used to skip forward 4 years
+            interval: 'every year',
+            due: '2020-02-29',
+            nextDue: '2021-02-28',
+        },
     ];
 
     test.concurrent.each<RecurrenceCase>(recurrenceCases)(

@claremacrae
Copy link
Collaborator

@schemar Without comments I'm making slow progress on reading the code, so thought I would see if I could break it!

These added to Task.test.ts fail. Have I got my nextDue values correct?

        // Some boundary cases
        {
            interval: 'every 11 months', // should skip forward to February next year
            due: '2020-03-31',
            nextDue: '2021-02-28', // gives 2022-01-31
        },
        {
            interval: 'every 13 months', // should skip forward to February next year
            due: '2020-01-31',
            nextDue: '2021-02-28', // Gives 2022-03-31
        },

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

One thing that's not ideal is the change to the after instance (using the existing Moment methods). It is not transparent at all, that after changes through the various function invocations.

I see what you mean.

Perhaps rename it to something like currentAfter? I thought of nextAfter but that's misleading too...

For now I will add documentation to the code.

@schemar I grabbed my tests from #614 and added them to Task.test.ts, updating the comments.

They all pass, which is great. Feel free to add them, or I can...

I will add these three tests.

@schemar Without comments I'm making slow progress on reading the code, so thought I would see if I could break it!

These added to Task.test.ts fail. Have I got my nextDue values correct?

        // Some boundary cases
        {
            interval: 'every 11 months', // should skip forward to February next year
            due: '2020-03-31',
            nextDue: '2021-02-28', // gives 2022-01-31
        },
        {
            interval: 'every 13 months', // should skip forward to February next year
            due: '2020-01-31',
            nextDue: '2021-02-28', // Gives 2022-03-31
        },

Looks correct to me. Let me investigate.

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

@claremacrae there are comments in the code now, hope it helps.

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

@schemar Without comments I'm making slow progress on reading the code, so thought I would see if I could break it!

These added to Task.test.ts fail. Have I got my nextDue values correct?

        // Some boundary cases
        {
            interval: 'every 11 months', // should skip forward to February next year
            due: '2020-03-31',
            nextDue: '2021-02-28', // gives 2022-01-31
        },
        {
            interval: 'every 13 months', // should skip forward to February next year
            due: '2020-01-31',
            nextDue: '2021-02-28', // Gives 2022-03-31
        },

Thank you! I added the test cases to the Recurrence test file and fixed the bug.

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thank you @schemar. The comments made a huge difference.

Only minor comments...

src/Recurrence.ts Show resolved Hide resolved
src/Recurrence.ts Outdated Show resolved Hide resolved
src/Recurrence.ts Show resolved Hide resolved
tests/Recurrence.test.ts Show resolved Hide resolved
@claremacrae
Copy link
Collaborator

Thank you! I added the test cases to the Recurrence test file and fixed the bug.

The fix is really clear.

I have to go out for an errand - will try to think of extra corner cases for tests on my walk... Other than that, I think the code in this is good to go.

@claremacrae
Copy link
Collaborator

Is it worth jotting down the outline of what content might go in the user docs?
Did you have any ideas?

@claremacrae
Copy link
Collaborator

I've converted the list of 3 outstanding tasks in the description to checklists, and marked 2 as done - in the believe that the current tests in Task.test.ts are good enough.

@schemar schemar marked this pull request as ready for review September 30, 2022 12:39
@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

Thank you for all the very valid comments @claremacrae! Unfortunately I won't have more time to work on this until at least Thursday. Would be lovely if you could pick it up from here. If not, I will pick it up next week 👍

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

Is it worth jotting down the outline of what content might go in the user docs? Did you have any ideas?

I added a short warning. Feel free to change as desired!

@claremacrae
Copy link
Collaborator

Thank you for all the very valid comments @claremacrae! Unfortunately I won't have more time to work on this until at least Thursday. Would be lovely if you could pick it up from here. If not, I will pick it up next week 👍

Huge thanks @schemar!

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

I wonder if there are cases where the while loop is endless…

@claremacrae
Copy link
Collaborator

I wonder if there are cases where the while loop is endless…

I thought that. I will try to come up with a test to make it happen….

@claremacrae
Copy link
Collaborator

Also, how does it work with invalid dates in the recurrence rule and the task. Like the user writing 31 Sep, which I did recently.

@schemar
Copy link
Collaborator Author

schemar commented Sep 30, 2022

Also, how does it work with invalid dates in the recurrence rule and the task. Like the user writing 31 Sep, which I did recently.

No idea.

@claremacrae
Copy link
Collaborator

I've done a few more hours of testing, and some documentation.
I've pushed the changes.
I will add more info later, but there is a show-stopper issue that every month on the last gets stuck on the same date, if the new date is invalid. More info later...

@claremacrae
Copy link
Collaborator

I've done a few more hours of testing, and some documentation.
I've pushed the changes.
I will add more info later, but there is a show-stopper issue that every month on the last gets stuck on the same date, if the new date is invalid. More info later...

@schemar, Hi, below is the behaviour mentioned above.

Explanation of what's going on:

  • The new due date works fine on every occurrence until 2022-01-31.
  • The next occurrence should be 2022-02-28, but is 2022-01-31
  • At which point, it get stuck on that date forever more.
- [ ] #task do stuff 🔁 every month on the 31st 📅 2022-01-31
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2021-12-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2021-11-30 ✅ 2022-10-03

@aubreyz
Copy link
Contributor

aubreyz commented Oct 3, 2022

This may be an ignorant comment, but in the tasks dialog box the dates work fine. So what library is that using and could it use the same (and why does it differ?). For example if you type "one month after 2022-01-31" in the start field it yields 2022-02-28.

@claremacrae
Copy link
Collaborator

This may be an ignorant comment, but in the tasks dialog box the dates work fine. So what library is that using and could it use the same (and why does it differ?). For example if you type "one month after 2022-01-31" in the start field it yields 2022-02-28.

Oh wow, that's a really useful and interesting insight - thank you! Here's a bit more info...

  • The date-related libraries used by Tasks:
    • chrono: for the Add/edit dialog, and auto-complete's converting of plain text date descriptions to actual dates
    • rrule: for calculating new dates for recurring tasks
  • There have been Tasks bugs caused by behaviours in both libraries, prompting this suggestion: Improve date parsing - maybe replace chrono and/or rrule

Your comment has prompted a couple of ideas:

  • I was wanting to find a way to test many recurrence examples, and writing the tests by hand is tedious and time-consuming. Perhaps I could generate a large number of recurrence tests that do the calculations in both libraries, and report any differences
  • Better still, if we can map some recurrence rules written in rrules syntax to chrono syntax instead, perhaps we could rewrite some of the recurrence code based on chrono and get better results.

@claremacrae
Copy link
Collaborator

Things to consider when fixing the on the 31st case:

The following are all treated as the same thing:

every month on the 31st
every month on the 31th
every month on 31st
every month on 31th

(I may edit this comment with more cases)

@claremacrae
Copy link
Collaborator

claremacrae commented Oct 5, 2022

There is also this case to consider (taken from the rule tests):

every week on the 3rd, 10th, 17th and 24th

@schemar
Copy link
Collaborator Author

schemar commented Oct 5, 2022

Would ignoring all on the be on option? I considered it when I did the original implementation, but wasn't sure if it was needed. If the user explicitly wants recurrence on a certain day, then that should be honored in my opinion. What do you think?

@claremacrae
Copy link
Collaborator

Would ignoring all on the be on option? I considered it when I did the original implementation, but wasn't sure if it was needed. If the user explicitly wants recurrence on a certain day, then that should be honored in my opinion. What do you think?

Ooh, I like that.

I think if we ignored on and documented the limitation, that is probably the best we can do at this point.

(See about 2 or 3 comments back for the the being optional)

@dgreen
Copy link

dgreen commented Oct 5, 2022

I just tried Apple Calendar and BusyCal on the iPhone and they skip November when I schedule October 31st and monthly repeat. Monthly repeat in BusyCal is shown as Monthly on 31st after I save the entry. I found this discussion involving differing behavior of Outlook and Google (9 years ago post).

A trio of thoughts:

  1. I wonder if there is covered by standard like iCalendar.

  2. I generally want either last day of month or last business day of month. I wonder if this could be provide some day in addition to any solution chosen.

  3. it seems like another challenge present in this implementation is that while an original desire of say October 31st and monthly is chosen by the user, any implementation that respects monthly will modify the date and then executing the monthly reschedule won't know the original request.

@claremacrae
Copy link
Collaborator

I found this discussion involving differing behavior of Outlook and Google (9 years ago post).

Thanks @dgreen. Did you mean to add a link in the above text?

@dgreen
Copy link

dgreen commented Oct 6, 2022

I found this discussion involving differing behavior of Outlook and Google (9 years ago post).

Thanks @dgreen. Did you mean to add a link in the above text?

Thanks. I did. The link is https://ux.stackexchange.com/questions/36889/calendar-recurrences-on-the-last-day-of-the-month and I edited the pos.

@claremacrae claremacrae self-requested a review October 6, 2022 22:14
@claremacrae
Copy link
Collaborator

Hi @schemar I've fixed the below, reverting it back to the previous behaviour if on is in a monthly rule.

Note done: Are there any cases where on in yearly is problematical?

I've also added a major chunk to the docs on recurring tasks, explaining the 3 different main alternatives for how the new date is calculated.

If you have time, I would appreciate your reviewing my new changes...

I may now have time to release this on Friday morning or over the weekend...

Explanation of what's going on:

  • The new due date works fine on every occurrence until 2022-01-31.
  • The next occurrence should be 2022-02-28, but is 2022-01-31
  • At which point, it get stuck on that date forever more.
- [ ] #task do stuff 🔁 every month on the 31st 📅 2022-01-31
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2022-01-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2021-12-31 ✅ 2022-10-03
- [x] #task do stuff 🔁 every month on the 31st 📅 2021-11-30 ✅ 2022-10-03

Copy link
Collaborator Author

@schemar schemar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, @claremacrae ! Just two minor comments in-line.

I really appreciate the extended documentation!

src/Recurrence.ts Outdated Show resolved Hide resolved
src/Recurrence.ts Show resolved Hide resolved
@claremacrae
Copy link
Collaborator

I think I might have figured out how to make it work with 'due on the 31st' not getting stuck in a loop....
By moving the due date forward...

@claremacrae claremacrae merged commit ece82d1 into main Oct 8, 2022
@claremacrae claremacrae deleted the monthly_recurrence branch October 8, 2022 20:49
@claremacrae claremacrae changed the title Ensure monthly and yearly recurrences fix: Improve next-date calculation for monthly and yearly recurring tasks Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: recurrence Anything to do with recurring/repeating tasks type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Understand and document what recurrence does when advancing to non-existent dates
4 participants