🐛 (explorer) merge configs correctly

[sophiamersmann]

Problem

Explorers incorrectly merge (partial) Grapher configs with explorer-level Grapher settings.

We naively merge configs like this

{
   ...partialGrapherConfig,
   ...explorerProgram.grapherConfigOnlyGrapherProps
}

where partialGrapherConfig is the indicator config (of type GrapherInterface), but explorerProgram.grapherConfigOnlyGrapherProps is of type ExplorerGrapherInterface that includes explorer-flavoured flat config fields, like yAxisMin. Overwriting the y-axis min of the partial Grapher config, which is given as yAxis.min, thus won't work.

Bug

I don't know of an example in the wild where this currently leads to a problem, but you can fabricate a bug locally by following these steps:

Set yAxis.min of an indicator that is used in the conflict explorer by running

update chart_configs cc
join variables v on cc.id = v.grapherConfigIdETL
set
  cc.patch = JSON_MERGE_PATCH(cc.patch, '{"yAxis":{"min": 1}}'),
  cc.full = JSON_MERGE_PATCH(cc.full, '{"yAxis":{"min": 1}}')
where v.id = 985431

against your local database.

Navigate to this view of the conflict explorer. Note that

// the partial grapher config sets y-axis min to 1
explorer.partialGrapherConfigsByVariableId.get(985431).yAxis.min // = 1

// the explorer overwrites this by setting yAxis.min to 0
explorer.explorerProgram.grapherConfig.yAxisMin // = 0

// the y-axis min being used in Grapher is 1, but should be 0
// i.e. the explorer settings didn't successfully overwrite the indicator settings
grapher.yAxis.min // = 1, but should be 0

I ran the same query against the database on staging. You'll see that on staging Grapher's y-axis min is correctly set to 0 (staging).

Solution

Before the explorer's Grapher config gets merged with the partial config, we translate explorer-specific fields to valid GrapherInterface fields, e.g. yAxisMin is translated to { yAxis: { min: ... } }. This is defined by the toGrapherObject function of an explorer cell definition.

The toGrapherObject function is conceptually similar to the parse function. Alternatively, I could have reworked the parse function so that the given key/value pair is always parsed as a valid GrapherInterface object. Maybe that would have been the cleaner solution? I'm not sure. In the end, I went with the toGrapherObject function because I thought it would be useful to keep both Grapher row representations around:

• explorerGrapherConfig: explorer-flavoured settings that directly map to a Grapher row
• grapherConfig: corresponding chart config that is guaranteed to be a valid GrapherInterface

Testing

Since I don't know of any bugs in the wild (doesn't mean they don't exist though), these changes should not result in any visual changes.

[sophiamersmann]

• 🔨 (grapher) turn related questions into an object #4108
• 🔨 remove default layer from chart configs #4103
• ✨ (line chart) set y-axis min default to 0 #4080
• 🐛 (explorer) merge configs correctly #4091 👈
• ✨ log explorer views to google analytics #4098
• ✨ log grapher views to google analytics #4076
• master

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

Join @sophiamersmann and the rest of your teammates on Graphite

[owidbot]

Quick links (staging server):

Site	Admin	Wizard

Login: ssh owid@staging-site-fix-explorer-config-merge

• Site-screenshots: https://github.com/owid/site-screenshots/compare/fix-explorer-config-merge
• SVG tester: https://github.com/owid/owid-grapher-svgs/compare/fix-explorer-config-merge

SVG tester:

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

Edited: 2024-10-30 12:03:55 UTC
Execution time: 1.15 seconds