diff --git a/internal/library/duplicate_classify_test.go b/internal/library/duplicate_classify_test.go index b5dba9f8..55a289bf 100644 --- a/internal/library/duplicate_classify_test.go +++ b/internal/library/duplicate_classify_test.go @@ -101,7 +101,8 @@ func TestWrongSlots(t *testing.T) { }, []dbq.MbReleaseTrack{track("Killjoy", 166500), track("Dreamland", 214000)}, 1, []int{0}, true}, {"two slots fit the file", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 221500)}, -1, nil, false}, {"a slot near but not within the fit", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 226000)}, -1, nil, false}, - {"no slot fits", copies("Reach", "Slimewar (into the void)"), void, -1, nil, false}, + {"no slot fits, and every slot is clearly another song's", copies("Reach", "Slimewar (into the void)"), void, -1, []int{0, 1}, true}, + {"no slot fits, but one is near", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 226000), track("Reach", 281000)}, -1, nil, false}, {"an unknown length counts for nothing", copies("Void", "Reach"), []dbq.MbReleaseTrack{track("Void", 220000), track("Reach", 0)}, -1, nil, false}, } for _, tc := range cases { diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index cf82e664..ea707ca4 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -405,6 +405,27 @@ func TestResolveDuplicates_RemovesACopyFromAnotherSongsSlot_Integration(t *testi } } +// One recording filling two slots whose songs are both clearly something else +// (Shnabubula's whole album in one file): both copies go, nothing is kept. +func TestResolveDuplicates_RemovesEveryCopyWhenNoSlotIsTheirs_Integration(t *testing.T) { + f := newOneAlbumFixture(t, "acoustic", "Void", "Reach") + seedTrackList(t, f.pool, f.album, map[string]int{"Void": 282000, "Reach": 421000}) + res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", true) + if err != nil { + t.Fatal(err) + } + if res.WrongFiles != 2 || exists(f.aPath) || exists(f.bPath) { + t.Fatalf("result %+v, void exists %v, reach exists %v; want both copies removed", + res, exists(f.aPath), exists(f.bPath)) + } + if status, _, _, auto := (resolveFixture{pool: f.pool, groupID: f.groupID}).group(t); status != "merged" || !auto { + t.Errorf("group = (%s, auto=%v), want settled by the resolver", status, auto) + } + if n := auditCount(t, f.pool, "duplicate_wrong_file", f.groupID); n != 1 { + t.Errorf("wrong-file audit rows = %d, want 1", n) + } +} + // An album whose track list the catalogue never read: the resolver asks for // it, once, and decides with what comes back. func TestResolveDuplicates_FetchesAMissingTrackList_Integration(t *testing.T) { diff --git a/internal/library/duplicate_settle.go b/internal/library/duplicate_settle.go index bc651a79..0d4cb594 100644 --- a/internal/library/duplicate_settle.go +++ b/internal/library/duplicate_settle.go @@ -31,6 +31,7 @@ import ( // long as the file and every other slot's song is clearly not, the copies // in the other slots are removed and their tracks marked missing, so the // reacquisition sweeper wants the album back and a good copy fills them. +// When every slot's song is clearly not the file, every copy goes. // Anything less certain stays with the operator. const ( @@ -52,8 +53,11 @@ type slotCopy struct { } // wrongSlots decides which copies of a one-album mismatch sit in a slot that -// is not theirs. keep is the copy whose slot fits the recording; ok is false -// when the track list cannot say, and nothing should be removed. +// is not theirs. keep is the copy whose slot fits the recording, or -1 when +// every copy is clearly another song's (Shnabubula's whole album, 45 minutes +// in one file, filed as both "Fading Light" and "Looking for Answers"): then +// all of them go. ok is false when the track list cannot say, and nothing +// should be removed. // // Several copies may fit when they carry the same title ("Killjoy" twice // beside "Dreamland"): they are one song held twice, not a wrong slot, so they @@ -92,7 +96,9 @@ func wrongSlots(copies []slotCopy, tracks []dbq.MbReleaseTrack) (keep int, loser return -1, nil, false } } - if keep < 0 || len(losers) == 0 { + // A copy that neither fits nor is clearly another song's has returned + // above, so with no keep every copy is a loser. + if len(losers) == 0 { return -1, nil, false } return keep, losers, true @@ -261,17 +267,21 @@ func removeWrongSlots( if err != nil { return 0, err } - kept := g.members[keep] - audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionDuplicateWrongFile, map[string]any{ - "automatic": true, - "group_id": syncpkg.FormatUUID(g.id), - "artist": kept.ArtistName, - "kept_title": kept.Title, - "kept_path": kept.FilePath, - "removed": removedFiles, - }) + details := map[string]any{ + "automatic": true, + "group_id": syncpkg.FormatUUID(g.id), + "artist": g.members[0].ArtistName, + "removed": removedFiles, + } + keptPath := "" + if keep >= 0 { + kept := g.members[keep] + details["kept_title"], details["kept_path"] = kept.Title, kept.FilePath + keptPath = kept.FilePath + } + audit.WriteOrLog(ctx, pool, logger, pgtype.UUID{}, pgtype.UUID{}, audit.ActionDuplicateWrongFile, details) logger.Info("duplicate resolve: removed copies from slots that were not theirs", - "group_id", syncpkg.FormatUUID(g.id), "kept", kept.FilePath, "removed", len(gone)) + "group_id", syncpkg.FormatUUID(g.id), "kept", keptPath, "removed", len(gone)) return len(gone), removeErr } diff --git a/web/src/routes/collection/duplicates/+page.svelte b/web/src/routes/collection/duplicates/+page.svelte index 6d569e16..6667dbf7 100644 --- a/web/src/routes/collection/duplicates/+page.svelte +++ b/web/src/routes/collection/duplicates/+page.svelte @@ -248,6 +248,11 @@ {item.details.artist}: {(item.details.titles ?? []).join(' and ')} are different versions, not duplicates. Both kept.
+ {:else if item.action === 'duplicate_wrong_file' && !item.details.kept_title} ++ {item.details.artist}: one recording was filed as {removed.map((r) => r.title).join(' and ')}, + and it is none of them. Removed; the album will be fetched again. +
{:else if item.action === 'duplicate_wrong_file'}{item.details.artist}: the recording of {item.details.kept_title} was also filed as diff --git a/web/src/routes/collection/duplicates/duplicates.test.ts b/web/src/routes/collection/duplicates/duplicates.test.ts index 60ccfcc9..9007accc 100644 --- a/web/src/routes/collection/duplicates/duplicates.test.ts +++ b/web/src/routes/collection/duplicates/duplicates.test.ts @@ -372,6 +372,18 @@ describe('admin duplicates', () => { kept_title: 'Void', removed: [{ track_id: 't-9', file_path: '/data/Music/Savant/Void/17 - Reach.m4a', title: 'Reach' }] } + }, + { + id: 'a-4', + action: 'duplicate_wrong_file', + created_at: ago(HOUR), + details: { + artist: 'Shnabubula', + removed: [ + { track_id: 't-5', file_path: '/data/Music/Shnabubula/05 - Fading Light.mp3', title: 'Fading Light' }, + { track_id: 't-10', file_path: '/data/Music/Shnabubula/10 - Looking for Answers.mp3', title: 'Looking for Answers' } + ] + } } ]); await fireEvent.click(screen.getByRole('tab', { name: /resolved automatically/i })); @@ -381,6 +393,9 @@ describe('admin duplicates', () => { 'Savant: the recording of Void was also filed as Reach. Removed from there; the album will be fetched again.' ); expect(items[1]).toContain('/data/Music/Savant/Void/17 - Reach.m4a'); + expect(items[2]).toContain( + 'Shnabubula: one recording was filed as Fading Light and Looking for Answers, and it is none of them. Removed; the album will be fetched again.' + ); }); });