Summary

Fix 500s from the diff context resource

Review Request #9065 — Created July 10, 2017 and submitted July 11, 2017, 1:22 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

ce44bed...

Reviewers

Groups

reviewboard

People

Description

The refactoring of views into mixins broke subtly broke the diff context
resource. It was passing in all arguments positionally and the new
CheckLocalSiteAccessViewMixin was pulling local_site_name out of both
kwargs and (mistakenly) from a caller passing *args. We now pass all
arguments in as keyword arguments to avoid this mess.

This patch also removes a bunch of dead imports I noticed in
reviews/views as a result of the aforementioned refactor.

Testing Done


Ran unit tests.
With this patch applied, there are no more 500s on the diff context

  resource when viewing interdiffs.

Issues

Description	From	Last Updated
Looks good, but it would be nicer to remove the imports in a separate change, since otherwise that file isn't …	chipx86	July 10, 2017, 6:55 p.m.
It doesn't look like you posted the right revision range...	david	July 11, 2017, 11:52 a.m.
No blank line.	chipx86	July 11, 2017, 12:14 a.m.
Wrong order.	chipx86	July 11, 2017, 12:14 a.m.
Did you mean to leave this in?	chipx86	July 11, 2017, 12:13 a.m.
Two blank lines.	chipx86	July 11, 2017, 12:13 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Added unit tests

Diff:

Revision 2 (+129 -2)

Show changes

	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_diff_context.py
	reviewboard/webapi/tests/urls.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:

~		With this patch applied, there are no more 500s on the diff context
~		resource when viewing interdiffs.
	~	Ran unit tests.
	~	With this patch applied, there are no more 500s on the diff context resource when viewing interdiffs.

The issue has been resolved. Show all issues

Looks good, but it would be nicer to remove the imports in a separate change, since otherwise that file isn't even being touched.

chipx86 July 10, 2017, 6:07 p.m.

Actually.. can you also add a unit test? Ideally this would have been caught by the tests :/

reviewboard/webapi/tests/test_diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/webapi/tests/test_diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Wrong order.
```
reviewboard/webapi/tests/test_diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Did you mean to leave this in?
```
reviewboard/webapi/tests/urls.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Two blank lines.
```

Change Summary:

Addressed Christian's issues

Commit:

-	ed38c29af9850e64ca62806816c6ac76be2af372
+	ce44bed17729cf290e8d01f35be8ed21daa4ced8

Diff:

Revision 3 (+418 -92)

Show changes

	reviewboard/attachments/tests.py
	reviewboard/diffviewer/tests/test_diff_chunk_generator.py
	reviewboard/diffviewer/tests/test_diff_opcode_generator.py
	reviewboard/scmtools/tests/test_perforce.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

It doesn't look like you posted the right revision range...

Diff:

Revision 4 (+130 -2)

Show changes

	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_diff_context.py
	reviewboard/webapi/tests/urls.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (54c9d2f)