diff --git a/adminSiteClient/AbstractChartEditor.ts b/adminSiteClient/AbstractChartEditor.ts index 0fda11075f3..26e3b6ea7f7 100644 --- a/adminSiteClient/AbstractChartEditor.ts +++ b/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 ) } diff --git a/adminSiteClient/ChartEditor.ts b/adminSiteClient/ChartEditor.ts index 6c9e7809eaf..c9a7bbca7b5 100644 --- a/adminSiteClient/ChartEditor.ts +++ b/adminSiteClient/ChartEditor.ts @@ -75,7 +75,7 @@ export class ChartEditor extends AbstractChartEditor { /** parent variable id, derived from the config */ @computed get parentVariableId(): number | undefined { - return getParentVariableIdFromChartConfig(this.fullConfig) + return getParentVariableIdFromChartConfig(this.liveConfig) } @computed get availableTabs(): EditorTab[] { diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index 231a2314cda..a8fd13b3e57 100644 --- a/adminSiteServer/apiRouter.ts +++ b/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>( diff --git a/adminSiteServer/app.test.tsx b/adminSiteServer/app.test.tsx index 4062e43d3c8..3d73df7a504 100644 --- a/adminSiteServer/app.test.tsx +++ b/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 () => { @@ -620,19 +606,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( @@ -648,12 +642,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 }) @@ -680,15 +669,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( @@ -735,7 +727,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") } } diff --git a/db/migration/1730455806132-RemoveDefaultConfigLayer.ts b/db/migration/1730455806132-RemoveDefaultConfigLayer.ts new file mode 100644 index 00000000000..5f4a566f080 --- /dev/null +++ b/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 { + 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 { + // 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 { + await this.recomputeFullChartConfigs(queryRunner, { + useDefaultLayer: false, + }) + await this.updateChartsXParentsView(queryRunner) + } + + public async down(queryRunner: QueryRunner): Promise { + await this.recomputeFullChartConfigs(queryRunner, { + useDefaultLayer: true, + }) + } +} diff --git a/db/model/Chart.ts b/db/model/Chart.ts index 637f3f8ccaf..1f44e590f12 100644 --- a/db/model/Chart.ts +++ b/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 diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 14a3406f66a..7dd6130a28e 100644 --- a/db/model/Variable.ts +++ b/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 diff --git a/devTools/svgTester/dump-data.ts b/devTools/svgTester/dump-data.ts index 86bb43174a0..49a26ae98c5 100644 --- a/devTools/svgTester/dump-data.ts +++ b/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 } } ) diff --git a/packages/@ourworldindata/grapher/src/schema/README.md b/packages/@ourworldindata/grapher/src/schema/README.md index 2d6ae518365..dfee8e758e2 100644 --- a/packages/@ourworldindata/grapher/src/schema/README.md +++ b/packages/@ourworldindata/grapher/src/schema/README.md @@ -6,7 +6,7 @@ The schema is versioned. There is one yaml file with a version number. For nonbr edit the yaml file as is. A github action will then generate a .latest.yaml and two json files (one .latest.json and one with the version number.json) and upload them to S3 so they can be accessed publicly. -If you update the default value of an existing property or you add a new property with a default value, make sure to regenerate the default object from the schema and save it to `defaultGrapherConfig.ts` (see below). You should also write a migration to update the `chart_configs.full` column in the database for all stand-alone charts. +If you update the default value of an existing property or you add a new property with a default value, make sure to regenerate the default object from the schema and save it to `defaultGrapherConfig.ts` (see below). Breaking changes should be done by renaming the schema file to an increased version number. Make sure to also rename the authorative url inside the schema file (the "$id" field at the top level) to point to the new version number json. Then write the migrations from the last to diff --git a/packages/@ourworldindata/utils/src/Util.ts b/packages/@ourworldindata/utils/src/Util.ts index 45bcdfd4263..60209959c34 100644 --- a/packages/@ourworldindata/utils/src/Util.ts +++ b/packages/@ourworldindata/utils/src/Util.ts @@ -1968,7 +1968,7 @@ export function traverseObjects>( } export function getParentVariableIdFromChartConfig( - config: GrapherInterface // could be a patch config + config: GrapherInterface ): number | undefined { const { type = ChartTypeName.LineChart, dimensions } = config diff --git a/site/DataPageV2.tsx b/site/DataPageV2.tsx index 7133cd00653..e3c4814ca76 100644 --- a/site/DataPageV2.tsx +++ b/site/DataPageV2.tsx @@ -1,5 +1,4 @@ import { - defaultGrapherConfig, getVariableDataRoute, getVariableMetadataRoute, GrapherProgrammaticInterface, @@ -16,7 +15,6 @@ import { GrapherInterface, ImageMetadata, Url, - diffGrapherConfigs, } from "@ourworldindata/utils" import { MarkdownTextWrap } from "@ourworldindata/components" import React from "react" @@ -105,12 +103,6 @@ export const DataPageV2 = (props: { dataApiUrl: DATA_API_URL, } - // We bake the Grapher config without defaults - const grapherConfigToBake = diffGrapherConfigs( - grapherConfig, - defaultGrapherConfig - ) - // Only embed the tags that are actually used by the datapage, instead of the complete JSON object with ~240 properties const minimalTagToSlugMap = pick( tagToSlugMap, @@ -190,7 +182,7 @@ export const DataPageV2 = (props: {