fix: address code review findings
This commit is contained in:
@@ -44,12 +44,30 @@ func SyncArtistDiscography(
|
|||||||
return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err)
|
return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Step 2: If we have cached data, return it.
|
// Step 2: If we have cached data, return it. Re-apply the per-artist type
|
||||||
|
// toggles even on a cache hit so user changes to ignore_singles /
|
||||||
|
// ignore_compilations take effect without waiting for cache expiry.
|
||||||
|
// (Status/type inclusion was already applied when the rows were first
|
||||||
|
// synced and stored, so only the toggles can change.)
|
||||||
if len(cachedReleases) > 0 {
|
if len(cachedReleases) > 0 {
|
||||||
if err := ctx.Err(); err != nil {
|
if err := ctx.Err(); err != nil {
|
||||||
return nil, fmt.Errorf("sync artist discography: %w", err)
|
return nil, fmt.Errorf("sync artist discography: %w", err)
|
||||||
}
|
}
|
||||||
return cachedReleases, nil
|
opts, err := getArtistFilterOptions(db, artistID)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("sync artist discography: read artist filter options: %w", err)
|
||||||
|
}
|
||||||
|
filtered := cachedReleases[:0]
|
||||||
|
for _, r := range cachedReleases {
|
||||||
|
if opts.IgnoreSingles && r.Type == "Single" {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if opts.IgnoreCompilations && r.Type == "Compilation" {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
filtered = append(filtered, r)
|
||||||
|
}
|
||||||
|
return filtered, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Step 3: Cache miss — fetch from MusicBrainz API.
|
// Step 3: Cache miss — fetch from MusicBrainz API.
|
||||||
@@ -137,11 +155,12 @@ func SyncArtistDiscography(
|
|||||||
|
|
||||||
// getArtistFilterOptions reads per-artist type filtering preferences.
|
// getArtistFilterOptions reads per-artist type filtering preferences.
|
||||||
// Defaults to no filtering if artist_settings row doesn't exist.
|
// Defaults to no filtering if artist_settings row doesn't exist.
|
||||||
func getArtistFilterOptions(db *database.DB, artistMBID string) (FilterOptions, error) {
|
// artistID is the Navidrome artist ID (artist_settings.id), not the MusicBrainz ID.
|
||||||
|
func getArtistFilterOptions(db *database.DB, artistID string) (FilterOptions, error) {
|
||||||
var opts FilterOptions
|
var opts FilterOptions
|
||||||
err := db.Conn().QueryRow(
|
err := db.Conn().QueryRow(
|
||||||
"SELECT COALESCE(ignore_singles, 0), COALESCE(ignore_compilations, 0) FROM artist_settings WHERE id = ?",
|
"SELECT COALESCE(ignore_singles, 0), COALESCE(ignore_compilations, 0) FROM artist_settings WHERE id = ?",
|
||||||
artistMBID,
|
artistID,
|
||||||
).Scan(&opts.IgnoreSingles, &opts.IgnoreCompilations)
|
).Scan(&opts.IgnoreSingles, &opts.IgnoreCompilations)
|
||||||
if err == sql.ErrNoRows {
|
if err == sql.ErrNoRows {
|
||||||
return opts, nil
|
return opts, nil
|
||||||
|
|||||||
@@ -37,8 +37,13 @@ func NormalizeString(s string) string {
|
|||||||
// Remove parenthesized content (e.g., (Deluxe), (Remastered))
|
// Remove parenthesized content (e.g., (Deluxe), (Remastered))
|
||||||
s = parenRe.ReplaceAllString(s, "")
|
s = parenRe.ReplaceAllString(s, "")
|
||||||
|
|
||||||
// Remove years (4-digit numbers between 1000-2999)
|
// Remove years (4-digit numbers between 1000-2999). If stripping the
|
||||||
s = yearRe.ReplaceAllString(s, "")
|
// year would empty the entire string (e.g. an album literally titled
|
||||||
|
// "1989" or "2112"), keep the original form so the title can still match.
|
||||||
|
stripped := yearRe.ReplaceAllString(s, "")
|
||||||
|
if strings.TrimSpace(stripped) != "" {
|
||||||
|
s = stripped
|
||||||
|
}
|
||||||
|
|
||||||
// Replace common separators with spaces before stripping other special chars
|
// Replace common separators with spaces before stripping other special chars
|
||||||
s = strings.ReplaceAll(s, "-", " ")
|
s = strings.ReplaceAll(s, "-", " ")
|
||||||
|
|||||||
@@ -36,6 +36,9 @@ func TestNormalizeString_Basic(t *testing.T) {
|
|||||||
// Digits that are not years should stay
|
// Digits that are not years should stay
|
||||||
{"30 Seconds to Mars", "30 seconds to mars"},
|
{"30 Seconds to Mars", "30 seconds to mars"},
|
||||||
{"1941 - The Greatest Hits", "the greatest hits"},
|
{"1941 - The Greatest Hits", "the greatest hits"},
|
||||||
|
// Year-only title is preserved (not collapsed to empty) so it can still match
|
||||||
|
{"1989", "1989"},
|
||||||
|
{"2112", "2112"},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
|
|||||||
Reference in New Issue
Block a user