From b7f8a436fba108b9bb58e75c0d52bd5851cb5e53 Mon Sep 17 00:00:00 2001 From: Nathnael Date: Mon, 24 Aug 2026 08:01:12 +0000 Subject: [PATCH] fix(reports): filter the plan side of plan-versus-actual reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cargo filter was applied only to the operated and attainment subqueries. The plan side selected targets by metric and dimension alone, and the FULL OUTER JOIN put every filtered-out key back as a row of zeros — ?categories=FERTILIZER returned all ten planned categories. planKeyFilter restricts targets to the selected categories (or container classes), reading ot.cargo_category for a station plan and ot.dimension_key otherwise. Values are whitelisted against the vocabulary and inlined, because the fragment is assembled into raw CTE text and the runner does not validate multiselect values. Two related grain leaks close with it: - planCountryFilter narrows a station plan to the chosen country. All seven station targets are Ethiopian, so the Djibouti view was listing 33 Ethiopian targets as stations that moved nothing. - planGrainFilter drops the plan entirely when origin, destination, train number or direction is set. No target carries a route, so the plan there was the whole corridor's target sitting beside one slice of its work, and the implement rate read as a miss that never happened. Fixes cargo-volume-performance, cargo-volume-by-station, trainset-performance and teu-performance together. Co-Authored-By: Claude Opus 5 --- .../reports/operations-classification.spec.ts | 71 +++++++++++++++++++ .../reports/operations-classification.ts | 61 +++++++++++++++- 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/apps/edr-freight-api/src/modules/reports/operations-classification.spec.ts b/apps/edr-freight-api/src/modules/reports/operations-classification.spec.ts index 63a7db20b..a37bc3f0c 100644 --- a/apps/edr-freight-api/src/modules/reports/operations-classification.spec.ts +++ b/apps/edr-freight-api/src/modules/reports/operations-classification.spec.ts @@ -7,6 +7,7 @@ import { TARGET_DIMENSION_KEYS, cycleRateExpr, implementRateExpr, + plannedRowsSql, } from './operations-classification'; import { TARGET_DIMENSIONS, TARGET_METRICS } from '../operations-reporting/entities/operations-target.entity'; @@ -91,4 +92,74 @@ describe('operations classification', () => { expect(rate(65, 78)).toBeLessThan(100); expect(cycleRateExpr('ad', 'sc')).toContain('NULLIF(sc, 0)'); }); + + /** + * The plan side is FULL OUTER JOINed to the operated side, so a plan row for + * a category the user filtered out comes back as a row of zeros — the bug + * where `?categories=FERTILIZER` still returned all ten planned categories. + */ + describe('plannedRowsSql cargo filter', () => { + const sqlFor = (dimension: string, params: Record): string => + plannedRowsSql('VOLUME_TONS', dimension, params, 'SELECT 1'); + + it('restricts targets to the selected categories', () => { + expect(sqlFor('cargo_category', { categories: ['FERTILIZER'] })).toContain( + "AND ot.dimension_key IN ('FERTILIZER')", + ); + }); + + it('restricts a station target on its cargo type, not its key', () => { + const sql = sqlFor('station', { categories: ['SAND'] }); + expect(sql).toContain("AND ot.cargo_category IN ('SAND')"); + expect(sql).not.toContain('ot.dimension_key IN'); + }); + + it('filters a container class report on its own vocabulary', () => { + expect(sqlFor('container_class', { classes: ['CONTAINER_EXPORT'] })).toContain( + "AND ot.dimension_key IN ('CONTAINER_EXPORT')", + ); + }); + + it('leaves every target when nothing is selected', () => { + expect(sqlFor('cargo_category', {})).not.toContain('ot.dimension_key IN'); + }); + + it('matches nothing on a value no category expression can emit', () => { + expect(sqlFor('cargo_category', { categories: ["x'; DROP TABLE"] })).toContain('AND FALSE'); + }); + + it('narrows a station plan to the chosen country rather than suppressing it', () => { + const sql = sqlFor('station', { country: 'Djibouti' }); + expect(sql).toContain("y.country = 'Djibouti'"); + expect(sql).not.toContain('AND FALSE'); + }); + + it('ignores a country that is not one of the two sides', () => { + expect(sqlFor('station', { country: "' OR true --" })).not.toContain('y.country'); + }); + + it('leaves the country alone on a plan not keyed by station', () => { + expect(sqlFor('cargo_category', { country: 'Djibouti' })).not.toContain('y.country'); + }); + + /** + * No target carries a route, a train or a direction, so beside a + * route-filtered actual the plan would be the whole corridor's target. + */ + it.each(['origin', 'destination', 'trainNumber', 'direction'])( + 'reports no plan at all when %s narrows below the target grain', + (key) => { + expect(sqlFor('cargo_category', { [key]: 'X' })).toContain('AND FALSE'); + }, + ); + + it('keeps the plan when only period, date and category are set', () => { + const sql = sqlFor('cargo_category', { + period: 'month', + dateFrom: '2026-01-01', + categories: ['SAND'], + }); + expect(sql).not.toContain('AND FALSE'); + }); + }); }); diff --git a/apps/edr-freight-api/src/modules/reports/operations-classification.ts b/apps/edr-freight-api/src/modules/reports/operations-classification.ts index 1c8f8504c..cffb787ee 100644 --- a/apps/edr-freight-api/src/modules/reports/operations-classification.ts +++ b/apps/edr-freight-api/src/modules/reports/operations-classification.ts @@ -513,7 +513,9 @@ export const PLAN_GRANULARITY_NOTE = 'Plan is the committed figure and never moves. Required is the same target treated as a ' + 'quota: whatever is still outstanding, spread across the time still left, so a period ' + 'that fell behind raises what the periods after it must carry. A target already met in ' + - 'full requires nothing further.'; + 'full requires nothing further. No target carries a route, a train or a direction, so ' + + 'filtering by one leaves the plan columns empty rather than comparing a corridor’s whole ' + + 'target against one slice of its work.'; /** * The user's date filter as open-ended bounds, so the clipping arithmetic below @@ -575,6 +577,60 @@ END`; * Period bounds ride on `:planFrom` / `:planTo`, which the caller must bind * with {@link plannedRowsParams} — they come from the user's date filter. */ +/** + * The plan side of a plan-versus-actual report has to obey the same cargo + * filter the operated side does. Without it the FULL OUTER JOIN re-introduces + * every planned key the user filtered out, as a row of zeros. + * + * Values are whitelisted against the vocabulary and inlined rather than bound: + * this fragment is assembled into raw CTE text, and the filter params are not + * validated upstream. An unknown value matches nothing — same as it does on the + * operated side, where the CASE can never emit it. + */ +const planKeyFilter = (dimension: string, params: Record): string => { + const isClass = dimension === 'container_class'; + const selected = (isClass ? params.classes : params.categories) as string[] | null; + if (!selected?.length) return ''; + const vocab = isClass ? CONTAINER_CLASSES : CARGO_CATEGORIES; + const valid = selected.filter((v) => vocab.some((o) => o.value === v)); + // A station target is keyed on the yard and carries its cargo type alongside. + const column = dimension === 'station' ? 'ot.cargo_category' : 'ot.dimension_key'; + return valid.length ? `AND ${column} IN (${quote(valid)})` : 'AND FALSE'; +}; + +/** + * Filters that narrow the operated population below the grain any target is + * kept at. No target carries a route, a train or a direction, so a plan read + * beside a route-filtered actual is the whole corridor's plan sitting next to + * one slice of its work — the implement rate then reads as a miss that never + * happened. + * + * There is no honest number to show, so the plan side reports nothing at all + * and Implement Rate goes NULL, the same way it does for a period with no + * target. `country` is absent on purpose: on a station plan it is a property of + * the planned key itself, and {@link planCountryFilter} narrows rather than + * suppresses. + */ +const PLAN_GRAIN_BREAKERS = ['origin', 'destination', 'trainNumber', 'direction']; + +const planGrainFilter = (params: Record): string => + PLAN_GRAIN_BREAKERS.some((key) => params[key]) ? 'AND FALSE' : ''; + +/** + * A station target is keyed on a yard code, so the country filter — which + * decides which end of the corridor the report calls "the station" — is a real + * predicate on the plan, not a grain break. Without it the Djibouti view lists + * every Ethiopian station's target as a row that moved nothing. + */ +const planCountryFilter = (dimension: string, params: Record): string => { + if (dimension !== 'station') return ''; + const country = COUNTRY_FILTER.options?.find((o) => o.value === params.country)?.value; + if (!country) return ''; + return `AND EXISTS (SELECT 1 FROM freight.yards y + WHERE y.code = ot.dimension_key AND y.deleted_at IS NULL + AND y.country = '${country}')`; +}; + export const plannedRowsSql = ( metric: string, dimension: string, @@ -597,6 +653,9 @@ export const plannedRowsSql = ( AND ot.metric = '${metric}' AND ot.dimension = '${dimension}' AND ot.planned_value > 0 + ${planKeyFilter(dimension, params)} + ${planCountryFilter(dimension, params)} + ${planGrainFilter(params)} ), -- One row per target per bucket. Generated a day at a time rather than a -- bucket at a time: the ragged units restart their blocks each January, so