Not entirely sure whether the issue is in Date, Date iCal, or Feeds. I'm on the latest dev of each and whenever I import an ics with recurring dates that are terminated by an UNTIL, the final occurrence (aka the given UNTIL) is omitted from the added dates.

Here is a test event I used to test this issue specifically.

BEGIN:VEVENT
UID:34781-1747
DTSTART;TZID=America/Los_Angeles:20140323T190000
DTEND;TZID=America/Los_Angeles:20140323T210000
DTSTAMP:20140226T110020
RRULE:FREQ=DAILY;UNTIL=20140328T190000;WKST=SU
CREATED:20140226T102041
DESCRIPTION:description
LAST-MODIFIED:20140226T102041
SEQUENCE:1393441220
SUMMARY:event
TRANSP:OPAQUE
END:VEVENT

The desired dates are the 23rd through 28th of march inclusive. The UNTIL datetime specified here correctly matches the datetime of what would be the final occurrence of this event.

When this event is imported the resulting dates that are stored are 23rd through 27th. The 28th gets omitted. 

Repeats every day until Fri Mar 28 2014 .
Sunday, March 23, 2014 - 7:00pm to 9:00pm
Monday, March 24, 2014 - 7:00pm to 9:00pm
Tuesday, March 25, 2014 - 7:00pm to 9:00pm
Wednesday, March 26, 2014 - 7:00pm to 9:00pm
Thursday, March 27, 2014 - 7:00pm to 9:00pm

This is where it gets strange. I edited the imported event to see how it was stored on the site. The start and end datetimes matched the DTSTART and DTEND. The RRULE matched as well, and the terminator for the RRULE was correctly set to end on the 28th. Upon saving the "edit", which didn't involve any changes whatsoever, the 28th finally showed up as an occurrence.

Repeats every day until Fri Mar 28 2014 .
Sunday, March 23, 2014 - 7:00pm to 9:00pm
Monday, March 24, 2014 - 7:00pm to 9:00pm
Tuesday, March 25, 2014 - 7:00pm to 9:00pm
Wednesday, March 26, 2014 - 7:00pm to 9:00pm
Thursday, March 27, 2014 - 7:00pm to 9:00pm
Friday, March 28, 2014 - 7:00pm to 9:00pm

Reimporting the ics did not result in any updates. Deleting the node through the import delete menu and then importing again returned the event to it's 28th is omitted state.

This leads me to believe that the date repeat info is all stored correctly, but whatever handles the actual generation of each date is somehow different when using Date iCal Import than when using the date repeat field form.

I'm gonna keep researching this and I'll chime back in if I figure it out.

Comments

RedEight’s picture

RedEight CreditAttribution: RedEight commented

Found something that might be helpful. My brain is foggy right now so I can't deduce what it means...

I added 

if(module_exists('devel')){
        dpm($values);
      }

to line 476 of date/date_api/date_api_ical.inc (date_ical_parse_rrule function) to see if I couldn't trace out a difference.

The response from import script

datetime (String, 19 characters ) 2014-03-28 19:00:00
all_day (Boolean) FALSE
tz (String, 3 characters ) UTC

And the response from saving on the edit form. 

datetime (String, 19 characters ) 2014-03-29 06:59:59
all_day (Boolean) FALSE
tz (String, 3 characters ) UTC

A couple parts of this seem off. First, The import script has the time in local time as defined in the ical file, but the tz attribute is set to UTC. Since local time is PST (Los Angeles), that means it is UTC-8:00. So a direct conversion from 19:00:00 local time to UTC would be 8 hours later or 03:00:00 the following day. So then where does the 06:59:59 come from when saving the "edited" date?

The only method I can think of to get from 2014-03-28 19:00:00 to 2014-03-29 06:59:59 would be to convert 19:00:00 to 7PM, dump the PM, assume it's AM and somehow add a day??? Even then you wouldn't end up with the 06:59:59...

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

That definitely sounds weird. I'll look into it as soon as I get chance. Or perhaps Axel, my co-maintainer, could take a look. I'm really swamped at work right now.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

And by Axel I of course meant Vlad. Sorry Vlad >_<.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

Sorry for the enormous delay on this... I've been incredibly busy at work lately.

Anyway, I finally got a chance to look at this, and debugging your issue actually led me down a horrid rabbit hole involving deep-seated bugs in the Date module's various "time zone handling" options for date fields. I'm not entirely finished debugging your issue, but I can definitely confirm that it's related to the way Date does timezone conversion when it saves dates to the DB. In most cases, it converts from the imported date's original timezone to UTC, stores that to the DB, then converts the date back to the site's default timezone upon display (or to a different timezone, depending on your Date field's "Time zone handling" setting).

So, I tried a little experiment, chaging your event's time to noon instead of 9pm. This prevents the timezone conversion from PST to UTC from altering the date. When I re-did the import, the 6th repeat was correctly added! So clearly, the final repeat was being left off because converting from March 28 at 7pm PST (the final repeat) to UTC results in March 29 at 1am (which is after the UNTIL value).

This indicates that the problem is most likely a bug in Date iCal, since the repeats are apparently being calculated after the UTC conversion, which clearly doesn't happen when you save the node from the usual edit form. I'll try to find a way to fix this.

RedEight’s picture

RedEight CreditAttribution: RedEight commented

First, thanks for taking the time to look into this. When I started trying to figure it out myself I saw how big a can of worms this could be with Date, Feeds, and Date_iCal and each of their current devs possibly being the culprit. What you found makes perfect sense. If there is any more information you need from me that might be helpful just ask.

  • Commit 428cfc8 on 7.x-3.x by coredumperror: 
    Issue #2207173: RRULEs with non-UTC UNTIL values are now supported....
coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented
Assigned: Unassigned » coredumperror
Status: Active » Needs review

Yeesh, this one was a doozey. But I'm finally done, and I've pushed this feature up to the newest dev version of Date iCal.

Surprisingly enough, the problem ultimately turned out not to be with Date iCal. According to the official iCal spec, the UNTIL value on RRULEs must be specified in UTC. But whatever software is creating your iCal feed doesn't respect that rule, and is specifying the UNTIL in local time. That causes all the non-Date iCal code that assumes the UNTIL is in UTC to incorrectly compare it to the date of the original event, which breaks the repeat list.

So what I did was add a new setting to the iCal Parser settings page (click the Settings link in the "Parser" portion of the sidebar). If you check the new "RRULE UNTILs are not in UTC" checkbox on that page, then Save, your next import should include the final repeat.

Please let me know if this works for you.

RedEight’s picture

RedEight CreditAttribution: RedEight commented

Hmm, I must have found a different specification. I found RFC 5545 (https://tools.ietf.org/html/rfc5545) which seems to be an update on RFC 2445 (http://www.ietf.org/rfc/rfc2445.txt).

Of interest is page 40 paragraph 2 of RFC 5545.

The value of the UNTIL rule part MUST have the same
value type as the "DTSTART" property.  Furthermore, if the
"DTSTART" property is specified as a date with local time, then
the UNTIL rule part MUST also be specified as a date with local
time.

I took a look at your code. You mention that iCalcreator is adding a Z to indicate UTC. wouldn't that be against RFC 5545 for the TZ to differ between DTSTART and UNTIL? Seems like the real bug is in iCalcreator. I'm gonna take a look at the git and see if I can't find an issue.

Also, I tested the new dev. After ticking the new option in the ical parser I ran an import and got the following error. 

Internal Server Error

The server encountered an internal error or misconfiguration and was unable to complete your request.

Please contact the server administrator, xxx@xxxxxxx.com and inform them of the time the error occurred, and anything you might have done that may have caused the error.

More information about this error may be available in the server error log.

I'm gonna have to work with my server tech to see if I can't get you some more info on what broke under the hood. I tried the import with the box unchecked and it worked perfectly with the exception of the issue it's always had omitting the last occurrence (as expected)

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

Hmmm, the site I used to check the iCal spec may be out of date, then. I think you're right about iCalcreator being the culprit here. My original intention was to check for the existence of the "Z" in the UNTIL value, and use that to automatically determine which timezone to use for the conversion (so no checkbox would be required). But since iCalcreator was automatically adding the Z at parse-time, I couldn't do that, and I had to fall back to the checkbox.

I'm definitely concerned about that internal server error you're getting. And it's especially annoying that you're given no details about what happened. Does your site have the "Database logging" module enabled? If so you can go to /admin/reports/dblog to hopefully find the error message.

RedEight’s picture

RedEight CreditAttribution: RedEight commented

Ok, made sure logging was on, cleared the logs, then ran the import again. I'll list the errors in order.
Notice: Undefined offset: 2 in _date_ical_get_repeat_dates() (line 59 of XXXX/sites/all/modules/date_ical/date_ical.utils.inc).
Warning: DateTime::getTimezone(): The DateTime object has not been correctly initialized by its constructor in DateObject->__construct() (line 283 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::format(): The DateTime object has not been correctly initialized by its constructor in DateObject->format() (line 384 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::format(): The DateTime object has not been correctly initialized by its constructor in DateObject->format() (line 384 of XXXX/sites/all/modules/date/date_api/date_api.module).
...(repeats )
Warning: DateTime::setTimezone(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 358 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::setDate(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 359 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::setTime(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 360 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::format(): The DateTime object has not been correctly initialized by its constructor in DateObject->format() (line 384 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::format(): The DateTime object has not been correctly initialized by its constructor in DateObject->format() (line 384 of XXXX/sites/all/modules/date/date_api/date_api.module).
...
Warning: DateTime::setTimezone(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 358 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::setDate(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 359 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::setDate(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 359 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::setTime(): The DateTime object has not been correctly initialized by its constructor in DateObject->setTimezone() (line 360 of XXXX/sites/all/modules/date/date_api/date_api.module).
Warning: DateTime::format(): The DateTime object has not been correctly initialized by its constructor in DateObject->format() (line 384 of /home/joshua/public/reallifechurch.org/beta/sites/all/modules/date/date_api/date_api.module).
Notice: Undefined offset: 1 in _date_ical_get_repeat_dates() (line 62 of XXXX/sites/all/modules/date_ical/date_ical.utils.inc).
Notice: Undefined offset: 3 in _date_ical_get_repeat_dates() (line 62 of XXXX/sites/all/modules/date_ical/date_ical.utils.inc).
Notice: Undefined variable: datetime in date_ical_parse_date() (line 451 of XXXX/sites/all/modules/date/date_api/date_api_ical.inc).
Notice: Undefined index: FREQ in _date_repeat_calc() (line 49 of XXXX/sites/all/modules/date/date_repeat/date_repeat_calc.inc).
Warning: date_format() expects parameter 1 to be DateTime, null given in _date_repeat_calc() (line 65 of XXXX/sites/all/modules/date/date_repeat/date_repeat_calc.inc).

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

Ohhh, shoot. I bet this is related to RRULEs which don't have UNTILs. That was really dumb of me to not test that better.

I'll release a fix as soon as I get a chance.

RedEight’s picture

RedEight CreditAttribution: RedEight commented

>_< Sounds like something I've done one too many times...

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

OK, I pushed a "fix", but I couldn't actually re-produce the errors you were seeing, so I can't really tell why that was happening. I just wrapped the parse code that was crashing in an if check to ensure that it wouldn't crash.

However, I also added a log message for the times when it would have crashed. With the newest dev version I just released, If you could please do another import and then check your system log for a message from date_ical about RRULEs, I'd really appreciate if you could paste it to this issue. I tried various different RRULEs and couldn't get them to crash on my dev site, so I need to see this message to find out what format of RRULE was causing a problem.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

Oh BTW, I think you missed a critical section of the RFC in regards to UNTIL timezones. The very next sentence of the new RFC after the one you posted is the really important one, which shows that iCalcreator is not actually to blame here:

If the "DTSTART" property is specified as a date with UTC
time or a date with local time and time zone reference, then the
UNTIL rule part MUST be specified as a date with UTC time.

Your example event has DTSTART specified as a "date with local time and a time zone reference", so UNTIL does actually need to be in UTC. I'm not sure if iCalcreator properly differentiates between UNTILs that are alongside DTSTARTs which are in "local time" and those with time zone references, though, so it may still be at fault in some cases. But it's not in this one.

RedEight’s picture

RedEight CreditAttribution: RedEight commented

My bad on misreading the spec. I believe you are right. I will have to talk with the people generating my ical about correcting that.

I haven't had a chance to check the changes yet. I should have time tomorrow (2014-04-23) to test it out and let you know the results. *fingers crossed*

RedEight’s picture

RedEight CreditAttribution: RedEight commented

Ok, I got it all squared away today and was finally able to test the new dev. Good news. It worked! The dates are correctly imported and handled with all repeats

As requested, here are the messeges from date_ical regarding the problem RRULEs.

The RRULE string RRULE:FREQ=DAILY;UNTIL=20130324;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130407;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130414;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130421;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130630;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130707;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130714;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130817;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20130818;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=WEEKLY;UNTIL=20131201;BYDAY=SU,SA;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=WEEKLY;UNTIL=20140301T090000Z;BYDAY=MO,TU,WE,TH,FR;WKST=SU;INTER VAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20140614;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20140628;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20140802;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.
The RRULE string RRULE:FREQ=DAILY;UNTIL=20141005;WKST=SU;INTERVAL=1 could not be parsed to fix the UNTIL value. Date repeats may not be calculated correctly.

Looks like they are passing in just a date in some cases which causes a majority of the problems. The one exception appears to be due to a space in the word "INTER VAL" which should be INTERVAL. Of note with this specific event, the until date doesn't match the last occurrence of the RRULE like it should according to the spec. Here is the event from the ics file

BEGIN:VEVENT
UID:XXXXXXXX
DTSTART;TZID=America/Los_Angeles:20140210T090000
DTEND;TZID=America/Los_Angeles:20140210T180000
DTSTAMP:20140423T142748
RRULE:FREQ=WEEKLY;BYDAY=MO,TU,WE,TH,FR;UNTIL=20140301T090000;WKST=SU
CREATED:20140207T144524
DESCRIPTION:XXXXXXXX
LAST-MODIFIED:20140220T154018
LOCATION:XXXXXXXX
SEQUENCE:1398288468
SUMMARY:XXXXXXXX
TRANSP:OPAQUE
END:VEVENT

as you can see, its weekdays from February 10th to March 1st, but March 1st is a Saturday and not a weekday.

Thank you so much for your time. If there is any other information you need just let me know.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented
Status: Needs review » Needs work

Excellent! These error messages shed new light on how I should be parsing that RRULE string, and helped me identify what I was doing wrong. I just pushed a fix to git, so the new dev version, whenever it gets built, should fix all the errors you were seeing. Except maybe that one with "INTER VAL". I don't really know why the parser would complain about that in the first place, to be honest.

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented
Status: Needs work » Fixed

Oops, set the issue status wrong.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

RedEight’s picture

RedEight CreditAttribution: RedEight commented
Status: Closed (fixed) » Needs work

Sorry to reopen this but I ran into the issue again when I updated to the dev version with the code pushed on Apr 23rd.

I checked the git diff to see what changed and saw that you had added a ? after the Z on line 58 of date_ical.utils.inc
if (preg_match('/^(.*?)UNTIL=(.*?)Z?(.*?)$/', $repeat_data['RRULE'], $matches)) {
When I ran this against the test cases included in the apr 23rd commit using regex101.com I noticed that the second matching set would be blank and the third would contain what should be in the second. This meant that the strpos($matches[2], 'T') would always return false for these values.

I was able to come up with the following regex that correctly handled those problem values and appears to handle all the others as well.

if (preg_match('/^(.*?)UNTIL=([^Z;]{0,15})Z?(.*?)$/', $repeat_data['RRULE'], $matches)) {

I think it works because "([^Z;]{0,15})" is greedy where "(.*?)" grabs as little as possible.

After making this change date_ical.utils.inc I was able to import all events correctly except that one strange "INTER VAL" event.

Let me know if this is a viable fix. I'm still learning regex so go easy on me =]

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented
Status: Needs work » Fixed

Wow, you're right! That's a serious testing fail on my part.

I've pushed a fix up to git which should solve that problem. Rather than using ([^Z;]{0,15}), though, I used ([\dT]+), because it's more explicit about what's being grabbed (one or more characters which are either numeric digits (\d) or capital "T").

RedEight’s picture

RedEight CreditAttribution: RedEight commented

Good call on the capture. Yours is much clearer regarding what is being captured. Thanks for the fantastic support by the way.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.