diff --git a/app/Facades/Download.php b/app/Facades/Download.php index 9c98d8ac..bdc4898a 100644 --- a/app/Facades/Download.php +++ b/app/Facades/Download.php @@ -6,7 +6,7 @@ use App\Models\Song; use Illuminate\Support\Facades\Facade; /** - * @method static string fromSong(Song $song) + * @method static string getLocalPath(Song $song) * @see \App\Services\DownloadService */ class Download extends Facade diff --git a/app/Http/Controllers/API/MakeSongsPublicController.php b/app/Http/Controllers/API/MakeSongsPublicController.php index 9c213ccb..7ddcb631 100644 --- a/app/Http/Controllers/API/MakeSongsPublicController.php +++ b/app/Http/Controllers/API/MakeSongsPublicController.php @@ -19,7 +19,7 @@ class MakeSongsPublicController extends Controller SongService $songService, Authenticatable $user ) { - $songs = Song::query()->find($request->songs); + $songs = Song::query()->findMany($request->songs); $songs->each(fn ($song) => $this->authorize('own', $song)); $songService->makeSongsPublic($songs); diff --git a/app/Http/Controllers/API/SongController.php b/app/Http/Controllers/API/SongController.php index 4e9ce8a2..b83aa2e5 100644 --- a/app/Http/Controllers/API/SongController.php +++ b/app/Http/Controllers/API/SongController.php @@ -47,13 +47,13 @@ class SongController extends Controller { $this->authorize('access', $song); - return SongResource::make($this->songRepository->getOne($song->id)); + return SongResource::make($this->songRepository->getOne($song->id, $this->user)); } public function update(SongUpdateRequest $request) { // Don't use SongRepository::findMany() because it'd be already catered to the current user. - Song::query()->find($request->songs)->each(fn (Song $song) => $this->authorize('edit', $song)); + Song::query()->findMany($request->songs)->each(fn (Song $song) => $this->authorize('edit', $song)); $updatedSongs = $this->songService->updateSongs($request->songs, SongUpdateData::fromRequest($request)); $albums = $this->albumRepository->getMany($updatedSongs->pluck('album_id')->toArray()); diff --git a/app/Http/Controllers/Download/DownloadAlbumController.php b/app/Http/Controllers/Download/DownloadAlbumController.php index af39d751..36ff8a15 100644 --- a/app/Http/Controllers/Download/DownloadAlbumController.php +++ b/app/Http/Controllers/Download/DownloadAlbumController.php @@ -12,8 +12,12 @@ use Illuminate\Contracts\Auth\Authenticatable; class DownloadAlbumController extends Controller { /** @param User $user */ - public function __invoke(Album $album, SongRepository $repository, DownloadService $download, Authenticatable $user) - { - return response()->download($download->from($repository->getByAlbum($album, $user))); + public function __invoke( + Album $album, + SongRepository $repository, + DownloadService $service, + Authenticatable $user + ) { + return response()->download($service->getDownloadablePath($repository->getByAlbum($album, $user))); } } diff --git a/app/Http/Controllers/Download/DownloadArtistController.php b/app/Http/Controllers/Download/DownloadArtistController.php index bbd7a02a..75668af9 100644 --- a/app/Http/Controllers/Download/DownloadArtistController.php +++ b/app/Http/Controllers/Download/DownloadArtistController.php @@ -15,9 +15,9 @@ class DownloadArtistController extends Controller public function __invoke( Artist $artist, SongRepository $repository, - DownloadService $download, + DownloadService $service, Authenticatable $user ) { - return response()->download($download->from($repository->getByArtist($artist, $user))); + return response()->download($service->getDownloadablePath($repository->getByArtist($artist, $user))); } } diff --git a/app/Http/Controllers/Download/DownloadFavoritesController.php b/app/Http/Controllers/Download/DownloadFavoritesController.php index 7423eef1..a4322bc1 100644 --- a/app/Http/Controllers/Download/DownloadFavoritesController.php +++ b/app/Http/Controllers/Download/DownloadFavoritesController.php @@ -13,6 +13,6 @@ class DownloadFavoritesController extends Controller /** @param User $user */ public function __invoke(DownloadService $download, SongRepository $repository, Authenticatable $user) { - return response()->download($download->from($repository->getFavorites($user))); + return response()->download($download->getDownloadablePath($repository->getFavorites($user))); } } diff --git a/app/Http/Controllers/Download/DownloadPlaylistController.php b/app/Http/Controllers/Download/DownloadPlaylistController.php index d1e95f38..2f80a8b5 100644 --- a/app/Http/Controllers/Download/DownloadPlaylistController.php +++ b/app/Http/Controllers/Download/DownloadPlaylistController.php @@ -23,7 +23,7 @@ class DownloadPlaylistController extends Controller $this->authorize('download', $playlist); return response()->download( - $download->from( + $download->getDownloadablePath( $playlist->is_smart ? $smartPlaylistService->getSongs($playlist, $user) : $repository->getByStandardPlaylist($playlist, $user) diff --git a/app/Http/Controllers/Download/DownloadSongsController.php b/app/Http/Controllers/Download/DownloadSongsController.php index 729e3fbe..72277ff4 100644 --- a/app/Http/Controllers/Download/DownloadSongsController.php +++ b/app/Http/Controllers/Download/DownloadSongsController.php @@ -4,16 +4,18 @@ namespace App\Http\Controllers\Download; use App\Http\Controllers\Controller; use App\Http\Requests\Download\DownloadSongsRequest; +use App\Models\Song; use App\Repositories\SongRepository; use App\Services\DownloadService; class DownloadSongsController extends Controller { - public function __invoke(DownloadSongsRequest $request, DownloadService $download, SongRepository $repository) + public function __invoke(DownloadSongsRequest $request, DownloadService $service, SongRepository $repository) { - $songs = $repository->getMany($request->songs); + // Don't use SongRepository::findMany() because it'd have been already catered to the current user. + $songs = Song::query()->findMany($request->songs); $songs->each(fn ($song) => $this->authorize('download', $song)); - return response()->download($download->from($repository->getMany($request->songs))); + return response()->download($service->getDownloadablePath($repository->getMany($request->songs))); } } diff --git a/app/Models/SongZipArchive.php b/app/Models/SongZipArchive.php index 8a3ee906..1cc0988f 100644 --- a/app/Models/SongZipArchive.php +++ b/app/Models/SongZipArchive.php @@ -40,7 +40,7 @@ class SongZipArchive public function addSong(Song $song): static { attempt(function () use ($song): void { - $path = Download::fromSong($song); + $path = Download::getLocalPath($song); $this->archive->addFile($path, $this->generateZipContentFileNameFromPath($path)); }); diff --git a/app/Providers/MacroProvider.php b/app/Providers/MacroProvider.php index e370baf6..902f11e5 100644 --- a/app/Providers/MacroProvider.php +++ b/app/Providers/MacroProvider.php @@ -16,7 +16,7 @@ class MacroProvider extends ServiceProvider { Collection::macro('orderByArray', function (array $orderBy, string $key = 'id'): Collection { /** @var Collection $this */ - return $this->sortBy(static fn ($item) => array_search($item->$key, $orderBy, true)); + return $this->sortBy(static fn ($item) => array_search($item->$key, $orderBy, true))->values(); }); Builder::macro('logSql', function (): Builder { diff --git a/app/Repositories/SongRepository.php b/app/Repositories/SongRepository.php index deb3ba89..be5cb8dc 100644 --- a/app/Repositories/SongRepository.php +++ b/app/Repositories/SongRepository.php @@ -213,6 +213,7 @@ class SongRepository extends Repository return $inThatOrder ? $songs->orderByArray($ids) : $songs; } + /** @param string $id */ public function getOne($id, ?User $scopedUser = null): Song { /** @var ?User $scopedUser */ diff --git a/app/Services/DownloadService.php b/app/Services/DownloadService.php index 59c4f63d..8d49f351 100644 --- a/app/Services/DownloadService.php +++ b/app/Services/DownloadService.php @@ -14,10 +14,10 @@ class DownloadService { } - public function from(Collection $songs): string + public function getDownloadablePath(Collection $songs): string { if ($songs->count() === 1) { - return $this->fromSong($songs->first()); + return $this->getLocalPath($songs->first()); } return (new SongZipArchive()) @@ -26,7 +26,7 @@ class DownloadService ->getPath(); } - public function fromSong(Song $song): string + public function getLocalPath(Song $song): string { if ($song->s3_params) { // The song is hosted on Amazon S3. diff --git a/database/factories/SongFactory.php b/database/factories/SongFactory.php index 9e0562eb..4272276b 100644 --- a/database/factories/SongFactory.php +++ b/database/factories/SongFactory.php @@ -14,12 +14,9 @@ class SongFactory extends Factory /** @return array */ public function definition(): array { - /** @var Album $album */ - $album = Album::factory()->create(); - return [ - 'album_id' => $album->id, - 'artist_id' => $album->artist->id, + 'album_id' => Album::factory(), + 'artist_id' => static fn (array $attributes) => Album::find($attributes['album_id'])->artist_id, 'title' => $this->faker->sentence, 'length' => $this->faker->randomFloat(2, 10, 500), 'track' => random_int(1, 20), diff --git a/tests/Feature/DownloadTest.php b/tests/Feature/DownloadTest.php index 926940ec..609e05d0 100644 --- a/tests/Feature/DownloadTest.php +++ b/tests/Feature/DownloadTest.php @@ -22,17 +22,16 @@ class DownloadTest extends TestCase { parent::setUp(); - static::createSampleMediaSet(); $this->downloadService = self::mock(DownloadService::class); } public function testNonLoggedInUserCannotDownload(): void { /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->never(); $this->get("download/songs?songs[]=$song->id") @@ -42,18 +41,18 @@ class DownloadTest extends TestCase public function testDownloadOneSong(): void { /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); /** @var User $user */ $user = User::factory()->create(); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->once() ->with(Mockery::on(static function (Collection $retrievedSongs) use ($song) { return $retrievedSongs->count() === 1 && $retrievedSongs->first()->id === $song->id; })) - ->andReturn($this->mediaPath . '/blank.mp3'); + ->andReturn(test_path('songs/blank.mp3')); $this->get("download/songs?songs[]={$song->id}&api_token=" . $user->createToken('Koel')->plainTextToken) ->assertOk(); @@ -65,19 +64,17 @@ class DownloadTest extends TestCase $user = User::factory()->create(); /** @var array|Collection $songs */ - $songs = Song::query()->take(2)->orderBy('id')->get(); + $songs = Song::factory(2)->create(); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->once() ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs): bool { - $retrievedIds = $retrievedSongs->pluck('id')->all(); - $requestedIds = $songs->pluck('id')->all(); - self::assertEqualsCanonicalizing($requestedIds, $retrievedIds); + self::assertEqualsCanonicalizing($retrievedSongs->pluck('id')->all(), $songs->pluck('id')->all()); return true; })) - ->andReturn($this->mediaPath . '/blank.mp3'); // should be a zip file, but we're testing here… + ->andReturn(test_path('songs/blank.mp3')); // should be a zip file, but we're testing here… $this->get( "download/songs?songs[]={$songs[0]->id}&songs[]={$songs[1]->id}&api_token=" @@ -89,22 +86,22 @@ class DownloadTest extends TestCase public function testDownloadAlbum(): void { /** @var Album $album */ - $album = Album::query()->first(); + $album = Album::factory()->create(); - $songs = Song::factory(3)->create(['album_id' => $album->id]); + $songs = Song::factory(3)->for($album)->create(); /** @var User $user */ $user = User::factory()->create(); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->once() ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs): bool { self::assertEqualsCanonicalizing($retrievedSongs->pluck('id')->all(), $songs->pluck('id')->all()); return true; })) - ->andReturn($this->mediaPath . '/blank.mp3'); + ->andReturn(test_path('songs/blank.mp3')); $this->get("download/album/{$album->id}?api_token=" . $user->createToken('Koel')->plainTextToken) ->assertOk(); @@ -113,22 +110,22 @@ class DownloadTest extends TestCase public function testDownloadArtist(): void { /** @var Artist $artist */ - $artist = Artist::query()->first(); + $artist = Artist::factory()->create(); - $songs = Song::factory(3)->create(['artist_id' => $artist->id]); + $songs = Song::factory(3)->for($artist)->create(); /** @var User $user */ $user = User::factory()->create(); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->once() ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs): bool { self::assertEqualsCanonicalizing($retrievedSongs->pluck('id')->all(), $songs->pluck('id')->all()); return true; })) - ->andReturn($this->mediaPath . '/blank.mp3'); + ->andReturn(test_path('songs/blank.mp3')); $this->get("download/artist/{$artist->id}?api_token=" . $user->createToken('Koel')->plainTextToken) ->assertOk(); @@ -147,14 +144,14 @@ class DownloadTest extends TestCase $playlist->songs()->attach($songs); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs): bool { self::assertEqualsCanonicalizing($retrievedSongs->pluck('id')->all(), $songs->pluck('id')->all()); return true; })) ->once() - ->andReturn($this->mediaPath . '/blank.mp3'); + ->andReturn(test_path('songs/blank.mp3')); $this->get("download/playlist/{$playlist->id}?api_token=" . $user->createToken('Koel')->plainTextToken) ->assertOk(); @@ -180,14 +177,14 @@ class DownloadTest extends TestCase $favorites = Interaction::factory(3)->for($user)->create(['liked' => true]); $this->downloadService - ->shouldReceive('from') + ->shouldReceive('getDownloadablePath') ->with(Mockery::on(static function (Collection $songs) use ($favorites): bool { self::assertEqualsCanonicalizing($songs->pluck('id')->all(), $favorites->pluck('song_id')->all()); return true; })) ->once() - ->andReturn($this->mediaPath . '/blank.mp3'); + ->andReturn(test_path('songs/blank.mp3')); $this->get('download/favorites?api_token=' . $user->createToken('Koel')->plainTextToken) ->assertOk(); diff --git a/tests/Feature/KoelPlus/DownloadTest.php b/tests/Feature/KoelPlus/DownloadTest.php new file mode 100644 index 00000000..dd559d27 --- /dev/null +++ b/tests/Feature/KoelPlus/DownloadTest.php @@ -0,0 +1,53 @@ +create(); + $apiToken = $owner->createToken('Koel')->plainTextToken; + + // Can't download a private song that doesn't belong to the user + /** @var Song $externalPrivateSong */ + $externalPrivateSong = Song::factory()->private()->create(); + $this->get("download/songs?songs[]=$externalPrivateSong->id&api_token=" . $apiToken) + ->assertForbidden(); + + // Can download a public song that doesn't belong to the user + /** @var Song $externalPublicSong */ + $externalPublicSong = Song::factory()->public()->create(); + + $downloadService = self::mock(DownloadService::class); + $downloadService->shouldReceive('getDownloadablePath') + ->once() + ->andReturn(test_path('songs/blank.mp3')); + + $this->get("download/songs?songs[]=$externalPublicSong->id&api_token=" . $apiToken) + ->assertOk(); + + // Can download a private song that belongs to the user + /** @var Song $ownSong */ + $ownSong = Song::factory()->for($owner, 'owner')->private()->create(); + $downloadService->shouldReceive('getDownloadablePath') + ->once() + ->andReturn(test_path('songs/blank.mp3')); + $this->get("download/songs?songs[]=$ownSong->id&api_token=" . $apiToken) + ->assertOk(); + } +} diff --git a/tests/Feature/SongTest.php b/tests/Feature/SongTest.php index 47a0f456..de94fe64 100644 --- a/tests/Feature/SongTest.php +++ b/tests/Feature/SongTest.php @@ -96,13 +96,11 @@ class SongTest extends TestCase public function testSingleUpdateAllInfoNoCompilation(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $this->putAs('/api/songs', [ 'songs' => [$song->id], @@ -136,13 +134,11 @@ class SongTest extends TestCase public function testSingleUpdateSomeInfoNoCompilation(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $originalArtistId = $song->artist->id; @@ -167,11 +163,9 @@ class SongTest extends TestCase public function testMultipleUpdateNoCompilation(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); - $songIds = Song::query()->latest()->take(3)->pluck('id')->all(); + $songIds = Song::factory(3)->create()->pluck('id')->all(); $this->putAs('/api/songs', [ 'songs' => $songIds, @@ -207,14 +201,14 @@ class SongTest extends TestCase public function testMultipleUpdateCreatingNewAlbumsAndArtists(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); /** @var array|Collection $originalSongs */ - $originalSongs = Song::query()->latest()->take(3)->get(); + $originalSongs = Song::factory(3)->create(); $originalSongIds = $originalSongs->pluck('id')->all(); + $originalAlbumNames = $originalSongs->pluck('album.name')->all(); + $originalAlbumIds = $originalSongs->pluck('album_id')->all(); $this->putAs('/api/songs', [ 'songs' => $originalSongIds, @@ -233,12 +227,10 @@ class SongTest extends TestCase // Even though the album name doesn't change, a new artist should have been created // and thus, a new album with the same name was created as well. - self::assertSame($songs[0]->album->name, $originalSongs[0]->album->name); - self::assertNotSame($songs[0]->album->id, $originalSongs[0]->album->id); - self::assertSame($songs[1]->album->name, $originalSongs[1]->album->name); - self::assertNotSame($songs[1]->album->id, $originalSongs[1]->album->id); - self::assertSame($songs[2]->album->name, $originalSongs[2]->album->name); - self::assertNotSame($songs[2]->album->id, $originalSongs[2]->album->id); + collect([0, 1, 2])->each(static function (int $i) use ($songs, $originalAlbumNames, $originalAlbumIds): void { + self::assertSame($songs[$i]->album->name, $originalAlbumNames[$i]); + self::assertNotSame($songs[$i]->album_id, $originalAlbumIds[$i]); + }); // And of course, the new artist is... self::assertSame('John Cena', $songs[0]->artist->name); // JOHN CENA!!! @@ -248,13 +240,11 @@ class SongTest extends TestCase public function testSingleUpdateAllInfoWithCompilation(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $this->putAs('/api/songs', [ 'songs' => [$song->id], @@ -293,8 +283,6 @@ class SongTest extends TestCase public function testUpdateSingleSongWithEmptyTrackAndDisc(): void { - static::createSampleMediaSet(); - /** @var User $user */ $user = User::factory()->admin()->create(); diff --git a/tests/Feature/YouTubeTest.php b/tests/Feature/YouTubeTest.php index b53b49fd..b500a9ec 100644 --- a/tests/Feature/YouTubeTest.php +++ b/tests/Feature/YouTubeTest.php @@ -21,10 +21,8 @@ class YouTubeTest extends TestCase public function testSearchYouTubeVideos(): void { - static::createSampleMediaSet(); - /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $this->youTubeService ->shouldReceive('searchVideosRelatedToSong') diff --git a/tests/Integration/Services/MediaScannerTest.php b/tests/Integration/Services/MediaScannerTest.php index 0a6e10aa..9d88ba3c 100644 --- a/tests/Integration/Services/MediaScannerTest.php +++ b/tests/Integration/Services/MediaScannerTest.php @@ -236,10 +236,8 @@ class MediaScannerTest extends TestCase /** @var User $owner */ $owner = User::factory()->admin()->create(); - static::createSampleMediaSet(); - /** @var Song $song */ - $song = Song::query()->first(); + $song = Song::factory()->create(); $this->scanner->scanWatchRecord( new InotifyWatchRecord("DELETE $song->path"), diff --git a/tests/TestCase.php b/tests/TestCase.php index fd6fe9ff..e1e5a62e 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -3,15 +3,11 @@ namespace Tests; use App\Facades\License; -use App\Models\Album; -use App\Models\Artist; -use App\Models\Song; use App\Services\License\CommunityLicenseService; use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; use Illuminate\Foundation\Testing\DatabaseTransactions; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; use Illuminate\Support\Facades\File; -use ReflectionClass; use Tests\Traits\CreatesApplication; use Tests\Traits\MakesHttpRequests; @@ -37,29 +33,6 @@ abstract class TestCase extends BaseTestCase parent::tearDown(); } - protected static function createSampleMediaSet(): void - { - /** @var Artist $artist */ - $artist = Artist::factory()->create(); - - /** @var array $albums */ - $albums = Album::factory(3)->for($artist)->create(); - - // 7-15 songs per albums - foreach ($albums as $album) { - Song::factory(random_int(7, 15))->for($artist)->for($album)->create(); - } - } - - protected static function getNonPublicProperty($object, string $property): mixed - { - $reflection = new ReflectionClass($object); - $property = $reflection->getProperty($property); - $property->setAccessible(true); - - return $property->getValue($object); - } - private static function createSandbox(): void { config(['koel.album_cover_dir' => 'sandbox/img/covers/']);