From 116203d2d9acbd11dac16e85c0d381bc0744723d Mon Sep 17 00:00:00 2001 From: Marigold Date: Mon, 16 Sep 2024 11:50:42 +0200 Subject: [PATCH 1/6] :bug: Align charts.updatedAt and chart_configs.updatedAt --- db/migration/1726480222200-AlignUpdatedAtOfCharts.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 db/migration/1726480222200-AlignUpdatedAtOfCharts.ts diff --git a/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts b/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts new file mode 100644 index 00000000000..10d1ac6391d --- /dev/null +++ b/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts @@ -0,0 +1,11 @@ +import { MigrationInterface, QueryRunner } from "typeorm"; + +export class AlignUpdatedAtOfCharts1726480222200 implements MigrationInterface { + + public async up(queryRunner: QueryRunner): Promise { + } + + public async down(queryRunner: QueryRunner): Promise { + } + +} From a813b4ab9f5ab5325f54d0d94826194b139c107c Mon Sep 17 00:00:00 2001 From: Marigold Date: Mon, 16 Sep 2024 11:58:06 +0200 Subject: [PATCH 2/6] wip --- .../1726480222200-AlignUpdatedAtOfCharts.ts | 11 ----------- .../1726480222201-AlignUpdatedAtOfCharts.ts | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 11 deletions(-) delete mode 100644 db/migration/1726480222200-AlignUpdatedAtOfCharts.ts create mode 100644 db/migration/1726480222201-AlignUpdatedAtOfCharts.ts diff --git a/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts b/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts deleted file mode 100644 index 10d1ac6391d..00000000000 --- a/db/migration/1726480222200-AlignUpdatedAtOfCharts.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { MigrationInterface, QueryRunner } from "typeorm"; - -export class AlignUpdatedAtOfCharts1726480222200 implements MigrationInterface { - - public async up(queryRunner: QueryRunner): Promise { - } - - public async down(queryRunner: QueryRunner): Promise { - } - -} diff --git a/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts new file mode 100644 index 00000000000..4d5d3d16fa2 --- /dev/null +++ b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts @@ -0,0 +1,18 @@ +import { MigrationInterface, QueryRunner } from "typeorm" + +export class AlignUpdatedAtOfCharts1726480222201 implements MigrationInterface { + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + UPDATE chart_configs cf + INNER JOIN charts c ON c.configId = cf.id + SET cf.updatedAt = LEAST(cf.updatedAt, c.updatedAt) + WHERE c.updatedAt IS NOT NULL; + `) + } + + public async down(queryRunner: QueryRunner): Promise { + throw new Error( + "Cannot automatically revert migration 'AlignUpdatedAtOfCharts1726480222201'" + ) + } +} From 745999b130365e72883e63d887e340c7e544cee0 Mon Sep 17 00:00:00 2001 From: Marigold Date: Mon, 16 Sep 2024 12:02:03 +0200 Subject: [PATCH 3/6] wip --- db/migration/1726480222201-AlignUpdatedAtOfCharts.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts index 4d5d3d16fa2..436922c4466 100644 --- a/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts +++ b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts @@ -10,7 +10,7 @@ export class AlignUpdatedAtOfCharts1726480222201 implements MigrationInterface { `) } - public async down(queryRunner: QueryRunner): Promise { + public async down(): Promise { throw new Error( "Cannot automatically revert migration 'AlignUpdatedAtOfCharts1726480222201'" ) From f078e48e8ad33a4fe8ef821290edf1f02a1130b5 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Tue, 17 Sep 2024 11:19:53 +0200 Subject: [PATCH 4/6] =?UTF-8?q?=E2=9C=A8=20ensure=20chart=20and=20config?= =?UTF-8?q?=20updatedAt=20timestamps=20are=20in=20sync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteServer/apiRouter.ts | 17 ++++++-- adminSiteServer/app.test.tsx | 42 +++++++++++++++++++ .../1726480222201-AlignUpdatedAtOfCharts.ts | 24 +++++++++-- db/model/ChartConfigs.ts | 31 +++++++++----- db/model/Variable.ts | 32 +++++++++++++- db/tests/basic.test.ts | 25 +---------- 6 files changed, 127 insertions(+), 44 deletions(-) diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index 5d845c205bb..092352db035 100644 --- a/adminSiteServer/apiRouter.ts +++ b/adminSiteServer/apiRouter.ts @@ -434,6 +434,8 @@ const updateExistingChart = async ( if (!chartConfigId) throw new JsonError(`No chart config found for id ${chartId}`, 404) + const now = new Date() + // update configs await db.knexRaw( knex, @@ -441,12 +443,14 @@ const updateExistingChart = async ( UPDATE chart_configs SET patch=?, - full=? + full=?, + updatedAt=? WHERE id = ? `, [ serializeChartConfig(patchConfig), fullConfigStringified, + now, chartConfigId.configId, ] ) @@ -456,10 +460,10 @@ const updateExistingChart = async ( knex, `-- sql UPDATE charts - SET isInheritanceEnabled=?, lastEditedAt=?, lastEditedByUserId=? + SET isInheritanceEnabled=?, updatedAt=?, lastEditedAt=?, lastEditedByUserId=? WHERE id = ? `, - [shouldInherit, new Date(), user.id, chartId] + [shouldInherit, now, now, user.id, chartId] ) // We need to get the full config and the md5 hash from the database instead of @@ -1643,6 +1647,8 @@ deleteRouteWithRWTransaction( // no-op if the variable doesn't have an ETL config if (!variable.etl) return { success: true } + const now = new Date() + // remove reference in the variables table await db.knexRaw( trx, @@ -1669,6 +1675,7 @@ deleteRouteWithRWTransaction( await updateExistingFullConfig(trx, { configId: variable.admin.configId, config: variable.admin.patchConfig, + updatedAt: now, }) } @@ -1678,6 +1685,7 @@ deleteRouteWithRWTransaction( variableId, { patchConfigAdmin: variable.admin?.patchConfig, + updatedAt: now, } ) @@ -1734,6 +1742,8 @@ deleteRouteWithRWTransaction( // no-op if the variable doesn't have an admin-authored config if (!variable.admin) return { success: true } + const now = new Date() + // remove reference in the variables table await db.knexRaw( trx, @@ -1761,6 +1771,7 @@ deleteRouteWithRWTransaction( variableId, { patchConfigETL: variable.etl?.patchConfig, + updatedAt: now, } ) diff --git a/adminSiteServer/app.test.tsx b/adminSiteServer/app.test.tsx index 0fb7c8b6c59..8e1cbef037a 100644 --- a/adminSiteServer/app.test.tsx +++ b/adminSiteServer/app.test.tsx @@ -51,6 +51,8 @@ import { ChartConfigsTableName, ChartsTableName, DatasetsTableName, + DbPlainChart, + DbRawChartConfig, VariablesTableName, } from "@ourworldindata/types" import { defaultGrapherConfig } from "@ourworldindata/grapher" @@ -725,4 +727,44 @@ describe("OwidAdminApp: indicator-level chart configs", () => { ) expect(fullConfig).not.toHaveProperty("note") }) + + it("should update timestamps on chart update", async () => { + // make sure the database is in a clean state + const chartCount = await getCountForTable(ChartsTableName) + expect(chartCount).toBe(0) + const chartConfigsCount = await getCountForTable(ChartConfigsTableName) + expect(chartConfigsCount).toBe(0) + + // make a request to create a chart + const response = await makeRequestAgainstAdminApi({ + method: "POST", + path: "/charts", + body: JSON.stringify(testChartConfig), + }) + const chartId = response.chartId + + // helper functions to get the updatedAt timestamp of the chart and its config + const chartUpdatedAt = async (): Promise => + (await testKnexInstance!(ChartsTableName).first()).updatedAt + const configUpdatedAt = async (): Promise => + (await testKnexInstance!(ChartConfigsTableName).first()).updatedAt + + // verify that both updatedAt timestamps are null initially + expect(await chartUpdatedAt()).toBeNull() + expect(await configUpdatedAt()).toBeNull() + + // update the chart + await makeRequestAgainstAdminApi({ + method: "PUT", + path: `/charts/${chartId}`, + body: JSON.stringify({ ...testChartConfig, title: "New title" }), + }) + + // verify that the updatedAt timestamps are the same + const chartAfterUpdate = await chartUpdatedAt() + const configAfterUpdate = await configUpdatedAt() + expect(chartAfterUpdate).not.toBeNull() + expect(configAfterUpdate).not.toBeNull() + expect(chartAfterUpdate).toEqual(configAfterUpdate) + }) }) diff --git a/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts index 436922c4466..49ce5b4d958 100644 --- a/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts +++ b/db/migration/1726480222201-AlignUpdatedAtOfCharts.ts @@ -8,11 +8,27 @@ export class AlignUpdatedAtOfCharts1726480222201 implements MigrationInterface { SET cf.updatedAt = LEAST(cf.updatedAt, c.updatedAt) WHERE c.updatedAt IS NOT NULL; `) + + await queryRunner.query(` + ALTER TABLE charts + MODIFY updatedAt datetime DEFAULT NULL; + `) + await queryRunner.query(` + ALTER TABLE chart_configs + MODIFY updatedAt datetime DEFAULT NULL; + `) } - public async down(): Promise { - throw new Error( - "Cannot automatically revert migration 'AlignUpdatedAtOfCharts1726480222201'" - ) + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + ALTER TABLE charts + MODIFY updatedAt datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP; + `) + await queryRunner.query(` + ALTER TABLE chart_configs + MODIFY updatedAt datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP; + `) + + // We can't automatically revert the data changes } } diff --git a/db/model/ChartConfigs.ts b/db/model/ChartConfigs.ts index 18a4148a8c3..68460c40fd3 100644 --- a/db/model/ChartConfigs.ts +++ b/db/model/ChartConfigs.ts @@ -6,21 +6,18 @@ import { import * as db from "../db.js" -interface ConfigWithId { - configId: DbInsertChartConfig["id"] - config: GrapherInterface -} - export async function updateExistingConfigPair( knex: db.KnexReadWriteTransaction, { configId, patchConfig, fullConfig, + updatedAt, }: { configId: DbInsertChartConfig["id"] patchConfig: GrapherInterface fullConfig: GrapherInterface + updatedAt: Date } ): Promise { await db.knexRaw( @@ -29,27 +26,35 @@ export async function updateExistingConfigPair( UPDATE chart_configs SET patch = ?, - full = ? + full = ?, + updatedAt = ? WHERE id = ? `, [ serializeChartConfig(patchConfig), serializeChartConfig(fullConfig), + updatedAt, configId, ] ) } +type ConfigId = DbInsertChartConfig["id"] +interface UpdateFields { + config: GrapherInterface + updatedAt: Date +} + export async function updateExistingPatchConfig( knex: db.KnexReadWriteTransaction, - params: ConfigWithId + params: UpdateFields & { configId: ConfigId } ): Promise { await updateExistingConfig(knex, { ...params, column: "patch" }) } export async function updateExistingFullConfig( knex: db.KnexReadWriteTransaction, - params: ConfigWithId + params: UpdateFields & { configId: ConfigId } ): Promise { await updateExistingConfig(knex, { ...params, column: "full" }) } @@ -60,7 +65,9 @@ async function updateExistingConfig( column, configId, config, - }: ConfigWithId & { + updatedAt, + }: UpdateFields & { + configId: ConfigId column: "patch" | "full" } ): Promise { @@ -68,9 +75,11 @@ async function updateExistingConfig( knex, `-- sql UPDATE chart_configs - SET ?? = ? + SET + ?? = ?, + updatedAt = ? WHERE id = ? `, - [column, serializeChartConfig(config), configId] + [column, serializeChartConfig(config), updatedAt, configId] ) } diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 7c24e5f27b3..716083ea1ab 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -251,9 +251,11 @@ export async function updateAllChartsThatInheritFromIndicator( trx: db.KnexReadWriteTransaction, variableId: number, { + updatedAt, patchConfigETL, patchConfigAdmin, }: { + updatedAt: Date patchConfigETL?: GrapherInterface patchConfigAdmin?: GrapherInterface } @@ -262,6 +264,7 @@ export async function updateAllChartsThatInheritFromIndicator( trx, variableId ) + for (const chart of inheritingCharts) { const fullConfig = mergeGrapherConfigs( defaultGrapherConfig, @@ -274,12 +277,28 @@ export async function updateAllChartsThatInheritFromIndicator( `-- sql UPDATE chart_configs cc JOIN charts c ON c.configId = cc.id - SET cc.full = ? + SET + cc.full = ?, + cc.updatedAt = ? WHERE c.id = ? `, - [JSON.stringify(fullConfig), chart.chartId] + [JSON.stringify(fullConfig), updatedAt, chart.chartId] + ) + } + + // update timestamp of all updated charts + if (inheritingCharts.length > 0) { + await db.knexRaw( + trx, + `-- sql + UPDATE charts + SET updatedAt = ? + WHERE id IN (?) + `, + [updatedAt, inheritingCharts.map((chart) => chart.chartId)] ) } + // let the caller know if any charts were updated return inheritingCharts } @@ -299,11 +318,14 @@ export async function updateGrapherConfigETLOfVariable( variableId, }) + const now = new Date() + if (variable.etl) { await updateExistingConfigPair(trx, { configId: variable.etl.configId, patchConfig: configETL, fullConfig: configETL, + updatedAt: now, }) } else { await insertNewGrapherConfigForVariable(trx, { @@ -323,6 +345,7 @@ export async function updateGrapherConfigETLOfVariable( await updateExistingFullConfig(trx, { configId: variable.admin.configId, config: fullConfig, + updatedAt: now, }) } @@ -332,6 +355,7 @@ export async function updateGrapherConfigETLOfVariable( { patchConfigETL: configETL, patchConfigAdmin: variable.admin?.patchConfig, + updatedAt: now, } ) @@ -366,11 +390,14 @@ export async function updateGrapherConfigAdminOfVariable( patchConfigAdmin ) + const now = new Date() + if (variable.admin) { await updateExistingConfigPair(trx, { configId: variable.admin.configId, patchConfig: patchConfigAdmin, fullConfig: fullConfigAdmin, + updatedAt: now, }) } else { await insertNewGrapherConfigForVariable(trx, { @@ -387,6 +414,7 @@ export async function updateGrapherConfigAdminOfVariable( { patchConfigETL: variable.etl?.patchConfig ?? {}, patchConfigAdmin: patchConfigAdmin, + updatedAt: now, } ) diff --git a/db/tests/basic.test.ts b/db/tests/basic.test.ts index 6ef2e4d670e..f6cfc52e926 100644 --- a/db/tests/basic.test.ts +++ b/db/tests/basic.test.ts @@ -62,7 +62,7 @@ test("it can query a user created in fixture via TypeORM", async () => { expect(user.email).toBe("admin@example.com") }) -test("timestamps are automatically created and updated", async () => { +test("createdAt timestamp is automatically created", async () => { await knexReadWriteTransaction( async (trx) => { const user = await knexInstance! @@ -95,29 +95,6 @@ test("timestamps are automatically created and updated", async () => { if (created) { expect(created.createdAt).not.toBeNull() expect(created.updatedAt).toBeNull() - await sleep(1000, undefined) - await trx - .table(ChartsTableName) - .where({ id: chartId }) - .update({ isIndexable: true }) - const updated = await knexRawFirst( - trx, - "select * from charts where id = ?", - [chartId] - ) - expect(updated).not.toBeNull() - if (updated) { - expect(updated.createdAt).not.toBeNull() - expect(updated.updatedAt).not.toBeNull() - expect( - updated.updatedAt!.getTime() - - updated.createdAt.getTime() - ).toBeGreaterThan(800) - expect( - updated.updatedAt!.getTime() - - updated.createdAt.getTime() - ).toBeLessThanOrEqual(2000) - } } }, TransactionCloseMode.KeepOpen, From 220f518771278f1dd42cb3c33ad6c184818964ee Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Tue, 17 Sep 2024 11:26:25 +0200 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=92=84=20linting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteServer/app.test.tsx | 2 -- db/model/Variable.ts | 8 ++++---- db/tests/basic.test.ts | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/adminSiteServer/app.test.tsx b/adminSiteServer/app.test.tsx index 8e1cbef037a..8acb2fb4c94 100644 --- a/adminSiteServer/app.test.tsx +++ b/adminSiteServer/app.test.tsx @@ -51,8 +51,6 @@ import { ChartConfigsTableName, ChartsTableName, DatasetsTableName, - DbPlainChart, - DbRawChartConfig, VariablesTableName, } from "@ourworldindata/types" import { defaultGrapherConfig } from "@ourworldindata/grapher" diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 716083ea1ab..a03c63168a1 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -291,10 +291,10 @@ export async function updateAllChartsThatInheritFromIndicator( await db.knexRaw( trx, `-- sql - UPDATE charts - SET updatedAt = ? - WHERE id IN (?) - `, + UPDATE charts + SET updatedAt = ? + WHERE id IN (?) + `, [updatedAt, inheritingCharts.map((chart) => chart.chartId)] ) } diff --git a/db/tests/basic.test.ts b/db/tests/basic.test.ts index f6cfc52e926..1c2c6733e15 100644 --- a/db/tests/basic.test.ts +++ b/db/tests/basic.test.ts @@ -21,7 +21,7 @@ import { UsersTableName, DbInsertChartConfig, } from "@ourworldindata/types" -import { cleanTestDb, sleep } from "./testHelpers.js" +import { cleanTestDb } from "./testHelpers.js" let knexInstance: Knex | undefined = undefined From 5534827db498186b8dec633cac9135ebf99f0469 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Tue, 17 Sep 2024 14:44:42 +0200 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=94=A8=20small=20refactor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- db/model/Variable.ts | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/db/model/Variable.ts b/db/model/Variable.ts index a03c63168a1..ef0ac7c2877 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -279,23 +279,11 @@ export async function updateAllChartsThatInheritFromIndicator( JOIN charts c ON c.configId = cc.id SET cc.full = ?, - cc.updatedAt = ? + cc.updatedAt = ?, + c.updatedAt = ? WHERE c.id = ? `, - [JSON.stringify(fullConfig), updatedAt, chart.chartId] - ) - } - - // update timestamp of all updated charts - if (inheritingCharts.length > 0) { - await db.knexRaw( - trx, - `-- sql - UPDATE charts - SET updatedAt = ? - WHERE id IN (?) - `, - [updatedAt, inheritingCharts.map((chart) => chart.chartId)] + [JSON.stringify(fullConfig), updatedAt, updatedAt, chart.chartId] ) }