Page MenuHomePhabricator

"Needs Audit" no longer excludes author commits
Closed, ResolvedPublic

Description

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.

See D14013 for details.

Event Timeline

cburroughs updated the task description. (Show Details)
cburroughs added a project: Audit.
cburroughs added a subscriber: cburroughs.
cburroughs renamed this task from This "Needs Audit" no longer excludes author commits to "Needs Audit" no longer excludes author commits.Sep 16 2015, 1:55 PM

I also see this problem.

toelke added a subscriber: toelke.Sep 21 2015, 9:27 AM
mikn added a subscriber: mikn.Sep 23 2015, 12:33 PM
alexnb added a subscriber: alexnb.Sep 29 2015, 1:41 PM
revi added a subscriber: revi.Oct 1 2015, 7:24 AM

Same problem for anyone following this user guide:
https://secure.phabricator.com/book/phabricator/article/audit/#audits-in-small-teams

Please fix this, I have to switch back to the previously installed version now.

mime added a subscriber: mime.Oct 5 2015, 10:03 PM

Indeed, this breaks the flow on our team as well, especially on item 3) above:

  • "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.
anda added a subscriber: anda.Nov 3 2015, 7:58 AM

Can't agree more.
This is unfortunate...

Is there any chance this is gonna be fixed soon or is there a workaround?

Please see Planning. Thanks!

the3ver added a subscriber: the3ver.Jan 5 2016, 1:00 PM
eadler added a project: Restricted Project.Jan 9 2016, 12:53 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
cburroughs added a project: Restricted Project.Apr 20 2016, 3:13 PM
cburroughs moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 20 2016, 3:56 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:09 PM
epriestley moved this task from Backlog to EditEngine on the Audit board.Jan 10 2017, 4:38 PM

We could resolve this by implementing "Responsible Users", like Differential.

The major issue with that is that bucketing doesn't support pagination. We cheat through this in Differential by just using a large page size and trusting that no one will ever have 1K open revisions (or, at least, won't care about bucketing if they do).

Although 1K open audits is more reasonable, I'm inclined to do this anyway and cross the pagination bridge when we come to it. So:

  • Audit will change to have a "Responsible Users" constraint, like Differential, which matches commits you need to audit or respond to.
  • Audit will get bucketing, like Differential ("Need Update", "Waiting on Audit", etc).
  • When the buckets overflow, we'll degrade somewhat badly and warn you about it. This should be rare in normal use and other tools exist to deal with these cases (unbucketed queries, bin/audit delete, future integrations in T5815 and bulk edit + EditEngine).

After D17192, audits bucket into a dashboard like Differential. Feedback from the Differential bucketing changes has generally been positive, so I think this is probably generally a good direction. It also improves consistency between the applications, if nothing else.

Buckets can "overflow": results are paginated first, then bucketed. If you have more than 1,000 open audits, the dashboard will show you the most recent 1,000, divided into appropriate buckets. This can be misleading because, say, the first bucket may appear empty, but actually have results on the second or third page. We don't have a fix for this which feels particularly good yet, but the UI will at least warn you about it when it happens. This hasn't presented a problem so far in Differential, but since you can have more audits in Diffusion it may be more of an issue.

In particular, audits are currently divided into these buckets:

  • Needs Attention: Your commits which have concerns raised on them.
  • Ready to Audit: Other users' commits which you've been requested or required to audit.
  • Waiting on Authors: Your unaddressed concerns raised with other users' commits.
  • Waiting on Auditors: Your authored commits which need audit from others.

This may need some tweaking, and will get shaken up a bit once T2393 happens, but I believe it should generally address the original issue here.

I upgraded to 5efbf4d74aa72c5a7d7f161ea8c5aa33ac0e3189, after which when navigating to the list of audits on dashboard I get an error:

Exception: Query "need" is unknown to application search engine "PhabricatorCommitSearchEngine"!

The dashboard panel used the "Needs Audit" query (I believe was at least based on a default query) - is that expected for this? After modifying the dashboard to use a new panel using the new "Active Audits" query the dashboard correctly shows the results.

is that expected for this?

Yes, although I think I neglected to call it out as a "Compatibility" issue in the changelog.

We usually don't migrate saved queries. Some of the reasons for this are:

  • The migrations tend to be difficult to test convincingly, and are more fragile / prone to decay than other types of migrations.
  • Sometimes there's no faithful modern version of a historical query, and changing the behavior subtly might be worse than breaking in an obvious way.
  • We don't see too many complaints about the occasional breaks that do occur: saved queries are a somewhat-advanced feature, and they're pretty easy to rebuild, and when we do make a break we usually improve the UI substantially alongside it.
  • We do see complaints about migrations. Broadly, administrators seem more willing to absorb minor user questions after upgrades (which are unavoidable in the general case anyway, since workflows sometimes change) than long migrations during upgrades.

Or, more concisely: I am lazy; so far, I seem to be getting away with it.

Ah I guess it was a custom query - I checked someone else's dashboard who I know shows audits and theirs did not have the same issue as it was using a built-in one. Thanks for explaining, I can see how migrating a saved query would be difficult/impossible with no major benefits. I had never come across that type of error showing on page like that before and just wanted to check.

🐕

I've hit the same problem that @cspeckmim ran into. As best I can tell, in the new system there's no way to quite replicate the behaviour of the old 'needs' or 'problem' audit queries. The closest attempts I've come up with so far are:

  • query for Audit Status = Audit Required/Needs Verification and Responsible Users = viewer(). But that includes results where the user is the diff author, not auditor.
  • query for Audit Status = Audit Required/Needs Verification and Auditors = viewer(). But that misses results where user is a member of a package or project auditor.

I think this would work if I could query for Auditors = viewer()/projects(viewer())/packages(viewer()), but it seems the functions aren't composable (at least in the UI).

Is there some other approach I should take here?

More generally: I think your decision to not try to migrate saved queries makes a lot of sense. In lieu of that, I'd like to cast a vote for more prominent mention in the changelog. It would be great just to see a note that some queries would break and some hints on identifying and replacing/repairing the broken ones.

We support viewerprojects() elsewhere but it isn't currently a component of the "Auditors" datasource. See also T9362#207080. I'll file a version of T9362 for the "Auditors" field.

(We should possibly also make these composable directly, but exactviewer() isn't really the same as exact(viewer()) for any reasonable definition of what "exact" might mean as a function, and I don't really want to turn tokenizer functions into lambda calculus if we can avoid it.)

I think we currently get about equal amounts of "your changelogs should be more detailed" and "your changelogs have too much irrelevant stuff" feedback so I'll take that feedback under consideration but don't plan to make changes unless the overall balance of feedback shifts substantially in one direction.

I'm good with the current balance of the changelogs - the one thing I think would be nifty is being able to accumulate the Upgrade/Compatibility sections across changelogs, for sites that aren't able to upgrade weekly. I was considering making a script to scrape that info but haven't had a chance to sit down for it. Also it'd be relying on the changelog format to be consistent (seems to be pretty darn consistent).

The general headers/sections are mostly scripted now so they should be fairly consistent, but I'm still editing the actual markup at the end of the day.

It's possible we might write some kind of "Changelog" / "Release Notes" application eventually but I don't recall much interest in this and we probably wouldn't write it for ourselves.