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.

When triggering audits, count "Accepted" revisions as successfully reviewed

Summary:
See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem.

Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes.

Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed".

Test Plan:
- With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish <hash>` to republish the commit.
- Before change: audit by package P triggered.
- After change: audit by package P no longer triggered.

Reviewers: amckinley

Reviewed By: amckinley

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

epriestley 67f062b0 81134d7e

+13 -4
+13 -4
src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php
··· 223 223 224 224 // If auditing is configured to trigger on unreviewed changes, check if 225 225 // the revision was "Accepted" when it landed. If not, trigger an audit. 226 + 227 + // We may be running before the revision actually closes, so we'll count 228 + // either an "Accepted" or a "Closed, Previously Accepted" revision as 229 + // good enough. 230 + 226 231 if ($audit_unreviewed) { 227 232 $commit_unreviewed = true; 228 233 if ($revision) { 229 - $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED; 230 - if ($revision->isPublished()) { 231 - if ($revision->getProperty($was_accepted)) { 232 - $commit_unreviewed = false; 234 + if ($revision->isAccepted()) { 235 + $commit_unreviewed = false; 236 + } else { 237 + $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED; 238 + if ($revision->isPublished()) { 239 + if ($revision->getProperty($was_accepted)) { 240 + $commit_unreviewed = false; 241 + } 233 242 } 234 243 } 235 244 }