1141871 - Show review details

Status: RESOLVED WONTFIX

(MozReview Graveyard :: General, defect, P3)

Description

[Ed Morley [:emorley]]

In bug 1001463, one of the sub review requests has received a "ship it".

However all of the entries in the mozreview overview on show_bug.cgi still show as "pending", when I'd expect the one for the "r/4895/#review4039" to show as something other than pending.

Comment 1

[Ed Morley [:emorley]]

Attachment: Screenshot

Comment 2

[:glob ✱]

reviewboard is returning the status as "pending":

{"status": "pending", "last_updated": "2015-03-09T19:56:54Z", "reviewers": ["gps"], "summary": "qimportbz: default to importing all patches & improve prompt text (bug 1001463)", "issue_open_count": 0, "submitter": "edmorley", "commit": "43db45d5f580e5e1f8c902e459617119cd3f3f09", "id": 4895}

Comment 3

[Mark Côté [:mcote]]

Thanks for the feedback.  This is by design, though it seems that this design is flawed. :)

Review Board review requests have a status field that is unrelated to "ship its".  After publishing, this status stays at "pending" until the review request is explicitly closed, at which point it moves to "submitted" or "discarded" (and if the latter, is automatically hidden).

In my use, I close the review request as submitted when I have committed the code.  It also clears up my Review Board dashboard, which I sometimes use, as it is more convenient than the Bugzilla flag list if I have several MozReview requests to look at.  We will probably have the review requests automatically closed after we implement autoland to inbound.

That all said, I'm getting the feeling that the status field is not particularly useful.  We could replace it with, say, a column with the number of "ship its", or perhaps with a custom hybrid field that would transition from "pending" to "ship it!" if there are any "ship its" at all.  The latter might be the best for now.

In general, we're aware that we need to work on the whole reviewer-selection model.

Comment 4

[Ed Morley [:emorley]]

Ah ok - I didn't even realise requests could be closed - I thought "ship it" was the final action.

Comment 5

[Mark Côté [:mcote]]

Attachment: MozReview Request: bz://1141871/mcote

/r/5353 - Bug 1141871 - Include "ship its" in review-request summaries. r=smacleod

Pull down this commit:

hg pull review -r 4d25a20bbdef4e6db50d0fdbfaf2be29c213522a

Comment 6

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5353/#review4619

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 1)
> +    d['ship_its'] = [x.user.username for x in
> +                     review_request.get_public_reviews() if x.ship_it]

This is going to repeat usernames over and over again if they ship-it multiple times (and will include ship-its from previous diffs).

I think we should either
a) only include a shipit count right now
or
b) Properly handle de-duplicating these - but really we also aren't giving anyone a way to mark that their ship-it no longer applies. I think what we really want to do is check a users latest review only (ignoring the ship-it status of any that came before it).

That way your ship-it state is always associated with your last review, it carries forward, but can be cleared by posting a non-ship-it review.

This might be the sort of logic we want to use for the approval extension hook as well.

Comment 7

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/5353/#review4621

> This is going to repeat usernames over and over again if they ship-it multiple times (and will include ship-its from previous diffs).
> 
> I think we should either
> a) only include a shipit count right now
> or
> b) Properly handle de-duplicating these - but really we also aren't giving anyone a way to mark that their ship-it no longer applies. I think what we really want to do is check a users latest review only (ignoring the ship-it status of any that came before it).
> 
> That way your ship-it state is always associated with your last review, it carries forward, but can be cleared by posting a non-ship-it review.
> 
> This might be the sort of logic we want to use for the approval extension hook as well.

Yeah, good call.

Comment 9

[Mark Côté [:mcote]]

Attachment: MozReview Request: Bug 1141871 - Include "ship its" in review-request summaries. r=smacleod

Comment 11

[Mark Côté [:mcote]]

We removed the status column a while ago, and with the move to Phabricator we won't doing more here.