🎉 ETL-authored chart configs#6553
Open
pabloarosado wants to merge 24 commits into
Open
Conversation
Adds a new JSON column `chart_configs.etlConfig` and extends the merge
chain that computes `chart_configs.full` from two layers to three:
full = merge(variableETL, etlConfig, patch)
Contract:
- ETL writes only to `etlConfig` (via the new PUT /charts/:id/etlConfig
endpoint, modeled on PUT /variables/:id/grapherConfigETL).
- Admin writes only to `patch` (unchanged behaviour).
- The two writers no longer collide; ETL re-pushes preserve admin
patches by construction.
Behaviour for existing charts is unchanged because `etlConfig` is NULL
and `mergeGrapherConfigs` filters empty configs. No data migration.
Multi-dim view chart_configs rows get the column for free (they live in
chart_configs); the same merge is applied for forward-compatibility,
even though their `etlConfig` will be NULL until the MDIM editor work.
Deferred to follow-ups:
- Admin client label tweak ("Linked to ETL" chip).
- narrativeCharts.ts, dataCallouts.ts, ExplorerViews.ts — they use
other merge paths that don't reach this layer.
- ETL-side chart_upsert.py rewrite (separate etl repo PR).
Tests: 5 new vitest cases in adminSiteServer/tests/charts.test.ts
covering PUT/DELETE behaviour, 3-layer precedence, and admin-patch
survival across ETL re-push.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Quick links (staging server):
Login:
Archive:SVG tester:Number of differences (graphers): 0 ✅ Edited: 2026-05-28 16:30:43 UTC |
The chart editor reconstructs the chart from `parent + patch`. After the previous commit, `patch` is computed as `diff(submitted, merge( variableETL, etlConfig))` and is therefore much leaner than before. But the parent endpoint kept returning only `variableETL` — so the editor's reconstruction was missing the etlConfig contributions (map colors, subtitle, note, hasMapTab, etc.), making the chart appear broken in the editor preview even though `chart_configs.full` was correct. Fix: have `/charts/:id.parent.json` return `merge(variableETL, etlConfig)` as the parent config. The editor's patch + this parent now reconstruct to the same full config that's stored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the previous shim that merged etlConfig into the parent
endpoint's `config` field server-side. The editor now receives the
two layers above the admin's patch separately and merges them
locally, so the layered model is explicit on both sides of the API.
Server (`adminSiteServer/apiRoutes/charts.ts`):
- `getChartParentJson` returns `{variableId, variableConfig, etlConfig,
isInheritanceEnabled}` instead of `{variableId, config, isActive}`.
Private admin endpoint; only consumer is `ChartEditorPage`.
Client (`adminSiteClient/AbstractChartEditor.ts`):
- `AbstractChartEditorManager` gains `etlConfig?: GrapherInterface`.
- The editor stores `etlConfig` alongside `parentConfig`.
- `activeParentConfig` is now `merge(variableConfig if inherit, etlConfig)`
— so `etlConfig` continues to apply even with indicator-inheritance
disabled, matching the server-side save logic.
- `isPropertyInherited` / `canPropertyBeInherited` no longer gate on
`isInheritanceEnabled` alone; they check the effective parent.
- IndicatorChartEditor and NarrativeChartEditor are unaffected — they
don't set `etlConfig`, so behaviour is identical to today.
Client (`adminSiteClient/ChartEditorPage.tsx`):
- Fetches both `variableConfig` and `etlConfig` from the parent
endpoint and threads them into the editor.
Tests updated to use the new response shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings PUT/DELETE /charts/:id/etlConfig up to the same housekeeping as saveGrapher, and re-diffs the chart's patch on each call so the layered model behaves correctly across repeated ETL pushes. Changes: - Re-diff existing `patch` against the new parent stack before writing. Strips redundant patch entries (notably `dimensions`, which the bootstrap creation flow forced into patch and which would otherwise block future ETL changes to dimensions — important for the indicator-upgrader-replacement workflow). Differs from `diffGrapherConfigs` only in that we don't preserve REQUIRED_KEYS unconditionally; dimensions fall through to the parent stack when they match, while the chart-table fields ($schema, id, slug, version, isPublished) always stay in patch. - Bump `version` on every ETL push for cache-busting (same as saveGrapher). - Refresh `chart_dimensions` from the new `fullConfig.dimensions`. The previous version left this table stale when ETL changed dimensions, breaking dependency tracking, search indexing, and downstream uses. - Save the rendered `full` to R2 by slug (when published), in addition to the UUID-keyed R2 object. The previous version only wrote the UUID object, so the `/grapher/<slug>` URL kept serving stale content for published charts. - Record a `chart_revisions` entry so ETL pushes appear in the chart's audit history alongside admin saves. Refactor: factored out `rediffPatchAgainstNewParentStack`, `refreshChartDimensionsAndR2`, and `insertChartRevision` helpers shared by PUT and DELETE. Resolves codex findings 1, 2, and 3 from the PR review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pabloarosado
changed the title
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a7869969c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The rediff only dropped fields from patch when they matched the new parent stack. For `dimensions` specifically that's insufficient: the regular admin chart-save path forces `dimensions` into patch unconditionally (it's in the global `REQUIRED_KEYS` of `diffGrapherConfigs`). After any admin save, patch.dimensions is repopulated with the current dimensions — and on the next ETL push that changes the indicator, patch.dimensions differs from the new etlConfig.dimensions, gets retained by the rediff, and wins the merge. Result: ETL can never change which indicator an ETL-managed chart plots after an admin has saved it. Fix: for ETL-managed charts (where the new parent stack supplies `dimensions`), patch never owns dimensions. Strip them from the result unconditionally. Admin overrides of other fields (title, subtitle, map, axes, etc.) are unaffected. Resolves the codex finding on the open PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous fix unconditionally stripped `dimensions` from patch on every ETL push. That made `dimensions` the only field admins couldn't actually override on ETL-managed charts — an inconsistency with how every other field (title, subtitle, map config, …) behaves. The real root cause is in `diffGrapherConfigs`: `dimensions` is in `REQUIRED_KEYS`, so it's always retained in patch even when the value matches the parent stack. That means every admin no-op Save dumps the current dimensions back into patch as residue, which then looks like a real admin override and blocks subsequent ETL changes. Fix at the source: in `updateExistingChart`, when the chart has an etlConfig (i.e. is ETL-managed) and the diff's `dimensions` value matches the parent stack, drop it from patch. Admins who actually changed dimensions still see them retained (real override), and that override survives subsequent ETL pushes — same semantics as every other field. Reverts the rediff hack: `rediffPatchAgainstNewParentStack` no longer special-cases dimensions; it applies the same "drop if matches parent" rule to all fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
@pabloarosado, quickly chiming in: I think that we need to add a migration for table |
A genuine admin dimension override is intentionally kept and wins over etlConfig (manual variable changes survive ETL pushes); only dimensions that echo the parent (e.g. bootstrap leftovers) are stripped. Addresses Codex feedback on #6553. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the chart_configs.etlConfig column with a charts.configIdETL pointer to a dedicated chart_configs row, mirroring variables.grapherConfigIdETL. Keeps chart_configs a generic config store and lets each owner declare its own layers via pointers (per Sophia's feedback). The rendered full is still merge(variableETL, etlConfig, patch); the ETL config row is MySQL-only and never uploaded to R2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ges) Adds a nullable, unique catalogPath column to the charts table linking an ETL-authored chart to its ETL step (e.g. animal_welfare/latest/banning_of_chick_culling#banning_of_chick_culling), mirroring multi_dim_data_pages.catalogPath. Persisted via the etlConfig endpoint (COALESCE so non-ETL callers don't clobber it). NULL for hand-authored charts. Pairs with the ETL-side change. Targets the chart-config feature branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔨🤖 Store ETL chart config as a separate config row (rows model) + catalogPath
pabloarosado
changed the title
pabloarosado
changed the title
Contributor
Author
Thanks for chiming in @lucasrodes! Yes, I agree; I have refactored the logic in this PR a bit. In my latest changes I've also included |
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b320cf046
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Enables an ETL-authored grapher-config layer for charts (stored separately from admin edits) by introducing a charts.configIdETL pointer to a dedicated chart_configs row, plus a stable charts.catalogPath identifier. This keeps admin edits in patch while letting ETL update its own layer without clobbering admin changes, and updates both server- and client-side config merging to account for the new layer.
Changes:
- Add
charts.configIdETL(FK tochart_configs) andcharts.catalogPath(unique, nullable) to support ETL-managed chart identity + config layering. - Update config inheritance/merge logic to include chart-level ETL config when recomputing full configs (including indicator-driven inheritance updates).
- Add admin API endpoints and tests for PUT/DELETE
/admin/api/charts/:id/etlConfig, and update the admin editor to merge variable + chart ETL layers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/@ourworldindata/types/src/dbTypes/Charts.ts | Extends chart DB insert/type definitions with configIdETL and catalogPath. |
| db/model/Variable.ts | Includes chart-level ETL config in inheritance recomputation for charts inheriting from an indicator. |
| db/migration/1779983981061-AddConfigIdEtlToCharts.ts | Adds charts.configIdETL with FK to chart_configs. |
| db/migration/1780587927881-AddCatalogPathToCharts.ts | Adds unique nullable charts.catalogPath for stable ETL identity. |
| adminSiteServer/tests/charts.test.ts | Updates parent JSON expectations and adds a new test suite covering chart-level ETL config endpoints/precedence. |
| adminSiteServer/apiRoutes/charts.ts | Implements ETL config PUT/DELETE endpoints and updates admin save logic to preserve/merge the ETL layer. |
| adminSiteServer/apiRouter.ts | Wires new chart ETL config endpoints into the admin API router. |
| adminSiteClient/ChartEditorPage.tsx | Consumes updated parent endpoint fields and exposes etlConfig to the editor manager. |
| adminSiteClient/AbstractChartEditor.ts | Adds editor-level support for an always-applied chart ETL config layer in parent/merge computations. |
PUT /charts/:id/etlConfig resolved the parent indicator from the chart's stale stored config, so when ETL re-points a chart at a new y-variable (e.g. dataset re-versioning) the rendered full inherited the OLD indicator's fields until a later recompute. Resolve the parent from the incoming config (existing full + this push's etlConfig) instead. Adds a vitest case covering the re-version scenario. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ETL layer is a separate chart_configs row (via charts.configIdETL), not a chart_configs.etlConfig column. Updates three comments that still referenced the old column. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
put/deleteChartsChartIdEtlConfig now compare the recomputed full config against the stored one (ignoring version/id). If nothing changed, the push is a no-op: no version bump, no chart revision, no lastEditedAt bump, no R2 re-upload, no static build. The ETL config row is still kept up to date. So --force re-runs, routine data refreshes, and bulk ETL runs no longer churn a chart's version/history; only real config changes (incl. variable re-versions) bump it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iling test The re-version test asserted that pushing an etlConfig with a new y-variable re-points the chart. It doesn't: `dimensions` is a REQUIRED_KEY in diffGrapherConfigs, so bootstrap/create dims stay in `patch` and override the new etlConfig dims on merge (the known "stale dimensions" limitation). The parent-from-incoming change didn't address that and could make the inherited parent inconsistent with the plotted variable, so revert it and drop the test. Re-versioning an ETL chart's variables needs the dimensions handling sorted out separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964b8eaf2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The no-op early return in putChartsChartIdEtlConfig (taken when an ETL push doesn't change the rendered `full`) skipped two writes it shouldn't: - The rediffed patch. When ETL adopts a field that was an admin override (e.g. pushes an etlConfig title matching the patch), rediff drops it from the patch but `full` is unchanged, so the early return left the stale entry in cc.patch. A later ETL push to that field was then masked as an admin override, so ETL could never update a field it had just taken ownership of. - A newly-supplied catalogPath, which for a chart that already has a configIdETL is only written past the early return, so a no-op re-push silently dropped it. Now both are persisted on the no-op path (without bumping version/revision/R2/ build, since nothing the reader sees changed). Adds regression tests for each. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On a dataset re-version, the etlConfig PUT endpoint resolved the chart's parent indicator from the pre-push full config, so the chart started plotting the new variable while still inheriting the old variable's fields (title, subtitle, note), and nothing later recomputed it. Resolve the parent from the dimensions the chart will plot after the push instead: the admin patch's override when present (patch is the top merge layer, so it wins), else the incoming ETL config's. This is the correct version of the reverted 6959579: that change resolved the parent purely from the incoming config, which broke when an admin had deliberately overridden the dimensions. Restore the re-versioning test dropped in 964b8ea (its dimensions half was fixed meanwhile by 34f23ff's no-op patch persistence) and add a test for the admin-override case. Verified live against staging-site-etl-chart-config before this fix: dimensions re-pointed correctly but the note stayed stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…intact For an ETL-managed chart, the grapher `dimensions` live only in the ETL config layer (they're deliberately stripped from the admin patch). The DELETE endpoint rebuilt the chart from the leftover layers, so detaching dropped the dimensions entirely and blanked the chart. Instead, re-diff the chart's current rendered config against the remaining parent and store that as the new patch: detaching changes nothing visible. Fields matching the indicator's config stay inherited; everything the ETL layer provided (dimensions, title, etc.) becomes a regular admin override. The render-neutral path must still persist the absorbed patch (same trap as the PUT no-op path fixed in 34f23ff). Update the DELETE test to the new semantics and add a test for the realistic adopted-chart state (patch without dimensions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Lets ETL author a chart's grapher config, while admins keep editing the same chart — and admin edits survive the next ETL run. This is Phase 1 of bringing charts and MDIMs closer together.
How
A chart's ETL config lives in its own
chart_configsrow, pointed to by a newcharts.configIdETL— exactly howvariables.grapherConfigIdETLalready works. The rendered config is:ETL writes the
etlConfigrow; admins writepatch. Different places, so they never collide, and an ETL re-push can't clobber an admin edit.We also add
charts.catalogPath(ETL's stable identity for a chart, like multi-dims already have).Additional notes
chart_configs, so the config table stays generic.PUT/DELETE /admin/api/charts/:id/etlConfig, modeled on the variable equivalents.configIdETLis NULL, sofullis unchanged.