1321068 - "reviewed"/"not reviewed" controls disappeared

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: DiffViewer, defect)

Description

[Mark Côté [:mcote]]

Waldo just noted that the controls to mark a file as reviewed/not reviewed are missing.  Seems likely that something that went out this week broke it, e.g. bug 1251286.

Comment 1

[:glob ✱]

ah; this worked by adding work items to the end of the 'diff_files' funcqueue, which is no longer processed.

Comment 2

[:glob ✱]

Attachment: mozreview: trigger an event when a filediff is loaded (bug 1321068)

Review commit: https://reviewboard.mozilla.org/r/96488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96488/

Comment 3

[:glob ✱]

Attachment: mozreview: add 'reviewed' buttons via events (bug 1321068)

listen for the diff file loaded event to add 'reviewed' buttons to the filediff
ui.  this was triggered via the funcqueue, however the queue is no longer used
to render file diffs.

Review commit: https://reviewboard.mozilla.org/r/96496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96496/

Comment 4

[David Walsh :davidwalsh]

Comment on attachment 8815618 [details]
mozreview: add 'reviewed' buttons via events (bug 1321068)

https://reviewboard.mozilla.org/r/96496/#review96812

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:60
(Diff revision 1)
>        return 'not reviewed';
>      }
>    };
>  
> -  var renderDiffButton = function() {
> -    $.funcQueue('diff_files').add(function() {
> +  var renderDiffButton = function(fileDiffID) {
> +    fileDiffReviewerCollection.each(function(diffReviewable) {

Instead of an `each` with a few different conditions where we just `return`, what are your thoughts on using `filter`?

```js
var relevantDiffs = fileDiffReviewerCollection.filter(function(diffReviewable) {
  return diffReviewable.get('file_diff_id') == fileDiffID && document.getElementById('file' + fileDiffID);
});
```

That could cut down on the complexity within the `each` a bit.

Comment 5

[:glob ✱]

Comment on attachment 8815618 [details]
mozreview: add 'reviewed' buttons via events (bug 1321068)

https://reviewboard.mozilla.org/r/96496/#review96812

> Instead of an `each` with a few different conditions where we just `return`, what are your thoughts on using `filter`?
> 
> ```js
> var relevantDiffs = fileDiffReviewerCollection.filter(function(diffReviewable) {
>   return diffReviewable.get('file_diff_id') == fileDiffID && document.getElementById('file' + fileDiffID);
> });
> ```
> 
> That could cut down on the complexity within the `each` a bit.

good idea.

i'll go a step further - we're selecting by id so we should be using .find to return a single diffReviewable object.

Comment 6

[:glob ✱]

Comment on attachment 8815618 [details]
mozreview: add 'reviewed' buttons via events (bug 1321068)

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

Comment 7

[David Walsh :davidwalsh]

Comment on attachment 8815613 [details]
mozreview: trigger an event when a filediff is loaded (bug 1321068)

https://reviewboard.mozilla.org/r/96488/#review97742

Comment 8

[David Walsh :davidwalsh]

Comment on attachment 8815618 [details]
mozreview: add 'reviewed' buttons via events (bug 1321068)

https://reviewboard.mozilla.org/r/96496/#review100370

Worked wonderfully for me!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/webtools/reviewboard/rev/8c09c3912c62
mozreview: trigger an event when a filediff is loaded r=davidwalsh

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/04b0741a1b88
mozreview: add 'reviewed' buttons via events r=davidwalsh