Merge branch '507-disc-number' into 'develop'

Resolve "Track position don't take care about disc number" Closes #507 See merge request funkwhale/funkwhale!486
2018-12-06 08:53:32 +00:00 · 2018-12-06 08:53:32 +00:00 · fd1ce6ef51
commit fd1ce6ef51
--- a/api/funkwhale_api/federation/serializers.py
+++ b/api/funkwhale_api/federation/serializers.py
@ -729,6 +729,7 @@ class AlbumSerializer(MusicEntitySerializer):

 class TrackSerializer(MusicEntitySerializer):
    position = serializers.IntegerField(min_value=0, allow_null=True, required=False)
+    disc = serializers.IntegerField(min_value=1, allow_null=True, required=False)
    artists = serializers.ListField(child=ArtistSerializer(), min_length=1)
    album = AlbumSerializer()
    license = serializers.URLField(allow_null=True, required=False)
@ -742,6 +743,7 @@ class TrackSerializer(MusicEntitySerializer):
            "published": instance.creation_date.isoformat(),
            "musicbrainzId": str(instance.mbid) if instance.mbid else None,
            "position": instance.position,
+            "disc": instance.disc_number,
            "license": instance.local_license["identifiers"][0]
            if instance.local_license
            else None,
--- a/api/funkwhale_api/music/metadata.py
+++ b/api/funkwhale_api/music/metadata.py
@ -92,7 +92,7 @@ def get_mp3_recording_id(f, k):
        raise TagNotFound(k)


-def convert_track_number(v):
+def convert_position(v):
    try:
        return int(v)
    except ValueError:
@ -156,8 +156,9 @@ CONF = {
        "fields": {
            "track_number": {
                "field": "TRACKNUMBER",
-                "to_application": convert_track_number,
+                "to_application": convert_position,
            },
+            "disc_number": {"field": "DISCNUMBER", "to_application": convert_position},
            "title": {},
            "artist": {},
            "album_artist": {
@ -179,8 +180,9 @@ CONF = {
        "fields": {
            "track_number": {
                "field": "TRACKNUMBER",
-                "to_application": convert_track_number,
+                "to_application": convert_position,
            },
+            "disc_number": {"field": "DISCNUMBER", "to_application": convert_position},
            "title": {},
            "artist": {},
            "album_artist": {
@ -202,8 +204,9 @@ CONF = {
        "fields": {
            "track_number": {
                "field": "TRACKNUMBER",
-                "to_application": convert_track_number,
+                "to_application": convert_position,
            },
+            "disc_number": {"field": "DISCNUMBER", "to_application": convert_position},
            "title": {},
            "artist": {},
            "album_artist": {"field": "albumartist"},
@ -222,7 +225,8 @@ CONF = {
        "getter": get_id3_tag,
        "clean_pictures": clean_id3_pictures,
        "fields": {
-            "track_number": {"field": "TRCK", "to_application": convert_track_number},
+            "track_number": {"field": "TRCK", "to_application": convert_position},
+            "disc_number": {"field": "TPOS", "to_application": convert_position},
            "title": {"field": "TIT2"},
            "artist": {"field": "TPE1"},
            "album_artist": {"field": "TPE2"},
@ -246,8 +250,9 @@ CONF = {
        "fields": {
            "track_number": {
                "field": "tracknumber",
-                "to_application": convert_track_number,
+                "to_application": convert_position,
            },
+            "disc_number": {"field": "discnumber", "to_application": convert_position},
            "title": {},
            "artist": {},
            "album_artist": {"field": "albumartist"},
@ -267,6 +272,7 @@ CONF = {

 ALL_FIELDS = [
    "track_number",
+    "disc_number",
    "title",
    "artist",
    "album_artist",
--- a/api/funkwhale_api/music/migrations/0036_track_disc_number.py
+++ b/api/funkwhale_api/music/migrations/0036_track_disc_number.py
@ -0,0 +1,18 @@
+# Generated by Django 2.0.9 on 2018-12-04 15:10
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('music', '0035_auto_20181203_1515'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='track',
+            name='disc_number',
+            field=models.PositiveIntegerField(blank=True, null=True),
+        ),
+    ]
--- a/api/funkwhale_api/music/models.py
+++ b/api/funkwhale_api/music/models.py
@ -440,6 +440,12 @@ class TrackQuerySet(models.QuerySet):
            models.Prefetch("uploads", queryset=uploads, to_attr="playable_uploads")
        )

+    def order_for_album(self):
+        """
+        Order by disc number then position
+        """
+        return self.order_by("disc_number", "position", "title")
+

 def get_artist(release_list):
    return Artist.get_or_create_from_api(
@ -450,6 +456,7 @@ def get_artist(release_list):
 class Track(APIModelMixin):
    title = models.CharField(max_length=255)
    artist = models.ForeignKey(Artist, related_name="tracks", on_delete=models.CASCADE)
+    disc_number = models.PositiveIntegerField(null=True, blank=True)
    position = models.PositiveIntegerField(null=True, blank=True)
    album = models.ForeignKey(
        Album, related_name="tracks", null=True, blank=True, on_delete=models.CASCADE
@ -485,7 +492,7 @@ class Track(APIModelMixin):
    tags = TaggableManager(blank=True)

    class Meta:
-        ordering = ["album", "position"]
+        ordering = ["album", "disc_number", "position"]

    def __str__(self):
        return self.title
--- a/api/funkwhale_api/music/serializers.py
+++ b/api/funkwhale_api/music/serializers.py
@ -88,6 +88,7 @@ class AlbumTrackSerializer(serializers.ModelSerializer):
            "artist",
            "creation_date",
            "position",
+            "disc_number",
            "uploads",
            "listen_url",
            "duration",
@ -130,10 +131,7 @@ class AlbumSerializer(serializers.ModelSerializer):
        )

    def get_tracks(self, o):
-        ordered_tracks = sorted(
-            o.tracks.all(),
-            key=lambda v: (v.position, v.title) if v.position else (99999, v.title),
-        )
+        ordered_tracks = o.tracks.all()
        return AlbumTrackSerializer(ordered_tracks, many=True).data

    def get_is_playable(self, obj):
@ -193,6 +191,7 @@ class TrackSerializer(serializers.ModelSerializer):
            "artist",
            "creation_date",
            "position",
+            "disc_number",
            "lyrics",
            "uploads",
            "listen_url",
--- a/api/funkwhale_api/music/tasks.py
+++ b/api/funkwhale_api/music/tasks.py
@ -274,6 +274,7 @@ def federation_audio_track_to_metadata(payload):
        "title": payload["name"],
        "album": payload["album"]["name"],
        "track_number": payload["position"],
+        "disc_number": payload.get("disc"),
        "artist": payload["artists"][0]["name"],
        "album_artist": payload["album"]["artists"][0]["name"],
        "date": payload["album"].get("released"),
@ -497,6 +498,7 @@ def get_track_from_import_metadata(data):
        "mbid": track_mbid,
        "artist": artist,
        "position": track_number,
+        "disc_number": data.get("disc_number"),
        "fid": track_fid,
        "from_activity_id": from_activity_id,
        "license": licenses.match(data.get("license"), data.get("copyright")),
--- a/api/funkwhale_api/music/views.py
+++ b/api/funkwhale_api/music/views.py
@ -93,8 +93,10 @@ class AlbumViewSet(viewsets.ReadOnlyModelViewSet):

    def get_queryset(self):
        queryset = super().get_queryset()
-        tracks = models.Track.objects.select_related("artist").with_playable_uploads(
-            utils.get_actor_from_request(self.request)
+        tracks = (
+            models.Track.objects.select_related("artist")
+            .with_playable_uploads(utils.get_actor_from_request(self.request))
+            .order_for_album()
        )
        qs = queryset.prefetch_related(Prefetch("tracks", queryset=tracks))
        return qs.distinct()
--- a/api/tests/federation/test_serializers.py
+++ b/api/tests/federation/test_serializers.py
@ -632,7 +632,9 @@ def test_activity_pub_album_serializer_to_ap(factories):


 def test_activity_pub_track_serializer_to_ap(factories):
-    track = factories["music.Track"](license="cc-by-4.0", copyright="test")
+    track = factories["music.Track"](
+        license="cc-by-4.0", copyright="test", disc_number=3
+    )
    expected = {
        "@context": serializers.AP_CONTEXT,
        "published": track.creation_date.isoformat(),
@ -641,6 +643,7 @@ def test_activity_pub_track_serializer_to_ap(factories):
        "id": track.fid,
        "name": track.title,
        "position": track.position,
+        "disc": track.disc_number,
        "license": track.license.conf["identifiers"][0],
        "copyright": "test",
        "artists": [
@ -668,6 +671,7 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock):
        "musicbrainzId": str(uuid.uuid4()),
        "name": "Black in back",
        "position": 5,
+        "disc": 1,
        "album": {
            "type": "Album",
            "id": "http://hello.album",
@ -713,6 +717,7 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock):
    assert track.fid == data["id"]
    assert track.title == data["name"]
    assert track.position == data["position"]
+    assert track.disc_number == data["disc"]
    assert track.creation_date == published
    assert str(track.mbid) == data["musicbrainzId"]

--- a/api/tests/music/test_metadata.py
+++ b/api/tests/music/test_metadata.py
@ -20,6 +20,7 @@ def test_get_all_metadata_at_once():
        "album": "Peer Gynt Suite no. 1, op. 46",
        "date": datetime.date(2012, 8, 15),
        "track_number": 1,
+        "disc_number": 1,
        "musicbrainz_albumid": uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75"),
        "musicbrainz_recordingid": uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656"),
        "musicbrainz_artistid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"),
@ -40,6 +41,7 @@ def test_get_all_metadata_at_once():
        ("album", "Peer Gynt Suite no. 1, op. 46"),
        ("date", datetime.date(2012, 8, 15)),
        ("track_number", 1),
+        ("disc_number", 1),
        ("musicbrainz_albumid", uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75")),
        ("musicbrainz_recordingid", uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656")),
        ("musicbrainz_artistid", uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823")),
@ -67,6 +69,7 @@ def test_can_get_metadata_from_ogg_file(field, value):
        ("album", "Peer Gynt Suite no. 1, op. 46"),
        ("date", datetime.date(2012, 8, 15)),
        ("track_number", 1),
+        ("disc_number", 1),
        ("musicbrainz_albumid", uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75")),
        ("musicbrainz_recordingid", uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656")),
        ("musicbrainz_artistid", uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823")),
@ -94,6 +97,7 @@ def test_can_get_metadata_from_opus_file(field, value):
        ("album", "Ballast der Republik"),
        ("date", datetime.date(2012, 5, 4)),
        ("track_number", 1),
+        ("disc_number", 1),
        ("musicbrainz_albumid", uuid.UUID("1f0441ad-e609-446d-b355-809c445773cf")),
        ("musicbrainz_recordingid", uuid.UUID("124d0150-8627-46bc-bc14-789a3bc960c8")),
        ("musicbrainz_artistid", uuid.UUID("c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1")),
@ -122,6 +126,7 @@ def test_can_get_metadata_from_ogg_theora_file(field, value):
        ("album", "You Can't Stop Da Funk"),
        ("date", datetime.date(2006, 2, 7)),
        ("track_number", 2),
+        ("disc_number", 1),
        ("musicbrainz_albumid", uuid.UUID("ce40cdb1-a562-4fd8-a269-9269f98d4124")),
        ("musicbrainz_recordingid", uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcb")),
        ("musicbrainz_artistid", uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13")),
@ -163,6 +168,7 @@ def test_can_get_pictures(name):
        ("album", "The Slip"),
        ("date", datetime.date(2008, 5, 5)),
        ("track_number", 1),
+        ("disc_number", 1),
        ("musicbrainz_albumid", uuid.UUID("12b57d46-a192-499e-a91f-7da66790a1c1")),
        ("musicbrainz_recordingid", uuid.UUID("30f3f33e-8d0c-4e69-8539-cbd701d18f28")),
        ("musicbrainz_artistid", uuid.UUID("b7ffd2af-418f-4be2-bdd1-22f8b48613da")),
--- a/api/tests/music/test_models.py
+++ b/api/tests/music/test_models.py
@ -512,3 +512,13 @@ def test_can_create_license(db):
        redistribute=True,
        url="http://cc",
    )
+
+
+def test_track_order_for_album(factories):
+    album = factories["music.Album"]()
+    t1 = factories["music.Track"](album=album, position=1, disc_number=1)
+    t2 = factories["music.Track"](album=album, position=1, disc_number=2)
+    t3 = factories["music.Track"](album=album, position=2, disc_number=1)
+    t4 = factories["music.Track"](album=album, position=2, disc_number=2)
+
+    assert list(models.Track.objects.order_for_album()) == [t1, t3, t2, t4]
--- a/api/tests/music/test_serializers.py
+++ b/api/tests/music/test_serializers.py
@ -72,7 +72,7 @@ def test_artist_with_albums_serializer(factories, to_api_date):

 def test_album_track_serializer(factories, to_api_date):
    upload = factories["music.Upload"](
-        track__license="cc-by-4.0", track__copyright="test"
+        track__license="cc-by-4.0", track__copyright="test", track__disc_number=2
    )
    track = upload.track
    setattr(track, "playable_uploads", [upload])
@ -84,6 +84,7 @@ def test_album_track_serializer(factories, to_api_date):
        "mbid": str(track.mbid),
        "title": track.title,
        "position": track.position,
+        "disc_number": track.disc_number,
        "uploads": [serializers.TrackUploadSerializer(upload).data],
        "creation_date": to_api_date(track.creation_date),
        "listen_url": track.listen_url,
@ -174,7 +175,7 @@ def test_album_serializer(factories, to_api_date):

 def test_track_serializer(factories, to_api_date):
    upload = factories["music.Upload"](
-        track__license="cc-by-4.0", track__copyright="test"
+        track__license="cc-by-4.0", track__copyright="test", track__disc_number=2
    )
    track = upload.track
    setattr(track, "playable_uploads", [upload])
@ -185,6 +186,7 @@ def test_track_serializer(factories, to_api_date):
        "mbid": str(track.mbid),
        "title": track.title,
        "position": track.position,
+        "disc_number": track.disc_number,
        "uploads": [serializers.TrackUploadSerializer(upload).data],
        "creation_date": to_api_date(track.creation_date),
        "lyrics": track.get_lyrics_url(),
--- a/api/tests/music/test_tasks.py
+++ b/api/tests/music/test_tasks.py
@ -23,6 +23,7 @@ def test_can_create_track_from_file_metadata_no_mbid(db, mocker):
        "album": "Test album",
        "date": datetime.date(2012, 8, 15),
        "track_number": 4,
+        "disc_number": 2,
        "license": "Hello world: http://creativecommons.org/licenses/by-sa/4.0/",
        "copyright": "2018 Someone",
    }
@ -34,6 +35,7 @@ def test_can_create_track_from_file_metadata_no_mbid(db, mocker):
    assert track.title == metadata["title"]
    assert track.mbid is None
    assert track.position == 4
+    assert track.disc_number == 2
    assert track.license.code == "cc-by-sa-4.0"
    assert track.copyright == metadata["copyright"]
    assert track.album.title == metadata["album"]
@ -66,6 +68,7 @@ def test_can_create_track_from_file_metadata_mbid(factories, mocker):
    assert track.title == metadata["title"]
    assert track.mbid == metadata["musicbrainz_recordingid"]
    assert track.position == 4
+    assert track.disc_number is None
    assert track.album.title == metadata["album"]
    assert track.album.mbid == metadata["musicbrainz_albumid"]
    assert track.album.artist.mbid == metadata["musicbrainz_albumartistid"]
@ -402,6 +405,7 @@ def test_federation_audio_track_to_metadata(now):
        "musicbrainzId": str(uuid.uuid4()),
        "name": "Black in back",
        "position": 5,
+        "disc": 2,
        "published": published.isoformat(),
        "license": "http://creativecommons.org/licenses/by-sa/4.0/",
        "copyright": "2018 Someone",
@ -441,6 +445,7 @@ def test_federation_audio_track_to_metadata(now):
        "title": payload["name"],
        "date": released,
        "track_number": payload["position"],
+        "disc_number": payload["disc"],
        "license": "http://creativecommons.org/licenses/by-sa/4.0/",
        "copyright": "2018 Someone",
        # musicbrainz
--- a/changes/changelog.d/507.enhancement
+++ b/changes/changelog.d/507.enhancement
@ -0,0 +1 @@
+Store disc number and order tracks by disc number / position) (#507)
--- a/dev.yml
+++ b/dev.yml
@ -29,7 +29,7 @@ services:
    env_file:
      - .env.dev
      - .env
-    image: postgres
+    image: postgres:9.6
    command: postgres -c log_min_duration_statement=0
    volumes:
      - "./data/${COMPOSE_PROJECT_NAME-node1}/postgres:/var/lib/postgresql/data"
				`@ -0,0 +1 @@`
				`Store disc number and order tracks by disc number / position) (#507)`