Provide a "Reviewers" attachment to "differential.revision.search"
ClosedPublic
Actions

Authored by epriestley on Apr 6 2017, 9:29 PM.

Details

Reviewers

chad
lvital

Commits

rPcefbdbcffe9e: Provide a "Reviewers" attachment to "differential.revision.search"

Summary

Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan

Screen Shot 2017-04-06 at 2.27.47 PM.png (1×2 px, 237 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Apr 6 2017, 9:29 PM

Harbormaster completed remote builds in B16333: Diff 42404.Apr 6 2017, 9:29 PM

Harbormaster completed remote builds in B16333: Diff 42404.

Get rid of isCurrent which is kind of meaningless and confusing.
Add an isBlocking to try to future-proof against that being separated out.

Harbormaster completed remote builds in B16334: Diff 42405.Apr 6 2017, 9:39 PM

noidea

src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php
43	I know you're stripping this away, but out of curiosity, what would this have meant / why is it too complicated? Does it mean whether they've taken any action on the review? Or whether they've taken the last action?

This revision is now accepted and ready to land.Apr 6 2017, 9:40 PM

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.

Closed by commit rPcefbdbcffe9e: Provide a "Reviewers" attachment to "differential.revision.search" (authored by epriestley, committed by epriestley). · Explain WhyApr 6 2017, 9:46 PM

This revision was automatically updated to reflect the committed changes.

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.

Makes some sense... or rather, makes sense why you removed it. Thanks!

epriestley mentioned this in T12669: differential.revision.search doesn't return the reviewers..May 4 2017, 3:40 PM