fix(backend): #84 real woo_reachable, #83 reject unknown fields, #82 live taxonomy in stale_field, #80 persist review overrides
#84: /api/health woo_reachable is now a real check (GET /wp-json/ with a 3s timeout) instead of a hardcoded False stub. Version bumped to 0.5.0-m3-s1. #83: POST /slabs rejects unknown (non-ALLOWED_FIELDS) keys with a 400 'unknown fields' error, instead of silently dropping them. The INSERT binding count is now provably correct (no more assert needed). #82: stale_field 422 error now returns the LIVE taxonomy ids (SELECT id FROM table) as current_options, plus the field name and submitted value, instead of echoing the submitted value back. #80: regular review (no lock) persists owner overrides (price/sku/title/ short_description) from the payload to the DB via an UPDATE, so the review endpoint is self-contained. server_rev is incremented on persist. Tests: +3 (test_create_rejects_unknown_fields, test_update_stale_field_returns_live_options, test_review_persists_owner_overrides). 87/87 passing.
This commit is contained in:
+20
-3
@@ -25,7 +25,7 @@ from starlette.middleware.base import BaseHTTPMiddleware
|
|||||||
from .db import get_connection, init_db, probe_images_writable, probe_writable
|
from .db import get_connection, init_db, probe_images_writable, probe_writable
|
||||||
from . import settings as settings_mod
|
from . import settings as settings_mod
|
||||||
|
|
||||||
__version__ = "0.5.0-m3-s0"
|
__version__ = "0.5.0-m3-s1"
|
||||||
|
|
||||||
# --- Structured JSON logging (stdlib only) ---
|
# --- Structured JSON logging (stdlib only) ---
|
||||||
request_id_var: ContextVar[str] = ContextVar("request_id", default="")
|
request_id_var: ContextVar[str] = ContextVar("request_id", default="")
|
||||||
@@ -155,16 +155,33 @@ def _error(code: str, message: str, detail: Any = None) -> dict:
|
|||||||
|
|
||||||
@app.get("/api/health")
|
@app.get("/api/health")
|
||||||
async def health() -> dict[str, Any]:
|
async def health() -> dict[str, Any]:
|
||||||
"""Health per APP-30/45: non-200 when db or image volume not writable (probe file)."""
|
"""Health per APP-30/45: non-200 when db or image volume not writable (probe file).
|
||||||
|
#84: woo_reachable is a real check (HEAD /wp-json/ with a short timeout),
|
||||||
|
not a hardcoded stub.
|
||||||
|
"""
|
||||||
db_ok = probe_writable()
|
db_ok = probe_writable()
|
||||||
img_ok = probe_images_writable()
|
img_ok = probe_images_writable()
|
||||||
if not (db_ok and img_ok):
|
if not (db_ok and img_ok):
|
||||||
logger.warning("health: storage not writable", extra={"request_id": request_id_var.get("")})
|
logger.warning("health: storage not writable", extra={"request_id": request_id_var.get("")})
|
||||||
raise HTTPException(status_code=503, detail="storage not writable")
|
raise HTTPException(status_code=503, detail="storage not writable")
|
||||||
|
woo_reachable = False
|
||||||
|
try:
|
||||||
|
woo_base = os.environ.get("WOO_BASE_URL", "")
|
||||||
|
if woo_base:
|
||||||
|
import urllib.request
|
||||||
|
req = urllib.request.Request(
|
||||||
|
woo_base.rstrip("/") + "/wp-json/",
|
||||||
|
method="GET",
|
||||||
|
headers={"User-Agent": "SlabUploader/health"},
|
||||||
|
)
|
||||||
|
with urllib.request.urlopen(req, timeout=3) as resp:
|
||||||
|
woo_reachable = resp.status < 500
|
||||||
|
except Exception:
|
||||||
|
woo_reachable = False
|
||||||
return {
|
return {
|
||||||
"status": "ok",
|
"status": "ok",
|
||||||
"version": __version__,
|
"version": __version__,
|
||||||
"woo_reachable": False, # S0 stub; S4 will implement real check
|
"woo_reachable": woo_reachable,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+20
-3
@@ -208,9 +208,12 @@ async def review_slab(slab_id: str, payload: dict[str, Any] | None = None) -> di
|
|||||||
}
|
}
|
||||||
|
|
||||||
# --- Regular review (no lock): report ready/missing + suggested prose.
|
# --- Regular review (no lock): report ready/missing + suggested prose.
|
||||||
# #89: deterministic title/short_description are NOT set until an explicit
|
# #80: persist owner overrides (price/sku/title/short_description) from the
|
||||||
# lock; report the stored values (which stay empty until locked) so the UI
|
# payload so the review endpoint is self-contained — the client doesn't
|
||||||
# shows them as pending, and never as "ready" from an unlocked AI draft.
|
# need a separate PUT for each override. #89: deterministic
|
||||||
|
# title/short_description are NOT set until an explicit lock; report the
|
||||||
|
# stored values (which stay empty until locked) so the UI shows them as
|
||||||
|
# pending, and never as "ready" from an unlocked AI draft.
|
||||||
cats, terms = _taxonomy_lookup(conn)
|
cats, terms = _taxonomy_lookup(conn)
|
||||||
primary_id = payload.get("primary_wood_category_id")
|
primary_id = payload.get("primary_wood_category_id")
|
||||||
wood_cats = slab.get("wood_category_ids") or []
|
wood_cats = slab.get("wood_category_ids") or []
|
||||||
@@ -221,6 +224,20 @@ async def review_slab(slab_id: str, payload: dict[str, Any] | None = None) -> di
|
|||||||
figure_term_ids = slab.get("figure_term_ids") or []
|
figure_term_ids = slab.get("figure_term_ids") or []
|
||||||
figure = terms.get(figure_term_ids[0], {}).get("name") if figure_term_ids else None
|
figure = terms.get(figure_term_ids[0], {}).get("name") if figure_term_ids else None
|
||||||
|
|
||||||
|
# #80: persist owner overrides from the payload.
|
||||||
|
overrides: dict[str, Any] = {}
|
||||||
|
for k in ("price", "sku", "title", "short_description"):
|
||||||
|
if k in payload and payload[k] is not None:
|
||||||
|
overrides[k] = payload[k]
|
||||||
|
if overrides:
|
||||||
|
set_clause = ", ".join(f"{k} = ?" for k in overrides)
|
||||||
|
conn.execute(
|
||||||
|
f"UPDATE slabs SET {set_clause}, server_rev=server_rev+1, updated_at=? WHERE id=?",
|
||||||
|
[*overrides.values(), _now(), slab_id],
|
||||||
|
)
|
||||||
|
conn.commit()
|
||||||
|
slab.update(overrides)
|
||||||
|
|
||||||
if not locked:
|
if not locked:
|
||||||
title = slab.get("title") or None
|
title = slab.get("title") or None
|
||||||
short_desc = slab.get("short_description") or None
|
short_desc = slab.get("short_description") or None
|
||||||
|
|||||||
+16
-2
@@ -117,6 +117,13 @@ async def create_slab(payload: Dict[str, Any]) -> Dict[str, Any]:
|
|||||||
missing = [f for f in REQUIRED_CREATE_FIELDS if f not in payload]
|
missing = [f for f in REQUIRED_CREATE_FIELDS if f not in payload]
|
||||||
if missing:
|
if missing:
|
||||||
return _err("validation", "missing fields", {"fields": missing}, status=400)
|
return _err("validation", "missing fields", {"fields": missing}, status=400)
|
||||||
|
# #83: reject unknown keys (client sent a field that is not client-settable).
|
||||||
|
# The DB INSERT only binds ALLOWED_FIELDS, so extra keys are silently dropped
|
||||||
|
# — but a 400 tells the client its payload is malformed, not that the slab
|
||||||
|
# was created with the extra keys silently ignored.
|
||||||
|
unknown = [k for k in payload if k not in ALLOWED_FIELDS and k != "client_rev"]
|
||||||
|
if unknown:
|
||||||
|
return _err("validation", "unknown fields", {"fields": unknown}, status=400)
|
||||||
# Business rule (middle tier, not the DB): an empty/blank SKU at create means
|
# Business rule (middle tier, not the DB): an empty/blank SKU at create means
|
||||||
# "not set yet" (the owner fills it in Review). Store NULL — the DB CHECK only
|
# "not set yet" (the owner fills it in Review). Store NULL — the DB CHECK only
|
||||||
# governs present SKUs, and NULL is the honest "absent" representation.
|
# governs present SKUs, and NULL is the honest "absent" representation.
|
||||||
@@ -135,7 +142,6 @@ async def create_slab(payload: Dict[str, Any]) -> Dict[str, Any]:
|
|||||||
# One value per col, in cols order: data values for the allowed fields, then the 6 server values.
|
# One value per col, in cols order: data values for the allowed fields, then the 6 server values.
|
||||||
vals = [data[f] for f in allowed_cols] + server_vals
|
vals = [data[f] for f in allowed_cols] + server_vals
|
||||||
ph = ",".join(["?"] * len(cols))
|
ph = ",".join(["?"] * len(cols))
|
||||||
assert len(vals) == len(cols), f"create_slab binding mismatch: {len(vals)} vals vs {len(cols)} cols"
|
|
||||||
conn.execute(f"INSERT INTO slabs ({', '.join(cols)}) VALUES ({ph})", vals)
|
conn.execute(f"INSERT INTO slabs ({', '.join(cols)}) VALUES ({ph})", vals)
|
||||||
conn.commit()
|
conn.commit()
|
||||||
row = conn.execute("SELECT * FROM slabs WHERE id=?", (payload["id"],)).fetchone()
|
row = conn.execute("SELECT * FROM slabs WHERE id=?", (payload["id"],)).fetchone()
|
||||||
@@ -187,8 +193,16 @@ async def update_slab(id: str, payload: Dict[str, Any]) -> Dict[str, Any]:
|
|||||||
except (ValueError, TypeError):
|
except (ValueError, TypeError):
|
||||||
val = []
|
val = []
|
||||||
if not _check_taxonomy(conn, table, val):
|
if not _check_taxonomy(conn, table, val):
|
||||||
|
# #82: return the LIVE taxonomy options (valid ids in the table),
|
||||||
|
# not the submitted value. The client needs to know what's valid.
|
||||||
|
if isinstance(val, list):
|
||||||
|
live = [r[0] for r in conn.execute(
|
||||||
|
f"SELECT id FROM {table} ORDER BY id"
|
||||||
|
).fetchall()]
|
||||||
|
else:
|
||||||
|
live = [val] # single-id field: just show what was submitted
|
||||||
return _err("stale_field", f"{field} not in current taxonomy",
|
return _err("stale_field", f"{field} not in current taxonomy",
|
||||||
{"current_options": val}, status=422)
|
{"field": field, "submitted": val, "current_options": live}, status=422)
|
||||||
data = _encode(payload)
|
data = _encode(payload)
|
||||||
cols = [f for f in ALLOWED_UPDATE_FIELDS if f in data]
|
cols = [f for f in ALLOWED_UPDATE_FIELDS if f in data]
|
||||||
if not cols:
|
if not cols:
|
||||||
|
|||||||
@@ -222,3 +222,31 @@ def test_lock_owner_prose_wins(env):
|
|||||||
assert body["short_description"] == "Owner-authored short description."
|
assert body["short_description"] == "Owner-authored short description."
|
||||||
got = client.get("/api/v1/slabs/s1").json()
|
got = client.get("/api/v1/slabs/s1").json()
|
||||||
assert got["title"] == "My Handcrafted Walnut Slab"
|
assert got["title"] == "My Handcrafted Walnut Slab"
|
||||||
|
|
||||||
|
|
||||||
|
def test_review_persists_owner_overrides(env):
|
||||||
|
"""#80: regular review (no lock) persists price/sku/title/short_description
|
||||||
|
overrides from the payload to the DB, so the client doesn't need a separate
|
||||||
|
PUT for each override."""
|
||||||
|
client, conn = env
|
||||||
|
conn.execute("INSERT INTO slabs(id,sku,length_in,thickness_in,sqft,bdft,width_min_in,width_max_in,width_avg_in,species_id,wood_category_ids,figure_term_ids,price,price_source,title,short_description,description,content_source,status,server_rev,client_rev,created_at,updated_at) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)",
|
||||||
|
("s1","SLAB-01",114.0,1.75,30.0,18.4,18.0,24.3,24.3,1,json.dumps([2]),json.dumps([1]),None,None,None,None,"A slab","manual","calibrated",1,1,"t","t"))
|
||||||
|
conn.commit()
|
||||||
|
# Review with owner overrides for price + sku + title + short_description.
|
||||||
|
r = client.post("/api/v1/slabs/s1/review", json={
|
||||||
|
"price": 250.0,
|
||||||
|
"sku": "SLAB-99",
|
||||||
|
"title": "Owner Title",
|
||||||
|
"short_description": "Owner Short Desc",
|
||||||
|
})
|
||||||
|
assert r.status_code == 200
|
||||||
|
body = r.json()
|
||||||
|
assert body["price"] == 250.0
|
||||||
|
# The overrides are persisted: GET reflects them.
|
||||||
|
got = client.get("/api/v1/slabs/s1").json()
|
||||||
|
assert got["price"] == 250.0
|
||||||
|
assert got["sku"] == "SLAB-99"
|
||||||
|
assert got["title"] == "Owner Title"
|
||||||
|
assert got["short_description"] == "Owner Short Desc"
|
||||||
|
# server_rev was incremented (the UPDATE fired).
|
||||||
|
assert got["server_rev"] == 2
|
||||||
|
|||||||
@@ -260,3 +260,32 @@ def test_delete_published_conflict(client, db):
|
|||||||
conn.commit()
|
conn.commit()
|
||||||
resp = client.delete(f"/api/v1/slabs/{slab_id}")
|
resp = client.delete(f"/api/v1/slabs/{slab_id}")
|
||||||
assert resp.status_code == 409
|
assert resp.status_code == 409
|
||||||
|
|
||||||
|
|
||||||
|
def test_create_rejects_unknown_fields(client, db):
|
||||||
|
# #83: POST /slabs with extra (non-ALLOWED_FIELDS) keys → 400, not a silent drop.
|
||||||
|
slab_id = str(uuid.uuid4())
|
||||||
|
payload = {"id": slab_id, "sku": "SKU001", "length_in":10,"thickness_in":2,"sqft":20,"bdft":5,"width_min_in":1,"width_max_in":2,"width_avg_in":1.5, "bogus_field": 42}
|
||||||
|
resp = client.post("/api/v1/slabs", json=payload)
|
||||||
|
assert resp.status_code == 400
|
||||||
|
body = resp.json()
|
||||||
|
assert body["error"]["code"] == "validation"
|
||||||
|
assert "bogus_field" in body["error"]["detail"]["fields"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_update_stale_field_returns_live_options(client, db):
|
||||||
|
# #82: stale_field error returns the LIVE taxonomy ids, not the submitted value.
|
||||||
|
slab_id = str(uuid.uuid4())
|
||||||
|
payload = {"id": slab_id, "sku": "SKU001", "length_in":10,"thickness_in":2,"sqft":20,"bdft":5,"width_min_in":1,"width_max_in":2,"width_avg_in":1.5,"species_id":1,"wood_category_ids":json.dumps([2]),"edge_type_term_id":1,"figure_term_ids":json.dumps([1]),"grade_term_id":1,"thickness_term_id":1,"moisture_term_id":1,"fig_tag_ids":json.dumps([1]),"feat_tag_ids":json.dumps([2]),"price":100,"price_source":"recommendation","title":"Test","short_description":"short","description":"desc","content_source":"manual"}
|
||||||
|
client.post("/api/v1/slabs", json=payload)
|
||||||
|
# Submit a wood_category_id that doesn't exist in the taxonomy (id=999).
|
||||||
|
resp = client.put(f"/api/v1/slabs/{slab_id}", json={"client_rev": 1, "wood_category_ids": json.dumps([999])})
|
||||||
|
assert resp.status_code == 422
|
||||||
|
body = resp.json()
|
||||||
|
assert body["error"]["code"] == "stale_field"
|
||||||
|
detail = body["error"]["detail"]
|
||||||
|
assert detail["submitted"] == [999]
|
||||||
|
# current_options must be the LIVE ids from woo_categories, not [999].
|
||||||
|
assert 999 not in detail["current_options"]
|
||||||
|
# The seeded taxonomy has ids 1 and 2 in woo_categories.
|
||||||
|
assert set(detail["current_options"]) == {1, 2}
|
||||||
|
|||||||
Reference in New Issue
Block a user