diff --git a/api/funkwhale_api/music/filters.py b/api/funkwhale_api/music/filters.py index 1f73fc9b0..87537b675 100644 --- a/api/funkwhale_api/music/filters.py +++ b/api/funkwhale_api/music/filters.py @@ -6,19 +6,9 @@ from funkwhale_api.common import fields from . import models -class ListenableMixin(filters.FilterSet): - listenable = filters.BooleanFilter(name="_", method="filter_listenable") - - def filter_listenable(self, queryset, name, value): - queryset = queryset.annotate(files_count=Count("tracks__files")) - if value: - return queryset.filter(files_count__gt=0) - else: - return queryset.filter(files_count=0) - - -class ArtistFilter(ListenableMixin): +class ArtistFilter(filters.FilterSet): q = fields.SearchFilter(search_fields=["name"]) + listenable = filters.BooleanFilter(name="_", method="filter_listenable") class Meta: model = models.Artist @@ -27,6 +17,13 @@ class ArtistFilter(ListenableMixin): "listenable": "exact", } + def filter_listenable(self, queryset, name, value): + queryset = queryset.annotate(files_count=Count("albums__tracks__files")) + if value: + return queryset.filter(files_count__gt=0) + else: + return queryset.filter(files_count=0) + class TrackFilter(filters.FilterSet): q = fields.SearchFilter(search_fields=["title", "album__title", "artist__name"]) @@ -72,10 +69,17 @@ class ImportJobFilter(filters.FilterSet): } -class AlbumFilter(ListenableMixin): +class AlbumFilter(filters.FilterSet): listenable = filters.BooleanFilter(name="_", method="filter_listenable") q = fields.SearchFilter(search_fields=["title", "artist__name" "source"]) class Meta: model = models.Album fields = ["listenable", "q", "artist"] + + def filter_listenable(self, queryset, name, value): + queryset = queryset.annotate(files_count=Count("tracks__files")) + if value: + return queryset.filter(files_count__gt=0) + else: + return queryset.filter(files_count=0) diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index d533d8525..207b22dfb 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -319,9 +319,10 @@ class Track(APIModelMixin): "mbid": {"musicbrainz_field_name": "id"}, "title": {"musicbrainz_field_name": "title"}, "artist": { - # we use the artist from the release to avoid #237 - "musicbrainz_field_name": "release-list", - "converter": get_artist, + "musicbrainz_field_name": "artist-credit", + "converter": lambda v: Artist.get_or_create_from_api( + mbid=v[0]["artist"]["id"] + )[0], }, "album": {"musicbrainz_field_name": "release-list", "converter": import_album}, } @@ -389,19 +390,37 @@ class Track(APIModelMixin): tracks = [t for m in data["release"]["medium-list"] for t in m["track-list"]] track_data = None for track in tracks: - if track["recording"]["id"] == mbid: + if track["recording"]["id"] == str(mbid): track_data = track break if not track_data: raise ValueError("No track found matching this ID") + track_artist_mbid = None + for ac in track_data["recording"]["artist-credit"]: + try: + ac_mbid = ac["artist"]["id"] + except TypeError: + # it's probably a string, like "feat." + continue + + if ac_mbid == str(album.artist.mbid): + continue + + track_artist_mbid = ac_mbid + break + track_artist_mbid = track_artist_mbid or album.artist.mbid + if track_artist_mbid == str(album.artist.mbid): + track_artist = album.artist + else: + track_artist = Artist.get_or_create_from_api(track_artist_mbid)[0] return cls.objects.update_or_create( mbid=mbid, defaults={ "position": int(track["position"]), "title": track["recording"]["title"], "album": album, - "artist": album.artist, + "artist": track_artist, }, ) diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py index c34970d0b..14ea54d51 100644 --- a/api/funkwhale_api/music/serializers.py +++ b/api/funkwhale_api/music/serializers.py @@ -60,8 +60,15 @@ class TrackFileSerializer(serializers.ModelSerializer): return url +class ArtistSimpleSerializer(serializers.ModelSerializer): + class Meta: + model = models.Artist + fields = ("id", "mbid", "name", "creation_date") + + class AlbumTrackSerializer(serializers.ModelSerializer): files = TrackFileSerializer(many=True, read_only=True) + artist = ArtistSimpleSerializer(read_only=True) class Meta: model = models.Track @@ -77,12 +84,6 @@ class AlbumTrackSerializer(serializers.ModelSerializer): ) -class ArtistSimpleSerializer(serializers.ModelSerializer): - class Meta: - model = models.Artist - fields = ("id", "mbid", "name", "creation_date") - - class AlbumSerializer(serializers.ModelSerializer): tracks = serializers.SerializerMethodField() artist = ArtistSimpleSerializer(read_only=True) diff --git a/api/tests/music/test_models.py b/api/tests/music/test_models.py index df18a0909..1bd4282fe 100644 --- a/api/tests/music/test_models.py +++ b/api/tests/music/test_models.py @@ -44,6 +44,7 @@ def test_import_album_stores_release_group(factories): def test_import_track_from_release(factories, mocker): album = factories["music.Album"](mbid="430347cb-0879-3113-9fde-c75b658c298e") + artist = factories["music.Artist"](mbid="a5211c65-2465-406b-93ec-213588869dc1") album_data = { "release": { "id": album.mbid, @@ -64,6 +65,9 @@ def test_import_track_from_release(factories, mocker): "id": "2109e376-132b-40ad-b993-2bb6812e19d4", "title": "Teen Age Riot", "length": "417973", + "artist-credit": [ + {"artist": {"id": artist.mbid, "name": artist.name}} + ], }, "track_or_recording_length": "417973", } @@ -84,10 +88,66 @@ def test_import_track_from_release(factories, mocker): assert track.title == track_data["recording"]["title"] assert track.mbid == track_data["recording"]["id"] assert track.album == album - assert track.artist == album.artist + assert track.artist == artist assert track.position == int(track_data["position"]) +def test_import_track_with_different_artist_than_release(factories, mocker): + album = factories["music.Album"](mbid="430347cb-0879-3113-9fde-c75b658c298e") + recording_data = { + "recording": { + "id": "94ab07eb-bdf3-4155-b471-ba1dc85108bf", + "title": "Flaming Red Hair", + "length": "159000", + "artist-credit": [ + { + "artist": { + "id": "a5211c65-2465-406b-93ec-213588869dc1", + "name": "Plan 9", + "sort-name": "Plan 9", + "disambiguation": "New Zealand group", + } + } + ], + "release-list": [ + { + "id": album.mbid, + "title": "The Lord of the Rings: The Fellowship of the Ring - The Complete Recordings", + "status": "Official", + "quality": "normal", + "text-representation": {"language": "eng", "script": "Latn"}, + "artist-credit": [ + { + "artist": { + "id": "9b58672a-e68e-4972-956e-a8985a165a1f", + "name": "Howard Shore", + "sort-name": "Shore, Howard", + } + } + ], + "date": "2005-12-13", + "country": "US", + "release-event-count": 1, + "barcode": "093624945420", + "artist-credit-phrase": "Howard Shore", + } + ], + "release-count": 3, + "artist-credit-phrase": "Plan 9", + } + } + artist = factories["music.Artist"](mbid="a5211c65-2465-406b-93ec-213588869dc1") + mocker.patch( + "funkwhale_api.musicbrainz.api.recordings.get", return_value=recording_data + ) + + track = models.Track.get_or_create_from_api(recording_data["recording"]["id"])[0] + assert track.title == recording_data["recording"]["title"] + assert track.mbid == recording_data["recording"]["id"] + assert track.album == album + assert track.artist == artist + + def test_import_job_is_bound_to_track_file(factories, mocker): track = factories["music.Track"]() job = factories["music.ImportJob"](mbid=track.mbid) diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py index 51ca96b5d..0d7400dfc 100644 --- a/api/tests/music/test_serializers.py +++ b/api/tests/music/test_serializers.py @@ -43,7 +43,7 @@ def test_album_track_serializer(factories, to_api_date): expected = { "id": track.id, - "artist": track.artist.id, + "artist": serializers.ArtistSimpleSerializer(track.artist).data, "album": track.album.id, "mbid": str(track.mbid), "title": track.title, diff --git a/api/tests/test_import_audio_file.py b/api/tests/test_import_audio_file.py index 43e596ff7..f63e69b63 100644 --- a/api/tests/test_import_audio_file.py +++ b/api/tests/test_import_audio_file.py @@ -37,6 +37,7 @@ def test_can_create_track_from_file_metadata_no_mbid(db, mocker): def test_can_create_track_from_file_metadata_mbid(factories, mocker): album = factories["music.Album"]() + artist = factories["music.Artist"]() mocker.patch( "funkwhale_api.music.models.Album.get_or_create_from_api", return_value=(album, True), @@ -55,6 +56,9 @@ def test_can_create_track_from_file_metadata_mbid(factories, mocker): "recording": { "id": "2109e376-132b-40ad-b993-2bb6812e19d4", "title": "Teen Age Riot", + "artist-credit": [ + {"artist": {"id": artist.mbid, "name": artist.name}} + ], }, } ], @@ -79,7 +83,7 @@ def test_can_create_track_from_file_metadata_mbid(factories, mocker): assert track.mbid == track_data["recording"]["id"] assert track.position == 4 assert track.album == album - assert track.artist == album.artist + assert track.artist == artist def test_management_command_requires_a_valid_username(factories, mocker): diff --git a/changes/changelog.d/237.enhancement b/changes/changelog.d/237.enhancement new file mode 100644 index 000000000..1b5eed8f2 --- /dev/null +++ b/changes/changelog.d/237.enhancement @@ -0,0 +1,11 @@ +Store track artist and album artist separately (#237) + +Better handling of tracks with a different artist than the album artist +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Some tracks involve a different artist than the album artist (e.g. a featuring) +and Funkwhale has been known to do weird things when importing such tracks, resulting +in albums that contained a single track, for instance. + +The situation should be improved with this release, as Funkwhale is now able to +store separately the track and album artist, and display it properly in the interface. diff --git a/front/src/audio/backend.js b/front/src/audio/backend.js index 5a82719a3..c37156675 100644 --- a/front/src/audio/backend.js +++ b/front/src/audio/backend.js @@ -2,7 +2,6 @@ var Album = { clean (album) { // we manually rebind the album and artist to each child track album.tracks = album.tracks.map((track) => { - track.artist = album.artist track.album = album return track }) diff --git a/front/src/components/audio/track/Row.vue b/front/src/components/audio/track/Row.vue index bd3ceb2aa..5870ac799 100644 --- a/front/src/components/audio/track/Row.vue +++ b/front/src/components/audio/track/Row.vue @@ -16,9 +16,18 @@ - + {{ track.artist.name }} + @@ -35,8 +44,6 @@