Skip to content

Commit 7d83386

Browse files
Staging -> Production: changes since May 2025 (#11)
* Feature: User favourites (#2) * Feature: initial routers / methods for favourites - add env documentation / adminer image for easier inspection of db - create / delete favourite entry in database - fetch list of favourites by user * Chore: update formatting, tests * update test methods / logging - * updates * expand tests / logging * env updates * .env updates (#8) * env updates * update example env * initial routes * update methods * Add secondary country to signals data (#10) * initial routes * update methods * Fix: Update Python setup action to v4 and fix cache configuration * Update signal.py * Collaborative signal editing (user groups etc) (#9) * full routers / entities / test setup * update methods * Delete signal_collaborators.sql --------- Co-authored-by: amaguire-undp <andrew.maguire@undp.org> * add endpoints to get auth user's groups + signals * add emails to user group routes * configure application with bugsnag * update app configuration with bugsnag * Enhance user groups functionality and improve API robustness * update user groups calls * update user group calls * Update .gitignore * update user groups issue --------- Co-authored-by: amaguire-undp <andrew.maguire@undp.org>
1 parent 1e0324f commit 7d83386

13 files changed

Lines changed: 2517 additions & 345 deletions

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,3 +146,6 @@ create_test_user.sql
146146
Taskfile.yml
147147
.env.local
148148
/.logs
149+
/logs
150+
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.

main.py

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from fastapi import Depends, FastAPI, Request
1111
from fastapi.middleware.cors import CORSMiddleware
1212
from fastapi.responses import JSONResponse
13+
from starlette.middleware.base import BaseHTTPMiddleware
1314

1415
from src import routers
1516
from src.authentication import authenticate_user
@@ -23,6 +24,9 @@
2324
# Get application version
2425
app_version = os.environ.get("RELEASE_VERSION", "dev")
2526
app_env = os.environ.get("ENVIRONMENT", "development")
27+
# Override environment setting if in local mode
28+
if os.environ.get("ENV_MODE") == "local":
29+
app_env = "local"
2630
logging.info(f"Starting application - version: {app_version}, environment: {app_env}")
2731

2832
# Configure Bugsnag for error tracking
@@ -90,14 +94,61 @@ async def global_exception_handler(request: Request, exc: Exception):
9094
content={"detail": "Internal server error"},
9195
)
9296

93-
# allow cors
94-
app.add_middleware(
95-
CORSMiddleware,
96-
allow_origins=["*"],
97-
allow_credentials=True,
98-
allow_methods=["*"],
99-
allow_headers=["*"],
100-
)
97+
# Configure CORS - simplified for local development
98+
local_origins = [
99+
"http://localhost:5175",
100+
"http://127.0.0.1:5175",
101+
"http://localhost:3000",
102+
"http://127.0.0.1:3000"
103+
]
104+
105+
# Create a custom middleware class for handling CORS
106+
class CORSHandlerMiddleware(BaseHTTPMiddleware):
107+
async def dispatch(self, request: Request, call_next):
108+
# Handle OPTIONS preflight requests
109+
if request.method == "OPTIONS":
110+
headers = {
111+
"Access-Control-Allow-Origin": "*",
112+
"Access-Control-Allow-Methods": "GET, POST, PUT, DELETE, OPTIONS, PATCH",
113+
"Access-Control-Allow-Headers": "access_token, Authorization, Content-Type, Accept, X-API-Key",
114+
"Access-Control-Allow-Credentials": "true",
115+
"Access-Control-Max-Age": "600", # Cache preflight for 10 minutes
116+
}
117+
118+
# Set specific origin if in local mode
119+
origin = request.headers.get("origin")
120+
if os.environ.get("ENV_MODE") == "local" and origin:
121+
headers["Access-Control-Allow-Origin"] = origin
122+
123+
return JSONResponse(content={}, status_code=200, headers=headers)
124+
125+
# Process all other requests normally
126+
response = await call_next(request)
127+
return response
128+
129+
# Apply custom CORS middleware BEFORE the standard CORS middleware
130+
app.add_middleware(CORSHandlerMiddleware)
131+
132+
# Standard CORS middleware (as a backup)
133+
if os.environ.get("ENV_MODE") == "local":
134+
logging.info(f"Local mode: using specific CORS origins: {local_origins}")
135+
app.add_middleware(
136+
CORSMiddleware,
137+
allow_origins=local_origins,
138+
allow_credentials=True,
139+
allow_methods=["*"],
140+
allow_headers=["*", "access_token", "Authorization", "Content-Type"],
141+
expose_headers=["*"],
142+
)
143+
else:
144+
# Production mode - use more restrictive CORS
145+
app.add_middleware(
146+
CORSMiddleware,
147+
allow_origins=["*"],
148+
allow_credentials=True,
149+
allow_methods=["*"],
150+
allow_headers=["*", "access_token", "Authorization", "Content-Type"],
151+
)
101152

102153
# Add Bugsnag exception handling middleware
103154
# Important: Add middleware AFTER registering exception handlers
@@ -137,5 +188,11 @@ async def test_error():
137188
else:
138189
return {"status": "disabled", "message": "Bugsnag is not enabled"}
139190

191+
# Add special route for handling OPTIONS requests to /users/me
192+
@app.options("/users/me", include_in_schema=False)
193+
async def options_users_me():
194+
"""Handle OPTIONS requests to /users/me specifically."""
195+
return {}
196+
140197
# Use the Bugsnag middleware wrapped app for ASGI
141198
app = bugsnag_app

0 commit comments

Comments
 (0)