🎉 (grapher) make charts inherit indicator-level settings / TAS-567

[sophiamersmann]

Summary

Implements chart config inheritance. Inheritance is opt-out, but disabled for all existing charts.

Database

• Adds variables.grapherConfigIdAdmin and variables.grapherConfigIdETL that point to rows in the chart_configs table
  • Moves configs from variables.grapherConfigETL to the chart_configs table (variables.grapherConfigAdmin is discarded)
  • For ETL-authored configs, fullConfig = patchConfig (since they don't have a parent)
  • For admin-authored configs, fullConfigAdmin = patchConfigETL + patchConfigAdmin
  • The merged config is thus either the admin's full config of the ETL's full config (in that order)
• Adds a charts.isInheritanceEnabled column (disabled by default)
• Adds a view charts_x_parents that lists every chart and the parent variable it may inherit from
  • The parent variable is the y-variable of a single y-indicator chart (scatter plots excluded)

Admin API

• Chart configs
  • /charts/:chartId.config.json returns the full config of a chart
  • /charts/:chartId.patchConfig.json returns the patch config (needed in admin)
  • /charts/:chartId.parent.json returns the parent variable id and config and whether inheritance is currently enabled (needed in the admin)
• A new query parameter has been added to the routes that save new or existing charts (/charts and /charts/:chartId)
  • /charts/:chartId?inheritance=enable saves a chart, with inheritance enabled
  • /charts/:chartId?inheritance=disable saved a chart, with inheritance disabled
  • If the inheritance parameter is not given then a chart is saved without changing the inheritance setting
  • (It's a bit odd to have query params for a POST/PUT route, no? But if we had standalone routes to enable/disable inheritance, then we'd need to make two API calls in the admin, which also means we'd be triggering baking twice (unless we dance around that somehow). Since both calls potentially update a chart's config, the database could also be in a weird intermediate state if the first API call goes through but the second fails. Instead of the query params, we could also add additional routes for updating charts with inheritance enabled/disabled, but seemed more complex than the query param to me...)
• Endpoints to edit grapherConfigETL
  • PUT /variables/:variableId/grapherConfigETL creates a new config or updates an existing one
  • DELETE /variables/:variableId/grapherConfigETL deletes an ETL-authored config
  • Note that updating the Grapher config of a variable updates all charts that inherit from a variable, and thus might trigger a bake

Grapher schema

• version and title are now optional
  • In practice, the version gets set automatically by the server if missing on save
  • In practice, charts should have a title (but indicator-level charts don't need one)
• $schema and dimensions are required
  • To know the $schema is esp. important for merging Grapher configs
  • $schema is also set to a constant value, the current schema version (i.e. the schema validator fails if the $schema is set to any other value) – it makes sense for now, but we might want to lift this restriction in the future
• Related: Grapher serialised details and topicIds, both were legacy and have been removed

Admin

• The chart editor has been refactored to work with different types of charts (e.g. normal Grapher charts and indicator charts)
  • The implementation for editing indicator-level charts is missing though; the resp. classes (<IndicatorChartEditorPage />, <IndicatorChartEditor />) are mostly empty for now and are not linked to
• The chart editor is now structured as follows:
  • The <ChartEditorPage /> fetches data, inits a ChartEditor instance and renders <ChartEditorView />, with itself as its manager
  • ChartEditor handles state and IO that is specific to "normal" Grapher charts; common state and functionality that likely every editable chart needs is inherited from AbstractChartEditor
  • IndicatorChartEditor, for example, will handle state and IO that is specific to indicator-level charts
• Config inheritance has been integrated into the chart editor
  • If inheritance is possible (i.e. the current config has a parent variable with a config), then a new Inheritance tab is shown
  • The Inheritance tab allows to toggle inheritance and prints the parent and full config
  • The patch config is sent to the server on save
  • Text fields with linked/unlinked default values (title, subtitle, etc) either use the parent value as a default (if it exists), or a value computed from metadata (doing this more thoroughly is part of the follow-up work)
  • The 'Data to show' section (where selectedEntityNames) is edited can also be linked to the parent selection
• I have removed a few pieces of code that haven't been working for some time:
  • Admin: "New chart" button on chart editor page doesn't work as expected #3875
  • Admin: Bind tab to the URL #3876

Caveats

• It's impossible to inherit dimensions from a parent indicator (since the dimensions are needed to figure out the parent indicator of a chart)
• The admin might load data a bit slower. The chart editor used to load all data at once. Now, ChartEditorPage loads all things related to the chart, and later ChartEditorView loads all things related to the admin (variables data, details)
• The colour section in the Customize tab doesn't currently work, see Colour selection is broken on scatterplots #3819

Testing

• Examples:
  • Chart without parent indicator: http://staging-site-inherit-indicator-settings/admin/charts/3375/edit
  • Chart with parent indicator that doesn't have a config: http://staging-site-inherit-indicator-settings/admin/charts/4711/edit
  • Chart with parent indicator that has a config: http://staging-site-inherit-indicator-settings/admin/charts/4408/edit

To do

• Make inheritance opt-out but disable inheritance for all existing charts

To do after merging

• ✨ Post grapherConfigETL update to Admin API etl#3084
• https://github.com/owid/automation/pull/13

[sophiamersmann]

• 🎉 (grapher) make charts inherit indicator-level settings / TAS-567 #3793 : 2 dependent PRs (#3827 , #3886 ) 👈
• 🔨 (grapher) let chart configs inherit default values / TAS-565 #3782 : 1 other dependent PR (#3890 )
• 🔨 (db) add chart_configs table / TAS-563 #3767 : 1 other dependent PR (#3743 )
• 🔨 Add wrangler.toml file #3846
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @sophiamersmann and the rest of your teammates on Graphite

[notion-workspace]

Make charts inherit indicator-level settings

[owidbot]

Quick links (staging server):

Site	Admin	Wizard

Login: ssh owid@staging-site-inherit-indicator-settings

• Site-screenshots: https://github.com/owid/site-screenshots/compare/inherit-indicator-settings
• SVG tester: https://github.com/owid/owid-grapher-svgs/compare/inherit-indicator-settings

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-08-20 10:04:48 UTC
Execution time: 1.22 seconds

[sophiamersmann]

Merge activity

• Sep 5, 4:50 AM EDT: @sophiamersmann started a stack merge that includes this pull request via Graphite.