Skip to content

Commit

Permalink
Fix bug in ZipDownloadJob when titles contains /
Browse files Browse the repository at this point in the history
A newly added album in production[1] caused the ZipDownloadJob to fail
because the album title contained a `/` which later code considered to
be part of the file path.

This commit fixes that issue and a similar one in track titles by
sanitizing the generated filenames using the Zaru gem.

[1] https://jam.coop/artists/kristoffer-lislegaard/albums/floating-body-diving-mind
[2] https://github.com/madrobby/zaru
  • Loading branch information
chrislo committed Nov 14, 2023
1 parent 41dac25 commit 734fc67
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 2 deletions.
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ gem 'stripe', '~> 10.0'
gem 'tailwindcss-rails'
gem 'turbo-rails'
gem 'tzinfo-data', platforms: %i[mingw mswin x64_mingw jruby]
gem 'zaru', '~> 1.0'

group :development, :test do
gem 'debug', platforms: %i[mri mingw x64_mingw]
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ GEM
websocket-extensions (0.1.5)
xpath (3.2.0)
nokogiri (~> 1.8)
zaru (1.0.0)
zeitwerk (2.6.12)

PLATFORMS
Expand Down Expand Up @@ -392,6 +393,7 @@ DEPENDENCIES
tailwindcss-rails
turbo-rails
tzinfo-data
zaru (~> 1.0)

RUBY VERSION
ruby 3.2.2p53
Expand Down
4 changes: 2 additions & 2 deletions app/jobs/zip_download_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def perform(album, format: :mp3v0)
Dir.mktmpdir do |dir|
filenames = download_all_tracks(album, format, dir)

zipfile_name = "#{album.artist.name} - #{album.title}.zip"
zipfile_name = Zaru.sanitize!("#{album.artist.name} - #{album.title}.zip")

Zip::File.open(File.join(dir, zipfile_name), create: true) do |zipfile|
filenames.each do |filename|
Expand Down Expand Up @@ -42,7 +42,7 @@ def download_all_tracks(album, format, dir)
end

def track_filename(track, format)
"#{track.position.to_s.rjust(2, '0')} - #{track.title}.#{extension(format)}"
Zaru.sanitize!("#{track.position.to_s.rjust(2, '0')} - #{track.title}.#{extension(format)}")
end

def extension(format)
Expand Down
21 changes: 21 additions & 0 deletions test/jobs/zip_download_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,25 @@ class ZipDownloadJobTest < ActiveJob::TestCase
ZipDownloadJob.perform_now(album)
assert_equal 1, album.reload.downloads.count
end

test 'creates a zip file when the album title contains a /' do
album = create(:album, title: 'Slash / Slash')
track = create(:track, album:, title: 'First / Track', position: 1)
create(:transcode, track:, format: :mp3v0)

ZipDownloadJob.perform_now(album)

assert_equal 1, album.downloads.count

entries = []
album.downloads.last.file.open do |zip_file|
Zip::File.open(zip_file) do |zip|
zip.each do |entry|
entries << entry.name
end
end
end

assert entries.include? '01 - First Track.mp3'
end
end

0 comments on commit 734fc67

Please sign in to comment.