Skip to content

fix(calendar): keep yearly events in DTSTART's month when BYMONTH is missing - #4222

Open
pascalpfammatter wants to merge 1 commit into
MagicMirrorOrg:developfrom
pascalpfammatter:fix/yearly-bymonthday-without-bymonth
Open

fix(calendar): keep yearly events in DTSTART's month when BYMONTH is missing#4222
pascalpfammatter wants to merge 1 commit into
MagicMirrorOrg:developfrom
pascalpfammatter:fix/yearly-bymonthday-without-bymonth

Conversation

@pascalpfammatter

Copy link
Copy Markdown

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:

DTSTART;VALUE=DATE:20231002
DTEND;VALUE=DATE:20231003
RRULE:FREQ=YEARLY;WKST=MO;INTERVAL=1;BYMONTHDAY=2
SUMMARY:Ted Birthday

FREQ=YEARLY together with BYMONTHDAY=2, but no BYMONTH. BYMONTHDAY is an expanding rule part for FREQ=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 BYMONTHDAY rules to the DTSTART month breaks legitimate rules such as FREQ=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

expandRecurringEvent confines a yearly rule to DTSTART's month, but only when the rule is provably a redundant restatement of DTSTART rather than a real expansion. All of these must hold:

  • FREQ=YEARLY
  • BYMONTHDAY holds exactly one value
  • that value equals DTSTART's day-of-month
  • no BYMONTH, BYDAY, BYYEARDAY, BYWEEKNO or BYSETPOS

Under 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.

Rule Affected? Result
FREQ=YEARLY;...;BYMONTHDAY=2, DTSTART Oct 2 yes once a year, 2 October
FREQ=YEARLY;BYMONTHDAY=1,3 no, two values unchanged, expands over 12 months
FREQ=YEARLY;BYMONTHDAY=13;BYDAY=FR no, BYDAY present unchanged, every Friday the 13th
FREQ=YEARLY;BYMONTHDAY=7, DTSTART on the 2nd no, day mismatch unchanged, expands
FREQ=YEARLY;BYMONTHDAY=2;BYMONTH=10 no, BYMONTH present unchanged, once a year
FREQ=YEARLY no, no BYMONTHDAY unchanged, once a year

The rule is read through node-ical's public options, and both the current string freq and 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/Kolkata and UTC. lint:js and lint:prettier are 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.

…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
@KristjanESPERANTO

Copy link
Copy Markdown
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 rrule.options and guessing between camelCase/lowercase keys and string/numeric freq, read rrule.origOptions. It only contains what was actually written in the RRULE (not what the library fills in as a default), so detecting "no BYMONTH" becomes unambiguous.

	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();
	},

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