Merge branch '928-track-mbid-uniqueness' into 'develop'

Fix #348, #474, #557, #740, #928: improved deduplication logic to prevent skipped uploads

Closes #928, #740, #557, #474, and #348

See merge request funkwhale/funkwhale!1010

api/funkwhale_api/music/migrations/0050_auto_20200129_1344.py

@@ -0,0 +1,18 @@
+# Generated by Django 2.2.9 on 2020-01-29 13:44
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('music', '0049_auto_20200122_1020'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='track',
+            name='mbid',
+            field=models.UUIDField(blank=True, db_index=True, null=True),
+        ),
+    ]

api/funkwhale_api/music/models.py

@@ -475,6 +475,7 @@ def get_artist(release_list):
 
 
 class Track(APIModelMixin):
+    mbid = models.UUIDField(db_index=True, null=True, blank=True)
     title = models.CharField(max_length=MAX_LENGTHS["TRACK_TITLE"])
     artist = models.ForeignKey(Artist, related_name="tracks", on_delete=models.CASCADE)
     disc_number = models.PositiveIntegerField(null=True, blank=True)

api/funkwhale_api/music/tasks.py

@@ -625,9 +625,18 @@ def _get_track(data, attributed_to=None, **forced_values):
         else truncate(data.get("copyright"), models.MAX_LENGTHS["COPYRIGHT"])
     )
 
-    query = Q(title__iexact=track_title, artist=artist, album=album, position=position)
+    query = Q(
+        title__iexact=track_title,
+        artist=artist,
+        album=album,
+        position=position,
+        disc_number=disc_number,
+    )
     if track_mbid:
-        query |= Q(mbid=track_mbid)
+        if album_mbid:
+            query |= Q(mbid=track_mbid, album__mbid=album_mbid)
+        else:
+            query |= Q(mbid=track_mbid)
     if track_fid:
         query |= Q(fid=track_fid)
 

api/tests/music/test_tasks.py

@@ -1164,3 +1164,134 @@ def test_can_download_image_file_for_album_mbid(binary_cover, mocker, factories)
 
     assert album.attachment_cover.file.read() == binary_cover
     assert album.attachment_cover.mimetype == "image/jpeg"
+
+
+def test_can_import_track_with_same_mbid_in_different_albums(factories, mocker):
+    artist = factories["music.Artist"]()
+    upload = factories["music.Upload"](
+        playable=True, track__artist=artist, track__album__artist=artist
+    )
+    assert upload.track.mbid is not None
+    data = {
+        "title": upload.track.title,
+        "artists": [{"name": artist.name, "mbid": artist.mbid}],
+        "album": {
+            "title": "The Slip",
+            "mbid": uuid.UUID("12b57d46-a192-499e-a91f-7da66790a1c1"),
+            "release_date": datetime.date(2008, 5, 5),
+            "artists": [{"name": artist.name, "mbid": artist.mbid}],
+        },
+        "position": 1,
+        "disc_number": 1,
+        "mbid": upload.track.mbid,
+    }
+
+    mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data)
+    mocker.patch.object(tasks, "populate_album_cover")
+
+    new_upload = factories["music.Upload"](library=upload.library)
+
+    tasks.process_upload(upload_id=new_upload.pk)
+
+    new_upload.refresh_from_db()
+
+    assert new_upload.import_status == "finished"
+
+
+def test_import_track_with_same_mbid_in_same_albums_skipped(factories, mocker):
+    artist = factories["music.Artist"]()
+    upload = factories["music.Upload"](
+        playable=True, track__artist=artist, track__album__artist=artist
+    )
+    assert upload.track.mbid is not None
+    data = {
+        "title": upload.track.title,
+        "artists": [{"name": artist.name, "mbid": artist.mbid}],
+        "album": {
+            "title": upload.track.album.title,
+            "mbid": upload.track.album.mbid,
+            "artists": [{"name": artist.name, "mbid": artist.mbid}],
+        },
+        "position": 1,
+        "disc_number": 1,
+        "mbid": upload.track.mbid,
+    }
+
+    mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data)
+    mocker.patch.object(tasks, "populate_album_cover")
+
+    new_upload = factories["music.Upload"](library=upload.library)
+
+    tasks.process_upload(upload_id=new_upload.pk)
+
+    new_upload.refresh_from_db()
+
+    assert new_upload.import_status == "skipped"
+
+
+def test_can_import_track_with_same_position_in_different_discs(factories, mocker):
+    upload = factories["music.Upload"](playable=True)
+    artist_data = [
+        {
+            "name": upload.track.album.artist.name,
+            "mbid": upload.track.album.artist.mbid,
+        }
+    ]
+    data = {
+        "title": upload.track.title,
+        "artists": artist_data,
+        "album": {
+            "title": "The Slip",
+            "mbid": upload.track.album.mbid,
+            "release_date": datetime.date(2008, 5, 5),
+            "artists": artist_data,
+        },
+        "position": upload.track.position,
+        "disc_number": 2,
+        "mbid": None,
+    }
+
+    mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data)
+    mocker.patch.object(tasks, "populate_album_cover")
+
+    new_upload = factories["music.Upload"](library=upload.library)
+
+    tasks.process_upload(upload_id=new_upload.pk)
+
+    new_upload.refresh_from_db()
+
+    assert new_upload.import_status == "finished"
+
+
+def test_can_import_track_with_same_position_in_same_discs_skipped(factories, mocker):
+    upload = factories["music.Upload"](playable=True)
+    artist_data = [
+        {
+            "name": upload.track.album.artist.name,
+            "mbid": upload.track.album.artist.mbid,
+        }
+    ]
+    data = {
+        "title": upload.track.title,
+        "artists": artist_data,
+        "album": {
+            "title": "The Slip",
+            "mbid": upload.track.album.mbid,
+            "release_date": datetime.date(2008, 5, 5),
+            "artists": artist_data,
+        },
+        "position": upload.track.position,
+        "disc_number": upload.track.disc_number,
+        "mbid": None,
+    }
+
+    mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data)
+    mocker.patch.object(tasks, "populate_album_cover")
+
+    new_upload = factories["music.Upload"](library=upload.library)
+
+    tasks.process_upload(upload_id=new_upload.pk)
+
+    new_upload.refresh_from_db()
+
+    assert new_upload.import_status == "skipped"

changes/changelog.d/348.bugfix

@@ -0,0 +1 @@
+Improved deduplication logic to prevent skipped files during import (#348, #474, #557, #740, #928)

front/src/views/admin/library/ArtistDetail.vue

@@ -9,7 +9,7 @@
           <div class="ui column">
             <div class="segment-content">
               <h2 class="ui header">
-                <img v-if="object.cover" v-lazy="$store.getters['instance/absoluteUrl'](object.cover.square_crop)">
+                <img v-if="object.cover && object.cover.square_crop" v-lazy="$store.getters['instance/absoluteUrl'](object.cover.square_crop)">
                 <img v-else src="../../../assets/audio/default-cover.png">
                 <div class="content">
                   {{ object.name | truncate(100) }}