See PHI2016. An install would benefit from an "auditors" attachment in the Diffusion API, similar to the "reviewers" attachment in Differential.
Description
Revisions and Commits
Related Objects
- Mentioned In
- 2021 Week 11 (Mid March)
- Mentioned Here
- T10574: A pathway toward "All Reviewers Must Accept"
D10103: Move Audit to proper Subscriptions
D17183: Use EditEngine stacked comments in Diffusion
D17249: Remove an unused method in Audit for building comment actions
D17252: Add a "Needs Verification" state to Audit
D17250: Remove old Audit code "Action" transaction editing code
D17264: Write an explicit edge for commit membership in packages
T12156: User is unable to close an audit for own commits, even audit.can-author-close-audit is set to TRUE
Event Timeline
PhabricatorAuditStatusConstants appears to have a set of unused constants:
- NONE
- AUDIT_NOT_REQUIRED
- CC
- CLOSED
I think none of these is a sensible state for an auditor in the modern codebase.
Long ago, we used to write a <package, commit> audit relationship with "AUDIT_NOT_REQUIRED", meaning "This commit affects the package, but package owners don't need to audit it.".
In D17264 (Jan 2017), this was changed to write an edge instead and just use audits for actual audits. This stuck (this is still the representation today) but the old constant (and rows) weren't removed at the time since it would have destroyed data.
I believe CC is an ancient version of "Subscribe". There are no readers or writers. Commit implemented SubscribableInterface in D10103 (August 2014) and the existing "CC" audits migrated at that time.
NONE came from D1242 and it's not entirely clear to me that it did anything then.
CLOSED came from D2013 and was a status associated with "Close Audit". At the time, "Close Audit" meant "force the audit to close", i.e. remove all open concerns. This was removed in D17249, but that was just cleanup of dead code. Slightly later, there was no evidence of "Close Audit" in D17252, when "Request Verification" was introduced. D17250 removed the old action implementation. D17183 probably was the actual cutover point. It's not entirely obvious that "Close Audit" was removed on purpose?
I think this removal was probably setup for "Needs Verification", but it's possible it was accidental. Even if it was accidental, no one has asked for it back for ~3 years, so it's canonical now. There is some evidence here that this was intended:
https://secure.phabricator.com/T12156#208724
In short:
- NONE probably never did anything (Dec 2011).
- CC was mooted by proper Subscribers support (August 2014).
- CLOSED was mooted by "Request Verification" or some adjacent change (Jan 2017).
- AUDIT_NOT_REQUIRED was mooted by moving packages to edges (Jan 2017).
I believe these can all be safely removed now, and all corresponding rows can be destroyed.
One remaining artifact here is this configuration option:
$this->newOption( 'audit.can-author-close-audit', 'bool', false) ->setBoolOptions( array( pht('Enable Closing Audits'), pht('Disable Closing Audits'), )) ->setDescription(pht('Controls whether Author can Close Audits.')),
This is misleading, since it actually means "Can author self-accept audits?" in modern Phabricator, and should probably get a text fix at a minimum, although the actual option name would still be misleading.
In the longer term, this can be fixed via T10574, but that's probably a long ways away.
For now, I fixed the explicit misinformation in audit.can-author-close-audit, at least.
The reporting install reported that this stuff seems to be in good shape, so calling this resolved.