revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-29 10:12:41 +01:00
Code Issues Releases Wiki Activity

Move many "reviewers" readers to new storage

Browse source 
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
This commit is contained in:
epriestley 2017-03-20 12:35:30 -07:00
parent 8ad5d28686
commit dccd799b1b
10 changed files with 123 additions and 167 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/__phutil_library_map__.php
View file

@ -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',

9
src/applications/differential/customfield/DifferentialCustomField.php
View file

@ -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();
}

5
src/applications/differential/customfield/DifferentialProjectReviewersField.php
View file

@ -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;
}

5
src/applications/differential/customfield/DifferentialReviewersField.php
View file

@ -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;
}

60
src/applications/differential/query/DifferentialRevisionQuery.php
View file

@ -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;
}
}
}

24
src/applications/differential/storage/DifferentialReviewer.php
View file

@ -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);
}
}

56
src/applications/differential/storage/DifferentialReviewerProxy.php
View file

@ -1,56 +0,0 @@
<?php
final class DifferentialReviewerProxy extends Phobject {
private $reviewerPHID;
private $status;
private $diffID;
private $authority = array();
public function __construct($reviewer_phid, array $edge_data) {
$this->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,
);
}
}

12
src/applications/differential/storage/DifferentialRevision.php
View file

@ -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;
}

23
src/applications/differential/storage/DifferentialTransaction.php
View file

@ -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;
}

94
src/applications/differential/view/DifferentialReviewersView.php
View file

@ -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;
}
}