9820: Infinite loop in adoptAlarmTime (DST Boundary)
authorPhilipp Schüle <p.schuele@metaways.de>
Mon, 31 Mar 2014 11:50:28 +0000 (13:50 +0200)
committerPhilipp Schüle <p.schuele@metaways.de>
Tue, 8 Apr 2014 08:26:28 +0000 (10:26 +0200)
- don't add bogus extra minute on event calculation
- fix computeNext which retunred occurences before next
- adds a test

https://forge.tine20.org/mantisbt/view.php?id=9820

Change-Id: I15cf0225941c82eaae4be0da0d27f9c218e4a93a
Reviewed-on: http://gerrit.tine20.com/customers/494
Tested-by: Jenkins CI (http://ci.tine20.com/)
Reviewed-by: Philipp Schüle <p.schuele@metaways.de>
tests/tine20/Calendar/Controller/EventNotificationsTests.php
tine20/Calendar/Controller/Event.php
tine20/Calendar/Model/Rrule.php
tine20/Tinebase/Record/RecordSet.php

index c7a7299..4d2a6bd 100644 (file)
@@ -4,7 +4,7 @@
  * 
  * @package     Calendar
  * @license     http://www.gnu.org/licenses/agpl.html AGPL Version 3
- * @copyright   Copyright (c) 2009-2013 Metaways Infosystems GmbH (http://www.metaways.de)
+ * @copyright   Copyright (c) 2009-2014 Metaways Infosystems GmbH (http://www.metaways.de)
  * @author      Cornelius Weiss <c.weiss@metaways.de>
  */
 
@@ -1010,6 +1010,39 @@ class Calendar_Controller_EventNotificationsTests extends Calendar_TestCase
     }
     
     /**
+     * testAdoptAlarmDSTBoundaryAllDayEvent
+     * 
+     * @see 0009820: Infinite loop in adoptAlarmTime / computeNextOccurrence (DST Boundary)
+     */
+    public function testAdoptAlarmDSTBoundaryAllDayEvent()
+    {
+        $event = $this->_getEvent();
+        $event->is_all_day_event = 1;
+        $event->dtstart = new Tinebase_DateTime('2014-03-03 23:00:00');
+        $event->dtend = new Tinebase_DateTime('2014-03-04 22:59:59');
+        $event->originator_tz = 'Europe/Berlin';
+        $event->rrule = 'FREQ=DAILY';
+        $event->alarms = new Tinebase_Record_RecordSet('Tinebase_Model_Alarm', array(
+            new Tinebase_Model_Alarm(array(
+                'minutes_before' => 15,
+            ), TRUE)
+        ));
+        
+        $savedEvent = Calendar_Controller_Event::getInstance()->create($event);
+        
+        $alarm = $savedEvent->alarms->getFirstRecord();
+        $alarm->sent_time = new Tinebase_DateTime('2014-03-29 22:46:01');
+        $alarm->alarm_time = new Tinebase_DateTime('2014-03-29 22:45:00');
+        $alarm->setOption('recurid', $savedEvent->uid . '-2014-03-29 23:00:00');
+        Tinebase_Alarm::getInstance()->update($alarm);
+        $alarm = $this->_eventController->get($savedEvent->getId())->alarms->getFirstRecord();
+        
+        Calendar_Controller_Event::getInstance()->adoptAlarmTime($savedEvent, $alarm, 'instance');
+        
+        $this->assertEquals('2014-03-30 21:45:00', $alarm->alarm_time->toString());
+    }
+    
+    /**
      * checks if mail for persona got send
      * 
      * @param string $_personas
index d99d2a4..30d2c96 100644 (file)
@@ -1204,10 +1204,9 @@ class Calendar_Controller_Event extends Tinebase_Controller_Record_Abstract impl
                 $recurid = $_alarm->getOption('recurid');
                 $instanceStart = $recurid ? new Tinebase_DateTime(substr($recurid, -19)) : clone $_record->dtstart;
                 $eventLength = $_record->dtstart->diff($_record->dtend);
-        
-                // make sure we hit the next instance
+                
                 $instanceStart->setTimezone($_record->originator_tz);
-                $from = $instanceStart->add($eventLength)->addMinute(1);
+                $from = $instanceStart->add($eventLength);
                 $from->setTimezone('UTC');
             }
             
@@ -1243,7 +1242,6 @@ class Calendar_Controller_Event extends Tinebase_Controller_Record_Abstract impl
         $_alarm->setOption('minutes_before', $minutesBefore);
         $_alarm->alarm_time = $eventStart->subMinute($minutesBefore);
         
-        // don't repeat same alarm @see bug #7430
         if ($_record->rrule && $_alarm->sent_status == Tinebase_Model_Alarm::STATUS_PENDING && $_alarm->alarm_time < $_alarm->sent_time) {
             $this->adoptAlarmTime($_record, $_alarm, 'instance');
         }
index fbe1428..be80cd2 100644 (file)
@@ -356,6 +356,9 @@ class Calendar_Model_Rrule extends Tinebase_Record_Abstract
      */
     public static function computeNextOccurrence($_event, $_exceptions, $_from, $_which = 1)
     {
+        if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__
+                . ' $from = ' . $_from->toString());
+        
         if ($_which === 0 || ($_event->dtstart >= $_from && $_event->dtend > $_from)) {
             return $_event;
         }
@@ -411,6 +414,10 @@ class Calendar_Model_Rrule extends Tinebase_Record_Abstract
             $recurSet->merge(self::computeRecurrenceSet($_event, $exceptions, $from, $until));
             $attempts++;
             
+            // NOTE: computeRecurrenceSet also returns events during $from in some cases, but we need 
+            // to events later than $from.
+            $recurSet = $recurSet->filter(function($event) use ($from) {return $event->dtstart >= $from;});
+            
             if (count($recurSet) >= abs($_which)) {
                 if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__
                     . " found next occurrence after $attempts attempt(s)");
@@ -428,7 +435,10 @@ class Calendar_Model_Rrule extends Tinebase_Record_Abstract
         }
         
         $recurSet->sort('dtstart', $_which > 0 ? 'ASC' : 'DESC');
-        return $recurSet[abs($_which)-1];
+        $nextOccurrence = $recurSet[abs($_which)-1];
+        if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__
+                . ' $nextOccurrence->dtstart = ' . $nextOccurrence->dtstart->toString());
+        return $nextOccurrence;
     }
     
     /**
index ec0a19d..cd0184a 100644 (file)
@@ -538,7 +538,7 @@ class Tinebase_Record_RecordSet implements IteratorAggregate, Countable, ArrayAc
      * @param string $_value
      * @return Tinebase_Record_RecordSet
      */
-    public function filter($_field, $_value, $_valueIsRegExp = FALSE)
+    public function filter($_field, $_value = NULL, $_valueIsRegExp = FALSE)
     {
         $matchingRecords = $this->_getMatchingRecords($_field, $_value, $_valueIsRegExp);
         
@@ -572,22 +572,26 @@ class Tinebase_Record_RecordSet implements IteratorAggregate, Countable, ArrayAc
      */
     protected function _getMatchingRecords($_field, $_value, $_valueIsRegExp = FALSE)
     {
-        // NOTE: indices may lead to wrong results if a record is changed after build of indices
-        if (FALSE && array_key_exists($_field, $this->_indices)) {
-            if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . ' Filtering with indices, expecting fast results ;-)');
-            $valueMap = $this->_indices[$_field];
-        } else {
-            if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . " Filtering field '$_field' of '{$this->_recordClass}' without indices, expecting slow results");
-            $valueMap = $this->$_field;
-        }
-        
-        if ($_valueIsRegExp) {
-            $matchingMap = preg_grep($_value,  $valueMap);
+        if (is_callable($_field)) {
+            $matchingRecords = array_filter($this->_listOfRecords, $_field);
         } else {
-            $matchingMap = array_flip((array)array_keys($valueMap, $_value));
+            // NOTE: indices may lead to wrong results if a record is changed after build of indices
+            if (FALSE && array_key_exists($_field, $this->_indices)) {
+                if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . ' Filtering with indices, expecting fast results ;-)');
+                $valueMap = $this->_indices[$_field];
+            } else {
+                if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . " Filtering field '$_field' of '{$this->_recordClass}' without indices, expecting slow results");
+                $valueMap = $this->$_field;
+            }
+            
+            if ($_valueIsRegExp) {
+                $matchingMap = preg_grep($_value,  $valueMap);
+            } else {
+                $matchingMap = array_flip((array)array_keys($valueMap, $_value));
+            }
+            
+            $matchingRecords = array_intersect_key($this->_listOfRecords, $matchingMap);
         }
-        
-        $matchingRecords = array_intersect_key($this->_listOfRecords, $matchingMap);
         return $matchingRecords;
     }