iCal parsing bug?

mbalfour

I think I’ve found a bug in ical.js in the calendar module, but I’m not super-familiar with the iCal format, so I figured I’d post here and see if this is correct.

My ics file has some entries that look like this:

BEGIN:VEVENT
CREATED:20160103T174939Z
DTEND;TZID=US/Central:20160226T190000
DTSTAMP:20160225T150003Z
DTSTART;TZID=US/Central:20160226T090000
LAST-MODIFIED:20160103T174940Z
RRULE:FREQ=DAILY;UNTIL=20160229T055959Z
SEQUENCE:0
SUMMARY:Work
UID:00296911-7E2C-4229-9E49-1FBBA96F5344
END:VEVENT
BEGIN:VEVENT
CREATED:20160103T175015Z
DTEND;TZID=US/Central:20160228T170000
DTSTAMP:20160225T150003Z
DTSTART;TZID=US/Central:20160228T110000
LAST-MODIFIED:20160216T070945Z
RECURRENCE-ID;TZID=US/Central:20160228T090000
SEQUENCE:0
SUMMARY:Working
UID:00296911-7E2C-4229-9E49-1FBBA96F5344
END:VEVENT

What’s happening is that instead of this showing up as a recurring event, it’s showing up as a single-date event. It appears this is because I’ve got multiple entries with the same UID, and so the second one is just clobbering over the first. My best guess is that the appropriate behaviour is for it to merge with the first.

I think the correct fix is in ical.js, to change this:

        if (curr.uid)
                par[curr.uid] = curr

To this:

        if (curr.uid)
        {
            if (par[curr.uid] === undefined)
            {
                par[curr.uid] = curr
            }
            else
            {
                // TODO: Is this the correct way to merge?  Do I need to take LAST-MODIFIED field into account?
                var key;
                for (key in curr)
                {
                    par[key] = curr[key];
                }
            }
        }

In my limited test case it works, but does anyone know this code or the ical format well enough to chime in?

mbalfour

Ugh.

So, the fix above is only partially right, and the problems with it speak to the minimalistic nature of this ical implementation. :( I’ve verified that the first half of this is right - we shouldn’t overwrite any existing entries to “par[curr.uid]”. The second half, where I’m trying to merge it, is just flat-out wrong though.

The problem that I’m facing is that my first ics entry is setting up a recurrence rule, for example:
RRULE:FREQ=DAILY;UNTIL=20160229T055959Z
“repeat daily until 2/29/2016”

The second ics entry is trying to say “do something different on a specific day”:
RECURRENCE-ID;TZID=US/Central:20160228T090000
“change some information for the 2/28/2016 calendar entry”

There’s simply no concept in ical.js (or consequently calendarfetcher.js) for storing exceptions and changes to a recurring rule, so I don’t think I can get this to display correctly without some larger-scale changes, or swapping out the ical implementation for a more complete & full-featured one.

The warning to everyone: The calendar module is currently a bit dangerous, in that this bug will cause your calendar to silently display misleading information if you have recurring calendar entries and modified single occurrences of them. :(

mbalfour

OK, made the larger-scale changes and they seem to work. :) I don’t have anything in a state to pull-request into MagicMirror yet, since I’m doing this with a variant of the calendar module to give a monthly view. I’ve changed ical.js to create a recurrences[] array to contain all the recurrence exceptions, and an exdate[] array to contain all the “except this date” exceptions. This is now pull-requested on the ical.js repository.

On the calendar module side, it took a few changes in calendarfetcher.js to use these arrays to correctly generate the lists of upcoming events. This will take me longer to get back to MagicMirror, since I’m trying to merge my monthly-view calendar and the existing calendar module, since they share about 90% of the same code.

KirAsh4

You could also use my calendar_monthly module if you want to hack that. Send me PR requests and I can push them up to that main branch.

Ultimately I will merge that module into the default calendar one, I just haven’t had time to sit down and do that yet.

mbalfour

Well bummer, I hadn’t seen your module when working on this. :( I’ve just merged my changes back into the main calendar code and sent a pull request for it: https://github.com/MichMich/MagicMirror/pull/375

(I also sent a pull request to the ical.js repo - https://github.com/peterbraden/ical.js/pull/64 )

Looking at your code, it looks like we’re trying to accomplish slightly different things, but with similar approaches, so it probably wouldn’t be too bad to merge them together as well. The main difference is that I want the ical events to show up on the monthly calendar view, and it looks like you’re displaying an “empty” calendar, right? But you’ve got some cool features like exposing the css styling out more dynamically, having an option to turn off the headers, etc. And your html creation code is cleaner than mine. :)

I can take a look at bringing these two together eventually myself, but I have to admit it’s a bit lower on my list now that I’ve fixed the functional parts of the calendar and gotten all my events showing up. (A family calendar isn’t much use if you can’t trust that it’s displaying everything correctly)

My new first priority is to finish off a module that lets us text message the display to display a notification to the family (“Went out to buy groceries be back in an hour”). :) The base concept is that it shows the summary of the latest gmail message an account receives, since you can text gmail accounts. Guess it’s possible other people might find other reasons to want that too…

KirAsh4

@mbalfour said in iCal parsing bug?:

The main difference is that I want the ical events to show up on the monthly calendar view, and it looks like you’re displaying an “empty” calendar, right?

Correct. That was the request made, a plain and simple calendar without any bells and whistles.

My new first priority is to finish off a module that lets us text message the display to display a notification to the family (“Went out to buy groceries be back in an hour”). :) The base concept is that it shows the summary of the latest gmail message an account receives, since you can text gmail accounts. Guess it’s possible other people might find other reasons to want that too…

Make sure you make it that only allowed numbers can text that e-mail, otherwise you may end up with spam messages.