Summary:

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

Review Request #11960 — Created Jan. 21, 2022 and submitted Feb. 28, 2022, 6:18 p.m.

Information
Owner:	kylemclean
Repository:	Review Board
Branch:	release-4.0.x
Bugs:	4958
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

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.

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
	Fix diff error bundle using wrong string type on Python 3.
	Add unit tests for DownloadPatchErrorBundleView.
	Make requested changes from Christian Hammond's review

Screenshots

Issues

Description	From	Last Updated
Please add the bug number to the "Bugs" field.	david	Jan. 22, 2022, 8:52 a.m.
Can we add a unit test for this?	david	Jan. 23, 2022, 10:12 a.m.
E501 line too long (83 > 79 characters)	reviewbot	Jan. 23, 2022, 10:19 a.m.
W292 no newline at end of file	reviewbot	Jan. 23, 2022, 10:14 a.m.
Imports in Python are in 3 groups, each separated by a blank line: Standard library imports Third-party modules (from the …	chipx86	Feb. 22, 2022, 8:03 p.m.
Docstring summaries must be one line. If this can't fit, just simplify the reference to DownloadPatchErrorBundleView.	chipx86	Feb. 22, 2022, 8:03 p.m.
The trailing """ must be on its own line. Same below.	chipx86	Feb. 22, 2022, 8:03 p.m.
This can be: self.spy_on(..., op=kgb.SpyOpReturn(HttpResponse()))	chipx86	Feb. 22, 2022, 8:03 p.m.
Can you group this with the other from ... imports in this import group? Make sure it's grouped alphabetically.	chipx86	Feb. 22, 2022, 8:03 p.m.

flake8 passed.

JSHint passed.

Show all issues

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

Show all issues

Can we add a unit test for this?

kylemclean Jan. 23, 2022, 10:13 a.m.

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.

Change Summary:

Add bug number to Bugs field

Bugs:

+	4958

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
-		Fix diff error bundle using wrong string type on Python 3.
+		Fix diff error bundle using wrong string type on Python 3.
+		Add unit tests for DownloadPatchErrorBundleView.

Diff:

Revision 2 (+154 -4)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 2)
Show all issues
```
E501 line too long (83 > 79 characters)
```
reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 2)
Show all issues
```
W292 no newline at end of file
```

Change Summary:

Fix code formatting.

Commits:

	Summary
-		Fix diff error bundle using wrong string type on Python 3.
-		Add unit tests for DownloadPatchErrorBundleView.
+		Fix diff error bundle using wrong string type on Python 3.
+		Add unit tests for DownloadPatchErrorBundleView.

Diff:

Revision 3 (+156 -4)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 3)

Show all issues

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

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

Inside of each group, we order them:

import ... entries before from ... entries
Alphabetical within each.

reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 3)

Show all issues

Docstring summaries must be one line.

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

reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 3)
Show all issues
```
The trailing """ must be on its own line.
Same below.
```

reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 3)

Show all issues

This can be:

self.spy_on(...,
            op=kgb.SpyOpReturn(HttpResponse()))

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.

Change Summary:

Make requested changes from Christian Hammond's review

Commits:

	Summary
-		Fix diff error bundle using wrong string type on Python 3.
-		Add unit tests for DownloadPatchErrorBundleView.
+		Fix diff error bundle using wrong string type on Python 3.
+		Add unit tests for DownloadPatchErrorBundleView.
+		Make requested changes from Christian Hammond's review

Diff:

Revision 4 (+163 -17)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (bd2f2f7)