Skip to content

Commit d86d733

Browse files
authored
fix: editor cannot create excel step (#1329)
## Problem * Editor could not create Excel step even though connection has been shared by Owner * Caused by returning the wrong connection to the Editor * Editor could add step on frontend even though connection has not been shared by Owner (cannot actually create step in the backend, but its confusing for the user) ## Solution * Return the correct Excel connection for the Editor (should see the shared connection instead of own connection) * Should prompt to get ## Tests - [ ] Editor can create excel step for Pipe where Excel connection has already been shared - [ ] Editor sees prompt for Owner to create Excel connection if there is none ## Other misc fixes * Updated dependency array in useCallback
1 parent db3f409 commit d86d733

File tree

10 files changed

+175
-62
lines changed

10 files changed

+175
-62
lines changed

packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ describe('updateStep mutation', () => {
491491
})
492492
})
493493

494-
it('should call patchFlowConnectionMetadata when its an excel app', async () => {
494+
it('should call patchFlowConnectionMetadata or addFlowConnection when its an excel app', async () => {
495495
context.currentUser.withAccessibleSteps = createMockWithAccessibleSteps({
496496
owner,
497497
currentUser: context.currentUser,
@@ -519,9 +519,10 @@ describe('updateStep mutation', () => {
519519
connectionId: mockConnectionId,
520520
parameterKey: 'fileId',
521521
parameterValue: '1234567890',
522+
addedBy: owner.id,
522523
trx: expect.anything(),
523524
})
524-
expect(addSpy).not.toHaveBeenCalled()
525+
expect(addSpy).toHaveBeenCalled()
525526
})
526527

527528
it('should call patchFlowConnectionMetadata when app has connection fields and parameter exists', async () => {

packages/backend/src/graphql/queries/get-app.ts

Lines changed: 70 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,84 @@
11
import App from '@/models/app'
22
import Connection from '@/models/connection'
3+
import Context from '@/types/express/context'
34

45
import type { QueryResolvers } from '../__generated__/types.generated'
56

7+
const getSharedConnections = async (
8+
context: Context,
9+
flowId: string,
10+
key: string,
11+
isDraft: boolean,
12+
) => {
13+
const flow = await context.currentUser
14+
.withAccessibleFlows({ requiredRole: 'viewer' })
15+
.findById(flowId)
16+
.throwIfNotFound({ message: 'Pipe not found' })
17+
18+
const flowConnections = await context.currentUser
19+
.withAccessibleFlowConnections({ requiredRole: 'viewer' })
20+
.join('connections', 'connections.id', 'flow_connections.connection_id')
21+
.withGraphFetched('connection')
22+
.where({
23+
'flows.id': flowId,
24+
'connections.key': key,
25+
'connections.draft': isDraft,
26+
})
27+
28+
if (flowConnections.length === 0) {
29+
return {
30+
flowRole: flow.role,
31+
sharedConnections: [],
32+
}
33+
}
34+
35+
return {
36+
flowRole: flow.role,
37+
sharedConnections: flowConnections
38+
// in the rare case that the connection was deleted or not found
39+
// flowConnection.connection is null, which will throw error
40+
// when we attempt to assign the description
41+
.filter((flowConnection) => flowConnection?.connection)
42+
.map((flowConnection) =>
43+
Object.assign(flowConnection?.connection, {
44+
description: 'This connection can only be used in this pipe.',
45+
}),
46+
),
47+
}
48+
}
49+
650
const getApp: QueryResolvers['getApp'] = async (_parent, params, context) => {
7-
const app = await App.findOneByKey(params.key)
51+
const { flowId, key } = params
52+
const app = await App.findOneByKey(key)
853

954
if (!context.currentUser) {
1055
return app
1156
}
1257

1358
if (app.auth?.connectionType === 'system-added') {
59+
/**
60+
* NOTE: flow id is only provided in the pipe editor.
61+
* it is not provided at the 'My Apps' page, so no need to fetch shared connections
62+
*/
63+
if (flowId) {
64+
const { sharedConnections, flowRole } = await getSharedConnections(
65+
context,
66+
flowId,
67+
key,
68+
true,
69+
)
70+
71+
if (flowRole === 'editor') {
72+
return { ...app, connections: sharedConnections }
73+
}
74+
}
75+
1476
const connections = await app.auth.getSystemAddedConnections(
1577
context.currentUser,
1678
)
1779

80+
// NOTE: we don't merge the connections as only one system-added connection is allowed in a Pipe.
81+
// for example, you cannot use two different Excel connections in one Pipe.
1882
return {
1983
...app,
2084
connections,
@@ -29,28 +93,12 @@ const getApp: QueryResolvers['getApp'] = async (_parent, params, context) => {
2993
*/
3094

3195
if (params.flowId) {
32-
const flow = await context.currentUser
33-
.withAccessibleFlows({ requiredRole: 'viewer' })
34-
.findById(params.flowId)
35-
.throwIfNotFound({ message: 'Pipe not found' })
36-
37-
const flowConnections = await context.currentUser
38-
.withAccessibleFlowConnections({ requiredRole: 'viewer' })
39-
.join('connections', 'connections.id', 'flow_connections.connection_id')
40-
.withGraphFetched('connection')
41-
.where({
42-
'flows.id': params.flowId,
43-
'connections.key': params.key,
44-
'connections.draft': false,
45-
})
46-
47-
sharedConnections = flowConnections.map((flowConnection) =>
48-
Object.assign(flowConnection.connection, {
49-
description: 'This connection can only be used in this pipe.',
50-
}),
51-
)
96+
const { sharedConnections: sharedConnectionsFromFlow, flowRole } =
97+
await getSharedConnections(context, flowId, key, false)
98+
99+
sharedConnections = sharedConnectionsFromFlow
52100

53-
if (flow.role === 'editor') {
101+
if (flowRole === 'editor') {
54102
return {
55103
...app,
56104
connections: sharedConnections,

packages/backend/src/helpers/add-flow-connection.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ async function addFlowConnection({
4040
connectionId,
4141
parameterKey,
4242
parameterValue: step.parameters[parameterKey] as string,
43+
addedBy,
4344
trx,
4445
})
4546
} else {

packages/backend/src/models/flow-connections.ts

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -134,25 +134,39 @@ class FlowConnections extends Base {
134134
connectionId,
135135
parameterKey,
136136
parameterValue,
137+
addedBy,
137138
trx,
138139
}: {
139140
flowId: string
140141
connectionId: string
141142
parameterKey: string
142143
parameterValue: string
144+
addedBy: string
143145
trx?: Transaction
144146
}) => {
145-
return await this.query(trx)
146-
.where({
147-
flow_id: flowId,
148-
connection_id: connectionId,
149-
})
150-
.patch({
151-
// ensure distinct metadata values, we do it in the query to avoid
152-
// having additional queries to get the array, de-duplicate and then
153-
// update the DB
154-
metadata: FlowConnections.raw(
155-
`
147+
/**
148+
* EDGE CASE
149+
* handle edge case where if we delete the M365-Excel connection and add it back via an update step, we need to ensure that the connection exists first.
150+
* new M365-Excel connection will have a new connection id, so it will not exist for patching.
151+
* we need to insert it instead.
152+
*/
153+
const exists = await this.query(trx).findOne({
154+
flow_id: flowId,
155+
connection_id: connectionId,
156+
})
157+
158+
if (exists) {
159+
return await this.query(trx)
160+
.where({
161+
flow_id: flowId,
162+
connection_id: connectionId,
163+
})
164+
.patch({
165+
// ensure distinct metadata values, we do it in the query to avoid
166+
// having additional queries to get the array, de-duplicate and then
167+
// update the DB
168+
metadata: FlowConnections.raw(
169+
`
156170
jsonb_set(
157171
metadata,
158172
?::text[],
@@ -165,9 +179,18 @@ class FlowConnections extends Base {
165179
true
166180
)
167181
`,
168-
[`{${parameterKey}}`, parameterKey, parameterValue],
169-
),
170-
})
182+
[`{${parameterKey}}`, parameterKey, parameterValue],
183+
),
184+
})
185+
}
186+
187+
return await this.addFlowConnection({
188+
flowId,
189+
connectionId,
190+
addedBy,
191+
connectionType: 'connection',
192+
trx,
193+
})
171194
}
172195

173196
/**

packages/frontend/src/components/Editor/constants.ts

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,23 +6,69 @@ export const EDITOR_RIGHT_DRAWER_WIDTH = '60%'
66
export const MIN_FLOW_STEP_WIDTH = '320px'
77

88
/**
9-
* NOTE: there are certain fields that behave like connections
10-
* we use this to manage two things:
11-
* 1. which apps do not allow collaborators to edit their connections
12-
* 2. which fields behave like connections
9+
* Field definition for fields that collaborators cannot add new options for.
10+
* Each field is identified by both its label and key to handle cases where
11+
* different actions may share the same key but have different labels.
12+
*/
13+
type RestrictedField = {
14+
/** display label of the field */
15+
label: string
16+
/** key of the field */
17+
key: string
18+
}
19+
20+
/**
21+
* Purpose:
22+
* - Prevents collaborators from modifying connections
23+
* - Prevents collaborators from adding new options for specified fields
24+
*
25+
* Structure:
26+
* - Key: The app identifier (e.g., 'm365-excel', 'tiles')
27+
* - Value: Array of fields that collaborators cannot add new options for
1328
*
14-
* we do not allow collaborators to edit these fields
15-
* we use both the label and key as there are some
16-
* actions that have the same key but use different labels
29+
* Any key (app) that is specified here will have its connection fields restricted for collaborators.
30+
* The value (array of fields) specifies the fields where collaborators cannot add new options.
31+
*
32+
* Example:
33+
* ```
34+
* {
35+
* // collaborators cannot modify the connection, but can perform use all other fields without restrictions
36+
* // in this case, the collaborator cannot change the M365-Excel connection,
37+
* // but can use all the fields without restrictions
38+
* 'm365-excel': [],
39+
*
40+
* // collaborators cannot modify the connection,
41+
* // collaborators also cannot add new options for these specified fields
42+
* // in this case, the collaborator cannot add new tiles inline via the dropdown
43+
* 'tiles': [
44+
* { label: 'Select Tile', key: 'tableId' },
45+
* ]
46+
* }
47+
* ```
1748
*/
18-
const NON_EDITABLE_APPS_FIELDS: Record<string, Record<string, string>[]> = {
49+
const NON_EDITABLE_APPS_FIELDS: Record<string, RestrictedField[]> = {
50+
/**
51+
* M365-Excel
52+
* collaborators cannot modify the connection, each user can only have one M365-Excel connection.
53+
*/
1954
'm365-excel': [],
55+
56+
/**
57+
* TILES
58+
* collaborators cannot add new Tiles directly via the dropdown.
59+
* they can only select from Tiles that the Owner has used in the Pipe.
60+
*/
61+
tiles: [{ label: 'Select Tile', key: 'tableId' }],
2062
}
2163

2264
export const NON_EDITABLE_APP_CONNECTIONS = Object.keys(
2365
NON_EDITABLE_APPS_FIELDS,
2466
)
2567

26-
export const NON_EDITABLE_CONNECTION_FIELDS = Object.values(
68+
/**
69+
* Flattened array of all fields where collaborators cannot add new options.
70+
* This is derived from NON_EDITABLE_APPS_FIELDS and used for quick field-level checks.
71+
*/
72+
export const COLLABORATOR_RESTRICTED_FIELDS = Object.values(
2773
NON_EDITABLE_APPS_FIELDS,
2874
).flat()

packages/frontend/src/components/EditorLayout/index.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export default function EditorLayout() {
8888
},
8989
})
9090
},
91-
[flow?.id, flowId, updateFlow],
91+
[flow?.updatedAt, flowId, updateFlow],
9292
)
9393

9494
const onFlowStatusUpdate = useCallback(

packages/frontend/src/components/FlowStepConfigurationModal/ChooseAndAddConnection/ConfigureExcelConnection.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
import { Button, ModalCloseButton } from '@opengovsg/design-system-react'
1414

1515
import microsoftFolder from '@/assets/microsoft-folder.svg'
16+
import { EditorContext } from '@/contexts/Editor'
1617
import useAuthentication from '@/hooks/useAuthentication'
1718

1819
import BackButton from '../BackButton'
@@ -134,6 +135,7 @@ export default function ConfigureExcelConnection(
134135
props: ConfigureExcelConnectionProps,
135136
) {
136137
const { onBack, onCreateOrUpdateStep } = props
138+
const { flow } = useContext(EditorContext)
137139
const { modalState, step } = useContext(FlowStepConfigurationContext)
138140
const { selectedApp, selectedConnectionId } = modalState
139141
const connectionModalLabel = selectedApp?.auth?.connectionModalLabel
@@ -190,6 +192,8 @@ export default function ConfigureExcelConnection(
190192
await onCreateOrUpdateStep(selectedConnectionId)
191193
}
192194
/>
195+
) : flow.role !== 'owner' && !selectedConnectionId ? (
196+
<>Only the owner of this Pipe can create an Excel connection</>
193197
) : (
194198
<ConfigurationConnectionContent
195199
userEmail={currentUser?.email ?? ''}

packages/frontend/src/components/FlowStepConfigurationModal/ChooseAppAndEvent/index.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ export default function ChooseAppAndEvent(props: ChooseAppAndEventProps) {
3131
allApps,
3232
onDrawerOpen,
3333
setCurrentStepId,
34+
flowId,
3435
} = useContext(EditorContext)
3536

3637
const { modalState, patchModalState, prevStepId, isTrigger, step } =
@@ -44,7 +45,7 @@ export default function ChooseAppAndEvent(props: ChooseAppAndEventProps) {
4445

4546
// This is used for specifically Excel connections (to skip the connection configuration modal)
4647
const { data: appConnectionsData } = useQuery(GET_APP_CONNECTIONS, {
47-
variables: { key: selectedApp?.key },
48+
variables: { key: selectedApp?.key, flowId },
4849
skip: selectedApp?.key !== EXCEL_APP_KEY,
4950
})
5051
// Check and return the one and only Excel connection

packages/frontend/src/components/InputCreator/index.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { EditorContext } from '@/contexts/Editor'
1313
import { useIsFieldHidden } from '@/helpers/isFieldHidden'
1414
import useDynamicData from '@/hooks/useDynamicData'
1515

16-
import { NON_EDITABLE_CONNECTION_FIELDS } from '../Editor/constants'
16+
import { COLLABORATOR_RESTRICTED_FIELDS } from '../Editor/constants'
1717

1818
import BooleanRadio from './BooleanRadio'
1919

@@ -58,7 +58,7 @@ export default function InputCreator(props: InputCreatorProps): JSX.Element {
5858
const { flow } = useContext(EditorContext)
5959

6060
const canCollaboratorAddNew =
61-
!NON_EDITABLE_CONNECTION_FIELDS.find(
61+
!COLLABORATOR_RESTRICTED_FIELDS.find(
6262
(item) => item.label === label && item.key === name,
6363
) && flow?.role === 'editor'
6464
const isReadOnly =

packages/frontend/src/components/NewsDrawer/NewsItemList.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,6 @@ const IF_THEN_EXTERNAL_LINK =
1111
'https://guide.plumber.gov.sg/user-guides/actions/toolbox'
1212

1313
export const NEWS_ITEM_LIST: NewsItemProps[] = [
14-
{
15-
date: '2025-11-21',
16-
tag: NEW_FEATURE_TAG,
17-
title: 'Collaborators — add editors and viewers to your pipes',
18-
details: dedent`
19-
With collaborators, you can add editors or viewers to help build or troubleshoot your pipe. Read more about permissions and set up [here](https://guide.plumber.gov.sg/user-guides/pipe-settings#collaborators).
20-
`,
21-
multimedia: {
22-
url: 'https://file.go.gov.sg/plumber-collaborators-whats-new.png',
23-
},
24-
},
2514
{
2615
date: '2025-09-22',
2716
tag: NEW_FEATURE_TAG,

0 commit comments

Comments
 (0)