From 5297f091af9c65b5d5391821d2b6fa598048132d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 18:29:18 -0500 Subject: [PATCH] Let a display's playlist actually be cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "No playlist" was an option you could select that did nothing. The picker offered it, and the change handler opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it sent no request, changed nothing, and said nothing. The guard was honest about why: there was no way to do it. PUT /devices/:id has never read playlist_id (200, ignored), and POST /playlists/:id/assign can only ever set one. Reported on #234 as "I also selected No playlist ... it still showed the same video". It did, and my first explanation blamed the playlist-swap deferral. The deferral would have stranded it too — that is fixed separately and tested — but on this path nothing was ever sent, so the deferral never got the chance. DELETE /api/devices/:id/playlist, device-scoped rather than playlist-scoped because there is no playlist to authorize against when clearing. Ownership goes through checkDeviceOwnership like every other device mutation, so a viewer and a stranger are refused. Clearing an already-clear display is a no-op success, since it lives in a dropdown someone can pick twice. The now-empty playlist is pushed to the device so the screen stops, rather than leaving the old content up until something else happens to refresh it. Validated on an Android 12 emulator against the reporter's shape: cleared while a YouTube item was on screen, zero plays afterwards, device row cleared. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- frontend/js/api.js | 1 + frontend/js/views/device-detail.js | 11 ++- server/routes/devices.js | 33 +++++++ server/test/clear-device-playlist.test.js | 107 ++++++++++++++++++++++ 4 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 server/test/clear-device-playlist.test.js diff --git a/frontend/js/api.js b/frontend/js/api.js index 9ec7996..9fb8b82 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -193,6 +193,7 @@ export const api = { getItemSchedules: (id, itemId) => request(`/playlists/${id}/items/${itemId}/schedules`), setItemSchedules: (id, itemId, blocks) => request(`/playlists/${id}/items/${itemId}/schedules`, { method: 'PUT', body: JSON.stringify({ blocks }) }), assignPlaylistToDevice: (playlistId, device_id) => request(`/playlists/${playlistId}/assign`, { method: 'POST', body: JSON.stringify({ device_id }) }), + clearDevicePlaylist: (device_id) => request(`/devices/${device_id}/playlist`, { method: 'DELETE' }), publishPlaylist: (id) => request(`/playlists/${id}/publish`, { method: 'POST' }), discardPlaylistDraft: (id) => request(`/playlists/${id}/discard`, { method: 'POST' }), diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 07df81b..53b6159 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -1039,10 +1039,15 @@ function setupActions(device) { playlistPicker.addEventListener('change', async () => { const newPlaylistId = playlistPicker.value; - if (!newPlaylistId) return; // Don't allow deselecting for now try { - await api.assignPlaylistToDevice(newPlaylistId, device.id); - device.playlist_id = newPlaylistId; + // Empty value is the "No playlist" option. It used to be discarded right here, so the + // option was offered, selecting it did nothing, and nothing said so (#234). + if (newPlaylistId) { + await api.assignPlaylistToDevice(newPlaylistId, device.id); + } else { + await api.clearDevicePlaylist(device.id); + } + device.playlist_id = newPlaylistId || null; const assignments = await api.getAssignments(device.id); const pc = document.getElementById('playlistContainer'); pc.innerHTML = renderPlaylist(assignments); diff --git a/server/routes/devices.js b/server/routes/devices.js index 30e56b5..e098d59 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -215,6 +215,39 @@ router.get('/:id/preview-payload', (req, res) => { }); // Update device +// Clear a device's playlist — the "No playlist" option in the dashboard picker. +// +// There was no way to do this. PUT /devices/:id ignores playlist_id (it always has), and +// POST /playlists/:id/assign can only ever SET one, so the picker carried a guard that +// silently discarded the selection: `if (!newPlaylistId) return; // Don't allow deselecting`. +// The option was offered, selecting it did nothing, and no error said so — reported on #234 +// as "I selected No playlist and it still showed the same video". It did. +// +// Device-scoped rather than playlist-scoped because there is no playlist to authorize +// against when clearing; ownership is checked the same way every other device mutation +// checks it. Clearing an already-clear device is a no-op success, so the button is safe to +// press twice. +router.delete('/:id/playlist', (req, res) => { + const device = checkDeviceOwnership(req, res); + if (!device) return; + + db.prepare('UPDATE devices SET playlist_id = NULL, updated_at = ? WHERE id = ?') + .run(Math.floor(Date.now() / 1000), req.params.id); + + // Push the now-empty playlist so the screen stops, rather than leaving the old content up + // until something else happens to update it. + try { + const io = req.app.get('io'); + if (io) { + const { buildPlaylistPayload } = require('../ws/deviceSocket'); + const commandQueue = require('../lib/command-queue'); + commandQueue.queueOrEmitPlaylistUpdate(io.of('/device'), req.params.id, buildPlaylistPayload); + } + } catch (e) { /* silent — the DB is the source of truth, the push is best-effort */ } + + res.json({ success: true }); +}); + router.put('/:id', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; diff --git a/server/test/clear-device-playlist.test.js b/server/test/clear-device-playlist.test.js new file mode 100644 index 0000000..b03b82b --- /dev/null +++ b/server/test/clear-device-playlist.test.js @@ -0,0 +1,107 @@ +'use strict'; + +// "No playlist" was an option you could select that did nothing. +// +// The dashboard picker offered ``, and its change handler +// opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it +// sent no request, changed nothing, and raised no error. The guard was honest about why: there was +// no way to do it. PUT /devices/:id has never read playlist_id (it returns 200 and ignores it), and +// POST /playlists/:id/assign can only set one. +// +// Reported on #234 as "I also selected No playlist ... it still showed the same video". It did. +// +// The invariant: clearing a display's playlist actually clears it, and only the people allowed to +// change that display can do it. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-clearpl-')); +process.env.DATA_DIR = tmp; +process.env.JWT_SECRET = 'test-secret-clear-playlist'; + +const express = require('express'); +const { db } = require('../db/database'); +const { requireAuth, generateToken } = require('../middleware/auth'); + +// devices -> workspaces -> organizations -> users, FK-enforced, so seed the whole chain. +function seed(suffix) { + const u = 'u-' + suffix, o = 'o-' + suffix, ws = 'ws-' + suffix; + const dev = 'd-' + suffix, pl = 'p-' + suffix; + db.prepare("INSERT OR IGNORE INTO users (id, email, password_hash, role) VALUES (?, ?, 'x', 'user')") + .run(u, suffix + '@test.local'); + db.prepare('INSERT OR IGNORE INTO organizations (id, name, owner_user_id) VALUES (?, ?, ?)').run(o, 'org ' + suffix, u); + db.prepare('INSERT OR IGNORE INTO workspaces (id, organization_id, name) VALUES (?, ?, ?)').run(ws, o, 'ws ' + suffix); + // accessContext resolves through the MEMBERSHIP tables, not organizations.owner_user_id — + // seeding only the owner column gets a legitimate owner a 403 and looks like an authz bug. + db.prepare("INSERT OR IGNORE INTO organization_members (organization_id, user_id, role) VALUES (?, ?, 'org_owner')").run(o, u); + db.prepare("INSERT INTO playlists (id, name, workspace_id, user_id) VALUES (?, 'PL', ?, ?)").run(pl, ws, u); + db.prepare(`INSERT INTO devices (id, name, workspace_id, user_id, playlist_id, created_at, updated_at) + VALUES (?, 'Screen', ?, ?, ?, strftime('%s','now'), strftime('%s','now'))`).run(dev, ws, u, pl); + return { u, ws, dev, pl }; +} + +const mine = seed('mine'); +const theirs = seed('theirs'); + +const app = express(); +app.use(express.json()); +app.use('/api/devices', requireAuth, require('../routes/devices')); +const server = app.listen(0); + +const userRow = (id) => db.prepare('SELECT id, email, role FROM users WHERE id = ?').get(id); +const tokenFor = (u, ws) => generateToken(userRow(u), ws); + +async function del(deviceId, token) { + await new Promise(r => (server.listening ? r() : server.once('listening', r))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/devices/${deviceId}/playlist`, { + method: 'DELETE', + headers: token ? { Authorization: `Bearer ${token}` } : {}, + }); + return res.status; +} + +const playlistOf = (id) => db.prepare('SELECT playlist_id FROM devices WHERE id = ?').get(id).playlist_id; + +test('THE BUG: PUT /devices/:id silently ignores playlist_id, so it could not clear one', async () => { + // Pinned so nobody "fixes" the picker by pointing it back at PUT and re-creating the silence. + const src = fs.readFileSync(path.join(__dirname, '..', 'routes', 'devices.js'), 'utf8'); + const put = src.slice(src.indexOf("router.put('/:id'")); + const body = put.slice(0, put.indexOf('\nrouter.')); + assert.ok(!/playlist_id\s*[,=]/.test(body), 'PUT now touches playlist_id — update this test and the picker'); +}); + +test('THE FIX: clearing a playlist actually clears it', async () => { + assert.equal(playlistOf(mine.dev), mine.pl, 'precondition: a playlist is assigned'); + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null, 'the display must end up with no playlist'); +}); + +test('clearing an already-clear display is a harmless no-op', async () => { + // The button is in a dropdown a person can pick twice; it must not 404 or 500 on the second go. + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null); +}); + +test('someone else\'s display cannot be cleared', async () => { + const before = playlistOf(theirs.dev); + const status = await del(theirs.dev, tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); + assert.equal(playlistOf(theirs.dev), before, 'a refused call must not have changed anything'); +}); + +test('an unauthenticated caller cannot clear a playlist', async () => { + const before = playlistOf(theirs.dev); + assert.equal(await del(theirs.dev), 401); + assert.equal(playlistOf(theirs.dev), before); +}); + +test('a display that does not exist is refused, not invented', async () => { + const status = await del('no-such-device', tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); +}); + +test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} });