1226231 - Hard to estimate scope/size of review for mozreview review requests and properly budget time for them

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

When someone asks me for review in bugzilla, it's easy to get a decent estimate of the time I should expect the review to take: I look at the size, in bytes, of the attached diff.  Bugzilla conveniently shows that in the attachment listing.  This is particularly important when there are multiple patches, because you can get this information without having to open them one by one.

I see nothing equivalent with mozreview.  The bugzilla attachments are all the same 40-byte standard redirect, and the actual mozreview UI gives no indication of how extensive the changes in each changeset are.

If I were doing a review every so often, on a FIFO basis, this would not matter.  But since most of my time is spent doing reviews and I have to figure out how to prioritize them and schedule them into whatever time I have available, it's pretty important to estimate up front, and quickly, whether I'm looking at a 10-minute review or a 3-hour one.  Some indication of diff size (at -U8, for consistency with everything else) on the actual bugzilla attachment would be quite helpful here.

Comment 1

[Mark Côté [:mcote]]

We could add this to the MozReview table.  Would that work?  Another option would be including it in the attachment description.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Adding it to the mozreview table means I have to follow a link from the bug to get the info.  It would be better than nothing, but having it directly in the attachment description would work better for me.  Best yet, do both?  ;)

Comment 3

[Mark Côté [:mcote]]

Oh I meant the MozReview table *in* the bug.  But yeah, we could add it to the table in MozReview as well (which we call the commits table).  Or I can do all three, as you say. :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  I don't see much point in duplicating between the mozreview table and the attachment descriptions.  Whichever of those is easier worksforme.  ;)

Comment 5

[Gregory Szorc [:gps]]

We can get file counts easily. I'm not sure about diff sizes or diff stats (insertions and deletions count). But we can calculate those in Mercurial land pretty easily. I even think there is a {diffstat} template that exposes it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

File counts aren't very useful.  What's most useful is some measure of "how much text will I need to read?"

Comment 8

[Andrew McCreight [:mccr8]]

Not having the file size of the diff is a clear regression over bugzilla patches. It isn't a super big problem, but it is unfortunate. Personally I'd be fine with this just being in mozreview and not bugzilla proper.

Comment 9

[Gregory Szorc [:gps]]

As a quick fix, Mercurial can calculate the "diff stats" when it submits changesets to MozReview and include them in the review request somehow. Perhaps in the review request description (which is a copy of the commit message today).

Slightly more effort is to store the diff stats explicitly as a structured type on the review request and add custom UI to display it. Benefits of the latter is we can expose the data easier. e.g. an IRC bot could be like "you just got a one-liner code review: should be an easy review."

Comment 10

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #9)
> As a quick fix, Mercurial can calculate the "diff stats" when it submits
> changesets to MozReview and include them in the review request somehow.
> Perhaps in the review request description (which is a copy of the commit
> message today).
> 
> Slightly more effort is to store the diff stats explicitly as a structured
> type on the review request and add custom UI to display it. Benefits of the
> latter is we can expose the data easier. e.g. an IRC bot could be like "you
> just got a one-liner code review: should be an easy review."

Review Board has the insertion / deletion counts stored for every file change in each diff - it would be quite easy to sum these and spit them out in all the right places.

Comment 11

[Steven MacLeod [:smacleod]]

To fix this we'll want to do a couple things:
 a) Iterate over the filediff objects for the relevant diffset and sum the insertions / deletions.
 b) Spit out this information in the ReviewRequestSummary resource.
 c) Output this information as part of the Commits table (we're already low on space, this may be more difficult).
 d) Update the MozReview BMO extension to make use of the new data in the ReviewRequestSummary resource.

Comment 12

[Gregory Szorc [:gps]]

I /think/ this is covered by the commits in bug 1286017, but we'll want the upcoming notification bots (notably IRC) to be to display the diff size in the notification. By including it, people can quickly see it is small and do a quick review if so inclined.

Comment 13

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #12)
> I /think/ this is covered by the commits in bug 1286017
Correct, the bot should be able to hit the summary API to fetch this information. Alternatively, we *could* tack it onto the pulse messages quite easily. If that'd be better please file a bug for it.

Comment 14

[Gregory Szorc [:gps]]

The Pulse messages are intentionally light weight. I don't think diff size meets the criteria for being included.

Comment 15

[Mark Côté [:mcote]]

bz: The number of lines added (if any) and removed (if any) are now displayed both in the Review Board commits table and in Bugzilla's MozReview table.  I realize this isn't exactly what you were looking for in comment 0, but does this satisfy your estimation needs?

Btw we can get a bit more accurate (see bug 1287551) but I think this is pretty useful on its own.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I'm still trying to get used to the new setup.  Hard to tell how well it works until I use it for a bit...

For one thing, I need to train myself to click on the mozreview link, not the bug link, when deciding whether to do the review.  Sadly, the bug is where a lot of the information that I need to do the review is.  :(

Comment 17

[Mark Côté [:mcote]]

Sorry, clicking on the mozreview vs bug link from where?  Both mozreview (Review Board specifically) and Bugzilla have the same lines-changed stats.  But maybe (probably) I'm misunderstanding you.

Comment 18

[Boris Zbarsky [:bzbarsky]]

From the bugmail.  I hadn't noticed that the bug had the stats, since they were not next to the things I normally click on to do the review (the attachment table).  Thank you for pointing those out.

Comment 19

[Mark Côté [:mcote]]

No problem, it had only landed earlier that day. :)

I'm going to resolve this since I believe we've been able to meet your minimum requirements here.