Implement archive review UI.

Review Request #10794 — Created Nov. 9, 2019 and updated

perplex
Review Board
master
reviewboard, students

Currently there is no binary file support for RB, exluding my other
changes. With this arhive files can be posted and reviewed at a
basic level. Reviewers can look through a repersentation of the archive
proposed to be submitted. No support for commonenting or presenting the
actual changes is include in this PR as that was out of scope. The
archive is represented as a JSON file tree and does not have any css or
less changes to make it more unified with the rest of RB. These style
changes will need to be included in a later review request. The JSON
file tree is created using a library called JSTree and is currently
under a MIT license, https://github.com/vakata/jstree/. This feature
does require the changes from my other binary file PRs to function.
Specifically: r/10729, r/10728, and one change from r/10766.

Create a test repo and walked through the steps of a review process.
Posted change, look and interacted with change, then shipped change.
Also wrote tests for parsing the archive files; empty, one level,
and multi level archives.

Summary
Add testing and fix some small issues
Implement rendering of archives with JSTree library
Address comments
Move parsing of archives to backend
address suggestions
Address comments
base framework for archive review ui
Loading file attachments...

Description From Last Updated

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 29 'obj' is defined but never used.

reviewbotreviewbot

E401 multiple imports on one line

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 13 Do not use 'new' for side effects.

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E401 multiple imports on one line

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

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

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

The rest of this file is sorted.

brenniebrennie

I think you copied this docstring and did update it.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing file-level docstring.

brenniebrennie

Missing class docstring.

brenniebrennie

Missing docstring.

brenniebrennie

TODO.

brenniebrennie

nit: blank line between these.

brenniebrennie

Missing docstring.

brenniebrennie

nit: blank line between these.

brenniebrennie

Missing docstring. Typo: "recurse"

brenniebrennie

nit: blank line between these.

brenniebrennie

nit: add a trailing comma

brenniebrennie

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

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

reviewbotreviewbot

We don't need to specify empty initialize methods

brenniebrennie

nit: remove this line.

brenniebrennie

nit: remove this line.

brenniebrennie

Leftover debug logging?

brenniebrennie

Since reviewboard already uses fontawesome which has its own loading spinners, it may be better to replace the occurrences of ...

amalik2amalik2

I'm most likely wrong about this since I know very little about licensing but I think that even for permissive ...

amalik2amalik2

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

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

reviewbotreviewbot

diffAgainstArchiveMetadata should also be mentioned in the docstring

amalik2amalik2

Should this be mode? The subclasses of BaseArchiveView define mode instead of node.

amalik2amalik2

Did you define CSS classes anywhere for the different subclasses of BaseArchiveView? I can't find anything by doing a search ...

amalik2amalik2

I'm curious as to how this works... does jstree modify the String prototype to add a jstree method to it?

amalik2amalik2

Single quotes should be used

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

What is this used for?

amalik2amalik2

Is there a revision selector slider that gets rendered somewhere? (the text/image based review UIs have it)

amalik2amalik2

Not a huge deal but in general, using Boolean(value) is easier to understand than !!value

amalik2amalik2

Wouldn't this crash if there is a diff type mismatch? this._archiveView looks like it would be undefined in that situation.

amalik2amalik2

To be more consistent with the other CSS files, it would be better to rename the css file to something ...

amalik2amalik2

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

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

reviewbotreviewbot
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

Commits:

Summary
-
Move parsing of archives to backend
-
base framework for archive review ui
+
Move parsing of archives to backend
+
base framework for archive review ui
+
Implement rendering of archives with JSTree library

Diff:

Revision 3 (+5730 -5144)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

Summary:

-WIP: Archive Review UI
+Archive Review UI

Description:

~  

base framework for archive review ui

  ~

Currently there is no binary file support for RB, exluding my other changes. With this arhive files can be posted and reviewed at a basic level. Reviewers can look through a repersentation of the archive proposed to be submitted. No support for commonenting or presenting the actual changes is include in this PR as that was out of scope. The archive is represented as a JSON file tree and does not have any css or less changes to make it more unified with the rest of RB. These style changes will need to be included in a later review request. The JSON file tree is created using a library called JSTree and is currently under a MIT license, https://github.com/vakata/jstree/. This feature does require the changes from my other binary file PRs to function. Specifically: r/10729, r/10728, and one change from r/10766.

Testing Done:

  +

Create a test repo and walked through the steps of a review process. Posted change, look and interacted with change, then shipped change. Also wrote tests for parsing the archive files; empty, one level, and multi level archives.

Commits:

Summary
-
Move parsing of archives to backend
-
base framework for archive review ui
-
Implement rendering of archives with JSTree library
+
Add testing and fix some small issues
+
Implement rendering of archives with JSTree library
+
Move parsing of archives to backend
+
base framework for archive review ui

Diff:

Revision 4 (+5847 -5165)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

Commits:

Summary
-
Add testing and fix some small issues
-
Implement rendering of archives with JSTree library
-
Move parsing of archives to backend
-
base framework for archive review ui
+
Add testing and fix some small issues
+
Implement rendering of archives with JSTree library
+
Move parsing of archives to backend
+
address suggestions
+
base framework for archive review ui

Diff:

Revision 5 (+5851 -5167)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

PE
PE
brennie
  1. 
      
  2. dev-requirements.txt (Diff revision 5)
     
     

    The rest of this file is sorted.

  3. I think you copied this docstring and did update it.

  4. Missing docstring.

  5. Missing docstring.

  6. Missing docstring.

  7. Missing docstring.

  8. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    Missing file-level docstring.

  9. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    Missing class docstring.

  10. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    Missing docstring.

  11. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
  12. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     

    nit: blank line between these.

  13. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    Missing docstring.

  14. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     

    nit: blank line between these.

  15. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    Missing docstring.

    Typo: "recurse"

  16. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     

    nit: blank line between these.

  17. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     

    nit: add a trailing comma

  18. We don't need to specify empty initialize methods

  19. nit: remove this line.

  20. nit: remove this line.

  21. Leftover debug logging?

  22. 
      
PE
Review request changed

Commits:

Summary
-
Add testing and fix some small issues
-
Implement rendering of archives with JSTree library
-
Move parsing of archives to backend
-
address suggestions
-
base framework for archive review ui
+
Add testing and fix some small issues
+
Implement rendering of archives with JSTree library
+
Move parsing of archives to backend
+
address suggestions
+
Address comments
+
base framework for archive review ui

Diff:

Revision 6 (+5907 -5187)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

amalik2
  1. 
      
  2. Since reviewboard already uses fontawesome which has its own loading spinners, it may be better to replace the occurrences of background:url("throbber.gif") in this CSS file with the fontawesome equivalent. That way, you wouldn't need to add in the throbber.gif file

    1. I don't want to diverge the local copy of the library from what is being produced.

  3. reviewboard/static/lib/js/jstree.min.js (Diff revision 6)
     
     

    I'm most likely wrong about this since I know very little about licensing but I think that even for permissive licenses like MIT, you need to put the entire license text where you use the code. You probably should verify this with the mentors

    1. From what I understand MIT licenses only require the license where the software is not where it is used.

      https://opensource.org/licenses/MIT

  4. diffAgainstArchiveMetadata should also be mentioned in the docstring

  5. Should this be mode? The subclasses of BaseArchiveView define mode instead of node.

  6. Did you define CSS classes anywhere for the different subclasses of BaseArchiveView? I can't find anything by doing a search for 'archive-diff-'

    1. As stated in the description and in the demo video alot of work will need to be done to actually bring a more unifed look to the archive diffing. For that reason I'm not including style changes at this time.

  7. I'm curious as to how this works... does jstree modify the String prototype to add a jstree method to it?

  8. Single quotes should be used

  9. Where/how is this used? I don't see any usages in this file

  10. Where/how is this used? I don't see any usages in this file

  11. Where/how is this used? I don't see any usages in this file

  12. What is this used for?

  13. Is there a revision selector slider that gets rendered somewhere? (the text/image based review UIs have it)

    1. I believe that is handled by the file attachment class. The mentions to a slider in a the image reviewable are for the onion and diffing sliders that overlap images.

    2. https://github.com/reviewboard/reviewboard/blob/master/reviewboard/static/rb/js/views/imageReviewableView.es6.js#L990

      I had to explicitly include this in my code also to get it to show up. You can test this by creating a file attachment and updating it. When viewing details about the attachment, it should show the revision selector in the review header

  14. Not a huge deal but in general, using Boolean(value) is easier to understand than !!value

    1. This the same as the image reviewable view.

  15. Wouldn't this crash if there is a diff type mismatch? this._archiveView looks like it would be undefined in that situation.

  16. reviewboard/staticbundles.py (Diff revision 6)
     
     

    To be more consistent with the other CSS files, it would be better to rename the css file to something like jstree.min.css, and then move it outside of the jstree folder (and remove the jstree folder altogether)

    1. This is consistent with what is done in the JS folder. Look at the flot folder in static/lib/js.

  17. 
      
PE
Review request changed

Commits:

Summary
-
Add testing and fix some small issues
-
Implement rendering of archives with JSTree library
-
Move parsing of archives to backend
-
address suggestions
-
Address comments
-
base framework for archive review ui
+
Add testing and fix some small issues
+
Implement rendering of archives with JSTree library
+
Address comments
+
Move parsing of archives to backend
+
address suggestions
+
Address comments
+
base framework for archive review ui

Diff:

Revision 7 (+5903 -5199)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Loading...