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

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.

The issue has been resolved. Show all issues

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

The issue has been resolved. 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	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

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)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```
reviewboard/diffviewer/tests/test_download_patch_error_bundle_view.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Change Summary:

Fix code formatting.

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

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)

The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

This can be:

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

reviewboard/diffviewer/views.py (Diff revision 3)

The issue has been resolved. 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	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
	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.	fc4da4c6e8c34ed53c8ffe65dcea63452f524088
	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

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:: Completed
Change Summary:: Pushed to release-4.0.x (bd2f2f7)