Summary

Make view diff URL always include the diff revision number.

Review Request #6857 — Created Jan. 31, 2015 and submitted March 5, 2015, 12:44 p.m.

Information

Owner

jessq

Repository

Review Board

Branch

master

Bugs

3726

Depends On

Commit

aee25a0...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 26 E203 whitespace before ':'	reviewbot	Jan. 31, 2015, 4:47 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Jan. 31, 2015, 4:34 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Jan. 31, 2015, 4:47 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Jan. 31, 2015, 7:09 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Jan. 31, 2015, 7:02 p.m.
We have a list of all diffsets pre-computed earlier in the function (see the for diffset in diffsets line). We …	chipx86	Feb. 8, 2015, 2:28 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Jan. 31, 2015, 7:09 p.m.
It's actually wrong that we were hard-coding a URL here. We should instead have the template pass in a latest_diff_url …	chipx86	Feb. 8, 2015, 2:28 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Feb. 8, 2015, 2:42 p.m.
list comprehension redefines 'file_attachment' from line 581	reviewbot	Feb. 8, 2015, 2:28 p.m.
Col: 55 W291 trailing whitespace	reviewbot	Feb. 8, 2015, 2:42 p.m.
Col: 29 E131 continuation line unaligned for hanging indent	reviewbot	Feb. 8, 2015, 2:42 p.m.
list comprehension redefines 'file_attachment' from line 581	reviewbot	Feb. 8, 2015, 2:36 p.m.
Col: 28 E502 the backslash is redundant between brackets	reviewbot	Feb. 8, 2015, 2:42 p.m.
Col: 13 E131 continuation line unaligned for hanging indent	reviewbot	Feb. 8, 2015, 2:42 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Feb. 8, 2015, 2:36 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 8, 2015, 2:47 p.m.
list comprehension redefines 'file_attachment' from line 580	reviewbot	Feb. 8, 2015, 2:45 p.m.
This won't work if there's a draft that doesn't have a new diff. How about doing: if draft and draft.diffset: …	david	Feb. 16, 2015, 10:33 p.m.
Please remove this line.	david	Feb. 16, 2015, 10:33 p.m.
One last comment here. This will probably break on file-attachment only review requests. We should put the second case into …	david	Feb. 19, 2015, 6:03 a.m.
You can just do if diffsets:	chipx86	March 3, 2015, 11:46 a.m.
You don't need to pass this. Instead, use review_request.pk in the template.	chipx86	March 3, 2015, 11:46 a.m.
This could now change to just be latest_diff_revision is not None	david	March 4, 2015, 7:37 p.m.

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

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 26
 E203 whitespace before ':'
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

Commit:

1126d8765cce2f98bf1cfff0b528bd87eaf09faa

df7e71f31656d97e6c136180e147b9aaeea2750a

Diff:

Revision 2 (+2 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

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

Summary:

I added the diff revision number in the view diff url.

Make view diff URL always include the diff revision number.

Description:

~		I added the diff revision number in the view diff url.
	~	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.

Testing Done:

~		On my local server, I went to My Dashboard, clicked a review request with 2 diff revisions, clicked "View Diff" in the top right corner, and was taken to ".../r/1/diff/2/#index_header" instead of ".../r/1/diff/#index_header". Likewise, for a review request with only 1 diff revision, you are taken to ".../r/1/diff/1/#index_header".
	~	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/.

Commit:

df7e71f31656d97e6c136180e147b9aaeea2750a

4e6a9fb4a70bb3446e256ee7e6807d078fcd7941

Diff:

Revision 3 (+2 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

Commit:

4e6a9fb4a70bb3446e256ee7e6807d078fcd7941

68a2a1b589b3bdc7abb3f243637e79807d2f2f25

Diff:

Revision 4 (+2 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

Ship it!

```
Ship It!
```

reviewboard/reviews/views.py (Diff revision 4)

The issue has been resolved. Show all issues

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.

reviewboard/templates/reviews/review_detail.html (Diff revision 4)

The issue has been resolved. Show all issues

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.

Commit:

68a2a1b589b3bdc7abb3f243637e79807d2f2f25

6b453b0559340814b965e05e560f18f513eee527

Diff:

Revision 5 (+7 -1)

Show changes

	.DS_Store
	reviewboard/reviews/views.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 581
```

Commit:

6b453b0559340814b965e05e560f18f513eee527

c0da9428a8c9302099e20baf0f6f4f18ce5b57f8

Diff:

Revision 6 (+8 -1)

Show changes

	.DS_Store
	reviewboard/reviews/views.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 55
 W291 trailing whitespace
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 29
 E131 continuation line unaligned for hanging indent
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 581
```

Commit:

c0da9428a8c9302099e20baf0f6f4f18ce5b57f8

f32f365a0e8382cd0c7c1a1d64da313f7e158649

Diff:

Revision 7 (+8 -1)

Show changes

	.DS_Store
	reviewboard/reviews/views.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 28
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 13
 E131 continuation line unaligned for hanging indent
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

Commit:

f32f365a0e8382cd0c7c1a1d64da313f7e158649

d8be7911ef32380af3447b550f39baa7b4c13ea5

Diff:

Revision 8 (+8 -1)

Show changes

	.DS_Store
	reviewboard/reviews/views.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 580
```

Commit:

d8be7911ef32380af3447b550f39baa7b4c13ea5

fd42bdf27c7609c31cf72cfec312e2e18eef54d2

Diff:

Revision 9 (+8 -1)

Show changes

	.DS_Store
	reviewboard/reviews/views.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/templates/reviews/review_detail.html

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

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

The issue has been resolved. Show all issues

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.

mike_conley Feb. 10, 2015, 6:45 p.m.

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.

reviewboard/reviews/views.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Please remove this line.
```

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

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

Branch:

release-2.0.x

master

Commit:

fd42bdf27c7609c31cf72cfec312e2e18eef54d2

cb3acb37433db8fd2294e4903d528d5665e6987a

Diff:

Revision 10 (+8 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

reviewboard/reviews/views.py (Diff revision 10)

The issue has been resolved. Show all issues

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

Commit:

cb3acb37433db8fd2294e4903d528d5665e6987a

8ea19ec6d4932a3ebe775b7f6ea08fa79a7fb2d1

Diff:

Revision 11 (+10 -1)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (c49c1a7)

Ship it!

Ship It!

jessq Feb. 22, 2015, 12:24 a.m.

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.

JE

jessq Feb. 22, 2015, 12:25 a.m.
```
Is this a bug?
```

Status:: Re-opened

reviewboard/reviews/views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
You can just do if diffsets:
```
reviewboard/reviews/views.py (Diff revision 11)
The issue has been resolved. Show all issues
```
You don't need to pass this. Instead, use review_request.pk in the template.
```

Testing Done:

		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.

Commit:

8ea19ec6d4932a3ebe775b7f6ea08fa79a7fb2d1

2e7c1bfe35ec929551d3f2ebbec9e1b458952076

Diff:

Revision 12 (+10 -2)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

Ship it!

reviewboard/reviews/views.py (Diff revision 12)
The issue has been resolved. Show all issues
```
This could now change to just be latest_diff_revision is not None
```

Commit:

2e7c1bfe35ec929551d3f2ebbec9e1b458952076

7ae6242b472e9308cfe7a36ce4b14a84600ab9d3

Diff:

Revision 13 (+10 -2)

Show changes

	.DS_Store
	reviewboard/.DS_Store
	reviewboard/reviews/.DS_Store
	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

The .DS_Store files probably shouldn't be committed.

Commit:

7ae6242b472e9308cfe7a36ce4b14a84600ab9d3

aee25a0b299d08f831a6a4c334ad1475cdbb5144

Diff:

Revision 14 (+10 -2)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/reviews/review_detail.html

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (d05b14a)