diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ffdface..cfb7438b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Under the hood +- **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 + ("Vault Knights copy" when that name is taken), for each way of installing it, and how many + currencies it has. + - **A system can be duplicated, and an install is never stopped by a name.** A duplicate is a new, unpublished system made from the original's draft, unpublished changes included, and named "Hearth copy", "Hearth copy 02" and so on. Installing a file whose system has a name already diff --git a/backend/__tests__/system_builder_citysys.test.js b/backend/__tests__/system_builder_citysys.test.js index acc4303e..dd4a289b 100644 --- a/backend/__tests__/system_builder_citysys.test.js +++ b/backend/__tests__/system_builder_citysys.test.js @@ -130,9 +130,11 @@ describe('previewing an install', () => { expect(res.body).toMatchObject({ name: 'Vault Knights', manifest: { author: 'Cody', version: 3, origin: 'org_vault' }, - inside: { words: 1, derived: 1, healthModel: 'wounds' }, + inside: { words: 1, derived: 1, currencies: 0, healthModel: 'wounds' }, problems: [], installed: [], restores: null, + installsAs: { new: 'Vault Knights', update: null, keep_both: 'Vault Knights' }, }); + expect((await preview(fileOf({ ...VAULT, currencies: [{ id: 'gold', name: 'Gold' }, { id: 'favor', name: 'Favor' }] }))).body.inside.currencies).toBe(2); expect(await rows()).toEqual(before); }); @@ -279,6 +281,28 @@ describe('installing under a name already here', () => { expect(await nameOf(res.body.id)).toMatchObject({ name: 'Vault Knights copy', draft: 'Vault Knights copy', published: null }); }); + it('says beforehand the name each way of installing would give, and installing keeps to it', async () => { + const mine = await create(); + 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. + 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. + 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' }); + }); + + it('names one brought back from deletion as it will be', async () => { + const id = (await install(fileOf(VAULT), 'new')).body.id; + await request(app).delete(`/api/systems/${id}`).set(gm); + await create(); + expect((await preview(fileOf(VAULT))).body).toMatchObject({ restores: { id }, installsAs: { new: 'Vault Knights copy' } }); + expect((await install(fileOf(VAULT), 'new')).body).toMatchObject({ id, restored: true, name: 'Vault Knights copy' }); + }); + it('ignores a deleted system\'s name', async () => { const mine = await create(); await request(app).delete(`/api/systems/${mine}`).set(gm); diff --git a/backend/__tests__/system_builder_store.test.js b/backend/__tests__/system_builder_store.test.js index 2fa203fa..8f14ff6b 100644 --- a/backend/__tests__/system_builder_store.test.js +++ b/backend/__tests__/system_builder_store.test.js @@ -138,7 +138,14 @@ describe('the systems routes', () => { expect(res.body.id).toMatch(/^sys_[0-9a-f]{16}$/); expect(res.body.problems).toEqual([]); const [only] = await list(); - expect(only).toMatchObject({ id: res.body.id, name: 'Vault Knights', version: 0, published: false, unpublishedChanges: true, installed: false }); + expect(only).toMatchObject({ id: res.body.id, name: 'Vault Knights', version: 0, published: false, unpublishedChanges: true, installed: false, problemCount: 0 }); + }); + + 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]); + expect((await list())[0].problemCount).toBe(1); + expect((await request(app).get(`/api/systems/${id}`).set(bearer(GM))).body.problems).toEqual([{ where: 'definition', message: expect.any(String) }]); }); it('never collides with a built-in system id', () => { @@ -166,6 +173,8 @@ describe('the systems routes', () => { const saved = await request(app).put(`/api/systems/${id}/draft`).set(bearer(GM)).send({ definition: broken }); expect(saved.status).toBe(200); expect(saved.body.problems).toEqual([{ where: 'derived hp', message: 'Depends on itself: hp → hp' }]); + // The list counts them, so SYSTEMS.EXE can badge it without fetching the system. + expect((await list())[0].problemCount).toBe(1); const refused = await request(app).post(`/api/systems/${id}/publish`).set(bearer(GM)); expect(refused.status).toBe(409); @@ -174,6 +183,7 @@ describe('the systems routes', () => { const fixed = { format: 1, name: 'Draft', derived: [{ id: 'hp', formula: '@con + 10' }] }; await request(app).put(`/api/systems/${id}/draft`).set(bearer(GM)).send({ definition: fixed }); + expect((await list())[0].problemCount).toBe(0); const published = await request(app).post(`/api/systems/${id}/publish`).set(bearer(GM)); expect(published.status).toBe(200); expect(published.body).toEqual({ version: 1 }); diff --git a/backend/systemBuilder/citysys.js b/backend/systemBuilder/citysys.js index 3e18ebfe..f6877bf6 100644 --- a/backend/systemBuilder/citysys.js +++ b/backend/systemBuilder/citysys.js @@ -100,6 +100,7 @@ const summarize = (definition) => { return { words: isPlainObject(definition.words) ? Object.keys(definition.words).length : 0, partsOff: isPlainObject(definition.parts) ? Object.values(definition.parts).filter((p) => isPlainObject(p) && p.on === false).length : 0, + currencies: count(definition.currencies), derived: count(definition.derived), lookups: isPlainObject(definition.lookups) ? Object.keys(definition.lookups).length : 0, sheetFields: fields, diff --git a/backend/systemBuilder/store.js b/backend/systemBuilder/store.js index 0cef346a..661813b5 100644 --- a/backend/systemBuilder/store.js +++ b/backend/systemBuilder/store.js @@ -33,7 +33,16 @@ const parse = (text) => { /** A stored error: `status` is what a route answers with. */ const fail = (status, message, extra) => Object.assign(new Error(message), { status, ...(extra || {}) }); -/** Every system, newest change first, without their definitions. */ +/** A draft's problems, as publishing would list them. */ +const problemsOf = (draft) => { + const checked = checkDefinition(draft); + return checked.fatal ? [{ where: 'definition', message: checked.fatal }] : checked.problems; +}; + +/** + * Every system, newest change first, without their definitions. Each says how many problems its + * draft has, so SYSTEMS.EXE can badge them without fetching every system. + */ const listSystems = (db, cb) => { db.all( `SELECT id, name, version, draft, published, source_hash, updated_at, published_at @@ -52,6 +61,7 @@ const listSystems = (db, cb) => { unpublishedChanges: r.published == null || r.draft !== r.published, // Installed from a file, rather than made here: only an install records the file's hash. installed: r.source_hash != null, + problemCount: problemsOf(parse(r.draft)).length, }))); }, ); @@ -64,7 +74,6 @@ const getSystem = (db, id, cb) => { if (err) return cb(err); if (!row) return cb(fail(404, 'No such system')); const draft = parse(row.draft); - const checked = checkDefinition(draft); cb(null, { id: row.id, name: row.name, @@ -73,7 +82,7 @@ const getSystem = (db, id, cb) => { publishedAt: row.published_at, draft, published: parse(row.published), - problems: checked.fatal ? [{ where: 'definition', message: checked.fatal }] : checked.problems, + problems: problemsOf(draft), }); }); }; @@ -262,7 +271,10 @@ const exportSystem = (db, id, cb) => { const editedSinceInstall = (row) => citysys.hashOf(row.draft) !== row.source_hash || (row.published != null && citysys.hashOf(row.published) !== row.source_hash); -/** The systems here with a file's origin: the ones in use, and the most recently deleted. */ +/** + * The systems here with a file's origin: the ones in use, and the most recently deleted; and + * every system in use, whose names a new one must not take. + */ const matchesFor = (db, origin, cb) => { db.all( `SELECT id, name, version, draft, published, source_hash, deleted_at FROM custom_systems @@ -270,17 +282,38 @@ const matchesFor = (db, origin, cb) => { [origin], (err, rows) => { if (err) return cb(err); - cb(null, { - installed: rows.filter((r) => r.deleted_at == null), - deleted: rows.find((r) => r.deleted_at != null) || null, + db.all('SELECT id, name FROM custom_systems WHERE deleted_at IS NULL', [], (err2, inUse) => { + if (err2) return cb(err2); + cb(null, { + installed: rows.filter((r) => r.deleted_at == null), + deleted: rows.find((r) => r.deleted_at != null) || null, + inUse, + }); }); }, ); }; +/** + * 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. + */ +const installNames = (name, found) => { + const here = found.installed[0]; + const taken = (rows) => rows.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)), + }; +}; + /** * What installing a file would do, changing nothing: its cover, what is inside, its problems, - * the copies already here, and whether it would bring a deleted system back. + * the copies already here, whether it would bring a deleted system back, and the name it would + * go in under for each way of installing it (installNames). */ const previewInstall = (db, text, cb) => { const read = citysys.readFile(text); @@ -301,6 +334,7 @@ const previewInstall = (db, text, cb) => { name: found.deleted.name, replacesChanges: found.deleted.draft !== JSON.stringify(definition) && editedSinceInstall(found.deleted), } : null, + installsAs: installNames(definition.name, found), }); }); }; @@ -322,9 +356,8 @@ const installSystem = (db, text, mode, cb) => { const { manifest, definition } = read.file; const publishable = read.problems.length === 0; - /** The file's definition as stored, under a name none of `others` has. */ - const named = (others) => { - const name = uniqueName(definition.name, others.map((r) => r.name)); + /** The file's definition as stored, under the name this way of installing gives it. */ + const named = (name) => { const stored = JSON.stringify({ ...definition, name }); return { name, stored, hash: citysys.hashOf(stored) }; }; @@ -347,24 +380,21 @@ const installSystem = (db, text, mode, cb) => { matchesFor(db, manifest.origin, (err, found) => { if (err) return cb(err); - db.all('SELECT id, name FROM custom_systems WHERE deleted_at IS NULL', [], (err2, systems) => { - if (err2) return cb(err2); - const here = found.installed[0]; - if (mode === 'keep_both') { - const id = newId(); - return insert(id, id, named(systems)); - } - 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 })); - // Its own name is not in the way of itself. - return replace(here, named(systems.filter((s) => s.id !== here.id))); - } - if (here) return cb(fail(409, 'Already installed. Update it or keep both.', { installed: found.installed.map((r) => ({ id: r.id, name: r.name })) })); - if (found.deleted) return replace(found.deleted, named(systems), { restored: true }); - return insert(newId(), manifest.origin, named(systems)); - }); + const here = found.installed[0]; + const names = installNames(definition.name, found); + if (mode === 'keep_both') { + const id = newId(); + return insert(id, id, named(names.keep_both)); + } + 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 })); + 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 })) })); + if (found.deleted) return replace(found.deleted, named(names.new), { restored: true }); + return insert(newId(), manifest.origin, named(names.new)); }); };