diff --git a/adminSiteClient/EditorTextTab.tsx b/adminSiteClient/EditorTextTab.tsx index 9736d891ff0..1bf40594ec7 100644 --- a/adminSiteClient/EditorTextTab.tsx +++ b/adminSiteClient/EditorTextTab.tsx @@ -1,6 +1,6 @@ -import { faMinus, faPlus } from "@fortawesome/free-solid-svg-icons" +import { faMinus } from "@fortawesome/free-solid-svg-icons" import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" -import { LogoOption, RelatedQuestionsConfig } from "@ourworldindata/types" +import { LogoOption } from "@ourworldindata/types" import { getErrorMessageRelatedQuestionUrl } from "@ourworldindata/grapher" import { slugify } from "@ourworldindata/utils" import { action, computed } from "mobx" @@ -37,21 +37,6 @@ export class EditorTextTab< } } - @action.bound onAddRelatedQuestion() { - const { grapher } = this.props.editor - if (!grapher.relatedQuestions) grapher.relatedQuestions = [] - grapher.relatedQuestions.push({ - text: "", - url: "", - }) - } - - @action.bound onRemoveRelatedQuestion(idx: number) { - const { grapher } = this.props.editor - if (!grapher.relatedQuestions) grapher.relatedQuestions = [] - grapher.relatedQuestions.splice(idx, 1) - } - @action.bound onToggleTitleAnnotationEntity(value: boolean) { const { grapher } = this.props.editor grapher.hideAnnotationFieldsInTitle ??= {} @@ -86,7 +71,6 @@ export class EditorTextTab< render() { const { editor } = this.props const { grapher, features } = editor - const { relatedQuestions = [] } = grapher return (
@@ -250,50 +234,49 @@ export class EditorTextTab<
- {relatedQuestions.map( - (question: RelatedQuestionsConfig, idx: number) => ( -
- { - question.text = value - })} - placeholder="e.g. How did countries respond to the pandemic?" - helpText="Short question promoting exploration of related content" - softCharacterLimit={50} - /> - {question.text && ( - { - question.url = value - })} - placeholder="e.g. https://ourworldindata.org/coronavirus" - helpText="Page or section of a page where the answer to the previous question can be found." - errorMessage={getErrorMessageRelatedQuestionUrl( - question - )} - /> - )} - -
- ) - )} - {!relatedQuestions.length && ( - - )} + } + })} + placeholder="e.g. How did countries respond to the pandemic?" + helpText="Short question promoting exploration of related content" + softCharacterLimit={50} + /> + {grapher.relatedQuestion?.text && ( + { + grapher.relatedQuestion!.url = value + })} + placeholder="e.g. https://ourworldindata.org/coronavirus" + helpText="Page or section of a page where the answer to the previous question can be found." + errorMessage={getErrorMessageRelatedQuestionUrl( + grapher.relatedQuestion + )} + /> + )} + {grapher.relatedQuestion?.text && ( + + )} +
| undefined = undefined let serverKnexInstance: Knex | undefined = undefined @@ -192,8 +195,7 @@ async function makeRequestAgainstAdminApi( describe("OwidAdminApp", () => { const testChartConfig = { - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, slug: "test-chart", title: "Test chart", type: "LineChart", @@ -311,16 +313,14 @@ describe("OwidAdminApp: indicator-level chart configs", () => { display: '{ "unit": "kg", "shortUnit": "kg" }', } const testVariableConfigETL = { - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, hasMapTab: true, note: "Indicator note", selectedEntityNames: ["France", "Italy", "Spain"], hideRelativeToggle: false, } const testVariableConfigAdmin = { - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, title: "Admin title", subtitle: "Admin subtitle", } @@ -331,14 +331,12 @@ describe("OwidAdminApp: indicator-level chart configs", () => { id: otherVariableId, } const otherTestVariableConfig = { - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, note: "Other indicator note", } const testChartConfig = { - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, slug: "test-chart", title: "Test chart", type: "Marimekko", @@ -352,8 +350,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => { ], } const testMultiDimConfig = { - grapherConfigSchema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + grapherConfigSchema: currentSchema, title: { title: "Energy use", titleVariant: "by energy source", @@ -612,8 +609,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => { ) expect(fullConfig).toEqual({ - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, id: chartId, isPublished: false, version: 1, @@ -633,8 +629,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => { `/charts/${chartId}.patchConfig.json` ) expect(patchConfig).toEqual({ - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, id: chartId, version: 1, isPublished: false, @@ -670,8 +665,7 @@ describe("OwidAdminApp: indicator-level chart configs", () => { `/charts/${chartId}.config.json` ) expect(fullConfigAfterDelete).toEqual({ - $schema: - "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: currentSchema, id: chartId, version: 1, isPublished: false, diff --git a/db/migration/1730722779919-TurnRelatedQuestionIntoObject.ts b/db/migration/1730722779919-TurnRelatedQuestionIntoObject.ts new file mode 100644 index 00000000000..c2d83fdd557 --- /dev/null +++ b/db/migration/1730722779919-TurnRelatedQuestionIntoObject.ts @@ -0,0 +1,91 @@ +import { MigrationInterface, QueryRunner } from "typeorm" + +export class TurnRelatedQuestionIntoObject1730722779919 + implements MigrationInterface +{ + private async updateSchema( + queryRunner: QueryRunner, + newSchema: string + ): Promise { + await queryRunner.query( + ` + -- sql + UPDATE chart_configs cc + SET cc.patch = JSON_SET(cc.patch, '$.$schema', ?), + cc.full = JSON_SET(cc.full, '$.$schema', ?) + `, + [newSchema, newSchema] + ) + } + + private async turnRelatedQuestionIntoObject( + queryRunner: QueryRunner, + config: "patch" | "full" + ): Promise { + await queryRunner.query( + ` + -- sql + UPDATE chart_configs + SET + ?? = JSON_REMOVE( + JSON_SET( + ??, + '$.relatedQuestion', + ??->'$.relatedQuestions[0]' + ), + '$.relatedQuestions' + ) + WHERE ?? ->> '$.relatedQuestions' is not null + `, + [config, config, config, config] + ) + } + + private async turnRelatedQuestionIntoArray( + queryRunner: QueryRunner, + config: "patch" | "full" + ): Promise { + await queryRunner.query( + ` + -- sql + UPDATE chart_configs + SET + ?? = JSON_REMOVE( + JSON_SET( + ??, + '$.relatedQuestions', + JSON_ARRAY( + JSON_OBJECT( + 'url', ?? ->> '$.relatedQuestion.url', + 'text', ?? ->> '$.relatedQuestion.text' + ) + ) + ), + '$.relatedQuestion' + ) + WHERE ?? ->> '$.relatedQuestion' IS NOT NULL + `, + [config, config, config, config, config] + ) + } + + public async up(queryRunner: QueryRunner): Promise { + await this.turnRelatedQuestionIntoObject(queryRunner, "patch") + await this.turnRelatedQuestionIntoObject(queryRunner, "full") + + await this.updateSchema( + queryRunner, + "https://files.ourworldindata.org/schemas/grapher-schema.006.json" + ) + } + + public async down(queryRunner: QueryRunner): Promise { + await this.turnRelatedQuestionIntoArray(queryRunner, "patch") + await this.turnRelatedQuestionIntoArray(queryRunner, "full") + + await this.updateSchema( + queryRunner, + "https://files.ourworldindata.org/schemas/grapher-schema.005.json" + ) + } +} diff --git a/devTools/schemaProcessor/columns.json b/devTools/schemaProcessor/columns.json index 75a5b3579a4..223f8944f22 100644 --- a/devTools/schemaProcessor/columns.json +++ b/devTools/schemaProcessor/columns.json @@ -617,12 +617,12 @@ }, { "type": "string", - "pointer": "/relatedQuestions/0/url", + "pointer": "/relatedQuestion/url", "editor": "textfield" }, { "type": "string", - "pointer": "/relatedQuestions/0/text", + "pointer": "/relatedQuestion/text", "editor": "textfield" }, { diff --git a/explorer/ExplorerProgram.ts b/explorer/ExplorerProgram.ts index ee906f5b26c..bf0b4f1e1cf 100644 --- a/explorer/ExplorerProgram.ts +++ b/explorer/ExplorerProgram.ts @@ -400,15 +400,6 @@ export class ExplorerProgram extends GridProgram { // assume config is valid against the latest schema mergedConfig.$schema = defaultGrapherConfig.$schema - // TODO: can be removed once relatedQuestions is refactored - const { relatedQuestionUrl, relatedQuestionText } = - this.explorerGrapherConfig - if (relatedQuestionUrl && relatedQuestionText) { - mergedConfig.relatedQuestions = [ - { url: relatedQuestionUrl, text: relatedQuestionText }, - ] - } - return mergedConfig } diff --git a/explorer/GrapherGrammar.ts b/explorer/GrapherGrammar.ts index 1daefb2ce37..a0055c14605 100644 --- a/explorer/GrapherGrammar.ts +++ b/explorer/GrapherGrammar.ts @@ -284,13 +284,13 @@ export const GrapherGrammar: Grammar = { keyword: "relatedQuestionText", description: "The text used for the related question (at the very bottom of the chart)", - toGrapherObject: () => ({}), // handled in code (can be done properly once the relatedQuestion field is refactored) + toGrapherObject: (value) => ({ relatedQuestion: { text: value } }), }, relatedQuestionUrl: { ...UrlCellDef, keyword: "relatedQuestionUrl", description: "The link of the related question text", - toGrapherObject: () => ({}), // handled in code (can be done properly once the relatedQuestion field is refactored) + toGrapherObject: (value) => ({ relatedQuestion: { url: value } }), }, mapTargetTime: { ...IntegerCellDef, diff --git a/packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx b/packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx index 9aaffebffd6..e45ffc41ea9 100644 --- a/packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx +++ b/packages/@ourworldindata/grapher/src/captionedChart/CaptionedChart.tsx @@ -94,7 +94,7 @@ export interface CaptionedChartManager detailRenderers: MarkdownTextWrap[] // related question - relatedQuestions?: RelatedQuestionsConfig[] + relatedQuestion?: RelatedQuestionsConfig showRelatedQuestion?: boolean } @@ -271,7 +271,7 @@ export class CaptionedChart extends React.Component { } private renderRelatedQuestion(): React.ReactElement { - const { relatedQuestions } = this.manager + const { relatedQuestion } = this.manager return ( diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index f285401a030..fa251d50767 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -400,6 +400,7 @@ export class Grapher @observable.ref hideTotalValueLabel?: boolean = undefined @observable.ref missingDataStrategy?: MissingDataStrategy = undefined @observable.ref showSelectionOnlyInDataTable?: boolean = undefined + @observable relatedQuestion?: RelatedQuestionsConfig = undefined // todo: Persistables? @observable.ref xAxis = new AxisConfig(undefined, this) @observable.ref yAxis = new AxisConfig(undefined, this) @@ -426,7 +427,6 @@ export class Grapher */ @observable includedEntities?: number[] = undefined @observable comparisonLines?: ComparisonLineConfig[] = undefined // todo: Persistables? - @observable relatedQuestions?: RelatedQuestionsConfig[] = undefined // todo: Persistables? @observable.ref annotation?: Annotation = undefined @@ -1249,10 +1249,8 @@ export class Grapher // todo: did this name get botched in a merge? @computed get hasFatalErrors(): boolean { - const { relatedQuestions = [] } = this - return relatedQuestions.some( - (question) => !!getErrorMessageRelatedQuestionUrl(question) - ) + if (!this.relatedQuestion) return false + return !!getErrorMessageRelatedQuestionUrl(this.relatedQuestion) } disposers: (() => void)[] = [] @@ -3055,14 +3053,8 @@ export class Grapher @observable isShareMenuActive = false @computed get hasRelatedQuestion(): boolean { - if ( - this.hideRelatedQuestion || - !this.relatedQuestions || - !this.relatedQuestions.length - ) - return false - const question = this.relatedQuestions[0] - return !!question && !!question.text && !!question.url + if (this.hideRelatedQuestion) return false + return !!(this.relatedQuestion?.url && this.relatedQuestion?.text) } @computed get isRelatedQuestionTargetDifferentFromCurrentPage(): boolean { @@ -3073,11 +3065,10 @@ export class Grapher // "ourworldindata.org" and yet should still yield a match. // - Note that this won't work on production previews (where the // path is /admin/posts/preview/ID) - const { hasRelatedQuestion, relatedQuestions = [] } = this - const relatedQuestion = relatedQuestions[0] - return ( + const { hasRelatedQuestion, relatedQuestion } = this + return !!( hasRelatedQuestion && - !!relatedQuestion && + relatedQuestion?.url && getWindowUrl().pathname !== Url.fromURL(relatedQuestion.url).pathname ) @@ -3085,7 +3076,7 @@ export class Grapher @computed get showRelatedQuestion(): boolean { return ( - !!this.relatedQuestions && + !!this.relatedQuestion && !!this.hasRelatedQuestion && !!this.isRelatedQuestionTargetDifferentFromCurrentPage ) @@ -3477,10 +3468,9 @@ const defaultObject = objectWithPersistablesToObject( export const getErrorMessageRelatedQuestionUrl = ( question: RelatedQuestionsConfig ): string | undefined => { - return question.text - ? (!question.url && "Missing URL") || - (!question.url.match(/^https?:\/\//) && - "URL should start with http(s)://") || - undefined - : undefined + if (!question.text) return undefined + if (!question.url) return "Missing URL" + if (!question.url.match(/^https?:\/\//)) + return "URL should start with http(s)://" + return undefined } diff --git a/packages/@ourworldindata/grapher/src/schema/README.md b/packages/@ourworldindata/grapher/src/schema/README.md index dfee8e758e2..71570e37069 100644 --- a/packages/@ourworldindata/grapher/src/schema/README.md +++ b/packages/@ourworldindata/grapher/src/schema/README.md @@ -18,7 +18,6 @@ Checklist for breaking changes: - Rename the authorative url inside the schema file to point to the new version number json - Write the migrations from the last to the current version of the schema, including the migration of pointing to the URL of the new schema version - Regenerate the default object from the schema and save it to `defaultGrapherConfig.ts` (see below) -- Write a migration to update the `chart_configs.full` column in the database for all stand-alone charts - Write a migration to update configs in code (see `migrations/migrations.ts`) - Update the hardcoded default schema version in ETL diff --git a/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts b/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts index dd0f68334e2..1243ba4e985 100644 --- a/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts +++ b/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts @@ -4,11 +4,17 @@ import { GrapherInterface } from "@ourworldindata/types" -export const latestSchemaVersion = "005" as const -export const outdatedSchemaVersions = ["001", "002", "003", "004"] as const +export const latestSchemaVersion = "006" as const +export const outdatedSchemaVersions = [ + "001", + "002", + "003", + "004", + "005", +] as const export const defaultGrapherConfig = { - $schema: "https://files.ourworldindata.org/schemas/grapher-schema.005.json", + $schema: "https://files.ourworldindata.org/schemas/grapher-schema.006.json", map: { projection: "World", hideTimeline: false, diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.005.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml similarity index 98% rename from packages/@ourworldindata/grapher/src/schema/grapher-schema.005.yaml rename to packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml index 4465a68f6c0..f906da32f22 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.005.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml @@ -1,7 +1,7 @@ $schema: "http://json-schema.org/draft-07/schema#" # if you update the required keys, make sure that the mergeGrapherConfigs and # diffGrapherConfigs functions both reflect this change -$id: "https://files.ourworldindata.org/schemas/grapher-schema.005.json" +$id: "https://files.ourworldindata.org/schemas/grapher-schema.006.json" required: - $schema - dimensions @@ -14,13 +14,13 @@ properties: type: string description: Url of the concrete schema version to use to validate this document format: uri - default: "https://files.ourworldindata.org/schemas/grapher-schema.005.json" + default: "https://files.ourworldindata.org/schemas/grapher-schema.006.json" # for now, we only validate configs in our database using this schema. # 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 constraint. - const: "https://files.ourworldindata.org/schemas/grapher-schema.005.json" + const: "https://files.ourworldindata.org/schemas/grapher-schema.006.json" id: type: integer description: Internal DB id. Useful internally for OWID but not required if just using grapher directly. @@ -354,17 +354,18 @@ properties: patternProperties: ".*": type: string - relatedQuestions: - type: array - description: Links to related questions - items: - type: object - properties: - url: - type: string - text: - type: string - additionalProperties: false + relatedQuestion: + type: object + description: Links to a related question. Displayed in the footer of the chart. + properties: + text: + type: string + url: + type: string + required: + - text + - url + additionalProperties: false title: type: string description: Big title text of the chart @@ -419,6 +420,7 @@ properties: type: boolean description: Whether to hide "Change in" in relative line charts default: false + additionalProperties: false excludedEntities: type: array description: Entities that should be excluded from the chart diff --git a/packages/@ourworldindata/grapher/src/schema/migrations/migrations.ts b/packages/@ourworldindata/grapher/src/schema/migrations/migrations.ts index 7a4fefa30f0..7b9b1858c84 100644 --- a/packages/@ourworldindata/grapher/src/schema/migrations/migrations.ts +++ b/packages/@ourworldindata/grapher/src/schema/migrations/migrations.ts @@ -60,6 +60,18 @@ const migrateFrom004To005 = ( return config } +const migrateFrom005To006 = ( + config: AnyConfigWithValidSchema +): AnyConfigWithValidSchema => { + if (config.relatedQuestions && config.relatedQuestions.length > 0) { + const { text, url } = config.relatedQuestions[0] + if (text && url) config.relatedQuestion = config.relatedQuestions[0] + } + delete config.relatedQuestions + config.$schema = createSchemaForVersion("006") + return config +} + export const runMigration = ( config: AnyConfigWithValidSchema ): AnyConfigWithValidSchema => { @@ -70,5 +82,6 @@ export const runMigration = ( .with("002", () => migrateFrom002To003(config)) .with("003", () => migrateFrom003To004(config)) .with("004", () => migrateFrom004To005(config)) + .with("005", () => migrateFrom005To006(config)) .exhaustive() } diff --git a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts index e82a2d23b1c..244bc61abc4 100644 --- a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts +++ b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts @@ -179,8 +179,8 @@ export enum GrapherTabOption { } export interface RelatedQuestionsConfig { - text: string - url: string + text?: string + url?: string } export enum MissingDataStrategy { @@ -559,7 +559,7 @@ export interface GrapherInterface extends SortConfig { hasChartTab?: boolean hasMapTab?: boolean tab?: GrapherTabOption - relatedQuestions?: RelatedQuestionsConfig[] + relatedQuestion?: RelatedQuestionsConfig details?: DetailDictionary internalNotes?: string variantName?: string @@ -681,7 +681,7 @@ export const grapherKeysToSerialize = [ "selectedFacetStrategy", "hideFacetControl", "comparisonLines", - "relatedQuestions", + "relatedQuestion", "missingDataStrategy", // internals