refactor(api): code-review fixes — widen iso() to date, fix import order (R6 S1a)
/code-review high round 1 (5 findings): - iso(dt) now typed `date | None` — MediaAsset.upload_date is a plain `date`, not a datetime; the helper's signature was too narrow for the type-contract epic. +test. - restore alphabetical app.* import order in audit.py and plex.py (the new iso/_common imports had been dropped in mid-block). The other 3 findings are deliberate design choices, left as-is for UAT: push/push-plan now gate write-scope before the 404 (dependency ordering); the unified 403 message; and owned_or_404's owner_attr kept for S1b's DownloadShare.to_user_id-style owners. Gate: ruff clean, pytest 164 green.
This commit is contained in:
@@ -10,9 +10,9 @@ from sqlalchemy.orm import Session
|
||||
from app import audit
|
||||
from app.audit import AuditAction
|
||||
from app.db import get_db
|
||||
from app.utils import iso
|
||||
from app.models import AuditLog, User
|
||||
from app.routes.admin import admin_user
|
||||
from app.utils import iso
|
||||
|
||||
router = APIRouter(prefix="/api/admin/audit", tags=["admin"])
|
||||
|
||||
|
||||
@@ -41,16 +41,16 @@ from app.models import (
|
||||
PlexState,
|
||||
User,
|
||||
)
|
||||
from app.routes._common import owned_or_404
|
||||
from app.utils import iso
|
||||
from app.plex import paths as plex_paths
|
||||
from app.plex import people as plex_people
|
||||
from app.plex import stream as plex_stream
|
||||
from app.plex import sync as plex_sync
|
||||
from app.plex import watch_sync as plex_watch
|
||||
from app.plex.client import PlexClient, PlexError, PlexNotConfigured
|
||||
from app.routes._common import owned_or_404
|
||||
from app.routes.admin import admin_user
|
||||
from app.routes.feed import _to_tsquery_str
|
||||
from app.utils import iso
|
||||
|
||||
log = logging.getLogger("siftlode.plex")
|
||||
|
||||
|
||||
@@ -1,12 +1,13 @@
|
||||
"""Small cross-cutting helpers shared across modules (kept dependency-light on purpose)."""
|
||||
import re
|
||||
from datetime import datetime, timezone
|
||||
from datetime import date, datetime, timezone
|
||||
|
||||
|
||||
def iso(dt: datetime | None) -> str | None:
|
||||
"""Serialize a datetime for a JSON response: ISO-8601, or None when the column is NULL.
|
||||
Single source of truth for the `x.isoformat() if x else None` idiom the routes had
|
||||
hand-rolled ~17 times — so a future tz/format tweak lands in one place."""
|
||||
def iso(dt: date | None) -> str | None:
|
||||
"""Serialize a date/datetime for a JSON response: ISO-8601, or None when the column is
|
||||
NULL. Single source of truth for the `x.isoformat() if x else None` idiom the routes had
|
||||
hand-rolled ~17 times — so a future tz/format tweak lands in one place. Accepts `date`
|
||||
too (datetime is a subclass) — e.g. MediaAsset.upload_date is a plain `date`."""
|
||||
return dt.isoformat() if dt is not None else None
|
||||
|
||||
# Single source of truth for "looks like an email" — used by every endpoint that accepts an
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
"""iso(): the single datetime→JSON serializer the routes share (R6 S1a)."""
|
||||
from datetime import datetime, timezone
|
||||
"""iso(): the single date/datetime→JSON serializer the routes share (R6 S1a)."""
|
||||
from datetime import date, datetime, timezone
|
||||
|
||||
from app.utils import iso
|
||||
|
||||
@@ -8,6 +8,11 @@ def test_none_maps_to_none():
|
||||
assert iso(None) is None
|
||||
|
||||
|
||||
def test_plain_date_serializes_as_iso_date():
|
||||
# MediaAsset.upload_date is a `date`, not a datetime — the contract must accept it.
|
||||
assert iso(date(2026, 7, 26)) == "2026-07-26"
|
||||
|
||||
|
||||
def test_aware_datetime_keeps_offset():
|
||||
dt = datetime(2026, 7, 26, 12, 30, 0, tzinfo=timezone.utc)
|
||||
assert iso(dt) == "2026-07-26T12:30:00+00:00"
|
||||
|
||||
Reference in New Issue
Block a user