🐛 Align charts.updatedAt and chart_configs.updatedAt (#3962)

* 🐛 Align charts.updatedAt and chart_configs.updatedAt

* ✨ ensure chart and config updatedAt timestamps are in sync

---------

Co-authored-by: sophiamersmann <[email protected]>

adminSiteServer/apiRouter.ts

@@ -434,19 +434,23 @@ 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,
         `-- sql
             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,
             }
         )
 

adminSiteServer/app.test.tsx

@@ -725,4 +725,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<Date> =>
+            (await testKnexInstance!(ChartsTableName).first()).updatedAt
+        const configUpdatedAt = async (): Promise<Date> =>
+            (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)
+    })
 })

db/migration/1726480222201-AlignUpdatedAtOfCharts.ts

@@ -0,0 +1,34 @@
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class AlignUpdatedAtOfCharts1726480222201 implements MigrationInterface {
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        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;
+        `)
+
+        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(queryRunner: QueryRunner): Promise<void> {
+        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
+    }
+}

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<void> {
     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<void> {
     await updateExistingConfig(knex, { ...params, column: "patch" })
 }
 
 export async function updateExistingFullConfig(
     knex: db.KnexReadWriteTransaction,
-    params: ConfigWithId
+    params: UpdateFields & { configId: ConfigId }
 ): Promise<void> {
     await updateExistingConfig(knex, { ...params, column: "full" })
 }
@@ -60,17 +65,21 @@ async function updateExistingConfig(
         column,
         configId,
         config,
-    }: ConfigWithId & {
+        updatedAt,
+    }: UpdateFields & {
+        configId: ConfigId
         column: "patch" | "full"
     }
 ): Promise<void> {
     await db.knexRaw(
         knex,
         `-- sql
             UPDATE chart_configs
-            SET ?? = ?
+            SET
+                ?? = ?,
+                updatedAt = ?
             WHERE id = ?
         `,
-        [column, serializeChartConfig(config), configId]
+        [column, serializeChartConfig(config), updatedAt, configId]
     )
 }

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,16 @@ 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 = ?,
+                    c.updatedAt = ?
                 WHERE c.id = ?
             `,
-            [JSON.stringify(fullConfig), chart.chartId]
+            [JSON.stringify(fullConfig), updatedAt, updatedAt, chart.chartId]
         )
     }
+
     // let the caller know if any charts were updated
     return inheritingCharts
 }
@@ -299,11 +306,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 +333,7 @@ export async function updateGrapherConfigETLOfVariable(
         await updateExistingFullConfig(trx, {
             configId: variable.admin.configId,
             config: fullConfig,
+            updatedAt: now,
         })
     }
 
@@ -332,6 +343,7 @@ export async function updateGrapherConfigETLOfVariable(
         {
             patchConfigETL: configETL,
             patchConfigAdmin: variable.admin?.patchConfig,
+            updatedAt: now,
         }
     )
 
@@ -366,11 +378,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 +402,7 @@ export async function updateGrapherConfigAdminOfVariable(
         {
             patchConfigETL: variable.etl?.patchConfig ?? {},
             patchConfigAdmin: patchConfigAdmin,
+            updatedAt: now,
         }
     )
 

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<any, unknown[]> | undefined = undefined
 
@@ -62,7 +62,7 @@ test("it can query a user created in fixture via TypeORM", async () => {
     expect(user.email).toBe("[email protected]")
 })
 
-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<DbPlainChart>(
-                    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,