recaptime.dev / phorge
@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
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Improve audit behavior for "uninteresting" auditors

Summary:
Ref T10939. Fixes T10174. We can currently trigger "uninteresting" auditors in two ways:

- Packages with auditing disabled ("NONE" audits).
- Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; "NOT REQUIRED" audits).

These audits aren't interesting (we only write them so we can list "commits in this package" from other UIs) but right now they take up the audit slot. In particular:

- They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing "which packages do these paths belong to" now, and shows all packages for each path.
- They block Herald from adding real auditors.

Change this:

- Don't show uninteresting auditors.
- Let Herald upgrade uninteresting auditors into real auditors.

Test Plan:
- Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.
- With a package with auditing disabled, triggered a "None" audit and saw it no longer appear in the UI with the patch applied.
- With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to "Audit Required".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10174, T10939

Differential Revision: https://secure.phabricator.com/D15940

epriestley de1a30ef 9c24798e

+38 -7
+15 -4
src/applications/audit/editor/PhabricatorAuditEditor.php
··· 159 159 $requests = mpull($requests, null, 'getAuditorPHID'); 160 160 foreach ($add as $phid) { 161 161 if (isset($requests[$phid])) { 162 - continue; 162 + $request = $requests[$phid]; 163 + 164 + // Only update an existing request if the current status is not 165 + // an interesting status. 166 + if ($request->isInteresting()) { 167 + continue; 168 + } 169 + } else { 170 + $request = id(new PhabricatorRepositoryAuditRequest()) 171 + ->setCommitPHID($object->getPHID()) 172 + ->setAuditorPHID($phid); 163 173 } 164 174 165 175 if ($this->getIsHeraldEditor()) { ··· 170 180 $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED; 171 181 $audit_reason = $this->getAuditReasons($phid); 172 182 } 173 - $requests[] = id(new PhabricatorRepositoryAuditRequest()) 174 - ->setCommitPHID($object->getPHID()) 175 - ->setAuditorPHID($phid) 183 + 184 + $request 176 185 ->setAuditStatus($audit_requested) 177 186 ->setAuditReasons($audit_reason) 178 187 ->save(); 188 + 189 + $requests[$phid] = $request; 179 190 } 180 191 181 192 $object->attachAudits($requests);
+6 -1
src/applications/diffusion/controller/DiffusionCommitController.php
··· 455 455 if ($audit_requests) { 456 456 $user_requests = array(); 457 457 $other_requests = array(); 458 + 458 459 foreach ($audit_requests as $audit_request) { 460 + if (!$audit_request->isInteresting()) { 461 + continue; 462 + } 463 + 459 464 if ($audit_request->isUser()) { 460 465 $user_requests[] = $audit_request; 461 466 } else { ··· 471 476 472 477 if ($other_requests) { 473 478 $view->addProperty( 474 - pht('Project/Package Auditors'), 479 + pht('Group Auditors'), 475 480 $this->renderAuditStatusView($other_requests)); 476 481 } 477 482 }
+7 -2
src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php
··· 18 18 $object = $adapter->getObject(); 19 19 20 20 $auditors = $object->getAudits(); 21 - $auditors = mpull($auditors, null, 'getAuditorPHID'); 22 - $current = array_keys($auditors); 21 + 22 + $current = array(); 23 + foreach ($auditors as $auditor) { 24 + if ($auditor->isInteresting()) { 25 + $current[] = $auditor->getAuditorPHID(); 26 + } 27 + } 23 28 24 29 $allowed_types = array( 25 30 PhabricatorPeopleUserPHIDType::TYPECONST,
+10
src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php
··· 49 49 return $this->assertAttached($this->commit); 50 50 } 51 51 52 + public function isInteresting() { 53 + switch ($this->getAuditStatus()) { 54 + case PhabricatorAuditStatusConstants::NONE: 55 + case PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED: 56 + return false; 57 + } 58 + 59 + return true; 60 + } 61 + 52 62 53 63 /* -( PhabricatorPolicyInterface )----------------------------------------- */ 54 64