Page MenuHomePhabricator

`exact()` function cannot have `viewer()` as argument
Open, Needs TriagePublic

Description

I'm trying to set up a panel listing reviews where the current user has been explicitly tagged.

We have a #reviewers group that list people with good knowledge of the whole platform and provide some pairs of eyes in addition to the real owner of the review.

Unfortunately this means that the "My Reviews" panel becomes unusable for people in the #reviewers group, listing nearly every revision under the Sun.

Ideally a "My Reviews" panel that combines the viewer() and exact() function would totally fit my needs, but as far as I can tell it is not possible to nest them.

Event Timeline

em created this task.Aug 11 2016, 2:07 PM
chad added a subscriber: chad.Aug 11 2016, 2:50 PM

Unfortunately this means that the "My Reviews" panel becomes unusable for people in the #reviewers group, listing nearly every revision under the Sun.

Our expectation is users will use combinations of Herald, Owners, Blocking Reviewers, Reviewers, etc to properly target reviews.

chad added a comment.Aug 11 2016, 2:59 PM

This seems to be what you're looking for specifically?

em added a comment.Aug 11 2016, 3:24 PM

I used the Responsible Users field, as I'd like to see reviews that I have created, not only those for which I'm a reviewer. Bucket by Action is really nice, and filtering by Reviewer limits its usefulness (indeed, No bucketing is automatically forced).

chad added a comment.Aug 11 2016, 3:34 PM

I don't understand what this ticket is about then.

chad added a comment.Aug 11 2016, 4:30 PM

Generally the reason we ask for feature requests to state problems, and not solutions, is that we have a backlog of several years of requests. The request would likely be entering a long tail and may never be built in a reasonable timeframe for your use. By stating the request as a problem, we may be able to help find a solution, though imperfect, that works for you today or group your problem with other similar problems that might lead to a more streamlined solution. We'd rather find a way to help you today than let a request sit in Maniphest for years before it gets addressed.

eadler added a subscriber: eadler.Aug 11 2016, 6:39 PM
em added a comment.EditedAug 11 2016, 9:10 PM

I'm sorry if I sound like proposing solutions instead of taking more time to think about the larger problem, it's just that I have two readily available tools (viewer() and exact()) that combined would do exactly what I needed right now, but I just cannot combine them.

Maybe the workflow we're using is wrong, maybe a right workflow for us doesn't even exist, maybe it's just some missing feature in Phabricator, I really don't know.

I totally understand that you have plenty of feature requests already, I'm definitely not demanding that my requests must be taken care of, I really appreciate all your work.

Sadly I'm not sure what is not clear, I think I expressed my issue clearly enough and I would just end up repeating myself. I haven't followed the development of Phabricator close enough to understand where my expectation don't match yours.

WONTFIX is totally fine for me and is probably the right way to close this task: you already described a way to addess the problem by using Herald, Owners, Blocking Reviewers, Reviewers, etc. which is definitely too much work for a simple dashboard panel I wanted to setup for my colleagues.

Thanks!

chad added a comment.Aug 11 2016, 9:32 PM

I'm trying to set up a panel listing reviews where the current user has been explicitly tagged.

This is what I understood the problem to be, which I replied with a solution for. But it sounds like this is not the only issue you're having setting up queries? That was the part that was confusing to me. So taking a step back, maybe we can start over and you can state what queries you are trying to accomplish and why. They may not be possible in a single dashboard due to performance concerns.

chad added a comment.Aug 11 2016, 9:37 PM

Specifically, it sounds like you want to exclude diffs you're on as a project reviewer for? But only as a panel, you still want to be on those diffs overall?

em added a comment.Aug 11 2016, 9:53 PM

The current status:

  • a My Reviews panel that list reviews for which I'm the author or a reviewer
  • results on that panel get grouped in action "buckets"

My issue with that:

  • being on the #reviewers group the panels list all reviews since they are tagged with it

My try at tackling that issue:

  • creating a query that use exact(@em) in the Responsible Users field provides what I need, by matching for the exact user and ignoring groups such as #reviewers

My issue with that:

  • I cannot sensibly use that query on a shared panel, since my username is hardcoded
  • usually I use viewer() instead of hardcoding a specific user to create shareable queries, but as far as I can tell I cannot combine exact() and viewer()

Your proposed solution in the screenshot:

  • set Reviewers to viewer() instead of Responsible Users
  • somewhat surprising, the Reviewers field is stricter than Responsible Users and doesn't seem to match by groups, which incidentally is what I'm looking for

My issue with such solution:

  • reviews where I'm the author and not a reviewer aren't matched
  • grouping by action "buckets" does not work

To recap, I'd like to see in a query panel all the revisions for which I'm author or reviewers, but only if I'm explicitly listed as one of those and not if I'm not listed but a group I'm part of is listed.

chad added a comment.Aug 11 2016, 9:54 PM

Thanks, this is super helpful. Sorry for the back and forth.

em added a comment.Aug 11 2016, 9:57 PM

No worries, I just started reporting issues upstream and it takes a while for me to understand what are your expectations and where they differ from mine. :D

Is it a reasonable possibility at looking at being more surgical with how project reviewers are added? Our general MO here is reviews should programmatically get targeted to the minimum set of responsible individuals, and people who want to be included outside of that use tools like Herald to watch for diffs they are not included on. Things like setting up code ownership packages, or making different projects for different types of diffs might make reviewing much easier.

I'll leave it up to @epriestley. Looking at D15925, there is a reasonable blueprint to adding current_exact(), or similar, but I don't know what long term plans he might have for this.

em added a comment.Aug 11 2016, 10:18 PM

It's very likely that our workflow is suboptimal, to be honest I just learned about this #reviewers group so I'll definitely discuss this with other people here.

For what is worth, something like current_exact() (or exact_viewer() if I were to bikeshed the name) would definitely fit my needs.

chad added a comment.Aug 11 2016, 10:22 PM

Yeah, my main concern here is it sounds like you're working around #reviewers by installing a different dashboard on everyone's homepage. In which case maybe moving #reviewers to a CC instead might be better. People will still retain awareness, but the reviews will be optional. Pinterest also has some sort of round robin thing built for not overloading a group of reviewers. https://engineering.pinterest.com/blog/shipping-faster-new-pr-processing-system

eadler added a project: Restricted Project.Sep 15 2016, 6:03 PM
brablc added a subscriber: brablc.Apr 25 2018, 3:12 PM