Skip to content

Commit

Permalink
🔨 remove default layer from chart configs
Browse files Browse the repository at this point in the history
  • Loading branch information
sophiamersmann committed Nov 11, 2024
1 parent b6436c6 commit 3c89b72
Show file tree
Hide file tree
Showing 13 changed files with 180 additions and 105 deletions.
16 changes: 13 additions & 3 deletions adminSiteClient/AbstractChartEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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
)
}
Expand Down
2 changes: 1 addition & 1 deletion adminSiteClient/ChartEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] {
Expand Down
17 changes: 4 additions & 13 deletions adminSiteServer/apiRouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ import {
} from "@ourworldindata/types"
import { uuidv7 } from "uuidv7"
import {
defaultGrapherConfig,
migrateGrapherConfigToLatestVersion,
getVariableDataRoute,
getVariableMetadataRoute,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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">>(
Expand Down
90 changes: 41 additions & 49 deletions adminSiteServer/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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(
Expand All @@ -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
})

Expand All @@ -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(
Expand Down Expand Up @@ -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")
}
}

Expand Down
113 changes: 113 additions & 0 deletions db/migration/1730455806132-RemoveDefaultConfigLayer.ts
Original file line number Diff line number Diff line change
@@ -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,
})
}
}
2 changes: 1 addition & 1 deletion db/model/Chart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions db/model/Variable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
import {
getVariableDataRoute,
getVariableMetadataRoute,
defaultGrapherConfig,
} from "@ourworldindata/grapher"
import pl from "nodejs-polars"
import { uuidv7 } from "uuidv7"
Expand Down Expand Up @@ -271,7 +270,6 @@ export async function updateAllChartsThatInheritFromIndicator(

for (const chart of inheritingCharts) {
const fullConfig = mergeGrapherConfigs(
defaultGrapherConfig,
patchConfigETL ?? {},
patchConfigAdmin ?? {},
chart.patchConfig
Expand Down
8 changes: 1 addition & 7 deletions devTools/svgTester/dump-data.ts
Original file line number Diff line number Diff line change
@@ -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"

Expand All @@ -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 }
}
)

Expand Down
2 changes: 1 addition & 1 deletion packages/@ourworldindata/grapher/src/schema/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/@ourworldindata/utils/src/Util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1964,7 +1964,7 @@ export function traverseObjects<T extends Record<string, any>>(
}

export function getParentVariableIdFromChartConfig(
config: GrapherInterface // could be a patch config
config: GrapherInterface
): number | undefined {
const { type = ChartTypeName.LineChart, dimensions } = config

Expand Down
Loading

0 comments on commit 3c89b72

Please sign in to comment.