Updated search page to support drag&drop and client-side file hashing (sha1 only)
Updated static folder to contain js, css and lib categories
Updated service and backend to provide batch lookup for sha1 lists.
Updated relevant files for inclusion of CryptoJS library.
Details
Details
- Reviewers
olasd zack - Group Reviewers
Developers
Updated relevant tests to reflect changes.
Tests requiring POST data currently do not pass, to be continued.
Diff Detail
Diff Detail
- Repository
- rDWAPPS Web applications
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 31 Build 36: Software Heritage Python tests Build 35: arc lint + arc unit
Event Timeline
Comment Actions
- Updated search page to: * support drag&dropping files to search them. * calculate hash client-side
This comment was removed by jbertran.
Comment Actions
The overall diff looks good, thanks for a great first feature. My comments above are mostly nits, but please go through them and address them accordingly.
On the other hand, the git commit history doesn't look good :) We really would like to have separate logic commits, while here we have only 3 that mix together unrelated stuff. Please rewrite your history to fix that.
Some other suggestions about the commits:
- please separate each commit in a short description (72 chars max), an empty line, and a longer description (if needed), pretty much as you would do in an email
- commits that add blurb, such as the jquery one, should be separate from everything else as otherwise they pollute the diff
- the commit fixing a phabricator bug should have something like Closes: Txx. That will automatically close the issue in phabricator, and also nicely cross-reference the task and the commit
Sorry for being a pain :-), next iteration will be much more swifter!
(Note: I haven't yet actually tested the code, but will do so.)
| debian/control | ||
|---|---|---|
| 13 ↗ | (On Diff #10) | white space issue here, libjs-cryptojs is not aligned |
| resources/test/webapp.ini | ||
| 3 ↗ | (On Diff #10) | please revert this change, our internal as-deployed URLs shouldn't be used in tests |
| swh/web/ui/service.py | ||
| 13 ↗ | (On Diff #10) | We should look for all hashes at once, rather than one-by-one, as we discussed. |
| swh/web/ui/static/css/style.css | ||
| 28 ↗ | (On Diff #10) | remove this line, maybe? |
| 31 ↗ | (On Diff #10) | nitpick: please expand "nfound" to "not-found", it's annoying to have some parts of names fully expanded and some not :) |
| swh/web/ui/static/js/filedrop.js | ||
| 5 ↗ | (On Diff #10) | lots of whitespace issues in this snippet, please "M-x untabify" it |
| 135 ↗ | (On Diff #10) | Please add a comment about why this conversion is needed and what it does. Also: this is here because, in the end, the same conversion function is not actually part of cryptoJs, right? |
| swh/web/ui/templates/layout.html | ||
| 6 ↗ | (On Diff #10) | I'm curious: why is this change needed? |
| swh/web/ui/templates/upload_and_search.html | ||
| 32–33 ↗ | (On Diff #10) | as we are not sure whether we are going to have a form to insert checksums by hand, I suggest rephrasing this as something like "Filesizes over 20Mb can be slow to process, so use with care". |
| 63 ↗ | (On Diff #10) | s/hash/checksum/ |
| swh/web/ui/views/api.py | ||
| 7 | this looks like an extra line that should go away | |
| swh/web/ui/views/browse.py | ||
| 38–39 ↗ | (On Diff #10) | please submit this as a task on the forge too |