-
-
-
reviewboard/reviews/views.py (Diff revision 1) list comprehension redefines 'file_attachment' from line 580
Make view diff URL always include the diff revision number.
Review Request #6857 — Created Jan. 31, 2015 and submitted
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 ':' |
reviewbot | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
We have a list of all diffsets pre-computed earlier in the function (see the for diffset in diffsets line). We … |
chipx86 | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
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 | |
Col: 80 E501 line too long (91 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 581 |
reviewbot | |
Col: 55 W291 trailing whitespace |
reviewbot | |
Col: 29 E131 continuation line unaligned for hanging indent |
reviewbot | |
list comprehension redefines 'file_attachment' from line 581 |
reviewbot | |
Col: 28 E502 the backslash is redundant between brackets |
reviewbot | |
Col: 13 E131 continuation line unaligned for hanging indent |
reviewbot | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
list comprehension redefines 'file_attachment' from line 580 |
reviewbot | |
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 | |
Please remove this line. |
david | |
One last comment here. This will probably break on file-attachment only review requests. We should put the second case into … |
david | |
You can just do if diffsets: |
chipx86 | |
You don't need to pass this. Instead, use review_request.pk in the template. |
chipx86 | |
This could now change to just be latest_diff_revision is not None |
david |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+2 -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
-
reviewboard/reviews/views.py (Diff revision 2) 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: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 3 (+2 -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
-
-
reviewboard/reviews/views.py (Diff revision 3) list comprehension redefines 'file_attachment' from line 580
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+2 -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
-
reviewboard/reviews/views.py (Diff revision 4) list comprehension redefines 'file_attachment' from line 580
-
-
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 (whichget_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) 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 useslocal_site_reverse()
to compute the proper URL. We can then use that directly here.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+7 -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
-
-
reviewboard/reviews/views.py (Diff revision 5) list comprehension redefines 'file_attachment' from line 581
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+8 -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
-
-
reviewboard/reviews/views.py (Diff revision 6) Col: 29 E131 continuation line unaligned for hanging indent
-
reviewboard/reviews/views.py (Diff revision 6) list comprehension redefines 'file_attachment' from line 581
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+8 -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
-
reviewboard/reviews/views.py (Diff revision 7) Col: 28 E502 the backslash is redundant between brackets
-
reviewboard/reviews/views.py (Diff revision 7) Col: 13 E131 continuation line unaligned for hanging indent
-
reviewboard/reviews/views.py (Diff revision 7) list comprehension redefines 'file_attachment' from line 580
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+8 -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
-
-
reviewboard/reviews/views.py (Diff revision 8) list comprehension redefines 'file_attachment' from line 580
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+8 -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
-
-
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.
-
-
You accidentally commited your
.DS_Store
file. Please remove it from this review request.I suggest adding
.DS_Store
to your~/.gitignore
file.
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 10 (+8 -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
-
-
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 anelse: latest_diff_revision = None
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+10 -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
-
-
-
reviewboard/reviews/views.py (Diff revision 11) You don't need to pass this. Instead, use
review_request.pk
in the template.
Testing Done: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||
Diff: |
Revision 12 (+10 -2) |
-
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 12) This could now change to just be
latest_diff_revision is not None
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+10 -2) |
-
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
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+10 -2) |