Land on the forecast somebody chose, not the newest one
App.jsx fell back to versions[0] when localStorage held no selection, and the version list is ordered created_at DESC -- so a fresh browser landed on whatever version was created last. That is a scratch scenario as readily as the plan, and sales accounts, which never pick a version themselves, had no way back to the one they were meant to read. pf.version.is_default names the landing version, at most one per source by partial unique index, the way pf.layout.is_default already works. Setting it is admin-only and enforced in the route: it decides where everyone else lands, so it follows the pf.log rule rather than being the version's own setting like its name. A stored selection still wins -- the default is the fallback, not an override, so nobody building scenarios is thrown back to the plan on every load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
41e76978f4
commit
91c53ac6d8
28
CLAUDE.md
28
CLAUDE.md
@ -371,6 +371,34 @@ and `last_used` still come back from the route; nothing reads them.
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
## Where a browser lands
|
||||||
|
|
||||||
|
`App.jsx` restores `pf_sourceId` / `pf_versionId` from `localStorage` and
|
||||||
|
re-checks both against the live list. What it falls back to when there is
|
||||||
|
nothing stored — a new account, a new machine, a cleared browser — is
|
||||||
|
**`pf.version.is_default`**, one per source by partial unique index
|
||||||
|
(`version_one_default`), the way `pf.layout.is_default` works.
|
||||||
|
|
||||||
|
It used to fall back to `versions[0]`, and the list is ordered
|
||||||
|
`created_at DESC`, so that meant *the newest version* — whatever somebody
|
||||||
|
created last, a scratch copy as readily as the plan. Accounts that never pick a
|
||||||
|
version themselves, which is every sales account, therefore followed the
|
||||||
|
forecasting work around.
|
||||||
|
|
||||||
|
**Admin only**, enforced in `PUT /api/versions/:id`: it decides where everyone
|
||||||
|
else lands, so it follows the `pf.log` rule rather than being the version's own
|
||||||
|
setting like its name. The checkbox on the Baseline page's **This forecast**
|
||||||
|
panel is greyed out for other accounts rather than offering a click that
|
||||||
|
answers 403. The `StatusBar` picker marks the default `(default)`.
|
||||||
|
|
||||||
|
**A stored selection still wins.** The default is the fallback, not an
|
||||||
|
override — someone who has been working in another version stays there. The
|
||||||
|
corollary is that an account which already landed in the wrong scenario keeps
|
||||||
|
landing there until it switches once.
|
||||||
|
|
||||||
|
Setting the default is two statements — clear the old, set the new — so the
|
||||||
|
route runs them in one transaction; the index refuses the instant in between.
|
||||||
|
|
||||||
## Renaming a version
|
## Renaming a version
|
||||||
|
|
||||||
Safe at any time, including on a closed one. Nothing derives from the name: the
|
Safe at any time, including on a closed one. Nothing derives from the name: the
|
||||||
|
|||||||
@ -446,17 +446,43 @@ ${colDefs},
|
|||||||
// the value alone cannot tell from "not mentioned".
|
// the value alone cannot tell from "not mentioned".
|
||||||
router.put('/versions/:id', async (req, res) => {
|
router.put('/versions/:id', async (req, res) => {
|
||||||
const { name, description, exclude_iters,
|
const { name, description, exclude_iters,
|
||||||
adjustment_segment, adjustment_bucket, unlabeled_load } = req.body;
|
adjustment_segment, adjustment_bucket, unlabeled_load,
|
||||||
|
is_default } = req.body;
|
||||||
const set = (v) => (v === undefined ? null : (String(v).trim() || null));
|
const set = (v) => (v === undefined ? null : (String(v).trim() || null));
|
||||||
|
|
||||||
|
// is_default decides where every account without a saved selection
|
||||||
|
// lands, so it is not the version's own setting the way its name is:
|
||||||
|
// it follows the pf.log rule, an admin override and nothing else. The
|
||||||
|
// UI greys the box out rather than offering a click that answers 403.
|
||||||
|
if (is_default !== undefined && !req.session?.user?.is_admin) {
|
||||||
|
return res.status(403).json({
|
||||||
|
error: 'Only an admin can set the default forecast'
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// The write is two statements when the default moves -- clear the old
|
||||||
|
// one, set the new -- and the partial unique index refuses the moment
|
||||||
|
// in between, so they share a transaction.
|
||||||
|
const client = await pool.connect();
|
||||||
try {
|
try {
|
||||||
const result = await pool.query(`
|
await client.query('BEGIN');
|
||||||
|
if (is_default === true) {
|
||||||
|
await client.query(`
|
||||||
|
UPDATE pf.version SET is_default = false
|
||||||
|
WHERE source_id = (SELECT source_id FROM pf.version WHERE id = $1)
|
||||||
|
AND id <> $1
|
||||||
|
AND is_default
|
||||||
|
`, [req.params.id]);
|
||||||
|
}
|
||||||
|
const result = await client.query(`
|
||||||
UPDATE pf.version SET
|
UPDATE pf.version SET
|
||||||
name = COALESCE($2, name),
|
name = COALESCE($2, name),
|
||||||
description = COALESCE($3, description),
|
description = COALESCE($3, description),
|
||||||
exclude_iters = COALESCE($4, exclude_iters),
|
exclude_iters = COALESCE($4, exclude_iters),
|
||||||
adjustment_segment = CASE WHEN $5::bool THEN $6::text ELSE adjustment_segment END,
|
adjustment_segment = CASE WHEN $5::bool THEN $6::text ELSE adjustment_segment END,
|
||||||
adjustment_bucket = CASE WHEN $7::bool THEN $8::text ELSE adjustment_bucket END,
|
adjustment_bucket = CASE WHEN $7::bool THEN $8::text ELSE adjustment_bucket END,
|
||||||
unlabeled_load = CASE WHEN $9::bool THEN $10::text ELSE unlabeled_load END
|
unlabeled_load = CASE WHEN $9::bool THEN $10::text ELSE unlabeled_load END,
|
||||||
|
is_default = CASE WHEN $11::bool THEN $12::bool ELSE is_default END
|
||||||
WHERE id = $1
|
WHERE id = $1
|
||||||
RETURNING *
|
RETURNING *
|
||||||
`, [
|
`, [
|
||||||
@ -466,22 +492,35 @@ ${colDefs},
|
|||||||
exclude_iters ? JSON.stringify(exclude_iters) : null,
|
exclude_iters ? JSON.stringify(exclude_iters) : null,
|
||||||
adjustment_segment !== undefined, set(adjustment_segment),
|
adjustment_segment !== undefined, set(adjustment_segment),
|
||||||
adjustment_bucket !== undefined, set(adjustment_bucket),
|
adjustment_bucket !== undefined, set(adjustment_bucket),
|
||||||
unlabeled_load !== undefined, set(unlabeled_load)
|
unlabeled_load !== undefined, set(unlabeled_load),
|
||||||
|
is_default !== undefined, is_default === true
|
||||||
]);
|
]);
|
||||||
if (result.rows.length === 0) {
|
if (result.rows.length === 0) {
|
||||||
|
await client.query('ROLLBACK');
|
||||||
return res.status(404).json({ error: 'Version not found' });
|
return res.status(404).json({ error: 'Version not found' });
|
||||||
}
|
}
|
||||||
|
await client.query('COMMIT');
|
||||||
res.json(result.rows[0]);
|
res.json(result.rows[0]);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
|
await client.query('ROLLBACK').catch(() => {});
|
||||||
console.error(err);
|
console.error(err);
|
||||||
// (source_id, name) is unique, so a rename can collide. The POST has
|
// (source_id, name) is unique, so a rename can collide. The POST has
|
||||||
// said so for as long as it has existed; this said it with a raw
|
// said so for as long as it has existed; this said it with a raw
|
||||||
// Postgres message, which reads as a server fault rather than a name
|
// Postgres message, which reads as a server fault rather than a name
|
||||||
// that is already taken.
|
// that is already taken.
|
||||||
if (err.code === '23505') {
|
if (err.code === '23505') {
|
||||||
return res.status(409).json({ error: 'A version with that name already exists for this source' });
|
return res.status(409).json({
|
||||||
|
// Two unique constraints raise 23505 here, and the name is
|
||||||
|
// only one of them -- version_one_default is the other,
|
||||||
|
// reachable only if two requests set the default at once.
|
||||||
|
error: err.constraint === 'version_one_default'
|
||||||
|
? 'Another forecast was just made the default — reload and try again'
|
||||||
|
: 'A version with that name already exists for this source'
|
||||||
|
});
|
||||||
}
|
}
|
||||||
res.status(500).json({ error: err.message });
|
res.status(500).json({ error: err.message });
|
||||||
|
} finally {
|
||||||
|
client.release();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@ -122,6 +122,22 @@ ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS adjustment_segment text;
|
|||||||
ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS adjustment_bucket text;
|
ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS adjustment_bucket text;
|
||||||
ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS unlabeled_load text;
|
ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS unlabeled_load text;
|
||||||
|
|
||||||
|
-- Where an account lands when it has no saved selection of its own.
|
||||||
|
--
|
||||||
|
-- Without it the app fell back to the newest version, which is whatever
|
||||||
|
-- somebody created last -- a scratch scenario as readily as the plan. Sales
|
||||||
|
-- accounts, which never pick a version, therefore followed whatever the
|
||||||
|
-- forecasting work happened to be doing that morning.
|
||||||
|
--
|
||||||
|
-- At most one per source, by partial unique index, the way pf.layout does it:
|
||||||
|
-- two defaults is not a state anything could resolve, so the database refuses
|
||||||
|
-- it rather than the route picking one.
|
||||||
|
ALTER TABLE pf.version ADD COLUMN IF NOT EXISTS is_default boolean NOT NULL DEFAULT false;
|
||||||
|
|
||||||
|
CREATE UNIQUE INDEX IF NOT EXISTS version_one_default
|
||||||
|
ON pf.version (source_id)
|
||||||
|
WHERE is_default;
|
||||||
|
|
||||||
-- What the entry did, stamped when it did it.
|
-- What the entry did, stamped when it did it.
|
||||||
--
|
--
|
||||||
-- Not a cache: a log entry's forecast rows never change after it is written.
|
-- Not a cache: a log entry's forecast rows never change after it is written.
|
||||||
|
|||||||
@ -67,11 +67,18 @@ export default function App() {
|
|||||||
// localStorage is cleared and then set straight back when the fetch lands.
|
// localStorage is cleared and then set straight back when the fetch lands.
|
||||||
// Forecast's load effect is keyed on that id, so the round trip made every
|
// Forecast's load effect is keyed on that id, so the round trip made every
|
||||||
// page load fetch and aggregate the whole version twice.
|
// page load fetch and aggregate the whole version twice.
|
||||||
|
//
|
||||||
|
// What it falls back TO is the source's default version, set on the Baseline
|
||||||
|
// page, and only the newest version when nobody has named one. The list is
|
||||||
|
// ordered created_at DESC, so the old fallback landed an account on whatever
|
||||||
|
// scenario was created last -- a scratch copy as readily as the plan. Sales
|
||||||
|
// accounts never pick a version themselves, so that is where they stayed.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!sourceId || !versionsLoaded) return
|
if (!sourceId || !versionsLoaded) return
|
||||||
if (versions.length === 0) { setVersionId(''); return }
|
if (versions.length === 0) { setVersionId(''); return }
|
||||||
if (!versionId || !versions.some(v => String(v.id) === String(versionId))) {
|
if (!versionId || !versions.some(v => String(v.id) === String(versionId))) {
|
||||||
setVersionId(String(versions[0].id))
|
const landing = versions.find(v => v.is_default) || versions[0]
|
||||||
|
setVersionId(String(landing.id))
|
||||||
}
|
}
|
||||||
}, [versions, versionsLoaded, sourceId, versionId])
|
}, [versions, versionsLoaded, sourceId, versionId])
|
||||||
|
|
||||||
|
|||||||
@ -66,7 +66,7 @@ export default function StatusBar({ view, sources = [], sourceId, setSourceId, v
|
|||||||
>
|
>
|
||||||
{versions.length === 0
|
{versions.length === 0
|
||||||
? <option value="">— no versions —</option>
|
? <option value="">— no versions —</option>
|
||||||
: versions.map(v => <option key={v.id} value={v.id}>{v.name}</option>)}
|
: versions.map(v => <option key={v.id} value={v.id}>{v.name}{v.is_default ? ' (default)' : ''}</option>)}
|
||||||
</select>
|
</select>
|
||||||
{selectedVersion && (
|
{selectedVersion && (
|
||||||
<span className={`font-medium ${selectedVersion.status === 'open' ? 'text-green-600' : 'text-gray-400'}`}>
|
<span className={`font-medium ${selectedVersion.status === 'open' ? 'text-green-600' : 'text-gray-400'}`}>
|
||||||
|
|||||||
@ -536,6 +536,23 @@ export default function Baseline({ sources = [], sourceId, versions = [], versio
|
|||||||
} catch (err) { flash(err.message, 'error') }
|
} catch (err) { flash(err.message, 'error') }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The default is the source's, not this version's: setting it here clears it
|
||||||
|
// wherever it was. Admin only, server-side as well as here.
|
||||||
|
async function saveDefaultVersion(checked) {
|
||||||
|
try {
|
||||||
|
const res = await fetch(`/api/versions/${versionId}`, {
|
||||||
|
method: 'PUT',
|
||||||
|
headers: { 'Content-Type': 'application/json' },
|
||||||
|
body: JSON.stringify({ is_default: checked }),
|
||||||
|
})
|
||||||
|
if (!res.ok) { const d = await res.json(); flash(d.error, 'error'); return }
|
||||||
|
await refreshVersions(sourceId)
|
||||||
|
flash(checked
|
||||||
|
? 'Default forecast — this is where anyone without a saved selection lands'
|
||||||
|
: 'No default forecast — the newest one is used instead')
|
||||||
|
} catch (err) { flash(err.message, 'error') }
|
||||||
|
}
|
||||||
|
|
||||||
const selectedVersion = versions.find(v => String(v.id) === versionId)
|
const selectedVersion = versions.find(v => String(v.id) === versionId)
|
||||||
|
|
||||||
return (
|
return (
|
||||||
@ -595,6 +612,19 @@ export default function Baseline({ sources = [], sourceId, versions = [], versio
|
|||||||
placeholder="optional"
|
placeholder="optional"
|
||||||
className="border border-gray-200 rounded px-2 py-1 text-sm w-full" />
|
className="border border-gray-200 rounded px-2 py-1 text-sm w-full" />
|
||||||
</div>
|
</div>
|
||||||
|
{/* Where an account with no saved selection of its own lands. One per
|
||||||
|
source, so ticking it here unticks it wherever it was. */}
|
||||||
|
<label className={`flex items-center gap-2 text-sm pb-1 ${me?.is_admin ? '' : 'opacity-50'}`}
|
||||||
|
title={me?.is_admin
|
||||||
|
? 'Where anyone without a saved selection lands'
|
||||||
|
: 'Only an admin can set the default forecast'}>
|
||||||
|
<input
|
||||||
|
type="checkbox"
|
||||||
|
checked={!!selectedVersion?.is_default}
|
||||||
|
disabled={!me?.is_admin}
|
||||||
|
onChange={e => saveDefaultVersion(e.target.checked)} />
|
||||||
|
Default forecast
|
||||||
|
</label>
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user