Updated playlist management API

api/config/api_urls.py

@@ -26,9 +26,6 @@ router.register(r"subscriptions", audio_views.SubscriptionsViewSet, "subscriptio
 router.register(r"albums", views.AlbumViewSet, "albums")
 router.register(r"licenses", views.LicenseViewSet, "licenses")
 router.register(r"playlists", playlists_views.PlaylistViewSet, "playlists")
-router.register(
-    r"playlist-tracks", playlists_views.PlaylistTrackViewSet, "playlist-tracks"
-)
 router.register(r"mutations", common_views.MutationViewSet, "mutations")
 router.register(r"attachments", common_views.AttachmentViewSet, "attachments")
 v1_patterns = router.urls

api/funkwhale_api/playlists/models.py

@@ -203,6 +203,15 @@ class PlaylistTrackQuerySet(models.QuerySet):
         else:
             return self.exclude(track__pk__in=tracks).distinct()
 
+    def by_index(self, index):
+        plts = self.order_by("index").values_list("id", flat=True)
+        try:
+            plt_id = plts[index]
+        except IndexError:
+            raise PlaylistTrack.DoesNotExist
+
+        return PlaylistTrack.objects.get(pk=plt_id)
+
 
 class PlaylistTrack(models.Model):
     track = models.ForeignKey(

api/funkwhale_api/playlists/serializers.py

@@ -1,7 +1,5 @@
-from django.db import transaction
 from rest_framework import serializers
 
-from funkwhale_api.common import preferences
 from funkwhale_api.federation import serializers as federation_serializers
 from funkwhale_api.music.models import Track
 from funkwhale_api.music.serializers import TrackSerializer
@@ -16,64 +14,13 @@ class PlaylistTrackSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = models.PlaylistTrack
-        fields = ("id", "track", "playlist", "index", "creation_date")
+        fields = ("track", "index", "creation_date")
 
     def get_track(self, o):
         track = o._prefetched_track if hasattr(o, "_prefetched_track") else o.track
         return TrackSerializer(track).data
 
 
-class PlaylistTrackWriteSerializer(serializers.ModelSerializer):
-    index = serializers.IntegerField(required=False, min_value=0, allow_null=True)
-    allow_duplicates = serializers.BooleanField(required=False)
-
-    class Meta:
-        model = models.PlaylistTrack
-        fields = ("id", "track", "playlist", "index", "allow_duplicates")
-
-    def validate_playlist(self, value):
-        if self.context.get("request"):
-            # validate proper ownership on the playlist
-            if self.context["request"].user != value.user:
-                raise serializers.ValidationError(
-                    "You do not have the permission to edit this playlist"
-                )
-        existing = value.playlist_tracks.count()
-        max_tracks = preferences.get("playlists__max_tracks")
-        if existing >= max_tracks:
-            raise serializers.ValidationError(
-                "Playlist has reached the maximum of {} tracks".format(max_tracks)
-            )
-        return value
-
-    @transaction.atomic
-    def create(self, validated_data):
-        index = validated_data.pop("index", None)
-        allow_duplicates = validated_data.pop("allow_duplicates", True)
-        instance = super().create(validated_data)
-
-        instance.playlist.insert(instance, index, allow_duplicates)
-        return instance
-
-    @transaction.atomic
-    def update(self, instance, validated_data):
-        update_index = "index" in validated_data
-        index = validated_data.pop("index", None)
-        allow_duplicates = validated_data.pop("allow_duplicates", True)
-        super().update(instance, validated_data)
-        if update_index:
-            instance.playlist.insert(instance, index, allow_duplicates)
-
-        return instance
-
-    def get_unique_together_validators(self):
-        """
-        We explicitely disable unique together validation here
-        because it collides with our internal logic
-        """
-        return []
-
-
 class PlaylistSerializer(serializers.ModelSerializer):
     tracks_count = serializers.SerializerMethodField(read_only=True)
     duration = serializers.SerializerMethodField(read_only=True)

api/funkwhale_api/playlists/views.py

@@ -93,40 +93,43 @@ class PlaylistViewSet(
             ),
         )
 
+    @action(methods=["post", "delete"], detail=True)
+    @transaction.atomic
+    def remove(self, request, *args, **kwargs):
+        playlist = self.get_object()
+        try:
+            index = int(request.data["index"])
+            assert index >= 0
+        except (KeyError, ValueError, AssertionError, TypeError):
+            return Response(status=400)
 
-class PlaylistTrackViewSet(
+        try:
-    mixins.RetrieveModelMixin,
+            plt = playlist.playlist_tracks.by_index(index)
-    mixins.CreateModelMixin,
+        except models.PlaylistTrack.DoesNotExist:
-    mixins.UpdateModelMixin,
+            return Response(status=404)
-    mixins.DestroyModelMixin,
+        plt.delete(update_indexes=True)
-    mixins.ListModelMixin,
-    viewsets.GenericViewSet,
-):
 
-    serializer_class = serializers.PlaylistTrackSerializer
+        return Response(status=204)
-    queryset = models.PlaylistTrack.objects.all()
-    permission_classes = [
-        oauth_permissions.ScopePermission,
-        permissions.OwnerPermission,
-    ]
-    required_scope = "playlists"
-    anonymous_policy = "setting"
-    owner_field = "playlist.user"
-    owner_checks = ["write"]
 
-    def get_serializer_class(self):
+    @action(methods=["post"], detail=True)
-        if self.request.method in ["PUT", "PATCH", "DELETE", "POST"]:
+    @transaction.atomic
-            return serializers.PlaylistTrackWriteSerializer
+    def move(self, request, *args, **kwargs):
-        return self.serializer_class
+        playlist = self.get_object()
+        try:
+            from_index = int(request.data["from"])
+            assert from_index >= 0
+        except (KeyError, ValueError, AssertionError, TypeError):
+            return Response({"detail": "invalid from index"}, status=400)
 
-    def get_queryset(self):
+        try:
-        return self.queryset.filter(
+            to_index = int(request.data["to"])
-            fields.privacy_level_query(
+            assert to_index >= 0
-                self.request.user,
+        except (KeyError, ValueError, AssertionError, TypeError):
-                lookup_field="playlist__privacy_level",
+            return Response({"detail": "invalid to index"}, status=400)
-                user_field="playlist__user",
-            )
-        ).for_nested_serialization(music_utils.get_actor_from_request(self.request))
 
-    def perform_destroy(self, instance):
+        try:
-        instance.delete(update_indexes=True)
+            plt = playlist.playlist_tracks.by_index(from_index)
+        except models.PlaylistTrack.DoesNotExist:
+            return Response(status=404)
+        playlist.insert(plt, to_index)
+        return Response(status=204)

api/tests/playlists/test_serializers.py

@@ -1,96 +1,8 @@
 from funkwhale_api.federation import serializers as federation_serializers
-from funkwhale_api.playlists import models, serializers
+from funkwhale_api.playlists import serializers
 from funkwhale_api.users import serializers as users_serializers
 
 
-def test_cannot_max_500_tracks_per_playlist(factories, preferences):
-    preferences["playlists__max_tracks"] = 2
-    playlist = factories["playlists.Playlist"]()
-    factories["playlists.PlaylistTrack"].create_batch(size=2, playlist=playlist)
-    track = factories["music.Track"]()
-    serializer = serializers.PlaylistTrackWriteSerializer(
-        data={"playlist": playlist.pk, "track": track.pk}
-    )
-
-    assert serializer.is_valid() is False
-    assert "playlist" in serializer.errors
-
-
-def test_create_insert_is_called_when_index_is_None(factories, mocker):
-    insert = mocker.spy(models.Playlist, "insert")
-    playlist = factories["playlists.Playlist"]()
-    track = factories["music.Track"]()
-    serializer = serializers.PlaylistTrackWriteSerializer(
-        data={"playlist": playlist.pk, "track": track.pk, "index": None}
-    )
-    assert serializer.is_valid() is True
-
-    plt = serializer.save()
-    insert.assert_called_once_with(playlist, plt, None, True)
-    assert plt.index == 0
-
-
-def test_create_insert_is_called_when_index_is_provided(factories, mocker):
-    playlist = factories["playlists.Playlist"]()
-    first = factories["playlists.PlaylistTrack"](playlist=playlist, index=0)
-    insert = mocker.spy(models.Playlist, "insert")
-    factories["playlists.Playlist"]()
-    track = factories["music.Track"]()
-    serializer = serializers.PlaylistTrackWriteSerializer(
-        data={"playlist": playlist.pk, "track": track.pk, "index": 0}
-    )
-    assert serializer.is_valid() is True
-
-    plt = serializer.save()
-    first.refresh_from_db()
-    insert.assert_called_once_with(playlist, plt, 0, True)
-    assert plt.index == 0
-    assert first.index == 1
-
-
-def test_update_insert_is_called_when_index_is_provided(factories, mocker):
-    playlist = factories["playlists.Playlist"]()
-    first = factories["playlists.PlaylistTrack"](playlist=playlist, index=0)
-    second = factories["playlists.PlaylistTrack"](playlist=playlist, index=1)
-    insert = mocker.spy(models.Playlist, "insert")
-    factories["playlists.Playlist"]()
-    factories["music.Track"]()
-    serializer = serializers.PlaylistTrackWriteSerializer(
-        second, data={"playlist": playlist.pk, "track": second.track.pk, "index": 0}
-    )
-    assert serializer.is_valid() is True
-
-    plt = serializer.save()
-    first.refresh_from_db()
-    insert.assert_called_once_with(playlist, plt, 0, True)
-    assert plt.index == 0
-    assert first.index == 1
-
-
-def test_update_insert_is_called_with_duplicate_override_when_duplicates_allowed(
-    factories, mocker
-):
-    playlist = factories["playlists.Playlist"]()
-    plt = factories["playlists.PlaylistTrack"](playlist=playlist, index=0)
-    insert = mocker.spy(models.Playlist, "insert")
-    factories["playlists.Playlist"]()
-    factories["music.Track"]()
-
-    serializer = serializers.PlaylistTrackWriteSerializer(
-        plt,
-        data={
-            "playlist": playlist.pk,
-            "track": plt.track.pk,
-            "index": 0,
-            "allow_duplicates": True,
-        },
-    )
-    assert serializer.is_valid() is True
-    plt = serializer.save()
-
-    insert.assert_called_once_with(playlist, plt, 0, True)
-
-
 def test_playlist_serializer_include_covers(factories, api_request):
     playlist = factories["playlists.Playlist"]()
     t1 = factories["music.Track"](album__with_cover=True)

api/tests/playlists/test_views.py

@@ -1,7 +1,7 @@
 import pytest
 from django.urls import reverse
 
-from funkwhale_api.playlists import models, serializers
+from funkwhale_api.playlists import models
 
 
 def test_can_create_playlist_via_api(logged_in_api_client):
@@ -60,21 +60,8 @@ def test_playlist_inherits_user_privacy(logged_in_api_client):
     assert playlist.privacy_level == user.privacy_level
 
 
-def test_can_add_playlist_track_via_api(factories, logged_in_api_client):
-    tracks = factories["music.Track"].create_batch(5)
-    playlist = factories["playlists.Playlist"](user=logged_in_api_client.user)
-    url = reverse("api:v1:playlist-tracks-list")
-    data = {"playlist": playlist.pk, "track": tracks[0].pk}
-
-    response = logged_in_api_client.post(url, data)
-    assert response.status_code == 201
-    plts = logged_in_api_client.user.playlists.latest("id").playlist_tracks.all()
-    assert plts.first().track == tracks[0]
-
-
 @pytest.mark.parametrize(
-    "name,method",
+    "name,method", [("api:v1:playlists-list", "post")],
-    [("api:v1:playlist-tracks-list", "post"), ("api:v1:playlists-list", "post")],
 )
 def test_url_requires_login(name, method, factories, api_client):
     url = reverse(name)
@@ -87,26 +74,30 @@ def test_url_requires_login(name, method, factories, api_client):
 def test_only_can_add_track_on_own_playlist_via_api(factories, logged_in_api_client):
     track = factories["music.Track"]()
     playlist = factories["playlists.Playlist"]()
-    url = reverse("api:v1:playlist-tracks-list")
+    url = reverse("api:v1:playlists-add", kwargs={"pk": playlist.pk})
-    data = {"playlist": playlist.pk, "track": track.pk}
+    data = {"tracks": [track.pk]}
 
-    response = logged_in_api_client.post(url, data)
+    response = logged_in_api_client.post(url, data, format="json")
-    assert response.status_code == 400
+    assert response.status_code == 404
     assert playlist.playlist_tracks.count() == 0
 
 
 def test_deleting_plt_updates_indexes(mocker, factories, logged_in_api_client):
     remove = mocker.spy(models.Playlist, "remove")
     factories["music.Track"]()
-    plt = factories["playlists.PlaylistTrack"](
+    playlist = factories["playlists.Playlist"](user=logged_in_api_client.user)
-        index=0, playlist__user=logged_in_api_client.user
+    plt0 = factories["playlists.PlaylistTrack"](index=0, playlist=playlist)
-    )
+    plt1 = factories["playlists.PlaylistTrack"](index=1, playlist=playlist)
-    url = reverse("api:v1:playlist-tracks-detail", kwargs={"pk": plt.pk})
+    url = reverse("api:v1:playlists-remove", kwargs={"pk": playlist.pk})
 
-    response = logged_in_api_client.delete(url)
+    response = logged_in_api_client.delete(url, {"index": 0})
 
     assert response.status_code == 204
-    remove.assert_called_once_with(plt.playlist, 0)
+    remove.assert_called_once_with(plt0.playlist, 0)
+    with pytest.raises(plt0.DoesNotExist):
+        plt0.refresh_from_db()
+    plt1.refresh_from_db()
+    assert plt1.index == 0
 
 
 @pytest.mark.parametrize("level", ["instance", "me", "followers"])
@@ -130,38 +121,6 @@ def test_only_owner_can_edit_playlist(method, factories, logged_in_api_client):
     assert response.status_code == 404
 
 
-@pytest.mark.parametrize("method", ["PUT", "PATCH", "DELETE"])
-def test_only_owner_can_edit_playlist_track(method, factories, logged_in_api_client):
-    plt = factories["playlists.PlaylistTrack"]()
-    url = reverse("api:v1:playlist-tracks-detail", kwargs={"pk": plt.pk})
-    response = getattr(logged_in_api_client, method.lower())(url)
-
-    assert response.status_code == 404
-
-
-@pytest.mark.parametrize("level", ["instance", "me", "followers"])
-def test_playlist_track_privacy_respected_in_list_anon(
-    level, factories, api_client, preferences
-):
-    preferences["common__api_authentication_required"] = False
-    factories["playlists.PlaylistTrack"](playlist__privacy_level=level)
-    url = reverse("api:v1:playlist-tracks-list")
-    response = api_client.get(url)
-
-    assert response.data["count"] == 0
-
-
-@pytest.mark.parametrize("level", ["instance", "me", "followers"])
-def test_can_list_tracks_from_playlist(level, factories, logged_in_api_client):
-    plt = factories["playlists.PlaylistTrack"](playlist__user=logged_in_api_client.user)
-    url = reverse("api:v1:playlists-tracks", kwargs={"pk": plt.playlist.pk})
-    response = logged_in_api_client.get(url)
-    serialized_plt = serializers.PlaylistTrackSerializer(plt).data
-
-    assert response.data["count"] == 1
-    assert response.data["results"][0] == serialized_plt
-
-
 def test_can_add_multiple_tracks_at_once_via_api(
     factories, mocker, logged_in_api_client
 ):
@@ -176,10 +135,24 @@ def test_can_add_multiple_tracks_at_once_via_api(
     assert playlist.playlist_tracks.count() == len(track_ids)
 
     for plt in playlist.playlist_tracks.order_by("index"):
-        assert response.data["results"][plt.index]["id"] == plt.id
+        assert response.data["results"][plt.index]["index"] == plt.index
         assert plt.track == tracks[plt.index]
 
 
+def test_honor_max_playlist_size(factories, mocker, logged_in_api_client, preferences):
+    preferences["playlists__max_tracks"] = 3
+    playlist = factories["playlists.Playlist"](user=logged_in_api_client.user)
+    tracks = factories["music.Track"].create_batch(
+        size=preferences["playlists__max_tracks"] + 1
+    )
+    track_ids = [t.id for t in tracks]
+    mocker.spy(playlist, "insert_many")
+    url = reverse("api:v1:playlists-add", kwargs={"pk": playlist.pk})
+    response = logged_in_api_client.post(url, {"tracks": track_ids})
+
+    assert response.status_code == 400
+
+
 def test_can_clear_playlist_from_api(factories, mocker, logged_in_api_client):
     playlist = factories["playlists.Playlist"](user=logged_in_api_client.user)
     factories["playlists.PlaylistTrack"].create_batch(size=5, playlist=playlist)
@@ -199,3 +172,19 @@ def test_update_playlist_from_api(factories, mocker, logged_in_api_client):
 
     assert response.status_code == 200
     assert response.data["user"]["username"] == playlist.user.username
+
+
+def test_move_plt_updates_indexes(mocker, factories, logged_in_api_client):
+    playlist = factories["playlists.Playlist"](user=logged_in_api_client.user)
+    plt0 = factories["playlists.PlaylistTrack"](index=0, playlist=playlist)
+    plt1 = factories["playlists.PlaylistTrack"](index=1, playlist=playlist)
+    url = reverse("api:v1:playlists-move", kwargs={"pk": playlist.pk})
+
+    response = logged_in_api_client.post(url, {"from": 1, "to": 0})
+
+    assert response.status_code == 204
+
+    plt0.refresh_from_db()
+    plt1.refresh_from_db()
+    assert plt0.index == 1
+    assert plt1.index == 0

api/tests/users/oauth/test_api_permissions.py

@@ -22,7 +22,6 @@ from funkwhale_api.users.oauth import scopes
         ("api:v1:listen-detail", {"uuid": uuid.uuid4()}, "read:libraries", "get"),
         ("api:v1:uploads-list", {}, "read:libraries", "get"),
         ("api:v1:playlists-list", {}, "read:playlists", "get"),
-        ("api:v1:playlist-tracks-list", {}, "read:playlists", "get"),
         ("api:v1:favorites:tracks-list", {}, "read:favorites", "get"),
         ("api:v1:history:listenings-list", {}, "read:listenings", "get"),
         ("api:v1:radios:radios-list", {}, "read:radios", "get"),

docs/swagger.yml

@@ -1,5 +1,4 @@
 # Undocumented endpoints:
-#  /api/v1/playlist-tracks
 #  /api/v1/radios
 #  /api/v1/history
 
@@ -1626,6 +1625,63 @@ paths:
                         type: "array"
                         items:
                           $ref: "./api/definitions.yml#/PlaylistTrack"
+  /api/v1/playlists/{id}/move:
+    parameters:
+      - name: id
+        in: path
+        required: true
+        schema:
+          type: "integer"
+          format: "int64"
+    post:
+      tags:
+        - "Content curation"
+      summary: Move a track to another index within its playlist
+      requestBody:
+        required: true
+        content:
+          application/json:
+            schema:
+              type: object
+              properties:
+                from:
+                  type: "integer"
+                  format: "int64"
+                  description: Current index of the track
+                to:
+                  type: "integer"
+                  format: "int64"
+                  description: New index of the track
+               
+      responses:
+        204:
+  /api/v1/playlists/{id}/remove:
+    parameters:
+      - name: id
+        in: path
+        required: true
+        schema:
+          type: "integer"
+          format: "int64"
+    post:
+      tags:
+        - "Content curation"
+      summary: Remove a track from its playlist
+      requestBody:
+        required: true
+        content:
+          application/json:
+            schema:
+              type: object
+              properties:
+                index:
+                  type: "integer"
+                  format: "int64"
+                  description: Index of the track to remove
+               
+      responses:
+        204:
+          
   /api/v1/playlists/{id}/clear:
     parameters:
       - name: id

front/src/components/playlists/Editor.vue

@@ -61,7 +61,7 @@
         <div class="table-wrapper">
           <table class="ui compact very basic unstackable table">
             <draggable v-model="plts" tag="tbody" @update="reorder">
-              <tr v-for="(plt, index) in plts" :key="plt.id">
+              <tr v-for="(plt, index) in plts" :key="`${index}-${plt.track.id}`">
                 <td class="left aligned">{{ plt.index + 1}}</td>
                 <td class="center aligned">
                   <img class="ui mini image" v-if="plt.track.album && plt.track.album.cover.original" v-lazy="$store.getters['instance/absoluteUrl'](plt.track.album.cover.small_square_crop)">
@@ -125,20 +125,19 @@ export default {
       let self = this
       self.isLoading = true
       let plt = this.plts[newIndex]
-      let url = 'playlist-tracks/' + plt.id + '/'
+      let url = `playlists/${this.playlist.id}/move`
-      axios.patch(url, {index: newIndex}).then((response) => {
+      axios.post(url, {from: oldIndex, to: newIndex}).then((response) => {
         self.success()
       }, error => {
         self.errored(error.backendErrors)
       })
     },
     removePlt (index) {
-      let plt = this.plts[index]
       this.plts.splice(index, 1)
       let self = this
       self.isLoading = true
-      let url = 'playlist-tracks/' + plt.id + '/'
+      let url = `playlists/${this.playlist.id}/remove`
-      axios.delete(url).then((response) => {
+      axios.post(url, {index}).then((response) => {
         self.success()
         self.$store.dispatch('playlists/fetchOwn')
       }, error => {

front/src/components/playlists/PlaylistModal.vue

@@ -139,14 +139,13 @@ export default {
     addToPlaylist (playlistId, allowDuplicate) {
       let self = this
       let payload = {
-        track: this.track.id,
+        tracks: [this.track.id],
-        playlist: playlistId,
         allow_duplicates: allowDuplicate
       }
 
       self.lastSelectedPlaylist = playlistId
 
-      return axios.post('playlist-tracks/', payload).then(response => {
+      return axios.post(`playlists/${playlistId}/add`, payload).then(response => {
         logger.default.info('Successfully added track to playlist')
         self.update(false)
         self.$store.dispatch('playlists/fetchOwn')