From 915981b4a774796b6b31b628010ea28938cf78e7 Mon Sep 17 00:00:00 2001 From: Paul Trowbridge Date: Wed, 23 Sep 2026 01:57:24 -0400 Subject: [PATCH] Put the forecasts in the nav, and lock the admin pages down Nav. The source and version dropdowns sat in a status bar that is otherwise read-only, so they did not read as the controls they were. The forecasts are now listed in the sidebar above the three steps, each row carrying its status and a star for the default. Not nested under the Forecast button, which is where this started: Baseline works on the same version, so hanging the list off one step would either misrepresent the scope or duplicate it under two. The source is gone from the nav entirely -- gs.osm_skinny means nothing to someone editing a forecast. The sidebar lists every source's versions and sets the source from the one picked, since a version carries its source_id; `versions` stays scoped to the selected source, because Baseline and Forecast are, so the sidebar gets its own list rather than widening what versions means everywhere. The picker only appears on Setup now, which is the one place the raw table name is the right label. The status bar keeps what follows FROM the selection -- the forecast's name and its row count. The table it writes to moved into the hover panel, still click-to-copy, since pasting it into psql is why it was ever on screen. Permissions. requireAdmin existed in lib/auth.js and was applied to nothing, so all 26 non-admin accounts could call every Setup and Baseline endpoint directly. It is now on source register/deregister, col_meta writes, Generate SQL, dim refresh, version create/delete/ close/reopen, PUT /versions/:id, all of plans.js, and the baseline and reference loads. Deliberately not on any read, nor on the forecast operations, which carry their own owner and territory rules. Both pages stay visible and go read-only, via one
each rather than a flag threaded through forty-five controls, with a banner saying why. The Tags panel on Baseline is excluded: its permission is ownership, not admin, and a rep may rename the initiatives they named themselves. The table browser reads are open to everyone, because gating what a page needs in order to render turned Setup into a 403 -- which it then rendered as a crash, since the error body was passed to .map during render and unmounting React blanks the app. Setup's list fetches now coerce to an array, so an unexpected response costs an empty panel and not the session. That fragility predated the gating. Toolbar. The zone labels were the same visual weight as the buttons they name, and the rules between groups were the same weight as the zone borders, so nothing separated at the level it belonged to. Labels are heavier and darker, group rules are stronger and taller, and the forecast zone has a container of its own instead of floating. Fit widths is removed, button and function. It measured the leaf header over viewport rows only, so anything below the fold never counted; it passed an option draw() does not have and raced draw()'s throttle, leaving columns cleared but unmeasured at their floor widths; and the persist that followed saved nothing, because saveColumnSizes returns only overrides, which resetAutoSize had just emptied. Recoverable from git if it is ever worth fixing rather than dropping. Co-Authored-By: Claude Opus 5 --- routes/operations.js | 10 +-- routes/plans.js | 8 +-- routes/sources.js | 12 ++-- routes/tables.js | 3 + routes/versions.js | 12 ++-- ui/src/App.jsx | 34 ++++++++++- ui/src/components/ReadOnlyNotice.jsx | 22 +++++++ ui/src/components/Sidebar.jsx | 91 +++++++++++++++++++++++++++- ui/src/components/StatusBar.jsx | 81 ++++++++++++------------- ui/src/index.css | 25 ++++++-- ui/src/views/Baseline.jsx | 11 ++++ ui/src/views/Forecast.jsx | 73 ++-------------------- ui/src/views/Setup.jsx | 30 +++++++-- 13 files changed, 271 insertions(+), 141 deletions(-) create mode 100644 ui/src/components/ReadOnlyNotice.jsx diff --git a/routes/operations.js b/routes/operations.js index 950b54d..3dcdf3b 100644 --- a/routes/operations.js +++ b/routes/operations.js @@ -3,7 +3,7 @@ const { tableFromArrays, tableToIPC } = require('apache-arrow'); const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, buildScopeClause, buildTerritoryClause, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc, SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, VERSION_JOIN, ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET } = require('../lib/sql_generator'); -const { sessionUser, sessionTerritory } = require('../lib/auth'); +const { sessionUser, sessionTerritory, requireAdmin } = require('../lib/auth'); const { fcTable } = require('../lib/utils'); module.exports = function(pool) { @@ -572,7 +572,7 @@ module.exports = function(pool) { }); // load baseline rows from source table — additive, no delete - router.post('/versions/:id/baseline', async (req, res) => { + router.post('/versions/:id/baseline', requireAdmin, async (req, res) => { const { where_clause, date_offset, note, filters, raw_where, label, bucket, tag } = req.body; const pf_user = sessionUser(req); const dateOffset = date_offset || '0 days'; @@ -611,7 +611,7 @@ module.exports = function(pool) { // edit a baseline or reference segment in place — only allowed before any // scale/recode/clone has been applied on this version, since those would // have been calibrated against the old segment's totals. - router.put('/versions/:id/baseline/:logid', async (req, res) => { + router.put('/versions/:id/baseline/:logid', requireAdmin, async (req, res) => { const versionId = parseInt(req.params.id); const logid = parseInt(req.params.logid); const { where_clause, date_offset, note, filters, raw_where, label, bucket, tag } = req.body; @@ -698,7 +698,7 @@ module.exports = function(pool) { }); // delete all baseline rows and log entries for a version - router.delete('/versions/:id/baseline', async (req, res) => { + router.delete('/versions/:id/baseline', requireAdmin, async (req, res) => { const versionId = parseInt(req.params.id); try { const ctx = await getContext(versionId, 'baseline'); @@ -732,7 +732,7 @@ module.exports = function(pool) { }); // load reference rows from source table (additive — does not clear prior reference rows) - router.post('/versions/:id/reference', async (req, res) => { + router.post('/versions/:id/reference', requireAdmin, async (req, res) => { const { where_clause, date_offset, note, filters, raw_where, label, bucket, tag } = req.body; const pf_user = sessionUser(req); const dateOffset = date_offset || '0 days'; diff --git a/routes/plans.js b/routes/plans.js index 5a03614..08673ec 100644 --- a/routes/plans.js +++ b/routes/plans.js @@ -1,5 +1,5 @@ const express = require('express'); -const { sessionUser } = require('../lib/auth'); +const { sessionUser, requireAdmin } = require('../lib/auth'); // Planned load segments — a version's definition, before it has been run. // @@ -41,7 +41,7 @@ module.exports = function(pool) { } }); - router.post('/versions/:id/plans', async (req, res) => { + router.post('/versions/:id/plans', requireAdmin, async (req, res) => { const versionId = parseInt(req.params.id); const b = req.body || {}; if (!['baseline', 'reference'].includes(b.kind)) { @@ -73,7 +73,7 @@ module.exports = function(pool) { // Any subset of the fields, plus run_logid when a run has just happened. // The flag-and-value pair rather than COALESCE on the value: clearing a // label to blank has to be distinguishable from not mentioning it. - router.patch('/plans/:pid', async (req, res) => { + router.patch('/plans/:pid', requireAdmin, async (req, res) => { const pid = parseInt(req.params.pid); const b = req.body || {}; const keys = [...FIELDS, 'run_logid'].filter(k => b[k] !== undefined); @@ -109,7 +109,7 @@ module.exports = function(pool) { } }); - router.delete('/plans/:pid', async (req, res) => { + router.delete('/plans/:pid', requireAdmin, async (req, res) => { try { const { rows } = await pool.query( `DELETE FROM pf.load_plan WHERE id = $1 RETURNING id`, [parseInt(req.params.pid)] diff --git a/routes/sources.js b/routes/sources.js index 1a514b3..07fd397 100644 --- a/routes/sources.js +++ b/routes/sources.js @@ -1,7 +1,7 @@ const express = require('express'); const { generateSQL, buildTerritoryClause } = require('../lib/sql_generator'); const { RELATION_COLUMNS_SQL } = require('../lib/utils'); -const { sessionTerritory } = require('../lib/auth'); +const { sessionTerritory, requireAdmin } = require('../lib/auth'); const { sessionUser } = require('../lib/auth'); module.exports = function(pool) { @@ -22,7 +22,7 @@ module.exports = function(pool) { // register a source table // auto-populates col_meta from information_schema with role='ignore' - router.post('/sources', async (req, res) => { + router.post('/sources', requireAdmin, async (req, res) => { const { schema, tname, label } = req.body; const created_by = sessionUser(req); if (!schema || !tname) { @@ -81,7 +81,7 @@ module.exports = function(pool) { }); // save col_meta — accepts full array, upserts each row - router.put('/sources/:id/cols', async (req, res) => { + router.put('/sources/:id/cols', requireAdmin, async (req, res) => { const sourceId = parseInt(req.params.id); const cols = req.body; if (!Array.isArray(cols)) { @@ -142,7 +142,7 @@ module.exports = function(pool) { }); // generate SQL for all operations from current col_meta and store in pf.sql - router.post('/sources/:id/generate-sql', async (req, res) => { + router.post('/sources/:id/generate-sql', requireAdmin, async (req, res) => { const sourceId = parseInt(req.params.id); try { const srcResult = await pool.query( @@ -338,7 +338,7 @@ module.exports = function(pool) { // Slow by nature -- it reads the whole source, which for a view over a // transaction table is millions of rows -- so it is a deliberate action rather // than something that happens on a page load. - router.post('/sources/:id/dim/:group/refresh', async (req, res) => { + router.post('/sources/:id/dim/:group/refresh', requireAdmin, async (req, res) => { const sourceId = parseInt(req.params.id); const group = req.params.group; try { @@ -473,7 +473,7 @@ module.exports = function(pool) { } }); - router.delete('/sources/:id', async (req, res) => { + router.delete('/sources/:id', requireAdmin, async (req, res) => { try { const result = await pool.query( `DELETE FROM pf.source WHERE id = $1 RETURNING *`, diff --git a/routes/tables.js b/routes/tables.js index 3dd4ee4..028d8cd 100644 --- a/routes/tables.js +++ b/routes/tables.js @@ -1,6 +1,9 @@ const express = require('express'); const { RELATION_COLUMNS_SQL } = require('../lib/utils'); +// Reads, deliberately open to everyone: Setup is visible read-only to +// non-admins, so gating what the page needs in order to render turned it into +// a 403 the client rendered as a crash. The writes it leads to are gated. module.exports = function(pool) { const router = express.Router(); diff --git a/routes/versions.js b/routes/versions.js index 5ecbc59..4540443 100644 --- a/routes/versions.js +++ b/routes/versions.js @@ -1,6 +1,6 @@ const express = require('express'); const { fcTable, mapType, RELATION_COLUMNS_SQL } = require('../lib/utils'); -const { sessionUser, sessionTerritory } = require('../lib/auth'); +const { sessionUser, sessionTerritory, requireAdmin } = require('../lib/auth'); const { buildTerritoryClause } = require('../lib/sql_generator'); // Copy a version's definition onto a freshly created one: its load segments as @@ -90,7 +90,7 @@ module.exports = function(pool) { // create a new version // inserts version row, then CREATE TABLE pf.fc_{tname}_{version_id} in one transaction - router.post('/sources/:id/versions', async (req, res) => { + router.post('/sources/:id/versions', requireAdmin, async (req, res) => { const sourceId = parseInt(req.params.id); const { name, description, exclude_iters, clone_from } = req.body; const created_by = sessionUser(req); @@ -514,7 +514,7 @@ ${colDefs}, // The three name columns are flag-and-value pairs rather than COALESCE: // clearing one back to the built-in means writing null, which COALESCE on // the value alone cannot tell from "not mentioned". - router.put('/versions/:id', async (req, res) => { + router.put('/versions/:id', requireAdmin, async (req, res) => { const { name, description, exclude_iters, adjustment_segment, adjustment_bucket, unlabeled_load, is_default } = req.body; @@ -595,7 +595,7 @@ ${colDefs}, }); // close a version — blocks further edits - router.post('/versions/:id/close', async (req, res) => { + router.post('/versions/:id/close', requireAdmin, async (req, res) => { const pf_user = sessionUser(req); try { const result = await pool.query(` @@ -615,7 +615,7 @@ ${colDefs}, }); // reopen a closed version - router.post('/versions/:id/reopen', async (req, res) => { + router.post('/versions/:id/reopen', requireAdmin, async (req, res) => { try { const result = await pool.query(` UPDATE pf.version @@ -635,7 +635,7 @@ ${colDefs}, // delete a version — drops forecast table then deletes version record // log entries are removed by ON DELETE CASCADE on pf.log.version_id - router.delete('/versions/:id', async (req, res) => { + router.delete('/versions/:id', requireAdmin, async (req, res) => { const versionId = parseInt(req.params.id); const client = await pool.connect(); try { diff --git a/ui/src/App.jsx b/ui/src/App.jsx index 9f1998f..77d70f2 100644 --- a/ui/src/App.jsx +++ b/ui/src/App.jsx @@ -39,8 +39,35 @@ export default function App() { return list }, [sourceId]) + // Every source's forecasts, for the sidebar only. + // + // `versions` stays scoped to the selected source, because Baseline and + // Forecast are, and widening it would change what it means everywhere. The + // sidebar is the one place that has to be cross-source: it is how a forecast + // gets chosen, and a forecast carries its own source_id, so choosing one + // settles the source. Without this, a second registered source would be + // unreachable from anywhere but Setup -- and invisible from everywhere else, + // which is worse. + // + // A fan-out rather than an endpoint: there is one source today, so this is one + // request, and a `GET /versions` is not worth adding until there are enough to + // make the difference measurable. + const [allVersions, setAllVersions] = useState([]) + const refreshAllVersions = useCallback(async (list) => { + const srcs = list ?? sources + if (!srcs.length) { setAllVersions([]); return } + const lists = await Promise.all(srcs.map(s => + fetch(`/api/sources/${s.id}/versions`).then(r => r.json()).catch(() => []))) + setAllVersions(lists.flat().filter(v => v && v.id != null)) + }, [sources]) + useEffect(() => { refreshSources() }, []) + // Recomputed when the selected source's list changes, which is what creating + // or deleting a version does -- so the sidebar follows without every caller + // having to know it exists. + useEffect(() => { if (sourcesLoaded) refreshAllVersions() }, [sourcesLoaded, sources, versions]) + // The selection is restored from localStorage and survives a deregister, so it // has to be re-checked against the list itself rather than only at mount: // deleting the selected source otherwise leaves a dead id behind and every @@ -90,7 +117,12 @@ export default function App() { return (
- +
diff --git a/ui/src/components/ReadOnlyNotice.jsx b/ui/src/components/ReadOnlyNotice.jsx new file mode 100644 index 0000000..435152c --- /dev/null +++ b/ui/src/components/ReadOnlyNotice.jsx @@ -0,0 +1,22 @@ +// Says why everything on the page is greyed out. +// +// The disabling itself is one `
` around the page rather than +// a flag threaded through every control -- Baseline alone has forty-five of +// them, and the one that got missed would be the one that mattered. A disabled +// fieldset takes its descendant form controls out with it natively, so nothing +// has to remember to opt in. +// +// None of this is the permission. The routes carry requireAdmin; this only +// stops an account being offered a button that would answer 403. +export default function ReadOnlyNotice({ what }) { + return ( +
+ + + + + Read-only — {what} is managed by an administrator. +
+ ) +} diff --git a/ui/src/components/Sidebar.jsx b/ui/src/components/Sidebar.jsx index 788b3c8..76b710d 100644 --- a/ui/src/components/Sidebar.jsx +++ b/ui/src/components/Sidebar.jsx @@ -32,7 +32,41 @@ const NAV = [ }, ] -export default function Sidebar({ view, setView, expanded, setExpanded }) { +// The forecasts are listed here, above the steps, rather than offered as a +// dropdown in the status bar. Two reasons: a scenario is a thing you open, not +// a setting you change, and the status bar is otherwise read-only, so a control +// sitting in it did not read as one. +// +// Not nested under the Forecast button, which is where this started: Baseline +// works on the same version, so hanging it off one step would either +// misrepresent the scope or duplicate the list under two. It reads +// "what am I working on" and then "what step am I on". +// +// The source is deliberately absent. `gs.osm_skinny` means nothing to someone +// editing a forecast, and a version already carries its source, so choosing a +// forecast settles it. Picking the source is a Setup concern and the status bar +// only offers it there. +export default function Sidebar({ + view, setView, expanded, setExpanded, + versions = [], versionId, setVersionId, + sourceId, setSourceId, +}) { + const selected = versions.find(v => String(v.id) === String(versionId)) + + function pick(v) { + // The source comes with the forecast. This is the whole reason the source + // needs no control of its own outside Setup: every version carries its + // source_id, so choosing one settles both. Set first, so the version is not + // briefly validated against the outgoing source's list. + if (v.source_id != null && String(v.source_id) !== String(sourceId)) { + setSourceId(String(v.source_id)) + } + setVersionId(String(v.id)) + // Setup is the one step a version means nothing to, so choosing one there + // is a statement of intent to go and work on it. + if (view === 'setup') setView('forecast') + } + return (
+ {expanded ? ( +
+
+ Forecasts +
+ {/* Capped and scrollable: the list grows with the work, and the steps + below it must never be pushed off the bottom. */} +
+ {versions.length === 0 && ( +
none yet
+ )} + {versions.map(v => { + const active = String(v.id) === String(versionId) + return ( + + ) + })} +
+
+ ) : ( + // Collapsed, the list has nowhere to go, so all that survives is which + // one is open — as a tooltip, on a button that opens the rail again. +
+ +
+ )} +