diff --git a/app/Services/SongService.php b/app/Services/SongService.php index b6fc85d6..8d12ced5 100644 --- a/app/Services/SongService.php +++ b/app/Services/SongService.php @@ -23,13 +23,24 @@ class SongService /** @return Collection|array */ public function updateSongs(array $ids, SongUpdateData $data): Collection { + if (count($ids) === 1) { + // If we're only updating one song, an empty non-required should be converted to the default values. + // This allows the user to clear those fields. + $data->disc = $data->disc ?: 1; + $data->track = $data->track ?: 0; + $data->lyrics = $data->lyrics ?: ''; + $data->year = $data->year ?: null; + $data->genre = $data->genre ?: ''; + $data->albumArtistName = $data->albumArtistName ?: $data->artistName; + } + return DB::transaction(function () use ($ids, $data): Collection { return collect($ids)->reduce(function (Collection $updated, string $id) use ($data): Collection { /** @var Song|null $song */ $song = Song::with('album', 'album.artist', 'artist')->find($id); if ($song) { - $updated->push($this->updateSong($song, $data)); + $updated->push($this->updateSong($song, clone $data)); } return $updated; @@ -39,15 +50,19 @@ class SongService private function updateSong(Song $song, SongUpdateData $data): Song { + // for non-nullable fields, if the provided data is empty, use the existing value $data->albumName = $data->albumName ?: $song->album->name; $data->artistName = $data->artistName ?: $song->artist->name; - $data->albumArtistName = $data->albumArtistName ?: $song->album_artist->name; $data->title = $data->title ?: $song->title; - $data->lyrics = $data->lyrics ?: $song->lyrics; - $data->track = $data->track ?: $song->track; - $data->disc = $data->disc ?: $song->disc; - $data->genre = $data->genre ?: $song->genre; - $data->year = $data->year ?: $song->year; + + // For nullable fields, use the existing value only if the provided data is explicitly null. + // This allows us to clear those fields. + $data->albumArtistName ??= $song->album_artist->name; + $data->lyrics ??= $song->lyrics; + $data->track ??= $song->track; + $data->disc ??= $song->disc; + $data->genre ??= $song->genre; + $data->year ??= $song->year; $albumArtist = Artist::getOrCreate($data->albumArtistName); $artist = Artist::getOrCreate($data->artistName); diff --git a/app/Values/SongUpdateData.php b/app/Values/SongUpdateData.php index f6e1785a..d5495271 100644 --- a/app/Values/SongUpdateData.php +++ b/app/Values/SongUpdateData.php @@ -31,7 +31,7 @@ final class SongUpdateData implements Arrayable track: (int) $request->input('data.track'), disc: (int) $request->input('data.disc'), genre: $request->input('data.genre'), - year: (int) $request->input('data.year') ?: null, + year: (int) $request->input('data.year'), lyrics: $request->input('data.lyrics'), ); } diff --git a/tests/Feature/SongTest.php b/tests/Feature/SongTest.php index 208552cc..5a5d3d3b 100644 --- a/tests/Feature/SongTest.php +++ b/tests/Feature/SongTest.php @@ -93,15 +93,16 @@ class SongTest extends TestCase $this->putAs('/api/songs', [ 'songs' => $songIds, 'data' => [ - 'title' => 'foo', + 'title' => null, 'artist_name' => 'John Cena', 'album_name' => 'One by One', - 'lyrics' => 'bar', + 'lyrics' => null, 'track' => 9999, ], ], $user) ->assertOk(); + /** @var Collection|array $songs */ $songs = Song::query()->whereIn('id', $songIds)->get(); // All of these songs must now belong to a new album and artist set @@ -112,6 +113,13 @@ class SongTest extends TestCase self::assertSame('John Cena', $songs[0]->artist->name); self::assertSame($songs[0]->artist_id, $songs[1]->artist_id); self::assertSame($songs[0]->artist_id, $songs[2]->artist_id); + + self::assertNotSame($songs[0]->title, $songs[1]->title); + self::assertNotSame($songs[0]->lyrics, $songs[1]->lyrics); + + self::assertSame(9999, $songs[0]->track); + self::assertSame(9999, $songs[1]->track); + self::assertSame(9999, $songs[2]->track); } public function testMultipleUpdateCreatingNewAlbumsAndArtists(): void @@ -160,7 +168,6 @@ class SongTest extends TestCase /** @var Song $song */ $song = Song::query()->first(); - $this->putAs('/api/songs', [ 'songs' => [$song->id], 'data' => [ @@ -196,6 +203,32 @@ class SongTest extends TestCase self::assertTrue($album->artist->is($albumArtist)); } + public function testUpdateSingleSongWithEmptyTrackAndDisc(): void + { + /** @var User $user */ + $user = User::factory()->admin()->create(); + + /** @var Song $song */ + $song = Song::factory()->create([ + 'track' => 12, + 'disc' => 2, + ]); + + $this->putAs('/api/songs', [ + 'songs' => [$song->id], + 'data' => [ + 'track' => null, + 'disc' => null, + ], + ], $user) + ->assertOk(); + + $song->refresh(); + + self::assertSame(0, $song->track); + self::assertSame(1, $song->disc); + } + public function testDeletingByChunk(): void { self::assertNotSame(0, Song::query()->count());