From 943080a4de3fb27f0952ef626228dd46c34182ab Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 18 Oct 2013 12:47:36 -0700 Subject: [PATCH] Make Phrequent time accounting aware of the stack Summary: Ref T3569. Fixes T3567. When figuring out how much time has been spent on an object, subtract "preemptive" events which interrupted the object. Also, make the UI look vaguely sane: {F72773} Test Plan: Added a bunch of unit tests, mucked around in the UI. Reviewers: btrahan Reviewed By: btrahan CC: hach-que, skyronic, aran Maniphest Tasks: T3567, T3569 Differential Revision: https://secure.phabricator.com/D7349 --- src/__phutil_library_map__.php | 4 + .../event/PhrequentUIEventListener.php | 95 ++++++----- .../query/PhrequentUserTimeQuery.php | 102 +++++++----- .../phrequent/storage/PhrequentTimeBlock.php | 155 ++++++++++++++++++ .../phrequent/storage/PhrequentUserTime.php | 37 ++++- .../__tests__/PhrequentTimeBlockTestCase.php | 128 +++++++++++++++ 6 files changed, 433 insertions(+), 88 deletions(-) create mode 100644 src/applications/phrequent/storage/PhrequentTimeBlock.php create mode 100644 src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 41633c43ca..1878561a61 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1958,6 +1958,8 @@ phutil_register_library_map(array( 'PhrequentDAO' => 'applications/phrequent/storage/PhrequentDAO.php', 'PhrequentListController' => 'applications/phrequent/controller/PhrequentListController.php', 'PhrequentSearchEngine' => 'applications/phrequent/query/PhrequentSearchEngine.php', + 'PhrequentTimeBlock' => 'applications/phrequent/storage/PhrequentTimeBlock.php', + 'PhrequentTimeBlockTestCase' => 'applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php', 'PhrequentTrackController' => 'applications/phrequent/controller/PhrequentTrackController.php', 'PhrequentTrackableInterface' => 'applications/phrequent/interface/PhrequentTrackableInterface.php', 'PhrequentUIEventListener' => 'applications/phrequent/event/PhrequentUIEventListener.php', @@ -4230,6 +4232,8 @@ phutil_register_library_map(array( 1 => 'PhabricatorApplicationSearchResultsControllerInterface', ), 'PhrequentSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhrequentTimeBlock' => 'Phobject', + 'PhrequentTimeBlockTestCase' => 'PhabricatorTestCase', 'PhrequentTrackController' => 'PhrequentController', 'PhrequentUIEventListener' => 'PhutilEventListener', 'PhrequentUserTime' => diff --git a/src/applications/phrequent/event/PhrequentUIEventListener.php b/src/applications/phrequent/event/PhrequentUIEventListener.php index df8146c55c..aa934a39aa 100644 --- a/src/applications/phrequent/event/PhrequentUIEventListener.php +++ b/src/applications/phrequent/event/PhrequentUIEventListener.php @@ -61,9 +61,9 @@ final class PhrequentUIEventListener $event->setValue('actions', $actions); } - private function handlePropertyEvent($event) { - $user = $event->getUser(); - $object = $event->getValue('object'); + private function handlePropertyEvent($ui_event) { + $user = $ui_event->getUser(); + $object = $ui_event->getValue('object'); if (!$object || !$object->getPHID()) { // No object, or the object has no PHID yet.. @@ -75,51 +75,64 @@ final class PhrequentUIEventListener return; } - $depth = false; + $events = id(new PhrequentUserTimeQuery()) + ->setViewer($user) + ->withObjectPHIDs(array($object->getPHID())) + ->needPreemptingEvents(true) + ->execute(); + $event_groups = mgroup($events, 'getUserPHID'); - $stack = PhrequentUserTimeQuery::loadUserStack($user); - if ($stack) { - $stack = array_values($stack); - for ($ii = 0; $ii < count($stack); $ii++) { - if ($stack[$ii]->getObjectPHID() == $object->getPHID()) { - $depth = ($ii + 1); - break; - } - } - } - - $time_spent = PhrequentUserTimeQuery::getTotalTimeSpentOnObject( - $object->getPHID()); - - if (!$depth && !$time_spent) { + if (!$events) { return; } - require_celerity_resource('phrequent-css'); + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($user) + ->withPHIDs(array_keys($event_groups)) + ->execute(); - $property = array(); - if ($depth == 1) { - $property[] = phutil_tag( - 'div', - array( - 'class' => 'phrequent-tracking-property phrequent-active', - ), - pht('Currently Tracking')); - } else if ($depth > 1) { - $property[] = phutil_tag( - 'div', - array( - 'class' => 'phrequent-tracking-property phrequent-on-stack', - ), - pht('On Stack')); + $status_view = new PHUIStatusListView(); + + foreach ($event_groups as $user_phid => $event_group) { + $item = new PHUIStatusItemView(); + $item->setTarget($handles[$user_phid]->renderLink()); + + $state = 'stopped'; + foreach ($event_group as $event) { + if ($event->getDateEnded() === null) { + if ($event->isPreempted()) { + $state = 'suspended'; + } else { + $state = 'active'; + break; + } + } + } + + switch ($state) { + case 'active': + $item->setIcon('time-green', pht('Working Now')); + break; + case 'suspended': + $item->setIcon('time-yellow', pht('Interrupted')); + break; + case 'stopped': + $item->setIcon('time-orange', pht('Not Working Now')); + break; + } + + $block = new PhrequentTimeBlock($event_group); + $item->setNote( + phabricator_format_relative_time( + $block->getTimeSpentOnObject( + $object->getPHID(), + time()))); + + $status_view->addItem($item); } - if ($time_spent) { - $property[] = phabricator_format_relative_time_detailed($time_spent); - } - - $view = $event->getValue('view'); - $view->addProperty(pht('Time Spent'), $property); + $view = $ui_event->getValue('view'); + $view->addProperty(pht('Time Spent'), $status_view); } } diff --git a/src/applications/phrequent/query/PhrequentUserTimeQuery.php b/src/applications/phrequent/query/PhrequentUserTimeQuery.php index a6321906df..dedcce86d0 100644 --- a/src/applications/phrequent/query/PhrequentUserTimeQuery.php +++ b/src/applications/phrequent/query/PhrequentUserTimeQuery.php @@ -21,6 +21,8 @@ final class PhrequentUserTimeQuery private $order = self::ORDER_ID_ASC; private $ended = self::ENDED_ALL; + private $needPreemptingEvents; + public function withUserPHIDs($user_phids) { $this->userPHIDs = $user_phids; return $this; @@ -41,6 +43,11 @@ final class PhrequentUserTimeQuery return $this; } + public function needPreemptingEvents($need_events) { + $this->needPreemptingEvents = $need_events; + return $this; + } + private function buildWhereClause(AphrontDatabaseConnection $conn) { $where = array(); @@ -150,6 +157,61 @@ final class PhrequentUserTimeQuery return $usertime->loadAllFromArray($data); } + protected function didFilterPage(array $page) { + if ($this->needPreemptingEvents) { + $usertime = new PhrequentUserTime(); + $conn_r = $usertime->establishConnection('r'); + + $preempt = array(); + foreach ($page as $event) { + $preempt[] = qsprintf( + $conn_r, + '(userPHID = %s AND + (dateStarted BETWEEN %d AND %d) AND + (dateEnded IS NULL OR dateEnded > %d))', + $event->getUserPHID(), + $event->getDateStarted(), + nonempty($event->getDateEnded(), PhabricatorTime::getNow()), + $event->getDateStarted()); + } + + $preempting_events = queryfx_all( + $conn_r, + 'SELECT * FROM %T WHERE %Q ORDER BY dateStarted ASC, id ASC', + $usertime->getTableName(), + implode(' OR ', $preempt)); + $preempting_events = $usertime->loadAllFromArray($preempting_events); + + $preempting_events = mgroup($preempting_events, 'getUserPHID'); + + foreach ($page as $event) { + $e_start = $event->getDateStarted(); + $e_end = $event->getDateEnded(); + + $select = array(); + $user_events = idx($preempting_events, $event->getUserPHID(), array()); + foreach ($user_events as $u_event) { + if ($u_event->getID() == $event->getID()) { + // Don't allow an event to preempt itself. + continue; + } + + $u_start = $u_event->getDateStarted(); + $u_end = $u_event->getDateEnded(); + + if (($u_start >= $e_start) && ($u_end <= $e_end) && + ($u_end === null || $u_end > $e_start)) { + $select[] = $u_event; + } + } + + $event->attachPreemptingEvents($select); + } + } + + return $page; + } + /* -( Helper Functions ) --------------------------------------------------- */ public static function getEndedSearchOptions() { @@ -204,46 +266,6 @@ final class PhrequentUserTimeQuery return $count['N'] > 0; } - public static function loadUserStack(PhabricatorUser $user) { - if (!$user->isLoggedIn()) { - return array(); - } - - return id(new PhrequentUserTime())->loadAllWhere( - 'userPHID = %s AND dateEnded IS NULL - ORDER BY dateStarted DESC, id DESC', - $user->getPHID()); - } - - public static function getTotalTimeSpentOnObject($phid) { - $usertime_dao = new PhrequentUserTime(); - $conn = $usertime_dao->establishConnection('r'); - - // First calculate all the time spent where the - // usertime blocks have ended. - $sum_ended = queryfx_one( - $conn, - 'SELECT SUM(usertime.dateEnded - usertime.dateStarted) N '. - 'FROM %T usertime '. - 'WHERE usertime.objectPHID = %s '. - 'AND usertime.dateEnded IS NOT NULL', - $usertime_dao->getTableName(), - $phid); - - // Now calculate the time spent where the usertime - // blocks have not yet ended. - $sum_not_ended = queryfx_one( - $conn, - 'SELECT SUM(UNIX_TIMESTAMP() - usertime.dateStarted) N '. - 'FROM %T usertime '. - 'WHERE usertime.objectPHID = %s '. - 'AND usertime.dateEnded IS NULL', - $usertime_dao->getTableName(), - $phid); - - return $sum_ended['N'] + $sum_not_ended['N']; - } - public static function getUserTimeSpentOnObject( PhabricatorUser $user, $phid) { diff --git a/src/applications/phrequent/storage/PhrequentTimeBlock.php b/src/applications/phrequent/storage/PhrequentTimeBlock.php new file mode 100644 index 0000000000..866275ac08 --- /dev/null +++ b/src/applications/phrequent/storage/PhrequentTimeBlock.php @@ -0,0 +1,155 @@ +events = $events; + } + + public function getTimeSpentOnObject($phid, $now) { + $ranges = idx($this->getObjectTimeRanges($now), $phid, array()); + + $sum = 0; + foreach ($ranges as $range) { + $sum += ($range[1] - $range[0]); + } + + return $sum; + } + + public function getObjectTimeRanges($now) { + $ranges = array(); + + $object_ranges = array(); + foreach ($this->events as $event) { + + // First, convert each event's preempting stack into a linear timeline + // of events. + + $timeline = array(); + $timeline[] = array( + 'at' => $event->getDateStarted(), + 'type' => 'start', + ); + $timeline[] = array( + 'at' => nonempty($event->getDateEnded(), $now), + 'type' => 'end', + ); + + $base_phid = $event->getObjectPHID(); + + $preempts = $event->getPreemptingEvents(); + + foreach ($preempts as $preempt) { + $same_object = ($preempt->getObjectPHID() == $base_phid); + $timeline[] = array( + 'at' => $preempt->getDateStarted(), + 'type' => $same_object ? 'start' : 'push', + ); + $timeline[] = array( + 'at' => nonempty($preempt->getDateEnded(), $now), + 'type' => $same_object ? 'end' : 'pop', + ); + } + + // Now, figure out how much time was actually spent working on the + // object. + + $timeline = isort($timeline, 'at'); + + $stack = array(); + $depth = null; + + $ranges = array(); + foreach ($timeline as $timeline_event) { + switch ($timeline_event['type']) { + case 'start': + $stack[] = $depth; + $depth = 0; + $range_start = $timeline_event['at']; + break; + case 'end': + if ($depth == 0) { + $ranges[] = array($range_start, $timeline_event['at']); + } + $depth = array_pop($stack); + break; + case 'push': + if ($depth == 0) { + $ranges[] = array($range_start, $timeline_event['at']); + } + $depth++; + break; + case 'pop': + $depth--; + if ($depth == 0) { + $range_start = $timeline_event['at']; + } + break; + } + } + + $object_ranges[$base_phid][] = $ranges; + } + + // Finally, collapse all the ranges so we don't double-count time. + + foreach ($object_ranges as $phid => $ranges) { + $object_ranges[$phid] = self::mergeTimeRanges(array_mergev($ranges)); + } + + return $object_ranges; + } + + + /** + * Merge a list of time ranges (pairs of `` epochs) so that no + * elements overlap. For example, the ranges: + * + * array( + * array(50, 150), + * array(100, 175), + * ); + * + * ...are merged to: + * + * array( + * array(50, 175), + * ); + * + * This is used to avoid double-counting time on objects which had timers + * started multiple times. + * + * @param list> List of possibly overlapping time ranges. + * @return list> Nonoverlapping time ranges. + */ + public static function mergeTimeRanges(array $ranges) { + $ranges = isort($ranges, 0); + + $result = array(); + + $current = null; + foreach ($ranges as $key => $range) { + if ($current === null) { + $current = $range; + continue; + } + + if ($range[0] <= $current[1]) { + $current[1] = max($range[1], $current[1]); + continue; + } + + $result[] = $current; + $current = $range; + } + + $result[] = $current; + + return $result; + } + +} diff --git a/src/applications/phrequent/storage/PhrequentUserTime.php b/src/applications/phrequent/storage/PhrequentUserTime.php index 3fd32593e4..5d4ca08c80 100644 --- a/src/applications/phrequent/storage/PhrequentUserTime.php +++ b/src/applications/phrequent/storage/PhrequentUserTime.php @@ -1,8 +1,5 @@ preemptingEvents = $events; + return $this; + } + + public function getPreemptingEvents() { + return $this->assertAttached($this->preemptingEvents); + } + + public function isPreempted() { + if ($this->getDateEnded() !== null) { + return false; + } + foreach ($this->getPreemptingEvents() as $event) { + if ($event->getDateEnded() === null && + $event->getObjectPHID() != $this->getObjectPHID()) { + return true; + } + } + return false; } } diff --git a/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php b/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php new file mode 100644 index 0000000000..0c110c36d5 --- /dev/null +++ b/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php @@ -0,0 +1,128 @@ +assertEqual($expect, PhrequentTimeBlock::mergeTimeRanges($input)); + + + // Identical ranges. + $input = array( + array(1, 1), + array(1, 1), + array(1, 1), + ); + $expect = array( + array(1, 1), + ); + + $this->assertEqual($expect, PhrequentTimeBlock::mergeTimeRanges($input)); + + + // Range which is a strict subset of another range. + $input = array( + array(2, 7), + array(1, 10), + ); + $expect = array( + array(1, 10), + ); + + $this->assertEqual($expect, PhrequentTimeBlock::mergeTimeRanges($input)); + + + // These are discontinuous and should not be merged. + $input = array( + array(5, 6), + array(7, 8), + ); + $expect = array( + array(5, 6), + array(7, 8), + ); + + $this->assertEqual($expect, PhrequentTimeBlock::mergeTimeRanges($input)); + + + // These overlap only on an edge, but should merge. + $input = array( + array(5, 6), + array(6, 7), + ); + $expect = array( + array(5, 7), + ); + + $this->assertEqual($expect, PhrequentTimeBlock::mergeTimeRanges($input)); + } + + public function testPreemptingEvents() { + + // Roughly, this is: got into work, started T1, had a meeting from 10-11, + // left the office around 2 to meet a client at a coffee shop, worked on + // T1 again for about 40 minutes, had the meeting from 3-3:30, finished up + // on T1, headed back to the office, got another hour of work in, and + // headed home. + + $event = $this->newEvent('T1', 900, 1700); + + $event->attachPreemptingEvents( + array( + $this->newEvent('meeting', 1000, 1100), + $this->newEvent('offsite', 1400, 1600), + $this->newEvent('T1', 1420, 1580), + $this->newEvent('offsite meeting', 1500, 1550), + )); + + $block = new PhrequentTimeBlock(array($event)); + + $ranges = $block->getObjectTimeRanges(1800); + $this->assertEqual( + array( + 'T1' => array( + array(900, 1000), // Before morning meeting. + array(1100, 1400), // After morning meeting. + array(1420, 1500), // Coffee, before client meeting. + array(1550, 1580), // Coffee, after client meeting. + array(1600, 1700), // After returning from off site. + ), + ), + $ranges); + + + $event = $this->newEvent('T2', 100, 300); + $event->attachPreemptingEvents( + array( + $this->newEvent('meeting', 200, null), + )); + + $block = new PhrequentTimeBlock(array($event)); + + $ranges = $block->getObjectTimeRanges(1000); + $this->assertEqual( + array( + 'T2' => array( + array(100, 200), + ), + ), + $ranges); + } + + private function newEvent($object_phid, $start_time, $end_time) { + return id(new PhrequentUserTime()) + ->setObjectPHID($object_phid) + ->setDateStarted($start_time) + ->setDateEnded($end_time); + } + +}