diff --git a/api/funkwhale_api/playlists/models.py b/api/funkwhale_api/playlists/models.py index 9112dc49c..15332c75a 100644 --- a/api/funkwhale_api/playlists/models.py +++ b/api/funkwhale_api/playlists/models.py @@ -70,7 +70,7 @@ class Playlist(models.Model): return self.name @transaction.atomic - def insert(self, plt, index=None): + def insert(self, plt, index=None, allow_duplicates=True): """ Given a PlaylistTrack, insert it at the correct index in the playlist, and update other tracks index if necessary. @@ -96,6 +96,10 @@ class Playlist(models.Model): if index < 0: raise exceptions.ValidationError("Index must be zero or positive") + if not allow_duplicates: + existing_without_current_plt = existing.exclude(pk=plt.pk) + self._check_duplicate_add(existing_without_current_plt, [plt.track]) + if move: # we remove the index temporarily, to avoid integrity errors plt.index = None @@ -125,7 +129,7 @@ class Playlist(models.Model): return to_update.update(index=models.F("index") - 1) @transaction.atomic - def insert_many(self, tracks): + def insert_many(self, tracks, allow_duplicates=True): existing = self.playlist_tracks.select_for_update() now = timezone.now() total = existing.filter(index__isnull=False).count() @@ -134,6 +138,10 @@ class Playlist(models.Model): raise exceptions.ValidationError( "Playlist would reach the maximum of {} tracks".format(max_tracks) ) + + if not allow_duplicates: + self._check_duplicate_add(existing, tracks) + self.save(update_fields=["modification_date"]) start = total plts = [ @@ -144,6 +152,26 @@ class Playlist(models.Model): ] return PlaylistTrack.objects.bulk_create(plts) + def _check_duplicate_add(self, existing_playlist_tracks, tracks_to_add): + track_ids = [t.pk for t in tracks_to_add] + + duplicates = existing_playlist_tracks.filter( + track__pk__in=track_ids + ).values_list("track__pk", flat=True) + if duplicates: + duplicate_tracks = [t for t in tracks_to_add if t.pk in duplicates] + raise exceptions.ValidationError( + { + "non_field_errors": [ + { + "tracks": duplicate_tracks, + "playlist_name": self.name, + "code": "tracks_already_exist_in_playlist", + } + ] + } + ) + class PlaylistTrackQuerySet(models.QuerySet): def for_nested_serialization(self, actor=None): diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index 8e1227da0..ccdf82f4b 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -24,10 +24,11 @@ class PlaylistTrackSerializer(serializers.ModelSerializer): 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") + fields = ("id", "track", "playlist", "index", "allow_duplicates") def validate_playlist(self, value): if self.context.get("request"): @@ -47,17 +48,21 @@ class PlaylistTrackWriteSerializer(serializers.ModelSerializer): @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) + + 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) + instance.playlist.insert(instance, index, allow_duplicates) + return instance def get_unique_together_validators(self): @@ -151,3 +156,7 @@ class PlaylistAddManySerializer(serializers.Serializer): tracks = serializers.PrimaryKeyRelatedField( many=True, queryset=Track.objects.for_nested_serialization() ) + allow_duplicates = serializers.BooleanField(required=False) + + class Meta: + fields = "allow_duplicates" diff --git a/api/funkwhale_api/playlists/views.py b/api/funkwhale_api/playlists/views.py index ee9b9c4e3..861dc8175 100644 --- a/api/funkwhale_api/playlists/views.py +++ b/api/funkwhale_api/playlists/views.py @@ -55,7 +55,10 @@ class PlaylistViewSet( serializer = serializers.PlaylistAddManySerializer(data=request.data) serializer.is_valid(raise_exception=True) try: - plts = playlist.insert_many(serializer.validated_data["tracks"]) + plts = playlist.insert_many( + serializer.validated_data["tracks"], + serializer.validated_data["allow_duplicates"], + ) except exceptions.ValidationError as e: payload = {"playlist": e.detail} return Response(payload, status=400) diff --git a/api/tests/playlists/test_models.py b/api/tests/playlists/test_models.py index b90f52518..92ffbf4ed 100644 --- a/api/tests/playlists/test_models.py +++ b/api/tests/playlists/test_models.py @@ -124,6 +124,139 @@ def test_insert_many_honor_max_tracks(preferences, factories): playlist.insert_many([track, track, track]) +def test_can_insert_duplicate_by_default(factories): + playlist = factories["playlists.Playlist"]() + track = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=track) + + new = factories["playlists.PlaylistTrack"](playlist=playlist, index=1, track=track) + playlist.insert(new) + + new.refresh_from_db() + assert new.index == 1 + + +def test_cannot_insert_duplicate(factories): + playlist = factories["playlists.Playlist"](name="playlist") + track = factories["music.Track"]() + + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=track) + + with pytest.raises(exceptions.ValidationError) as e: + new = factories["playlists.PlaylistTrack"]( + playlist=playlist, index=1, track=track + ) + playlist.insert(new, allow_duplicates=False) + + errors = e.value.detail["non_field_errors"] + assert len(errors) == 1 + + err = errors[0] + assert err["code"] == "tracks_already_exist_in_playlist" + assert err["playlist_name"] == "playlist" + assert err["tracks"] == [track.title] + + +def test_can_insert_track_to_playlist_with_existing_duplicates(factories): + playlist = factories["playlists.Playlist"]() + existing_duplicate = factories["music.Track"]() + factories["playlists.PlaylistTrack"]( + playlist=playlist, index=0, track=existing_duplicate + ) + factories["playlists.PlaylistTrack"]( + playlist=playlist, index=1, track=existing_duplicate + ) + factories["playlists.PlaylistTrack"]( + playlist=playlist, index=2, track=existing_duplicate + ) + + new_track = factories["music.Track"]() + new_plt = factories["playlists.PlaylistTrack"]( + playlist=playlist, index=3, track=new_track + ) + + # no error + playlist.insert(new_plt, allow_duplicates=False) + + +def test_can_insert_duplicate_with_override(factories): + playlist = factories["playlists.Playlist"]() + track = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=track) + + new = factories["playlists.PlaylistTrack"](playlist=playlist, index=1, track=track) + playlist.insert(new, allow_duplicates=True) + + new.refresh_from_db() + assert new.index == 1 + + +def test_can_insert_many_duplicates_by_default(factories): + playlist = factories["playlists.Playlist"]() + + t1 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=t1) + + t2 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=1, track=t2) + + t4 = factories["music.Track"]() + + tracks = [t1, t4, t2] + + plts = playlist.insert_many(tracks) + + assert len(plts) == 3 + assert plts[0].track == t1 + assert plts[1].track == t4 + assert plts[2].track == t2 + + +def test_cannot_insert_many_duplicates(factories): + playlist = factories["playlists.Playlist"](name="playlist") + + t1 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=t1) + + t2 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=1, track=t2) + + t4 = factories["music.Track"]() + + with pytest.raises(exceptions.ValidationError) as e: + tracks = [t1, t4, t2] + playlist.insert_many(tracks, allow_duplicates=False) + + errors = e.value.detail["non_field_errors"] + assert len(errors) == 1 + + err = errors[0] + assert err["code"] == "tracks_already_exist_in_playlist" + assert err["playlist_name"] == "playlist" + assert err["tracks"] == [t1.title, t2.title] + + +def test_can_insert_many_duplicates_with_override(factories): + playlist = factories["playlists.Playlist"]() + + t1 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=0, track=t1) + + t2 = factories["music.Track"]() + factories["playlists.PlaylistTrack"](playlist=playlist, index=1, track=t2) + + t4 = factories["music.Track"]() + + tracks = [t1, t4, t2] + + plts = playlist.insert_many(tracks, allow_duplicates=True) + + assert len(plts) == 3 + assert plts[0].track == t1 + assert plts[1].track == t4 + assert plts[2].track == t2 + + @pytest.mark.parametrize( "privacy_level,expected", [("me", False), ("instance", False), ("everyone", True)] ) diff --git a/api/tests/playlists/test_serializers.py b/api/tests/playlists/test_serializers.py index 0afc927ad..250094729 100644 --- a/api/tests/playlists/test_serializers.py +++ b/api/tests/playlists/test_serializers.py @@ -24,7 +24,7 @@ def test_create_insert_is_called_when_index_is_None(factories, mocker): assert serializer.is_valid() is True plt = serializer.save() - insert.assert_called_once_with(playlist, plt, None) + insert.assert_called_once_with(playlist, plt, None, True) assert plt.index == 0 @@ -41,7 +41,7 @@ def test_create_insert_is_called_when_index_is_provided(factories, mocker): plt = serializer.save() first.refresh_from_db() - insert.assert_called_once_with(playlist, plt, 0) + insert.assert_called_once_with(playlist, plt, 0, True) assert plt.index == 0 assert first.index == 1 @@ -60,11 +60,35 @@ def test_update_insert_is_called_when_index_is_provided(factories, mocker): plt = serializer.save() first.refresh_from_db() - insert.assert_called_once_with(playlist, plt, 0) + 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"]() diff --git a/changes/changelog.d/784.feature b/changes/changelog.d/784.feature new file mode 100644 index 000000000..8e61fb6e3 --- /dev/null +++ b/changes/changelog.d/784.feature @@ -0,0 +1 @@ +Display a confirmation dialog when adding duplicate songs to a playlist (#784) diff --git a/front/src/components/playlists/Editor.vue b/front/src/components/playlists/Editor.vue index fca53f067..3ce554301 100644 --- a/front/src/components/playlists/Editor.vue +++ b/front/src/components/playlists/Editor.vue @@ -18,13 +18,23 @@ +