sciencehistory / chf-sufia

sufia-based hydra app
Other
9 stars 4 forks source link

Better filenames for downloaded audio files, both originals and derivatives. #1262

Closed eddierubeiz closed 5 years ago

eddierubeiz commented 5 years ago

This PR changes the way downloaded audio files are named. The existing set up was unusable for oral history derivatives, as our current recipe for derivative download names uses the first three words of the work title, which, for oral histories, are always "Oral History Interview".

Here's a summary of the changes: BEFORE Original files: <first three words of edited name of *fileset*>_<fileset id>.<original file extension> Derivatives: <first three words of *work* title>_<generic work id>_<fileset id>_<derivative label>.<derivative extension>

AFTER Originals and derivatives for non-audio files: as above. Originals for audio files: mark_h_0030_1_1.mp3 <edited name of fileset>.<original file extension> Derivatives for audio files: mark_h_0030_1_1.flac <edited name of fileset>.<derivative extension>

Note: audio download filenames in the playlist were buggy. This PR also fixes that.

jrochkind commented 5 years ago

Nice and concise now, great!

We talked about removing the special audio file handling from ImageServiceHelper entirely, because using ImageServiceHelper is no longer part of our public UI -- it uses the 'playlist' component instead.

The 'ordinary' file handling is shown for admins only, to get access to management functions for an audio file -- but does not need download options there.

I'm going to merge this PR, but then make a new PR that removes that func from ImageServiceHelper, to reduce the maintenance burden of unneeded code.

jrochkind commented 5 years ago

Okay, when I try this branch with some test data, I get an exception with my test data:

undefined method `mime_type' for #<CurationConcerns::GenericWorkShowPresenter:0x00007faa0e903a20>

original_extension = Mime::Type.lookup(item.mime_type)&.symbol&.to_s

app/controllers/downloads_controller_override.rb:63:in `download_filename'
app/helpers/image_service_helper.rb:175:in `download_options'

This has something to do with the fact that the thing passed in is a presenter representing a work, but the code is assuming it will be a FileSet.

Rather than really get to the bottom and debug it entirely, I will go ahead and remove the customization for audio files as discussed above -- but do it right in this branch/PR. We don't need to debug code we don't need to have at all.

jrochkind commented 5 years ago

OK, I'm done futzing with it. @eddierubeiz can you review my new work, and once both tests are green, feel free to merge!

jrochkind commented 5 years ago

Hmm, why is it failing tests!

jrochkind commented 5 years ago

OK, I guess my changes broke the ability for "ordinary thumb/file" blocks to include an audio player.

I think that's fine, I don't think that's a requirement now that we have the "playlist" up top. Although I didn't actually mean to break it.

But why fix code we don't need.