Add a link for viewing deleted file content.
Review Request #7796 — Created Dec. 3, 2015 and submitted
In the diff viewer, it's often useful to evaluate whether or not it was
right to remove a file. Currently, we instead see the message:This file was deleted. The content cannot be displayed.
This change will make it possible to view deleted file content by
providing a clickable link that is labelled "Show content.". An event
listener will requeue the corresponding diff file, where eventually a
GET request ending in the following will be made:fragment/<FileDiffID>/?<OtherQueryStringPairs>&show-deleted=1
To accomplish this without running into any potential caching issues, we
added theshow_deleted
flag to the ETag of the corresponding diff file.
- Ran and passed all relevant reviewboard unit tests.
- Manually verified that deleted files could be correctly displayed on
Chrome and Firefox without triggering a full page reload, and that
diff files were cached correctly.
Description | From | Last Updated |
---|---|---|
What does this do? This won't be compatible with Firefox. Can we do color: unset instead? |
|
|
showDeletedContent. |
|
|
This should come out of the DiffViewerPage's router (see line 59--). You'll want to add some aditional routes, namely: { … |
|
|
Don't we want to allow like show-deleted-content=123,456,738 to show 3 different deleted filediffs? |
|
|
Maybe "-show-deleted", keep it small and consistent. |
|
|
Let's conditionally add this, only if set, so that we don't impact existing caches negatively. (Also, this only makes sense … |
|
|
This means that, by default, we're always going to show deleted content. We should instead be comparing it to 1 … |
|
|
Common usage for functions that take options in JavaScript is to define an options variable that can optionally be passed, … |
|
|
Let's only pass this value if it's 1, so that the URLs can be simple in the common case. |
|
|
We need to use a class, not an ID, since there will be maybe of these. |
|
|
Rather than having to do this query and chop off part of the string (which is likely to break at … |
|
|
No trailing period, or some browsers will break. |
|
|
Doesn't look like any of this is used? |
|
|
I want to check on this logic a bit. We do this exact thing in queueLoadDiff, except we replace a … |
|
|
As mentioned earlier, this should be a class, not an ID. Let's call this "show-deleted-content-action". |
|
|
This would be better as: etag += ':%s' % show_deleted That way, we're not mixing unicode and byte strings. |
|
|
The template serial should be last. |
|
|
This can be options.showDeleted. |
|
|
To prevent an issue with default behavior of the event kicking in due to an event handler crashing, we should … |
|
|
options.showDeleted |
|
|
Should be on the same line. |
|
|
No need for the quotes around showDeleted. |
|
|
Let's move the <a> to the next line, so we have a shorter line length. |
|
|
Can we just use this below, instead of diffViewerPageView? |
|
|
This still needs to be in the form of } else { |
|
|
As I suggest later in this review, we might want to do: options = options || {}; if (options.showDeleted) { … |
|
|
Can you quickly explain why this removal was necessary? |
|
|
Also curious about this removal - is this unrelated clean-up? |
|
|
Instead of options !== undefined, I see more of this pattern in the codebase: At the top of the function: … |
|
|
We should probably keep that documentation in here. |
|
|
/** |
|
|
Needs Args block. |
|
Change Summary:
Implement a rudimentary way to pass a
show_content_id
parameter.
- Clicking on "Show content." will pass a
show_content=<DiffFileID>
query parameter into the URL query string. - In
_setFiles
, a regex is used to extractshow_content
, which gets
passed toqueueLoadDiff
, which in turn passes it togetRenderedDiff
,
which appends it to the end of its own URL query string. - When an API call to get the diff fragment is made, the
get()
method
will copy theshow_content
parameter to the context. - The Django template will use the
show_content
parameter (after
applyingslugify
) to decide whether or not to render the deleted file.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+35 -9) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Removed Files: |

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/templatetags/difftags.py reviewboard/diffviewer/views.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/templatetags/difftags.py reviewboard/diffviewer/views.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/pages/views/diffViewerPageView.js
Change Summary:
Override CSS inheritance so that hyperlinks are underlined and blue.
The
.sidebyside
class overrode the default behavior for<a>
tags. This
resets it back to default.
Fix the caching issue by adding
show_deleted_content
to the ETag.
- Renamed variable
show_content
->show_deleted_content
. - Renamed URL parameter
show_deleted_content
->show-deleted-content
. show_deleted_content
is now a required key inrenderer_settings
.- There are two types of URLs involving
show-deleted-content
:
1./r/17/diff/4/?show-deleted-content=<FileDiffID>
2./r/17/diff/4/fragment/<FileDiffID>/?show-deleted-content=<0 or 1>
- We no longer require
slugify
in the template. - The URL now has a
#<index>
to jump back to the correct file anchor.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+36 -13) |

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/css/pages/diffviewer.less
-
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3) What does this do? This won't be compatible with Firefox.
Can we do
color: unset
instead? -
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 3) showDeletedContent
. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) This should come out of the DiffViewerPage's router (see line 59--).
You'll want to add some aditional routes, namely:
{ ':revision/?page=:page&show-deleted-content=:showContent': 'revision', ':revision/?show-deleted-content=:showContent': 'revisionNoPage' }
And add an additional event handler for the router's
route:revisionNoPage
event. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) Don't we want to allow like
show-deleted-content=123,456,738
to show 3 different deleted filediffs?
Change Summary:
Address the issues raised by Barret.
- Anchors now have a hardcoded blue color that works across browsers.
- Renamed
showContent
->showDeletedContent
. - We now show deleted files without a full page reload. We instead use
an event listener that displays the deleted file (by requeueing a diff
file for loading) when "Show content." is clicked. - Fixed a couple of typos.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+69 -12) |

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
Change Summary:
Remove an extraneous file index in the URL.

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
-
-
reviewboard/diffviewer/renderers.py (Diff revision 5) Maybe "-show-deleted", keep it small and consistent.
-
reviewboard/diffviewer/views.py (Diff revision 5) Let's conditionally add this, only if set, so that we don't impact existing caches negatively. (Also, this only makes sense for deleted files, so it shouldn't be a part of all keys.)
-
reviewboard/diffviewer/views.py (Diff revision 5) This means that, by default, we're always going to show deleted content. We should instead be comparing it to
1
so that we only show deleted content if explicitly asked. Otherwise, third-party consumers of any of these views are going to be in for a surprise.This is better written as:
show_deleted_content = ( self.request.GET.get('show-deleted-content') == '1')
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 5) Common usage for functions that take options in JavaScript is to define an
options
variable that can optionally be passed, and that would have ashowDeletedContent
key if the caller wants to pass this.Callers that don't want to pass a value of
true
for that can just leave off this parameter (so no need to update the call inqueueLoadDiff
). Callers that do want to passtrue
can pass{ showDeletedContent: true }
. -
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 5) Let's only pass this value if it's
1
, so that the URLs can be simple in the common case. -
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 5) We need to use a class, not an ID, since there will be maybe of these.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 5) Rather than having to do this query and chop off part of the string (which is likely to break at some point), let's set a data attribute on the element for the
DiffReviewableView
containing the filediff ID, and then usingthis.$el.data('filediff-id')
instead. -
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 5) No trailing period, or some browsers will break.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 5) Doesn't look like any of this is used?
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 5) I want to check on this logic a bit.
We do this exact thing in
queueLoadDiff
, except we replace a different container, but in both cases we're replacing it with content from the same source. Is this the correct ID?In either case, we're basically duplicating some logic between here and
queueLoadDiff
, and it's complex enough logic that it's worth taking this and putting it into a utility function (say,_loadFileDiffContent
). -
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 5) As mentioned earlier, this should be a class, not an ID.
Let's call this "show-deleted-content-action".
Change Summary:
Factor out the duplicated logic for loading diff file content.
- Eliminated the need for extracting FileDiffID on click altogether.
- Changed
queueLoadDiff
to acceptdiffReviewable
andoptions
instead.
Address most of the issues raised by Christian.
- Renamed
show_deleted_content
->show_deleted
. - For URLs and etags,
show_deleted
is now optional. - Renamed
#show-content-link
->.show-deleted-content-action
. - Added
file-diff-id
as a data attribute on the table. - Cleaned up various extraneous code.
Diff: |
Revision 6 (+75 -40)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
Change Summary:
Updated the Description (renamed flag) and Testing Done text.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
Thanks for all your work on this last semester! Don't know if you're still up for working with us to get this in, but if so, got just a few things I'd like to tweak and then I think it's ready to land :)
-
reviewboard/diffviewer/views.py (Diff revision 6) This would be better as:
etag += ':%s' % show_deleted
That way, we're not mixing unicode and byte strings.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 6) The template serial should be last.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 6) This can be
options.showDeleted
. -
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 6) To prevent an issue with default behavior of the event kicking in due to an event handler crashing, we should do:
e.preventDefault(); e.stopPropagation(); this.trigger('...')
Then, no need for
return
at the end. -
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6) Should be on the same line.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6) No need for the quotes around
showDeleted
. -
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 6) Let's move the
<a>
to the next line, so we have a shorter line length.
Change Summary:
Address the issues raised by Christian.
- Used string interpolation instead of string casting.
- Reordered the template serial to be last.
- Avoided an issue with default behaviour of the event kicking in due
to an event handler crashing. - Eliminated extraneous quotes around object properties.
- Fixed various whitespace issues.
Diff: |
Revision 7 (+79 -40)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
-
Thanks for doing that so quickly! Late night?
Only a couple small things and then I plan to land this for 2.6 :)
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 7) Can we just use
this
below, instead ofdiffViewerPageView
? -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 7) This still needs to be in the form of
} else {
Change Summary:
Revert a couple of changes.
- Restored else to be back on the same line as the curly brace.
- Restored
diffViewerPageView
back tothis
.
Diff: |
Revision 8 (+75 -38)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
-
This is great, Adriano! Just a few tiny nits - otherwise, this makes sense to me.
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 8) As I suggest later in this review, we might want to do:
options = options || {}; if (options.showDeleted) { // ... }
To be more consistent.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 8) Can you quickly explain why this removal was necessary?
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 8) Also curious about this removal - is this unrelated clean-up?
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 8) Instead of
options !== undefined
, I see more of this pattern in the codebase:At the top of the function:
options = options || {}; // ... if (options.something) { // Do a thing }
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 8) We should probably keep that documentation in here.
Change Summary:
Address the issues raised by Mike.
- Used
options == options || {}
instead ofoptions !== undefined
. - Reverted some unrelated cleanup.
- Fleshed out some comments.
Diff: |
Revision 9 (+81 -31)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
Change Summary:
Address the issues raised by Barret.
- Brought the JavaScript docstrings up to standard.
Diff: |
Revision 10 (+116 -35)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less
Change Summary:
Correct a couple of comment typos.
Diff: |
Revision 11 (+116 -35)
|
---|

-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/views.py reviewboard/diffviewer/renderers.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html reviewboard/static/rb/js/pages/views/diffViewerPageView.js reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js reviewboard/static/rb/css/pages/diffviewer.less