From 355a12c6a6cc766fba2783d72160725077380354 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sat, 25 Jul 2026 23:12:09 +0300 Subject: [PATCH] feat: complete task 4 - fix scanner diff.go TypeFilter.suppressed to use centralized filter --- docs/plans/2026-07-21-fix-review-findings.md | 10 +- internal/scanner/diff.go | 23 +-- internal/scanner/diff_test.go | 150 +++++++++++++++++++ 3 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 internal/scanner/diff_test.go diff --git a/docs/plans/2026-07-21-fix-review-findings.md b/docs/plans/2026-07-21-fix-review-findings.md index d2dac91..58e7f0e 100644 --- a/docs/plans/2026-07-21-fix-review-findings.md +++ b/docs/plans/2026-07-21-fix-review-findings.md @@ -56,11 +56,11 @@ Problem: Three separate filter implementations (`musicbrainz.FilterReleaseGroups - [x] Run tests - must pass before task 4 ### Task 4: Fix scanner diff.go TypeFilter.suppressed to use centralized filter -- [ ] Update `TypeFilter.suppressed` in `diff.go` to use the same logic as `ApplyTypeToggles` (i.e., treat `EP` in SecondaryTypes as a Single when `IgnoreSingles=true`) -- [ ] Since scanner is separate package, either: (a) export `ApplyTypeToggles` from musicbrainz and import, or (b) duplicate the minimal logic with a comment referencing the canonical source. Choose (a) for DRY. -- [ ] Update `scanner/diff.go` to import `musicbrainz` and use the shared filter -- [ ] Write tests in `diff_test.go` verifying scanner filter matches musicbrainz filter for all release type combinations -- [ ] Run tests - must pass before task 5 +- [x] Update `TypeFilter.suppressed` in `diff.go` to use the same logic as `ApplyTypeToggles` (i.e., treat `EP` in SecondaryTypes as a Single when `IgnoreSingles=true`) +- [x] Since scanner is separate package, either: (a) export `ApplyTypeToggles` from musicbrainz and import, or (b) duplicate the minimal logic with a comment referencing the canonical source. Choose (a) for DRY. +- [x] Update `scanner/diff.go` to import `musicbrainz` and use the shared filter +- [x] Write tests in `diff_test.go` verifying scanner filter matches musicbrainz filter for all release type combinations +- [x] Run tests - must pass before task 5 ### Task 5: Fix ArtistCacheFresh lexicographic time comparison - [ ] Change `external_releases.cached_at` from TEXT to INTEGER (unix epoch seconds) via migration `006_cached_at_to_integer` diff --git a/internal/scanner/diff.go b/internal/scanner/diff.go index efcd2ad..2f9c960 100644 --- a/internal/scanner/diff.go +++ b/internal/scanner/diff.go @@ -2,6 +2,7 @@ package scanner import ( "naviwatcher/internal/database" + "naviwatcher/internal/musicbrainz" ) // MissingRelease describes an external release that has no sufficiently similar @@ -34,23 +35,13 @@ type TypeFilter struct { // secondary types, matching musicbrainz.FilterReleaseGroups so both the // cache-miss (store-time) and read-time paths agree. func (f TypeFilter) suppressed(ext database.ExternalRelease) bool { - if f.IgnoreSingles && (ext.Type == "Single" || hasType(ext.SecondaryTypes, "Single")) { - return true + // Use the centralized filtering logic from musicbrainz package + opts := musicbrainz.FilterOptions{ + IgnoreSingles: f.IgnoreSingles, + IgnoreCompilations: f.IgnoreCompilations, } - if f.IgnoreCompilations && (ext.Type == "Compilation" || hasType(ext.SecondaryTypes, "Compilation")) { - return true - } - return false -} - -// hasType reports whether types contains want. -func hasType(types []string, want string) bool { - for _, t := range types { - if t == want { - return true - } - } - return false + filtered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{ext}, opts) + return len(filtered) == 0 } // FindMissingReleases compares an artist's external discography against the diff --git a/internal/scanner/diff_test.go b/internal/scanner/diff_test.go new file mode 100644 index 0000000..53eedc9 --- /dev/null +++ b/internal/scanner/diff_test.go @@ -0,0 +1,150 @@ +package scanner + +import ( + "testing" + + "naviwatcher/internal/database" + "naviwatcher/internal/musicbrainz" +) + +func TestTypeFilterSuppressedMatchesMusicbrainzFilter(t *testing.T) { + // Test cases covering various combinations of types and secondary types + testCases := []struct { + name string + releaseType string + secondaryTypes []string + ignoreSingles bool + ignoreCompilations bool + expectedSuppressed bool + }{ + // Single type tests + {"Single primary type", "Single", []string{}, true, false, true}, + {"Single primary type with EP ignore", "Single", []string{}, false, true, false}, + + // EP as primary type (should be treated as Single when IgnoreSingles=true) + {"EP primary type", "EP", []string{}, true, false, true}, + {"EP primary type with EP ignore", "EP", []string{}, false, true, false}, + + // Album type tests + {"Album primary type", "Album", []string{}, true, false, false}, + {"Album primary type with Compilation ignore", "Album", []string{}, false, true, false}, + + // Compilation type tests + {"Compilation primary type", "Compilation", []string{}, true, false, false}, + {"Compilation primary type with Compilation ignore", "Compilation", []string{}, false, true, true}, + + // Secondary types - Single + {"Album with Single secondary", "Album", []string{"Single"}, true, false, true}, + {"Album with Single secondary (no ignore)", "Album", []string{"Single"}, false, false, false}, + {"EP with Single secondary", "EP", []string{"Single"}, true, false, true}, + + // Secondary types - EP (should trigger Single ignore) + {"Album with EP secondary", "Album", []string{"EP"}, true, false, true}, + {"Album with EP secondary (no ignore)", "Album", []string{"EP"}, false, false, false}, + + // Secondary types - Compilation + {"Album with Compilation secondary", "Album", []string{"Compilation"}, true, false, false}, + {"Album with Compilation secondary (with ignore)", "Album", []string{"Compilation"}, false, true, true}, + + // Multiple secondary types + {"Album with Single and EP secondary", "Album", []string{"Single", "EP"}, true, false, true}, + {"Album with Compilation secondary", "Album", []string{"Compilation"}, false, true, true}, + {"Album with multiple secondary types", "Album", []string{"Single", "Compilation"}, true, true, true}, + + // Edge cases + {"Empty types", "", []string{}, false, false, false}, + {"Unknown type", "Live", []string{}, false, false, false}, + } + + for _, tc := range testCases { + tc := tc // capture range variable + t.Run(tc.name, func(t *testing.T) { + // Create test release + release := database.ExternalRelease{ + Type: tc.releaseType, + SecondaryTypes: tc.secondaryTypes, + } + + // Test scanner filter + scannerFilter := TypeFilter{ + IgnoreSingles: tc.ignoreSingles, + IgnoreCompilations: tc.ignoreCompilations, + } + scannerSuppressed := scannerFilter.suppressed(release) + + // Test musicbrainz filter + mbFilter := musicbrainz.FilterOptions{ + IgnoreSingles: tc.ignoreSingles, + IgnoreCompilations: tc.ignoreCompilations, + } + mbFiltered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{release}, mbFilter) + mbSuppressed := len(mbFiltered) == 0 + + // Both should agree + if scannerSuppressed != mbSuppressed { + t.Errorf("Scanner and MusicBrainz filter disagree for %v: scanner=%v, musicbrainz=%v", + tc, scannerSuppressed, mbSuppressed) + } + + // Check against expected value + if scannerSuppressed != tc.expectedSuppressed { + t.Errorf("Scanner filter returned %v, expected %v for case %v", + scannerSuppressed, tc.expectedSuppressed, tc.name) + } + }) + } +} + +// Test that verifies the specific case mentioned in the issue: EP in SecondaryTypes counts as Single +func TestTypeFilterTreatsEPAsSingleWhenIgnoreSingles(t *testing.T) { + testCases := []struct { + name string + releaseType string + secondaryTypes []string + ignoreSingles bool + ignoreCompilations bool + expectedSuppressed bool + }{ + {"Album with EP secondary - should be suppressed when IgnoreSingles=true", "Album", []string{"EP"}, true, false, true}, + {"Album with EP secondary - should NOT be suppressed when IgnoreSingles=false", "Album", []string{"EP"}, false, false, false}, + {"Single with EP secondary - should be suppressed when IgnoreSingles=true", "Single", []string{"EP"}, true, false, true}, + {"Compilation with EP secondary - should be suppressed when IgnoreSingles=true (because EP in secondary counts as Single)", "Compilation", []string{"EP"}, true, false, true}, + } + + for _, tc := range testCases { + tc := tc // capture range variable + t.Run(tc.name, func(t *testing.T) { + // Create test release + release := database.ExternalRelease{ + Type: tc.releaseType, + SecondaryTypes: tc.secondaryTypes, + } + + // Test scanner filter + scannerFilter := TypeFilter{ + IgnoreSingles: tc.ignoreSingles, + IgnoreCompilations: tc.ignoreCompilations, + } + scannerSuppressed := scannerFilter.suppressed(release) + + // Test musicbrainz filter + mbFilter := musicbrainz.FilterOptions{ + IgnoreSingles: tc.ignoreSingles, + IgnoreCompilations: tc.ignoreCompilations, + } + mbFiltered := musicbrainz.ApplyTypeToggles([]database.ExternalRelease{release}, mbFilter) + mbSuppressed := len(mbFiltered) == 0 + + // Both should agree and match expected + if scannerSuppressed != mbSuppressed { + t.Errorf("Scanner and MusicBrainz filter disagree for %v: scanner=%v, musicbrainz=%v", + tc, scannerSuppressed, mbSuppressed) + } + + if scannerSuppressed != tc.expectedSuppressed { + t.Errorf("Filter returned %v, expected %v for case %v", + scannerSuppressed, tc.expectedSuppressed, tc.name) + } + }) + } +} \ No newline at end of file