-
Notifications
You must be signed in to change notification settings - Fork 10
[COLLAB-CON-1]: getFlowConnections query #1337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop-v2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| import { describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { ForbiddenError } from '@/errors/graphql-errors' | ||
| import getFlowConnections from '@/graphql/queries/get-flow-connections' | ||
| import type Context from '@/types/express/context' | ||
|
|
||
| vi.mock('@/models/app', () => ({ | ||
| default: { | ||
| findAll: vi.fn(() => [ | ||
| { key: 'slack', name: 'Slack', iconUrl: 'https://example.com/slack.png' }, | ||
| { | ||
| key: 'telegram-bot', | ||
| name: 'Telegram', | ||
| iconUrl: 'https://example.com/telegram.png', | ||
| }, | ||
| { | ||
| key: 'tiles', | ||
| name: 'Tables', | ||
| iconUrl: 'https://example.com/tiles.png', | ||
| }, | ||
| ]), | ||
| }, | ||
| })) | ||
|
|
||
| const mockFlowConnectionsQuery = vi.fn() | ||
|
|
||
| vi.mock('@/models/flow-connections', () => ({ | ||
| default: { | ||
| query: () => mockFlowConnectionsQuery(), | ||
| }, | ||
| })) | ||
|
|
||
| const mockFlow = { | ||
| id: 'flow-123', | ||
| role: 'editor', | ||
| } | ||
|
|
||
| const context = { | ||
| currentUser: { | ||
| withAccessibleFlows: vi.fn().mockReturnValue({ | ||
| findById: vi.fn().mockReturnValue(mockFlow), | ||
| }), | ||
| }, | ||
| } as unknown as Context | ||
|
|
||
| describe('getFlowConnections', () => { | ||
| it('should use connection screenName and app key for regular connections', async () => { | ||
| mockFlowConnectionsQuery.mockReturnValue({ | ||
| where: vi.fn().mockReturnThis(), | ||
| withGraphFetched: vi.fn().mockResolvedValue([ | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'conn-456', | ||
| connectionType: 'connection', | ||
| connection: { | ||
| key: 'slack', | ||
| formattedData: { | ||
| screenName: 'My Slack Workspace', | ||
| }, | ||
| }, | ||
| user: { email: 'owner@open.gov.sg' }, | ||
| }, | ||
| ]), | ||
| }) | ||
|
|
||
| const result = await getFlowConnections({}, { flowId: 'flow-123' }, context) | ||
|
|
||
| expect(result).toEqual([ | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'conn-456', | ||
| connectionType: 'connection', | ||
| addedBy: 'owner@open.gov.sg', | ||
| appName: 'Slack', | ||
| appIconUrl: 'https://example.com/slack.png', | ||
| connectionName: 'My Slack Workspace', | ||
| }, | ||
| ]) | ||
| }) | ||
|
|
||
| it('should use table name and "tiles" as appKey for table connections', async () => { | ||
| mockFlowConnectionsQuery.mockReturnValue({ | ||
| where: vi.fn().mockReturnThis(), | ||
| withGraphFetched: vi.fn().mockResolvedValue([ | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'table-789', | ||
| connectionType: 'table', | ||
| table: { | ||
| name: 'My Customer Table', | ||
| }, | ||
| user: { email: 'owner@open.gov.sg' }, | ||
| }, | ||
| ]), | ||
| }) | ||
|
|
||
| const result = await getFlowConnections({}, { flowId: 'flow-123' }, context) | ||
|
|
||
| expect(result).toEqual([ | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'table-789', | ||
| connectionType: 'table', | ||
| addedBy: 'owner@open.gov.sg', | ||
| appName: 'Tables', | ||
| appIconUrl: 'https://example.com/tiles.png', | ||
| connectionName: 'My Customer Table', | ||
| }, | ||
| ]) | ||
| }) | ||
|
|
||
| it('should handle mixed connection types correctly', async () => { | ||
| mockFlowConnectionsQuery.mockReturnValue({ | ||
| where: vi.fn().mockReturnThis(), | ||
| withGraphFetched: vi.fn().mockResolvedValue([ | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'conn-456', | ||
| connectionType: 'connection', | ||
| connection: { | ||
| key: 'telegram-bot', | ||
| formattedData: { | ||
| screenName: 'My Telegram Bot', | ||
| }, | ||
| }, | ||
| user: { | ||
| email: 'owner@open.gov.sg', | ||
| }, | ||
| }, | ||
| { | ||
| flowId: 'flow-123', | ||
| connectionId: 'table-789', | ||
| connectionType: 'table', | ||
| table: { | ||
| name: 'Sales Data', | ||
| }, | ||
| user: { | ||
| email: 'owner@open.gov.sg', | ||
| }, | ||
| }, | ||
| ]), | ||
| }) | ||
|
|
||
| const result = await getFlowConnections({}, { flowId: 'flow-123' }, context) | ||
|
|
||
| expect(result).toHaveLength(2) | ||
| expect(result[0]).toMatchObject({ | ||
| connectionType: 'connection', | ||
| appName: 'Telegram', | ||
| connectionName: 'My Telegram Bot', | ||
| }) | ||
| expect(result[1]).toMatchObject({ | ||
| connectionType: 'table', | ||
| appName: 'Tables', | ||
| connectionName: 'Sales Data', | ||
| }) | ||
| }) | ||
|
|
||
| it('should throw a ForbiddenError if the user does not have access to the flow', async () => { | ||
| context.currentUser.withAccessibleFlows = vi.fn().mockReturnValue({ | ||
| findById: vi.fn().mockReturnValue(null), | ||
| }) | ||
|
|
||
| await expect( | ||
| getFlowConnections({}, { flowId: 'flow-123' }, context), | ||
| ).rejects.toThrow(ForbiddenError) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { IApp } from '@/../../types' | ||
| import { ForbiddenError } from '@/errors/graphql-errors' | ||
| import App from '@/models/app' | ||
| import FlowConnections from '@/models/flow-connections' | ||
|
|
||
| import { QueryResolvers } from '../__generated__/types.generated' | ||
|
|
||
| const getFlowConnections: QueryResolvers['getFlowConnections'] = async ( | ||
| _parent, | ||
| params, | ||
| context, | ||
| ) => { | ||
| const apps = await App.findAll() | ||
|
|
||
| const flow = await context.currentUser | ||
| .withAccessibleFlows({ requiredRole: 'editor' }) | ||
| .findById(params.flowId) | ||
|
|
||
| if (!flow) { | ||
| throw new ForbiddenError( | ||
| 'You do not have sufficient permissions for this pipe', | ||
| ) | ||
| } | ||
|
|
||
| const rawFlowConnections = await FlowConnections.query() | ||
| .where({ | ||
| flow_id: params.flowId, | ||
| }) | ||
| .withGraphFetched({ | ||
| connection: true, | ||
| user: true, | ||
| table: true, | ||
| }) | ||
|
|
||
| const filteredFlowConnections = rawFlowConnections.filter( | ||
| (flowConnection) => { | ||
| if (flowConnection.connectionType === 'table') { | ||
| return !!flowConnection.table | ||
| } | ||
| return !!flowConnection.connection | ||
| }, | ||
| ) | ||
|
|
||
| const flowConnections = await Promise.all( | ||
| filteredFlowConnections.map(async (flowConnection) => { | ||
| let connectionName = flowConnection?.connection?.formattedData?.screenName | ||
| if (flowConnection.connectionType === 'table' && flowConnection.table) { | ||
| connectionName = flowConnection.table?.name | ||
| } | ||
|
|
||
| const appKey = flowConnection.connection?.key | ||
| const app = apps.find( | ||
| (app: IApp) => | ||
| app.key === | ||
| (flowConnection.connectionType === 'table' ? 'tiles' : appKey), | ||
| ) | ||
|
|
||
| if (!app) { | ||
| throw new Error(`App not found for key: ${appKey}`) | ||
| } | ||
|
Comment on lines
+51
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message will be misleading for table connections. When Fix: const appKey = flowConnection.connectionType === 'table'
? 'tiles'
: flowConnection.connection?.key
const app = apps.find((app: IApp) => app.key === appKey)
if (!app) {
throw new Error(`App not found for key: ${appKey}`)
}Spotted by Graphite |
||
|
|
||
| return { | ||
| flowId: flowConnection.flowId, | ||
| connectionId: flowConnection.connectionId, | ||
| connectionType: flowConnection.connectionType, | ||
| addedBy: flowConnection?.user?.email || '', | ||
| appName: app.name, | ||
| appIconUrl: app.iconUrl, | ||
|
kevinkim-ogp marked this conversation as resolved.
|
||
| connectionName: connectionName as string, | ||
| } | ||
| }), | ||
| ) | ||
|
|
||
| return flowConnections | ||
| } | ||
|
|
||
| export default getFlowConnections | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Filter doesn't match connectionType, causing undefined connectionName
The filter keeps records where
connectionORtableexists, but the subsequent logic usesconnectionTypeto determine which relation to read from. IfconnectionTypeis'table'but onlyconnectionexists (or vice versa), the record passes the filter butconnectionNamebecomes undefined. The GraphQL schema declaresconnectionName: String!as non-nullable, so returning undefined will cause a runtime error. The filter needs to verify the correct relation exists based onconnectionType.Additional Locations (1)
packages/backend/src/graphql/queries/get-flow-connections.ts#L33-L37