We have external interest in providing an "All Reviewers Must Accept" acceptance condition. This spans several technical and product areas which have various ambiguities, but here's some attempt at a concrete plan for pursuing it.
Context:
- T731 discusses multiple acceptance conditions.
- T9372 discusses primarily technical concerns with impacts on bucketing in UIs. T4144 is related, and discuses more product concerns.
- T1279 has additional discussion, although much of it is very old and no out of date.
Some of the ways to move forward are unambiguous; others are not. This is roughly from most-sure to least-sure:
**Move Reviewers Away from Edges**: Long ago, reviewers had a dedicated table. They were migrated to edges after we built edge infrastructure, but they aren't simple edges (the relationship contains additional state data). Changes in T1279 made use of edge data to represent this relationship.
However, Differential is the //only// application which uses edge data. Edge data saw more use in the similar system at Facebook, but we often need to query against rich edges and have selected dedicated tables for storage in essentially all other cases. We should move this data off edges to a dedicated table to enable us to remove edge data completely in the future. Doing this //before// making the system more complex is desirable. See also T8996#133764.
This has zero product impact and is a pure infrastructure blocker.
**Explicit Blocking Reviewers**: I think we should pursue the `Reviewers: username!` syntax and allow blocking reviewers to be adjusted via the UI, now that we can reasonably do this with typeahead functions (T4100). Specifically, the UI representation would be a new `blocking(reviewer)` or `required(reviewer)` function. This isn't technically required to have an "All Reviewers Must Accept" rule (and won't meaningfully function when that rule is selected) but I think it has broader utility than acceptance conditions do and will improve the quality of the product feedback we receive about them, and want to pursue it first, rather than waiting until it gets harder to build.
This will show requirements more clearly, and allow required reviewers to be set manually from the CLI or web UI, as a general flexible tool for solving problems in this class.
**Adjusting Rules**: For v0, I think it's OK to have rule selection work like this:
- Default condition is "any reviewer", just like today.
- Reviewer rules can **ONLY** be changed by a Global Herald action, `[ Set acceptance condition: ][ "All Reviewers Must Accept" ]`.
- If you want this to apply globally, you trigger this "Always".
- If you want this to apply selectively, you trigger it per-repository or whatever else.
- If you want this to be //more// flexible (e.g., editable) you can write add/remove rules which bind it to a tag, or tell us about your use case.
I think this probably covers most use cases, is reasonable to retain in the long run no matter what else happens, and saves us from needing to deal with cases where users are choosing their own acceptance conditions and administrators don't want them to, but also don't want to treat it as a social/cultural problem. It's also very easy to build.
The only downside to this that I can see is that we won't know the rule ahead of time, but I can't foresee any cases where that causes a problem. For example, I don't think there's anything we'd want to do differently when showing the user a commit message template if we knew the eventual rule would be "All Must Accept".
**Rules Themselves**: I think T731 has a reasonable outline for this technically. The hard part of this still is explaining the rules to users, particularly rules like this (from T731):
> Our current ruleset is roughly ( (90% of reviewers approve) AND (no one requested changes) ) OR ( (it has been 8 business hours) AND (at lease 2 reviewers approve) )
We won't implement such a rule upstream, but it should be possible to implement such a rule as an extension and have a reasonable user experience (for example, a new hire should be able to see that a revision will auto-accept in 3 hours, and understand how the rule works overall). We can provide UI hooks for this, it's just a lot of mess to accommodate without butchering the UI.
**Bucketing**: I think this is the most ambiguous case. In T9372, I leaned toward composing queries, but this is very complicated, not particularly flexible, and can not be implemented efficiently in the general case.
In the other extreme, we could treat buckets as a UI filter. This is simple and flexible, but comes at the cost of pagination not working as well: if you have more than a page of items total, all buckets may have more items and we can't tell you if they do or not. Users would more naturally expect to be able to page through buckets individually.
For example, if your 1,000 most recent associated revisions are all waiting on you to land them, your "Blocking Others" bucket will be empty with a message like "there might be stuff in here or not, who knows?".
Perhaps this is acceptable to sacrifice. It is essentially what we do today and no one has ever complained, but it's very rare to have 1,000 revisions, and it is particularly unusual that you would be interested in a revision older than your top 1,000. In contrast, it is more common to have 1,000 audits (although it shouldn't necessarily be common), and it would be nice to have a general-purpose pathway forward here which is flexible enough to handle cases where applications have large result sets.
I //think// we can move "All Reviewers Must Accept" forward without fully resolving this case, and it seems plausible to start with the "UI Filter" approach and discourage extension until we're settled on it. If we move to "composed queries" later, essentially all of the "UI Filter" code would still need to be be present anyway.
---
| Step | Complexity |
|----|----|
| Edge Infrastrucure | 4-8 Hours |
| Blocking UI | 2-4 Hours |
| Herald Actions | 1-2 Hours |
| Actual Rules | 4-6 Hours |
| UI Filter Bucketing | 4-6 Hours |
| **Total Estimate** | **~26 hours** |