Move many "reviewers" readers to new storage · recaptime.dev/phorge@dccd799

-2

src/__phutil_library_map__.php

··· 494 494 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', 495 495 'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php', 496 496 'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php', 497 - 'DifferentialReviewerProxy' => 'applications/differential/storage/DifferentialReviewerProxy.php', 498 497 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', 499 498 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php', 500 499 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php', ··· 5255 5254 'DifferentialReviewer' => 'DifferentialDAO', 5256 5255 'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 5257 5256 'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType', 5258 - 'DifferentialReviewerProxy' => 'Phobject', 5259 5257 'DifferentialReviewerStatus' => 'Phobject', 5260 5258 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction', 5261 5259 'DifferentialReviewersAddBlockingSelfHeraldAction' => 'DifferentialReviewersHeraldAction',

+9

src/applications/differential/customfield/DifferentialCustomField.php

··· 70 70 return array(); 71 71 } 72 72 73 + protected function getActiveDiff() { 74 + $object = $this->getObject(); 75 + try { 76 + return $object->getActiveDiff(); 77 + } catch (Exception $ex) { 78 + return null; 79 + } 80 + } 81 + 73 82 public function getRequiredHandlePHIDsForRevisionHeaderWarnings() { 74 83 return array(); 75 84 }

+4 -1

src/applications/differential/customfield/DifferentialProjectReviewersField.php

··· 42 42 ->setReviewers($reviewers) 43 43 ->setHandles($handles); 44 44 45 - // TODO: Active diff stuff. 45 + $diff = $this->getActiveDiff(); 46 + if ($diff) { 47 + $view->setActiveDiff($diff); 48 + } 46 49 47 50 return $view; 48 51 }

+4 -1

src/applications/differential/customfield/DifferentialReviewersField.php

··· 43 43 ->setReviewers($reviewers) 44 44 ->setHandles($handles); 45 45 46 - // TODO: Active diff stuff. 46 + $diff = $this->getActiveDiff(); 47 + if ($diff) { 48 + $view->setActiveDiff($diff); 49 + } 47 50 48 51 return $view; 49 52 }

+31 -29

src/applications/differential/query/DifferentialRevisionQuery.php

··· 605 605 if ($this->reviewers) { 606 606 $joins[] = qsprintf( 607 607 $conn_r, 608 - 'JOIN %T e_reviewers ON e_reviewers.src = r.phid '. 609 - 'AND e_reviewers.type = %s '. 610 - 'AND e_reviewers.dst in (%Ls)', 611 - PhabricatorEdgeConfig::TABLE_NAME_EDGE, 612 - DifferentialRevisionHasReviewerEdgeType::EDGECONST, 608 + 'JOIN %T reviewer ON reviewer.revisionPHID = r.phid 609 + AND reviewer.reviewerStatus != %s 610 + AND reviewer.reviewerPHID in (%Ls)', 611 + id(new DifferentialReviewer())->getTableName(), 612 + DifferentialReviewerStatus::STATUS_RESIGNED, 613 613 $this->reviewers); 614 614 } 615 615 ··· 972 972 } 973 973 974 974 private function loadReviewers( 975 - AphrontDatabaseConnection $conn_r, 975 + AphrontDatabaseConnection $conn, 976 976 array $revisions) { 977 977 978 978 assert_instances_of($revisions, 'DifferentialRevision'); 979 - $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; 979 + 980 + $reviewer_table = new DifferentialReviewer(); 981 + $reviewer_rows = queryfx_all( 982 + $conn, 983 + 'SELECT * FROM %T WHERE revisionPHID IN (%Ls) 984 + ORDER BY id ASC', 985 + $reviewer_table->getTableName(), 986 + mpull($revisions, 'getPHID')); 987 + $reviewer_list = $reviewer_table->loadAllFromArray($reviewer_rows); 988 + $reviewer_map = mgroup($reviewer_list, 'getRevisionPHID'); 980 989 981 - $edges = id(new PhabricatorEdgeQuery()) 982 - ->withSourcePHIDs(mpull($revisions, 'getPHID')) 983 - ->withEdgeTypes(array($edge_type)) 984 - ->needEdgeData(true) 985 - ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST) 986 - ->execute(); 990 + foreach ($reviewer_map as $key => $reviewers) { 991 + $reviewer_map[$key] = mpull($reviewers, null, 'getReviewerPHID'); 992 + } 987 993 988 994 $viewer = $this->getViewer(); 989 995 $viewer_phid = $viewer->getPHID(); 996 + 990 997 $allow_key = 'differential.allow-self-accept'; 991 998 $allow_self = PhabricatorEnv::getEnvConfig($allow_key); 992 999 ··· 994 1001 if ($this->needReviewerAuthority && $viewer_phid) { 995 1002 $authority = $this->loadReviewerAuthority( 996 1003 $revisions, 997 - $edges, 1004 + $reviewer_map, 998 1005 $allow_self); 999 1006 } 1000 1007 1001 1008 foreach ($revisions as $revision) { 1002 - $revision_edges = $edges[$revision->getPHID()][$edge_type]; 1003 - $reviewers = array(); 1004 - foreach ($revision_edges as $reviewer_phid => $edge) { 1005 - $reviewer = new DifferentialReviewerProxy( 1006 - $reviewer_phid, 1007 - $edge['data']); 1008 - 1009 + $reviewers = idx($reviewer_map, $revision->getPHID(), array()); 1010 + foreach ($reviewers as $reviewer_phid => $reviewer) { 1009 1011 if ($this->needReviewerAuthority) { 1010 1012 if (!$viewer_phid) { 1011 1013 // Logged-out users never have authority. ··· 1031 1033 1032 1034 private function loadReviewerAuthority( 1033 1035 array $revisions, 1034 - array $edges, 1036 + array $reviewers, 1035 1037 $allow_self) { 1036 1038 1037 1039 $revision_map = mpull($revisions, null, 'getPHID'); ··· 1045 1047 $package_type = PhabricatorOwnersPackagePHIDType::TYPECONST; 1046 1048 1047 1049 $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; 1048 - foreach ($edges as $src => $types) { 1050 + foreach ($reviewers as $revision_phid => $reviewer_list) { 1049 1051 if (!$allow_self) { 1050 - if ($revision_map[$src]->getAuthorPHID() == $viewer_phid) { 1052 + if ($revision_map[$revision_phid]->getAuthorPHID() == $viewer_phid) { 1051 1053 // If self-review isn't permitted, the user will never have 1052 1054 // authority over projects on revisions they authored because you 1053 1055 // can't accept your own revisions, so we don't need to load any ··· 1055 1057 continue; 1056 1058 } 1057 1059 } 1058 - $edge_data = idx($types, $edge_type, array()); 1059 - foreach ($edge_data as $dst => $data) { 1060 - $phid_type = phid_get_type($dst); 1060 + 1061 + foreach ($reviewer_list as $reviewer_phid => $reviewer) { 1062 + $phid_type = phid_get_type($reviewer_phid); 1061 1063 if ($phid_type == $project_type) { 1062 - $project_phids[] = $dst; 1064 + $project_phids[] = $reviewer_phid; 1063 1065 } 1064 1066 if ($phid_type == $package_type) { 1065 - $package_phids[] = $dst; 1067 + $package_phids[] = $reviewer_phid; 1066 1068 } 1067 1069 } 1068 1070 }

+23 -1

src/applications/differential/storage/DifferentialReviewer.php

··· 6 6 protected $revisionPHID; 7 7 protected $reviewerPHID; 8 8 protected $reviewerStatus; 9 - 10 9 protected $lastActionDiffPHID; 11 10 protected $lastCommentDiffPHID; 11 + 12 + private $authority = array(); 12 13 13 14 protected function getConfiguration() { 14 15 return array( ··· 24 25 ), 25 26 ), 26 27 ) + parent::getConfiguration(); 28 + } 29 + 30 + public function getStatus() { 31 + // TODO: This is an older method for compatibility with some callers 32 + // which have not yet been cleaned up. 33 + return $this->getReviewerStatus(); 34 + } 35 + 36 + public function isUser() { 37 + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; 38 + return (phid_get_type($this->getReviewerPHID()) == $user_type); 39 + } 40 + 41 + public function attachAuthority(PhabricatorUser $user, $has_authority) { 42 + $this->authority[$user->getCacheFragment()] = $has_authority; 43 + return $this; 44 + } 45 + 46 + public function hasAuthority(PhabricatorUser $viewer) { 47 + $cache_fragment = $viewer->getCacheFragment(); 48 + return $this->assertAttachedKey($this->authority, $cache_fragment); 27 49 } 28 50 29 51 }

-56

src/applications/differential/storage/DifferentialReviewerProxy.php

··· 1 - <?php 2 - 3 - final class DifferentialReviewerProxy extends Phobject { 4 - 5 - private $reviewerPHID; 6 - private $status; 7 - private $diffID; 8 - private $authority = array(); 9 - 10 - public function __construct($reviewer_phid, array $edge_data) { 11 - $this->reviewerPHID = $reviewer_phid; 12 - $this->status = idx($edge_data, 'status'); 13 - $this->diffID = idx($edge_data, 'diff'); 14 - } 15 - 16 - public function getReviewerPHID() { 17 - return $this->reviewerPHID; 18 - } 19 - 20 - public function getStatus() { 21 - return $this->status; 22 - } 23 - 24 - public function getDiffID() { 25 - return $this->diffID; 26 - } 27 - 28 - public function isUser() { 29 - $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; 30 - return (phid_get_type($this->getReviewerPHID()) == $user_type); 31 - } 32 - 33 - public function attachAuthority(PhabricatorUser $user, $has_authority) { 34 - $this->authority[$user->getPHID()] = $has_authority; 35 - return $this; 36 - } 37 - 38 - public function hasAuthority(PhabricatorUser $viewer) { 39 - // It would be nice to use assertAttachedKey() here, but we don't extend 40 - // PhabricatorLiskDAO, and faking that seems sketchy. 41 - 42 - $viewer_phid = $viewer->getPHID(); 43 - if (!array_key_exists($viewer_phid, $this->authority)) { 44 - throw new Exception(pht('You must %s first!', 'attachAuthority()')); 45 - } 46 - return $this->authority[$viewer_phid]; 47 - } 48 - 49 - public function getEdgeData() { 50 - return array( 51 - 'status' => $this->status, 52 - 'diffID' => $this->diffID, 53 - ); 54 - } 55 - 56 - }

+1 -11

src/applications/differential/storage/DifferentialRevision.php

··· 296 296 return idx($this->relationships, $relation, array()); 297 297 } 298 298 299 - public function getPrimaryReviewer() { 300 - $reviewers = $this->getReviewers(); 301 - $last = $this->lastReviewerPHID; 302 - if (!$last || !in_array($last, $reviewers)) { 303 - return head($this->getReviewers()); 304 - } 305 - return $last; 306 - } 307 - 308 299 public function getHashes() { 309 300 return $this->assertAttached($this->hashes); 310 301 } ··· 406 397 } 407 398 408 399 public function attachReviewerStatus(array $reviewers) { 409 - assert_instances_of($reviewers, 'DifferentialReviewerProxy'); 410 - 400 + assert_instances_of($reviewers, 'DifferentialReviewer'); 411 401 $this->reviewerStatus = $reviewers; 412 402 return $this; 413 403 }

-23

src/applications/differential/storage/DifferentialTransaction.php

··· 212 212 $tags[] = self::MAILTAG_UPDATED; 213 213 } 214 214 break; 215 - case PhabricatorTransactions::TYPE_EDGE: 216 - switch ($this->getMetadataValue('edge:type')) { 217 - case DifferentialRevisionHasReviewerEdgeType::EDGECONST: 218 - $tags[] = self::MAILTAG_REVIEWERS; 219 - break; 220 - } 221 - break; 222 215 case PhabricatorTransactions::TYPE_COMMENT: 223 216 case self::TYPE_INLINE: 224 217 $tags[] = self::MAILTAG_COMMENT; ··· 598 591 599 592 public function getNoEffectDescription() { 600 593 switch ($this->getTransactionType()) { 601 - case PhabricatorTransactions::TYPE_EDGE: 602 - switch ($this->getMetadataValue('edge:type')) { 603 - case DifferentialRevisionHasReviewerEdgeType::EDGECONST: 604 - return pht( 605 - 'The reviewers you are trying to add are already reviewing '. 606 - 'this revision.'); 607 - } 608 - break; 609 594 case self::TYPE_ACTION: 610 595 switch ($this->getNewValue()) { 611 596 case DifferentialAction::ACTION_CLOSE: ··· 624 609 return pht('This revision already requires changes.'); 625 610 case DifferentialAction::ACTION_REQUEST: 626 611 return pht('Review is already requested for this revision.'); 627 - case DifferentialAction::ACTION_RESIGN: 628 - return pht( 629 - 'You can not resign from this revision because you are not '. 630 - 'a reviewer.'); 631 612 case DifferentialAction::ACTION_CLAIM: 632 613 return pht( 633 614 'You can not commandeer this revision because you already own '. 634 615 'it.'); 635 - case DifferentialAction::ACTION_ACCEPT: 636 - return pht('You have already accepted this revision.'); 637 - case DifferentialAction::ACTION_REJECT: 638 - return pht('You have already requested changes to this revision.'); 639 616 } 640 617 break; 641 618 }

+51 -43

src/applications/differential/view/DifferentialReviewersView.php

··· 7 7 private $diff; 8 8 9 9 public function setReviewers(array $reviewers) { 10 - assert_instances_of($reviewers, 'DifferentialReviewerProxy'); 10 + assert_instances_of($reviewers, 'DifferentialReviewer'); 11 11 $this->reviewers = $reviewers; 12 12 return $this; 13 13 } ··· 31 31 $phid = $reviewer->getReviewerPHID(); 32 32 $handle = $this->handles[$phid]; 33 33 34 - // If we're missing either the diff or action information for the 35 - // reviewer, render information as current. 36 - $is_current = (!$this->diff) || 37 - (!$reviewer->getDiffID()) || 38 - ($this->diff->getID() == $reviewer->getDiffID()); 34 + $action_phid = $reviewer->getLastActionDiffPHID(); 35 + $is_current_action = $this->isCurrent($action_phid); 36 + 37 + $comment_phid = $reviewer->getLastCommentDiffPHID(); 38 + $is_current_comment = $this->isCurrent($comment_phid); 39 39 40 40 $item = new PHUIStatusItemView(); 41 41 42 42 $item->setHighlighted($reviewer->hasAuthority($viewer)); 43 43 44 - switch ($reviewer->getStatus()) { 44 + switch ($reviewer->getReviewerStatus()) { 45 45 case DifferentialReviewerStatus::STATUS_ADDED: 46 - $item->setIcon( 47 - PHUIStatusItemView::ICON_OPEN, 48 - 'bluegrey', 49 - pht('Review Requested')); 46 + if ($comment_phid) { 47 + if ($is_current_comment) { 48 + $item->setIcon( 49 + 'fa-comment', 50 + 'blue', 51 + pht('Commented')); 52 + } else { 53 + $item->setIcon( 54 + 'fa-comment-o', 55 + 'bluegrey', 56 + pht('Commented Previously')); 57 + } 58 + } else { 59 + $item->setIcon( 60 + PHUIStatusItemView::ICON_OPEN, 61 + 'bluegrey', 62 + pht('Review Requested')); 63 + } 50 64 break; 51 65 52 66 case DifferentialReviewerStatus::STATUS_ACCEPTED: 53 - if ($is_current) { 67 + if ($is_current_action) { 54 68 $item->setIcon( 55 69 PHUIStatusItemView::ICON_ACCEPT, 56 70 'green', 57 71 pht('Accepted')); 58 72 } else { 59 73 $item->setIcon( 60 - PHUIStatusItemView::ICON_ACCEPT, 74 + 'fa-check-circle-o', 61 75 'bluegrey', 62 76 pht('Accepted Prior Diff')); 63 77 } 64 - break; 65 - 66 - case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER: 67 - $item->setIcon( 68 - 'fa-check-circle-o', 69 - 'bluegrey', 70 - pht('Accepted Prior Diff')); 71 78 break; 72 79 73 80 case DifferentialReviewerStatus::STATUS_REJECTED: 74 - if ($is_current) { 81 + if ($is_current_action) { 75 82 $item->setIcon( 76 83 PHUIStatusItemView::ICON_REJECT, 77 84 'red', ··· 84 91 } 85 92 break; 86 93 87 - case DifferentialReviewerStatus::STATUS_REJECTED_OLDER: 88 - $item->setIcon( 89 - 'fa-times-circle-o', 90 - 'bluegrey', 91 - pht('Rejected Prior Diff')); 92 - break; 93 - 94 - case DifferentialReviewerStatus::STATUS_COMMENTED: 95 - if ($is_current) { 96 - $item->setIcon( 97 - 'fa-question-circle', 98 - 'blue', 99 - pht('Commented')); 100 - } else { 101 - $item->setIcon( 102 - 'fa-question-circle-o', 103 - 'bluegrey', 104 - pht('Commented Previously')); 105 - } 106 - break; 107 - 108 94 case DifferentialReviewerStatus::STATUS_BLOCKING: 109 95 $item->setIcon( 110 96 PHUIStatusItemView::ICON_MINUS, ··· 116 102 $item->setIcon( 117 103 PHUIStatusItemView::ICON_QUESTION, 118 104 'bluegrey', 119 - pht('%s?', $reviewer->getStatus())); 105 + pht('%s?', $reviewer->getReviewerStatus())); 120 106 break; 121 107 122 108 } ··· 126 112 } 127 113 128 114 return $view; 115 + } 116 + 117 + private function isCurrent($action_phid) { 118 + if (!$this->diff) { 119 + echo "A\n"; 120 + return true; 121 + } 122 + 123 + if (!$action_phid) { 124 + return true; 125 + } 126 + 127 + $diff_phid = $this->diff->getPHID(); 128 + if (!$diff_phid) { 129 + return true; 130 + } 131 + 132 + if ($diff_phid == $action_phid) { 133 + return true; 134 + } 135 + 136 + return false; 129 137 } 130 138 131 139 }

Configure Feed

Configure Feed