Skip to content

Commit 1fc70db

Browse files
authored
[PLU-493]: Remove uploaded attachments on duplicate Pipe (#1039)
## Problem Manually uploaded attachments in the Email by Postman step are not handled properly when duplicating Pipe as it is not copied to a new S3 bucket and step parameters are still referencing the old attachment. It causes the following error: ![image](https://github.com/user-attachments/assets/a2618398-ee85-4a0a-b40d-ccaf89999b60) **How to replicate** 1. Create a Pipe with Email by Postman action 2. Upload your own attachment 3. Check step 4. Duplicate the pipe 5. Delete attachment from the action in the duplicated Pipe 6. Check step 7. See the error ## Solution This is a short-term solution to address the issue when duplicating a Pipe: * Remove all manually uploaded attachments from the Pipe's config * Remove all manually uploaded attachments from the Email by Postman's step parameters * Does a check to remove step parameter values that start with `s3:` * Confirmed that only manually uploaded attachments have this prefix in their value **NOTE**: The long-term solution to follow in the future would be to duplicate the attachments in the S3 bucket and update the step parameters in the Email by Postman actions. ## What changed? * Remove `attachments` from the config of the newly duplicated Pipe * Remove all manually uploaded attachments from the Email by Postman's step parameters * Refactor to helper function * Add functionality to handle nested objects * Add tests for the helper functions ## How to test? 1. Create a Pipe with both FormSG and manually uploaded attachments 2. Duplicate this Pipe 3. Open the duplicated Pipe 4. Run 'Check step' on the FormSG step 5. Check that Postman step ONLY contains the FormSG attachments and does not contain the manually uploaded attachment 6. Manually upload an attachment and 'Check step' 7. Verify that email is sent with both form and manually uploaded attachments ## Screenshots https://github.com/user-attachments/assets/7463e2c7-122f-4f8c-9a9f-f865ae767db4
1 parent 4e97f26 commit 1fc70db

File tree

3 files changed

+366
-57
lines changed

3 files changed

+366
-57
lines changed
Lines changed: 296 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,296 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import {
4+
updateStepIdForKey,
5+
updateStepVariables,
6+
} from '@/helpers/update-duplicated-steps'
7+
8+
describe('duplicate flow helper functions', () => {
9+
describe('updateStepIdForKey', () => {
10+
const stepIdMap = {
11+
'old-step-1': 'new-step-1',
12+
'old-step-2': 'new-step-2',
13+
'old-step-3': 'new-step-3',
14+
}
15+
16+
it('should replace step IDs in simple variable references', () => {
17+
const input = '{{step.old-step-1.fields.name}}'
18+
const result = updateStepIdForKey(input, stepIdMap)
19+
expect(result).toBe('{{step.new-step-1.fields.name}}')
20+
})
21+
22+
it('should replace multiple step IDs in the same string', () => {
23+
const input =
24+
'Hello {{step.old-step-1.output.name}} and {{step.old-step-2.fields.email}}'
25+
const result = updateStepIdForKey(input, stepIdMap)
26+
expect(result).toBe(
27+
'Hello {{step.new-step-1.output.name}} and {{step.new-step-2.fields.email}}',
28+
)
29+
})
30+
31+
it('should handle complex variable paths', () => {
32+
const input = '{{step.old-step-1.fields.123.answer}}'
33+
const result = updateStepIdForKey(input, stepIdMap)
34+
expect(result).toBe('{{step.new-step-1.fields.123.answer}}')
35+
})
36+
37+
it('should not affect non-step variables', () => {
38+
const input = 'Hello {{user.name}} and {{config.setting}}'
39+
const result = updateStepIdForKey(input, stepIdMap)
40+
expect(result).toBe('Hello {{user.name}} and {{config.setting}}')
41+
})
42+
43+
it('should handle data-id attributes in HTML/XML', () => {
44+
const input = '<div data-id="step.old-step-1.output">Content</div>'
45+
const result = updateStepIdForKey(input, stepIdMap)
46+
expect(result).toBe('<div data-id="step.new-step-1.output">Content</div>')
47+
})
48+
49+
it('should handle mixed content with variables and plain text', () => {
50+
const input =
51+
'Subject: {{step.old-step-1.fields.subject}} - Reply to {{step.old-step-2.email}}'
52+
const result = updateStepIdForKey(input, stepIdMap)
53+
expect(result).toBe(
54+
'Subject: {{step.new-step-1.fields.subject}} - Reply to {{step.new-step-2.email}}',
55+
)
56+
})
57+
58+
it('should not replace partial matches', () => {
59+
const input = 'step.old-step-12.field should not change'
60+
const result = updateStepIdForKey(input, stepIdMap)
61+
expect(result).toBe('step.old-step-12.field should not change')
62+
})
63+
64+
it('should handle empty string', () => {
65+
const result = updateStepIdForKey('', stepIdMap)
66+
expect(result).toBe('')
67+
})
68+
69+
it('should handle string with no step references', () => {
70+
const input = 'This is just plain text'
71+
const result = updateStepIdForKey(input, stepIdMap)
72+
expect(result).toBe('This is just plain text')
73+
})
74+
75+
it('should handle empty step ID map', () => {
76+
const input = '{{step.old-step-1.fields.name}}'
77+
const result = updateStepIdForKey(input, {})
78+
expect(result).toBe('{{step.old-step-1.fields.name}}')
79+
})
80+
})
81+
82+
describe('updateStepVariables', () => {
83+
const stepIdMap = {
84+
'old-step-1': 'new-step-1',
85+
'old-step-2': 'new-step-2',
86+
}
87+
88+
it('should update string parameters', () => {
89+
const parameters = {
90+
subject: '{{step.old-step-1.fields.title}}',
91+
body: 'Static text',
92+
}
93+
94+
const result = updateStepVariables(parameters, stepIdMap)
95+
96+
expect(result).toEqual({
97+
subject: '{{step.new-step-1.fields.title}}',
98+
body: 'Static text',
99+
})
100+
})
101+
102+
it('should update nested object parameters', () => {
103+
const parameters = {
104+
config: {
105+
url: '{{step.old-step-1.output.url}}',
106+
headers: {
107+
auth: '{{step.old-step-2.token}}',
108+
},
109+
},
110+
static: 'value',
111+
}
112+
113+
const result = updateStepVariables(parameters, stepIdMap)
114+
115+
expect(result).toEqual({
116+
config: {
117+
url: '{{step.new-step-1.output.url}}',
118+
headers: {
119+
auth: '{{step.new-step-2.token}}',
120+
},
121+
},
122+
static: 'value',
123+
})
124+
})
125+
126+
it('should filter out S3 object keys from arrays', () => {
127+
const parameters = {
128+
attachments: [
129+
's3:bucket:file1.pdf',
130+
's3:another-bucket:file2.pdf',
131+
'{{step.old-step-1.attachment}}',
132+
],
133+
}
134+
135+
const result = updateStepVariables(parameters, stepIdMap)
136+
137+
expect(result).toEqual({
138+
attachments: ['{{step.new-step-1.attachment}}'],
139+
})
140+
})
141+
142+
it('should update step variables in array of objects', () => {
143+
const parameters = {
144+
conditions: [
145+
{
146+
field: 'status',
147+
value: '{{step.old-step-1.status}}',
148+
},
149+
{
150+
field: 'email',
151+
value: '{{step.old-step-2.email}}',
152+
},
153+
],
154+
}
155+
156+
const result = updateStepVariables(parameters, stepIdMap)
157+
158+
expect(result).toEqual({
159+
conditions: [
160+
{
161+
field: 'status',
162+
value: '{{step.new-step-1.status}}',
163+
},
164+
{
165+
field: 'email',
166+
value: '{{step.new-step-2.email}}',
167+
},
168+
],
169+
})
170+
})
171+
172+
it('should handle array with only S3 attachments', () => {
173+
const parameters = {
174+
files: ['s3:bucket:file1.pdf', 's3:bucket:file2.pdf'],
175+
}
176+
177+
const result = updateStepVariables(parameters, stepIdMap)
178+
179+
expect(result).toEqual({
180+
files: [],
181+
})
182+
})
183+
184+
it('should handle mixed array with strings and objects', () => {
185+
const parameters = {
186+
mixed: [
187+
'{{step.old-step-1.value}}',
188+
's3:bucket:file.pdf',
189+
{
190+
type: 'condition',
191+
value: '{{step.old-step-2.condition}}',
192+
},
193+
'static-string',
194+
],
195+
}
196+
197+
const result = updateStepVariables(parameters, stepIdMap)
198+
199+
expect(result).toEqual({
200+
mixed: [
201+
'{{step.new-step-1.value}}',
202+
{
203+
type: 'condition',
204+
value: '{{step.new-step-2.condition}}',
205+
},
206+
'static-string',
207+
],
208+
})
209+
})
210+
211+
it('should handle deeply nested structures', () => {
212+
const parameters = {
213+
level1: {
214+
level2: {
215+
level3: {
216+
value: '{{step.old-step-1.deep.value}}',
217+
array: [
218+
{
219+
nested: '{{step.old-step-2.nested}}',
220+
},
221+
],
222+
},
223+
},
224+
},
225+
}
226+
227+
const result = updateStepVariables(parameters, stepIdMap)
228+
229+
expect(result).toEqual({
230+
level1: {
231+
level2: {
232+
level3: {
233+
value: '{{step.new-step-1.deep.value}}',
234+
array: [
235+
{
236+
nested: '{{step.new-step-2.nested}}',
237+
},
238+
],
239+
},
240+
},
241+
},
242+
})
243+
})
244+
245+
it('should handle empty parameters object', () => {
246+
const result = updateStepVariables({}, stepIdMap)
247+
expect(result).toEqual({})
248+
})
249+
250+
it('should handle parameters with null and undefined values', () => {
251+
const parameters: Record<string, any> = {
252+
nullValue: null,
253+
undefinedValue: undefined,
254+
stringValue: '{{step.old-step-1.value}}',
255+
}
256+
257+
const result = updateStepVariables(parameters, stepIdMap)
258+
259+
expect(result).toEqual({
260+
nullValue: null,
261+
undefinedValue: undefined,
262+
stringValue: '{{step.new-step-1.value}}',
263+
})
264+
})
265+
266+
it('should handle parameters with number and boolean values', () => {
267+
const parameters = {
268+
number: 42,
269+
boolean: true,
270+
string: '{{step.old-step-1.value}}',
271+
}
272+
273+
const result = updateStepVariables(parameters, stepIdMap)
274+
275+
expect(result).toEqual({
276+
number: 42,
277+
boolean: true,
278+
string: '{{step.new-step-1.value}}',
279+
})
280+
})
281+
282+
it('should handle empty arrays', () => {
283+
const parameters: Record<string, any> = {
284+
emptyArray: [],
285+
stringValue: '{{step.old-step-1.value}}',
286+
}
287+
288+
const result = updateStepVariables(parameters, stepIdMap)
289+
290+
expect(result).toEqual({
291+
emptyArray: [],
292+
stringValue: '{{step.new-step-1.value}}',
293+
})
294+
})
295+
})
296+
})

packages/backend/src/graphql/mutations/duplicate-flow.ts

Lines changed: 2 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,11 @@
1-
import type { IJSONObject } from '@plumber/types'
2-
31
import { isEmpty } from 'lodash'
42

53
import logger from '@/helpers/logger'
4+
import { updateStepVariables } from '@/helpers/update-duplicated-steps'
65
import Flow from '@/models/flow'
7-
import Step from '@/models/step'
86

97
import { MutationResolvers } from '../__generated__/types.generated'
108

11-
function updateStepIdForKey(
12-
value: string,
13-
oldToNewStepIdsMap: Record<string, string>,
14-
) {
15-
/**
16-
* Example: this step position 3 has a key: "subject" and
17-
* a value of "{{step.123.fields.111.answer}} blah {{step.456.fields.222.answer}}"
18-
* 123 and 456 belongs to the previous 2 steps, so the mapping will be the old steps to new steps
19-
* */
20-
let newValue = value
21-
for (const stepIdMapping of Object.entries(oldToNewStepIdsMap)) {
22-
const [oldStepId, newStepId] = stepIdMapping
23-
// Replaces data-id in postman also and all step variables with the curly braces notation
24-
const partialOldVariable = `step.${oldStepId}.`
25-
const partialNewVariable = `step.${newStepId}.`
26-
27-
newValue = newValue.replaceAll(partialOldVariable, partialNewVariable)
28-
}
29-
return newValue
30-
}
31-
32-
function updateStepVariables(
33-
parameters: Step['parameters'],
34-
oldToNewStepIdsMap: Record<string, string>,
35-
): Step['parameters'] {
36-
const entries = Object.entries(parameters)
37-
return entries.reduce((result, [key, value]) => {
38-
if (typeof value === 'string') {
39-
return {
40-
...result,
41-
[key]: updateStepIdForKey(value, oldToNewStepIdsMap),
42-
}
43-
}
44-
45-
if (Array.isArray(value)) {
46-
return {
47-
...result,
48-
[key]: value.map((item) => {
49-
// could be a string or an object: attachments array would contain strings but conditions would contain objects
50-
if (typeof item === 'string') {
51-
return updateStepIdForKey(item, oldToNewStepIdsMap)
52-
}
53-
return updateStepVariables(item as IJSONObject, oldToNewStepIdsMap)
54-
}),
55-
}
56-
}
57-
58-
return {
59-
...result,
60-
[key]: value,
61-
}
62-
}, {})
63-
}
64-
659
// transaction does 2 things: update duplicate count for flow + duplicate flow + steps
6610
const duplicateFlow: MutationResolvers['duplicateFlow'] = async (
6711
_parent,
@@ -92,6 +36,7 @@ const duplicateFlow: MutationResolvers['duplicateFlow'] = async (
9236
delete prevConfig['duplicateCount']
9337
delete prevConfig['templateConfig']
9438
delete prevConfig['showSurvey']
39+
delete prevConfig['attachments']
9540

9641
const duplicatedFlow = await context.currentUser
9742
.$relatedQuery('flows', trx)

0 commit comments

Comments
 (0)