[WIP] Support exploring archive files attached to reviews.
Review Request #11402 — Created Jan. 24, 2021 and updated
Information | |
---|---|
mfliu | |
Review Board | |
master | |
Reviewers | |
reviewboard, students | |
ReviewBoard currently supports attaching individual files for review, but while some filetypes can be opened in-browser and examined, archive files can only be downloaded. This commit adds a new review UI that allows archives to be browsed in ReviewBoard without downloading the file. Users can navigate the archived files, see metadata, and download individual files.
Unit tests for
get_js_model_data
using empty archives, simple archives with files, and archives with nested directories.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Michael, your project seems so interesting for sure! The minor and general slips that I've seen are for the import … |
CL clarissaudrey | |
Have you given some thought to how wide the indent would be for each directory level contained within the zip? … |
|
|
It seems like there is a lot of room for the table to grow within the File tab. Some questions … |
|
|
I think this is a good idea. I think a seperate download button will make things a bit clearer for … |
|
|
It should match any other tables that exist in review board. An example would be the dashboard that displays all … |
|
|
Maybe a drop down/popup that appears when you hover over the file name? |
|
|
This model will have all of the data that the view will display. The templates will json-encode the return value … |
|
|
F401 'tarfile.TarFile' imported but unused |
![]() |
|
Should be using single quotes |
|
|
These should all be single quotes as well |
|
|
single quotes in the print statement for strings |
|
|
Let's make sure that everything after base here stays in alphabetical order. |
|
|
This should inherit from object |
|
|
This should inherit from object |
|
|
I'm sure it's already on your radar, but this is kind of backwards. We should check explicitly for application/x-tar when … |
|
|
We can probably leave this out for now. Unless you're planning on using backbone routing to add a bunch of … |
|
|
F821 undefined name 'FileNotFoundError' |
![]() |
|
What were you thinking you might use for this extraction? Are you considering a third party service or are you … |
|
|
For my own understanding, why are mime types guessed here? I've never heard of that before. |
|
|
Why is "wah" added to the end of this? |
|
|
There is some white space on lines 7,8 and 12 that needs to be removed. |
|
|
I think this whitespace needs to be removed |
|
-
-
reviewboard/static/rb/js/models/archiveReviewableModel.es6.js (Diff revision 1) This model will have all of the data that the view will display. The templates will json-encode the return value from your
ArchiveReviewUI.get_js_model_data()
method and pass that into the constructor for your model here (which is how data gets from the Python on the server into JS on the client). In this case you'd probably have some kind of tree data representing the archive contents.The view then takes the data stored in the model and displays it.
Change Summary:
Update the Zip archive parsing to use intermediary class hierarchy instead of directly parsing to a Python dictionary
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+488) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
-
reviewboard/reviews/ui/archive.py (Diff revision 2) single quotes in the print statement for strings
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+570) |
Checks run (2 succeeded)
Change Summary:
Add tar file support and switch to using Underscore template for very basic rendering of hierarchy
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+694) |
Checks run (2 succeeded)
-
-
reviewboard/reviews/ui/__init__.py (Diff revision 4) Let's make sure that everything after
base
here stays in alphabetical order. -
-
-
reviewboard/reviews/ui/archive.py (Diff revision 4) I'm sure it's already on your radar, but this is kind of backwards. We should check explicitly for
application/x-tar
when instantiating theTarArchiveFormat
, and theelse
clause should raise.Inn fact, we might want to have a dict mapping mimetype to the format class so it can just be something like:
try: archiveFormat = self.formats[self.obj.mimetype] archive = archiveFormat(self.obj.filename, file_bytes) data['archive_hierarchy'] = archive.to_dict() except KeyError: raise NotImplementedError
-
reviewboard/static/rb/js/views/archiveReviewableView.es6.js (Diff revision 4) We can probably leave this out for now. Unless you're planning on using backbone routing to add a bunch of separate URLs that are all served by the same view, we won't need this.
Change Summary:
Rebase to tip of master
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+694) |
Checks run (2 succeeded)
Change Summary:
Address review comments
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+670) |
Checks run (2 succeeded)
Change Summary:
Added more sophisticated rendering system, actually displaying the archive hierarchy as a tree using a self-recursive underscore template
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+708) |
Checks run (2 succeeded)
-
-
Have you given some thought to how wide the indent would be for each directory level contained within the zip? You may have to make the default width of the Name column match the string length of the deepest file with the added indents (i.e. if there are 3 levels of directories then the minimum width of the name column should be the width of the longest filename is this directory level with three indents). If you want the Name column width to be fixed for all zips, tarballs or other archive files then you may need to speak with the mentors to figure out the average number of directory levels in zip files to calculate the average number of indents that will be required).
If you are interested in considering an alternate way of displaying the different directory nesting levels you might want to consider subsituting the indent with the relative path for the file as shown below.
test.zip
file.text
directory_name
directory_name/other_file.text -
It seems like there is a lot of room for the table to grow within the File tab. Some questions you may want to think about:
-Will the name column have a max width? If yes how will that max width be calculated relative to the window size and file tab size.
-How will you display file names (with or without indents) that exceed the name column width? Will you use a scroll bar?
-If you use a scroll bar where will that be place to avoid text being covered (either column name or file names)? Do the scroll bars used in the rest of Review Board work with your design? -
I think this is a good idea. I think a seperate download button will make things a bit clearer for users that haven't used this feature before. You'll just need to take into consideration where this download button should be placed and how easily it can be found at different screen widths.
-
It should match any other tables that exist in review board. An example would be the dashboard that displays all review requests but before you start implementing this it might be worth asking one of the mentors if there is another table that would be a better reference for you to use.
-
Change Summary:
Add support for downloading individual files with in an archive
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+800) |
Checks run (1 failed, 1 succeeded)
flake8
Change Summary:
Light refactoring and adding docstrings
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+862) |
Checks run (2 succeeded)
-
Your work looks really interesting. I have some questions that are just for my own knowledge and also to get some clarifications on a few things. I also found a few white spaces that need to be removed. I'm not sure if you're there yet but you may want to update your mockup just so that when you want to start implementing you have a clear design that you can work from. It might save you time instead of having to design and implement at the same time. Keep up the great work!
-
reviewboard/reviews/ui/archive.py (Diff revision 9) What were you thinking you might use for this extraction? Are you considering a third party service or are you going to code it yourself?
-
reviewboard/reviews/ui/archive.py (Diff revision 9) For my own understanding, why are mime types guessed here? I've never heard of that before.
-
-
reviewboard/static/rb/js/models/archiveReviewableModel.es6.js (Diff revision 9) There is some white space on lines 7,8 and 12 that needs to be removed.
-
reviewboard/static/rb/js/views/archiveReviewableView.es6.js (Diff revision 9) I think this whitespace needs to be removed
Change Summary:
Add support for RAR files and clean up some comments in JS files.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+924) |
Checks run (2 succeeded)
-
-
Michael, your project seems so interesting for sure! The minor and general slips that I've seen are for the import statements, where you need to sort it alphabetically, e.g
```from reviewboard.reviews.ui.archive import ArchiveReviewUI
should come before the ```from reviewboard.reviews.ui.base import register_ui
or the
from xxx import C,A,B
should befrom xxx import A,B,C
.
Change Summary:
Add support for commenting on individual archive files by clicking on their name.
Re-order some includes.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+1404) |
Checks run (2 succeeded)
Change Summary:
Add tests for
get_js_model_data
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+2474) |