From a493d34b8f31e58c62394792d54c52fa885208f4 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Thu, 6 Dec 2018 08:53:31 +0000 Subject: [PATCH] Resolve "Track position don't take care about disc number" --- api/funkwhale_api/federation/serializers.py | 2 ++ api/funkwhale_api/music/metadata.py | 18 ++++++++++++------ .../music/migrations/0036_track_disc_number.py | 18 ++++++++++++++++++ api/funkwhale_api/music/models.py | 9 ++++++++- api/funkwhale_api/music/serializers.py | 7 +++---- api/funkwhale_api/music/tasks.py | 2 ++ api/funkwhale_api/music/views.py | 6 ++++-- api/tests/federation/test_serializers.py | 7 ++++++- api/tests/music/test_metadata.py | 6 ++++++ api/tests/music/test_models.py | 10 ++++++++++ api/tests/music/test_serializers.py | 6 ++++-- api/tests/music/test_tasks.py | 5 +++++ changes/changelog.d/507.enhancement | 1 + dev.yml | 2 +- 14 files changed, 82 insertions(+), 17 deletions(-) create mode 100644 api/funkwhale_api/music/migrations/0036_track_disc_number.py create mode 100644 changes/changelog.d/507.enhancement diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 40d4833c8..6c4ffeb58 100644 --- 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, diff --git a/api/funkwhale_api/music/metadata.py b/api/funkwhale_api/music/metadata.py index 52315d905..9a61e67e1 100644 --- 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", diff --git a/api/funkwhale_api/music/migrations/0036_track_disc_number.py b/api/funkwhale_api/music/migrations/0036_track_disc_number.py new file mode 100644 index 000000000..d40ec5e8d --- /dev/null +++ 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), + ), + ] diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 97c74a9a8..ff7561b4b 100644 --- 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 diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py index 228f1c231..6157c5a75 100644 --- 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", diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 16fe0573c..ea3f0f57b 100644 --- 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")), diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index 022b60f05..19a00884b 100644 --- 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() diff --git a/api/tests/federation/test_serializers.py b/api/tests/federation/test_serializers.py index c313d96a2..fe0485b52 100644 --- 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"] diff --git a/api/tests/music/test_metadata.py b/api/tests/music/test_metadata.py index f9c6149dd..4413f3a3b 100644 --- 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")), diff --git a/api/tests/music/test_models.py b/api/tests/music/test_models.py index cabe15260..500d69886 100644 --- 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] diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py index 4244691c1..155f99890 100644 --- 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(), diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 1af3c1ba4..b7b04674f 100644 --- 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 diff --git a/changes/changelog.d/507.enhancement b/changes/changelog.d/507.enhancement new file mode 100644 index 000000000..c21f51d81 --- /dev/null +++ b/changes/changelog.d/507.enhancement @@ -0,0 +1 @@ +Store disc number and order tracks by disc number / position) (#507) \ No newline at end of file diff --git a/dev.yml b/dev.yml index 5ac74424c..a77edb7db 100644 --- 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"