From 752b38609c1599e15dc4fb3eef55fbb6f340f516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 12 Aug 2026 11:59:56 -0400 Subject: [PATCH] feat(artist): add Share and Download actions to the Artist detail page (#5944) * feat(ui): add Share button to artist detail page * feat(ui): add Download button to artist detail page * fix(ui): scope artist share/download to album-artist content Gate the artist Share/Download actions on album-artist stats and show the album-artist size, since ZipArtist and the share query only cover album_artist_id songs. Previously the total (role-inclusive) size was shown and guest-only artists could produce an empty archive. Applies to the artist toolbar, the shared context menu, and the download dialog title. * fix: match artist download/share to album-artist participation ZipArtist and the artist share query filtered the deprecated album_artist_id column, which only stores the first album artist of a track. Secondary album-artists (co-credited but not first) got an empty download/share even though the UI offered it. Filter by the album-artist role participation instead, matching the artist's album-artist stats used to gate the actions. Also cover the artist-specific size branch of the download dialog. * fix(share): scope artist shares to the owner's libraries The artist share query broadened to album-artist participation, which could pull a secondary album artist's tracks from libraries the (non-admin) share owner cannot access into the public share. Load the artist share as the owner so their library access is applied, mirroring how playlist shares already work. Adds a repository test covering co-album-artist inclusion and library scoping. * test(share): assert album participation branch of artist shares Link the co-album-artist fixtures to albums and assert share.Albums (used by Subsonic getShares) includes the accessible album and excludes the one in a library the owner cannot access, so the album participation + scoping branch is covered too. * fix: exclude missing files from artist download/share actions An artist's stats still count files that went missing, so the toolbar/context menu could offer Download/Share for an artist whose files are all gone, while the share query (missing=false) returns nothing and downloads open dead paths. Hide the actions when the artist is missing and exclude missing files from ZipArtist, matching the share semantics. * refactor: dedupe artist download-size and share-owner lookups Extract the 'album-artist download size (or none when missing)' rule into a single artistDownloadSize() helper shared by the toolbar, context menu, and download dialog, and factor the duplicated share-owner context lookup into a shareRepository.ownerContext() method used by both the artist and playlist share cases. * refactor(ui): move artistDownloadSize helper to common utils is for domain-agnostic, potentially portable code; this helper is Navidrome-specific (artist stats shape), so it belongs in common. Consumers import it directly from common/artist to avoid pulling in the common barrel. --- core/archiver.go | 9 ++- core/archiver_test.go | 8 ++- persistence/share_repository.go | 36 +++++++--- persistence/share_repository_test.go | 83 ++++++++++++++++++++++ ui/src/artist/ArtistActions.jsx | 41 +++++++++++ ui/src/artist/ArtistActions.test.jsx | 65 ++++++++++++++++- ui/src/common/ContextMenus.jsx | 10 ++- ui/src/common/ContextMenus.test.jsx | 78 ++++++++++++++++++++ ui/src/common/artist.js | 6 ++ ui/src/common/artist.test.js | 24 +++++++ ui/src/common/index.js | 1 + ui/src/dialogs/DownloadMenuDialog.jsx | 11 ++- ui/src/dialogs/DownloadMenuDialog.test.jsx | 58 +++++++++++++++ 13 files changed, 411 insertions(+), 19 deletions(-) create mode 100644 ui/src/common/ContextMenus.test.jsx create mode 100644 ui/src/common/artist.js create mode 100644 ui/src/common/artist.test.js create mode 100644 ui/src/dialogs/DownloadMenuDialog.test.jsx diff --git a/core/archiver.go b/core/archiver.go index 8c42f8f49..c9436279e 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/persistence" "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" ) @@ -40,7 +41,13 @@ func (a *archiver) ZipAlbum(ctx context.Context, id string, format string, bitra } func (a *archiver) ZipArtist(ctx context.Context, id string, format string, bitrate int, out io.Writer) error { - return a.zipAlbums(ctx, id, format, bitrate, out, squirrel.Eq{"album_artist_id": id}) + // Match by album-artist participation, not the deprecated album_artist_id + // column (first album artist only), so co-album-artists are included too. + filter := squirrel.And{ + persistence.ParticipantIDFilter("media_file", id, model.RoleAlbumArtist), + squirrel.Eq{"missing": false}, + } + return a.zipAlbums(ctx, id, format, bitrate, out, filter) } func (a *archiver) zipAlbums(ctx context.Context, id string, format string, bitrate int, out io.Writer, filters squirrel.Sqlizer) error { diff --git a/core/archiver_test.go b/core/archiver_test.go index 2ba8f1fc0..461af1800 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -11,6 +11,7 @@ import ( "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/persistence" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/mock" @@ -69,8 +70,11 @@ var _ = Describe("Archiver", func() { mfRepo := &mockMediaFileRepository{} mfRepo.On("GetAll", []model.QueryOptions{{ - Filters: squirrel.Eq{"album_artist_id": "1"}, - Sort: "album", + Filters: squirrel.And{ + persistence.ParticipantIDFilter("media_file", "1", model.RoleAlbumArtist), + squirrel.Eq{"missing": false}, + }, + Sort: "album", }}).Return(mfs, nil) ds.On("MediaFile", mock.Anything).Return(mfRepo) diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 0013e782b..4b6dc9240 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -82,13 +82,20 @@ func (r *shareRepository) loadMedia(share *model.Share) error { } switch share.ResourceType { case "artist": - albumRepo := NewAlbumRepository(r.ctx, r.db) - share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_artist_id": ids}), Sort: "artist"}) + // Match by album-artist participation, not the deprecated album_artist_id + // column (first album artist only), so co-album-artists are included too. + // Load as the share owner so their library access is applied. + ctx, err := r.ownerContext(share) if err != nil { return err } - mfRepo := NewMediaFileRepository(r.ctx, r.db) - share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_artist_id": ids}), Sort: "artist"}) + albumRepo := NewAlbumRepository(ctx, r.db) + share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(ParticipantIDFilter("album", ids, model.RoleAlbumArtist)), Sort: "artist"}) + if err != nil { + return err + } + mfRepo := NewMediaFileRepository(ctx, r.db) + share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(ParticipantIDFilter("media_file", ids, model.RoleAlbumArtist)), Sort: "artist"}) return err case "album": albumRepo := NewAlbumRepository(r.ctx, r.db) @@ -101,14 +108,10 @@ func (r *shareRepository) loadMedia(share *model.Share) error { return err case "playlist": // Load tracks as the share owner so their library access is applied. - owner, err := NewUserRepository(r.ctx, r.db).Get(share.UserID) + ctx, err := r.ownerContext(share) if err != nil { - return fmt.Errorf("loading share owner %q: %w", share.UserID, err) + return err } - if owner == nil { - return fmt.Errorf("share owner %q not found", share.UserID) - } - ctx := request.WithUser(r.ctx, *owner) plsRepo := NewPlaylistRepository(ctx, r.db) // Tracks returns nil when the playlist is no longer visible to the owner // (e.g. it was made private after the share was created); leave the share @@ -133,6 +136,19 @@ func (r *shareRepository) loadMedia(share *model.Share) error { return nil } +// ownerContext returns a context scoped to the share owner, so repository +// queries apply the owner's library access when a public share is rendered. +func (r *shareRepository) ownerContext(share *model.Share) (context.Context, error) { + owner, err := NewUserRepository(r.ctx, r.db).Get(share.UserID) + if err != nil { + return nil, fmt.Errorf("loading share owner %q: %w", share.UserID, err) + } + if owner == nil { + return nil, fmt.Errorf("share owner %q not found", share.UserID) + } + return request.WithUser(r.ctx, *owner), nil +} + func sortByIdPosition(mfs model.MediaFiles, ids []string) model.MediaFiles { m := map[string]int{} for i, mf := range mfs { diff --git a/persistence/share_repository_test.go b/persistence/share_repository_test.go index 3ae456031..3af91b2af 100644 --- a/persistence/share_repository_test.go +++ b/persistence/share_repository_test.go @@ -228,6 +228,89 @@ var _ = Describe("ShareRepository", func() { }) }) + Describe("Artist share library scoping", func() { + var otherLib model.Library + var owner model.User + const primaryID = "share-aa-primary" + const secondaryID = "share-aa-secondary" + + BeforeEach(func() { + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + b := GetDBXBuilder() + + // A second library the owner has no access to + lr := NewLibraryRepository(adminCtx, b) + otherLib = model.Library{ID: 0, Name: "Artist Share Other Library", Path: "/share/artist/other"} + Expect(lr.Put(&otherLib)).To(Succeed()) + + ar := NewArtistRepository(adminCtx, b) + Expect(createArtistWithLibrary(ar, &model.Artist{ID: primaryID, Name: "AA Primary", OrderArtistName: "aa primary"}, 1)).To(Succeed()) + Expect(createArtistWithLibrary(ar, &model.Artist{ID: secondaryID, Name: "AA Secondary", OrderArtistName: "aa secondary"}, 1)).To(Succeed()) + + // Secondary is a co-album-artist (not the first): album_artist_id points at + // primary, so the legacy-column filter would miss both tracks. + aaParticipants := model.Participants{model.RoleAlbumArtist: { + {Artist: model.Artist{ID: primaryID, Name: "AA Primary"}}, + {Artist: model.Artist{ID: secondaryID, Name: "AA Secondary"}}, + }} + alr := NewAlbumRepository(adminCtx, b) + Expect(alr.Put(&model.Album{ID: "art-album-ok", LibraryID: 1, Name: "Art Album OK", AlbumArtistID: primaryID, AlbumArtist: "AA Primary", Participants: aaParticipants})).To(Succeed()) + Expect(alr.Put(&model.Album{ID: "art-album-other", LibraryID: otherLib.ID, Name: "Art Album Other", AlbumArtistID: primaryID, AlbumArtist: "AA Primary", Participants: aaParticipants})).To(Succeed()) + + mr := NewMediaFileRepository(adminCtx, b) + Expect(mr.Put(&model.MediaFile{ID: "art-ok", LibraryID: 1, AlbumID: "art-album-ok", Path: "a/ok.mp3", Title: "ArtOK", AlbumArtistID: primaryID, Participants: aaParticipants})).To(Succeed()) + Expect(mr.Put(&model.MediaFile{ID: "art-other", LibraryID: otherLib.ID, AlbumID: "art-album-other", Path: "a/other.mp3", Title: "ArtOther", AlbumArtistID: primaryID, Participants: aaParticipants})).To(Succeed()) + + // Non-admin owner with access to library 1 only + owner = createUserWithLibraries("artist-share-owner", []int{1}) + ur := NewUserRepository(adminCtx, b) + Expect(ur.Put(&owner)).To(Succeed()) + Expect(ur.SetUserLibraries(owner.ID, []int{1})).To(Succeed()) + + _, err := b.NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]any{ + "id": "art-share", "user": owner.ID, "desc": "Artist scope share", + "type": "artist", "ids": secondaryID, "created": time.Now(), "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + b := GetDBXBuilder() + _, _ = b.NewQuery(`DELETE FROM share WHERE id = 'art-share'`).Execute() + mr := NewMediaFileRepository(adminCtx, b).(*mediaFileRepository) + _, _ = mr.executeSQL(squirrel.Delete("media_file").Where(squirrel.Eq{"id": []string{"art-ok", "art-other"}})) + alr := NewAlbumRepository(adminCtx, b).(*albumRepository) + _, _ = alr.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": []string{"art-album-ok", "art-album-other"}})) + ar := NewArtistRepository(adminCtx, b).(*artistRepository) + _, _ = ar.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": []string{primaryID, secondaryID}})) + lr := NewLibraryRepository(adminCtx, b).(*libraryRepository) + _ = lr.delete(squirrel.Eq{"id": otherLib.ID}) + _ = NewUserRepository(adminCtx, b).Delete(owner.ID) + }) + + It("includes co-album-artist tracks the owner can access and excludes those they cannot", func() { + // Read as admin (mimics the public-share render path); loadMedia must still + // scope to the owner's libraries. + adminRepo := NewShareRepository(request.WithUser(log.NewContext(GinkgoT().Context()), adminUser), GetDBXBuilder()) + share, err := adminRepo.Get("art-share") + Expect(err).ToNot(HaveOccurred()) + + Expect(share.Tracks).To(ContainElement(HaveField("ID", "art-ok")), + "a co-album-artist track (not matched by album_artist_id) must be included") + Expect(share.Tracks).ToNot(ContainElement(HaveField("ID", "art-other")), + "a track outside the owner's libraries must not appear in the share") + + Expect(share.Albums).To(ContainElement(HaveField("ID", "art-album-ok")), + "a co-album-artist album must be included") + Expect(share.Albums).ToNot(ContainElement(HaveField("ID", "art-album-other")), + "an album outside the owner's libraries must not appear in the share") + }) + }) + Describe("Ownership Checks", func() { var ownerUser = model.User{ID: "2222", UserName: "regular-user"} var otherUser = model.User{ID: "3333", UserName: "third-user"} diff --git a/ui/src/artist/ArtistActions.jsx b/ui/src/artist/ArtistActions.jsx index 0b48f232d..2f329a034 100644 --- a/ui/src/artist/ArtistActions.jsx +++ b/ui/src/artist/ArtistActions.jsx @@ -13,9 +13,19 @@ import { } from 'react-admin' import ShuffleIcon from '@material-ui/icons/Shuffle' import PlayArrowIcon from '@material-ui/icons/PlayArrow' +import ShareIcon from '@material-ui/icons/Share' +import CloudDownloadOutlinedIcon from '@material-ui/icons/CloudDownloadOutlined' import { IoIosRadio } from 'react-icons/io' import { playShuffle, playTopSongs } from './actions.js' import { playSimilar } from '../common/playbackActions.js' +import { + openShareMenu, + openDownloadMenu, + DOWNLOAD_MENU_ARTIST, +} from '../actions' +import config from '../config' +import { formatBytes } from '../utils' +import { artistDownloadSize } from '../common/artist' const useStyles = makeStyles((theme) => ({ toolbar: { @@ -62,6 +72,9 @@ const ArtistActions = ({ className, record, ...rest }) => { const [loadingAction, setLoadingAction] = React.useState(null) const isLoading = !!loadingAction + const albumArtistSize = artistDownloadSize(record) + const hasAlbumArtistContent = Boolean(albumArtistSize) + const handlePlay = React.useCallback(async () => { setLoadingAction('play') try { @@ -101,6 +114,14 @@ const ArtistActions = ({ className, record, ...rest }) => { } }, [dispatch, notify, record]) + const handleShare = React.useCallback(() => { + dispatch(openShareMenu([record.id], 'artist', record.name)) + }, [dispatch, record]) + + const handleDownload = React.useCallback(() => { + dispatch(openDownloadMenu(record, DOWNLOAD_MENU_ARTIST)) + }, [dispatch, record]) + return ( { loading={loadingAction === 'radio'} icon={} /> + {config.enableSharing && hasAlbumArtistContent && ( + } + /> + )} + {config.enableDownloads && hasAlbumArtistContent && ( + } + /> + )} ) } diff --git a/ui/src/artist/ArtistActions.test.jsx b/ui/src/artist/ArtistActions.test.jsx index a11ee50e3..ad25177b3 100644 --- a/ui/src/artist/ArtistActions.test.jsx +++ b/ui/src/artist/ArtistActions.test.jsx @@ -4,6 +4,11 @@ import { TestContext } from 'ra-test' import { describe, it, expect, vi, beforeEach } from 'vitest' import ArtistActions from './ArtistActions' import subsonic from '../subsonic' +import { + openShareMenu, + openDownloadMenu, + DOWNLOAD_MENU_ARTIST, +} from '../actions' import { ThemeProvider, createTheme } from '@material-ui/core/styles' const mockDispatch = vi.fn() @@ -13,6 +18,11 @@ vi.mock('../subsonic', () => ({ default: { getSimilarSongs2: vi.fn(), getTopSongs: vi.fn() }, })) +const { mockConfig } = vi.hoisted(() => ({ + mockConfig: { enableSharing: true, enableDownloads: true }, +})) +vi.mock('../config', () => ({ default: mockConfig })) + const mockNotify = vi.fn() const mockGetList = vi.fn().mockResolvedValue({ data: [{ id: 's1' }] }) @@ -27,7 +37,11 @@ vi.mock('react-admin', async (importOriginal) => { }) describe('ArtistActions', () => { - const defaultRecord = { id: 'ar1', name: 'Artist' } + const defaultRecord = { + id: 'ar1', + name: 'Artist', + stats: { albumartist: { songCount: 3, albumCount: 1, size: 1024 } }, + } const renderArtistActions = (record = defaultRecord) => { const theme = createTheme() @@ -48,6 +62,8 @@ describe('ArtistActions', () => { vi.clearAllMocks() // Mock console.error to suppress error logging in tests vi.spyOn(console, 'error').mockImplementation(() => {}) + mockConfig.enableSharing = true + mockConfig.enableDownloads = true const songWithReplayGain = { id: 'rec1', @@ -227,4 +243,51 @@ describe('ArtistActions', () => { expect(mockDispatch).not.toHaveBeenCalled() }) }) + + describe('Share action', () => { + it('shows the share button and dispatches openShareMenu when clicked', () => { + renderArtistActions() + fireEvent.click(screen.getByText('ra.action.share')) + expect(mockDispatch).toHaveBeenCalledWith( + openShareMenu(['ar1'], 'artist', 'Artist'), + ) + }) + + it('hides the share button when sharing is disabled', () => { + mockConfig.enableSharing = false + renderArtistActions() + expect(screen.queryByText('ra.action.share')).not.toBeInTheDocument() + }) + }) + + describe('Download action', () => { + it('shows the download button with album-artist size and dispatches openDownloadMenu when clicked', () => { + renderArtistActions() + expect(screen.getByText('ra.action.download (1 KB)')).toBeInTheDocument() + fireEvent.click(screen.getByText(/ra\.action\.download/)) + expect(mockDispatch).toHaveBeenCalledWith( + openDownloadMenu(defaultRecord, DOWNLOAD_MENU_ARTIST), + ) + }) + + it('hides the download button when downloads are disabled', () => { + mockConfig.enableDownloads = false + renderArtistActions() + expect(screen.queryByText(/ra\.action\.download/)).not.toBeInTheDocument() + }) + }) + + describe('Album-artist gating', () => { + it('hides Share and Download for artists with no album-artist content', () => { + renderArtistActions({ id: 'ar1', name: 'Artist', stats: {} }) + expect(screen.queryByText('ra.action.share')).not.toBeInTheDocument() + expect(screen.queryByText(/ra\.action\.download/)).not.toBeInTheDocument() + }) + + it('hides Share and Download for a missing artist', () => { + renderArtistActions({ ...defaultRecord, missing: true }) + expect(screen.queryByText('ra.action.share')).not.toBeInTheDocument() + expect(screen.queryByText(/ra\.action\.download/)).not.toBeInTheDocument() + }) + }) }) diff --git a/ui/src/common/ContextMenus.jsx b/ui/src/common/ContextMenus.jsx index 47c9c6786..7ad8c735c 100644 --- a/ui/src/common/ContextMenus.jsx +++ b/ui/src/common/ContextMenus.jsx @@ -24,6 +24,7 @@ import { import { LoveButton } from './LoveButton' import config from '../config' import { formatBytes } from '../utils' +import { artistDownloadSize } from './artist' const useStyles = makeStyles({ noWrap: { @@ -70,6 +71,9 @@ const ContextMenu = ({ const notify = useNotify() const [anchorEl, setAnchorEl] = useState(null) + const isArtist = resource === 'artist' + const downloadSize = isArtist ? artistDownloadSize(record) : record?.size + const options = { play: { enabled: true, @@ -103,7 +107,7 @@ const ContextMenu = ({ }, ...(!hideShare && { share: { - enabled: config.enableSharing, + enabled: config.enableSharing && (!isArtist || downloadSize), needData: false, label: translate('ra.action.share'), action: (record) => @@ -111,9 +115,9 @@ const ContextMenu = ({ }, }), download: { - enabled: config.enableDownloads && record.size, + enabled: config.enableDownloads && downloadSize, needData: false, - label: `${translate('ra.action.download')} (${formatBytes(record.size)})`, + label: `${translate('ra.action.download')} (${formatBytes(downloadSize)})`, action: () => { dispatch( openDownloadMenu( diff --git a/ui/src/common/ContextMenus.test.jsx b/ui/src/common/ContextMenus.test.jsx new file mode 100644 index 000000000..72a98b64b --- /dev/null +++ b/ui/src/common/ContextMenus.test.jsx @@ -0,0 +1,78 @@ +import React from 'react' +import { render, fireEvent, screen } from '@testing-library/react' +import { TestContext } from 'ra-test' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { ThemeProvider, createTheme } from '@material-ui/core/styles' +import { AlbumContextMenu, ArtistContextMenu } from './ContextMenus' + +const mockDispatch = vi.fn() +vi.mock('react-redux', () => ({ useDispatch: () => mockDispatch })) + +const { mockConfig } = vi.hoisted(() => ({ + mockConfig: { + enableSharing: true, + enableDownloads: true, + enableFavourites: false, + }, +})) +vi.mock('../config', () => ({ default: mockConfig })) + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useNotify: () => vi.fn(), + useDataProvider: () => ({ getList: vi.fn() }), + useTranslate: () => (x) => x, + } +}) + +describe('ContextMenus', () => { + const renderMenu = (Menu, record) => { + render( + + + + + , + ) + fireEvent.click(screen.getByLabelText('more')) + } + + beforeEach(() => { + vi.clearAllMocks() + mockConfig.enableSharing = true + mockConfig.enableDownloads = true + }) + + describe('ArtistContextMenu', () => { + const withAlbumArtist = { + id: 'ar1', + name: 'Artist', + stats: { albumartist: { songCount: 3, albumCount: 1, size: 1024 } }, + } + + it('shows the album-artist size on the download item', () => { + renderMenu(ArtistContextMenu, withAlbumArtist) + expect(screen.getByText('ra.action.download (1 KB)')).toBeInTheDocument() + }) + + it('hides download and share for artists with no album-artist content', () => { + renderMenu(ArtistContextMenu, { id: 'ar1', name: 'Artist', stats: {} }) + expect(screen.queryByText(/ra\.action\.download/)).not.toBeInTheDocument() + expect(screen.queryByText('ra.action.share')).not.toBeInTheDocument() + }) + }) + + describe('AlbumContextMenu', () => { + it('uses the total size on the album download item', () => { + renderMenu(AlbumContextMenu, { + id: 'al1', + name: 'Album', + duration: 100, + size: 1024 * 1024, + }) + expect(screen.getByText('ra.action.download (1 MB)')).toBeInTheDocument() + }) + }) +}) diff --git a/ui/src/common/artist.js b/ui/src/common/artist.js new file mode 100644 index 000000000..370700160 --- /dev/null +++ b/ui/src/common/artist.js @@ -0,0 +1,6 @@ +// Size of an artist's downloadable album-artist content, or undefined when there +// is nothing to download (a missing artist, or no album-artist songs). Download +// and Share only cover album-artist songs, so callers gate on this, not the +// role-inclusive total. +export const artistDownloadSize = (record) => + record?.missing ? undefined : record?.stats?.albumartist?.size diff --git a/ui/src/common/artist.test.js b/ui/src/common/artist.test.js new file mode 100644 index 000000000..eff9935a6 --- /dev/null +++ b/ui/src/common/artist.test.js @@ -0,0 +1,24 @@ +import { describe, it, expect } from 'vitest' +import { artistDownloadSize } from './artist' + +describe('artistDownloadSize', () => { + it('returns the album-artist size', () => { + expect( + artistDownloadSize({ stats: { albumartist: { size: 1024 } } }), + ).toEqual(1024) + }) + + it('returns undefined for a missing artist', () => { + expect( + artistDownloadSize({ + missing: true, + stats: { albumartist: { size: 1024 } }, + }), + ).toBeUndefined() + }) + + it('returns undefined when there is no album-artist content', () => { + expect(artistDownloadSize({ stats: {} })).toBeUndefined() + expect(artistDownloadSize(undefined)).toBeUndefined() + }) +}) diff --git a/ui/src/common/index.js b/ui/src/common/index.js index 7b5b376f3..047f5b3cf 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -1,4 +1,5 @@ export * from './AddToPlaylistButton' +export * from './artist' export * from './ArtistLinkField' export * from './BatchPlayButton' export * from './BitrateField' diff --git a/ui/src/dialogs/DownloadMenuDialog.jsx b/ui/src/dialogs/DownloadMenuDialog.jsx index 2104cbcad..61e84a08a 100644 --- a/ui/src/dialogs/DownloadMenuDialog.jsx +++ b/ui/src/dialogs/DownloadMenuDialog.jsx @@ -8,8 +8,9 @@ import { DialogTitle, } from '@material-ui/core' import subsonic from '../subsonic' -import { closeDownloadMenu } from '../actions' +import { closeDownloadMenu, DOWNLOAD_MENU_ARTIST } from '../actions' import { formatBytes } from '../utils' +import { artistDownloadSize } from '../common/artist' import { useTranscodingOptions } from './useTranscodingOptions' const DownloadMenuDialog = () => { @@ -22,6 +23,12 @@ const DownloadMenuDialog = () => { const { TranscodingOptionsInput, format, maxBitRate, originalFormat } = useTranscodingOptions() + // Artist downloads only include album-artist songs, so show that size + const downloadSize = + recordType === DOWNLOAD_MENU_ARTIST + ? artistDownloadSize(record) + : record?.size + const handleClose = (e) => { dispatch(closeDownloadMenu()) e.stopPropagation() @@ -55,7 +62,7 @@ const DownloadMenuDialog = () => { smart_count: 1, }).toLocaleLowerCase(), name: record?.name || record?.title, - size: formatBytes(record?.size), + size: formatBytes(downloadSize), })} diff --git a/ui/src/dialogs/DownloadMenuDialog.test.jsx b/ui/src/dialogs/DownloadMenuDialog.test.jsx new file mode 100644 index 000000000..b9ce7f077 --- /dev/null +++ b/ui/src/dialogs/DownloadMenuDialog.test.jsx @@ -0,0 +1,58 @@ +import * as React from 'react' +import { TestContext } from 'ra-test' +import { render, screen, cleanup } from '@testing-library/react' +import { describe, afterEach, it, expect, vi } from 'vitest' +import DownloadMenuDialog from './DownloadMenuDialog' +import { DOWNLOAD_MENU_ALBUM, DOWNLOAD_MENU_ARTIST } from '../actions' + +vi.mock('./useTranscodingOptions', () => ({ + useTranscodingOptions: () => ({ + TranscodingOptionsInput: () => null, + format: '', + maxBitRate: 0, + originalFormat: true, + }), +})) + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useTranslate: () => (key, opts) => + opts?.size ? `${key}:${opts.name}:${opts.size}` : key, + } +}) + +const renderDialog = (record, recordType) => + render( + + + , + ) + +describe('DownloadMenuDialog', () => { + afterEach(cleanup) + + it('shows the album-artist size (not the total) for an artist download', () => { + renderDialog( + { + id: 'ar1', + name: 'Artist', + size: 999999999, + stats: { albumartist: { size: 1024 } }, + }, + DOWNLOAD_MENU_ARTIST, + ) + expect(screen.getByText(/:Artist:1 KB$/)).toBeInTheDocument() + }) + + it('shows the total size for an album download', () => { + renderDialog( + { id: 'al1', name: 'Album', size: 1024 * 1024 }, + DOWNLOAD_MENU_ALBUM, + ) + expect(screen.getByText(/:Album:1 MB$/)).toBeInTheDocument() + }) +})