From cbba53ef739115857fd8fa685abbe64df81971f7 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Fri, 19 Jul 2024 10:09:16 +0000 Subject: [PATCH 01/39] =?UTF-8?q?=F0=9F=8E=89=20(grapher)=20make=20charts?= =?UTF-8?q?=20inherit=20indicator-level=20settings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/AbstractChartEditor.ts | 103 ++++ adminSiteClient/AdminApp.tsx | 12 + adminSiteClient/ChartEditor.ts | 193 ++---- adminSiteClient/ChartEditorContext.ts | 6 + adminSiteClient/ChartEditorPage.tsx | 509 ++-------------- adminSiteClient/ChartEditorView.tsx | 553 ++++++++++++++++++ adminSiteClient/DimensionCard.tsx | 9 +- adminSiteClient/EditorBasicTab.tsx | 43 +- adminSiteClient/EditorCustomizeTab.tsx | 30 +- adminSiteClient/EditorDataTab.tsx | 10 +- adminSiteClient/EditorExportTab.tsx | 12 +- adminSiteClient/EditorFeatures.tsx | 6 +- adminSiteClient/EditorMapTab.tsx | 6 +- adminSiteClient/EditorReferencesTab.tsx | 7 +- adminSiteClient/EditorTextTab.tsx | 38 +- adminSiteClient/GrapherConfigGridEditor.tsx | 9 +- adminSiteClient/IndicatorChartEditor.ts | 40 ++ adminSiteClient/IndicatorChartEditorPage.tsx | 72 +++ adminSiteClient/SaveButtons.tsx | 65 +- adminSiteClient/VariableSelector.tsx | 19 +- adminSiteServer/apiRouter.ts | 265 +++++++-- adminSiteServer/app.test.tsx | 207 ++++++- baker/GrapherBaker.tsx | 9 +- baker/siteRenderers.tsx | 16 +- ...veIndicatorChartsToTheChartConfigsTable.ts | 239 ++++++++ db/model/Chart.ts | 66 ++- db/model/ChartConfigs.ts | 68 +++ db/model/Variable.ts | 349 ++++++++++- db/tests/testHelpers.ts | 30 +- packages/@ourworldindata/grapher/src/index.ts | 1 + .../src/schema/grapher-schema.004.yaml | 375 ++++++------ .../types/src/dbTypes/InheritingCharts.ts | 8 + .../types/src/dbTypes/Variables.ts | 43 +- packages/@ourworldindata/types/src/index.ts | 4 - packages/@ourworldindata/utils/src/Util.ts | 27 +- packages/@ourworldindata/utils/src/index.ts | 2 +- site/DataPageV2.tsx | 7 +- 37 files changed, 2434 insertions(+), 1024 deletions(-) create mode 100644 adminSiteClient/AbstractChartEditor.ts create mode 100644 adminSiteClient/ChartEditorContext.ts create mode 100644 adminSiteClient/ChartEditorView.tsx create mode 100644 adminSiteClient/IndicatorChartEditor.ts create mode 100644 adminSiteClient/IndicatorChartEditorPage.tsx create mode 100644 db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts create mode 100644 db/model/ChartConfigs.ts create mode 100644 packages/@ourworldindata/types/src/dbTypes/InheritingCharts.ts diff --git a/adminSiteClient/AbstractChartEditor.ts b/adminSiteClient/AbstractChartEditor.ts new file mode 100644 index 00000000000..c425e58797f --- /dev/null +++ b/adminSiteClient/AbstractChartEditor.ts @@ -0,0 +1,103 @@ +import { + isEqual, + omit, + GrapherInterface, + diffGrapherConfigs, + mergeGrapherConfigs, +} from "@ourworldindata/utils" +import { computed, observable, when } from "mobx" +import { EditorFeatures } from "./EditorFeatures.js" +import { Admin } from "./Admin.js" +import { defaultGrapherConfig, Grapher } from "@ourworldindata/grapher" + +export type EditorTab = + | "basic" + | "data" + | "text" + | "customize" + | "map" + | "scatter" + | "marimekko" + | "revisions" + | "refs" + | "export" + +export interface AbstractChartEditorManager { + admin: Admin + patchConfig: GrapherInterface + parentConfig?: GrapherInterface +} + +export abstract class AbstractChartEditor< + Manager extends AbstractChartEditorManager = AbstractChartEditorManager, +> { + manager: Manager + + @observable.ref grapher = new Grapher() + @observable.ref currentRequest: Promise | undefined // Whether the current chart state is saved or not + @observable.ref tab: EditorTab = "basic" + @observable.ref errorMessage?: { title: string; content: string } + @observable.ref previewMode: "mobile" | "desktop" + @observable.ref showStaticPreview = false + @observable.ref savedPatchConfig: GrapherInterface = {} + @observable.ref parentConfig: GrapherInterface | undefined = undefined + + constructor(props: { manager: Manager }) { + this.manager = props.manager + this.previewMode = + localStorage.getItem("editorPreviewMode") === "mobile" + ? "mobile" + : "desktop" + + when( + () => this.manager.parentConfig !== undefined, + () => (this.parentConfig = this.manager.parentConfig) + ) + + when( + () => this.grapher.hasData && this.grapher.isReady, + () => (this.savedPatchConfig = this.patchConfig) + ) + } + + @computed get originalGrapherConfig(): GrapherInterface { + const { patchConfig, parentConfig } = this.manager + if (!parentConfig) return patchConfig + return mergeGrapherConfigs(parentConfig, patchConfig) + } + + @computed get liveConfig(): GrapherInterface { + return this.grapher.object + } + + @computed get patchConfig(): GrapherInterface { + const { liveConfig, parentConfig } = this + if (!parentConfig) return liveConfig + // Grapher's toObject method doesn't include default values, + // but the patch config might need to overwrite non-default values + // from the parent layer. That's why we merge the default grapher config + // in here. The parent config also contains defaults, meaning we're + // getting rid of the defaults when we diff against the parent config below. + const liveConfigWithDefaults = mergeGrapherConfigs( + defaultGrapherConfig, + liveConfig + ) + return diffGrapherConfigs(liveConfigWithDefaults, parentConfig) + } + + @computed get isModified(): boolean { + return !isEqual( + omit(this.patchConfig, "version"), + omit(this.savedPatchConfig, "version") + ) + } + + @computed get features(): EditorFeatures { + return new EditorFeatures(this) + } + + abstract get isNewGrapher(): boolean + abstract get availableTabs(): EditorTab[] + + abstract saveGrapher(props?: { onError?: () => void }): Promise +} diff --git a/adminSiteClient/AdminApp.tsx b/adminSiteClient/AdminApp.tsx index 56804458a34..014ae1609f1 100644 --- a/adminSiteClient/AdminApp.tsx +++ b/adminSiteClient/AdminApp.tsx @@ -41,6 +41,7 @@ import { BulkGrapherConfigEditorPage } from "./BulkGrapherConfigEditor.js" import { GdocsIndexPage, GdocsMatchProps } from "./GdocsIndexPage.js" import { GdocsPreviewPage } from "./GdocsPreviewPage.js" import { GdocsStoreProvider } from "./GdocsStore.js" +import { IndicatorChartEditorPage } from "./IndicatorChartEditorPage.js" @observer class AdminErrorMessage extends React.Component<{ admin: Admin }> { @@ -154,6 +155,17 @@ export class AdminApp extends React.Component<{ path="/charts" component={ChartIndexPage} /> + ( + + )} + /> { ) } -export interface Namespace { - name: string - description?: string - isArchived: boolean -} - -// This contains the dataset/variable metadata for the entire database -// Used for variable selector interface - -export interface NamespaceData { - datasets: Dataset[] -} - -export class EditorDatabase { - @observable.ref namespaces: Namespace[] - @observable.ref variableUsageCounts: Map = new Map() - @observable dataByNamespace: Map = new Map() - - constructor(json: any) { - this.namespaces = json.namespaces - } -} - -export type FieldWithDetailReferences = - | "subtitle" - | "note" - | "axisLabelX" - | "axisLabelY" - -export type DetailReferences = Record - -export interface DimensionErrorMessage { - displayName?: string -} - -export interface ChartEditorManager { - admin: Admin - grapher: Grapher - database: EditorDatabase +export interface ChartEditorManager extends AbstractChartEditorManager { logs: Log[] references: References | undefined redirects: ChartRedirect[] pageviews?: RawPageview allTopics: Topic[] - details: DetailDictionary - invalidDetailReferences: DetailReferences - errorMessages: Partial> - errorMessagesForDimensions: Record< - DimensionProperty, - DimensionErrorMessage[] - > } -interface VariableIdUsageRecord { - variableId: number - usageCount: number -} - -export class ChartEditor { - manager: ChartEditorManager - // Whether the current chart state is saved or not - @observable.ref currentRequest: Promise | undefined - @observable.ref tab: EditorTab = "basic" - @observable.ref errorMessage?: { title: string; content: string } - @observable.ref previewMode: "mobile" | "desktop" - @observable.ref showStaticPreview = false - @observable.ref savedGrapherJson: string = "" - +export class ChartEditor extends AbstractChartEditor { // This gets set when we save a new chart for the first time // so the page knows to update the url @observable.ref newChartId?: number - constructor(props: { manager: ChartEditorManager }) { - this.manager = props.manager - this.previewMode = - localStorage.getItem("editorPreviewMode") === "mobile" - ? "mobile" - : "desktop" - when( - () => this.grapher.isReady, - () => (this.savedGrapherJson = JSON.stringify(this.grapher.object)) - ) - } - - @computed get isModified(): boolean { - return JSON.stringify(this.grapher.object) !== this.savedGrapherJson - } - - @computed get grapher() { - return this.manager.grapher - } - - @computed get database() { - return this.manager.database - } - @computed get logs() { return this.manager.logs } @@ -174,10 +80,6 @@ export class ChartEditor { return this.manager.allTopics } - @computed get details() { - return this.manager.details - } - @computed get availableTabs(): EditorTab[] { const tabs: EditorTab[] = ["basic", "data", "text", "customize"] if (this.grapher.hasMapTab) tabs.push("map") @@ -193,36 +95,43 @@ export class ChartEditor { return this.grapher.id === undefined } - @computed get features() { - return new EditorFeatures(this) - } + @action.bound async updateParentConfig() { + const newIndicatorId = getParentIndicatorIdFromChartConfig( + this.liveConfig + ) + const currentIndicatorId = this.parentConfig?.dimensions?.[0].variableId + if (newIndicatorId !== currentIndicatorId) { + this.parentConfig = newIndicatorId + ? await fetchParentConfigForChart( + this.manager.admin, + newIndicatorId + ) + : undefined + } - async loadVariableUsageCounts(): Promise { - const data = (await this.manager.admin.getJSON( - `/api/variables.usages.json` - )) as VariableIdUsageRecord[] - const finalData = new Map( - data.map(({ variableId, usageCount }: VariableIdUsageRecord) => [ - variableId, - +usageCount, - ]) + // it's intentional that we don't use this.patchConfig here + const patchConfig = diffGrapherConfigs( + this.liveConfig, + this.parentConfig ?? {} ) - runInAction(() => (this.database.variableUsageCounts = finalData)) + const config = mergeGrapherConfigs(this.parentConfig ?? {}, patchConfig) + + this.grapher.updateFromObject(config) + // TODO(inheritance): what does this do? is this necessary? + this.grapher.updateAuthoredVersion({ + ...this.grapher.toObject(), + }) } async saveGrapher({ onError, }: { onError?: () => void } = {}): Promise { - const { grapher, isNewGrapher } = this - const currentGrapherObject = this.grapher.object + const { grapher, isNewGrapher, patchConfig } = this // Chart title and slug may be autocalculated from data, in which case they won't be in props // But the server will need to know what we calculated in order to do its job - if (!currentGrapherObject.title) - currentGrapherObject.title = grapher.displayTitle - - if (!currentGrapherObject.slug) - currentGrapherObject.slug = grapher.displaySlug + if (!patchConfig.title) patchConfig.title = grapher.displayTitle + if (!patchConfig.slug) patchConfig.slug = grapher.displaySlug const targetUrl = isNewGrapher ? "/api/charts" @@ -230,7 +139,7 @@ export class ChartEditor { const json = await this.manager.admin.requestJSON( targetUrl, - currentGrapherObject, + patchConfig, isNewGrapher ? "POST" : "PUT" ) @@ -250,9 +159,9 @@ export class ChartEditor { } async saveAsNewGrapher(): Promise { - const currentGrapherObject = this.grapher.object + const { patchConfig } = this - const chartJson = { ...currentGrapherObject } + const chartJson = { ...patchConfig } delete chartJson.id delete chartJson.isPublished @@ -294,3 +203,19 @@ export class ChartEditor { } } } + +export async function fetchParentConfigForChart( + admin: Admin, + indicatorId: number +): Promise { + const indicatorChart = await admin.getJSON( + `/api/variables/mergedGrapherConfig/${indicatorId}.json` + ) + return mergeGrapherConfigs(defaultGrapherConfig, indicatorChart) +} + +export function isChartEditorInstance( + editor: AbstractChartEditor +): editor is ChartEditor { + return editor instanceof ChartEditor +} diff --git a/adminSiteClient/ChartEditorContext.ts b/adminSiteClient/ChartEditorContext.ts new file mode 100644 index 00000000000..f57b8c0d2e5 --- /dev/null +++ b/adminSiteClient/ChartEditorContext.ts @@ -0,0 +1,6 @@ +import React from "react" + +export const ChartEditorContext = React.createContext({ + errorMessages: {}, + errorMessagesForDimensions: {}, +}) diff --git a/adminSiteClient/ChartEditorPage.tsx b/adminSiteClient/ChartEditorPage.tsx index e1d1a49fc82..3b7cc6d1279 100644 --- a/adminSiteClient/ChartEditorPage.tsx +++ b/adminSiteClient/ChartEditorPage.tsx @@ -1,188 +1,63 @@ import React from "react" import { observer } from "mobx-react" +import { observable, computed, runInAction, action } from "mobx" import { - observable, - computed, - runInAction, - autorun, - action, - reaction, - IReactionDisposer, -} from "mobx" -import { Prompt, Redirect } from "react-router-dom" -import { - Bounds, - capitalize, + getParentIndicatorIdFromChartConfig, RawPageview, - DetailDictionary, - get, - set, - groupBy, - extractDetailsFromSyntax, - getIndexableKeys, } from "@ourworldindata/utils" -import { - Topic, - GrapherInterface, - GrapherStaticFormat, - ChartRedirect, - DimensionProperty, -} from "@ourworldindata/types" -import { Grapher } from "@ourworldindata/grapher" +import { Topic, GrapherInterface, ChartRedirect } from "@ourworldindata/types" import { Admin } from "./Admin.js" import { ChartEditor, - EditorDatabase, Log, References, ChartEditorManager, - Dataset, - getFullReferencesCount, - DetailReferences, - FieldWithDetailReferences, + fetchParentConfigForChart, } from "./ChartEditor.js" -import { EditorBasicTab } from "./EditorBasicTab.js" -import { EditorDataTab } from "./EditorDataTab.js" -import { EditorTextTab } from "./EditorTextTab.js" -import { EditorCustomizeTab } from "./EditorCustomizeTab.js" -import { EditorScatterTab } from "./EditorScatterTab.js" -import { EditorMapTab } from "./EditorMapTab.js" -import { EditorHistoryTab } from "./EditorHistoryTab.js" -import { EditorReferencesTab } from "./EditorReferencesTab.js" -import { SaveButtons } from "./SaveButtons.js" -import { LoadingBlocker } from "./Forms.js" -import { AdminLayout } from "./AdminLayout.js" import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js" -import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" -import { faMobile, faDesktop } from "@fortawesome/free-solid-svg-icons" -import { - VisionDeficiency, - VisionDeficiencySvgFilters, - VisionDeficiencyDropdown, - VisionDeficiencyEntity, -} from "./VisionDeficiencies.js" -import { EditorMarimekkoTab } from "./EditorMarimekkoTab.js" -import { EditorExportTab } from "./EditorExportTab.js" -import { runDetailsOnDemand } from "../site/detailsOnDemand.js" - -@observer -class TabBinder extends React.Component<{ editor: ChartEditor }> { - dispose!: IReactionDisposer - componentDidMount(): void { - //window.addEventListener("hashchange", this.onHashChange) - this.onHashChange() - - this.dispose = autorun(() => { - //setTimeout(() => window.location.hash = `#${tab}-tab`, 100) - }) - } - - componentWillUnmount(): void { - //window.removeEventListener("hashchange", this.onHashChange) - this.dispose() - } - - render(): null { - return null - } - - @action.bound onHashChange(): void { - const match = window.location.hash.match(/#(.+?)-tab/) - if (match) { - const tab = match[1] - if ( - this.props.editor.grapher && - this.props.editor.availableTabs.includes(tab) - ) - this.props.editor.tab = tab - } - } -} +import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js" @observer export class ChartEditorPage extends React.Component<{ grapherId?: number - newGrapherIndex?: number - grapherConfig?: any + grapherConfig?: GrapherInterface }> - implements ChartEditorManager + implements ChartEditorManager, ChartEditorViewManager { - @observable.ref grapher = new Grapher() - @observable.ref database = new EditorDatabase({}) @observable logs: Log[] = [] @observable references: References | undefined = undefined - @observable redirects: ChartRedirect[] = [] - @observable pageviews?: RawPageview = undefined - @observable allTopics: Topic[] = [] - @observable details: DetailDictionary = {} - - @observable.ref grapherElement?: React.ReactElement - static contextType = AdminAppContext - context!: AdminAppContextType - - @observable simulateVisionDeficiency?: VisionDeficiency - fetchedGrapherConfig?: any + patchConfig: GrapherInterface = {} + parentConfig: GrapherInterface | undefined = undefined - async fetchGrapher(): Promise { - const { grapherId } = this.props + async fetchGrapherConfig(): Promise { + const { grapherId, grapherConfig } = this.props if (grapherId !== undefined) { - this.fetchedGrapherConfig = await this.context.admin.getJSON( - `/api/charts/${grapherId}.config.json` + this.patchConfig = await this.context.admin.getJSON( + `/api/charts/${grapherId}.patchConfig.json` ) + } else if (grapherConfig) { + this.patchConfig = grapherConfig } - this.updateGrapher() - } - - @observable private _isDbSet = false - @observable private _isGrapherSet = false - @computed get isReady(): boolean { - return this._isDbSet && this._isGrapherSet - } - - @action.bound private updateGrapher(): void { - const config = this.fetchedGrapherConfig ?? this.props.grapherConfig - const grapherConfig = { - ...config, - // binds the grapher instance to this.grapher - getGrapherInstance: (grapher: Grapher) => { - this.grapher = grapher - }, - dataApiUrlForAdmin: - this.context.admin.settings.DATA_API_FOR_ADMIN_UI, // passed this way because clientSettings are baked and need a recompile to be updated - bounds: this.bounds, - staticFormat: this.staticFormat, - } - this.grapher.renderToStatic = !!this.editor?.showStaticPreview - this.grapherElement = - this._isGrapherSet = true - } - - @action.bound private setDb(json: any): void { - this.database = new EditorDatabase(json) - this._isDbSet = true } - async fetchData(): Promise { - const { admin } = this.context - - const [namespaces, variables] = await Promise.all([ - admin.getJSON(`/api/editorData/namespaces.json`), - admin.getJSON(`/api/editorData/variables.json`), - ]) - - this.setDb(namespaces) - - const groupedByNamespace = groupBy( - variables.datasets, - (d) => d.namespace - ) - for (const namespace in groupedByNamespace) { - this.database.dataByNamespace.set(namespace, { - datasets: groupedByNamespace[namespace] as Dataset[], - }) + async fetchParentConfig(): Promise { + const { grapherId, grapherConfig } = this.props + if (grapherId !== undefined) { + this.parentConfig = await this.context.admin.getJSON( + `/api/charts/${grapherId}.parentConfig.json` + ) + } else if (grapherConfig) { + const parentIndicatorId = + getParentIndicatorIdFromChartConfig(grapherConfig) + if (parentIndicatorId) { + this.parentConfig = await fetchParentConfigForChart( + this.context.admin, + parentIndicatorId + ) + } } } @@ -234,136 +109,17 @@ export class ChartEditorPage runInAction(() => (this.allTopics = json.topics)) } - async fetchDetails(): Promise { - await runDetailsOnDemand() - - runInAction(() => { - if (window.details) this.details = window.details - }) - } - - @computed private get isMobilePreview(): boolean { - return this.editor?.previewMode === "mobile" - } - - @computed private get bounds(): Bounds { - return this.isMobilePreview - ? new Bounds(0, 0, 380, 525) - : this.grapher.defaultBounds - } - - @computed private get staticFormat(): GrapherStaticFormat { - return this.isMobilePreview - ? GrapherStaticFormat.square - : GrapherStaticFormat.landscape - } - - // unvalidated terms extracted from the subtitle and note fields - // these may point to non-existent details e.g. ["not_a_real_term", "pvotery"] - @computed - get currentDetailReferences(): DetailReferences { - return { - subtitle: extractDetailsFromSyntax(this.grapher.currentSubtitle), - note: extractDetailsFromSyntax(this.grapher.note), - axisLabelX: extractDetailsFromSyntax( - this.grapher.xAxisConfig.label ?? "" - ), - axisLabelY: extractDetailsFromSyntax( - this.grapher.yAxisConfig.label ?? "" - ), - } - } - - // the actual Detail objects, indexed by category.term - @computed get currentlyReferencedDetails(): GrapherInterface["details"] { - const grapherConfigDetails: GrapherInterface["details"] = {} - const allReferences = Object.values(this.currentDetailReferences).flat() - - allReferences.forEach((term) => { - const detail = get(this.details, term) - if (detail) { - set(grapherConfigDetails, term, detail) - } - }) - - return grapherConfigDetails - } - - @computed - get invalidDetailReferences(): ChartEditorManager["invalidDetailReferences"] { - const { subtitle, note, axisLabelX, axisLabelY } = - this.currentDetailReferences - return { - subtitle: subtitle.filter((term) => !this.details[term]), - note: note.filter((term) => !this.details[term]), - axisLabelX: axisLabelX.filter((term) => !this.details[term]), - axisLabelY: axisLabelY.filter((term) => !this.details[term]), - } - } - - @computed get errorMessages(): ChartEditorManager["errorMessages"] { - const { invalidDetailReferences } = this - - const errorMessages: ChartEditorManager["errorMessages"] = {} - - // add error messages for each field with invalid detail references - getIndexableKeys(invalidDetailReferences).forEach( - (key: FieldWithDetailReferences) => { - const references = invalidDetailReferences[key] - if (references.length) { - errorMessages[key] = - `Invalid detail(s) specified: ${references.join(", ")}` - } - } - ) - - return errorMessages - } - - @computed - get errorMessagesForDimensions(): ChartEditorManager["errorMessagesForDimensions"] { - const errorMessages: ChartEditorManager["errorMessagesForDimensions"] = - { - [DimensionProperty.y]: [], - [DimensionProperty.x]: [], - [DimensionProperty.color]: [], - [DimensionProperty.size]: [], - [DimensionProperty.table]: [], // not used - } - - this.grapher.dimensionSlots.forEach((slot) => { - slot.dimensions.forEach((dimension, dimensionIndex) => { - const details = extractDetailsFromSyntax( - dimension.display.name ?? "" - ) - const hasDetailsInDisplayName = details.length > 0 - - // add error message if details are referenced in the display name - if (hasDetailsInDisplayName) { - errorMessages[slot.property][dimensionIndex] = { - displayName: "Detail syntax is not supported", - } - } - }) - }) - - return errorMessages - } - @computed get admin(): Admin { return this.context.admin } - @computed get editor(): ChartEditor | undefined { - if (!this.isReady) return undefined - + @computed get editor(): ChartEditor { return new ChartEditor({ manager: this }) } @action.bound refresh(): void { - void this.fetchGrapher() - void this.fetchDetails() - void this.fetchData() + void this.fetchGrapherConfig() + void this.fetchParentConfig() void this.fetchLogs() void this.fetchRefs() void this.fetchRedirects() @@ -374,207 +130,18 @@ export class ChartEditorPage // this.fetchTopics() } - disposers: IReactionDisposer[] = [] - componentDidMount(): void { this.refresh() - - this.disposers.push( - reaction( - () => this.editor && this.editor.previewMode, - () => { - if (this.editor) { - localStorage.setItem( - "editorPreviewMode", - this.editor.previewMode - ) - } - this.updateGrapher() - } - ) - ) } + // TODO(inheritance) // This funny construction allows the "new chart" link to work by forcing an update // even if the props don't change - UNSAFE_componentWillReceiveProps(): void { - setTimeout(() => this.refresh(), 0) - } - - componentWillUnmount(): void { - this.disposers.forEach((dispose) => dispose()) - } + // UNSAFE_componentWillReceiveProps(): void { + // setTimeout(() => this.refresh(), 0) + // } render(): React.ReactElement { - return ( - -
- {(this.editor === undefined || - this.editor.currentRequest) && } - {this.editor !== undefined && this.renderReady(this.editor)} -
- - ) - } - - renderReady(editor: ChartEditor): React.ReactElement { - const { grapher, availableTabs } = editor - - return ( - - {!editor.newChartId && ( - - )} - {editor.newChartId && ( - - )} - -
- -
- {editor.tab === "basic" && ( - - )} - {editor.tab === "text" && ( - - )} - {editor.tab === "data" && ( - - )} - {editor.tab === "customize" && ( - - )} - {editor.tab === "scatter" && ( - - )} - {editor.tab === "marimekko" && ( - - )} - {editor.tab === "map" && ( - - )} - {editor.tab === "revisions" && ( - - )} - {editor.tab === "refs" && ( - - )} - {editor.tab === "export" && ( - - )} -
- {editor.tab !== "export" && } -
-
-
- {this.grapherElement} -
-
-
- - -
-
- Emulate vision deficiency:{" "} - - (this.simulateVisionDeficiency = - option.deficiency) - )} - /> -
-
- - {/* Include svg filters necessary for vision deficiency emulation */} - -
- - ) + return } } diff --git a/adminSiteClient/ChartEditorView.tsx b/adminSiteClient/ChartEditorView.tsx new file mode 100644 index 00000000000..f3d81531be4 --- /dev/null +++ b/adminSiteClient/ChartEditorView.tsx @@ -0,0 +1,553 @@ +import React from "react" +import { observer } from "mobx-react" +import { + observable, + computed, + runInAction, + action, + reaction, + IReactionDisposer, +} from "mobx" +import { Prompt, Redirect } from "react-router-dom" +import { + Bounds, + capitalize, + DetailDictionary, + get, + set, + groupBy, + extractDetailsFromSyntax, + getIndexableKeys, +} from "@ourworldindata/utils" +import { + GrapherInterface, + GrapherStaticFormat, + DimensionProperty, +} from "@ourworldindata/types" +import { Grapher } from "@ourworldindata/grapher" +import { Admin } from "./Admin.js" +import { getFullReferencesCount, isChartEditorInstance } from "./ChartEditor.js" +import { EditorBasicTab } from "./EditorBasicTab.js" +import { EditorDataTab } from "./EditorDataTab.js" +import { EditorTextTab } from "./EditorTextTab.js" +import { EditorCustomizeTab } from "./EditorCustomizeTab.js" +import { EditorScatterTab } from "./EditorScatterTab.js" +import { EditorMapTab } from "./EditorMapTab.js" +import { EditorHistoryTab } from "./EditorHistoryTab.js" +import { EditorReferencesTab } from "./EditorReferencesTab.js" +import { + SaveButtonsForChart, + SaveButtonsForIndicatorChart, +} from "./SaveButtons.js" +import { LoadingBlocker } from "./Forms.js" +import { AdminLayout } from "./AdminLayout.js" +import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" +import { faMobile, faDesktop } from "@fortawesome/free-solid-svg-icons" +import { + VisionDeficiency, + VisionDeficiencySvgFilters, + VisionDeficiencyDropdown, + VisionDeficiencyEntity, +} from "./VisionDeficiencies.js" +import { EditorMarimekkoTab } from "./EditorMarimekkoTab.js" +import { EditorExportTab } from "./EditorExportTab.js" +import { runDetailsOnDemand } from "../site/detailsOnDemand.js" +import { ChartEditorContext } from "./ChartEditorContext.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" +import { isIndicatorChartEditorInstance } from "./IndicatorChartEditor.js" + +interface Variable { + id: number + name: string +} + +export interface Dataset { + id: number + name: string + namespace: string + version: string | undefined + variables: Variable[] + isPrivate: boolean + nonRedistributable: boolean +} + +export interface Namespace { + name: string + description?: string + isArchived: boolean +} + +// This contains the dataset/variable metadata for the entire database +// Used for variable selector interface +export interface NamespaceData { + datasets: Dataset[] +} + +export class EditorDatabase { + @observable.ref namespaces: Namespace[] + @observable.ref variableUsageCounts: Map = new Map() + @observable dataByNamespace: Map = new Map() + + constructor(json: any) { + this.namespaces = json.namespaces + } +} + +export interface DimensionErrorMessage { + displayName?: string +} + +export type FieldWithDetailReferences = + | "subtitle" + | "note" + | "axisLabelX" + | "axisLabelY" + +export type DetailReferences = Record + +export interface ChartEditorViewManager { + admin: Admin + editor: Editor +} + +@observer +export class ChartEditorView< + Editor extends AbstractChartEditor, +> extends React.Component<{ + manager: ChartEditorViewManager +}> { + @observable.ref database = new EditorDatabase({}) + @observable details: DetailDictionary = {} + @observable.ref grapherElement?: React.ReactElement + + @observable simulateVisionDeficiency?: VisionDeficiency + + @computed private get manager(): ChartEditorViewManager { + return this.props.manager + } + + @observable private _isDbSet = false + @observable private _isGrapherSet = false + @computed get isReady(): boolean { + return this._isDbSet && this._isGrapherSet + } + + @action.bound private updateGrapher(): void { + const config = this.manager.editor.originalGrapherConfig + const grapherConfig = { + ...config, + // binds the grapher instance to this.grapher + getGrapherInstance: (grapher: Grapher) => { + this.manager.editor.grapher = grapher + }, + dataApiUrlForAdmin: + this.manager.admin.settings.DATA_API_FOR_ADMIN_UI, // passed this way because clientSettings are baked and need a recompile to be updated + bounds: this.bounds, + staticFormat: this.staticFormat, + } + this.manager.editor.grapher.renderToStatic = + !!this.editor?.showStaticPreview + this.grapherElement = + this._isGrapherSet = true + } + + @action.bound private setDb(json: any): void { + this.database = new EditorDatabase(json) + this._isDbSet = true + } + + async fetchData(): Promise { + const { admin } = this.manager + + const [namespaces, variables] = await Promise.all([ + admin.getJSON(`/api/editorData/namespaces.json`), + admin.getJSON(`/api/editorData/variables.json`), + ]) + + this.setDb(namespaces) + + const groupedByNamespace = groupBy( + variables.datasets, + (d) => d.namespace + ) + for (const namespace in groupedByNamespace) { + this.database.dataByNamespace.set(namespace, { + datasets: groupedByNamespace[namespace] as Dataset[], + }) + } + + const usageData = (await admin.getJSON( + `/api/variables.usages.json` + )) as { + variableId: number + usageCount: number + }[] + this.database.variableUsageCounts = new Map( + usageData.map(({ variableId, usageCount }) => [ + variableId, + +usageCount, + ]) + ) + } + + async fetchDetails(): Promise { + await runDetailsOnDemand() + + runInAction(() => { + if (window.details) this.details = window.details + }) + } + + @computed private get isMobilePreview(): boolean { + return this.editor?.previewMode === "mobile" + } + + @computed private get bounds(): Bounds { + return this.isMobilePreview + ? new Bounds(0, 0, 380, 525) + : this.manager.editor.grapher.defaultBounds + } + + @computed private get staticFormat(): GrapherStaticFormat { + return this.isMobilePreview + ? GrapherStaticFormat.square + : GrapherStaticFormat.landscape + } + + // unvalidated terms extracted from the subtitle and note fields + // these may point to non-existent details e.g. ["not_a_real_term", "pvotery"] + @computed + get currentDetailReferences(): DetailReferences { + const { grapher } = this.manager.editor + return { + subtitle: extractDetailsFromSyntax(grapher.currentSubtitle), + note: extractDetailsFromSyntax(grapher.note), + axisLabelX: extractDetailsFromSyntax( + grapher.xAxisConfig.label ?? "" + ), + axisLabelY: extractDetailsFromSyntax( + grapher.yAxisConfig.label ?? "" + ), + } + } + + // the actual Detail objects, indexed by category.term + @computed get currentlyReferencedDetails(): GrapherInterface["details"] { + const grapherConfigDetails: GrapherInterface["details"] = {} + const allReferences = Object.values(this.currentDetailReferences).flat() + + allReferences.forEach((term) => { + const detail = get(this.details, term) + if (detail) { + set(grapherConfigDetails, term, detail) + } + }) + + return grapherConfigDetails + } + + @computed + get invalidDetailReferences(): DetailReferences { + const { subtitle, note, axisLabelX, axisLabelY } = + this.currentDetailReferences + return { + subtitle: subtitle.filter((term) => !this.details[term]), + note: note.filter((term) => !this.details[term]), + axisLabelX: axisLabelX.filter((term) => !this.details[term]), + axisLabelY: axisLabelY.filter((term) => !this.details[term]), + } + } + + @computed get errorMessages(): Partial< + Record + > { + const { invalidDetailReferences } = this + + const errorMessages: Partial< + Record + > = {} + + // add error messages for each field with invalid detail references + getIndexableKeys(invalidDetailReferences).forEach( + (key: FieldWithDetailReferences) => { + const references = invalidDetailReferences[key] + if (references.length) { + errorMessages[key] = + `Invalid detail(s) specified: ${references.join(", ")}` + } + } + ) + + return errorMessages + } + + @computed + get errorMessagesForDimensions(): Record< + DimensionProperty, + DimensionErrorMessage[] + > { + const errorMessages: Record< + DimensionProperty, + DimensionErrorMessage[] + > = { + [DimensionProperty.y]: [], + [DimensionProperty.x]: [], + [DimensionProperty.color]: [], + [DimensionProperty.size]: [], + [DimensionProperty.table]: [], // not used + } + + this.manager.editor.grapher.dimensionSlots.forEach((slot) => { + slot.dimensions.forEach((dimension, dimensionIndex) => { + const details = extractDetailsFromSyntax( + dimension.display.name ?? "" + ) + const hasDetailsInDisplayName = details.length > 0 + + // add error message if details are referenced in the display name + if (hasDetailsInDisplayName) { + errorMessages[slot.property][dimensionIndex] = { + displayName: "Detail syntax is not supported", + } + } + }) + }) + + return errorMessages + } + + @computed get editor(): Editor | undefined { + if (!this.isReady) return undefined + + return this.manager.editor + } + + @action.bound refresh(): void { + void this.fetchDetails() + void this.fetchData() + } + + componentDidMount(): void { + this.refresh() + this.updateGrapher() + + this.disposers.push( + reaction( + () => this.editor && this.editor.previewMode, + () => { + if (this.editor) { + localStorage.setItem( + "editorPreviewMode", + this.editor.previewMode + ) + } + this.updateGrapher() + } + ) + ) + } + + // TODO(inheritance) + // This funny construction allows the "new chart" link to work by forcing an update + // even if the props don't change + // UNSAFE_componentWillReceiveProps(): void { + // setTimeout(() => this.refresh(), 0) + // } + + disposers: IReactionDisposer[] = [] + componentWillUnmount(): void { + this.disposers.forEach((dispose) => dispose()) + } + + render(): React.ReactElement { + return ( + +
+ {(this.editor === undefined || + this.editor.currentRequest) && } + {this.editor !== undefined && this.renderReady(this.editor)} +
+ + ) + } + + renderReady(editor: Editor): React.ReactElement { + const { grapher, availableTabs } = editor + + const chartEditor = isChartEditorInstance(editor) ? editor : undefined + const indicatorChartEditor = isIndicatorChartEditorInstance(editor) + ? editor + : undefined + + const saveButtons = chartEditor ? ( + + ) : indicatorChartEditor ? ( + + ) : null + + return ( + + {!editor.isNewGrapher && ( + + )} + {chartEditor?.newChartId && ( + + )} +
+ +
+ {editor.tab === "basic" && ( + + )} + {editor.tab === "text" && ( + + )} + {editor.tab === "data" && ( + + )} + {editor.tab === "customize" && ( + + )} + {editor.tab === "scatter" && ( + + )} + {editor.tab === "marimekko" && ( + + )} + {editor.tab === "map" && ( + + )} + {chartEditor && chartEditor.tab === "revisions" && ( + + )} + {chartEditor && chartEditor.tab === "refs" && ( + + )} + {editor.tab === "export" && ( + + )} +
+ {editor.tab !== "export" && saveButtons} +
+
+
+ {this.grapherElement} +
+
+
+ + +
+
+ Emulate vision deficiency:{" "} + + (this.simulateVisionDeficiency = + option.deficiency) + )} + /> +
+
+ + {/* Include svg filters necessary for vision deficiency emulation */} + +
+ + ) + } +} diff --git a/adminSiteClient/DimensionCard.tsx b/adminSiteClient/DimensionCard.tsx index cbb6ab2d9a0..d45e7193d73 100644 --- a/adminSiteClient/DimensionCard.tsx +++ b/adminSiteClient/DimensionCard.tsx @@ -4,7 +4,7 @@ import { observer } from "mobx-react" import { ChartDimension } from "@ourworldindata/grapher" import { OwidVariableRoundingMode } from "@ourworldindata/types" import { startCase } from "@ourworldindata/utils" -import { ChartEditor, DimensionErrorMessage } from "./ChartEditor.js" +import { DimensionErrorMessage } from "./ChartEditorView.js" import { Toggle, BindAutoString, @@ -22,11 +22,14 @@ import { } from "@fortawesome/free-solid-svg-icons" import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" import { OwidTable } from "@ourworldindata/core-table" +import { AbstractChartEditor } from "./AbstractChartEditor.js" @observer -export class DimensionCard extends React.Component<{ +export class DimensionCard< + Editor extends AbstractChartEditor, +> extends React.Component<{ dimension: ChartDimension - editor: ChartEditor + editor: Editor isDndEnabled?: boolean onChange: (dimension: ChartDimension) => void onEdit?: () => void diff --git a/adminSiteClient/EditorBasicTab.tsx b/adminSiteClient/EditorBasicTab.tsx index c2e4994e689..639609a9bab 100644 --- a/adminSiteClient/EditorBasicTab.tsx +++ b/adminSiteClient/EditorBasicTab.tsx @@ -30,7 +30,6 @@ import { } from "@ourworldindata/utils" import { FieldsRow, Section, SelectField, Toggle } from "./Forms.js" import { VariableSelector } from "./VariableSelector.js" -import { ChartEditor, ChartEditorManager } from "./ChartEditor.js" import { DimensionCard } from "./DimensionCard.js" import { DragDropContext, @@ -38,12 +37,21 @@ import { Draggable, DropResult, } from "react-beautiful-dnd" +import { ChartEditorContext } from "./ChartEditorContext.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" +import { EditorDatabase } from "./ChartEditorView.js" +import { isChartEditorInstance } from "./ChartEditor.js" @observer -class DimensionSlotView extends React.Component<{ +class DimensionSlotView< + Editor extends AbstractChartEditor, +> extends React.Component<{ slot: DimensionSlot - editor: ChartEditor + editor: Editor + database: EditorDatabase }> { + static contextType = ChartEditorContext + disposers: IReactionDisposer[] = [] @observable.ref isSelectingVariables: boolean = false @@ -53,8 +61,8 @@ class DimensionSlotView extends React.Component<{ } @computed - get errorMessages(): ChartEditorManager["errorMessagesForDimensions"] { - return this.props.editor.manager.errorMessagesForDimensions + get errorMessages() { + return this.context.errorMessagesForDimensions } @action.bound private onAddVariables(variableIds: OwidVariableId[]) { @@ -75,6 +83,7 @@ class DimensionSlotView extends React.Component<{ this.isSelectingVariables = false this.updateDimensionsAndRebuildTable(dimensionConfigs) + this.updateParentConfig() } @action.bound private onRemoveDimension(variableId: OwidVariableId) { @@ -83,10 +92,12 @@ class DimensionSlotView extends React.Component<{ (d) => d.variableId !== variableId ) ) + this.updateParentConfig() } @action.bound private onChangeDimension() { this.updateDimensionsAndRebuildTable() + this.updateParentConfig() } @action.bound private updateDefaults() { @@ -165,6 +176,12 @@ class DimensionSlotView extends React.Component<{ this.grapher.rebuildInputOwidTable() } + @action.bound private updateParentConfig() { + if (isChartEditorInstance(this.props.editor)) { + void this.props.editor.updateParentConfig() + } + } + @action.bound private onDragEnd(result: DropResult) { const { source, destination } = result if (!destination) return @@ -176,6 +193,7 @@ class DimensionSlotView extends React.Component<{ ) this.updateDimensionsAndRebuildTable(dimensions) + this.updateParentConfig() } @computed get isDndEnabled() { @@ -267,6 +285,7 @@ class DimensionSlotView extends React.Component<{ {isSelectingVariables && ( (this.isSelectingVariables = false) @@ -280,7 +299,9 @@ class DimensionSlotView extends React.Component<{ } @observer -class VariablesSection extends React.Component<{ editor: ChartEditor }> { +class VariablesSection< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor; database: EditorDatabase }> { base: React.RefObject = React.createRef() @observable.ref isAddingVariable: boolean = false @@ -296,6 +317,7 @@ class VariablesSection extends React.Component<{ editor: ChartEditor }> { key={slot.name} slot={slot} editor={props.editor} + database={props.database} /> ))} @@ -305,7 +327,9 @@ class VariablesSection extends React.Component<{ editor: ChartEditor }> { } @observer -export class EditorBasicTab extends React.Component<{ editor: ChartEditor }> { +export class EditorBasicTab< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor; database: EditorDatabase }> { @action.bound onChartTypeChange(value: string) { const { grapher } = this.props.editor grapher.type = value as ChartTypeName @@ -371,7 +395,10 @@ export class EditorBasicTab extends React.Component<{ editor: ChartEditor }> { /> - + ) } diff --git a/adminSiteClient/EditorCustomizeTab.tsx b/adminSiteClient/EditorCustomizeTab.tsx index 7cc973c0820..20f56b51571 100644 --- a/adminSiteClient/EditorCustomizeTab.tsx +++ b/adminSiteClient/EditorCustomizeTab.tsx @@ -1,7 +1,6 @@ import React from "react" import { observable, computed, action } from "mobx" import { observer } from "mobx-react" -import { ChartEditor } from "./ChartEditor.js" import { ComparisonLineConfig, ColorSchemeName, @@ -37,6 +36,8 @@ import { } from "./ColorSchemeDropdown.js" import { EditorColorScaleSection } from "./EditorColorScaleSection.js" import Select from "react-select" +import { ChartEditorContext } from "./ChartEditorContext.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" @observer export class ColorSchemeSelector extends React.Component<{ grapher: Grapher }> { @@ -103,7 +104,9 @@ interface SortOrderDropdownOption { } @observer -class SortOrderSection extends React.Component<{ editor: ChartEditor }> { +class SortOrderSection< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { @computed get sortConfig(): SortConfig { return this.grapher._sortConfig } @@ -203,7 +206,9 @@ class SortOrderSection extends React.Component<{ editor: ChartEditor }> { } @observer -class FacetSection extends React.Component<{ editor: ChartEditor }> { +class FacetSection extends React.Component<{ + editor: Editor +}> { base: React.RefObject = React.createRef() @computed get grapher() { @@ -280,7 +285,9 @@ class FacetSection extends React.Component<{ editor: ChartEditor }> { } @observer -class TimelineSection extends React.Component<{ editor: ChartEditor }> { +class TimelineSection< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { base: React.RefObject = React.createRef() @computed get grapher() { @@ -387,7 +394,9 @@ class TimelineSection extends React.Component<{ editor: ChartEditor }> { } @observer -class ComparisonLineSection extends React.Component<{ editor: ChartEditor }> { +class ComparisonLineSection< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { @observable comparisonLines: ComparisonLineConfig[] = [] @action.bound onAddComparisonLine() { @@ -445,11 +454,16 @@ class ComparisonLineSection extends React.Component<{ editor: ChartEditor }> { } @observer -export class EditorCustomizeTab extends React.Component<{ - editor: ChartEditor +export class EditorCustomizeTab< + Editor extends AbstractChartEditor, +> extends React.Component<{ + editor: Editor }> { + // TODO + static contextType = ChartEditorContext + @computed get errorMessages() { - return this.props.editor.manager.errorMessages + return this.context.errorMessages } render() { diff --git a/adminSiteClient/EditorDataTab.tsx b/adminSiteClient/EditorDataTab.tsx index 14a81edf41b..af28179a3b3 100644 --- a/adminSiteClient/EditorDataTab.tsx +++ b/adminSiteClient/EditorDataTab.tsx @@ -9,7 +9,6 @@ import { } from "@ourworldindata/types" import { Grapher } from "@ourworldindata/grapher" import { ColorBox, SelectField, Section } from "./Forms.js" -import { ChartEditor } from "./ChartEditor.js" import { faArrowsAltV, faTimes } from "@fortawesome/free-solid-svg-icons" import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" import { @@ -18,6 +17,7 @@ import { Droppable, DropResult, } from "react-beautiful-dnd" +import { AbstractChartEditor } from "./AbstractChartEditor.js" interface EntityItemProps extends React.HTMLProps { grapher: Grapher @@ -164,7 +164,9 @@ export class KeysSection extends React.Component<{ grapher: Grapher }> { } @observer -class MissingDataSection extends React.Component<{ editor: ChartEditor }> { +class MissingDataSection< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { @computed get grapher() { return this.props.editor.grapher } @@ -208,7 +210,9 @@ class MissingDataSection extends React.Component<{ editor: ChartEditor }> { } @observer -export class EditorDataTab extends React.Component<{ editor: ChartEditor }> { +export class EditorDataTab< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { render() { const { editor } = this.props const { grapher, features } = editor diff --git a/adminSiteClient/EditorExportTab.tsx b/adminSiteClient/EditorExportTab.tsx index f21574c51b3..5fe70b2181c 100644 --- a/adminSiteClient/EditorExportTab.tsx +++ b/adminSiteClient/EditorExportTab.tsx @@ -1,13 +1,13 @@ import { IReactionDisposer, action, autorun, computed, observable } from "mobx" import { observer } from "mobx-react" import React from "react" -import { ChartEditor } from "./ChartEditor.js" import { Section, Toggle } from "./Forms.js" import { Grapher } from "@ourworldindata/grapher" import { triggerDownloadFromBlob, GrapherStaticFormat, } from "@ourworldindata/utils" +import { AbstractChartEditor } from "./AbstractChartEditor.js" type ExportSettings = Required< Pick< @@ -55,18 +55,20 @@ const DEFAULT_SETTINGS: ExportSettings = { shouldIncludeDetailsInStaticExport: false, } -interface EditorExportTabProps { - editor: ChartEditor +interface EditorExportTabProps { + editor: Editor } @observer -export class EditorExportTab extends React.Component { +export class EditorExportTab< + Editor extends AbstractChartEditor, +> extends React.Component> { @observable private settings = DEFAULT_SETTINGS private originalSettings: Partial = DEFAULT_SETTINGS private originalGrapher: OriginalGrapher private disposers: IReactionDisposer[] = [] - constructor(props: EditorExportTabProps) { + constructor(props: EditorExportTabProps) { super(props) this.originalGrapher = this.grabRelevantPropertiesFromGrapher() } diff --git a/adminSiteClient/EditorFeatures.tsx b/adminSiteClient/EditorFeatures.tsx index 256b8c5bb37..2d9761add1c 100644 --- a/adminSiteClient/EditorFeatures.tsx +++ b/adminSiteClient/EditorFeatures.tsx @@ -1,11 +1,11 @@ import { computed } from "mobx" -import { ChartEditor } from "./ChartEditor.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" // Responsible for determining what parts of the editor should be shown, based on the // type of chart being edited export class EditorFeatures { - editor: ChartEditor - constructor(editor: ChartEditor) { + editor: AbstractChartEditor + constructor(editor: AbstractChartEditor) { this.editor = editor } diff --git a/adminSiteClient/EditorMapTab.tsx b/adminSiteClient/EditorMapTab.tsx index 25e4594fee8..8a167264423 100644 --- a/adminSiteClient/EditorMapTab.tsx +++ b/adminSiteClient/EditorMapTab.tsx @@ -9,9 +9,9 @@ import { ColumnSlug, isEmpty, ToleranceStrategy } from "@ourworldindata/utils" import { action, computed } from "mobx" import { observer } from "mobx-react" import React from "react" -import { ChartEditor } from "./ChartEditor.js" import { EditorColorScaleSection } from "./EditorColorScaleSection.js" import { NumberField, Section, SelectField, Toggle } from "./Forms.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" @observer class VariableSection extends React.Component<{ @@ -161,7 +161,9 @@ class TooltipSection extends React.Component<{ mapConfig: MapConfig }> { } @observer -export class EditorMapTab extends React.Component<{ editor: ChartEditor }> { +export class EditorMapTab< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { @computed get grapher() { return this.props.editor.grapher } diff --git a/adminSiteClient/EditorReferencesTab.tsx b/adminSiteClient/EditorReferencesTab.tsx index 6051c991fcb..ef02ee79413 100644 --- a/adminSiteClient/EditorReferencesTab.tsx +++ b/adminSiteClient/EditorReferencesTab.tsx @@ -13,6 +13,7 @@ import { formatValue, ChartRedirect, } from "@ourworldindata/utils" +import { AbstractChartEditor } from "./AbstractChartEditor.js" const BASE_URL = BAKED_GRAPHER_URL.replace(/^https?:\/\//, "") @@ -214,8 +215,10 @@ export class EditorReferencesTab extends React.Component<{ } @observer -class AddRedirectForm extends React.Component<{ - editor: ChartEditor +class AddRedirectForm< + Editor extends AbstractChartEditor, +> extends React.Component<{ + editor: Editor onSuccess: (redirect: ChartRedirect) => void }> { static contextType = AdminAppContext diff --git a/adminSiteClient/EditorTextTab.tsx b/adminSiteClient/EditorTextTab.tsx index 0a5ba8b0220..43b657d55ad 100644 --- a/adminSiteClient/EditorTextTab.tsx +++ b/adminSiteClient/EditorTextTab.tsx @@ -15,7 +15,7 @@ import { observer } from "mobx-react" import React from "react" import Select from "react-select" import { TOPICS_CONTENT_GRAPH } from "../settings/clientSettings.js" -import { ChartEditor, ChartEditorManager } from "./ChartEditor.js" +import { isChartEditorInstance } from "./ChartEditor.js" import { AutoTextField, BindAutoString, @@ -27,9 +27,15 @@ import { TextField, Toggle, } from "./Forms.js" +import { ChartEditorContext } from "./ChartEditorContext.js" +import { AbstractChartEditor } from "./AbstractChartEditor.js" @observer -export class EditorTextTab extends React.Component<{ editor: ChartEditor }> { +export class EditorTextTab< + Editor extends AbstractChartEditor, +> extends React.Component<{ editor: Editor }> { + static contextType = ChartEditorContext + @action.bound onSlug(slug: string) { this.props.editor.grapher.slug = slugify(slug) } @@ -74,8 +80,8 @@ export class EditorTextTab extends React.Component<{ editor: ChartEditor }> { grapher.hideAnnotationFieldsInTitle.changeInPrefix = value || undefined } - @computed get errorMessages(): ChartEditorManager["errorMessages"] { - return this.props.editor.manager.errorMessages + @computed get errorMessages() { + return this.context.errorMessages } @computed get showAnyAnnotationFieldInTitleToggle() { @@ -88,7 +94,8 @@ export class EditorTextTab extends React.Component<{ editor: ChartEditor }> { } render() { - const { grapher, references, features } = this.props.editor + const { editor } = this.props + const { grapher, features } = editor const { relatedQuestions } = grapher return ( @@ -186,15 +193,16 @@ export class EditorTextTab extends React.Component<{ editor: ChartEditor }> { placeholder={grapher.originUrlWithProtocol} helpText="The page containing this chart where more context can be found" /> - {references && - (references.postsWordpress.length > 0 || - references.postsGdocs.length > 0) && ( + {isChartEditorInstance(editor) && + editor.references && + (editor.references.postsWordpress.length > 0 || + editor.references.postsGdocs.length > 0) && (

Origin url suggestions

    {[ - ...references.postsWordpress, - ...references.postsGdocs, + ...editor.references.postsWordpress, + ...editor.references.postsGdocs, ].map((post) => (
  • {post.url}
    • ))} @@ -213,10 +221,12 @@ export class EditorTextTab extends React.Component<{ editor: ChartEditor }> { /> - + {isChartEditorInstance(editor) && ( + + )}
    {relatedQuestions.map( diff --git a/adminSiteClient/GrapherConfigGridEditor.tsx b/adminSiteClient/GrapherConfigGridEditor.tsx index ae6eb349504..fe18cfca9ba 100644 --- a/adminSiteClient/GrapherConfigGridEditor.tsx +++ b/adminSiteClient/GrapherConfigGridEditor.tsx @@ -7,6 +7,7 @@ import { getWindowUrl, setWindowUrl, excludeNull, + mergeGrapherConfigs, } from "@ourworldindata/utils" import { GrapherConfigPatch } from "../adminShared/AdminSessionTypes.js" import { @@ -28,14 +29,12 @@ import { import { Disposer, observer } from "mobx-react" import { observable, computed, action, runInAction, autorun } from "mobx" import { match, P } from "ts-pattern" -//import * as lodash from "lodash" import { cloneDeep, isArray, isEmpty, isEqual, isNil, - merge, omitBy, pick, range, @@ -347,8 +346,10 @@ export class GrapherConfigGridEditor extends React.Component { + @computed get availableTabs(): EditorTab[] { + const tabs: EditorTab[] = ["basic", "data", "text", "customize"] + if (this.grapher.hasMapTab) tabs.push("map") + if (this.grapher.isScatter) tabs.push("scatter") + if (this.grapher.isMarimekko) tabs.push("marimekko") + tabs.push("export") + return tabs + } + + @computed get isNewGrapher() { + return this.manager.isNewGrapher + } + + async saveGrapher({ + onError, + }: { onError?: () => void } = {}): Promise { + // TODO(inheritance) + console.log("save indicator chart") + onError?.() + } +} + +export function isIndicatorChartEditorInstance( + editor: AbstractChartEditor +): editor is IndicatorChartEditor { + return editor instanceof IndicatorChartEditor +} diff --git a/adminSiteClient/IndicatorChartEditorPage.tsx b/adminSiteClient/IndicatorChartEditorPage.tsx new file mode 100644 index 00000000000..119fab2d100 --- /dev/null +++ b/adminSiteClient/IndicatorChartEditorPage.tsx @@ -0,0 +1,72 @@ +import React from "react" +import { observer } from "mobx-react" +import { observable, computed, action } from "mobx" +import { isEmpty } from "@ourworldindata/utils" +import { GrapherInterface, DimensionProperty } from "@ourworldindata/types" +import { Grapher } from "@ourworldindata/grapher" +import { Admin } from "./Admin.js" +import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js" +import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js" +import { + IndicatorChartEditor, + IndicatorChartEditorManager, +} from "./IndicatorChartEditor.js" + +@observer +export class IndicatorChartEditorPage + extends React.Component<{ + variableId: number + }> + implements + IndicatorChartEditorManager, + ChartEditorViewManager +{ + @observable.ref grapher = new Grapher() + + static contextType = AdminAppContext + context!: AdminAppContextType + + patchConfig: GrapherInterface = {} + + isNewGrapher = false + + async fetchGrapher(): Promise { + const { variableId } = this.props + const config = await this.context.admin.getJSON( + `/api/variables/grapherConfigAdmin/${variableId}.patchConfig.json` + ) + if (isEmpty(config)) { + this.isNewGrapher = true + this.patchConfig = { + dimensions: [{ variableId, property: DimensionProperty.y }], + } + } + } + + @computed get admin(): Admin { + return this.context.admin + } + + @computed get editor(): IndicatorChartEditor { + return new IndicatorChartEditor({ manager: this }) + } + + @action.bound refresh(): void { + void this.fetchGrapher() + } + + componentDidMount(): void { + this.refresh() + } + + // TODO(inheritance) + // This funny construction allows the "new chart" link to work by forcing an update + // even if the props don't change + // UNSAFE_componentWillReceiveProps(): void { + // setTimeout(() => this.refresh(), 0) + // } + + render(): React.ReactElement { + return + } +} diff --git a/adminSiteClient/SaveButtons.tsx b/adminSiteClient/SaveButtons.tsx index 1ed6ecc5c0c..3bedd822669 100644 --- a/adminSiteClient/SaveButtons.tsx +++ b/adminSiteClient/SaveButtons.tsx @@ -3,13 +3,22 @@ import { ChartEditor } from "./ChartEditor.js" import { action, computed } from "mobx" import { observer } from "mobx-react" import { isEmpty } from "@ourworldindata/utils" +import { ChartEditorContext } from "./ChartEditorContext.js" +import { IndicatorChartEditor } from "./IndicatorChartEditor.js" @observer -export class SaveButtons extends React.Component<{ editor: ChartEditor }> { +export class SaveButtonsForChart extends React.Component<{ + editor: ChartEditor +}> { + // TODO + static contextType = ChartEditorContext + @action.bound onSaveChart() { void this.props.editor.saveGrapher() } + // TODO + @action.bound onSaveAsNew() { void this.props.editor.saveAsNewGrapher() } @@ -21,8 +30,7 @@ export class SaveButtons extends React.Component<{ editor: ChartEditor }> { } @computed get hasEditingErrors(): boolean { - const { editor } = this.props - const { errorMessages, errorMessagesForDimensions } = editor.manager + const { errorMessages, errorMessagesForDimensions } = this.context if (!isEmpty(errorMessages)) return true @@ -58,7 +66,7 @@ export class SaveButtons extends React.Component<{ editor: ChartEditor }> { disabled={isSavingDisabled} > Save as new - {" "} +
) + } +} + +@observer +export class SaveButtonsForIndicatorChart extends React.Component<{ + editor: IndicatorChartEditor +}> { + static contextType = ChartEditorContext + + @action.bound onSaveChart() { + void this.props.editor.saveGrapher() + } + + @computed get hasEditingErrors(): boolean { + const { errorMessages, errorMessagesForDimensions } = this.context + + if (!isEmpty(errorMessages)) return true + + const allErrorMessagesForDimensions = Object.values( + errorMessagesForDimensions + ).flat() + return allErrorMessagesForDimensions.some((error) => error) + } + + render() { + const { hasEditingErrors } = this + const { editor } = this.props + const { grapher } = editor + + const isSavingDisabled = grapher.hasFatalErrors || hasEditingErrors - /*return
- - {" "} - {" "} -
*/ + return ( +
+ +
+ ) } } diff --git a/adminSiteClient/VariableSelector.tsx b/adminSiteClient/VariableSelector.tsx index b146dd28e15..5728944d6d9 100644 --- a/adminSiteClient/VariableSelector.tsx +++ b/adminSiteClient/VariableSelector.tsx @@ -22,17 +22,18 @@ import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" import { faArchive } from "@fortawesome/free-solid-svg-icons" import { - ChartEditor, Dataset, EditorDatabase, Namespace, NamespaceData, -} from "./ChartEditor.js" +} from "./ChartEditorView.js" import { TextField, Toggle, Modal } from "./Forms.js" import { DimensionSlot } from "@ourworldindata/grapher" +import { AbstractChartEditor } from "./AbstractChartEditor.js" -interface VariableSelectorProps { - editor: ChartEditor +interface VariableSelectorProps { + database: EditorDatabase + editor: Editor slot: DimensionSlot onDismiss: () => void onComplete: (variableIds: OwidVariableId[]) => void @@ -49,7 +50,9 @@ interface Variable { } @observer -export class VariableSelector extends React.Component { +export class VariableSelector< + Editor extends AbstractChartEditor, +> extends React.Component> { @observable.ref chosenNamespaces: Namespace[] = [] @observable.ref searchInput?: string @observable.ref isProjection?: boolean @@ -63,7 +66,7 @@ export class VariableSelector extends React.Component { @observable rowHeight: number = 32 @computed get database(): EditorDatabase { - return this.props.editor.database + return this.props.database } @computed get searchWords(): SearchWord[] { @@ -200,7 +203,7 @@ export class VariableSelector extends React.Component { render() { const { slot } = this.props - const { database } = this.props.editor + const { database } = this.props const { searchInput, chosenVariables, @@ -498,7 +501,7 @@ export class VariableSelector extends React.Component { dispose!: IReactionDisposer base: React.RefObject = React.createRef() componentDidMount() { - void this.props.editor.loadVariableUsageCounts() + // void this.props.editor.loadVariableUsageCounts() // TODO??? this.initChosenVariablesAndNamespaces() } diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index e948fe60785..6bcbc29ea1c 100644 --- a/adminSiteServer/apiRouter.ts +++ b/adminSiteServer/apiRouter.ts @@ -13,12 +13,15 @@ import { expectInt, isValidSlug } from "../serverUtils/serverUtil.js" import { OldChartFieldList, assignTagsForCharts, + getParentConfigForChart, getChartConfigById, getChartSlugById, getGptTopicSuggestions, getRedirectsByChartId, oldChartFieldList, setChartTags, + getParentConfigForChartFromConfig, + getPatchConfigByChartId, } from "../db/model/Chart.js" import { Request } from "./authentication.js" import { @@ -26,7 +29,13 @@ import { fetchS3MetadataByPath, fetchS3DataValuesByPath, searchVariables, + getGrapherConfigsForVariable, + updateGrapherConfigAdminOfVariable, + updateGrapherConfigETLOfVariable, + updateAllChartsThatInheritFromIndicator, + getParentConfigForIndicatorChartAdmin, } from "../db/model/Variable.js" +import { updateExistingFullConfig } from "../db/model/ChartConfigs.js" import { getCanonicalUrl } from "@ourworldindata/components" import { camelCaseProperties, @@ -37,8 +46,6 @@ import { parseIntOrUndefined, DbRawPostWithGdocPublishStatus, OwidVariableWithSource, - OwidChartDimensionInterface, - DimensionProperty, TaggableType, DbChartTagJoin, pick, @@ -287,7 +294,7 @@ const saveNewChart = async ( } // compute patch and full configs - const parentConfig = defaultGrapherConfig + const parentConfig = await getParentConfigForChartFromConfig(knex, config) const patchConfig = diffGrapherConfigs(config, parentConfig) const fullConfig = mergeGrapherConfigs(parentConfig, patchConfig) @@ -354,7 +361,7 @@ const updateExistingChart = async ( } // compute patch and full configs - const parentConfig = defaultGrapherConfig + const parentConfig = await getParentConfigForChart(knex, chartId) const patchConfig = diffGrapherConfigs(config, parentConfig) const fullConfig = mergeGrapherConfigs(parentConfig, patchConfig) @@ -638,6 +645,28 @@ getRouteWithROTransaction( async (req, res, trx) => expectChartById(trx, req.params.chartId) ) +getRouteWithROTransaction( + apiRouter, + "/charts/:chartId.parentConfig.json", + async (req, res, trx) => { + const chartId = expectInt(req.params.chartId) + return getParentConfigForChart(trx, chartId) + } +) + +getRouteWithROTransaction( + apiRouter, + "/charts/:chartId.patchConfig.json", + async (req, res, trx) => { + const chartId = expectInt(req.params.chartId) + const config = await getPatchConfigByChartId(trx, chartId) + if (!config) { + throw new JsonError(`Chart with id ${chartId} not found`, 500) + } + return config + } +) + getRouteWithROTransaction( apiRouter, "/editorData/namespaces.json", @@ -1146,21 +1175,25 @@ getRouteWithROTransaction( const whereClause = filterSExpr?.toSql() ?? "true" const resultsWithStringGrapherConfigs = await db.knexRaw( trx, - `SELECT variables.id as id, - variables.name as name, - variables.grapherConfigAdmin as config, - d.name as datasetname, - namespaces.name as namespacename, - variables.createdAt as createdAt, - variables.updatedAt as updatedAt, - variables.description as description -FROM variables -LEFT JOIN active_datasets as d on variables.datasetId = d.id -LEFT JOIN namespaces on d.namespace = namespaces.name -WHERE ${whereClause} -ORDER BY variables.id DESC -LIMIT 50 -OFFSET ${offset.toString()}` + `-- sql + SELECT + variables.id as id, + variables.name as name, + chart_configs.patch as config, + d.name as datasetname, + namespaces.name as namespacename, + variables.createdAt as createdAt, + variables.updatedAt as updatedAt, + variables.description as description + FROM variables + LEFT JOIN active_datasets as d on variables.datasetId = d.id + LEFT JOIN namespaces on d.namespace = namespaces.name + LEFT JOIN chart_configs on variables.grapherConfigIdAdmin = chart_configs.id + WHERE ${whereClause} + ORDER BY variables.id DESC + LIMIT 50 + OFFSET ${offset.toString()} + ` ) const results = resultsWithStringGrapherConfigs.map((row: any) => ({ @@ -1169,11 +1202,14 @@ OFFSET ${offset.toString()}` })) const resultCount = await db.knexRaw<{ count: number }>( trx, - `SELECT count(*) as count -FROM variables -LEFT JOIN active_datasets as d on variables.datasetId = d.id -LEFT JOIN namespaces on d.namespace = namespaces.name -WHERE ${whereClause}` + `-- sql + SELECT count(*) as count + FROM variables + LEFT JOIN active_datasets as d on variables.datasetId = d.id + LEFT JOIN namespaces on d.namespace = namespaces.name + LEFT JOIN chart_configs on variables.grapherConfigIdAdmin = chart_configs.id + WHERE ${whereClause} + ` ) return { rows: results, numTotalRows: resultCount[0].count } } @@ -1187,14 +1223,25 @@ patchRouteWithRWTransaction( const variableIds = new Set(patchesList.map((patch) => patch.id)) const configsAndIds = await db.knexRaw< - Pick - >(trx, `SELECT id, grapherConfigAdmin FROM variables where id IN (?)`, [ - [...variableIds.values()], - ]) + Pick & { + grapherConfigAdmin: DbRawChartConfig["patch"] + } + >( + trx, + `-- sql + SELECT v.id, cc.patch AS grapherConfigAdmin + FROM variables v + LEFT JOIN chart_configs cc ON v.grapherConfigIdAdmin = cc.id + WHERE v.id IN (?) + `, + [[...variableIds.values()]] + ) const configMap = new Map( configsAndIds.map((item: any) => [ item.id, - item.grapherConfigAdmin ? JSON.parse(item.grapherConfig) : {}, + item.grapherConfigAdmin + ? JSON.parse(item.grapherConfigAdmin) + : {}, ]) ) // console.log("ids", configsAndIds.map((item : any) => item.id)) @@ -1204,11 +1251,9 @@ patchRouteWithRWTransaction( } for (const [variableId, newConfig] of configMap.entries()) { - await db.knexRaw( - trx, - `UPDATE variables SET grapherConfigAdmin = ? where id = ?`, - [JSON.stringify(newConfig), variableId] - ) + const variable = await getGrapherConfigsForVariable(trx, variableId) + if (!variable) continue + await updateGrapherConfigAdminOfVariable(trx, variable, newConfig) } return { success: true } @@ -1236,6 +1281,43 @@ getRouteWithROTransaction( } ) +getRouteWithROTransaction( + apiRouter, + "/variables/grapherConfigAdmin/:variableId.patchConfig.json", + async (req, res, trx) => { + const variableId = expectInt(req.params.variableId) + console.log("here", variableId) + const variable = await getGrapherConfigsForVariable(trx, variableId) + if (!variable) { + throw new JsonError(`Variable with id ${variableId} not found`, 500) + } + return variable.admin?.patchConfig ?? {} + } +) + +getRouteWithROTransaction( + apiRouter, + "/variables/grapherConfigAdmin/:variableId.parentConfig.json", + async (req, res, trx) => { + const variableId = expectInt(req.params.variableId) + const parentConfig = await getParentConfigForIndicatorChartAdmin( + trx, + variableId + ) + return parentConfig ?? {} + } +) + +getRouteWithROTransaction( + apiRouter, + "/variables/mergedGrapherConfig/:variableId.json", + async (req, res, trx) => { + const variableId = expectInt(req.params.variableId) + const config = await getMergedGrapherConfigForVariable(trx, variableId) + return config ?? {} + } +) + // Used in VariableEditPage getRouteWithROTransaction( apiRouter, @@ -1272,22 +1354,9 @@ getRouteWithROTransaction( await assignTagsForCharts(trx, charts) const grapherConfig = await getMergedGrapherConfigForVariable( - variableId, - trx + trx, + variableId ) - if ( - grapherConfig && - (!grapherConfig.dimensions || grapherConfig.dimensions.length === 0) - ) { - const dimensions: OwidChartDimensionInterface[] = [ - { - variableId: variableId, - property: DimensionProperty.y, - display: variable.display, - }, - ] - grapherConfig.dimensions = dimensions - } const variablesWithCharts: OwidVariableWithSource & { charts: Record @@ -1304,6 +1373,104 @@ getRouteWithROTransaction( } ) +// inserts a new config or updates an existing one +putRouteWithRWTransaction( + apiRouter, + "/variables/:variableId/grapherConfigETL", + async (req, res, trx) => { + const variableId = expectInt(req.params.variableId) + + const variable = await getGrapherConfigsForVariable(trx, variableId) + if (!variable) { + throw new JsonError(`Variable with id ${variableId} not found`, 500) + } + + const updatedChartIds = await updateGrapherConfigETLOfVariable( + trx, + variable, + req.body + ) + + // trigger build if any chart has been updated + if (updatedChartIds.length > 0) { + await triggerStaticBuild( + res.locals.user, + `Updating ETL config for variable ${variableId}` + ) + } + + return { success: true } + } +) + +deleteRouteWithRWTransaction( + apiRouter, + "/variables/:variableId/grapherConfigETL", + async (req, res, trx) => { + const variableId = expectInt(req.params.variableId) + + const variable = await getGrapherConfigsForVariable(trx, variableId) + if (!variable) { + throw new JsonError(`Variable with id ${variableId} not found`, 500) + } + + if (!variable.etl) { + throw new JsonError( + `Variable with id ${variableId} doesn't have an ETL config`, + 500 + ) + } + + // remove reference in the variables table + await db.knexRaw( + trx, + `-- sql + UPDATE variables + SET grapherConfigIdETL = NULL + WHERE id = ? + `, + [variableId] + ) + + // delete row in the chart_configs table + await db.knexRaw( + trx, + `-- sql + DELETE FROM chart_configs + WHERE id = ? + `, + [variable.etl.configId] + ) + + // update admin config if there is one + if (variable.admin) { + await updateExistingFullConfig(trx, { + configId: variable.admin.configId, + config: variable.admin.patchConfig, + }) + } + + // update all charts that inherit from the indicator + const updatedChartIds = await updateAllChartsThatInheritFromIndicator( + trx, + variableId, + { + patchConfigAdmin: variable.admin?.patchConfig, + } + ) + + // trigger build if any chart has been updated + if (updatedChartIds.length > 0) { + await triggerStaticBuild( + res.locals.user, + `Updating ETL config for variable ${variableId}` + ) + } + + return { success: true } + } +) + getRouteWithROTransaction( apiRouter, "/datasets.json", diff --git a/adminSiteServer/app.test.tsx b/adminSiteServer/app.test.tsx index 1857d553b46..45d0b052c77 100644 --- a/adminSiteServer/app.test.tsx +++ b/adminSiteServer/app.test.tsx @@ -43,12 +43,17 @@ import knex, { Knex } from "knex" import { dbTestConfig } from "../db/tests/dbTestConfig.js" import { TransactionCloseMode, - knexRaw, knexReadWriteTransaction, setKnexInstance, } from "../db/db.js" -import { cleanTestDb } from "../db/tests/testHelpers.js" -import { ChartsTableName } from "@ourworldindata/types" +import { cleanTestDb, TABLES_IN_USE } from "../db/tests/testHelpers.js" +import { + ChartConfigsTableName, + ChartsTableName, + DatasetsTableName, + VariablesTableName, +} from "@ourworldindata/types" +import { defaultGrapherConfig } from "@ourworldindata/grapher" import path from "path" import fs from "fs" @@ -101,15 +106,22 @@ beforeAll(async () => { async function cleanupDb() { // We leave the user in the database for other tests to use // For other cases it is good to drop any rows created in the test + const tables = TABLES_IN_USE.filter((table) => table !== "users") await knexReadWriteTransaction( async (trx) => { - await knexRaw(trx, `DELETE FROM posts_gdocs`, []) + for (const table of tables) { + await trx.raw(`DELETE FROM ${table}`) + } }, TransactionCloseMode.KeepOpen, testKnexInstance ) } +afterEach(async () => { + await cleanupDb() +}) + afterAll((done: any) => { void cleanupDb() .then(() => @@ -129,6 +141,35 @@ async function getCountForTable(tableName: string): Promise { return count[0]["count(*)"] as number } +async function fetchJsonFromApi(path: string) { + const url = `http://localhost:8765/admin/api${path}` + const response = await fetch(url, { + headers: { cookie: `sessionid=${cookieId}` }, + }) + expect(response.status).toBe(200) + return await response.json() +} + +const currentSchema = defaultGrapherConfig["$schema"] +const testChartConfig = { + slug: "test-chart", + title: "Test chart", + type: "LineChart", +} +// TODO(inheritance) +// const testChartConfigWithDimensions = { +// id: 10, +// slug: "test-chart", +// title: "Test chart", +// type: "LineChart", +// dimensions: [ +// { +// variableId: 123456, +// property: "y", +// }, +// ], +// } + describe("OwidAdminApp", () => { it("should be able to create an app", () => { expect(app).toBeTruthy() @@ -147,29 +188,66 @@ describe("OwidAdminApp", () => { expect(text).toBe("v18.16.0") }) - it("should be able to add a new chart via the api", async () => { + it("should be able to edit a chart via the api", async () => { const chartCount = await getCountForTable(ChartsTableName) expect(chartCount).toBe(0) + const chartConfigsCount = await getCountForTable(ChartConfigsTableName) + expect(chartConfigsCount).toBe(0) + + const chartId = 1 // since the chart is the first to be inserted + const response = await fetch("http://localhost:8765/admin/api/charts", { method: "POST", headers: { "Content-Type": "application/json", cookie: `sessionid=${cookieId}`, }, - body: JSON.stringify({ - slug: "test-chart", - config: { - title: "Test chart", - type: "LineChart", - }, - }), + body: JSON.stringify(testChartConfig), }) expect(response.status).toBe(200) const text = await response.json() expect(text.success).toBe(true) + expect(text.chartId).toBe(chartId) + // check that a row in the charts table has been added const chartCountAfter = await getCountForTable(ChartsTableName) expect(chartCountAfter).toBe(1) + + // check that a row in the chart_configs table has been added + const chartConfigsCountAfter = await getCountForTable( + ChartConfigsTableName + ) + expect(chartConfigsCountAfter).toBe(1) + + // fetch the full config and check if it's merged with the default + const fullConfig = await fetchJsonFromApi( + `/charts/${chartId}.config.json` + ) + expect(fullConfig).toHaveProperty("$schema", currentSchema) + expect(fullConfig).toHaveProperty("id", chartId) // must match the db id + expect(fullConfig).toHaveProperty("version", 1) // added version + expect(fullConfig).toHaveProperty("slug", "test-chart") + expect(fullConfig).toHaveProperty("title", "Test chart") + expect(fullConfig).toHaveProperty("type", "LineChart") // default property + expect(fullConfig).toHaveProperty("tab", "chart") // default property + + // fetch the patch config and check if it's diffed correctly + const patchConfig = await fetchJsonFromApi( + `/charts/${chartId}.patchConfig.json` + ) + expect(patchConfig).toEqual({ + $schema: defaultGrapherConfig["$schema"], + id: chartId, + version: 1, + slug: "test-chart", + title: "Test chart", + }) + + // fetch the parent config and check if it's the default object + const parentConfig = await fetchJsonFromApi( + `/charts/${chartId}.parentConfig.json` + ) + expect(parentConfig).toEqual(defaultGrapherConfig) }) it("should be able to create a GDoc article", async () => { @@ -190,15 +268,106 @@ describe("OwidAdminApp", () => { expect(text.id).toBe(gdocId) // Fetch the GDoc to verify it was created - const gdocResponse = await fetch( - `http://localhost:8765/admin/api/gdocs/${gdocId}`, + const gdoc = await fetchJsonFromApi(`/gdocs/${gdocId}`) + expect(gdoc.id).toBe(gdocId) + expect(gdoc.content.title).toBe("Basic article") + }) +}) + +describe("OwidAdminApp: indicator-level chart configs", () => { + const dummyDataset = { + name: "Dummy dataset", + description: "Dataset description", + namespace: "owid", + createdByUserId: 1, + metadataEditedAt: new Date(), + metadataEditedByUserId: 1, + dataEditedAt: new Date(), + dataEditedByUserId: 1, + } + const dummyVariable = { + unit: "kg", + coverage: "Global by country", + timespan: "2000-2020", + datasetId: 1, + display: '{ "unit": "kg", "shortUnit": "kg" }', + } + + const variableId = 1 + const testVariableConfig = { + hasMapTab: true, + } + + beforeAll(async () => { + await testKnexInstance!(DatasetsTableName).insert([dummyDataset]) + await testKnexInstance!(VariablesTableName).insert([dummyVariable]) + }) + + it("should be able to edit ETL grapher configs via the api", async () => { + const chartConfigsCount = await getCountForTable(ChartConfigsTableName) + expect(chartConfigsCount).toBe(0) + + // add a grapher config for a variable + let response = await fetch( + `http://localhost:8765/admin/api/variables/${variableId}/grapherConfigETL`, { - headers: { cookie: `sessionid=${cookieId}` }, + method: "PUT", + headers: { + "Content-Type": "application/json", + cookie: `sessionid=${cookieId}`, + }, + body: JSON.stringify(testVariableConfig), } ) - expect(gdocResponse.status).toBe(200) - const gdoc = await gdocResponse.json() - expect(gdoc.id).toBe(gdocId) - expect(gdoc.content.title).toBe("Basic article") + expect(response.status).toBe(200) + let text = await response.json() + expect(text.success).toBe(true) + + // check that a row in the chart_configs table has been added + const chartConfigsCountAfter = await getCountForTable( + ChartConfigsTableName + ) + expect(chartConfigsCountAfter).toBe(1) + + // get inserted configs from the database + const row = await testKnexInstance!(ChartConfigsTableName).first() + const patchConfig = JSON.parse(row.patch) + const fullConfig = JSON.parse(row.full) + + // for ETL configs, patch and full configs should be the same + expect(patchConfig).toEqual(fullConfig) + + // check that $schema and dimensions field were added to the config + expect(patchConfig).toEqual({ + ...testVariableConfig, + $schema: currentSchema, + dimensions: [ + { + property: "y", + variableId, + }, + ], + }) + + // delete the grapher config we just added + response = await fetch( + `http://localhost:8765/admin/api/variables/${variableId}/grapherConfigETL`, + { + method: "DELETE", + headers: { + "Content-Type": "application/json", + cookie: `sessionid=${cookieId}`, + }, + } + ) + expect(response.status).toBe(200) + text = await response.json() + expect(text.success).toBe(true) + + // check that the row in the chart_configs table has been deleted + const chartConfigsCountAfterDelete = await getCountForTable( + ChartConfigsTableName + ) + expect(chartConfigsCountAfterDelete).toBe(0) }) }) diff --git a/baker/GrapherBaker.tsx b/baker/GrapherBaker.tsx index f810f822d3c..baabc0cb882 100644 --- a/baker/GrapherBaker.tsx +++ b/baker/GrapherBaker.tsx @@ -9,8 +9,8 @@ import { deserializeJSONFromHTML, uniq, keyBy, - mergePartialGrapherConfigs, compact, + mergeGrapherConfigs, } from "@ourworldindata/utils" import fs from "fs-extra" import * as lodash from "lodash" @@ -135,16 +135,15 @@ export async function renderDataPageV2( knex: db.KnexReadWriteTransaction ) { const grapherConfigForVariable = await getMergedGrapherConfigForVariable( - variableId, - knex + knex, + variableId ) // Only merge the grapher config on the indicator if the caller tells us to do so - // this is true for preview pages for datapages on the indicator level but false // if we are on Grapher pages. Once we have a good way in the grapher admin for how // to use indicator level defaults, we should reconsider how this works here. - // TODO(inheritance): use mergeGrapherConfigs instead const grapher = useIndicatorGrapherConfigs - ? mergePartialGrapherConfigs(grapherConfigForVariable, pageGrapher) + ? mergeGrapherConfigs(grapherConfigForVariable ?? {}, pageGrapher ?? {}) : pageGrapher ?? {} const faqDocIds = compact( diff --git a/baker/siteRenderers.tsx b/baker/siteRenderers.tsx index 3a4ec6635bf..54ee8976c02 100644 --- a/baker/siteRenderers.tsx +++ b/baker/siteRenderers.tsx @@ -40,10 +40,10 @@ import { JsonError, Url, IndexPost, - mergePartialGrapherConfigs, OwidGdocType, OwidGdoc, OwidGdocDataInsightInterface, + mergeGrapherConfigs, } from "@ourworldindata/utils" import { extractFormattingOptions } from "../serverUtils/wordpressUtils.js" import { @@ -761,7 +761,16 @@ export const renderExplorerPage = async ( if (requiredVariableIds.length) { partialGrapherConfigRows = await knexRaw( knex, - `SELECT id, grapherConfigETL, grapherConfigAdmin FROM variables WHERE id IN (?)`, + `-- sql + SELECT + v.id, + cc_etl.patch AS grapherConfigETL, + cc_admin.patch AS grapherConfigAdmin + FROM variables v + LEFT JOIN chart_configs cc_admin ON cc_admin.id=v.grapherConfigIdAdmin + LEFT JOIN chart_configs cc_etl ON cc_etl.id=v.grapherConfigIdETL + WHERE v.id IN (?) + `, [requiredVariableIds] ) @@ -801,8 +810,7 @@ export const renderExplorerPage = async ( config: row.grapherConfigETL as string, }) : {} - // TODO(inheritance): use mergeGrapherConfigs instead - return mergePartialGrapherConfigs(etlConfig, adminConfig) + return mergeGrapherConfigs(etlConfig, adminConfig) }) const wpContent = transformedProgram.wpBlockId diff --git a/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts new file mode 100644 index 00000000000..a972f51d45a --- /dev/null +++ b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts @@ -0,0 +1,239 @@ +import { defaultGrapherConfig } from "@ourworldindata/grapher" +import { DimensionProperty, GrapherInterface } from "@ourworldindata/types" +import { mergeGrapherConfigs, omit } from "@ourworldindata/utils" +import { MigrationInterface, QueryRunner } from "typeorm" + +export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 + implements MigrationInterface +{ + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`-- sql + ALTER TABLE variables + ADD COLUMN grapherConfigIdAdmin binary(16) UNIQUE AFTER sort, + ADD COLUMN grapherConfigIdETL binary(16) UNIQUE AFTER grapherConfigIdAdmin, + ADD CONSTRAINT fk_variables_grapherConfigIdAdmin + FOREIGN KEY (grapherConfigIdAdmin) + REFERENCES chart_configs (id) + ON DELETE RESTRICT + ON UPDATE RESTRICT, + ADD CONSTRAINT fk_variables_grapherConfigIdETL + FOREIGN KEY (grapherConfigIdETL) + REFERENCES chart_configs (id) + ON DELETE RESTRICT + ON UPDATE RESTRICT + `) + + // note that we copy the ETL-authored configs to the chart_configs table, + // but drop the admin-authored configs + + const variables = await queryRunner.query(`-- sql + SELECT id, grapherConfigETL + FROM variables + WHERE grapherConfigETL IS NOT NULL + `) + + for (const { id: variableId, grapherConfigETL } of variables) { + let config: GrapherInterface = JSON.parse(grapherConfigETL) + + // if the config has no schema, assume it's the default version + if (!config.$schema) { + config.$schema = defaultGrapherConfig.$schema + } + + // check if the given dimensions are correct + if (config.dimensions && config.dimensions.length >= 1) { + // make sure there is only a single entry + config.dimensions = config.dimensions.slice(0, 1) + // make sure the variable id matches + config.dimensions[0].variableId = variableId + } + + // fill dimensions if not given to make the config plottable + if (!config.dimensions || config.dimensions.length === 0) { + config.dimensions = [ + { property: DimensionProperty.y, variableId }, + ] + } + + // we have v3 configs in the database (the current version is v4); + // turn these into v4 configs by removing the `data` property + // which was the breaking change that lead to v4 + // (we don't have v2 or v1 configs in the database, so we don't need to handle those) + if ( + config.$schema === + "https://files.ourworldindata.org/schemas/grapher-schema.003.json" + ) { + config = omit(config, "data") + config.$schema = defaultGrapherConfig.$schema + } + + // insert config into the chart_configs table + const configId = await getBinaryUUID(queryRunner) + await queryRunner.query( + `-- sql + INSERT INTO chart_configs (id, patch, full) + VALUES (?, ?, ?) + `, + [configId, JSON.stringify(config), JSON.stringify(config)] + ) + + // update reference in the variables table + await queryRunner.query( + `-- sql + UPDATE variables + SET grapherConfigIdETL = ? + WHERE id = ? + `, + [configId, variableId] + ) + } + + // drop `grapherConfigAdmin` and `grapherConfigETL` columns + await queryRunner.query(`-- sql + ALTER TABLE variables + DROP COLUMN grapherConfigAdmin, + DROP COLUMN grapherConfigETL + `) + + // add a view that lists all charts that inherit from an indicator + await queryRunner.query(`-- sql + CREATE VIEW inheriting_charts AS ( + WITH y_dimensions AS ( + SELECT + * + FROM + chart_dimensions + WHERE + property = 'y' + ), + single_y_indicator_charts As ( + SELECT + c.id as chartId, + cc.patch as patchConfig, + max(yd.variableId) as variableId + FROM + charts c + JOIN chart_configs cc ON cc.id = c.configId + JOIN y_dimensions yd ON c.id = yd.chartId + WHERE + cc.full ->> '$.type' != 'ScatterPlot' + GROUP BY + c.id + HAVING + COUNT(distinct yd.variableId) = 1 + ) + SELECT + variableId, + chartId + FROM + single_y_indicator_charts + ORDER BY + variableId + ) + `) + + // update the full column of every chart that inherits from an indicator + const inheritancePairs = await queryRunner.query(`-- sql + SELECT + ic.chartId, + ic.variableId, + cc_chart.patch AS chartPatch, + cc_variable.patch AS variablePatch + FROM inheriting_charts ic + JOIN chart_configs cc_chart ON cc_chart.id = ( + SELECT configId FROM charts + WHERE id = ic.chartId + ) + JOIN chart_configs cc_variable ON cc_variable.id = ( + SELECT grapherConfigIdETL FROM variables + WHERE id = ic.variableId + ) + `) + for (const pair of inheritancePairs) { + const fullChartConfig = mergeGrapherConfigs( + defaultGrapherConfig, + JSON.parse(pair.variablePatch), + JSON.parse(pair.chartPatch) + ) + await queryRunner.query( + `-- sql + UPDATE chart_configs + SET full = ? + WHERE id = ( + SELECT configId FROM charts + WHERE id = ? + ) + `, + [JSON.stringify(fullChartConfig), pair.chartId] + ) + } + } + + public async down(queryRunner: QueryRunner): Promise { + // drop view + await queryRunner.query(`-- sql + DROP VIEW inheriting_charts + `) + + // add back the `grapherConfigAdmin` and `grapherConfigETL` columns + await queryRunner.query(`-- sql + ALTER TABLE variables + ADD COLUMN grapherConfigAdmin json AFTER sort, + ADD COLUMN grapherConfigETL json AFTER grapherConfigAdmin + `) + + // copy configs from the chart_configs table to the variables table + await queryRunner.query(`-- sql + UPDATE variables v + JOIN chart_configs cc ON v.grapherConfigIdETL = cc.id + SET v.grapherConfigETL = cc.patch + `) + + // remove constraints on the `grapherConfigIdAdmin` and `grapherConfigIdETL` columns + await queryRunner.query(`-- sql + ALTER TABLE variables + DROP CONSTRAINT fk_variables_grapherConfigIdAdmin, + DROP CONSTRAINT fk_variables_grapherConfigIdETL + `) + + // drop rows from the chart_configs table + await queryRunner.query(`-- sql + DELETE FROM chart_configs + WHERE id IN ( + SELECT grapherConfigIdETL FROM variables + WHERE grapherConfigIdETL IS NOT NULL + ) + `) + + // remove the `grapherConfigIdAdmin` and `grapherConfigIdETL` columns + await queryRunner.query(`-- sql + ALTER TABLE variables + DROP COLUMN grapherConfigIdAdmin, + DROP COLUMN grapherConfigIdETL + `) + + // restore full configs of standalone charts + const charts = await queryRunner.query(`-- sql + SELECT id AS configId, patch FROM chart_configs + `) + for (const chart of charts) { + const fullConfig = mergeGrapherConfigs( + defaultGrapherConfig, + JSON.parse(chart.patch) + ) + await queryRunner.query( + `-- sql + UPDATE chart_configs + SET full = ? + WHERE id = ? + `, + [JSON.stringify(fullConfig), chart.configId] + ) + } + } +} + +const getBinaryUUID = async (queryRunner: QueryRunner): Promise => { + const rows = await queryRunner.query(`SELECT UUID_TO_BIN(UUID(), 1) AS id`) + return rows[0].id +} diff --git a/db/model/Chart.ts b/db/model/Chart.ts index 41f04cf2138..fbb48a80beb 100644 --- a/db/model/Chart.ts +++ b/db/model/Chart.ts @@ -1,11 +1,16 @@ import * as lodash from "lodash" import * as db from "../db.js" -import { getDataForMultipleVariables } from "./Variable.js" +import { + getDataForMultipleVariables, + getGrapherConfigsForVariable, +} from "./Variable.js" import { JsonError, KeyChartLevel, MultipleOwidVariableDataDimensionsMap, DbChartTagJoin, + mergeGrapherConfigs, + getParentIndicatorIdFromChartConfig, } from "@ourworldindata/utils" import { GrapherInterface, @@ -19,6 +24,7 @@ import { DbRawChartConfig, DbEnrichedChartConfig, } from "@ourworldindata/types" +import { defaultGrapherConfig } from "@ourworldindata/grapher" import { OpenAI } from "openai" import { BAKED_BASE_URL, @@ -154,6 +160,24 @@ export async function getRawChartById( return chart } +export async function getPatchConfigByChartId( + knex: db.KnexReadonlyTransaction, + id: number +): Promise { + const chart = await db.knexRawFirst>( + knex, + `-- sql + SELECT patch + FROM chart_configs cc + JOIN charts c ON c.configId = cc.id + WHERE c.id = ? + `, + [id] + ) + if (!chart) return undefined + return parseChartConfig(chart.patch) +} + export async function getEnrichedChartById( knex: db.KnexReadonlyTransaction, id: number @@ -232,6 +256,46 @@ export async function getChartConfigBySlug( return { id: row.id, config: parseChartConfig(row.config) } } +export async function getParentConfigForChart( + trx: db.KnexReadonlyTransaction, + chartId: number +): Promise { + // check if the chart inherits settings from an indicator + const parentVariable = await db.knexRawFirst<{ id: number }>( + trx, + `-- sql + SELECT variableId AS id + FROM inheriting_charts + WHERE chartId = ? + `, + [chartId] + ) + + // all charts inherit from the default config + if (!parentVariable) return defaultGrapherConfig + + const variable = await getGrapherConfigsForVariable(trx, parentVariable.id) + return mergeGrapherConfigs( + defaultGrapherConfig, + variable?.etl?.patchConfig ?? {}, + variable?.admin?.patchConfig ?? {} + ) +} + +export async function getParentConfigForChartFromConfig( + trx: db.KnexReadonlyTransaction, + config: GrapherInterface +): Promise { + const parentVariableId = getParentIndicatorIdFromChartConfig(config) + if (!parentVariableId) return defaultGrapherConfig + const variable = await getGrapherConfigsForVariable(trx, parentVariableId) + return mergeGrapherConfigs( + defaultGrapherConfig, + variable?.etl?.patchConfig ?? {}, + variable?.admin?.patchConfig ?? {} + ) +} + export async function setChartTags( knex: db.KnexReadWriteTransaction, chartId: number, diff --git a/db/model/ChartConfigs.ts b/db/model/ChartConfigs.ts new file mode 100644 index 00000000000..f66d3ff7eef --- /dev/null +++ b/db/model/ChartConfigs.ts @@ -0,0 +1,68 @@ +import { DbInsertChartConfig, GrapherInterface } from "@ourworldindata/types" + +import * as db from "../db.js" + +interface ConfigWithId { + configId: DbInsertChartConfig["id"] + config: GrapherInterface +} + +export async function updateExistingConfigPair( + knex: db.KnexReadWriteTransaction, + { + configId, + patchConfig, + fullConfig, + }: { + configId: DbInsertChartConfig["id"] + patchConfig: GrapherInterface + fullConfig: GrapherInterface + } +): Promise { + await db.knexRaw( + knex, + `-- sql + UPDATE chart_configs + SET + patch = ?, + full = ? + WHERE id = ? + `, + [JSON.stringify(patchConfig), JSON.stringify(fullConfig), configId] + ) +} + +export async function updateExistingPatchConfig( + knex: db.KnexReadWriteTransaction, + params: ConfigWithId +): Promise { + await updateExistingConfig(knex, { ...params, column: "patch" }) +} + +export async function updateExistingFullConfig( + knex: db.KnexReadWriteTransaction, + params: ConfigWithId +): Promise { + await updateExistingConfig(knex, { ...params, column: "full" }) +} + +export async function updateExistingConfig( + knex: db.KnexReadWriteTransaction, + { + column, + configId, + config, + }: ConfigWithId & { + column: "patch" | "full" + } +): Promise { + await db.knexRaw( + knex, + `-- sql + UPDATE chart_configs + SET ?? = ? + WHERE id = ? + `, + [column, JSON.stringify(config), configId] + ) +} diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 8bcd74abe0e..075814e1f31 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -1,10 +1,17 @@ import _ from "lodash" import { Writable } from "stream" import * as db from "../db.js" -import { retryPromise, isEmpty } from "@ourworldindata/utils" +import { + retryPromise, + isEmpty, + omitUndefinedValues, + mergeGrapherConfigs, +} from "@ourworldindata/utils" import { getVariableDataRoute, getVariableMetadataRoute, + defaultGrapherConfig, + DEFAULT_GRAPHER_CONFIG_SCHEMA, } from "@ourworldindata/grapher" import pl from "nodejs-polars" import { DATA_API_URL } from "../../settings/serverSettings.js" @@ -20,33 +27,335 @@ import { GrapherInterface, DbRawVariable, VariablesTableName, + DbRawChartConfig, + parseChartConfig, + DbEnrichedChartConfig, + DbEnrichedVariable, } from "@ourworldindata/types" -import { knexRaw } from "../db.js" +import { knexRaw, knexRawFirst } from "../db.js" +import { + updateExistingConfigPair, + updateExistingFullConfig, +} from "./ChartConfigs.js" + +interface ChartConfigPair { + configId: DbEnrichedChartConfig["id"] + patchConfig: DbEnrichedChartConfig["patch"] + fullConfig: DbEnrichedChartConfig["full"] +} -//export type Field = keyof VariableRow +interface VariableWithGrapherConfigs { + variableId: DbEnrichedVariable["id"] + admin?: ChartConfigPair + etl?: ChartConfigPair +} -export async function getMergedGrapherConfigForVariable( - variableId: number, - knex: db.KnexReadonlyTransaction -): Promise { - const rows: Pick< - DbRawVariable, - "grapherConfigAdmin" | "grapherConfigETL" - >[] = await knexRaw( +export async function getGrapherConfigsForVariable( + knex: db.KnexReadonlyTransaction, + variableId: number +): Promise { + const variable = await knexRawFirst< + Pick< + DbRawVariable, + "id" | "grapherConfigIdAdmin" | "grapherConfigIdETL" + > & { + patchConfigAdmin?: DbRawChartConfig["patch"] + patchConfigETL?: DbRawChartConfig["patch"] + fullConfigAdmin?: DbRawChartConfig["full"] + fullConfigETL?: DbRawChartConfig["full"] + } + >( knex, - `SELECT grapherConfigAdmin, grapherConfigETL FROM variables WHERE id = ?`, + `-- sql + SELECT + v.id, + v.grapherConfigIdAdmin, + v.grapherConfigIdETL, + cc_admin.patch AS patchConfigAdmin, + cc_admin.full AS fullConfigAdmin, + cc_etl.patch AS patchConfigETL, + cc_etl.full AS fullConfigETL + FROM variables v + LEFT JOIN chart_configs cc_admin ON v.grapherConfigIdAdmin = cc_admin.id + LEFT JOIN chart_configs cc_etl ON v.grapherConfigIdETL = cc_etl.id + WHERE v.id = ? + `, [variableId] ) - if (!rows.length) return - const row = rows[0] - const grapherConfigAdmin = row.grapherConfigAdmin - ? JSON.parse(row.grapherConfigAdmin) + + if (!variable) return + + const maybeParseChartConfig = ( + config: string | undefined + ): GrapherInterface => (config ? parseChartConfig(config) : {}) + + const admin = variable.grapherConfigIdAdmin + ? { + configId: variable.grapherConfigIdAdmin, + patchConfig: maybeParseChartConfig(variable.patchConfigAdmin), + fullConfig: maybeParseChartConfig(variable.fullConfigAdmin), + } : undefined - const grapherConfigETL = row.grapherConfigETL - ? JSON.parse(row.grapherConfigETL) + + const etl = variable.grapherConfigIdETL + ? { + configId: variable.grapherConfigIdETL, + patchConfig: maybeParseChartConfig(variable.patchConfigETL), + fullConfig: maybeParseChartConfig(variable.fullConfigETL), + } : undefined - // TODO(inheritance): use mergeGrapherConfigs instead - return _.merge({}, grapherConfigAdmin, grapherConfigETL) + + return omitUndefinedValues({ + variableId: variable.id, + admin, + etl, + }) +} + +export async function getMergedGrapherConfigForVariable( + knex: db.KnexReadonlyTransaction, + variableId: number +): Promise { + const variable = await getGrapherConfigsForVariable(knex, variableId) + return variable?.admin?.fullConfig ?? variable?.etl?.fullConfig +} + +export async function insertNewGrapherConfigForVariable( + knex: db.KnexReadonlyTransaction, + { + type, + variableId, + patchConfig, + fullConfig, + }: { + type: "admin" | "etl" + variableId: number + patchConfig: GrapherInterface + fullConfig: GrapherInterface + } +): Promise { + // insert chart config into the database + const configId = await db.getBinaryUUID(knex) + await db.knexRaw( + knex, + `-- sql + INSERT INTO chart_configs (id, patch, full) + VALUES (?, ?, ?) + `, + [configId, JSON.stringify(patchConfig), JSON.stringify(fullConfig)] + ) + + // make a reference to the config from the variables table + const column = + type === "admin" ? "grapherConfigIdAdmin" : "grapherConfigIdETL" + await db.knexRaw( + knex, + `-- sql + UPDATE variables + SET ?? = ? + WHERE id = ? + `, + [column, configId, variableId] + ) +} + +function makeConfigValidForIndicator({ + config, + variableId, +}: { + config: GrapherInterface + variableId: number +}): GrapherInterface { + const updatedConfig = { ...config } + + // if no schema is given, assume it's the latest + if (!updatedConfig.$schema) { + updatedConfig.$schema = DEFAULT_GRAPHER_CONFIG_SCHEMA + } + + // check if the given dimensions are correct + if (updatedConfig.dimensions && updatedConfig.dimensions.length >= 1) { + // make sure there is only a single entry + updatedConfig.dimensions = updatedConfig.dimensions.slice(0, 1) + // make sure the variable id matches + updatedConfig.dimensions[0].variableId = variableId + } + + // fill dimensions if not given to make the updatedConfig plottable + if (!updatedConfig.dimensions || updatedConfig.dimensions.length === 0) { + updatedConfig.dimensions = [ + { property: DimensionProperty.y, variableId }, + ] + } + + return updatedConfig +} + +async function findAllChartsThatInheritFromIndicator( + trx: db.KnexReadonlyTransaction, + variableId: number +): Promise< + { chartId: number; patchConfig: GrapherInterface; isPublished: boolean }[] +> { + const charts = await db.knexRaw<{ + chartId: number + patchConfig: string + isPublished: boolean + }>( + trx, + `-- sql + SELECT + c.id as chartId, + cc.patch as patchConfig, + cc.full ->> "$.isPublished" as isPublished + FROM inheriting_charts ic + JOIN charts c ON c.id = ic.chartId + JOIN chart_configs cc ON cc.id = c.configId + WHERE ic.variableId = ? + `, + [variableId] + ) + return charts.map((chart) => ({ + ...chart, + patchConfig: parseChartConfig(chart.patchConfig), + })) +} + +export async function updateAllChartsThatInheritFromIndicator( + trx: db.KnexReadWriteTransaction, + variableId: number, + { + patchConfigETL, + patchConfigAdmin, + }: { + patchConfigETL?: GrapherInterface + patchConfigAdmin?: GrapherInterface + } +): Promise { + const inheritingCharts = await findAllChartsThatInheritFromIndicator( + trx, + variableId + ) + for (const chart of inheritingCharts) { + const fullConfig = mergeGrapherConfigs( + defaultGrapherConfig, + patchConfigETL ?? {}, + patchConfigAdmin ?? {}, + chart.patchConfig + ) + await db.knexRaw( + trx, + `-- sql + UPDATE chart_configs cc + JOIN charts c ON c.configId = cc.id + SET cc.full = ? + WHERE c.id = ? + `, + [JSON.stringify(fullConfig), chart.chartId] + ) + } + // let the caller know if any published charts were updated + return inheritingCharts + .filter((chart) => chart.isPublished) + .map((chart) => chart.chartId) +} + +export async function updateGrapherConfigETLOfVariable( + trx: db.KnexReadWriteTransaction, + variable: VariableWithGrapherConfigs, + config: GrapherInterface +): Promise { + const { variableId } = variable + + const configETL = makeConfigValidForIndicator({ + config, + variableId, + }) + + if (variable.etl) { + await updateExistingConfigPair(trx, { + configId: variable.etl.configId, + patchConfig: configETL, + fullConfig: configETL, + }) + } else { + await insertNewGrapherConfigForVariable(trx, { + type: "etl", + variableId, + patchConfig: configETL, + fullConfig: configETL, + }) + } + + // update admin-authored full config it is exists + if (variable.admin) { + const fullConfig = mergeGrapherConfigs( + configETL, + variable.admin.patchConfig + ) + await updateExistingFullConfig(trx, { + configId: variable.admin.configId, + config: fullConfig, + }) + } + + const updatedChartIds = await updateAllChartsThatInheritFromIndicator( + trx, + variableId, + { + patchConfigETL: configETL, + patchConfigAdmin: variable.admin?.patchConfig, + } + ) + + return updatedChartIds +} + +export async function updateGrapherConfigAdminOfVariable( + trx: db.KnexReadWriteTransaction, + variable: VariableWithGrapherConfigs, + config: GrapherInterface +): Promise { + const { variableId } = variable + + const patchConfigAdmin = makeConfigValidForIndicator({ + config, + variableId, + }) + + const fullConfigAdmin = mergeGrapherConfigs( + variable.etl?.patchConfig ?? {}, + patchConfigAdmin + ) + + if (variable.admin) { + await updateExistingConfigPair(trx, { + configId: variable.admin.configId, + patchConfig: patchConfigAdmin, + fullConfig: fullConfigAdmin, + }) + } else { + await insertNewGrapherConfigForVariable(trx, { + type: "admin", + variableId, + patchConfig: patchConfigAdmin, + fullConfig: fullConfigAdmin, + }) + } + + await updateAllChartsThatInheritFromIndicator(trx, variableId, { + patchConfigETL: variable.etl?.patchConfig ?? {}, + patchConfigAdmin: patchConfigAdmin, + }) +} + +export async function getParentConfigForIndicatorChartAdmin( + trx: db.KnexReadonlyTransaction, + variableId: number +): Promise { + // check if there is an ETL-authored indicator chart + const variable = await getGrapherConfigsForVariable(trx, variableId) + return variable?.etl?.fullConfig } // TODO: these are domain functions and should live somewhere else diff --git a/db/tests/testHelpers.ts b/db/tests/testHelpers.ts index 00037dd4ee7..96904959974 100644 --- a/db/tests/testHelpers.ts +++ b/db/tests/testHelpers.ts @@ -1,13 +1,33 @@ +import { + ChartConfigsTableName, + ChartDimensionsTableName, + ChartRevisionsTableName, + ChartsTableName, + DatasetsTableName, + PostsGdocsTableName, + UsersTableName, + VariablesTableName, +} from "@ourworldindata/types" import { Knex } from "knex" +// the order is important here since we drop rows from the tables in this order +export const TABLES_IN_USE = [ + ChartDimensionsTableName, + ChartRevisionsTableName, + ChartsTableName, + ChartConfigsTableName, + VariablesTableName, + DatasetsTableName, + PostsGdocsTableName, + UsersTableName, +] + export async function cleanTestDb( knexInstance: Knex ): Promise { - await knexInstance.raw("DELETE FROM chart_dimensions") - await knexInstance.raw("DELETE FROM chart_revisions") - await knexInstance.raw("DELETE from charts") - await knexInstance.raw("DELETE FROM posts_gdocs") - await knexInstance.raw("DELETE FROM users") + for (const table of TABLES_IN_USE) { + await knexInstance.raw(`DELETE FROM ${table}`) + } } export function sleep(time: number, value: unknown): Promise { diff --git a/packages/@ourworldindata/grapher/src/index.ts b/packages/@ourworldindata/grapher/src/index.ts index 9e9d08b9902..ed27bd627f5 100644 --- a/packages/@ourworldindata/grapher/src/index.ts +++ b/packages/@ourworldindata/grapher/src/index.ts @@ -26,6 +26,7 @@ export { grapherInterfaceWithHiddenTabsOnly, CONTINENTS_INDICATOR_ID, POPULATION_INDICATOR_ID_USED_IN_ADMIN, + DEFAULT_GRAPHER_CONFIG_SCHEMA, } from "./core/GrapherConstants" export { getVariableDataRoute, diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml index 327bc684c85..afcf1b398ae 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml @@ -1,193 +1,7 @@ $schema: "http://json-schema.org/draft-07/schema#" $id: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" -$defs: - axis: - type: object - properties: - removePointsOutsideDomain: - type: boolean - default: false - label: - type: string - description: Axis label - min: - type: number - description: Minimum domain value of the axis. Inferred from data if not provided. - scaleType: - type: string - description: Toggle linear/logarithmic - default: linear - enum: - - linear - - log - max: - type: number - description: Maximum domain value of the axis. Inferred from data if not provided. - canChangeScaleType: - type: boolean - description: Allow user to change lin/log - default: false - facetDomain: - type: string - description: Whether the axis domain should be the same across faceted charts (if possible) - default: shared - enum: - - independent - - shared - additionalProperties: false - colorScale: - type: object - description: Color scale definition - properties: - customNumericLabels: - type: array - description: | - Custom labels for each numeric bin. Only applied when strategy is `manual`. - `null` falls back to default label. - items: - type: - - string - - "null" - customCategoryColors: - type: object - description: Map of categorical values to colors. Colors are CSS colors, usually in the form `#aa9944` - patternProperties: - ".*": - type: string - baseColorScheme: - type: string - description: | - One of the predefined base color schemes. - If not provided, a default is automatically chosen based on the chart type. - enum: - - YlGn - - YlGnBu - - GnBu - - BuGn - - PuBuGn - - BuPu - - RdPu - - PuRd - - OrRd - - YlOrRd - - YlOrBr - - Purples - - Blues - - Greens - - Oranges - - Reds - - Greys - - PuOr - - BrBG - - PRGn - - PiYG - - RdBu - - RdGy - - RdYlBu - - Spectral - - RdYlGn - - Accent - - Dark2 - - Paired - - Pastel1 - - Pastel2 - - Set1 - - Set2 - - Set3 - - PuBu - - Magma - - Inferno - - Plasma - - Viridis - - continents - - stackedAreaDefault - - owid-distinct - - SingleColorDenim - - SingleColorTeal - - SingleColorPurple - - SingleColorDustyCoral - - SingleColorDarkCopper - - OwidCategoricalA - - OwidCategoricalB - - OwidCategoricalC - - OwidCategoricalD - - OwidCategoricalE - - OwidEnergy - - OwidEnergyLines - - OwidDistinctLines - - BinaryMapPaletteA - - BinaryMapPaletteB - - BinaryMapPaletteC - - BinaryMapPaletteD - - BinaryMapPaletteE - - SingleColorGradientDenim - - SingleColorGradientTeal - - SingleColorGradientPurple - - SingleColorGradientDustyCoral - - SingleColorGradientDarkCopper - equalSizeBins: - type: boolean - default: true - description: Whether the visual scaling for the color legend is disabled. - customHiddenCategories: - type: object - description: Allow hiding categories from the legend - patternProperties: - ".*": - type: boolean - binningStrategy: - type: string - description: The strategy for generating the bin boundaries - default: ckmeans - enum: - - equalInterval - - quantiles - - ckmeans - - manual - legendDescription: - type: string - description: A custom legend description. Only used in ScatterPlot legend titles for now. - customNumericColors: - type: array - description: | - Override some or all colors for the numerical color legend. - Colors are CSS colors, usually in the form `#aa9944` - `null` falls back the color scheme color. - items: - type: - - string - - "null" - customNumericValues: - type: array - description: Custom maximum brackets for each numeric bin. Only applied when strategy is `manual` - items: - type: number - customNumericColorsActive: - type: boolean - default: false - description: Whether `customNumericColors` are used to override the color scheme - colorSchemeInvert: - type: boolean - default: false - description: Reverse the order of colors in the color scheme - customCategoryLabels: - type: object - description: Map of category values to color legend labels. - patternProperties: - ".*": - type: string - binningStrategyBinCount: - type: integer - default: 5 - description: The *suggested* number of bins for the automatic binning algorithm - minimum: 0 - customNumericMinValue: - type: number - description: The minimum bracket of the first bin. Inferred from data if not provided. - additionalProperties: false required: - - title - - version + - $schema - dimensions type: object description: | @@ -199,6 +13,7 @@ properties: description: Url of the concrete schema version to use to validate this document format: uri default: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" + const: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" id: type: integer description: Internal DB id. Useful internally for OWID but not required if just using grapher directly. @@ -677,3 +492,189 @@ properties: - hide - show additionalProperties: false + +$defs: + axis: + type: object + properties: + removePointsOutsideDomain: + type: boolean + default: false + label: + type: string + description: Axis label + min: + type: number + description: Minimum domain value of the axis. Inferred from data if not provided. + scaleType: + type: string + description: Toggle linear/logarithmic + default: linear + enum: + - linear + - log + max: + type: number + description: Maximum domain value of the axis. Inferred from data if not provided. + canChangeScaleType: + type: boolean + description: Allow user to change lin/log + default: false + facetDomain: + type: string + description: Whether the axis domain should be the same across faceted charts (if possible) + default: shared + enum: + - independent + - shared + additionalProperties: false + colorScale: + type: object + description: Color scale definition + properties: + customNumericLabels: + type: array + description: | + Custom labels for each numeric bin. Only applied when strategy is `manual`. + `null` falls back to default label. + items: + type: + - string + - "null" + customCategoryColors: + type: object + description: Map of categorical values to colors. Colors are CSS colors, usually in the form `#aa9944` + patternProperties: + ".*": + type: string + baseColorScheme: + type: string + description: | + One of the predefined base color schemes. + If not provided, a default is automatically chosen based on the chart type. + enum: + - YlGn + - YlGnBu + - GnBu + - BuGn + - PuBuGn + - BuPu + - RdPu + - PuRd + - OrRd + - YlOrRd + - YlOrBr + - Purples + - Blues + - Greens + - Oranges + - Reds + - Greys + - PuOr + - BrBG + - PRGn + - PiYG + - RdBu + - RdGy + - RdYlBu + - Spectral + - RdYlGn + - Accent + - Dark2 + - Paired + - Pastel1 + - Pastel2 + - Set1 + - Set2 + - Set3 + - PuBu + - Magma + - Inferno + - Plasma + - Viridis + - continents + - stackedAreaDefault + - owid-distinct + - SingleColorDenim + - SingleColorTeal + - SingleColorPurple + - SingleColorDustyCoral + - SingleColorDarkCopper + - OwidCategoricalA + - OwidCategoricalB + - OwidCategoricalC + - OwidCategoricalD + - OwidCategoricalE + - OwidEnergy + - OwidEnergyLines + - OwidDistinctLines + - BinaryMapPaletteA + - BinaryMapPaletteB + - BinaryMapPaletteC + - BinaryMapPaletteD + - BinaryMapPaletteE + - SingleColorGradientDenim + - SingleColorGradientTeal + - SingleColorGradientPurple + - SingleColorGradientDustyCoral + - SingleColorGradientDarkCopper + equalSizeBins: + type: boolean + default: true + description: Whether the visual scaling for the color legend is disabled. + customHiddenCategories: + type: object + description: Allow hiding categories from the legend + patternProperties: + ".*": + type: boolean + binningStrategy: + type: string + description: The strategy for generating the bin boundaries + default: ckmeans + enum: + - equalInterval + - quantiles + - ckmeans + - manual + legendDescription: + type: string + description: A custom legend description. Only used in ScatterPlot legend titles for now. + customNumericColors: + type: array + description: | + Override some or all colors for the numerical color legend. + Colors are CSS colors, usually in the form `#aa9944` + `null` falls back the color scheme color. + items: + type: + - string + - "null" + customNumericValues: + type: array + description: Custom maximum brackets for each numeric bin. Only applied when strategy is `manual` + items: + type: number + customNumericColorsActive: + type: boolean + default: false + description: Whether `customNumericColors` are used to override the color scheme + colorSchemeInvert: + type: boolean + default: false + description: Reverse the order of colors in the color scheme + customCategoryLabels: + type: object + description: Map of category values to color legend labels. + patternProperties: + ".*": + type: string + binningStrategyBinCount: + type: integer + default: 5 + description: The *suggested* number of bins for the automatic binning algorithm + minimum: 0 + customNumericMinValue: + type: number + description: The minimum bracket of the first bin. Inferred from data if not provided. + additionalProperties: false diff --git a/packages/@ourworldindata/types/src/dbTypes/InheritingCharts.ts b/packages/@ourworldindata/types/src/dbTypes/InheritingCharts.ts new file mode 100644 index 00000000000..3a956f68cc7 --- /dev/null +++ b/packages/@ourworldindata/types/src/dbTypes/InheritingCharts.ts @@ -0,0 +1,8 @@ +export const InheritingChartsTableName = "inheriting_charts" + +export interface DbInsertInheritingChart { + variableId: number + chartId: number +} + +export type DbPlainInheritingChart = Required diff --git a/packages/@ourworldindata/types/src/dbTypes/Variables.ts b/packages/@ourworldindata/types/src/dbTypes/Variables.ts index ce241a997c8..3c2d40c5a3f 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Variables.ts +++ b/packages/@ourworldindata/types/src/dbTypes/Variables.ts @@ -1,7 +1,6 @@ import { OwidVariableType } from "../OwidVariable.js" import { OwidVariableDisplayConfigInterface } from "../OwidVariableDisplayConfigInterface.js" import { JsonString } from "../domainTypes/Various.js" -import { GrapherInterface } from "../grapherTypes/GrapherTypes.js" export const VariablesTableName = "variables" export interface DbInsertVariable { @@ -20,8 +19,8 @@ export interface DbInsertVariable { descriptionShort?: string | null dimensions?: JsonString | null display: JsonString - grapherConfigAdmin?: JsonString | null - grapherConfigETL?: JsonString | null + grapherConfigIdAdmin?: Buffer | null + grapherConfigIdETL?: Buffer | null id?: number license?: JsonString | null licenses?: JsonString | null @@ -66,8 +65,6 @@ export type DbEnrichedVariable = Omit< | "dimensions" | "descriptionKey" | "originalMetadata" - | "grapherConfigAdmin" - | "grapherConfigETL" | "processingLog" | "sort" > & { @@ -77,8 +74,6 @@ export type DbEnrichedVariable = Omit< dimensions: VariableDisplayDimension | null descriptionKey: string[] | null originalMetadata: unknown | null - grapherConfigAdmin: GrapherInterface | null - grapherConfigETL: GrapherInterface | null processingLog: unknown | null sort: string[] | null } @@ -149,30 +144,6 @@ export function serializeVariableOriginalMetadata( return originalMetadata ? JSON.stringify(originalMetadata) : null } -export function parseVariableGrapherConfigAdmin( - grapherConfigAdmin: JsonString | null -): GrapherInterface { - return grapherConfigAdmin ? JSON.parse(grapherConfigAdmin) : null -} - -export function serializeVariableGrapherConfigAdmin( - grapherConfigAdmin: GrapherInterface | null -): JsonString | null { - return grapherConfigAdmin ? JSON.stringify(grapherConfigAdmin) : null -} - -export function parseVariableGrapherConfigETL( - grapherConfigETL: JsonString | null -): GrapherInterface { - return grapherConfigETL ? JSON.parse(grapherConfigETL) : null -} - -export function serializeVariableGrapherConfigETL( - grapherConfigETL: GrapherInterface | null -): JsonString | null { - return grapherConfigETL ? JSON.stringify(grapherConfigETL) : null -} - export function parseVariableProcessingLog( processingLog: JsonString | null ): any { @@ -204,10 +175,6 @@ export function parseVariablesRow(row: DbRawVariable): DbEnrichedVariable { dimensions: parseVariableDimensions(row.dimensions), descriptionKey: parseVariableDescriptionKey(row.descriptionKey), originalMetadata: parseVariableOriginalMetadata(row.originalMetadata), - grapherConfigAdmin: parseVariableGrapherConfigAdmin( - row.grapherConfigAdmin - ), - grapherConfigETL: parseVariableGrapherConfigETL(row.grapherConfigETL), processingLog: parseVariableProcessingLog(row.processingLog), sort: parseVariableSort(row.sort), } @@ -224,12 +191,6 @@ export function serializeVariablesRow(row: DbEnrichedVariable): DbRawVariable { originalMetadata: serializeVariableOriginalMetadata( row.originalMetadata ), - grapherConfigAdmin: serializeVariableGrapherConfigAdmin( - row.grapherConfigAdmin - ), - grapherConfigETL: serializeVariableGrapherConfigETL( - row.grapherConfigETL - ), processingLog: serializeVariableProcessingLog(row.processingLog), sort: serializeVariableSort(row.sort), } diff --git a/packages/@ourworldindata/types/src/index.ts b/packages/@ourworldindata/types/src/index.ts index 87b227f14bc..7ea9b84004f 100644 --- a/packages/@ourworldindata/types/src/index.ts +++ b/packages/@ourworldindata/types/src/index.ts @@ -641,10 +641,6 @@ export { serializeVariableOriginalMetadata, parseVariableLicenses, serializeVariableLicenses, - parseVariableGrapherConfigAdmin, - serializeVariableGrapherConfigAdmin, - parseVariableGrapherConfigETL, - serializeVariableGrapherConfigETL, parseVariableProcessingLog, serializeVariableProcessingLog, type License, diff --git a/packages/@ourworldindata/utils/src/Util.ts b/packages/@ourworldindata/utils/src/Util.ts index 93f0aa0289e..2049fd207b5 100644 --- a/packages/@ourworldindata/utils/src/Util.ts +++ b/packages/@ourworldindata/utils/src/Util.ts @@ -174,6 +174,9 @@ import { TagGraphRoot, TagGraphRootName, TagGraphNode, + GrapherInterface, + ChartTypeName, + DimensionProperty, } from "@ourworldindata/types" import { PointVector } from "./PointVector.js" import React from "react" @@ -1775,13 +1778,6 @@ export function filterValidStringValues( return filteredValues } -// TODO(inheritance): remove in favour of mergeGrapherConfigs -export function mergePartialGrapherConfigs>( - ...grapherConfigs: (T | undefined)[] -): T { - return merge({}, ...grapherConfigs) -} - /** Works for: * #dod:text * #dod:text-hyphenated @@ -1969,3 +1965,20 @@ export function traverseObjects>( } return result } + +export function getParentIndicatorIdFromChartConfig( + config: GrapherInterface +): number | undefined { + const { type, dimensions } = config + + if (type === ChartTypeName.ScatterPlot) return undefined + if (!dimensions) return undefined + + const yVariableIds = dimensions + .filter((d) => d.property === DimensionProperty.y) + .map((d) => d.variableId) + + if (yVariableIds.length !== 1) return undefined + + return yVariableIds[0] +} diff --git a/packages/@ourworldindata/utils/src/index.ts b/packages/@ourworldindata/utils/src/index.ts index b68c5c87c28..8f751c4ea29 100644 --- a/packages/@ourworldindata/utils/src/index.ts +++ b/packages/@ourworldindata/utils/src/index.ts @@ -109,7 +109,6 @@ export { type NodeWithUrl, filterValidStringValues, traverseEnrichedSpan, - mergePartialGrapherConfigs, copyToClipboard, checkIsGdocPost, checkIsGdocPostExcludingFragments, @@ -124,6 +123,7 @@ export { createTagGraph, formatInlineList, lazy, + getParentIndicatorIdFromChartConfig, } from "./Util.js" export { diff --git a/site/DataPageV2.tsx b/site/DataPageV2.tsx index 34e4b05f5f6..7133cd00653 100644 --- a/site/DataPageV2.tsx +++ b/site/DataPageV2.tsx @@ -9,7 +9,7 @@ import { SiteFooterContext, DataPageDataV2, serializeJSONForHTML, - mergePartialGrapherConfigs, + mergeGrapherConfigs, compact, FaqEntryData, pick, @@ -88,10 +88,9 @@ export const DataPageV2 = (props: { compact(grapher?.dimensions?.map((d) => d.variableId)) ) - // TODO(inheritance): use mergeGrapherConfigs instead - const mergedGrapherConfig = mergePartialGrapherConfigs( + const mergedGrapherConfig = mergeGrapherConfigs( datapageData.chartConfig as GrapherInterface, - grapher + grapher ?? {} ) // Note that we cannot set `bindUrlToWindow` and `isEmbeddedInADataPage` here, From e417f30d03748fbdd732f8e1ae92d88548cd5b66 Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Fri, 2 Aug 2024 18:46:39 +0200 Subject: [PATCH 02/39] =?UTF-8?q?=F0=9F=94=A8=20adapt=20this=20PR=20to=20s?= =?UTF-8?q?tring=20based=20UUIDs=20v7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/VariableEditPage.tsx | 19 ++++++++++++------- adminSiteServer/apiRouter.ts | 13 +++++++++---- ...veIndicatorChartsToTheChartConfigsTable.ts | 12 ++++-------- db/model/Variable.ts | 3 ++- .../@ourworldindata/types/src/OwidVariable.ts | 2 -- .../types/src/dbTypes/Variables.ts | 4 ++-- .../utils/src/metadataHelpers.ts | 1 - 7 files changed, 29 insertions(+), 25 deletions(-) diff --git a/adminSiteClient/VariableEditPage.tsx b/adminSiteClient/VariableEditPage.tsx index ba23524191a..bf8b886b33d 100644 --- a/adminSiteClient/VariableEditPage.tsx +++ b/adminSiteClient/VariableEditPage.tsx @@ -55,7 +55,8 @@ interface VariablePageData extends Omit { datasetNamespace: string charts: ChartListItem[] - grapherConfig: GrapherInterface | undefined + grapherConfig: GrapherInterface | undefined // admin+ETL merged + grapherConfigETL: GrapherInterface | undefined source: { id: number; name: string } origins: OwidOrigin[] } @@ -121,7 +122,10 @@ class VariableEditable // XXX refactor with DatasetEditPage @observer -class VariableEditor extends React.Component<{ variable: VariablePageData }> { +class VariableEditor extends React.Component<{ + variable: VariablePageData + grapherConfigETL?: GrapherInterface +}> { @observable newVariable!: VariableEditable @observable isDeleted: boolean = false @@ -146,8 +150,12 @@ class VariableEditor extends React.Component<{ variable: VariablePageData }> { ) } + @computed get grapherConfigETL(): GrapherInterface | undefined { + return this.props.grapherConfigETL + } + render() { - const { variable } = this.props + const { variable, grapherConfigETL } = this.props const { newVariable, isV2MetadataVariable } = this const pathFragments = variable.catalogPath @@ -405,10 +413,7 @@ class VariableEditor extends React.Component<{ variable: VariablePageData }> { label="Grapher Config ETL" field="v" store={{ - v: YAML.stringify( - newVariable.presentation - .grapherConfigETL - ), + v: YAML.stringify(grapherConfigETL), }} disabled textarea diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts index 6bcbc29ea1c..28955b78803 100644 --- a/adminSiteServer/apiRouter.ts +++ b/adminSiteServer/apiRouter.ts @@ -1353,22 +1353,27 @@ getRouteWithROTransaction( await assignTagsForCharts(trx, charts) - const grapherConfig = await getMergedGrapherConfigForVariable( + const variableWithConfigs = await getGrapherConfigsForVariable( trx, variableId ) + const grapherConfigETL = variableWithConfigs?.etl?.fullConfig + const mergedGrapherConfig = + variableWithConfigs?.admin?.fullConfig ?? grapherConfigETL - const variablesWithCharts: OwidVariableWithSource & { + const variableWithCharts: OwidVariableWithSource & { charts: Record grapherConfig: GrapherInterface | undefined + grapherConfigETL: GrapherInterface | undefined } = { ...variable, charts, - grapherConfig, + grapherConfig: mergedGrapherConfig, + grapherConfigETL, } return { - variable: variablesWithCharts, + variable: variableWithCharts, } /*, vardata: await getVariableData([variableId]) }*/ } ) diff --git a/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts index a972f51d45a..d05bb0348ef 100644 --- a/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts +++ b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts @@ -2,6 +2,7 @@ import { defaultGrapherConfig } from "@ourworldindata/grapher" import { DimensionProperty, GrapherInterface } from "@ourworldindata/types" import { mergeGrapherConfigs, omit } from "@ourworldindata/utils" import { MigrationInterface, QueryRunner } from "typeorm" +import { uuidv7 } from "uuidv7" export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 implements MigrationInterface @@ -9,8 +10,8 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 public async up(queryRunner: QueryRunner): Promise { await queryRunner.query(`-- sql ALTER TABLE variables - ADD COLUMN grapherConfigIdAdmin binary(16) UNIQUE AFTER sort, - ADD COLUMN grapherConfigIdETL binary(16) UNIQUE AFTER grapherConfigIdAdmin, + ADD COLUMN grapherConfigIdAdmin char(36) UNIQUE AFTER sort, + ADD COLUMN grapherConfigIdETL char(36) UNIQUE AFTER grapherConfigIdAdmin, ADD CONSTRAINT fk_variables_grapherConfigIdAdmin FOREIGN KEY (grapherConfigIdAdmin) REFERENCES chart_configs (id) @@ -68,7 +69,7 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 } // insert config into the chart_configs table - const configId = await getBinaryUUID(queryRunner) + const configId = uuidv7() await queryRunner.query( `-- sql INSERT INTO chart_configs (id, patch, full) @@ -232,8 +233,3 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 } } } - -const getBinaryUUID = async (queryRunner: QueryRunner): Promise => { - const rows = await queryRunner.query(`SELECT UUID_TO_BIN(UUID(), 1) AS id`) - return rows[0].id -} diff --git a/db/model/Variable.ts b/db/model/Variable.ts index 075814e1f31..b9020fe79d7 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -14,6 +14,7 @@ import { DEFAULT_GRAPHER_CONFIG_SCHEMA, } from "@ourworldindata/grapher" import pl from "nodejs-polars" +import { uuidv7 } from "uuidv7" import { DATA_API_URL } from "../../settings/serverSettings.js" import { escape } from "mysql2" import { @@ -135,7 +136,7 @@ export async function insertNewGrapherConfigForVariable( } ): Promise { // insert chart config into the database - const configId = await db.getBinaryUUID(knex) + const configId = uuidv7() await db.knexRaw( knex, `-- sql diff --git a/packages/@ourworldindata/types/src/OwidVariable.ts b/packages/@ourworldindata/types/src/OwidVariable.ts index c0785a2be85..f07899ff4fa 100644 --- a/packages/@ourworldindata/types/src/OwidVariable.ts +++ b/packages/@ourworldindata/types/src/OwidVariable.ts @@ -1,7 +1,6 @@ import { OwidOrigin } from "./OwidOrigin.js" import { OwidSource } from "./OwidSource.js" import { OwidVariableDisplayConfigInterface } from "./OwidVariableDisplayConfigInterface.js" -import { GrapherInterface } from "./grapherTypes/GrapherTypes.js" export interface OwidVariableWithSource { id: number @@ -73,7 +72,6 @@ export interface OwidVariablePresentation { attribution?: string topicTagsLinks?: string[] faqs?: FaqLink[] - grapherConfigETL?: GrapherInterface } export type OwidProcessingLevel = "minor" | "major" diff --git a/packages/@ourworldindata/types/src/dbTypes/Variables.ts b/packages/@ourworldindata/types/src/dbTypes/Variables.ts index 3c2d40c5a3f..1b836ec7e64 100644 --- a/packages/@ourworldindata/types/src/dbTypes/Variables.ts +++ b/packages/@ourworldindata/types/src/dbTypes/Variables.ts @@ -19,8 +19,8 @@ export interface DbInsertVariable { descriptionShort?: string | null dimensions?: JsonString | null display: JsonString - grapherConfigIdAdmin?: Buffer | null - grapherConfigIdETL?: Buffer | null + grapherConfigIdAdmin?: string | null + grapherConfigIdETL?: string | null id?: number license?: JsonString | null licenses?: JsonString | null diff --git a/packages/@ourworldindata/utils/src/metadataHelpers.ts b/packages/@ourworldindata/utils/src/metadataHelpers.ts index 5a81a5ef18d..946c6a43888 100644 --- a/packages/@ourworldindata/utils/src/metadataHelpers.ts +++ b/packages/@ourworldindata/utils/src/metadataHelpers.ts @@ -283,7 +283,6 @@ export function grabMetadataForGdocLinkedIndicator( title: metadata.presentation?.titlePublic || chartConfigTitle || - metadata.presentation?.grapherConfigETL?.title || metadata.display?.name || metadata.name || "", From 52e02bb01cc64ee5690a9eacce406068825dc0b0 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Tue, 6 Aug 2024 18:05:16 +0200 Subject: [PATCH 03/39] =?UTF-8?q?=F0=9F=90=9D=20make=20migration=20faster?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...veIndicatorChartsToTheChartConfigsTable.ts | 166 +++++++++++------- 1 file changed, 104 insertions(+), 62 deletions(-) diff --git a/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts index d05bb0348ef..f71e8561b83 100644 --- a/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts +++ b/db/migration/1721296631522-MoveIndicatorChartsToTheChartConfigsTable.ts @@ -1,12 +1,110 @@ import { defaultGrapherConfig } from "@ourworldindata/grapher" -import { DimensionProperty, GrapherInterface } from "@ourworldindata/types" -import { mergeGrapherConfigs, omit } from "@ourworldindata/utils" +import { mergeGrapherConfigs } from "@ourworldindata/utils" import { MigrationInterface, QueryRunner } from "typeorm" import { uuidv7 } from "uuidv7" export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 implements MigrationInterface { + private async validateGrapherConfigETLs( + queryRunner: QueryRunner + ): Promise { + // we have v3 configs in the database (the current version is v4); + // turn these into v4 configs by removing the `data` property + // which was the breaking change that lead to v4 + // (we don't have v2 or v1 configs in the database, so we don't need to handle those) + await queryRunner.query( + `-- sql + UPDATE variables + SET grapherConfigETL = JSON_SET( + JSON_REMOVE(grapherConfigETL, '$.data'), + '$.$schema', + 'https://files.ourworldindata.org/schemas/grapher-schema.004.json' + ) + WHERE + grapherConfigETL IS NOT NULL + AND grapherConfigETL ->> '$.$schema' = 'https://files.ourworldindata.org/schemas/grapher-schema.003.json' + ` + ) + + // if the config has no schema, assume it's the default version + await queryRunner.query( + `-- sql + UPDATE variables + SET grapherConfigETL = JSON_SET( + grapherConfigETL, + '$.$schema', + 'https://files.ourworldindata.org/schemas/grapher-schema.004.json' + ) + WHERE + grapherConfigETL IS NOT NULL + AND grapherConfigETL ->> '$.$schema' IS NULL + ` + ) + + // fill dimensions to make the config plottable + // (at the time of writing, the dimensions field is empty for all configs) + await queryRunner.query( + `-- sql + UPDATE variables + SET grapherConfigETL = JSON_SET( + grapherConfigETL, + '$.dimensions', + JSON_ARRAY(JSON_OBJECT('variableId', id, 'property', 'y')) + ) + WHERE grapherConfigETL IS NOT NULL + ` + ) + } + + private async moveGrapherConfigETLsToChartConfigsTable( + queryRunner: QueryRunner + ): Promise { + // ~ 68000 entries at the time of writing + const variables: { id: number }[] = await queryRunner.query(`-- sql + SELECT id + FROM variables + WHERE grapherConfigETL IS NOT NULL + `) + + // generate UUIDs for every config + const ids = variables.map((v) => ({ + variableId: v.id, + uuid: uuidv7(), + })) + + // insert a new row for each config with dummy values for the config fields + await queryRunner.query( + `-- sql + INSERT INTO chart_configs (id, patch, full) + VALUES ? + `, + [ids.map(({ uuid }) => [uuid, "{}", "{}"])] + ) + + // add a reference to the chart_configs uuid in the variables table + const variablesUpdateValues = ids + .map( + ({ variableId, uuid }) => + `(${variableId},'${uuid}',unit,coverage,timespan,datasetId,display)` + ) + .join(",") + await queryRunner.query(`-- sql + INSERT INTO variables (id, grapherConfigIdETL, unit, coverage, timespan, datasetId, display) + VALUES ${variablesUpdateValues} + ON DUPLICATE KEY UPDATE grapherConfigIdETL=VALUES(grapherConfigIdETL) + `) + + // copy configs from the variables table to the chart_configs table + await queryRunner.query(`-- sql + UPDATE chart_configs + JOIN variables ON variables.grapherConfigIdETL = chart_configs.id + SET + patch = variables.grapherConfigETL, + full = variables.grapherConfigETL + `) + } + public async up(queryRunner: QueryRunner): Promise { await queryRunner.query(`-- sql ALTER TABLE variables @@ -27,67 +125,11 @@ export class MoveIndicatorChartsToTheChartConfigsTable1721296631522 // note that we copy the ETL-authored configs to the chart_configs table, // but drop the admin-authored configs - const variables = await queryRunner.query(`-- sql - SELECT id, grapherConfigETL - FROM variables - WHERE grapherConfigETL IS NOT NULL - `) - - for (const { id: variableId, grapherConfigETL } of variables) { - let config: GrapherInterface = JSON.parse(grapherConfigETL) - - // if the config has no schema, assume it's the default version - if (!config.$schema) { - config.$schema = defaultGrapherConfig.$schema - } - - // check if the given dimensions are correct - if (config.dimensions && config.dimensions.length >= 1) { - // make sure there is only a single entry - config.dimensions = config.dimensions.slice(0, 1) - // make sure the variable id matches - config.dimensions[0].variableId = variableId - } - - // fill dimensions if not given to make the config plottable - if (!config.dimensions || config.dimensions.length === 0) { - config.dimensions = [ - { property: DimensionProperty.y, variableId }, - ] - } - - // we have v3 configs in the database (the current version is v4); - // turn these into v4 configs by removing the `data` property - // which was the breaking change that lead to v4 - // (we don't have v2 or v1 configs in the database, so we don't need to handle those) - if ( - config.$schema === - "https://files.ourworldindata.org/schemas/grapher-schema.003.json" - ) { - config = omit(config, "data") - config.$schema = defaultGrapherConfig.$schema - } - - // insert config into the chart_configs table - const configId = uuidv7() - await queryRunner.query( - `-- sql - INSERT INTO chart_configs (id, patch, full) - VALUES (?, ?, ?) - `, - [configId, JSON.stringify(config), JSON.stringify(config)] - ) + // first, make sure all given grapherConfigETLs are valid + await this.validateGrapherConfigETLs(queryRunner) - // update reference in the variables table - await queryRunner.query( - `-- sql - UPDATE variables - SET grapherConfigIdETL = ? - WHERE id = ? - `, - [configId, variableId] - ) - } + // then, move all grapherConfigETLs to the chart_configs table + await this.moveGrapherConfigETLsToChartConfigsTable(queryRunner) // drop `grapherConfigAdmin` and `grapherConfigETL` columns await queryRunner.query(`-- sql From 0e7e4097dcd01102d64cf5dde8e7585d5c49ed15 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Wed, 7 Aug 2024 11:07:23 +0000 Subject: [PATCH 04/39] =?UTF-8?q?=F0=9F=94=A8=20(grapher)=20remove=20DEFAU?= =?UTF-8?q?LT=5FGRAPHER=5FCONFIG=5FSCHEMA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- db/model/Variable.ts | 3 +-- packages/@ourworldindata/grapher/src/index.ts | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/db/model/Variable.ts b/db/model/Variable.ts index b9020fe79d7..22b54e6bf89 100644 --- a/db/model/Variable.ts +++ b/db/model/Variable.ts @@ -11,7 +11,6 @@ import { getVariableDataRoute, getVariableMetadataRoute, defaultGrapherConfig, - DEFAULT_GRAPHER_CONFIG_SCHEMA, } from "@ourworldindata/grapher" import pl from "nodejs-polars" import { uuidv7 } from "uuidv7" @@ -171,7 +170,7 @@ function makeConfigValidForIndicator({ // if no schema is given, assume it's the latest if (!updatedConfig.$schema) { - updatedConfig.$schema = DEFAULT_GRAPHER_CONFIG_SCHEMA + updatedConfig.$schema = defaultGrapherConfig.$schema } // check if the given dimensions are correct diff --git a/packages/@ourworldindata/grapher/src/index.ts b/packages/@ourworldindata/grapher/src/index.ts index ed27bd627f5..9e9d08b9902 100644 --- a/packages/@ourworldindata/grapher/src/index.ts +++ b/packages/@ourworldindata/grapher/src/index.ts @@ -26,7 +26,6 @@ export { grapherInterfaceWithHiddenTabsOnly, CONTINENTS_INDICATOR_ID, POPULATION_INDICATOR_ID_USED_IN_ADMIN, - DEFAULT_GRAPHER_CONFIG_SCHEMA, } from "./core/GrapherConstants" export { getVariableDataRoute, From 939209f6d03d33287c573bbfecb7d09b9894ed14 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Thu, 8 Aug 2024 10:14:30 +0200 Subject: [PATCH 05/39] =?UTF-8?q?=F0=9F=92=84=20add=20a=20comment=20for=20?= =?UTF-8?q?const=20value=20of=20$schema?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/schema/grapher-schema.004.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml index afcf1b398ae..0a8a90746f9 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml @@ -13,6 +13,11 @@ properties: description: Url of the concrete schema version to use to validate this document format: uri default: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" + # for now, we only use the schema to validate configs in our database. + # since we expect all configs in our database to be valid against the latest schema, + # we restrict the $schema field to a single value, the latest schema version. + # if we ever need to validate configs against multiple schema versions, + # we can remove this const constraint. const: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" id: type: integer From d0fb7f6757c5983c7e2291f25748a46cf0a4b593 Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Thu, 8 Aug 2024 10:18:28 +0200 Subject: [PATCH 06/39] =?UTF-8?q?=F0=9F=92=84=20add=20a=20comment=20for=20?= =?UTF-8?q?required=20keys=20of=20the=20schema?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../@ourworldindata/grapher/src/schema/grapher-schema.004.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml index 0a8a90746f9..18069b46293 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.004.yaml @@ -1,5 +1,7 @@ $schema: "http://json-schema.org/draft-07/schema#" $id: "https://files.ourworldindata.org/schemas/grapher-schema.004.json" +# if you update the required keys, make sure that the mergeGrapherConfigs and +# diffGrapherConfigs functions reflects the change required: - $schema - dimensions From 493e728096b15b3c6309b37b4f12e0141bce1a5c Mon Sep 17 00:00:00 2001 From: sophiamersmann Date: Thu, 8 Aug 2024 10:29:15 +0000 Subject: [PATCH 07/39] =?UTF-8?q?=F0=9F=90=9D=20wip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/AbstractChartEditor.ts | 41 ++++-- adminSiteClient/ChartEditor.ts | 54 ++++---- adminSiteClient/ChartEditorPage.tsx | 7 +- adminSiteClient/EditorBasicTab.tsx | 8 +- adminSiteClient/EditorDataTab.tsx | 27 ++-- adminSiteClient/EditorHistoryTab.tsx | 47 ++++--- adminSiteServer/apiRouter.ts | 19 ++- db/model/Chart.ts | 21 +--- .../grapher/src/core/Grapher.jsdom.test.ts | 5 + .../grapher/src/core/Grapher.tsx | 119 ++++++++++++------ .../types/src/grapherTypes/GrapherTypes.ts | 10 +- .../@ourworldindata/utils/src/TimeBounds.ts | 8 +- packages/@ourworldindata/utils/src/index.ts | 1 + .../utils/src/persistable/Persistable.ts | 17 +++ 14 files changed, 262 insertions(+), 122 deletions(-) diff --git a/adminSiteClient/AbstractChartEditor.ts b/adminSiteClient/AbstractChartEditor.ts index c425e58797f..306e1c1147b 100644 --- a/adminSiteClient/AbstractChartEditor.ts +++ b/adminSiteClient/AbstractChartEditor.ts @@ -4,6 +4,7 @@ import { GrapherInterface, diffGrapherConfigs, mergeGrapherConfigs, + merge, } from "@ourworldindata/utils" import { computed, observable, when } from "mobx" import { EditorFeatures } from "./EditorFeatures.js" @@ -60,29 +61,34 @@ export abstract class AbstractChartEditor< ) } + /** default object with all possible keys */ + private fullDefaultObject = merge( + {}, + Grapher.defaultObject(), // contains all keys + defaultGrapherConfig // contains a subset of keys + ) + @computed get originalGrapherConfig(): GrapherInterface { const { patchConfig, parentConfig } = this.manager if (!parentConfig) return patchConfig return mergeGrapherConfigs(parentConfig, patchConfig) } - @computed get liveConfig(): GrapherInterface { - return this.grapher.object + @computed get fullConfig(): GrapherInterface { + return mergeGrapherConfigs(this.fullDefaultObject, this.grapher.object) } @computed get patchConfig(): GrapherInterface { - const { liveConfig, parentConfig } = this - if (!parentConfig) return liveConfig - // Grapher's toObject method doesn't include default values, - // but the patch config might need to overwrite non-default values - // from the parent layer. That's why we merge the default grapher config - // in here. The parent config also contains defaults, meaning we're - // getting rid of the defaults when we diff against the parent config below. - const liveConfigWithDefaults = mergeGrapherConfigs( - defaultGrapherConfig, - liveConfig + const { fullConfig, parentConfigWithDefaults, fullDefaultObject } = this + return diffGrapherConfigs( + fullConfig, + parentConfigWithDefaults ?? fullDefaultObject ) - return diffGrapherConfigs(liveConfigWithDefaults, parentConfig) + } + + @computed get parentConfigWithDefaults(): GrapherInterface | undefined { + if (!this.parentConfig) return undefined + return mergeGrapherConfigs(this.fullDefaultObject, this.parentConfig) } @computed get isModified(): boolean { @@ -96,6 +102,15 @@ export abstract class AbstractChartEditor< return new EditorFeatures(this) } + // TODO: only works for first level at the moment + isPropertyInherited(property: keyof GrapherInterface): boolean { + if (!this.parentConfig) return false + return ( + !Object.hasOwn(this.patchConfig, property) && + Object.hasOwn(this.parentConfig, property) + ) + } + abstract get isNewGrapher(): boolean abstract get availableTabs(): EditorTab[] diff --git a/adminSiteClient/ChartEditor.ts b/adminSiteClient/ChartEditor.ts index e47214e5bc0..ec889e0d3ca 100644 --- a/adminSiteClient/ChartEditor.ts +++ b/adminSiteClient/ChartEditor.ts @@ -12,19 +12,18 @@ import { ChartRedirect, Json, GrapherInterface, - diffGrapherConfigs, getParentIndicatorIdFromChartConfig, mergeGrapherConfigs, } from "@ourworldindata/utils" import { action, computed, observable, runInAction } from "mobx" import { BAKED_GRAPHER_URL } from "../settings/clientSettings.js" -import { defaultGrapherConfig } from "@ourworldindata/grapher" import { AbstractChartEditor, AbstractChartEditorManager, EditorTab, } from "./AbstractChartEditor.js" import { Admin } from "./Admin.js" +import { isEmpty } from "../gridLang/GrammarUtils.js" export interface Log { userId: number @@ -96,31 +95,34 @@ export class ChartEditor extends AbstractChartEditor { } @action.bound async updateParentConfig() { + const currentIndicatorId = this.parentConfig?.dimensions?.[0].variableId const newIndicatorId = getParentIndicatorIdFromChartConfig( - this.liveConfig + this.fullConfig ) - const currentIndicatorId = this.parentConfig?.dimensions?.[0].variableId - if (newIndicatorId !== currentIndicatorId) { - this.parentConfig = newIndicatorId - ? await fetchParentConfigForChart( - this.manager.admin, - newIndicatorId - ) - : undefined + + // fetch the new parent config if the indicator has changed + let newParentConfig: GrapherInterface | undefined + if ( + newIndicatorId && + (currentIndicatorId === undefined || + newIndicatorId !== currentIndicatorId) + ) { + newParentConfig = await fetchParentConfigForChart( + this.manager.admin, + newIndicatorId + ) } - // it's intentional that we don't use this.patchConfig here - const patchConfig = diffGrapherConfigs( - this.liveConfig, - this.parentConfig ?? {} + const newConfig = mergeGrapherConfigs( + newParentConfig ?? {}, + this.patchConfig ) - const config = mergeGrapherConfigs(this.parentConfig ?? {}, patchConfig) - this.grapher.updateFromObject(config) - // TODO(inheritance): what does this do? is this necessary? - this.grapher.updateAuthoredVersion({ - ...this.grapher.toObject(), - }) + this.grapher.reset() + this.grapher.updateFromObject(newConfig) + this.grapher.updateAuthoredVersion(newConfig) + + this.parentConfig = newParentConfig } async saveGrapher({ @@ -207,11 +209,17 @@ export class ChartEditor extends AbstractChartEditor { export async function fetchParentConfigForChart( admin: Admin, indicatorId: number -): Promise { +): Promise { const indicatorChart = await admin.getJSON( `/api/variables/mergedGrapherConfig/${indicatorId}.json` ) - return mergeGrapherConfigs(defaultGrapherConfig, indicatorChart) + // TODO: why doesn't isEmpty work here? + console.log( + indicatorChart, + isEmpty(indicatorChart), + Object.keys(indicatorChart).length === 0 + ) + return Object.keys(indicatorChart).length === 0 ? undefined : indicatorChart } export function isChartEditorInstance( diff --git a/adminSiteClient/ChartEditorPage.tsx b/adminSiteClient/ChartEditorPage.tsx index 3b7cc6d1279..09bf71949fc 100644 --- a/adminSiteClient/ChartEditorPage.tsx +++ b/adminSiteClient/ChartEditorPage.tsx @@ -16,6 +16,7 @@ import { } from "./ChartEditor.js" import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js" import { ChartEditorView, ChartEditorViewManager } from "./ChartEditorView.js" +import { isEmpty } from "../gridLang/GrammarUtils.js" @observer export class ChartEditorPage @@ -46,9 +47,13 @@ export class ChartEditorPage async fetchParentConfig(): Promise { const { grapherId, grapherConfig } = this.props if (grapherId !== undefined) { - this.parentConfig = await this.context.admin.getJSON( + const parentConfig = await this.context.admin.getJSON( `/api/charts/${grapherId}.parentConfig.json` ) + this.parentConfig = + Object.keys(parentConfig).length === 0 + ? undefined + : parentConfig } else if (grapherConfig) { const parentIndicatorId = getParentIndicatorIdFromChartConfig(grapherConfig) diff --git a/adminSiteClient/EditorBasicTab.tsx b/adminSiteClient/EditorBasicTab.tsx index 639609a9bab..388dfed5938 100644 --- a/adminSiteClient/EditorBasicTab.tsx +++ b/adminSiteClient/EditorBasicTab.tsx @@ -82,7 +82,12 @@ class DimensionSlotView< this.isSelectingVariables = false - this.updateDimensionsAndRebuildTable(dimensionConfigs) + // TODO: why does this sometimes fail? + try { + this.updateDimensionsAndRebuildTable(dimensionConfigs) + } catch { + console.log("updateDimensionsAndRebuildTable failed") + } this.updateParentConfig() } @@ -138,6 +143,7 @@ class DimensionSlotView< } componentDidMount() { + // TODO: re-enable // We want to add the reaction only after the grapher is loaded, so we don't update the initial chart (as configured) by accident. when( () => this.grapher.isReady, diff --git a/adminSiteClient/EditorDataTab.tsx b/adminSiteClient/EditorDataTab.tsx index af28179a3b3..e32c3e2abb3 100644 --- a/adminSiteClient/EditorDataTab.tsx +++ b/adminSiteClient/EditorDataTab.tsx @@ -84,15 +84,21 @@ class EntityItem extends React.Component { } @observer -export class KeysSection extends React.Component<{ grapher: Grapher }> { +export class KeysSection extends React.Component<{ + editor: AbstractChartEditor +}> { @observable.ref dragKey?: EntityName + @computed get grapher() { + return this.props.editor.grapher + } + @action.bound onAddKey(entityName: EntityName) { - this.props.grapher.selection.selectEntity(entityName) + this.grapher.selection.selectEntity(entityName) } @action.bound onDragEnd(result: DropResult) { - const { selection } = this.props.grapher + const { selection } = this.grapher const { source, destination } = result if (!destination) return @@ -105,12 +111,16 @@ export class KeysSection extends React.Component<{ grapher: Grapher }> { } render() { - const { grapher } = this.props + const { grapher } = this const { selection } = grapher const { unselectedEntityNames, selectedEntityNames } = selection return (
+ from parent config:{" "} + {this.props.editor + .isPropertyInherited("selectedEntityNames") + .toString()} { key={entityName} grapher={grapher} entityName={entityName} - onRemove={() => + onRemove={() => { selection.deselectEntity( entityName ) - } + console.log( + selection.numSelectedEntities + ) + }} /> )} @@ -278,7 +291,7 @@ export class EditorDataTab<
- + {features.canSpecifyMissingDataStrategy && ( )} diff --git a/adminSiteClient/EditorHistoryTab.tsx b/adminSiteClient/EditorHistoryTab.tsx index 0fa832b4acf..5ac0efe7a8f 100644 --- a/adminSiteClient/EditorHistoryTab.tsx +++ b/adminSiteClient/EditorHistoryTab.tsx @@ -3,10 +3,15 @@ import { observer } from "mobx-react" import { ChartEditor, Log } from "./ChartEditor.js" import { Section, Timeago } from "./Forms.js" import { computed, action, observable } from "mobx" -import { Json, copyToClipboard } from "@ourworldindata/utils" +import { + Json, + copyToClipboard, + diffGrapherConfigs, +} from "@ourworldindata/utils" import YAML from "yaml" import { notification, Modal } from "antd" import ReactDiffViewer, { DiffMethod } from "react-diff-viewer-continued" +import { defaultGrapherConfig } from "@ourworldindata/grapher" function LogCompareModal({ log, @@ -128,14 +133,14 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { @action.bound copyYamlToClipboard() { // Use the Clipboard API to copy the config into the users clipboard - const chartConfigObject = { - ...this.props.editor.grapher.object, + const patchConfig = { + ...this.props.editor.patchConfig, } - delete chartConfigObject.id - delete chartConfigObject.dimensions - delete chartConfigObject.version - delete chartConfigObject.isPublished - const chartConfigAsYaml = YAML.stringify(chartConfigObject) + delete patchConfig.id + delete patchConfig.dimensions + delete patchConfig.version + delete patchConfig.isPublished + const chartConfigAsYaml = YAML.stringify(patchConfig) void copyToClipboard(chartConfigAsYaml) notification["success"]({ message: "Copied YAML to clipboard", @@ -146,11 +151,7 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { } render() { - // Avoid modifying the original JSON object - // Due to mobx memoizing computed values, the JSON can be mutated. - const chartConfigObject = { - ...this.props.editor.grapher.object, - } + const { patchConfig, fullConfig, parentConfig } = this.props.editor return (
{this.logs.map((log, i) => ( @@ -173,9 +174,27 @@ export class EditorHistoryTab extends React.Component<{ editor: ChartEditor }> { rows={7} readOnly className="form-control" - value={JSON.stringify(chartConfigObject, undefined, 2)} + value={JSON.stringify(patchConfig, undefined, 2)} + /> + +
+