Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔨 (db) add chart_configs table / TAS-563 #3767

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Jul 3, 2024

Summary

Adds a chart_configs table to the database, adds a configId column to the charts table that points to a chart_configs row, and removes the config column.

Shouldn't result in any functionality change.

Details

  • All configs are copied from the charts table to the chart_configs table (for now, patch config = full config since inheritance hasn't been implemented yet)
  • Configs have a UUID, but it's not yet used (the id of the chart is still the sequential primary key of the charts table)

Testing

  • Bakes locally
  • Bakes staging site

Site:

Admin:

  • Chart editor:
    • Create new chart
    • Save as new chart
    • Update existing chart
    • Publish chart
    • Unpublish chart
    • Delete chart
  • Bulk chart editor
  • Chart test page
  • Variable page (chart list)

On the ETL side

Mojmir opened a PR for changes to the ETL: owid/etl#2957

To do

To do after merging

Copy link
Member Author

sophiamersmann commented Jul 3, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @sophiamersmann and the rest of your teammates on Graphite Graphite

@owidbot
Copy link
Contributor

owidbot commented Jul 8, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-db-chart-configs

SVG tester:

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

Edited: 2024-07-08 14:35:44 UTC
Execution time: 1.19 seconds

@sophiamersmann sophiamersmann force-pushed the db-chart-configs branch 2 times, most recently from f15f1c5 to 94680ac Compare July 8, 2024 14:11
@sophiamersmann sophiamersmann marked this pull request as ready for review July 8, 2024 14:38
@sophiamersmann sophiamersmann force-pushed the db-chart-configs branch 4 times, most recently from 98345c7 to f587577 Compare July 10, 2024 12:51
db/migration/1719842654592-AddChartConfigsTable.ts Outdated Show resolved Hide resolved
db/migration/1719842654592-AddChartConfigsTable.ts Outdated Show resolved Hide resolved
db/tests/basic.test.ts Outdated Show resolved Hide resolved
db/model/Post.ts Outdated Show resolved Hide resolved
db/migration/1719842654592-AddChartConfigsTable.ts Outdated Show resolved Hide resolved
baker/SiteBaker.tsx Outdated Show resolved Hide resolved
adminSiteServer/apiRouter.ts Outdated Show resolved Hide resolved
adminSiteServer/apiRouter.ts Show resolved Hide resolved
@danyx23
Copy link
Contributor

danyx23 commented Jul 11, 2024

Looks good overall! I think we should add the slug as a materialized computed column on chart_configs for now and add an index - even if we move it back to chart eventually. We use this quite extensively and loosing the ability to use an index could hurt performance quite a bit, especially if we start pushing more and more stuff into chart_configs later in the cycle.

@danyx23
Copy link
Contributor

danyx23 commented Jul 11, 2024

Just to note that I haven't run this locally yet - the PR review so far was just about looking at the code. Ping me again when you are ready and then I'll run through the migration and QA the code a bit more :)

@sophiamersmann sophiamersmann changed the title 🔨 (db) add chart_configs table 🔨 (db) add chart_configs table / TAS-563 Jul 15, 2024
Copy link

@sophiamersmann sophiamersmann force-pushed the db-chart-configs branch 2 times, most recently from 7673208 to be4487e Compare July 15, 2024 13:53
@danyx23 danyx23 changed the base branch from enable-wrangler-toml to graphite-base/3767 August 13, 2024 16:40
@danyx23 danyx23 changed the base branch from graphite-base/3767 to master August 13, 2024 16:41
Copy link
Member Author

sophiamersmann commented Sep 3, 2024

Merge activity

@sophiamersmann sophiamersmann merged commit 6c3057a into master Sep 3, 2024
21 checks passed
@sophiamersmann sophiamersmann deleted the db-chart-configs branch September 3, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants