From a106dc67d175bc88dbeeaebfe330181ae233f8dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 20 May 2025 21:53:02 -0400 Subject: [PATCH] feat: hide missing artists from regular users and Subsonic API (#4092) * Handle missing artists for non-admin users * feat(artist): enhance ArtistList with missing row styling and class management Signed-off-by: Deluan --------- Signed-off-by: Deluan --- model/artist.go | 4 +- persistence/artist_repository.go | 30 ++++++- persistence/artist_repository_test.go | 95 +++++++++++++++++++++- persistence/persistence.go | 1 + server/subsonic/browsing.go | 2 +- ui/src/artist/ArtistList.jsx | 14 +++- ui/src/dataProvider/wrapperDataProvider.js | 3 +- 7 files changed, 140 insertions(+), 9 deletions(-) diff --git a/model/artist.go b/model/artist.go index 9c83150bd..68836ff28 100644 --- a/model/artist.go +++ b/model/artist.go @@ -32,6 +32,8 @@ type Artist struct { SimilarArtists Artists `structs:"similar_artists" json:"-"` ExternalInfoUpdatedAt *time.Time `structs:"external_info_updated_at" json:"externalInfoUpdatedAt,omitempty"` + Missing bool `structs:"missing" json:"missing"` + CreatedAt *time.Time `structs:"created_at" json:"createdAt,omitempty"` UpdatedAt *time.Time `structs:"updated_at" json:"updatedAt,omitempty"` } @@ -76,7 +78,7 @@ type ArtistRepository interface { UpdateExternalInfo(a *Artist) error Get(id string) (*Artist, error) GetAll(options ...QueryOptions) (Artists, error) - GetIndex(roles ...Role) (ArtistIndexes, error) + GetIndex(includeMissing bool, roles ...Role) (ArtistIndexes, error) // The following methods are used exclusively by the scanner: RefreshPlayCounts() (int64, error) diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index ecb8f8bf6..b6b1aba44 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -116,6 +116,7 @@ func NewArtistRepository(ctx context.Context, db dbx.Builder) model.ArtistReposi "name": fullTextFilter(r.tableName), "starred": booleanFilter, "role": roleFilter, + "missing": booleanFilter, }) r.setSortMappings(map[string]string{ "name": "order_artist_name", @@ -202,7 +203,7 @@ func (r *artistRepository) getIndexKey(a model.Artist) string { } // TODO Cache the index (recalculate when there are changes to the DB) -func (r *artistRepository) GetIndex(roles ...model.Role) (model.ArtistIndexes, error) { +func (r *artistRepository) GetIndex(includeMissing bool, roles ...model.Role) (model.ArtistIndexes, error) { options := model.QueryOptions{Sort: "name"} if len(roles) > 0 { roleFilters := slice.Map(roles, func(r model.Role) Sqlizer { @@ -210,6 +211,13 @@ func (r *artistRepository) GetIndex(roles ...model.Role) (model.ArtistIndexes, e }) options.Filters = And(roleFilters) } + if !includeMissing { + if options.Filters == nil { + options.Filters = Eq{"artist.missing": false} + } else { + options.Filters = And{options.Filters, Eq{"artist.missing": false}} + } + } artists, err := r.GetAll(options) if err != nil { return nil, err @@ -236,6 +244,26 @@ func (r *artistRepository) purgeEmpty() error { return nil } +// markMissing sets the Missing flag based on album data. +func (r *artistRepository) markMissing() (int64, error) { + q := Expr(` + update artist + set missing = not exists ( + select 1 from album_artists aa + join album a on aa.album_id = a.id + where aa.artist_id = artist.id and a.missing = false + ) + `) + c, err := r.executeSQL(q) + if err != nil { + return 0, fmt.Errorf("marking missing artists: %w", err) + } + if c > 0 { + log.Debug(r.ctx, "Marked missing artists", "totalUpdated", c) + } + return c, nil +} + // RefreshPlayCounts updates the play count and last play date annotations for all artists, based // on the media files associated with them. func (r *artistRepository) RefreshPlayCounts() (int64, error) { diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index f9e58d216..0c7018dc8 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" + "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" @@ -94,7 +95,7 @@ var _ = Describe("ArtistRepository", func() { er := repo.Put(&artistBeatles) Expect(er).To(BeNil()) - idx, err := repo.GetIndex() + idx, err := repo.GetIndex(false) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("F")) @@ -112,7 +113,7 @@ var _ = Describe("ArtistRepository", func() { // BFR Empty SortArtistName is not saved in the DB anymore XIt("returns the index when PreferSortTags is true and SortArtistName is empty", func() { - idx, err := repo.GetIndex() + idx, err := repo.GetIndex(false) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -134,7 +135,7 @@ var _ = Describe("ArtistRepository", func() { er := repo.Put(&artistBeatles) Expect(er).To(BeNil()) - idx, err := repo.GetIndex() + idx, err := repo.GetIndex(false) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -151,7 +152,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns the index when SortArtistName is empty", func() { - idx, err := repo.GetIndex() + idx, err := repo.GetIndex(false) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -233,5 +234,91 @@ var _ = Describe("ArtistRepository", func() { Expect(m).ToNot(HaveKey("mbz_artist_id")) }) }) + + Describe("Missing artist visibility", func() { + var raw *artistRepository + var missing model.Artist + + insertMissing := func() { + missing = model.Artist{ID: "m1", Name: "Missing", OrderArtistName: "missing"} + Expect(repo.Put(&missing)).To(Succeed()) + raw = repo.(*artistRepository) + _, err := raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missing.ID})) + Expect(err).ToNot(HaveOccurred()) + } + + removeMissing := func() { + if raw != nil { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missing.ID})) + } + } + + Context("regular user", func() { + BeforeEach(func() { + ctx := log.NewContext(context.TODO()) + ctx = request.WithUser(ctx, model.User{ID: "u1"}) + repo = NewArtistRepository(ctx, GetDBXBuilder()) + insertMissing() + }) + + AfterEach(func() { removeMissing() }) + + It("does not return missing artist in GetAll", func() { + artists, err := repo.GetAll(model.QueryOptions{Filters: squirrel.Eq{"artist.missing": false}}) + Expect(err).ToNot(HaveOccurred()) + Expect(artists).To(HaveLen(2)) + }) + + It("does not return missing artist in Search", func() { + res, err := repo.Search("missing", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(res).To(BeEmpty()) + }) + + It("does not return missing artist in GetIndex", func() { + idx, err := repo.GetIndex(false) + Expect(err).ToNot(HaveOccurred()) + // Only 2 artists should be present + total := 0 + for _, ix := range idx { + total += len(ix.Artists) + } + Expect(total).To(Equal(2)) + }) + }) + + Context("admin user", func() { + BeforeEach(func() { + ctx := log.NewContext(context.TODO()) + ctx = request.WithUser(ctx, model.User{ID: "admin", IsAdmin: true}) + repo = NewArtistRepository(ctx, GetDBXBuilder()) + insertMissing() + }) + + AfterEach(func() { removeMissing() }) + + It("returns missing artist in GetAll", func() { + artists, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(artists).To(HaveLen(3)) + }) + + It("returns missing artist in Search", func() { + res, err := repo.Search("missing", 0, 10, true) + Expect(err).ToNot(HaveOccurred()) + Expect(res).To(HaveLen(1)) + }) + + It("returns missing artist in GetIndex when included", func() { + idx, err := repo.GetIndex(true) + Expect(err).ToNot(HaveOccurred()) + total := 0 + for _, ix := range idx { + total += len(ix.Artists) + } + Expect(total).To(Equal(3)) + }) + }) + }) }) }) diff --git a/persistence/persistence.go b/persistence/persistence.go index 579f13707..31e03db61 100644 --- a/persistence/persistence.go +++ b/persistence/persistence.go @@ -170,6 +170,7 @@ func (s *SQLStore) GC(ctx context.Context) error { err := chain.RunSequentially( trace(ctx, "purge empty albums", func() error { return s.Album(ctx).(*albumRepository).purgeEmpty() }), trace(ctx, "purge empty artists", func() error { return s.Artist(ctx).(*artistRepository).purgeEmpty() }), + trace(ctx, "mark missing artists", func() error { _, err := s.Artist(ctx).(*artistRepository).markMissing(); return err }), trace(ctx, "purge empty folders", func() error { return s.Folder(ctx).(*folderRepository).purgeEmpty() }), trace(ctx, "clean album annotations", func() error { return s.Album(ctx).(*albumRepository).cleanAnnotations() }), trace(ctx, "clean artist annotations", func() error { return s.Artist(ctx).(*artistRepository).cleanAnnotations() }), diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index c00a9f1ab..76023c862 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -38,7 +38,7 @@ func (api *Router) getArtist(r *http.Request, libId int, ifModifiedSince time.Ti var indexes model.ArtistIndexes if lib.LastScanAt.After(ifModifiedSince) { - indexes, err = api.ds.Artist(ctx).GetIndex(model.RoleAlbumArtist) + indexes, err = api.ds.Artist(ctx).GetIndex(false, model.RoleAlbumArtist) if err != nil { log.Error(ctx, "Error retrieving Indexes", err) return nil, 0, err diff --git a/ui/src/artist/ArtistList.jsx b/ui/src/artist/ArtistList.jsx index d3fc4ceee..f26cff217 100644 --- a/ui/src/artist/ArtistList.jsx +++ b/ui/src/artist/ArtistList.jsx @@ -17,6 +17,7 @@ import FavoriteIcon from '@material-ui/icons/Favorite' import FavoriteBorderIcon from '@material-ui/icons/FavoriteBorder' import { makeStyles } from '@material-ui/core/styles' import { useDrag } from 'react-dnd' +import clsx from 'clsx' import { ArtistContextMenu, List, @@ -49,6 +50,9 @@ const useStyles = makeStyles({ }, }, }, + missingRow: { + opacity: 0.3, + }, contextMenu: { visibility: 'hidden', }, @@ -95,7 +99,15 @@ const ArtistDatagridRow = (props) => { }), [record], ) - return + const classes = useStyles() + const computedClasses = clsx( + props.className, + classes.row, + record?.missing && classes.missingRow, + ) + return ( + + ) } const ArtistDatagridBody = (props) => ( diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index 1e3321255..f387136cb 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -23,7 +23,8 @@ const mapResource = (resource, params) => { return [`playlist/${plsId}/tracks`, params] } case 'album': - case 'song': { + case 'song': + case 'artist': { if (params.filter && !isAdmin()) { params.filter.missing = false }