After more testing, it looks like http://drupal.org/node/93256 has introduced inconsistencies. On initial creation of a daily repeat pattern, the last day of the month often does not get a date. Editing this with "All occurrences" then causes a duplicate event on the day of the event you clicked to edit.

CommentFileSizeAuthor
#4 eventrepeat_fix93256_v2.patch910 bytesseanbfuller
#1 eventrepeat_fix93256.patch2.03 KBseanbfuller

Comments

seanbfuller’s picture

seanbfuller commented
Status: Active » Needs review
StatusFileSize
new eventrepeat_fix93256.patch2.03 KB

The attached patch rolls back two sections of code that were causing repeating event creation and update actions to result in incorrect distribution of events.
This portion of the patch:

@@ -1909,7 +1913,7 @@
           //mistakenly rendered as one w/ no end date--this check is done when loading the repeat patterns
           //for the date range being rendered)
           if ($repeatpattern['repeat_COUNT_remaining'] != 0) {
-            $repeatpattern['repeat_COUNT_remaining'] == $repeatpattern['repeat_COUNT_remaining'] > 1 ?
+            $repeatpattern['repeat_COUNT_remaining'] > 1 ?
             $repeatpattern['repeat_COUNT_remaining']-- : $repeatpattern['repeat_COUNT_remaining'] = -1;
           }
         } else {

Was causing daily repeating events to fail to render on the last day of some months.

This section of the patch:

@@ -1926,10 +1930,16 @@
       //last rendered value.  if no dates are returned, then close the sequence
       $dates_to_render = _eventrepeat_render_nodes($repeatpattern['rid'], NULL, $update = TRUE, FALSE, FALSE);
       $update = FALSE;
-      if (!$dates_to_render) {
-        db_query("UPDATE {event_repeat} SET repeat_last_rendered = %d WHERE rid = %d",
-          $repeatpattern['repeat_end'], $repeatpattern['rid']);
+
+      // If there are no dates to render, set the last rendered date to the
+      // end date, but only do this if the end date is not 0. If it is 0,
+      // then the update line above will have already set the end date to the
+      // date we ended on. This will keep us from starting over on repeat patterns
+      // that don't have repeat expiration dates.
+      if (!$dates_to_render && $repeatpattern['repeat_end'] != 0) {
+        db_query("UPDATE {event_repeat} SET repeat_last_rendered = %d WHERE rid = %d", $repeatpattern['repeat_end'], $repeatpattern['rid']);
       }
+
     }
   }
 }

Was causing a duplicate event on the day of the event you clicked on to edit. All other events would update just fine, but for some reason the node you were actually editing was getting an insert in addition to the update.

seanbfuller’s picture

seanbfuller commented
Status: Needs review » Postponed (maintainer needs more info)

OK, I might have spoken too soon. Now both stanzas of code seem to be working as intended. My theory that they were not working was based on rolling each back, and seeing the symptoms disappear. Upon further testing, it looks like this may have been a time zone issue, as the new install was set to GMT 0.. I'll keep an eye on this.

francisu’s picture

francisu commented

Sean, I think backing out the first portion of the patch is correct. The 2nd portion is OK.

In making the first portion:

I tried stepping through the code and looking carefully at the condition and I did not see any difference between

$repeatpattern['repeat_COUNT_remaining'] == $repeatpattern['repeat_COUNT_remaining'] > 1 ?

and

$repeatpattern['repeat_COUNT_remaining'] > 1 ?

But now on further reflection I do. If the 2nd part of the condition is true, it's equal to the first part if the first part is > 0. If it's false, it's equal to the first part if the first part == 0. So it's really the same as:

$repeatpattern['repeat_COUNT_remaining'] > 0 == $repeatpattern['repeat_COUNT_remaining'] > 1 ?

If the values are, the condition is:

0 - true
1 - false
2 (and higher) - true

My change of course broke the 0 value case.

I would now write this as:

$repeatpattern['repeat_COUNT_remaining'] == 0 || $repeatpattern['repeat_COUNT_remaining'] > 1

Which I think is much clearer. Is there a reason it is written the other way (performance perhaps)? If so, a comment documenting what it does would be useful.

Sorry about introducing the bug.

seanbfuller’s picture

seanbfuller commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new eventrepeat_fix93256_v2.patch910 bytes

Thanks for checking that out. I was getting lost in there and starting to confuse myself. I didn't actually write that line, so I was a bit confused by it at first also. Here is an updated patch against latest 4.7.x-2.x that changes the stanza as you outlined above. Seems to be working now.

francisu’s picture

francisu commented

Hmmm, wait a minute, in looking at the patch in context:

           //mistakenly rendered as one w/ no end date--this check is done when loading the repeat patterns
           //for the date range being rendered)
           if ($repeatpattern['repeat_COUNT_remaining'] != 0) {
-            $repeatpattern['repeat_COUNT_remaining'] > 1 ?
+            $repeatpattern['repeat_COUNT_remaining'] == 0 || $repeatpattern['repeat_COUNT_remaining'] > 1 ?
             $repeatpattern['repeat_COUNT_remaining']-- : $repeatpattern['repeat_COUNT_remaining'] = -1;
           }

The outer condition checks for != 0, so the first part of the condition of the patch is not necessary. Leaving us with the code I originally submitted:

           if ($repeatpattern['repeat_COUNT_remaining'] != 0) {
             $repeatpattern['repeat_COUNT_remaining'] > 1 ?
             $repeatpattern['repeat_COUNT_remaining']-- : $repeatpattern['repeat_COUNT_remaining'] = -1;
           }

So I don't think the patch as it's written now will really change anything.

seanbfuller’s picture

seanbfuller commented
Status: Needs review » Closed (fixed)

Closing this. Feel free to reopen if anything related to this comes up.