@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Move determination of reviewer authority into DifferentialRevisionQuery
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
+130
-42
+1
src/applications/differential/controller/DifferentialRevisionViewController.php
+1
src/applications/differential/controller/DifferentialRevisionViewController.php
+2
-16
src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php
+2
-16
src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php
···
27
27
return null;
28
28
}
29
29
30
-
$highlight = array();
31
-
if ($this->getUser()->getPHID() != $this->getRevision()->getAuthorPHID()) {
32
-
// Determine which of these projects the viewer is a member of, so we can
33
-
// highlight them. (If the viewer is the author, skip this since they
34
-
// can't review.)
35
-
$phids = mpull($reviewers, 'getReviewerPHID');
36
-
$projects = id(new PhabricatorProjectQuery())
37
-
->setViewer($this->getUser())
38
-
->withPHIDs($phids)
39
-
->withMemberPHIDs(array($this->getUser()->getPHID()))
40
-
->execute();
41
-
$highlight = mpull($projects, 'getPHID');
42
-
}
43
-
44
30
$view = id(new DifferentialReviewersView())
31
+
->setUser($this->getUser())
45
32
->setReviewers($reviewers)
46
-
->setHandles($this->getLoadedHandles())
47
-
->setHighlightPHIDs($highlight);
33
+
->setHandles($this->getLoadedHandles());
48
34
49
35
$diff = $this->getRevision()->loadActiveDiff();
50
36
if ($diff) {
+1
-1
src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
+1
-1
src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
···
32
32
}
33
33
34
34
$view = id(new DifferentialReviewersView())
35
+
->setUser($this->getUser())
35
36
->setReviewers($reviewers)
36
-
->setHighlightPHIDs(array($this->getUser()->getPHID()))
37
37
->setHandles($this->getLoadedHandles());
38
38
39
39
$diff = $this->getRevision()->loadActiveDiff();
+100
-7
src/applications/differential/query/DifferentialRevisionQuery.php
+100
-7
src/applications/differential/query/DifferentialRevisionQuery.php
···
59
59
private $needCommitPHIDs = false;
60
60
private $needHashes = false;
61
61
private $needReviewerStatus = false;
62
+
private $needReviewerAuthority;
62
63
63
64
private $buildingGlobalOrder;
64
65
···
347
348
}
348
349
349
350
351
+
/**
352
+
* Request information about the viewer's authority to act on behalf of each
353
+
* reviewer. In particular, they have authority to act on behalf of projects
354
+
* they are a member of.
355
+
*
356
+
* @param bool True to load and attach authority.
357
+
* @return this
358
+
* @task config
359
+
*/
360
+
public function needReviewerAuthority($need_reviewer_authority) {
361
+
$this->needReviewerAuthority = $need_reviewer_authority;
362
+
return $this;
363
+
}
364
+
365
+
350
366
/* -( Query Execution )---------------------------------------------------- */
351
367
352
368
···
450
466
$this->loadHashes($conn_r, $revisions);
451
467
}
452
468
453
-
if ($this->needReviewerStatus) {
469
+
if ($this->needReviewerStatus || $this->needReviewerAuthority) {
454
470
$this->loadReviewers($conn_r, $revisions);
455
471
}
456
472
···
1040
1056
->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST)
1041
1057
->execute();
1042
1058
1059
+
$viewer = $this->getViewer();
1060
+
$viewer_phid = $viewer->getPHID();
1061
+
1062
+
// Figure out which of these reviewers the viewer has authority to act as.
1063
+
if ($this->needReviewerAuthority && $viewer_phid) {
1064
+
$allow_key = 'differential.allow-self-accept';
1065
+
$allow_self = PhabricatorEnv::getEnvConfig($allow_key);
1066
+
$authority = $this->loadReviewerAuthority(
1067
+
$revisions,
1068
+
$edges,
1069
+
$allow_self);
1070
+
}
1071
+
1043
1072
foreach ($revisions as $revision) {
1044
1073
$revision_edges = $edges[$revision->getPHID()][$edge_type];
1045
1074
$reviewers = array();
1046
-
foreach ($revision_edges as $user_phid => $edge) {
1047
-
$data = $edge['data'];
1048
-
$reviewers[] = new DifferentialReviewer(
1049
-
$user_phid,
1050
-
idx($data, 'status'),
1051
-
idx($data, 'diff'));
1075
+
foreach ($revision_edges as $reviewer_phid => $edge) {
1076
+
$reviewer = new DifferentialReviewer($reviewer_phid, $edge['data']);
1077
+
1078
+
if ($this->needReviewerAuthority) {
1079
+
if (!$viewer_phid) {
1080
+
// Logged-out users never have authority.
1081
+
$has_authority = false;
1082
+
} else if ((!$allow_self) &&
1083
+
($revision->getAuthorPHID() == $viewer_phid)) {
1084
+
// The author can never have authority unless we allow self-accept.
1085
+
$has_authority = false;
1086
+
} else {
1087
+
// Otherwise, look up whether th viewer has authority.
1088
+
$has_authority = isset($authority[$reviewer_phid]);
1089
+
}
1090
+
1091
+
$reviewer->attachAuthority($viewer, $has_authority);
1092
+
}
1093
+
1094
+
$reviewers[$reviewer_phid] = $reviewer;
1052
1095
}
1053
1096
1054
1097
$revision->attachReviewerStatus($reviewers);
···
1090
1133
1091
1134
return array($blocking, $active, $waiting);
1092
1135
}
1136
+
1137
+
private function loadReviewerAuthority(
1138
+
array $revisions,
1139
+
array $edges,
1140
+
$allow_self) {
1141
+
1142
+
$revision_map = mpull($revisions, null, 'getPHID');
1143
+
$viewer_phid = $this->getViewer()->getPHID();
1144
+
1145
+
// Find all the project reviewers which the user may have authority over.
1146
+
$project_phids = array();
1147
+
$project_type = PhabricatorProjectPHIDTypeProject::TYPECONST;
1148
+
$edge_type = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
1149
+
foreach ($edges as $src => $types) {
1150
+
if (!$allow_self) {
1151
+
if ($revision_map[$src]->getAuthorPHID() == $viewer_phid) {
1152
+
// If self-review isn't permitted, the user will never have
1153
+
// authority over projects on revisions they authored because you
1154
+
// can't accept your own revisions, so we don't need to load any
1155
+
// data about these reviewers.
1156
+
continue;
1157
+
}
1158
+
}
1159
+
$edge_data = idx($types, $edge_type, array());
1160
+
foreach ($edge_data as $dst => $data) {
1161
+
if (phid_get_type($dst) == $project_type) {
1162
+
$project_phids[] = $dst;
1163
+
}
1164
+
}
1165
+
}
1166
+
1167
+
// Now, figure out which of these projects the viewer is actually a
1168
+
// member of.
1169
+
$project_authority = array();
1170
+
if ($project_phids) {
1171
+
$project_authority = id(new PhabricatorProjectQuery())
1172
+
->setViewer($this->getViewer())
1173
+
->withPHIDs($project_phids)
1174
+
->withMemberPHIDs(array($viewer_phid))
1175
+
->execute();
1176
+
$project_authority = mpull($project_authority, 'getPHID');
1177
+
}
1178
+
1179
+
// Finally, the viewer has authority over themselves.
1180
+
return array(
1181
+
$viewer_phid => true,
1182
+
) + array_fuse($project_authority);
1183
+
}
1184
+
1185
+
1093
1186
1094
1187
1095
1188
}
+23
-6
src/applications/differential/storage/DifferentialReviewer.php
+23
-6
src/applications/differential/storage/DifferentialReviewer.php
···
2
2
3
3
final class DifferentialReviewer {
4
4
5
-
protected $reviewerPHID;
6
-
protected $status;
7
-
protected $diffID;
5
+
private $reviewerPHID;
6
+
private $status;
7
+
private $diffID;
8
+
private $authority = array();
8
9
9
-
public function __construct($reviewer_phid, $status, $diff_id = null) {
10
+
public function __construct($reviewer_phid, array $edge_data) {
10
11
$this->reviewerPHID = $reviewer_phid;
11
-
$this->status = $status;
12
-
$this->diffID = $diff_id;
12
+
$this->status = idx($edge_data, 'status');
13
+
$this->diffID = idx($edge_data, 'diff');
13
14
}
14
15
15
16
public function getReviewerPHID() {
···
27
28
public function isUser() {
28
29
$user_type = PhabricatorPeoplePHIDTypeUser::TYPECONST;
29
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("You must attachAuthority() first!");
45
+
}
46
+
return $this->authority[$viewer_phid];
30
47
}
31
48
32
49
}
+3
-12
src/applications/differential/view/DifferentialReviewersView.php
+3
-12
src/applications/differential/view/DifferentialReviewersView.php
···
5
5
private $reviewers;
6
6
private $handles;
7
7
private $diff;
8
-
private $highlightPHIDs = array();
9
8
10
9
public function setReviewers(array $reviewers) {
11
10
assert_instances_of($reviewers, 'DifferentialReviewer');
···
24
23
return $this;
25
24
}
26
25
27
-
public function setHighlightPHIDs(array $phids) {
28
-
$this->highlightPHIDs = $phids;
29
-
return $this;
30
-
}
26
+
public function render() {
27
+
$viewer = $this->getUser();
31
28
32
-
public function render() {
33
29
$view = new PHUIStatusListView();
34
-
35
-
$highlighted = array_fuse($this->highlightPHIDs);
36
-
37
30
foreach ($this->reviewers as $reviewer) {
38
31
$phid = $reviewer->getReviewerPHID();
39
32
$handle = $this->handles[$phid];
···
46
39
47
40
$item = new PHUIStatusItemView();
48
41
49
-
if (isset($highlighted[$phid])) {
50
-
$item->setHighlighted(true);
51
-
}
42
+
$item->setHighlighted($reviewer->hasAuthority($viewer));
52
43
53
44
switch ($reviewer->getStatus()) {
54
45
case DifferentialReviewerStatus::STATUS_ADDED: