fix(calendar): keep yearly events in DTSTART's month when BYMONTH is missing - #4222
Open
pascalpfammatter wants to merge 1 commit into
Open
Conversation
…missing
Several calendar clients export a yearly event as
DTSTART;VALUE=DATE:20231002
RRULE:FREQ=YEARLY;WKST=MO;INTERVAL=1;BYMONTHDAY=2
restating DTSTART's day-of-month in BYMONTHDAY but omitting BYMONTH. Since
BYMONTHDAY is an expanding rule part for FREQ=YEARLY, the recurrence expands
to the 2nd of every month, so the event shows up twelve times a year instead
of once. Google Calendar and the clients that emit this render it once a year
on DTSTART's date.
Confine such a rule to DTSTART's month, but only when it is provably a
redundant restatement of DTSTART rather than a real expansion: BYMONTHDAY must
hold exactly one value, that value must equal DTSTART's day-of-month, and no
other BYxxx part may shape the recurrence. Rules that genuinely expand, such as
FREQ=YEARLY;BYMONTHDAY=1,3 or FREQ=YEARLY;BYMONTHDAY=13;BYDAY=FR, are untouched.
Refs MagicMirrorOrg#2547, MagicMirrorOrg#3047
Collaborator
|
Thanks for the deep dive here, and for the references to the earlier discussions. I think the direction here is fine. What do you think of this: instead of reading isYearlyRuleMissingByMonth (event) {
const options = event.rrule?.origOptions;
if (!options || options.freq !== "YEARLY") {
return false;
}
const isSingleMonthDay = Array.isArray(options.byMonthDay) && options.byMonthDay.length === 1;
// Any other BYxxx part means the rule shapes the recurrence on purpose.
const hasShapingPart = Boolean(options.byMonth || options.byDay || options.byYearDay || options.byWeekNo || options.bySetPos);
if (!isSingleMonthDay || hasShapingPart) {
return false;
}
return options.byMonthDay[0] === event.start.getDate();
}, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A birthday in my Google Calendar showed up on my mirror as "tomorrow" when it is actually in October. It turns out it had been appearing on the 2nd of every month, and I had simply never noticed until it landed on a day I paid attention to.
The event looks like this in the ICS feed:
FREQ=YEARLYtogether withBYMONTHDAY=2, but noBYMONTH.BYMONTHDAYis an expanding rule part forFREQ=YEARLY, so the expander returns the 2nd of every month — twelve occurrences a year instead of one. Google Calendar's own UI, and the client that wrote the rule, show it once a year on 2 October.Out of 3354 events in my calendar exactly two had this shape, both created by an older mobile calendar app. Newer clients write a plain
RRULE:FREQ=YEARLY, which is why this is easy to miss — but the old events are never rewritten, so they keep misbehaving forever.Why not fix it in the RRULE library
This has come up before, and each time it was closed as bad input data rather than fixed: #2547 (2021), #3047 (2023), and recently jens-maus/node-ical#531. In that last one a fix was actually written (ggaabe/rrule-temporal#127) and then withdrawn, because a reviewer pointed out that restricting yearly
BYMONTHDAYrules to theDTSTARTmonth breaks legitimate rules such asFREQ=YEARLY;BYMONTHDAY=1,3, which really must expand across all twelve months.That objection is correct, and it is why I don't think the RRULE engine is the right place for this. A general-purpose expander has to follow RFC 5545. A calendar display, on the other hand, can afford to be liberal about a well-known broken export as long as the detection is narrow enough that no correct rule is touched.
What this does
expandRecurringEventconfines a yearly rule toDTSTART's month, but only when the rule is provably a redundant restatement ofDTSTARTrather than a real expansion. All of these must hold:FREQ=YEARLYBYMONTHDAYholds exactly one valueDTSTART's day-of-monthBYMONTH,BYDAY,BYYEARDAY,BYWEEKNOorBYSETPOSUnder those conditions the expansion yields
DTSTART's own date plus eleven dates the author never wrote, so dropping the extras cannot lose a real occurrence.FREQ=YEARLY;...;BYMONTHDAY=2, DTSTART Oct 2FREQ=YEARLY;BYMONTHDAY=1,3FREQ=YEARLY;BYMONTHDAY=13;BYDAY=FRBYDAYpresentFREQ=YEARLY;BYMONTHDAY=7, DTSTART on the 2ndFREQ=YEARLY;BYMONTHDAY=2;BYMONTH=10BYMONTHpresentFREQ=YEARLYBYMONTHDAYThe rule is read through node-ical's public
options, and both the current stringfreqand the older numeric one are accepted.Tests
Six new cases in
calendar_fetcher_utils_spec.js. The five "unaffected" rows above are tests that already pass without the change — they are there to pin the behaviour that must not regress. Only the first one fails before the fix.Calendar unit tests pass on
Europe/Zurich,America/New_York,Pacific/Auckland,Asia/KolkataandUTC.lint:jsandlint:prettierare clean.Against my own feed over the next 365 days the number of occurrences goes from 132 to 110, and the difference is exactly the 22 spurious instances from those two events — nothing else changes.
One thing worth deciding
I've made this unconditional, since the detection is narrow enough that no correct rule is affected. If you would rather have it behind a calendar config option, say so and I'll move it.