From 803b2da8ee37c80f36c2c93ba42ef0496ffaef08 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 10 Oct 2026 11:35:43 -0400 Subject: [PATCH] refactor(playlists): simplify #6310 tests and update check comments Builds the removeOrphans fixture with a single INSERT instead of shifting positions in two UPDATE passes, and has the Insert test keep the playlist id instead of reading a private repository field. Two comments now say that addTracks owns the existence check as well as the library check, and that Insert closes holes left by any id keepAccessible drops. No behavior change. --- persistence/playlist_repository.go | 2 +- persistence/playlist_repository_test.go | 10 ++-------- persistence/playlist_track_repository.go | 2 +- persistence/playlist_track_repository_test.go | 4 +++- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index c7559de18..1a1b05a5c 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -281,7 +281,7 @@ func (r *playlistRepository) updatePlaylist(ctx context.Context, playlistId stri } // addTracks is the only path that writes playlist_tracks rows (smart playlists aside), so it owns -// the library check: every caller, including a full replace through Put, goes through it. +// the library and existence check: every caller, including a full replace through Put, goes through it. func (r *playlistRepository) addTracks(ctx context.Context, playlistId string, startingPos int, mediaFileIds []string) (int, error) { mediaFileIds, err := r.keepAccessible(ctx, mediaFileIds) if err != nil { diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index d0f1f7f44..b7be0588b 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -472,17 +472,11 @@ var _ = Describe("PlaylistRepository", func() { Describe("removeOrphans", func() { It("deletes rows pointing to no track and renumbers the rest", func() { pls := model.Playlist{Name: "Orphans", OwnerID: "userid"} - pls.AddMediaFilesByID([]string{"1001", "1002"}) Expect(repo.Put(ctx, &pls)).To(Succeed()) DeferCleanup(func() { Expect(repo.Delete(ctx, pls.ID)).To(Succeed()) }) - // Rows an older version stored for unknown ids, interleaved with real tracks - for _, q := range []string{"-(id * 2)", "-id"} { - _, err := GetDBXBuilder().NewQuery(`UPDATE playlist_tracks SET id = ` + q + ` WHERE playlist_id = {:id}`). - Bind(dbx.Params{"id": pls.ID}).Execute() - Expect(err).ToNot(HaveOccurred()) - } + // Rows as an older version stored them, with an unknown id between real tracks _, err := GetDBXBuilder().NewQuery(`INSERT INTO playlist_tracks (playlist_id, media_file_id, id) - VALUES ({:id}, 'no-such-track', 1), ({:id}, '!!not-an-id!!', 3)`). + VALUES ({:id}, '1001', 1), ({:id}, 'no-such-track', 2), ({:id}, '1002', 3)`). Bind(dbx.Params{"id": pls.ID}).Execute() Expect(err).ToNot(HaveOccurred()) diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 392446cef..587410305 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -213,7 +213,7 @@ func (r *playlistTrackRepository) Insert(ctx context.Context, mediaFileIds []str if err != nil || inserted == n { return inserted, err } - // The shift above reserved a slot per requested id, so ids dropped by the library filter + // The shift above reserved a slot per requested id, so ids dropped by keepAccessible // leave a hole. Close it. return inserted, r.playlistRepo.renumber(ctx, r.playlistId) } diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index aba4fdd95..29911ebd4 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -83,12 +83,14 @@ var _ = Describe("PlaylistTrackRepository", func() { Describe("Insert", func() { var tracks model.PlaylistTrackRepository + var plsID string BeforeEach(func() { plsRepo := NewPlaylistRepository(GetDBXBuilder()) pls := model.Playlist{Name: "Insert", OwnerID: "userid", OwnerName: "userid"} Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) DeferCleanup(func() { Expect(plsRepo.Delete(ctx, pls.ID)).To(Succeed()) }) + plsID = pls.ID tracks = plsRepo.Tracks(ctx, pls.ID, false) Expect(tracks.Add(ctx, []string{songDayInALife.ID, songRadioactivity.ID})).To(Equal(2)) @@ -127,7 +129,7 @@ var _ = Describe("PlaylistTrackRepository", func() { It("drops ids that match no track, keeping positions contiguous", func() { Expect(tracks.Insert(ctx, []string{"no-such-track", songComeTogether.ID}, 1)).To(Equal(1)) - Expect(storedRows(tracks.(*playlistTrackRepository).playlistId)).To(Equal([]string{ + Expect(storedRows(plsID)).To(Equal([]string{ "1:" + songComeTogether.ID, "2:" + songDayInALife.ID, "3:" + songRadioactivity.ID, })) })