From ace3184ed31b3667dc6c1bffd3062acd5158348c Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 16 Jan 2024 19:45:42 +0000 Subject: [PATCH] Published albums must have at least one track Publishing an album with no tracks doesn't make much sense and causes exceptions like this one [1], because the album has no preview to point the audio element at. [1]: https://app.rollbar.com/a/gofreerange/fix/item/jam/49 Fixes #148. --- app/controllers/albums_controller.rb | 7 +++++-- app/models/album.rb | 16 ++++++++++----- test/controllers/albums_controller_test.rb | 24 ++++++++++++++++++++-- test/factories/album.rb | 2 +- test/models/album_test.rb | 15 ++++++++++++-- test/system/publishing_an_album_test.rb | 2 +- 6 files changed, 53 insertions(+), 13 deletions(-) diff --git a/app/controllers/albums_controller.rb b/app/controllers/albums_controller.rb index d913e671..2024b319 100644 --- a/app/controllers/albums_controller.rb +++ b/app/controllers/albums_controller.rb @@ -27,8 +27,11 @@ def update end def publish - @album.publish - AlbumMailer.with(album: @album).published.deliver_later + if @album.publish + AlbumMailer.with(album: @album).published.deliver_later + else + flash[:alert] = "Errors prohibited this album from being saved: #{@album.errors.full_messages.to_sentence}" + end redirect_to artist_album_url(@album.artist, @album) end diff --git a/app/models/album.rb b/app/models/album.rb index f3546533..c2d4bc9e 100644 --- a/app/models/album.rb +++ b/app/models/album.rb @@ -16,6 +16,7 @@ class Album < ApplicationRecord validates :title, presence: true validates :price, presence: true, numericality: true validates :released_on, comparison: { less_than_or_equal_to: Time.zone.today, allow_blank: true } + validates :number_of_tracks, comparison: { greater_than: 0 }, if: :published? validates( :cover, attached: { message: 'file cannot be missing' }, @@ -45,11 +46,12 @@ def pending end def publish - published! - update!(first_published_on: Time.current) if first_published_on.blank? - - ZipDownloadJob.perform_later(self, format: :mp3v0) - ZipDownloadJob.perform_later(self, format: :flac) + saved = update(publication_status: :published, first_published_on: first_published_on || Time.current) + if saved + ZipDownloadJob.perform_later(self, format: :mp3v0) + ZipDownloadJob.perform_later(self, format: :flac) + end + saved end def unpublish @@ -60,6 +62,10 @@ def released_on super || first_published_on end + def number_of_tracks + tracks.length + end + def metadata_or_cover_changed? title_previously_changed? || attachment_changes['cover'].present? end diff --git a/test/controllers/albums_controller_test.rb b/test/controllers/albums_controller_test.rb index e2bb082a..1952f372 100644 --- a/test/controllers/albums_controller_test.rb +++ b/test/controllers/albums_controller_test.rb @@ -4,7 +4,7 @@ class AlbumsControllerTestSignedInAsAdmin < ActionDispatch::IntegrationTest def setup - @album = create(:album) + @album = create(:album, :with_tracks) @user = create(:user, admin: true) log_in_as(@user) end @@ -118,6 +118,26 @@ def setup end end + test '#publish does not succeed if album has validation errors' do + @album.tracks.destroy_all + patch publish_artist_album_url(@album.artist, @album) + assert_not @album.reload.published? + end + + test '#publish redirects back to album page with error message if album has validation errors' do + @album.tracks.destroy_all + patch publish_artist_album_url(@album.artist, @album) + assert_redirected_to artist_album_url(@album.artist, @album) + assert_equal 'Errors prohibited this album from being saved: Number of tracks must be greater than 0', flash[:alert] + end + + test '#publish does not send email to artist if album has validation errors' do + @album.tracks.destroy_all + assert_enqueued_emails 0 do + patch publish_artist_album_url(@album.artist, @album) + end + end + test '#unpublish' do patch unpublish_artist_album_url(@album.artist, @album) assert_redirected_to artist_album_url(@album.artist, @album) @@ -128,7 +148,7 @@ def setup class AlbumsControllerTestSignedInAsArtist < ActionDispatch::IntegrationTest setup do - @album = create(:album) + @album = create(:published_album) user = create(:user) user.artists << @album.artist log_in_as(user) diff --git a/test/factories/album.rb b/test/factories/album.rb index bbaee385..24b879ae 100644 --- a/test/factories/album.rb +++ b/test/factories/album.rb @@ -44,7 +44,7 @@ publication_status { :pending } end - factory :published_album, parent: :album do + factory :published_album, parent: :album, traits: %i[with_tracks] do publication_status { :published } end end diff --git a/test/models/album_test.rb b/test/models/album_test.rb index bb9d9ad9..c9930a3d 100644 --- a/test/models/album_test.rb +++ b/test/models/album_test.rb @@ -95,7 +95,7 @@ class AlbumTest < ActiveSupport::TestCase end test 'publish enqueues ZipDownloadJob to prepare mp3v0 download' do - album = create(:unpublished_album) + album = create(:unpublished_album, :with_tracks) args_matcher = ->(job_args) { job_args[1][:format] == :mp3v0 } assert_enqueued_with(job: ZipDownloadJob, args: args_matcher) do @@ -104,7 +104,7 @@ class AlbumTest < ActiveSupport::TestCase end test 'publish enqueues ZipDownloadJob to prepare flac download' do - album = create(:unpublished_album) + album = create(:unpublished_album, :with_tracks) args_matcher = ->(job_args) { job_args[1][:format] == :flac } assert_enqueued_with(job: ZipDownloadJob, args: args_matcher) do @@ -223,4 +223,15 @@ class AlbumTest < ActiveSupport::TestCase assert_includes album.errors[:released_on], "must be less than or equal to #{Time.zone.today}" end end + + test 'is valid if not published and has no tracks' do + album = build(:unpublished_album, tracks: []) + assert album.valid? + end + + test 'is not valid if published and has no tracks' do + album = build(:album, tracks: [], publication_status: :published) + assert_not album.valid? + assert_includes album.errors[:number_of_tracks], 'must be greater than 0' + end end diff --git a/test/system/publishing_an_album_test.rb b/test/system/publishing_an_album_test.rb index d7929f7d..aa296f55 100644 --- a/test/system/publishing_an_album_test.rb +++ b/test/system/publishing_an_album_test.rb @@ -4,7 +4,7 @@ class PublishingAnAlbumTest < ApplicationSystemTestCase setup do - @album = create(:unpublished_album) + @album = create(:unpublished_album, :with_tracks) user = create(:user) user.artists << @album.artist log_in_as(user)