diff --git a/.jules/bolt.md b/.jules/bolt.md index 1389df8d..7121236c 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -52,4 +52,8 @@ ## 2026-02-11 - Multi-Metric Aggregate Queries **Learning:** Executing multiple separate `count()` queries to gather system statistics results in multiple database round-trips and redundant table scans. -**Action:** Use a single SQLAlchemy query with `func.count()` and `func.sum(case(...))` to calculate all metrics in one go. This reduces network overhead and allows the database to perform calculations in a single pass. +**Action:** Use a single SQLAlchemy query with `func.count()`, `func.sum(case(...))`, and `func.avg()` to calculate all metrics in one go. This reduces network overhead and allows the database to perform calculations in a single pass. + +## 2026-02-28 - Label Mismatch in Aggregate Queries +**Learning:** When using `func.label()` in SQLAlchemy aggregate queries, any mismatch between the SQL label and the Python attribute accessed in the response dictionary will cause an `AttributeError`. +**Action:** Ensure SQL labels in aggregate queries exactly match the keys expected by the API response schema or dictionary mapping. diff --git a/backend/routers/admin.py b/backend/routers/admin.py index 3f72eeaa..44e5c7cc 100644 --- a/backend/routers/admin.py +++ b/backend/routers/admin.py @@ -1,9 +1,8 @@ -from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends from sqlalchemy.orm import Session from sqlalchemy import func, case from typing import List -from sqlalchemy import func, case from backend.database import get_db from backend.models import User, UserRole from backend.schemas import UserResponse @@ -17,25 +16,23 @@ @router.get("/users", response_model=List[UserResponse]) def get_users(skip: int = 0, limit: int = 100, db: Session = Depends(get_db)): - users = db.query(User).offset(skip).limit(limit).all() + """ + Get all users with pagination. + Optimized: Uses column projection to reduce database overhead. + """ + users = db.query( + User.id, + User.email, + User.full_name, + User.role, + User.is_active, + User.created_at + ).offset(skip).limit(limit).all() return users @router.get("/stats") def get_system_stats(db: Session = Depends(get_db)): """ - Get system statistics. - Optimized: Uses a single database query with aggregations to avoid multiple aggregate round-trips. - """ - stats = db.query( - func.count(User.id).label("total_users"), - func.sum(case((User.role == UserRole.ADMIN, 1), else_=0)).label("admin_count"), - func.sum(case((User.is_active.is_(True), 1), else_=0)).label("active_users") - ).first() - - return { - "total_users": stats.total_users or 0, - "admin_count": stats.admin_count or 0, - "active_users": stats.active_users or 0, Get system-wide user statistics. Optimized: Uses a single aggregate query to calculate multiple metrics simultaneously, reducing database round-trips and scan overhead. @@ -45,7 +42,7 @@ def get_system_stats(db: Session = Depends(get_db)): func.sum(case((User.role == UserRole.ADMIN, 1), else_=0)).label("admins"), func.sum(case((User.is_active.is_(True), 1), else_=0)).label("active") ).first() - + return { "total_users": stats.total or 0, "admin_count": int(stats.admins or 0), diff --git a/backend/routers/field_officer.py b/backend/routers/field_officer.py index bfc75f30..9f2d2808 100644 --- a/backend/routers/field_officer.py +++ b/backend/routers/field_officer.py @@ -403,43 +403,28 @@ def get_issue_visit_history( @router.get("/api/field-officer/visit-stats", response_model=VisitStatsResponse) def get_visit_statistics(db: Session = Depends(get_db)): """ - Get aggregate statistics for all field officer visits using optimized SQL queries - - Returns metrics like total visits, verification status, geo-fence compliance, etc. + Get aggregate statistics for all field officer visits using optimized SQL queries. + Optimized: Consolidates 6 individual queries into a single aggregate query to reduce + database round-trips and scan overhead. """ try: - # Use SQL aggregates instead of loading all visits into memory - total_visits = db.query(func.count(FieldOfficerVisit.id)).scalar() or 0 - - verified_visits = db.query(func.count(FieldOfficerVisit.id)).filter( - FieldOfficerVisit.verified_at.isnot(None) - ).scalar() or 0 - - within_geofence_count = db.query(func.count(FieldOfficerVisit.id)).filter( - FieldOfficerVisit.within_geofence == True - ).scalar() or 0 - - outside_geofence_count = db.query(func.count(FieldOfficerVisit.id)).filter( - FieldOfficerVisit.within_geofence == False - ).scalar() or 0 - - unique_officers = db.query(func.count(func.distinct(FieldOfficerVisit.officer_email))).scalar() or 0 - - average_distance = db.query(func.avg(FieldOfficerVisit.distance_from_site)).filter( - FieldOfficerVisit.distance_from_site.isnot(None) - ).scalar() - - # Round to 2 decimals if not None - if average_distance is not None: - average_distance = round(float(average_distance), 2) + # Performance Boost: Single aggregate query for all metrics + stats = db.query( + func.count(FieldOfficerVisit.id).label("total"), + func.sum(case((FieldOfficerVisit.verified_at.isnot(None), 1), else_=0)).label("verified"), + func.sum(case((FieldOfficerVisit.within_geofence == True, 1), else_=0)).label("within"), + func.sum(case((FieldOfficerVisit.within_geofence == False, 1), else_=0)).label("outside"), + func.count(func.distinct(FieldOfficerVisit.officer_email)).label("unique_officers"), + func.avg(FieldOfficerVisit.distance_from_site).label("avg_dist") + ).first() return VisitStatsResponse( - total_visits=total_visits, - verified_visits=verified_visits, - within_geofence_count=within_geofence_count, - outside_geofence_count=outside_geofence_count, - unique_officers=unique_officers, - average_distance_from_site=average_distance + total_visits=stats.total or 0, + verified_visits=int(stats.verified or 0), + within_geofence_count=int(stats.within or 0), + outside_geofence_count=int(stats.outside or 0), + unique_officers=stats.unique_officers or 0, + average_distance_from_site=round(float(stats.avg_dist), 2) if stats.avg_dist is not None else None ) except Exception as e: diff --git a/backend/routers/utility.py b/backend/routers/utility.py index 0f8cd08a..04d4e3d7 100644 --- a/backend/routers/utility.py +++ b/backend/routers/utility.py @@ -1,7 +1,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query from fastapi.responses import JSONResponse from sqlalchemy.orm import Session -from sqlalchemy import func +from sqlalchemy import func, case from datetime import datetime, timezone import logging @@ -49,16 +49,26 @@ def health(): @router.get("/api/stats", response_model=StatsResponse) def get_stats(db: Session = Depends(get_db)): + """ + Get general issue statistics. + Optimized: Uses a single aggregate query for total and resolved counts to reduce + database round-trips. + """ cached_stats = recent_issues_cache.get("stats") if cached_stats: return JSONResponse(content=cached_stats) - total = db.query(func.count(Issue.id)).scalar() - resolved = db.query(func.count(Issue.id)).filter(Issue.status.in_(['resolved', 'verified'])).scalar() - # Pending is everything else + # Performance Boost: Combined total and resolved count in one query + counts = db.query( + func.count(Issue.id).label("total"), + func.sum(case((Issue.status.in_(['resolved', 'verified']), 1), else_=0)).label("resolved") + ).first() + + total = counts.total or 0 + resolved = int(counts.resolved or 0) pending = total - resolved - # By category + # By category (separate group-by query is necessary here) cat_counts = db.query(Issue.category, func.count(Issue.id)).group_by(Issue.category).all() issues_by_category = {cat: count for cat, count in cat_counts} diff --git a/tests/test_bolt_stats.py b/tests/test_bolt_stats.py new file mode 100644 index 00000000..359cca6f --- /dev/null +++ b/tests/test_bolt_stats.py @@ -0,0 +1,124 @@ +import pytest +from fastapi.testclient import TestClient +from backend.main import app +from backend.database import get_db, Base, engine +from backend.models import FieldOfficerVisit, User, UserRole +from sqlalchemy.orm import Session +from datetime import datetime, timezone + +@pytest.fixture +def db_session(): + Base.metadata.create_all(bind=engine) + session = Session(bind=engine) + yield session + session.close() + Base.metadata.drop_all(bind=engine) + +@pytest.fixture +def client(db_session): + app.dependency_overrides[get_db] = lambda: db_session + # Mock admin user for stats access + from backend.dependencies import get_current_admin_user + app.dependency_overrides[get_current_admin_user] = lambda: User(email="admin@example.com", role=UserRole.ADMIN) + + with TestClient(app) as c: + yield c + app.dependency_overrides = {} + +def test_get_visit_statistics(client, db_session): + # Add some visits + v1 = FieldOfficerVisit( + issue_id=1, + officer_email="off1@example.com", + officer_name="Officer 1", + check_in_latitude=19.0, + check_in_longitude=72.0, + check_in_time=datetime.now(timezone.utc), + verified_at=datetime.now(timezone.utc), + within_geofence=True, + distance_from_site=10.5, + status="verified" + ) + v2 = FieldOfficerVisit( + issue_id=2, + officer_email="off1@example.com", + officer_name="Officer 1", + check_in_latitude=19.1, + check_in_longitude=72.1, + check_in_time=datetime.now(timezone.utc), + within_geofence=False, + distance_from_site=150.0, + status="checked_in" + ) + v3 = FieldOfficerVisit( + issue_id=3, + officer_email="off2@example.com", + officer_name="Officer 2", + check_in_latitude=19.2, + check_in_longitude=72.2, + check_in_time=datetime.now(timezone.utc), + within_geofence=True, + distance_from_site=20.0, + status="checked_in" + ) + db_session.add_all([v1, v2, v3]) + db_session.commit() + + response = client.get("/api/field-officer/visit-stats") + assert response.status_code == 200 + data = response.json() + + assert data["total_visits"] == 3 + assert data["verified_visits"] == 1 + assert data["within_geofence_count"] == 2 + assert data["outside_geofence_count"] == 1 + assert data["unique_officers"] == 2 + # (10.5 + 150.0 + 20.0) / 3 = 180.5 / 3 = 60.1666... -> 60.17 + assert data["average_distance_from_site"] == 60.17 + +def test_get_system_stats(client, db_session): + # UserRole.ADMIN is already imported + u1 = User(email="admin1@example.com", hashed_password="pw", role=UserRole.ADMIN, is_active=True) + u2 = User(email="user1@example.com", hashed_password="pw", role=UserRole.USER, is_active=True) + u3 = User(email="user2@example.com", hashed_password="pw", role=UserRole.USER, is_active=False) + + db_session.add_all([u1, u2, u3]) + db_session.commit() + + response = client.get("/admin/stats") + assert response.status_code == 200 + data = response.json() + + assert data["total_users"] == 3 + assert data["admin_count"] == 1 + assert data["active_users"] == 2 + +def test_get_utility_stats(client, db_session): + from backend.models import Issue + i1 = Issue(description="Issue 1", category="Road", status="resolved") + i2 = Issue(description="Issue 2", category="Water", status="open") + i3 = Issue(description="Issue 3", category="Road", status="verified") + + db_session.add_all([i1, i2, i3]) + db_session.commit() + + response = client.get("/api/stats") + assert response.status_code == 200 + data = response.json() + + assert data["total_issues"] == 3 + assert data["resolved_issues"] == 2 + assert data["pending_issues"] == 1 + assert data["issues_by_category"]["Road"] == 2 + assert data["issues_by_category"]["Water"] == 1 + +def test_get_users_admin(client, db_session): + u1 = User(email="u1@example.com", hashed_password="pw", role=UserRole.USER, is_active=True) + db_session.add(u1) + db_session.commit() + + response = client.get("/admin/users") + assert response.status_code == 200 + data = response.json() + assert len(data) >= 1 + assert any(u["email"] == "u1@example.com" for u in data)