Better handling of album comments (#1013)

* Change album comment behaviour

* Don't check first item

* Fix previously imported album comments.

* Remove song comments if album comment is present

db/migration/20210418232815_fix_album_comments.go

@@ -0,0 +1,66 @@
+package migrations
+
+import (
+	"database/sql"
+	"strings"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/pressly/goose"
+)
+
+func init() {
+	goose.AddMigration(upFixAlbumComments, downFixAlbumComments)
+}
+
+func upFixAlbumComments(tx *sql.Tx) error {
+	const zwsp = string('\u200b')
+	rows, err := tx.Query(`
+	SELECT album.id, group_concat(media_file.comment, '` + zwsp + `') FROM album, media_file WHERE media_file.album_id = album.id GROUP BY album.id;
+	   `)
+	if err != nil {
+		return err
+	}
+	defer rows.Close()
+
+	stmt, err := tx.Prepare("UPDATE album SET comment = ? WHERE id = ?")
+	if err != nil {
+		return err
+	}
+	var id string
+	var comments sql.NullString
+
+	for rows.Next() {
+		err = rows.Scan(&id, &comments)
+		if err != nil {
+			return err
+		}
+		if !comments.Valid {
+			continue
+		}
+		comment := getComment(comments.String, zwsp)
+		_, err = stmt.Exec(comment, id)
+
+		if err != nil {
+			log.Error("Error setting album's comments", "albumId", id, err)
+		}
+	}
+	return rows.Err()
+}
+
+func downFixAlbumComments(tx *sql.Tx) error {
+	return nil
+}
+
+func getComment(comments string, separator string) string {
+	cs := strings.Split(comments, separator)
+	if len(cs) == 0 {
+		return ""
+	}
+	first := cs[0]
+	for _, c := range cs[1:] {
+		if first != c {
+			return ""
+		}
+	}
+	return first
+}

persistence/album_repository.go

@@ -263,15 +263,18 @@ func (r *albumRepository) refresh(ids ...string) error {
 	return err
 }
 
-// Return the first non empty comment, if any
 func getComment(comments string, separator string) string {
 	cs := strings.Split(comments, separator)
-	for _, c := range cs {
-		if c != "" {
-			return c
+	if len(cs) == 0 {
+		return ""
+	}
+	first := cs[0]
+	for _, c := range cs[1:] {
+		if first != c {
+			return ""
 		}
 	}
-	return ""
+	return first
 }
 
 func getMinYear(years string) int {

persistence/album_repository_test.go

@@ -96,8 +96,11 @@ var _ = Describe("AlbumRepository", func() {
 		It("returns empty string if there are no comments", func() {
 			Expect(getComment("", "")).To(Equal(""))
 		})
-		It("returns first occurrence of non-empty comment", func() {
-			Expect(getComment(zwsp+zwsp+"first"+zwsp+"second", zwsp)).To(Equal("first"))
+		It("returns empty string if comments are different", func() {
+			Expect(getComment("first"+zwsp+"second", zwsp)).To(Equal(""))
+		})
+		It("returns comment if all comments are the same", func() {
+			Expect(getComment("first"+zwsp+"first", zwsp)).To(Equal("first"))
 		})
 	})
 

ui/src/album/AlbumShow.js

@@ -40,6 +40,7 @@ const AlbumShowLayout = (props) => {
           <AlbumSongs
             resource={'albumSong'}
             exporter={false}
+            album={record}
             actions={
               <AlbumActions className={classes.albumActions} record={record} />
             }

ui/src/album/AlbumSongs.js

@@ -157,7 +157,17 @@ const AlbumSongs = (props) => {
   )
 }
 
+export const removeAlbumCommentsFromSongs = ({ album, data }) => {
+  if (album?.comment && data) {
+    Object.values(data).forEach((song) => {
+      song.comment = ''
+    })
+  }
+}
+
 const SanitizedAlbumSongs = (props) => {
+  removeAlbumCommentsFromSongs(props)
+
   const { loaded, loading, total, ...rest } = useListContext(props)
   return <>{loaded && <AlbumSongs {...rest} actions={props.actions} />}</>
 }

ui/src/album/AlbumSongs.test.js

@@ -0,0 +1,24 @@
+import { removeAlbumCommentsFromSongs } from './AlbumSongs'
+
+describe('removeAlbumCommentsFromSongs', () => {
+  const data = { 1: { comment: 'one' }, 2: { comment: 'two' } }
+  it('does not remove song comments if album does not have comment', () => {
+    const album = { comment: '' }
+    removeAlbumCommentsFromSongs({ album, data })
+    expect(data['1'].comment).toEqual('one')
+    expect(data['2'].comment).toEqual('two')
+  })
+
+  it('removes song comments if album has comment', () => {
+    const album = { comment: 'test' }
+    removeAlbumCommentsFromSongs({ album, data })
+    expect(data['1'].comment).toEqual('')
+    expect(data['2'].comment).toEqual('')
+  })
+
+  it('does not crash if album and data arr not available', () => {
+    expect(() => {
+      removeAlbumCommentsFromSongs({})
+    }).not.toThrow()
+  })
+})