🐛 Align charts.updatedAt and chart_configs.updatedAt

[Marigold]

I found some charts where charts.updatedAt and chart_configs.updatedAt. They should always match (or at least be very close), we depend on it in chart-sync.

[owidbot]

Quick links (staging server):

Site	Admin	Wizard

Login: ssh owid@staging-site-align-updatedat

• Site-screenshots: https://github.com/owid/site-screenshots/compare/align-updatedat
• SVG tester: https://github.com/owid/owid-grapher-svgs/compare/align-updatedat

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-09-16 09:59:24 UTC
Execution time: 1.21 seconds

[sophiamersmann]

Thinking about this some more, we'll run into the same issue again the next time someone runs a migration on chart_configs. Maybe we should also remove the auto-update clause from the updatedAt columns and update updatedAt manually in the code whenever a chart is saved. That way, they'll always be identical. What do you think?

If you don't mind me committing to your branch, then I could just add the rest of the code.

[rakyi]

If we always want them to be identical, shouldn't we have only one place to store that piece of data?

[Marigold]

If you don't mind me committing to your branch, then I could just add the rest of the code.

Please do!

If we always want them to be identical, shouldn't we have only one place to store that piece of data?

Yeah, that'd be ideal, but I have no idea how much harder could it be. As far as I know, virtual columns can't reference other tables. We could add trigger to automatically update it in mysql rather than in code, but I'll leave it to @sophiamersmann to pick her favorite option.

[sophiamersmann]

Yeah, I agree that one place to store this information would be better, but the complexity of database triggers is probably not worth it. I added a database test, though, for good measure.

@Marigold, I added the code necessary to update the updatedAt columns manually. This is how it works:

• If a chart config is updated, the updatedAt columns of both the charts row and the configs row are assigned the same timestamp
  • This also happens when inheritance is enabled/disabled for a chart (since toggling inheritance might also update a chart's full config)
• If a Grapher config of a parent variable is updated, then the updatedAt fields of all inheriting charts are updated as well (since updating a parent variable config might update the full config of a chart). However, updating a parent variable doesn't necessarily update the full config of an inheriting chart. So, it is possible for the full config to stay the same while updatedAt is increased. Let me know if this is a problem.

More generally speaking, my current approach makes it so that the updatedAt field of a chart is updated whenever the full (i.e. rendered) config of a chart changes (or might change). Alternatively, we could also make it so that the updatedAt field is only touched when the patch config of a chart is changed. Let me know what makes more sense to you!

[Marigold]

So, it is possible for the full config to stay the same while updatedAt is increased. Let me know if this is a problem.

Nope, this is fine for us. We are detecting identical configs in chart-sync anyway.

I can't review my own PR, but consider it reviewed (even though I'm not a grapher expert).

[sophiamersmann]

It's good to go in then, I think 👍🏻

[rakyi]

Yeah, I agree that one place to store this information would be better, but the complexity of database triggers is probably not worth it. I added a database test, though, for good measure.

Nice! Just to clarify, I didn't mean DB triggers. I thought maybe we could store it in the DB literally just once and then use a join or a separate query. But I obviously lack a lot of context.