Page MenuHomePhabricator

Support an "auditors" attachment for "diffusion.commit.search"
Closed, ResolvedPublic

Description

See PHI2016. An install would benefit from an "auditors" attachment in the Diffusion API, similar to the "reviewers" attachment in Differential.

Event Timeline

epriestley created this task.

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.