From dccd799b1b44852d928a91347d6925a1efaf21e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Mar 2017 12:35:30 -0700 Subject: [PATCH] Move many "reviewers" readers to new storage Summary: Ref T10967. When we query for revisions with particular reviewers, use the new table to drive the query. When we load revisions for use in the application, also use the new table to drive the query. This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over. (This also changes the icon for "commented" from a question mark to a speech bubble.) Test Plan: - Viewed revision lists and detail views on old and new code, saw identical outcomes. - Updated revisions, accepted/rejected/commented on revisions. - Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating. - Grepped for removed methods (like `getEdgeData()` and `getDiffID()`). Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17517 --- src/__phutil_library_map__.php | 2 - .../customfield/DifferentialCustomField.php | 9 ++ .../DifferentialProjectReviewersField.php | 5 +- .../DifferentialReviewersField.php | 5 +- .../query/DifferentialRevisionQuery.php | 60 ++++++------ .../storage/DifferentialReviewer.php | 24 ++++- .../storage/DifferentialReviewerProxy.php | 56 ----------- .../storage/DifferentialRevision.php | 12 +-- .../storage/DifferentialTransaction.php | 23 ----- .../view/DifferentialReviewersView.php | 94 ++++++++++--------- 10 files changed, 123 insertions(+), 167 deletions(-) delete mode 100644 src/applications/differential/storage/DifferentialReviewerProxy.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 645a9ee6b2..66dfda8a65 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -494,7 +494,6 @@ phutil_register_library_map(array( 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', 'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php', 'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php', - 'DifferentialReviewerProxy' => 'applications/differential/storage/DifferentialReviewerProxy.php', 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php', 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php', @@ -5255,7 +5254,6 @@ phutil_register_library_map(array( 'DifferentialReviewer' => 'DifferentialDAO', 'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType', - 'DifferentialReviewerProxy' => 'Phobject', 'DifferentialReviewerStatus' => 'Phobject', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction', 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'DifferentialReviewersHeraldAction', diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php index 0382034a0e..0b2d330775 100644 --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -70,6 +70,15 @@ abstract class DifferentialCustomField return array(); } + protected function getActiveDiff() { + $object = $this->getObject(); + try { + return $object->getActiveDiff(); + } catch (Exception $ex) { + return null; + } + } + public function getRequiredHandlePHIDsForRevisionHeaderWarnings() { return array(); } diff --git a/src/applications/differential/customfield/DifferentialProjectReviewersField.php b/src/applications/differential/customfield/DifferentialProjectReviewersField.php index 87ea0c34a3..5fe5258cdb 100644 --- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php +++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php @@ -42,7 +42,10 @@ final class DifferentialProjectReviewersField ->setReviewers($reviewers) ->setHandles($handles); - // TODO: Active diff stuff. + $diff = $this->getActiveDiff(); + if ($diff) { + $view->setActiveDiff($diff); + } return $view; } diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index ad96a22b88..4d7ebfc691 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -43,7 +43,10 @@ final class DifferentialReviewersField ->setReviewers($reviewers) ->setHandles($handles); - // TODO: Active diff stuff. + $diff = $this->getActiveDiff(); + if ($diff) { + $view->setActiveDiff($diff); + } return $view; } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 5dff433e9c..c3fb98e607 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -605,11 +605,11 @@ final class DifferentialRevisionQuery if ($this->reviewers) { $joins[] = qsprintf( $conn_r, - 'JOIN %T e_reviewers ON e_reviewers.src = r.phid '. - 'AND e_reviewers.type = %s '. - 'AND e_reviewers.dst in (%Ls)', - PhabricatorEdgeConfig::TABLE_NAME_EDGE, - DifferentialRevisionHasReviewerEdgeType::EDGECONST, + 'JOIN %T reviewer ON reviewer.revisionPHID = r.phid + AND reviewer.reviewerStatus != %s + AND reviewer.reviewerPHID in (%Ls)', + id(new DifferentialReviewer())->getTableName(), + DifferentialReviewerStatus::STATUS_RESIGNED, $this->reviewers); } @@ -972,21 +972,28 @@ final class DifferentialRevisionQuery } private function loadReviewers( - AphrontDatabaseConnection $conn_r, + AphrontDatabaseConnection $conn, array $revisions) { assert_instances_of($revisions, 'DifferentialRevision'); - $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; - $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(mpull($revisions, 'getPHID')) - ->withEdgeTypes(array($edge_type)) - ->needEdgeData(true) - ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST) - ->execute(); + $reviewer_table = new DifferentialReviewer(); + $reviewer_rows = queryfx_all( + $conn, + 'SELECT * FROM %T WHERE revisionPHID IN (%Ls) + ORDER BY id ASC', + $reviewer_table->getTableName(), + mpull($revisions, 'getPHID')); + $reviewer_list = $reviewer_table->loadAllFromArray($reviewer_rows); + $reviewer_map = mgroup($reviewer_list, 'getRevisionPHID'); + + foreach ($reviewer_map as $key => $reviewers) { + $reviewer_map[$key] = mpull($reviewers, null, 'getReviewerPHID'); + } $viewer = $this->getViewer(); $viewer_phid = $viewer->getPHID(); + $allow_key = 'differential.allow-self-accept'; $allow_self = PhabricatorEnv::getEnvConfig($allow_key); @@ -994,18 +1001,13 @@ final class DifferentialRevisionQuery if ($this->needReviewerAuthority && $viewer_phid) { $authority = $this->loadReviewerAuthority( $revisions, - $edges, + $reviewer_map, $allow_self); } foreach ($revisions as $revision) { - $revision_edges = $edges[$revision->getPHID()][$edge_type]; - $reviewers = array(); - foreach ($revision_edges as $reviewer_phid => $edge) { - $reviewer = new DifferentialReviewerProxy( - $reviewer_phid, - $edge['data']); - + $reviewers = idx($reviewer_map, $revision->getPHID(), array()); + foreach ($reviewers as $reviewer_phid => $reviewer) { if ($this->needReviewerAuthority) { if (!$viewer_phid) { // Logged-out users never have authority. @@ -1031,7 +1033,7 @@ final class DifferentialRevisionQuery private function loadReviewerAuthority( array $revisions, - array $edges, + array $reviewers, $allow_self) { $revision_map = mpull($revisions, null, 'getPHID'); @@ -1045,9 +1047,9 @@ final class DifferentialRevisionQuery $package_type = PhabricatorOwnersPackagePHIDType::TYPECONST; $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; - foreach ($edges as $src => $types) { + foreach ($reviewers as $revision_phid => $reviewer_list) { if (!$allow_self) { - if ($revision_map[$src]->getAuthorPHID() == $viewer_phid) { + if ($revision_map[$revision_phid]->getAuthorPHID() == $viewer_phid) { // If self-review isn't permitted, the user will never have // authority over projects on revisions they authored because you // can't accept your own revisions, so we don't need to load any @@ -1055,14 +1057,14 @@ final class DifferentialRevisionQuery continue; } } - $edge_data = idx($types, $edge_type, array()); - foreach ($edge_data as $dst => $data) { - $phid_type = phid_get_type($dst); + + foreach ($reviewer_list as $reviewer_phid => $reviewer) { + $phid_type = phid_get_type($reviewer_phid); if ($phid_type == $project_type) { - $project_phids[] = $dst; + $project_phids[] = $reviewer_phid; } if ($phid_type == $package_type) { - $package_phids[] = $dst; + $package_phids[] = $reviewer_phid; } } } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 731fc9be14..5904ae2d7f 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -6,10 +6,11 @@ final class DifferentialReviewer protected $revisionPHID; protected $reviewerPHID; protected $reviewerStatus; - protected $lastActionDiffPHID; protected $lastCommentDiffPHID; + private $authority = array(); + protected function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( @@ -26,4 +27,25 @@ final class DifferentialReviewer ) + parent::getConfiguration(); } + public function getStatus() { + // TODO: This is an older method for compatibility with some callers + // which have not yet been cleaned up. + return $this->getReviewerStatus(); + } + + public function isUser() { + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + return (phid_get_type($this->getReviewerPHID()) == $user_type); + } + + public function attachAuthority(PhabricatorUser $user, $has_authority) { + $this->authority[$user->getCacheFragment()] = $has_authority; + return $this; + } + + public function hasAuthority(PhabricatorUser $viewer) { + $cache_fragment = $viewer->getCacheFragment(); + return $this->assertAttachedKey($this->authority, $cache_fragment); + } + } diff --git a/src/applications/differential/storage/DifferentialReviewerProxy.php b/src/applications/differential/storage/DifferentialReviewerProxy.php deleted file mode 100644 index bf38ee27e3..0000000000 --- a/src/applications/differential/storage/DifferentialReviewerProxy.php +++ /dev/null @@ -1,56 +0,0 @@ -reviewerPHID = $reviewer_phid; - $this->status = idx($edge_data, 'status'); - $this->diffID = idx($edge_data, 'diff'); - } - - public function getReviewerPHID() { - return $this->reviewerPHID; - } - - public function getStatus() { - return $this->status; - } - - public function getDiffID() { - return $this->diffID; - } - - public function isUser() { - $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; - return (phid_get_type($this->getReviewerPHID()) == $user_type); - } - - public function attachAuthority(PhabricatorUser $user, $has_authority) { - $this->authority[$user->getPHID()] = $has_authority; - return $this; - } - - public function hasAuthority(PhabricatorUser $viewer) { - // It would be nice to use assertAttachedKey() here, but we don't extend - // PhabricatorLiskDAO, and faking that seems sketchy. - - $viewer_phid = $viewer->getPHID(); - if (!array_key_exists($viewer_phid, $this->authority)) { - throw new Exception(pht('You must %s first!', 'attachAuthority()')); - } - return $this->authority[$viewer_phid]; - } - - public function getEdgeData() { - return array( - 'status' => $this->status, - 'diffID' => $this->diffID, - ); - } - -} diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index df43043d6d..ff826a36b7 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -296,15 +296,6 @@ final class DifferentialRevision extends DifferentialDAO return idx($this->relationships, $relation, array()); } - public function getPrimaryReviewer() { - $reviewers = $this->getReviewers(); - $last = $this->lastReviewerPHID; - if (!$last || !in_array($last, $reviewers)) { - return head($this->getReviewers()); - } - return $last; - } - public function getHashes() { return $this->assertAttached($this->hashes); } @@ -406,8 +397,7 @@ final class DifferentialRevision extends DifferentialDAO } public function attachReviewerStatus(array $reviewers) { - assert_instances_of($reviewers, 'DifferentialReviewerProxy'); - + assert_instances_of($reviewers, 'DifferentialReviewer'); $this->reviewerStatus = $reviewers; return $this; } diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 7342f35498..868a24ebb0 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -212,13 +212,6 @@ final class DifferentialTransaction $tags[] = self::MAILTAG_UPDATED; } break; - case PhabricatorTransactions::TYPE_EDGE: - switch ($this->getMetadataValue('edge:type')) { - case DifferentialRevisionHasReviewerEdgeType::EDGECONST: - $tags[] = self::MAILTAG_REVIEWERS; - break; - } - break; case PhabricatorTransactions::TYPE_COMMENT: case self::TYPE_INLINE: $tags[] = self::MAILTAG_COMMENT; @@ -598,14 +591,6 @@ final class DifferentialTransaction public function getNoEffectDescription() { switch ($this->getTransactionType()) { - case PhabricatorTransactions::TYPE_EDGE: - switch ($this->getMetadataValue('edge:type')) { - case DifferentialRevisionHasReviewerEdgeType::EDGECONST: - return pht( - 'The reviewers you are trying to add are already reviewing '. - 'this revision.'); - } - break; case self::TYPE_ACTION: switch ($this->getNewValue()) { case DifferentialAction::ACTION_CLOSE: @@ -624,18 +609,10 @@ final class DifferentialTransaction return pht('This revision already requires changes.'); case DifferentialAction::ACTION_REQUEST: return pht('Review is already requested for this revision.'); - case DifferentialAction::ACTION_RESIGN: - return pht( - 'You can not resign from this revision because you are not '. - 'a reviewer.'); case DifferentialAction::ACTION_CLAIM: return pht( 'You can not commandeer this revision because you already own '. 'it.'); - case DifferentialAction::ACTION_ACCEPT: - return pht('You have already accepted this revision.'); - case DifferentialAction::ACTION_REJECT: - return pht('You have already requested changes to this revision.'); } break; } diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index fd558d85f1..54b7e35257 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -7,7 +7,7 @@ final class DifferentialReviewersView extends AphrontView { private $diff; public function setReviewers(array $reviewers) { - assert_instances_of($reviewers, 'DifferentialReviewerProxy'); + assert_instances_of($reviewers, 'DifferentialReviewer'); $this->reviewers = $reviewers; return $this; } @@ -31,47 +31,54 @@ final class DifferentialReviewersView extends AphrontView { $phid = $reviewer->getReviewerPHID(); $handle = $this->handles[$phid]; - // If we're missing either the diff or action information for the - // reviewer, render information as current. - $is_current = (!$this->diff) || - (!$reviewer->getDiffID()) || - ($this->diff->getID() == $reviewer->getDiffID()); + $action_phid = $reviewer->getLastActionDiffPHID(); + $is_current_action = $this->isCurrent($action_phid); + + $comment_phid = $reviewer->getLastCommentDiffPHID(); + $is_current_comment = $this->isCurrent($comment_phid); $item = new PHUIStatusItemView(); $item->setHighlighted($reviewer->hasAuthority($viewer)); - switch ($reviewer->getStatus()) { + switch ($reviewer->getReviewerStatus()) { case DifferentialReviewerStatus::STATUS_ADDED: - $item->setIcon( - PHUIStatusItemView::ICON_OPEN, - 'bluegrey', - pht('Review Requested')); + if ($comment_phid) { + if ($is_current_comment) { + $item->setIcon( + 'fa-comment', + 'blue', + pht('Commented')); + } else { + $item->setIcon( + 'fa-comment-o', + 'bluegrey', + pht('Commented Previously')); + } + } else { + $item->setIcon( + PHUIStatusItemView::ICON_OPEN, + 'bluegrey', + pht('Review Requested')); + } break; case DifferentialReviewerStatus::STATUS_ACCEPTED: - if ($is_current) { + if ($is_current_action) { $item->setIcon( PHUIStatusItemView::ICON_ACCEPT, 'green', pht('Accepted')); } else { $item->setIcon( - PHUIStatusItemView::ICON_ACCEPT, + 'fa-check-circle-o', 'bluegrey', pht('Accepted Prior Diff')); } break; - case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER: - $item->setIcon( - 'fa-check-circle-o', - 'bluegrey', - pht('Accepted Prior Diff')); - break; - case DifferentialReviewerStatus::STATUS_REJECTED: - if ($is_current) { + if ($is_current_action) { $item->setIcon( PHUIStatusItemView::ICON_REJECT, 'red', @@ -84,27 +91,6 @@ final class DifferentialReviewersView extends AphrontView { } break; - case DifferentialReviewerStatus::STATUS_REJECTED_OLDER: - $item->setIcon( - 'fa-times-circle-o', - 'bluegrey', - pht('Rejected Prior Diff')); - break; - - case DifferentialReviewerStatus::STATUS_COMMENTED: - if ($is_current) { - $item->setIcon( - 'fa-question-circle', - 'blue', - pht('Commented')); - } else { - $item->setIcon( - 'fa-question-circle-o', - 'bluegrey', - pht('Commented Previously')); - } - break; - case DifferentialReviewerStatus::STATUS_BLOCKING: $item->setIcon( PHUIStatusItemView::ICON_MINUS, @@ -116,7 +102,7 @@ final class DifferentialReviewersView extends AphrontView { $item->setIcon( PHUIStatusItemView::ICON_QUESTION, 'bluegrey', - pht('%s?', $reviewer->getStatus())); + pht('%s?', $reviewer->getReviewerStatus())); break; } @@ -128,4 +114,26 @@ final class DifferentialReviewersView extends AphrontView { return $view; } + private function isCurrent($action_phid) { + if (!$this->diff) { + echo "A\n"; + return true; + } + + if (!$action_phid) { + return true; + } + + $diff_phid = $this->diff->getPHID(); + if (!$diff_phid) { + return true; + } + + if ($diff_phid == $action_phid) { + return true; + } + + return false; + } + }