From 22f0235045479ea3978057115a9c99fff0b3c091 Mon Sep 17 00:00:00 2001 From: Qasim Ali Date: Wed, 24 Apr 2019 11:31:46 +0200 Subject: [PATCH] refactor playlist duplicate error structure - use non_field_errors struct when writing duplicate track errors - generalize frontend error handler and update frontend error parsing --- api/funkwhale_api/playlists/models.py | 32 ++++- api/funkwhale_api/playlists/serializers.py | 15 +- api/funkwhale_api/playlists/views.py | 5 +- api/tests/playlists/test_models.py | 133 ++++++++++++++++++ api/tests/playlists/test_serializers.py | 30 +++- changes/changelog.d/784.feature | 1 + front/src/components/playlists/Editor.vue | 47 ++++++- .../components/playlists/PlaylistModal.vue | 39 ++++- front/src/main.js | 5 +- 9 files changed, 285 insertions(+), 22 deletions(-) create mode 100644 changes/changelog.d/784.feature 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 @@ +
+

Some tracks in your queue are already in this playlist:

+ + +
@@ -91,17 +101,25 @@ export default { return { plts: this.playlistTracks, isLoading: false, - errors: [] + errors: [], + duplicateTrackAddInfo: {}, + showDuplicateTrackAddConfirmation: false } }, methods: { success () { this.isLoading = false this.errors = [] + this.showDuplicateTrackAddConfirmation = false }, errored (errors) { this.isLoading = false - this.errors = errors + if (errors.length == 1 && errors[0].code == 'tracks_already_exist_in_playlist') { + this.duplicateTrackAddInfo = errors[0] + this.showDuplicateTrackAddConfirmation = true + } else { + this.errors = errors + } }, reorder ({oldIndex, newIndex}) { let self = this @@ -139,21 +157,31 @@ export default { self.errored(error.backendErrors) }) }, - insertMany (tracks) { + insertMany (tracks, allowDuplicates) { let self = this let ids = tracks.map(t => { return t.id }) + let payload = { + tracks: ids, + allow_duplicates: allowDuplicates + } self.isLoading = true let url = 'playlists/' + this.playlist.id + '/add/' - axios.post(url, {tracks: ids}).then((response) => { + axios.post(url, payload).then((response) => { response.data.results.forEach(r => { self.plts.push(r) }) self.success() self.$store.dispatch('playlists/fetchOwn') }, error => { - self.errored(error.backendErrors) + // if backendErrors isn't populated (e.g. duplicate track exceptions raised by + // the playlist model), read directly from the response + if (error.rawPayload.playlist) { + self.errored(error.rawPayload.playlist.non_field_errors) + } else { + self.errored(error.backendErrors) + } }) } }, @@ -173,6 +201,9 @@ export default { if (this.errors.length > 0) { return 'errored' } + if (this.showDuplicateTrackAddConfirmation) { + return 'confirmDuplicateAdd' + } return 'saved' } }, @@ -192,4 +223,8 @@ export default { diff --git a/front/src/components/playlists/PlaylistModal.vue b/front/src/components/playlists/PlaylistModal.vue index 522f150fc..44969afbf 100644 --- a/front/src/components/playlists/PlaylistModal.vue +++ b/front/src/components/playlists/PlaylistModal.vue @@ -18,6 +18,19 @@
+
+

%{ track } is already in %{ playlist }.

+ + +
The track can't be added to a playlist
    @@ -52,7 +65,7 @@ v-if="track" class="ui green icon basic small right floated button" :title="labels.addToPlaylist" - @click="addToPlaylist(playlist.id)"> + @click="addToPlaylist(playlist.id, false)"> Add track
@@ -84,26 +97,38 @@ export default { data () { return { formKey: String(new Date()), - errors: [] + errors: [], + duplicateTrackAddInfo: {}, + showDuplicateTrackAddConfirmation: false, + lastSelectedPlaylist: -1, } }, methods: { update (v) { this.$store.commit('playlists/showModal', v) }, - addToPlaylist (playlistId) { + addToPlaylist (playlistId, allowDuplicate) { let self = this let payload = { track: this.track.id, - playlist: playlistId + playlist: playlistId, + allow_duplicates: allowDuplicate } + + self.lastSelectedPlaylist = playlistId + return axios.post('playlist-tracks/', payload).then(response => { logger.default.info('Successfully added track to playlist') self.update(false) self.$store.dispatch('playlists/fetchOwn') }, error => { - logger.default.error('Error while adding track to playlist') - self.errors = error.backendErrors + if (error.backendErrors.length == 1 && error.backendErrors[0].code == 'tracks_already_exist_in_playlist') { + self.duplicateTrackAddInfo = error.backendErrors[0] + self.showDuplicateTrackAddConfirmation = true + } else { + self.errors = error.backendErrors + self.showDuplicateTrackAddConfirmation = false + } }) } }, @@ -126,9 +151,11 @@ export default { watch: { '$store.state.route.path' () { this.$store.commit('playlists/showModal', false) + this.showDuplicateTrackAddConfirmation = false }, '$store.state.playlists.showModal' () { this.formKey = String(new Date()) + this.showDuplicateTrackAddConfirmation = false } } } diff --git a/front/src/main.js b/front/src/main.js index 959b776b3..dd755dee3 100644 --- a/front/src/main.js +++ b/front/src/main.js @@ -97,8 +97,11 @@ axios.interceptors.response.use(function (response) { if (error.response.data.detail) { error.backendErrors.push(error.response.data.detail) } else { + error.rawPayload = error.response.data for (var field in error.response.data) { - if (error.response.data.hasOwnProperty(field)) { + // some views (e.g. v1/playlists/{id}/add) have deeper nested data (e.g. data[field] + // is another object), so don't try to unpack non-array fields + if (error.response.data.hasOwnProperty(field) && error.response.data[field].forEach) { error.response.data[field].forEach(e => { error.backendErrors.push(e) })