I have found a case, when an exception is not excluded from the list of dates.

View: https://skitch.com/temaruk/gqh6r/fullscreen
Edit: https://skitch.com/temaruk/gqh6m/fullscreen

Is it by design? I would expect even the starting date excluded if that is set.
( I tried to check how this should work at http://www.kanzaki.com/docs/ical/exdate.html )

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

The "EXDATE" property can be used to exclude the value specified in
"DTSTART". However, in such cases the original "DTSTART" date MUST
still be maintained by the calendaring and scheduling system because
the original "DTSTART" value has inherent usage dependencies by other
properties such as the "RECURRENCE-ID".

That seems a little ambiguous. I guess it means we should remove it from the array of results, but it's a little vague.

PatchRanger’s picture

Status: Active » Needs review
FileSize
2.29 KB

Confirmed. I've made tests for this issue. Should fail by 'Exceptions work as expected' assertion.
Please note that test includes testcase for Date API calculations from original topic starter message - it passes. It makes me think that the problem is inside form submission handler, it looks like it incorrectly converts result of form submission to Date API call.
Am going to continue debugging.

PatchRanger’s picture

Here is working solution.
It fixes date exception handling.
It does not deal with start date exception due to ambiguity in specification (EDIT: see comment below).
Patch includes full test coverage: tests break before patch and pass after applying.
Drupal.org testbot is currently stuck for Date-7.x-2.x: see http://qa.drupal.org/pifr/test/200603. I've submitted the patch, that fixes it: #1835214: Automated tests failing - Exception thrown in Date2 migration.
As there is no chance to demonstrate here that this patch really solves this issue and that patch indeed cures testbot, see my special repository at Travis instead: https://travis-ci.org/PatchRanger/drupal7-date/builds/6874960#L233 - all tests passed.

Please review the patch.

PatchRanger’s picture

Here is what I've discovered after further investigation (see http://drupalcode.org/project/date.git/blob/refs/heads/7.x-2.x:/date_rep...):

  // The start date always goes into the results, whether or not it meets
  // the rules. RFC 2445 includes examples where the start date DOES NOT
  // meet the rules, but the expected results always include the start date.

Any suggestions how to deal with it?

PatchRanger’s picture

PatchRanger’s picture

By doing 'blame' at this lines of code, I found out that this message about RFC 2445 was added by this commit: http://drupalcode.org/project/date.git/commit/b537326dda467daf935a40f5aa... .
As you could see, it was created - and has never been changed since. In fact, it was not just created - it was moved from date_repeat.module: see http://drupalcode.org/project/date.git/blobdiff/f8a1f25d967ba85eea5e57e2... . Here are the most important lines of it:

  // RFC 2445 says if no day or monthday is specified when creating repeats for weeks,
  // months, or years, impute the value to be the day of week of the start date.

So, it appears, that original wording was more accurate and correct. Let's think how it affects our situation.

PatchRanger’s picture

Finally done.
@KarenS Thank you, following your suggestion I've finally got it working.
Only tests and patch+tests are attached.
As they aren't going to be tested by testbot, here are links to Travis environment:

Please note that I am using patch from #1835214: Automated tests failing - Exception thrown in Date2 migration to cure testbot at my Travis environment.
Please review.

Dave Cohen’s picture

This other issue is marked a dup of this one: #1842708: Check that start date matches repeat rules.

That other issue refers to the problem where the starting date is always considered a match. While this issue refers to when an exception explicitly includes the start date. Are they really duplicates of one another? I can't tell from just looking at the patch, will this patch exclude the start date when it does not match the criteria, but is not explicitly excluded?

Thanks for any info.

PatchRanger’s picture

@Dave Cohen Yes, you were right marking that one as a duplicate, because this patch really will exclude the start date when it does not match the criteria, but is not explicitly excluded: see these lines of the patch:

     $start = "1997-09-02 09:00:00";
     $end = "1997-09-30 09:00:00";
     $rule = "RRULE:FREQ=WEEKLY;INTERVAL=2;UNTIL=19970924T000000Z;WKST=SU;BYDAY=MO,WE,FR";
-    // should be (1997 9:00 AM EDT)September 2,3,5,15,17,19
+    // should be (1997 9:00 AM EDT)September 3,5,15,17,19
     $dates = date_repeat_calc($rule, $start, $end, array());
-    $shouldbe = '1997-09-02 09:00:00, 1997-09-03 09:00:00, 1997-09-05 09:00:00, 1997-09-15 09:00:00, 1997-09-17 09:00:00, 1997-09-19 09:00:00';
+    $shouldbe = '1997-09-03 09:00:00, 1997-09-05 09:00:00, 1997-09-15 09:00:00, 1997-09-17 09:00:00, 1997-09-19 09:00:00';
     $result = implode(', ', $dates);
     $this->assertEqual($result, $shouldbe, $rule . '; Starting ' . $start . ';  results: ' . $result);

See? It corrects existing test, making it to behave as I told above: start date is excluded without explicit exclusion because of not matching the criteria.

Status: Needs review » Needs work

The last submitted patch, date-fix_date_exclusion-1365516-7.patch, failed testing.

ejk’s picture

Issue summary: View changes

Just ran into this issue with my code and had to track it down. I'm going to have to add custom handling outside of calling this function to work around it.

I don't see how there's any confusion between what data fields need to be maintained in the VEVENT object and what dates are actually calculated from that data. If an EXDATE matches the DTSTART time, it should be excluded from the results.