1227508 - The FileDiff review button doesn't render after adjusting the diff revisions

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mauro Doglio [:mdoglio]]

This is happening because those buttons are rendered based on data created on the server side via a custom field. Changing the diff revisions slider triggers a new rendering of the diff viewers based on data fetched from the api which doesn't have the custom code to create the file_dff_reviewer objects.
The solution is probably to always create these objects on the client side using the api.

Comment 1

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: allow authenticated users to track file review (bug 1227508); r=mcote

mozreview: allow authenticated users to track file review (bug 1227508); r?mcote

Comment 2

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: fix rendering of file diff review button (bug 1227508); r=mcote

mozreview: fix rendering of file diff review button (bug 1227508); r?mcote

To synchronise the rendering of the diff review buttons with the rendering
of the diff viewers we are using a client side function queue. This queue requires
every function to explictly declare when the next item should be executed. In bug 1227365
$.funcQueue('diff_files').next() was placed outside of the function, preventing the
the review button from rendering.

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: render diff review buttons for all the diffs (bug 1227508); r=mcote

mozreview: render diff review buttons for all the diffs (bug 1227508); r?mcote

It looks like there's a use case for having the review button rendered
on every revision of a diff and not only on the last one.

Comment 4

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r=mcote

mozreview: fix filediff review button rendering (bug 1227508); r?mcote

The filediff review button needs to render both on page load and when a revision
is selected with the slider but not for interdiffs.

Comment 5

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r=mcote

mozreview: add title attr to file review buttons (bug 1227508); r?mcote

Comment 6

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote

testing: improve coverage of FileDiffReviewerTest (bug 1227508); r?mcote

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8692570 [details]
MozReview Request: mozreview: allow authenticated users to track file review (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26273/#review23805

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 8692571 [details]
MozReview Request: mozreview: fix rendering of file diff review button (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26275/#review24107

Comment 9

[Mark Côté [:mcote]]

Comment on attachment 8692572 [details]
MozReview Request: mozreview: render diff review buttons for all the diffs (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26277/#review24109

::: pylib/mozreview/mozreview/fields.py:313
(Diff revision 1)
> +            files = sum([list(diff.files.all()) for diff in diffsets], [])

Huh that's neat.  Interestingly the help for `sum()` says

    Return the sum of a sequence of numbers
    
But I guess they don't actually have to be numbers. :)

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8692573 [details]
MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26279/#review24111

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:21
(Diff revision 1)
> +    var renderDiffButton = function(collection){

You don't actually use `collection` here, so I would drop it.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:56
(Diff revision 1)
> +

Don't think this blank line is needed.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:67
(Diff revision 1)
> +

I think you can ditch this blank line too.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:68
(Diff revision 1)
> +      if (revision.indexOf('-') === -1) {

What's this looking for '-' about?  Can you add a comment?

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 8692574 [details]
MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26281/#review24113

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:30
(Diff revision 1)
> +            reviewButton.title = 'Click here to change the review status'+

Space before +.

Comment 12

[Mark Côté [:mcote]]

Comment on attachment 8692575 [details]
MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26283/#review24115

Comment 13

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692573 [details]
MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r=mcote

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

Comment 14

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692574 [details]
MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r=mcote

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

Comment 15

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692575 [details]
MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote

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

Comment 16

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692570 [details]
MozReview Request: mozreview: allow authenticated users to track file review (bug 1227508); r=mcote

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

Comment 17

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692571 [details]
MozReview Request: mozreview: fix rendering of file diff review button (bug 1227508); r=mcote

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

Comment 18

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692572 [details]
MozReview Request: mozreview: render diff review buttons for all the diffs (bug 1227508); r=mcote

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

Comment 19

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692573 [details]
MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r=mcote

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

Comment 20

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692574 [details]
MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r=mcote

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

Comment 21

[Mauro Doglio [:mdoglio]]

Comment on attachment 8692575 [details]
MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote

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