From 867e314c4b51ddd4b90ce68264c2252ec792e2f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacio=20Foche=20P=C3=A9rez?= Date: Wed, 4 Mar 2026 16:15:19 +0100 Subject: [PATCH] fix(mapping): pass correct model to MappingDialog for category options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MappingDialog always used modelFactory("categoryOptions") which resolved to CategoryOptionMappedModel — a model that expands each category option into N rows (one per parent category) with composite IDs. This caused three bugs in global mapping: duplicate destination entries, mappings appearing lost on re-open, and automap results invisible. Fix by passing the source model from MappingTable to MappingDialog via config, so the destination picker uses the same model as the source (GlobalCategoryOptionModel for global mapping, which lists plain IDs without category expansion). Falls back to modelFactory for nested mapping contexts. Also adds .claude/ to .gitignore and includes OpenSpec change artifacts. Closes: https://app.clickup.com/t/869bvjqn0 Co-Authored-By: Claude Opus 4.6 --- .gitignore | 3 + .../.openspec.yaml | 2 + .../fix-category-option-mapping/design.md | 57 +++++++++++++++++++ .../fix-category-option-mapping/proposal.md | 25 ++++++++ .../specs/category-option-mapping/spec.md | 44 ++++++++++++++ .../fix-category-option-mapping/tasks.md | 19 +++++++ .../mapping-dialog/MappingDialog.tsx | 4 +- .../components/mapping-table/MappingTable.tsx | 9 ++- 8 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 openspec/changes/fix-category-option-mapping/.openspec.yaml create mode 100644 openspec/changes/fix-category-option-mapping/design.md create mode 100644 openspec/changes/fix-category-option-mapping/proposal.md create mode 100644 openspec/changes/fix-category-option-mapping/specs/category-option-mapping/spec.md create mode 100644 openspec/changes/fix-category-option-mapping/tasks.md diff --git a/.gitignore b/.gitignore index f861804cf..674eed813 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,9 @@ cypress/fixtures/ .idea/* .vscode/* +# Claude Code +.claude/ + # Configuration /**/app-config.json diff --git a/openspec/changes/fix-category-option-mapping/.openspec.yaml b/openspec/changes/fix-category-option-mapping/.openspec.yaml new file mode 100644 index 000000000..5aae5cfa4 --- /dev/null +++ b/openspec/changes/fix-category-option-mapping/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-04 diff --git a/openspec/changes/fix-category-option-mapping/design.md b/openspec/changes/fix-category-option-mapping/design.md new file mode 100644 index 000000000..b46de1786 --- /dev/null +++ b/openspec/changes/fix-category-option-mapping/design.md @@ -0,0 +1,57 @@ +## Context + +Category options in DHIS2 can belong to multiple categories. The mapping system uses a composite ID format `"{categoryId}-{categoryOptionId}"` to disambiguate which category context a mapping applies in. This composite format is used in nested mapping (within a category combo) by `CategoryOptionMappedModel.modelTransform`. + +However, in **global mapping**, category options should be listed once each with their plain DHIS2 ID — they're mapped globally regardless of which category they belong to. The current code has three intertwined problems: + +1. **`MappingDialog` always uses `modelFactory("categoryOptions")`** which resolves to `CategoryOptionMappedModel` (the first class with `mappingType = "categoryOptions"`). This model's `modelTransform` expands each option into N rows by category → duplicates. + +2. **ID format mismatch on read-back**: When a user selects a destination item, the composite ID (e.g. `"catA-opt1"`) is stored as `mappedId`. When re-opening, `MappingDialog` strips it to plain ID via `_.last(split("-"))` → `"opt1"`. But the table lists composite IDs, so `"opt1"` matches nothing → selection appears lost. + +3. **Automap stores plain IDs**: `lookupSimilar()` returns plain DHIS2 IDs. With `initialShowOnlySelected=true` and composite IDs in the table, the result is invisible. + +The backend consumer (`mapCategoryOptionCombo` in `src/utils/synchronization.ts`) already handles both formats using `_.last(candidate.split("-"))`, so fixing the UI won't break sync operations. + +## Goals / Non-Goals + +**Goals:** +- Each category option appears exactly once in the global mapping destination picker +- Selected mappings are visually persistent when re-opening the dialog +- Automap results are visible in the selection window +- Nested (non-global) category option mapping continues to work with composite IDs + +**Non-Goals:** +- Changing the mapping storage format in the dataStore +- Modifying the backend sync consumer (`mapCategoryOptionCombo`) +- Fixing mapping for other metadata types + +## Decisions + +### 1. Use plain `CategoryOptionModel` for the global mapping destination picker + +**Change:** In `MappingDialog`, when the mapping context is global category options, use a model that does NOT have the category-exploding `modelTransform`. This can be done by passing the appropriate model from the `MappingTable` (which already knows whether it's global or nested) instead of re-resolving via `modelFactory`. + +**Rationale:** The `MappingTable` already holds the model class for the source rows. For global mapping, this is `GlobalCategoryOptionModel` (no transform). The destination picker should use the same kind of model — plain category options without category expansion. + +**Approach:** Add the model (or mappingType context) to `MappingDialogConfig` so `MappingDialog` can use it directly instead of calling `modelFactory(mappingType)`. When a global model is provided, the dialog uses it; otherwise it falls back to the current `modelFactory` behavior for backward compatibility with nested mapping. + +**Alternatives considered:** +- Creating a separate `GlobalCategoryOptionDestinationModel` — too many model classes already, adds complexity. +- Modifying `modelFactory` to accept a "global" flag — leaks UI concerns into the factory. + +### 2. Store and read back plain IDs consistently for global category options + +**Change:** Since the destination picker will now list plain IDs (no composite), the `mappedId` stored will also be plain. The `mappedId` read-back logic in `MappingDialog` (lines 56-64) using `_.last(split("-"))` will naturally produce the correct plain ID. No change needed to the read-back logic — it already handles plain IDs correctly (`_.last("opt1".split("-"))` = `"opt1"`). + +**Rationale:** With plain IDs in both storage and display, the selection will match correctly when the dialog is re-opened. + +### 3. No change needed for automap + +**Change:** None. Automap already stores plain IDs. Once the destination picker uses plain IDs too, the automap result will match rows correctly. + +**Rationale:** The automap bug is a symptom of the same root cause (composite vs plain ID mismatch in the destination table). Fixing decision #1 resolves it automatically. + +## Risks / Trade-offs + +- **[Low] Existing global mappings with composite `mappedId` values** → The `_.last(split("-"))` read-back logic already strips these to plain IDs, so existing stored mappings will display correctly. New mappings will be stored with plain IDs. The backend consumer handles both formats. +- **[Low] Nested (non-global) category option mapping** → Unchanged. The `MappingDialog` will only use the plain model when a global-context model is explicitly provided; nested mapping continues using `CategoryOptionMappedModel` via `modelFactory`. diff --git a/openspec/changes/fix-category-option-mapping/proposal.md b/openspec/changes/fix-category-option-mapping/proposal.md new file mode 100644 index 000000000..23ac8570b --- /dev/null +++ b/openspec/changes/fix-category-option-mapping/proposal.md @@ -0,0 +1,25 @@ +## Why + +Category option mapping in Global Mapping is broken in three ways: the destination picker shows duplicate entries (one per category a category option belongs to), selected mappings appear lost when re-opening the dialog, and automap results are invisible in the selection window. All three stem from an inconsistency between how category option IDs are stored (composite `categoryId-optionId` vs plain `optionId`) and how they are read back in the UI. + +## What Changes + +- **Fix duplicate destination entries**: The `MappingDialog` uses `modelFactory("categoryOptions")` which resolves to `CategoryOptionMappedModel` — a model with a `modelTransform` that explodes each category option into N rows (one per parent category). For the destination picker in global mapping, use plain `CategoryOptionModel` instead to list each category option once. +- **Fix mapping not persisting visually**: `MappingDialog` stores composite IDs as `mappedId` but strips them back to plain IDs with `_.last(split("-"))` when reading, causing a mismatch with the composite row IDs in the table. Ensure consistent ID handling so the previously selected item is correctly highlighted. +- **Fix automap selection invisible**: `AutoMapUseCase` stores plain destination IDs, but the destination table lists composite IDs. The `initialShowOnlySelected` filter then shows zero rows. Align the ID format used by automap with what the table displays. + +## Capabilities + +### New Capabilities + +_(none — this is a bug fix)_ + +### Modified Capabilities + +_(no spec-level requirement changes — the intended mapping behavior was correct by design, only the ID handling is broken)_ + +## Impact + +- **Code**: `src/presentation/react/core/components/mapping-dialog/MappingDialog.tsx` (ID consistency), `src/models/dhis/mapping.ts` (`GlobalCategoryOptionModel`), `src/models/dhis/factory.ts` (model resolution for global mapping context) +- **User impact**: Global category option mapping will show each option once, persist selections visually, and display automap results correctly +- **Risk**: Low — changes are scoped to the mapping UI; the backend mapping consumer (`mapCategoryOptionCombo` in `synchronization.ts`) already handles both composite and plain keys via `_.last(split("-"))` diff --git a/openspec/changes/fix-category-option-mapping/specs/category-option-mapping/spec.md b/openspec/changes/fix-category-option-mapping/specs/category-option-mapping/spec.md new file mode 100644 index 000000000..7b03e44e7 --- /dev/null +++ b/openspec/changes/fix-category-option-mapping/specs/category-option-mapping/spec.md @@ -0,0 +1,44 @@ +## ADDED Requirements + +### Requirement: Global mapping destination picker SHALL list each category option exactly once + +When the user opens the mapping dialog for a category option in the global mapping context, the destination picker MUST display each category option from the destination instance exactly once, using its plain DHIS2 ID. + +#### Scenario: Category option belonging to multiple categories appears once +- **WHEN** a category option exists in the destination instance and belongs to 3 different categories +- **THEN** the destination picker SHALL display that category option exactly once, not 3 times + +#### Scenario: Destination picker uses plain IDs for global mapping +- **WHEN** the mapping dialog opens for a global category option mapping +- **THEN** each row in the destination picker SHALL use the plain category option ID (not the composite `categoryId-optionId` format) + +### Requirement: Selected global category option mapping SHALL be visible when re-opening the dialog + +When the user has selected a destination category option in global mapping, re-opening the mapping dialog for the same source element MUST show the previously selected destination item as selected. + +#### Scenario: Previously mapped category option is highlighted on dialog re-open +- **WHEN** the user maps source category option A to destination category option B and closes the dialog +- **AND** the user re-opens the mapping dialog for source category option A +- **THEN** destination category option B SHALL be shown as selected in the picker + +#### Scenario: "Show only selected" filter displays the mapped item +- **WHEN** the user re-opens a mapping dialog for a category option that was previously mapped +- **AND** the dialog opens with the "show only selected" filter active +- **THEN** the previously mapped destination category option SHALL appear in the filtered list + +### Requirement: Automap results for category options SHALL be visible in the mapping dialog + +When automap has assigned a destination category option, opening the mapping dialog MUST show that automapped selection. + +#### Scenario: Automapped category option is visible in the dialog +- **WHEN** automap has matched source category option A to destination category option B +- **AND** the user opens the mapping dialog for source category option A +- **THEN** destination category option B SHALL be shown as selected in the picker + +### Requirement: Nested (non-global) category option mapping SHALL continue using composite IDs + +The composite `categoryId-optionId` format used in nested category option mapping (within category combos) MUST remain unchanged. + +#### Scenario: Nested mapping still uses composite IDs +- **WHEN** the user maps a category option within a category combo mapping context (non-global) +- **THEN** the destination picker SHALL use composite `categoryId-optionId` IDs as before diff --git a/openspec/changes/fix-category-option-mapping/tasks.md b/openspec/changes/fix-category-option-mapping/tasks.md new file mode 100644 index 000000000..4b3b360ad --- /dev/null +++ b/openspec/changes/fix-category-option-mapping/tasks.md @@ -0,0 +1,19 @@ +## 1. Pass model from MappingTable to MappingDialog + +- [ ] 1.1 [FE] Add a `model` field to `MappingDialogConfig` in `MappingDialog.tsx` so the caller can specify which model the destination picker should use +- [ ] 1.2 [FE] In `MappingDialog`, use the provided `model` from config when available, falling back to `modelFactory(mappingType)` for backward compatibility +- [ ] 1.3 [FE] In `MappingTable.tsx`, pass the current source model (which is `GlobalCategoryOptionModel` for global mapping) when opening the `MappingDialog` + +## 2. Verify ID consistency + +- [ ] 2.1 [FE] Verify that with the plain model in the destination picker, stored `mappedId` values are plain IDs, and `_.last(split("-"))` read-back produces matching values +- [ ] 2.2 [FE] Verify that nested (non-global) category option mapping still works by confirming the fallback to `modelFactory` produces `CategoryOptionMappedModel` with composite IDs + +## 3. Unit Tests + +- [ ] 3.1 [FE] Add a test verifying `MappingDialog` uses the provided model instead of calling `modelFactory` when a model is passed in the config +- [ ] 3.2 [FE] Run `yarn test` to confirm all existing tests pass + +## 4. Validation + +- [ ] 4.1 [FE] Run `yarn test` and `yarn build core-app` to confirm everything compiles and passes diff --git a/src/presentation/react/core/components/mapping-dialog/MappingDialog.tsx b/src/presentation/react/core/components/mapping-dialog/MappingDialog.tsx index d395b978a..c2a793b37 100644 --- a/src/presentation/react/core/components/mapping-dialog/MappingDialog.tsx +++ b/src/presentation/react/core/components/mapping-dialog/MappingDialog.tsx @@ -7,6 +7,7 @@ import React, { useEffect, useState, useMemo, useCallback } from "react"; import { DataSource, isDhisInstance } from "../../../../../domain/instance/entities/DataSource"; import { MetadataMappingDictionary } from "../../../../../domain/mapping/entities/MetadataMapping"; import i18n from "../../../../../utils/i18n"; +import { D2Model } from "../../../../../models/dhis/default"; import { modelFactory } from "../../../../../models/dhis/factory"; import { MetadataType } from "../../../../../utils/d2"; import { useAppContext } from "../../contexts/AppContext"; @@ -19,6 +20,7 @@ export interface MappingDialogConfig { mappingType?: string; mappingPath?: string[]; firstElement?: MetadataType; + model?: typeof D2Model; } export interface MappingDialogProps { @@ -69,7 +71,7 @@ const MappingDialog: React.FC = ({ const [selected, updateSelected] = useState(defaultSelection); - const model = useMemo(() => modelFactory(mappingType), [mappingType]); + const model = useMemo(() => config.model ?? modelFactory(mappingType), [config.model, mappingType]); const modelName = useMemo(() => model.getModelName(), [model]); const api = useMemo(() => { return isDhisInstance(instance) ? compositionRoot.instances.getApi(instance) : defaultApi; diff --git a/src/presentation/react/core/components/mapping-table/MappingTable.tsx b/src/presentation/react/core/components/mapping-table/MappingTable.tsx index 9d60ae872..8d624fcdf 100644 --- a/src/presentation/react/core/components/mapping-table/MappingTable.tsx +++ b/src/presentation/react/core/components/mapping-table/MappingTable.tsx @@ -292,6 +292,7 @@ export default function MappingTable({ mappingPath, mappingType, firstElement, + model: firstElement.model, }); } } @@ -326,7 +327,13 @@ export default function MappingTable({ .value(); if (types.length === 1) { - setMappingConfig({ elements, mappingPath, mappingType: types[0], firstElement }); + setMappingConfig({ + elements, + mappingPath, + mappingType: types[0], + firstElement, + model: firstElement?.model, + }); setSelectedIds([]); } else if (types.length > 1) { snackbar.error(i18n.t("You need to select all items from the same type"));