Add admin for multidimensional data pages #4505

rakyi · 2025-01-30T15:03:59Z

Resolves #4458

ETL path and a new/changed admin API will be in a follow-up PR.

owidbot · 2025-01-30T15:22:26Z

Quick links (staging server):

Site Dev	Site Preview	Admin	Wizard	Docs

Login: ssh owid@staging-site-mdim-admin

Site-screenshots: https://github.com/owid/site-screenshots/compare/mdim-admin
SVG tester: https://github.com/owid/owid-grapher-svgs/compare/mdim-admin

SVG tester:

Number of differences (default views): 2153 (ca33c1) ❌
Number of differences (all views): 1496 (b5db44) ❌

Edited: 2025-02-12 13:55:41 UTC
Execution time: 1.28 seconds

rakyi · 2025-02-06T16:27:31Z

adminSiteClient/AdminApp.tsx

-                                            slug={match.params.slug}
-                                            gitCmsBranchName={gitCmsBranchName}
-                                            manager={admin}
+            <QueryClientProvider client={queryClient}>


The changes below are mostly whitespace, except the new route and the new provider.

rakyi · 2025-02-06T16:29:17Z

adminSiteServer/apiRouter.ts

-    handleMultiDimDataPageRequest
-)
+getRouteWithROTransaction(apiRouter, "/multi-dims.json", handleGetMultiDims)
+putRouteWithRWTransaction(apiRouter, "/multi-dim/:slug", handlePutMultiDim)


We will change the API in a follow-up PR (including making it plural).

rakyi · 2025-02-06T16:30:12Z

adminSiteServer/validation.ts

Most of the code moved from adminSiteServer/apiRoutes/charts.ts.

rakyi · 2025-02-06T16:31:06Z

packages/@ourworldindata/types/src/dbTypes/ChartConfigs.ts

-    row: DbRawChartConfig
-): DbEnrichedChartConfig {
+    row: Pick<DbRawChartConfig, "patch" | "full">
+): Pick<DbEnrichedChartConfig, "patch" | "full"> {


To make it work for cases when we don't pass the whole config around.

rakyi · 2025-02-06T16:31:29Z

packages/@ourworldindata/types/src/domainTypes/Various.ts

-    constructor(message: string, status?: number) {
-        super(message)
+    constructor(message: string, status?: number, options?: ErrorOptions) {
+        super(message, options)


To make the cause work.

rakyi · 2025-02-06T16:33:25Z

adminSiteClient/ChartViewIndexPage.tsx


 import { AdminLayout } from "./AdminLayout.js"
 import { AdminAppContext } from "./AdminAppContext.js"
 import { Timeago } from "./Forms.js"
-import { ColumnsType } from "antd/es/table/InternalTable.js"


We can use the public type instead of the internal one.

marcelgerber

Nicely done!

marcelgerber · 2025-02-07T09:25:47Z

db/model/Gdoc/GdocBase.ts

+                        !(await multiDimDataPageExists(knex, {
+                            slug: link.target,
+                            published: true,
+                        }))


Why this change? And, if we change it, why not get rid of isMultiDimDataPagePublished then (which is now unused)?

Because I wanted to get rid of isMultiDimDataPagePublished after I needed the existence check based on other stuff than published and then I forgot to remove it. Removing!

marcelgerber · 2025-02-10T09:42:26Z

adminSiteServer/apiRoutes/charts.ts

+        if (newConfig.slug) {
+            await validateGrapherSlug(knex, newConfig.slug, existingConfig?.id)
+        }


We need a slug to be present if the chart is published, and should make sure to fail if it's not present.

Ah, of course! Good catch, fixing.

marcelgerber · 2025-02-10T10:04:09Z

adminSiteServer/multiDim.ts

+        multiDim.config.views.map((view) => view.fullConfigId)
+    )
+
+    await Promise.all(


I'm unsure about the limitless concurrency this can run in, if the mdim has many views.
Maybe using p-map with {concurrency: 10} or something is better suited here?

I'm happy to change it, and I'd be curious to hear if this was an issue in practice before.

I'm not quite sure honestly!
I think undici (Node's fetch implementation) limits the number of open connections by itself, and I believe we have run into some kind of error related to that in the past?

With {concurrency: 10} it's a lot slower. Do you think 32 would be ok? Or even higher?

Yeah, go for it! It's all just vibes-based anyway 😁

Hmm, in that case, I'd probably leave it as Promise.all() and fix it if we ever hit a problem. The S3 client doesn't seem to use undici.

marcelgerber · 2025-02-10T10:10:46Z

adminSiteServer/apiRoutes/mdims.ts

    if (
        FEATURE_FLAGS.has(FeatureFlagFeature.MultiDimDataPage) &&
-        (await isMultiDimDataPagePublished(trx, slug))
+        (await multiDimDataPageExists(trx, { slug, published: true }))
    ) {


This code path here should also call validateGrapherSlug

Hmm, true. I didn't realize we are checking only for published graphers in the validation. That's even more complex.

marcelgerber · 2025-02-10T10:15:41Z

adminSiteServer/apiRoutes/mdims.ts

+        multiDim = await setMultiDimPublished(trx, multiDim, published)
+        action = published ? "publish" : "unpublish"
+    }
+    if (slug !== undefined && slug !== multiDim.slug) {


A few things here:

I think we should first handle slug, then publish state.

In theory, we only need to call validateGrapherSlug if the mdim is published.

And we should probably also call it at the point when a mdim is published (i.e. publish state changes to true).

Why do you think we should handle the slug first? I don't see it.

I'd prefer if we kept the mdim slug valid even before publishing. I'd change the logic when we'll have a use case for it.

I added the additional validation — I think it's fine to validate the slug twice in when the mdim is published instead of making the logic more complex.

(1) otherwise we could, in theory, end up in a state where we mark the mdim as published (under the old slug), and then the slug rename fails, i.e. we have it published under the old slug.
I realize that this cannot currently happen, because we either change the slug or the published state, not both at once - but just to be a tiny bit on the safer side, I still think switching these around makes sense.

Ah, because of the R2! If it was just the DB it wouldn't matter, because the transaction gets rolled back. Changing.

I switched the order and also moved the validation logic earlier, so it can also fail before we update stuff to R2.

marcelgerber · 2025-02-10T10:23:13Z

adminSiteClient/MultiDimIndexPage.tsx

+            render: (slug, { id }) => (
+                <SlugField id={id} slug={slug} slugMutation={slugMutation} />
+            ),
+            sorter: (a, b) => a.title.localeCompare(b.title),


Suggested change

sorter: (a, b) => a.title.localeCompare(b.title),

sorter: (a, b) => a.slug.localeCompare(b.slug),

Good catch!

marcelgerber · 2025-02-10T10:28:20Z

adminSiteClient/MultiDimIndexPage.tsx

+
+    return (
+        <AdminLayout title="Multidimensional Data Pages">
+            <NotificationContext.Provider value={null}>


I don't think we need this extra context, since it's always null and not antd-specific anyhow?

Yep, you are right. I based my code on the images admin, it was a total cargo culting. 🙂

marcelgerber · 2025-02-10T10:37:08Z

adminSiteClient/MultiDimIndexPage.tsx

The combination of react-query and antd makes this page feel really nice! 🎉

marcelgerber

All good now! Thanks for addressing these.

rakyi · 2025-02-12T10:11:19Z

Thanks for the review! Can you check again the last commit, pls?

marcelgerber · 2025-02-12T10:47:18Z

adminSiteServer/apiRoutes/mdims.ts

+    if (!(await multiDimDataPageExists(trx, { slug }))) {
+        await validateNewGrapherSlug(trx, slug)
+    }


hm, do we need that if here?
If a mdim already exists under this slug, and we're trying to create a new one, then this will fail either way: at the very least, it'll fail at the UNIQUE index on the DB table.

We need the if, because this handler does an upsert. It won't fail on the unique constraint, it will update the existing row(s) if the mdim already exists.

I see, but then we need to actually fail if an mdim exists. Which we currently don't do!

I renamed the function we call to clear up the confusion about create vs upsert.

marcelgerber · 2025-02-12T10:49:52Z

adminSiteServer/apiRoutes/mdims.ts

            action = "publish"
        }
    }
+    // Note: Keep this change last, since we don't want to update the configs


I don't know precisely when a DB rollback will happen, honestly.
Could it happen that we change the slug (and propagate that to R2), and then roll that back in the DB afterwards?

It can still happen, it can roll back any time we throw an error before the helper knex function somewhere in our Express handler returns. But the later we do the R2 upload, the better.

We have this problem in many places, and it's fine. The proper way to do this would be with https://en.wikipedia.org/wiki/Change_data_capture, if we really needed the correctness, i.e. first commit the changes to the DB and then propagate the changes to external systems.

rakyi · 2025-02-12T14:27:04Z

Remove MultiDimDataPage feature flag #4554
Add admin for multidimensional data pages #4505 👈 (View in Graphite)
master

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

github-actions bot assigned rakyi Jan 30, 2025

rakyi force-pushed the mdim-admin branch from 72fc603 to a6d1a53 Compare January 30, 2025 15:07

rakyi force-pushed the mdim-admin branch 5 times, most recently from 3b776df to b74eab7 Compare February 6, 2025 16:24

rakyi commented Feb 6, 2025

View reviewed changes

rakyi requested a review from marcelgerber February 6, 2025 16:32

rakyi marked this pull request as ready for review February 6, 2025 16:32

rakyi commented Feb 6, 2025

View reviewed changes

rakyi force-pushed the mdim-admin branch 3 times, most recently from 9c3aa88 to a73c680 Compare February 7, 2025 09:25

Add admin for multidimensional data pages

f158ace

rakyi force-pushed the mdim-admin branch from a73c680 to f158ace Compare February 7, 2025 10:06

marcelgerber reviewed Feb 10, 2025

View reviewed changes

Address PR comments

b5e73ed

rakyi requested a review from marcelgerber February 12, 2025 08:44

marcelgerber approved these changes Feb 12, 2025

View reviewed changes

Fix the update and validation logic

a8f698d

rakyi requested a review from marcelgerber February 12, 2025 10:11

marcelgerber reviewed Feb 12, 2025

View reviewed changes

Rename createMultiDimConfig to upsertMultiDimConfig

04a5f0f

rakyi mentioned this pull request Feb 12, 2025

Remove MultiDimDataPage feature flag #4554

Open

	sorter: (a, b) => a.title.localeCompare(b.title),
	sorter: (a, b) => a.slug.localeCompare(b.slug),

Add admin for multidimensional data pages #4505

Are you sure you want to change the base?

Add admin for multidimensional data pages #4505

Conversation

rakyi commented Jan 30, 2025 • edited Loading

owidbot commented Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber left a comment

Choose a reason for hiding this comment

rakyi commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakyi commented Feb 12, 2025 • edited Loading

rakyi commented Jan 30, 2025 •

edited

Loading

owidbot commented Jan 30, 2025 •

edited

Loading

rakyi commented Feb 12, 2025 •

edited

Loading