🔨 remove default layer from chart configs / TAS-675

adminSiteClient/AbstractChartEditor.ts

@@ -80,9 +80,19 @@ export abstract class AbstractChartEditor<
         return mergeGrapherConfigs(parentConfig ?? {}, patchConfig)
     }
 
-    /** live-updating full config */
+    /** live-updating config */
+    @computed get liveConfig(): GrapherInterface {
+        return this.grapher.object
+    }
+
+    @computed get liveConfigWithDefaults(): GrapherInterface {
+        return mergeGrapherConfigs(defaultGrapherConfig, this.liveConfig)
+    }
+
+    /** patch config merged with parent config */
     @computed get fullConfig(): GrapherInterface {
-        return mergeGrapherConfigs(defaultGrapherConfig, this.grapher.object)
+        if (!this.activeParentConfig) return this.liveConfig
+        return mergeGrapherConfigs(this.activeParentConfig, this.liveConfig)
     }
 
     /** parent config currently applied to grapher */
@@ -103,7 +113,7 @@ export abstract class AbstractChartEditor<
     /** patch config of the chart that is written to the db on save */
     @computed get patchConfig(): GrapherInterface {
         return diffGrapherConfigs(
-            this.fullConfig,
+            this.liveConfigWithDefaults,
             this.activeParentConfigWithDefaults ?? defaultGrapherConfig
         )
     }

adminSiteClient/ChartEditor.ts

@@ -75,7 +75,7 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
 
     /** parent variable id, derived from the config */
     @computed get parentVariableId(): number | undefined {
-        return getParentVariableIdFromChartConfig(this.fullConfig)
+        return getParentVariableIdFromChartConfig(this.liveConfig)
     }
 
     @computed get availableTabs(): EditorTab[] {

adminSiteClient/IndicatorChartEditorPage.tsx

@@ -11,7 +11,7 @@ import {
     IndicatorChartEditorManager,
     type Chart,
 } from "./IndicatorChartEditor.js"
-import { defaultGrapherConfig } from "@ourworldindata/grapher"
+import { latestGrapherConfigSchema } from "@ourworldindata/grapher"
 
 @observer
 export class IndicatorChartEditorPage
@@ -40,7 +40,7 @@ export class IndicatorChartEditorPage
         )
         if (isEmpty(config)) {
             this.patchConfig = {
-                $schema: defaultGrapherConfig.$schema,
+                $schema: latestGrapherConfigSchema,
                 dimensions: [{ variableId, property: DimensionProperty.y }],
             }
             this.isNewGrapher = true

adminSiteServer/apiRouter.ts

@@ -109,7 +109,6 @@ import {
 } from "@ourworldindata/types"
 import { uuidv7 } from "uuidv7"
 import {
-    defaultGrapherConfig,
     migrateGrapherConfigToLatestVersion,
     getVariableDataRoute,
     getVariableMetadataRoute,
@@ -328,14 +327,10 @@ const saveNewChart = async (
     const parent = shouldInherit
         ? await getParentByChartConfig(knex, config)
         : undefined
-    const fullParentConfig = mergeGrapherConfigs(
-        defaultGrapherConfig,
-        parent?.config ?? {}
-    )
 
     // compute patch and full configs
-    const patchConfig = diffGrapherConfigs(config, fullParentConfig)
-    const fullConfig = mergeGrapherConfigs(fullParentConfig, patchConfig)
+    const patchConfig = diffGrapherConfigs(config, parent?.config ?? {})
+    const fullConfig = mergeGrapherConfigs(parent?.config ?? {}, patchConfig)
     const fullConfigStringified = serializeChartConfig(fullConfig)
 
     // insert patch & full configs into the chart_configs table
@@ -424,14 +419,10 @@ const updateExistingChart = async (
     const parent = shouldInherit
         ? await getParentByChartConfig(knex, config)
         : undefined
-    const fullParentConfig = mergeGrapherConfigs(
-        defaultGrapherConfig,
-        parent?.config ?? {}
-    )
 
     // compute patch and full configs
-    const patchConfig = diffGrapherConfigs(config, fullParentConfig)
-    const fullConfig = mergeGrapherConfigs(fullParentConfig, patchConfig)
+    const patchConfig = diffGrapherConfigs(config, parent?.config ?? {})
+    const fullConfig = mergeGrapherConfigs(parent?.config ?? {}, patchConfig)
     const fullConfigStringified = serializeChartConfig(fullConfig)
 
     const chartConfigId = await db.knexRawFirst<Pick<DbPlainChart, "configId">>(

adminSiteServer/app.test.tsx

@@ -249,36 +249,22 @@ describe("OwidAdminApp", () => {
         )?.config
         expect(parentConfig).toBeUndefined()
 
-        // fetch the full config and verify it's merged with the default
+        // fetch the full config and verify that id, version and isPublished are added
         const fullConfig = await fetchJsonFromAdminApi(
             `/charts/${chartId}.config.json`
         )
-        expect(fullConfig).toHaveProperty(
-            "$schema",
-            "https://files.ourworldindata.org/schemas/grapher-schema.005.json"
-        )
-        expect(fullConfig).toHaveProperty("id", chartId) // must match the db id
-        expect(fullConfig).toHaveProperty("version", 1) // automatically added
-        expect(fullConfig).toHaveProperty("isPublished", false) // automatically added
-        expect(fullConfig).toHaveProperty("slug", "test-chart")
-        expect(fullConfig).toHaveProperty("title", "Test chart")
-        expect(fullConfig).toHaveProperty("type", "LineChart") // default property
-        expect(fullConfig).toHaveProperty("tab", "chart") // default property
+        expect(fullConfig).toEqual({
+            ...testChartConfig,
+            id: chartId, // must match the db id
+            version: 1, // automatically added
+            isPublished: false, // automatically added
+        })
 
-        // fetch the patch config and verify it's diffed correctly
+        // fetch the patch config and verify it's identical to the full config
         const patchConfig = await fetchJsonFromAdminApi(
             `/charts/${chartId}.patchConfig.json`
         )
-        expect(patchConfig).toEqual({
-            $schema:
-                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
-            id: chartId,
-            version: 1,
-            isPublished: false,
-            slug: "test-chart",
-            title: "Test chart",
-            // note that the type is not included
-        })
+        expect(patchConfig).toEqual(fullConfig)
     })
 
     it("should be able to create a GDoc article", async () => {
@@ -621,19 +607,27 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         expect(parentConfig).toEqual(mergedGrapherConfig)
 
         // fetch the full config of the chart and verify that it's been merged
-        // with the indicator config and the default config
+        // with the indicator config
         const fullConfig = await fetchJsonFromAdminApi(
             `/charts/${chartId}.config.json`
         )
-        expect(fullConfig).toHaveProperty("slug", "test-chart")
-        expect(fullConfig).toHaveProperty("title", "Test chart")
-        expect(fullConfig).toHaveProperty("type", "Marimekko")
-        expect(fullConfig).toHaveProperty("selectedEntityNames", [])
-        expect(fullConfig).toHaveProperty("hideRelativeToggle", false)
-        expect(fullConfig).toHaveProperty("note", "Indicator note") // inherited from variable
-        expect(fullConfig).toHaveProperty("hasMapTab", true) // inherited from variable
-        expect(fullConfig).toHaveProperty("subtitle", "Admin subtitle") // inherited from variable
-        expect(fullConfig).toHaveProperty("tab", "chart") // default value
+
+        expect(fullConfig).toEqual({
+            $schema:
+                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
+            id: chartId,
+            isPublished: false,
+            version: 1,
+            slug: "test-chart",
+            title: "Test chart",
+            type: "Marimekko",
+            selectedEntityNames: [],
+            hideRelativeToggle: false,
+            dimensions: [{ variableId, property: "y" }],
+            subtitle: "Admin subtitle", // inherited from variable
+            note: "Indicator note", // inherited from variable
+            hasMapTab: true, // inherited from variable
+        })
 
         // fetch the patch config and verify it's diffed correctly
         const patchConfig = await fetchJsonFromAdminApi(
@@ -649,12 +643,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
             title: "Test chart",
             type: "Marimekko",
             selectedEntityNames: [],
-            dimensions: [
-                {
-                    variableId,
-                    property: "y",
-                },
-            ],
+            dimensions: [{ variableId, property: "y" }],
             // note that `hideRelativeToggle` is not included
         })
 
@@ -681,15 +670,18 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
         const fullConfigAfterDelete = await fetchJsonFromAdminApi(
             `/charts/${chartId}.config.json`
         )
-        // was inherited from variable, should be unset now
-        expect(fullConfigAfterDelete).not.toHaveProperty("note")
-        expect(fullConfigAfterDelete).not.toHaveProperty("subtitle")
-        // was inherited from variable, is now inherited from the default config
-        expect(fullConfigAfterDelete).toHaveProperty("hasMapTab", false)
-        // was inherited from variable, is now inherited from the default config
-        // (although the "original" chart config sets hideRelativeToggle to false)
-        expect(fullConfigAfterDelete).toHaveProperty("hideRelativeToggle", true)
-        expect(fullConfigAfterDelete).toHaveProperty("tab", "chart") // default value
+        expect(fullConfigAfterDelete).toEqual({
+            $schema:
+                "https://files.ourworldindata.org/schemas/grapher-schema.005.json",
+            id: chartId,
+            version: 1,
+            isPublished: false,
+            dimensions: [{ property: "y", variableId: 1 }],
+            selectedEntityNames: [],
+            slug: "test-chart",
+            title: "Test chart",
+            type: "Marimekko",
+        })
 
         // fetch the patch config and verify it's diffed correctly
         const patchConfigAfterDelete = await fetchJsonFromAdminApi(
@@ -736,7 +728,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
             } else {
                 expect(chartRow.isInheritanceEnabled).toBeFalsy()
                 expect(fullConfig).not.toHaveProperty("note")
-                expect(fullConfig).toHaveProperty("hasMapTab", false)
+                expect(fullConfig).not.toHaveProperty("hasMapTab")
             }
         }
 

db/migration/1730455806132-RemoveDefaultConfigLayer.ts

@@ -0,0 +1,113 @@
+import { defaultGrapherConfig } from "@ourworldindata/grapher"
+import { mergeGrapherConfigs } from "@ourworldindata/utils"
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class RemoveDefaultConfigLayer1730455806132
+    implements MigrationInterface
+{
+    private async recomputeFullChartConfigs(
+        queryRunner: QueryRunner,
+        { useDefaultLayer }: { useDefaultLayer: boolean }
+    ): Promise<void> {
+        const charts = await queryRunner.query(`
+             -- sql
+             SELECT
+                 cc.id AS configId,
+                 cc.patch AS patchConfig,
+                 cc_etl.patch AS etlConfig,
+                 cc_admin.patch AS adminConfig,
+                 c.isInheritanceEnabled
+             FROM charts c
+             JOIN chart_configs cc ON cc.id = c.configId
+             LEFT JOIN charts_x_parents cxp ON cxp.chartId = c.id
+             LEFT JOIN variables v ON v.id = cxp.variableId
+             LEFT JOIN chart_configs cc_etl ON cc_etl.id = v.grapherConfigIdETL
+             LEFT JOIN chart_configs cc_admin ON cc_admin.id = v.grapherConfigIdAdmin
+         `)
+        for (const chart of charts) {
+            const patchConfig = JSON.parse(chart.patchConfig)
+
+            const etlConfig = chart.etlConfig ? JSON.parse(chart.etlConfig) : {}
+            const adminConfig = chart.adminConfig
+                ? JSON.parse(chart.adminConfig)
+                : {}
+
+            const fullConfig = mergeGrapherConfigs(
+                useDefaultLayer ? defaultGrapherConfig : {},
+                chart.isInheritanceEnabled ? etlConfig : {},
+                chart.isInheritanceEnabled ? adminConfig : {},
+                patchConfig
+            )
+
+            await queryRunner.query(
+                `
+                    -- sql
+                    UPDATE chart_configs cc
+                    SET cc.full = ?
+                    WHERE cc.id = ?
+                `,
+                [JSON.stringify(fullConfig), chart.configId]
+            )
+        }
+    }
+
+    private async updateChartsXParentsView(
+        queryRunner: QueryRunner
+    ): Promise<void> {
+        // identical to the current view definition,
+        // but uses `COALESCE(cc.full ->> '$.type', 'LineChart')`
+        // instead of `cc.full ->> '$.type'` since the full config
+        // might not have a type
+        await queryRunner.query(`-- sql
+          ALTER VIEW charts_x_parents AS (
+            WITH y_dimensions AS (
+              SELECT
+                *
+              FROM
+                chart_dimensions
+              WHERE
+                property = 'y'
+            ),
+            single_y_indicator_charts AS (
+              SELECT
+                c.id as chartId,
+                cc.patch as patchConfig,
+                -- should only be one
+                max(yd.variableId) as variableId
+              FROM
+                charts c
+                JOIN chart_configs cc ON cc.id = c.configId
+                JOIN y_dimensions yd ON c.id = yd.chartId
+              WHERE
+                -- scatter plots can't inherit settings
+                COALESCE(cc.full ->> '$.type', 'LineChart') != 'ScatterPlot'
+              GROUP BY
+                c.id
+              HAVING
+                -- restrict to single y-variable charts
+                COUNT(distinct yd.variableId) = 1
+            )
+            SELECT
+              variableId,
+              chartId
+            FROM
+              single_y_indicator_charts
+            ORDER BY
+              variableId
+          )
+        `)
+    }
+
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        await this.recomputeFullChartConfigs(queryRunner, {
+            useDefaultLayer: false,
+        })
+        await this.updateChartsXParentsView(queryRunner)
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await this.recomputeFullChartConfigs(queryRunner, {
+            useDefaultLayer: true,
+        })
+    }
+}

db/model/Chart.ts

@@ -567,7 +567,7 @@ export const getMostViewedGrapherIdsByChartType = async (
             JOIN chart_configs cc ON slug = SUBSTRING_INDEX(a.url, '/', -1)
             JOIN charts c ON c.configId = cc.id
             WHERE a.url LIKE "https://ourworldindata.org/grapher/%"
-                AND cc.full ->> "$.type" = ?
+                AND COALESCE(cc.full ->> "$.type", 'LineChart') = ?
                 AND cc.full ->> "$.isPublished" = "true"
                 and (cc.full ->> "$.hasChartTab" = "true" or cc.full ->> "$.hasChartTab" is null)
             ORDER BY a.views_365d DESC

db/model/Variable.ts

@@ -11,7 +11,6 @@ import {
 import {
     getVariableDataRoute,
     getVariableMetadataRoute,
-    defaultGrapherConfig,
 } from "@ourworldindata/grapher"
 import pl from "nodejs-polars"
 import { uuidv7 } from "uuidv7"
@@ -271,7 +270,6 @@ export async function updateAllChartsThatInheritFromIndicator(
 
     for (const chart of inheritingCharts) {
         const fullConfig = mergeGrapherConfigs(
-            defaultGrapherConfig,
             patchConfigETL ?? {},
             patchConfigAdmin ?? {},
             chart.patchConfig

devTools/svgTester/dump-data.ts

@@ -1,8 +1,6 @@
 #! /usr/bin/env node
 
 import { getPublishedGraphersBySlug } from "../../baker/GrapherImageBaker.js"
-import { defaultGrapherConfig } from "@ourworldindata/grapher"
-import { diffGrapherConfigs } from "@ourworldindata/utils"
 
 import { TransactionCloseMode, knexReadonlyTransaction } from "../../db/db.js"
 
@@ -26,11 +24,7 @@ async function main(parsedArgs: parseArgs.ParsedArgs) {
         const allGraphers = [...graphersBySlug.values()]
         const saveJobs: utils.SaveGrapherSchemaAndDataJob[] = allGraphers.map(
             (grapher) => {
-                // since we're not baking defaults, we also exclude them here
-                return {
-                    config: diffGrapherConfigs(grapher, defaultGrapherConfig),
-                    outDir,
-                }
+                return { config: grapher, outDir }
             }
         )