Skip to content

Commit 9557a90

Browse files
committed
update user groups issue
1 parent 5203c91 commit 9557a90

10 files changed

Lines changed: 1090 additions & 58 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,3 +148,4 @@ Taskfile.yml
148148
/.logs
149149
/logs
150150
webapp_logs.zip
151+
/.schemas

docs/user_groups_issue.md

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
# User Groups Discrepancy Issue
2+
3+
## Summary
4+
5+
We've identified a discrepancy between the database state and API responses when retrieving user groups. When a user requests their groups, they're not receiving all groups where they are members. Specifically, when user ID `1062` requests their groups, they should receive 3 groups (IDs 22, 25, and 28), but only receive 2 groups (IDs 25 and 28).
6+
7+
## Investigation Details
8+
9+
### Debug Logs
10+
11+
The logs show only two groups being returned:
12+
13+
```
14+
2025-05-21 21:38:17,604 - src.routers.user_groups - DEBUG - Fetching groups for user 1062...
15+
2025-05-21 21:38:17,604 - src.database.user_groups - DEBUG - Getting user groups with users for user_id: 1062
16+
2025-05-21 21:38:17,604 - src.database.user_groups - DEBUG - Fetching groups where user 1062 is a member...
17+
2025-05-21 21:38:18,656 - src.database.user_groups - DEBUG - Found 1 groups where user 1062 is a member: [28]
18+
2025-05-21 21:38:18,656 - src.database.user_groups - DEBUG - Fetching groups where user 1062 is an admin...
19+
2025-05-21 21:38:19,606 - src.database.user_groups - DEBUG - Found 1 groups where user 1062 is an admin: [25]
20+
2025-05-21 21:38:19,606 - src.database.user_groups - DEBUG - Added 1 additional groups as admin (not already as member): [25]
21+
2025-05-21 21:38:19,606 - src.database.user_groups - DEBUG - Total: Found 2 user groups with users for user_id: 1062, Group IDs: [25, 28]
22+
```
23+
24+
### Database State
25+
26+
Our database queries confirm that the user should be in 3 groups:
27+
28+
```sql
29+
SELECT id, name, user_ids, admin_ids
30+
FROM user_groups
31+
WHERE 1062 = ANY(user_ids) OR 1062 = ANY(admin_ids)
32+
ORDER BY id;
33+
```
34+
35+
Result:
36+
```
37+
id | name | user_ids | admin_ids
38+
----+-------------+-----------------+-----------
39+
22 | UNDP Studio | {1062,774,1067} | {}
40+
25 | GROUP 3 | {1062} | {1062}
41+
28 | test4 | {774,1062} | {774}
42+
```
43+
44+
### Code Analysis
45+
46+
The code in `user_groups.py` uses the correct SQL syntax to retrieve groups where a user is a member or admin:
47+
48+
1. First query: `SELECT ... FROM user_groups WHERE %s = ANY(user_ids) ORDER BY created_at DESC;`
49+
2. Second query: `SELECT ... FROM user_groups WHERE %s = ANY(admin_ids) ORDER BY created_at DESC;`
50+
51+
These queries should correctly find all groups, but the first query is only returning group 28, not both 22 and 28 as expected.
52+
53+
## Impact
54+
55+
Users may not see all groups they belong to in the application, which could lead to:
56+
57+
1. Reduced access to signals shared in "missing" groups
58+
2. Confusion about group membership
59+
3. Workflow disruptions if users expect to find signals in specific groups
60+
61+
## Possible Causes
62+
63+
1. SQL query execution issues
64+
2. Application-level filtering not visible in the code
65+
3. A caching or stale data issue
66+
4. Transaction isolation level issues
67+
5. Potential race condition if groups are being modified simultaneously
68+
69+
## Fix Implemented
70+
71+
We've implemented a comprehensive solution with multiple layers of improvements:
72+
73+
### 1. Enhanced Primary Functions
74+
75+
Modified the approach in the affected functions to use a single combined query with explicit array handling:
76+
77+
```python
78+
# Run a direct SQL query to ensure array type handling is consistent
79+
query = """
80+
SELECT
81+
id,
82+
name,
83+
signal_ids,
84+
user_ids,
85+
admin_ids,
86+
collaborator_map,
87+
created_at
88+
FROM
89+
user_groups
90+
WHERE
91+
%s = ANY(user_ids) OR %s = ANY(admin_ids)
92+
ORDER BY
93+
created_at DESC;
94+
"""
95+
96+
await cursor.execute(query, (user_id, user_id))
97+
```
98+
99+
We also added explicit type conversion when checking for user membership:
100+
101+
```python
102+
# Track membership rigorously by explicitly converting IDs to integers
103+
is_member = False
104+
if data['user_ids']:
105+
is_member = user_id in [int(uid) for uid in data['user_ids']]
106+
107+
is_admin = False
108+
if data['admin_ids']:
109+
is_admin = user_id in [int(aid) for aid in data['admin_ids']]
110+
```
111+
112+
### 2. Debug Functions
113+
114+
Added a `debug_user_groups` function that runs multiple direct queries to diagnose issues:
115+
116+
```python
117+
async def debug_user_groups(cursor: AsyncCursor, user_id: int) -> dict:
118+
# Various direct SQL queries to check database state
119+
# and array position functions
120+
# ...
121+
```
122+
123+
### 3. Fallback Implementation
124+
125+
Created a completely separate direct SQL implementation in `user_groups_direct.py` as a fallback:
126+
127+
```python
128+
async def get_user_groups_direct(cursor: AsyncCursor, user_id: int) -> List[UserGroup]:
129+
"""
130+
Get all groups that a user is a member of or an admin of using direct SQL.
131+
"""
132+
# Simple, direct SQL with minimal processing
133+
# ...
134+
```
135+
136+
### 4. Automatic Fallback in API
137+
138+
Modified the user groups router to automatically detect and handle discrepancies:
139+
140+
```python
141+
# Check if there's a mismatch between direct query and regular function
142+
if missing_ids:
143+
logger.warning(f"MISMATCH! Direct query found groups {direct_group_ids} but function returned only {fetched_ids}")
144+
logger.warning(f"Missing groups: {missing_ids}")
145+
146+
# Fall back to direct SQL implementation
147+
logger.warning("Falling back to direct SQL implementation")
148+
user_groups = await user_groups_direct.get_user_groups_with_users_direct(cursor, user.id)
149+
logger.info(f"Direct SQL implementation returned {len(user_groups)} groups")
150+
```
151+
152+
These changes ensure:
153+
- More reliable querying of user group memberships
154+
- Better debug information if issues persist
155+
- Automatic fallback to a simpler implementation if needed
156+
- More detailed logging throughout the process
157+
158+
After these changes, users should see all groups where they are members or admins consistently.
159+
160+
## Additional Fix: Signal Entity can_edit Attribute
161+
162+
During testing, we discovered that the Signal entity was missing the `can_edit` attribute that's dynamically added in user group contexts. This caused AttributeError exceptions when trying to access `signal.can_edit`.
163+
164+
### Issue
165+
```python
166+
AttributeError: 'Signal' object has no attribute 'can_edit'
167+
```
168+
169+
### Solution
170+
Added the `can_edit` field to the Signal entity definition:
171+
172+
```python
173+
can_edit: bool = Field(
174+
default=False,
175+
description="Whether the current user can edit this signal (set dynamically based on group membership and collaboration).",
176+
)
177+
```
178+
179+
This ensures that:
180+
- The Signal model accepts the `can_edit` attribute when created
181+
- The attribute defaults to `False` for signals that don't have edit permissions
182+
- Both the regular and direct SQL implementations can properly set this attribute
183+
- Frontend code can safely access `signal.can_edit` without errors
184+
185+
The fix has been applied to both endpoints:
186+
1. `/user-groups/me` (user groups without signals)
187+
2. `/user-groups/me/with-signals` (user groups with signals)
188+
189+
Both endpoints now include the same debug checks and automatic fallback to direct SQL if discrepancies are detected.

src/authentication.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ async def authenticate_user(
142142
token = test_token
143143

144144
# Default user data for local development
145-
local_email = "test.user@undp.org"
146-
name = "Test User"
147-
unit = "Data Futures Exchange (DFx)"
148-
acclab = False
145+
local_email = os.environ.get("TEST_USER_EMAIL", "test.user@undp.org")
146+
name = os.environ.get("TEST_USER_NAME", "Test User")
147+
unit = os.environ.get("TEST_USER_UNIT", "Data Futures Exchange (DFx)")
148+
acclab = os.environ.get("TEST_USER_ACCLAB", False)
149149

150150
# Check for specific test tokens
151151
if token == "test-admin-token":

src/database/signals.py

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,12 @@ async def search_signals(cursor: AsyncCursor, filters: SignalFilters) -> SignalP
9292
AND (%(score)s IS NULL OR score = %(score)s)
9393
AND (%(unit)s IS NULL OR unit_region = %(unit)s OR unit_name = %(unit)s)
9494
AND (%(query)s IS NULL OR text_search_field @@ websearch_to_tsquery('english', %(query)s))
95+
AND (%(user_email)s IS NOT NULL AND (
96+
private = FALSE OR
97+
created_by = %(user_email)s OR
98+
%(is_admin)s = TRUE OR
99+
%(is_staff)s = TRUE
100+
))
95101
ORDER BY
96102
{filters.order_by} {filters.direction}
97103
OFFSET
@@ -128,6 +134,7 @@ async def create_signal(cursor: AsyncCursor, signal: Signal, user_group_ids: Lis
128134
An ID of the signal in the database.
129135
"""
130136
logger.info(f"Creating new signal with headline: '{signal.headline}', created by: {signal.created_by}")
137+
logger.info(f"All Signal fields: {signal.model_dump()}")
131138
if user_group_ids:
132139
logger.info(f"Will add signal to user groups: {user_group_ids}")
133140

@@ -152,7 +159,8 @@ async def create_signal(cursor: AsyncCursor, signal: Signal, user_group_ids: Lis
152159
keywords,
153160
location,
154161
secondary_location,
155-
score
162+
score,
163+
private
156164
)
157165
VALUES (
158166
%(status)s,
@@ -172,7 +180,8 @@ async def create_signal(cursor: AsyncCursor, signal: Signal, user_group_ids: Lis
172180
%(keywords)s,
173181
%(location)s,
174182
%(secondary_location)s,
175-
%(score)s
183+
%(score)s,
184+
%(private)s
176185
)
177186
RETURNING
178187
id
@@ -411,7 +420,8 @@ async def update_signal(cursor: AsyncCursor, signal: Signal, user_group_ids: Lis
411420
keywords = COALESCE(%(keywords)s, keywords),
412421
location = COALESCE(%(location)s, location),
413422
secondary_location = COALESCE(%(secondary_location)s, secondary_location),
414-
score = COALESCE(%(score)s, score)
423+
score = COALESCE(%(score)s, score),
424+
private = COALESCE(%(private)s, private)
415425
WHERE
416426
id = %(id)s
417427
RETURNING
@@ -559,6 +569,8 @@ async def read_user_signals(
559569
cursor: AsyncCursor,
560570
user_email: str,
561571
status: Status,
572+
is_admin: bool = False,
573+
is_staff: bool = False,
562574
) -> list[Signal]:
563575
"""
564576
Read signals from the database using a user email and status filter.
@@ -571,6 +583,10 @@ async def read_user_signals(
571583
An email of the user whose signals to read.
572584
status : Status
573585
A status of signals to filter by.
586+
is_admin : bool, optional
587+
Whether the user is an admin, by default False
588+
is_staff : bool, optional
589+
Whether the user is staff, by default False
574590
575591
Returns
576592
-------
@@ -596,6 +612,9 @@ async def read_user_signals(
596612
created_by = %s AND status = %s
597613
;
598614
"""
615+
# Since this function is explicitly for reading a user's OWN signals,
616+
# we don't need additional private/public filtering here.
617+
# The user is the creator, so they should see all their signals regardless of privacy setting.
599618
await cursor.execute(query, (user_email, status))
600619
rows = await cursor.fetchall()
601620
return [Signal(**row) for row in rows]

0 commit comments

Comments
 (0)