Skip to content

Commit 98f135b

Browse files
paulnoirelpaulnoirel
committed
Fix _get_members_set to get role name
1 parent b71471a commit 98f135b

File tree

2 files changed

+113
-69
lines changed

2 files changed

+113
-69
lines changed

‎libs/labelbox/src/labelbox/schema/user_group.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -679,6 +679,12 @@ def _get_members_set(
679679
member_nodes = members_data.get("nodes", [])
680680
user_group_roles = members_data.get("userGroupRoles", [])
681681

682+
# Get all roles to map IDs to names
683+
from labelbox.schema.role import get_roles
684+
685+
all_roles = get_roles(self.client)
686+
role_id_to_role = {role.uid: role for role in all_roles.values()}
687+
682688
# Create a mapping from userId to roleId
683689
user_role_mapping = {
684690
role_data["userId"]: role_data["roleId"]
@@ -694,15 +700,9 @@ def _get_members_set(
694700

695701
# Get the role for this user from the mapping
696702
role_id = user_role_mapping.get(node["id"])
697-
if role_id:
698-
# We need to fetch the role details since we only have the roleId
699-
# For now, create a minimal Role object with just the ID
700-
role_values: defaultdict[str, Any] = defaultdict(lambda: None)
701-
role_values["id"] = role_id
702-
# We don't have the role name from this response, so we'll leave it as None
703-
# The Role object will fetch the name when needed
704-
role = Role(self.client, role_values)
705-
703+
if role_id and role_id in role_id_to_role:
704+
# Use the actual Role object with proper name resolution
705+
role = role_id_to_role[role_id]
706706
members.add(UserGroupMember(user=user, role=role))
707707

708708
return members

‎libs/labelbox/tests/unit/schema/test_user_group.py

Lines changed: 104 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,11 @@ def test_user_group_color_values(self):
100100

101101
class TestUserGroup:
102102
def setup_method(self):
103+
# Reset the global roles cache before each test
104+
import labelbox.schema.role as role_module
105+
106+
role_module._ROLES = None
107+
103108
self.client = MagicMock(Client)
104109
self.client.get_roles.return_value = {
105110
"LABELER": Role(self.client, {"id": "role_id", "name": "LABELER"}),
@@ -167,26 +172,36 @@ def test_get(self):
167172
"orgRole": {"id": "role_id_2", "name": "LABELER"},
168173
},
169174
]
170-
self.client.execute.return_value = {
171-
"userGroupV2": {
172-
"id": "group_id",
173-
"name": "Test Group",
174-
"color": "4ED2F9",
175-
"description": "",
176-
"projects": {
177-
"nodes": projects,
178-
"totalCount": 2,
179-
},
180-
"members": {
181-
"nodes": group_members,
182-
"totalCount": 2,
183-
"userGroupRoles": [
184-
{"userId": "user_id_1", "roleId": "role_id_1"},
185-
{"userId": "user_id_2", "roleId": "role_id_2"},
186-
],
187-
},
188-
}
189-
}
175+
self.client.execute.side_effect = [
176+
# Mock userGroupV2 query response first (get() executes this first)
177+
{
178+
"userGroupV2": {
179+
"id": "group_id",
180+
"name": "Test Group",
181+
"color": "4ED2F9",
182+
"description": "",
183+
"projects": {
184+
"nodes": projects,
185+
"totalCount": 2,
186+
},
187+
"members": {
188+
"nodes": group_members,
189+
"totalCount": 2,
190+
"userGroupRoles": [
191+
{"userId": "user_id_1", "roleId": "role_id_1"},
192+
{"userId": "user_id_2", "roleId": "role_id_2"},
193+
],
194+
},
195+
}
196+
},
197+
# Mock get_roles query response second (_get_members_set calls this)
198+
{
199+
"roles": [
200+
{"id": "role_id_1", "name": "LABELER"},
201+
{"id": "role_id_2", "name": "REVIEWER"},
202+
]
203+
},
204+
]
190205
group = UserGroup(self.client)
191206
assert group.id == ""
192207
assert group.name == ""
@@ -244,7 +259,7 @@ def test_update(self, group_user, group_project, mock_role):
244259
}
245260
}
246261
},
247-
# Mock get query response after update
262+
# Mock get query response after update (get() executes userGroupV2 first)
248263
{
249264
"userGroupV2": {
250265
"id": "group_id",
@@ -270,6 +285,12 @@ def test_update(self, group_user, group_project, mock_role):
270285
},
271286
}
272287
},
288+
# Mock get_roles query response (called by _get_members_set)
289+
{
290+
"roles": [
291+
{"id": "role_id", "name": "LABELER"},
292+
]
293+
},
273294
]
274295

275296
group.update()
@@ -297,7 +318,7 @@ def test_update_without_members_should_work(self, group_project):
297318
}
298319
}
299320
},
300-
# Mock get query response
321+
# Mock get query response (get() executes userGroupV2 first)
301322
{
302323
"userGroupV2": {
303324
"id": "group_id",
@@ -315,6 +336,8 @@ def test_update_without_members_should_work(self, group_project):
315336
},
316337
}
317338
},
339+
# Mock get_roles query response (even though no members, _get_members_set is still called)
340+
{"roles": []},
318341
]
319342

320343
group.update()
@@ -356,42 +379,51 @@ def test_user_groups_empty(self):
356379
assert len(user_groups) == 0
357380

358381
def test_user_groups(self):
359-
self.client.execute.return_value = {
360-
"userGroupsV2": {
361-
"totalCount": 2,
362-
"nextCursor": None,
363-
"nodes": [
364-
{
365-
"id": "group_id_1",
366-
"name": "Group 1",
367-
"color": "9EC5FF",
368-
"description": "",
369-
"projects": {
370-
"nodes": [],
371-
"totalCount": 0,
372-
},
373-
"members": {
374-
"nodes": [],
375-
"totalCount": 0,
376-
},
377-
},
378-
{
379-
"id": "group_id_2",
380-
"name": "Group 2",
381-
"color": "CEB8FF",
382-
"description": "",
383-
"projects": {
384-
"nodes": [],
385-
"totalCount": 0,
382+
# Mock get_user_groups and get_roles responses
383+
# The order is: userGroupsV2 query first, then get_roles when processing groups
384+
self.client.execute.side_effect = [
385+
# get_user_groups query (first)
386+
{
387+
"userGroupsV2": {
388+
"totalCount": 2,
389+
"nextCursor": None,
390+
"nodes": [
391+
{
392+
"id": "group_id_1",
393+
"name": "Group 1",
394+
"color": "9EC5FF",
395+
"description": "",
396+
"projects": {
397+
"nodes": [],
398+
"totalCount": 0,
399+
},
400+
"members": {
401+
"nodes": [],
402+
"totalCount": 0,
403+
"userGroupRoles": [],
404+
},
386405
},
387-
"members": {
388-
"nodes": [],
389-
"totalCount": 0,
406+
{
407+
"id": "group_id_2",
408+
"name": "Group 2",
409+
"color": "CEB8FF",
410+
"description": "",
411+
"projects": {
412+
"nodes": [],
413+
"totalCount": 0,
414+
},
415+
"members": {
416+
"nodes": [],
417+
"totalCount": 0,
418+
"userGroupRoles": [],
419+
},
390420
},
391-
},
392-
],
393-
}
394-
}
421+
],
422+
}
423+
},
424+
# get_roles query (called when processing first group, then cached)
425+
{"roles": []},
426+
]
395427
user_groups = list(UserGroup.get_user_groups(self.client))
396428
assert len(user_groups) == 2
397429
assert user_groups[0].name == "Group 1"
@@ -448,7 +480,7 @@ def test_create(self, group_user, group_project, mock_role):
448480
}
449481
}
450482
},
451-
# Mock get query response after create
483+
# Mock get query response after create (get() executes userGroupV2 first)
452484
{
453485
"userGroupV2": {
454486
"id": "group_id",
@@ -474,6 +506,12 @@ def test_create(self, group_user, group_project, mock_role):
474506
},
475507
}
476508
},
509+
# Mock get_roles query response (called by _get_members_set)
510+
{
511+
"roles": [
512+
{"id": "role_id", "name": "LABELER"},
513+
]
514+
},
477515
]
478516

479517
group.create()
@@ -501,7 +539,7 @@ def test_create_without_members_should_work(self, group_project):
501539
}
502540
}
503541
},
504-
# Mock get query response
542+
# Mock get query response (get() executes userGroupV2 first)
505543
{
506544
"userGroupV2": {
507545
"id": "group_id",
@@ -519,6 +557,8 @@ def test_create_without_members_should_work(self, group_project):
519557
},
520558
}
521559
},
560+
# Mock get_roles query response (even though no members, _get_members_set is still called)
561+
{"roles": []},
522562
]
523563

524564
group.create()
@@ -588,7 +628,7 @@ def test_create_mutation():
588628
}
589629
}
590630
},
591-
# Second call: get query
631+
# Second call: get query (get() executes userGroupV2 first)
592632
{
593633
"userGroupV2": {
594634
"id": "group_id",
@@ -599,6 +639,8 @@ def test_create_mutation():
599639
"members": {"nodes": [], "totalCount": 0, "userGroupRoles": []},
600640
}
601641
},
642+
# Third call: get_roles query (called by _get_members_set)
643+
{"roles": []},
602644
]
603645

604646
group.create()
@@ -646,7 +688,7 @@ def test_update_mutation():
646688
}
647689
}
648690
},
649-
# Second call: get query
691+
# Second call: get query (get() executes userGroupV2 first)
650692
{
651693
"userGroupV2": {
652694
"id": "group_id",
@@ -657,6 +699,8 @@ def test_update_mutation():
657699
"members": {"nodes": [], "totalCount": 0, "userGroupRoles": []},
658700
}
659701
},
702+
# Third call: get_roles query (called by _get_members_set)
703+
{"roles": []},
660704
]
661705

662706
group.update()

0 commit comments

Comments
 (0)