diff --git a/go.mod b/go.mod index cdf8fc699..4339b9c55 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/navidrome/navidrome go 1.27 // Fork to implement raw tags support -replace go.senan.xyz/taglib => github.com/deluan/go-taglib v0.0.0-20260720134629-a133b9719ea3 +replace go.senan.xyz/taglib => github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df require ( github.com/Masterminds/squirrel v1.5.4 diff --git a/go.sum b/go.sum index d852285f1..71d9facfd 100644 --- a/go.sum +++ b/go.sum @@ -29,8 +29,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1 h1:5RVFMOWjMyRy8cARdy79nAmgYw3hK/4HUq48LQ6Wwqo= github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1/go.mod h1:ZXNYxsqcloTdSy/rNShjYzMhyjf0LaoftYK0p+A3h40= -github.com/deluan/go-taglib v0.0.0-20260720134629-a133b9719ea3 h1:j7eSXqgtjhlNfwnMEzRdXnJGZTEw4I7J9TeQAll83bU= -github.com/deluan/go-taglib v0.0.0-20260720134629-a133b9719ea3/go.mod h1:QGxQ4Z1IWyY9w56xNEFjYAaWE8uSxA/gneQ7RPcFJrY= +github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df h1:LdLQVAWVc6hCzqnrfVIEXOhP+r0iSit+EvsXwZDyL70= +github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df/go.mod h1:QGxQ4Z1IWyY9w56xNEFjYAaWE8uSxA/gneQ7RPcFJrY= github.com/deluan/rest v0.0.0-20211102003136-6260bc399cbf h1:tb246l2Zmpt/GpF9EcHCKTtwzrd0HGfEmoODFA/qnk4= github.com/deluan/rest v0.0.0-20211102003136-6260bc399cbf/go.mod h1:tSgDythFsl0QgS/PFWfIZqcJKnkADWneY80jaVRlqK8= github.com/deluan/sanitize v0.0.0-20241120162836-fdfd8fdfaa55 h1:wSCnggTs2f2ji6nFwQmfwgINcmSMj0xF0oHnoyRSPe4= diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index bf8d6d5a8..bc6bffd25 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -316,11 +316,12 @@ func (r *playlistRepository) refreshCounters(pls *model.Playlist) error { } // Update playlist's total duration, size and count + now := time.Now() upd := Update("playlist"). Set("duration", res.Duration). Set("size", res.Size). Set("song_count", res.Count). - Set("updated_at", time.Now()). + Set("updated_at", now). Where(Eq{"id": pls.ID}) _, err = r.executeSQL(upd) if err != nil { @@ -329,6 +330,7 @@ func (r *playlistRepository) refreshCounters(pls *model.Playlist) error { pls.SongCount = int(res.Count) pls.Duration = res.Duration pls.Size = int64(res.Size) + pls.UpdatedAt = now return nil } diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 65ae4656b..9d2ac9590 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -58,7 +58,8 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return false } - now := time.Now() + // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree + now := pls.UpdatedAt updSql := Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID}) if _, err = r.executeSQL(updSql); err != nil { log.Error(r.ctx, "Error updating smart playlist", "playlist", pls.Name, "id", pls.ID, err) diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index ddc155fab..6f8684d5c 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -45,6 +45,23 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { }) }) + Context("after an evaluation", func() { + It("stamps updated_at and evaluated_at with the same instant", func() { + newPls := model.Playlist{Name: "Evaluated", OwnerID: "userid", Rules: rules} + Expect(repo.Put(&newPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(newPls.ID) }) + + refreshed, err := repo.GetWithTracks(newPls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + stored, err := repo.Get(newPls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(stored.EvaluatedAt).ToNot(BeNil()) + Expect(stored.UpdatedAt).To(BeTemporally("==", *stored.EvaluatedAt)) + Expect(refreshed.UpdatedAt).To(BeTemporally("==", stored.UpdatedAt)) + }) + }) + Context("invalid rules", func() { It("fails to Put it in the DB", func() { rules = &criteria.Criteria{ diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index 774a9c430..e64fc9e82 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/http" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" @@ -133,15 +132,7 @@ func (api *Router) buildPlaylist(ctx context.Context, p model.Playlist) response pls.SongCount = int32(p.SongCount) pls.Duration = int32(p.Duration) pls.Created = p.CreatedAt - if p.IsSmartPlaylist() { - if p.EvaluatedAt != nil { - pls.Changed = *p.EvaluatedAt - } else { - pls.Changed = time.Now() - } - } else { - pls.Changed = p.UpdatedAt - } + pls.Changed = p.UpdatedAt player, ok := request.PlayerFrom(ctx) if ok && isClientInList(conf.Server.Subsonic.MinimalClients, player.Client) { diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index f18f33b47..c7775c0fa 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -220,7 +220,7 @@ var _ = Describe("buildPlaylist", func() { Expect(result.SongCount).To(Equal(int32(10))) Expect(result.Duration).To(Equal(int32(600))) Expect(result.Created).To(Equal(playlist.CreatedAt)) - Expect(result.Changed).To(Equal(evaluatedAt)) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) // These should not be set Expect(result.Comment).To(BeEmpty()) @@ -245,7 +245,7 @@ var _ = Describe("buildPlaylist", func() { Expect(result.SongCount).To(Equal(int32(10))) Expect(result.Duration).To(Equal(int32(600))) Expect(result.Created).To(Equal(playlist.CreatedAt)) - Expect(result.Changed).To(Equal(*playlist.EvaluatedAt)) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) Expect(result.Comment).To(Equal("Test comment")) Expect(result.Owner).To(Equal("admin")) Expect(result.Public).To(BeTrue()) @@ -271,6 +271,21 @@ var _ = Describe("buildPlaylist", func() { }) }) + Context("when it was never evaluated", func() { + BeforeEach(func() { + playlist.EvaluatedAt = nil + player := model.Player{Client: "regular-client"} + ctx = request.WithPlayer(ctx, player) + }) + + It("omits validUntil but still reports changed", func() { + result := router.buildPlaylist(ctx, playlist) + + Expect(result.ValidUntil).To(BeNil()) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) + }) + }) + Context("with a per-playlist refreshDelay", func() { BeforeEach(func() { playlist.Rules.RefreshDelay = 24 * time.Hour diff --git a/ui/src/album/AlbumActions.jsx b/ui/src/album/AlbumActions.jsx index 96cfab09a..c3b3a4bc2 100644 --- a/ui/src/album/AlbumActions.jsx +++ b/ui/src/album/AlbumActions.jsx @@ -27,7 +27,7 @@ import { } from '../actions' import { formatBytes } from '../utils' import config from '../config' -import { ToggleFieldsMenu } from '../common' +import { RefreshMetadataButton, ToggleFieldsMenu } from '../common' const useStyles = makeStyles({ toolbar: { display: 'flex', justifyContent: 'space-between', width: '100%' }, @@ -138,6 +138,11 @@ const AlbumActions = ({ )} +
{isNotSmall && }
diff --git a/ui/src/album/AlbumActions.test.jsx b/ui/src/album/AlbumActions.test.jsx new file mode 100644 index 000000000..246a15558 --- /dev/null +++ b/ui/src/album/AlbumActions.test.jsx @@ -0,0 +1,68 @@ +import React from 'react' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import { ThemeProvider, createTheme } from '@material-ui/core/styles' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import AlbumActions from './AlbumActions' + +const { mockConfig, mockPermissions } = vi.hoisted(() => ({ + mockConfig: { + enableSharing: false, + enableDownloads: false, + losslessFormats: 'FLAC,WAV', + }, + mockPermissions: { value: 'admin' }, +})) +vi.mock('../config', () => ({ default: mockConfig })) + +vi.mock('react-redux', () => ({ + useDispatch: () => vi.fn(), + useSelector: () => ({}), +})) + +const mockRefreshMetadata = vi.fn() + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useNotify: () => vi.fn(), + useDataProvider: () => ({ refreshMetadata: mockRefreshMetadata }), + usePermissions: () => ({ permissions: mockPermissions.value }), + useTranslate: () => (x) => x, + } +}) + +describe('AlbumActions', () => { + const record = { id: 'al1', name: 'Album', size: 1024 } + const refreshLabel = 'resources.album.actions.refresh' + + beforeEach(() => { + vi.clearAllMocks() + mockPermissions.value = 'admin' + mockRefreshMetadata.mockResolvedValue({ data: { id: 'al1' } }) + }) + + const renderAlbumActions = () => + render( + + + , + ) + + it('refreshes the album metadata for admins', async () => { + renderAlbumActions() + fireEvent.click(screen.getByRole('button', { name: refreshLabel })) + + await waitFor(() => + expect(mockRefreshMetadata).toHaveBeenCalledWith('album', 'al1'), + ) + }) + + it('hides the action for non-admin users', () => { + mockPermissions.value = 'regular' + renderAlbumActions() + expect( + screen.queryByRole('button', { name: refreshLabel }), + ).not.toBeInTheDocument() + }) +}) diff --git a/ui/src/artist/ArtistActions.jsx b/ui/src/artist/ArtistActions.jsx index 2f329a034..c2614c05a 100644 --- a/ui/src/artist/ArtistActions.jsx +++ b/ui/src/artist/ArtistActions.jsx @@ -26,6 +26,7 @@ import { import config from '../config' import { formatBytes } from '../utils' import { artistDownloadSize } from '../common/artist' +import { RefreshMetadataButton } from '../common/RefreshMetadataButton' const useStyles = makeStyles((theme) => ({ toolbar: { @@ -33,6 +34,7 @@ const useStyles = makeStyles((theme) => ({ padding: '0 !important', background: 'transparent', boxShadow: 'none', + alignItems: 'center', '& .MuiToolbar-root': { minHeight: 'auto', padding: '0 !important', @@ -174,6 +176,11 @@ const ArtistActions = ({ className, record, ...rest }) => { icon={} /> )} + ) } diff --git a/ui/src/artist/ArtistActions.test.jsx b/ui/src/artist/ArtistActions.test.jsx index ad25177b3..05f53f835 100644 --- a/ui/src/artist/ArtistActions.test.jsx +++ b/ui/src/artist/ArtistActions.test.jsx @@ -18,20 +18,26 @@ vi.mock('../subsonic', () => ({ default: { getSimilarSongs2: vi.fn(), getTopSongs: vi.fn() }, })) -const { mockConfig } = vi.hoisted(() => ({ +const { mockConfig, mockPermissions } = vi.hoisted(() => ({ mockConfig: { enableSharing: true, enableDownloads: true }, + mockPermissions: { value: 'admin' }, })) vi.mock('../config', () => ({ default: mockConfig })) const mockNotify = vi.fn() const mockGetList = vi.fn().mockResolvedValue({ data: [{ id: 's1' }] }) +const mockRefreshMetadata = vi.fn().mockResolvedValue({ data: { id: 'ar1' } }) vi.mock('react-admin', async (importOriginal) => { const actual = await importOriginal() return { ...actual, useNotify: () => mockNotify, - useDataProvider: () => ({ getList: mockGetList }), + useDataProvider: () => ({ + getList: mockGetList, + refreshMetadata: mockRefreshMetadata, + }), + usePermissions: () => ({ permissions: mockPermissions.value }), useTranslate: () => (x) => x, } }) @@ -64,6 +70,7 @@ describe('ArtistActions', () => { vi.spyOn(console, 'error').mockImplementation(() => {}) mockConfig.enableSharing = true mockConfig.enableDownloads = true + mockPermissions.value = 'admin' const songWithReplayGain = { id: 'rec1', @@ -290,4 +297,25 @@ describe('ArtistActions', () => { expect(screen.queryByText(/ra\.action\.download/)).not.toBeInTheDocument() }) }) + + describe('Refresh metadata action', () => { + const refreshLabel = 'resources.album.actions.refresh' + + it('refreshes the artist metadata for admins', async () => { + renderArtistActions() + fireEvent.click(screen.getByRole('button', { name: refreshLabel })) + + await waitFor(() => + expect(mockRefreshMetadata).toHaveBeenCalledWith('artist', 'ar1'), + ) + }) + + it('hides the action for non-admin users', () => { + mockPermissions.value = 'regular' + renderArtistActions() + expect( + screen.queryByRole('button', { name: refreshLabel }), + ).not.toBeInTheDocument() + }) + }) }) diff --git a/ui/src/common/ContextMenus.jsx b/ui/src/common/ContextMenus.jsx index 606506459..4407392c1 100644 --- a/ui/src/common/ContextMenus.jsx +++ b/ui/src/common/ContextMenus.jsx @@ -30,6 +30,7 @@ import { LoveButton } from './LoveButton' import config from '../config' import { formatBytes } from '../utils' import { artistDownloadSize } from './artist' +import { useRefreshMetadata } from './useRefreshMetadata' const useStyles = makeStyles({ noWrap: { @@ -75,6 +76,7 @@ const ContextMenu = ({ const translate = useTranslate() const notify = useNotify() const { permissions } = usePermissions() + const refreshMetadata = useRefreshMetadata() const [anchorEl, setAnchorEl] = useState(null) const isArtist = resource === 'artist' @@ -139,11 +141,7 @@ const ContextMenu = ({ enabled: permissions === 'admin', needData: false, label: translate('resources.album.actions.refresh'), - action: (record) => - dataProvider - .refreshMetadata(resource, record.id) - .then(() => notify('message.metadataRefreshStarted')) - .catch(() => notify('ra.page.error', 'warning')), + action: (record) => refreshMetadata(resource, record.id), }, ...(!hideInfo && { info: { diff --git a/ui/src/common/RefreshMetadataButton.jsx b/ui/src/common/RefreshMetadataButton.jsx new file mode 100644 index 000000000..aa0533ade --- /dev/null +++ b/ui/src/common/RefreshMetadataButton.jsx @@ -0,0 +1,62 @@ +import React, { useCallback } from 'react' +import PropTypes from 'prop-types' +import Tooltip from '@material-ui/core/Tooltip' +import { makeStyles } from '@material-ui/core/styles' +import { Button, usePermissions, useTranslate } from 'react-admin' +import { MdRefresh } from 'react-icons/md' +import { useRefreshMetadata } from './useRefreshMetadata' + +const useStyles = makeStyles({ + // Tooltip needs a ref-holding child, and react-admin's Button does not forward one. + wrapper: { display: 'inline-flex', verticalAlign: 'middle' }, + button: { minWidth: 'auto' }, +}) + +// react-admin's Button, not an IconButton: the toolbars use it, so colour and the icon-only swap +// at xs match without restating either rule. +export const RefreshMetadataButton = ({ + resource, + record, + className, + size, +}) => { + const translate = useTranslate() + const { permissions } = usePermissions() + const refreshMetadata = useRefreshMetadata() + const classes = useStyles() + + const handleClick = useCallback( + () => refreshMetadata(resource, record?.id), + [refreshMetadata, resource, record], + ) + + if (permissions !== 'admin' || !record?.id) return null + + const label = translate('resources.album.actions.refresh') + return ( + + + + + + ) +} + +RefreshMetadataButton.propTypes = { + resource: PropTypes.oneOf(['album', 'artist']).isRequired, + record: PropTypes.object, + className: PropTypes.string, + size: PropTypes.oneOf(['small', 'medium']), +} + +RefreshMetadataButton.defaultProps = { + size: 'small', +} diff --git a/ui/src/common/RefreshMetadataButton.test.jsx b/ui/src/common/RefreshMetadataButton.test.jsx new file mode 100644 index 000000000..7bda0497c --- /dev/null +++ b/ui/src/common/RefreshMetadataButton.test.jsx @@ -0,0 +1,104 @@ +import React from 'react' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import { ThemeProvider, createTheme } from '@material-ui/core/styles' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { RefreshMetadataButton } from './RefreshMetadataButton' + +const mockNotify = vi.fn() +const mockRefreshMetadata = vi.fn() +const { mockPermissions } = vi.hoisted(() => ({ + mockPermissions: { value: 'admin' }, +})) + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useNotify: () => mockNotify, + useDataProvider: () => ({ refreshMetadata: mockRefreshMetadata }), + usePermissions: () => ({ permissions: mockPermissions.value }), + useTranslate: () => (x) => x, + } +}) + +describe('RefreshMetadataButton', () => { + const record = { id: 'al-1', name: 'Album' } + + beforeEach(() => { + vi.clearAllMocks() + mockPermissions.value = 'admin' + mockRefreshMetadata.mockResolvedValue({ data: { id: 'al-1' } }) + }) + + const renderButton = (props = {}) => + render( + + + , + ) + + it('renders an icon-only button labelled by the refresh action', () => { + renderButton() + const button = screen.getByRole('button', { + name: 'resources.album.actions.refresh', + }) + expect(button).toBeInTheDocument() + expect(button).toHaveTextContent('') + }) + + it('shows the label as a tooltip on hover', async () => { + renderButton() + fireEvent.mouseOver( + screen.getByRole('button', { + name: 'resources.album.actions.refresh', + }), + ) + await waitFor(() => + expect(screen.getByRole('tooltip')).toHaveTextContent( + 'resources.album.actions.refresh', + ), + ) + }) + + it('renders nothing for non-admin users', () => { + mockPermissions.value = 'regular' + const { container } = renderButton() + expect(container).toBeEmptyDOMElement() + }) + + it('renders nothing without a record', () => { + const { container } = renderButton({ record: undefined }) + expect(container).toBeEmptyDOMElement() + }) + + it('requests a refresh for the record and notifies success', async () => { + renderButton() + fireEvent.click(screen.getByRole('button')) + + await waitFor(() => + expect(mockRefreshMetadata).toHaveBeenCalledWith('album', 'al-1'), + ) + await waitFor(() => + expect(mockNotify).toHaveBeenCalledWith('message.metadataRefreshStarted'), + ) + }) + + it('passes the artist resource through', async () => { + renderButton({ resource: 'artist', record: { id: 'ar-1' } }) + fireEvent.click(screen.getByRole('button')) + + await waitFor(() => + expect(mockRefreshMetadata).toHaveBeenCalledWith('artist', 'ar-1'), + ) + }) + + it('notifies a warning when the request fails', async () => { + mockRefreshMetadata.mockRejectedValue(new Error('boom')) + renderButton() + fireEvent.click(screen.getByRole('button')) + + await waitFor(() => + expect(mockNotify).toHaveBeenCalledWith('ra.page.error', 'warning'), + ) + }) +}) diff --git a/ui/src/common/index.js b/ui/src/common/index.js index 047f5b3cf..0177df326 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -36,6 +36,8 @@ export * from './useTraceUpdate' export * from './Writable' export * from './SongSimpleList' export * from './RatingField' +export * from './RefreshMetadataButton' +export * from './useRefreshMetadata' export * from './useRating' export * from './useSelectedFields' export * from './ToggleFieldsMenu' diff --git a/ui/src/common/useRefreshMetadata.js b/ui/src/common/useRefreshMetadata.js new file mode 100644 index 000000000..d34042919 --- /dev/null +++ b/ui/src/common/useRefreshMetadata.js @@ -0,0 +1,16 @@ +import { useCallback } from 'react' +import { useDataProvider, useNotify } from 'react-admin' + +export const useRefreshMetadata = () => { + const dataProvider = useDataProvider() + const notify = useNotify() + + return useCallback( + (resource, id) => + dataProvider + .refreshMetadata(resource, id) + .then(() => notify('message.metadataRefreshStarted')) + .catch(() => notify('ra.page.error', 'warning')), + [dataProvider, notify], + ) +}