Skip to content

Commit

Permalink
Update grapher configs in R2 when variable config changes (#4096)
Browse files Browse the repository at this point in the history 
* Fix missing updates of configs in R2 after we update the configs in
  the DB on variable grapher config upsert/deletion in the API
* Update mdim view grapher configs on variable grapher config
  upsert/deletion
* Trigger baking if a published mdim config is created/updated
* Update API tests to include mdims
  • Loading branch information
@rakyi
rakyi authored Nov 1, 2024
1 parent 4cc4c95 commit fc61042
Show file tree
Hide file tree
Showing 5 changed files with 344 additions and 38 deletions.
89 changes: 69 additions & 20 deletions adminSiteServer/apiRouter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import {
BAKED_BASE_URL,
ADMIN_BASE_URL,
DATA_API_URL,
FEATURE_FLAGS,
} from "../settings/serverSettings.js"
import { FeatureFlagFeature } from "../settings/clientSettings.js"
import { expectInt, isValidSlug } from "../serverUtils/serverUtil.js"
import {
OldChartFieldList,
Expand All @@ -34,6 +36,7 @@ import {
updateGrapherConfigAdminOfVariable,
updateGrapherConfigETLOfVariable,
updateAllChartsThatInheritFromIndicator,
updateAllMultiDimViewsThatInheritFromIndicator,
getAllChartsForIndicator,
} from "../db/model/Variable.js"
import { updateExistingFullConfig } from "../db/model/ChartConfigs.js"
Expand Down Expand Up @@ -102,6 +105,7 @@ import {
parseChartConfig,
MultiDimDataPageConfigRaw,
R2GrapherConfigDirectory,
ChartConfigsTableName,
} from "@ourworldindata/types"
import { uuidv7 } from "uuidv7"
import {
Expand Down Expand Up @@ -182,6 +186,7 @@ import {
} from "./chartConfigR2Helpers.js"
import { fetchImagesFromDriveAndSyncToS3 } from "../db/model/Image.js"
import { createMultiDimConfig } from "./multiDim.js"
import { isMultiDimDataPagePublished } from "../db/model/MultiDimDataPage.js"

const apiRouter = new FunctionalRouter()

Expand Down Expand Up @@ -722,6 +727,23 @@ const saveGrapher = async (
}
}

async function updateGrapherConfigsInR2(
knex: db.KnexReadonlyTransaction,
updatedCharts: { chartConfigId: string; isPublished: boolean }[],
updatedMultiDimViews: { chartConfigId: string; isPublished: boolean }[]
) {
const idsToUpdate = [
...updatedCharts.filter(({ isPublished }) => isPublished),
...updatedMultiDimViews,
].map(({ chartConfigId }) => chartConfigId)
const builder = knex<DbRawChartConfig>(ChartConfigsTableName)
.select("id", "full", "fullMd5")
.whereIn("id", idsToUpdate)
for await (const { id, full, fullMd5 } of builder.stream()) {
await saveGrapherConfigToR2ByUUID(id, full, fullMd5)
}
}

getRouteWithROTransaction(apiRouter, "/charts.json", async (req, res, trx) => {
const limit = parseIntOrUndefined(req.query.limit as string) ?? 10000
const charts = await db.knexRaw<OldChartFieldList>(
Expand Down Expand Up @@ -1155,6 +1177,15 @@ putRouteWithRWTransaction(
}
const rawConfig = req.body as MultiDimDataPageConfigRaw
const id = await createMultiDimConfig(trx, slug, rawConfig)
if (
FEATURE_FLAGS.has(FeatureFlagFeature.MultiDimDataPage) &&
(await isMultiDimDataPagePublished(trx, id))
) {
await triggerStaticBuild(
res.locals.user,
`Publishing multidimensional chart ${slug}`
)
}
return { success: true, id }
}
)
Expand Down Expand Up @@ -1647,11 +1678,13 @@ putRouteWithRWTransaction(
throw new JsonError(`Variable with id ${variableId} not found`, 500)
}

const { savedPatch, updatedCharts } =
const { savedPatch, updatedCharts, updatedMultiDimViews } =
await updateGrapherConfigETLOfVariable(trx, variable, validConfig)

// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews)
const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews]

if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) {
await triggerStaticBuild(
res.locals.user,
`Updating ETL config for variable ${variableId}`
Expand Down Expand Up @@ -1708,18 +1741,25 @@ deleteRouteWithRWTransaction(
})
}

// update all charts that inherit from the indicator
const updates = {
patchConfigAdmin: variable.admin?.patchConfig,
updatedAt: now,
}
const updatedCharts = await updateAllChartsThatInheritFromIndicator(
trx,
variableId,
{
patchConfigAdmin: variable.admin?.patchConfig,
updatedAt: now,
}
updates
)
const updatedMultiDimViews =
await updateAllMultiDimViewsThatInheritFromIndicator(
trx,
variableId,
updates
)
await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews)
const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews]

// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) {
await triggerStaticBuild(
res.locals.user,
`Updating ETL config for variable ${variableId}`
Expand Down Expand Up @@ -1752,11 +1792,13 @@ putRouteWithRWTransaction(
throw new JsonError(`Variable with id ${variableId} not found`, 500)
}

const { savedPatch, updatedCharts } =
const { savedPatch, updatedCharts, updatedMultiDimViews } =
await updateGrapherConfigAdminOfVariable(trx, variable, validConfig)

// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews)
const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews]

if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) {
await triggerStaticBuild(
res.locals.user,
`Updating admin-authored config for variable ${variableId}`
Expand Down Expand Up @@ -1804,18 +1846,25 @@ deleteRouteWithRWTransaction(
[variable.admin.configId]
)

// update all charts that inherit from the indicator
const updates = {
patchConfigETL: variable.etl?.patchConfig,
updatedAt: now,
}
const updatedCharts = await updateAllChartsThatInheritFromIndicator(
trx,
variableId,
{
patchConfigETL: variable.etl?.patchConfig,
updatedAt: now,
}
updates
)
const updatedMultiDimViews =
await updateAllMultiDimViewsThatInheritFromIndicator(
trx,
variableId,
updates
)
await updateGrapherConfigsInR2(trx, updatedCharts, updatedMultiDimViews)
const allUpdatedConfigs = [...updatedCharts, ...updatedMultiDimViews]

// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
if (allUpdatedConfigs.some(({ isPublished }) => isPublished)) {
await triggerStaticBuild(
res.locals.user,
`Updating admin-authored config for variable ${variableId}`
Expand Down
127 changes: 127 additions & 0 deletions adminSiteServer/app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ import {
ChartConfigsTableName,
ChartsTableName,
DatasetsTableName,
MultiDimDataPagesTableName,
MultiDimXChartConfigsTableName,
VariablesTableName,
} from "@ourworldindata/types"
import path from "path"
Expand Down Expand Up @@ -363,6 +365,67 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
},
],
}
const testMultiDimConfig = {
grapherConfigSchema:
"https://files.ourworldindata.org/schemas/grapher-schema.005.json",
title: {
title: "Energy use",
titleVariant: "by energy source",
},
views: [
{
config: { title: "Total energy use" },
dimensions: {
source: "all",
metric: "total",
},
indicators: {
y: variableId,
},
},
{
dimensions: {
metric: "per_capita",
source: "all",
},
indicators: {
y: otherVariableId,
},
},
],
dimensions: [
{
name: "Energy source",
slug: "source",
choices: [
{
name: "All sources",
slug: "all",
group: "Aggregates",
description: "Total energy use",
},
],
},
{
name: "Metric",
slug: "metric",
choices: [
{
name: "Total consumption",
slug: "total",
description:
"The amount of energy consumed nationally per year",
},
{
name: "Consumption per capita",
slug: "per_capita",
description:
"The average amount of energy each person consumes per year",
},
],
},
],
}

beforeEach(async () => {
await testKnexInstance!(DatasetsTableName).insert([dummyDataset])
Expand Down Expand Up @@ -432,6 +495,70 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
...testVariableConfigAdmin,
})

// create mdim config that uses both of the variables
await makeRequestAgainstAdminApi({
method: "PUT",
path: "/multi-dim/energy",
body: JSON.stringify(testMultiDimConfig),
})
const mdim = await testKnexInstance!(MultiDimDataPagesTableName).first()
expect(mdim.slug).toBe("energy")
const savedMdimConfig = JSON.parse(mdim.config)
// variableId should be normalized to an array
expect(savedMdimConfig.views[0].indicators.y).toBeInstanceOf(Array)

const [mdxcc1, mdxcc2] = await testKnexInstance!(
MultiDimXChartConfigsTableName
)
expect(mdxcc1.multiDimId).toBe(mdim.id)
expect(mdxcc1.viewId).toBe("total__all")
expect(mdxcc1.variableId).toBe(variableId)
expect(mdxcc2.multiDimId).toBe(mdim.id)
expect(mdxcc2.viewId).toBe("per_capita__all")
expect(mdxcc2.variableId).toBe(otherVariableId)

// view config should override the variable config
const expectedMergedViewConfig = {
...mergedGrapherConfig,
title: "Total energy use",
selectedEntityNames: [], // mdims define their own default entities
}
const fullViewConfig1 = await testKnexInstance!(ChartConfigsTableName)
.where("id", mdxcc1.chartConfigId)
.first()
expect(JSON.parse(fullViewConfig1.full)).toEqual(
expectedMergedViewConfig
)

// update the admin-authored config for the variable
await makeRequestAgainstAdminApi({
method: "PUT",
path: `/variables/${variableId}/grapherConfigAdmin`,
body: JSON.stringify({
...testVariableConfigAdmin,
subtitle: "Newly updated subtitle",
}),
})
const expectedMergedViewConfigUpdated = {
...expectedMergedViewConfig,
subtitle: "Newly updated subtitle",
}
const fullViewConfig1Updated = await testKnexInstance!(
ChartConfigsTableName
)
.where("id", mdxcc1.chartConfigId)
.first()
expect(JSON.parse(fullViewConfig1Updated.full)).toEqual(
expectedMergedViewConfigUpdated
)

// clean-up the mdim tables
await testKnexInstance!(MultiDimXChartConfigsTableName).truncate()
await testKnexInstance!(MultiDimDataPagesTableName).delete()
await testKnexInstance!(ChartConfigsTableName)
.whereIn("id", [mdxcc1.chartConfigId, mdxcc2.chartConfigId])
.delete()

// delete the admin-authored grapher config we just added
// and verify that the merged config is now the same as the ETL config
await makeRequestAgainstAdminApi({
Expand Down
11 changes: 11 additions & 0 deletions db/model/MultiDimDataPage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ export async function upsertMultiDimDataPage(
return result[0]
}

export async function isMultiDimDataPagePublished(
knex: KnexReadonlyTransaction,
id: number
): Promise<boolean> {
const result = await knex(MultiDimDataPagesTableName)
.select(knex.raw("1"))
.where({ id, published: true })
.first()
return Boolean(result)
}

const createOnlyPublishedFilter = (
onlyPublished: boolean
): Record<string, any> => (onlyPublished ? { published: true } : {})
Expand Down
Loading

0 comments on commit fc61042

Please sign in to comment.