✨ make inheritance opt-in

owid · Aug 13, 2024 · 57ccd76 · 57ccd76
1 parent 6c78c2d
commit 57ccd76
Show file tree

Hide file tree

Showing 14 changed files with 345 additions and 200 deletions.
diff --git a/adminSiteClient/AbstractChartEditor.ts b/adminSiteClient/AbstractChartEditor.ts
@@ -6,7 +6,7 @@ import {
     mergeGrapherConfigs,
     merge,
 } from "@ourworldindata/utils"
-import { computed, observable, when } from "mobx"
+import { action, computed, observable, when } from "mobx"
 import { EditorFeatures } from "./EditorFeatures.js"
 import { Admin } from "./Admin.js"
 import { defaultGrapherConfig, Grapher } from "@ourworldindata/grapher"
@@ -28,6 +28,7 @@ export interface AbstractChartEditorManager {
     admin: Admin
     patchConfig: GrapherInterface
     parentConfig?: GrapherInterface
+    isInheritanceEnabled?: boolean
 }
 
 export abstract class AbstractChartEditor<
@@ -42,7 +43,11 @@ export abstract class AbstractChartEditor<
     @observable.ref previewMode: "mobile" | "desktop"
     @observable.ref showStaticPreview = false
     @observable.ref savedPatchConfig: GrapherInterface = {}
+
+    // parent config derived from the current chart config (not necessarily in use)
     @observable.ref parentConfig: GrapherInterface | undefined = undefined
+    // if inheritance is enabled, the parent config is applied to grapher
+    @observable.ref isInheritanceEnabled: boolean | undefined = undefined
 
     constructor(props: { manager: Manager }) {
         this.manager = props.manager
@@ -56,14 +61,20 @@ export abstract class AbstractChartEditor<
             () => (this.parentConfig = this.manager.parentConfig)
         )
 
+        when(
+            () => this.manager.isInheritanceEnabled !== undefined,
+            () =>
+                (this.isInheritanceEnabled = this.manager.isInheritanceEnabled)
+        )
+
         when(
             () => this.grapher.hasData && this.grapher.isReady,
             () => (this.savedPatchConfig = this.patchConfig)
         )
     }
 
     /** default object with all possible keys */
-    private fullDefaultObject = merge(
+    fullDefaultObject = merge(
         {},
         Grapher.defaultObject(), // contains all keys
         defaultGrapherConfig // contains a subset of keys with the right defaults
@@ -81,16 +92,27 @@ export abstract class AbstractChartEditor<
         return mergeGrapherConfigs(this.fullDefaultObject, this.grapher.object)
     }
 
+    /** patch config of the chart that is written to the db on save */
     @computed get patchConfig(): GrapherInterface {
         return diffGrapherConfigs(
             this.fullConfig,
-            this.parentConfigWithDefaults ?? this.fullDefaultObject
+            this.activeParentConfigWithDefaults ?? this.fullDefaultObject
         )
     }
 
-    @computed get parentConfigWithDefaults(): GrapherInterface | undefined {
-        if (!this.parentConfig) return undefined
-        return mergeGrapherConfigs(this.fullDefaultObject, this.parentConfig)
+    /** parent config currently applied to grapher */
+    @computed get activeParentConfig(): GrapherInterface | undefined {
+        return this.isInheritanceEnabled ? this.parentConfig : undefined
+    }
+
+    @computed get activeParentConfigWithDefaults():
+        | GrapherInterface
+        | undefined {
+        if (!this.activeParentConfig) return undefined
+        return mergeGrapherConfigs(
+            this.fullDefaultObject,
+            this.activeParentConfig
+        )
     }
 
     @computed get isModified(): boolean {
@@ -104,12 +126,18 @@ export abstract class AbstractChartEditor<
         return new EditorFeatures(this)
     }
 
+    @action.bound updateLiveGrapher(config: GrapherInterface): void {
+        this.grapher.reset()
+        this.grapher.updateFromObject(config)
+        this.grapher.updateAuthoredVersion(config)
+    }
+
     // TODO: only works for first level at the moment
     isPropertyInherited(property: keyof GrapherInterface): boolean {
-        if (!this.parentConfig) return false
+        if (!this.activeParentConfig) return false
         return (
             !Object.hasOwn(this.patchConfig, property) &&
-            Object.hasOwn(this.parentConfig, property)
+            Object.hasOwn(this.activeParentConfig, property)
         )
     }
 

diff --git a/adminSiteClient/ChartEditor.ts b/adminSiteClient/ChartEditor.ts
@@ -12,11 +12,11 @@ import {
     ChartRedirect,
     Json,
     GrapherInterface,
-    getParentIndicatorIdFromChartConfig,
+    getParentVariableIdFromChartConfig,
     mergeGrapherConfigs,
     isEmpty,
 } from "@ourworldindata/utils"
-import { action, computed, observable, runInAction } from "mobx"
+import { action, computed, observable, runInAction, when } from "mobx"
 import { BAKED_GRAPHER_URL } from "../settings/clientSettings.js"
 import {
     AbstractChartEditor,
@@ -98,7 +98,7 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
     @action.bound async updateParentConfig() {
         const currentParentIndicatorId =
             this.parentConfig?.dimensions?.[0].variableId
-        const newParentIndicatorId = getParentIndicatorIdFromChartConfig(
+        const newParentIndicatorId = getParentVariableIdFromChartConfig(
             this.grapher.object
         )
 
@@ -109,22 +109,25 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
             (currentParentIndicatorId === undefined ||
                 newParentIndicatorId !== currentParentIndicatorId)
         ) {
-            newParentConfig = await fetchParentConfigForChart(
+            newParentConfig = await fetchMergedGrapherConfigByVariableId(
                 this.manager.admin,
                 newParentIndicatorId
             )
         }
 
+        // update the live grapher object
         const newConfig = mergeGrapherConfigs(
             newParentConfig ?? {},
             this.patchConfig
         )
-
-        this.grapher.reset()
-        this.grapher.updateFromObject(newConfig)
-        this.grapher.updateAuthoredVersion(newConfig)
+        this.updateLiveGrapher(newConfig)
 
         this.parentConfig = newParentConfig
+
+        // disable inheritance if there is no parent config
+        if (!this.parentConfig) {
+            this.isInheritanceEnabled = false
+        }
     }
 
     async saveGrapher({
@@ -137,9 +140,12 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
         if (!patchConfig.title) patchConfig.title = grapher.displayTitle
         if (!patchConfig.slug) patchConfig.slug = grapher.displaySlug
 
+        const query = new URLSearchParams({
+            inheritance: this.isInheritanceEnabled ? "1" : "0",
+        })
         const targetUrl = isNewGrapher
-            ? "/api/charts"
-            : `/api/charts/${grapher.id}`
+            ? `/api/charts?${query}`
+            : `/api/charts/${grapher.id}?${query}`
 
         const json = await this.manager.admin.requestJSON(
             targetUrl,
@@ -172,8 +178,13 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
         // Need to open intermediary tab before AJAX to avoid popup blockers
         const w = window.open("/", "_blank") as Window
 
+        const query = new URLSearchParams({
+            inheritance: this.isInheritanceEnabled ? "1" : "0",
+        })
+        const targetUrl = `/api/charts?${query}`
+
         const json = await this.manager.admin.requestJSON(
-            "/api/charts",
+            targetUrl,
             chartJson,
             "POST"
         )
@@ -208,7 +219,7 @@ export class ChartEditor extends AbstractChartEditor<ChartEditorManager> {
     }
 }
 
-export async function fetchParentConfigForChart(
+export async function fetchMergedGrapherConfigByVariableId(
     admin: Admin,
     indicatorId: number
 ): Promise<GrapherInterface | undefined> {

diff --git a/adminSiteClient/ChartEditorPage.tsx b/adminSiteClient/ChartEditorPage.tsx
@@ -2,7 +2,7 @@ import React from "react"
 import { observer } from "mobx-react"
 import { observable, computed, runInAction, action } from "mobx"
 import {
-    getParentIndicatorIdFromChartConfig,
+    getParentVariableIdFromChartConfig,
     RawPageview,
     isEmpty,
 } from "@ourworldindata/utils"
@@ -13,7 +13,7 @@ import {
     Log,
     References,
     ChartEditorManager,
-    fetchParentConfigForChart,
+    fetchMergedGrapherConfigByVariableId,
 } from "./ChartEditor.js"
 import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
 import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js"
@@ -33,6 +33,8 @@ export class ChartEditorPage
     patchConfig: GrapherInterface = {}
     parentConfig: GrapherInterface | undefined = undefined
 
+    isInheritanceEnabled: boolean | undefined = undefined
+
     async fetchGrapherConfig(): Promise<void> {
         const { grapherId, grapherConfig } = this.props
         if (grapherId !== undefined) {
@@ -47,19 +49,21 @@ export class ChartEditorPage
     async fetchParentConfig(): Promise<void> {
         const { grapherId, grapherConfig } = this.props
         if (grapherId !== undefined) {
-            const parentConfig = await this.context.admin.getJSON(
-                `/api/charts/${grapherId}.parentConfig.json`
+            const parent = await this.context.admin.getJSON(
+                `/api/charts/${grapherId}.parent.json`
             )
-            this.parentConfig = isEmpty(parentConfig) ? undefined : parentConfig
+            this.parentConfig = parent?.config
+            this.isInheritanceEnabled = parent?.isActive ?? false
         } else if (grapherConfig) {
             const parentIndicatorId =
-                getParentIndicatorIdFromChartConfig(grapherConfig)
+                getParentVariableIdFromChartConfig(grapherConfig)
             if (parentIndicatorId) {
-                this.parentConfig = await fetchParentConfigForChart(
+                this.parentConfig = await fetchMergedGrapherConfigByVariableId(
                     this.context.admin,
                     parentIndicatorId
                 )
             }
+            this.isInheritanceEnabled = false
         }
     }
 

diff --git a/adminSiteClient/EditorBasicTab.tsx b/adminSiteClient/EditorBasicTab.tsx
@@ -181,8 +181,9 @@ class DimensionSlotView<
     }
 
     @action.bound private updateParentConfig() {
-        if (isChartEditorInstance(this.props.editor)) {
-            void this.props.editor.updateParentConfig()
+        const { editor } = this.props
+        if (isChartEditorInstance(editor)) {
+            void editor.updateParentConfig()
         }
     }
 

diff --git a/adminSiteClient/EditorInheritanceTab.tsx b/adminSiteClient/EditorInheritanceTab.tsx
@@ -1,54 +1,84 @@
 import React from "react"
 import { observer } from "mobx-react"
-import { AbstractChartEditor } from "./AbstractChartEditor.js"
-import { Section } from "./Forms.js"
+import { Section, Toggle } from "./Forms.js"
+import { ChartEditor } from "./ChartEditor.js"
+import { action } from "mobx"
+import { mergeGrapherConfigs } from "@ourworldindata/utils"
 
 @observer
-export class EditorInheritanceTab<
-    Editor extends AbstractChartEditor,
-> extends React.Component<{ editor: Editor }> {
-    render() {
-        const { parentConfig, fullConfig, grapher } = this.props.editor
+export class EditorInheritanceTab extends React.Component<{
+    editor: ChartEditor
+}> {
+    @action.bound onToggleInheritance(newValue: boolean) {
+        const { patchConfig, parentConfig } = this.props.editor
+
+        // update live grapher
+        const newParentConfig = newValue ? parentConfig : undefined
+        const newConfig = mergeGrapherConfigs(
+            newParentConfig ?? {},
+            patchConfig
+        )
+        this.props.editor.updateLiveGrapher(newConfig)
 
-        if (!parentConfig)
-            return (
-                <div className="InheritanceTab">
-                    Doesn't inherit settings from any indicator
-                </div>
-            )
+        this.props.editor.isInheritanceEnabled = newValue
+    }
 
-        const parentIndicatorId = parentConfig.dimensions?.[0].variableId
+    render() {
+        const {
+            parentConfig,
+            isInheritanceEnabled = false,
+            fullConfig,
+            grapher,
+        } = this.props.editor
 
-        if (!parentIndicatorId)
-            return (
-                <div className="InheritanceTab">
-                    Does inherit settings from any indicator but can't find the
-                    indicator's id (shouldn't happen, please report this bug!)
-                </div>
-            )
+        const parentIndicatorId = parentConfig?.dimensions?.[0].variableId
+        const column = parentIndicatorId
+            ? grapher.inputTable.get(parentIndicatorId.toString())
+            : undefined
 
-        const column = grapher.inputTable.get(parentIndicatorId.toString())
+        const parentVariableEditLink = (
+            <a
+                href={`/admin/variables/${parentIndicatorId}`}
+                target="_blank"
+                rel="noopener"
+            >
+                {column?.name ?? parentIndicatorId}
+            </a>
+        )
 
         return (
             <div className="InheritanceTab">
                 <Section name="Parent indicator">
-                    Inherits settings from indicator{" "}
-                    <a
-                        href={`/admin/variables/${parentIndicatorId}`}
-                        target="_blank"
-                        rel="noopener"
-                    >
-                        {column.name}.
-                    </a>
-                </Section>
-                <Section name="Inherited Config">
-                    <textarea
-                        rows={7}
-                        readOnly
-                        className="form-control"
-                        value={JSON.stringify(parentConfig, undefined, 2)}
+                    {isInheritanceEnabled ? (
+                        <p>
+                            This chart inherits settings from indicator{" "}
+                            {parentVariableEditLink}. Toggle the option below to
+                            disable inheritance.
+                        </p>
+                    ) : (
+                        <p>
+                            This chart may inherit settings from indicator{" "}
+                            {parentVariableEditLink}. Toggle the option below to
+                            enable inheritance and enrich this chart's config by
+                            indicator-level settings.
+                        </p>
+                    )}
+                    <Toggle
+                        label="Inherit settings from indicator"
+                        value={isInheritanceEnabled}
+                        onValue={this.onToggleInheritance}
                     />
                 </Section>
+                {isInheritanceEnabled && (
+                    <Section name="Inherited Config">
+                        <textarea
+                            rows={7}
+                            readOnly
+                            className="form-control"
+                            value={JSON.stringify(parentConfig, undefined, 2)}
+                        />
+                    </Section>
+                )}
                 <Section name="Full Config">
                     <textarea
                         rows={7}