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

@@ -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):

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"

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)

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)]
 )

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"]()

changes/changelog.d/784.feature

@@ -0,0 +1 @@
+Display a confirmation dialog when adding duplicate songs to a playlist (#784)

front/src/components/playlists/Editor.vue

@@ -18,13 +18,23 @@
           </ul>
         </div>
       </template>
+      <div v-else-if="status === 'confirmDuplicateAdd'" class="ui warning message">
+        <p translate-context="Content/Playlist/Paragraph"
+            v-translate="{playlist: playlist.name}">Some tracks in your queue are already in this playlist:</p>
+        <ul id="duplicateTrackList" class="ui relaxed divided list">
+          <li v-for="track in duplicateTrackAddInfo.tracks" class="ui item">{{ track }}</li>
+        </ul>
+        <button
+          class="ui small green button"
+          @click="insertMany(queueTracks, true)"><translate translate-context="*/Playlist/Button.Label/Verb">Add anyways</translate></button>
+      </div>
       <template v-else-if="status === 'saved'">
         <i class="green check icon"></i> <translate translate-context="Content/Playlist/Paragraph">Changes synced with server</translate>
       </template>
     </div>
     <div class="ui bottom attached segment">
       <div
-        @click="insertMany(queueTracks)"
+        @click="insertMany(queueTracks, false)"
         :disabled="queueTracks.length === 0"
         :class="['ui', {disabled: queueTracks.length === 0}, 'labeled', 'icon', 'button']"
         :title="labels.copyTitle">
@@ -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
+      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 => {
+        // 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 {
 
 <!-- Add "scoped" attribute to limit CSS to this component only -->
 <style scoped>
+#duplicateTrackList {
+  max-height: 10em;
+  overflow-y: auto;
+}
 </style>

front/src/components/playlists/PlaylistModal.vue

@@ -18,6 +18,19 @@
 
         <playlist-form :key="formKey"></playlist-form>
         <div class="ui divider"></div>
+        <div v-if="showDuplicateTrackAddConfirmation" class="ui warning message">
+          <p translate-context="Popup/Playlist/Paragraph"
+            v-translate="{track: track.title, playlist: duplicateTrackAddInfo.playlist_name}"
+            :translate-params="{track: track.title, playlist: duplicateTrackAddInfo.playlist_name}"><strong>%{ track }</strong> is already in <strong>%{ playlist }</strong>.</p>
+          <button
+            @click="update(false)"
+            class="ui small cancel button"><translate translate-context="*/*/Button.Label/Verb">Cancel</translate>
+          </button>
+          <button
+            class="ui small green button"
+            @click="addToPlaylist(lastSelectedPlaylist, true)">
+              <translate translate-context="*/Playlist/Button.Label/Verb">Add anyways</translate></button>
+        </div>
         <div v-if="errors.length > 0" class="ui negative message">
           <div class="header"><translate translate-context="Popup/Playlist/Error message.Title">The track can't be added to a playlist</translate></div>
           <ul class="list">
@@ -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)">
                   <i class="plus icon"></i> <translate translate-context="Popup/Playlist/Table.Button.Label/Verb">Add track</translate>
                 </div>
               </td>
@@ -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')
+        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
     }
   }
 }

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)
           })