Merge branch '237-track-album-artist' into 'develop'

Resolve "Funkwhale unable to import Albums with multiple Artists" Closes #237 See merge request funkwhale/funkwhale!311
2018-07-09 20:47:55 +00:00 · 2018-07-09 20:47:55 +00:00 · f18a04b490
parent ce5502cab7 1458c084a7
commit f18a04b490
14 changed files with 180 additions and 47 deletions
--- 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)
--- 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,
            },
        )

--- 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)
--- 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)
--- 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,
--- 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):
--- a/changes/changelog.d/237.enhancement
+++ 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.
--- 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
    })
--- a/front/src/components/audio/track/Row.vue
+++ b/front/src/components/audio/track/Row.vue
@ -16,9 +16,18 @@
      </router-link>
    </td>
    <td colspan="6">
-      <router-link class="artist discrete link" :to="{name: 'library.artists.detail', params: {id: track.artist.id }}">
+      <router-link v-if="track.artist.id === albumArtist.id" class="artist discrete link" :to="{name: 'library.artists.detail', params: {id: track.artist.id }}">
        {{ track.artist.name }}
      </router-link>
+      <template v-else>
+        <router-link class="artist discrete link" :to="{name: 'library.artists.detail', params: {id: albumArtist.id }}">
+          {{ albumArtist.name }}
+        </router-link>
+         /
+         <router-link class="artist discrete link" :to="{name: 'library.artists.detail', params: {id: track.artist.id }}">
+          {{ track.artist.name }}
+        </router-link>
+      </template>
    </td>
    <td colspan="6">
      <router-link class="album discrete link" :to="{name: 'library.albums.detail', params: {id: track.album.id }}">
@ -35,8 +44,6 @@
 </template>

 <script>
-import backend from '@/audio/backend'
-
 import TrackFavoriteIcon from '@/components/favorites/TrackFavoriteIcon'
 import TrackPlaylistIcon from '@/components/playlists/TrackPlaylistIcon'
 import PlayButton from '@/components/audio/PlayButton'
@ -44,6 +51,7 @@ import PlayButton from '@/components/audio/PlayButton'
 export default {
  props: {
    track: {type: Object, required: true},
+    artist: {type: Object, required: false},
    displayPosition: {type: Boolean, default: false}
  },
  components: {
@ -51,9 +59,13 @@ export default {
    TrackPlaylistIcon,
    PlayButton
  },
-  data () {
-    return {
-      backend: backend
+  computed: {
+    albumArtist () {
+      if (this.artist) {
+        return this.artist
+      } else {
+        return this.track.album.artist
+      }
    }
  }
 }
--- a/front/src/components/audio/track/Table.vue
+++ b/front/src/components/audio/track/Table.vue
@ -14,6 +14,7 @@
      <track-row
        :display-position="displayPosition"
        :track="track"
+        :artist="artist"
        :key="index + '-' + track.id"
        v-for="(track, index) in tracks"></track-row>
    </tbody>
@ -63,6 +64,7 @@ import Modal from '@/components/semantic/Modal'
 export default {
  props: {
    tracks: {type: Array, required: true},
+    artist: {type: Object, required: false},
    displayPosition: {type: Boolean, default: false}
  },
  components: {
--- a/front/src/components/library/Album.vue
+++ b/front/src/components/library/Album.vue
@ -43,7 +43,7 @@
        <h2>
          <translate>Tracks</translate>
        </h2>
-        <track-table v-if="album" :display-position="true" :tracks="album.tracks"></track-table>
+        <track-table v-if="album" :artist="album.artist" :display-position="true" :tracks="album.tracks"></track-table>
      </div>
    </template>
  </div>
--- a/front/src/components/library/Artist.vue
+++ b/front/src/components/library/Artist.vue
@ -15,7 +15,7 @@
                  tag="div"
                  translate-plural="%{ count } tracks in %{ albumsCount } albums"
                  :translate-n="totalTracks"
-                  :translate-params="{count: totalTracks, albumsCount: albums.length}">
+                  :translate-params="{count: totalTracks, albumsCount: totalAlbums}">
                  %{ count } track in %{ albumsCount } albums
                </translate>
              </div>
@ -40,7 +40,7 @@
      <div v-if="isLoadingAlbums" class="ui vertical stripe segment">
        <div :class="['ui', 'centered', 'active', 'inline', 'loader']"></div>
      </div>
-      <div v-else-if="albums" class="ui vertical stripe segment">
+      <div v-else-if="albums && albums.length > 0" class="ui vertical stripe segment">
        <h2>
          <translate>Albums by this artist</translate>
        </h2>
@ -50,33 +50,41 @@
          </div>
        </div>
      </div>
+      <div v-if="tracks.length > 0" class="ui vertical stripe segment">
+        <h2>
+          <translate>Tracks by this artist</translate>
+        </h2>
+        <track-table :display-position="true" :tracks="tracks"></track-table>
+      </div>
    </template>
  </div>
 </template>

 <script>
+import _ from 'lodash'
 import axios from 'axios'
 import logger from '@/logging'
 import backend from '@/audio/backend'
 import AlbumCard from '@/components/audio/album/Card'
 import RadioButton from '@/components/radios/Button'
 import PlayButton from '@/components/audio/PlayButton'
-
-const FETCH_URL = 'artists/'
+import TrackTable from '@/components/audio/track/Table'

 export default {
  props: ['id'],
  components: {
    AlbumCard,
    RadioButton,
-    PlayButton
+    PlayButton,
+    TrackTable
  },
  data () {
    return {
      isLoading: true,
      isLoadingAlbums: true,
      artist: null,
-      albums: null
+      albums: null,
+      tracks: []
    }
  },
  created () {
@ -86,14 +94,17 @@ export default {
    fetchData () {
      var self = this
      this.isLoading = true
-      let url = FETCH_URL + this.id + '/'
      logger.default.debug('Fetching artist "' + this.id + '"')
-      axios.get(url).then((response) => {
+      axios.get('tracks/', {params: {artist: this.id}}).then((response) => {
+        self.tracks = response.data.results
+      })
+      axios.get('artists/' + this.id + '/').then((response) => {
        self.artist = response.data
        self.isLoading = false
        self.isLoadingAlbums = true
        axios.get('albums/', {params: {artist: this.id, ordering: '-release_date'}}).then((response) => {
-          self.albums = JSON.parse(JSON.stringify(response.data.results)).map((album) => {
+          let parsed = JSON.parse(JSON.stringify(response.data.results))
+          self.albums = parsed.map((album) => {
            return backend.Album.clean(album)
          })

@ -108,12 +119,21 @@ export default {
        title: this.$gettext('Artist')
      }
    },
+    totalAlbums () {
+      let trackAlbums = _.uniqBy(this.tracks, (t) => {
+        return t.album.id
+      })
+      return this.albums.length + trackAlbums.length
+    },
    totalTracks () {
+      if (this.albums.length === 0) {
+        return 0 + this.tracks.length
+      }
      return this.albums.map((album) => {
        return album.tracks.length
      }).reduce((a, b) => {
        return a + b
-      })
+      }) + this.tracks.length
    },
    wikipediaUrl () {
      return 'https://en.wikipedia.org/w/index.php?search=' + this.artist.name
--- a/front/src/components/library/Home.vue
+++ b/front/src/components/library/Home.vue
@ -72,7 +72,8 @@ export default {
      var self = this
      this.isLoadingArtists = true
      let params = {
-        ordering: '-creation_date'
+        ordering: '-creation_date',
+        listenable: true
      }
      let url = ARTISTS_URL
      logger.default.time('Loading latest artists')
--- a/front/src/components/requests/Form.vue
+++ b/front/src/components/requests/Form.vue
@ -13,7 +13,7 @@
      </div>
      <div class="field">
        <label><translate>Comment</translate></label>
-        <textarea v-model="currentComment" rows="3" :placeholder="comentPlaceholder" maxlength="2000"></textarea>
+        <textarea v-model="currentComment" rows="3" :placeholder="labels.commentPlaceholder" maxlength="2000"></textarea>
      </div>
      <button class="ui submit button" type="submit"><translate>Submit</translate></button>
    </form>