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 18, 2024
1 parent 1fc853c commit fc31b87
Show file tree
Hide file tree
Showing 19 changed files with 195 additions and 116 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
4 changes: 2 additions & 2 deletions adminSiteClient/IndicatorChartEditorPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
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
Loading

0 comments on commit fc31b87

Please sign in to comment.