Summary

Fix binary diff rendering.

Review Request #10766 — Created Oct. 26, 2019 and updated Nov. 30, 2019, 4:12 p.m.

Information

Owner

perplex

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Follows up on previous binary file changes. This will allow the
frontend to render binary files for visual reviewing. This will
only display a diff if the binary file is a image. If the file is
a not a binary, exe for example, it will display this message
'This is a binary file. The content cannot be displayed.' This also
fixes problems where if the same commit with a binary is upload
multiple times the backend would hard fail. Now it returns without
doing anything.

Testing Done

Walked through a standard review process: posted changes,
interact with changes, commented, and posted changes. Also
covered by previouse tests written for image review UI.

Commits

Summary	ID
Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003
use html instead of innerhtml	ad1c108f3969d54da1abf31e8f507a54c4dd14fc
address comments	921855571820bd43a761197d2c8742390127ae5d

Issues

Description	From	Last Updated
Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did …	amalik2	Nov. 30, 2019, 3:52 p.m.
The title of the review request should also be more descriptive (it should be a complete sentence). I think there …	amalik2	Nov. 30, 2019, 4:05 p.m.
E501 line too long (97 > 79 characters)	reviewbot	Nov. 1, 2019, 9:13 p.m.
E501 line too long (102 > 79 characters)	reviewbot	Nov. 1, 2019, 9:13 p.m.
E722 do not use bare except'	reviewbot	Nov. 1, 2019, 9:13 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Nov. 1, 2019, 9:13 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Nov. 1, 2019, 9:15 p.m.
Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 56 Expected '{' and instead saw '.'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 56 Expected an identifier and instead saw '.'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 56 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 57 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 79 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 79 Expected an identifier and instead saw ')'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 79 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 80 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 27 Expected an identifier and instead saw 'else'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 27 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:24 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Nov. 7, 2019, 1:32 p.m.
Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 56 Expected '{' and instead saw '.'.	reviewbot	Nov. 7, 2019, 2:24 p.m.
Col: 56 Expected an identifier and instead saw '.'.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 56 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 57 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 79 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 79 Expected an identifier and instead saw ')'.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 79 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 80 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 27 Expected an identifier and instead saw 'else'.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 27 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 7, 2019, 2:25 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2019, 2:25 p.m.
This will block non-image review UIs, which will brick 3rd party extensions.	brennie	Nov. 30, 2019, 3:52 p.m.
Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more …	amalik2	Nov. 30, 2019, 3:52 p.m.
Same thing as the above comment	amalik2	Nov. 30, 2019, 3:52 p.m.
$container.eq(0).html(xhr.responseText)	brennie	Nov. 30, 2019, 3:52 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/managers.py (Diff revision 1)
Show all issues
```
E501 line too long (97 > 79 characters)
```
reviewboard/attachments/managers.py (Diff revision 1)
Show all issues
```
E501 line too long (102 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
E722 do not use bare except'
```
reviewboard/webapi/resources/draft_filediff.py (Diff revision 1)
Show all issues
```
E501 line too long (103 > 79 characters)
```

Summary:

-	WIP: Debug statements and some backend fixes
+	WIP: Binary Diff Rendering

Summary:

-	WIP: Binary Diff Rendering
+	Binary Diff Rendering

Description:

~		Debug statements and some backend fixes
	~	Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.

Commits:

	Summary	ID
	Debug statements and some backend fixes	a8832f3835561f2b34701936e838dfb8c860581e
	Rebase onto FileDiff binary flag branch	605b595db017e27f6e713ded54177462d06a8005

Diff:

Revision 2 (+16 -22)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/reviews/views.py
	reviewboard/webapi/resources/draft_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/draft_filediff.py (Diff revision 2)
Show all issues
```
E501 line too long (103 > 79 characters)
```

Commits:

	Summary	ID
	Rebase onto FileDiff binary flag branch	605b595db017e27f6e713ded54177462d06a8005
	Rebase onto FileDiff binary flag branch	605b595db017e27f6e713ded54177462d06a8005
	Fix line length for comment	84368dc50c63ad8a3c23f757b1f65f51303cb1c7

Diff:

Revision 3 (+19 -23)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/reviews/views.py
	reviewboard/webapi/resources/draft_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Rebase onto FileDiff binary flag branch	605b595db017e27f6e713ded54177462d06a8005
	Fix line length for comment	84368dc50c63ad8a3c23f757b1f65f51303cb1c7
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab

Diff:

Revision 4 (+5911 -5899)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/webapi/resources/draft_filediff.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/webapi/resources/draft_filediff.py (Diff revision 4)
Show all issues
```
E501 line too long (103 > 79 characters)
```

JSHint

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 55
 Expected ')' to match '(' from line 381 and instead saw 's'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 56
 Expected '{' and instead saw '.'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 56
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 56
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 57
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 79
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 79
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 79
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 27
 Expected an identifier and instead saw 'else'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 27
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 4)
Show all issues
```
Col: 31
 Missing semicolon.
```

Commits:

	Summary	ID
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003

Diff:

Revision 5 (+5916 -5902)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/webapi/resources/draft_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 55
 Expected ')' to match '(' from line 381 and instead saw 's'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 56
 Expected '{' and instead saw '.'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 56
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 56
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 57
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 79
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 79
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 79
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 27
 Expected an identifier and instead saw 'else'.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 27
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)
Show all issues
```
Col: 31
 Missing semicolon.
```

Commits:

	Summary	ID
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003
	use html instead of innerhtml	ad1c108f3969d54da1abf31e8f507a54c4dd14fc

Diff:

Revision 6 (+11595 -11615)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did for this review request as well.

reviewboard/reviews/views.py (Diff revision 6)

Show all issues

Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more sense to move this if statement to the beginning of this block

reviewboard/reviews/views.py (Diff revision 6)
Show all issues
```
Same thing as the above comment
```

Show all issues

The title of the review request should also be more descriptive (it should be a complete sentence). I think there was a page on notion that discusses this but I can't remember what it is, but you can look at recent review requests that have landed for an example.

Each line in the description should be no more than 72 characters as well.

reviewboard/reviews/views.py (Diff revision 6)

Show all issues

This will block non-image review UIs, which will brick 3rd party extensions.

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 6)
Show all issues
```
$container.eq(0).html(xhr.responseText)
```

Summary:

-	Binary Diff Rendering
+	Fix binary diff rendering.

Description:

~		Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.
	~	Follows up on previous binary file changes. This will allow the
	+	frontend to render binary files for visual reviewing. This will
	+	only display a diff if the binary file is a image. If the file is
	+	a not a binary, exe for example, it will display this message
	+	'This is a binary file. The content cannot be displayed.' This also
	+	fixes problems where if the same commit with a binary is upload
	+	multiple times the backend would hard fail. Now it returns without
	+	doing anything.

Testing Done:

	+	Walked through a standard review process: posted changes,
	+	interact with changes, commented, and posted changes. Also
	+	covered by previouse tests written for image review UI.

Commits:

	Summary	ID
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003
	use html instead of innerhtml	ad1c108f3969d54da1abf31e8f507a54c4dd14fc
	Debug statements and some backend fixes	cce36a415102bebecc26af8669378b2747212328
	Fix frontend diff ui script not running and remove debug statements	39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
	shorten line to adhere to max line length	bb8bf7fbaa1c845b22044320c34aaf7797866003
	use html instead of innerhtml	ad1c108f3969d54da1abf31e8f507a54c4dd14fc
	address comments	921855571820bd43a761197d2c8742390127ae5d

Diff:

Revision 7 (+11592 -11620)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.