From e3bbf4ab4b875908714db351a98ac0644f48b02f Mon Sep 17 00:00:00 2001 From: Phan An Date: Sun, 15 Sep 2024 15:33:59 +0200 Subject: [PATCH] chore: some cleanups --- .../Controllers/API/ScrobbleController.php | 2 +- app/Models/Playlist.php | 42 ++++++++----------- app/Models/User.php | 40 +++++++++--------- app/Services/PlaylistCollaborationService.php | 2 +- app/Services/PlaylistService.php | 10 ++--- tests/Feature/KoelPlus/PlaylistSongTest.php | 4 +- tests/Feature/PlaylistSongTest.php | 6 +-- tests/Feature/PlaylistTest.php | 2 +- tests/Feature/SongPlayTest.php | 2 +- .../Services/PlaylistServiceTest.php | 20 ++++----- 10 files changed, 60 insertions(+), 70 deletions(-) diff --git a/app/Http/Controllers/API/ScrobbleController.php b/app/Http/Controllers/API/ScrobbleController.php index 33ec4587..452a5294 100644 --- a/app/Http/Controllers/API/ScrobbleController.php +++ b/app/Http/Controllers/API/ScrobbleController.php @@ -14,7 +14,7 @@ class ScrobbleController extends Controller /** @param User $user */ public function __invoke(ScrobbleRequest $request, Song $song, Authenticatable $user) { - if (!$song->artist->is_unknown && $user->connectedToLastfm()) { + if (!$song->artist->is_unknown && $user->connected_to_lastfm) { ScrobbleJob::dispatch($user, $song, $request->timestamp); } diff --git a/app/Models/Playlist.php b/app/Models/Playlist.php index 47019a79..0cfd14c4 100644 --- a/app/Models/Playlist.php +++ b/app/Models/Playlist.php @@ -4,6 +4,7 @@ namespace App\Models; use App\Casts\SmartPlaylistRulesCast; use App\Facades\License as LicenseFacade; +use App\Models\Song as Playable; use App\Values\SmartPlaylistRuleGroupCollection; use Carbon\Carbon; use Illuminate\Database\Eloquent\Casts\Attribute; @@ -16,7 +17,6 @@ use Illuminate\Support\Arr; use Illuminate\Support\Collection; use Illuminate\Support\Str; use Laravel\Scout\Searchable; -use LogicException; /** * @property string $id @@ -24,8 +24,7 @@ use LogicException; * @property bool $is_smart * @property int $user_id * @property User $user - * @property Collection $songs - * @property array $song_ids + * @property Collection $playables * @property ?SmartPlaylistRuleGroupCollection $rule_groups * @property ?SmartPlaylistRuleGroupCollection $rules * @property Carbon $created_at @@ -61,9 +60,9 @@ class Playlist extends Model }); } - public function songs(): BelongsToMany + public function playables(): BelongsToMany { - return $this->belongsToMany(Song::class) + return $this->belongsToMany(Playable::class) ->withTimestamps() ->withPivot('position') ->orderByPivot('position'); @@ -100,13 +99,6 @@ class Playlist extends Model return Attribute::get(fn () => $this->rules); } - public function songIds(): Attribute - { - throw_if($this->is_smart, new LogicException('Smart playlist contents are generated dynamically.')); - - return Attribute::get(fn () => $this->songs->pluck('id')->all()); - } - protected function cover(): Attribute { return Attribute::get(static fn (?string $value): ?string => playlist_cover_url($value)); @@ -156,39 +148,39 @@ class Playlist extends Model } /** - * @param Collection|array|Song|array $songs + * @param Collection|array|Playable|array $playables */ - public function addPlayables(Collection|Song|array $songs, ?User $collaborator = null): void + public function addPlayables(Collection|Playable|array $playables, ?User $collaborator = null): void { $collaborator ??= $this->user; - $maxPosition = $this->songs()->getQuery()->max('position') ?? 0; + $maxPosition = $this->playables()->getQuery()->max('position') ?? 0; - if (!is_array($songs)) { - $songs = Collection::wrap($songs)->pluck('id')->all(); + if (!is_array($playables)) { + $playables = Collection::wrap($playables)->pluck('id')->all(); } $data = []; - foreach ($songs as $song) { - $data[$song] = [ + foreach ($playables as $playable) { + $data[$playable] = [ 'position' => ++$maxPosition, 'user_id' => $collaborator->id, ]; } - $this->songs()->attach($data); + $this->playables()->attach($data); } /** - * @param Collection|Song|array $songs + * @param Collection|Playable|array $playables */ - public function removePlayables(Collection|Song|array $songs): void + public function removePlayables(Collection|Playable|array $playables): void { - if (!is_array($songs)) { - $songs = Collection::wrap($songs)->pluck('id')->all(); + if (!is_array($playables)) { + $playables = Collection::wrap($playables)->pluck('id')->all(); } - $this->songs()->detach($songs); + $this->playables()->detach($playables); } protected function isCollaborative(): Attribute diff --git a/app/Models/User.php b/app/Models/User.php index 0812c670..583cd769 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -20,27 +20,28 @@ use Laravel\Sanctum\HasApiTokens; use Laravel\Sanctum\PersonalAccessToken; /** - * @property UserPreferences $preferences - * @property int $id - * @property bool $is_admin - * @property string $name - * @property string $email - * @property string $password - * @property-read bool $has_custom_avatar - * @property-read string $avatar - * @property Collection $playlists - * @property Collection $playlist_folders - * @property PersonalAccessToken $currentAccessToken * @property ?Carbon $invitation_accepted_at + * @property ?Carbon $invited_at * @property ?User $invitedBy * @property ?string $invitation_token - * @property ?Carbon $invited_at - * @property-read bool $is_prospect * @property Collection $collaboratedPlaylists - * @property ?string $sso_provider - * @property ?string $sso_id - * @property bool $is_sso + * @property Collection $playlists + * @property Collection $playlist_folders * @property Collection $podcasts + * @property PersonalAccessToken $currentAccessToken + * @property UserPreferences $preferences + * @property bool $is_admin + * @property int $id + * @property string $email + * @property string $name + * @property string $password + * @property-read ?string $sso_id + * @property-read ?string $sso_provider + * @property-read bool $connected_to_lastfm Whether the user is connected to Last.fm + * @property-read bool $has_custom_avatar + * @property-read bool $is_prospect + * @property-read bool $is_sso + * @property-read string $avatar */ class User extends Authenticatable { @@ -126,11 +127,8 @@ class User extends Authenticatable return Attribute::get(fn (): bool => License::isPlus() && $this->sso_provider); } - /** - * Determine if the user is connected to Last.fm. - */ - public function connectedToLastfm(): bool + protected function connectedToLastfm(): Attribute { - return (bool) $this->preferences->lastFmSessionKey; + return Attribute::get(fn (): bool => (bool) $this->preferences->lastFmSessionKey); } } diff --git a/app/Services/PlaylistCollaborationService.php b/app/Services/PlaylistCollaborationService.php index 3e719d4e..c489b765 100644 --- a/app/Services/PlaylistCollaborationService.php +++ b/app/Services/PlaylistCollaborationService.php @@ -65,7 +65,7 @@ class PlaylistCollaborationService DB::transaction(static function () use ($playlist, $user): void { $playlist->collaborators()->detach($user); - $playlist->songs()->wherePivot('user_id', $user->id)->detach(); + $playlist->playables()->wherePivot('user_id', $user->id)->detach(); }); } } diff --git a/app/Services/PlaylistService.php b/app/Services/PlaylistService.php index 60d3b366..eefe4378 100644 --- a/app/Services/PlaylistService.php +++ b/app/Services/PlaylistService.php @@ -95,7 +95,7 @@ class PlaylistService $playables = Collection::wrap($playables); $playlist->addPlayables( - $playables->filter(static fn ($song): bool => !$playlist->songs->contains($song)), + $playables->filter(static fn ($song): bool => !$playlist->playables->contains($song)), $user ); @@ -119,7 +119,7 @@ class PlaylistService public function makePlaylistContentPublic(Playlist $playlist): void { - $playlist->songs()->where('is_public', false)->update(['is_public' => true]); + $playlist->playables()->where('is_public', false)->update(['is_public' => true]); } public function movePlayablesInPlaylist(Playlist $playlist, array $movingIds, string $target, string $type): void @@ -128,11 +128,11 @@ class PlaylistService throw_if($playlist->is_smart, OperationNotApplicableForSmartPlaylistException::class); DB::transaction(static function () use ($playlist, $movingIds, $target, $type): void { - $targetPosition = $playlist->songs()->wherePivot('song_id', $target)->value('position'); + $targetPosition = $playlist->playables()->wherePivot('song_id', $target)->value('position'); $insertPosition = $type === 'before' ? $targetPosition : $targetPosition + 1; // create a "gap" for the moving songs by incrementing the position of the songs after the target - $playlist->songs() + $playlist->playables() ->newPivotQuery() ->where('position', $type === 'before' ? '>=' : '>', $targetPosition) ->whereNotIn('song_id', $movingIds) @@ -144,7 +144,7 @@ class PlaylistService $values[$id] = ['position' => $insertPosition++]; } - $playlist->songs()->syncWithoutDetaching($values); + $playlist->playables()->syncWithoutDetaching($values); }); } } diff --git a/tests/Feature/KoelPlus/PlaylistSongTest.php b/tests/Feature/KoelPlus/PlaylistSongTest.php index fb70d5cb..560fbf06 100644 --- a/tests/Feature/KoelPlus/PlaylistSongTest.php +++ b/tests/Feature/KoelPlus/PlaylistSongTest.php @@ -58,7 +58,7 @@ class PlaylistSongTest extends PlusTestCase ->assertSuccessful(); $playlist->refresh(); - $songs->each(static fn (Song $song) => self::assertTrue($playlist->songs->contains($song))); + $songs->each(static fn (Song $song) => self::assertTrue($playlist->playables->contains($song))); } public function testCollaboratorCanRemoveSongs(): void @@ -74,6 +74,6 @@ class PlaylistSongTest extends PlusTestCase ->assertSuccessful(); $playlist->refresh(); - $songs->each(static fn (Song $song) => self::assertFalse($playlist->songs->contains($song))); + $songs->each(static fn (Song $song) => self::assertFalse($playlist->playables->contains($song))); } } diff --git a/tests/Feature/PlaylistSongTest.php b/tests/Feature/PlaylistSongTest.php index b3cb36cb..a3ce70c5 100644 --- a/tests/Feature/PlaylistSongTest.php +++ b/tests/Feature/PlaylistSongTest.php @@ -68,7 +68,7 @@ class PlaylistSongTest extends TestCase $this->postAs("api/playlists/$playlist->id/songs", ['songs' => $songs->pluck('id')->all()], $playlist->user) ->assertSuccessful(); - self::assertEqualsCanonicalizing($songs->pluck('id')->all(), $playlist->songs->pluck('id')->all()); + self::assertEqualsCanonicalizing($songs->pluck('id')->all(), $playlist->playables->pluck('id')->all()); } public function testRemoveSongsFromPlaylist(): void @@ -83,7 +83,7 @@ class PlaylistSongTest extends TestCase $playlist->addPlayables($toRemainSongs->merge($toBeRemovedSongs)); - self::assertCount(7, $playlist->songs); + self::assertCount(7, $playlist->playables); $this->deleteAs( "api/playlists/$playlist->id/songs", @@ -94,7 +94,7 @@ class PlaylistSongTest extends TestCase $playlist->refresh(); - self::assertEqualsCanonicalizing($toRemainSongs->pluck('id')->all(), $playlist->songs->pluck('id')->all()); + self::assertEqualsCanonicalizing($toRemainSongs->pluck('id')->all(), $playlist->playables->pluck('id')->all()); } public function testNonOwnerCannotModifyPlaylist(): void diff --git a/tests/Feature/PlaylistTest.php b/tests/Feature/PlaylistTest.php index e907f283..26d0003c 100644 --- a/tests/Feature/PlaylistTest.php +++ b/tests/Feature/PlaylistTest.php @@ -43,7 +43,7 @@ class PlaylistTest extends TestCase self::assertSame('Foo Bar', $playlist->name); self::assertTrue($playlist->ownedBy($user)); self::assertNull($playlist->getFolder()); - self::assertEqualsCanonicalizing($songs->pluck('id')->all(), $playlist->songs->pluck('id')->all()); + self::assertEqualsCanonicalizing($songs->pluck('id')->all(), $playlist->playables->pluck('id')->all()); } public function testCreatingSmartPlaylist(): void diff --git a/tests/Feature/SongPlayTest.php b/tests/Feature/SongPlayTest.php index d0f5f56b..0815bcb2 100644 --- a/tests/Feature/SongPlayTest.php +++ b/tests/Feature/SongPlayTest.php @@ -75,7 +75,7 @@ class SongPlayTest extends TestCase ->shouldReceive('stream') ->once(); - $this->get("play/$song->id/1/128?t=$token->audioToken") + $this->get("play/$song->id/1?t=$token->audioToken") ->assertOk(); } } diff --git a/tests/Integration/Services/PlaylistServiceTest.php b/tests/Integration/Services/PlaylistServiceTest.php index f2c1b94f..25d110c8 100644 --- a/tests/Integration/Services/PlaylistServiceTest.php +++ b/tests/Integration/Services/PlaylistServiceTest.php @@ -50,7 +50,7 @@ class PlaylistServiceTest extends TestCase self::assertSame('foo', $playlist->name); self::assertTrue($user->is($playlist->user)); self::assertFalse($playlist->is_smart); - self::assertEqualsCanonicalizing($playlist->songs->pluck('id')->all(), $songs->pluck('id')->all()); + self::assertEqualsCanonicalizing($playlist->playables->pluck('id')->all(), $songs->pluck('id')->all()); } public function testCreateSmartPlaylist(): void @@ -201,9 +201,9 @@ class PlaylistServiceTest extends TestCase $playlist->refresh(); self::assertCount(2, $addedSongs); - self::assertCount(5, $playlist->songs); + self::assertCount(5, $playlist->playables); self::assertEqualsCanonicalizing($addedSongs->pluck('id')->all(), $songs->pluck('id')->all()); - $songs->each(static fn (Song $song) => self::assertTrue($playlist->songs->contains($song))); + $songs->each(static fn (Song $song) => self::assertTrue($playlist->playables->contains($song))); } public function testAddEpisodesToPlaylist(): void @@ -222,7 +222,7 @@ class PlaylistServiceTest extends TestCase $playlist->refresh(); self::assertCount(2, $addedEpisodes); - self::assertCount(5, $playlist->songs); + self::assertCount(5, $playlist->playables); self::assertEqualsCanonicalizing($addedEpisodes->pluck('id')->all(), $episodes->pluck('id')->all()); } @@ -238,7 +238,7 @@ class PlaylistServiceTest extends TestCase $playlist->refresh(); self::assertCount(4, $addedEpisodes); - self::assertCount(7, $playlist->songs); + self::assertCount(7, $playlist->playables); self::assertEqualsCanonicalizing($addedEpisodes->pluck('id')->all(), $playables->pluck('id')->all()); } @@ -268,7 +268,7 @@ class PlaylistServiceTest extends TestCase $this->service->makePlaylistContentPublic($playlist); - $playlist->songs->each(static fn (Song $song) => self::assertTrue($song->is_public)); + $playlist->playables->each(static fn (Song $song) => self::assertTrue($song->is_public)); } public function testMoveSongsInPlaylist(): void @@ -282,17 +282,17 @@ class PlaylistServiceTest extends TestCase $playlist->addPlayables($songs); $this->service->movePlayablesInPlaylist($playlist, [$ids[2], $ids[3]], $ids[0], 'after'); - self::assertSame([$ids[0], $ids[2], $ids[3], $ids[1]], $playlist->refresh()->songs->pluck('id')->all()); + self::assertSame([$ids[0], $ids[2], $ids[3], $ids[1]], $playlist->refresh()->playables->pluck('id')->all()); $this->service->movePlayablesInPlaylist($playlist, [$ids[0]], $ids[3], 'before'); - self::assertSame([$ids[2], $ids[0], $ids[3], $ids[1]], $playlist->refresh()->songs->pluck('id')->all()); + self::assertSame([$ids[2], $ids[0], $ids[3], $ids[1]], $playlist->refresh()->playables->pluck('id')->all()); // move to the first position $this->service->movePlayablesInPlaylist($playlist, [$ids[0], $ids[1]], $ids[2], 'before'); - self::assertSame([$ids[0], $ids[1], $ids[2], $ids[3]], $playlist->refresh()->songs->pluck('id')->all()); + self::assertSame([$ids[0], $ids[1], $ids[2], $ids[3]], $playlist->refresh()->playables->pluck('id')->all()); // move to the last position $this->service->movePlayablesInPlaylist($playlist, [$ids[0], $ids[1]], $ids[3], 'after'); - self::assertSame([$ids[2], $ids[3], $ids[0], $ids[1]], $playlist->refresh()->songs->pluck('id')->all()); + self::assertSame([$ids[2], $ids[3], $ids[0], $ids[1]], $playlist->refresh()->playables->pluck('id')->all()); } }