-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Conversation
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
0aebc5e
to
a20fd20
Compare
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. |
One thing that's not ideal is the change to the |
By intentionally breaking the recurrence code, I found that in addition to
obsidian-tasks/tests/Commands/ToggleDone.test.ts Lines 96 to 110 in 11a3712
obsidian-tasks/tests/Task.test.ts Lines 437 to 717 in 1a889bc
|
I see what you mean. Perhaps rename it to something like |
@schemar I grabbed my tests from #614 and added them to They all pass, which is great. 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)( |
@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 // 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
}, |
For now I will add documentation to the code.
I will add these three tests.
Looks correct to me. Let me investigate. |
@claremacrae there are comments in the code now, hope it helps. |
See tests for failing edge cases.
Thank you! I added the test cases to the Recurrence test file and fixed the bug. |
There was a problem hiding this 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...
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. |
Is it worth jotting down the outline of what content might go in the user docs? |
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 |
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 👍 |
I added a short warning. Feel free to change as desired! |
Huge thanks @schemar! |
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…. |
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. |
I've done a few more hours of testing, and some documentation. |
@schemar, Hi, below is the behaviour mentioned above. Explanation of what's going on:
|
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...
Your comment has prompted a couple of ideas:
|
Things to consider when fixing the The following are all treated as the same thing:
(I may edit this comment with more cases) |
There is also this case to consider (taken from the rule tests):
|
Would ignoring all |
Ooh, I like that. I think if we ignored (See about 2 or 3 comments back for the |
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:
|
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. |
Hi @schemar I've fixed the below, reverting it back to the previous behaviour if Note done: Are there any cases where 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...
|
There was a problem hiding this 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!
I think I might have figured out how to make it work with 'due on the 31st' not getting stuck in a loop.... |
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.
edited to turn the list in to checkboxes..
Fixes #614, #1087