Yeah, I could see that being a not very elegant path to go down. I guess it depends on how difficult it is to implement f(g(h())). If it is a deep rabbit hole, then it's probably best to just fill in a few specific use cases that we think are compelling, e.g. exactviewer() (assuming you think it's useful).

Thanks for the feedback. There is definitely a problem in general where people are added to too many reviews, but it's generally due to being part of too many owner packages. We have been encouraging teams to prune their ownership to minimize gatekeeping, but it's still helpful to have targeted dashboards.

In D15926#185059, @epriestley wrote:

Particularly, the use cases I think this is most valuable for are:

• Distinguish between things in your "Must Review" bucket and "Should Review" bucket that are there because you have to review them, vs there because some package or project you have authority over has to review them ("Is this for me personally, or just a platform eng revision?")
• Show which projects/packages you have authority over ("Why do I have to review this?").
• Show status of projects/packages ("Why is this blocked on me?").

In T12804#226416, @chad wrote:

I don't believe moving buttons and changing layout will affect page load speed. If you have real performance issues, we should address those in another task.

I think the majority of issues I (and many others at Twitter) run into is due to performance / slow page loads. If there is anything in the UI that could be minimized / simplified to speed up page loads, that would be really helpful. However, I imagine that performance is mainly a back-end issue (and probably our instance's fault in some ways).

@epriestley yes, the tested owner packages were both weak dominion. Sorry for not mentioning that earlier. You've explained the issue perfectly as far as I can tell. The changes you have planned make a lot more sense than the current behavior and should fix the issues we were seeing. Thank you!

That could work too. Perhaps a box of donuts instead though.

In T12758#225035, @epriestley wrote:

Another solution I could imagine is to add a distinct action, other than "Accept", which works like the old "Accept" did, i.e. "Accept for all reviewers I have authority to accept for", say "Accept Magnanimously".

I'm hesitant to pursue this today because I want to be cautious about adding too many actions, and I think other changes may add more valuable actions ("Accept Next Update", draft-state actions from T2543, etc). This action would probably be fine today in isolation since the dropdown doesn't feel too cluttered right now, but if we added it I think it might be the least valuable / least frequently used action in a list that is starting to look pretty bloated six months from now.

(I think it's also somewhat hard to pick a 2-3 word phrase which clearly describes how this potential action differs from "Accept" -- I can't come up with anything good offhand.)

Yep, those repro steps look right to me.

FWIW, a check-all box is what we had in mind.

Thanks, closing this one!

Or I guess, you* can close this since I can't. Whoops.

Scratch that, you're right @epriestley . We just got confused during the workflow and didn't realize jmeador previously accepted.

Whoops, I think I may have totally fudged the screenshots. Let me try to reproduce with proper screenshots.

Just tested, works great. Thanks for the quick fix!

In D17633#212017, @epriestley wrote:

The meaning of isCurrent was "is this reviewer relevant for state calculations against the current state of the revision", which is kind of involved.

For example, a revision with one user who has "Rejected" is normally in state "Needs Revision", until the next update, but not if the author does "Request Review". When they do, they internally void the outstanding rejects and make them noncurrent.

But current-ness depends on a lot of internal state stuff and on differential.sticky-accept and I think it's ultimately too wrapped up in state calculation to be of much use. At best, you could use it to render "Accepted Current Diff" vs "Accepted Older Diff" some of the time, but it doesn't always mean that and there's no real human-readable label for what it does mean.

No prob! If it helps:

Not sure if this is a known bug / missing feature, but FYI just in case...

In T12372#215275, @epriestley wrote:

Great, thanks for testing it! We'll upstream that and I'll file something to put a longer-term fix in place.

"witter.com" <fakeemailtest+163@t>

ah yes this is my very favorite email address of all

Confirming that the patch above works (tested using send-test --to)!

Great, thank you!

Sounds reasonable to me.

Any suggestions to test this on staging without mass emailing everyone / patching prod? I am trying to test this on staging with bin/mail show-outbound --id xxxx (staging has mail disabled), but the recipients / To: list look fine with or without the patch.

Thanks for the quick fix and the huge amount of detail under the hood, we'll give that patch a shot!

As far as "opt-in", are you mostly concerned about performance?

NOTE: Since packages do not own paths exclusively, any user can create a package on / of every repository and be allowed to force-accept every package review because their "everything" package is now a containing package for all other packages. However, they could already just remove the reviewers, so I don't think this is important. We could add options to Owners (e.g., an optional whitelist of "Stronger" packages) or something to prevent this, but I don't plan to do this. Just fire anyone abusing it.

Responding to as much as I can. @eadler @scode feel free to chime in / correct anything below.