Page MenuHomePhabricator

Modernize Audit search engine
ClosedPublic

Authored by epriestley on Aug 31 2015, 12:27 PM.

Details

Reviewers
chad
Maniphest Tasks
T9279: Sort commit history chronologically
Commits
rPbce0698a0f5d: Modernize Audit search engine
Restricted Diffusion Commit
Summary

Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes:

  • Added order by commit date, default to order by commit date with newest commits first.
  • Added explicit "Needs Audit by".
  • Added new packages(...) typeahead function.
  • Picked up automatic subscribers, projects, and order fields.

This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.

Test Plan

Diff Detail

Event Timeline

epriestley retitled this revision from to Modernize Audit search engine.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad accepted this revision.Aug 31 2015, 4:03 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Aug 31 2015, 4:03 PM
This revision was automatically updated to reflect the committed changes.

This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.

This '(Needs Audit' as he successor to 'Needs Attention') no longer excludes commits where I am the author. Is that also intentional?

This '(Needs Audit' as he successor to 'Needs Attention') no longer excludes commits where I am the author. Is that also intentional?

I don't think I specifically intended this, although it's expected from the text of the change.

Are these cases common? In particular, I'd expect automatically-triggered-Package-audits to not trigger if the author is a package owner, but maybe that's buggy or not common in your use case.

The ideal upstream solution to this is to figure out general bucketing (see T9372) but that task only chips away at describing the problem.

In the short term, I'd like to assess how broken/painful this is and figure out if we need to work around it in advance of T9372. For example, if this pain is almost entirely from Owners doing weird stuff, maybe we can just fix that and then limp along until T9372. Can you give me some more context around how these audits are occurring?

Virtually all of our audits are triggered by Herald and not Owners, roughly following the small teams guide.

For each team there is a herald rule along the lines of:

  • If Accepted Differential revision does not exist and "commit is on master etc" and Repository projects include all of team-foo
  • then Add auditors: team-foo

The intent is that all commits are reviewed by going through either differential or Audit (with the hope that the percentage of differential increases over time). My workflow (which I think is typical) is :

  • click on the audit app which takes me to Needs Audit/Attention.
  • Open a bunch (recent/interesting/easy depending on my mood) of audits in new tabs
  • Read code and dish out checkmarks

The new behavior introduces three pain points:

  1. I now have to dodge my own commits while going through the queue.
  2. Since (I think) my own commits are included in the little count that shows up by the Audit app I can never work the queue down to zero.
  3. "Concern Raised" does not show up in "Needs Audit", which I think will be bad for follow-through (If I raised a concern on another commit I want to make sure it is taken care of). They show up in "Open Audits" but that query is global and not just the ones relevant to me.

Just noting that I use it in almost exactly the same way (although the audits show on my default dashboard rather than me manually going into the audit application).

Did this meet the pain threshold to fix Real Soon or would you prefer a followup task?

Yeah, go ahead and file this so I don't lose track of it? This is definitely painful but I'd like to find a way forward through or en route to T9372, rather than moving away from that and then needing to disrupt the interface multiple times.

I am currently waiting for divine inspiration to strike and illuminate a path through T9372.

I am currently waiting for divine inspiration to strike and illuminate a path through T9372.