Address PR comments

owid · Feb 12, 2025 · b5e73ed · b5e73ed
1 parent f158ace
commit b5e73ed
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 43 deletions.
diff --git a/adminSiteClient/MultiDimIndexPage.tsx b/adminSiteClient/MultiDimIndexPage.tsx
@@ -4,7 +4,7 @@ import {
     useQuery,
     useQueryClient,
 } from "@tanstack/react-query"
-import { createContext, useContext, useEffect, useMemo, useState } from "react"
+import { useContext, useEffect, useMemo, useState } from "react"
 import {
     Input,
     Popconfirm,
@@ -46,7 +46,7 @@ function SlugField({
     id: number
     slug: string
     slugMutation: UseMutationResult<
-        unknown,
+        MultiDim,
         unknown,
         { id: number; slug: string },
         unknown
@@ -65,13 +65,13 @@ function SlugField({
 
 function createColumns(
     slugMutation: UseMutationResult<
-        unknown,
+        MultiDim,
         unknown,
         { id: number; slug: string },
         unknown
     >,
     publishMutation: UseMutationResult<
-        unknown,
+        MultiDim,
         unknown,
         { id: number; published: boolean },
         unknown
@@ -131,7 +131,7 @@ function createColumns(
             render: (slug, { id }) => (
                 <SlugField id={id} slug={slug} slugMutation={slugMutation} />
             ),
-            sorter: (a, b) => a.title.localeCompare(b.title),
+            sorter: (a, b) => a.slug.localeCompare(b.slug),
         },
         {
             title: "Last updated",
@@ -198,8 +198,6 @@ async function patchMultiDim(admin: Admin, id: number, data: Json) {
     return deserializeMultiDim(multiDim)
 }
 
-const NotificationContext = createContext(null)
-
 export function MultiDimIndexPage() {
     const { admin } = useContext(AdminAppContext)
     const [notificationApi, notificationContextHolder] =
@@ -260,23 +258,21 @@ export function MultiDimIndexPage() {
 
     return (
         <AdminLayout title="Multidimensional Data Pages">
-            <NotificationContext.Provider value={null}>
-                {notificationContextHolder}
-                <main>
-                    <Input
-                        placeholder="Search by title or slug"
-                        value={search}
-                        onChange={(e) => setSearch(e.target.value)}
-                        style={{ width: 500, marginBottom: 20 }}
-                        autoFocus
-                    />
-                    <Table
-                        columns={columns}
-                        dataSource={filteredMdims}
-                        rowKey={(x) => x.id}
-                    />
-                </main>
-            </NotificationContext.Provider>
+            {notificationContextHolder}
+            <main>
+                <Input
+                    placeholder="Search by title or slug"
+                    value={search}
+                    onChange={(e) => setSearch(e.target.value)}
+                    style={{ width: 500, marginBottom: 20 }}
+                    autoFocus
+                />
+                <Table
+                    columns={columns}
+                    dataSource={filteredMdims}
+                    rowKey={(x) => x.id}
+                />
+            </main>
         </AdminLayout>
     )
 }
diff --git a/adminSiteServer/apiRoutes/charts.ts b/adminSiteServer/apiRoutes/charts.ts
@@ -288,9 +288,7 @@ export const saveGrapher = async (
 
     // When a chart is published, check for conflicts
     if (newConfig.isPublished) {
-        if (newConfig.slug) {
-            await validateGrapherSlug(knex, newConfig.slug, existingConfig?.id)
-        }
+        await validateGrapherSlug(knex, newConfig.slug, existingConfig?.id)
         if (
             existingConfig &&
             existingConfig.isPublished &&

diff --git a/adminSiteServer/apiRoutes/mdims.ts b/adminSiteServer/apiRoutes/mdims.ts
@@ -20,7 +20,7 @@ import {
 import { triggerStaticBuild } from "./routeUtils.js"
 import { Request } from "../authentication.js"
 import * as db from "../../db/db.js"
-import { validateGrapherSlug } from "../validation.js"
+import { validateGrapherSlug, validateMultiDimSlug } from "../validation.js"
 import e from "express"
 
 export async function handleGetMultiDims(
@@ -72,6 +72,7 @@ export async function handlePutMultiDim(
         FEATURE_FLAGS.has(FeatureFlagFeature.MultiDimDataPage) &&
         (await multiDimDataPageExists(trx, { slug, published: true }))
     ) {
+        await validateMultiDimSlug(trx, slug)
         await triggerStaticBuild(
             res.locals.user,
             `Publishing multidimensional chart ${slug}`
@@ -104,6 +105,7 @@ export async function handlePatchMultiDim(
         }
     }
     if (action) {
+        await validateMultiDimSlug(trx, multiDim.slug)
         await triggerStaticBuild(
             res.locals.user,
             `${action === "publish" ? "Publishing" : "Unpublishing"} multidimensional chart ${multiDim.slug}`

diff --git a/adminSiteServer/validation.ts b/adminSiteServer/validation.ts
@@ -47,7 +47,7 @@ async function isSlugUsedInOtherGrapher(
 
 export async function validateGrapherSlug(
     knex: db.KnexReadonlyTransaction,
-    slug: string,
+    slug?: string,
     existingConfigId?: number
 ) {
     if (!isValidSlug(slug)) {
@@ -65,7 +65,32 @@ export async function validateGrapherSlug(
     }
     if (await multiDimDataPageExists(knex, { slug })) {
         throw new JsonError(
-            `This slug is in use by a multi-dimensional data page: ${slug}`
+            `This slug is in use by a multidimensional data page: ${slug}`
+        )
+    }
+    return slug
+}
+
+/**
+ * Validates an existing slug of a multidimensional data page,
+ * e.g. before publishing.
+ */
+export async function validateMultiDimSlug(
+    knex: db.KnexReadonlyTransaction,
+    slug: string,
+    existingConfigId?: number
+) {
+    if (!isValidSlug(slug)) {
+        throw new JsonError(`Invalid chart slug ${slug}`)
+    }
+    if (await isSlugUsedInRedirect(knex, slug, existingConfigId)) {
+        throw new JsonError(
+            `This chart slug was previously used by another chart: ${slug}`
+        )
+    }
+    if (await isSlugUsedInOtherGrapher(knex, slug, existingConfigId)) {
+        throw new JsonError(
+            `This chart slug is in use by another published chart: ${slug}`
         )
     }
     return slug

diff --git a/db/model/MultiDimDataPage.ts b/db/model/MultiDimDataPage.ts
@@ -30,17 +30,6 @@ export async function upsertMultiDimDataPage(
     return result[0]
 }
 
-export async function isMultiDimDataPagePublished(
-    knex: KnexReadonlyTransaction,
-    slug: string
-): Promise<boolean> {
-    const result = await knex(MultiDimDataPagesTableName)
-        .select(knex.raw("1"))
-        .where({ slug, published: true })
-        .first()
-    return Boolean(result)
-}
-
 export async function multiDimDataPageExists(
     knex: KnexReadonlyTransaction,
     data: Partial<DbPlainMultiDimDataPage>

diff --git a/serverUtils/serverUtil.tsx b/serverUtils/serverUtil.tsx
@@ -16,8 +16,8 @@ export const renderToHtmlPage = (element: any) =>
     `<!doctype html>${ReactDOMServer.renderToStaticMarkup(element)}`
 
 // Determine if input is suitable for use as a url slug
-export const isValidSlug = (slug: any) =>
-    lodash.isString(slug) && slug.length > 1 && slug.match(/^[\w-]+$/)
+export const isValidSlug = (slug: any): slug is string =>
+    lodash.isString(slug) && slug.length > 1 && slug.match(/^[\w-]+$/) !== null
 
 export function base64ToBytes(base64: Base64String): Uint8Array {
     return Buffer.from(base64, "base64")