chore: refactor tests and factories

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

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);

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());

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)
+    public function __invoke(
-    {
+        Album $album,
-        return response()->download($download->from($repository->getByAlbum($album, $user)));
+        SongRepository $repository,
+        DownloadService $service,
+        Authenticatable $user
+    ) {
+        return response()->download($service->getDownloadablePath($repository->getByAlbum($album, $user)));
     }
 }

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)));
     }
 }

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)));
     }
 }

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)

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)));
     }
 }

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));
         });
 

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 {

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 */

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.

database/factories/SongFactory.php

@@ -14,12 +14,9 @@ class SongFactory extends Factory
     /** @return array<mixed> */
     public function definition(): array
     {
-        /** @var Album $album */
-        $album = Album::factory()->create();
-
         return [
-            'album_id' => $album->id,
+            'album_id' => Album::factory(),
-            'artist_id' => $album->artist->id,
+            '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),

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<Song>|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();
+                self::assertEqualsCanonicalizing($retrievedSongs->pluck('id')->all(), $songs->pluck('id')->all());
-                $requestedIds = $songs->pluck('id')->all();
-                self::assertEqualsCanonicalizing($requestedIds, $retrievedIds);
 
                 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();

tests/Feature/KoelPlus/DownloadTest.php

@@ -0,0 +1,53 @@
+<?php
+
+namespace Tests\Feature\KoelPlus;
+
+use App\Facades\License;
+use App\Models\Song;
+use App\Models\User;
+use App\Services\DownloadService;
+use Tests\TestCase;
+
+class DownloadTest extends TestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        License::fakePlusLicense();
+    }
+
+    public function testDownloadPolicy(): void
+    {
+        /** @var User $owner */
+        $owner = User::factory()->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();
+    }
+}

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<array-key, Song>|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);
+        collect([0, 1, 2])->each(static function (int $i) use ($songs, $originalAlbumNames, $originalAlbumIds): void {
-        self::assertNotSame($songs[0]->album->id, $originalSongs[0]->album->id);
+            self::assertSame($songs[$i]->album->name, $originalAlbumNames[$i]);
-        self::assertSame($songs[1]->album->name, $originalSongs[1]->album->name);
+            self::assertNotSame($songs[$i]->album_id, $originalAlbumIds[$i]);
-        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);
 
         // 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();
 

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')

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"),

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<Album> $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/']);