Update downloads_controller to serve file view properly. #866
Conversation
…download_controller_view
…alib/jupiter into download_controller_view
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| @@ -1,12 +1,4 @@ | |||
| module ItemsHelper | |||
| def file_view_url(file) | |||
There was a problem hiding this comment.
We don't need all these custom helpers? Just use rails routes properly
| @@ -11,7 +11,7 @@ def perform(attachment_id) | |||
| item = attachment.record.owner | |||
|
|
|||
| path = ActiveStorage::Blob.service.send(:path_for, attachment.blob.key) | |||
| original_filename = attachment.blob.filename.to_s | |||
| original_filename = attachment.filename.to_s | |||
There was a problem hiding this comment.
Don't need to walk the chain of the blob object to get filename, can get it right from the activestorage object
| file_set_id: @item.files.first.fileset_uuid, | ||
| file_name: @item.files.first.filename, | ||
| only_path: true |
There was a problem hiding this comment.
This was a bug here. We need the absolute url not just the path (relative url)!
From here: https://scholar.google.ca/intl/en/scholar/inclusion.html#indexing
The content of the tag is the absolute URL of the PDF file; for security reasons, it must refer to a file in the same subdirectory as the HTML abstract.
|
Need to do some manual testing to make sure theirs no regressions with the file download changes I have made. But so far looks good. Added comments for my big changes ( of course theres still a few lingering urls that should be moved over to the new download routes i created but can maybe tackle this later) EDIT: Manual testing seems to be fine, should be all good to go on my end |
|
Assume this is no longer WIP. I am looking into the manual testing right now. |
|
Validated all the file download/view links, filename saved in fedora etc, all looks good to me. Now sure if there's an easy way for me to manually test the redirect urls. |
|
Yeah had hard time testing the redirects as well since they expect some weird formats and kept failing on validation errors ( So not sure best way to verify this either /shrug |
| @@ -0,0 +1,44 @@ | |||
| require 'test_helper' | |||
|
|
|||
| class DownloadsControllerTest < ActionDispatch::IntegrationTest | |||
There was a problem hiding this comment.
New tests for testing the downloads controller, view and download actions. Making sure the file being viewed/downloaded is being streamed back with the proper headers and content
weiweishi
changed the title
Update the downloads_controller to:
Updated how urls and filename are handled with regular Rails/ActiveStorage way. And added more tests for the download controllers.