Summary

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 Oct. 30, 2012, 7:53 p.m.

Information

Owner

miche.chuang

Repository

Review Board

Branch

issue-2552

Bugs

2552

Depends On

Reviewers

Groups

reviewboard

People

Description

- Fixed URL to return 404.
- Changed to use pk when getting a FileDiffResource queryset instead of local id to pass unit test.

Testing Done

Tested on local development server against custom urls.

Issues

Description	From	Last Updated
Nit: no newline needed.	mike_conley	Oct. 7, 2012, 10:57 a.m.
Spaces on either side of the =	mike_conley	Oct. 7, 2012, 10:57 a.m.
Nit: whitespace	mike_conley	Oct. 7, 2012, 10:57 a.m.
Whitespace in this block. Spaces on either side of the =.	mike_conley	Oct. 7, 2012, 10:57 a.m.
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_conley	Oct. 14, 2012, 12:19 p.m.
No spaces on either side of the equals when passing in named parameters.	mike_conley	Oct. 14, 2012, 12:19 p.m.
Needs to be < 80 columns.	chipx86	Oct. 24, 2012, 11:28 p.m.
Put these in the parameter list to the function, defaulting to None.	chipx86	Oct. 24, 2012, 11:28 p.m.
This is specific to FileDiffCommentResource, and so should live there.	chipx86	Oct. 25, 2012, 12:36 a.m.
Is there a time when we would not have a review ID?	chipx86	Oct. 19, 2012, 1:01 a.m.
Needs to be < 80 columns. Change args, *kwargs to be on the next line.	chipx86	Oct. 24, 2012, 11:28 p.m.
We should just reuse review_request_id.	chipx86	Oct. 25, 2012, 12:38 a.m.
Blank line before this.	chipx86	Oct. 25, 2012, 12:38 a.m.
Given the length of arguments and the wrapping, do one parameter per line, all aligned. That includes 'request' (on the …	chipx86	Oct. 25, 2012, 12:40 a.m.

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

reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: no newline needed.
```
reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Spaces on either side of the =
```
reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: whitespace
```
reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Whitespace in this block. Spaces on either side of the =.
```

Change Summary:

Fixed whitespace.

Diff:

Revision 2 (+20 -5)

Show changes

reviewboard/webapi/resources.py

Change Summary:

Fixed trailing whitespace.

Diff:

Revision 3 (+20 -5)

Show changes

reviewboard/webapi/resources.py

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

reviewboard/webapi/resources.py (Diff revision 3)

The issue has been resolved. 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

reviewboard/webapi/resources.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No spaces on either side of the equals when passing in named parameters.
```

Diff:

Revision 4 (+23 -5)

Show changes

reviewboard/webapi/resources.py

Fix it!

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.

miche.chuang Oct. 25, 2012, 1:03 a.m.

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.)

reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Needs to be < 80 columns.
```
reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Put these in the parameter list to the function, defaulting to None.
```
reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This is specific to FileDiffCommentResource, and so should live there.
```

reviewboard/webapi/resources.py (Diff revision 4)

An issue was opened. Show all issues

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

miche.chuang Oct. 25, 2012, 1:03 a.m.

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.

reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Needs to be < 80 columns. Change *args, **kwargs to be on the next line.
```
reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We should just reuse review_request_id.
```
reviewboard/webapi/resources.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line before this.
```

reviewboard/webapi/resources.py (Diff revision 4)

The issue has been resolved. 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).

Change Summary:

Fixes from Christian's code review.

Branch:

+	master

Diff:

Revision 5 (+35 -7)

Show changes

reviewboard/webapi/resources.py

Branch:

-	master
+	issue-2552

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.6.x (7c4c0ce). Thanks!