Updated playlistclear to use playlist lookup helper and added save failure tests.

mopidy/mpd/protocol/stored_playlists.py

@@ -230,8 +230,7 @@ def playlistclear(context, name):
     The playlist will be created if it does not exist.
     """
     _check_playlist_name(name)
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name, must_exist=False)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
     if not playlist:
         playlist = context.core.playlists.create(name).get()
 
@@ -239,7 +238,7 @@ def playlistclear(context, name):
     playlist = playlist.replace(tracks=[])
     if context.core.playlists.save(playlist).get() is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add('playlistdelete', songpos=protocol.UINT)
@@ -266,7 +265,7 @@ def playlistdelete(context, name, songpos):
     saved_playlist = context.core.playlists.save(playlist).get()
     if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add(
@@ -307,7 +306,7 @@ def playlistmove(context, name, from_pos, to_pos):
     saved_playlist = context.core.playlists.save(playlist).get()
     if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add('rename')

tests/dummy_backend.py

@@ -102,11 +102,15 @@ class DummyPlaylistsProvider(backend.PlaylistsProvider):
     def __init__(self, backend):
         super(DummyPlaylistsProvider, self).__init__(backend)
         self._playlists = []
+        self._allow_save = True
 
     def set_dummy_playlists(self, playlists):
         """For tests using the dummy provider through an actor proxy."""
         self._playlists = playlists
 
+    def set_allow_save(self, enabled):
+        self._allow_save = enabled
+
     def as_list(self):
         return [
             Ref.playlist(uri=pl.uri, name=pl.name) for pl in self._playlists]
@@ -137,6 +141,9 @@ class DummyPlaylistsProvider(backend.PlaylistsProvider):
             self._playlists.remove(playlist)
 
     def save(self, playlist):
+        if not self._allow_save:
+            return None
+
         old_playlist = self.lookup(playlist.uri)
 
         if old_playlist is not None:

tests/mpd/protocol/test_stored_playlists.py

@@ -320,6 +320,13 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertInResponse('OK')
         self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
 
+    def test_playlistclear_creates_playlist_save_fails(self):
+        self.backend.playlists.set_allow_save(False)
+        self.send_request('playlistclear "name"')
+
+        self.assertInResponse('ACK [0@0] {playlistclear} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistclear_invalid_name_acks(self):
         self.send_request('playlistclear "foo/bar"')
         self.assertInResponse('ACK [2@0] {playlistclear} playlist name is '
@@ -342,6 +349,22 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertEqual(
             2, len(self.backend.playlists.get_items('dummy:a1').get()))
 
+    def test_playlistdelete_save_fails(self):
+        tracks = [
+            Track(uri='dummy:a'),
+            Track(uri='dummy:b'),
+            Track(uri='dummy:c'),
+        ]   # len() == 3
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='name', uri='dummy:a1', tracks=tracks)])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('playlistdelete "name" "2"')
+
+        self.assertInResponse('ACK [0@0] {playlistdelete} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistdelete_invalid_name_acks(self):
         self.send_request('playlistdelete "foo/bar" "0"')
         self.assertInResponse('ACK [2@0] {playlistdelete} playlist name is '
@@ -374,6 +397,22 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             "dummy:c",
             self.backend.playlists.get_items('dummy:a1').get()[0].uri)
 
+    def test_playlistmove_save_falis(self):
+        tracks = [
+            Track(uri='dummy:a'),
+            Track(uri='dummy:b'),
+            Track(uri='dummy:c')    # this one is getting moved to top
+        ]
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='name', uri='dummy:a1', tracks=tracks)])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('playlistmove "name" "2" "0"')
+
+        self.assertInResponse('ACK [0@0] {playlistmove} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistmove_invalid_name_acks(self):
         self.send_request('playlistmove "foo/bar" "0" "1"')
         self.assertInResponse('ACK [2@0] {playlistmove} playlist name is '
@@ -421,6 +460,17 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertIsNotNone(
             self.backend.playlists.lookup('dummy:new_name').get())
 
+    def test_rename_save_fails(self):
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='old_name', uri='dummy:a1', tracks=[Track(uri='b')])])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('rename "old_name" "new_name"')
+
+        self.assertInResponse('ACK [0@0] {rename} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_rename_unknown_playlist_acks(self):
         self.send_request('rename "foo" "bar"')
         self.assertInResponse('ACK [50@0] {rename} No such playlist')
@@ -472,6 +522,13 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertInResponse('OK')
         self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
 
+    def test_save_fails(self):
+        self.backend.playlists.set_allow_save(False)
+        self.send_request('save "name"')
+
+        self.assertInResponse('ACK [0@0] {save} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_save_invalid_name_acks(self):
         self.send_request('save "foo/bar"')
         self.assertInResponse('ACK [2@0] {save} playlist name is invalid: '