From e87e2654e8932bdefe4b738e0b5c353f8c18c441 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Mon, 19 Mar 2018 15:28:33 +0100
Subject: [PATCH] Permissions and db state fixes with new index field
---
api/funkwhale_api/playlists/models.py | 16 +++++-
api/funkwhale_api/playlists/serializers.py | 35 +++++++++++-
api/funkwhale_api/playlists/views.py | 25 ++++-----
api/tests/playlists/test_models.py | 15 ++++++
api/tests/playlists/test_serializers.py | 62 +++++++++++++++++++++-
api/tests/playlists/test_views.py | 15 ++++++
6 files changed, 148 insertions(+), 20 deletions(-)
diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py
index b400934722..d8c90aef9a 100644
--- a/api/funkwhale_api/playlists/models.py
+++ b/api/funkwhale_api/playlists/models.py
@@ -67,13 +67,18 @@ class Playlist(models.Model):
plt.save(update_fields=['index'])
return index
+ def remove(self, index):
+ existing = self.playlist_tracks.select_for_update()
+ to_update = existing.filter(index__gt=index)
+ return to_update.update(index=models.F('index') - 1)
+
class PlaylistTrack(models.Model):
track = models.ForeignKey(
'music.Track',
related_name='playlist_tracks',
on_delete=models.CASCADE)
- index = models.PositiveIntegerField(null=True)
+ index = models.PositiveIntegerField(null=True, blank=True)
playlist = models.ForeignKey(
Playlist, related_name='playlist_tracks', on_delete=models.CASCADE)
creation_date = models.DateTimeField(default=timezone.now)
@@ -81,3 +86,12 @@ class PlaylistTrack(models.Model):
class Meta:
ordering = ('-playlist', 'index')
unique_together = ('playlist', 'index')
+
+ def delete(self, *args, **kwargs):
+ playlist = self.playlist
+ index = self.index
+ update_indexes = kwargs.pop('update_indexes', False)
+ r = super().delete(*args, **kwargs)
+ if index is not None and update_indexes:
+ playlist.remove(index)
+ return r
diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py
index 55603ee318..43e2414d40 100644
--- a/api/funkwhale_api/playlists/serializers.py
+++ b/api/funkwhale_api/playlists/serializers.py
@@ -1,4 +1,5 @@
from django.conf import settings
+from django.db import transaction
from rest_framework import serializers
from taggit.models import Tag
@@ -12,16 +13,23 @@ class PlaylistTrackSerializer(serializers.ModelSerializer):
class Meta:
model = models.PlaylistTrack
- fields = ('id', 'track', 'playlist', 'position')
+ fields = ('id', 'track', 'playlist', 'index', 'creation_date')
-class PlaylistTrackCreateSerializer(serializers.ModelSerializer):
+class PlaylistTrackWriteSerializer(serializers.ModelSerializer):
+ index = serializers.IntegerField(
+ required=False, min_value=0, allow_null=True)
class Meta:
model = models.PlaylistTrack
fields = ('id', 'track', 'playlist', 'index')
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()
if existing >= settings.PLAYLISTS_MAX_TRACKS:
raise serializers.ValidationError(
@@ -29,6 +37,29 @@ class PlaylistTrackCreateSerializer(serializers.ModelSerializer):
settings.PLAYLISTS_MAX_TRACKS))
return value
+ @transaction.atomic
+ def create(self, validated_data):
+ index = validated_data.pop('index', None)
+ instance = super().create(validated_data)
+ instance.playlist.insert(instance, index)
+ return instance
+
+ @transaction.atomic
+ def update(self, instance, validated_data):
+ update_index = 'index' in validated_data
+ index = validated_data.pop('index', None)
+ super().update(instance, validated_data)
+ if update_index:
+ instance.playlist.insert(instance, index)
+ 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):
diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py
index 3307b52765..02e53cb169 100644
--- a/api/funkwhale_api/playlists/views.py
+++ b/api/funkwhale_api/playlists/views.py
@@ -27,6 +27,7 @@ class PlaylistViewSet(
permissions.OwnerPermission,
IsAuthenticatedOrReadOnly,
]
+ owner_checks = ['write']
@detail_route(methods=['get'])
def tracks(self, request, *args, **kwargs):
@@ -66,25 +67,19 @@ class PlaylistTrackViewSet(
permissions.OwnerPermission,
IsAuthenticatedOrReadOnly,
]
+ owner_field = 'playlist.user'
+ owner_checks = ['write']
- def create(self, request, *args, **kwargs):
- serializer = serializers.PlaylistTrackCreateSerializer(
- data=request.data)
- serializer.is_valid(raise_exception=True)
- if serializer.validated_data['playlist'].user != request.user:
- return Response(
- {'playlist': [
- 'This playlist does not exists or you do not have the'
- 'permission to edit it']
- },
- status=400)
- instance = self.perform_create(serializer)
- serializer = self.get_serializer(instance=instance)
- headers = self.get_success_headers(serializer.data)
- return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers)
+ def get_serializer_class(self):
+ if self.request.method in ['PUT', 'PATCH', 'DELETE', 'POST']:
+ return serializers.PlaylistTrackWriteSerializer
+ return self.serializer_class
def get_queryset(self):
return self.queryset.filter(
fields.privacy_level_query(
self.request.user,
lookup_field='playlist__privacy_level'))
+
+ def perform_destroy(self, instance):
+ instance.delete(update_indexes=True)
diff --git a/api/tests/playlists/test_models.py b/api/tests/playlists/test_models.py
index ab60cdb849..dfe40187d8 100644
--- a/api/tests/playlists/test_models.py
+++ b/api/tests/playlists/test_models.py
@@ -72,3 +72,18 @@ def test_cannot_insert_at_negative_index(factories):
new = factories['playlists.PlaylistTrack'](playlist=plt.playlist)
with pytest.raises(forms.ValidationError):
plt.playlist.insert(new, -1)
+
+
+def test_remove_update_indexes(factories):
+ playlist = factories['playlists.Playlist']()
+ first = factories['playlists.PlaylistTrack'](playlist=playlist, index=0)
+ second = factories['playlists.PlaylistTrack'](playlist=playlist, index=1)
+ third = factories['playlists.PlaylistTrack'](playlist=playlist, index=2)
+
+ second.delete(update_indexes=True)
+
+ first.refresh_from_db()
+ third.refresh_from_db()
+
+ assert first.index == 0
+ assert third.index == 1
diff --git a/api/tests/playlists/test_serializers.py b/api/tests/playlists/test_serializers.py
index 6daff8e609..8e30919e6e 100644
--- a/api/tests/playlists/test_serializers.py
+++ b/api/tests/playlists/test_serializers.py
@@ -1,16 +1,74 @@
+from funkwhale_api.playlists import models
from funkwhale_api.playlists import serializers
-def test_cannot_max_500_tracks_per_playlist(mocker, factories, settings):
+def test_cannot_max_500_tracks_per_playlist(factories, settings):
settings.PLAYLISTS_MAX_TRACKS = 2
playlist = factories['playlists.Playlist']()
plts = factories['playlists.PlaylistTrack'].create_batch(
size=2, playlist=playlist)
track = factories['music.Track']()
- serializer = serializers.PlaylistTrackCreateSerializer(data={
+ 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)
+ 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)
+ 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']()
+ track = 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)
+ assert plt.index == 0
+ assert first.index == 1
diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py
index 3c62dcda65..192aad3824 100644
--- a/api/tests/playlists/test_views.py
+++ b/api/tests/playlists/test_views.py
@@ -80,6 +80,21 @@ def test_only_can_add_track_on_own_playlist_via_api(
assert playlist.playlist_tracks.count() == 0
+def test_deleting_plt_updates_indexes(
+ mocker, factories, logged_in_api_client):
+ remove = mocker.spy(models.Playlist, 'remove')
+ track = 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})
+
+ response = logged_in_api_client.delete(url)
+
+ assert response.status_code == 204
+ remove.assert_called_once_with(plt.playlist, 0)
+
+
@pytest.mark.parametrize('level', ['instance', 'me', 'followers'])
def test_playlist_privacy_respected_in_list_anon(level, factories, api_client):
factories['playlists.Playlist'](privacy_level=level)
--
GitLab