fix(subscriptions): address code review feedback

- Use shared _require_admin from admin_users in subscriptions API endpoint
- Remove unnecessary Alpine.js hidden-div workaround in pricing.html
- Replace fragile string replace for OCR page count with proper Jinja {:,} format
- Improve comment wording in upload quota cleanup code
- Extract _scalar_count() helper in subscription.py to reduce repetition
- Add aria-valuemin='0' to all progressbar elements in subscription/index templates

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

app/api/files.py

@@ -1303,7 +1303,8 @@ async def ui_upload(request: Request, db: DbSession, file: UploadFile = File(...
         try:
             check_upload_allowed(db, upload_owner_id, tier_id)
         except QuotaExceeded as qe:
-            # Clean up the already-saved file before rejecting
+            # Clean up the temporarily written file before returning the error
+            # to avoid consuming disk space for a rejected upload.
             if os.path.exists(target_path):
                 os.remove(target_path)
             raise HTTPException(

app/api/subscriptions.py

@@ -14,6 +14,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request, status
 from sqlalchemy import func
 from sqlalchemy.orm import Session
 
+from app.api.admin_users import _require_admin
 from app.database import get_db
 from app.utils.subscription import (
     TIER_ORDER,
@@ -29,22 +30,7 @@ logger = logging.getLogger(__name__)
 router = APIRouter(prefix="/subscriptions", tags=["subscriptions"])
 
 DbSession = Annotated[Session, Depends(get_db)]
-
+AdminUser = Annotated[dict, Depends(_require_admin)]
-
-# ---------------------------------------------------------------------------
-# Auth helpers
-# ---------------------------------------------------------------------------
-
-
-def _get_current_user(request: Request) -> dict | None:
-    return request.session.get("user")
-
-
-def _require_admin(request: Request) -> dict:
-    user = request.session.get("user")
-    if not user or not user.get("is_admin"):
-        raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required")
-    return user
 
 
 # ---------------------------------------------------------------------------
@@ -67,7 +53,7 @@ def my_subscription(request: Request, db: DbSession) -> dict[str, Any]:
     """Return the authenticated user's subscription tier and current usage counts."""
     from app.config import settings
 
-    user = _get_current_user(request)
+    user = request.session.get("user")
 
     if not settings.multi_user_enabled:
         # In single-user mode there is no concept of a subscription plan
@@ -94,10 +80,8 @@ def my_subscription(request: Request, db: DbSession) -> dict[str, Any]:
 
 
 @router.get("/platform", summary="Platform-wide usage statistics (admin only)")
-def platform_stats(request: Request, db: DbSession) -> dict[str, Any]:
+def platform_stats(request: Request, db: DbSession, _admin: AdminUser) -> dict[str, Any]:
     """Return aggregate statistics across all users and tiers (admin only)."""
-    _require_admin(request)
-
     from app.models import FileRecord, UserProfile
 
     today = datetime.now(timezone.utc).date()

app/utils/subscription.py

@@ -170,15 +170,17 @@ def _today_utc() -> date:
     return datetime.now(timezone.utc).date()
 
 
+def _scalar_count(query) -> int:
+    """Execute a count query and return an int, defaulting to 0 for NULL."""
+    return query.scalar() or 0
+
+
 def get_lifetime_file_count(db: Session, owner_id: str) -> int:
     """Total files ever processed by this user (not counting duplicates)."""
     from app.models import FileRecord
 
-    return (
+    return _scalar_count(
-        db.query(func.count(FileRecord.id))
+        db.query(func.count(FileRecord.id)).filter(FileRecord.owner_id == owner_id, FileRecord.is_duplicate.is_(False))
-        .filter(FileRecord.owner_id == owner_id, FileRecord.is_duplicate.is_(False))
-        .scalar()
-        or 0
     )
 
 
@@ -187,15 +189,12 @@ def get_today_file_count(db: Session, owner_id: str) -> int:
     from app.models import FileRecord
 
     today = _today_utc()
-    return (
+    return _scalar_count(
-        db.query(func.count(FileRecord.id))
+        db.query(func.count(FileRecord.id)).filter(
-        .filter(
             FileRecord.owner_id == owner_id,
             FileRecord.is_duplicate.is_(False),
             func.date(FileRecord.created_at) == today,
         )
-        .scalar()
-        or 0
     )
 
 
@@ -204,15 +203,12 @@ def get_month_file_count(db: Session, owner_id: str) -> int:
     from app.models import FileRecord
 
     today = _today_utc()
-    return (
+    return _scalar_count(
-        db.query(func.count(FileRecord.id))
+        db.query(func.count(FileRecord.id)).filter(
-        .filter(
             FileRecord.owner_id == owner_id,
             FileRecord.is_duplicate.is_(False),
             func.strftime("%Y-%m", FileRecord.created_at) == today.strftime("%Y-%m"),
         )
-        .scalar()
-        or 0
     )
 
 

frontend/templates/index.html

@@ -113,7 +113,7 @@
         </div>
         {% if user_tier.lifetime_file_limit > 0 %}
         {% set pct = ([((user_usage.lifetime / user_tier.lifetime_file_limit) * 100) | int, 100] | min) %}
-        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar"
+        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar" aria-valuemin="0"
              aria-valuenow="{{ user_usage.lifetime }}" aria-valuemax="{{ user_tier.lifetime_file_limit }}">
           <div class="h-2 rounded-full {% if pct >= 90 %}bg-red-500{% elif pct >= 70 %}bg-yellow-500{% else %}bg-green-500{% endif %}"
                style="width: {{ pct }}%"></div>
@@ -132,7 +132,7 @@
         </div>
         {% if user_tier.daily_upload_limit > 0 %}
         {% set pct = ([((user_usage.today / user_tier.daily_upload_limit) * 100) | int, 100] | min) %}
-        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar"
+        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar" aria-valuemin="0"
              aria-valuenow="{{ user_usage.today }}" aria-valuemax="{{ user_tier.daily_upload_limit }}">
           <div class="h-2 rounded-full {% if pct >= 90 %}bg-red-500{% elif pct >= 70 %}bg-yellow-500{% else %}bg-blue-500{% endif %}"
                style="width: {{ pct }}%"></div>
@@ -151,7 +151,7 @@
         </div>
         {% if user_tier.monthly_upload_limit > 0 %}
         {% set pct = ([((user_usage.month / user_tier.monthly_upload_limit) * 100) | int, 100] | min) %}
-        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar"
+        <div class="w-full bg-gray-100 rounded-full h-2" role="progressbar" aria-valuemin="0"
              aria-valuenow="{{ user_usage.month }}" aria-valuemax="{{ user_tier.monthly_upload_limit }}">
           <div class="h-2 rounded-full {% if pct >= 90 %}bg-red-500{% elif pct >= 70 %}bg-yellow-500{% else %}bg-indigo-500{% endif %}"
                style="width: {{ pct }}%"></div>

frontend/templates/pricing.html

@@ -31,9 +31,6 @@
         >
           Annual <span class="ml-1 text-xs bg-green-400 text-green-900 rounded-full px-2 py-0.5 font-bold">Save ~17%</span>
         </button>
-
-        <!-- Tier cards — responsive 4-column grid -->
-        <div class="hidden" x-effect="$el.classList.remove('hidden')"></div>
       </div>
     </div>
   </div>
@@ -221,7 +218,7 @@
                 {% if tier.max_ocr_pages_monthly == 0 %}
                 <span class="font-semibold text-green-600">Unlimited</span>
                 {% else %}
-                <span class="font-medium text-gray-800">{{ tier.max_ocr_pages_monthly | int | string | replace("2500", "2 500") }}</span>
+                <span class="font-medium text-gray-800">{{ "{:,}".format(tier.max_ocr_pages_monthly) }}</span>
                 {% endif %}
               </td>
               {% endfor %}

frontend/templates/subscription.html

@@ -68,7 +68,7 @@
           {% if tier.lifetime_file_limit > 0 %}
           <div class="mt-2">
             {% set lifetime_pct = ((usage.lifetime / tier.lifetime_file_limit) * 100) | int %}
-            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar"
+            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar" aria-valuemin="0"
                  aria-valuenow="{{ usage.lifetime }}" aria-valuemax="{{ tier.lifetime_file_limit }}">
               <div class="h-1.5 rounded-full
                 {% if lifetime_pct >= 90 %}bg-red-500{% elif lifetime_pct >= 70 %}bg-yellow-500{% else %}bg-green-500{% endif %}"
@@ -88,7 +88,7 @@
           {% if tier.daily_upload_limit > 0 %}
           <div class="mt-2">
             {% set today_pct = ((usage.today / tier.daily_upload_limit) * 100) | int %}
-            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar"
+            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar" aria-valuemin="0"
                  aria-valuenow="{{ usage.today }}" aria-valuemax="{{ tier.daily_upload_limit }}">
               <div class="h-1.5 rounded-full
                 {% if today_pct >= 90 %}bg-red-500{% elif today_pct >= 70 %}bg-yellow-500{% else %}bg-blue-500{% endif %}"
@@ -108,7 +108,7 @@
           {% if tier.monthly_upload_limit > 0 %}
           <div class="mt-2">
             {% set month_pct = ((usage.month / tier.monthly_upload_limit) * 100) | int %}
-            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar"
+            <div class="w-full bg-gray-200 rounded-full h-1.5" role="progressbar" aria-valuemin="0"
                  aria-valuenow="{{ usage.month }}" aria-valuemax="{{ tier.monthly_upload_limit }}">
               <div class="h-1.5 rounded-full
                 {% if month_pct >= 90 %}bg-red-500{% elif month_pct >= 70 %}bg-yellow-500{% else %}bg-indigo-500{% endif %}"