Merge branch 'master' into artwork-blurhash

model/criteria/fields.go

@@ -71,6 +71,11 @@ var fieldMap = map[string]FieldInfo{
 	"albumlastplayed":      {},
 	"albumdateloved":       {},
 	"albumdaterated":       {},
+	"albumdateadded":       {},
+	"albumdatemodified":    {},
+	"albumduration":        {},
+	"albumsongcount":       {},
+	"albumsize":            {},
 	"artistrating":         {},
 	"artistloved":          {Boolean: true},
 	"artistplaycount":      {},

persistence/criteria_sql.go

@@ -21,6 +21,7 @@ const (
 	smartPlaylistJoinNone            smartPlaylistJoinType = 0
 	smartPlaylistJoinAlbumAnnotation smartPlaylistJoinType = 1 << iota
 	smartPlaylistJoinArtistAnnotation
+	smartPlaylistJoinAlbum
 )
 
 func (j smartPlaylistJoinType) has(other smartPlaylistJoinType) bool {
@@ -103,6 +104,11 @@ var smartPlaylistFields = map[string]smartPlaylistField{
 	"albumlastplayed":      {expr: "album_annotation.play_date", joinType: smartPlaylistJoinAlbumAnnotation},
 	"albumdateloved":       {expr: "album_annotation.starred_at", joinType: smartPlaylistJoinAlbumAnnotation},
 	"albumdaterated":       {expr: "album_annotation.rated_at", joinType: smartPlaylistJoinAlbumAnnotation},
+	"albumdateadded":       {expr: "album.created_at", joinType: smartPlaylistJoinAlbum},
+	"albumdatemodified":    {expr: "album.updated_at", joinType: smartPlaylistJoinAlbum},
+	"albumduration":        {expr: "album.duration", joinType: smartPlaylistJoinAlbum},
+	"albumsongcount":       {expr: "album.song_count", joinType: smartPlaylistJoinAlbum},
+	"albumsize":            {expr: "album.size", joinType: smartPlaylistJoinAlbum},
 	"artistrating":         {expr: "artist_annotation.rating", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation},
 	"artistloved":          {expr: "artist_annotation.starred", coalesceDefault: false, joinType: smartPlaylistJoinArtistAnnotation},
 	"artistplaycount":      {expr: "artist_annotation.play_count", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation},

persistence/criteria_sql_test.go

@@ -51,6 +51,11 @@ var _ = Describe("Smart playlist criteria SQL", func() {
 		Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1),
 		Entry("album annotation", criteria.Gt{"albumRating": 3}, "album_annotation.rating > ?", 3),
 		Entry("artist annotation", criteria.Is{"artistLoved": true}, "artist_annotation.starred = ?", true),
+		Entry("album column", criteria.Gt{"albumSongCount": 5}, "album.song_count > ?", 5),
+		Entry("album duration column", criteria.Lt{"albumDuration": 600}, "album.duration < ?", 600),
+		Entry("album size column", criteria.Gt{"albumSize": 1000}, "album.size > ?", 1000),
+		Entry("album date column", criteria.After{"albumDateAdded": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.created_at > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
+		Entry("album modified column", criteria.Before{"albumDateModified": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.updated_at < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
 		// Annotation fields use a COALESCE default (0 for numeric, false for bool) so that tracks
 		// with no annotation row behave as that default. To keep the annotation index usable, the
 		// COALESCE is dropped when the compared value cannot match the default (the missing-row
@@ -298,6 +303,11 @@ var _ = Describe("Smart playlist criteria SQL", func() {
 			Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "random"}).OrderBy()).To(Equal("random() asc"))
 		})
 
+		It("sorts by album columns bare, with no COALESCE default", func() {
+			Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-albumDateAdded,trackNumber"}).OrderBy()).
+				To(Equal("album.created_at desc, media_file.track_number asc"))
+		})
+
 		It("sorts by multiple fields", func() {
 			Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title,-rating"}).OrderBy()).To(Equal("media_file.title asc, COALESCE(annotation.rating, 0) desc"))
 		})
@@ -323,6 +333,28 @@ var _ = Describe("Smart playlist criteria SQL", func() {
 		}
 	})
 
+	It("declares a joinType matching the table each field selects from", func() {
+		// Omitting the joinType still compiles, so without this the field would only fail at
+		// refresh time with "no such column".
+		joinByTable := map[string]smartPlaylistJoinType{
+			"media_file":        smartPlaylistJoinNone,
+			"annotation":        smartPlaylistJoinNone,
+			"album":             smartPlaylistJoinAlbum,
+			"album_annotation":  smartPlaylistJoinAlbumAnnotation,
+			"artist_annotation": smartPlaylistJoinArtistAnnotation,
+		}
+		for name, field := range smartPlaylistFields {
+			if field.expr == "" {
+				continue
+			}
+			table, _, ok := strings.Cut(field.expr, ".")
+			Expect(ok).To(BeTrue(), "field %q has expr %q with no table prefix", name, field.expr)
+			want, known := joinByTable[table]
+			Expect(known).To(BeTrue(), "field %q selects from unknown table %q", name, table)
+			Expect(field.joinType).To(Equal(want), "field %q selects from %q but declares the wrong joinType", name, table)
+		}
+	})
+
 	Describe("JSON condition merging", func() {
 		It("merges multiple role conditions in an OR group into a single EXISTS", func() {
 			expr := criteria.Any{
@@ -585,5 +617,21 @@ var _ = Describe("Smart playlist criteria SQL", func() {
 
 			Expect(newSmartPlaylistCriteria(c).RequiredJoins().has(smartPlaylistJoinArtistAnnotation)).To(BeTrue())
 		})
+
+		It("keeps a sort-only album join out of the expression joins", func() {
+			c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-albumDateAdded"}
+			cSQL := newSmartPlaylistCriteria(c)
+
+			Expect(cSQL.ExpressionJoins()).To(Equal(smartPlaylistJoinNone))
+			Expect(cSQL.RequiredJoins().has(smartPlaylistJoinAlbum)).To(BeTrue())
+		})
+
+		It("distinguishes the album join from the album annotation join", func() {
+			c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}}
+			joins := newSmartPlaylistCriteria(c).RequiredJoins()
+
+			Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue())
+			Expect(joins.has(smartPlaylistJoinAlbum)).To(BeFalse())
+		})
 	})
 })

persistence/e2e/smartplaylist_test.go

@@ -204,6 +204,43 @@ var _ = Describe("Smart Playlists", func() {
 		})
 	})
 
+	Describe("Album aggregate fields", func() {
+		// Abbey Road and IV have two tracks each; the other four albums have one.
+		It("matches albums with more than one track", func() {
+			results := evaluateRule(`{"all":[{"gt":{"albumsongcount":1}}]}`)
+			Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog"))
+		})
+
+		It("matches single-track albums", func() {
+			results := evaluateRule(`{"all":[{"is":{"albumsongcount":1}}]}`)
+			Expect(results).To(ConsistOf("So What", "Bohemian Rhapsody", "All Along the Watchtower",
+				"We Are the Champions"))
+		})
+
+		It("matches albumDateAdded inTheLast 1 day", func() {
+			results := evaluateRule(`{"all":[{"inTheLast":{"albumdateadded":1}}]}`)
+			Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog",
+				"So What", "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions"))
+		})
+
+		It("matches albumDateModified before a far-future date", func() {
+			results := evaluateRule(`{"all":[{"before":{"albumdatemodified":"2099-01-01"}}]}`)
+			Expect(results).To(HaveLen(8))
+		})
+
+		// Fixture durations are randomized, so only the aggregate being populated can be asserted.
+		It("resolves albumDuration and albumSize to non-zero aggregates", func() {
+			Expect(evaluateRule(`{"all":[{"gt":{"albumduration":0}}]}`)).To(HaveLen(8))
+			Expect(evaluateRule(`{"all":[{"gt":{"albumsize":0}}]}`)).To(HaveLen(8))
+		})
+
+		It("groups by album date and orders within the album (issue #5347)", func() {
+			results := evaluateRuleOrdered(
+				`{"all":[{"is":{"album":"Abbey Road"}}],"sort":"-albumdateadded,tracknumber"}`)
+			Expect(results).To(Equal([]string{"Come Together", "Something"}))
+		})
+	})
+
 	Describe("Logic operators", func() {
 		It("matches with ALL (AND)", func() {
 			results := evaluateRule(`{"all":[{"is":{"genre":"Blues"}},{"gt":{"bpm":130}}]}`)

persistence/smart_playlist_repository.go

@@ -122,7 +122,7 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ
 	exprJoins := rulesSQL.ExpressionJoins()
 	countSq := Select("count(*) as count").From("media_file")
 	countSq = r.addMediaFileAnnotationJoin(countSq, userID)
-	countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, userID)
+	countSq = r.addSmartPlaylistJoins(countSq, exprJoins, userID)
 	countSq = r.applyLibraryFilter(countSq, "media_file")
 
 	cond, err := rulesSQL.Where()
@@ -144,7 +144,7 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ
 }
 
 // buildSmartPlaylistQuery constructs the SQL query to select media files matching the smart playlist criteria,
-// including necessary joins for annotations and library filtering.
+// including the joins its fields require and library filtering.
 func (r *playlistRepository) buildSmartPlaylistQuery(pls *model.Playlist, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder {
 	orderBy := rulesSQL.OrderBy()
 	sq := Select("row_number() over (order by "+orderBy+") as id", "'"+pls.ID+"' as playlist_id", "media_file.id as media_file_id").
@@ -152,7 +152,7 @@ func (r *playlistRepository) buildSmartPlaylistQuery(pls *model.Playlist, rulesS
 	sq = r.addMediaFileAnnotationJoin(sq, userID)
 
 	requiredJoins := rulesSQL.RequiredJoins()
-	sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, userID)
+	sq = r.addSmartPlaylistJoins(sq, requiredJoins, userID)
 	sq = r.applyLibraryFilter(sq, "media_file")
 	return sq
 }
@@ -166,9 +166,8 @@ func (r *playlistRepository) addMediaFileAnnotationJoin(sq SelectBuilder, userID
 		" AND annotation.user_id = ?)", userID)
 }
 
-// addSmartPlaylistAnnotationJoins adds left joins to the annotation table for albums and artists as needed based on
-// the smart playlist criteria, filtering by user ID to include user-specific annotations in the evaluation.
-func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, joins smartPlaylistJoinType, userID string) SelectBuilder {
+// addSmartPlaylistJoins adds the left joins required by the criteria's fields.
+func (r *playlistRepository) addSmartPlaylistJoins(sq SelectBuilder, joins smartPlaylistJoinType, userID string) SelectBuilder {
 	if joins.has(smartPlaylistJoinAlbumAnnotation) {
 		sq = sq.LeftJoin("annotation AS album_annotation ON ("+
 			"album_annotation.item_id = media_file.album_id"+
@@ -181,6 +180,9 @@ func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, j
 			" AND artist_annotation.item_type = 'artist'"+
 			" AND artist_annotation.user_id = ?)", userID)
 	}
+	if joins.has(smartPlaylistJoinAlbum) {
+		sq = sq.LeftJoin("album ON album.id = media_file.album_id")
+	}
 	return sq
 }
 

persistence/smart_playlist_repository_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/criteria"
 	"github.com/navidrome/navidrome/model/request"
+	"github.com/navidrome/navidrome/utils/slice"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/pocketbase/dbx"
@@ -392,6 +393,40 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() {
 		})
 	})
 
+	Describe("Smart Playlists with Album Aggregate Criteria", func() {
+		BeforeEach(func() {
+			DeferCleanup(configtest.SetupConfig())
+			conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second
+		})
+
+		trackIDsOf := func(rules *criteria.Criteria) []string {
+			newPls := model.Playlist{Name: "Album Aggregates", OwnerID: "userid", Rules: rules}
+			Expect(repo.Put(&newPls)).To(Succeed())
+			DeferCleanup(func() { _ = repo.Delete(newPls.ID) })
+
+			pls, err := repo.GetWithTracks(newPls.ID, true, false)
+			Expect(err).ToNot(HaveOccurred())
+			return slice.Map(pls.Tracks, func(t model.PlaylistTrack) string { return t.MediaFileID })
+		}
+
+		It("filters on albumSongCount", func() {
+			// albumMultiDisc (ID "104") is the only fixture album with SongCount > 3
+			rules := &criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumSongCount": 3}}}
+
+			Expect(trackIDsOf(rules)).To(ConsistOf("2001", "2002", "2003", "2004"))
+		})
+
+		It("sorts by an album field not referenced in the expression (issue #5347)", func() {
+			// All four tracks share album 104, so the album date ties and disc/track number decide.
+			rules := &criteria.Criteria{
+				Expression: criteria.All{criteria.Is{"album": "Multi Disc Album"}},
+				Sort:       "-albumDateAdded,discNumber,trackNumber",
+			}
+
+			Expect(trackIDsOf(rules)).To(HaveExactElements("2002", "2004", "2003", "2001"))
+		})
+	})
+
 	Describe("Smart Playlists with Tag Criteria", func() {
 		var mfRepo model.MediaFileRepository
 		var testPlaylistID string

scheduler/log_adapter.go

@@ -11,7 +11,7 @@ func (l *logger) Info(msg string, keysAndValues ...any) {
 		"Scheduler: " + msg,
 	}
 	args = append(args, keysAndValues...)
-	log.Debug(args...)
+	log.Trace(args...)
 }
 
 func (l *logger) Error(err error, msg string, keysAndValues ...any) {

ui/src/common/SongDatagrid.jsx

@@ -78,7 +78,7 @@ const useStyles = makeStyles({
   },
 })
 
-const DiscSubtitleRow = forwardRef(
+export const DiscSubtitleRow = forwardRef(
   ({ record, onClick, colSpan, contextAlwaysVisible }, ref) => {
     const translate = useTranslate()
     const isDesktop = useMediaQuery((theme) => theme.breakpoints.up('md'))
@@ -153,13 +153,17 @@ const DiscSubtitleRow = forwardRef(
             {subtitle}
           </Typography>
           {isLightboxOpen && !imageError && (
-            <Lightbox
-              imagePadding={50}
-              animationDuration={200}
-              imageTitle={record.album + ' - ' + subtitle}
-              mainSrc={fullImageUrl}
-              onCloseRequest={handleCloseLightbox}
-            />
+            // Lightbox portals out of the row, but React still bubbles its
+            // events up this tree, where the row's onClick would play the disc.
+            <span onClick={(e) => e.stopPropagation()}>
+              <Lightbox
+                imagePadding={50}
+                animationDuration={200}
+                imageTitle={record.album + ' - ' + subtitle}
+                mainSrc={fullImageUrl}
+                onCloseRequest={handleCloseLightbox}
+              />
+            </span>
           )}
         </TableCell>
         <TableCell>

ui/src/common/SongDatagrid.test.jsx

@@ -0,0 +1,74 @@
+import React from 'react'
+import { render, fireEvent, screen } from '@testing-library/react'
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { createTheme, ThemeProvider } from '@material-ui/core/styles'
+import { DiscSubtitleRow } from './SongDatagrid'
+
+vi.mock('../subsonic', () => ({
+  default: { getDiscCoverArtUrl: () => 'http://localhost/cover.jpg' },
+}))
+
+vi.mock('react-redux', () => ({ useDispatch: () => vi.fn() }))
+
+vi.mock('../common', () => ({ AlbumContextMenu: () => null }))
+
+vi.mock('react-dnd', () => ({ useDrag: () => [{}, vi.fn()] }))
+
+const record = {
+  id: 'song-1',
+  albumId: 'album-1',
+  album: 'The Album',
+  discNumber: 2,
+  discSubtitle: 'Bonus Disc',
+  updatedAt: '2024-01-01',
+}
+
+const renderRow = (onClick) =>
+  render(
+    <ThemeProvider theme={createTheme()}>
+      <table>
+        <tbody>
+          <DiscSubtitleRow record={record} onClick={onClick} colSpan={3} />
+        </tbody>
+      </table>
+    </ThemeProvider>,
+  )
+
+const openLightbox = () => {
+  fireEvent.click(document.querySelector('img'))
+  expect(document.querySelector('.ril__closeButton')).toBeTruthy()
+}
+
+describe('DiscSubtitleRow', () => {
+  beforeEach(() => vi.clearAllMocks())
+
+  it('plays the disc when the row is clicked', () => {
+    const onClick = vi.fn()
+    renderRow(onClick)
+    fireEvent.click(screen.getByText('Bonus Disc'))
+    expect(onClick).toHaveBeenCalledWith(2)
+  })
+
+  it('does not play the disc when opening the lightbox', () => {
+    const onClick = vi.fn()
+    renderRow(onClick)
+    openLightbox()
+    expect(onClick).not.toHaveBeenCalled()
+  })
+
+  it('does not play the disc when closing the lightbox', () => {
+    const onClick = vi.fn()
+    renderRow(onClick)
+    openLightbox()
+    fireEvent.click(document.querySelector('.ril__closeButton'))
+    expect(onClick).not.toHaveBeenCalled()
+  })
+
+  it('does not play the disc when clicking the lightbox backdrop', () => {
+    const onClick = vi.fn()
+    renderRow(onClick)
+    openLightbox()
+    fireEvent.click(document.querySelector('.ril__inner'))
+    expect(onClick).not.toHaveBeenCalled()
+  })
+})