From 40fe26b3ff441a041fbbfd7c71b6ec503c7740da Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Thu, 20 Feb 2025 17:31:10 +0100 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=94=A8=20refactor=20excludedEntities?= =?UTF-8?q?=20and=20includedEntities=20to=20use=20entity=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/EditorMarimekkoTab.tsx | 89 +++++++------------ adminSiteClient/EditorScatterTab.tsx | 74 ++++++--------- adminSiteClient/GrapherConfigGridEditor.tsx | 2 +- ...064108477-MigrateExcludeIncludeEntities.ts | 65 ++++++++++++++ devTools/schemaProcessor/columns.json | 4 +- .../grapher/src/core/Grapher.jsdom.test.ts | 4 +- .../grapher/src/core/Grapher.tsx | 8 +- .../src/scatterCharts/ScatterPlotChart.tsx | 62 ++++++------- .../ScatterPlotChartConstants.ts | 5 +- .../src/schema/defaultGrapherConfig.ts | 4 +- .../src/schema/grapher-schema.006.yaml | 17 ++-- .../src/stackedCharts/MarimekkoChart.tsx | 52 +++++------ .../stackedCharts/MarimekkoChartConstants.ts | 5 +- .../types/src/grapherTypes/GrapherTypes.ts | 8 +- 14 files changed, 205 insertions(+), 194 deletions(-) create mode 100644 db/migration/1740064108477-MigrateExcludeIncludeEntities.ts diff --git a/adminSiteClient/EditorMarimekkoTab.tsx b/adminSiteClient/EditorMarimekkoTab.tsx index 2e1ffd7c300..a2d94546e20 100644 --- a/adminSiteClient/EditorMarimekkoTab.tsx +++ b/adminSiteClient/EditorMarimekkoTab.tsx @@ -2,7 +2,6 @@ import { faMinus, faTrash } from "@fortawesome/free-solid-svg-icons" import { FontAwesomeIcon } from "@fortawesome/react-fontawesome/index.js" import { EntityName } from "@ourworldindata/types" import { Grapher } from "@ourworldindata/grapher" -import { excludeUndefined } from "@ourworldindata/utils" import lodash from "lodash" import { action, computed, IReactionDisposer, observable, reaction } from "mobx" import { observer } from "mobx-react" @@ -18,91 +17,71 @@ export class EditorMarimekkoTab extends Component<{ grapher: Grapher }> { } @computed private get includedEntityNames(): EntityName[] { - const { includedEntities, inputTable } = this.props.grapher - const { entityIdToNameMap } = inputTable - const includedEntityIds = includedEntities ?? [] - return excludeUndefined( - includedEntityIds.map((entityId) => entityIdToNameMap.get(entityId)) - ) + return this.props.grapher.includedEntityNames ?? [] + } + + @computed private get excludedEntityNames(): EntityName[] { + return this.props.grapher.excludedEntityNames ?? [] } @computed private get includedEntityChoices() { - const { inputTable } = this.props.grapher + const { inputTable, includedEntityNames = [] } = this.props.grapher return inputTable.availableEntityNames - .filter( - (entityName) => !this.includedEntityNames.includes(entityName) - ) + .filter((entityName) => !includedEntityNames.includes(entityName)) .sort() } - @action.bound onIncludeEntity(entity: string) { - const { grapher } = this.props - if (grapher.includedEntities === undefined) { - grapher.includedEntities = [] - } - - const entityId = grapher.table.entityNameToIdMap.get(entity)! - if (grapher.includedEntities.indexOf(entityId) === -1) - grapher.includedEntities.push(entityId) + @computed private get excludedEntityChoices() { + const { inputTable, excludedEntityNames = [] } = this.props.grapher + return inputTable.availableEntityNames + .filter((entityName) => !excludedEntityNames.includes(entityName)) + .sort() } - @action.bound onUnincludeEntity(entity: string) { + @action.bound onExcludeEntity(entityName: string) { const { grapher } = this.props - if (!grapher.includedEntities) return + if (grapher.excludedEntityNames === undefined) { + grapher.excludedEntityNames = [] + } - const entityId = grapher.table.entityNameToIdMap.get(entity) - grapher.includedEntities = grapher.includedEntities.filter( - (e) => e !== entityId - ) + if (!grapher.excludedEntityNames.includes(entityName)) + grapher.excludedEntityNames.push(entityName) } - @computed private get excludedEntityNames(): EntityName[] { - const { excludedEntities, inputTable } = this.props.grapher - const { entityIdToNameMap } = inputTable - const excludedEntityIds = excludedEntities ?? [] - return excludeUndefined( - excludedEntityIds.map((entityId) => entityIdToNameMap.get(entityId)) + @action.bound onUnexcludeEntity(entityName: string) { + const { grapher } = this.props + if (!grapher.excludedEntityNames) return + grapher.excludedEntityNames = grapher.excludedEntityNames.filter( + (e) => e !== entityName ) } - @computed private get excludedEntityChoices() { - const { inputTable } = this.props.grapher - return inputTable.availableEntityNames - .filter( - (entityName) => !this.excludedEntityNames.includes(entityName) - ) - .sort() - } - - @action.bound onExcludeEntity(entity: string) { + @action.bound onIncludeEntity(entityName: string) { const { grapher } = this.props - if (grapher.excludedEntities === undefined) { - grapher.excludedEntities = [] + if (grapher.includedEntityNames === undefined) { + grapher.includedEntityNames = [] } - const entityId = grapher.table.entityNameToIdMap.get(entity)! - if (grapher.excludedEntities.indexOf(entityId) === -1) - grapher.excludedEntities.push(entityId) + if (!grapher.includedEntityNames.includes(entityName)) + grapher.includedEntityNames.push(entityName) } - @action.bound onUnexcludeEntity(entity: string) { + @action.bound onUnincludeEntity(entityName: string) { const { grapher } = this.props - if (!grapher.excludedEntities) return - - const entityId = grapher.table.entityNameToIdMap.get(entity) - grapher.excludedEntities = grapher.excludedEntities.filter( - (e) => e !== entityId + if (!grapher.includedEntityNames) return + grapher.includedEntityNames = grapher.includedEntityNames.filter( + (e) => e !== entityName ) } @action.bound onClearExcludedEntities() { const { grapher } = this.props - grapher.excludedEntities = [] + grapher.excludedEntityNames = [] } @action.bound onClearIncludedEntities() { const { grapher } = this.props - grapher.includedEntities = [] + grapher.includedEntityNames = [] } @action.bound onXOverrideYear(value: number | undefined) { diff --git a/adminSiteClient/EditorScatterTab.tsx b/adminSiteClient/EditorScatterTab.tsx index 582f4489d19..fe79c176453 100644 --- a/adminSiteClient/EditorScatterTab.tsx +++ b/adminSiteClient/EditorScatterTab.tsx @@ -6,7 +6,7 @@ import { ScatterPointLabelStrategy, } from "@ourworldindata/types" import { Grapher } from "@ourworldindata/grapher" -import { debounce, excludeUndefined } from "@ourworldindata/utils" +import { debounce } from "@ourworldindata/utils" import { action, computed, observable } from "mobx" import { observer } from "mobx-react" import { Component } from "react" @@ -33,91 +33,71 @@ export class EditorScatterTab extends Component<{ grapher: Grapher }> { } @computed private get includedEntityNames(): EntityName[] { - const { includedEntities, inputTable } = this.props.grapher - const { entityIdToNameMap } = inputTable - const includedEntityIds = includedEntities ?? [] - return excludeUndefined( - includedEntityIds.map((entityId) => entityIdToNameMap.get(entityId)) - ) + return this.props.grapher.includedEntityNames ?? [] } @computed private get excludedEntityNames(): EntityName[] { - const { excludedEntities, inputTable } = this.props.grapher - const { entityIdToNameMap } = inputTable - const excludedEntityIds = excludedEntities ?? [] - return excludeUndefined( - excludedEntityIds.map((entityId) => entityIdToNameMap.get(entityId)) - ) + return this.props.grapher.excludedEntityNames ?? [] } @computed private get includedEntityChoices() { - const { inputTable } = this.props.grapher + const { inputTable, includedEntityNames = [] } = this.props.grapher return inputTable.availableEntityNames - .filter( - (entityName) => !this.includedEntityNames.includes(entityName) - ) + .filter((entityName) => !includedEntityNames.includes(entityName)) .sort() } @computed private get excludedEntityChoices() { - const { inputTable } = this.props.grapher + const { inputTable, excludedEntityNames = [] } = this.props.grapher return inputTable.availableEntityNames - .filter( - (entityName) => !this.excludedEntityNames.includes(entityName) - ) + .filter((entityName) => !excludedEntityNames.includes(entityName)) .sort() } - @action.bound onExcludeEntity(entity: string) { + @action.bound onExcludeEntity(entityName: string) { const { grapher } = this.props - if (grapher.excludedEntities === undefined) { - grapher.excludedEntities = [] + if (grapher.excludedEntityNames === undefined) { + grapher.excludedEntityNames = [] } - const entityId = grapher.table.entityNameToIdMap.get(entity)! - if (grapher.excludedEntities.indexOf(entityId) === -1) - grapher.excludedEntities.push(entityId) + if (!grapher.excludedEntityNames.includes(entityName)) + grapher.excludedEntityNames.push(entityName) } - @action.bound onUnexcludeEntity(entity: string) { + @action.bound onUnexcludeEntity(entityName: string) { const { grapher } = this.props - if (!grapher.excludedEntities) return - - const entityId = grapher.table.entityNameToIdMap.get(entity) - grapher.excludedEntities = grapher.excludedEntities.filter( - (e) => e !== entityId + if (!grapher.excludedEntityNames) return + grapher.excludedEntityNames = grapher.excludedEntityNames.filter( + (e) => e !== entityName ) } - @action.bound onIncludeEntity(entity: string) { + @action.bound onIncludeEntity(entityName: string) { const { grapher } = this.props - if (grapher.includedEntities === undefined) { - grapher.includedEntities = [] + if (grapher.includedEntityNames === undefined) { + grapher.includedEntityNames = [] } - const entityId = grapher.table.entityNameToIdMap.get(entity)! - if (grapher.includedEntities.indexOf(entityId) === -1) - grapher.includedEntities.push(entityId) + if (!grapher.includedEntityNames.includes(entityName)) + grapher.includedEntityNames.push(entityName) } - @action.bound onUnincludeEntity(entity: string) { + @action.bound onUnincludeEntity(entityName: string) { const { grapher } = this.props - if (!grapher.includedEntities) return - - const entityId = grapher.table.entityNameToIdMap.get(entity) - grapher.includedEntities = grapher.includedEntities.filter( - (e) => e !== entityId + if (!grapher.includedEntityNames) return + grapher.includedEntityNames = grapher.includedEntityNames.filter( + (e) => e !== entityName ) } @action.bound onClearExcludedEntities() { const { grapher } = this.props - grapher.excludedEntities = [] + grapher.excludedEntityNames = [] } @action.bound onClearIncludedEntities() { const { grapher } = this.props - grapher.includedEntities = [] + grapher.includedEntityNames = [] } @action.bound onToggleConnection(value: boolean) { diff --git a/adminSiteClient/GrapherConfigGridEditor.tsx b/adminSiteClient/GrapherConfigGridEditor.tsx index b064688980f..e33ab1d7e3f 100644 --- a/adminSiteClient/GrapherConfigGridEditor.tsx +++ b/adminSiteClient/GrapherConfigGridEditor.tsx @@ -469,7 +469,7 @@ export class GrapherConfigGridEditor extends React.Component // TODO: handle different kinds of arrays here. In effect, the ones to handle are - // includedEntities, excludedEntities (both with a yet to extract control) + // includedEntityNames, excludedEntityNames (both with a yet to extract control) // selection. The seleciton should be refactored because ATM it's 3 arrays and it // is annoying to keep those in sync and target more than one field with a column. undefined diff --git a/db/migration/1740064108477-MigrateExcludeIncludeEntities.ts b/db/migration/1740064108477-MigrateExcludeIncludeEntities.ts new file mode 100644 index 00000000000..533288195c8 --- /dev/null +++ b/db/migration/1740064108477-MigrateExcludeIncludeEntities.ts @@ -0,0 +1,65 @@ +import { MigrationInterface, QueryRunner } from "typeorm" + +export class MigrateExcludeIncludeEntities1740064108477 + implements MigrationInterface +{ + public async up(queryRunner: QueryRunner): Promise { + // This query touches all configs where `excludedEntities` or `includedEntities` + // are present and converts the existing array of ids to an array of names. + // Example: + // - `excludedEntities`: [23, 31416, 8, 6, 3, 1, 13] + // => `excludedEntityNames`: ["Australia", "Korea", "Italy", "Germany", "France", "United Kingdom", "United States"] + + const fields = [ + { oldName: "excludedEntities", newName: "excludedEntityNames" }, + { oldName: "includedEntities", newName: "includedEntityNames" }, + ] + + const tables = [ + { table: "chart_configs", column: "patch" }, + { table: "chart_configs", column: "full" }, + { table: "chart_revisions", column: "config" }, + { table: "suggested_chart_revisions", column: "suggestedConfig" }, + ] + + for (const { oldName, newName } of fields) { + for (const { table, column } of tables) { + await queryRunner.query( + `-- sql + WITH migrated AS ( + SELECT + t.id, + ${column} -> "$.${oldName}" AS ${oldName}, + JSON_ARRAYAGG(entities.name) AS ${newName} + FROM ${table} t, + JSON_TABLE( + t.${column}, + '$.${oldName}[*]' COLUMNS (id INT PATH '$') + ) AS json_ids + JOIN entities ON json_ids.id = entities.id + WHERE ${column} -> "$.${oldName}" IS NOT NULL + GROUP BY t.id + ) + UPDATE ${table} t + INNER JOIN migrated ON t.id = migrated.id + SET ${column} = JSON_SET(${column}, '$.${newName}', migrated.${newName}) + WHERE ${column} ->> '$.${oldName}' IS NOT NULL; + ` + ) + + // Now that the new field is set, drop the old one + await queryRunner.query( + `-- sql + UPDATE ${table} + SET ${column} = JSON_REMOVE(${column}, "$.${oldName}") + WHERE ${column} -> "$.${oldName}" IS NOT NULL + ` + ) + } + } + } + + public async down(): Promise { + // intentially empty + } +} diff --git a/devTools/schemaProcessor/columns.json b/devTools/schemaProcessor/columns.json index f9d3f306d59..e6d86ce9884 100644 --- a/devTools/schemaProcessor/columns.json +++ b/devTools/schemaProcessor/columns.json @@ -654,7 +654,7 @@ }, { "type": "array", - "pointer": "/excludedEntities", + "pointer": "/excludedEntityNames", "editor": "primitiveListEditor" }, { @@ -759,7 +759,7 @@ }, { "type": "array", - "pointer": "/includedEntities", + "pointer": "/includedEntityNames", "editor": "primitiveListEditor" }, { diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts b/packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts index dba594860e5..8e2ca20e0b5 100755 --- a/packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts +++ b/packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts @@ -1080,14 +1080,14 @@ describe("tableForSelection", () => { [1, "UK", "", 2002, 1, null, null, null], [1, "UK", "", 2003, null, null, null, null], [2, "Barbados", "", 2000, null, null, null, null], // x, y value missing - [3, "USA", "", 2001, 2, 1, null, null], // excluded via excludedEntities + [3, "USA", "", 2001, 2, 1, null, null], // excluded via excludedEntityNames [4, "France", "", 2000, 0, null, null, null], // y value missing ]) const grapher = new Grapher({ table, chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot], - excludedEntities: [3], + excludedEntityNames: ["USA"], xSlug: "x", ySlugs: "y", }) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 965b35869d6..7a3c946511b 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -450,12 +450,8 @@ export class Grapher @observable selectedEntityNames: EntityName[] = [] @observable focusedSeriesNames: SeriesName[] = [] - @observable excludedEntities?: number[] = undefined - /** IncludedEntities are usually empty which means use all available entities. When - includedEntities is set it means "only use these entities". excludedEntities - are evaluated afterwards and can still remove entities even if they were included before. - */ - @observable includedEntities?: number[] = undefined + @observable excludedEntityNames?: EntityName[] = undefined + @observable includedEntityNames?: EntityName[] = undefined @observable comparisonLines?: ComparisonLineConfig[] = undefined // todo: Persistables? @observable relatedQuestions?: RelatedQuestionsConfig[] = undefined // todo: Persistables? diff --git a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx index 8c134e82194..654e7ac2dae 100644 --- a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx +++ b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx @@ -136,32 +136,37 @@ export class ScatterPlotChart }>() private filterManuallySelectedEntities(table: OwidTable): OwidTable { - const { includedEntities, excludedEntities } = this.manager - const excludedEntityIdsSet = new Set(excludedEntities) - const includedEntityIdsSet = new Set(includedEntities) - const excludeEntitiesFilter = (entityId: any): boolean => - !excludedEntityIdsSet.has(entityId as number) - const includedEntitiesFilter = (entityId: any): boolean => - includedEntityIdsSet.size > 0 - ? includedEntityIdsSet.has(entityId as number) - : true - const filterFn = (entityId: any): boolean => - excludeEntitiesFilter(entityId) && includedEntitiesFilter(entityId) - const excludedList = excludedEntities ? excludedEntities.join(", ") : "" - const includedList = includedEntities ? includedEntities.join(", ") : "" + const { includedEntityNames, excludedEntityNames } = this.manager + + if (!includedEntityNames && !excludedEntityNames) return table + + const excludedSet = new Set(excludedEntityNames) + const includedSet = new Set(includedEntityNames) + + const excludeFilter = (entityName: EntityName): boolean => + !excludedSet.has(entityName) + const includeFilter = (entityName: EntityName): boolean => + includedSet.size > 0 ? includedSet.has(entityName) : true + + const filterFn = (entityName: any): boolean => + excludeFilter(entityName) && includeFilter(entityName) + + const excludedList = excludedEntityNames + ? excludedEntityNames.join(", ") + : "" + const includedList = includedEntityNames + ? includedEntityNames.join(", ") + : "" + return table.columnFilter( - OwidTableSlugs.entityId, + OwidTableSlugs.entityName, filterFn, - `Excluded entity ids specified by author: ${excludedList} - Included entity ids specified by author: ${includedList}` + `Excluded entities specified by author: ${excludedList} - Included entities specified by author: ${includedList}` ) } transformTable(table: OwidTable): OwidTable { - const { includedEntities, excludedEntities } = this.manager - - if (excludedEntities || includedEntities) { - table = this.filterManuallySelectedEntities(table) - } + table = this.filterManuallySelectedEntities(table) if (this.xScaleType === ScaleType.log && this.xColumnSlug) table = table.replaceNonPositiveCellsForLogScale([this.xColumnSlug]) @@ -223,11 +228,7 @@ export class ScatterPlotChart } transformTableForDisplay(table: OwidTable): OwidTable { - const { includedEntities, excludedEntities } = this.manager - - if (excludedEntities || includedEntities) { - table = this.filterManuallySelectedEntities(table) - } + table = this.filterManuallySelectedEntities(table) // Drop any rows which have non-number values for X or Y. table = table @@ -278,12 +279,7 @@ export class ScatterPlotChart size?: ValueRange } { const { inputTable } = this - const { - animationStartTime, - animationEndTime, - includedEntities, - excludedEntities, - } = this.manager + const { animationStartTime, animationEndTime } = this.manager if (!animationStartTime || !animationEndTime) return {} @@ -292,9 +288,7 @@ export class ScatterPlotChart animationEndTime ) - if (excludedEntities || includedEntities) { - table = this.filterManuallySelectedEntities(table) - } + table = this.filterManuallySelectedEntities(table) if (this.manager.matchingEntitiesOnly && !this.colorColumn.isMissing) { table = table.filterByEntityNames( diff --git a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChartConstants.ts b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChartConstants.ts index 935b05abf7b..e03e1a55ad4 100644 --- a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChartConstants.ts +++ b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChartConstants.ts @@ -11,7 +11,6 @@ import { SeriesName, Color, Time, - EntityId, EntityName, } from "@ourworldindata/types" import { @@ -30,8 +29,8 @@ export interface ScatterPlotManager extends ChartManager { addCountryMode?: EntitySelectionMode xOverrideTime?: Time | undefined tableAfterAuthorTimelineAndActiveChartTransform?: OwidTable - includedEntities?: EntityId[] - excludedEntities?: EntityId[] + includedEntityNames?: EntityName[] + excludedEntityNames?: EntityName[] startTime?: Time endTime?: Time hasTimeline?: boolean diff --git a/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts b/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts index d9af04c43af..415693de4e0 100644 --- a/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts +++ b/packages/@ourworldindata/grapher/src/schema/defaultGrapherConfig.ts @@ -72,7 +72,8 @@ export const defaultGrapherConfig = { time: false, changeInPrefix: false, }, - excludedEntities: [], + excludedEntityNames: [], + includedEntityNames: [], xAxis: { removePointsOutsideDomain: false, scaleType: "linear", @@ -90,7 +91,6 @@ export const defaultGrapherConfig = { sortBy: "total", sortOrder: "desc", hideFacetControl: true, - includedEntities: [], entityTypePlural: "countries and regions", missingDataStrategy: "auto", } as GrapherInterface diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml index 0935a544a04..a3bea737041 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml @@ -433,13 +433,18 @@ properties: type: boolean description: Whether to hide "Change in" in relative line charts default: false - excludedEntities: + excludedEntityNames: type: array description: Entities that should be excluded from the chart default: [] items: - type: integer - minimum: 0 + type: string + includedEntityNames: + type: array + description: Entities to include. Opposite of excludedEntityNames. If this is set then all entities not specified here are excluded. + default: [] + items: + type: string xAxis: $ref: "#/$defs/axis" timelineMaxTime: @@ -499,12 +504,6 @@ properties: type: boolean default: true description: Whether to hide the faceting control - includedEntities: - type: array - description: Entities to include. Opposite of excludedEntities. If this is set then all entities not specified here are excluded. - default: [] - items: - type: number entityTypePlural: description: Plural of the entity type (i.e. when entityType is 'country' this would be 'countries') default: countries and regions diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx index 5e4224b7ce8..53cc3f50cd2 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx +++ b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx @@ -277,34 +277,40 @@ export class MarimekkoChart }>() private filterManuallySelectedEntities(table: OwidTable): OwidTable { - const { excludedEntities, includedEntities } = this.manager - const excludedEntityIdsSet = new Set(excludedEntities) - const includedEntityIdsSet = new Set(includedEntities) - const excludeEntitiesFilter = (entityId: any): boolean => - !excludedEntityIdsSet.has(entityId as number) - const includedEntitiesFilter = (entityId: any): boolean => - includedEntityIdsSet.size > 0 - ? includedEntityIdsSet.has(entityId as number) - : true - const filterFn = (entityId: any): boolean => - excludeEntitiesFilter(entityId) && includedEntitiesFilter(entityId) - const excludedList = excludedEntities ? excludedEntities.join(", ") : "" - const includedList = includedEntities ? includedEntities.join(", ") : "" + const { includedEntityNames, excludedEntityNames } = this.manager + + if (!includedEntityNames && !excludedEntityNames) return table + + const excludedSet = new Set(excludedEntityNames) + const includedSet = new Set(includedEntityNames) + + const excludeFilter = (entityName: EntityName): boolean => + !excludedSet.has(entityName) + const includeFilter = (entityName: EntityName): boolean => + includedSet.size > 0 ? includedSet.has(entityName) : true + + const filterFn = (entityName: any): boolean => + excludeFilter(entityName) && includeFilter(entityName) + + const excludedList = excludedEntityNames + ? excludedEntityNames.join(", ") + : "" + const includedList = includedEntityNames + ? includedEntityNames.join(", ") + : "" + return table.columnFilter( - OwidTableSlugs.entityId, + OwidTableSlugs.entityName, filterFn, - `Excluded entity ids specified by author: ${excludedList} - Included entity ids specified by author: ${includedList}` + `Excluded entities specified by author: ${excludedList} - Included entities specified by author: ${includedList}` ) } transformTable(table: OwidTable): OwidTable { - const { excludedEntities, includedEntities } = this.manager const { yColumnSlugs, manager, colorColumnSlug, xColumnSlug } = this if (!this.yColumnSlugs.length) return table - if (excludedEntities || includedEntities) { - table = this.filterManuallySelectedEntities(table) - } + table = this.filterManuallySelectedEntities(table) // TODO: remove this filter once we don't have mixed type columns in datasets table = table.replaceNonNumericCellsWithErrorValues(yColumnSlugs) @@ -343,13 +349,7 @@ export class MarimekkoChart } transformTableForDisplay(table: OwidTable): OwidTable { - const { includedEntities, excludedEntities } = this.manager - - if (excludedEntities || includedEntities) { - table = this.filterManuallySelectedEntities(table) - } - - return table + return this.filterManuallySelectedEntities(table) } @computed get inputTable(): OwidTable { diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChartConstants.ts b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChartConstants.ts index 4ed6f08c81a..aed837b9225 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChartConstants.ts +++ b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChartConstants.ts @@ -5,7 +5,6 @@ import { SortConfig, Time, Bounds, - EntityId, EntityName, } from "@ourworldindata/utils" import { OwidTable } from "@ourworldindata/core-table" @@ -13,13 +12,13 @@ import { StackedPoint } from "./StackedConstants" import { DualAxis } from "../axis/Axis" export interface MarimekkoChartManager extends ChartManager { endTime?: Time - excludedEntities?: EntityId[] matchingEntitiesOnly?: boolean xOverrideTime?: number tableAfterAuthorTimelineAndActiveChartTransform?: OwidTable sortConfig?: SortConfig hideNoDataArea?: boolean - includedEntities?: number[] + excludedEntityNames?: EntityName[] + includedEntityNames?: EntityName[] } export interface EntityColorData { diff --git a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts index 92c60a4b0d7..0174c7f7e9c 100644 --- a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts +++ b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts @@ -591,8 +591,8 @@ export interface GrapherInterface extends SortConfig { compareEndPointsOnly?: boolean matchingEntitiesOnly?: boolean hideTotalValueLabel?: boolean - excludedEntities?: number[] - includedEntities?: number[] + excludedEntityNames?: EntityName[] + includedEntityNames?: EntityName[] selectedEntityNames?: EntityName[] selectedEntityColors?: { [entityName: string]: string | undefined } focusedSeriesNames?: SeriesName[] @@ -708,7 +708,6 @@ export const grapherKeysToSerialize = [ "scatterPointLabelStrategy", "compareEndPointsOnly", "matchingEntitiesOnly", - "includedEntities", "hideTotalValueLabel", "xAxis", "yAxis", @@ -721,7 +720,8 @@ export const grapherKeysToSerialize = [ "sortBy", "sortOrder", "sortColumnSlug", - "excludedEntities", + "excludedEntityNames", + "includedEntityNames", "selectedFacetStrategy", "hideFacetControl", "comparisonLines", From f8b9d8c807a6851d6b14776990397d9decd6eb9c Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Thu, 20 Feb 2025 17:38:05 +0100 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=94=A8=20remove=20unused=20methods=20?= =?UTF-8?q?that=20map=20entity=20ids=20or=20codes=20to=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- db/model/Entity.ts | 14 -------------- .../core-table/src/OwidTable.ts | 18 ------------------ 2 files changed, 32 deletions(-) diff --git a/db/model/Entity.ts b/db/model/Entity.ts index ba190d3f757..92faf295ff9 100644 --- a/db/model/Entity.ts +++ b/db/model/Entity.ts @@ -14,17 +14,3 @@ export async function mapEntityNamesToEntityIds( return entityNameToIdMap } - -export async function mapEntityIdsToEntityNames( - knex: db.KnexReadonlyTransaction -): Promise> { - const entities = (await knex(EntitiesTableName).select( - "id", - "name" - )) as Pick[] - const entityIdToNameMap = new Map( - entities.map((entity) => [entity.id, entity.name]) - ) - - return entityIdToNameMap -} diff --git a/packages/@ourworldindata/core-table/src/OwidTable.ts b/packages/@ourworldindata/core-table/src/OwidTable.ts index cfb8ef423fc..7c3ec5583b9 100644 --- a/packages/@ourworldindata/core-table/src/OwidTable.ts +++ b/packages/@ourworldindata/core-table/src/OwidTable.ts @@ -67,23 +67,6 @@ export class OwidTable extends CoreTable { return this.entityNameColumn.uniqValuesAsSet } - // todo: can we remove at some point? - @imemo get entityIdToNameMap(): Map { - return this.valueIndex( - this.entityIdColumn.slug, - this.entityNameColumn.slug - ) as Map - } - - // todo: can we remove at some point? - @imemo get entityCodeToNameMap(): Map { - return this.valueIndex( - this.entityCodeColumn.slug, - this.entityNameColumn.slug - ) as Map - } - - // todo: can we remove at some point? @imemo get entityNameToIdMap(): Map { return this.valueIndex( this.entityNameColumn.slug, @@ -91,7 +74,6 @@ export class OwidTable extends CoreTable { ) as Map } - // todo: can we remove at some point? @imemo get entityNameToCodeMap(): Map { return this.valueIndex( this.entityNameColumn.slug,