URL to diff comments returns 404 if at least one of diff-id and file-id are invalid.

Review Request #3401 — Created Oct. 6, 2012 and submitted

Information

Review Board
issue-2552

Reviewers

- Fixed URL to return 404.
- Changed to use pk when getting a FileDiffResource queryset instead of local id to pass unit test.
Tested on local development server against custom urls.
Description From Last Updated

Nit: no newline needed.

mike_conleymike_conley

Spaces on either side of the =

mike_conleymike_conley

Nit: whitespace

mike_conleymike_conley

Whitespace in this block. Spaces on either side of the =.

mike_conleymike_conley

So, looking through the rest of resources.py, I think this pattern is more common: diff_revision = kwargs.get('diff_revision', None) review_id = …

mike_conleymike_conley

No spaces on either side of the equals when passing in named parameters.

mike_conleymike_conley

Needs to be < 80 columns.

chipx86chipx86

Put these in the parameter list to the function, defaulting to None.

chipx86chipx86

This is specific to FileDiffCommentResource, and so should live there.

chipx86chipx86

Is there a time when we would not have a review ID?

chipx86chipx86

Needs to be < 80 columns. Change *args, **kwargs to be on the next line.

chipx86chipx86

We should just reuse review_request_id.

chipx86chipx86

Blank line before this.

chipx86chipx86

Given the length of arguments and the wrapping, do one parameter per line, all aligned. That includes 'request' (on the …

chipx86chipx86
mike_conley
  1. Hey Michelle,
    
    Great work here. There might be some more efficient ways of doing what you're doing, but if there are, I don't know them.
    
    I'm just harping on some style stuff, but nothing major.
    
    Thanks,
    
    -Mike
  2. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Nit: no newline needed.
  3. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Spaces on either side of the =
  4. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Nit: whitespace
  5. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Whitespace in this block. Spaces on either side of the =.
  6. 
      
MI
MI
mike_conley
  1. Tests pass, and local testing verifies that this patch indeed fixes the problem.
    
    I went digging through resources.py to see what kind of patterns we use, and I've got a few recommendations. See below.
    
    Once those are fixed, I'll give my ship-it.
    
    -Mike
  2. reviewboard/webapi/resources.py (Diff revision 3)
     
     
     
     
     
    Show all issues
    So, looking through the rest of resources.py, I think this pattern is more common:
    
    diff_revision = kwargs.get('diff_revision', None)
    review_id = kwargs.get('review_id', None)
    
    if diff_revision:
      # Do stuff
    elif review_id
      # Do other stuff
  3. reviewboard/webapi/resources.py (Diff revision 3)
     
     
     
    Show all issues
    No spaces on either side of the equals when passing in named parameters.
  4. 
      
MI
chipx86
  1. So, I made some comments on style, since I think that'll be helpful, but I have a couple questions/comments about the change.
    
    1) Does this do anything for accessing actual individual comments with fake diff revisions or review IDs? (Is that broken too?)
    
    2) Can you go into more detail on the second part of the change involving the local IDs? This doesn't seem completely right to me...
    
    3) There should be unit tests covering each failure case, and making sure the results are expected. These should fail before this change, and pass after.
    1. 1) Accessing individual diff comments with fake diff revisions or review IDs isn't broken.
      
      2) Previously, we would pass in the local ID to FileDiffResource.get_queryset but the function would assume that review_request_id was always primary key instead of the local ID.  I added in a check to convert the local ID to the PK if we were using a local site.
      
      3) The reason why unit tests weren't failing before was because the bugs were hiding each other.  Our unit tests would pass in local IDs for the review request ID, which would be handled like a PK instead and thus wouldn't grab an actual FileDiffResource. However, accessing diff comments also had a bug that would return an empty list of diff comments if the FileDiffResource didn't exist instead of a DOES_NOT_EXIST. Since the unit test only checked to make sure that we weren't returning a DOES_NOT_EXIST, it would always pass. (Sorry if that's a bit rambly.) 
  2. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Show all issues
    Needs to be < 80 columns.
  3. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    Show all issues
    Put these in the parameter list to the function, defaulting to None.
  4. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    Show all issues
    This is specific to FileDiffCommentResource, and so should live there.
  5. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    Show all issues
    Is there a time when we would not have a review ID?
    1. I think we only have the review ID when we try to get the diff comments using review-requests/<review-request>/reviews/<review-id>/diff-comments, and not when we use review-request/<review-request>/files/<file-id>/diffs/<diff-revision>/diff-comments.
  6. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Show all issues
    Needs to be < 80 columns. Change *args, **kwargs to be on the next line.
  7. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Show all issues
    We should just reuse review_request_id.
  8. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Show all issues
    Blank line before this.
  9. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
     
    Show all issues
    Given the length of arguments and the wrapping, do one parameter per line, all aligned. That includes 'request' (on the next line).
  10. 
      
MI
MI
david
  1. Ship It!
  2. 
      
MI
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.6.x (7c4c0ce). Thanks!