From 5441cfb5f36e41cae83248467033f8ab85d4724b Mon Sep 17 00:00:00 2001
From: Phan An <me@phanan.net>
Date: Tue, 29 Nov 2022 13:16:43 +0100
Subject: [PATCH] fix: update multiple songs duplicate values (#1607)

---
 app/Services/SongService.php  | 29 +++++++++++++++++++-------
 app/Values/SongUpdateData.php |  2 +-
 tests/Feature/SongTest.php    | 39 ++++++++++++++++++++++++++++++++---
 3 files changed, 59 insertions(+), 11 deletions(-)

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<array-key, Song> */
     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<array-key, Song> $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());