- Download Diff

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
Debug statements and some backend fixes
Fix frontend diff ui script not running and remove debug statements
shorten line to adhere to max line length
use html instead of innerhtml
address comments

Screenshots

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
-	Debug statements and some backend fixes
+	Rebase onto FileDiff binary flag branch

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
-	Rebase onto FileDiff binary flag branch
+	Rebase onto FileDiff binary flag branch
+	Fix line length for comment

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
-	Rebase onto FileDiff binary flag branch
-	Fix line length for comment
+	Debug statements and some backend fixes
+	Fix frontend diff ui script not running and remove debug statements

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
-	Debug statements and some backend fixes
-	Fix frontend diff ui script not running and remove debug statements
+	Debug statements and some backend fixes
+	Fix frontend diff ui script not running and remove debug statements
+	shorten line to adhere to max line length

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
-	Debug statements and some backend fixes
-	Fix frontend diff ui script not running and remove debug statements
-	shorten line to adhere to max line length
+	Debug statements and some backend fixes
+	Fix frontend diff ui script not running and remove debug statements
+	shorten line to adhere to max line length
+	use html instead of innerhtml

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
-	Debug statements and some backend fixes
-	Fix frontend diff ui script not running and remove debug statements
-	shorten line to adhere to max line length
-	use html instead of innerhtml
+	Debug statements and some backend fixes
+	Fix frontend diff ui script not running and remove debug statements
+	shorten line to adhere to max line length
+	use html instead of innerhtml
+	address comments

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.

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Fix binary diff rendering.

flake8

Summary:

Summary:

Description:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 failed)

flake8

JSHint

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Commits:

Diff:

Checks run (2 succeeded)

Summary:

Description:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)