Make view diff URL always include the diff revision number.

Review Request #6857 — Created Jan. 31, 2015 and submitted

Information

Review Board
master
aee25a0...

Reviewers

If you click the "View Diff" button when viewing a review request, it will take you to the URL .../r/<review-request-id>/diff/, which shows the latest diff. However, a URL like .../r/<review-request-id>/diff/<revision-id>/, which specifies the revision number of the latest diff would be more useful. For example, if you copy and paste the URL while the latest diff is 6 and revisit it at a later time when new diff revisions have been made, it will still take you to diff revision 6.

Tested with logged in, anonymous, and non-admin users viewing published review requests with 1 and 2 diff revisions.

Tested with an un-published draft that adds a second revision of the diff. The user who owns the draft is linked to ../diff/2/ and a non-admin user is linked to ../diff/1/.

Tested with drafts without a diff, and attachment only review requests.

Description From Last Updated

Col: 26 E203 whitespace before ':'

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

We have a list of all diffsets pre-computed earlier in the function (see the for diffset in diffsets line). We …

chipx86chipx86

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

It's actually wrong that we were hard-coding a URL here. We should instead have the template pass in a latest_diff_url …

chipx86chipx86

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 581

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

Col: 29 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 581

reviewbotreviewbot

Col: 28 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 13 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 580

reviewbotreviewbot

This won't work if there's a draft that doesn't have a new diff. How about doing: if draft and draft.diffset: …

daviddavid

Please remove this line.

daviddavid

One last comment here. This will probably break on file-attachment only review requests. We should put the second case into …

daviddavid

You can just do if diffsets:

chipx86chipx86

You don't need to pass this. Instead, use review_request.pk in the template.

chipx86chipx86

This could now change to just be latest_diff_revision is not None

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 26
     E203 whitespace before ':'
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  4. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  3. 
      
david
  1. Can you test this with an un-published draft that adds a second revision of the diff? I'd like to know that it gives the correct links for both the user who owns the draft (should link to the new diff) and another non-admin user (who should see only the public diff).

    I'd also like you to rewrite your summary and description. You can see Christian's in-progress instructions at https://reviews.reviewboard.org/r/6858/diff/#3

  2. 
      
JE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 3)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  4. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  3. 
      
brennie
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     

    We have a list of all diffsets pre-computed earlier in the function (see the for diffset in diffsets line). We don't want to introduce a new database query (which get_latest_diffset() will do), so we should instead find the latest by comparing timestamps earlier when we're iterating, and then use that diffset instead.

  3. It's actually wrong that we were hard-coding a URL here. We should instead have the template pass in a latest_diff_url variable that uses local_site_reverse() to compute the proper URL. We can then use that directly here.

  4. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
     list comprehension redefines 'file_attachment' from line 581
    
  4. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 55
     W291 trailing whitespace
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 29
     E131 continuation line unaligned for hanging indent
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
     list comprehension redefines 'file_attachment' from line 581
    
  5. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
  2. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Col: 28
     E502 the backslash is redundant between brackets
    
  3. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Col: 13
     E131 continuation line unaligned for hanging indent
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  5. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
  2. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 8)
     
     
     list comprehension redefines 'file_attachment' from line 580
    
  4. 
      
JE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        .DS_Store
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 9)
     
     

    This won't work if there's a draft that doesn't have a new diff. How about doing:

    if draft and draft.diffset:
        latest_diff_revision = draft.diffset.revision
    else:
        latest_diff_revision = diffsets[-1].revision
    

    This also avoids Python's weird ternary form, which we generally don't use.

    1. That's on me - I'd seen at least once usage of ternary in the codebase, and thought it was good for use. My bad.

  3. reviewboard/reviews/views.py (Diff revision 9)
     
     

    Please remove this line.

  4. 
      
brennie
  1. You accidentally commited your .DS_Store file. Please remove it from this review request.

    I suggest adding .DS_Store to your ~/.gitignore file.

  2. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
     
     

    One last comment here. This will probably break on file-attachment only review requests.

    We should put the second case into an elif len(diffsets) > 0: and then have an else: latest_diff_revision = None

  3. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
JE
david
  1. Ship It!
    1. I've realized that the #index-header gets truncated like it's supposed to, but (at least for me) the page is no longer taking you to the anchor, which is right above the diff_revision_label.
    2. Is this a bug?
  2. 
      
JE
chipx86
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 11)
     
     

    You can just do if diffsets:

  3. reviewboard/reviews/views.py (Diff revision 11)
     
     

    You don't need to pass this. Instead, use review_request.pk in the template.

  4. 
      
JE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 12)
     
     

    This could now change to just be latest_diff_revision is not None

  3. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/reviews/.DS_Store
        .DS_Store
        reviewboard/.DS_Store
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/reviews/.DS_Store
        .DS_Store
        reviewboard/.DS_Store
    
    
  2. 
      
JY
  1. The .DS_Store files probably shouldn't be committed.

  2. 
      
JE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
JE
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (d05b14a)
Loading...