Skip to content

Commit

Permalink
cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@sophiamersmann
sophiamersmann committed Aug 15, 2024
1 parent d20b90e commit bab068e
Show file tree
Hide file tree
Showing 12 changed files with 104 additions and 150 deletions.
12 changes: 4 additions & 8 deletions adminSiteClient/VariableEditPage.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ interface VariablePageData
extends Omit<OwidVariableWithDataAndSource, "source"> {
datasetNamespace: string
charts: ChartListItem[]
grapherConfig: GrapherInterface | undefined // admin+ETL merged
grapherConfigETL: GrapherInterface | undefined
source: { id: number; name: string }
origins: OwidOrigin[]
Expand Down Expand Up @@ -124,7 +123,6 @@ class VariableEditable
@observer
class VariableEditor extends React.Component<{
variable: VariablePageData
grapherConfigETL?: GrapherInterface
}> {
@observable newVariable!: VariableEditable
@observable isDeleted: boolean = false
Expand All @@ -150,12 +148,8 @@ class VariableEditor extends React.Component<{
)
}

@computed get grapherConfigETL(): GrapherInterface | undefined {
return this.props.grapherConfigETL
}

render() {
const { variable, grapherConfigETL } = this.props
const { variable } = this.props
const { newVariable, isV2MetadataVariable } = this

const pathFragments = variable.catalogPath
Expand Down Expand Up @@ -413,7 +407,9 @@ class VariableEditor extends React.Component<{
label="Grapher Config ETL"
field="v"
store={{
v: YAML.stringify(grapherConfigETL),
v: YAML.stringify(
variable.grapherConfigETL
),
}}
disabled
textarea
Expand Down
45 changes: 10 additions & 35 deletions adminSiteServer/apiRouter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import {
updateGrapherConfigAdminOfVariable,
updateGrapherConfigETLOfVariable,
updateAllChartsThatInheritFromIndicator,
getParentConfigForIndicatorChartAdmin,
} from "../db/model/Variable.js"
import { updateExistingFullConfig } from "../db/model/ChartConfigs.js"
import { getCanonicalUrl } from "@ourworldindata/components"
Expand Down Expand Up @@ -378,12 +377,12 @@ const updateExistingChart = async (
const parent = shouldInherit
? await getParentByChartConfig(knex, config)
: undefined

// compute patch and full configs
const fullParentConfig = mergeGrapherConfigs(
defaultGrapherConfig,
parent?.config ?? {}
)

// compute patch and full configs
const patchConfig = diffGrapherConfigs(config, fullParentConfig)
const fullConfig = mergeGrapherConfigs(fullParentConfig, patchConfig)

Expand Down Expand Up @@ -686,11 +685,11 @@ getRouteWithROTransaction(
"/charts/:chartId.parent.json",
async (req, res, trx) => {
const chartId = expectInt(req.params.chartId)
const parent = await getParentByChartId(trx, chartId)
const isInheritanceEnabled = await isInheritanceEnabledForChart(
trx,
chartId
)
const parent = await getParentByChartId(trx, chartId)
return omitUndefinedValues({
variableId: parent?.variableId,
config: parent?.config,
Expand All @@ -704,10 +703,7 @@ getRouteWithROTransaction(
"/charts/:chartId.patchConfig.json",
async (req, res, trx) => {
const chartId = expectInt(req.params.chartId)
const config = await getPatchConfigByChartId(trx, chartId)
if (!config) {
throw new JsonError(`Chart with id ${chartId} not found`, 500)
}
const config = await expectPatchConfigByChartId(trx, chartId)
return config
}
)
Expand Down Expand Up @@ -1353,19 +1349,6 @@ getRouteWithROTransaction(
}
)

getRouteWithROTransaction(
apiRouter,
"/variables/grapherConfigAdmin/:variableId.parentConfig.json",
async (req, res, trx) => {
const variableId = expectInt(req.params.variableId)
const parentConfig = await getParentConfigForIndicatorChartAdmin(
trx,
variableId
)
return parentConfig ?? {}
}
)

getRouteWithROTransaction(
apiRouter,
"/variables/mergedGrapherConfig/:variableId.json",
Expand Down Expand Up @@ -1416,17 +1399,13 @@ getRouteWithROTransaction(
variableId
)
const grapherConfigETL = variableWithConfigs?.etl?.fullConfig
const mergedGrapherConfig =
variableWithConfigs?.admin?.fullConfig ?? grapherConfigETL

const variableWithCharts: OwidVariableWithSource & {
charts: Record<string, any>
grapherConfig: GrapherInterface | undefined
grapherConfigETL: GrapherInterface | undefined
} = {
...variable,
charts,
grapherConfig: mergedGrapherConfig,
grapherConfigETL,
}

Expand All @@ -1448,14 +1427,14 @@ putRouteWithRWTransaction(
throw new JsonError(`Variable with id ${variableId} not found`, 500)
}

const updatedChartIds = await updateGrapherConfigETLOfVariable(
const updatedCharts = await updateGrapherConfigETLOfVariable(
trx,
variable,
req.body
)

// trigger build if any chart has been updated
if (updatedChartIds.length > 0) {
// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
await triggerStaticBuild(
res.locals.user,
`Updating ETL config for variable ${variableId}`
Expand All @@ -1477,12 +1456,8 @@ deleteRouteWithRWTransaction(
throw new JsonError(`Variable with id ${variableId} not found`, 500)
}

if (!variable.etl) {
throw new JsonError(
`Variable with id ${variableId} doesn't have an ETL config`,
500
)
}
// no-op if the variable doesn't have an ETL config
if (!variable.etl) return { success: true }

// remove reference in the variables table
await db.knexRaw(
Expand Down Expand Up @@ -1522,7 +1497,7 @@ deleteRouteWithRWTransaction(
}
)

// trigger build if any chart has been updated
// trigger build if any published chart has been updated
if (updatedCharts.some((chart) => chart.isPublished)) {
await triggerStaticBuild(
res.locals.user,
Expand Down
56 changes: 30 additions & 26 deletions adminSiteServer/app.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ async function cleanupDb() {
await knexReadWriteTransaction(
async (trx) => {
for (const table of tables) {
await trx.raw(`DELETE FROM ${table}`)
await trx.raw(`DELETE FROM ??`, [table])
}
},
TransactionCloseMode.KeepOpen,
Expand Down Expand Up @@ -189,14 +189,13 @@ async function makeRequestAgainstAdminApi(
return json
}

const currentSchema = defaultGrapherConfig["$schema"]
const testChartConfig = {
slug: "test-chart",
title: "Test chart",
type: "LineChart",
}

describe("OwidAdminApp", () => {
const testChartConfig = {
slug: "test-chart",
title: "Test chart",
type: "LineChart",
}

it("should be able to create an app", () => {
expect(app).toBeTruthy()
expect(app!.server).toBeTruthy()
Expand Down Expand Up @@ -251,9 +250,12 @@ describe("OwidAdminApp", () => {
const fullConfig = await fetchJsonFromAdminApi(
`/charts/${chartId}.config.json`
)
expect(fullConfig).toHaveProperty("$schema", currentSchema)
expect(fullConfig).toHaveProperty(
"$schema",
defaultGrapherConfig.$schema
)
expect(fullConfig).toHaveProperty("id", chartId) // must match the db id
expect(fullConfig).toHaveProperty("version", 1) // added version
expect(fullConfig).toHaveProperty("version", 1) // automatically added
expect(fullConfig).toHaveProperty("slug", "test-chart")
expect(fullConfig).toHaveProperty("title", "Test chart")
expect(fullConfig).toHaveProperty("type", "LineChart") // default property
Expand Down Expand Up @@ -366,24 +368,20 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
body: JSON.stringify(testVariableConfig),
})

// check that a row in the chart_configs table has been added
const chartConfigsCountAfter = await getCountForTable(
ChartConfigsTableName
)
expect(chartConfigsCountAfter).toBe(1)

// get inserted configs from the database
const row = await testKnexInstance!(ChartConfigsTableName).first()
const patchConfig = JSON.parse(row.patch)
const fullConfig = JSON.parse(row.full)
const patchConfigETL = JSON.parse(row.patch)
const fullConfigETL = JSON.parse(row.full)

// for ETL configs, patch and full configs should be the same
expect(patchConfig).toEqual(fullConfig)
expect(patchConfigETL).toEqual(fullConfigETL)

// check that $schema and dimensions field were added to the config
expect(patchConfig).toEqual({
expect(patchConfigETL).toEqual({
...testVariableConfig,
$schema: currentSchema,

// automatically added
$schema: defaultGrapherConfig.$schema,
dimensions: [
{
property: "y",
Expand All @@ -399,7 +397,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => {

// since no admin-authored config exists, the merged config should be
// the same as the ETL config
expect(mergedGrapherConfig).toEqual(fullConfig)
expect(mergedGrapherConfig).toEqual(fullConfigETL)

// delete the grapher config we just added
await makeRequestAgainstAdminApi({
Expand All @@ -415,6 +413,10 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
})

it("should update all charts that inherit from an indicator", async () => {
// make sure the database is in a clean state
const chartConfigsCount = await getCountForTable(ChartConfigsTableName)
expect(chartConfigsCount).toBe(0)

// add grapherConfigETL for the variable
await makeRequestAgainstAdminApi({
method: "PUT",
Expand Down Expand Up @@ -530,9 +532,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
shouldBeEnabled?: boolean
}): Promise<void> => {
const chartRow = await testKnexInstance!(ChartsTableName)
.where({
id: chartId,
})
.where({ id: chartId })
.first()

const fullConfig = await fetchJsonFromAdminApi(
Expand All @@ -550,6 +550,10 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
}
}

// make sure the database is in a clean state
const chartConfigsCount = await getCountForTable(ChartConfigsTableName)
expect(chartConfigsCount).toBe(0)

// add grapherConfigETL for the variable
await makeRequestAgainstAdminApi({
method: "PUT",
Expand Down Expand Up @@ -604,7 +608,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => {
await checkInheritance({ shouldBeEnabled: false })
})

it("should recompute configs when the parent changes", async () => {
it("should recompute configs when the parent of a chart changes", async () => {
// add grapherConfigETL for the variables
await makeRequestAgainstAdminApi({
method: "PUT",
Expand Down
4 changes: 2 additions & 2 deletions baker/siteRenderers.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -767,8 +767,8 @@ export const renderExplorerPage = async (
cc_etl.patch AS grapherConfigETL,
cc_admin.patch AS grapherConfigAdmin
FROM variables v
LEFT JOIN chart_configs cc_admin ON cc_admin.id=v.grapherConfigIdAdmin
LEFT JOIN chart_configs cc_etl ON cc_etl.id=v.grapherConfigIdETL
LEFT JOIN chart_configs cc_admin ON cc_admin.id=v.grapherConfigIdAdmin
LEFT JOIN chart_configs cc_etl ON cc_etl.id=v.grapherConfigIdETL
WHERE v.id IN (?)
`,
[requiredVariableIds]
Expand Down
22 changes: 13 additions & 9 deletions db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522
}

public async up(queryRunner: QueryRunner): Promise<void> {
// inheritance is opt-in
await queryRunner.query(`-- sql
ALTER TABLE charts
ADD COLUMN isInheritanceEnabled TINYINT(1) NOT NULL DEFAULT 0 AFTER configId
`)

// add grapherConfigIdAdmin and grapherConfigIdETL to the variables table
await queryRunner.query(`-- sql
ALTER TABLE variables
ADD COLUMN grapherConfigIdAdmin char(36) UNIQUE AFTER sort,
Expand All @@ -124,12 +131,6 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522
ON UPDATE RESTRICT
`)

// inheritance is opt-in
await queryRunner.query(`-- sql
ALTER TABLE charts
ADD COLUMN isInheritanceEnabled TINYINT(1) NOT NULL DEFAULT 0 AFTER configId
`)

// note that we copy the ETL-authored configs to the chart_configs table,
// but drop the admin-authored configs

Expand Down Expand Up @@ -157,20 +158,23 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522
WHERE
property = 'y'
),
single_y_indicator_charts As (
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
cc.full ->> '$.type' != 'ScatterPlot'
GROUP BY
c.id
HAVING
-- restrict to single y-variable charts
COUNT(distinct yd.variableId) = 1
)
SELECT
Expand All @@ -193,8 +197,8 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522
// add back the `grapherConfigAdmin` and `grapherConfigETL` columns
await queryRunner.query(`-- sql
ALTER TABLE variables
ADD COLUMN grapherConfigAdmin json AFTER sort,
ADD COLUMN grapherConfigETL json AFTER grapherConfigAdmin
ADD COLUMN grapherConfigAdmin JSON AFTER sort,
ADD COLUMN grapherConfigETL JSON AFTER grapherConfigAdmin
`)

// copy configs from the chart_configs table to the variables table
Expand Down
9 changes: 4 additions & 5 deletions db/model/Chart.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,10 +263,10 @@ export async function isInheritanceEnabledForChart(
>(
trx,
`-- sql
SELECT isInheritanceEnabled
FROM charts
WHERE id = ?
`,
SELECT isInheritanceEnabled
FROM charts
WHERE id = ?
`,
[chartId]
)
return row?.isInheritanceEnabled ?? false
Expand Down Expand Up @@ -297,7 +297,6 @@ export async function getParentByChartId(
const variable = await getGrapherConfigsForVariable(trx, parentVariableId)
const parentConfig =
variable?.admin?.fullConfig ?? variable?.etl?.fullConfig
if (!parentConfig) return { variableId: parentVariableId }
return {
variableId: parentVariableId,
config: parentConfig,
Expand Down
Loading

0 comments on commit bab068e

Please sign in to comment.