I pushed that stuff live but bucketing has some bugs, looking into it.

This also indirectly flipped the ordering from oldest-first to newest-first. I'm inclined to retain that ordering, since it's consistent with every other application. The "oldest-first" ordering was a sort of paternalistic attempt to force users to review stuff, but I think we've generally moved the software away from that a bit and it doesn't feel very much at home to me any more.

(You can create an oldest-first dashboard yourself now if you prefer the old ordering, and can probably force it on all users after T4103 if you really want.)

Bucketing is now controlled by a "Bucket: ..." option in the search query controls instead of hardcoded as a behavior of the "Active Revisions" filter. This means that you can apply bucketing to other queries. You can also extend DifferentialRevisionResultBucket to implement your own bucketing algorithms (but: here be dragons).

In particular, you can bucket queries like "Responsible Users: SomeProject" to see bucketed revisions by a project reviewer, and "Responsible Users: exact(username)" to see bucketed revisions by individual reviewer, excluding the projects and packages they have authority over. The default ("Responsible Users: username") shows your stuff plus all the packages and projects you have authority over.

The buckets themselves are now more granular and action-oriented (here, "you" is "whatever is listed in 'Responsible Users' in the query"):

• Must Review: Stuff you're blocking.
• Ready to Review: Stuff you're not blocking explicitly, but which does need review.
• Ready to Land: Your accepted revisions.
• Ready to Update: Your authored "needs revision" revisions.
• Waiting on Review: Your authored "needs review" revisions.
• Waiting on Authors: Stuff you're reviewing that's waiting on someone to land or update it.

Reviewer calculation now looks at individual reviewer relationships rather than only at overall revision status. For example, accepting a revision always kicks it out of your "Must Review" and "Ready to Review" buckets, while it would previously retain it there if the revision as a whole did not move to "Accepted".

This might have a few weird edge cases and quasi-bugs (for example, if you have differential.allow-self-accept enabled, many of these states are ambiguous, but that option is fairly silly anyway), yell if you catch any revisions that seem out of place.

D15926 is a proposed change which would show more information about individual reviewers in the list view, but it also adds a fair amount of UI clutter and we aren't sure if it's still necessary given all the other changes. If the composition of the "Must Review" and "Ready for Review" buckets remains mysterious, we'd like specific feedback about use cases that remain difficult to parse or work with.

Broadly, this represents an update that makes bucketing better aware of individual reviewer status and of aggregate (package/project) reviewers.

These changes are live on this install, although it may be difficult to get much of a sense of them without more realistic data.

Next up, in some order:

• Explicit manual blocking reviewers.
• "Soft Accept" / "Unblock" action ("TBR").
• Support for non-hierarchical packages / implicit ownership.

I'm no longer able to search for "Responsible Users: (Current Viewer)". Is that expected?

Yes, since it would really mean exact(current-viewer) without additional work. Are you using it for something (dashboards?)

The default home page (and homepage sidebar notification count) are also still doing the old grouping / counts. We should probably fix that but it's not a strict dependency here and there's adjacent work (T3583).

In T10939#175518, @epriestley wrote:

• "Soft Accept" / "Unblock" action ("TBR").

This doesn't matter for this task, but for ease of communication. TBR in our org stands for to be reviewed. Historically it should have been spelled as TBNR since it will never be reviewed. In reviewboard the mode of operation is that you create a diff with the correct reviewers and then submit with a special flag. In Phabricator land the flag is the default prompt for an unaccepted revision. We're excited about the ability to use the Audit application to enforce the eventual review but we're a bit aways from making that useful.

Just to make sure I'm understanding correctly:

After everything that's currently in flight lands (i.e., up to D15936) I expect you to be able to get this far:

• Import existing OWNERS files by using the API to create packages, as you do today.
• Have the script set "Auto Review: Blocking" and "Dominion: Weak" on the packages, so they automatically become blocking reviewers with non-hierarchical ownership.
• "Auto Review" will add package reviewers to new revisions on the server-side, without the need for client-side rules. "Weak Dominion" will keep shallow-path package owners from being added to deep paths owned elsewhere.
• The dashboard will reflect package reviewers and user state in a reasonable way.

At this point, you may end up with some packages that are blocking reviewers but whose owners are not currently available. The revision author can move forward by doing any of these:

• Editing "Reviewers" and removing the blocks (after D15933, they can downgrade blocks to normal reviewers via the web UI).
• Editing "Reviewers" and removing the reviewers entirely.
• Editing "Reviewers: ..." from the CLI with --edit and removing blocks (after D15934) or reviewers.
• Using "arc land" and "Y"'ing through the prompt about the revision not being accepted.

It sounds like that's sufficient for now, and the "Soft Accept" / "Partial Accept" / "Accept for Package" / "Unblock" actions aren't too important for the moment? You can sort of accomplish them with the mechanisms above anyway, it's just less convenient and your intent is less clear than if we had a real "Remove some of the blocking reviewers, explicitly" action.

Once the author forces their way through landing, I currently expect "Auditing: Enabled" to trigger an audit for any packages which did not accept, providing a safety net to catch these changes later in the flow. In at least some subset of cases this seems to work correctly (e.g., we use audits for this purpose on this server, and have for many years, and they appear to work as expected) although T10181 says it doesn't, and it almost certainly needs some updates to account for package reviewers. I plan to look at T10181, figure out what's going on there, then update the behavior of "Auditing: Enabled" to fix whatever actual bugs are described in T10181 / T10174 and account for package reviewers, but I'm not expecting this to involve any major changes. So the flow would then continue:

• The API calls would also have set "Auditing: Enabled", above.
• If a revision is forced through with any of the above mechanisms, the "Auditing" setting causes audits trigger automatically as a safety net to catch changes which didn't receive real review by some packages.

This stuff, discussed above, still won't have a clean solution:

• Users with root ownership (say, on "/") won't have any special way to clear blocks made by subpackages (say, "/a/b/") which they don't also own. They can do all the "Edit Reviewers" stuff, but when anyone does this to clear blocks their intent won't be made explicitly clear. This is likely solvable culturally ("When you force your way through things, please note why you're doing it on the revision.") but an eventual explicit action would make this clearer.
• Users with root ownership won't have an obvious way to see that they "indirectly" own some of the packages blocking a revision, although they can likely infer this from external knowledge.
• Users won't have any special way to clear some of the blocks they own, e.g. "I'm comfortable accepting this for Platform, but someone else should clear Security". They can still use the non-special ways + comment, as above.

Does that sound reasonable, at least for now? If so, I'll focus on the audit stuff and skip the "Clear some blocks" action, at least for the moment.

Independent of this, I've also grown more hesitant about pursuing the "Clear some blocks" action before doing infrastructure work (T10967 + EditEngine). Now that I've reacquainted myself with the code I think it's significantly messier than I'd originally thought. Given the new availability of workarounds, I think it's a good candidate to wait until later for if everything above is roughly on target.

In T10939#175557, @epriestley wrote:

Yes, since it would really mean exact(current-viewer) without additional work. Are you using it for something (dashboards?)

I used it for custom Differential queries. after updating, all of my custom queries were broken. I did it this way to make it easier to share my queries with others.

(I'll restore viewer(), it's just not a one line change.)

In T10939#175619, @epriestley wrote:

Just to make sure I'm understanding correctly:

This stuff, discussed above, still won't have a clean solution:

• Users with root ownership (say, on "/") won't have any special way to clear blocks made by subpackages (say, "/a/b/") which they don't also own.

But a root owner's accept will still count for the sub-package, provided dominion is weak?

• Users with root ownership won't have an obvious way to see that they "indirectly" own some of the packages blocking a revision, although they can likely infer this from external knowledge.

If this fix for this is minor we'd appreciate this being fixed, but its non-blocking.

• Users won't have any special way to clear some of the blocks they own, e.g. "I'm comfortable accepting this for Platform, but someone else should clear Security". They can still use the non-special ways + comment, as above.

As it is today.

Does that sound reasonable, at least for now? If so, I'll focus on the audit stuff and skip the "Clear some blocks" action, at least for the moment.

This is all reasonable. (cc @scode just in case I'm being unreasonable)

But a root owner's accept will still count for the sub-package, provided dominion is weak?

No, they currently (well, "currently" in the "slightly-ahead-of-HEAD" future sense) don't own the package at all.

We could conceivably add another rule like, uh, "Reverse Upward Dominion: Give owners of parent packages authority over this package without making them members", but dominion currently only works down the chain by ceding control of paths to sub-packages. I think incorporating this into an eventual "Clear some of the blocks" action is probably cleaner, where they don't really get authority but get to do an "easy clear" with a note instead of a "hard/forced clear".

Specifically, I'm vaguely imagining that this future UI looks something like this:

Except less bad, and I'd like to make you confirm / check five boxes / sign your name in blood if you tick any of the red "Force Unblock" boxes.

If this fix for this [indirect ownership] is minor we'd appreciate this being fixed, but its non-blocking.

I think this is probably enough work that it makes sense to hold until "Clear some of the blocks", but I can poke at it.

I've consolidated the general state of the world in Audit in T10978. I think we'll run into some of that eventually but I'm not sure how much is important vs nice-to-have, so my general plan here is to just scratch the surface for now:

• Document/verify audit rules (T10181):
  • Fix some of the weird audit/package rules that we have today, some of which are inconsistent.
  • Document the expected rules.
  • Verify locally that stuff actually works in the way that I think it does.
• Figure out what's going on with T10174.
• Restore viewer() to Differential.

Then pause this until you have a chance to evaluate where things are and the Differential changes get a chance to settle for a bit. Then we can plan our way forward through continuing improvements to this workflow in Audit, or fixing any bugs or rough edges you hit prior to that, or building "Clear some blocks" sooner, or whatever else.

Thanks @epriestley!

Sounds good (including your last comment). I think that from our perspective, we're it seems likely that we'll end up realizing a few problematic edge cases or details once we start integrating fully and it's probably best to wait until then than to try to gain perfect confidence ahead of time. The work done so far and what you outline in your last comment seems sufficient to start doing the integration work and we can surface issues as they arise.

Blocking reviewers can now be added, edited and removed via the web UI with the blocking(...) function. For example, entering blocking(epriestley) into the "Reviewers" typeahead will add epriestley as a blocking reviewer. You can use this to upgrade reviewers to blocking or downgrade blocking reviewers to non-blocking.

Blocking reviewers can now be added, edited and removed via the CLI. On the CLI, typing Reviewers: epriestley! will add epriestley as a blocking reviewer. This status will reflected in arc diff --edit, etc.

(Two limitations: the transcripts for these actions are lackluster, and the "Add Reviewers: ..." action doesn't support adding blocking reviewers yet -- only "Edit Revision". Improving these cases is probably blocked on T10967.)

Owners packages can now select "Dominion" rules, which allows them to switch from always controlling paths to ceding control to other packages which express a more specific ownership of a path. I picked a funny name here since the other names we were using ("indirect ownership") weren't very descriptive or evocative and this one can at least be searched for fairly easily, and is somewhat memorable/meaningful once you understand the feature. The documentation discusses this option in more detail.

I couldn't find any real bugs with the existing audit behavior (T10181). I updated it to handle multiple reviewers better and have a behavior more consistent with the "Auto Review" behavior. The new behavior is now documented. If you find it behaving in a way that isn't consistent with the documentation, let me know. Basically, I can't find any actual bugs and didn't see any repro steps anywhere, so I've fixed everything I could guess at, but might have missed something which just didn't get written down anywhere.

Internal audit relationships ("None", "Not Applicable") were previously given unusual prominence and behaviors. They no longer show as audits in the UI, and Herald can now overwrite them. This should resolve T10174. In particular, you can write stronger or more specific Herald audit rules if you want to enforce tighter rulesets than "Auditing: Enabled" does.

I've consolidated some additional audit issues beyond these workflows in T10978. Installs that are able to make greater use of packages and auditing after these improvements may start running into issues discussed there as the workflow progresses.

Per above, I'm planning to let these changes settle and stabilize for a bit now, and expect to plan the next phase of work after feedback about what is or isn't working so far.

(I'll fix viewer() in Differential by the time we promote to stable at the latest.)

These changes are live on this install, and I've manually updated the documentation since I think the cronjob runs at 2AM or something.

#twitter hat off. FreeBSD hat on.

It seems as a result of the dashboarding changes project reviewers no longer cause the diff to show up on the dashboard. For example https://reviews.freebsd.org/D6135 has a group reviewer of #arm but even when I join the project I don't see it anywhere on the dashboard.

That's not expected, but I can't immediately reproduce it. Here's my local install, note that D4 appears in "Ready to Review" even though I (admin) am only a reviewer via project membership in Stonework (a project):

The default is still using the old algorithm, is that what you mean by dashboard (i.e., not "Active Revisions" in Differential)? But it looks like that install has a custom homepage dashboard with the active filter configured, which I'd expect to work.

The default is still

Er, "the default panel on the non-dashboardized homepage".

I'm referring to the home page dashboard. I just made some config changes (particularly, changing the limit of the number of revisions to show) and it seems to have 'fixed it' so the problem above would be PEBKAC :/

Ah! Okay, cool. Not really PEBCAK, I need to fix that dashboard too and it's super confusing that it does something weird/different with no clues about it. I just wanted to see if anyone caught major issues with the new one before swapping it.

My "before release cut" cleanup list is currently:

• Fix viewer() token in Differential.
• Fix homepage dashboard.
• Fix homepage "Differential" action count in left application sidebar.

In T10939#176076, @eadler wrote:

I'm referring to the home page dashboard.

By this I mean: as installed on the FreeBSD install.

That cleanup stuff should be cleaned up now, so I'm formally pausing this for feedback.

I don't know how to reproduce the issue, but figured I'd let you know that I'm seeing this in our error logs:

[23-May-2016 10:25:49 Australia/Sydney] [2016-05-23 10:25:49] ERROR 2: Illegal string offset 'phid' at [/usr/src/phabricator/src/applications/differential/customfield/DifferentialReviewersField.php:246]
[23-May-2016 10:25:49 Australia/Sydney] arcanist(head=master, ref.master=2234c8cacc21), phabricator(head=master, ref.master=3d3fff4991ab, custom=1), phlab(head=master, ref.master=84b62e6917ec), phutil(head=master, ref.master=bd56873ae4c0)
[23-May-2016 10:25:49 Australia/Sydney]   #0 DifferentialReviewersField::validateCommitMessageValue(array) called at [<phabricator>/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php:70]
[23-May-2016 10:25:49 Australia/Sydney]   #1 DifferentialParseCommitMessageConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:122]
[23-May-2016 10:25:49 Australia/Sydney]   #2 ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
[23-May-2016 10:25:49 Australia/Sydney]   #3 ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
[23-May-2016 10:25:49 Australia/Sydney]   #4 ConduitCall::execute() called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:81]
[23-May-2016 10:25:49 Australia/Sydney]   #5 PhabricatorConduitAPIController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[23-May-2016 10:25:49 Australia/Sydney]   #6 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[23-May-2016 10:25:49 Australia/Sydney]   #7 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

Just FYI.

General question: Why is the owners option "Add as reviewer (blocking)" if an owner uploads a revision? I know that this is expected, but why? For example at Wikimedia we currently have the system, that every change needs a review from an "owner" before the patch gets merged, so it's not useful, if an owner of the package uploads a change, and no reviewers get added. Maybe the a compromise may be a setting, where you can choose, if a package gets added as reviewer too, if the author of the revision is an owner too? I think that would help all.

@joshuaspence, see T11010, I think. Not sure why nginx was 502'ing there, but should be fixed in HEAD.

As an alternative to Luke081515.2's request for an option, I just filed T11118. This would also allow an easy return to the old behaviour of always triggering audits, regardless of who the author is.

In T10939#176633, @Luke081515.2 wrote:

General question: Why is the owners option "Add as reviewer (blocking)" if an owner uploads a revision? I know that this is expected, but why? For example at Wikimedia we currently have the system, that every change needs a review from an "owner" before the patch gets merged, so it's not useful, if an owner of the package uploads a change, and no reviewers get added. Maybe the a compromise may be a setting, where you can choose, if a package gets added as reviewer too, if the author of the revision is an owner too? I think that would help all.

This is essentially identical to the behavior described above, except that the "NOTE" above is untrue.

From elsewhere:

• Create owner package with weak dominion A/ with path A/
• Create a revision that modifies directory A/B/ with no owner package in it
• Create a weak dominion owner package A/B/ for path A/B/ with myself as one of the owners
• Update revision with any changes

This does not remove package A as a reviewer, but reasonably could if package A was added by automatic rules and has not been touched since then (we can track this after T10939). If a Herald rule or a user explicitly added it, we should not remove it, and probably should not remove it if other users have interacted with it by acting with authority on behalf of it.

Alongside this scenario, it was reported that package "A" can not be removed as a reviewer. If so, this is strictly a bug.