Fix binary diff rendering.

Review Request #10766 — Created Oct. 26, 2019 and updated

perplex
Review Board
master
reviewboard, students

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.

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.

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

amalik2amalik2

The title of the review request should also be more descriptive (it should be a complete sentence). I think there …

amalik2amalik2

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (102 > 79 characters)

reviewbotreviewbot

E722 do not use bare except'

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.

reviewbotreviewbot

Col: 56 Expected '{' and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 79 Missing semicolon.

reviewbotreviewbot

Col: 79 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 79 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw 'else'.

reviewbotreviewbot

Col: 27 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.

reviewbotreviewbot

Col: 56 Expected '{' and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 79 Missing semicolon.

reviewbotreviewbot

Col: 79 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 79 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw 'else'.

reviewbotreviewbot

Col: 27 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

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

brenniebrennie

Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more …

amalik2amalik2

Same thing as the above comment

amalik2amalik2

$container.eq(0).html(xhr.responseText)

brenniebrennie
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

PE
PE
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
PE
Review request changed

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

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

PE
amalik2
  1. 
      
  2. 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.

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

    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

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

    Same thing as the above comment

  5. 
      
amalik2
  1. 
      
  2. 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.

  3. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
     
     
     
     
     

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

  3. $container.eq(0).html(xhr.responseText)

  4. 
      
PE
PE
PE
PE
Review request changed

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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...