Add test for deleting playlist

locale/en.ftl

@@ -20,6 +20,7 @@ error-playlist-not-found = Playlist not found.
 error-playlist-item-not-found = Playlist item not found.
 error-item-not-in-playlist = Item not in playlist.
 error-empty-playlist = You don't have anything to play. Please add some songs to your playlist and try again.
+error-active-playlist = Cannot delete the active playlist.
 error-history-entry-not-found = History entry not found.
 error-media-not-found = Media object not found.
 error-no-self-favorite = You can't favorite your own plays.

src/controllers/playlists.js

@@ -1,4 +1,4 @@
-import { HTTPError, PlaylistNotFoundError } from '../errors/index.js';
+import { HTTPError } from '../errors/index.js';
 import { serializePlaylist, serializePlaylistItem } from '../utils/serialize.js';
 import getOffsetPagination from '../utils/getOffsetPagination.js';
 import toItemResponse from '../utils/toItemResponse.js';
@@ -89,10 +89,6 @@ async function getPlaylist(req) {
 
   const playlist = await playlists.getUserPlaylist(user, id);
 
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
-
   return toItemResponse(
     serializePlaylist(playlist),
     { url: req.fullUrl },
@@ -136,16 +132,15 @@ async function createPlaylist(req) {
 async function deletePlaylist(req) {
   const { user } = req;
   const { id } = req.params;
-  const { playlists } = req.uwave;
+  const { db, playlists } = req.uwave;
 
-  const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
+  await db.transaction().execute(async (tx) => {
+    const playlist = await playlists.getUserPlaylist(user, id, tx);
 
-  await playlists.deletePlaylist(playlist);
+    await playlists.deletePlaylist(playlist, tx);
+  });
 
-  return toItemResponse({}, { url: req.fullUrl });
+  return toItemResponse({});
 }
 
 const patchableKeys = ['name', 'description'];
@@ -174,9 +169,6 @@ async function updatePlaylist(req) {
   });
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   const updatedPlaylist = await playlists.updatePlaylist(playlist, patch);
 
@@ -204,9 +196,6 @@ async function renamePlaylist(req) {
   const { playlists } = req.uwave;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   const updatedPlaylist = await playlists.updatePlaylist(playlist, { name });
 
@@ -230,9 +219,6 @@ async function activatePlaylist(req) {
   const { id } = req.params;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   await db.updateTable('users')
     .where('id', '=', user.id)
@@ -260,9 +246,6 @@ async function getPlaylistItems(req) {
   const pagination = getOffsetPagination(req.query);
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   const items = await playlists.getPlaylistItems(playlist, filter, pagination);
 
@@ -298,9 +281,6 @@ async function addPlaylistItems(req) {
   const { at, after, items } = req.body;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   let options;
   if (at === 'start' || at === 'end') {
@@ -348,9 +328,6 @@ async function removePlaylistItems(req) {
   const { items } = req.body;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   await playlists.removePlaylistItems(playlist, items);
 
@@ -378,9 +355,6 @@ async function movePlaylistItems(req) {
   const { at, after, items } = req.body;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   let options;
   if (at === 'start' || at === 'end') {
@@ -412,9 +386,6 @@ async function shufflePlaylistItems(req) {
   const { id } = req.params;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   await playlists.shufflePlaylist(playlist);
 
@@ -436,9 +407,6 @@ async function getPlaylistItem(req) {
   const { id, itemID } = req.params;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   const { playlistItem, media } = await playlists.getPlaylistItem(playlist, itemID);
 
@@ -476,9 +444,6 @@ async function updatePlaylistItem(req) {
   };
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   const { playlistItem, media } = await playlists.getPlaylistItem(playlist, itemID);
   const updatedItem = await playlists.updatePlaylistItem(playlistItem, patch);
@@ -501,9 +466,6 @@ async function removePlaylistItem(req) {
   const { id, itemID } = req.params;
 
   const playlist = await playlists.getUserPlaylist(user, id);
-  if (!playlist) {
-    throw new PlaylistNotFoundError({ id });
-  }
 
   await playlists.removePlaylistItems(playlist, [itemID]);
 

src/errors/index.js

@@ -252,6 +252,12 @@ const EmptyPlaylistError = createErrorClass('EmptyPlaylistError', {
   base: Forbidden,
 });
 
+const PlaylistActiveError = createErrorClass('PlaylistActiveError', {
+  code: 'active-playlist',
+  string: 'error-active-playlist',
+  base: BadRequest,
+});
+
 const WaitlistLockedError = createErrorClass('WaitlistLockedError', {
   code: 'waitlist-locked',
   string: 'error-waitlist-locked',
@@ -311,6 +317,7 @@ export {
   SourceNotFoundError,
   SourceNoImportError,
   EmptyPlaylistError,
+  PlaylistActiveError,
   WaitlistLockedError,
   AlreadyInWaitlistError,
   UserNotInWaitlistError,

src/plugins/playlists.js

@@ -4,6 +4,7 @@ import {
   ItemNotInPlaylistError,
   MediaNotFoundError,
   UserNotFoundError,
+  PlaylistActiveError,
 } from '../errors/index.js';
 import Page from '../Page.js';
 import routes from '../routes/playlists.js';
@@ -13,6 +14,7 @@ import {
   arrayCycle,
   arrayShuffle as arrayShuffle,
   fromJson,
+  isForeignKeyError,
   json,
   jsonb,
   jsonEach,
@@ -249,35 +251,42 @@ class PlaylistsRepository {
   async createPlaylist(user, { name }, tx = this.#uw.db) {
     const id = /** @type {PlaylistID} */ (randomUUID());
 
-    const playlist = await tx.insertInto('playlists')
-      .values({
-        id,
-        name,
-        userID: user.id,
-        items: jsonb([]),
-      })
-      .returning([
-        'id',
-        'userID',
-        'name',
-        (eb) => jsonLength(eb.ref('items')).as('size'),
-        'createdAt',
-        'updatedAt',
-      ])
-      .executeTakeFirstOrThrow();
+    const result = await tx.transaction().execute(async (tx) => {
+      const playlist = await tx.insertInto('playlists')
+        .values({
+          id,
+          name,
+          userID: user.id,
+          items: jsonb([]),
+        })
+        .returning([
+          'id',
+          'userID',
+          'name',
+          (eb) => jsonLength(eb.ref('items')).as('size'),
+          'createdAt',
+          'updatedAt',
+        ])
+        .executeTakeFirstOrThrow();
 
-    let active = false;
-    // If this is the user's first playlist, immediately activate it.
-    if (user.activePlaylistID == null) {
-      this.#logger.info({ userId: user.id, playlistId: playlist.id }, 'activating first playlist');
-      await tx.updateTable('users')
-        .where('users.id', '=', user.id)
+      const updated = await tx.updateTable('users')
+        .where('id', '=', user.id)
+        .where('activePlaylistID', 'is', null)
         .set({ activePlaylistID: playlist.id })
-        .execute();
-      active = true;
+        .returning(['activePlaylistID'])
+        .executeTakeFirst();
+
+      return {
+        playlist,
+        active: updated != null && updated.activePlaylistID === playlist.id,
+      };
+    });
+
+    if (result.active) {
+      this.#logger.info({ userId: user.id, playlistId: result.playlist.id }, 'activated first playlist');
     }
 
-    return { playlist, active };
+    return result;
   }
 
   /**
@@ -345,12 +354,34 @@ class PlaylistsRepository {
   }
 
   /**
+   * Delete a playlist. An active playlist cannot be deleted.
+   *
    * @param {Playlist} playlist
+   * @returns {Promise<void>}
    */
   async deletePlaylist(playlist, tx = this.#uw.db) {
-    await tx.deleteFrom('playlists')
-      .where('id', '=', playlist.id)
-      .execute();
+    // This *must* be executed in a transaction, else it would be possible for
+    // the items to be deleted but not the playlist metadata.
+    // Maybe it'd be better to just require the `tx` parameter, or not support
+    // passing one in?
+    if (!tx.isTransaction) {
+      return tx.transaction().execute((tx) => this.deletePlaylist(playlist, tx));
+    }
+
+    try {
+      // Missing `ON DELETE CASCADE`, so we have to do it manually, unfortunately...
+      await tx.deleteFrom('playlistItems')
+        .where('playlistID', '=', playlist.id)
+        .execute();
+      await tx.deleteFrom('playlists')
+        .where('id', '=', playlist.id)
+        .execute();
+    } catch (err) {
+      if (isForeignKeyError(err)) {
+        throw new PlaylistActiveError();
+      }
+      throw err;
+    }
   }
 
   /**