feat(playlist): use own controller for playlist songs (#1367)

app/Facades/YouTube.php

@@ -4,6 +4,9 @@ namespace App\Facades;
 
 use Illuminate\Support\Facades\Facade;
 
+/**
+ * @method static bool enabled()
+ */
 class YouTube extends Facade
 {
     protected static function getFacadeAccessor(): string

app/Http/Controllers/API/PlaylistController.php

@@ -3,20 +3,17 @@
 namespace App\Http\Controllers\API;
 
 use App\Http\Requests\API\PlaylistStoreRequest;
-use App\Http\Requests\API\PlaylistSyncRequest;
 use App\Http\Requests\API\PlaylistUpdateRequest;
 use App\Models\Playlist;
 use App\Models\User;
 use App\Repositories\PlaylistRepository;
 use App\Services\PlaylistService;
-use App\Services\SmartPlaylistService;
 use Illuminate\Contracts\Auth\Authenticatable;
 
 class PlaylistController extends Controller
 {
     private PlaylistRepository $playlistRepository;
     private PlaylistService $playlistService;
-    private SmartPlaylistService $smartPlaylistService;
 
     /** @var User */
     private ?Authenticatable $currentUser;
@@ -24,12 +21,10 @@ class PlaylistController extends Controller
     public function __construct(
         PlaylistRepository $playlistRepository,
         PlaylistService $playlistService,
-        SmartPlaylistService $smartPlaylistService,
         ?Authenticatable $currentUser
     ) {
         $this->playlistRepository = $playlistRepository;
         $this->playlistService = $playlistService;
-        $this->smartPlaylistService = $smartPlaylistService;
         $this->currentUser = $currentUser;
     }
 
@@ -61,28 +56,6 @@ class PlaylistController extends Controller
         return response()->json($playlist);
     }
 
-    public function sync(PlaylistSyncRequest $request, Playlist $playlist)
-    {
-        $this->authorize('owner', $playlist);
-
-        abort_if($playlist->is_smart, 403, 'A smart playlist\'s content cannot be updated manually.');
-
-        $playlist->songs()->sync((array) $request->songs);
-
-        return response()->json();
-    }
-
-    public function getSongs(Playlist $playlist)
-    {
-        $this->authorize('owner', $playlist);
-
-        return response()->json(
-            $playlist->is_smart
-                ? $this->smartPlaylistService->getSongs($playlist)->pluck('id')
-                : $playlist->songs->pluck('id')
-        );
-    }
-
     public function destroy(Playlist $playlist)
     {
         $this->authorize('owner', $playlist);

app/Http/Controllers/API/PlaylistSongController.php

@@ -0,0 +1,39 @@
+<?php
+
+namespace App\Http\Controllers\API;
+
+use App\Http\Requests\API\PlaylistSongUpdateRequest;
+use App\Models\Playlist;
+use App\Services\SmartPlaylistService;
+
+class PlaylistSongController extends Controller
+{
+    private SmartPlaylistService $smartPlaylistService;
+
+    public function __construct(SmartPlaylistService $smartPlaylistService)
+    {
+        $this->smartPlaylistService = $smartPlaylistService;
+    }
+
+    public function index(Playlist $playlist)
+    {
+        $this->authorize('owner', $playlist);
+
+        return response()->json(
+            $playlist->is_smart
+                ? $this->smartPlaylistService->getSongs($playlist)->pluck('id')
+                : $playlist->songs->pluck('id')
+        );
+    }
+
+    public function update(PlaylistSongUpdateRequest $request, Playlist $playlist)
+    {
+        $this->authorize('owner', $playlist);
+
+        abort_if($playlist->is_smart, 403, 'A smart playlist\'s content cannot be updated manually.');
+
+        $playlist->songs()->sync((array) $request->songs);
+
+        return response()->json();
+    }
+}

app/Http/Requests/API/PlaylistSongUpdateRequest.php

@@ -8,7 +8,7 @@ use Illuminate\Validation\Rule;
 /**
  * @property array<string> $songs
  */
-class PlaylistSyncRequest extends Request
+class PlaylistSongUpdateRequest extends Request
 {
     /** @return array<mixed> */
     public function rules(): array

routes/api.php

@@ -1,5 +1,6 @@
 <?php
 
+use App\Facades\YouTube;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Route;
 
@@ -45,9 +46,13 @@ Route::group(['namespace' => 'API'], static function (): void {
         ]);
 
         // Playlist routes
-        Route::resource('playlist', 'PlaylistController')->only(['index', 'store', 'update', 'destroy']);
-        Route::put('playlist/{playlist}/sync', 'PlaylistController@sync')->where(['playlist' => '\d+']);
-        Route::get('playlist/{playlist}/songs', 'PlaylistController@getSongs')->where(['playlist' => '\d+']);
+        Route::apiResource('playlist', 'PlaylistController');
+
+        Route::put('playlist/{playlist}/sync', 'PlaylistSongController@update');
+
+        Route::apiResource('playlist.songs', 'PlaylistSongController')->only('index', 'update');
+        Route::put('playlist/{playlist}/songs', 'PlaylistSongController@update');
+        Route::get('playlist/{playlist}/songs', 'PlaylistSongController@index');
 
         // User and user profile routes
         Route::resource('user', 'UserController', ['only' => ['store', 'update', 'destroy']]);

tests/Feature/PlaylistSongTest.php

@@ -0,0 +1,83 @@
+<?php
+
+namespace Tests\Feature;
+
+use App\Models\Playlist;
+use App\Models\Song;
+use App\Models\User;
+use Illuminate\Support\Collection;
+
+class PlaylistSongTest extends TestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        static::createSampleMediaSet();
+    }
+
+    public function testUpdatePlaylistSongs(): void
+    {
+        $this->doTestUpdatePlaylistSongs();
+    }
+
+    /** @deprecated  */
+    public function testSyncPlaylist(): void
+    {
+        $this->doTestUpdatePlaylistSongs(true);
+    }
+
+    private function doTestUpdatePlaylistSongs(bool $useDeprecatedRoute = false): void
+    {
+        /** @var User $user */
+        $user = User::factory()->create();
+
+        /** @var Playlist $playlist */
+        $playlist = Playlist::factory()->create([
+            'user_id' => $user->id,
+        ]);
+
+        /** @var array<Song>|Collection $songs */
+        $songs = Song::orderBy('id')->take(4)->get();
+        $playlist->songs()->attach($songs->pluck('id')->all());
+
+        /** @var Song $removedSong */
+        $removedSong = $songs->pop();
+
+        $path = $useDeprecatedRoute ? "api/playlist/$playlist->id/sync" : "api/playlist/$playlist->id/songs";
+
+        $this->putAsUser($path, [
+            'songs' => $songs->pluck('id')->all(),
+        ], $user);
+
+        // We should still see the first 3 songs, but not the removed one
+        foreach ($songs as $song) {
+            self::assertDatabaseHas('playlist_song', [
+                'playlist_id' => $playlist->id,
+                'song_id' => $song->id,
+            ]);
+        }
+
+        self::assertDatabaseMissing('playlist_song', [
+            'playlist_id' => $playlist->id,
+            'song_id' => $removedSong->id,
+        ]);
+    }
+
+    public function testGetPlaylistSongs(): void
+    {
+        /** @var User $user */
+        $user = User::factory()->create();
+
+        /** @var Playlist $playlist */
+        $playlist = Playlist::factory()->create([
+            'user_id' => $user->id,
+        ]);
+
+        $songs = Song::factory(2)->create();
+        $playlist->songs()->saveMany($songs);
+
+        $this->getAsUser("api/playlist/$playlist->id/songs", $user)
+            ->assertJson($songs->pluck('id')->all());
+    }
+}

tests/Feature/PlaylistTest.php

@@ -136,41 +136,6 @@ class PlaylistTest extends TestCase
         $response->assertStatus(403);
     }
 
-    public function testSyncPlaylist(): void
-    {
-        /** @var User $user */
-        $user = User::factory()->create();
-
-        /** @var Playlist $playlist */
-        $playlist = Playlist::factory()->create([
-            'user_id' => $user->id,
-        ]);
-
-        /** @var array<Song>|Collection $songs */
-        $songs = Song::orderBy('id')->take(4)->get();
-        $playlist->songs()->attach($songs->pluck('id')->all());
-
-        /** @var Song $removedSong */
-        $removedSong = $songs->pop();
-
-        $this->putAsUser("api/playlist/$playlist->id/sync", [
-            'songs' => $songs->pluck('id')->all(),
-        ], $user);
-
-        // We should still see the first 3 songs, but not the removed one
-        foreach ($songs as $song) {
-            self::assertDatabaseHas('playlist_song', [
-                'playlist_id' => $playlist->id,
-                'song_id' => $song->id,
-            ]);
-        }
-
-        self::assertDatabaseMissing('playlist_song', [
-            'playlist_id' => $playlist->id,
-            'song_id' => $removedSong->id,
-        ]);
-    }
-
     public function testDeletePlaylist(): void
     {
         /** @var User $user */
@@ -193,21 +158,4 @@ class PlaylistTest extends TestCase
         $this->deleteAsUser("api/playlist/$playlist->id")
             ->assertStatus(403);
     }
-
-    public function testGetPlaylist(): void
-    {
-        /** @var User $user */
-        $user = User::factory()->create();
-
-        /** @var Playlist $playlist */
-        $playlist = Playlist::factory()->create([
-            'user_id' => $user->id,
-        ]);
-
-        $songs = Song::factory(2)->create();
-        $playlist->songs()->saveMany($songs);
-
-        $this->getAsUser("api/playlist/$playlist->id/songs", $user)
-            ->assertJson($songs->pluck('id')->all());
-    }
 }