From f6458fd75a72f0e17c9af5822b05148f32c64386 Mon Sep 17 00:00:00 2001 From: Agate Date: Mon, 27 Jul 2020 15:31:49 +0200 Subject: [PATCH] Updated playlist management API --- api/config/api_urls.py | 3 - api/funkwhale_api/playlists/models.py | 9 ++ api/funkwhale_api/playlists/serializers.py | 55 +-------- api/funkwhale_api/playlists/views.py | 67 +++++------ api/tests/playlists/test_serializers.py | 90 +-------------- api/tests/playlists/test_views.py | 105 ++++++++---------- api/tests/users/oauth/test_api_permissions.py | 1 - docs/swagger.yml | 58 +++++++++- front/src/components/playlists/Editor.vue | 11 +- .../components/playlists/PlaylistModal.vue | 5 +- 10 files changed, 157 insertions(+), 247 deletions(-) diff --git a/api/config/api_urls.py b/api/config/api_urls.py index c96ca60c9..909dff45e 100644 --- a/api/config/api_urls.py +++ b/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 diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index affa86ea5..a20dbc937 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/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( diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 727edd525..07a7298a7 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/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) diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 2cc348f46..e4b1d3037 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/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( - mixins.RetrieveModelMixin, - mixins.CreateModelMixin, - mixins.UpdateModelMixin, - mixins.DestroyModelMixin, - mixins.ListModelMixin, - viewsets.GenericViewSet, -): + try: + plt = playlist.playlist_tracks.by_index(index) + except models.PlaylistTrack.DoesNotExist: + return Response(status=404) + plt.delete(update_indexes=True) - serializer_class = serializers.PlaylistTrackSerializer - 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"] + return Response(status=204) - def get_serializer_class(self): - if self.request.method in ["PUT", "PATCH", "DELETE", "POST"]: - return serializers.PlaylistTrackWriteSerializer - return self.serializer_class + @action(methods=["post"], detail=True) + @transaction.atomic + def move(self, request, *args, **kwargs): + 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): - return self.queryset.filter( - fields.privacy_level_query( - self.request.user, - lookup_field="playlist__privacy_level", - user_field="playlist__user", - ) - ).for_nested_serialization(music_utils.get_actor_from_request(self.request)) + try: + to_index = int(request.data["to"]) + assert to_index >= 0 + except (KeyError, ValueError, AssertionError, TypeError): + return Response({"detail": "invalid to index"}, status=400) - def perform_destroy(self, instance): - instance.delete(update_indexes=True) + try: + 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) diff --git a/api/tests/playlists/test_serializers.py b/api/tests/playlists/test_serializers.py index ef89ea584..b1cae0f36 100644 --- a/api/tests/playlists/test_serializers.py +++ b/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) diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index 2be64b2bb..b71b242e5 100644 --- a/api/tests/playlists/test_views.py +++ b/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", - [("api:v1:playlist-tracks-list", "post"), ("api:v1:playlists-list", "post")], + "name,method", [("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") - data = {"playlist": playlist.pk, "track": track.pk} + url = reverse("api:v1:playlists-add", kwargs={"pk": playlist.pk}) + data = {"tracks": [track.pk]} - response = logged_in_api_client.post(url, data) - assert response.status_code == 400 + response = logged_in_api_client.post(url, data, format="json") + 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"]( - index=0, playlist__user=logged_in_api_client.user - ) - url = reverse("api:v1:playlist-tracks-detail", kwargs={"pk": plt.pk}) + 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-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 diff --git a/api/tests/users/oauth/test_api_permissions.py b/api/tests/users/oauth/test_api_permissions.py index b030803ee..68b352f7e 100644 --- a/api/tests/users/oauth/test_api_permissions.py +++ b/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"), diff --git a/docs/swagger.yml b/docs/swagger.yml index d8a41471f..e2d364d25 100644 --- a/docs/swagger.yml +++ b/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 diff --git a/front/src/components/playlists/Editor.vue b/front/src/components/playlists/Editor.vue index 39c64d2cd..caa9a85c9 100644 --- a/front/src/components/playlists/Editor.vue +++ b/front/src/components/playlists/Editor.vue @@ -61,7 +61,7 @@
- +
{{ plt.index + 1}} @@ -125,20 +125,19 @@ export default { let self = this self.isLoading = true let plt = this.plts[newIndex] - let url = 'playlist-tracks/' + plt.id + '/' - axios.patch(url, {index: newIndex}).then((response) => { + let url = `playlists/${this.playlist.id}/move` + 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 + '/' - axios.delete(url).then((response) => { + let url = `playlists/${this.playlist.id}/remove` + axios.post(url, {index}).then((response) => { self.success() self.$store.dispatch('playlists/fetchOwn') }, error => { diff --git a/front/src/components/playlists/PlaylistModal.vue b/front/src/components/playlists/PlaylistModal.vue index 401c3c4f9..e22f234fb 100644 --- a/front/src/components/playlists/PlaylistModal.vue +++ b/front/src/components/playlists/PlaylistModal.vue @@ -139,14 +139,13 @@ export default { addToPlaylist (playlistId, allowDuplicate) { let self = this let payload = { - track: this.track.id, - playlist: playlistId, + tracks: [this.track.id], 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')