diff --git a/CHANGELOG.md b/CHANGELOG.md index cfb7438b..a7dc5a00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,13 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Under the hood +- **A new system can't take a name already in use, and an installed system can always be + updated.** Creating a system with the name of another is refused, as renaming is. Installing a + newer file of a system already here now always offers to update it. If it was changed here + since it was installed, updating asks first, since those changes are replaced, rather than + being refused outright. An updated system keeps the name it has here, so one the GM renamed + stays renamed, and browsers fetch the update straight away. + - **The system list counts each system's problems, and an install says its name first.** The list of a GM's systems now says how many problems each draft has, so SYSTEMS.EXE can mark them without opening each one. Looking at a file before installing it now says the name it will go in under diff --git a/backend/__tests__/system_builder_citysys.test.js b/backend/__tests__/system_builder_citysys.test.js index dd4a289b..7cc29cb4 100644 --- a/backend/__tests__/system_builder_citysys.test.js +++ b/backend/__tests__/system_builder_citysys.test.js @@ -45,7 +45,7 @@ const create = async (definition = VAULT) => (await request(app).post('/api/syst const publish = (id) => request(app).post(`/api/systems/${id}/publish`).set(gm); const exported = async (id) => (await request(app).get(`/api/systems/${id}/export`).set(gm)).text; const preview = (file) => request(app).post('/api/systems/install/preview').set(gm).send({ file }); -const install = (file, mode) => request(app).post('/api/systems/install').set(gm).send({ file, mode }); +const install = (file, mode, extra = {}) => request(app).post('/api/systems/install').set(gm).send({ file, mode, ...extra }); const rows = () => new Promise((resolve) => db.all('SELECT id, name, version, origin, deleted_at FROM custom_systems ORDER BY created_at, id', (e, r) => resolve(r))); const fileOf = (definition, origin = 'org_vault', version = 3) => JSON.stringify(citysys.buildFile({ definition, version, origin })); @@ -182,25 +182,54 @@ describe('installing', () => { expect(runtime.render(id).name).toBe('Vault Knights'); }); - it('as an update: refused over changes made here, over problems, and when not installed', async () => { + it('as an update: refused over problems, and when not installed', async () => { expect((await install(fileOf(VAULT), 'update')).status).toBe(404); const id = (await install(fileOf(VAULT), 'new')).body.id; const broken = { ...VAULT, derived: [{ id: 'a', formula: '@a' }] }; - expect((await install(fileOf(broken), 'update')).status).toBe(409); + const res = await install(fileOf(broken), 'update', { replaceChanges: true }); + expect(res.status).toBe(409); + expect(res.body.error).toBe('The file has problems; install it as a second copy to fix them'); + expect(JSON.parse((await get(db, 'SELECT published FROM custom_systems WHERE id = ?', [id])).published)).toEqual(VAULT); + }); + + it('as an update over changes made here: only once the GM has been warned, and then it replaces them', async () => { + const id = (await install(fileOf(VAULT), 'new')).body.id; await request(app).put(`/api/systems/${id}/draft`).set(gm).send({ definition: { ...VAULT, description: 'Mine now' } }); expect((await preview(fileOf(VAULT))).body.installed[0].edited).toBe(true); - const res = await install(fileOf({ ...VAULT, description: 'Theirs' }), 'update'); - expect(res.status).toBe(409); - expect(res.body.error).toBe('This system has been changed here since it was installed. Keep both instead.'); + const theirs = fileOf({ ...VAULT, description: 'Theirs' }, 'org_vault', 4); + for (const extra of [{}, { replaceChanges: 'yes' }]) { + const res = await install(theirs, 'update', extra); + expect(res.status).toBe(409); + expect(res.body).toEqual({ error: 'This system has been changed here since it was installed. Replacing it loses those changes.', changed: true }); + } expect(JSON.parse((await get(db, 'SELECT draft FROM custom_systems WHERE id = ?', [id])).draft).description).toBe('Mine now'); + const res = await install(theirs, 'update', { replaceChanges: true }); + expect(res.body).toMatchObject({ id, name: 'Vault Knights', published: true }); + const row = await get(db, 'SELECT draft, published, version FROM custom_systems WHERE id = ?', [id]); + expect(JSON.parse(row.draft).description).toBe('Theirs'); + expect(row.version).toBe(2); + // Unchanged since this install, so the next update needs no warning. + expect((await preview(theirs)).body.installed[0].edited).toBe(false); }); - it('never overwrites a system made here, even from its own file', async () => { + it('treats a system made here as changed, even from its own file', async () => { const id = await create(); await publish(id); const file = await exported(id); expect((await preview(file)).body.installed).toEqual([{ id, name: 'Vault Knights', version: 1, edited: true }]); - expect((await install(file, 'update')).status).toBe(409); + expect((await install(file, 'update')).body.changed).toBe(true); + }); + + it('keeps the name the GM chose through an update', async () => { + const id = (await install(fileOf(VAULT), 'new')).body.id; + await request(app).put(`/api/systems/${id}/name`).set(gm).send({ name: 'Vault Knights (our table)' }); + expect((await preview(fileOf(VAULT))).body).toMatchObject({ installed: [{ edited: true }], installsAs: { update: 'Vault Knights (our table)' } }); + const res = await install(fileOf({ ...VAULT, description: 'v2' }, 'org_vault', 4), 'update', { replaceChanges: true }); + expect(res.body).toMatchObject({ id, name: 'Vault Knights (our table)' }); + const row = await get(db, 'SELECT name, draft, published FROM custom_systems WHERE id = ?', [id]); + expect([row.name, JSON.parse(row.draft).name, JSON.parse(row.published).name]).toEqual(Array(3).fill('Vault Knights (our table)')); + expect(JSON.parse(row.published).description).toBe('v2'); + expect(runtime.render(id).name).toBe('Vault Knights (our table)'); }); it('keeping both: a second copy with its own id and origin, so the two never collide', async () => { @@ -266,8 +295,8 @@ describe('installing under a name already here', () => { it('keeps an updated system\'s own name, and its copy name while the other is still here', async () => { const id = (await install(fileOf(VAULT), 'new')).body.id; expect((await install(fileOf({ ...VAULT, description: 'v2' }, 'org_vault', 4), 'update')).body).toMatchObject({ id, name: 'Vault Knights' }); - const other = await create(); await request(app).delete(`/api/systems/${id}`).set(gm); + const other = await create(); const back = await install(fileOf(VAULT), 'new'); expect(back.body).toMatchObject({ id, restored: true, name: 'Vault Knights copy' }); expect((await install(fileOf({ ...VAULT, description: 'v3' }, 'org_vault', 5), 'update')).body).toMatchObject({ id, name: 'Vault Knights copy' }); @@ -286,13 +315,13 @@ describe('installing under a name already here', () => { const asNew = (await preview(fileOf(VAULT))).body.installsAs; expect(asNew).toEqual({ new: 'Vault Knights copy', update: null, keep_both: 'Vault Knights copy' }); const id = (await install(fileOf(VAULT), 'new')).body.id; - // Installed: an update keeps its name while the other is here; keeping both counts on. + // Installed: an update keeps the name it has here; keeping both counts on. expect((await preview(fileOf(VAULT))).body.installsAs).toEqual({ new: null, update: 'Vault Knights copy', keep_both: 'Vault Knights copy 02' }); expect((await install(fileOf(VAULT), 'keep_both')).body.name).toBe('Vault Knights copy 02'); - // With the other gone, an update takes the file's own name back. + // Even with the other gone: its name is the one it has here, not the file's. await request(app).delete(`/api/systems/${mine}`).set(gm); - expect((await preview(fileOf(VAULT))).body.installsAs.update).toBe('Vault Knights'); - expect((await install(fileOf({ ...VAULT, description: 'v2' }, 'org_vault', 4), 'update')).body).toMatchObject({ id, name: 'Vault Knights' }); + expect((await preview(fileOf(VAULT))).body.installsAs.update).toBe('Vault Knights copy'); + expect((await install(fileOf({ ...VAULT, description: 'v2' }, 'org_vault', 4), 'update')).body).toMatchObject({ id, name: 'Vault Knights copy' }); }); it('names one brought back from deletion as it will be', async () => { diff --git a/backend/__tests__/system_builder_rename.test.js b/backend/__tests__/system_builder_rename.test.js index 1d4008e3..7b6c6701 100644 --- a/backend/__tests__/system_builder_rename.test.js +++ b/backend/__tests__/system_builder_rename.test.js @@ -118,6 +118,18 @@ describe('renaming a system', () => { expect(emitted).toEqual([]); }); + it('keeps through an update from the system\'s file, and every browser fetches the update', async () => { + const citysys = require_('../systemBuilder/citysys'); + const file = (description) => JSON.stringify(citysys.buildFile({ definition: { ...PUBLISHED, description }, version: 4, origin: 'org_hearth' })); + const { id } = (await request(app).post('/api/systems/install').set(bearer(GM)).send({ file: file('v1'), mode: 'keep_both' })).body; + await run(db, 'UPDATE custom_systems SET origin = ? WHERE id = ?', ['org_hearth', id]); + expect((await rename('Hearth, ours', id)).status).toBe(200); + emitted = []; + const res = await request(app).post('/api/systems/install').set(bearer(GM)).send({ file: file('v2'), mode: 'update', replaceChanges: true }); + expect(res.body).toMatchObject({ id, name: 'Hearth, ours' }); + expect(emitted).toEqual([{ event: 'customSystemChanged', data: { id } }]); + }); + it('is the main admin\'s alone', async () => { expect((await rename('Hearthfire', HEARTH, EDITOR)).status).toBe(403); expect((await rename('Hearthfire', HEARTH, PLAYER)).status).toBe(403); diff --git a/backend/__tests__/system_builder_store.test.js b/backend/__tests__/system_builder_store.test.js index 8f14ff6b..27d9d937 100644 --- a/backend/__tests__/system_builder_store.test.js +++ b/backend/__tests__/system_builder_store.test.js @@ -141,6 +141,19 @@ describe('the systems routes', () => { expect(only).toMatchObject({ id: res.body.id, name: 'Vault Knights', version: 0, published: false, unpublishedChanges: true, installed: false, problemCount: 0 }); }); + it('refuses a name another system has, whatever its capitals, so the GM can pick another', async () => { + const { id } = (await create({ name: 'Hearth' })).body; + for (const body of [{ name: 'hearth' }, { name: ' HEARTH ' }, { definition: { format: 1, name: 'Hearth' } }]) { + const res = await create(body); + expect(res.status, JSON.stringify(body)).toBe(409); + expect(res.body, JSON.stringify(body)).toEqual({ error: 'Another system is already called Hearth.' }); + } + expect((await list()).map((s) => s.id)).toEqual([id]); + // A deleted system's name is free again. + await request(app).delete(`/api/systems/${id}`).set(bearer(GM)); + expect((await create({ name: 'Hearth' })).status).toBe(200); + }); + it('counts a draft that cannot be read at all as one problem, as publishing would', async () => { const { id } = (await create({ name: 'Broken' })).body; await run(db, 'UPDATE custom_systems SET draft = ? WHERE id = ?', ['not json', id]); diff --git a/backend/routes/systems.js b/backend/routes/systems.js index 51e94668..4d72901a 100644 --- a/backend/routes/systems.js +++ b/backend/routes/systems.js @@ -139,15 +139,21 @@ module.exports = (db, io = null) => { store.previewInstall(db, req.body && req.body.file, (err, preview) => answer(res, err, preview)); }); + // `replaceChanges: true` only after the GM was warned that changes made here would go. router.post('/install', gm, (req, res) => { - const { file, mode } = req.body || {}; - store.installSystem(db, file, mode, (err, installed) => { + const { file, mode, replaceChanges } = req.body || {}; + store.installSystem(db, { text: file, mode, replaceChanges }, (err, installed) => { if (err) return res.status(err.status || 500).json({ error: err.status ? err.message : 'Could not reach the systems store', ...(err.problems ? { problems: err.problems } : {}), ...(err.installed ? { installed: err.installed } : {}), + ...(err.changed ? { changed: true } : {}), + }); + // An update or a system brought back may be one a browser already has: it fetches it again. + runtime.refresh(db, installed.id, () => { + if (io) io.emit('customSystemChanged', { id: installed.id }); + answer(res, null, installed); }); - runtime.refresh(db, installed.id, () => answer(res, null, installed)); }); }); diff --git a/backend/systemBuilder/store.js b/backend/systemBuilder/store.js index 661813b5..366ea20b 100644 --- a/backend/systemBuilder/store.js +++ b/backend/systemBuilder/store.js @@ -88,22 +88,38 @@ const getSystem = (db, id, cb) => { }; /** - * Create a system from a name, or from a whole definition (an import, a copy). Refused only - * when the definition cannot be stored at all; its ordinary problems come back with it. + * The refusal when another of `rows` (the systems not deleted) already has `name`, else null. + * CREATE and RENAME refuse it so the GM can pick another (decided with the user, 2026-10-06); + * a system's own name, `self`, is not in its way. + */ +const nameTaken = (rows, name, self = null) => { + const other = rows.find((r) => r.id !== self && sameName(r.name, name)); + return other ? fail(409, `Another system is already called ${other.name}.`) : null; +}; + +/** + * Create a system from a name, or from a whole definition (an import, a copy). Refused when the + * definition cannot be stored at all, or another system has its name; its ordinary problems + * come back with it. */ const createSystem = (db, { name, definition } = {}, cb) => { const def = definition === undefined ? blankDefinition(name) : definition; const checked = checkDefinition(def); if (checked.fatal) return cb(fail(400, checked.fatal)); if (typeof def.name !== 'string' || !def.name.trim()) return cb(fail(400, 'A system needs a name')); - const id = newId(); - // A system made here is its own origin: shared as a file, every copy keeps it. - db.run( - `INSERT INTO custom_systems (id, name, draft, version, origin, created_at, updated_at) - VALUES (?, ?, ?, 0, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)`, - [id, def.name.trim(), JSON.stringify(def), id], - (err) => (err ? cb(err) : cb(null, { id, problems: checked.problems })), - ); + db.all('SELECT id, name FROM custom_systems WHERE deleted_at IS NULL', [], (err, rows) => { + if (err) return cb(err); + const taken = nameTaken(rows, def.name); + if (taken) return cb(taken); + const id = newId(); + // A system made here is its own origin: shared as a file, every copy keeps it. + db.run( + `INSERT INTO custom_systems (id, name, draft, version, origin, created_at, updated_at) + VALUES (?, ?, ?, 0, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)`, + [id, def.name.trim(), JSON.stringify(def), id], + (err2) => (err2 ? cb(err2) : cb(null, { id, problems: checked.problems })), + ); + }); }; /** Replace a system's draft. Stored even with problems, which come back to show. */ @@ -206,8 +222,8 @@ const renameSystem = (db, id, wanted, cb) => { if (err) return cb(err); const row = rows.find((r) => r.id === id); if (!row) return cb(fail(404, 'No such system')); - const other = rows.find((r) => r.id !== id && sameName(r.name, name)); - if (other) return cb(fail(409, `Another system is already called ${other.name}.`)); + const taken = nameTaken(rows, name, id); + if (taken) return cb(taken); /** One copy with the new name; a copy that isn't there stays absent. */ const withName = (text) => { const definition = parse(text); @@ -297,16 +313,17 @@ const matchesFor = (db, origin, cb) => { /** * The name a file's system goes in under, for each way of installing it: its own, or * " copy"... when another system has it (decided with the user, 2026-10-06). An update - * replaces the copy here, so that copy's name is not in its way. Null where the way does not - * apply: `new` once it is installed, `update` until it is. + * keeps the name the system has here, whether the GM chose it or it came in as a copy: a name + * isn't a rule (same day). Null where the way does not apply: `new` once it is installed, + * `update` until it is. */ const installNames = (name, found) => { const here = found.installed[0]; - const taken = (rows) => rows.map((r) => r.name); + const taken = found.inUse.map((r) => r.name); return { - new: here ? null : uniqueName(name, taken(found.inUse)), - update: here ? uniqueName(name, taken(found.inUse.filter((r) => r.id !== here.id))) : null, - keep_both: uniqueName(name, taken(found.inUse)), + new: here ? null : uniqueName(name, taken), + update: here ? here.name : null, + keep_both: uniqueName(name, taken), }; }; @@ -342,14 +359,16 @@ const previewInstall = (db, text, cb) => { /** * Install a file. * new a system not here yet; one deleted here comes back under its old id - * update replace the copy already here, when it has not been changed since installing + * update replace the copy already here, keeping its name. One changed here since it was + * installed is replaced only with `replaceChanges`, the GM having been warned that + * those changes go (decided with the user, 2026-10-06; until then it was refused) * keep_both a second copy beside it, with a new id and a new origin of its own * Published straight away when the file has no problems; otherwise kept as a draft to fix. * Never a merge, and never refused over a name: one another system already has becomes * " copy", "copy 02"... (names.js; decided with the user, 2026-10-06), in the row and the * definition alike. Resolves { id, name, published, restored?, problems }. */ -const installSystem = (db, text, mode, cb) => { +const installSystem = (db, { text, mode, replaceChanges = false } = {}, cb) => { if (!['new', 'update', 'keep_both'].includes(mode)) return cb(fail(400, 'Install as new, update or keep both')); const read = citysys.readFile(text); if (read.fatal) return cb(fail(400, read.fatal)); @@ -388,8 +407,10 @@ const installSystem = (db, text, mode, cb) => { } if (mode === 'update') { if (!here) return cb(fail(404, 'This system is not installed here; install it as new')); - if (editedSinceInstall(here)) return cb(fail(409, 'This system has been changed here since it was installed. Keep both instead.')); if (!publishable) return cb(fail(409, 'The file has problems; install it as a second copy to fix them', { problems: read.problems })); + if (editedSinceInstall(here) && replaceChanges !== true) { + return cb(fail(409, 'This system has been changed here since it was installed. Replacing it loses those changes.', { changed: true })); + } return replace(here, named(names.update)); } if (here) return cb(fail(409, 'Already installed. Update it or keep both.', { installed: found.installed.map((r) => ({ id: r.id, name: r.name })) }));