Skip to content

Commit c0318f7

Browse files
icecrasher321icecrasher321
committed
show errors correctly
1 parent 08b60a5 commit c0318f7

6 files changed

Lines changed: 95 additions & 50 deletions

File tree

‎apps/sim/app/api/organizations/[id]/permission-groups/[groupId]/members/bulk/route.ts‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,14 @@ export const POST = withRouteHandler(
7676
const groupWorkspaceIds = group.appliesToAllWorkspaces
7777
? []
7878
: (await getGroupWorkspaces(id)).map((ws) => ws.id)
79-
const conflictUserIds = new Set(
80-
await findScopeConflicts({
81-
organizationId,
82-
excludeGroupId: id,
83-
appliesToAllWorkspaces: group.appliesToAllWorkspaces,
84-
workspaceIds: groupWorkspaceIds,
85-
candidateUserIds: targetUserIds,
86-
})
87-
)
79+
const conflicts = await findScopeConflicts({
80+
organizationId,
81+
excludeGroupId: id,
82+
appliesToAllWorkspaces: group.appliesToAllWorkspaces,
83+
workspaceIds: groupWorkspaceIds,
84+
candidateUserIds: targetUserIds,
85+
})
86+
const conflictUserIds = new Set(conflicts.map((c) => c.userId))
8887

8988
const { addedUserIds } = await db.transaction(async (tx) => {
9089
const existingInGroup = await tx

‎apps/sim/app/api/organizations/[id]/permission-groups/[groupId]/members/route.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { isOrganizationMember } from '@/lib/workspaces/permissions/utils'
1515
import {
1616
authorizeOrgAccessControl,
1717
findScopeConflicts,
18+
formatScopeConflictError,
1819
getGroupWorkspaces,
1920
loadGroupInOrganization,
2021
} from '@/app/api/organizations/[id]/permission-groups/utils'
@@ -102,14 +103,7 @@ export const POST = withRouteHandler(
102103
candidateUserIds: [userId],
103104
})
104105
if (conflicts.length > 0) {
105-
return NextResponse.json(
106-
{
107-
error: group.appliesToAllWorkspaces
108-
? 'This user already belongs to another all-workspaces group. A user can be in only one all-workspaces group.'
109-
: 'This user already belongs to another group targeting one of these workspaces. A user can have only one group per workspace.',
110-
},
111-
{ status: 409 }
112-
)
106+
return NextResponse.json({ error: formatScopeConflictError(conflicts) }, { status: 409 })
113107
}
114108

115109
const newMember = await db.transaction(async (tx) => {

‎apps/sim/app/api/organizations/[id]/permission-groups/[groupId]/route.ts‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
authorizeOrgAccessControl,
2020
findScopeConflicts,
2121
findWorkspacesNotInOrganization,
22+
formatScopeConflictError,
2223
getGroupWorkspaces,
2324
loadGroupInOrganization,
2425
} from '@/app/api/organizations/[id]/permission-groups/utils'
@@ -158,13 +159,7 @@ export const PUT = withRouteHandler(
158159
candidateUserIds: members.map((m) => m.userId),
159160
})
160161
if (conflicts.length > 0) {
161-
return NextResponse.json(
162-
{
163-
error:
164-
'This scope change would place some members in two groups for the same workspace. Resolve their group memberships first.',
165-
},
166-
{ status: 409 }
167-
)
162+
return NextResponse.json({ error: formatScopeConflictError(conflicts) }, { status: 409 })
168163
}
169164
}
170165

‎apps/sim/app/api/organizations/[id]/permission-groups/utils.test.ts‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ const { mockIsOrganizationAdminOrOwner, mockIsOrganizationOnEnterprisePlan, mock
1010
mockConflictRows: {
1111
value: [] as Array<{
1212
userId: string
13+
userName: string | null
14+
userEmail: string | null
15+
otherGroupId: string
16+
otherGroupName: string
1317
otherAppliesToAll: boolean
1418
otherWorkspaceId: string | null
1519
}>,
@@ -42,6 +46,7 @@ vi.mock('@sim/db/schema', () => ({
4246
permissionGroup: {},
4347
permissionGroupMember: {},
4448
permissionGroupWorkspace: {},
49+
user: {},
4550
workspace: {},
4651
}))
4752

@@ -107,8 +112,18 @@ describe('findScopeConflicts', () => {
107112
candidateUserIds: ['user-1'],
108113
}
109114

115+
/** Build a conflict-query row with sensible defaults. */
116+
const row = (overrides: { otherAppliesToAll: boolean; otherWorkspaceId: string | null }) => ({
117+
userId: 'user-1',
118+
userName: 'User One',
119+
userEmail: 'user-1@example.com',
120+
otherGroupId: 'group-2',
121+
otherGroupName: 'Marketing',
122+
...overrides,
123+
})
124+
110125
it('returns no conflicts when there are no candidate users', async () => {
111-
mockConflictRows.value = [{ userId: 'user-1', otherAppliesToAll: true, otherWorkspaceId: null }]
126+
mockConflictRows.value = [row({ otherAppliesToAll: true, otherWorkspaceId: null })]
112127

113128
const conflicts = await findScopeConflicts({
114129
...baseParams,
@@ -121,21 +136,20 @@ describe('findScopeConflicts', () => {
121136
})
122137

123138
it('flags an all-workspaces target when the user is in another all-workspaces group', async () => {
124-
mockConflictRows.value = [{ userId: 'user-1', otherAppliesToAll: true, otherWorkspaceId: null }]
139+
mockConflictRows.value = [row({ otherAppliesToAll: true, otherWorkspaceId: null })]
125140

126141
const conflicts = await findScopeConflicts({
127142
...baseParams,
128143
appliesToAllWorkspaces: true,
129144
workspaceIds: [],
130145
})
131146

132-
expect(conflicts).toEqual(['user-1'])
147+
expect(conflicts.map((c) => c.userId)).toEqual(['user-1'])
148+
expect(conflicts[0].conflictingGroupName).toBe('Marketing')
133149
})
134150

135151
it('allows an all-workspaces target when the user is only in a specific group', async () => {
136-
mockConflictRows.value = [
137-
{ userId: 'user-1', otherAppliesToAll: false, otherWorkspaceId: 'ws-1' },
138-
]
152+
mockConflictRows.value = [row({ otherAppliesToAll: false, otherWorkspaceId: 'ws-1' })]
139153

140154
const conflicts = await findScopeConflicts({
141155
...baseParams,
@@ -147,23 +161,20 @@ describe('findScopeConflicts', () => {
147161
})
148162

149163
it('flags a specific target that shares a workspace with another specific group', async () => {
150-
mockConflictRows.value = [
151-
{ userId: 'user-1', otherAppliesToAll: false, otherWorkspaceId: 'ws-1' },
152-
]
164+
mockConflictRows.value = [row({ otherAppliesToAll: false, otherWorkspaceId: 'ws-1' })]
153165

154166
const conflicts = await findScopeConflicts({
155167
...baseParams,
156168
appliesToAllWorkspaces: false,
157169
workspaceIds: ['ws-1', 'ws-2'],
158170
})
159171

160-
expect(conflicts).toEqual(['user-1'])
172+
expect(conflicts.map((c) => c.userId)).toEqual(['user-1'])
173+
expect(conflicts[0].conflictingGroupName).toBe('Marketing')
161174
})
162175

163176
it('allows a specific target whose workspaces are disjoint from the user other specific group', async () => {
164-
mockConflictRows.value = [
165-
{ userId: 'user-1', otherAppliesToAll: false, otherWorkspaceId: 'ws-3' },
166-
]
177+
mockConflictRows.value = [row({ otherAppliesToAll: false, otherWorkspaceId: 'ws-3' })]
167178

168179
const conflicts = await findScopeConflicts({
169180
...baseParams,
@@ -175,7 +186,7 @@ describe('findScopeConflicts', () => {
175186
})
176187

177188
it('allows a specific target when the user is only in an all-workspaces group', async () => {
178-
mockConflictRows.value = [{ userId: 'user-1', otherAppliesToAll: true, otherWorkspaceId: null }]
189+
mockConflictRows.value = [row({ otherAppliesToAll: true, otherWorkspaceId: null })]
179190

180191
const conflicts = await findScopeConflicts({
181192
...baseParams,

‎apps/sim/app/api/organizations/[id]/permission-groups/utils.ts‎

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
permissionGroup,
44
permissionGroupMember,
55
permissionGroupWorkspace,
6+
user,
67
workspace,
78
} from '@sim/db/schema'
89
import { and, asc, eq, inArray, ne } from 'drizzle-orm'
@@ -130,20 +131,34 @@ export async function listOrganizationWorkspaces(organizationId: string): Promis
130131
* All-vs-specific never conflicts (specific overrides all for its workspaces).
131132
* The candidate group itself (`excludeGroupId`) is ignored.
132133
*/
134+
/** A member whose other group membership would conflict with a candidate scope. */
135+
export interface ScopeConflict {
136+
userId: string
137+
userName: string | null
138+
userEmail: string | null
139+
/** The group the member already belongs to that causes the conflict. */
140+
conflictingGroupId: string
141+
conflictingGroupName: string
142+
}
143+
133144
export async function findScopeConflicts(params: {
134145
organizationId: string
135146
excludeGroupId: string
136147
appliesToAllWorkspaces: boolean
137148
workspaceIds: string[]
138149
candidateUserIds: string[]
139-
}): Promise<string[]> {
150+
}): Promise<ScopeConflict[]> {
140151
const { organizationId, excludeGroupId, appliesToAllWorkspaces, workspaceIds, candidateUserIds } =
141152
params
142153
if (candidateUserIds.length === 0) return []
143154

144155
const rows = await db
145156
.select({
146157
userId: permissionGroupMember.userId,
158+
userName: user.name,
159+
userEmail: user.email,
160+
otherGroupId: permissionGroup.id,
161+
otherGroupName: permissionGroup.name,
147162
otherAppliesToAll: permissionGroup.appliesToAllWorkspaces,
148163
otherWorkspaceId: permissionGroupWorkspace.workspaceId,
149164
})
@@ -153,6 +168,7 @@ export async function findScopeConflicts(params: {
153168
permissionGroupWorkspace,
154169
eq(permissionGroupWorkspace.permissionGroupId, permissionGroup.id)
155170
)
171+
.leftJoin(user, eq(permissionGroupMember.userId, user.id))
156172
.where(
157173
and(
158174
eq(permissionGroupMember.organizationId, organizationId),
@@ -162,20 +178,42 @@ export async function findScopeConflicts(params: {
162178
)
163179

164180
const targetWorkspaceSet = new Set(workspaceIds)
165-
const conflicts = new Set<string>()
181+
const conflictByUser = new Map<string, ScopeConflict>()
166182

167183
for (const row of rows) {
168-
if (conflicts.has(row.userId)) continue
169-
if (appliesToAllWorkspaces) {
170-
if (row.otherAppliesToAll) conflicts.add(row.userId)
171-
} else if (
172-
!row.otherAppliesToAll &&
173-
row.otherWorkspaceId &&
174-
targetWorkspaceSet.has(row.otherWorkspaceId)
175-
) {
176-
conflicts.add(row.userId)
184+
if (conflictByUser.has(row.userId)) continue
185+
const isConflict = appliesToAllWorkspaces
186+
? row.otherAppliesToAll
187+
: !row.otherAppliesToAll &&
188+
row.otherWorkspaceId !== null &&
189+
targetWorkspaceSet.has(row.otherWorkspaceId)
190+
if (isConflict) {
191+
conflictByUser.set(row.userId, {
192+
userId: row.userId,
193+
userName: row.userName,
194+
userEmail: row.userEmail,
195+
conflictingGroupId: row.otherGroupId,
196+
conflictingGroupName: row.otherGroupName,
197+
})
177198
}
178199
}
179200

180-
return Array.from(conflicts)
201+
return Array.from(conflictByUser.values())
202+
}
203+
204+
/**
205+
* Human-readable 409 message for a scope/membership conflict, naming the member
206+
* and the group they already belong to that overlaps the requested workspaces.
207+
*/
208+
export function formatScopeConflictError(conflicts: ScopeConflict[]): string {
209+
const [first] = conflicts
210+
if (!first) {
211+
return 'A member would be governed by two groups for the same workspace. Resolve their group memberships first.'
212+
}
213+
const who = first.userName || first.userEmail || 'A member'
214+
if (conflicts.length === 1) {
215+
return `${who} is already in the group "${first.conflictingGroupName}", which targets one of these workspaces. Remove them from one group first.`
216+
}
217+
const others = conflicts.length - 1
218+
return `${who} and ${others} other member${others === 1 ? '' : 's'} already belong to groups that target these workspaces (e.g. "${first.conflictingGroupName}"). Resolve their group memberships first.`
181219
}

‎apps/sim/ee/access-control/components/access-control.tsx‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import { useCallback, useMemo, useState } from 'react'
44
import { createLogger } from '@sim/logger'
5+
import { getErrorMessage } from '@sim/utils/errors'
56
import { ArrowRight, ChevronDown, Plus } from 'lucide-react'
67
import { useParams } from 'next/navigation'
78
import {
@@ -31,6 +32,7 @@ import {
3132
Skeleton,
3233
Switch,
3334
Tooltip,
35+
toast,
3436
} from '@/components/emcn'
3537
import { ArrowLeft } from '@/components/emcn/icons'
3638
import { getEnv, isTruthy } from '@/lib/core/config/env'
@@ -973,6 +975,9 @@ export function AccessControl() {
973975
} catch (error) {
974976
logger.error('Failed to update workspace scope', error)
975977
setViewingGroup(previous)
978+
toast.error("Couldn't update workspaces", {
979+
description: getErrorMessage(error, 'Please try again in a moment.'),
980+
})
976981
}
977982
},
978983
[viewingGroup, organizationId, organizationWorkspaces, updatePermissionGroup]
@@ -1000,6 +1005,9 @@ export function AccessControl() {
10001005
)
10011006
} catch (error) {
10021007
logger.error('Failed to toggle default group', error)
1008+
toast.error("Couldn't update the default group", {
1009+
description: getErrorMessage(error, 'Please try again in a moment.'),
1010+
})
10031011
}
10041012
},
10051013
[viewingGroup, organizationId, updatePermissionGroup]

0 commit comments

Comments
 (0)