Closed Bug 1227508 Opened 6 years ago Closed 6 years ago

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

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mdoglio, Assigned: mdoglio)

Details

Attachments

(6 files)

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.
mozreview: allow authenticated users to track file review (bug 1227508); r?mcote
Attachment #8692570 - Flags: review?(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.
Attachment #8692571 - Flags: review?(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.
Attachment #8692572 - Flags: review?(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.
Attachment #8692573 - Flags: review?(mcote)
mozreview: add title attr to file review buttons (bug 1227508); r?mcote
Attachment #8692574 - Flags: review?(mcote)
testing: improve coverage of FileDiffReviewerTest (bug 1227508); r?mcote
Attachment #8692575 - Flags: review?(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
Attachment #8692570 - Flags: review?(mcote) → review+
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
Attachment #8692571 - Flags: review?(mcote) → review+
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. :)
Attachment #8692572 - Flags: review?(mcote) → review+
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?
Attachment #8692573 - Flags: review?(mcote) → review+
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 +.
Attachment #8692574 - Flags: review?(mcote) → review+
Attachment #8692575 - Flags: review?(mcote) → review+
Comment on attachment 8692575 [details]
MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote

https://reviewboard.mozilla.org/r/26283/#review24115
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 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 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 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/
Attachment #8692570 - Attachment description: MozReview Request: mozreview: allow authenticated users to track file review (bug 1227508); r?mcote → MozReview Request: mozreview: allow authenticated users to track file review (bug 1227508); r=mcote
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/
Attachment #8692571 - Attachment description: MozReview Request: mozreview: fix rendering of file diff review button (bug 1227508); r?mcote → MozReview Request: mozreview: fix rendering of file diff review button (bug 1227508); r=mcote
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/
Attachment #8692572 - Attachment description: MozReview Request: mozreview: render diff review buttons for all the diffs (bug 1227508); r?mcote → MozReview Request: mozreview: render diff review buttons for all the diffs (bug 1227508); r=mcote
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/
Attachment #8692573 - Attachment description: MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r?mcote → MozReview Request: mozreview: fix filediff review button rendering (bug 1227508); r=mcote
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/
Attachment #8692574 - Attachment description: MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r?mcote → MozReview Request: mozreview: add title attr to file review buttons (bug 1227508); r=mcote
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/
Attachment #8692575 - Attachment description: MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r?mcote → MozReview Request: testing: improve coverage of FileDiffReviewerTest (bug 1227508); r=mcote
Assignee: nobody → mdoglio
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.