@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
Make implicit audits by the Owners tool use modern code
Summary:
Ref T10978. This updates audits triggered by Owners to use a modern transaction. Minor changes:
- After D17264, we no longer need the "AUDIT_NOT_REQUIRED" fake-audits to record package membership. This no longer creates them.
- This previously saved English-language, untranslatable text strings about audit details onto the audit relationship. I've removed them, per discussion in D17263.
The "Audit Reasons" here are potentially a little more useful than the Herald/Explicit-By-Owner ones were, since the rules are a little more complex, but I'd still like to see evidence that we need them.
In particular, the transaction record now says "Owners added auditors: ...", just like Differential, so the source of the auditors should be clear:
{F2549087}
T11118 (roughly "add several Owners audit modes", despite the title at time of writing) might impact this too. Basically, this is simple and maybe good enough; if it's not quite good enough we can refine it.
Test Plan: Ran `bin/repository reparse --owners <commit>` saw appropriate owners audits trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17266
+71
-47
+71
-47
src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+71
-47
src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
···
28
28
private function triggerOwnerAudits(
29
29
PhabricatorRepository $repository,
30
30
PhabricatorRepositoryCommit $commit) {
31
+
$viewer = PhabricatorUser::getOmnipotentUser();
31
32
32
33
if (!$repository->shouldPublish()) {
33
34
return;
···
48
49
return;
49
50
}
50
51
51
-
$data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
52
-
'commitID = %d',
53
-
$commit->getID());
54
-
$commit->attachCommitData($data);
52
+
$commit = id(new DiffusionCommitQuery())
53
+
->setViewer($viewer)
54
+
->withPHIDs(array($commit->getPHID()))
55
+
->needCommitData(true)
56
+
->needAuditRequests(true)
57
+
->executeOne();
58
+
if (!$commit) {
59
+
return;
60
+
}
61
+
62
+
$data = $commit->getCommitData();
55
63
56
64
$author_phid = $data->getCommitDetail('authorPHID');
57
65
$revision_id = $data->getCommitDetail('differential.revisionID');
58
66
if ($revision_id) {
59
67
$revision = id(new DifferentialRevisionQuery())
60
-
->setViewer(PhabricatorUser::getOmnipotentUser())
68
+
->setViewer($viewer)
61
69
->withIDs(array($revision_id))
62
70
->needReviewerStatus(true)
63
71
->executeOne();
···
65
73
$revision = null;
66
74
}
67
75
68
-
$requests = id(new PhabricatorRepositoryAuditRequest())
69
-
->loadAllWhere(
70
-
'commitPHID = %s',
71
-
$commit->getPHID());
76
+
$requests = $commit->getAudits();
72
77
$requests = mpull($requests, null, 'getAuditorPHID');
73
78
74
-
79
+
$auditor_phids = array();
75
80
foreach ($affected_packages as $package) {
76
81
$request = idx($requests, $package->getPHID());
77
82
if ($request) {
···
79
84
continue;
80
85
}
81
86
82
-
if ($package->getAuditingEnabled()) {
83
-
$reasons = $this->checkAuditReasons(
84
-
$commit,
85
-
$package,
86
-
$author_phid,
87
-
$revision);
88
-
89
-
if ($reasons) {
90
-
$audit_status = PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
91
-
} else {
92
-
$audit_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
93
-
}
94
-
} else {
95
-
$reasons = array();
96
-
$audit_status = PhabricatorAuditStatusConstants::NONE;
87
+
$should_audit = $this->shouldTriggerAudit(
88
+
$commit,
89
+
$package,
90
+
$author_phid,
91
+
$revision);
92
+
if (!$should_audit) {
93
+
continue;
97
94
}
98
95
99
-
$relationship = new PhabricatorRepositoryAuditRequest();
100
-
$relationship->setAuditorPHID($package->getPHID());
101
-
$relationship->setCommitPHID($commit->getPHID());
102
-
$relationship->setAuditReasons($reasons);
103
-
$relationship->setAuditStatus($audit_status);
96
+
$auditor_phids[] = $package->getPHID();
97
+
}
104
98
105
-
$relationship->save();
106
-
107
-
$requests[$package->getPHID()] = $relationship;
99
+
// If none of the packages are triggering audits, we're all done.
100
+
if (!$auditor_phids) {
101
+
return;
108
102
}
109
103
110
-
$commit->updateAuditStatus($requests);
111
-
$commit->save();
104
+
$audit_type = DiffusionCommitAuditorsTransaction::TRANSACTIONTYPE;
105
+
106
+
$owners_phid = id(new PhabricatorOwnersApplication())
107
+
->getPHID();
108
+
109
+
$content_source = $this->newContentSource();
110
+
111
+
$xactions = array();
112
+
$xactions[] = $commit->getApplicationTransactionTemplate()
113
+
->setTransactionType($audit_type)
114
+
->setNewValue(
115
+
array(
116
+
'+' => array_fuse($auditor_phids),
117
+
));
118
+
119
+
$editor = $commit->getApplicationTransactionEditor()
120
+
->setActor($viewer)
121
+
->setActingAsPHID($owners_phid)
122
+
->setContinueOnNoEffect(true)
123
+
->setContinueOnMissingFields(true)
124
+
->setContentSource($content_source);
125
+
126
+
$editor->applyTransactions($commit, $xactions);
112
127
}
113
128
114
-
private function checkAuditReasons(
129
+
private function shouldTriggerAudit(
115
130
PhabricatorRepositoryCommit $commit,
116
131
PhabricatorOwnersPackage $package,
117
132
$author_phid,
118
133
$revision) {
119
134
135
+
// Don't trigger an audit if auditing isn't enabled for the package.
136
+
if (!$package->getAuditingEnabled()) {
137
+
return false;
138
+
}
139
+
140
+
// Trigger an audit if we don't recognize the commit's author.
141
+
if (!$author_phid) {
142
+
return true;
143
+
}
144
+
120
145
$owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
121
146
array(
122
147
$package->getID(),
123
148
));
124
149
$owner_phids = array_fuse($owner_phids);
125
150
126
-
$reasons = array();
127
-
128
-
if (!$author_phid) {
129
-
$reasons[] = pht('Commit Author Not Recognized');
130
-
} else if (isset($owner_phids[$author_phid])) {
131
-
return $reasons;
151
+
// Don't trigger an audit if the author is a package owner.
152
+
if (isset($owner_phids[$author_phid])) {
153
+
return false;
132
154
}
133
155
156
+
// Trigger an audit of there is no corresponding revision.
134
157
if (!$revision) {
135
-
$reasons[] = pht('No Revision Specified');
136
-
return $reasons;
158
+
return true;
137
159
}
138
160
139
161
$accepted_statuses = array(
···
159
181
}
160
182
}
161
183
162
-
if (!$found_accept) {
163
-
$reasons[] = pht('Owners Not Involved');
184
+
// Don't trigger an audit if a package owner already reviewed the
185
+
// revision.
186
+
if ($found_accept) {
187
+
return false;
164
188
}
165
189
166
-
return $reasons;
190
+
return true;
167
191
}
168
192
169
193
}