From cf8bbad8a17862c96104b40b4221d4dced973059 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:25:40 +0200 Subject: [PATCH] =?UTF-8?q?refactor(api):=20code-review=20fixes=20?= =?UTF-8?q?=E2=80=94=20widen=20iso()=20to=20date,=20fix=20import=20order?= =?UTF-8?q?=20(R6=20S1a)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /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. --- backend/app/routes/audit.py | 2 +- backend/app/routes/plex.py | 4 ++-- backend/app/utils.py | 11 ++++++----- backend/tests/test_utils.py | 9 +++++++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/backend/app/routes/audit.py b/backend/app/routes/audit.py index deffc69..9e7a4ef 100644 --- a/backend/app/routes/audit.py +++ b/backend/app/routes/audit.py @@ -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"]) diff --git a/backend/app/routes/plex.py b/backend/app/routes/plex.py index 97cc4f4..ce0549f 100644 --- a/backend/app/routes/plex.py +++ b/backend/app/routes/plex.py @@ -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") diff --git a/backend/app/utils.py b/backend/app/utils.py index 866e6e0..d266187 100644 --- a/backend/app/utils.py +++ b/backend/app/utils.py @@ -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 diff --git a/backend/tests/test_utils.py b/backend/tests/test_utils.py index 9c6f5d3..3e93b7d 100644 --- a/backend/tests/test_utils.py +++ b/backend/tests/test_utils.py @@ -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"