1244884 - Add a quick way to get to interdiff since last review

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Steps to reproduce:

1) Load https://reviewboard.mozilla.org/r/25601/
2) I have to scroll to the bottom to see what the revision I reviewed was.
2) Then I have to scroll back up to the top to click the "diff" thing to see the
   changes since my last review.

That's annoying.  An "interdiff since my last review" would be good.  Or as smacleod notes on IRC some annotation above the version slider in the diff view showing where the last review was.

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: mozreview: display last reviewed diff revision (Bug 1244884).

We will display the diff revision that was published most recently, but
before, the reviewers last review. Using these timestamps has problems
when drafts are left open and new revisions are added, but it should
cover the majority of cases for now.

In the future we'll want to improve upon the method used to calculate
the revision (see the TODO comment in the code) and possibly the
messaging / UI.

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

Comment 2

[:glob ✱]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

https://reviewboard.mozilla.org/r/72690/#review70538

::: pylib/mozreview/mozreview/diffs.py:352
(Diff revision 1)
> +
> +    If no user is provided, return the last diff revision that
> +    any user has reviewed. If no review is present for the user
> +    (or any user when user is None) return None.
> +    """
> +    # TODO: There is a bit of complexity around identifying the revision

please file a bug for this TODO item, and reference the bug number in this comment.

::: pylib/mozreview/mozreview/diffs.py:380
(Diff revision 1)
> +    try:
> +        review = reviews[0]
> +    except IndexError:
> +        return None

using exceptions for flow control is bad due to their cost.

instead:

if reviews:
    review = reviews[0]
else:
    return None

::: pylib/mozreview/mozreview/static/mozreview/js/last_reviewed.js:4
(Diff revision 1)
> +$(document).ready(function() {
> +  var revision = $('#user_data').data('last-reviewed-revision');
> +
> +  if (typeof revision != 'undefined') {

use !== instead of != (and install jshint into your editor)

::: pylib/mozreview/mozreview/static/mozreview/js/last_reviewed.js:7
(Diff revision 1)
> +  var revision = $('#user_data').data('last-reviewed-revision');
> +
> +  if (typeof revision != 'undefined') {
> +    $('#diff_revision_selector')
> +      .prepend($('<p/>')
> +      .text(gettext('You last reviewed revision ' + revision + '.')));

is it possible to linkify "revision N", so clicking on it sets the "from" slider?

::: pylib/mozreview/mozreview/templates/mozreview/user-data.html:5
(Diff revision 1)
>  {% load mozreview %}
>  <div id="user_data"
>    data-scm-level="{{ request.mozreview_profile|scm_level }}"
>    {% if request.user and review_request %}
> +    {{ review_request|data_reviewed_revision:request.user|safe }}

as per the templatetag issue, this template should generate the data-last-reviewed-revision attribute.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:79
(Diff revision 1)
>  
>  @register.filter()
> +def data_reviewed_revision(review_request, user):
> +    revision = latest_revision_reviewed(review_request, user=user)
> +    return ('' if revision is None else
> +            'data-last-reviewed-revision="%s"' % revision)

you shouldn't return html here; return the revision and let the calling template determine how to use it in html.

Comment 3

[Steven MacLeod [:smacleod]]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

https://reviewboard.mozilla.org/r/72690/#review70538

> using exceptions for flow control is bad due to their cost.
> 
> instead:
> 
> if reviews:
>     review = reviews[0]
> else:
>     return None

Indexing is used here to cause Django to have `LIMIT 1` in the sql query. Your proposed solution will request all of the reviews. Going to drop this, please reopen with a rebuttal if you disagree still.

> is it possible to linkify "revision N", so clicking on it sets the "from" slider?

Yes, it's possible, we can iterate on this. Dropping.

> you shouldn't return html here; return the revision and let the calling template determine how to use it in html.

Ya, I was doing this because I was thinking there could be a review on revision 0, which would mean I'd have to do some silly string parsing in the JS to make things work. I'll just switch things though so 0 is considered no review.

Comment 4

[:glob ✱]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

https://reviewboard.mozilla.org/r/72690/#review70538

> Indexing is used here to cause Django to have `LIMIT 1` in the sql query. Your proposed solution will request all of the reviews. Going to drop this, please reopen with a rebuttal if you disagree still.

reopening; exceptions as flow control is an anti-pattern that i'm keen to avoid.  i'll try to find an alternative.

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

https://reviewboard.mozilla.org/r/72690/#review70538

> use !== instead of != (and install jshint into your editor)

jshint didn't notice this. But ya, no harm in changing it.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

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

Comment 7

[:glob ✱]

Comment on attachment 8782582 [details]
mozreview: display last reviewed diff revision (Bug 1244884).

https://reviewboard.mozilla.org/r/72690/#review70958

::: pylib/mozreview/mozreview/diffs.py:382
(Diff revisions 1 - 2)
>      reviews = review_request.get_public_reviews().order_by('-timestamp')
>  
> -    if user is not None:
> +    if user is not None and user.is_authenticated():
>          reviews = reviews.filter(user=user)
>  
> -    try:
> +    review = reviews.first()

thanks - much clearer :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/46b42389724b
mozreview: display last reviewed diff revision . r=glob