From 54d2029d476ed9611974286c791a2f6895b61db4 Mon Sep 17 00:00:00 2001 From: Phan An Date: Mon, 11 Oct 2021 13:30:27 +0200 Subject: [PATCH] feat(playlist): use own controller for playlist songs (#1367) --- app/Facades/YouTube.php | 3 + .../Controllers/API/PlaylistController.php | 27 ------ .../API/PlaylistSongController.php | 39 +++++++++ ...uest.php => PlaylistSongUpdateRequest.php} | 2 +- routes/api.php | 11 ++- tests/Feature/PlaylistSongTest.php | 83 +++++++++++++++++++ tests/Feature/PlaylistTest.php | 52 ------------ 7 files changed, 134 insertions(+), 83 deletions(-) create mode 100644 app/Http/Controllers/API/PlaylistSongController.php rename app/Http/Requests/API/{PlaylistSyncRequest.php => PlaylistSongUpdateRequest.php} (87%) create mode 100644 tests/Feature/PlaylistSongTest.php diff --git a/app/Facades/YouTube.php b/app/Facades/YouTube.php index 4c7658e2..0f2e26be 100644 --- a/app/Facades/YouTube.php +++ b/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 diff --git a/app/Http/Controllers/API/PlaylistController.php b/app/Http/Controllers/API/PlaylistController.php index 2f905a46..bbca28a4 100644 --- a/app/Http/Controllers/API/PlaylistController.php +++ b/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); diff --git a/app/Http/Controllers/API/PlaylistSongController.php b/app/Http/Controllers/API/PlaylistSongController.php new file mode 100644 index 00000000..1542dce9 --- /dev/null +++ b/app/Http/Controllers/API/PlaylistSongController.php @@ -0,0 +1,39 @@ +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(); + } +} diff --git a/app/Http/Requests/API/PlaylistSyncRequest.php b/app/Http/Requests/API/PlaylistSongUpdateRequest.php similarity index 87% rename from app/Http/Requests/API/PlaylistSyncRequest.php rename to app/Http/Requests/API/PlaylistSongUpdateRequest.php index d13b7be4..8ef4cca3 100644 --- a/app/Http/Requests/API/PlaylistSyncRequest.php +++ b/app/Http/Requests/API/PlaylistSongUpdateRequest.php @@ -8,7 +8,7 @@ use Illuminate\Validation\Rule; /** * @property array $songs */ -class PlaylistSyncRequest extends Request +class PlaylistSongUpdateRequest extends Request { /** @return array */ public function rules(): array diff --git a/routes/api.php b/routes/api.php index 6c60343d..6467a00a 100644 --- a/routes/api.php +++ b/routes/api.php @@ -1,5 +1,6 @@ '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']]); diff --git a/tests/Feature/PlaylistSongTest.php b/tests/Feature/PlaylistSongTest.php new file mode 100644 index 00000000..b5645b33 --- /dev/null +++ b/tests/Feature/PlaylistSongTest.php @@ -0,0 +1,83 @@ +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|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()); + } +} diff --git a/tests/Feature/PlaylistTest.php b/tests/Feature/PlaylistTest.php index 45c35de7..97d7eb6e 100644 --- a/tests/Feature/PlaylistTest.php +++ b/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|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()); - } }