1147393 - mozreview UI should let reviewers mark which files have been reviewed

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Patrick Brosset <:pbro>]

When reviewing a commit with a lot of changes (and a lot of files) on mozreview, it gets difficult to track your progress as a reviewer.

All file diffs are displayed on one page only in a given order. This order might not be the order you wish to review the changes, therefore your scroll position isn't an indication of where you are in the review.

I can see 2 ways:
- add a way to collapse/expand individual file diffs
- add a "reviewed" checkbox per file diff

Both could be implemented by the way.

Comment 1

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add FileDiffReviewer model (bug 1147393); r=dminor

mozreview: add FileDiffReviewer model (bug 1147393); r?smacleod

A FileDiffReviewer is a many-to-many relationship between a FileDiff
and one of its reviewer.

Comment 2

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add resource for FillDiffReviewer model; r=dminor

mozreview: add resource for FillDiffReviewer model; r?smacleod

For now FileDiffReviewerResource only allows to change the reviewed
status.

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add review request field for FileDiffReviewer; r=dminor

mozreview: add review request field for FileDiffReviewer; r?smacleod

This field allows to create FileDiffReviewer objects when a reviewer
looks at a set of FileDiff for the first time. It also injects in
the template a json structure that will be used to initialize the ui
counterpart of FileDiffReviewer in one of the following commits.

Comment 4

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add ui bits to track file review status; r=dminor

mozreview: add ui bits to track file review status; r?smacleod

Comment 5

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add tests for review file tracking (bug 1147393); r=dminor

mozreview: add tests for review file tracking (bug 1147393); r?smacleod

Comment 6

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/23325/#review21733

::: pylib/mozreview/mozreview/file_diff_reviewer/models.py:11
(Diff revision 1)
> +    reviewer_id = models.IntegerField()

Please add a comment saying that we can't use models.ForeignKey in reviewboard extensions.

::: pylib/mozreview/mozreview/models.py:16
(Diff revision 1)
> +from mozreview.file_diff_reviewer.models import  FileDiffReviewer

nit: only one space between import and FileDiffReviewer

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8679005 [details]
MozReview Request: mozreview: add resource for FillDiffReviewer model; r=dminor

https://reviewboard.mozilla.org/r/23327/#review21735

::: pylib/mozreview/mozreview/extension.py:121
(Diff revision 1)
> +        file_diff_reviewer_resource,

pedantic: please add this import in alphabetical order

::: pylib/mozreview/mozreview/file_diff_reviewer/resources.py:51
(Diff revision 1)
> +

nit: need an extra line here

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8679006 [details]
MozReview Request: mozreview: add review request field for FileDiffReviewer; r=dminor

https://reviewboard.mozilla.org/r/23329/#review21741

::: pylib/mozreview/mozreview/fields.py:15
(Diff revision 1)
> +from mozreview.file_diff_reviewer.models import FileDiffReviewer

pedantic: this should be after mozreview.extra_data

Comment 9

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/23331/#review21743

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:28
(Diff revision 1)
> +          var reviewButton = document.createElement('button');

I think we should use a checkbox here to be consistent with splinter.

If you do decide to go with a button, it would be nice if the grey and green you use matched a colour already in the reviewboard theme.

Comment 10

[Dan Minor [:dminor]]

Comment on attachment 8679448 [details]
MozReview Request: mozreview: add tests for review file tracking (bug 1147393); r=dminor

https://reviewboard.mozilla.org/r/23419/#review21747

Comment 12

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679004 [details]
MozReview Request: mozreview: add FileDiffReviewer model (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23325/diff/1-2/

Comment 13

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679005 [details]
MozReview Request: mozreview: add resource for FillDiffReviewer model; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23327/diff/1-2/

Comment 14

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679006 [details]
MozReview Request: mozreview: add review request field for FileDiffReviewer; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23329/diff/1-2/

Comment 15

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679008 [details]
MozReview Request: mozreview: add ui bits to track file review status; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23331/diff/1-2/

Comment 16

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679448 [details]
MozReview Request: mozreview: add tests for review file tracking (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23419/diff/1-2/

Comment 17

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/23331/#review21743

> I think we should use a checkbox here to be consistent with splinter.
> 
> If you do decide to go with a button, it would be nice if the grey and green you use matched a colour already in the reviewboard theme.

I'll keep the buttons and adapting the color scheme to the review status icons in the commit list.

Comment 18

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679004 [details]
MozReview Request: mozreview: add FileDiffReviewer model (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23325/diff/2-3/

Comment 19

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679005 [details]
MozReview Request: mozreview: add resource for FillDiffReviewer model; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23327/diff/2-3/

Comment 20

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679006 [details]
MozReview Request: mozreview: add review request field for FileDiffReviewer; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23329/diff/2-3/

Comment 21

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679008 [details]
MozReview Request: mozreview: add ui bits to track file review status; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23331/diff/2-3/

Comment 22

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679448 [details]
MozReview Request: mozreview: add tests for review file tracking (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23419/diff/2-3/

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 8679004 [details]
MozReview Request: mozreview: add FileDiffReviewer model (bug 1147393); r=dminor

https://reviewboard.mozilla.org/r/23325/#review22455

Comment 24

[Dan Minor [:dminor]]

Comment on attachment 8679008 [details]
MozReview Request: mozreview: add ui bits to track file review status; r=dminor

https://reviewboard.mozilla.org/r/23331/#review22457

Comment 25

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679004 [details]
MozReview Request: mozreview: add FileDiffReviewer model (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23325/diff/3-4/

Comment 26

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679005 [details]
MozReview Request: mozreview: add resource for FillDiffReviewer model; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23327/diff/3-4/

Comment 27

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679006 [details]
MozReview Request: mozreview: add review request field for FileDiffReviewer; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23329/diff/3-4/

Comment 28

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679008 [details]
MozReview Request: mozreview: add ui bits to track file review status; r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23331/diff/3-4/

Comment 29

[Mauro Doglio [:mdoglio]]

Comment on attachment 8679448 [details]
MozReview Request: mozreview: add tests for review file tracking (bug 1147393); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23419/diff/3-4/

Comment 30

[Mauro Doglio [:mdoglio]]

This will go to prod in the next few days.