From 639cd5da5b01b04b938a32fe515b92ea9dd15995 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 16 Mar 2018 22:55:35 +0100 Subject: [PATCH 01/41] Moved playlist tests in a dedicated directory --- api/tests/playlists/test_models.py | 20 ++++++++++++++++++ .../test_views.py} | 21 ------------------- 2 files changed, 20 insertions(+), 21 deletions(-) create mode 100644 api/tests/playlists/test_models.py rename api/tests/{test_playlists.py => playlists/test_views.py} (63%) diff --git a/api/tests/playlists/test_models.py b/api/tests/playlists/test_models.py new file mode 100644 index 000000000..9ec2a4af8 --- /dev/null +++ b/api/tests/playlists/test_models.py @@ -0,0 +1,20 @@ + + +def test_can_create_playlist(factories): + tracks = factories['music.Track'].create_batch(5) + playlist = factories['playlists.Playlist']() + + previous = None + for track in tracks: + previous = playlist.add_track(track, previous=previous) + + playlist_tracks = list(playlist.playlist_tracks.all()) + + previous = None + for idx, track in enumerate(tracks): + plt = playlist_tracks[idx] + assert plt.position == idx + assert plt.track == track + if previous: + assert playlist_tracks[idx + 1] == previous + assert plt.playlist == playlist diff --git a/api/tests/test_playlists.py b/api/tests/playlists/test_views.py similarity index 63% rename from api/tests/test_playlists.py rename to api/tests/playlists/test_views.py index f496a64cb..943de03e1 100644 --- a/api/tests/test_playlists.py +++ b/api/tests/playlists/test_views.py @@ -7,27 +7,6 @@ from funkwhale_api.playlists import models from funkwhale_api.playlists.serializers import PlaylistSerializer - -def test_can_create_playlist(factories): - tracks = factories['music.Track'].create_batch(5) - playlist = factories['playlists.Playlist']() - - previous = None - for track in tracks: - previous = playlist.add_track(track, previous=previous) - - playlist_tracks = list(playlist.playlist_tracks.all()) - - previous = None - for idx, track in enumerate(tracks): - plt = playlist_tracks[idx] - assert plt.position == idx - assert plt.track == track - if previous: - assert playlist_tracks[idx + 1] == previous - assert plt.playlist == playlist - - def test_can_create_playlist_via_api(logged_in_client): url = reverse('api:v1:playlists-list') data = { From 9fdbc7b8597e8f210c6fc88b307ee161191abf43 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 16 Mar 2018 23:30:11 +0100 Subject: [PATCH 02/41] factorized privacy_level field --- api/funkwhale_api/common/fields.py | 14 ++++++++++++++ api/funkwhale_api/users/models.py | 12 +++--------- 2 files changed, 17 insertions(+), 9 deletions(-) create mode 100644 api/funkwhale_api/common/fields.py diff --git a/api/funkwhale_api/common/fields.py b/api/funkwhale_api/common/fields.py new file mode 100644 index 000000000..ab8f6e773 --- /dev/null +++ b/api/funkwhale_api/common/fields.py @@ -0,0 +1,14 @@ +from django.db import models + + +PRIVACY_LEVEL_CHOICES = [ + ('me', 'Only me'), + ('followers', 'Me and my followers'), + ('instance', 'Everyone on my instance, and my followers'), + ('everyone', 'Everyone, including people on other instances'), +] + + +def get_privacy_field(): + return models.CharField( + max_length=30, choices=PRIVACY_LEVEL_CHOICES, default='instance') diff --git a/api/funkwhale_api/users/models.py b/api/funkwhale_api/users/models.py index a5478656b..9516c108f 100644 --- a/api/funkwhale_api/users/models.py +++ b/api/funkwhale_api/users/models.py @@ -10,15 +10,9 @@ from django.db import models from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ +from funkwhale_api.common import fields -PRIVACY_LEVEL_CHOICES = [ - ('me', 'Only me'), - ('followers', 'Me and my followers'), - ('instance', 'Everyone on my instance, and my followers'), - ('everyone', 'Everyone, including people on other instances'), -] - @python_2_unicode_compatible class User(AbstractUser): @@ -39,8 +33,8 @@ class User(AbstractUser): }, } - privacy_level = models.CharField( - max_length=30, choices=PRIVACY_LEVEL_CHOICES, default='instance') + privacy_level = fields.get_privacy_field() + def __str__(self): return self.username From 859f8a05702897de79c911adba50e0784a6dd615 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 16 Mar 2018 23:30:37 +0100 Subject: [PATCH 03/41] Replaced is_public flag by brand new privacy_level field on playlists --- api/funkwhale_api/playlists/admin.py | 2 +- .../migrations/0002_auto_20180316_2217.py | 22 +++++++++++++++++++ api/funkwhale_api/playlists/models.py | 4 +++- api/funkwhale_api/playlists/serializers.py | 2 +- 4 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 api/funkwhale_api/playlists/migrations/0002_auto_20180316_2217.py diff --git a/api/funkwhale_api/playlists/admin.py b/api/funkwhale_api/playlists/admin.py index b337154c9..1d58abf9b 100644 --- a/api/funkwhale_api/playlists/admin.py +++ b/api/funkwhale_api/playlists/admin.py @@ -5,7 +5,7 @@ from . import models @admin.register(models.Playlist) class PlaylistAdmin(admin.ModelAdmin): - list_display = ['name', 'user', 'is_public', 'creation_date'] + list_display = ['name', 'user', 'privacy_level', 'creation_date'] search_fields = ['name', ] list_select_related = True diff --git a/api/funkwhale_api/playlists/migrations/0002_auto_20180316_2217.py b/api/funkwhale_api/playlists/migrations/0002_auto_20180316_2217.py new file mode 100644 index 000000000..23d0a8eab --- /dev/null +++ b/api/funkwhale_api/playlists/migrations/0002_auto_20180316_2217.py @@ -0,0 +1,22 @@ +# Generated by Django 2.0.3 on 2018-03-16 22:17 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('playlists', '0001_initial'), + ] + + operations = [ + migrations.RemoveField( + model_name='playlist', + name='is_public', + ), + migrations.AddField( + model_name='playlist', + name='privacy_level', + field=models.CharField(choices=[('me', 'Only me'), ('followers', 'Me and my followers'), ('instance', 'Everyone on my instance, and my followers'), ('everyone', 'Everyone, including people on other instances')], default='instance', max_length=30), + ), + ] diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index e89dce81c..47aa46333 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -3,13 +3,15 @@ from django.utils import timezone from mptt.models import MPTTModel, TreeOneToOneField +from funkwhale_api.common import fields + class Playlist(models.Model): name = models.CharField(max_length=50) - is_public = models.BooleanField(default=False) user = models.ForeignKey( 'users.User', related_name="playlists", on_delete=models.CASCADE) creation_date = models.DateTimeField(default=timezone.now) + privacy_level = fields.get_privacy_field() def __str__(self): return self.name diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 7f889d53e..0732165e1 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -26,5 +26,5 @@ class PlaylistSerializer(serializers.ModelSerializer): class Meta: model = models.Playlist - fields = ('id', 'name', 'is_public', 'creation_date', 'playlist_tracks') + fields = ('id', 'name', 'privacy_level', 'creation_date', 'playlist_tracks') read_only_fields = ['id', 'playlist_tracks', 'creation_date'] From 2a3f43ecb14ac90b0b58a6bcde70de94664b41e6 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 16 Mar 2018 23:31:08 +0100 Subject: [PATCH 04/41] Ensure privacy_level is settable/inherited properly --- api/funkwhale_api/playlists/views.py | 7 +++++- api/tests/playlists/test_views.py | 35 +++++++++++++++++++++------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 1a88d231e..f78b5b801 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -32,7 +32,12 @@ class PlaylistViewSet( return self.queryset.filter(user=self.request.user) def perform_create(self, serializer): - return serializer.save(user=self.request.user) + return serializer.save( + user=self.request.user, + privacy_level=serializer.validated_data.get( + 'privacy_level', self.request.user.privacy_level) + + ) class PlaylistTrackViewSet( diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index 943de03e1..f3084e6a0 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -7,27 +7,44 @@ from funkwhale_api.playlists import models from funkwhale_api.playlists.serializers import PlaylistSerializer -def test_can_create_playlist_via_api(logged_in_client): +def test_can_create_playlist_via_api(logged_in_api_client): url = reverse('api:v1:playlists-list') + data = { + 'name': 'test', + 'privacy_level': 'everyone' + } + + response = logged_in_api_client.post(url, data) + + playlist = logged_in_api_client.user.playlists.latest('id') + assert playlist.name == 'test' + assert playlist.privacy_level == 'everyone' + + +def test_playlist_inherits_user_privacy(logged_in_api_client): + url = reverse('api:v1:playlists-list') + user = logged_in_api_client.user + user.privacy_level = 'me' + user.save() + data = { 'name': 'test', } - response = logged_in_client.post(url, data) - - playlist = logged_in_client.user.playlists.latest('id') - assert playlist.name == 'test' + response = logged_in_api_client.post(url, data) + playlist = user.playlists.latest('id') + assert playlist.privacy_level == user.privacy_level -def test_can_add_playlist_track_via_api(factories, logged_in_client): +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_client.user) + 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_client.post(url, data) - plts = logged_in_client.user.playlists.latest('id').playlist_tracks.all() + response = logged_in_api_client.post(url, data) + plts = logged_in_api_client.user.playlists.latest('id').playlist_tracks.all() assert plts.first().track == tracks[0] From 3e277aad4f0b2158f9b744336c996a7fabbcea03 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:30:53 +0100 Subject: [PATCH 05/41] Added helper to filter queryset by privacy level --- api/funkwhale_api/common/fields.py | 13 +++++++++++++ api/tests/common/test_fields.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 api/tests/common/test_fields.py diff --git a/api/funkwhale_api/common/fields.py b/api/funkwhale_api/common/fields.py index ab8f6e773..ef9f840dc 100644 --- a/api/funkwhale_api/common/fields.py +++ b/api/funkwhale_api/common/fields.py @@ -12,3 +12,16 @@ PRIVACY_LEVEL_CHOICES = [ def get_privacy_field(): return models.CharField( max_length=30, choices=PRIVACY_LEVEL_CHOICES, default='instance') + + +def privacy_level_query(user, lookup_field='privacy_level'): + if user.is_anonymous: + return models.Q(**{ + lookup_field: 'everyone', + }) + + return models.Q(**{ + '{}__in'.format(lookup_field): [ + 'me', 'followers', 'instance', 'everyone' + ] + }) diff --git a/api/tests/common/test_fields.py b/api/tests/common/test_fields.py new file mode 100644 index 000000000..7c63431a3 --- /dev/null +++ b/api/tests/common/test_fields.py @@ -0,0 +1,17 @@ +import pytest + +from django.contrib.auth.models import AnonymousUser +from django.db.models import Q + +from funkwhale_api.common import fields +from funkwhale_api.users.factories import UserFactory + + +@pytest.mark.parametrize('user,expected', [ + (AnonymousUser(), Q(privacy_level='everyone')), + (UserFactory.build(pk=1), + Q(privacy_level__in=['me', 'followers', 'instance', 'everyone'])), +]) +def test_privacy_level_query(user,expected): + query = fields.privacy_level_query(user) + assert query == expected From 367014f70ec76f619327ac6b2a7e2f8597ee96fb Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:31:22 +0100 Subject: [PATCH 06/41] Added owner permission to check user has the right to read/update object --- api/funkwhale_api/common/permissions.py | 39 ++++++++++++++++++++++ api/tests/common/test_permissions.py | 43 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 api/tests/common/test_permissions.py diff --git a/api/funkwhale_api/common/permissions.py b/api/funkwhale_api/common/permissions.py index ecfea4c9a..c99c275c1 100644 --- a/api/funkwhale_api/common/permissions.py +++ b/api/funkwhale_api/common/permissions.py @@ -1,4 +1,7 @@ +import operator + from django.conf import settings +from django.http import Http404 from rest_framework.permissions import BasePermission, DjangoModelPermissions @@ -20,3 +23,39 @@ class HasModelPermission(DjangoModelPermissions): """ def get_required_permissions(self, method, model_cls): return super().get_required_permissions(method, self.model) + + +class OwnerPermission(BasePermission): + """ + Ensure the request user is the owner of the object. + + Usage: + + class MyView(APIView): + model = MyModel + permission_classes = [OwnerPermission] + owner_field = 'owner' + owner_checks = ['read', 'write'] + """ + perms_map = { + 'GET': 'read', + 'OPTIONS': 'read', + 'HEAD': 'read', + 'POST': 'write', + 'PUT': 'write', + 'PATCH': 'write', + 'DELETE': 'write', + } + + def has_object_permission(self, request, view, obj): + method_check = self.perms_map[request.method] + owner_checks = getattr(view, 'owner_checks', ['read', 'write']) + if method_check not in owner_checks: + # check not enabled + return True + + owner_field = getattr(view, 'owner_field', 'user') + owner = operator.attrgetter(owner_field)(obj) + if owner != request.user: + raise Http404 + return True diff --git a/api/tests/common/test_permissions.py b/api/tests/common/test_permissions.py new file mode 100644 index 000000000..95ad6c88d --- /dev/null +++ b/api/tests/common/test_permissions.py @@ -0,0 +1,43 @@ +import pytest + +from rest_framework.views import APIView + +from django.contrib.auth.models import AnonymousUser +from django.http import Http404 + +from funkwhale_api.common import permissions + + +def test_owner_permission_owner_field_ok(nodb_factories, api_request): + playlist = nodb_factories['playlists.Playlist']() + view = APIView.as_view() + permission = permissions.OwnerPermission() + request = api_request.get('/') + setattr(request, 'user', playlist.user) + check = permission.has_object_permission(request, view, playlist) + + assert check is True + + +def test_owner_permission_owner_field_not_ok(nodb_factories, api_request): + playlist = nodb_factories['playlists.Playlist']() + view = APIView.as_view() + permission = permissions.OwnerPermission() + request = api_request.get('/') + setattr(request, 'user', AnonymousUser()) + + with pytest.raises(Http404): + permission.has_object_permission(request, view, playlist) + + + +def test_owner_permission_read_only(nodb_factories, api_request): + playlist = nodb_factories['playlists.Playlist']() + view = APIView.as_view() + setattr(view, 'owner_checks', ['write']) + permission = permissions.OwnerPermission() + request = api_request.get('/') + setattr(request, 'user', AnonymousUser()) + check = permission.has_object_permission(request, view, playlist) + + assert check is True From d7adaf398f76f8b1cd3d2a55bd53db9061202938 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:31:45 +0100 Subject: [PATCH 07/41] Can now use nodb_factory fixtue in tests for faster tests --- api/tests/conftest.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/api/tests/conftest.py b/api/tests/conftest.py index 4ff1a8ee7..62bc5ada6 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -1,8 +1,11 @@ +import factory import tempfile import shutil import pytest + from django.core.cache import cache as django_cache from dynamic_preferences.registries import global_preferences_registry + from rest_framework.test import APIClient from rest_framework.test import APIRequestFactory @@ -27,6 +30,16 @@ def cache(): @pytest.fixture def factories(db): from funkwhale_api import factories + for v in factories.registry.values(): + v._meta.strategy = factory.CREATE_STRATEGY + yield factories.registry + + +@pytest.fixture +def nodb_factories(): + from funkwhale_api import factories + for v in factories.registry.values(): + v._meta.strategy = factory.BUILD_STRATEGY yield factories.registry From 4f7fa09a78336af3b1c20eca032e6d63cc689568 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:31:59 +0100 Subject: [PATCH 08/41] Playlisttrack factory --- api/funkwhale_api/playlists/factories.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/api/funkwhale_api/playlists/factories.py b/api/funkwhale_api/playlists/factories.py index 19e4770cf..cddea6002 100644 --- a/api/funkwhale_api/playlists/factories.py +++ b/api/funkwhale_api/playlists/factories.py @@ -1,6 +1,7 @@ import factory from funkwhale_api.factories import registry +from funkwhale_api.music.factories import TrackFactory from funkwhale_api.users.factories import UserFactory @@ -11,3 +12,12 @@ class PlaylistFactory(factory.django.DjangoModelFactory): class Meta: model = 'playlists.Playlist' + + +@registry.register +class PlaylistTrackFactory(factory.django.DjangoModelFactory): + playlist = factory.SubFactory(PlaylistFactory) + track = factory.SubFactory(TrackFactory) + + class Meta: + model = 'playlists.PlaylistTrack' From 9a909798e790c148b014c05159a241d33f09ff20 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:33:02 +0100 Subject: [PATCH 09/41] Additional permissions checks on playlist views --- api/funkwhale_api/playlists/views.py | 42 ++++++++++++------ api/tests/playlists/test_views.py | 66 ++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 13 deletions(-) diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index f78b5b801..5ef00ebea 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -1,9 +1,11 @@ from rest_framework import generics, mixins, viewsets from rest_framework import status from rest_framework.response import Response +from rest_framework.permissions import IsAuthenticatedOrReadOnly from funkwhale_api.music.models import Track -from funkwhale_api.common.permissions import ConditionalAuthentication +from funkwhale_api.common import permissions +from funkwhale_api.common import fields from . import models from . import serializers @@ -12,24 +14,22 @@ from . import serializers class PlaylistViewSet( mixins.RetrieveModelMixin, mixins.CreateModelMixin, + mixins.UpdateModelMixin, mixins.DestroyModelMixin, mixins.ListModelMixin, viewsets.GenericViewSet): serializer_class = serializers.PlaylistSerializer queryset = (models.Playlist.objects.all()) - permission_classes = [ConditionalAuthentication] - - def create(self, request, *args, **kwargs): - serializer = self.get_serializer(data=request.data) - serializer.is_valid(raise_exception=True) - 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) + permission_classes = [ + permissions.ConditionalAuthentication, + permissions.OwnerPermission, + IsAuthenticatedOrReadOnly, + ] def get_queryset(self): - return self.queryset.filter(user=self.request.user) + return self.queryset.filter( + fields.privacy_level_query(self.request.user)) def perform_create(self, serializer): return serializer.save( @@ -41,23 +41,39 @@ class PlaylistViewSet( class PlaylistTrackViewSet( + mixins.RetrieveModelMixin, mixins.CreateModelMixin, + mixins.UpdateModelMixin, mixins.DestroyModelMixin, mixins.ListModelMixin, viewsets.GenericViewSet): serializer_class = serializers.PlaylistTrackSerializer queryset = (models.PlaylistTrack.objects.all()) - permission_classes = [ConditionalAuthentication] + permission_classes = [ + permissions.ConditionalAuthentication, + permissions.OwnerPermission, + IsAuthenticatedOrReadOnly, + ] 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_queryset(self): - return self.queryset.filter(playlist__user=self.request.user) + return self.queryset.filter( + fields.privacy_level_query( + self.request.user, + lookup_field='playlist__privacy_level')) diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index f3084e6a0..0a3549b84 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -1,4 +1,6 @@ import json +import pytest + from django.urls import reverse from django.core.exceptions import ValidationError from django.utils import timezone @@ -48,3 +50,67 @@ def test_can_add_playlist_track_via_api(factories, logged_in_api_client): response = logged_in_api_client.post(url, data) 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'), +]) +def test_url_requires_login(name, method, factories, api_client): + url = reverse(name) + + response = getattr(api_client, method)(url, {}) + + assert response.status_code == 401 + + +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 + } + + response = logged_in_api_client.post(url, data) + assert response.status_code == 400 + assert playlist.playlist_tracks.count() == 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) + url = reverse('api:v1:playlists-list') + response = api_client.get(url) + + assert response.data['count'] == 0 + + +@pytest.mark.parametrize('method', ['PUT', 'PATCH', 'DELETE']) +def test_only_owner_can_edit_playlist(method, factories, api_client): + playlist = factories['playlists.Playlist']() + url = reverse('api:v1:playlists-detail', kwargs={'pk': playlist.pk}) + response = api_client.get(url) + + assert response.status_code == 404 + + +@pytest.mark.parametrize('method', ['PUT', 'PATCH', 'DELETE']) +def test_only_owner_can_edit_playlist_track(method, factories, api_client): + plt = factories['playlists.PlaylistTrack']() + url = reverse('api:v1:playlist-tracks-detail', kwargs={'pk': plt.pk}) + response = api_client.get(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): + factories['playlists.PlaylistTrack'](playlist__privacy_level=level) + url = reverse('api:v1:playlist-tracks-list') + response = api_client.get(url) + + assert response.data['count'] == 0 From d8486beeb07ce0fc88f2337aac72083e7f8643c8 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Mar 2018 21:51:49 +0100 Subject: [PATCH 10/41] Can now list tracks from within playlist endpoint --- api/funkwhale_api/playlists/serializers.py | 5 ++--- api/funkwhale_api/playlists/views.py | 13 ++++++++++++- api/tests/common/test_permissions.py | 1 - api/tests/playlists/test_views.py | 16 +++++++++++++++- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 0732165e1..d35f33145 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -22,9 +22,8 @@ class PlaylistTrackCreateSerializer(serializers.ModelSerializer): class PlaylistSerializer(serializers.ModelSerializer): - playlist_tracks = PlaylistTrackSerializer(many=True, read_only=True) class Meta: model = models.Playlist - fields = ('id', 'name', 'privacy_level', 'creation_date', 'playlist_tracks') - read_only_fields = ['id', 'playlist_tracks', 'creation_date'] + fields = ('id', 'name', 'privacy_level', 'creation_date') + read_only_fields = ['id', 'creation_date'] diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 5ef00ebea..3307b5276 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -1,5 +1,6 @@ from rest_framework import generics, mixins, viewsets from rest_framework import status +from rest_framework.decorators import detail_route from rest_framework.response import Response from rest_framework.permissions import IsAuthenticatedOrReadOnly @@ -27,6 +28,17 @@ class PlaylistViewSet( IsAuthenticatedOrReadOnly, ] + @detail_route(methods=['get']) + def tracks(self, request, *args, **kwargs): + playlist = self.get_object() + plts = playlist.playlist_tracks.all() + serializer = serializers.PlaylistTrackSerializer(plts, many=True) + data = { + 'count': len(plts), + 'result': serializer.data + } + return Response(data, status=200) + def get_queryset(self): return self.queryset.filter( fields.privacy_level_query(self.request.user)) @@ -36,7 +48,6 @@ class PlaylistViewSet( user=self.request.user, privacy_level=serializer.validated_data.get( 'privacy_level', self.request.user.privacy_level) - ) diff --git a/api/tests/common/test_permissions.py b/api/tests/common/test_permissions.py index 95ad6c88d..b5c5160f8 100644 --- a/api/tests/common/test_permissions.py +++ b/api/tests/common/test_permissions.py @@ -30,7 +30,6 @@ def test_owner_permission_owner_field_not_ok(nodb_factories, api_request): permission.has_object_permission(request, view, playlist) - def test_owner_permission_read_only(nodb_factories, api_request): playlist = nodb_factories['playlists.Playlist']() view = APIView.as_view() diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index 0a3549b84..3c62dcda6 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -6,7 +6,7 @@ from django.core.exceptions import ValidationError from django.utils import timezone from funkwhale_api.playlists import models -from funkwhale_api.playlists.serializers import PlaylistSerializer +from funkwhale_api.playlists import serializers def test_can_create_playlist_via_api(logged_in_api_client): @@ -48,6 +48,7 @@ def test_can_add_playlist_track_via_api(factories, logged_in_api_client): } 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] @@ -114,3 +115,16 @@ def test_playlist_track_privacy_respected_in_list_anon( 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['result'][0] == serialized_plt From 944135e752df4cb1d1e258c4a8333609bcbd7a80 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 12:36:15 +0100 Subject: [PATCH 11/41] Capped number of tracks in playlists --- api/config/settings/common.py | 3 +++ api/funkwhale_api/playlists/serializers.py | 9 +++++++++ api/tests/playlists/test_serializers.py | 16 ++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 api/tests/playlists/test_serializers.py diff --git a/api/config/settings/common.py b/api/config/settings/common.py index 000289e51..be5adf85c 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -383,3 +383,6 @@ CACHEOPS = { # Custom Admin URL, use {% url 'admin:index' %} ADMIN_URL = env('DJANGO_ADMIN_URL', default='^api/admin/') CSRF_USE_SESSIONS = True + +# Playlist settings +PLAYLISTS_MAX_TRACKS = env.int('PLAYLISTS_MAX_TRACKS', default=500) diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index d35f33145..77470c4de 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -1,3 +1,4 @@ +from django.conf import settings from rest_framework import serializers from taggit.models import Tag @@ -20,6 +21,14 @@ class PlaylistTrackCreateSerializer(serializers.ModelSerializer): model = models.PlaylistTrack fields = ('id', 'track', 'playlist', 'position') + def validate_playlist(self, value): + existing = value.playlist_tracks.count() + if existing >= settings.PLAYLISTS_MAX_TRACKS: + raise serializers.ValidationError( + 'Playlist has reached the maximum of {} tracks'.format( + settings.PLAYLISTS_MAX_TRACKS)) + return value + class PlaylistSerializer(serializers.ModelSerializer): diff --git a/api/tests/playlists/test_serializers.py b/api/tests/playlists/test_serializers.py new file mode 100644 index 000000000..6daff8e60 --- /dev/null +++ b/api/tests/playlists/test_serializers.py @@ -0,0 +1,16 @@ +from funkwhale_api.playlists import serializers + + +def test_cannot_max_500_tracks_per_playlist(mocker, 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={ + 'playlist': playlist.pk, + 'track': track.pk, + }) + + assert serializer.is_valid() is False + assert 'playlist' in serializer.errors From 8821a1bb43073ad1c157cafbaeabf1b30d8ad4bd Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 14:06:51 +0100 Subject: [PATCH 12/41] Removed superfluous mptt requirement --- api/config/settings/common.py | 1 - .../playlists/migrations/0001_initial.py | 3 +-- api/funkwhale_api/playlists/models.py | 16 +++------------- api/requirements/base.txt | 1 - 4 files changed, 4 insertions(+), 17 deletions(-) diff --git a/api/config/settings/common.py b/api/config/settings/common.py index be5adf85c..1bad608fd 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -57,7 +57,6 @@ THIRD_PARTY_APPS = ( 'taggit', 'rest_auth', 'rest_auth.registration', - 'mptt', 'dynamic_preferences', 'django_filters', 'cacheops', diff --git a/api/funkwhale_api/playlists/migrations/0001_initial.py b/api/funkwhale_api/playlists/migrations/0001_initial.py index bc97d8122..987b2f9cf 100644 --- a/api/funkwhale_api/playlists/migrations/0001_initial.py +++ b/api/funkwhale_api/playlists/migrations/0001_initial.py @@ -4,7 +4,6 @@ from __future__ import unicode_literals from django.db import migrations, models from django.conf import settings import django.utils.timezone -import mptt.fields class Migration(migrations.Migration): @@ -34,7 +33,7 @@ class Migration(migrations.Migration): ('tree_id', models.PositiveIntegerField(db_index=True, editable=False)), ('position', models.PositiveIntegerField(db_index=True, editable=False)), ('playlist', models.ForeignKey(to='playlists.Playlist', related_name='playlist_tracks', on_delete=models.CASCADE)), - ('previous', mptt.fields.TreeOneToOneField(null=True, to='playlists.PlaylistTrack', related_name='next', blank=True, on_delete=models.CASCADE)), + ('previous', models.OneToOneField(null=True, to='playlists.PlaylistTrack', related_name='next', blank=True, on_delete=models.CASCADE)), ('track', models.ForeignKey(to='music.Track', related_name='playlist_tracks', on_delete=models.CASCADE)), ], options={ diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index 47aa46333..32e9a13b2 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -1,8 +1,6 @@ from django.db import models from django.utils import timezone -from mptt.models import MPTTModel, TreeOneToOneField - from funkwhale_api.common import fields @@ -28,18 +26,10 @@ class PlaylistTrack(MPTTModel): 'music.Track', related_name='playlist_tracks', on_delete=models.CASCADE) - previous = TreeOneToOneField( - 'self', - blank=True, - null=True, - related_name='next', - on_delete=models.CASCADE) + index = models.PositiveIntegerField(null=True) playlist = models.ForeignKey( Playlist, related_name='playlist_tracks', on_delete=models.CASCADE) - class MPTTMeta: - level_attr = 'position' - parent_attr = 'previous' - class Meta: - ordering = ('-playlist', 'position') + ordering = ('-playlist', 'index') + unique_together = ('playlist', 'index') diff --git a/api/requirements/base.txt b/api/requirements/base.txt index bfa1a47db..efcc4eea4 100644 --- a/api/requirements/base.txt +++ b/api/requirements/base.txt @@ -33,7 +33,6 @@ musicbrainzngs==0.6 youtube_dl>=2017.12.14 djangorestframework>=3.7,<3.8 djangorestframework-jwt>=1.11,<1.12 -django-mptt>=0.9,<0.10 google-api-python-client>=1.6,<1.7 arrow>=0.12,<0.13 persisting-theory>=0.2,<0.3 From 257e67b5a6ac52de2586f4e4313f3cdaecbd4795 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 14:11:09 +0100 Subject: [PATCH 13/41] New "index" field on playlist tracks, and .insert() metod to manage playlists --- api/funkwhale_api/playlists/admin.py | 2 +- .../migrations/0003_auto_20180319_1214.py | 52 ++++++++++++ api/funkwhale_api/playlists/models.py | 58 +++++++++++-- api/funkwhale_api/playlists/serializers.py | 2 +- api/tests/playlists/test_models.py | 82 +++++++++++++++---- 5 files changed, 175 insertions(+), 21 deletions(-) create mode 100644 api/funkwhale_api/playlists/migrations/0003_auto_20180319_1214.py diff --git a/api/funkwhale_api/playlists/admin.py b/api/funkwhale_api/playlists/admin.py index 1d58abf9b..68e447f38 100644 --- a/api/funkwhale_api/playlists/admin.py +++ b/api/funkwhale_api/playlists/admin.py @@ -12,6 +12,6 @@ class PlaylistAdmin(admin.ModelAdmin): @admin.register(models.PlaylistTrack) class PlaylistTrackAdmin(admin.ModelAdmin): - list_display = ['playlist', 'track', 'position', ] + list_display = ['playlist', 'track', 'index'] search_fields = ['track__name', 'playlist__name'] list_select_related = True diff --git a/api/funkwhale_api/playlists/migrations/0003_auto_20180319_1214.py b/api/funkwhale_api/playlists/migrations/0003_auto_20180319_1214.py new file mode 100644 index 000000000..0284f8f2c --- /dev/null +++ b/api/funkwhale_api/playlists/migrations/0003_auto_20180319_1214.py @@ -0,0 +1,52 @@ +# Generated by Django 2.0.3 on 2018-03-19 12:14 + +from django.db import migrations, models +import django.utils.timezone + + +class Migration(migrations.Migration): + + dependencies = [ + ('playlists', '0002_auto_20180316_2217'), + ] + + operations = [ + migrations.AlterModelOptions( + name='playlisttrack', + options={'ordering': ('-playlist', 'index')}, + ), + migrations.AddField( + model_name='playlisttrack', + name='creation_date', + field=models.DateTimeField(default=django.utils.timezone.now), + ), + migrations.AddField( + model_name='playlisttrack', + name='index', + field=models.PositiveIntegerField(null=True), + ), + migrations.RemoveField( + model_name='playlisttrack', + name='lft', + ), + migrations.RemoveField( + model_name='playlisttrack', + name='position', + ), + migrations.RemoveField( + model_name='playlisttrack', + name='previous', + ), + migrations.RemoveField( + model_name='playlisttrack', + name='rght', + ), + migrations.RemoveField( + model_name='playlisttrack', + name='tree_id', + ), + migrations.AlterUniqueTogether( + name='playlisttrack', + unique_together={('playlist', 'index')}, + ), + ] diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index 32e9a13b2..b40093472 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -1,4 +1,6 @@ +from django import forms from django.db import models +from django.db import transaction from django.utils import timezone from funkwhale_api.common import fields @@ -14,14 +16,59 @@ class Playlist(models.Model): def __str__(self): return self.name - def add_track(self, track, previous=None): - plt = PlaylistTrack(previous=previous, track=track, playlist=self) - plt.save() + @transaction.atomic + def insert(self, plt, index=None): + """ + Given a PlaylistTrack, insert it at the correct index in the playlist, + and update other tracks index if necessary. + """ + old_index = plt.index + move = old_index is not None + if index is not None and index == old_index: + # moving at same position, just skip + return index - return plt + existing = self.playlist_tracks.select_for_update() + if move: + existing = existing.exclude(pk=plt.pk) + total = existing.filter(index__isnull=False).count() + + if index is None: + # we simply increment the last track index by 1 + index = total + + if index > total: + raise forms.ValidationError('Index is not continuous') + + if index < 0: + raise forms.ValidationError('Index must be zero or positive') + + if move: + # we remove the index temporarily, to avoid integrity errors + plt.index = None + plt.save(update_fields=['index']) + + if move: + if index > old_index: + # new index is higher than current, we decrement previous tracks + to_update = existing.filter( + index__gt=old_index, index__lte=index) + to_update.update(index=models.F('index') - 1) + if index < old_index: + # new index is lower than current, we increment next tracks + to_update = existing.filter( + index__lt=old_index, index__gte=index) + to_update.update(index=models.F('index') + 1) + else: + to_update = existing.filter(index__gte=index) + to_update.update(index=models.F('index') + 1) + + plt.index = index + plt.save(update_fields=['index']) + return index -class PlaylistTrack(MPTTModel): +class PlaylistTrack(models.Model): track = models.ForeignKey( 'music.Track', related_name='playlist_tracks', @@ -29,6 +76,7 @@ class PlaylistTrack(MPTTModel): index = models.PositiveIntegerField(null=True) playlist = models.ForeignKey( Playlist, related_name='playlist_tracks', on_delete=models.CASCADE) + creation_date = models.DateTimeField(default=timezone.now) class Meta: ordering = ('-playlist', 'index') diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 77470c4de..55603ee31 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -19,7 +19,7 @@ class PlaylistTrackCreateSerializer(serializers.ModelSerializer): class Meta: model = models.PlaylistTrack - fields = ('id', 'track', 'playlist', 'position') + fields = ('id', 'track', 'playlist', 'index') def validate_playlist(self, value): existing = value.playlist_tracks.count() diff --git a/api/tests/playlists/test_models.py b/api/tests/playlists/test_models.py index 9ec2a4af8..ab60cdb84 100644 --- a/api/tests/playlists/test_models.py +++ b/api/tests/playlists/test_models.py @@ -1,20 +1,74 @@ +import pytest + +from django import forms -def test_can_create_playlist(factories): - tracks = factories['music.Track'].create_batch(5) +def test_can_insert_plt(factories): + plt = factories['playlists.PlaylistTrack']() + + assert plt.index is None + + plt.playlist.insert(plt) + plt.refresh_from_db() + + assert plt.index == 0 + + +def test_insert_use_last_idx_by_default(factories): playlist = factories['playlists.Playlist']() + plts = factories['playlists.PlaylistTrack'].create_batch( + size=3, playlist=playlist) - previous = None - for track in tracks: - previous = playlist.add_track(track, previous=previous) + for i, plt in enumerate(plts): + index = playlist.insert(plt) + plt.refresh_from_db() - playlist_tracks = list(playlist.playlist_tracks.all()) + assert index == i + assert plt.index == i - previous = None - for idx, track in enumerate(tracks): - plt = playlist_tracks[idx] - assert plt.position == idx - assert plt.track == track - if previous: - assert playlist_tracks[idx + 1] == previous - assert plt.playlist == playlist +def test_can_insert_at_index(factories): + playlist = factories['playlists.Playlist']() + first = factories['playlists.PlaylistTrack'](playlist=playlist) + playlist.insert(first) + new_first = factories['playlists.PlaylistTrack'](playlist=playlist) + index = playlist.insert(new_first, index=0) + first.refresh_from_db() + new_first.refresh_from_db() + + assert index == 0 + assert first.index == 1 + assert new_first.index == 0 + + +def test_can_insert_and_move(factories): + playlist = factories['playlists.Playlist']() + first = factories['playlists.PlaylistTrack'](playlist=playlist) + second = factories['playlists.PlaylistTrack'](playlist=playlist) + third = factories['playlists.PlaylistTrack'](playlist=playlist) + playlist.insert(first) + playlist.insert(second) + playlist.insert(third) + + playlist.insert(second, index=0) + + first.refresh_from_db() + second.refresh_from_db() + third.refresh_from_db() + + assert third.index == 2 + assert second.index == 0 + assert first.index == 1 + + +def test_cannot_insert_at_wrong_index(factories): + plt = factories['playlists.PlaylistTrack']() + new = factories['playlists.PlaylistTrack'](playlist=plt.playlist) + with pytest.raises(forms.ValidationError): + plt.playlist.insert(new, 2) + + +def test_cannot_insert_at_negative_index(factories): + plt = factories['playlists.PlaylistTrack']() + new = factories['playlists.PlaylistTrack'](playlist=plt.playlist) + with pytest.raises(forms.ValidationError): + plt.playlist.insert(new, -1) From e87e2654e8932bdefe4b738e0b5c353f8c18c441 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 15:28:33 +0100 Subject: [PATCH 14/41] 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 b40093472..d8c90aef9 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 55603ee31..43e2414d4 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 3307b5276..02e53cb16 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 ab60cdb84..dfe40187d 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 6daff8e60..8e30919e6 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 3c62dcda6..192aad382 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) From 15300e255cd31a3519ff0f9b14fcb2040552f192 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 15:31:55 +0100 Subject: [PATCH 15/41] Removed pytest warning --- api/setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/setup.cfg b/api/setup.cfg index 954b4d196..34daa8c68 100644 --- a/api/setup.cfg +++ b/api/setup.cfg @@ -6,7 +6,7 @@ exclude = .tox,.git,*/migrations/*,*/static/CACHE/*,docs,node_modules max-line-length = 120 exclude=.tox,.git,*/migrations/*,*/static/CACHE/*,docs,node_modules -[pytest] +[tool:pytest] DJANGO_SETTINGS_MODULE=config.settings.test python_files = tests.py test_*.py *_tests.py testpaths = tests From a34b1afd6cd9594cef6b5f422cadcde4ea925a70 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 17:38:25 +0100 Subject: [PATCH 16/41] Store for fetching user playlists --- front/src/store/auth.js | 1 + front/src/store/index.js | 2 ++ front/src/store/playlists.js | 24 +++++++++++++ front/test/unit/specs/store/auth.spec.js | 3 +- front/test/unit/specs/store/playlists.spec.js | 36 +++++++++++++++++++ 5 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 front/src/store/playlists.js create mode 100644 front/test/unit/specs/store/playlists.spec.js diff --git a/front/src/store/auth.js b/front/src/store/auth.js index 7944cae08..e72e1968f 100644 --- a/front/src/store/auth.js +++ b/front/src/store/auth.js @@ -91,6 +91,7 @@ export default { commit('profile', data) commit('username', data.username) dispatch('favorites/fetch', null, {root: true}) + dispatch('playlists/fetchOwn', null, {root: true}) Object.keys(data.permissions).forEach(function (key) { // this makes it easier to check for permissions in templates commit('permission', {key, status: data.permissions[String(key)].status}) diff --git a/front/src/store/index.js b/front/src/store/index.js index 2453c0e71..298fa04ec 100644 --- a/front/src/store/index.js +++ b/front/src/store/index.js @@ -8,6 +8,7 @@ import instance from './instance' import queue from './queue' import radios from './radios' import player from './player' +import playlists from './playlists' import ui from './ui' Vue.use(Vuex) @@ -20,6 +21,7 @@ export default new Vuex.Store({ instance, queue, radios, + playlists, player }, plugins: [ diff --git a/front/src/store/playlists.js b/front/src/store/playlists.js new file mode 100644 index 000000000..75cac87ba --- /dev/null +++ b/front/src/store/playlists.js @@ -0,0 +1,24 @@ +import axios from 'axios' + +export default { + namespaced: true, + state: { + playlists: [] + }, + mutations: { + playlists (state, value) { + state.playlists = value + } + }, + actions: { + fetchOwn ({commit, rootState}) { + let userId = rootState.auth.profile.id + if (!userId) { + return + } + return axios.get('playlists/', {params: {user: userId}}).then((response) => { + commit('playlists', response.data.results) + }) + } + } +} diff --git a/front/test/unit/specs/store/auth.spec.js b/front/test/unit/specs/store/auth.spec.js index 3271f5168..518dc10d4 100644 --- a/front/test/unit/specs/store/auth.spec.js +++ b/front/test/unit/specs/store/auth.spec.js @@ -180,7 +180,8 @@ describe('store/auth', () => { { type: 'permission', payload: {key: 'admin', status: true} } ], expectedActions: [ - { type: 'favorites/fetch', payload: null, options: {root: true} } + { type: 'favorites/fetch', payload: null, options: {root: true} }, + { type: 'playlists/fetchOwn', payload: null, options: {root: true} }, ] }, done) }) diff --git a/front/test/unit/specs/store/playlists.spec.js b/front/test/unit/specs/store/playlists.spec.js new file mode 100644 index 000000000..e82af60bb --- /dev/null +++ b/front/test/unit/specs/store/playlists.spec.js @@ -0,0 +1,36 @@ +var sinon = require('sinon') +import moxios from 'moxios' +import store from '@/store/playlists' + +import { testAction } from '../../utils' + +describe('store/playlists', () => { + var sandbox + + beforeEach(function () { + sandbox = sinon.sandbox.create() + moxios.install() + }) + afterEach(function () { + sandbox.restore() + moxios.uninstall() + }) + + describe('mutations', () => { + it('set playlists', () => { + const state = { playlists: [] } + store.mutations.playlists(state, [{id: 1, name: 'test'}]) + expect(state.playlists).to.deep.equal([{id: 1, name: 'test'}]) + }) + }) + describe('actions', () => { + it('fetchOwn does nothing with no user', (done) => { + testAction({ + action: store.actions.fetchOwn, + payload: null, + params: {state: { playlists: [] }, rootState: {auth: {profile: {}}}}, + expectedMutations: [] + }, done) + }) + }) +}) From d6f2c7d4c41d0bdc425a2bc0ccf826281d5c054f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 17:39:03 +0100 Subject: [PATCH 17/41] Form, modal and player icon to add track to playlist --- front/src/components/audio/Player.vue | 11 ++- front/src/components/playlists/Form.vue | 90 ++++++++++++++++++ .../components/playlists/PlaylistModal.vue | 91 +++++++++++++++++++ .../playlists/TrackPlaylistIcon.vue | 40 ++++++++ 4 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 front/src/components/playlists/Form.vue create mode 100644 front/src/components/playlists/PlaylistModal.vue create mode 100644 front/src/components/playlists/TrackPlaylistIcon.vue diff --git a/front/src/components/audio/Player.vue b/front/src/components/audio/Player.vue index 0c5ed44b6..75a01c52e 100644 --- a/front/src/components/audio/Player.vue +++ b/front/src/components/audio/Player.vue @@ -30,7 +30,12 @@
- + +
@@ -140,11 +145,13 @@ import ColorThief from '@/vendor/color-thief' import Track from '@/audio/track' import AudioTrack from '@/components/audio/Track' import TrackFavoriteIcon from '@/components/favorites/TrackFavoriteIcon' +import TrackPlaylistIcon from '@/components/playlists/TrackPlaylistIcon' export default { name: 'player', components: { TrackFavoriteIcon, + TrackPlaylistIcon, GlobalEvents, AudioTrack }, @@ -281,6 +288,7 @@ export default { cursor: pointer } .track-area { + margin-top: 0; .header, .meta, .artist, .album { color: white !important; } @@ -384,4 +392,5 @@ export default { .ui.feed.icon { margin: 0; } + diff --git a/front/src/components/playlists/Form.vue b/front/src/components/playlists/Form.vue new file mode 100644 index 000000000..8b39c6f6c --- /dev/null +++ b/front/src/components/playlists/Form.vue @@ -0,0 +1,90 @@ + + + + + + diff --git a/front/src/components/playlists/PlaylistModal.vue b/front/src/components/playlists/PlaylistModal.vue new file mode 100644 index 000000000..2e1627001 --- /dev/null +++ b/front/src/components/playlists/PlaylistModal.vue @@ -0,0 +1,91 @@ + + + + + + diff --git a/front/src/components/playlists/TrackPlaylistIcon.vue b/front/src/components/playlists/TrackPlaylistIcon.vue new file mode 100644 index 000000000..2684f7cb6 --- /dev/null +++ b/front/src/components/playlists/TrackPlaylistIcon.vue @@ -0,0 +1,40 @@ + + + + + + From 08b28a7d9864f6fec0f01df4c33b772f42cfb9aa Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 19:07:45 +0100 Subject: [PATCH 18/41] Added playlist tracks count and modification date in API --- .../migrations/0004_auto_20180319_1642.py | 23 +++++++++++++++++++ api/funkwhale_api/playlists/models.py | 5 ++++ api/funkwhale_api/playlists/serializers.py | 21 +++++++++++++++-- api/funkwhale_api/playlists/views.py | 9 +++++++- api/tests/playlists/test_models.py | 2 ++ api/tests/playlists/test_views.py | 10 ++++++++ 6 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 api/funkwhale_api/playlists/migrations/0004_auto_20180319_1642.py diff --git a/api/funkwhale_api/playlists/migrations/0004_auto_20180319_1642.py b/api/funkwhale_api/playlists/migrations/0004_auto_20180319_1642.py new file mode 100644 index 000000000..f4a525291 --- /dev/null +++ b/api/funkwhale_api/playlists/migrations/0004_auto_20180319_1642.py @@ -0,0 +1,23 @@ +# Generated by Django 2.0.3 on 2018-03-19 16:42 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('playlists', '0003_auto_20180319_1214'), + ] + + operations = [ + migrations.AddField( + model_name='playlist', + name='modification_date', + field=models.DateTimeField(auto_now=True), + ), + migrations.AlterField( + model_name='playlisttrack', + name='index', + field=models.PositiveIntegerField(blank=True, null=True), + ), + ] diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index d8c90aef9..f26dbeff4 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -11,6 +11,8 @@ class Playlist(models.Model): user = models.ForeignKey( 'users.User', related_name="playlists", on_delete=models.CASCADE) creation_date = models.DateTimeField(default=timezone.now) + modification_date = models.DateTimeField( + auto_now=True) privacy_level = fields.get_privacy_field() def __str__(self): @@ -65,10 +67,13 @@ class Playlist(models.Model): plt.index = index plt.save(update_fields=['index']) + self.save(update_fields=['modification_date']) return index + @transaction.atomic def remove(self, index): existing = self.playlist_tracks.select_for_update() + self.save(update_fields=['modification_date']) to_update = existing.filter(index__gt=index) return to_update.update(index=models.F('index') - 1) diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 43e2414d4..0680dc8d0 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -62,8 +62,25 @@ class PlaylistTrackWriteSerializer(serializers.ModelSerializer): class PlaylistSerializer(serializers.ModelSerializer): + tracks_count = serializers.SerializerMethodField() class Meta: model = models.Playlist - fields = ('id', 'name', 'privacy_level', 'creation_date') - read_only_fields = ['id', 'creation_date'] + fields = ( + 'id', + 'name', + 'tracks_count', + 'privacy_level', + 'creation_date', + 'modification_date') + read_only_fields = [ + 'id', + 'modification_date', + 'creation_date',] + + def get_tracks_count(self, obj): + try: + return obj.tracks_count + except AttributeError: + # no annotation? + return obj.playlist_tracks.count() diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 02e53cb16..dc3475fab 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -1,3 +1,5 @@ +from django.db.models import Count + from rest_framework import generics, mixins, viewsets from rest_framework import status from rest_framework.decorators import detail_route @@ -8,6 +10,7 @@ from funkwhale_api.music.models import Track from funkwhale_api.common import permissions from funkwhale_api.common import fields +from . import filters from . import models from . import serializers @@ -21,13 +24,17 @@ class PlaylistViewSet( viewsets.GenericViewSet): serializer_class = serializers.PlaylistSerializer - queryset = (models.Playlist.objects.all()) + queryset = ( + models.Playlist.objects.all() + .annotate(tracks_count=Count('playlist_tracks')) + ) permission_classes = [ permissions.ConditionalAuthentication, permissions.OwnerPermission, IsAuthenticatedOrReadOnly, ] owner_checks = ['write'] + filter_class = filters.PlaylistFilter @detail_route(methods=['get']) def tracks(self, request, *args, **kwargs): diff --git a/api/tests/playlists/test_models.py b/api/tests/playlists/test_models.py index dfe40187d..da3e23e28 100644 --- a/api/tests/playlists/test_models.py +++ b/api/tests/playlists/test_models.py @@ -5,6 +5,7 @@ from django import forms def test_can_insert_plt(factories): plt = factories['playlists.PlaylistTrack']() + modification_date = plt.playlist.modification_date assert plt.index is None @@ -12,6 +13,7 @@ def test_can_insert_plt(factories): plt.refresh_from_db() assert plt.index == 0 + assert plt.playlist.modification_date > modification_date def test_insert_use_last_idx_by_default(factories): diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index 192aad382..03babfabb 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -23,6 +23,16 @@ def test_can_create_playlist_via_api(logged_in_api_client): assert playlist.privacy_level == 'everyone' +def test_serializer_includes_tracks_count(factories, logged_in_api_client): + playlist = factories['playlists.Playlist']() + plt = factories['playlists.PlaylistTrack'](playlist=playlist) + + url = reverse('api:v1:playlists-detail', kwargs={'pk': playlist.pk}) + response = logged_in_api_client.get(url) + + assert response.data['tracks_count'] == 1 + + def test_playlist_inherits_user_privacy(logged_in_api_client): url = reverse('api:v1:playlists-list') user = logged_in_api_client.user From f917c5d0c4293cb0d7eb3cb4443e01cf0764e473 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 19 Mar 2018 19:07:52 +0100 Subject: [PATCH 19/41] Playlist filterset --- api/funkwhale_api/playlists/filters.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 api/funkwhale_api/playlists/filters.py diff --git a/api/funkwhale_api/playlists/filters.py b/api/funkwhale_api/playlists/filters.py new file mode 100644 index 000000000..cf8e7dd21 --- /dev/null +++ b/api/funkwhale_api/playlists/filters.py @@ -0,0 +1,13 @@ +from django_filters import rest_framework as filters + +from . import models + + + +class PlaylistFilter(filters.FilterSet): + + class Meta: + model = models.Playlist + fields = { + 'user': ['exact'], + } From 8ff775a126d22da6e96ac6a6a13a1ba038034d0a Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 14:36:35 +0100 Subject: [PATCH 20/41] Added tracks count and sort by modification date in front --- front/src/components/playlists/PlaylistModal.vue | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/front/src/components/playlists/PlaylistModal.vue b/front/src/components/playlists/PlaylistModal.vue index 2e1627001..7bf027db7 100644 --- a/front/src/components/playlists/PlaylistModal.vue +++ b/front/src/components/playlists/PlaylistModal.vue @@ -18,7 +18,8 @@
{{ playlist.name }}
- 45 tracks + {{ playlist.tracks_count }} tracks + Last modification {{ playlist.modification_date | ago}}
@@ -34,6 +35,7 @@ + + + diff --git a/front/src/components/audio/track/Table.vue b/front/src/components/audio/track/Table.vue index 00bcf9f7d..512ba1b49 100644 --- a/front/src/components/audio/track/Table.vue +++ b/front/src/components/audio/track/Table.vue @@ -11,34 +11,11 @@ - - - - - - - - - - - - {{ track.title }} - - - - - {{ track.artist.name }} - - - - - {{ track.album.title }} - - - - + @@ -83,9 +60,8 @@ curl -G -o "{{ track.files[0].filename }}" + + + + + diff --git a/front/src/router/index.js b/front/src/router/index.js index ba9aadd98..31bd0805c 100644 --- a/front/src/router/index.js +++ b/front/src/router/index.js @@ -21,7 +21,7 @@ import RadioBuilder from '@/components/library/radios/Builder' import BatchList from '@/components/library/import/BatchList' import BatchDetail from '@/components/library/import/BatchDetail' import RequestsList from '@/components/requests/RequestsList' - +import PlaylistDetail from '@/views/playlists/Detail' import Favorites from '@/components/favorites/List' Vue.use(Router) @@ -110,6 +110,7 @@ export default new Router({ }, { path: 'radios/build', name: 'library.radios.build', component: RadioBuilder, props: true }, { path: 'radios/build/:id', name: 'library.radios.edit', component: RadioBuilder, props: true }, + { path: 'playlists/:id', name: 'library.playlists.detail', component: PlaylistDetail, props: true }, { path: 'artists/:id', name: 'library.artists.detail', component: LibraryArtist, props: true }, { path: 'albums/:id', name: 'library.albums.detail', component: LibraryAlbum, props: true }, { path: 'tracks/:id', name: 'library.tracks.detail', component: LibraryTrack, props: true }, diff --git a/front/src/views/playlists/Detail.vue b/front/src/views/playlists/Detail.vue new file mode 100644 index 000000000..fc27b7126 --- /dev/null +++ b/front/src/views/playlists/Detail.vue @@ -0,0 +1,88 @@ + + From dd40a4c4d18143bc7f9fc567ccf118fa8078c8b1 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 19:58:38 +0100 Subject: [PATCH 28/41] Cleanup --- front/src/components/Pagination.vue | 1 - front/src/components/playlists/Form.vue | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/front/src/components/Pagination.vue b/front/src/components/Pagination.vue index 57ae4235d..47cf5183a 100644 --- a/front/src/components/Pagination.vue +++ b/front/src/components/Pagination.vue @@ -64,7 +64,6 @@ export default { } } }) - console.log(final) return final }, maxPage: function () { diff --git a/front/src/components/playlists/Form.vue b/front/src/components/playlists/Form.vue index 8b39c6f6c..267e2c10e 100644 --- a/front/src/components/playlists/Form.vue +++ b/front/src/components/playlists/Form.vue @@ -74,6 +74,7 @@ export default { logger.default.info('Successfully created playlist') self.success = true self.isLoading = false + self.name = '' self.$store.dispatch('playlists/fetchOwn') }, error => { logger.default.error('Error while creating playlist') From 16f631af1a379e6e52888278d433611e5f9e1e3d Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 22:44:28 +0100 Subject: [PATCH 29/41] Performance optimization on playlisttrack serialization --- api/funkwhale_api/playlists/models.py | 11 +++++++++++ api/funkwhale_api/playlists/views.py | 7 +++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index ed4307b67..6bb8fe178 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -94,6 +94,15 @@ class Playlist(models.Model): ] return PlaylistTrack.objects.bulk_create(plts) +class PlaylistTrackQuerySet(models.QuerySet): + def for_nested_serialization(self): + return (self.select_related() + .select_related('track__album__artist') + .prefetch_related( + 'track__tags', + 'track__files', + 'track__artist__albums__tracks__tags')) + class PlaylistTrack(models.Model): track = models.ForeignKey( @@ -105,6 +114,8 @@ class PlaylistTrack(models.Model): Playlist, related_name='playlist_tracks', on_delete=models.CASCADE) creation_date = models.DateTimeField(default=timezone.now) + objects = PlaylistTrackQuerySet.as_manager() + class Meta: ordering = ('-playlist', 'index') unique_together = ('playlist', 'index') diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index a077dec27..5de6067ff 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -46,7 +46,7 @@ class PlaylistViewSet( @detail_route(methods=['get']) def tracks(self, request, *args, **kwargs): playlist = self.get_object() - plts = playlist.playlist_tracks.all() + plts = playlist.playlist_tracks.all().for_nested_serialization() serializer = serializers.PlaylistTrackSerializer(plts, many=True) data = { 'count': len(plts), @@ -65,6 +65,9 @@ class PlaylistViewSet( except exceptions.ValidationError as e: payload = {'playlist': e.detail} return Response(payload, status=400) + ids = [p.id for p in plts] + plts = models.PlaylistTrack.objects.filter( + pk__in=ids).order_by('index').for_nested_serialization() serializer = serializers.PlaylistTrackSerializer(plts, many=True) data = { 'count': len(plts), @@ -93,7 +96,7 @@ class PlaylistTrackViewSet( viewsets.GenericViewSet): serializer_class = serializers.PlaylistTrackSerializer - queryset = (models.PlaylistTrack.objects.all()) + queryset = (models.PlaylistTrack.objects.all().for_nested_serialization()) permission_classes = [ permissions.ConditionalAuthentication, permissions.OwnerPermission, From 32dc18ed6e677333bf60060d8d4c24b50c003005 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 23:39:23 +0100 Subject: [PATCH 30/41] Added dangerous-button component, smarter modal --- .../src/components/common/DangerousButton.vue | 44 +++++++++++++++++++ front/src/components/globals.js | 4 ++ front/src/components/semantic/Modal.vue | 40 +++++++++++------ 3 files changed, 74 insertions(+), 14 deletions(-) create mode 100644 front/src/components/common/DangerousButton.vue diff --git a/front/src/components/common/DangerousButton.vue b/front/src/components/common/DangerousButton.vue new file mode 100644 index 000000000..03a579d29 --- /dev/null +++ b/front/src/components/common/DangerousButton.vue @@ -0,0 +1,44 @@ + + diff --git a/front/src/components/globals.js b/front/src/components/globals.js index b1d7d6104..79bbcf1b9 100644 --- a/front/src/components/globals.js +++ b/front/src/components/globals.js @@ -8,4 +8,8 @@ import Username from '@/components/common/Username' Vue.component('username', Username) +import DangerousButton from '@/components/common/DangerousButton' + +Vue.component('dangerous-button', DangerousButton) + export default {} diff --git a/front/src/components/semantic/Modal.vue b/front/src/components/semantic/Modal.vue index ec7a5a088..fec8fdd05 100644 --- a/front/src/components/semantic/Modal.vue +++ b/front/src/components/semantic/Modal.vue @@ -2,7 +2,7 @@
- +
@@ -19,26 +19,38 @@ export default { control: null } }, - mounted () { - this.control = $(this.$el).modal({ - onApprove: function () { - this.$emit('approved') - }.bind(this), - onDeny: function () { - this.$emit('deny') - }.bind(this), - onHidden: function () { - this.$emit('update:show', false) - }.bind(this) - }) + beforeDestroy () { + if (this.control) { + this.control.remove() + } + }, + methods: { + initModal () { + this.control = $(this.$el).modal({ + duration: 100, + onApprove: function () { + this.$emit('approved') + }.bind(this), + onDeny: function () { + this.$emit('deny') + }.bind(this), + onHidden: function () { + this.$emit('update:show', false) + }.bind(this) + }) + } }, watch: { show: { handler (newValue) { if (newValue) { + this.initModal() this.control.modal('show') } else { - this.control.modal('hide') + if (this.control) { + this.control.modal('hide') + this.control.remove() + } } } } From 053fc1171b98ee95738806e4f1da48f088ab52da Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 23:40:11 +0100 Subject: [PATCH 31/41] Renamed playlist icon class --- front/src/components/audio/track/Row.vue | 8 +++++--- front/src/components/playlists/TrackPlaylistIcon.vue | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/front/src/components/audio/track/Row.vue b/front/src/components/audio/track/Row.vue index 4eda9955a..8310e89c4 100644 --- a/front/src/components/audio/track/Row.vue +++ b/front/src/components/audio/track/Row.vue @@ -60,9 +60,11 @@ export default { - diff --git a/front/src/components/playlists/TrackPlaylistIcon.vue b/front/src/components/playlists/TrackPlaylistIcon.vue index 843d15392..bba4c515b 100644 --- a/front/src/components/playlists/TrackPlaylistIcon.vue +++ b/front/src/components/playlists/TrackPlaylistIcon.vue @@ -9,7 +9,7 @@ From a38f64852fdf0c37017674684ff6fb5c4b94e2ff Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Tue, 20 Mar 2018 23:41:15 +0100 Subject: [PATCH 32/41] Can now delete playlist --- .../components/playlists/PlaylistModal.vue | 108 ++++++++++-------- front/src/views/playlists/Detail.vue | 32 +++++- 2 files changed, 91 insertions(+), 49 deletions(-) diff --git a/front/src/components/playlists/PlaylistModal.vue b/front/src/components/playlists/PlaylistModal.vue index b470eb5ce..5fdf585df 100644 --- a/front/src/components/playlists/PlaylistModal.vue +++ b/front/src/components/playlists/PlaylistModal.vue @@ -1,53 +1,66 @@ @@ -100,6 +113,11 @@ export default { p.reverse() return p } + }, + watch: { + '$store.state.route.path' () { + this.$store.commit('playlists/showModal', false) + } } } diff --git a/front/src/views/playlists/Detail.vue b/front/src/views/playlists/Detail.vue index fc27b7126..1f3caa494 100644 --- a/front/src/views/playlists/Detail.vue +++ b/front/src/views/playlists/Detail.vue @@ -1,6 +1,9 @@ + + + diff --git a/front/src/components/playlists/CardList.vue b/front/src/components/playlists/CardList.vue new file mode 100644 index 000000000..4d4746090 --- /dev/null +++ b/front/src/components/playlists/CardList.vue @@ -0,0 +1,34 @@ + + + + + + diff --git a/front/src/components/playlists/Form.vue b/front/src/components/playlists/Form.vue index 267e2c10e..21ddfeaec 100644 --- a/front/src/components/playlists/Form.vue +++ b/front/src/components/playlists/Form.vue @@ -21,8 +21,11 @@
+
+ + +
- diff --git a/front/src/router/index.js b/front/src/router/index.js index 7cffb6f99..802844461 100644 --- a/front/src/router/index.js +++ b/front/src/router/index.js @@ -22,6 +22,7 @@ import BatchList from '@/components/library/import/BatchList' import BatchDetail from '@/components/library/import/BatchDetail' import RequestsList from '@/components/requests/RequestsList' import PlaylistDetail from '@/views/playlists/Detail' +import PlaylistList from '@/views/playlists/List' import Favorites from '@/components/favorites/List' Vue.use(Router) @@ -110,6 +111,17 @@ export default new Router({ }, { path: 'radios/build', name: 'library.radios.build', component: RadioBuilder, props: true }, { path: 'radios/build/:id', name: 'library.radios.edit', component: RadioBuilder, props: true }, + { + path: 'playlists/', + name: 'library.playlists.browse', + component: PlaylistList, + props: (route) => ({ + defaultOrdering: route.query.ordering, + defaultQuery: route.query.query, + defaultPaginateBy: route.query.paginateBy, + defaultPage: route.query.page + }) + }, { path: 'playlists/:id', name: 'library.playlists.detail', diff --git a/front/src/views/playlists/Detail.vue b/front/src/views/playlists/Detail.vue index 1f3caa494..52c38b618 100644 --- a/front/src/views/playlists/Detail.vue +++ b/front/src/views/playlists/Detail.vue @@ -34,7 +34,7 @@
-
+
diff --git a/front/src/views/playlists/List.vue b/front/src/views/playlists/List.vue new file mode 100644 index 000000000..fc5dcbe54 --- /dev/null +++ b/front/src/views/playlists/List.vue @@ -0,0 +1,158 @@ + + + + + + From 6a9a34d244ddc4c4298e5c5364bb214f420691cf Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Wed, 21 Mar 2018 12:19:07 +0100 Subject: [PATCH 37/41] Can now clear playlist --- api/funkwhale_api/playlists/views.py | 8 ++++++ api/tests/playlists/test_views.py | 12 ++++++++ .../src/components/common/DangerousButton.vue | 10 +++++-- front/src/components/playlists/Editor.vue | 28 +++++++++++++++++-- 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index 4880b1886..7b2e7651d 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -75,6 +75,14 @@ class PlaylistViewSet( } return Response(data, status=201) + @detail_route(methods=['delete']) + @transaction.atomic + def clear(self, request, *args, **kwargs): + playlist = self.get_object() + playlist.playlist_tracks.all().delete() + playlist.save(update_fields=['modification_date']) + return Response(status=204) + def get_queryset(self): return self.queryset.filter( fields.privacy_level_query(self.request.user)) diff --git a/api/tests/playlists/test_views.py b/api/tests/playlists/test_views.py index ae3fd0074..e70fef6f0 100644 --- a/api/tests/playlists/test_views.py +++ b/api/tests/playlists/test_views.py @@ -170,3 +170,15 @@ def test_can_add_multiple_tracks_at_once_via_api( for plt in playlist.playlist_tracks.order_by('index'): assert response.data['results'][plt.index]['id'] == plt.id assert plt.track == tracks[plt.index] + + +def test_can_clear_playlist_from_api( + factories, mocker, logged_in_api_client): + playlist = factories['playlists.Playlist'](user=logged_in_api_client.user) + plts = factories['playlists.PlaylistTrack'].create_batch( + size=5, playlist=playlist) + url = reverse('api:v1:playlists-clear', kwargs={'pk': playlist.pk}) + response = logged_in_api_client.delete(url) + + assert response.status_code == 204 + assert playlist.playlist_tracks.count() == 0 diff --git a/front/src/components/common/DangerousButton.vue b/front/src/components/common/DangerousButton.vue index 03a579d29..525b4c48f 100644 --- a/front/src/components/common/DangerousButton.vue +++ b/front/src/components/common/DangerousButton.vue @@ -1,5 +1,5 @@