Skip to content

Commit d39afda

Browse files
kevinkim-ogpkevinkim-ogp
authored
fix: show numeric hex-encoded columns (#1230)
## Problem When Excel column names are hex-encoded, they may become numeric strings. Our backward compatibility check ``` const isBackwardCompatibilityColumnId = !isNaN(Number(id)) ``` then mistakenly hides these columns. This happens when: 1. Column names encode into numeric-only hex strings (e.g., `recruiter`, `sixty`). 2. Column names that are encoded with `e` are interpreted like scientific notation (e.g., `53656e6420537461747573`, `446174652053656e74` 3. Column names are numeric themselves (e.g., `0`, `123`). _This can also occur with FormSG table fields, but this PR addresses both M365 Excel and FormSG._ ## Solution Strengthen the backward compatibility check to ensure: 1. Column ID is numeric. 2. Column ID ≤ 500. 4. The column’s hex encoding matches its ID. ## How to test? Create excel columns with numbers only or strings that encode into numeric-only strings. Use Excel find multiple table rows and check step, then use in For each and check step. - [ ] Columns appear in the test result - [ ] Columns are selectable in the subsequent steps - [ ] DB stores the correct step parameter (hex encoding, not array index). Sanity check - [ ] Tiles find multiple rows + For each still works - [ ] FormSG table field + For each still works
1 parent 82ac4a1 commit d39afda

File tree

3 files changed

+187
-78
lines changed

3 files changed

+187
-78
lines changed

‎packages/backend/src/apps/toolbox/__tests__/actions/for-each/get-data-out-metadata.test.ts‎

Lines changed: 160 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,19 @@ import { describe, expect, it } from 'vitest'
33
import getDataOutMetadata from '../../../actions/for-each/get-data-out-metadata'
44
import { FOR_EACH_INPUT_SOURCE } from '../../../common/constants'
55

6+
const GENERIC_TABLE_EXPECTED_RESULT = {
7+
iteration: {
8+
label: 'Item number',
9+
displayedValue: '1',
10+
},
11+
iterations: { label: 'Items found' },
12+
inputSource: { isHidden: true },
13+
items: {
14+
rows: [{ data: { isHidden: true }, rowId: { isHidden: true } }],
15+
inputSource: { isHidden: true },
16+
},
17+
}
18+
619
describe('getDataOutMetadata', () => {
720
const createMockExecutionStep = (dataOut: any) => ({
821
id: 'execution-step-id',
@@ -199,16 +212,7 @@ describe('getDataOutMetadata', () => {
199212
const result = await getDataOutMetadata(executionStep)
200213

201214
expect(result).toEqual({
202-
iteration: {
203-
label: 'Item number',
204-
displayedValue: '1',
205-
},
206-
iterations: {
207-
label: 'Items found',
208-
},
209-
inputSource: {
210-
isHidden: true,
211-
},
215+
...GENERIC_TABLE_EXPECTED_RESULT,
212216
items: {
213217
columns: [
214218
{
@@ -268,16 +272,8 @@ describe('getDataOutMetadata', () => {
268272
const result = await getDataOutMetadata(executionStep)
269273

270274
expect(result).toEqual({
271-
iteration: {
272-
label: 'Item number',
273-
displayedValue: '',
274-
},
275-
iterations: {
276-
label: 'Items found',
277-
},
278-
inputSource: {
279-
isHidden: true,
280-
},
275+
...GENERIC_TABLE_EXPECTED_RESULT,
276+
iteration: { label: 'Item number', displayedValue: '' },
281277
items: {
282278
columns: [
283279
{
@@ -318,16 +314,7 @@ describe('getDataOutMetadata', () => {
318314
const result = await getDataOutMetadata(executionStep)
319315

320316
expect(result).toEqual({
321-
iteration: {
322-
label: 'Item number',
323-
displayedValue: '1',
324-
},
325-
iterations: {
326-
label: 'Items found',
327-
},
328-
inputSource: {
329-
isHidden: true,
330-
},
317+
...GENERIC_TABLE_EXPECTED_RESULT,
331318
items: {
332319
columns: [
333320
{
@@ -351,6 +338,144 @@ describe('getDataOutMetadata', () => {
351338
},
352339
})
353340
})
341+
342+
it.each(['Recruiter', 'sixty', 'itty'])(
343+
'should not hide column when the hex encoded column happens to be a number: %s',
344+
async (columnName) => {
345+
const testHexColumnId = Buffer.from(columnName).toString('hex')
346+
const dataOut = {
347+
iterations: 1,
348+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
349+
items: {
350+
columns: {
351+
[testHexColumnId]: {
352+
id: testHexColumnId,
353+
name: columnName,
354+
value: `items.rows.__ITERATION__.data.${testHexColumnId}`,
355+
order: 1,
356+
},
357+
},
358+
rows: [{ data: { [testHexColumnId]: 'Vaser' } }],
359+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
360+
},
361+
}
362+
const executionStep = createMockExecutionStep(dataOut)
363+
const result = await getDataOutMetadata(executionStep)
364+
365+
expect(result).toEqual({
366+
...GENERIC_TABLE_EXPECTED_RESULT,
367+
items: {
368+
...GENERIC_TABLE_EXPECTED_RESULT.items,
369+
columns: {
370+
[testHexColumnId]: {
371+
id: { isHidden: true },
372+
name: { isHidden: true },
373+
order: { isHidden: true },
374+
value: {
375+
label: columnName,
376+
displayedValue: 'Vaser',
377+
order: 1,
378+
type: 'text',
379+
isHiddenFromList: false,
380+
},
381+
},
382+
},
383+
},
384+
})
385+
},
386+
)
387+
388+
it.each(['Date Sent', 'Send Status'])(
389+
'should not hide columns when the hex encoded column name contains an e and appears like scientific notation: %s',
390+
async (columnName) => {
391+
const testHexColumnId = Buffer.from(columnName).toString('hex')
392+
const dataOut = {
393+
iterations: 1,
394+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
395+
items: {
396+
columns: {
397+
[testHexColumnId]: {
398+
id: testHexColumnId,
399+
name: columnName,
400+
value: `items.rows.__ITERATION__.data.${testHexColumnId}`,
401+
order: 1,
402+
},
403+
},
404+
rows: [{ data: { [testHexColumnId]: 'Vaser' } }],
405+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
406+
},
407+
}
408+
const executionStep = createMockExecutionStep(dataOut)
409+
const result = await getDataOutMetadata(executionStep)
410+
411+
expect(result).toEqual({
412+
...GENERIC_TABLE_EXPECTED_RESULT,
413+
items: {
414+
...GENERIC_TABLE_EXPECTED_RESULT.items,
415+
columns: {
416+
[testHexColumnId]: {
417+
id: { isHidden: true },
418+
name: { isHidden: true },
419+
order: { isHidden: true },
420+
value: {
421+
label: columnName,
422+
displayedValue: 'Vaser',
423+
order: 1,
424+
type: 'text',
425+
isHiddenFromList: false,
426+
},
427+
},
428+
},
429+
},
430+
})
431+
},
432+
)
433+
434+
it.each(['0', '01', '1', '2', '123', '999', 'a'])(
435+
'should not hide column when the column name happens to be a number: %s',
436+
async (columnName) => {
437+
const testHexColumnId = Buffer.from(columnName).toString('hex')
438+
const dataOut = {
439+
iterations: 1,
440+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
441+
items: {
442+
columns: {
443+
[testHexColumnId]: {
444+
id: testHexColumnId,
445+
name: columnName,
446+
value: `items.rows.__ITERATION__.data.${testHexColumnId}`,
447+
order: 1,
448+
},
449+
},
450+
rows: [{ data: { [testHexColumnId]: 'Vaser' } }],
451+
inputSource: FOR_EACH_INPUT_SOURCE.M365_EXCEL,
452+
},
453+
}
454+
const executionStep = createMockExecutionStep(dataOut)
455+
const result = await getDataOutMetadata(executionStep)
456+
457+
expect(result).toEqual({
458+
...GENERIC_TABLE_EXPECTED_RESULT,
459+
items: {
460+
...GENERIC_TABLE_EXPECTED_RESULT.items,
461+
columns: {
462+
[testHexColumnId]: {
463+
id: { isHidden: true },
464+
name: { isHidden: true },
465+
order: { isHidden: true },
466+
value: {
467+
label: columnName,
468+
displayedValue: 'Vaser',
469+
order: 1,
470+
type: 'text',
471+
isHiddenFromList: false,
472+
},
473+
},
474+
},
475+
},
476+
})
477+
},
478+
)
354479
})
355480

356481
describe('when inputSource is TILES', () => {
@@ -386,16 +511,7 @@ describe('getDataOutMetadata', () => {
386511
const result = await getDataOutMetadata(executionStep)
387512

388513
expect(result).toEqual({
389-
iteration: {
390-
label: 'Item number',
391-
displayedValue: '1',
392-
},
393-
iterations: {
394-
label: 'Items found',
395-
},
396-
inputSource: {
397-
isHidden: true,
398-
},
514+
...GENERIC_TABLE_EXPECTED_RESULT,
399515
items: {
400516
columns: [
401517
{
@@ -460,16 +576,11 @@ describe('getDataOutMetadata', () => {
460576
const result = await getDataOutMetadata(executionStep)
461577

462578
expect(result).toEqual({
579+
...GENERIC_TABLE_EXPECTED_RESULT,
463580
iteration: {
464581
label: 'Item number',
465582
displayedValue: '',
466583
},
467-
iterations: {
468-
label: 'Items found',
469-
},
470-
inputSource: {
471-
isHidden: true,
472-
},
473584
items: {
474585
columns: [
475586
{
@@ -525,16 +636,7 @@ describe('getDataOutMetadata', () => {
525636
const result = await getDataOutMetadata(executionStep)
526637

527638
expect(result).toEqual({
528-
iteration: {
529-
label: 'Item number',
530-
displayedValue: '1',
531-
},
532-
iterations: {
533-
label: 'Items found',
534-
},
535-
inputSource: {
536-
isHidden: true,
537-
},
639+
...GENERIC_TABLE_EXPECTED_RESULT,
538640
items: {
539641
columns: [
540642
{
@@ -605,16 +707,7 @@ describe('getDataOutMetadata', () => {
605707
const result = await getDataOutMetadata(executionStep)
606708

607709
expect(result).toEqual({
608-
iteration: {
609-
label: 'Item number',
610-
displayedValue: '1',
611-
},
612-
iterations: {
613-
label: 'Items found',
614-
},
615-
inputSource: {
616-
isHidden: true,
617-
},
710+
...GENERIC_TABLE_EXPECTED_RESULT,
618711
items: {
619712
columns: [
620713
{
@@ -786,16 +879,7 @@ describe('getDataOutMetadata', () => {
786879
const result = await getDataOutMetadata(executionStep)
787880

788881
expect(result).toEqual({
789-
iteration: {
790-
label: 'Item number',
791-
displayedValue: '1',
792-
},
793-
iterations: {
794-
label: 'Items found',
795-
},
796-
inputSource: {
797-
isHidden: true,
798-
},
882+
...GENERIC_TABLE_EXPECTED_RESULT,
799883
items: {
800884
columns: [
801885
{

‎packages/backend/src/apps/toolbox/actions/for-each/get-data-out-metadata.ts‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,19 @@ import {
77

88
import { dataOutSchema } from './schema'
99

10+
// NOTE: this is for backward compatibility with the old dataOut format
11+
const isBackwardCompatibilityColumnId = (id: string, numColumns: number) => {
12+
// must be numeric
13+
// cannot have leading 0
14+
// must only contain digits
15+
// must be less than the number of columns
16+
if (Number(id) < numColumns && !id.startsWith('0') && /^\d+$/.test(id)) {
17+
return true
18+
}
19+
20+
return false
21+
}
22+
1023
async function getDataOutMetadata(
1124
executionStep: IExecutionStep,
1225
): Promise<IDataOutMetadata> {
@@ -78,7 +91,6 @@ async function getDataOutMetadata(
7891
Object.entries(items.columns)
7992
.sort((a, b) => a[1].order - b[1].order)
8093
.forEach(([id, column], index) => {
81-
const isBackwardCompatibilityColumnId = !isNaN(Number(id))
8294
tempColumnsMetadata[id] = {
8395
id: { isHidden: true },
8496
name: { isHidden: true },
@@ -90,7 +102,15 @@ async function getDataOutMetadata(
90102
: String(items?.rows?.[0]?.data?.[column.id] ?? ''),
91103
order: index + 1,
92104
type: column.id === 'rowId' ? 'tile_row_id' : 'text', // NOTE: only tiles will have rowId
93-
isHiddenFromList: isBackwardCompatibilityColumnId,
105+
/**
106+
* NOTE: this is for backward compatibility with the old dataOut format
107+
*
108+
* TODO (kevinkim-ogp): remove this once all users have moved to the new format
109+
*/
110+
isHiddenFromList: isBackwardCompatibilityColumnId(
111+
id,
112+
Object.keys(items.columns).length,
113+
),
94114
},
95115
order: { isHidden: true },
96116
}

‎packages/backend/src/apps/toolbox/common/get-for-each-variables.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ function processColumns(
5151
*
5252
* TODO: remove this once all users have moved the new dataOut format
5353
*/
54+
// do not override the real column id if it already exists
55+
// e.g., when column name is '0', it encodes to '30' for m365-excel and formsg tables
56+
if (processedColumns[String(index)]) {
57+
return
58+
}
5459
processedColumns[String(index)] = {
5560
id: column.id,
5661
name: column.name,

0 commit comments

Comments
 (0)