Fix diff error bundle using wrong string type on Python 3.

Review Request #11960 — Created Jan. 21, 2022 and submitted

Information

Review Board
release-4.0.x

Reviewers

This change fixes bug #4958 by changing zipfile in
DownloadPatchErrorBundleView to use BytesIO instead of StringIO, which
makes it work on both Python 2 and 3. Before this change, the view would fail
when Python 3 was used.

This change also adds two unit tests for DownloadPatchErrorBundleView that test
its response when there is or isn't a PatchError raised in rendering the diff.

A review request with a patch that does not apply cleanly was created.
The patch error bundle was successfully downloaded when running
the Review Board dev server on both Python 2 and 3.

Two unit tests were added for DownloadPatchErrorBundleView that test its
responses. It was verified that these unit tests fail on release-4.0.x
when Python 3 is used, and pass on both Python 2 and 3 after this change.

All unit tests in reviewboard.diffviewer.tests were run and passed on
both Python 2 and 3.

Summary ID
Fix diff error bundle using wrong string type on Python 3.
This commit fixes bug #4958 by changing `zipfile` in `DownloadPatchErrorBundleView` to use `BytesIO` instead of `StringIO`, which makes it work on both Python 2 and 3.
9bc012f456be3361fb47af7bf23a8bcd18054ed2
Add unit tests for DownloadPatchErrorBundleView.
Adds test_sends_404_when_no_patch_error and test_sends_bundle_when_patch_error unit tests, which test the responses of DownloadPatchErrorBundleView.
ef399bd8c618f44b14d262c2e16aee40ad641c30
Make requested changes from Christian Hammond's review
c30a4b37cd86f66a455a9e76bbda5f7a57514c22
Description From Last Updated

Please add the bug number to the "Bugs" field.

daviddavid

Can we add a unit test for this?

daviddavid

E501 line too long (83 > 79 characters)

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Imports in Python are in 3 groups, each separated by a blank line: Standard library imports Third-party modules (from the …

chipx86chipx86

Docstring summaries must be one line. If this can't fit, just simplify the reference to DownloadPatchErrorBundleView.

chipx86chipx86

The trailing """ must be on its own line. Same below.

chipx86chipx86

This can be: self.spy_on(..., op=kgb.SpyOpReturn(HttpResponse()))

chipx86chipx86

Can you group this with the other from ... imports in this import group? Make sure it's grouped alphabetically.

chipx86chipx86
david
  1. 
      
  2. Show all issues

    Please add the bug number to the "Bugs" field.

  3. Show all issues

    Can we add a unit test for this?

    1. I have added a unit test. It fails on Python 3 before this change, and passes on both Python 2 and 3 after this change.

  4. 
      
KY
KY
Review request changed
Change Summary:

Add unit tests to DownloadPatchErrorBundleView.

Description:
~  

This commit fixes bug #4958 by changing zipfile in

  ~

This change fixes bug #4958 by changing zipfile in

    DownloadPatchErrorBundleView to use BytesIO instead of StringIO, which
~   makes it work on both Python 2 and 3.

  ~ makes it work on both Python 2 and 3. Before this change, the view would fail
  + when Python 3 was used.

  +
  +

This change also adds two unit tests for DownloadPatchErrorBundleView that test

  + its response when there is or isn't a PatchError raised in rendering the diff.

Testing Done:
   

A review request with a patch that does not apply cleanly was created.

    The patch error bundle was successfully downloaded when running
    the Review Board dev server on both Python 2 and 3.

  +
  +

Two unit tests were added for DownloadPatchErrorBundleView that test its

  + responses. It was verified that these unit tests fail on release-4.0.x
  + when Python 3 is used, and pass on both Python 2 and 3 after this change.

  +
  +

All unit tests in reviewboard.diffviewer.tests were run and passed on

  + both Python 2 and 3.

Commits:
Summary ID
Fix diff error bundle using wrong string type on Python 3.
This commit fixes bug #4958 by changing `zipfile` in `DownloadPatchErrorBundleView` to use `BytesIO` instead of `StringIO`, which makes it work on both Python 2 and 3.
a46e017b48b5be34cab00737f9d5b7d113fa19a0
Fix diff error bundle using wrong string type on Python 3.
This commit fixes bug #4958 by changing `zipfile` in `DownloadPatchErrorBundleView` to use `BytesIO` instead of `StringIO`, which makes it work on both Python 2 and 3.
a46e017b48b5be34cab00737f9d5b7d113fa19a0
Add unit tests for DownloadPatchErrorBundleView.
Adds test_sends_404_when_no_patch_error and test_sends_bundle_when_patch_error unit tests, which test the responses of DownloadPatchErrorBundleView.
7cfc46ace886a3ed31b2647a2f844470ca068725

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

KY
chipx86
  1. 
      
  2. Show all issues

    Imports in Python are in 3 groups, each separated by a blank line:

    1. Standard library imports
    2. Third-party modules (from the point of view of the application)
    3. Application modules.

    Inside of each group, we order them:

    1. import ... entries before from ... entries
    2. Alphabetical within each.
  3. Show all issues

    Docstring summaries must be one line.

    If this can't fit, just simplify the reference to DownloadPatchErrorBundleView.

  4. Show all issues

    The trailing """ must be on its own line.

    Same below.

  5. Show all issues

    This can be:

    self.spy_on(...,
                op=kgb.SpyOpReturn(HttpResponse()))
    
  6. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues

    Can you group this with the other from ... imports in this import group? Make sure it's grouped alphabetically.

  7. 
      
KY
chipx86
  1. Ship It!
  2. 
      
KY
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (bd2f2f7)