Page MenuHomePhabricator

Display slightly more information about status of revision from main page.
Closed, DuplicatePublic


Problem Description: In the main page (is there a proper name for this?) , where Revisions Waiting For You is displayed, I am unsure who or exactly how many reviewers have accepted the revision. Basically, I wish I didn't have to go into each revision to see if everyone has accepted it and/or who else needs to accept it.

I know there are some tasks floating around to specify rules for allowing revisions not to be accepted without more complex conditions, which I definitely would use.

Solution ideas helping describe the problem: It would be convenient for items in the Revisions Waiting For You section to include either A) Some indication of which reviewers accepted or B) the number of reviewers that have accepted (2/3 accepted). For the reviewer specific indication some ideas may be either the text color, or a small icon immediately after the reviewer name in the list similar to what is shown when you click the link

Event Timeline

Thanks for the request!

Unfortunately, this doesn't describe a root problem, so we can't design a solution to it. See Describing Root Problems for help.

If you aren't sure what's wrong, let us know how we could improve that document to make it more clear what we need.

Next steps:

  • provide a description of the root problem you're encountering so we can accept this as a feature request; or
  • we'll close this task after a few days if we don't receive the information we need.

@epriestley @eadler : T1279 is exactly what I was looking for. Thanks guys. Feel free to close this task as duplicate.

T1279 is talking about the reviewer list on the detail page (/Dxxx), not the list page (/differential/). This was implemented years ago, so I suspect it isn't a duplicate, unless you're years out of date.

@epriestley Oh! You are right. Let me take a stab at my problem then. Some of this is caused by the lack of rules for complex acceptance conditions like in T731.

Currently, I have to click on revisions to obtain information about the status of each reviewer. Specifically from the differential page I do not know if the revision is accepted by everyone or just one person. So I have to click each revision to know whether the revision has been accepted by everyone and who hasn't accepted it. It would be simpler for me if I could see the accepted status of each reviewer in the reviewer list already provided in the overview. That way I can, from one page, go through each revision and know A) which revisions are completely clear to land and B) who I need to push to get their revisions done.

Hopefully that is a problem description.

I think that's just a rewording of the original description, and doesn't provide any new insight or context. It still focuses on a very narrow symptom problem (not being able to see individual reviewer status from the revision list), not the root problem.

I could ask you questions to figure out what your root problem is, but this is what we're trying to avoid by having a well-documented process and a clear description of what we need in requests in Describing Root Problems.

  • Have you read Describing Root Problems?
  • Do you believe you understood it, and that this request includes the information it asks for?

If you have, let's walk through what the document is really trying to ask for and we can try to figure out where the confusion occurred and how we could improve the document to be more clear.

I have read it, but after re-reading let me give this another go. My apologies, I know you guys are busy and I'm sitting here not knowing how to properly formulate a problem description.

Root problem: To ensure quality software, I want to make sure that all reviewers accept a change. Currently, revisions are considered accepted when any one person accepts the change. With that, they sit in my Revisions Waiting For You section. But, they aren't really waiting on me. At least some of them aren't. To determine which ones actually are waiting for me I have to click through all of them to see which ones are actually accepted by everyone.

After writing that, I realize my suggested solution may just be a way of supplementing T731 in the short-term.

Hopefully this helps, and sorry if it doesn't

Do you have blocking reviewers on these diffs?

Most likely your request is covered by other tasks.

T4144 is "please clarify what "blocking" actually means.
T731 and T10574 are "please make everyone accept"
Dashboards are good for building more targeted queries around what diffs and states you want to track (beyond our defaults).

It sounds like you have blocking reviewers enabled, but once you review them, they don't disappear (since they still "need review", just not yours). This is covered for resolution under T4144.

Root problem: To ensure quality software, I want to make sure that all reviewers accept a change.

That's exactly what we're after, thanks!

T731 and T10574 describe implementing this feature, and a few more general variants of it. These are the approaches we'd prefer to take in tackling this problem, rather than workarounds like showing 2 / 3 reviewers have accepted on the list.

If we implemented this list feature and then later implemented an "all reviewers must accept" rule, this information on the list would likely become much less useful or even completely useless by merit of implementing the proper rule, since the "Accepted" state would begin conveying the information you are interested in on its own.

Generally, we're hesitant to build stopgap/temporary solutions when we have a more general solution planned, because the more general solution often means obsoleting or undoing work, and the stopgap solution may make the general solution more difficult. Even in cases where we anticipate the temporary solution to have lasting value, we often prefer to implement the general solution first, because the design of the temporary solution often changes in the context of the bigger change, or once it no longer needs to solve that part of the problem.

As @chad mentions, T4144 is also related, although it's probably more in the vein of a "temporary solution with lasting value" that will be influenced heavily by alternate acceptance conditions.

I'm going to merge this into T10574, which I think is the most specific match for your problem.

(T10574 is broken into several parts, including "Explicit Blocking Reviewers". This may provide a workaround prior to full completion of the task, where you could manually mark all reviewers as blocking until "All Reviewers Must Accept" is formally supported.)