[WIP] Support exploring archive files attached to reviews.
Review Request #11402 — Created Jan. 24, 2021 and updated
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 | ID |
---|---|
218c102697622a239a83753cfb5ecf7b23f46f3a |
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? … |
mderose | |
It seems like there is a lot of room for the table to grow within the File tab. Some questions … |
mderose | |
I think this is a good idea. I think a seperate download button will make things a bit clearer for … |
mderose | |
It should match any other tables that exist in review board. An example would be the dashboard that displays all … |
mderose | |
Maybe a drop down/popup that appears when you hover over the file name? |
mderose | |
This model will have all of the data that the view will display. The templates will json-encode the return value … |
david | |
F401 'tarfile.TarFile' imported but unused |
reviewbot | |
Should be using single quotes |
ryankang | |
These should all be single quotes as well |
ryankang | |
single quotes in the print statement for strings |
ryankang | |
Let's make sure that everything after base here stays in alphabetical order. |
david | |
This should inherit from object |
david | |
This should inherit from object |
david | |
I'm sure it's already on your radar, but this is kind of backwards. We should check explicitly for application/x-tar when … |
david | |
We can probably leave this out for now. Unless you're planning on using backbone routing to add a bunch of … |
david | |
F821 undefined name 'FileNotFoundError' |
reviewbot | |
What were you thinking you might use for this extraction? Are you considering a third party service or are you … |
mderose | |
For my own understanding, why are mime types guessed here? I've never heard of that before. |
mderose | |
Why is "wah" added to the end of this? |
mderose | |
There is some white space on lines 7,8 and 12 that needs to be removed. |
mderose | |
I think this whitespace needs to be removed |
mderose |
-
-
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:
-
Summary ID a18125ebf173ca71aa579efb3ba8dac045cbdc4c 31e4236aa0eee6cc34eac6cadad07adfacc631d0
- Commits:
-
Summary ID 31e4236aa0eee6cc34eac6cadad07adfacc631d0 1b351f7f13a8fe3bbd3eaaf488a438b17b72cec6 - 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:
-
Summary ID 1b351f7f13a8fe3bbd3eaaf488a438b17b72cec6 427c531385165c71d0dcb6d80eabf4abc8caf3ff - Diff:
-
Revision 4 (+694)
Checks run (2 succeeded)
-
-
-
-
-
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
-
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:
-
Summary ID 427c531385165c71d0dcb6d80eabf4abc8caf3ff 0b08fbf59ac3bb8e7897340812221f11d75eec79 - Diff:
-
Revision 5 (+694)
Checks run (2 succeeded)
- Change Summary:
-
Address review comments
- Commits:
-
Summary ID 0b08fbf59ac3bb8e7897340812221f11d75eec79 05e1ad84b123205ada3494ecf39680d86fb714dd - 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:
-
Summary ID 05e1ad84b123205ada3494ecf39680d86fb714dd bbcc492a3ee3495c52cf11076a9847c1e7bfa5b2 - 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:
-
Summary ID bbcc492a3ee3495c52cf11076a9847c1e7bfa5b2 ed9822e43ec0a2a0fe4a6112a677b3b397bc2700 - Diff:
-
Revision 8 (+800)
- Change Summary:
-
Light refactoring and adding docstrings
- Commits:
-
Summary ID ed9822e43ec0a2a0fe4a6112a677b3b397bc2700 2c3953f15ee3903e11506dc8e28bd0cf5cb01db0 - 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!
-
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?
-
-
-
-
- Change Summary:
-
Add support for RAR files and clean up some comments in JS files.
- Commits:
-
Summary ID 2c3953f15ee3903e11506dc8e28bd0cf5cb01db0 b69cdede51e9ec020af20513d1bb387f18ae8f9d - 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:
-
Summary ID b69cdede51e9ec020af20513d1bb387f18ae8f9d 12e0318de552207996bb1ed5b90812d461aba6ec - Diff:
-
Revision 11 (+1404)
Checks run (2 succeeded)
- Change Summary:
-
Add tests for
get_js_model_data
- Commits:
-
Summary ID 12e0318de552207996bb1ed5b90812d461aba6ec 218c102697622a239a83753cfb5ecf7b23f46f3a - Diff:
-
Revision 12 (+2474)