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=1To accomplish this without running into any potential caching issues, we
added theshow_deletedflag 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? |
 brennie
|
|
|
showDeletedContent. |
brennie
|
|
|
This should come out of the DiffViewerPage's router (see line 59--). You'll want to add some aditional routes, namely: { … |
brennie
|
|
|
Don't we want to allow like show-deleted-content=123,456,738 to show 3 different deleted filediffs? |
brennie
|
|
|
Maybe "-show-deleted", keep it small and consistent. |
 chipx86
|
|
|
Let's conditionally add this, only if set, so that we don't impact existing caches negatively. (Also, this only makes sense … |
chipx86
|
|
|
This means that, by default, we're always going to show deleted content. We should instead be comparing it to 1 … |
chipx86
|
|
|
Common usage for functions that take options in JavaScript is to define an options variable that can optionally be passed, … |
chipx86
|
|
|
Let's only pass this value if it's 1, so that the URLs can be simple in the common case. |
chipx86
|
|
|
We need to use a class, not an ID, since there will be maybe of these. |
chipx86
|
|
|
Rather than having to do this query and chop off part of the string (which is likely to break at … |
chipx86
|
|
|
No trailing period, or some browsers will break. |
chipx86
|
|
|
Doesn't look like any of this is used? |
chipx86
|
|
|
I want to check on this logic a bit. We do this exact thing in queueLoadDiff, except we replace a … |
chipx86
|
|
|
As mentioned earlier, this should be a class, not an ID. Let's call this "show-deleted-content-action". |
chipx86
|
|
|
This would be better as: etag += ':%s' % show_deleted That way, we're not mixing unicode and byte strings. |
chipx86
|
|
|
The template serial should be last. |
chipx86
|
|
|
This can be options.showDeleted. |
chipx86
|
|
|
To prevent an issue with default behavior of the event kicking in due to an event handler crashing, we should … |
chipx86
|
|
|
options.showDeleted |
chipx86
|
|
|
Should be on the same line. |
chipx86
|
|
|
No need for the quotes around showDeleted. |
chipx86
|
|
|
Let's move the <a> to the next line, so we have a shorter line length. |
chipx86
|
|
|
Can we just use this below, instead of diffViewerPageView? |
chipx86
|
|
|
This still needs to be in the form of } else { |
chipx86
|
|
|
As I suggest later in this review, we might want to do: options = options || {}; if (options.showDeleted) { … |
 mike_conley
|
|
|
Can you quickly explain why this removal was necessary? |
mike_conley
|
|
|
Also curious about this removal - is this unrelated clean-up? |
mike_conley
|
|
|
Instead of options !== undefined, I see more of this pattern in the codebase: At the top of the function: … |
mike_conley
|
|
|
We should probably keep that documentation in here. |
mike_conley
|
|
|
/** |
brennie
|
|
|
Needs Args block. |
brennie
|
AD
- Change Summary:
-
Implement a rudimentary way to pass a
show_content_idparameter.- 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_contentparameter to the context. - The Django template will use the
show_contentparameter (after
applyingslugify) to decide whether or not to render the deleted file.
- Clicking on "Show content." will pass a
- Description:
-
Summary:
In the diff viewer, it's often useful to evaluate whether or not it was
right to remove a file. Currently, we instead see something like the following: This file was deleted.This change will make it possible to view deleted file content by
allowing the user to click on some kind of toggle. For example, something like: ~ This file was deleted (show content).~ This file was deleted. Show content.TODO:
~ - Figure out how to always display a deleted file in a nice way, similar
to a newly created file.
~ - Figure out how to add a toggle using Backbone/jQuery/JS/HTML/CSS.
~ - Figure out various caching issues (etags...?).
~ - Figure out how to display "Show content." in blue with underline.
- - Write appropriate unit tests.
- Figure out how to always display a deleted file in a nice way, similar
- 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
AD
- Change Summary:
-
Override CSS inheritance so that hyperlinks are underlined and blue.
The
.sidebysideclass overrode the default behavior for<a>tags. This
resets it back to default.
Fix the caching issue by adding
show_deleted_contentto the ETag.- Renamed variable
show_content->show_deleted_content. - Renamed URL parameter
show_deleted_content->show-deleted-content. show_deleted_contentis 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
slugifyin the template. - The URL now has a
#<index>to jump back to the correct file anchor.
- Renamed variable
- Summary:
-
[WIP] Implement a toggle for viewing deleted file content.Add a link for viewing deleted file content.
- Description:
-
- Summary:
- In the diff viewer, it's often useful to evaluate whether or not it was
~ right to remove a file. Currently, we instead see something like the ~ right to remove a file. Currently, we instead see the message: - following: ~ This file was deleted.~ This file was deleted. The content cannot be displayed.This change will make it possible to view deleted file content by
~ allowing the user to click on some kind of toggle. For example, ~ allowing the user to click on a link to a URL of the form: - something like: ~ This file was deleted. Show content.~ .?show-deleted-content=<FileDiffID>#<FileIndex>~ ~ This will refresh the page and trigger a new GET request to:
~ TODO:
~ .fragment/<FileDiffID>/?show-deleted-content=<0 or 1>+ ~ - Figure out various caching issues (etags...?).
~ - Figure out how to display "Show content." in blue with underline.
~ To accomplish this without running into any potential caching issues, we
~ added the show_deleted_content flag to the ETag of each diff fragment. - Testing Done:
-
+ - Ran and passed all relevant reviewboard unit tests.
+ - Manually verified that deleted files could be correctly displayed as
desired, and that fragments were cached correctly.
- 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
-
-
-
-
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:revisionNoPageevent. -
AD
- 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:
-
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
~ allowing the user to click on a link to a URL of the form: ~ allowing the user to click on a link labelled "Show content.". An event + listener will requeue the corresponding diff file, where eventually a + GET request ending in the following will be made: ~ .?show-deleted-content=<FileDiffID>#<FileIndex>~ fragment/<FileDiffID>/?show-deleted-content=1- - - This will refresh the page and trigger a new GET request to:
- - .fragment/<FileDiffID>/?show-deleted-content=<0 or 1>To accomplish this without running into any potential caching issues, we
added the show_deleted_content flag to the ETag of each diff fragment. - Testing Done:
-
- Ran and passed all relevant reviewboard unit tests.
~ - Manually verified that deleted files could be correctly displayed as
desired, and that fragments were cached correctly.
~ - Manually verified that deleted files could be correctly displayed on
Chrome and Firefox, and that fragments were cached correctly.
- 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
AD
- Change Summary:
-
Remove an extraneous file index in the URL.
- Diff:
-
Revision 5 (+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
-
-
-
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.)
-
This means that, by default, we're always going to show deleted content. We should instead be comparing it to
1so 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') -
Common usage for functions that take options in JavaScript is to define an
optionsvariable that can optionally be passed, and that would have ashowDeletedContentkey if the caller wants to pass this.Callers that don't want to pass a value of
truefor that can just leave off this parameter (so no need to update the call inqueueLoadDiff). Callers that do want to passtruecan pass{ showDeletedContent: true }. -
-
-
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
DiffReviewableViewcontaining the filediff ID, and then usingthis.$el.data('filediff-id')instead. -
-
-
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). -
As mentioned earlier, this should be a class, not an ID.
Let's call this "show-deleted-content-action".
AD
- Change Summary:
-
Factor out the duplicated logic for loading diff file content.
- Eliminated the need for extracting FileDiffID on click altogether.
- Changed
queueLoadDiffto acceptdiffReviewableandoptionsinstead.
Address most of the issues raised by Christian.
- Renamed
show_deleted_content->show_deleted. - For URLs and etags,
show_deletedis now optional. - Renamed
#show-content-link->.show-deleted-content-action. - Added
file-diff-idas 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
AD
- Change Summary:
-
Updated the Description (renamed flag) and Testing Done text.
- Description:
-
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
~ allowing the user to click on a link labelled "Show content.". An event ~ 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>/?show-deleted-content=1~ fragment/<FileDiffID>/?<OtherQueryStringPairs>&show-deleted=1To accomplish this without running into any potential caching issues, we
~ added the show_deleted_content flag to the ETag of each diff fragment. ~ added the show_deletedflag to the ETag of the corresponding diff file. - Testing Done:
-
- Ran and passed all relevant reviewboard unit tests.
~ - Manually verified that deleted files could be correctly displayed on
Chrome and Firefox, and that fragments were cached correctly.
~ - 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.
-
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 :)
-
This would be better as:
etag += ':%s' % show_deletedThat way, we're not mixing unicode and byte strings.
-
-
-
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
returnat the end. -
-
-
-
AD
- 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
AD
- Change Summary:
-
Revert a couple of changes.
- Restored else to be back on the same line as the curly brace.
- Restored
diffViewerPageViewback 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.
-
As I suggest later in this review, we might want to do:
options = options || {}; if (options.showDeleted) { // ... }To be more consistent.
-
-
-
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 } -
AD
- Change Summary:
-
Address the issues raised by Mike.
- Used
options == options || {}instead ofoptions !== undefined. - Reverted some unrelated cleanup.
- Fleshed out some comments.
- Used
- 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
AD
- 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
AD
- 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
Tool: Pyflakes Processed Files: reviewboard/diffviewer/templatetags/difftags.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/templatetags/difftags.py reviewboard/reviews/templatetags/reviewtags.py Ignored Files: reviewboard/templates/diffviewer/diff_file_fragment.html