diff --git a/apps/edr-freight-api/src/modules/operations-reporting/dto/create-operations-target.dto.ts b/apps/edr-freight-api/src/modules/operations-reporting/dto/create-operations-target.dto.ts index 2d8f5beb0..07c380cda 100644 --- a/apps/edr-freight-api/src/modules/operations-reporting/dto/create-operations-target.dto.ts +++ b/apps/edr-freight-api/src/modules/operations-reporting/dto/create-operations-target.dto.ts @@ -58,13 +58,18 @@ export class CreateOperationsTargetDto { @ApiPropertyOptional({ description: - 'Station targets only: which cargo category this station plan covers. Leave blank for the other dimensions.', + 'Station targets only: which cargo category this station plan covers. Ignored for the ' + + 'other dimensions, whose key already carries the category.', example: 'CONTAINER_IMPORT_MULTIMODAL', }) @IsOptional() + // `'' ?? null` is `''`, and an empty string matches neither the unique + // index's `COALESCE(cargo_category, '')` nor the report's join — it reads as + // a category that does not exist. Blank means absent. + @Transform(({ value }) => (value === '' ? null : value)) @IsString() @MaxLength(60) - cargoCategory?: string; + cargoCategory?: string | null; @ApiPropertyOptional() @IsOptional() diff --git a/apps/edr-freight-api/src/modules/operations-reporting/operations-reporting.module.ts b/apps/edr-freight-api/src/modules/operations-reporting/operations-reporting.module.ts index 180c12271..e2da0eb30 100644 --- a/apps/edr-freight-api/src/modules/operations-reporting/operations-reporting.module.ts +++ b/apps/edr-freight-api/src/modules/operations-reporting/operations-reporting.module.ts @@ -1,4 +1,4 @@ -import { Global, Module } from '@nestjs/common'; +import { Module } from '@nestjs/common'; import { TypeOrmModule } from '@nestjs/typeorm'; import { OperationsStandard } from './entities/operations-standard.entity'; @@ -13,10 +13,11 @@ import { OperationsTargetsService } from './operations-targets.service'; * standards (one settings row) and the planned targets the reports compare * actuals against. * - * Global because the reports module reads the standards row on every run and - * has no other reason to import this. + * Not global, and deliberately so: nothing outside this module injects either + * service. The reports read both tables in raw SQL — `STANDARDS_JOIN` and + * `plannedRowsSql` in `reports/operations-classification.ts` — so the exports + * below are for future callers, not current ones. */ -@Global() @Module({ imports: [TypeOrmModule.forFeature([OperationsStandard, OperationsTarget])], controllers: [OperationsStandardsController, OperationsTargetsController], diff --git a/apps/edr-freight-api/src/modules/operations-reporting/operations-targets.service.ts b/apps/edr-freight-api/src/modules/operations-reporting/operations-targets.service.ts index c33054aa5..b3753e343 100644 --- a/apps/edr-freight-api/src/modules/operations-reporting/operations-targets.service.ts +++ b/apps/edr-freight-api/src/modules/operations-reporting/operations-targets.service.ts @@ -1,5 +1,10 @@ import { PaginatedResponse } from '@edr/types'; -import { ConflictException, Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + ConflictException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Brackets, IsNull, Repository } from 'typeorm'; @@ -12,6 +17,8 @@ import { TARGET_DIMENSION_LABELS, TARGET_METRIC_LABELS, TARGET_PERIOD_LABELS, + TargetDimension, + TargetMetric, TargetPeriodType, } from './entities/operations-target.entity'; import { @@ -76,6 +83,27 @@ const LABELS_BY_DIMENSION: Record> = { const CARGO_CATEGORY_LABELS = LABELS_BY_DIMENSION.cargo_category; +/** + * The keys a target may be stored against, per dimension. A report matches a + * target by this exact string, so a key outside the set here is a plan no + * report can ever find — and nothing downstream would ever say so. `station` is + * absent on purpose: yard codes are admin-managed rows, resolved live. + * + * `UNCLASSIFIED` is accepted for `cargo_category` even though the admin form + * does not offer it, because `CARGO_CATEGORY_EXPR` does emit it — rejecting a + * key the reports can match would be stricter than the reports themselves. + */ +const KEYS_BY_DIMENSION: Record, Set> = { + cargo_category: new Set(CARGO_CATEGORIES.map((o) => o.value)), + container_class: new Set(CONTAINER_CLASSES.map((o) => o.value)), +}; + +/** The columns that decide which report row a target lines up with. */ +type TargetSlot = Pick< + OperationsTarget, + 'periodType' | 'periodStart' | 'metric' | 'dimension' | 'dimensionKey' | 'cargoCategory' +>; + @Injectable() export class OperationsTargetsService { constructor( @@ -152,35 +180,113 @@ export class OperationsTargetsService { } async create(dto: CreateOperationsTargetDto): Promise { - const periodStart = normalisePeriodStart(dto.periodType, dto.periodStart); - const cargoCategory = dto.cargoCategory ?? null; - await this.assertSlotFree({ ...dto, periodStart, cargoCategory }); - return this.repository.save(this.repository.create({ ...dto, periodStart, cargoCategory })); + const slot = await this.resolveSlot(dto); + await this.assertSlotFree(slot); + return this.repository.save(this.repository.create({ ...dto, ...slot })); } async update(id: string, dto: UpdateOperationsTargetDto): Promise { const current = await this.findById(id); - const periodType = dto.periodType ?? current.periodType; - const periodStart = normalisePeriodStart(periodType, dto.periodStart ?? current.periodStart); - const next = { - periodType, - periodStart, + const slot = await this.resolveSlot({ + periodType: dto.periodType ?? current.periodType, + periodStart: dto.periodStart ?? current.periodStart, metric: dto.metric ?? current.metric, dimension: dto.dimension ?? current.dimension, dimensionKey: dto.dimensionKey ?? current.dimensionKey, + // An absent key means "unchanged" only while the dimension still wants a + // category at all — `resolveSlot` drops it when the dimension no longer + // does, which is the whole point of routing both paths through it. cargoCategory: - dto.cargoCategory !== undefined ? (dto.cargoCategory ?? null) : current.cargoCategory ?? null, - }; - await this.assertSlotFree(next, id); + dto.cargoCategory !== undefined ? dto.cargoCategory : current.cargoCategory, + }); + await this.assertSlotFree(slot, id); await this.repository.update(id, { - ...next, + ...slot, ...(dto.plannedValue != null ? { plannedValue: dto.plannedValue } : {}), ...(dto.note !== undefined ? { note: dto.note } : {}), }); return this.findById(id); } + /** + * Everything that decides which report row a target lines up with, resolved + * in one place so `create` and `update` cannot drift apart. + * + * `cargoCategory` is **derived from the dimension, never carried over**. A + * station's plan is per station AND per cargo type; the other two dimensions + * already carry the category in `dimensionKey`. A stale category left on a + * row whose dimension has moved on is not cosmetic — it survives the + * `COALESCE(cargo_category, '')` unique index alongside the legitimate + * null-category row, `plannedRowsSql` groups by it, and the two plan rows + * then both join the same operated row: the category lists twice, each time + * carrying the full operated tonnage, while the summary tiles stay correct. + */ + private async resolveSlot(input: { + periodType: TargetPeriodType; + periodStart: string; + metric: TargetMetric; + dimension: TargetDimension; + dimensionKey: string; + cargoCategory?: string | null; + }): Promise { + const periodStart = normalisePeriodStart(input.periodType, input.periodStart); + await this.assertDimensionKey(input.dimension, input.dimensionKey); + + const base = { + periodType: input.periodType, + periodStart, + metric: input.metric, + dimension: input.dimension, + dimensionKey: input.dimensionKey, + }; + + if (input.dimension !== 'station') { + return { ...base, cargoCategory: null }; + } + + const cargoCategory = input.cargoCategory || null; + if (!cargoCategory) { + throw new BadRequestException( + 'A station target needs a cargo category — the plan is per station and per cargo type. ' + + 'Without one the report has nothing to match it against.', + ); + } + if (!KEYS_BY_DIMENSION.cargo_category.has(cargoCategory)) { + throw new BadRequestException( + `"${cargoCategory}" is not a cargo category the reports produce. ` + + `Expected one of: ${[...KEYS_BY_DIMENSION.cargo_category].join(', ')}`, + ); + } + return { ...base, cargoCategory }; + } + + /** + * A `dimensionKey` the reports never emit is a plan that silently never + * joins — the row lists fine and its label falls back to the raw key, so + * nothing downstream ever reports the mistake. Cheaper to reject on write. + */ + private async assertDimensionKey(dimension: TargetDimension, key: string): Promise { + if (dimension === 'station') { + const yards = await this.yardLabels(); + if (!yards.has(key)) { + throw new BadRequestException( + `"${key}" is not a known station code. A station target is keyed on ` + + '`yards.code`, which is what the reports match against.', + ); + } + return; + } + + const allowed = KEYS_BY_DIMENSION[dimension]; + if (!allowed.has(key)) { + throw new BadRequestException( + `"${key}" is not a ${TARGET_DIMENSION_LABELS[dimension].toLowerCase()} the reports ` + + `produce. Expected one of: ${[...allowed].join(', ')}`, + ); + } + } + async remove(id: string): Promise { await this.findById(id); await this.repository.softDelete(id); diff --git a/apps/edr-freight-web/backoffice/src/pages/ruleEngine/config/resources.ts b/apps/edr-freight-web/backoffice/src/pages/ruleEngine/config/resources.ts index cc899322a..e95724912 100644 --- a/apps/edr-freight-web/backoffice/src/pages/ruleEngine/config/resources.ts +++ b/apps/edr-freight-web/backoffice/src/pages/ruleEngine/config/resources.ts @@ -149,6 +149,12 @@ const TRADE_DIRECTIONS = [ * a report matches a target by this exact key, so a value here that the API * does not emit is a plan the report will never find. The API spec * `operations-classification.spec.ts` guards the API side of the pair. + * + * Drift is no longer silent: `OperationsTargetsService.assertDimensionKey` + * rejects any key outside the API's own vocabulary, so a stale entry here + * surfaces as a 400 on save rather than a plan that quietly never joins. + * `UNCLASSIFIED` is left out deliberately — the API accepts it, but there is no + * sense in planning against cargo nobody has classified. */ export const OPERATIONS_CARGO_CATEGORIES = [ { label: "Multimodal container import", value: "CONTAINER_IMPORT_MULTIMODAL" },