From a1d576bd15fbc405c1f063cca6d076aebb99f4f3 Mon Sep 17 00:00:00 2001 From: Kevin Cantrell Date: Wed, 29 Jul 2026 16:13:07 +0900 Subject: [PATCH] fix(rules): stop wiping live rule state on template update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH /v1/rules/:id blanket-deleted cw_rule_state for the whole template, erasing is_triggered mid-alert — the alert service could then never run its reset path, orphaning cw_rule_trigger_log rows that render as still-active alerts forever. - update() now deletes state only for devices removed from the template and administratively closes their open trigger-log rows (reset_at = now(), reset_value null); state for still-assigned devices survives edits. - remove() also closes all open trigger-log rows before deleting the template so history rows cannot be orphaned open. - New spec coverage for update/remove via a per-table client stub router. Co-Authored-By: Claude Fable 5 --- src/v1/rules/rules.service.spec.ts | 259 ++++++++++++++++++++++++++++- src/v1/rules/rules.service.ts | 58 ++++++- 2 files changed, 307 insertions(+), 10 deletions(-) diff --git a/src/v1/rules/rules.service.spec.ts b/src/v1/rules/rules.service.spec.ts index 32826ab..9d01998 100644 --- a/src/v1/rules/rules.service.spec.ts +++ b/src/v1/rules/rules.service.spec.ts @@ -1,4 +1,8 @@ -import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { + ForbiddenException, + InternalServerErrorException, + NotFoundException, +} from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { SupabaseService } from '../../supabase/supabase.service'; import { DevicesService } from '../devices/devices.service'; @@ -7,6 +11,16 @@ import { RulesService } from './rules.service'; type StubResult = { data: unknown; error: unknown }; +interface FilterCall { + method: string; + args: unknown[]; +} + +interface WriteCall { + payload?: unknown; + filters: FilterCall[]; +} + interface QueryStub { select: jest.Mock; insert: jest.Mock; @@ -17,6 +31,22 @@ interface QueryStub { single: jest.Mock; maybeSingle: jest.Mock; then: (resolve: (value: StubResult) => unknown) => unknown; + updateCalls: WriteCall[]; + deleteCalls: WriteCall[]; +} + +// update()/delete() return a chainable thenable that records its filters, so +// tests can assert the exact WHERE clause a write was issued with. +function buildWriteChain(result: StubResult, call: WriteCall) { + const chain: Record = {}; + for (const method of ['eq', 'in', 'is', 'not']) { + chain[method] = jest.fn((...args: unknown[]) => { + call.filters.push({ method, args }); + return chain; + }); + } + chain.then = (resolve: (value: StubResult) => unknown) => resolve(result); + return chain; } function buildQueryStub(handlers: { @@ -41,15 +71,23 @@ function buildQueryStub(handlers: { then: (resolve: (value: StubResult) => unknown) => resolve(handlers.insertReturn ?? { data: null, error: null }), }); - stub.update = jest.fn().mockReturnValue({ - eq: jest - .fn() - .mockResolvedValue(handlers.updateReturn ?? { data: null, error: null }), + stub.updateCalls = []; + stub.update = jest.fn((payload: unknown) => { + const call: WriteCall = { payload, filters: [] }; + stub.updateCalls!.push(call); + return buildWriteChain( + handlers.updateReturn ?? { data: null, error: null }, + call, + ); }); - stub.delete = jest.fn().mockReturnValue({ - eq: jest - .fn() - .mockResolvedValue(handlers.deleteReturn ?? { data: null, error: null }), + stub.deleteCalls = []; + stub.delete = jest.fn(() => { + const call: WriteCall = { filters: [] }; + stub.deleteCalls!.push(call); + return buildWriteChain( + handlers.deleteReturn ?? { data: null, error: null }, + call, + ); }); stub.eq = jest.fn().mockReturnValue(stub); stub.in = jest.fn().mockReturnValue(stub); @@ -65,6 +103,35 @@ function buildQueryStub(handlers: { return stub as QueryStub; } +// Routes from(table) to per-table stubs so tests only depend on same-table +// call order, not the service's global .from() sequence. A single stub for a +// table serves every call; an array is consumed in order, last stub repeating. +function buildClient(stubsByTable: Record) { + const queues = new Map(); + for (const [table, stubs] of Object.entries(stubsByTable)) { + queues.set(table, Array.isArray(stubs) ? [...stubs] : [stubs]); + } + const from = jest.fn((table: string) => { + const queue = queues.get(table); + if (!queue || queue.length === 0) { + throw new Error(`Unexpected from('${table}') call in test`); + } + return queue.length > 1 ? queue.shift()! : queue[0]; + }); + return { from }; +} + +function serviceWith(client: { from: jest.Mock }): RulesService { + return new RulesService( + { + getClient: jest.fn(() => client), + getAdminClient: jest.fn(), + } as unknown as SupabaseService, + {} as unknown as DevicesService, + {} as unknown as LocationsService, + ); +} + describe('RulesService', () => { let service: RulesService; @@ -314,4 +381,178 @@ describe('RulesService', () => { }); }); }); + + describe('update and remove state handling', () => { + const jwt = { sub: 'user-1', email: 'user@example.com', isStaff: false }; + + const deviceRows = (devEuis: string[]) => + devEuis.map((devEui) => ({ + dev_eui: devEui, + name: `Device ${devEui}`, + user_id: 'user-1', + cw_device_owners: [], + })); + + const templateRow = { + id: 1, + name: 'Freezer temp', + description: null, + device_type_id: null, + is_active: true, + created_at: null, + }; + + const assignmentRows = (devEuis: string[]) => + devEuis.map((devEui, index) => ({ + id: index + 1, + dev_eui: devEui, + template_id: 1, + is_active: true, + created_at: null, + })); + + const savePayload = (devEuis: string[]) => ({ + name: 'Freezer temp', + devEuis, + criteria: [ + { + subject: 'temperature_c', + operator: '>=', + triggerValue: -15, + resetValue: -18, + }, + ], + actions: [{ actionType: 1, config: { recipient: 'me@example.com' } }], + }); + + interface UpdateStubs { + state: QueryStub; + triggerLog: QueryStub; + templates: QueryStub; + } + + const buildUpdateClient = ( + existingDevEuis: string[], + overrides?: Partial< + Record<'stateHandlers', { deleteReturn: StubResult }> + >, + ): { client: { from: jest.Mock }; stubs: UpdateStubs } => { + const state = buildQueryStub({ + list: { data: [], error: null }, + ...(overrides?.stateHandlers ?? {}), + }); + const triggerLog = buildQueryStub({}); + const templates = buildQueryStub({ + maybeSingle: { data: templateRow, error: null }, + }); + const client = buildClient({ + cw_devices: buildQueryStub({ + list: { data: deviceRows(existingDevEuis), error: null }, + }), + cw_rule_templates: templates, + cw_device_rule_assignments: buildQueryStub({ + list: { data: assignmentRows(existingDevEuis), error: null }, + }), + cw_rule_template_criteria: buildQueryStub({}), + cw_rule_template_actions: buildQueryStub({}), + cw_rule_state: state, + cw_rule_trigger_log: triggerLog, + }); + return { client, stubs: { state, triggerLog, templates } }; + }; + + it('update preserves state for still-assigned devices (never a template-wide wipe)', async () => { + const { client, stubs } = buildUpdateClient(['AA', 'BB']); + const service = serviceWith(client); + + await service.update(1, savePayload(['AA']), jwt); + + expect(stubs.state.deleteCalls).toHaveLength(1); + const filters = stubs.state.deleteCalls[0].filters; + expect(filters).toContainEqual({ + method: 'eq', + args: ['template_id', 1], + }); + expect(filters).toContainEqual({ + method: 'not', + args: ['dev_eui', 'in', '("AA")'], + }); + }); + + it('update closes open trigger-log rows only for removed devices', async () => { + const { client, stubs } = buildUpdateClient(['AA', 'BB']); + const service = serviceWith(client); + + await service.update(1, savePayload(['AA']), jwt); + + expect(stubs.triggerLog.updateCalls).toHaveLength(1); + const call = stubs.triggerLog.updateCalls[0]; + const payload = call.payload as { reset_at: unknown }; + expect(Object.keys(payload)).toEqual(['reset_at']); + expect(typeof payload.reset_at).toBe('string'); + expect(call.filters).toContainEqual({ + method: 'eq', + args: ['template_id', 1], + }); + expect(call.filters).toContainEqual({ + method: 'is', + args: ['reset_at', null], + }); + expect(call.filters).toContainEqual({ + method: 'not', + args: ['dev_eui', 'in', '("AA")'], + }); + }); + + it('update happy path returns the re-fetched template', async () => { + const { client } = buildUpdateClient(['AA']); + const service = serviceWith(client); + + const result = await service.update(1, savePayload(['AA']), jwt); + + expect(result.id).toBe(1); + expect(result.name).toBe('Freezer temp'); + }); + + it('update surfaces InternalServerErrorException when state cleanup fails', async () => { + const { client } = buildUpdateClient(['AA'], { + stateHandlers: { + deleteReturn: { data: null, error: { message: 'boom' } }, + }, + }); + const service = serviceWith(client); + + await expect( + service.update(1, savePayload(['AA']), jwt), + ).rejects.toBeInstanceOf(InternalServerErrorException); + }); + + it('remove closes all open trigger-log rows and deletes state, children, template', async () => { + const { client, stubs } = buildUpdateClient(['AA']); + const service = serviceWith(client); + + await service.remove(1, jwt); + + expect(stubs.state.deleteCalls).toHaveLength(1); + expect(stubs.state.deleteCalls[0].filters).toEqual([ + { method: 'eq', args: ['template_id', 1] }, + ]); + + expect(stubs.triggerLog.updateCalls).toHaveLength(1); + const logCall = stubs.triggerLog.updateCalls[0]; + const logPayload = logCall.payload as { reset_at: unknown }; + expect(Object.keys(logPayload)).toEqual(['reset_at']); + expect(typeof logPayload.reset_at).toBe('string'); + expect(logCall.filters).toEqual([ + { method: 'eq', args: ['template_id', 1] }, + { method: 'is', args: ['reset_at', null] }, + ]); + + expect(stubs.templates.deleteCalls).toHaveLength(1); + expect(stubs.templates.deleteCalls[0].filters).toContainEqual({ + method: 'eq', + args: ['id', 1], + }); + }); + }); }); diff --git a/src/v1/rules/rules.service.ts b/src/v1/rules/rules.service.ts index ef3eeef..4906925 100644 --- a/src/v1/rules/rules.service.ts +++ b/src/v1/rules/rules.service.ts @@ -367,7 +367,7 @@ export class RulesService { } await this.replaceTemplateChildren(id, normalized); - await this.deleteTemplateState(id); + await this.cleanupStateForUnassignedDevices(id, normalized.devEuis); return this.findOne(id, user); } @@ -388,6 +388,7 @@ export class RulesService { ); await this.deleteTemplateState(id); + await this.closeOpenTriggerLogs(id); await this.deleteTemplateChildren(id); const client = this.supabaseService.getClient(); @@ -609,6 +610,55 @@ export class RulesService { } } + // State rows for devices that remain assigned must survive a template edit: + // deleting them would erase is_triggered mid-alert, so the reset action and + // trigger-log closure for that alert would never run. + private async cleanupStateForUnassignedDevices( + templateId: number, + keepDevEuis: string[], + ): Promise { + const { error } = await this.supabaseService + .getClient() + .from('cw_rule_state') + .delete() + .eq('template_id', templateId) + .not('dev_eui', 'in', toPostgrestList(keepDevEuis)); + + if (error) { + throw new InternalServerErrorException('Failed to clean up rule state'); + } + + await this.closeOpenTriggerLogs(templateId, { + excludeDevEuis: keepDevEuis, + }); + } + + private async closeOpenTriggerLogs( + templateId: number, + opts?: { excludeDevEuis?: string[] }, + ): Promise { + // reset_value stays null: these rows are closed administratively, not by a + // device reading that satisfied the reset criterion. + let query = this.supabaseService + .getClient() + .from('cw_rule_trigger_log') + .update({ reset_at: new Date().toISOString() }) + .eq('template_id', templateId) + .is('reset_at', null); + + if (opts?.excludeDevEuis?.length) { + query = query.not('dev_eui', 'in', toPostgrestList(opts.excludeDevEuis)); + } + + const { error } = await query; + + if (error) { + throw new InternalServerErrorException( + 'Failed to close rule trigger logs', + ); + } + } + private async deleteTemplateBestEffort(templateId: number): Promise { try { await this.deleteTemplateChildren(templateId); @@ -818,6 +868,12 @@ function normalizeSaveRequest( }; } +// PostgREST `not ... in` filters take a parenthesized, comma-separated list; +// values are quoted so EUIs survive as literals. +function toPostgrestList(values: string[]): string { + return `(${values.map((value) => `"${value}"`).join(',')})`; +} + function assertDevicesCanBeManaged( devices: ManagedDevice[], devEuis: string[],