From fef3c778fd4560275787551ceeab8e61aa4ebe58 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 May 2015 11:15:04 -0700 Subject: [PATCH] Replace user "status" with "availability" Summary: Ref T7707. Ref T8183. - Currently, user status is derived by looking at events they //created//. Instead, look at non-cancelled invites they are attending. - Prepare for on-user caching. - Mostly remove "Sporradic" as a status, although I left room for adding more information later. Test Plan: - Called user.query. - Viewed profile. - Viewed hovercard. - Used mentions. - Saw status immediately update when attending/leaving/cancelling a current event. - Created an event ending at 6 PM and an event from 6:10PM - 7PM, saw "Away until 7PM". Reviewers: chad, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8183, T7707 Differential Revision: https://secure.phabricator.com/D12833 --- resources/celerity/map.php | 4 +- .../storage/PhabricatorCalendarEvent.php | 12 --- .../PhabricatorCalendarEventInvitee.php | 4 + .../people/conduit/UserConduitAPIMethod.php | 13 +-- .../conduit/UserQueryConduitAPIMethod.php | 10 +- .../PhabricatorPeopleProfileController.php | 1 + .../PhabricatorUserStatusField.php | 11 +-- ...habricatorPeopleHovercardEventListener.php | 38 +++----- .../markup/PhabricatorMentionRemarkupRule.php | 19 +--- .../phid/PhabricatorPeopleUserPHIDType.php | 17 +--- .../people/query/PhabricatorPeopleQuery.php | 97 ++++++++++++++++--- .../people/storage/PhabricatorUser.php | 63 +++++++++--- .../css/layout/phabricator-hovercard-view.css | 1 + 13 files changed, 173 insertions(+), 117 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0372651ba5..36b773434a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -117,7 +117,7 @@ return array( 'rsrc/css/font/font-source-sans-pro.css' => '8906c07b', 'rsrc/css/font/phui-font-icon-base.css' => '3dad2ae3', 'rsrc/css/layout/phabricator-filetree-view.css' => 'fccf9f82', - 'rsrc/css/layout/phabricator-hovercard-view.css' => '44394670', + 'rsrc/css/layout/phabricator-hovercard-view.css' => 'dd9121a9', 'rsrc/css/layout/phabricator-side-menu-view.css' => 'c1db9e9c', 'rsrc/css/layout/phabricator-source-code-view.css' => '2ceee894', 'rsrc/css/phui/calendar/phui-calendar-day.css' => '38891735', @@ -713,7 +713,7 @@ return array( 'phabricator-filetree-view-css' => 'fccf9f82', 'phabricator-flag-css' => '5337623f', 'phabricator-hovercard' => '14ac66f5', - 'phabricator-hovercard-view-css' => '44394670', + 'phabricator-hovercard-view-css' => 'dd9121a9', 'phabricator-keyboard-shortcut' => '1ae869f2', 'phabricator-keyboard-shortcut-manager' => 'c1700f6f', 'phabricator-main-menu-view' => '663e3810', diff --git a/src/applications/calendar/storage/PhabricatorCalendarEvent.php b/src/applications/calendar/storage/PhabricatorCalendarEvent.php index 3dd984ef7d..c476911ff3 100644 --- a/src/applications/calendar/storage/PhabricatorCalendarEvent.php +++ b/src/applications/calendar/storage/PhabricatorCalendarEvent.php @@ -218,18 +218,6 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO } } - public function loadCurrentStatuses($user_phids) { - if (!$user_phids) { - return array(); - } - - $statuses = $this->loadAllWhere( - 'userPHID IN (%Ls) AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo', - $user_phids); - - return mpull($statuses, null, 'getUserPHID'); - } - public function getInvitees() { return $this->assertAttached($this->invitees); } diff --git a/src/applications/calendar/storage/PhabricatorCalendarEventInvitee.php b/src/applications/calendar/storage/PhabricatorCalendarEventInvitee.php index 9701a782e7..b1d2b2d34c 100644 --- a/src/applications/calendar/storage/PhabricatorCalendarEventInvitee.php +++ b/src/applications/calendar/storage/PhabricatorCalendarEventInvitee.php @@ -38,6 +38,10 @@ final class PhabricatorCalendarEventInvitee extends PhabricatorCalendarDAO ) + parent::getConfiguration(); } + public function isAttending() { + return ($this->getStatus() == self::STATUS_ATTENDING); + } + public function isUninvited() { if ($this->getStatus() == self::STATUS_UNINVITED) { return true; diff --git a/src/applications/people/conduit/UserConduitAPIMethod.php b/src/applications/people/conduit/UserConduitAPIMethod.php index 7a30ee84dd..82da171f43 100644 --- a/src/applications/people/conduit/UserConduitAPIMethod.php +++ b/src/applications/people/conduit/UserConduitAPIMethod.php @@ -6,9 +6,7 @@ abstract class UserConduitAPIMethod extends ConduitAPIMethod { return PhabricatorApplication::getByClass('PhabricatorPeopleApplication'); } - protected function buildUserInformationDictionary( - PhabricatorUser $user, - PhabricatorCalendarEvent $current_status = null) { + protected function buildUserInformationDictionary(PhabricatorUser $user) { $roles = array(); if ($user->getIsDisabled()) { @@ -48,9 +46,12 @@ abstract class UserConduitAPIMethod extends ConduitAPIMethod { 'roles' => $roles, ); - if ($current_status) { - $return['currentStatus'] = $current_status->getTextStatus(); - $return['currentStatusUntil'] = $current_status->getDateTo(); + // TODO: Modernize this once we have a more long-term view of what the + // data looks like. + $until = $user->getAwayUntil(); + if ($until) { + $return['currentStatus'] = 'away'; + $return['currentStatusUntil'] = $until; } return $return; diff --git a/src/applications/people/conduit/UserQueryConduitAPIMethod.php b/src/applications/people/conduit/UserQueryConduitAPIMethod.php index 5c81fd5d4f..c480ae0c73 100644 --- a/src/applications/people/conduit/UserQueryConduitAPIMethod.php +++ b/src/applications/people/conduit/UserQueryConduitAPIMethod.php @@ -43,7 +43,8 @@ final class UserQueryConduitAPIMethod extends UserConduitAPIMethod { $query = id(new PhabricatorPeopleQuery()) ->setViewer($request->getUser()) - ->needProfileImage(true); + ->needProfileImage(true) + ->needAvailability(true); if ($usernames) { $query->withUsernames($usernames); @@ -68,14 +69,9 @@ final class UserQueryConduitAPIMethod extends UserConduitAPIMethod { } $users = $query->execute(); - $statuses = id(new PhabricatorCalendarEvent())->loadCurrentStatuses( - mpull($users, 'getPHID')); - $results = array(); foreach ($users as $user) { - $results[] = $this->buildUserInformationDictionary( - $user, - idx($statuses, $user->getPHID())); + $results[] = $this->buildUserInformationDictionary($user); } return $results; } diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php index 688d798fbf..8404e56d91 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -20,6 +20,7 @@ final class PhabricatorPeopleProfileController ->setViewer($viewer) ->withUsernames(array($this->username)) ->needProfileImage(true) + ->needAvailability(true) ->executeOne(); if (!$user) { return new Aphront404Response(); diff --git a/src/applications/people/customfield/PhabricatorUserStatusField.php b/src/applications/people/customfield/PhabricatorUserStatusField.php index 9ff97054c3..69e16d83db 100644 --- a/src/applications/people/customfield/PhabricatorUserStatusField.php +++ b/src/applications/people/customfield/PhabricatorUserStatusField.php @@ -29,16 +29,7 @@ final class PhabricatorUserStatusField public function renderPropertyViewValue(array $handles) { $user = $this->getObject(); $viewer = $this->requireViewer(); - - $statuses = id(new PhabricatorCalendarEvent()) - ->loadCurrentStatuses(array($user->getPHID())); - if (!$statuses) { - return pht('Available'); - } - - $status = head($statuses); - - return $status->getTerseSummary($viewer); + return $user->getAvailabilityDescription($viewer); } } diff --git a/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php b/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php index 3bb955cf42..f2b957f363 100644 --- a/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php +++ b/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php @@ -26,12 +26,15 @@ final class PhabricatorPeopleHovercardEventListener return; } - $profile = $user->loadUserProfile(); + // Reload to get availability. + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withIDs(array($user->getID())) + ->needAvailability(true) + ->executeOne(); $hovercard->setTitle($user->getUsername()); - $hovercard->setDetail(pht('%s - %s.', $user->getRealname(), - nonempty($profile->getTitle(), - pht('No title was found befitting of this rare specimen')))); + $hovercard->setDetail($user->getRealName()); if ($user->getIsDisabled()) { $hovercard->addField(pht('Account'), pht('Disabled')); @@ -40,29 +43,14 @@ final class PhabricatorPeopleHovercardEventListener } else if (PhabricatorApplication::isClassInstalledForViewer( 'PhabricatorCalendarApplication', $viewer)) { - $statuses = id(new PhabricatorCalendarEvent())->loadCurrentStatuses( - array($user->getPHID())); - if ($statuses) { - $current_status = reset($statuses); - $dateto = phabricator_datetime($current_status->getDateTo(), $user); - $hovercard->addField(pht('Status'), - $current_status->getDescription()); - $hovercard->addField(pht('Until'), - $dateto); - } else { - $hovercard->addField(pht('Status'), pht('Available')); - } + $hovercard->addField( + pht('Status'), + $user->getAvailabilityDescription($viewer)); } - $hovercard->addField(pht('User since'), - phabricator_date($user->getDateCreated(), $user)); - - if ($profile->getBlurb()) { - $hovercard->addField(pht('Blurb'), - id(new PhutilUTF8StringTruncator()) - ->setMaximumGlyphs(120) - ->truncateString($profile->getBlurb())); - } + $hovercard->addField( + pht('User Since'), + phabricator_date($user->getDateCreated(), $viewer)); $event->setValue('hovercard', $hovercard); } diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 0451e29312..eb224aae48 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -72,16 +72,9 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { $users = id(new PhabricatorPeopleQuery()) ->setViewer($this->getEngine()->getConfig('viewer')) ->withUsernames($usernames) + ->needAvailability(true) ->execute(); - if ($users) { - $user_statuses = id(new PhabricatorCalendarEvent()) - ->loadCurrentStatuses(mpull($users, 'getPHID')); - $user_statuses = mpull($user_statuses, null, 'getUserPHID'); - } else { - $user_statuses = array(); - } - $actual_users = array(); $mentioned_key = self::KEY_MENTIONED; @@ -156,14 +149,8 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { if (!$user->isUserActivated()) { $tag->setDotColor(PHUITagView::COLOR_GREY); } else { - $status = idx($user_statuses, $user->getPHID()); - if ($status) { - $status = $status->getStatus(); - if ($status == PhabricatorCalendarEvent::STATUS_AWAY) { - $tag->setDotColor(PHUITagView::COLOR_RED); - } else if ($status == PhabricatorCalendarEvent::STATUS_AWAY) { - $tag->setDotColor(PHUITagView::COLOR_ORANGE); - } + if ($user->getAwayUntil()) { + $tag->setDotColor(PHUITagView::COLOR_RED); } } } diff --git a/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php b/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php index 16453987c9..ba271e6d92 100644 --- a/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php +++ b/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php @@ -27,7 +27,7 @@ final class PhabricatorPeopleUserPHIDType extends PhabricatorPHIDType { return id(new PhabricatorPeopleQuery()) ->withPHIDs($phids) ->needProfileImage(true) - ->needStatus(true); + ->needAvailability(true); } public function loadHandles( @@ -48,18 +48,9 @@ final class PhabricatorPeopleUserPHIDType extends PhabricatorPHIDType { if (!$user->isUserActivated()) { $availability = PhabricatorObjectHandle::AVAILABILITY_DISABLED; } else { - if ($user->hasStatus()) { - // NOTE: This first call returns an event; then we get the event - // status. - $status = $user->getStatus()->getStatus(); - switch ($status) { - case PhabricatorCalendarEvent::STATUS_AWAY: - $availability = PhabricatorObjectHandle::AVAILABILITY_NONE; - break; - case PhabricatorCalendarEvent::STATUS_SPORADIC: - $availability = PhabricatorObjectHandle::AVAILABILITY_PARTIAL; - break; - } + $until = $user->getAwayUntil(); + if ($until) { + $availability = PhabricatorObjectHandle::AVAILABILITY_NONE; } } diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php index b8b7ad1e29..2bcb62c2bb 100644 --- a/src/applications/people/query/PhabricatorPeopleQuery.php +++ b/src/applications/people/query/PhabricatorPeopleQuery.php @@ -20,7 +20,7 @@ final class PhabricatorPeopleQuery private $needPrimaryEmail; private $needProfile; private $needProfileImage; - private $needStatus; + private $needAvailability; public function withIDs(array $ids) { $this->ids = $ids; @@ -102,8 +102,8 @@ final class PhabricatorPeopleQuery return $this; } - public function needStatus($need) { - $this->needStatus = $need; + public function needAvailability($need) { + $this->needAvailability = $need; return $this; } @@ -200,15 +200,11 @@ final class PhabricatorPeopleQuery } } - if ($this->needStatus) { - $user_list = mpull($users, null, 'getPHID'); - $statuses = id(new PhabricatorCalendarEvent())->loadCurrentStatuses( - array_keys($user_list)); - foreach ($user_list as $phid => $user) { - $status = idx($statuses, $phid); - if ($status) { - $user->attachStatus($status); - } + if ($this->needAvailability) { + // TODO: Add caching. + $rebuild = $users; + if ($rebuild) { + $this->rebuildAvailabilityCache($rebuild); } } @@ -375,5 +371,82 @@ final class PhabricatorPeopleQuery ); } + private function rebuildAvailabilityCache(array $rebuild) { + $rebuild = mpull($rebuild, null, 'getPHID'); + + // Limit the window we look at because far-future events are largely + // irrelevant and this makes the cache cheaper to build and allows it to + // self-heal over time. + $min_range = PhabricatorTime::getNow(); + $max_range = $min_range + phutil_units('72 hours in seconds'); + + $events = id(new PhabricatorCalendarEventQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withInvitedPHIDs(array_keys($rebuild)) + ->withIsCancelled(false) + ->withDateRange($min_range, $max_range) + ->execute(); + + // Group all the events by invited user. Only examine events that users + // are actually attending. + $map = array(); + foreach ($events as $event) { + foreach ($event->getInvitees() as $invitee) { + if (!$invitee->isAttending()) { + continue; + } + + $invitee_phid = $invitee->getInviteePHID(); + if (!isset($rebuild[$invitee_phid])) { + continue; + } + + $map[$invitee_phid][] = $event; + } + } + + // Margin between meetings: pretend meetings start earlier than they do + // so we mark you away for the entire time if you have a series of + // back-to-back meetings, even if they don't strictly overlap. + $margin = phutil_units('15 minutes in seconds'); + + foreach ($rebuild as $phid => $user) { + $events = idx($map, $phid, array()); + + $cursor = $min_range; + if ($events) { + // Find the next time when the user has no meetings. If we move forward + // because of an event, we check again for events after that one ends. + while (true) { + foreach ($events as $event) { + $from = ($event->getDateFrom() - $margin); + $to = $event->getDateTo(); + if (($from <= $cursor) && ($to > $cursor)) { + $cursor = $to; + continue 2; + } + } + break; + } + } + + if ($cursor > $min_range) { + $availability = array( + 'until' => $cursor, + ); + $availability_ttl = $cursor; + } else { + $availability = null; + $availability_ttl = $max_range; + } + + // Never TTL the cache to longer than the maximum range we examined. + $availability_ttl = min($availability_ttl, $max_range); + + // TODO: Write the cache. + + $user->attachAvailability($availability); + } + } } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 3fbfac062f..89de29747b 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1,6 +1,7 @@ status = $status; - return $this; - } - - public function getStatus() { - return $this->assertAttached($this->status); - } - - public function hasStatus() { - return $this->status !== self::ATTACHABLE; - } - public function attachProfileImageURI($uri) { $this->profileImage = $uri; return $this; @@ -727,6 +715,53 @@ EOBODY; } +/* -( Availability )------------------------------------------------------- */ + + + /** + * @task availability + */ + public function attachAvailability($availability) { + $this->availability = $availability; + return $this; + } + + + /** + * Get the timestamp the user is away until, if they are currently away. + * + * @return int|null Epoch timestamp, or `null` if the user is not away. + * @task availability + */ + public function getAwayUntil() { + $availability = $this->availability; + + $this->assertAttached($availability); + if (!$availability) { + return null; + } + + return idx($availability, 'until'); + } + + + /** + * Describe the user's availability. + * + * @param PhabricatorUser Viewing user. + * @return string Human-readable description of away status. + * @task availability + */ + public function getAvailabilityDescription(PhabricatorUser $viewer) { + $until = $this->getAwayUntil(); + if ($until) { + return pht('Away until %s', phabricator_datetime($until, $viewer)); + } else { + return pht('Available'); + } + } + + /* -( Profile Image Cache )------------------------------------------------ */ diff --git a/webroot/rsrc/css/layout/phabricator-hovercard-view.css b/webroot/rsrc/css/layout/phabricator-hovercard-view.css index 05d520fb97..54cd365019 100644 --- a/webroot/rsrc/css/layout/phabricator-hovercard-view.css +++ b/webroot/rsrc/css/layout/phabricator-hovercard-view.css @@ -77,6 +77,7 @@ height: 50px; background-position: center; background-repeat: no-repeat; + background-size: 100%; } .phabricator-hovercard-tail { width: 396px;