From bf18c836ab44d1b5e83bc2d1c3cf2c06824451dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacio=20Foche=20P=C3=A9rez?= Date: Wed, 4 Mar 2026 13:01:31 +0100 Subject: [PATCH 1/2] fix(packages): await metadataPayloadBuilder.build() in CreatePackageUseCase The async build() call was missing `await`, causing a raw Promise object to be stored as package contents instead of the resolved MetadataPackage. This made all module-generated packages appear empty (0 elements on import, empty JSON downloads, no diff data, empty mapping tab). Also removes unnecessary generic type parameters from TransformationRepository to prevent Promise objects from being silently accepted as payload, and adds unit tests for CreatePackageUseCase. Closes: https://app.clickup.com/t/869bvjxj9 Co-Authored-By: Claude Opus 4.6 --- .../TEID2ApiRepository.ts | 7 +- .../TransformationD2ApiRepository.ts | 45 ++-- .../packages/usecases/CreatePackageUseCase.ts | 2 +- .../__tests__/CreatePackageUseCase.spec.ts | 237 ++++++++++++++++++ .../repositories/TransformationRepository.ts | 12 +- 5 files changed, 273 insertions(+), 30 deletions(-) create mode 100644 src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts diff --git a/src/data/tracked-entity-instances/TEID2ApiRepository.ts b/src/data/tracked-entity-instances/TEID2ApiRepository.ts index aa2f3ac13..ebca7e656 100644 --- a/src/data/tracked-entity-instances/TEID2ApiRepository.ts +++ b/src/data/tracked-entity-instances/TEID2ApiRepository.ts @@ -8,6 +8,7 @@ import { } from "../../domain/aggregated/entities/DataSynchronizationParams"; import { buildPeriodFromParams } from "../../domain/aggregated/utils"; import { Instance } from "../../domain/instance/entities/Instance"; +import { MetadataPackage } from "../../domain/metadata/entities/MetadataEntities"; import { SynchronizationResult } from "../../domain/reports/entities/SynchronizationResult"; import { cleanOrgUnitPaths } from "../../domain/synchronization/utils"; import { TEIsPackage } from "../../domain/tracked-entity-instances/entities/TEIsPackage"; @@ -134,11 +135,11 @@ export class TEID2ApiRepository implements TEIRepository { trackedEntities: data.trackedEntities.map(tei => this.buildD2TrackerTrackedEntity(tei)), }; - const versionedRequest = this.transformationRepository.mapPackageTo( + const versionedRequest = this.transformationRepository.mapPackageTo( this.instance.apiVersion, - baseRequest, + baseRequest as unknown as MetadataPackage, teiTransformations - ); + ) as unknown as TrackerPostRequest; const response = await this.api.tracker.post(teiPostParams, versionedRequest).getData(); diff --git a/src/data/transformations/TransformationD2ApiRepository.ts b/src/data/transformations/TransformationD2ApiRepository.ts index 6eba0110c..b294fd437 100644 --- a/src/data/transformations/TransformationD2ApiRepository.ts +++ b/src/data/transformations/TransformationD2ApiRepository.ts @@ -1,4 +1,5 @@ import _ from "lodash"; +import { MetadataPackage } from "../../domain/metadata/entities/MetadataEntities"; import { Transformation } from "../../domain/transformations/entities/Transformation"; import { TransformationRepository } from "../../domain/transformations/repositories/TransformationRepository"; import { API_VERSION } from "../../types/d2-api"; @@ -12,20 +13,20 @@ export class TransformationD2ApiRepository implements TransformationRepository { * @param payload payload to transform * @param transformations list of possible transformations to apply */ - public mapPackageTo( + public mapPackageTo( destination: number, - payload: Input, + payload: MetadataPackage, transformations: Transformation[] = [], origin?: number - ): Output { + ): MetadataPackage { const transformationstoApply = _.orderBy(transformations, ["apiVersion"]).filter( ({ apiVersion }) => apiVersion <= destination && apiVersion > (origin || API_VERSION) ); if (transformationstoApply.length > 0) { - return this.applyTransformations(payload, transformationstoApply); + return this.applyTransformations(payload, transformationstoApply); } else { - return payload as unknown as Output; + return payload; } } @@ -38,42 +39,46 @@ export class TransformationD2ApiRepository implements TransformationRepository { * @param payload payload to transform * @param transformations list of possible transformations to apply */ - public mapPackageFrom( + public mapPackageFrom( origin: number, - payload: Input, + payload: MetadataPackage, transformations: Transformation[] = [], destination?: number - ): Output { + ): MetadataPackage { const transformationstoApply = _.orderBy(transformations, ["apiVersion"], ["desc"]).filter( ({ apiVersion }) => apiVersion <= origin && apiVersion > (destination || API_VERSION) ); if (transformationstoApply.length > 0) { return this.cleanTransformationObject( - this.undoTransformations(payload, transformationstoApply) + this.undoTransformations(payload, transformationstoApply) ); } else { - return payload as unknown as Output; + return payload; } } - private applyTransformations(payload: Input, transformations: Transformation[]): Output { + private applyTransformations(payload: MetadataPackage, transformations: Transformation[]): MetadataPackage { return transformations.reduce( - (transformedPayload: Output, transformation: Transformation) => - transformation.apply ? transformation.apply(transformedPayload) : transformedPayload, - payload as unknown as Output + (transformedPayload: MetadataPackage, transformation: Transformation) => + transformation.apply + ? transformation.apply(transformedPayload) + : transformedPayload, + payload ); } - private undoTransformations(payload: Input, transformations: Transformation[]): Output { + private undoTransformations(payload: MetadataPackage, transformations: Transformation[]): MetadataPackage { return transformations.reduce( - (transformedPayload: Output, transformation: Transformation) => - transformation.undo ? transformation.undo(transformedPayload) : transformedPayload, - payload as unknown as Output + (transformedPayload: MetadataPackage, transformation: Transformation) => + transformation.undo + ? transformation.undo(transformedPayload) + : transformedPayload, + payload ); } - private cleanTransformationObject(payload: Output): Output { + private cleanTransformationObject(payload: MetadataPackage): MetadataPackage { return _.transform( payload as Record, (result, value, key) => { @@ -84,6 +89,6 @@ export class TransformationD2ApiRepository implements TransformationRepository { } }, {} as Record - ) as Output; + ) as MetadataPackage; } } diff --git a/src/domain/packages/usecases/CreatePackageUseCase.ts b/src/domain/packages/usecases/CreatePackageUseCase.ts index 4d0ffa86d..f5135cedc 100644 --- a/src/domain/packages/usecases/CreatePackageUseCase.ts +++ b/src/domain/packages/usecases/CreatePackageUseCase.ts @@ -38,7 +38,7 @@ export class CreatePackageUseCase implements UseCase { targetInstances: [], }; - const basePayload = contents ? contents : this.metadataPayloadBuilder.build(syncBuilder); + const basePayload = contents ? contents : await this.metadataPayloadBuilder.build(syncBuilder); const versionedPayload = this.transformationRepository.mapPackageTo( apiVersion, diff --git a/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts b/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts new file mode 100644 index 000000000..fd695e016 --- /dev/null +++ b/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts @@ -0,0 +1,237 @@ +import { Mock } from "vitest"; +import { CreatePackageUseCase } from "../CreatePackageUseCase"; +import { Package } from "../../entities/Package"; +import { MetadataModule } from "../../../modules/entities/MetadataModule"; +import { Instance } from "../../../instance/entities/Instance"; +import { MetadataPayloadBuilder } from "../../../metadata/builders/MetadataPayloadBuilder"; +import { DynamicRepositoryFactory } from "../../../common/factories/DynamicRepositoryFactory"; +import { TransformationRepository } from "../../../transformations/repositories/TransformationRepository"; +import { MetadataPackage } from "../../../metadata/entities/MetadataEntities"; +import { User } from "../../../user/entities/User"; +import { StorageClient } from "../../../storage/repositories/StorageClient"; +import { Namespace } from "../../../../data/storage/Namespaces"; + +describe("CreatePackageUseCase", () => { + let metadataPayloadBuilder: { build: Mock }; + let repositoryFactory: DynamicRepositoryFactory; + let transformationRepository: TransformationRepository; + let localInstance: Instance; + let mockStorageClient: { saveObjectInCollection: Mock }; + + const testUser: User = { + id: "user1", + name: "Test User", + email: "test@example.com", + username: "testuser", + userGroups: [], + organisationUnits: [], + dataViewOrganisationUnits: [], + isGlobalAdmin: false, + isAppConfigurator: false, + isAppExecutor: false, + }; + + const builtPayload: MetadataPackage = { + dataSets: [{ id: "ds1", name: "DataSet 1" } as any], + }; + + const preSuppliedContents: MetadataPackage = { + indicators: [{ id: "ind1", name: "Indicator 1" } as any], + }; + + beforeEach(() => { + metadataPayloadBuilder = { + build: vi.fn().mockResolvedValue(builtPayload), + }; + + mockStorageClient = { + saveObjectInCollection: vi.fn().mockResolvedValue(undefined), + }; + + transformationRepository = { + mapPackageTo: vi.fn().mockImplementation((_destination, payload, _transformations, _origin?) => payload), + mapPackageFrom: vi.fn().mockImplementation((_origin, payload, _transformations, _destination?) => payload), + }; + + localInstance = Instance.build({ + type: "local", + name: "Local", + url: "http://localhost:8080", + }); + + repositoryFactory = new DynamicRepositoryFactory(); + + repositoryFactory.bindByInstance( + "configRepository", + (_instance: Instance) => ({ + getStorageClient: vi.fn(), + getUserStorageClient: vi.fn(), + changeStorageClient: vi.fn(), + getStorageClientPromise: vi.fn().mockResolvedValue(mockStorageClient), + }), + ); + + repositoryFactory.bindByInstance( + "userRepository", + (_instance: Instance) => ({ + getCurrent: vi.fn().mockResolvedValue(testUser), + }), + ); + }); + + function buildTestModule(overrides?: Partial): MetadataModule { + return MetadataModule.build({ + id: "mod1", + name: "Test Module", + instance: "LOCAL", + department: { id: "dept1", name: "Department 1" }, + metadataIds: ["ds1"], + lastPackageVersion: "0.0.0", + ...overrides, + }); + } + + function buildTestPackage(overrides?: Partial): Package { + const module = buildTestModule(); + return Package.build({ + id: "pkg1", + name: "Test Package", + description: "A test package", + version: "1.0.0", + dhisVersion: "2.38.0", + module: { + id: module.id, + name: module.name, + instance: module.instance, + department: module.department, + }, + contents: {}, + ...overrides, + }); + } + + it("should use payload from metadataPayloadBuilder.build() when contents is not provided", async () => { + const module = buildTestModule(); + const sourcePackage = buildTestPackage(); + const dhisVersion = "2.38.0"; + + const useCase = new CreatePackageUseCase( + metadataPayloadBuilder as unknown as MetadataPayloadBuilder, + repositoryFactory, + transformationRepository, + localInstance, + ); + + const validations = await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); + + expect(validations).toEqual([]); + expect(metadataPayloadBuilder.build).toHaveBeenCalledTimes(1); + + const savedPackageCall = (mockStorageClient.saveObjectInCollection as Mock).mock.calls.find( + ([namespace]: [string]) => namespace === Namespace.PACKAGES + ); + + expect(savedPackageCall).toBeDefined(); + const savedPackage = savedPackageCall![1] as Package; + expect(savedPackage.contents).toEqual(builtPayload); + }); + + it("should verify the built payload is a resolved MetadataPackage, not a Promise", async () => { + const module = buildTestModule(); + const sourcePackage = buildTestPackage(); + const dhisVersion = "2.38.0"; + + const useCase = new CreatePackageUseCase( + metadataPayloadBuilder as unknown as MetadataPayloadBuilder, + repositoryFactory, + transformationRepository, + localInstance, + ); + + await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); + + const savedPackageCall = (mockStorageClient.saveObjectInCollection as Mock).mock.calls.find( + ([namespace]: [string]) => namespace === Namespace.PACKAGES + ); + + expect(savedPackageCall).toBeDefined(); + const savedPackage = savedPackageCall![1] as Package; + + // Verify the contents is the resolved value, not a Promise or empty object + expect(savedPackage.contents).not.toBeInstanceOf(Promise); + expect(savedPackage.contents).not.toEqual({}); + expect(savedPackage.contents).toHaveProperty("dataSets"); + expect(savedPackage.contents.dataSets).toEqual([{ id: "ds1", name: "DataSet 1" }]); + }); + + it("should use provided contents directly without calling metadataPayloadBuilder.build()", async () => { + const module = buildTestModule(); + const sourcePackage = buildTestPackage(); + const dhisVersion = "2.38.0"; + + const useCase = new CreatePackageUseCase( + metadataPayloadBuilder as unknown as MetadataPayloadBuilder, + repositoryFactory, + transformationRepository, + localInstance, + ); + + const validations = await useCase.execute("LOCAL", sourcePackage, module, dhisVersion, preSuppliedContents); + + expect(validations).toEqual([]); + expect(metadataPayloadBuilder.build).not.toHaveBeenCalled(); + + const savedPackageCall = (mockStorageClient.saveObjectInCollection as Mock).mock.calls.find( + ([namespace]: [string]) => namespace === Namespace.PACKAGES + ); + + expect(savedPackageCall).toBeDefined(); + const savedPackage = savedPackageCall![1] as Package; + expect(savedPackage.contents).toEqual(preSuppliedContents); + }); + + it("should pass the payload through transformationRepository.mapPackageTo()", async () => { + const module = buildTestModule(); + const sourcePackage = buildTestPackage(); + const dhisVersion = "2.38.0"; + + const useCase = new CreatePackageUseCase( + metadataPayloadBuilder as unknown as MetadataPayloadBuilder, + repositoryFactory, + transformationRepository, + localInstance, + ); + + await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); + + expect(transformationRepository.mapPackageTo).toHaveBeenCalledTimes(1); + expect(transformationRepository.mapPackageTo).toHaveBeenCalledWith( + 38, + builtPayload, + expect.any(Array), + ); + }); + + it("should save the module with updated lastPackageVersion", async () => { + const module = buildTestModule(); + const sourcePackage = buildTestPackage(); + const dhisVersion = "2.38.0"; + + const useCase = new CreatePackageUseCase( + metadataPayloadBuilder as unknown as MetadataPayloadBuilder, + repositoryFactory, + transformationRepository, + localInstance, + ); + + await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); + + const savedModuleCall = (mockStorageClient.saveObjectInCollection as Mock).mock.calls.find( + ([namespace]: [string]) => namespace === Namespace.MODULES + ); + + expect(savedModuleCall).toBeDefined(); + const savedModule = savedModuleCall![1] as MetadataModule; + expect(savedModule.lastPackageVersion).toEqual("1.0.0"); + }); +}); diff --git a/src/domain/transformations/repositories/TransformationRepository.ts b/src/domain/transformations/repositories/TransformationRepository.ts index 75833fe98..4db7dc8c8 100644 --- a/src/domain/transformations/repositories/TransformationRepository.ts +++ b/src/domain/transformations/repositories/TransformationRepository.ts @@ -2,17 +2,17 @@ import { Transformation } from "../entities/Transformation"; import { MetadataPackage } from "../../metadata/entities/MetadataEntities"; export interface TransformationRepository { - mapPackageTo( + mapPackageTo( destination: number, - payload: Input, + payload: MetadataPackage, transformations: Transformation[], origin?: number - ): Output; + ): MetadataPackage; - mapPackageFrom( + mapPackageFrom( origin: number, - payload: Input, + payload: MetadataPackage, transformations: Transformation[], destination?: number - ): Output; + ): MetadataPackage; } From 22a4be1f866263db10d9be45f29a9e752ff6d8eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacio=20Foche=20P=C3=A9rez?= Date: Wed, 4 Mar 2026 13:36:11 +0100 Subject: [PATCH 2/2] chore: add openspec change artifacts and gitignore .claude/ Include the OpenSpec proposal, design, specs, and tasks for the fix-empty-module-package-contents change. Add .claude/ to .gitignore. Also includes prettier formatting applied by pre-commit hook. Co-Authored-By: Claude Opus 4.6 --- .gitignore | 3 + .../.openspec.yaml | 2 + .../design.md | 67 ++++++ .../proposal.md | 24 +++ .../specs/package-generation/spec.md | 25 +++ .../tasks.md | 18 ++ openspec/config.yaml | 204 ++++++++++++++++++ .../TransformationD2ApiRepository.ts | 4 +- .../__tests__/CreatePackageUseCase.spec.ts | 42 ++-- 9 files changed, 360 insertions(+), 29 deletions(-) create mode 100644 openspec/changes/fix-empty-module-package-contents/.openspec.yaml create mode 100644 openspec/changes/fix-empty-module-package-contents/design.md create mode 100644 openspec/changes/fix-empty-module-package-contents/proposal.md create mode 100644 openspec/changes/fix-empty-module-package-contents/specs/package-generation/spec.md create mode 100644 openspec/changes/fix-empty-module-package-contents/tasks.md create mode 100644 openspec/config.yaml 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-empty-module-package-contents/.openspec.yaml b/openspec/changes/fix-empty-module-package-contents/.openspec.yaml new file mode 100644 index 000000000..5aae5cfa4 --- /dev/null +++ b/openspec/changes/fix-empty-module-package-contents/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-04 diff --git a/openspec/changes/fix-empty-module-package-contents/design.md b/openspec/changes/fix-empty-module-package-contents/design.md new file mode 100644 index 000000000..ac35fbf38 --- /dev/null +++ b/openspec/changes/fix-empty-module-package-contents/design.md @@ -0,0 +1,67 @@ +## Context + +`CreatePackageUseCase.execute()` builds a metadata package from a module by calling `metadataPayloadBuilder.build(syncBuilder)` — an async method that returns `Promise`. A refactor (commit fb983808) replaced the previous `await`-ed call with the builder pattern but dropped the `await` keyword. The result is that a raw Promise object is passed through `mapPackageTo()` and stored as the package `contents`, which serializes to `{}` in the dataStore. + +Every downstream operation (import, download JSON, diff, mapping) reads `contents` and finds it empty, producing the four symptoms reported in the ticket. + +The "create package from file" path is unaffected because its `contents` parameter is already resolved — the ternary short-circuits before hitting the builder call. + +## Goals / Non-Goals + +**Goals:** +- Restore correct metadata inclusion in packages generated from modules +- Prevent this class of bug from reoccurring via improved type constraints +- Add a unit test for `CreatePackageUseCase` covering the payload resolution path + +**Non-Goals:** +- Fixing or recovering previously-generated broken packages (users can regenerate) +- Changing the package format or storage schema +- Refactoring the broader transformation pipeline + +## Decisions + +### 1. Add missing `await` in CreatePackageUseCase (line 41) + +**Change:** `this.metadataPayloadBuilder.build(syncBuilder)` → `await this.metadataPayloadBuilder.build(syncBuilder)` + +**Rationale:** This is the minimal, correct fix. All other call sites (`DiffPackageUseCase`, `DownloadModuleSnapshotUseCase`, `CreatePullRequestUseCase`) already `await` the same method. The function is already `async`, so no signature change is needed. + +**Alternatives considered:** +- Making `build()` synchronous — not feasible, it performs API calls to fetch metadata. + +### 2. Constrain `mapPackageTo` generic to exclude Promise types + +**Change:** Add a type constraint to the `Input` parameter of `TransformationRepository.mapPackageTo` so that `Promise` is rejected at compile time. + +**Current signature:** +```typescript +mapPackageTo( + destination: number, + payload: Input, // accepts anything, including Promise + ... +): Output; +``` + +**Proposed signature:** +```typescript +mapPackageTo( + destination: number, + payload: MetadataPackage, + ... +): MetadataPackage; +``` + +**Rationale:** The generic type parameters `Input` and `Output` are only defaulted to `MetadataPackage` — they don't constrain it. Every call site in the codebase passes `MetadataPackage` in and expects `MetadataPackage` out, so the generics add no value and actively hide bugs. Removing them makes the interface honest about what it accepts. + +**Alternatives considered:** +- Adding a `NoPromise` conditional type constraint — overly clever, harder to understand. +- Keeping generics but adding `extends MetadataPackage` — same effect as removing them, but more verbose. + +### 3. Add unit test for CreatePackageUseCase + +**Rationale:** The use case currently has no unit tests, which allowed this regression to go undetected. A test that verifies the stored package `contents` is a resolved `MetadataPackage` (not a Promise) will catch this class of bug. + +## Risks / Trade-offs + +- **[Low] Removing generics from `mapPackageTo`/`mapPackageFrom`** → Any call site that relied on the generic flexibility will fail to compile. A codebase search confirms no call site uses non-default type arguments, so this risk is effectively zero. +- **[Low] Existing broken packages in dataStore** → Packages generated while the bug was active contain empty `contents`. Users need to regenerate these packages. No migration is needed since the storage format itself is correct; only the data was wrong. diff --git a/openspec/changes/fix-empty-module-package-contents/proposal.md b/openspec/changes/fix-empty-module-package-contents/proposal.md new file mode 100644 index 000000000..ba623b506 --- /dev/null +++ b/openspec/changes/fix-empty-module-package-contents/proposal.md @@ -0,0 +1,24 @@ +## Why + +When generating a package from a module, the package is saved with empty contents — the actual metadata objects (datasets, data elements, etc.) are missing. This causes four user-visible failures: imports report 0 elements, downloaded JSONs lack metadata, comparisons always show "ok", and the mapping tab is empty. The root cause is a missing `await` on an async call introduced during a refactor (commit fb983808), meaning a Promise object is stored instead of the resolved metadata payload. + +## What Changes + +- **Fix async/await bug in `CreatePackageUseCase`**: Add the missing `await` keyword when calling `metadataPayloadBuilder.build()` so the resolved `MetadataPackage` is stored instead of a Promise object. +- **Add type safety to `TransformationRepository.mapPackageTo`**: Tighten the generic type parameter to prevent `Promise` objects from being silently accepted as payload input, making this class of bug a compile-time error. + +## Capabilities + +### New Capabilities + +_(none — this is a bug fix, no new capabilities are introduced)_ + +### Modified Capabilities + +_(no spec-level requirement changes — the existing behavior was correct by design, only the implementation was broken)_ + +## Impact + +- **Code**: `src/domain/packages/usecases/CreatePackageUseCase.ts` (primary fix), `src/domain/transformations/repositories/TransformationRepository.ts` (type hardening) +- **User impact**: All module-generated packages will now contain their full metadata payload, restoring import, download, comparison, and mapping functionality +- **Risk**: Minimal — the fix aligns this call site with every other `metadataPayloadBuilder.build()` call in the codebase, all of which already use `await` diff --git a/openspec/changes/fix-empty-module-package-contents/specs/package-generation/spec.md b/openspec/changes/fix-empty-module-package-contents/specs/package-generation/spec.md new file mode 100644 index 000000000..3c40f14b0 --- /dev/null +++ b/openspec/changes/fix-empty-module-package-contents/specs/package-generation/spec.md @@ -0,0 +1,25 @@ +## ADDED Requirements + +### Requirement: Package generation SHALL resolve metadata payload before storage + +When `CreatePackageUseCase` generates a package from a module, it MUST await the `MetadataPayloadBuilder.build()` call so that the resolved `MetadataPackage` — not a Promise — is stored as the package contents. + +#### Scenario: Module package contains metadata objects after generation +- **WHEN** a user generates a package from a module that references a dataset +- **THEN** the stored package `contents` SHALL include the dataset and its dependent metadata objects + +#### Scenario: Package contents are serializable as valid metadata JSON +- **WHEN** a module package is generated and saved to the dataStore +- **THEN** the `contents` field SHALL be a plain object with metadata type keys (e.g., `dataSets`, `dataElements`), not a Promise or empty object + +### Requirement: TransformationRepository SHALL reject non-MetadataPackage payloads at compile time + +The `mapPackageTo` and `mapPackageFrom` methods MUST accept only `MetadataPackage` as the payload type, preventing Promise objects or other incorrect types from being passed silently. + +#### Scenario: Passing a Promise to mapPackageTo causes a compile error +- **WHEN** a developer passes `Promise` as the payload argument to `mapPackageTo` +- **THEN** the TypeScript compiler SHALL report a type error + +#### Scenario: Passing MetadataPackage to mapPackageTo compiles successfully +- **WHEN** a developer passes a resolved `MetadataPackage` as the payload argument to `mapPackageTo` +- **THEN** the TypeScript compiler SHALL accept the call without error diff --git a/openspec/changes/fix-empty-module-package-contents/tasks.md b/openspec/changes/fix-empty-module-package-contents/tasks.md new file mode 100644 index 000000000..293780248 --- /dev/null +++ b/openspec/changes/fix-empty-module-package-contents/tasks.md @@ -0,0 +1,18 @@ +## 1. Fix Missing Await in CreatePackageUseCase + +- [ ] 1.1 [BE] Add `await` before `this.metadataPayloadBuilder.build(syncBuilder)` in `src/domain/packages/usecases/CreatePackageUseCase.ts` line 41 + +## 2. Type Safety Improvement + +- [ ] 2.1 [BE] Remove generic type parameters from `mapPackageTo` and `mapPackageFrom` in `src/domain/transformations/repositories/TransformationRepository.ts`, replacing `Input`/`Output` with concrete `MetadataPackage` type +- [ ] 2.2 [BE] Update `TransformationRepositoryImpl` (data layer) to match the new non-generic signature +- [ ] 2.3 [BE] Verify all call sites compile cleanly with the tightened types (`yarn test` type-check) + +## 3. Unit Tests + +- [ ] 3.1 [BE] Create `CreatePackageUseCase.spec.ts` with a test that verifies the stored package `contents` is a resolved `MetadataPackage` (not a Promise or empty object) +- [ ] 3.2 [BE] Run `yarn test` to confirm all existing tests pass alongside the new test + +## 4. Validation + +- [ ] 4.1 [BE] Run full type-check and build: `yarn test` and `yarn build core-app` diff --git a/openspec/config.yaml b/openspec/config.yaml new file mode 100644 index 000000000..2ad61b30b --- /dev/null +++ b/openspec/config.yaml @@ -0,0 +1,204 @@ +schema: spec-driven + +context: | + Project: MetaData Sync (metadata-synchronization) + + What it is: + - DHIS2 Web Application for synchronizing metadata and data between DHIS2 instances + - Generates metadata packages for distribution across instances + - Supports scheduled synchronization via a server-side scheduler + - Multiple build variants for different deployment scenarios + - Manages metadata custodians (guardians) who approve sync requests + + Repository: EyeSeeTea/metadata-synchronization-blessed (GitHub) + + Key features: + - Metadata sync: push/pull metadata between DHIS2 instances with dependency resolution + - Data sync: transfer aggregated data and events between instances with automatic transformations + - Metadata packages: bundle metadata into modules, generate versioned packages, distribute via repositories + - Sync rules: define reusable synchronization configurations with schedules (cron) + - Metadata custodians: configure approval workflows for metadata changes + - Instance management: register and manage multiple DHIS2 instances with encrypted credentials + - Migration system: automatic dataStore schema migrations with versioning + - Sync history: audit trail of all synchronization operations + + Tech stack: + - Node.js + - TypeScript + - React (CRA via react-scripts, craco available for WDYR debugging) + - Material UI 4 (@material-ui/core, @material-ui/icons, @material-ui/lab) + - DHIS2 libraries: + - @dhis2/app-runtime (API hooks and data engine) + - @dhis2/d2-i18n (i18n) + - @dhis2/ui (DHIS2 design system) + - @eyeseetea/d2-api (typed DHIS2 API wrapper) + - @eyeseetea/d2-ui-components (shared UI components) + - @eyeseetea/feedback-component (user feedback widget) + - @monaco-editor/react (JSON viewer/editor for metadata) + - @octokit/rest (GitHub API for package distribution) + - cryptr (credential encryption) + - cronstrue (cron expression display) + - fluture (async functional programming) + - cmd-ts (CLI scripts) + - jszip (package bundling) + - lodash, axios, log4js + - Testing: Jest (unit, from CRA), Cypress (e2e) + - Husky + githooks for pre-commit checks + + Directory structure (Clean Architecture): + - src/ + - domain/ # Entities, use cases, repository interfaces + - data/ # Repository implementations, DHIS2 API calls + - presentation/ # React UI layer (or src/webapp/ — check actual structure) + - react/ # React components, pages, hooks + - models/ # Shared type definitions + - types/ # TypeScript declarations + - scheduler/ # Server-side sync scheduler (CLI entry point) + - migrations/ # DataStore schema migrations (versioned, auto-loaded) + - scripts/ # Utility CLI scripts + - compositionRoot.ts # Dependency injection wiring + + Build variants: + - yarn build all — builds all variants + - yarn build core-app — main synchronization app + - yarn build data-metadata-app — data + metadata sync focused + - yarn build module-package-app — module/package management + - yarn build modules-list — module listing + - yarn build package-exporter — package export tool + - yarn build msf-aggregate-data-app — MSF-specific aggregate data + - yarn build sp-emergency-responses — Samaritan's Purse emergency responses + + Server-side scheduler: + - yarn build-scheduler — builds scheduler as standalone Node.js server + - Runs via: node index.js -c app-config.json + - Requires app-config.json with encryptionKey, baseUrl, username, password + + Migrations: + - DataStore schema migrations in src/migrations/ + - Each migration defines old/new types (never rely on current app types, use unknown) + - *.ts files auto-loaded by version order + - CLI: yarn migrate 'http://admin:PASSWORD@localhost:8080' + - App checks dataStore version on startup, prompts user if migration needed + + Development setup: + - yarn start → dev server on localhost:8081, proxies DHIS2 requests to localhost:8080 + - yarn craco-start → dev server with why-did-you-render enabled + - yarn test → Jest unit tests + - Cypress e2e: requires eyeseetea/dhis2-data:2.30-datasync-sender docker instance + - Environment config via .env.local (copy from .env) + - public/app-config.json: must be created from app-config.template.json (encryptionKey) + - REACT_APP_DHIS2_BASE_URL: target DHIS2 instance + - REACT_APP_DHIS2_AUTH: optional USERNAME:PASSWORD override for proxy + + Code conventions: + - TypeScript strict mode + - Functional composition (fluture for async, lodash for data transforms) + - Clean Architecture: domain has no framework dependencies + - Naming: camelCase variables/functions, PascalCase React components + - i18n via @dhis2/d2-i18n, translations managed with yarn update-po / yarn localize + + Architecture: + - Clean Architecture — BUT significant technical debt exists + - Target layers: Presentation → Domain → Data + - Domain layer should have no framework dependencies, no DHIS2 imports + - Data layer implements domain repository interfaces using d2-api + - Presentation layer: React components should contain ZERO business logic + - CompositionRoot wires data implementations to domain interfaces + - IMPORTANT: Not all code follows Clean Architecture yet. Apply the boy scout principle: + every change must leave the code cleaner than it was found. + + Technical debt & boy scout principle: + - Many areas of the codebase predate the Clean Architecture adoption + - When modifying existing code, always refactor toward Clean Architecture + - Extract business logic from React components into use cases + - Move direct DHIS2 API calls into proper data/ repositories + - Add missing repository interfaces in domain/ for any data/ code you touch + - If you touch code that lacks tests, add tests as part of the change + - Document any technical debt you encounter but can't address in the current scope + + Git workflow (gitflow): + - Feature branches start from `development` and merge back to `development` + - If a feature depends on another unmerged feature, branch from that feature branch + and merge back to the same branch (not development), making the dependency explicit + - Branch naming: + - New features: feature/ + - Bug fixes: fix/ + - Commits follow Conventional Commits (https://www.conventionalcommits.org/en/v1.0.0/): + - feat: new feature + - fix: bug fix + - refactor: code change that neither fixes nor adds + - test: adding or updating tests + - docs: documentation changes + - chore: maintenance tasks + - Scope is encouraged: feat(sync): add retry logic for failed metadata pushes + + Extension principles: + - Maintain (and improve) Clean Architecture boundaries + - Domain logic must not depend on DHIS2 API specifics + - Encrypted credentials must never be logged or exposed + - Migration files must define self-contained old/new types (no importing from app) + - Preserve backward compatibility of dataStore schema (always provide migrations) + - Build variants must remain independently buildable + + Known constraints: + - Large metadata payloads can cause DHIS2 API timeouts + - Cross-version sync (e.g., 2.33 → 2.38) requires metadata transformations + - Encrypted credentials stored in dataStore via cryptr + - Tests require a specific Docker image (eyeseetea/dhis2-data:2.30-datasync-sender) + - Multiple build variants add complexity to changes that touch shared code + + Versioning: + - Semantic versioning + - Backward compatibility for dataStore schema (always migrate) + - Explicit change proposals required for sync behavior, migration schema, or package format changes + + Testing strategy: + - Unit tests (Jest) for domain logic, use cases, sync operations, transformations + - MANDATORY: every new feature must include unit tests + - MANDATORY: every modified function must have its tests updated or added (boy scout) + - Cypress e2e for user workflows (requires running DHIS2 Docker instance) + - Mock d2-api in unit tests using getMockApiFromClass pattern + + ClickUp project structure: + - Space: "Web Development" + - Folder: "Metadata & Data Sync" + - List: "Backlog" + +rules: + project: + - "Specs first: before modifying any behavior, check openspec/specs/ for existing specs. If behavior is unclear, read the spec. If no spec exists, propose one under changes/. Never change behavior without updating specs first." + - Follow Clean Architecture strictly in all new code: domain/ has no DHIS2 or React imports, data/ implements domain interfaces, presentation/ contains only UI. + - "Boy scout principle: every change must leave the code cleaner than found. When touching code that violates Clean Architecture, refactor it. When touching code without tests, add them. Document debt you can't fix now." + - "Test coverage is mandatory: every new feature must include unit tests. Every modified function must have tests added or updated. No exceptions." + - Do not introduce new direct DHIS2 API calls outside of data/; always go through repository interfaces defined in domain/. + - Follow code conventions: TypeScript strict, functional composition, correct naming (camelCase/PascalCase). + - "Migrations: when changing dataStore schema, always create a new migration in src/migrations/. Define old/new types locally in the migration file — never import from app types. Use unknown for unrelated fields." + - "Do NOT: change sync behavior without a spec + proposal, modify dataStore schema without a migration, introduce direct DHIS2 API calls in domain/ or presentation/, bypass the CompositionRoot, log or expose encrypted credentials." + - "Definition of Done: specs updated, code aligns with specs, all tests pass (new + existing), Clean Architecture boundaries respected (or improved), migrations included if dataStore changes, no breaking changes without proposal." + - "Clean Architecture: domain layer must have no framework dependencies (no React, no DHIS2 imports). React components must contain zero business logic — they only render state and forward events. All logic goes in use cases or the composition root." + - "Git: always branch from `development` unless the feature depends on an unmerged branch, in which case branch from that branch and merge back to it. Use feature/ for features, fix/ for bugs. All commits must follow Conventional Commits." + - "When modifying shared code used by multiple build variants, verify the change doesn't break any variant. At minimum, check that yarn build all still succeeds." + + proposal: + - Must include: Goals, Non-goals, User impact, Compatibility (sync behavior + dataStore schema + package format), and Rollback plan. + - If changing sync behavior, explicitly describe old vs new behavior for both metadata and data sync. + - If changing dataStore schema, include the migration strategy and describe backward compatibility. + - If the change affects build variants, list which variants are impacted. + + design: + - Must describe: which DHIS2 metadata types are involved, how data flows through domain → data → presentation, and which sync operations are affected. + - For sync-related changes, describe error handling, retry strategy, and how partial failures are managed. + - For package-related changes, describe impact on package format and distribution. + - Note known constraints (API timeouts, cross-version compat, large payloads) when relevant. + - "Boy scout: identify any technical debt in the affected area and include at least one cleanup task." + + tasks: + - Include at least one task for unit tests (Jest) for any domain logic or sync behavior changes. + - Include at least one task for Cypress e2e tests when user-facing workflows change. + - Include a task to update specs/ domain docs whenever behavior changes. + - "Include a boy scout task for any refactoring identified during design (e.g., 'Extract sync logic from component X into use case')." + - "Tag each task with the responsible agent role: [FE], [BE]." + + specs: + - Specs are source-of-truth; code must align to specs before merge. + - Any change that affects sync behavior, dataStore schema, package format, or migration logic must come with delta specs under changes//specs/. diff --git a/src/data/transformations/TransformationD2ApiRepository.ts b/src/data/transformations/TransformationD2ApiRepository.ts index b294fd437..eea1534f9 100644 --- a/src/data/transformations/TransformationD2ApiRepository.ts +++ b/src/data/transformations/TransformationD2ApiRepository.ts @@ -50,9 +50,7 @@ export class TransformationD2ApiRepository implements TransformationRepository { ); if (transformationstoApply.length > 0) { - return this.cleanTransformationObject( - this.undoTransformations(payload, transformationstoApply) - ); + return this.cleanTransformationObject(this.undoTransformations(payload, transformationstoApply)); } else { return payload; } diff --git a/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts b/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts index fd695e016..2666a251e 100644 --- a/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts +++ b/src/domain/packages/usecases/__tests__/CreatePackageUseCase.spec.ts @@ -61,22 +61,16 @@ describe("CreatePackageUseCase", () => { repositoryFactory = new DynamicRepositoryFactory(); - repositoryFactory.bindByInstance( - "configRepository", - (_instance: Instance) => ({ - getStorageClient: vi.fn(), - getUserStorageClient: vi.fn(), - changeStorageClient: vi.fn(), - getStorageClientPromise: vi.fn().mockResolvedValue(mockStorageClient), - }), - ); - - repositoryFactory.bindByInstance( - "userRepository", - (_instance: Instance) => ({ - getCurrent: vi.fn().mockResolvedValue(testUser), - }), - ); + repositoryFactory.bindByInstance("configRepository", (_instance: Instance) => ({ + getStorageClient: vi.fn(), + getUserStorageClient: vi.fn(), + changeStorageClient: vi.fn(), + getStorageClientPromise: vi.fn().mockResolvedValue(mockStorageClient), + })); + + repositoryFactory.bindByInstance("userRepository", (_instance: Instance) => ({ + getCurrent: vi.fn().mockResolvedValue(testUser), + })); }); function buildTestModule(overrides?: Partial): MetadataModule { @@ -119,7 +113,7 @@ describe("CreatePackageUseCase", () => { metadataPayloadBuilder as unknown as MetadataPayloadBuilder, repositoryFactory, transformationRepository, - localInstance, + localInstance ); const validations = await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); @@ -145,7 +139,7 @@ describe("CreatePackageUseCase", () => { metadataPayloadBuilder as unknown as MetadataPayloadBuilder, repositoryFactory, transformationRepository, - localInstance, + localInstance ); await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); @@ -173,7 +167,7 @@ describe("CreatePackageUseCase", () => { metadataPayloadBuilder as unknown as MetadataPayloadBuilder, repositoryFactory, transformationRepository, - localInstance, + localInstance ); const validations = await useCase.execute("LOCAL", sourcePackage, module, dhisVersion, preSuppliedContents); @@ -199,17 +193,13 @@ describe("CreatePackageUseCase", () => { metadataPayloadBuilder as unknown as MetadataPayloadBuilder, repositoryFactory, transformationRepository, - localInstance, + localInstance ); await useCase.execute("LOCAL", sourcePackage, module, dhisVersion); expect(transformationRepository.mapPackageTo).toHaveBeenCalledTimes(1); - expect(transformationRepository.mapPackageTo).toHaveBeenCalledWith( - 38, - builtPayload, - expect.any(Array), - ); + expect(transformationRepository.mapPackageTo).toHaveBeenCalledWith(38, builtPayload, expect.any(Array)); }); it("should save the module with updated lastPackageVersion", async () => { @@ -221,7 +211,7 @@ describe("CreatePackageUseCase", () => { metadataPayloadBuilder as unknown as MetadataPayloadBuilder, repositoryFactory, transformationRepository, - localInstance, + localInstance ); await useCase.execute("LOCAL", sourcePackage, module, dhisVersion);