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

Allow users to "Force accept" package reviews if they own a more general package

Browse source 
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".

The default UI looks something like this:

```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```

By default, force-accepts are not selected.

(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)

Test Plan: {F4314747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17569
This commit is contained in:
epriestley 2017-03-28 09:30:58 -07:00
parent ddc02ce420
commit 2fbc9a52da
9 changed files with 332 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/applications/differential/controller/DifferentialController.php
View file

@ -54,9 +54,7 @@ abstract class DifferentialController extends PhabricatorController {
$toc_view->setAuthorityPackages($packages);
}
// TODO: For Subversion, we should adjust these paths to be relative to
// the repository root where possible.
$paths = mpull($changesets, 'getFilename');
$paths = mpull($changesets, 'getOwnersFilename');
$control_query = id(new PhabricatorOwnersPackageQuery())
->setViewer($viewer)
@ -83,7 +81,7 @@ abstract class DifferentialController extends PhabricatorController {
if ($have_owners) {
$packages = $control_query->getControllingPackagesForPath(
$repository_phid,
$changeset->getFilename());
$changeset->getOwnersFilename());
$item->setPackages($packages);
}

2
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -1857,6 +1857,4 @@ final class DifferentialTransactionEditor
$acting_phid);
}
}

17
src/applications/differential/storage/DifferentialChangeset.php
View file

@ -75,6 +75,23 @@ final class DifferentialChangeset extends DifferentialDAO
return $name;
}
public function getOwnersFilename() {
// TODO: For Subversion, we should adjust these paths to be relative to
// the repository root where possible.
$path = $this->getFilename();
if (!isset($path[0])) {
return '/';
}
if ($path[0] != '/') {
$path = '/'.$path;
}
return $path;
}
public function addUnsavedHunk(DifferentialHunk $hunk) {
if ($this->hunks === self::ATTACHABLE) {
$this->hunks = array();

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

@ -39,6 +39,11 @@ final class DifferentialReviewer
return (phid_get_type($this->getReviewerPHID()) == $user_type);
}
public function isPackage() {
$package_type = PhabricatorOwnersPackagePHIDType::TYPECONST;
return (phid_get_type($this->getReviewerPHID()) == $package_type);
}
public function attachAuthority(PhabricatorUser $user, $has_authority) {
$this->authority[$user->getCacheFragment()] = $has_authority;
return $this;

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

@ -48,6 +48,7 @@ final class DifferentialRevision extends DifferentialDAO
private $customFields = self::ATTACHABLE;
private $drafts = array();
private $flags = array();
private $forceMap = array();
const TABLE_COMMIT = 'differential_commit';
@ -245,6 +246,243 @@ final class DifferentialRevision extends DifferentialDAO
return $this;
}
public function canReviewerForceAccept(
PhabricatorUser $viewer,
DifferentialReviewer $reviewer) {
if (!$reviewer->isPackage()) {
return false;
}
$map = $this->getReviewerForceAcceptMap($viewer);
if (!$map) {
return false;
}
if (isset($map[$reviewer->getReviewerPHID()])) {
return true;
}
return false;
}
private function getReviewerForceAcceptMap(PhabricatorUser $viewer) {
$fragment = $viewer->getCacheFragment();
if (!array_key_exists($fragment, $this->forceMap)) {
$map = $this->newReviewerForceAcceptMap($viewer);
$this->forceMap[$fragment] = $map;
}
return $this->forceMap[$fragment];
}
private function newReviewerForceAcceptMap(PhabricatorUser $viewer) {
$diff = $this->getActiveDiff();
if (!$diff) {
return null;
}
$repository_phid = $diff->getRepositoryPHID();
if (!$repository_phid) {
return null;
}
$paths = array();
try {
$changesets = $diff->getChangesets();
} catch (Exception $ex) {
$changesets = id(new DifferentialChangesetQuery())
->setViewer($viewer)
->withDiffs(array($diff))
->execute();
}
foreach ($changesets as $changeset) {
$paths[] = $changeset->getOwnersFilename();
}
if (!$paths) {
return null;
}
$reviewer_phids = array();
foreach ($this->getReviewers() as $reviewer) {
if (!$reviewer->isPackage()) {
continue;
}
$reviewer_phids[] = $reviewer->getReviewerPHID();
}
if (!$reviewer_phids) {
return null;
}
// Load all the reviewing packages which have control over some of the
// paths in the change. These are packages which the actor may be able
// to force-accept on behalf of.
$control_query = id(new PhabricatorOwnersPackageQuery())
->setViewer($viewer)
->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE))
->withPHIDs($reviewer_phids)
->withControl($repository_phid, $paths);
$control_packages = $control_query->execute();
if (!$control_packages) {
return null;
}
// Load all the packages which have potential control over some of the
// paths in the change and are owned by the actor. These are packages
// which the actor may be able to use their authority over to gain the
// ability to force-accept for other packages. This query doesn't apply
// dominion rules yet, and we'll bypass those rules later on.
$authority_query = id(new PhabricatorOwnersPackageQuery())
->setViewer($viewer)
->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE))
->withAuthorityPHIDs(array($viewer->getPHID()))
->withControl($repository_phid, $paths);
$authority_packages = $authority_query->execute();
if (!$authority_packages) {
return null;
}
$authority_packages = mpull($authority_packages, null, 'getPHID');
// Build a map from each path in the revision to the reviewer packages
// which control it.
$control_map = array();
foreach ($paths as $path) {
$control_packages = $control_query->getControllingPackagesForPath(
$repository_phid,
$path);
// Remove packages which the viewer has authority over. We don't need
// to check these for force-accept because they can just accept them
// normally.
$control_packages = mpull($control_packages, null, 'getPHID');
foreach ($control_packages as $phid => $control_package) {
if (isset($authority_packages[$phid])) {
unset($control_packages[$phid]);
}
}
if (!$control_packages) {
continue;
}
$control_map[$path] = $control_packages;
}
if (!$control_map) {
return null;
}
// From here on out, we only care about paths which we have at least one
// controlling package for.
$paths = array_keys($control_map);
// Now, build a map from each path to the packages which would control it
// if there were no dominion rules.
$authority_map = array();
foreach ($paths as $path) {
$authority_packages = $authority_query->getControllingPackagesForPath(
$repository_phid,
$path,
$ignore_dominion = true);
$authority_map[$path] = mpull($authority_packages, null, 'getPHID');
}
// For each path, find the most general package that the viewer has
// authority over. For example, we'll prefer a package that owns "/" to a
// package that owns "/src/".
$force_map = array();
foreach ($authority_map as $path => $package_map) {
$path_fragments = PhabricatorOwnersPackage::splitPath($path);
$fragment_count = count($path_fragments);
// Find the package that we have authority over which has the most
// general match for this path.
$best_match = null;
$best_package = null;
foreach ($package_map as $package_phid => $package) {
$package_paths = $package->getPathsForRepository($repository_phid);
foreach ($package_paths as $package_path) {
// NOTE: A strength of 0 means "no match". A strength of 1 means
// that we matched "/", so we can not possibly find another stronger
// match.
$strength = $package_path->getPathMatchStrength(
$path_fragments,
$fragment_count);
if (!$strength) {
continue;
}
if ($strength < $best_match || !$best_package) {
$best_match = $strength;
$best_package = $package;
if ($strength == 1) {
break 2;
}
}
}
}
if ($best_package) {
$force_map[$path] = array(
'strength' => $best_match,
'package' => $best_package,
);
}
}
// For each path which the viewer owns a package for, find other packages
// which that authority can be used to force-accept. Once we find a way to
// force-accept a package, we don't need to keep loooking.
$has_control = array();
foreach ($force_map as $path => $spec) {
$path_fragments = PhabricatorOwnersPackage::splitPath($path);
$fragment_count = count($path_fragments);
$authority_strength = $spec['strength'];
$control_packages = $control_map[$path];
foreach ($control_packages as $control_phid => $control_package) {
if (isset($has_control[$control_phid])) {
continue;
}
$control_paths = $control_package->getPathsForRepository(
$repository_phid);
foreach ($control_paths as $control_path) {
$strength = $control_path->getPathMatchStrength(
$path_fragments,
$fragment_count);
if (!$strength) {
continue;
}
if ($strength > $authority_strength) {
$authority = $spec['package'];
$has_control[$control_phid] = array(
'authority' => $authority,
'phid' => $authority->getPHID(),
);
break;
}
}
}
}
// Return a map from packages which may be force accepted to the packages
// which permit that forced acceptance.
return ipull($has_control, 'phid');
}
/* -( PhabricatorPolicyInterface )----------------------------------------- */

38
src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
View file

@ -84,11 +84,18 @@ final class DifferentialRevisionAcceptTransaction
}
}
$default_unchecked = array();
foreach ($reviewers as $reviewer) {
$reviewer_phid = $reviewer->getReviewerPHID();
if (!$reviewer->hasAuthority($viewer)) {
// If the viewer doesn't have authority to act on behalf of a reviewer,
// don't include that reviewer as an option.
continue;
// we check if they can accept by force.
if ($revision->canReviewerForceAccept($viewer, $reviewer)) {
$default_unchecked[$reviewer_phid] = true;
} else {
continue;
}
}
if ($reviewer->isAccepted($diff_phid)) {
@ -97,20 +104,37 @@ final class DifferentialRevisionAcceptTransaction
continue;
}
$reviewer_phid = $reviewer->getReviewerPHID();
$reviewer_phids[$reviewer_phid] = $reviewer_phid;
}
$handles = $viewer->loadHandles($reviewer_phids);
$head = array();
$tail = array();
foreach ($reviewer_phids as $reviewer_phid) {
$options[$reviewer_phid] = pht(
'Accept as %s',
$viewer->renderHandle($reviewer_phid));
$is_force = isset($default_unchecked[$reviewer_phid]);
$value[] = $reviewer_phid;
if ($is_force) {
$tail[] = $reviewer_phid;
$options[$reviewer_phid] = pht(
'Force accept as %s',
$viewer->renderHandle($reviewer_phid));
} else {
$head[] = $reviewer_phid;
$value[] = $reviewer_phid;
$options[$reviewer_phid] = pht(
'Accept as %s',
$viewer->renderHandle($reviewer_phid));
}
}
// Reorder reviewers so "force accept" reviewers come at the end.
$options =
array_select_keys($options, $head) +
array_select_keys($options, $tail);
return array($options, $value);
}

7
src/applications/differential/xaction/DifferentialRevisionActionTransaction.php
View file

@ -122,7 +122,12 @@ abstract class DifferentialRevisionActionTransaction
$field->setActionConflictKey('revision.action');
list($options, $value) = $this->getActionOptions($viewer, $revision);
if (count($options) > 1) {
// Show the options if the user can select on behalf of two or more
// reviewers, or can force-accept on behalf of one or more reviewers.
$can_multi = (count($options) > 1);
$can_force = (count($value) < count($options));
if ($can_multi || $can_force) {
$field->setOptions($options);
$field->setValue($value);
}

26
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
View file

@ -109,7 +109,17 @@ abstract class DifferentialRevisionReviewTransaction
// the desired set of states.
foreach ($revision->getReviewers() as $reviewer) {
if (!$reviewer->hasAuthority($viewer)) {
continue;
$can_force = false;
if ($is_accepted) {
if ($revision->canReviewerForceAccept($viewer, $reviewer)) {
$can_force = true;
}
}
if (!$can_force) {
continue;
}
}
$status = $reviewer->getReviewerStatus();
@ -152,11 +162,21 @@ abstract class DifferentialRevisionReviewTransaction
// reviewers you have authority for. When you resign, you only affect
// yourself.
$with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED);
$with_force = ($status == DifferentialReviewerStatus::STATUS_ACCEPTED);
if ($with_authority) {
foreach ($revision->getReviewers() as $reviewer) {
if ($reviewer->hasAuthority($viewer)) {
$map[$reviewer->getReviewerPHID()] = $status;
if (!$reviewer->hasAuthority($viewer)) {
if (!$with_force) {
continue;
}
if (!$revision->canReviewerForceAccept($viewer, $reviewer)) {
continue;
}
}
$map[$reviewer->getReviewerPHID()] = $status;
}
}

12
src/applications/owners/query/PhabricatorOwnersPackageQuery.php
View file

@ -348,7 +348,10 @@ final class PhabricatorOwnersPackageQuery
*
* @return list<PhabricatorOwnersPackage> List of controlling packages.
*/
public function getControllingPackagesForPath($repository_phid, $path) {
public function getControllingPackagesForPath(
$repository_phid,
$path,
$ignore_dominion = false) {
$path = (string)$path;
if (!isset($this->controlMap[$repository_phid][$path])) {
@ -382,9 +385,14 @@ final class PhabricatorOwnersPackageQuery
}
if ($best_match && $include) {
if ($ignore_dominion) {
$is_weak = false;
} else {
$is_weak = ($package->getDominion() == $weak_dominion);
}
$matches[$package_id] = array(
'strength' => $best_match,
'weak' => ($package->getDominion() == $weak_dominion),
'weak' => $is_weak,
'package' => $package,
);
}