D17517.diff
No OneTemporary
Actions

Size

18 KB

Referenced Files

None

Subscribers

None

D17517.diff
View Options

	diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
	--- a/src/__phutil_library_map__.php
	+++ b/src/__phutil_library_map__.php
	@@ -494,7 +494,6 @@
	'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 @@
	'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
	--- a/src/applications/differential/customfield/DifferentialCustomField.php
	+++ b/src/applications/differential/customfield/DifferentialCustomField.php
	@@ -70,6 +70,15 @@
	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
	--- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php
	+++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php
	@@ -42,7 +42,10 @@
	->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
	--- a/src/applications/differential/customfield/DifferentialReviewersField.php
	+++ b/src/applications/differential/customfield/DifferentialReviewersField.php
	@@ -43,7 +43,10 @@
	->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
	--- a/src/applications/differential/query/DifferentialRevisionQuery.php
	+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
	@@ -605,11 +605,11 @@
	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 @@
	}

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

	private function loadReviewerAuthority(
	array $revisions,
	- array $edges,
	+ array $reviewers,
	$allow_self) {

	$revision_map = mpull($revisions, null, 'getPHID');
	@@ -1045,9 +1047,9 @@
	$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 @@
	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
	--- a/src/applications/differential/storage/DifferentialReviewer.php
	+++ b/src/applications/differential/storage/DifferentialReviewer.php
	@@ -6,10 +6,11 @@
	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 @@
	) + 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
	--- a/src/applications/differential/storage/DifferentialReviewerProxy.php
	+++ /dev/null
	@@ -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,
	- );
	- }
	-
	-}
	diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
	--- a/src/applications/differential/storage/DifferentialRevision.php
	+++ b/src/applications/differential/storage/DifferentialRevision.php
	@@ -296,15 +296,6 @@
	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 @@
	}

	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
	--- a/src/applications/differential/storage/DifferentialTransaction.php
	+++ b/src/applications/differential/storage/DifferentialTransaction.php
	@@ -212,13 +212,6 @@
	$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 @@

	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 @@
	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
	--- a/src/applications/differential/view/DifferentialReviewersView.php
	+++ b/src/applications/differential/view/DifferentialReviewersView.php
	@@ -7,7 +7,7 @@
	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 @@
	$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 @@
	}
	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 @@
	$item->setIcon(
	PHUIStatusItemView::ICON_QUESTION,
	'bluegrey',
	- pht('%s?', $reviewer->getStatus()));
	+ pht('%s?', $reviewer->getReviewerStatus()));
	break;

	}
	@@ -128,4 +114,26 @@
	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;
	+ }
	+
	}

File Metadata

Mime Type: text/plain
Expires: Wed, Mar 26, 5:38 AM (1 w, 3 d ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 7566520
Default Alt Text: D17517.diff (18 KB)

D17517.diffNo OneTemporaryActions

D17517.diffView Options

File Metadata

Event Timeline

D17517.diff
No OneTemporary
Actions

D17517.diff
View Options