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

Fixes for Recurrence Rules #64

Merged
merged 2 commits into from
Jul 19, 2016
Merged

Conversation

mbalfour
Copy link
Contributor

Added better support for Recurrence Rules:

  • Updates to specific recurrences of events now get their own entries in
    a recurrences[] array, keyed by ISO date/time. (Previously, their
    records silently overwrote the main recurrence event)
  • Exceptions to recurrences now appear in an exdate[] array, keyed by
    ISO date/time.

Also added "knowledge" of the dtstamp, created, and lastmodified fields
so that they get stored as dates, not strings.

This should solve issues #57 and #63.

Added better support for Recurrence Rules:
- Updates to specific recurrences of events now get their own entries in
a recurrences[] array, keyed by ISO date/time.  (Previously, their
records silently overwrote the main recurrence event)
- Exceptions to recurrences now appear in an exdate[] array, keyed by
ISO date/time.

Also added "knowledge" of the dtstamp, created, and lastmodified fields
so that they get stored as dates, not strings.

This should solve issues peterbraden#57 and peterbraden#63.
@mbalfour mbalfour closed this Jul 16, 2016
@mbalfour mbalfour reopened this Jul 16, 2016
@mbalfour mbalfour closed this Jul 16, 2016
@mbalfour mbalfour reopened this Jul 16, 2016
@peterbraden
Copy link
Owner

This looks great, do you have a test case you could add so that we can verify this in our tests?

Thanks!

Code maintenance problems:
* Example.js should be pointing to node-ical, not ical now.
* ical should depend on rrule 2.1.0 or greater due to dependency issues
with underscore

Added tests:
* Verify that a recurrence-id doesn't overwrite the main entry
* Verify that exdates appear in the exdate array
* Verify that recurrence-id entries appear in the recurrences array
* Verify that the arrays are keyed by ISO date strings
@mbalfour
Copy link
Contributor Author

mbalfour commented Jul 19, 2016

Um, good question. I've just submitted some tests, I think correctly? I
ran into a few problems while trying to add them that I'm not sure if
they're because I'm a nodejs n00b or if there were legitimate problems:

Example.js problem: "Error: Cannot find module 'ical'".
It works if I change the first line to var ical = require('./node-ical').
I'm assuming this just wasn't maintained very well? I can't see how the
example would work, since fromURL isn't defined in ical.

test/test.js problem:
···················✗··············✗
✗ Errored » callback not fired
in with test6.ics (testing assembly.org) event with no ID
in node-ical
in undefined✗ Errored » callback not fired
in with test6.ics (testing assembly.org) event with rrule
in node-ical
in undefined✗ Errored » 33 honored ∙ 1 broken ∙ 2 errored ∙ 3 dropped

Looks like test6 was broken because rrule was throwing an exception deep
inside it related to underscore. Bumping up the rrule version number in
the package dependency from 2.0.0 to 2.1.0 (which removes the underscore
dependency) seemed to fix it.

My newly-added tests run cleanly, but I'm still not getting a clean test
run:
·········································✗

url request errors
  ✗ are passed back to the callback
    » expected null to be an instance of Error // test.js:409

✗ Broken » 41 honored ∙ 1 broken (0.200s)

I'm not sure what's up with this one, but I at least feel confident that it
has nothing to do with my changes!

Thanks,
Mike

On Mon, Jul 18, 2016 at 3:34 AM, Peter Braden [email protected]
wrote:

This looks great, do you have a test case you could add so that we can
verify this in our tests?

Thanks!


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#64 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ATOQNqxI8Bh6JxoJa7B339C1x4r4RCuRks5qWzqPgaJpZM4JOCOw
.

@peterbraden
Copy link
Owner

Ah ok, there must have been a problem with the suite. Thanks for adding the tests, let me see if I can get them to run clean.

@peterbraden peterbraden merged commit abca2cb into peterbraden:master Jul 19, 2016
@mykhailo-riabokon
Copy link

mykhailo-riabokon commented Aug 25, 2016

Hi there!

Thank you for this PR. And I would like to check some question about recurrent events.

For example you have one repeated event for 4 days. And You decided to edit only one event from series (second event for example). And as result we have next ical response

BEGIN:VCALENDAR
PRODID:-//Google Inc//Google Calendar 70.9054//EN
VERSION:2.0
CALSCALE:GREGORIAN
METHOD:PUBLISH
X-WR-CALNAME:ical
X-WR-TIMEZONE:Europe/Kiev
X-WR-CALDESC:
BEGIN:VTIMEZONE
TZID:Europe/Kiev
X-LIC-LOCATION:Europe/Kiev
BEGIN:DAYLIGHT
TZOFFSETFROM:+0200
TZOFFSETTO:+0300
TZNAME:EEST
DTSTART:19700329T030000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:+0300
TZOFFSETTO:+0200
TZNAME:EET
DTSTART:19701025T040000
RRULE:FREQ=YEARLY;BYMONTH=10;BYDAY=-1SU
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
DTSTART;TZID=Europe/Kiev:20160826T140000
DTEND;TZID=Europe/Kiev:20160826T150000
DTSTAMP:20160825T061505Z
UID:[email protected]
RECURRENCE-ID;TZID=Europe/Kiev:20160826T140000
CREATED:20160823T125221Z
DESCRIPTION:
LAST-MODIFIED:20160823T130320Z
LOCATION:
SEQUENCE:0
STATUS:CONFIRMED
SUMMARY:bla bla
TRANSP:OPAQUE
END:VEVENT
BEGIN:VEVENT
DTSTART;TZID=Europe/Kiev:20160825T140000
DTEND;TZID=Europe/Kiev:20160825T150000
RRULE:FREQ=DAILY;UNTIL=20160828T110000Z
DTSTAMP:20160825T061505Z
UID:[email protected]
CREATED:20160823T125221Z
DESCRIPTION:
LAST-MODIFIED:20160823T125221Z
LOCATION:
SEQUENCE:0
STATUS:CONFIRMED
SUMMARY:repeated
TRANSP:OPAQUE
END:VEVENT
END:VCALENDAR

And after parseICS method, I have only one event, (bla bla in description). And I think it's because of UID, they do have same UID, and event just overrides.

But edited event from series has RECURRENCE-ID which is parsed as a date param.

  // RECURRENCE-ID is the ID of a specific recurrence within a recurrence rule.
  // TODO:  It's also possible for it to have a range, like "THISANDPRIOR", "THISANDFUTURE".  This isn't currently handled.
  var recurrenceParam = function (name) {
      return dateParam(name);
  }

As I understood from RFC https://tools.ietf.org/html/rfc5545#section-3.8.4.4

Description:  The full range of calendar components specified by a
      recurrence set is referenced by referring to just the "UID"
      property value corresponding to the calendar component.  The
      "RECURRENCE-ID" property allows the reference to an individual
      instance within the recurrence set.

RECURRENCE-ID should be taken into account while parsing events. What do you think about this?

And I found this PR #44, and it should fix this problem, but it's not merged.

@mbalfour
Copy link
Contributor Author

I've started PR #66 which should fix the problem above. The specific problem in your test case that I wasn't accounting for in PR #64 was the case where a RECURRENCE-ID appears before the RRULE that it's modifying. Great find!

Note - I solved the EXDATE / RRULE / RECURRENCE-ID problem in a different way than PR #44 so I don't think that one is still a valid approach.

@mykhailo-riabokon
Copy link

@mbalfour Thank you! And I hope that your PR (#66) will be merge soon.

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.

3 participants