Merge branch '757-flac-cover' into 'master'

Fix #757: Ensure cover art from uploaded files is picked up properly on existing albums

See merge request funkwhale/funkwhale!682

api/funkwhale_api/federation/serializers.py

@@ -769,7 +769,7 @@ class TrackSerializer(MusicEntitySerializer):
         from_activity = self.context.get("activity")
         if from_activity:
             metadata["from_activity_id"] = from_activity.pk
-        track = music_tasks.get_track_from_import_metadata(metadata)
+        track = music_tasks.get_track_from_import_metadata(metadata, update_cover=True)
         return track
 
 

api/funkwhale_api/music/tasks.py

@@ -26,7 +26,9 @@ from . import serializers
 logger = logging.getLogger(__name__)
 
 
-def update_album_cover(album, source=None, cover_data=None, replace=False):
+def update_album_cover(
+    album, source=None, cover_data=None, musicbrainz=True, replace=False
+):
     if album.cover and not replace:
         return
     if cover_data:
@@ -39,7 +41,7 @@ def update_album_cover(album, source=None, cover_data=None, replace=False):
         cover = get_cover_from_fs(path)
         if cover:
             return album.get_image(data=cover)
-    if album.mbid:
+    if musicbrainz and album.mbid:
         try:
             logger.info(
                 "[Album %s] Fetching cover from musicbrainz release %s",
@@ -179,8 +181,8 @@ def process_upload(upload):
     import_metadata = upload.import_metadata or {}
     old_status = upload.import_status
     audio_file = upload.get_audio_file()
+    additional_data = {}
     try:
-        additional_data = {}
         if not audio_file:
             # we can only rely on user proveded data
             final_metadata = import_metadata
@@ -241,6 +243,15 @@ def process_upload(upload):
             "bitrate",
         ]
     )
+
+    # update album cover, if needed
+    if not track.album.cover:
+        update_album_cover(
+            track.album,
+            source=final_metadata.get("upload_source"),
+            cover_data=final_metadata.get("cover_data"),
+        )
+
     broadcast = getter(
         import_metadata, "funkwhale", "config", "broadcast", default=True
     )
@@ -369,7 +380,18 @@ def sort_candidates(candidates, important_fields):
 
 
 @transaction.atomic
-def get_track_from_import_metadata(data):
+def get_track_from_import_metadata(data, update_cover=False):
+    track = _get_track(data)
+    if update_cover and track and not track.album.cover:
+        update_album_cover(
+            track.album,
+            source=data.get("upload_source"),
+            cover_data=data.get("cover_data"),
+        )
+    return track
+
+
+def _get_track(data):
     track_uuid = getter(data, "funkwhale", "track", "uuid")
 
     if track_uuid:
@@ -380,12 +402,6 @@ def get_track_from_import_metadata(data):
         except models.Track.DoesNotExist:
             raise UploadImportError(code="track_uuid_not_found")
 
-        if not track.album.cover:
-            update_album_cover(
-                track.album,
-                source=data.get("upload_source"),
-                cover_data=data.get("cover_data"),
-            )
         return track
 
     from_activity_id = data.get("from_activity_id", None)
@@ -479,10 +495,6 @@ def get_track_from_import_metadata(data):
     album = get_best_candidate_or_create(
         models.Album, query, defaults=defaults, sort_fields=["mbid", "fid"]
     )[0]
-    if not album.cover:
-        update_album_cover(
-            album, source=data.get("upload_source"), cover_data=data.get("cover_data")
-        )
 
     # get / create track
     track_title = data["title"]

api/tests/music/test_tasks.py

@@ -152,7 +152,7 @@ def test_can_create_track_from_file_metadata_federation(factories, mocker, r_moc
     r_mock.get(metadata["cover_data"]["url"], body=io.BytesIO(b"coucou"))
     mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata)
 
-    track = tasks.get_track_from_import_metadata(metadata)
+    track = tasks.get_track_from_import_metadata(metadata, update_cover=True)
 
     assert track.title == metadata["title"]
     assert track.fid == metadata["fid"]
@@ -182,7 +182,9 @@ def test_sort_candidates(factories):
 
 def test_upload_import(now, factories, temp_signal, mocker):
     outbox = mocker.patch("funkwhale_api.federation.routes.outbox.dispatch")
-    track = factories["music.Track"]()
+    update_album_cover = mocker.patch("funkwhale_api.music.tasks.update_album_cover")
+    get_picture = mocker.patch("funkwhale_api.music.metadata.Metadata.get_picture")
+    track = factories["music.Track"](album__cover="")
     upload = factories["music.Upload"](
         track=None, import_metadata={"funkwhale": {"track": {"uuid": str(track.uuid)}}}
     )
@@ -195,6 +197,10 @@ def test_upload_import(now, factories, temp_signal, mocker):
     assert upload.track == track
     assert upload.import_status == "finished"
     assert upload.import_date == now
+    get_picture.assert_called_once_with("cover_front", "other")
+    update_album_cover.assert_called_once_with(
+        upload.track.album, cover_data=get_picture.return_value, source=upload.source
+    )
     handler.assert_called_once_with(
         upload=upload,
         old_status="pending",

changes/changelog.d/757.bugfix

+Ensure cover art from uploaded files is picked up properly on existing albums (#757)

dev.yml

@@ -52,7 +52,7 @@ services:
     command: python /app/manage.py runserver 0.0.0.0:${FUNKWHALE_API_PORT-5000}
     volumes:
       - ./api:/app
-      - "${MUSIC_DIRECTORY_PATH-./data/music}:/music:ro"
+      - "${MUSIC_DIRECTORY_SERVE_PATH-./data/music}:/music:ro"
     environment:
       - "FUNKWHALE_HOSTNAME=${FUNKWHALE_HOSTNAME-localhost}"
       - "FUNKWHALE_HOSTNAME_SUFFIX=funkwhale.test"
@@ -86,7 +86,7 @@ services:
       - "CACHE_URL=redis://redis:6379/0"
     volumes:
       - ./api:/app
-      - "${MUSIC_DIRECTORY_PATH-./data/music}:/music:ro"
+      - "${MUSIC_DIRECTORY_SERVE_PATH-./data/music}:/music:ro"
     networks:
       - internal
   nginx:
@@ -109,7 +109,7 @@ services:
     volumes:
       - ./docker/nginx/conf.dev:/etc/nginx/nginx.conf.template:ro
       - ./docker/nginx/entrypoint.sh:/entrypoint.sh:ro
-      - "${MUSIC_DIRECTORY_PATH-./data/music}:/music:ro"
+      - "${MUSIC_DIRECTORY_SERVE_PATH-./data/music}:/music:ro"
       - ./deploy/funkwhale_proxy.conf:/etc/nginx/funkwhale_proxy.conf:ro
       - "${MEDIA_ROOT-./api/funkwhale_api/media}:/protected/media:ro"
     networks: