Page MenuHomePhabricator
Create Task
Maniphest T11049

Differential home page and "Differential Revisions" Panel not showing same results
Closed, ResolvedPublic
Actions

Description

Context:

Prior to upgrading to last week's stable release, our Differential Revisions Panel on our dashboard would show the expected set of "must review" diffs.

Our setup (last week):

  • One repository, R1.
  • One project, P1. All of our core team are on this project.
  • One Herald rule that would add P1 as a blocking reviewer for any changes to R1.
  • One Dashboard with a "Differential Revisions" panel.

When a diff would come in, the diff would appear in our panel. When at least one person accepted or rejected the diff, it moved out of the "requires action" section. If the author re-uploaded the diff, the diff would reappear on everyone's panel.

After upgrading to last week's stable release we began trying out the new Owners features.

Our new setup looks like so (bolded is new):

  • One repository, R1.
  • One project, P1. All of our core team are on this project.
  • One owners group, O1. P1 is listed as the owner.
  • One Herald rule that would add O1 as a blocking reviewer for any changes to R1.
  • One Dashboard with a "Differential Revisions" panel.

We are now seeing a discrepancy between what's listed in our Differential Revisions panel and what's listed at /differential. Specifically, diffs that need review and that do appear in /differential are not appearing in the panel.

Diff repro steps:

  • Create a diff without any explicit reviewers.
  • Herald automatically adds O1 as a blocking reviewer.

Expected result: Diff shows up as "Must review" in panel.
Actual result: Diff shows up as "Must review" in /differential, but not in panel.

All of the missing diffs have no explicit owner specific. They only have the Herald-added O1 blocking reviewer.

My attempt at understanding why this is happening

Because we are all owners, and because owners packages don't require approval if the author is also an owner, diffs being sent out without explicit reviewers won't show up in our queue.

Based on the above, I am assuming that Owners is not the tool we are looking for and that the previous Project-based blocking reviewer model is more applicable to our use case. That being said, I'm surprised that /differential shows a different query than the panel (I'd assume they would be show the same results).

Related Objects

Event Timeline

featherless created this task.May 27 2016, 3:08 PM
epriestley added a subscriber: epriestley.May 27 2016, 3:29 PM

What query is the query panel actually executing? Does it look exactly like this?

Screen Shot 2016-05-27 at 8.29.13 AM.png (420×613 px, 40 KB)

featherless closed this task as Resolved.May 27 2016, 3:38 PM
featherless claimed this task.

Ah hah, I had a limit of 5 set. I'd assumed that meant "5 per bucket". I see now that it means "5 results across all buckets".

Removing the limit makes the dashboard work as expected.

Thank you!

epriestley added a comment.EditedMay 27 2016, 3:47 PM

There's some discussion of the behavior of limits in T9372#175512 / D15923.

I think we probably should at least show a warning ("This query was limited, so at least some and possibly all buckets may have more results than are shown.") but we can't easily paginate by bucket or determine if a specific bucket has more results currently, in the general case.

If you only care about results in the first bucket, you can create a custom query like:

Responsible Users: viewer()
Status: Needs Review
Bucket: Bucket by Required Action

This will exclude results in some of the buckets (like "Ready to Land" and "Ready to Update") and make it more likely that you get the correct results back even with a small limit.

We could also possibly make buckets with no results not render (instead of rendering with an "empty" message) when the query is returned on a dashboard.

Removing the limit entirely will work correctly (well, until it returns more than the internal limit of 1,000 results) but may also produce a very large panel.

featherless added a comment.May 27 2016, 4:12 PM

Ah you just connected the dots for me about how to create custom queries for Dashboards.

I had thought I could only select one of "Active revisions", "All revisions", and "Authored". Didn't realize those were the "saved queries" from Differential.

Cool! (and thanks again) We'll try making some custom queries now.

epriestley added a comment.May 27 2016, 4:17 PM

Yeah the way that flow works is real dumb / undiscoverable / confusing / hard to use right now. It works great if you know the secret magic, but the secret magic is pretty secret.

We're going to do a "Run Query > Create Dashboard from this Query" flow when we do the next iteration on it, which should improve things a lot.

Phabricator · Built by Phacility