From a700601038bc0131be4ee1072f90bfae3afb9419 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Sun, 23 Aug 2026 10:39:23 -0500 Subject: [PATCH] Fix rule history startDate filter dropping other rules' versions (#1056) * Fix rule history startDate filter dropping other rules' versions * address comments and fix e2e tests * fix test per coderabbit --- .../reportingRuleHistoryHelpers.test.ts | 56 +++++++++++++++++++ .../reportingRuleHistoryHelpers.ts | 31 +++++----- .../getCurrentPeriodRuleAlarmStatuses.test.ts | 41 ++++++++++++++ .../getCurrentPeriodRuleAlarmStatuses.ts | 29 +++++++--- .../ruleHistoryService.test.ts | 26 +++++++++ .../ruleHistoryService/ruleHistoryService.ts | 31 +++++----- 6 files changed, 179 insertions(+), 35 deletions(-) create mode 100644 server/services/reportingService/reportingRuleHistoryHelpers.test.ts create mode 100644 server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.test.ts diff --git a/server/services/reportingService/reportingRuleHistoryHelpers.test.ts b/server/services/reportingService/reportingRuleHistoryHelpers.test.ts new file mode 100644 index 0000000..e732696 --- /dev/null +++ b/server/services/reportingService/reportingRuleHistoryHelpers.test.ts @@ -0,0 +1,56 @@ +import { getSimplifiedRuleHistory } from './reportingRuleHistoryHelpers.js'; + +describe('reporting getSimplifiedRuleHistory', () => { + test('filters startDate per rule and returns retained versions chronologically', async () => { + const mockGetRawHistory = async () => [ + { + id: 'old-seed', + name: 'Old seed', + exactVersion: '2025-12-09 15:40:32.761966+00', + }, + { + id: 'new-rule', + name: 'New rule', + exactVersion: '2026-08-10 00:00:00.000000+00', + }, + { + id: 'old-seed', + name: 'Updated seed', + exactVersion: '2026-08-20 00:00:00.000000+00', + }, + { + id: 'new-rule', + name: 'Updated new rule', + exactVersion: '2026-08-22 00:00:00.000000+00', + }, + ]; + + const result = await getSimplifiedRuleHistory( + mockGetRawHistory, + ['name'], + undefined, + new Date('2026-08-15T00:00:00.000Z'), + ); + + expect( + result.map(({ id, exactVersion }) => ({ id, exactVersion })), + ).toEqual([ + { + id: 'old-seed', + exactVersion: '2025-12-09 15:40:32.761966+00', + }, + { + id: 'new-rule', + exactVersion: '2026-08-10 00:00:00.000000+00', + }, + { + id: 'old-seed', + exactVersion: '2026-08-20 00:00:00.000000+00', + }, + { + id: 'new-rule', + exactVersion: '2026-08-22 00:00:00.000000+00', + }, + ]); + }); +}); diff --git a/server/services/reportingService/reportingRuleHistoryHelpers.ts b/server/services/reportingService/reportingRuleHistoryHelpers.ts index 85cc288..23b55ef 100644 --- a/server/services/reportingService/reportingRuleHistoryHelpers.ts +++ b/server/services/reportingService/reportingRuleHistoryHelpers.ts @@ -96,21 +96,24 @@ export async function getSimplifiedRuleHistory( approxVersion: new Date(it.exactVersion), })); - return startDate - ? allVersions.filter( + if (!startDate) { + return allVersions; + } + + // "Version in effect at startDate, plus later versions" is per rule. The + // raw list is every rule concatenated; applying the neighbor check globally + // drops a rule whose next *row* belongs to a different, older rule. + return Object.values(_.groupBy(allVersions, (it) => it.id)) + .flatMap((versions) => { + const ordered = [...versions].sort( + (a, b) => a.approxVersion.getTime() - b.approxVersion.getTime(), + ); + return ordered.filter( (_, i) => - // Always keep the last version (which is what we're looking at if - // i == allVersions.length - 1), because that's definitely still - // in effect at the start date. Then, if we're not looking at the - // last version, keep this version if the _next_ version was - // created after the start date (which'd mean this was the version - // in effect _at_ the start date). The overall result here is to - // keep last version created at or before the start date, plus all - // created after it. - i === allVersions.length - 1 || - allVersions[i + 1].approxVersion > startDate, - ) - : allVersions; + i === ordered.length - 1 || ordered[i + 1].approxVersion > startDate, + ); + }) + .sort((a, b) => a.approxVersion.getTime() - b.approxVersion.getTime()); } /** diff --git a/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.test.ts b/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.test.ts new file mode 100644 index 0000000..647d95e --- /dev/null +++ b/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.test.ts @@ -0,0 +1,41 @@ +import { RuleAlarmStatus } from '../moderationConfigService/index.js'; +import makeGetCurrentPeriodRuleAlarmStatuses from './getCurrentPeriodRuleAlarmStatuses.js'; + +describe('getCurrentPeriodRuleAlarmStatuses', () => { + test('returns INSUFFICIENT_DATA when rule history has no matching entry', async () => { + const ruleId = 'rule-without-history'; + const ruleVersion = new Date('2026-08-22T00:00:00.000Z'); + const currentPeriod = { + ruleId, + approxRuleVersion: ruleVersion, + windowStart: new Date('2026-08-22T00:00:00.000Z'), + passCount: 50, + passingUsersCount: 50, + runsCount: 1000, + }; + const historicalPeriods = Array.from({ length: 25 }, (_, index) => ({ + ruleId, + approxRuleVersion: ruleVersion, + windowStart: new Date( + Date.parse('2026-08-22T00:00:00.000Z') - (index + 1) * 60 * 60 * 1000, + ), + passCount: index < 4 ? 1 : 0, + passingUsersCount: index < 4 ? 1 : 0, + runsCount: 200, + })); + const getStatuses = makeGetCurrentPeriodRuleAlarmStatuses( + async () => [currentPeriod, ...historicalPeriods], + async () => [], + ); + + await expect(getStatuses()).resolves.toEqual({ + [ruleId]: { + status: RuleAlarmStatus.INSUFFICIENT_DATA, + meta: { + lastPeriodPassRate: undefined, + secondToLastPeriodPassRate: undefined, + }, + }, + }); + }); +}); diff --git a/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.ts b/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.ts index e8f23e4..5697110 100644 --- a/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.ts +++ b/server/services/ruleAnomalyDetectionService/getCurrentPeriodRuleAlarmStatuses.ts @@ -3,7 +3,7 @@ import lodash from 'lodash'; import { type Dependencies } from '../../iocContainer/index.js'; import { inject } from '../../iocContainer/utils.js'; import { WEEK_MS } from '../../utils/time.js'; -import { type RuleAlarmStatus } from '../moderationConfigService/index.js'; +import { RuleAlarmStatus } from '../moderationConfigService/index.js'; import getRuleAlarmStatus from './getRuleAlarmStatus.js'; const { mapValues, groupBy } = lodash; @@ -18,11 +18,12 @@ const makeGetCurrentPeriodRuleAlarmStatuses = inject( const passStats = await getRuleStats({ startTime: oneWeekAgo }); const statsByRule = groupBy(passStats, (it) => it.ruleId); - const minVersionsByRule = await getMinimumAnomalyDetectionRuleVersions( - getRuleHistory, - undefined, - oneWeekAgo, - ); + const minVersionsByRule: Partial> = + await getMinimumAnomalyDetectionRuleVersions( + getRuleHistory, + undefined, + oneWeekAgo, + ); return mapValues(statsByRule, (passStats) => { const { ruleId } = passStats[0]; @@ -50,8 +51,22 @@ const makeGetCurrentPeriodRuleAlarmStatuses = inject( // model, we represent the number of passes _not_ as the number of times // that the rule passed in a period, but rather as the number of // distinct users that caused the rule to pass in that period. + const minVersion = minVersionsByRule[ruleId]; + // Without a version bound we cannot tell whether warehouse stats + // predate a condition/item-type change. Fail closed rather than + // treating missing history as "all stats apply". + if (minVersion === undefined) { + return { + status: RuleAlarmStatus.INSUFFICIENT_DATA, + meta: { + lastPeriodPassRate: undefined, + secondToLastPeriodPassRate: undefined, + }, + }; + } + const applicableStats = passStats - .filter((it) => it.approxRuleVersion >= minVersionsByRule[ruleId]) + .filter((it) => it.approxRuleVersion >= minVersion) .map((it) => ({ passes: it.passingUsersCount, runs: it.runsCount })); return { diff --git a/server/services/ruleHistoryService/ruleHistoryService.test.ts b/server/services/ruleHistoryService/ruleHistoryService.test.ts index 8be3540..8472b7a 100644 --- a/server/services/ruleHistoryService/ruleHistoryService.test.ts +++ b/server/services/ruleHistoryService/ruleHistoryService.test.ts @@ -111,5 +111,31 @@ describe('RuleHistory Service', () => { }, ]); }); + + test('startDate filter does not drop a newer rule when an older rule follows it in the list', async () => { + const mockGetRawHistory = async () => [ + { + id: 'new-rule', + name: 'Anomaly test', + statusIfUnexpired: RuleStatus.LIVE, + exactVersion: '2026-08-22 21:59:40.059097+00', + }, + { + id: 'old-seed', + name: 'Seed', + statusIfUnexpired: RuleStatus.LIVE, + exactVersion: '2025-12-09 15:40:32.761966+00', + }, + ]; + + const res = await getSimplifiedRuleHistory( + mockGetRawHistory, + ['name', 'statusIfUnexpired'], + undefined, + new Date('2026-08-15T00:00:00.000Z'), + ); + + expect(res.map((row) => row.id)).toEqual(['old-seed', 'new-rule']); + }); }); }); diff --git a/server/services/ruleHistoryService/ruleHistoryService.ts b/server/services/ruleHistoryService/ruleHistoryService.ts index 95378c7..68eb92a 100644 --- a/server/services/ruleHistoryService/ruleHistoryService.ts +++ b/server/services/ruleHistoryService/ruleHistoryService.ts @@ -135,21 +135,24 @@ export async function getSimplifiedRuleHistory( approxVersion: new Date(it.exactVersion), })); - return startDate - ? allVersions.filter( + if (!startDate) { + return allVersions; + } + + // "Version in effect at startDate, plus later versions" is per rule. The + // raw list is every rule concatenated; applying the neighbor check globally + // drops a rule whose next *row* belongs to a different, older rule. + return Object.values(_.groupBy(allVersions, (it) => it.id)) + .flatMap((versions) => { + const ordered = [...versions].sort( + (a, b) => a.approxVersion.getTime() - b.approxVersion.getTime(), + ); + return ordered.filter( (_, i) => - // Always keep the last version (which is what we're looking at if - // i == allVersions.length - 1), because that's definitely still - // in effect at the start date. Then, if we're not looking at the - // last version, keep this version if the _next_ version was - // created after the start date (which'd mean this was the version - // in effect _at_ the start date). The overall result here is to - // keep last version created at or before the start date, plus all - // created after it. - i === allVersions.length - 1 || - allVersions[i + 1].approxVersion > startDate, - ) - : allVersions; + i === ordered.length - 1 || ordered[i + 1].approxVersion > startDate, + ); + }) + .sort((a, b) => a.approxVersion.getTime() - b.approxVersion.getTime()); } /** -- 2.51.2