From 14367af5f18c82c80667401e13a2d1622631b18a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 20:53:21 -0500 Subject: [PATCH] Keep a workspace on schedules that outlive their device group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a device group converts its group schedules into per-device ones so the screens keep their programming. That INSERT omitted workspace_id, which is nullable with no default, so every converted row landed with workspace_id = NULL. A null workspace does not merely look untidy — it makes the row unreachable in three directions at once, and they compound into the worst possible combination: invisible the schedule list and the all-screens calendar both filter on workspace_id undeletable PUT and DELETE refuse a row with no workspace (403) still live services/scheduler.js has no workspace filter, so it keeps firing every 60 seconds "I deleted the group but the screens still switch content at 9am, and there is nothing in the calendar to remove." The only way out was direct database access. The conversion now carries the workspace, preferring the schedule's own and falling back to the group's so a legacy group schedule that itself predates workspace_id still converts into a reachable row. A boot migration repairs rows already orphaned in the field by recovering the workspace from the device each one targets; anything still unresolvable is left alone rather than guessed at. 4 tests: the converted row keeps its workspace, is visible to the query the list and calendar use, preserves the actual programming rather than just the ownership, and the repair recovers a row orphaned before this fix existed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/db/database.js | 8 ++ server/routes/device-groups.js | 16 ++- .../group-delete-schedule-conversion.test.js | 100 ++++++++++++++++++ 3 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 server/test/group-delete-schedule-conversion.test.js diff --git a/server/db/database.js b/server/db/database.js index ff84795..7bc4305 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -310,6 +310,14 @@ const migrations = [ // first may be pulled back to stable. Without it, publishing a beta would drag every existing // pre-release tester backwards, which is the harm the opt-in exists to prevent. "ALTER TABLE devices ADD COLUMN ota_channel_served TEXT", + // Repair for schedules orphaned by a group deletion before the conversion carried workspace_id. + // Such rows are invisible (list/calendar filter on workspace), undeletable (PUT/DELETE 403 on a + // null workspace) and still firing (the scheduler has no workspace filter) — so an operator + // cannot fix them from the dashboard at all. Recover the workspace from the device the schedule + // targets; anything still unresolvable is left alone rather than guessed at. + `UPDATE schedules SET workspace_id = (SELECT d.workspace_id FROM devices d WHERE d.id = schedules.device_id) + WHERE workspace_id IS NULL AND device_id IS NOT NULL + AND (SELECT d.workspace_id FROM devices d WHERE d.id = schedules.device_id) IS NOT NULL`, // #161: privilege tier reported by the player (0 unprivileged / 1 device-admin / 2 owner-or- // delegated-install) + whether a foreign device owner (MDM) manages it. Drives dashboard gating // of Tier-2 controls (reboot/kiosk/time) — shown only for owned panels. diff --git a/server/routes/device-groups.js b/server/routes/device-groups.js index 822f45b..a9a5b78 100644 --- a/server/routes/device-groups.js +++ b/server/routes/device-groups.js @@ -140,17 +140,27 @@ router.delete('/:id', requireGroupWrite, (req, res) => { let converted = 0; if (groupSchedules.length > 0 && members.length > 0) { + // workspace_id MUST be carried over. It is nullable with no default, so omitting it landed + // every converted schedule with workspace_id = NULL — and a null workspace does not merely + // look untidy, it makes the row unreachable in three directions at once: + // - the schedule list and the all-screens calendar filter on workspace_id: invisible + // - PUT and DELETE refuse a row with no workspace (403): undeletable + // - services/scheduler.js has NO workspace filter: it keeps firing every 60 seconds + // i.e. "I deleted the group but the screens still switch at 9am and there is nothing in the + // calendar to remove". The only way out was direct database access. const insert = db.prepare(` - INSERT INTO schedules (id, user_id, device_id, group_id, zone_id, content_id, + INSERT INTO schedules (id, user_id, workspace_id, device_id, group_id, zone_id, content_id, widget_id, layout_id, playlist_id, title, start_time, end_time, timezone, recurrence, recurrence_end, priority, enabled, color, created_at, updated_at) - VALUES (?, ?, ?, NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `); for (const schedule of groupSchedules) { for (const member of members) { insert.run( - uuidv4(), schedule.user_id, member.device_id, + // Prefer the schedule's own workspace, falling back to the group's, so a legacy + // group schedule predating workspace_id still converts into a reachable row. + uuidv4(), schedule.user_id, schedule.workspace_id || req.group.workspace_id, member.device_id, schedule.zone_id, schedule.content_id, schedule.widget_id, schedule.layout_id, schedule.playlist_id, schedule.title, schedule.start_time, schedule.end_time, schedule.timezone, diff --git a/server/test/group-delete-schedule-conversion.test.js b/server/test/group-delete-schedule-conversion.test.js new file mode 100644 index 0000000..7d8f70f --- /dev/null +++ b/server/test/group-delete-schedule-conversion.test.js @@ -0,0 +1,100 @@ +'use strict'; + +// Deleting a device group converts its group schedules into per-device ones so the screens keep +// their programming. That INSERT omitted workspace_id, which is nullable with no default — so every +// converted row landed with workspace_id = NULL. +// +// A null workspace does not merely look untidy. It makes the row unreachable in three directions at +// once, and they compound into the worst possible combination: +// +// invisible — the schedule list and the all-screens calendar both filter on workspace_id +// undeletable — PUT and DELETE refuse a row with no workspace (403) +// still live — services/scheduler.js has NO workspace filter, so it keeps firing every 60s +// +// i.e. "I deleted the group but the screens still switch content at 9am, and there is nothing in +// the calendar to remove." The only way out was direct database access. +// +// The invariant: a schedule that survives a group deletion stays owned by a workspace, so it can be +// seen and removed by the person whose screens it controls. + +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-groupdel-')); +process.env.DATA_DIR = tmp; +process.env.JWT_SECRET = 'test-secret-group-delete'; + +const express = require('express'); +const { db } = require('../db/database'); +const { requireAuth, generateToken } = require('../middleware/auth'); +const { resolveTenancy } = require('../lib/tenancy'); + +const O = 'o-gd', WS = 'ws-gd', U = 'u-gd', G = 'g-gd', DEV = 'dev-gd'; +db.prepare("INSERT OR IGNORE INTO users (id,email,password_hash,role) VALUES (?,?, 'x','user')").run(U, 'gd@t.local'); +db.prepare('INSERT OR IGNORE INTO organizations (id,name,owner_user_id) VALUES (?,?,?)').run(O, 'Org', U); +db.prepare('INSERT OR IGNORE INTO workspaces (id,organization_id,name) VALUES (?,?,?)').run(WS, O, 'WS'); +db.prepare("INSERT OR IGNORE INTO organization_members (organization_id,user_id,role) VALUES (?,?, 'org_owner')").run(O, U); +db.prepare(`INSERT OR IGNORE INTO devices (id,name,workspace_id,created_at,updated_at) + VALUES (?,?,?,strftime('%s','now'),strftime('%s','now'))`).run(DEV, 'Screen', WS); +db.prepare('INSERT OR IGNORE INTO device_groups (id,name,workspace_id,user_id) VALUES (?,?,?,?)').run(G, 'Group', WS, U); +db.prepare('INSERT OR IGNORE INTO device_group_members (group_id,device_id) VALUES (?,?)').run(G, DEV); +db.prepare(`INSERT OR IGNORE INTO schedules (id,user_id,workspace_id,group_id,title,start_time,end_time,timezone,priority,enabled) + VALUES ('sg-1',?,?,?, 'Morning menu','09:00','17:00','UTC',1,1)`).run(U, WS, G); + +const app = express(); +app.use(express.json()); +app.set('io', null); +app.use('/api/groups', requireAuth, resolveTenancy, require('../routes/device-groups')); +const server = app.listen(0); +const token = generateToken(db.prepare('SELECT id,email,role FROM users WHERE id = ?').get(U), WS); + +async function deleteGroup() { + await new Promise(r => (server.listening ? r() : server.once('listening', r))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/groups/${G}`, { + method: 'DELETE', headers: { Authorization: `Bearer ${token}` }, + }); + return { status: res.status, body: await res.json().catch(() => null) }; +} + +test('THE BUG: a schedule that survives a group deletion keeps its workspace', async () => { + const { status, body } = await deleteGroup(); + assert.equal(status, 200); + assert.ok(body.schedules_converted >= 1, 'the schedule should have been converted, not dropped'); + + const converted = db.prepare('SELECT * FROM schedules WHERE device_id = ? AND group_id IS NULL').all(DEV); + assert.equal(converted.length, 1); + assert.equal(converted[0].workspace_id, WS, 'a null workspace makes the row invisible AND undeletable AND live'); +}); + +test('the converted schedule is therefore visible to the workspace it controls', () => { + // This is the query the schedule list and the calendar both use. + const visible = db.prepare('SELECT COUNT(*) n FROM schedules WHERE workspace_id = ? AND device_id = ?').get(WS, DEV).n; + assert.equal(visible, 1); +}); + +test('the programming itself is preserved, not just the ownership', () => { + const s = db.prepare('SELECT * FROM schedules WHERE device_id = ? AND group_id IS NULL').get(DEV); + assert.equal(s.title, 'Morning menu'); + assert.equal(s.start_time, '09:00'); + assert.equal(s.end_time, '17:00'); + assert.equal(s.enabled, 1); +}); + +test('the repair recovers rows orphaned before this fix existed', () => { + // Simulate the old behaviour, then run the same statement the boot migration runs. + db.prepare(`INSERT INTO schedules (id,user_id,workspace_id,device_id,title,start_time,end_time,timezone,priority,enabled) + VALUES ('legacy-orphan',?,NULL,?, 'Orphan','06:00','08:00','UTC',1,1)`).run(U, DEV); + assert.equal(db.prepare("SELECT workspace_id FROM schedules WHERE id='legacy-orphan'").get().workspace_id, null); + + db.prepare(`UPDATE schedules SET workspace_id = (SELECT d.workspace_id FROM devices d WHERE d.id = schedules.device_id) + WHERE workspace_id IS NULL AND device_id IS NOT NULL + AND (SELECT d.workspace_id FROM devices d WHERE d.id = schedules.device_id) IS NOT NULL`).run(); + + assert.equal(db.prepare("SELECT workspace_id FROM schedules WHERE id='legacy-orphan'").get().workspace_id, WS, + 'an operator must be able to see and delete it from the dashboard'); +}); + +test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} });