Change stored_playlists.delete() to accepting an URI (#217)

docs/changes.rst

@@ -56,13 +56,18 @@ backends:
   dummy/mocked lower layers easier than with the old variant, where
   dependencies where looked up in Pykka's actor registry.
 
-- The stored playlists part of the core API have been revised a bit:
+- The stored playlists part of the core API have been revised to be more
+  focused around the playlist URI, and some redundant functionality have been
+  removed:
 
   - :attr:`mopidy.core.StoredPlaylistsController.playlists` no longer supports
     assignment to it. The `playlists` property on the backend layer still does,
     and all functionality is maintained by assigning to the playlists
     collections at the backend level.
 
+  - :meth:`mopidy.core.StoredPlaylistsController.delete` now accepts an URI,
+    and not a playlist object.
+
   - :meth:`mopidy.core.StoredPlaylistsController.save` now returns the saved
     playlist. The returned playlist may differ from the saved playlist, and
     should thus be used instead of the playlist passed to ``save()``.

mopidy/backends/base.py

@@ -180,7 +180,7 @@ class BaseStoredPlaylistsProvider(object):
         """
         raise NotImplementedError
 
-    def delete(self, playlist):
+    def delete(self, uri):
         """
         See :meth:`mopidy.core.StoredPlaylistsController.delete`.
 

mopidy/backends/local/stored_playlists.py

@@ -29,15 +29,13 @@ class LocalStoredPlaylistsProvider(base.BaseStoredPlaylistsProvider):
         self.save(playlist)
         return playlist
 
-    def delete(self, playlist):
-        if playlist not in self._playlists:
+    def delete(self, uri):
+        playlist = self.lookup(uri)
+        if not playlist:
             return
 
         self._playlists.remove(playlist)
-        filename = os.path.join(self._folder, playlist.name + '.m3u')
-
-        if os.path.exists(filename):
-            os.remove(filename)
+        self._delete_m3u(playlist)
 
     def lookup(self, uri):
         for playlist in self._playlists:
@@ -94,3 +92,8 @@ class LocalStoredPlaylistsProvider(base.BaseStoredPlaylistsProvider):
                 else:
                     uri = track.uri
                 file_handle.write(uri + '\n')
+
+    def _delete_m3u(self, playlist):
+        file_path = playlist.uri[len('file://'):]
+        if os.path.exists(file_path):
+            os.remove(file_path)

mopidy/core/stored_playlists.py

@@ -43,15 +43,20 @@ class StoredPlaylistsController(object):
             backend = self.backends[0]
         return backend.stored_playlists.create(name).get()
 
-    def delete(self, playlist):
+    def delete(self, uri):
         """
-        Delete playlist.
+        Delete playlist identified by the URI.
 
-        :param playlist: the playlist to delete
-        :type playlist: :class:`mopidy.models.Playlist`
+        If the URI doesn't match the URI schemes handled by the current
+        backends, nothing happens.
+
+        :param uri: URI of the playlist to delete
+        :type uri: string
         """
-        # TODO Support multiple backends
-        return self.backends[0].stored_playlists.delete(playlist).get()
+        uri_scheme = urlparse.urlparse(uri).scheme
+        backend = self.backends.by_uri_scheme.get(uri_scheme, None)
+        if backend:
+            return backend.stored_playlists.delete(uri).get()
 
     def get(self, **criteria):
         """

tests/backends/base/stored_playlists.py

@@ -47,12 +47,15 @@ class StoredPlaylistsControllerTest(object):
         self.assert_(not self.stored.playlists)
 
     def test_delete_non_existant_playlist(self):
-        self.stored.delete(Playlist())
+        self.stored.delete('file:///unknown/playlist')
 
-    def test_delete_playlist(self):
+    def test_delete_playlist_removes_it_from_the_collection(self):
         playlist = self.stored.create('test')
-        self.stored.delete(playlist)
-        self.assert_(not self.stored.playlists)
+        self.assertIn(playlist, self.stored.playlists)
+
+        self.stored.delete(playlist.uri)
+
+        self.assertNotIn(playlist, self.stored.playlists)
 
     def test_get_without_criteria(self):
         test = self.stored.get

tests/backends/local/stored_playlists_test.py

@@ -39,15 +39,12 @@ class LocalStoredPlaylistsControllerTest(
 
     def test_deleted_playlist_is_removed(self):
         path = os.path.join(settings.LOCAL_PLAYLIST_PATH, 'test.m3u')
-
         self.assertFalse(os.path.exists(path))
 
         playlist = self.stored.create('test')
-
         self.assertTrue(os.path.exists(path))
 
-        self.stored.delete(playlist)
-
+        self.stored.delete(playlist.uri)
         self.assertFalse(os.path.exists(path))
 
     def test_playlist_contents_is_written_to_disk(self):

tests/core/stored_playlists_test.py

@@ -59,6 +59,24 @@ class StoredPlaylistsTest(unittest.TestCase):
         self.assertFalse(self.sp1.create.called)
         self.sp2.create.assert_called_once_with('foo')
 
+    def test_delete_selects_the_dummy1_backend(self):
+        self.core.stored_playlists.delete('dummy1:a')
+
+        self.sp1.delete.assert_called_once_with('dummy1:a')
+        self.assertFalse(self.sp2.delete.called)
+
+    def test_delete_selects_the_dummy2_backend(self):
+        self.core.stored_playlists.delete('dummy2:a')
+
+        self.assertFalse(self.sp1.delete.called)
+        self.sp2.delete.assert_called_once_with('dummy2:a')
+
+    def test_delete_with_unknown_uri_scheme_does_nothing(self):
+        self.core.stored_playlists.delete('unknown:a')
+
+        self.assertFalse(self.sp1.delete.called)
+        self.assertFalse(self.sp2.delete.called)
+
     def test_lookup_selects_the_dummy1_backend(self):
         self.core.stored_playlists.lookup('dummy1:a')