Add a link for viewing deleted file content.

Review Request #7796 — Created Dec. 3, 2015 and submitted

Information

Review Board
master

Reviewers

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 the show_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?

brenniebrennie

showDeletedContent.

brenniebrennie

This should come out of the DiffViewerPage's router (see line 59--). You'll want to add some aditional routes, namely: { …

brenniebrennie

Don't we want to allow like show-deleted-content=123,456,738 to show 3 different deleted filediffs?

brenniebrennie

Maybe "-show-deleted", keep it small and consistent.

chipx86chipx86

Let's conditionally add this, only if set, so that we don't impact existing caches negatively. (Also, this only makes sense …

chipx86chipx86

This means that, by default, we're always going to show deleted content. We should instead be comparing it to 1 …

chipx86chipx86

Common usage for functions that take options in JavaScript is to define an options variable that can optionally be passed, …

chipx86chipx86

Let's only pass this value if it's 1, so that the URLs can be simple in the common case.

chipx86chipx86

We need to use a class, not an ID, since there will be maybe of these.

chipx86chipx86

Rather than having to do this query and chop off part of the string (which is likely to break at …

chipx86chipx86

No trailing period, or some browsers will break.

chipx86chipx86

Doesn't look like any of this is used?

chipx86chipx86

I want to check on this logic a bit. We do this exact thing in queueLoadDiff, except we replace a …

chipx86chipx86

As mentioned earlier, this should be a class, not an ID. Let's call this "show-deleted-content-action".

chipx86chipx86

This would be better as: etag += ':%s' % show_deleted That way, we're not mixing unicode and byte strings.

chipx86chipx86

The template serial should be last.

chipx86chipx86

This can be options.showDeleted.

chipx86chipx86

To prevent an issue with default behavior of the event kicking in due to an event handler crashing, we should …

chipx86chipx86

options.showDeleted

chipx86chipx86

Should be on the same line.

chipx86chipx86

No need for the quotes around showDeleted.

chipx86chipx86

Let's move the <a> to the next line, so we have a shorter line length.

chipx86chipx86

Can we just use this below, instead of diffViewerPageView?

chipx86chipx86

This still needs to be in the form of } else {

chipx86chipx86

As I suggest later in this review, we might want to do: options = options || {}; if (options.showDeleted) { …

mike_conleymike_conley

Can you quickly explain why this removal was necessary?

mike_conleymike_conley

Also curious about this removal - is this unrelated clean-up?

mike_conleymike_conley

Instead of options !== undefined, I see more of this pattern in the codebase: At the top of the function: …

mike_conleymike_conley

We should probably keep that documentation in here.

mike_conleymike_conley

/**

brenniebrennie

Needs Args block.

brenniebrennie
reviewbot
  1. 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
    
    
  2. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    What does this do? This won't be compatible with Firefox.

    Can we do color: unset instead?

    1. I tried using color: unset in Firefox, but that didn't seem to work. After looking at extensions.less, it seems that our preferred anchor colour is #0000FF, so I think I'll just hardcode that instead.

  3. Show all issues

    showDeletedContent.

  4. Show all issues

    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.

  5. Show all issues

    Don't we want to allow like

    show-deleted-content=123,456,738 to show 3 different deleted filediffs?

  6. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/renderers.py (Diff revision 5)
     
     
    Show all issues

    Maybe "-show-deleted", keep it small and consistent.

  3. reviewboard/diffviewer/views.py (Diff revision 5)
     
     
    Show all issues

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

  4. reviewboard/diffviewer/views.py (Diff revision 5)
     
     
     
    Show all issues

    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')
    
  5. Show all issues

    Common usage for functions that take options in JavaScript is to define an options variable that can optionally be passed, and that would have a showDeletedContent 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 in queueLoadDiff). Callers that do want to pass true can pass { showDeletedContent: true }.

  6. Show all issues

    Let's only pass this value if it's 1, so that the URLs can be simple in the common case.

  7. Show all issues

    We need to use a class, not an ID, since there will be maybe of these.

  8. Show all issues

    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 using this.$el.data('filediff-id') instead.

  9. Show all issues

    No trailing period, or some browsers will break.

  10. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Doesn't look like any of this is used?

  11. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  12. Show all issues

    As mentioned earlier, this should be a class, not an ID.

    Let's call this "show-deleted-content-action".

  13. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
AD
chipx86
  1. 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 :)

  2. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
    Show all issues

    This would be better as:

    etag += ':%s' % show_deleted
    

    That way, we're not mixing unicode and byte strings.

  3. Show all issues

    The template serial should be last.

  4. Show all issues

    This can be options.showDeleted.

  5. Show all issues

    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.

  6. Show all issues

    options.showDeleted

  7. Show all issues

    Should be on the same line.

  8. Show all issues

    No need for the quotes around showDeleted.

  9. Show all issues

    Let's move the <a> to the next line, so we have a shorter line length.

  10. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Thanks for doing that so quickly! Late night?

    Only a couple small things and then I plan to land this for 2.6 :)

  2. Show all issues

    Can we just use this below, instead of diffViewerPageView?

    1. I think it will still work, but the reason why I did this was because PyCharm's JS linter highlighted it as a potentially invalid usage of this, since the this from the closure is different from the this in the outer context.

    2. It's okay in this case. $.funcQueue takes a context parameter (see the this at the end of the call), which makes that available within the function. It's safe to use.

  3. Show all issues

    This still needs to be in the form of } else {

  4. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. This is great, Adriano! Just a few tiny nits - otherwise, this makes sense to me.

  2. Show all issues

    As I suggest later in this review, we might want to do:

    options = options || {};
    
    if (options.showDeleted) {
      // ...
    }
    

    To be more consistent.

  3. Show all issues

    Can you quickly explain why this removal was necessary?

  4. Show all issues

    Also curious about this removal - is this unrelated clean-up?

    1. Yeah, a lot of this was unrelated cleanup. I'll go ahead and revert them.

    2. I think clean-up is fine, but maybe for a separate patch.

  5. Show all issues

    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
    }
    
  6. Show all issues

    We should probably keep that documentation in here.

  7. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    /**

  3. Show all issues

    Needs Args block.

  4. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. This has a thumbs up from me. Thanks Adriano!

  2. 
      
AD
reviewbot
  1. 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
    
    
  2. 
      
AD
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.6.x (5b87238)
Loading...