diff --git a/packages/backend/src/apps/custom-api/__tests__/actions/http-request-interceptors.test.ts b/packages/backend/src/apps/custom-api/__tests__/actions/http-request-interceptors.test.ts index a458abc5b6..a4359f8a6c 100644 --- a/packages/backend/src/apps/custom-api/__tests__/actions/http-request-interceptors.test.ts +++ b/packages/backend/src/apps/custom-api/__tests__/actions/http-request-interceptors.test.ts @@ -38,10 +38,12 @@ describe('http request interceptors', () => { step: { id: 'herp-derp', appKey: 'webhook', + key: 'catchRawWebhook', position: 2, parameters: { url: CF_REDIRECTION_WORKER_FOR_UNIT_TESTS, }, + version: 1, }, setActionItem: vi.fn(), app, diff --git a/packages/backend/src/apps/custom-api/__tests__/actions/http-request.test.ts b/packages/backend/src/apps/custom-api/__tests__/actions/http-request.test.ts index 0eb2f257b6..53b7dc49bb 100644 --- a/packages/backend/src/apps/custom-api/__tests__/actions/http-request.test.ts +++ b/packages/backend/src/apps/custom-api/__tests__/actions/http-request.test.ts @@ -57,8 +57,10 @@ describe('make http request', () => { step: { id: 'herp-derp', appKey: 'webhook', + key: 'catchRawWebhook', position: 1, parameters: {}, + version: 1, }, http: { request: mocks.httpRequest, diff --git a/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.mrf.test.ts b/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.mrf.test.ts index 93f8092327..c64264c4ae 100644 --- a/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.mrf.test.ts +++ b/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.mrf.test.ts @@ -126,10 +126,12 @@ describe('decrypt form response - MRF specific', () => { step: { id: '123', appKey: apps.formsg.key, + key: 'newSubmission', position: 0, parameters: { nricFilter: undefined, }, + version: 1, }, flow: { id: 'flowid', diff --git a/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.test.ts b/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.test.ts index 71b50ad103..52ad715c2a 100644 --- a/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.test.ts +++ b/packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.test.ts @@ -93,10 +93,12 @@ describe('decrypt form response', () => { step: { id: '123', appKey: apps.formsg.key, + key: 'newSubmission', position: 0, parameters: { nricFilter: undefined, }, + version: 1, }, flow: { id: 'flowid', diff --git a/packages/backend/src/apps/formsg/__tests__/triggers/create-mrf-steps.itest.ts b/packages/backend/src/apps/formsg/__tests__/triggers/create-mrf-steps.itest.ts index e54b2e1b39..b5f9116868 100644 --- a/packages/backend/src/apps/formsg/__tests__/triggers/create-mrf-steps.itest.ts +++ b/packages/backend/src/apps/formsg/__tests__/triggers/create-mrf-steps.itest.ts @@ -97,6 +97,7 @@ describe('createMrfSteps', () => { expect(step.parameters).toEqual({ mrf: mrfWorkflow.actions[i] }) expect(step.config?.stepName).toBe(mrfWorkflow.actions[i].defaultStepName) expect(step.connectionId).toBe(allSteps[0].connectionId) + expect(step.version).toBe(1) } }) diff --git a/packages/backend/src/apps/formsg/triggers/new-submission/create-mrf-steps.ts b/packages/backend/src/apps/formsg/triggers/new-submission/create-mrf-steps.ts index 7532f29d2b..42e2f150c5 100644 --- a/packages/backend/src/apps/formsg/triggers/new-submission/create-mrf-steps.ts +++ b/packages/backend/src/apps/formsg/triggers/new-submission/create-mrf-steps.ts @@ -3,6 +3,7 @@ import { IGlobalVariable, IStep } from '@plumber/types' import get from 'lodash.get' import { raw, Transaction } from 'objection' +import { getStepVersion } from '@/helpers/get-step-version' import Step from '@/models/step' import { ParsedMrfWorkflow } from '../../common/types' @@ -168,6 +169,7 @@ export async function createMrfSteps( config: { stepName, }, + version: getStepVersion(MRF_APP_KEY, MRF_KEY), }) newMrfSteps.push(newStep) newMrfStepPositionToInsert = newStep.position + 1 diff --git a/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-row-impl.test.ts b/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-row-impl.test.ts new file mode 100644 index 0000000000..25ce74973d --- /dev/null +++ b/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-row-impl.test.ts @@ -0,0 +1,300 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import StepError from '@/errors/step' + +import getTableRowImpl from '../../actions/get-table-row/implementation' +import getTopNTableRows from '../../common/get-top-n-table-rows' + +// Mock the getTopNTableRows function +vi.mock('../../common/get-top-n-table-rows') + +describe('getTableRowImpl', () => { + const mockSession = {} as any + const mockGlobalVariable = {} as any + const tableId = '{test-table-id}' + + beforeEach(() => { + vi.clearAllMocks() + + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Email', 'Age', 'Group', 'RSVP-ed'], + rows: [ + ['Alice', 'alice@example.com', '25', 'A', 'Yes'], + ['Bob', 'bob@example.com', '30', 'B', 'No'], + ['Charlie', 'charlie@example.com', '35', 'A', 'Yes'], + ['David', 'david@example.com', '40', 'B', 'No'], + ['Eve', 'eve@example.com', '45', 'C', 'Yes'], + ['Frank', 'frank@example.com', '50', 'C', 'No'], + ], + headerSheetRowIndex: 0, + }) + }) + + describe('single filter', () => { + it('should find a row matching single filter', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Name', lookupValue: 'Bob' }], + }) + + expect(result).toEqual({ + tableRowIndex: 1, + sheetRowNumber: 3, + row: ['Bob', 'bob@example.com', '30', 'B', 'No'], + columns: ['Name', 'Email', 'Age', 'Group', 'RSVP-ed'], + }) + }) + + it('should return null when no row matches single filter', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Group', lookupValue: 'D' }], + }) + + expect(result).toBeNull() + }) + + it('should throw error when lookup column does not exist', async () => { + await expect( + getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'NonExistent', lookupValue: 'value' }], + }), + ).rejects.toThrow(StepError) + }) + }) + + describe('multiple filters', () => { + it('should find a row matching all filters', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Group', lookupValue: 'A' }, + { lookupColumn: 'RSVP-ed', lookupValue: 'Yes' }, + ], + }) + + // Should match Alice (first matching row, Charlie also matches) + expect(result).toEqual({ + tableRowIndex: 0, + sheetRowNumber: 2, + row: ['Alice', 'alice@example.com', '25', 'A', 'Yes'], + columns: ['Name', 'Email', 'Age', 'Group', 'RSVP-ed'], + }) + }) + + it('should find the first row when multiple rows match all filters', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Group', lookupValue: 'A' }, + { lookupColumn: 'RSVP-ed', lookupValue: 'Yes' }, + ], + }) + + // Alice and Charlie both match (Group=A, RSVP-ed=Yes), should return Alice + expect(result).toEqual({ + tableRowIndex: 0, + sheetRowNumber: 2, + row: ['Alice', 'alice@example.com', '25', 'A', 'Yes'], + columns: ['Name', 'Email', 'Age', 'Group', 'RSVP-ed'], + }) + }) + + it('should return null when only some filters match', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Group', lookupValue: 'B' }, + { lookupColumn: 'RSVP-ed', lookupValue: 'Yes' }, + ], + }) + + // Bob and David are Group B, but both have RSVP-ed=No + expect(result).toBeNull() + }) + + it('should return null when no rows match any filter', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Group', lookupValue: 'D' }, + { lookupColumn: 'Email', lookupValue: 'nonexistent@example.com' }, + ], + }) + + expect(result).toBeNull() + }) + + it('should handle three or more filters', async () => { + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Name', lookupValue: 'Charlie' }, + { lookupColumn: 'Group', lookupValue: 'A' }, + { lookupColumn: 'RSVP-ed', lookupValue: 'Yes' }, + ], + }) + + expect(result).toEqual({ + tableRowIndex: 2, + sheetRowNumber: 4, + row: ['Charlie', 'charlie@example.com', '35', 'A', 'Yes'], + columns: ['Name', 'Email', 'Age', 'Group', 'RSVP-ed'], + }) + }) + + it('should throw error when any filter column does not exist', async () => { + await expect( + getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Group', lookupValue: 'A' }, + { lookupColumn: 'NonExistent', lookupValue: 'value' }, + ], + }), + ).rejects.toThrow(StepError) + }) + }) + + describe('edge cases', () => { + it('should handle empty table rows', async () => { + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Email'], + rows: [], + headerSheetRowIndex: 0, + }) + + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Name', lookupValue: 'Alice' }], + }) + + expect(result).toBeNull() + }) + + it('should handle empty string lookup values', async () => { + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Email'], + rows: [ + ['Alice', ''], + ['Bob', 'bob@example.com'], + ], + headerSheetRowIndex: 0, + }) + + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [ + { lookupColumn: 'Name', lookupValue: 'Alice' }, + { lookupColumn: 'Email', lookupValue: '' }, + ], + }) + + expect(result).toEqual({ + tableRowIndex: 0, + sheetRowNumber: 2, + row: ['Alice', ''], + columns: ['Name', 'Email'], + }) + }) + + it('should calculate correct sheetRowNumber with non-zero headerSheetRowIndex', async () => { + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Email'], + rows: [ + ['Alice', 'alice@example.com'], + ['Bob', 'bob@example.com'], + ], + headerSheetRowIndex: 5, // Header is at row 6 (0-indexed row 5) + }) + + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Name', lookupValue: 'Bob' }], + }) + + expect(result).toEqual({ + tableRowIndex: 1, + sheetRowNumber: 8, // headerIndex(5) + tableRowIndex(1) + 2 + row: ['Bob', 'bob@example.com'], + columns: ['Name', 'Email'], + }) + }) + + it('should find last row when it matches', async () => { + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Status'], + rows: [ + ['Alice', 'Inactive'], + ['Bob', 'Inactive'], + ['Charlie', 'Active'], + ], + headerSheetRowIndex: 0, + }) + + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Status', lookupValue: 'Active' }], + }) + + expect(result).toEqual({ + tableRowIndex: 2, + sheetRowNumber: 4, + row: ['Charlie', 'Active'], + columns: ['Name', 'Status'], + }) + }) + + it('should match exact values (case sensitive)', async () => { + vi.mocked(getTopNTableRows).mockResolvedValue({ + columns: ['Name', 'Email'], + rows: [ + ['alice', 'alice@example.com'], + ['Alice', 'alice2@example.com'], + ], + headerSheetRowIndex: 0, + }) + + const result = await getTableRowImpl({ + $: mockGlobalVariable, + session: mockSession, + tableId, + filters: [{ lookupColumn: 'Name', lookupValue: 'Alice' }], + }) + + expect(result).toEqual({ + tableRowIndex: 1, + sheetRowNumber: 3, + row: ['Alice', 'alice2@example.com'], + columns: ['Name', 'Email'], + }) + }) + }) +}) diff --git a/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-rows.itest.ts b/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-rows.itest.ts index 9134ae3202..d33ea79dbe 100644 --- a/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-rows.itest.ts +++ b/packages/backend/src/apps/m365-excel/__tests__/actions/get-table-rows.itest.ts @@ -9,11 +9,27 @@ import Context from '@/types/express/context' import m365Excel from '../..' import getTableRowsAction from '../../actions/get-table-rows' +import { stepTransformer } from '../../common/transform-step-parameters' import { HexEncodedRowObject } from '../../common/workbook-helpers/tables/convert-row-to-hex-encoded-row-record' +const DEFAULT_PARAMETERS = { + fileId: 'test-file-id', + tableId: '{test-table-id}', + lookupColumn: 'Column1', + lookupValue: 'test-value', +} + +const { transformStepParameters } = stepTransformer + const getHexEncodedColumnName = (columnName: string) => Buffer.from(columnName).toString('hex') +const getColumnObject = (columnName: string) => ({ + id: getHexEncodedColumnName(columnName), + name: columnName, + value: `data.rows.*.data.${getHexEncodedColumnName(columnName)}`, +}) + // Mock dependencies const mocks = vi.hoisted(() => ({ WorkbookSession: { @@ -55,12 +71,8 @@ describe('getTableRowsAction', () => { appKey: m365Excel.name, key: getTableRowsAction.key, position: 2, - parameters: { - fileId: 'test-file-id', - tableId: '{test-table-id}', - lookupColumn: 'Column1', - lookupValue: 'test-value', - }, + parameters: DEFAULT_PARAMETERS, + version: 1, }, app: { name: m365Excel.name, @@ -71,16 +83,24 @@ describe('getTableRowsAction', () => { }, } as unknown as IGlobalVariable + // Simulate the Step model's afterFind hook, which transforms parameters + // before they reach the action's run function in real execution. + $.step.parameters = transformStepParameters( + $.step.key, + $.step.parameters, + $.step.version, + ) + // Setup default mock implementations mocks.getTopNTableRows.mockResolvedValue({ - columns: ['Column1', 'Column2'], + columns: ['Column1', 'Column2', 'Column3', 'Column4'], rows: [ - ['non-matching', 'data1'], - ['test-value', 'data2'], - ['test-value', 'data3'], - ['', 'data4'], - ['row5', ''], - ['', 'data6'], + ['non-matching', 'data1', 'a', '1'], + ['test-value', 'data2', 'b', '2'], + ['test-value', 'data3', 'a', '3'], + ['', 'data4', 'a', '3'], + ['row5', '', 'b', '2'], + ['', 'data6', 'c', '1'], ], headerSheetRowIndex: 0, }) @@ -121,24 +141,12 @@ describe('getTableRowsAction', () => { headerSheetRowIndex: 0, }) - $.step.parameters.lookupValue = 'test-value' await getTableRowsAction.run($) expect($.setActionItem).toHaveBeenCalledWith({ raw: { data: { - columns: expect.arrayContaining([ - expect.objectContaining({ - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }), - expect.objectContaining({ - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }), - ]), + columns: [getColumnObject('Column1'), getColumnObject('Column2')], rows: [], inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, }, @@ -151,38 +159,36 @@ describe('getTableRowsAction', () => { await getTableRowsAction.run($) expect($.setActionItem).toHaveBeenCalledWith({ - raw: expect.objectContaining({ + raw: { rowsFound: 2, // Two rows match 'test-value' data: { - columns: expect.arrayContaining([ - expect.objectContaining({ - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }), - expect.objectContaining({ - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }), - ]), - rows: expect.arrayContaining([ - expect.objectContaining({ + columns: [ + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), + ], + rows: [ + { data: { [getHexEncodedColumnName('Column1')]: 'test-value', [getHexEncodedColumnName('Column2')]: 'data2', + [getHexEncodedColumnName('Column3')]: 'b', + [getHexEncodedColumnName('Column4')]: '2', }, - }), - expect.objectContaining({ + }, + { data: { [getHexEncodedColumnName('Column1')]: 'test-value', [getHexEncodedColumnName('Column2')]: 'data3', + [getHexEncodedColumnName('Column3')]: 'a', + [getHexEncodedColumnName('Column4')]: '3', }, - }), - ]), + }, + ], inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, }, - }), + }, }) // Verify the rowData contains the expected rows @@ -201,42 +207,45 @@ describe('getTableRowsAction', () => { }) it('should return matching rows when lookup value is empty', async () => { - $.step.parameters.lookupValue = '' + $.step.parameters = { ...DEFAULT_PARAMETERS, lookupValue: '' } + $.step.parameters = transformStepParameters( + $.step.key, + $.step.parameters, + $.step.version, + ) await getTableRowsAction.run($) expect($.setActionItem).toHaveBeenCalledWith({ - raw: expect.objectContaining({ + raw: { rowsFound: 2, data: { - columns: expect.arrayContaining([ - expect.objectContaining({ - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }), - expect.objectContaining({ - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }), - ]), - rows: expect.arrayContaining([ - expect.objectContaining({ + columns: [ + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), + ], + rows: [ + { data: { [getHexEncodedColumnName('Column1')]: '', [getHexEncodedColumnName('Column2')]: 'data4', + [getHexEncodedColumnName('Column3')]: 'a', + [getHexEncodedColumnName('Column4')]: '3', }, - }), - expect.objectContaining({ + }, + { data: { [getHexEncodedColumnName('Column1')]: '', [getHexEncodedColumnName('Column2')]: 'data6', + [getHexEncodedColumnName('Column3')]: 'c', + [getHexEncodedColumnName('Column4')]: '1', }, - }), - ]), + }, + ], inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, }, - }), + }, }) // Verify the rowData contains the expected rows @@ -260,7 +269,12 @@ describe('getTableRowsAction', () => { it('should handle case-sensitive matching', async () => { // Change the lookup value to be different case - $.step.parameters.lookupValue = 'TEST-VALUE' + $.step.parameters = { ...DEFAULT_PARAMETERS, lookupValue: 'TEST-VALUE' } + $.step.parameters = transformStepParameters( + $.step.key, + $.step.parameters, + $.step.version, + ) await getTableRowsAction.run($) @@ -269,16 +283,10 @@ describe('getTableRowsAction', () => { raw: { data: { columns: [ - { - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }, - { - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }, + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), ], rows: [], inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, @@ -300,38 +308,32 @@ describe('getTableRowsAction', () => { headerSheetRowIndex: 0, }) // Change the lookup value to be different case - $.step.parameters.lookupValue = 'TEST-VALUE' + $.step.parameters = { ...DEFAULT_PARAMETERS, lookupValue: 'TEST-VALUE' } + $.step.parameters = transformStepParameters( + $.step.key, + $.step.parameters, + $.step.version, + ) await getTableRowsAction.run($) // Should find the exact case match expect($.setActionItem).toHaveBeenCalledWith({ - raw: expect.objectContaining({ + raw: { rowsFound: 1, data: { - rows: expect.arrayContaining([ - expect.objectContaining({ + rows: [ + { data: { [getHexEncodedColumnName('Column1')]: 'TEST-VALUE', [getHexEncodedColumnName('Column2')]: 'data2', }, - }), - ]), - columns: expect.arrayContaining([ - expect.objectContaining({ - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }), - expect.objectContaining({ - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }), - ]), + }, + ], + columns: [getColumnObject('Column1'), getColumnObject('Column2')], inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, }, - }), + }, }) }) @@ -353,18 +355,7 @@ describe('getTableRowsAction', () => { raw: expect.objectContaining({ rowsFound: 500, // Should be limited to 500 rows data: { - columns: expect.arrayContaining([ - expect.objectContaining({ - id: getHexEncodedColumnName('Column1'), - name: 'Column1', - value: `data.rows.*.data.${getHexEncodedColumnName('Column1')}`, - }), - expect.objectContaining({ - id: getHexEncodedColumnName('Column2'), - name: 'Column2', - value: `data.rows.*.data.${getHexEncodedColumnName('Column2')}`, - }), - ]), + columns: [getColumnObject('Column1'), getColumnObject('Column2')], rows: expect.arrayContaining( Array.from({ length: 500 }, (_, i) => expect.objectContaining({ @@ -397,4 +388,112 @@ describe('getTableRowsAction', () => { 'data500', ) }) + + describe('multiple filters', () => { + it('should find rows matching all filters', async () => { + $.step.parameters.filters = [ + { lookupColumn: 'Column3', lookupValue: 'a' }, + { lookupColumn: 'Column4', lookupValue: '3' }, + ] + + await getTableRowsAction.run($) + + expect($.setActionItem).toHaveBeenCalledWith({ + raw: { + rowsFound: 2, + data: { + columns: [ + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), + ], + rows: [ + { + data: { + [getHexEncodedColumnName('Column1')]: 'test-value', + [getHexEncodedColumnName('Column2')]: 'data3', + [getHexEncodedColumnName('Column3')]: 'a', + [getHexEncodedColumnName('Column4')]: '3', + }, + }, + { + data: { + [getHexEncodedColumnName('Column1')]: '', + [getHexEncodedColumnName('Column2')]: 'data4', + [getHexEncodedColumnName('Column3')]: 'a', + [getHexEncodedColumnName('Column4')]: '3', + }, + }, + ], + inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, + }, + }, + }) + + const call = ($.setActionItem as ReturnType).mock + .calls[0][0] + const rowData = call.raw.data.rows + expect(rowData).toHaveLength(2) + }) + + it('should return no rows when only some filters match', async () => { + $.step.parameters.filters = [ + { lookupColumn: 'Column1', lookupValue: 'test-value' }, + { lookupColumn: 'Column2', lookupValue: 'non-matching' }, + ] + + await getTableRowsAction.run($) + + expect($.setActionItem).toHaveBeenCalledWith({ + raw: { + data: { + columns: [ + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), + ], + rows: [], + inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, + }, + rowsFound: 0, + }, + }) + }) + + it('should handle empty string in multiple filters', async () => { + $.step.parameters.filters = [ + { lookupColumn: 'Column1', lookupValue: '' }, + { lookupColumn: 'Column2', lookupValue: 'data4' }, + ] + + await getTableRowsAction.run($) + + expect($.setActionItem).toHaveBeenCalledWith({ + raw: { + rowsFound: 1, + data: { + columns: [ + getColumnObject('Column1'), + getColumnObject('Column2'), + getColumnObject('Column3'), + getColumnObject('Column4'), + ], + rows: [ + { + data: { + [getHexEncodedColumnName('Column1')]: '', + [getHexEncodedColumnName('Column2')]: 'data4', + [getHexEncodedColumnName('Column3')]: 'a', + [getHexEncodedColumnName('Column4')]: '3', + }, + }, + ], + inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL, + }, + }, + }) + }) + }) }) diff --git a/packages/backend/src/apps/m365-excel/__tests__/common/schema.test.ts b/packages/backend/src/apps/m365-excel/__tests__/common/schema.test.ts index 5e915dec97..95313fb64b 100644 --- a/packages/backend/src/apps/m365-excel/__tests__/common/schema.test.ts +++ b/packages/backend/src/apps/m365-excel/__tests__/common/schema.test.ts @@ -3,7 +3,12 @@ import type { IGlobalVariable } from '@plumber/types' import { beforeEach, describe, expect, it } from 'vitest' import { z } from 'zod' -import { fileIdSchema, tableIdSchema } from '../../common/schema' +import { + fileIdSchema, + filtersSchema, + lookupParametersSchema, + tableIdSchema, +} from '../../common/schema' const VALID_FILE_ID = '1234ABCD1234ABCD1234ABCD1234ABCD' const VALID_TABLE_ID = `{${VALID_FILE_ID}}` @@ -79,3 +84,129 @@ describe('validate dynamic fields', () => { }) }) }) + +describe('filtersSchema', () => { + describe('invalid filters', () => { + it('rejects empty array', () => { + expect(filtersSchema.safeParse([])).toHaveProperty('success', false) + }) + + it('rejects duplicate lookup columns', () => { + expect( + filtersSchema.safeParse([ + { lookupColumn: 'Name', lookupValue: 'Alice' }, + { lookupColumn: 'Name', lookupValue: 'Bob' }, + ]), + ).toHaveProperty('success', false) + }) + + it('rejects multiple duplicates across filters', () => { + expect( + filtersSchema.safeParse([ + { lookupColumn: 'Name', lookupValue: 'Alice' }, + { lookupColumn: 'Age', lookupValue: '30' }, + { lookupColumn: 'Name', lookupValue: 'Bob' }, + ]), + ).toHaveProperty('success', false) + }) + }) + + describe('valid filters', () => { + it('accepts a single filter', () => { + expect( + filtersSchema.safeParse([ + { lookupColumn: 'Name', lookupValue: 'Alice' }, + ]), + ).toHaveProperty('success', true) + }) + + it('accepts multiple filters with unique columns', () => { + expect( + filtersSchema.safeParse([ + { lookupColumn: 'Name', lookupValue: 'Alice' }, + { lookupColumn: 'Age', lookupValue: '30' }, + { lookupColumn: 'Department', lookupValue: 'Engineering' }, + ]), + ).toHaveProperty('success', true) + }) + + it('defaults lookupValue to empty string when omitted', () => { + const result = filtersSchema.safeParse([{ lookupColumn: 'Name' }]) + expect(result).toHaveProperty('success', true) + if (result.success) { + expect(result.data[0].lookupValue).toBe('') + } + }) + }) +}) + +describe('lookupParametersSchema', () => { + describe('accepts new format', () => { + it('with filters array', () => { + expect( + lookupParametersSchema.safeParse({ + fileId: VALID_FILE_ID, + tableId: VALID_TABLE_ID, + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }), + ).toHaveProperty('success', true) + }) + + it('with multiple filters', () => { + expect( + lookupParametersSchema.safeParse({ + fileId: VALID_FILE_ID, + tableId: VALID_TABLE_ID, + filters: [ + { + lookupColumn: 'Status', + lookupValue: 'Active', + }, + { + lookupColumn: 'Department', + lookupValue: 'Engineering', + }, + ], + }), + ).toHaveProperty('success', true) + }) + + it('with exactly 3 filters (max allowed)', () => { + expect( + lookupParametersSchema.safeParse({ + fileId: VALID_FILE_ID, + tableId: VALID_TABLE_ID, + filters: [ + { + lookupColumn: 'Status', + lookupValue: 'Active', + }, + { + lookupColumn: 'Department', + lookupValue: 'Engineering', + }, + { + lookupColumn: 'Level', + lookupValue: 'Senior', + }, + ], + }), + ).toHaveProperty('success', true) + }) + + it('with empty filters array', () => { + expect( + lookupParametersSchema.safeParse({ + fileId: VALID_FILE_ID, + tableId: VALID_TABLE_ID, + filters: [], + }), + ).toHaveProperty('success', false) + }) + }) +}) diff --git a/packages/backend/src/apps/m365-excel/__tests__/common/transform-step-parameters.test.ts b/packages/backend/src/apps/m365-excel/__tests__/common/transform-step-parameters.test.ts new file mode 100644 index 0000000000..f901c6595c --- /dev/null +++ b/packages/backend/src/apps/m365-excel/__tests__/common/transform-step-parameters.test.ts @@ -0,0 +1,557 @@ +import { IJSONObject } from '@plumber/types' + +import { describe, expect, it } from 'vitest' + +import { stepTransformer } from '../../common/transform-step-parameters' + +const { transformStepParameters, getLatestStepVersion } = stepTransformer + +// There is 1 transformer (v1→v2), so latest version is 2. +describe('transformStepParameters', () => { + describe('version routing', () => { + it('version 1: applies the transformer (old format)', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + 1, + ) + + expect(result).toHaveProperty('filters') + expect(result).not.toHaveProperty('lookupColumn') + expect(result).not.toHaveProperty('lookupValue') + }) + + it('version 2: already latest, returns parameters unchanged', () => { + const input = { + fileId: 'file123', + tableId: 'table456', + filters: [{ lookupColumn: 'Email', lookupValue: 'test@example.com' }], + } + + expect(transformStepParameters('getTableRow', input, 2)).toEqual(input) + }) + }) + + describe('transformLookupConditionsToFilters - old format to new format', () => { + it('transforms old lookup parameters to filters array', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }) + }) + + it('transforms with empty lookupValue', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Status', + lookupValue: '', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Status', + lookupValue: '', + }, + ], + }) + }) + + it('transforms with missing lookupValue (undefined)', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Name', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Name', + lookupValue: '', + }, + ], + }) + }) + + it('preserves all other parameters during transformation', () => { + const result = transformStepParameters( + 'updateTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'ID', + lookupValue: '42', + columnsToUpdate: [{ columnName: 'Status', value: 'Complete' }], + someOtherField: 'preserved', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + columnsToUpdate: [{ columnName: 'Status', value: 'Complete' }], + someOtherField: 'preserved', + filters: [ + { + lookupColumn: 'ID', + lookupValue: '42', + }, + ], + }) + }) + }) + + describe('transformLookupConditionsToFilters - idempotency', () => { + it('is idempotent when filters already exist', () => { + const input = { + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + } + + const result = transformStepParameters('getTableRow', input, 1) + + expect(result).toEqual(input) + }) + + it('is idempotent when called multiple times', () => { + const input = { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + } + + const firstTransform = transformStepParameters('getTableRow', input, 1) + const secondTransform = transformStepParameters( + 'getTableRow', + firstTransform, + 1, + ) + const thirdTransform = transformStepParameters( + 'getTableRow', + secondTransform, + 1, + ) + + expect(firstTransform).toEqual(secondTransform) + expect(secondTransform).toEqual(thirdTransform) + }) + + it('prefers filters when both old and new formats present', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'OldColumn', + lookupValue: 'old value', + filters: [ + { + lookupColumn: 'NewColumn', + lookupValue: 'new value', + }, + ], + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'NewColumn', + lookupValue: 'new value', + }, + ], + }) + }) + + it('removes old parameters when filters exist', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }, + 1, + ) + + expect(result).not.toHaveProperty('lookupColumn') + expect(result).not.toHaveProperty('lookupValue') + expect(result).toHaveProperty('filters') + }) + }) + + describe('transformLookupConditionsToFilters - edge cases', () => { + it('handles empty filters array by falling back to old params', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + filters: [], + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }) + }) + + it('handles null filters by falling back to old params', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + filters: null, + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }) + }) + + it('handles undefined filters by falling back to old params', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + filters: undefined, + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Email', + lookupValue: 'test@example.com', + }, + ], + }) + }) + + it('returns parameters unchanged when no transformation needed', () => { + const input = { + fileId: 'file123', + tableId: 'table456', + someOtherField: 'value', + } + + const result = transformStepParameters('getTableRow', input, 1) + + expect(result).toEqual(input) + }) + + it('handles parameters with only lookupColumn (no lookupValue)', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Status', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Status', + lookupValue: '', + }, + ], + }) + }) + + it('handles parameters with only lookupValue (no lookupColumn)', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupValue: 'some value', + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: undefined, + lookupValue: 'some value', + }, + ], + }) + }) + }) + + describe('getLatestStepVersion', () => { + it('returns 2 for actions with one transformer', () => { + expect(getLatestStepVersion('getTableRow')).toBe(2) + expect(getLatestStepVersion('getTableRows')).toBe(2) + expect(getLatestStepVersion('updateTableRow')).toBe(2) + }) + + it('returns 1 for actions with no transformers', () => { + expect(getLatestStepVersion('createTableRow')).toBe(1) + }) + }) + + describe('action routing', () => { + it('transforms getTableRow action', () => { + const result = transformStepParameters( + 'getTableRow', + { lookupColumn: 'Email', lookupValue: 'test@example.com' }, + 1, + ) + + expect(result).toHaveProperty('filters') + }) + + it('transforms getTableRows action', () => { + const result = transformStepParameters( + 'getTableRows', + { lookupColumn: 'Status', lookupValue: 'Active' }, + 1, + ) + + expect(result).toHaveProperty('filters') + }) + + it('transforms updateTableRow action', () => { + const result = transformStepParameters( + 'updateTableRow', + { lookupColumn: 'ID', lookupValue: '123' }, + 1, + ) + + expect(result).toHaveProperty('filters') + }) + + it('returns parameters unchanged for non-transformable actions', () => { + const input = { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Email', + lookupValue: 'test@example.com', + } + + const result = transformStepParameters('someOtherAction', input, 1) + + expect(result).toEqual(input) + expect(result).not.toHaveProperty('filters') + }) + }) + + describe('real-world scenarios', () => { + it('handles typical old Pipe from database', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: '01ABCDEFGHIJKLMNOPQRSTUVWXYZ', + tableId: '{01ABCDEFGHIJKLMNOPQRSTUVWXYZ}', + lookupColumn: 'Email Address', + lookupValue: '{{1.email}}', + }, + 1, + ) + + expect(result).toEqual({ + fileId: '01ABCDEFGHIJKLMNOPQRSTUVWXYZ', + tableId: '{01ABCDEFGHIJKLMNOPQRSTUVWXYZ}', + filters: [ + { + lookupColumn: 'Email Address', + lookupValue: '{{1.email}}', + }, + ], + }) + }) + + it('handles new Pipe with filters (version 2, no transformation)', () => { + const input = { + fileId: '01ABCDEFGHIJKLMNOPQRSTUVWXYZ', + tableId: '{01ABCDEFGHIJKLMNOPQRSTUVWXYZ}', + filters: [ + { + lookupColumn: 'Email Address', + lookupValue: '{{1.email}}', + }, + ], + } + + const result = transformStepParameters('getTableRow', input, 2) + + expect(result).toEqual(input) + }) + + it('handles update action with columnsToUpdate', () => { + const result = transformStepParameters( + 'updateTableRow', + { + fileId: '01ABCDEFGHIJKLMNOPQRSTUVWXYZ', + tableId: '{01ABCDEFGHIJKLMNOPQRSTUVWXYZ}', + lookupColumn: 'ID', + lookupValue: '{{1.id}}', + columnsToUpdate: [ + { + columnName: 'Status', + value: 'Completed', + }, + { + columnName: 'Updated At', + value: '{{1.timestamp}}', + }, + ], + }, + 1, + ) + + expect(result).toEqual({ + fileId: '01ABCDEFGHIJKLMNOPQRSTUVWXYZ', + tableId: '{01ABCDEFGHIJKLMNOPQRSTUVWXYZ}', + columnsToUpdate: [ + { + columnName: 'Status', + value: 'Completed', + }, + { + columnName: 'Updated At', + value: '{{1.timestamp}}', + }, + ], + filters: [ + { + lookupColumn: 'ID', + lookupValue: '{{1.id}}', + }, + ], + }) + }) + + it('handles special characters in lookup values', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Name', + lookupValue: "O'Brien, Inc. & Co. ", + }, + 1, + ) + + expect((result.filters as IJSONObject[])?.[0]?.lookupValue).toBe( + "O'Brien, Inc. & Co. ", + ) + }) + + it('handles numeric lookupValue', () => { + const result = transformStepParameters( + 'getTableRow', + { + fileId: 'file123', + tableId: 'table456', + lookupColumn: 'Age', + lookupValue: 42, + }, + 1, + ) + + expect(result).toEqual({ + fileId: 'file123', + tableId: 'table456', + filters: [ + { + lookupColumn: 'Age', + lookupValue: 42, + }, + ], + }) + }) + }) +}) diff --git a/packages/backend/src/apps/m365-excel/actions/get-table-row/implementation/index.ts b/packages/backend/src/apps/m365-excel/actions/get-table-row/implementation/index.ts index aad320a5bd..bb862b3e93 100644 --- a/packages/backend/src/apps/m365-excel/actions/get-table-row/implementation/index.ts +++ b/packages/backend/src/apps/m365-excel/actions/get-table-row/implementation/index.ts @@ -1,5 +1,8 @@ import type { IGlobalVariable } from '@plumber/types' +import type { z } from 'zod' + +import { filtersSchema } from '@/apps/m365-excel/common/schema' import StepError from '@/errors/step' import getTopNTableRows from '../../../common/get-top-n-table-rows' @@ -16,8 +19,7 @@ interface GetTableRowImplParams { $: IGlobalVariable session: WorkbookSession tableId: string - lookupValue: string - lookupColumn: string + filters: z.infer } interface GetTableRowImplResults { @@ -71,7 +73,7 @@ interface GetTableRowImplResults { export default async function getTableRowImpl( args: GetTableRowImplParams, ): Promise { - const { $, session, tableId, lookupValue, lookupColumn } = args + const { $, session, tableId, filters } = args const { columns, rows, headerSheetRowIndex } = await getTopNTableRows( $, @@ -80,16 +82,23 @@ export default async function getTableRowImpl( MAX_ROWS, ) - const columnIndex = columns.indexOf(lookupColumn) - if (columnIndex === -1) { - throw new StepError( - `Column "${lookupColumn}" does not exist in your table.`, - `Check that your Excel table contains the "${lookupColumn}" column.`, - ) - } + const columnIndices = filters.map(({ lookupColumn }) => { + const idx = columns.indexOf(lookupColumn) + if (idx === -1) { + throw new StepError( + `Column "${lookupColumn}" does not exist in your table.`, + `Check that your Excel table contains the "${lookupColumn}" column.`, + ) + } + return idx + }) for (const [rowIndex, row] of rows.entries()) { - if (row[columnIndex] === lookupValue) { + if ( + filters.every( + ({ lookupValue }, i) => row[columnIndices[i]] === lookupValue, + ) + ) { return { tableRowIndex: rowIndex, sheetRowNumber: rowIndex + headerSheetRowIndex + 2, diff --git a/packages/backend/src/apps/m365-excel/actions/get-table-row/index.ts b/packages/backend/src/apps/m365-excel/actions/get-table-row/index.ts index 1ffe003155..b01a386e05 100644 --- a/packages/backend/src/apps/m365-excel/actions/get-table-row/index.ts +++ b/packages/backend/src/apps/m365-excel/actions/get-table-row/index.ts @@ -4,13 +4,18 @@ import z from 'zod' import StepError from '@/errors/step' +import { + LOOKUP_CONDITIONS_SUBFIELDS, + MAX_LOOKUP_CONDITIONS, +} from '../../common/constants' +import { lookupParametersSchema } from '../../common/schema' import { convertRowToHexEncodedRowRecord } from '../../common/workbook-helpers/tables' import WorkbookSession from '../../common/workbook-session' import { RATE_LIMIT_FOR_RELEASE_ONLY_REMOVE_AFTER_JULY_2024 } from '../../FOR_RELEASE_PERIOD_ONLY' import getDataOutMetadata from './get-data-out-metadata' import getTableRowImpl, { MAX_ROWS } from './implementation' -import { dataOutSchema, parametersSchema } from './schemas' +import { dataOutSchema } from './schemas' type DataOut = Required> @@ -65,48 +70,14 @@ const action: IRawAction = { }, }, { - key: 'lookupColumn' as const, - type: 'dropdown' as const, - showOptionValue: false, - required: true, - variables: false, - label: 'Lookup column', + key: 'filters', + label: 'Lookup conditions', description: - 'If multiple rows meet your condition, the topmost entry will be returned', - source: { - type: 'query' as const, - name: 'getDynamicData' as const, - arguments: [ - { - name: 'key', - value: 'listTableColumns', - }, - { - name: 'parameters.fileId', - value: '{parameters.fileId}', - }, - { - name: 'parameters.tableId', - value: '{parameters.tableId}', - }, - ], - }, - hiddenIf: { - fieldKey: 'tableId', - op: 'is_empty', - }, - }, - { - key: 'lookupValue' as const, - label: 'Lookup Value', - // We don't support matching on Excel-formatted text because it's very - // weird (e.g. currency cells have a trailing space), and will lead to too - // much user confusion. - description: - 'Case sensitive and should not include units (e.g., $5.20 → 5.2). Leave blank to search for empty cells.', - type: 'string' as const, - required: false, - variables: true, + 'Lookup values are case sensitive and should not include units (e.g., $5.20 → 5.2). Leave blank to search for empty cells.', + type: 'multirow-multicol' as const, + required: true, + subFields: LOOKUP_CONDITIONS_SUBFIELDS, + maxRows: MAX_LOOKUP_CONDITIONS, hiddenIf: { fieldKey: 'tableId', op: 'is_empty', @@ -122,7 +93,9 @@ const action: IRawAction = { await RATE_LIMIT_FOR_RELEASE_ONLY_REMOVE_AFTER_JULY_2024($.user?.email, $) } - const parametersParseResult = parametersSchema.safeParse($.step.parameters) + const parametersParseResult = lookupParametersSchema.safeParse( + $.step.parameters, + ) if (parametersParseResult.success === false) { throw new StepError( @@ -131,16 +104,14 @@ const action: IRawAction = { ) } - const { fileId, tableId, lookupColumn, lookupValue } = - parametersParseResult.data + const { fileId, tableId, filters } = parametersParseResult.data const session = await WorkbookSession.acquire($, fileId) const results = await getTableRowImpl({ $, session, tableId, - lookupValue, - lookupColumn, + filters, }) if (!results) { diff --git a/packages/backend/src/apps/m365-excel/actions/get-table-row/schemas.ts b/packages/backend/src/apps/m365-excel/actions/get-table-row/schemas.ts index ee060cdb5c..f9e3e14e20 100644 --- a/packages/backend/src/apps/m365-excel/actions/get-table-row/schemas.ts +++ b/packages/backend/src/apps/m365-excel/actions/get-table-row/schemas.ts @@ -1,20 +1,7 @@ import z from 'zod' -import { fileIdSchema, tableIdSchema } from '../../common/schema' import { hexEncodedRowRecordSchema } from '../../common/workbook-helpers/tables' -export const parametersSchema = z.object({ - fileId: fileIdSchema, - tableId: tableIdSchema, - // Populated by dynamic data, so no need to trim. - lookupColumn: z.string().min(1, { - message: 'Please select a column to lookup from.', - }), - // * We don't trim as we want to match _exactly_ on the user's input. - // * We allow empty strings to support optional form fields. - lookupValue: z.string().default(''), -}) - export const dataOutSchema = z.discriminatedUnion('foundRow', [ z.object({ foundRow: z.literal(false) }), z.object({ diff --git a/packages/backend/src/apps/m365-excel/actions/get-table-rows/index.ts b/packages/backend/src/apps/m365-excel/actions/get-table-rows/index.ts index eeb7fdb2ad..4ecac49c00 100644 --- a/packages/backend/src/apps/m365-excel/actions/get-table-rows/index.ts +++ b/packages/backend/src/apps/m365-excel/actions/get-table-rows/index.ts @@ -5,14 +5,19 @@ import z from 'zod' import { FOR_EACH_INPUT_SOURCE } from '@/apps/toolbox/common/constants' import StepError from '@/errors/step' -import { GET_TABLE_ROWS_LIMIT } from '../../common/constants' +import { + GET_TABLE_ROWS_LIMIT, + LOOKUP_CONDITIONS_SUBFIELDS, + MAX_LOOKUP_CONDITIONS, +} from '../../common/constants' import getTopNTableRows from '../../common/get-top-n-table-rows' +import { lookupParametersSchema } from '../../common/schema' import { convertRowToHexKeyedObject } from '../../common/workbook-helpers/tables/convert-row-to-hex-encoded-row-record' import WorkbookSession from '../../common/workbook-session' import { MAX_ROWS } from '../get-table-row/implementation' import getDataOutMetadata from './get-data-out-metadata' -import { dataOutSchema, parametersSchema } from './schemas' +import { dataOutSchema } from './schemas' type DataOut = z.infer @@ -68,51 +73,28 @@ const action: IRawAction = { }, }, { - key: 'lookupColumn' as const, - type: 'dropdown' as const, - showOptionValue: false, - required: true, - variables: false, - label: 'Lookup column', + key: 'filters', + label: 'Lookup conditions', description: - 'Only the first 500 rows that meet the condition will be returned', - source: { - type: 'query' as const, - name: 'getDynamicData' as const, - arguments: [ - { - name: 'key', - value: 'listTableColumns', - }, - { - name: 'parameters.fileId', - value: '{parameters.fileId}', - }, - { - name: 'parameters.tableId', - value: '{parameters.tableId}', - }, - ], + 'Lookup values are case sensitive and should not include units (e.g., $5.20 → 5.2). Leave blank to search for empty cells.', + type: 'multirow-multicol' as const, + required: true, + subFields: LOOKUP_CONDITIONS_SUBFIELDS, + maxRows: MAX_LOOKUP_CONDITIONS, + hiddenIf: { + fieldKey: 'tableId', + op: 'is_empty', }, }, - { - key: 'lookupValue' as const, - label: 'Lookup Value', - // We don't support matching on Excel-formatted text because it's very - // weird (e.g. currency cells have a trailing space), and will lead to too - // much user confusion. - description: - 'Case sensitive and should not include units (e.g., $5.20 → 5.2). Leave blank to search for empty cells.', - type: 'string' as const, - required: false, - variables: true, - }, ], getDataOutMetadata, async run($) { - const parametersParseResult = parametersSchema.safeParse($.step.parameters) + const parametersParseResult = lookupParametersSchema.safeParse( + $.step.parameters, + ) + if (parametersParseResult.success === false) { throw new StepError( 'There was a problem with the input.', @@ -120,8 +102,7 @@ const action: IRawAction = { ) } - const { fileId, tableId, lookupColumn, lookupValue } = - parametersParseResult.data + const { fileId, tableId, filters } = parametersParseResult.data const session = await WorkbookSession.acquire($, fileId) const { columns: rawColumns, rows } = await getTopNTableRows( @@ -131,18 +112,25 @@ const action: IRawAction = { MAX_ROWS, ) - const columnIndex = rawColumns.indexOf(lookupColumn) - if (columnIndex === -1) { - throw new StepError( - `Column "${lookupColumn}" does not exist in your table.`, - `Check that your Excel table contains the "${lookupColumn}" column.`, - ) - } + const columnIndices = filters.map(({ lookupColumn }) => { + const idx = rawColumns.indexOf(lookupColumn) + if (idx === -1) { + throw new StepError( + `Column "${lookupColumn}" does not exist in your table.`, + `Check that your Excel table contains the "${lookupColumn}" column.`, + ) + } + return idx + }) const rowsToReturn: { data: Record }[] = [] for (const [_, row] of rows.entries()) { - if (row[columnIndex] === lookupValue) { + if ( + filters.every( + ({ lookupValue }, i) => row[columnIndices[i]] === lookupValue, + ) + ) { rowsToReturn.push({ data: convertRowToHexKeyedObject({ row, columns: rawColumns }), }) diff --git a/packages/backend/src/apps/m365-excel/actions/get-table-rows/schemas.ts b/packages/backend/src/apps/m365-excel/actions/get-table-rows/schemas.ts index 1c97bb8ba0..fc22b04146 100644 --- a/packages/backend/src/apps/m365-excel/actions/get-table-rows/schemas.ts +++ b/packages/backend/src/apps/m365-excel/actions/get-table-rows/schemas.ts @@ -2,20 +2,6 @@ import z from 'zod' import { FOR_EACH_INPUT_SOURCE } from '@/apps/toolbox/common/constants' -import { fileIdSchema, tableIdSchema } from '../../common/schema' - -export const parametersSchema = z.object({ - fileId: fileIdSchema, - tableId: tableIdSchema, - // Populated by dynamic data, so no need to trim. - lookupColumn: z.string().min(1, { - message: 'Please select a column to lookup from.', - }), - // * We don't trim as we want to match _exactly_ on the user's input. - // * We allow empty strings to support optional form fields. - lookupValue: z.string().default(''), -}) - export const dataOutSchema = z.object({ rowsFound: z.number(), data: z.object({ diff --git a/packages/backend/src/apps/m365-excel/actions/update-table-row/index.ts b/packages/backend/src/apps/m365-excel/actions/update-table-row/index.ts index f1a65189a0..9116dea5f1 100644 --- a/packages/backend/src/apps/m365-excel/actions/update-table-row/index.ts +++ b/packages/backend/src/apps/m365-excel/actions/update-table-row/index.ts @@ -99,7 +99,7 @@ const action: IRawAction = { ) } - const { fileId, tableId, lookupColumn, lookupValue, columnsToUpdate } = + const { fileId, tableId, filters, columnsToUpdate } = parametersParseResult.data // @@ -114,8 +114,7 @@ const action: IRawAction = { $, session, tableId, - lookupValue, - lookupColumn, + filters, }) if (!findRowResults) { diff --git a/packages/backend/src/apps/m365-excel/actions/update-table-row/schemas.ts b/packages/backend/src/apps/m365-excel/actions/update-table-row/schemas.ts index 798144841f..ec872fc308 100644 --- a/packages/backend/src/apps/m365-excel/actions/update-table-row/schemas.ts +++ b/packages/backend/src/apps/m365-excel/actions/update-table-row/schemas.ts @@ -1,9 +1,9 @@ import z from 'zod' +import { lookupParametersSchema } from '../../common/schema' import { hexEncodedRowRecordSchema } from '../../common/workbook-helpers/tables' -import { parametersSchema as getTableRowParametersSchema } from '../get-table-row/schemas' -export const parametersSchema = getTableRowParametersSchema.extend({ +export const parametersSchema = lookupParametersSchema.extend({ columnsToUpdate: z .array( z.object({ diff --git a/packages/backend/src/apps/m365-excel/common/constants.ts b/packages/backend/src/apps/m365-excel/common/constants.ts index d7800e0903..786f19e4cc 100644 --- a/packages/backend/src/apps/m365-excel/common/constants.ts +++ b/packages/backend/src/apps/m365-excel/common/constants.ts @@ -5,3 +5,46 @@ export const APP_KEY = 'm365-excel' export const MS_GRAPH_OAUTH_BASE_URL = 'https://login.microsoftonline.com' export const GET_TABLE_ROWS_LIMIT = 500 + +export const MAX_LOOKUP_CONDITIONS = 3 + +export const LOOKUP_CONDITIONS_SUBFIELDS = [ + { + placeholder: 'Lookup column', + key: 'lookupColumn', + type: 'dropdown' as const, + required: true, + variables: false, + showOptionValue: false, + source: { + type: 'query' as const, + name: 'getDynamicData' as const, + arguments: [ + { + name: 'key', + value: 'listTableColumns', + }, + { + name: 'parameters.fileId', + value: '{parameters.fileId}', + }, + { + name: 'parameters.tableId', + value: '{parameters.tableId}', + }, + ], + }, + customStyle: { flex: 2 }, + }, + { + key: 'lookupValue' as const, + placeholder: 'Lookup value', + // We don't support matching on Excel-formatted text because it's very + // weird (e.g. currency cells have a trailing space), and will lead to too + // much user confusion. + type: 'string' as const, + required: false, + variables: true, + customStyle: { flex: 3, minWidth: 0, maxWidth: '60%' }, + }, +] diff --git a/packages/backend/src/apps/m365-excel/common/schema.ts b/packages/backend/src/apps/m365-excel/common/schema.ts index 326247ce99..f5ccae454e 100644 --- a/packages/backend/src/apps/m365-excel/common/schema.ts +++ b/packages/backend/src/apps/m365-excel/common/schema.ts @@ -1,5 +1,7 @@ import { z } from 'zod' +import { MAX_LOOKUP_CONDITIONS } from './constants' + /** * Very loose regex to just accept only alphanumeric characters and dashes * since there is no proper public documentation with M365 @@ -26,3 +28,33 @@ export const worksheetIdSchema = z .trim() .regex(WORKSHEET_ID_REGEX, { message: 'Invalid worksheet ID format.' }) .min(1, { message: 'Please select a worksheet to lookup from.' }) + +export const filtersSchema = z + .array( + z.object({ + // Populated by dynamic data, so no need to trim. + lookupColumn: z.string().min(1), + // * We don't trim as we want to match _exactly_ on the user's input. + // * We allow empty strings to support optional form fields. + lookupValue: z.string().default(''), + }), + ) + .min(1, { message: 'Please add at least one lookup condition.' }) + .max(MAX_LOOKUP_CONDITIONS, { + message: `You can only add up to ${MAX_LOOKUP_CONDITIONS} lookup conditions.`, + }) + .refine( + (filters) => { + const columns = filters.map((f) => f.lookupColumn) + return new Set(columns).size === columns.length + }, + { + message: 'Each lookup condition must use a different column.', + }, + ) + +export const lookupParametersSchema = z.object({ + fileId: fileIdSchema, + tableId: tableIdSchema, + filters: filtersSchema, +}) diff --git a/packages/backend/src/apps/m365-excel/common/transform-step-parameters.ts b/packages/backend/src/apps/m365-excel/common/transform-step-parameters.ts new file mode 100644 index 0000000000..6c38edcc65 --- /dev/null +++ b/packages/backend/src/apps/m365-excel/common/transform-step-parameters.ts @@ -0,0 +1,58 @@ +import type { IJSONObject } from '@plumber/types' + +import { createVersionedStepTransformer } from '@/helpers/transform-step-parameters' + +const ACTION_TRANSFORMERS: Record< + string, + ((parameters: IJSONObject) => IJSONObject)[] +> = { + getTableRow: [transformLookupConditionsToFilters], + getTableRows: [transformLookupConditionsToFilters], + updateTableRow: [transformLookupConditionsToFilters], +} + +/** + * Transforms old lookup parameters to new filters array format. + * Idempotent and safe to call multiple times. + * + * Handles three cases: + * 1. Database has old params only → transforms to filters, removes old params + * 2. Database has both formats (legacy) → keeps filters, removes old params + * 3. Database has new format only → returns clean data + * + * Example transformation: + * Input: { lookupColumn: 'Email', lookupValue: 'test@example.com' } + * Output: { filters: [{ lookupColumn: 'Email', lookupValue: 'test@example.com' }] } + */ +export function transformLookupConditionsToFilters( + parameters: IJSONObject, +): IJSONObject { + const { lookupColumn, lookupValue, filters, ...rest } = parameters + + // If filters already populated, use them (remove old params) + if (filters && Array.isArray(filters) && filters.length > 0) { + return { + ...rest, + filters, + } + } + + // Transform old parameters to filters format + if (lookupColumn || lookupValue !== undefined) { + return { + ...rest, + filters: [ + { + lookupColumn, + lookupValue: lookupValue ?? '', + }, + ], + } + } + + // No transformation needed + return parameters +} + +export const stepTransformer = + createVersionedStepTransformer(ACTION_TRANSFORMERS) diff --git a/packages/backend/src/apps/m365-excel/index.ts b/packages/backend/src/apps/m365-excel/index.ts index 0f0a4a3e74..ba1c9803ba 100644 --- a/packages/backend/src/apps/m365-excel/index.ts +++ b/packages/backend/src/apps/m365-excel/index.ts @@ -3,6 +3,7 @@ import type { IApp } from '@plumber/types' import getTransferDetails from './common/get-transfer-details' import beforeRequest from './common/interceptors/before-request' import requestErrorHandler from './common/interceptors/request-error-handler' +import { stepTransformer } from './common/transform-step-parameters' import actions from './actions' import auth from './auth' import dynamicData from './dynamic-data' @@ -30,6 +31,7 @@ const app: IApp = { 'There is a cap on total disk space and Excel actions across all Plumber users. To prevent disruption to your workflow, contact us if you have large files or need more than 100 Excel actions per hour.\n\nRead [our guide](https://guide.plumber.gov.sg/user-guides/actions/m365-excel) for more information.', }, category: 'data', + stepTransformer, } export default app diff --git a/packages/backend/src/apps/paysg/__tests__/actions/create-payment.test.ts b/packages/backend/src/apps/paysg/__tests__/actions/create-payment.test.ts index 82db1fce8d..63630d884d 100644 --- a/packages/backend/src/apps/paysg/__tests__/actions/create-payment.test.ts +++ b/packages/backend/src/apps/paysg/__tests__/actions/create-payment.test.ts @@ -27,6 +27,7 @@ describe('create payment', () => { step: { id: 'herp-derp', appKey: 'paysg', + key: 'createPayment', position: 2, parameters: { // Pre-fill some required fields @@ -38,6 +39,7 @@ describe('create payment', () => { description: 'test-description', paymentAmountCents: '12345', }, + version: 1, }, http: { post: mocks.httpPost, diff --git a/packages/backend/src/apps/paysg/__tests__/actions/get-payment.test.ts b/packages/backend/src/apps/paysg/__tests__/actions/get-payment.test.ts index b9ed10eaf3..315cb2f266 100644 --- a/packages/backend/src/apps/paysg/__tests__/actions/get-payment.test.ts +++ b/packages/backend/src/apps/paysg/__tests__/actions/get-payment.test.ts @@ -27,11 +27,13 @@ describe('get payment', () => { step: { id: 'herp-derp', appKey: 'paysg', + key: 'getPayment', position: 2, parameters: { // Pre-fill some required fields paymentId: 'sample-payment-id', }, + version: 1, }, http: { get: mocks.httpGet, diff --git a/packages/backend/src/apps/paysg/__tests__/actions/send-email.test.ts b/packages/backend/src/apps/paysg/__tests__/actions/send-email.test.ts index 2dcc5122eb..dd136ed26a 100644 --- a/packages/backend/src/apps/paysg/__tests__/actions/send-email.test.ts +++ b/packages/backend/src/apps/paysg/__tests__/actions/send-email.test.ts @@ -24,11 +24,13 @@ describe('send payment email', () => { step: { id: 'herp-derp', appKey: 'paysg', + key: 'sendEmail', position: 2, parameters: { // Pre-fill some required fields paymentId: 'sample-payment-id', }, + version: 1, }, http: { post: mocks.httpPost, diff --git a/packages/backend/src/apps/toolbox/__tests__/actions/only-continue-if.test.ts b/packages/backend/src/apps/toolbox/__tests__/actions/only-continue-if.test.ts index 13ca870198..e8b48420a4 100644 --- a/packages/backend/src/apps/toolbox/__tests__/actions/only-continue-if.test.ts +++ b/packages/backend/src/apps/toolbox/__tests__/actions/only-continue-if.test.ts @@ -154,6 +154,7 @@ describe('Only continue if', () => { condition: 'equals', text: 1, }, + version: 1, } mocks.stepQueryResult.mockResolvedValueOnce(MOCK_FLOW) @@ -177,6 +178,7 @@ describe('Only continue if', () => { condition: 'equals', text: 1, }, + version: 1, } mocks.stepQueryResult.mockResolvedValueOnce(MOCK_FLOW) diff --git a/packages/backend/src/db/migrations/20260430114420_add_steps_version.ts b/packages/backend/src/db/migrations/20260430114420_add_steps_version.ts new file mode 100644 index 0000000000..f854246c73 --- /dev/null +++ b/packages/backend/src/db/migrations/20260430114420_add_steps_version.ts @@ -0,0 +1,13 @@ +import { Knex } from 'knex' + +export async function up(knex: Knex): Promise { + await knex.schema.table('steps', (table) => { + table.integer('version').notNullable().defaultTo(1) + }) +} + +export async function down(knex: Knex): Promise { + await knex.schema.table('steps', (table) => { + table.dropColumn('version') + }) +} diff --git a/packages/backend/src/graphql/__tests__/mutations/create-step.itest.ts b/packages/backend/src/graphql/__tests__/mutations/create-step.itest.ts index 7703125f31..ce4cf9a224 100644 --- a/packages/backend/src/graphql/__tests__/mutations/create-step.itest.ts +++ b/packages/backend/src/graphql/__tests__/mutations/create-step.itest.ts @@ -2,6 +2,7 @@ import { randomUUID } from 'crypto' import { NotFoundError } from 'objection' import { beforeEach, describe, expect, it, vi } from 'vitest' +import apps from '@/apps' import { BadUserInputError } from '@/errors/graphql-errors' import createStep from '@/graphql/mutations/create-step' import Flow from '@/models/flow' @@ -449,6 +450,77 @@ describe('createStep mutation integration tests', async () => { expect((newStep as any).connectionId).toBeNull() }) + describe('version assignment', () => { + it('defaults to version 1 when app has no stepTransformer', async () => { + // postman has no stepTransformer - version should default to 1 + const newStep = await createStep( + null, + { + input: { + flow: { id: testFlow.id, updatedAt: testFlowTimestampString }, + previousStep: { id: existingSteps[0].id }, + key: 'sendTransactionalEmail', + appKey: 'postman', + parameters: {}, + }, + }, + context, + ) + + expect((newStep as any).version).toBe(1) + }) + + it('defaults to version 1 when no appKey is provided', async () => { + const newStep = await createStep( + null, + { + input: { + flow: { id: testFlow.id, updatedAt: testFlowTimestampString }, + previousStep: { id: existingSteps[2].id }, + }, + }, + context, + ) + + expect((newStep as any).version).toBe(1) + }) + + it('uses the version from stepTransformer.getLatestStepVersion when app has a stepTransformer', async () => { + const mockGetLatestStepVersion = vi.fn().mockReturnValue(3) + const originalPostman = (apps as any)['postman'] + apps['postman'] = { + ...originalPostman, + stepTransformer: { + getLatestStepVersion: mockGetLatestStepVersion, + transformStepParameters: vi.fn(), + }, + } + + try { + const newStep = await createStep( + null, + { + input: { + flow: { id: testFlow.id, updatedAt: testFlowTimestampString }, + previousStep: { id: existingSteps[0].id }, + key: 'sendTransactionalEmail', + appKey: 'postman', + parameters: {}, + }, + }, + context, + ) + + expect(mockGetLatestStepVersion).toHaveBeenCalledWith( + 'sendTransactionalEmail', + ) + expect((newStep as any).version).toBe(3) + } finally { + ;(apps as any)['postman'] = originalPostman + } + }) + }) + describe('updatedAt and updatedBy validation', () => { it('should succeed when input.flow.updatedAt matches flow.updatedAt (timestamp string)', async () => { const params = { diff --git a/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts b/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts index a7b0a7b677..23bb5043b9 100644 --- a/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts +++ b/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts @@ -2,6 +2,7 @@ import { randomUUID } from 'crypto' import { NotFoundError } from 'objection' import { beforeEach, describe, expect, it, vi } from 'vitest' +import apps from '@/apps' import { BadUserInputError } from '@/errors/graphql-errors' import updateStep from '@/graphql/mutations/update-step' import Flow from '@/models/flow' @@ -487,6 +488,121 @@ describe('updateStep mutation', () => { ) }) + describe('version assignment', () => { + it('does not include version in patch when app has no stepTransformer', async () => { + // postman has no stepTransformer + await updateStep(null, { input: { ...genericInputParams } }, context) + + expect(patchAndFetchByIdSpy).toHaveBeenCalledWith( + mockStepId, + expect.not.objectContaining({ version: expect.anything() }), + ) + }) + + it('transforms parameters and sets latest version when app has a stepTransformer', async () => { + const mockTransformStepParameters = vi + .fn() + .mockReturnValue({ transformed: true }) + const mockGetLatestStepVersion = vi.fn().mockReturnValue(3) + const originalPostman = apps['postman'] + apps['postman'] = { + ...originalPostman, + stepTransformer: { + transformStepParameters: mockTransformStepParameters, + getLatestStepVersion: mockGetLatestStepVersion, + }, + } + + context.currentUser.withAccessibleSteps = createMockWithAccessibleSteps({ + owner, + currentUser: context.currentUser, + stepKey: 'sendTransactionalEmail', + stepAppKey: 'postman', + stepVersion: 1, + stepConnection: { id: mockConnectionId, userId: owner.id }, + flowUpdatedAt: testFlowISODateString, + }) + + try { + await updateStep(null, { input: { ...genericInputParams } }, context) + + expect(mockTransformStepParameters).toHaveBeenCalledWith( + 'sendTransactionalEmail', + genericInputParams.parameters, + 1, // step.version is undefined in mock, falls back to 1 + ) + expect(mockGetLatestStepVersion).toHaveBeenCalledWith( + 'sendTransactionalEmail', + ) + expect(patchAndFetchByIdSpy).toHaveBeenCalledWith( + mockStepId, + expect.objectContaining({ + parameters: { transformed: true }, + version: 3, + }), + ) + } finally { + apps['postman'] = originalPostman + } + }) + + it('uses step.version from DB (not frontend) when transforming — stale frontend fix', async () => { + const mockTransformStepParameters = vi + .fn() + .mockReturnValue({ upgraded: true }) + const mockGetLatestStepVersion = vi.fn().mockReturnValue(2) + const originalPostman = apps['postman'] + apps['postman'] = { + ...originalPostman, + stepTransformer: { + transformStepParameters: mockTransformStepParameters, + getLatestStepVersion: mockGetLatestStepVersion, + }, + } + + // DB step is at version 1 (old format) + context.currentUser.withAccessibleSteps = createMockWithAccessibleSteps({ + owner, + currentUser: context.currentUser, + stepKey: 'sendTransactionalEmail', + stepAppKey: 'postman', + stepVersion: 1, + stepConnection: { id: mockConnectionId, userId: owner.id }, + flowUpdatedAt: testFlowISODateString, + }) + + try { + // Frontend sends params in old format (stale) + await updateStep( + null, + { + input: { + ...genericInputParams, + parameters: { oldFormatParam: 'value' }, + }, + }, + context, + ) + + // Transformer should be called with DB version (1), not whatever frontend thinks + expect(mockTransformStepParameters).toHaveBeenCalledWith( + 'sendTransactionalEmail', + { oldFormatParam: 'value' }, + 1, + ) + expect(patchAndFetchByIdSpy).toHaveBeenCalledWith( + mockStepId, + expect.objectContaining({ + parameters: { upgraded: true }, + version: 2, + }), + ) + } finally { + apps['postman'] = originalPostman + } + }) + }) + describe('FlowConnections integration', () => { let patchSpy: any let addSpy: any diff --git a/packages/backend/src/graphql/__tests__/mutations/with-accessible.mock.ts b/packages/backend/src/graphql/__tests__/mutations/with-accessible.mock.ts index b11b418378..6b8b0d9d47 100644 --- a/packages/backend/src/graphql/__tests__/mutations/with-accessible.mock.ts +++ b/packages/backend/src/graphql/__tests__/mutations/with-accessible.mock.ts @@ -29,6 +29,7 @@ interface MockWithAccessibleStepsOptions { stepStatus?: string stepRole?: string stepConfig?: Record + stepVersion?: number stepNotFound?: boolean flowUpdatedAt?: string } @@ -44,6 +45,7 @@ export function createMockWithAccessibleSteps({ stepStatus = 'completed', stepRole = 'owner', stepConfig = {}, + stepVersion, stepNotFound = false, flowUpdatedAt = MOCK_FLOW_UPDATED_AT, }: MockWithAccessibleStepsOptions) { @@ -67,6 +69,7 @@ export function createMockWithAccessibleSteps({ status: stepStatus, role: stepRole, flowId, + version: stepVersion, connection: stepAppKey === 'tiles' ? {} : stepConnection, config: stepConfig, flow: { diff --git a/packages/backend/src/graphql/mutations/ai/create-flow-with-steps.ts b/packages/backend/src/graphql/mutations/ai/create-flow-with-steps.ts index 81d2effe5e..5fb45263b3 100644 --- a/packages/backend/src/graphql/mutations/ai/create-flow-with-steps.ts +++ b/packages/backend/src/graphql/mutations/ai/create-flow-with-steps.ts @@ -2,6 +2,7 @@ import type { IStep } from '@plumber/types' import z from 'zod/v3' +import { getStepVersion } from '@/helpers/get-step-version' import { getAllLdFlags, getRestrictedAppKeys } from '@/helpers/launch-darkly' import logger from '@/helpers/logger' import Flow from '@/models/flow' @@ -93,6 +94,7 @@ const createFlowWithSteps: MutationResolvers['createFlowWithSteps'] = async ( const normalizedSteps = [steps[0], ...normalizedActionSteps] const stepsToInsert = normalizedSteps.map((step: IStep) => { return { + version: getStepVersion(step.appKey, step.key), type: step.type, appKey: step.appKey, key: step.key, diff --git a/packages/backend/src/graphql/mutations/create-step.ts b/packages/backend/src/graphql/mutations/create-step.ts index df8ee624a6..94ba015891 100644 --- a/packages/backend/src/graphql/mutations/create-step.ts +++ b/packages/backend/src/graphql/mutations/create-step.ts @@ -3,6 +3,7 @@ import { IStepConfig } from '@plumber/types' import { raw } from 'objection' import { BadUserInputError } from '@/errors/graphql-errors' +import { getStepVersion } from '@/helpers/get-step-version' import logger from '@/helpers/logger' import { validateApprovalConfig } from '@/helpers/validate-approval-config' import App from '@/models/app' @@ -96,6 +97,8 @@ const createStep: MutationResolvers['createStep'] = async ( }) .where('position', '>=', validationResult.newStepPosition) + const version = getStepVersion(input.appKey, input.key) + const step = await flow.$relatedQuery('steps', trx).insertAndFetch({ key: input.key, appKey: input.appKey, @@ -104,6 +107,7 @@ const createStep: MutationResolvers['createStep'] = async ( parameters: input.parameters, connectionId: input.connection?.id, config: input.config as IStepConfig, + version, }) // NOTE: add flow connection to the flow_connections table diff --git a/packages/backend/src/graphql/mutations/duplicate-branch.ts b/packages/backend/src/graphql/mutations/duplicate-branch.ts index 601c76be58..e3d087f352 100644 --- a/packages/backend/src/graphql/mutations/duplicate-branch.ts +++ b/packages/backend/src/graphql/mutations/duplicate-branch.ts @@ -3,6 +3,7 @@ import { IStepConfig } from '@plumber/types' import { raw } from 'objection' import { BadUserInputError } from '@/errors/graphql-errors' +import { getStepVersion } from '@/helpers/get-step-version' import Step from '@/models/step' import { getConnection } from '@/services/connection' @@ -81,6 +82,7 @@ const duplicateBranch: MutationResolvers['duplicateBranch'] = async ( parameters, connectionId: connection?.id, config: config as IStepConfig, + version: getStepVersion(appKey, key), }) newSteps.push(step) diff --git a/packages/backend/src/graphql/mutations/duplicate-flow.ts b/packages/backend/src/graphql/mutations/duplicate-flow.ts index e9021be7c7..2c1827675e 100644 --- a/packages/backend/src/graphql/mutations/duplicate-flow.ts +++ b/packages/backend/src/graphql/mutations/duplicate-flow.ts @@ -1,5 +1,6 @@ import { isEmpty } from 'lodash' +import { getStepVersion } from '@/helpers/get-step-version' import logger from '@/helpers/logger' import { updateStepVariables } from '@/helpers/update-duplicated-steps' import Flow from '@/models/flow' @@ -83,6 +84,7 @@ const duplicateFlow: MutationResolvers['duplicateFlow'] = async ( oldToNewStepIdsMap, ), config: !isEmpty(prevStepConfig) ? prevStepConfig : undefined, + version: getStepVersion(oldStep.appKey, oldStep.key), }) oldToNewStepIdsMap[oldStep.id] = duplicatedStep.id // update map after duplicating step } diff --git a/packages/backend/src/graphql/mutations/update-step.ts b/packages/backend/src/graphql/mutations/update-step.ts index d4409f3aa7..d207eb179a 100644 --- a/packages/backend/src/graphql/mutations/update-step.ts +++ b/packages/backend/src/graphql/mutations/update-step.ts @@ -1,4 +1,5 @@ import { IAction } from '@/../../types' +import apps from '@/apps' import { BadUserInputError } from '@/errors/graphql-errors' import { addFlowConnection, @@ -82,12 +83,29 @@ const updateStep: MutationResolvers['updateStep'] = async ( const stepName = input?.config?.stepName ?? step?.config?.stepName const existingConfig = step?.config ?? {} + let parameters = input.parameters + let version = step.version + + // transform the parameters if the step has a stepTransformer + const transformer = input.key + ? apps[input.appKey]?.stepTransformer + : undefined + if (transformer) { + parameters = transformer.transformStepParameters( + input.key, + input.parameters, + version, + ) + version = transformer.getLatestStepVersion(input.key) + } + const updatedStep = await Step.query(trx) .patchAndFetchById(input.id, { key: input.key, appKey: input.appKey, connectionId: input.connection.id, - parameters: input.parameters, + parameters, + version, status: shouldInvalidate ? 'incomplete' : step.status, updatedBy: context.currentUser.id, config: { diff --git a/packages/backend/src/graphql/schema.graphql b/packages/backend/src/graphql/schema.graphql index 9797d22237..570dd00c3e 100644 --- a/packages/backend/src/graphql/schema.graphql +++ b/packages/backend/src/graphql/schema.graphql @@ -282,6 +282,7 @@ type ActionSubstepArgument { # Only for multirow subFields: [ActionSubstepArgument] + maxRows: Int # Only for rich text customRteMenuOptions: [RteMenuOption] diff --git a/packages/backend/src/helpers/__tests__/get-step-version.test.ts b/packages/backend/src/helpers/__tests__/get-step-version.test.ts new file mode 100644 index 0000000000..d796fe5366 --- /dev/null +++ b/packages/backend/src/helpers/__tests__/get-step-version.test.ts @@ -0,0 +1,57 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + getLatestStepVersion: vi.fn(), +})) + +vi.mock('@/apps', () => ({ + default: { + appWithTransformer: { + stepTransformer: { + getLatestStepVersion: mocks.getLatestStepVersion, + }, + }, + appWithoutTransformer: {}, + }, +})) + +import { getStepVersion } from '../get-step-version' + +describe('getStepVersion', () => { + beforeEach(() => { + vi.resetAllMocks() + }) + + it('returns 1 when appKey is undefined', () => { + expect(getStepVersion(undefined, 'someKey')).toBe(1) + }) + + it('returns 1 when key is undefined', () => { + expect(getStepVersion('appWithTransformer', undefined)).toBe(1) + }) + + it('returns 1 when both appKey and key are undefined', () => { + expect(getStepVersion(undefined, undefined)).toBe(1) + }) + + it('returns 1 when the app does not have a stepTransformer', () => { + expect(getStepVersion('appWithoutTransformer', 'someKey')).toBe(1) + }) + + it('returns 1 when the app does not exist', () => { + expect(getStepVersion('nonExistentApp', 'someKey')).toBe(1) + }) + + it('returns the version from stepTransformer.getLatestStepVersion', () => { + mocks.getLatestStepVersion.mockReturnValue(3) + + expect(getStepVersion('appWithTransformer', 'someKey')).toBe(3) + expect(mocks.getLatestStepVersion).toHaveBeenCalledWith('someKey') + }) + + it('returns 1 when stepTransformer.getLatestStepVersion returns nullish', () => { + mocks.getLatestStepVersion.mockReturnValue(null) + + expect(getStepVersion('appWithTransformer', 'someKey')).toBe(1) + }) +}) diff --git a/packages/backend/src/helpers/__tests__/transform-step-parameters.test.ts b/packages/backend/src/helpers/__tests__/transform-step-parameters.test.ts new file mode 100644 index 0000000000..96f2492e3b --- /dev/null +++ b/packages/backend/src/helpers/__tests__/transform-step-parameters.test.ts @@ -0,0 +1,56 @@ +import type { IJSONObject } from '@plumber/types' + +import { describe, expect, it, vi } from 'vitest' + +import { createVersionedStepTransformer } from '../transform-step-parameters' + +const transformerA = vi.fn((p: IJSONObject) => ({ ...p, a: true })) +const transformerB = vi.fn((p: IJSONObject) => ({ ...p, b: true })) +const transformerC = vi.fn((p: IJSONObject) => ({ ...p, c: true })) + +// 3 transformers: A (v1→v2), B (v2→v3), C (v3→v4). Latest version is 4. +const { transformStepParameters: transform, getLatestStepVersion } = + createVersionedStepTransformer({ + myAction: [transformerA, transformerB, transformerC], + }) + +describe('createVersionedStepTransformer', () => { + it('version 1: runs all transformers (A → B → C)', () => { + const result = transform('myAction', { x: 1 }, 1) + expect(result).toEqual({ x: 1, a: true, b: true, c: true }) + }) + + it('version 2: skips first transformer, runs B → C', () => { + const result = transform('myAction', { x: 1 }, 2) + expect(result).toEqual({ x: 1, b: true, c: true }) + expect(result).not.toHaveProperty('a') + }) + + it('version 3: skips first two transformers, runs C only', () => { + const result = transform('myAction', { x: 1 }, 3) + expect(result).toEqual({ x: 1, c: true }) + expect(result).not.toHaveProperty('a') + expect(result).not.toHaveProperty('b') + }) + + it('version 4: already latest, runs no transformers', () => { + const input = { x: 1 } + const result = transform('myAction', input, 4) + expect(result).toEqual({ x: 1 }) + }) + + it('returns parameters unchanged for unknown action keys', () => { + const input = { x: 1 } + expect(transform('unknownAction', input, 1)).toEqual(input) + }) +}) + +describe('getLatestStepVersion', () => { + it('returns transformers.length + 1 for a known action', () => { + expect(getLatestStepVersion('myAction')).toBe(4) + }) + + it('returns 1 for an unknown action (no transformers)', () => { + expect(getLatestStepVersion('unknownAction')).toBe(1) + }) +}) diff --git a/packages/backend/src/helpers/flow-templates.ts b/packages/backend/src/helpers/flow-templates.ts index bb2ef6cd15..eb4575d471 100644 --- a/packages/backend/src/helpers/flow-templates.ts +++ b/packages/backend/src/helpers/flow-templates.ts @@ -14,6 +14,7 @@ import type { FlowConfig, StepConfig, } from '@/graphql/__generated__/types.generated' +import { getStepVersion } from '@/helpers/get-step-version' import Flow from '@/models/flow' import { createTableRows } from '@/models/tiles/dynamodb/table-row' import User from '@/models/user' @@ -254,6 +255,7 @@ export async function createFlowFromTemplate( appKey, key: eventKey, parameters: updatedParameters, + version: getStepVersion(appKey, eventKey), config: { templateConfig: { appEventKey, diff --git a/packages/backend/src/helpers/get-step-version.ts b/packages/backend/src/helpers/get-step-version.ts new file mode 100644 index 0000000000..328417d77e --- /dev/null +++ b/packages/backend/src/helpers/get-step-version.ts @@ -0,0 +1,8 @@ +import apps from '@/apps' + +export function getStepVersion(appKey?: string, key?: string): number { + if (!appKey || !key) { + return 1 + } + return apps[appKey]?.stepTransformer?.getLatestStepVersion(key) ?? 1 +} diff --git a/packages/backend/src/helpers/global-variable.ts b/packages/backend/src/helpers/global-variable.ts index 56cdb8b2db..93eb1ec04f 100644 --- a/packages/backend/src/helpers/global-variable.ts +++ b/packages/backend/src/helpers/global-variable.ts @@ -76,8 +76,10 @@ const globalVariable = async ( step: { id: step?.id, appKey: step?.appKey, + key: step?.key, position: step?.position, parameters: step?.parameters || {}, + version: step?.version, }, nextStep: { id: nextStep?.id, diff --git a/packages/backend/src/helpers/transform-step-parameters.ts b/packages/backend/src/helpers/transform-step-parameters.ts new file mode 100644 index 0000000000..d4a38478b3 --- /dev/null +++ b/packages/backend/src/helpers/transform-step-parameters.ts @@ -0,0 +1,78 @@ +import type { IJSONObject } from '@plumber/types' + +/** + * Creates a versioned step parameter transformer for an app. + * + * Each transformer[i] migrates parameters from version i+1 to i+2. + * The version stored on the step determines where to start: + * + * version 1 → transformers[0] onwards (oldest, needs all migrations) + * version 2 → transformers[1] onwards (skips first migration) + * version N → transformers[N-1] onwards + * version = transformers.length + 1 → nothing to run (already latest) + * + * ## How to add a new migration + * + * 1. Write a pure function `(params: IJSONObject) => IJSONObject` that upgrades + * parameters from the previous version to the next. + * 2. Append it to the transformer array for the relevant action key. + * 3. Ensure that newly created steps are created with the latest version. + * + * Existing steps in the database keep their old version number, so they will + * automatically pick up the new transformer on next execution. + * + * ## Example (see apps/m365-excel/common/transform-parameters.ts) + * + * ```ts + * const ACTION_TRANSFORMERS = { + * getTableRow: [migrateV1toV2, migrateV2toV3], + * } + * + * export const { transformStepParameters, getLatestStepVersion } = + * createVersionedStepTransformer(ACTION_TRANSFORMERS) + * ``` + * + * The returned object has two functions: + * - `transformStepParameters(stepKey, stepParameters, version) => IJSONObject` + * - `getLatestStepVersion(stepKey) => number` — prefer `getStepVersion(appKey, key)` from `@/helpers/get-step-version` when creating new steps; + * call this directly only when you already hold a `transformer` reference (e.g. update-step) + * + * @param transformers - Map of action key → ordered array of migration functions + */ +function createVersionedStepTransformer( + transformers: Record IJSONObject)[]>, +) { + function transformStepParameters( + stepKey: string, + stepParameters: IJSONObject, + stepVersion: number, + ): IJSONObject { + const fns = transformers[stepKey] + if (!fns) { + return stepParameters + } + + // startIndex < 0 guards against invalid version values (e.g. 0) + const startIndex = stepVersion - 1 + if (startIndex < 0) { + return stepParameters + } + + return fns + .slice(startIndex) + .reduce( + (parameters, transformer) => transformer(parameters), + stepParameters, + ) + } + + // Version N+1 is the latest, where N is the number of transformers. + // New steps should be created at this version so no transformation is needed. + function getLatestStepVersion(stepKey: string): number { + return (transformers[stepKey]?.length ?? 0) + 1 + } + + return { transformStepParameters, getLatestStepVersion } +} + +export { createVersionedStepTransformer } diff --git a/packages/backend/src/models/step.ts b/packages/backend/src/models/step.ts index ceaed60214..a85c89a062 100644 --- a/packages/backend/src/models/step.ts +++ b/packages/backend/src/models/step.ts @@ -36,6 +36,7 @@ class Step extends Base { executionSteps: ExecutionStep[] config: IStepConfig updatedBy?: string + version: number static tableName = 'steps' @@ -57,6 +58,7 @@ class Step extends Base { }, position: { type: 'integer' }, parameters: { type: 'object' }, + version: { type: 'integer', default: 1 }, }, } @@ -310,6 +312,23 @@ class Step extends Base { }) } } + + /** + * This hook transforms step parameters to ensure the frontend always receives + * data in the latest format, even when the database contains legacy formats. + * + * This approach allows zero-downtime migrations without database updates. + */ + async $afterFind(): Promise { + const app = apps[this.appKey] + if (app?.stepTransformer && this.key && this.parameters) { + this.parameters = app.stepTransformer.transformStepParameters( + this.key, + this.parameters, + this.version, + ) + } + } } export default Step diff --git a/packages/frontend/src/components/InputCreator/index.tsx b/packages/frontend/src/components/InputCreator/index.tsx index 6a9a85122d..7cec84cff7 100644 --- a/packages/frontend/src/components/InputCreator/index.tsx +++ b/packages/frontend/src/components/InputCreator/index.tsx @@ -222,6 +222,7 @@ export default function InputCreator(props: InputCreatorProps): JSX.Element { type={type} // These are InputCreatorProps which MultiRow will forward. stepId={stepId} + maxRows={schema.maxRows} /> ) } diff --git a/packages/frontend/src/components/MultiRow/index.tsx b/packages/frontend/src/components/MultiRow/index.tsx index 4dfb6350c2..427f0b0dbf 100644 --- a/packages/frontend/src/components/MultiRow/index.tsx +++ b/packages/frontend/src/components/MultiRow/index.tsx @@ -24,6 +24,7 @@ export type MultiRowProps = { showDivider?: boolean addRowButtonText?: string type?: string + maxRows?: number } & Omit function MultiRow(props: MultiRowProps): JSX.Element { @@ -36,6 +37,7 @@ function MultiRow(props: MultiRowProps): JSX.Element { addRowButtonText, showDivider, type, + maxRows, ...forwardedInputCreatorProps } = props @@ -86,6 +88,7 @@ function MultiRow(props: MultiRowProps): JSX.Element { const rowsToRender = !rows.length && required ? [{ ...newRowDefaultValue }] : rows const canRemoveRow = !required || rowsToRender.length > 1 + const canAddRow = maxRows == null || rowsToRender.length < maxRows return ( @@ -171,15 +174,17 @@ function MultiRow(props: MultiRowProps): JSX.Element { ) })} - + {canAddRow && ( + + )} ) }} diff --git a/packages/types/index.d.ts b/packages/types/index.d.ts index 4a41029705..0f39e431fb 100644 --- a/packages/types/index.d.ts +++ b/packages/types/index.d.ts @@ -451,6 +451,7 @@ export interface IFieldMultiRowMultiCol extends IBaseField { value?: string addRowButtonText?: string subFields: IFieldMultiRowMultiColSubField[] + maxRows?: number } export interface IFieldMultiRow extends IBaseField { @@ -459,6 +460,7 @@ export interface IFieldMultiRow extends IBaseField { addRowButtonText?: string subFields: IField[] + maxRows?: number } export type TRteMenuOption = @@ -659,6 +661,30 @@ export interface IApp { * precedence. */ setupMessage?: SetupMessage + + /** + * Optional versioned step parameter transformer for an app. + * + * Both functions must be provided together — use `createVersionedStepTransformer` + * from `@/helpers/transform-step-parameters` which returns them as a coupled pair. + * + * Enables zero-downtime parameter format migrations by transforming legacy + * parameters to the current format when steps are fetched or executed. + * + * `transformStepParameters` — called on every step fetch (afterFind hook); applies + * all migrations from `stepVersion` onwards. + * + * `getLatestStepVersion` — used when creating new steps so they start at the + * latest version and require no transformation. + */ + stepTransformer?: { + transformStepParameters: ( + stepKey: string, + stepParameters: IJSONObject, + stepVersion: number, + ) => IJSONObject + getLatestStepVersion: (stepKey: string) => number + } } export type AppCategory = 'data' | 'communication' | 'logic' | 'others' | 'ai' @@ -962,8 +988,10 @@ export type IGlobalVariable = { step?: { id: string appKey: string + key: string position: number parameters: IJSONObject + version: number } nextStep?: { id: string