upload files without API-Key

tests/test_review_inbox.py

@@ -0,0 +1,339 @@
+"""The admin half of the review inbox.
+
+    GET  /api/admin/catalog-batch/inbox           - drops awaiting review
+    POST /api/admin/catalog-batch/from-inbox      - run selected files
+    POST /api/admin/catalog-batch/inbox/dismiss   - discard selected files
+
+The sender half lives in test_uploads_api.py. What matters here is the gate
+between them: a drop arrives with no credential and runs only when an admin
+says so, and every assertion below is ultimately about one of two failures -
+
+*   something running that nobody approved, and
+*   a file being lost, or run twice, on its way out of the inbox.
+
+The response SHAPES are asserted literally rather than loosely, because
+frontend/src/pages/InboxPanel.jsx was written against this contract before the
+backend existed. `submission_id`, `file_id`, `pending_count` and the `dismissed`
+count are read by name there; a rename that only this file catches is cheap, and
+one that nothing catches is a blank admin tab with no error.
+"""
+from __future__ import annotations
+
+import io
+
+import pytest
+
+from app.api.batch_job_store import batch_job_store
+from app.core import batch_ingest
+
+UPLOAD = "/api/uploads/catalog"
+INBOX = "/api/admin/catalog-batch/inbox"
+FROM_INBOX = "/api/admin/catalog-batch/from-inbox"
+DISMISS = "/api/admin/catalog-batch/inbox/dismiss"
+
+HEADERS = ["Product Name", "Category", "Brand"]
+ROWS = [["Amul Butter 100g", "Butter", "Amul"]]
+
+
+def _csv(rows=ROWS) -> bytes:
+    return ("\n".join([",".join(HEADERS)] + [",".join(r) for r in rows])).encode()
+
+
+def _files(*pairs):
+    return [("files", (n, io.BytesIO(c), "application/octet-stream"))
+            for n, c in pairs]
+
+
+@pytest.fixture(autouse=True)
+def _isolate_sku_counter(tmp_path, monkeypatch):
+    from app.services import sku_service
+    monkeypatch.setattr(sku_service, "_data_dir", tmp_path / "sku_sequences")
+
+
+@pytest.fixture(autouse=True)
+def batch_root(tmp_path, monkeypatch):
+    root = tmp_path / "batch_uploads"
+    monkeypatch.setattr(batch_ingest, "BATCH_UPLOAD_DIR", root)
+    return root
+
+
+@pytest.fixture(autouse=True)
+def submitted(monkeypatch):
+    """Stub the worker and record what reached it.
+
+    Turns "did anything run?" into a direct assertion. A real worker would also
+    resolve BATCH_UPLOAD_DIR again after teardown and write into the repo's
+    data/ directory - see test_batch_catalog_ingest.
+    """
+    from app.core import batch_worker
+
+    seen: list = []
+    monkeypatch.setattr(batch_worker, "submit", seen.append)
+    return seen
+
+
+@pytest.fixture(autouse=True)
+def _clean_job_store():
+    batch_job_store._batches.clear()
+    batch_job_store._cancelled.clear()
+    yield
+    batch_job_store._batches.clear()
+    batch_job_store._cancelled.clear()
+
+
+@pytest.fixture
+def upload_headers(monkeypatch) -> dict:
+    """An `uploader` key, for asserting what it may NOT do here.
+
+    Patched on `security`, not on `settings`: security.py binds the dict object
+    at import, so rebinding the name in `settings` leaves principal_for_api_key
+    looking at the original and every request comes back 401.
+    """
+    from app.infrastructure import security
+
+    secret = "k" * 43
+    monkeypatch.setattr(security, "API_KEYS", {secret: ("catalog-drop", "uploader")})
+    return {"X-API-Key": secret}
+
+
+def _drop(client, sender: str, *names: str) -> str:
+    """Post a drop the way a colleague would - no credential - and return its id."""
+    response = client.post(
+        UPLOAD,
+        files=_files(*[(n, _csv()) for n in (names or ("a.csv",))]),
+        data={"sender": sender},
+    )
+    assert response.status_code == 202, response.text
+    return response.json()["batch_id"]
+
+
+# ---------------------------------------------------------------------------
+# Listing
+# ---------------------------------------------------------------------------
+def test_a_drop_appears_in_the_inbox(client, admin_headers):
+    batch_id = _drop(client, "priya", "catalog.csv")
+
+    body = client.get(INBOX, headers=admin_headers).json()
+
+    assert body["pending_count"] == 1
+    assert len(body["submissions"]) == 1
+    submission = body["submissions"][0]
+    assert submission["submission_id"] == batch_id
+    assert submission["submitted_by"] == "priya"
+    assert submission["created_at"] > 0
+    assert submission["files"][0]["file_id"] == f"{batch_id}:0"
+    assert submission["files"][0]["filename"] == "catalog.csv"
+    assert submission["files"][0]["rows_total"] == 1
+    assert submission["files"][0]["size_bytes"] > 0
+
+
+def test_an_empty_inbox_is_empty_not_an_error(client, admin_headers):
+    body = client.get(INBOX, headers=admin_headers).json()
+    assert body == {"pending_count": 0, "submissions": []}
+
+
+def test_the_inbox_survives_a_restart(client, admin_headers):
+    """Read from disk, not from the in-process cache.
+
+    The job store is a module singleton populated by what this process has seen.
+    An inbox backed by it would empty itself on every deploy, which looks exactly
+    like a colleague's files having been dealt with.
+    """
+    _drop(client, "priya")
+    batch_job_store._batches.clear()
+
+    assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 1
+
+
+def test_a_rejected_file_is_not_offered_for_selection(client, admin_headers):
+    """A file the upload already refused is on the manifest so the sender can be
+    told, but it has no bytes and cannot be run."""
+    response = client.post(
+        UPLOAD, files=_files(("good.csv", _csv()), ("bad.txt", b"nope")),
+    )
+    assert response.status_code == 202, response.text
+
+    body = client.get(INBOX, headers=admin_headers).json()
+    names = [f["filename"] for f in body["submissions"][0]["files"]]
+    assert names == ["good.csv"]
+    assert body["pending_count"] == 1
+
+
+def test_a_restart_does_not_relabel_the_inbox_interrupted(client, admin_headers):
+    """scan_interrupted() sweeps every non-terminal manifest.
+
+    Without its PENDING exemption, every boot would mark the whole inbox
+    "interrupted" and offer an admin a Resume button for work nobody started.
+    """
+    _drop(client, "priya")
+
+    assert batch_ingest.scan_interrupted() == []
+    assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 1
+
+
+def test_pending_drops_are_not_listed_as_runs(client, admin_headers):
+    """The Batch tab lists runs. A drop has never been near the pipeline, and a
+    row there with no progress and a Resume button would be a lie."""
+    _drop(client, "priya")
+
+    batches = client.get("/api/admin/catalog-batch/batches",
+                         headers=admin_headers).json()["batches"]
+    assert batches == []
+
+
+# ---------------------------------------------------------------------------
+# Starting
+# ---------------------------------------------------------------------------
+def test_starting_a_file_queues_it_and_empties_the_inbox(client, admin_headers, submitted):
+    batch_id = _drop(client, "priya", "catalog.csv")
+
+    started = client.post(FROM_INBOX, json={"file_ids": [f"{batch_id}:0"]},
+                          headers=admin_headers)
+
+    assert started.status_code == 202, started.text
+    run = started.json()
+    assert run["files_total"] == 1
+    assert run["submitted_by"] == "priya"
+    # A NEW id: the run is a distinct thing from the drop it came from.
+    assert run["batch_id"] != batch_id
+    assert submitted == [run["batch_id"]]
+
+    assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 0
+
+
+def test_files_from_two_drops_become_one_batch(client, admin_headers, submitted):
+    """The reason selection is per file rather than per drop."""
+    first = _drop(client, "priya", "monday.csv")
+    second = _drop(client, "arun", "today.csv")
+
+    started = client.post(
+        FROM_INBOX,
+        json={"file_ids": [f"{first}:0", f"{second}:0"]},
+        headers=admin_headers,
+    )
+
+    assert started.status_code == 202, started.text
+    run = started.json()
+    assert run["files_total"] == 2
+    assert sorted(f["filename"] for f in run["files"]) == ["monday.csv", "today.csv"]
+    # Both senders are preserved - the question gets asked precisely when a
+    # batch was assembled out of several drops.
+    assert run["submitted_by"] == "arun, priya"
+    assert len(submitted) == 1
+    assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 0
+
+
+def test_starting_one_file_leaves_its_siblings_in_the_inbox(client, admin_headers):
+    batch_id = _drop(client, "priya", "one.csv", "two.csv")
+
+    client.post(FROM_INBOX, json={"file_ids": [f"{batch_id}:0"]}, headers=admin_headers)
+
+    body = client.get(INBOX, headers=admin_headers).json()
+    assert body["pending_count"] == 1
+    remaining = body["submissions"][0]["files"][0]
+    assert remaining["filename"] == "two.csv"
+    # Its id is unchanged. The UI holds selections across polls, so renumbering
+    # here would silently repoint a tick at a different file.
+    assert remaining["file_id"] == f"{batch_id}:1"
+
+
+def test_starting_the_same_file_twice_is_refused(client, admin_headers, submitted):
+    """A stale checkbox in a second tab must not run a sheet again."""
+    batch_id = _drop(client, "priya")
+    payload = {"file_ids": [f"{batch_id}:0"]}
+
+    assert client.post(FROM_INBOX, json=payload, headers=admin_headers).status_code == 202
+    second = client.post(FROM_INBOX, json=payload, headers=admin_headers)
+
+    assert second.status_code == 409
+    assert "already" in second.json()["detail"].lower()
+    assert len(submitted) == 1
+
+
+def test_the_admin_chooses_the_network_stages(client, admin_headers):
+    """Not the sender - see the sender-side assertion in test_uploads_api."""
+    batch_id = _drop(client, "priya")
+
+    started = client.post(
+        FROM_INBOX,
+        json={"file_ids": [f"{batch_id}:0"], "fetch_images": True, "use_llm": True},
+        headers=admin_headers,
+    )
+
+    assert started.json()["fetch_images"] is True
+    assert started.json()["use_llm"] is True
+
+
+def test_starting_nothing_is_refused(client, admin_headers, submitted):
+    assert client.post(FROM_INBOX, json={"file_ids": []},
+                       headers=admin_headers).status_code == 400
+    assert submitted == []
+
+
+# ---------------------------------------------------------------------------
+# Dismissing
+# ---------------------------------------------------------------------------
+def test_dismissing_deletes_the_file_and_its_bytes(client, admin_headers, batch_root):
+    batch_id = _drop(client, "priya")
+    assert list((batch_root / batch_id).glob("*.csv"))
+
+    result = client.post(DISMISS, json={"file_ids": [f"{batch_id}:0"]},
+                         headers=admin_headers)
+
+    assert result.status_code == 200, result.text
+    assert result.json() == {"dismissed": 1}
+    assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 0
+    # The disposal path on an endpoint anyone can post to. If the bytes survived
+    # a dismiss, the volume would fill with sheets that were already refused.
+    assert not (batch_root / batch_id).exists()
+
+
+def test_dismissing_cannot_touch_a_running_batch(client, admin_headers, monkeypatch):
+    """A crafted id must not delete files out of a batch that is mid-run."""
+    from app.api import batch_common
+
+    manifest, _started = batch_common.stage_and_queue(
+        [("live.csv", _csv(), 1)], [], use_llm=False, fetch_images=False,
+    )
+    assert manifest.status != batch_ingest.PENDING
+
+    result = client.post(DISMISS, json={"file_ids": [f"{manifest.batch_id}:0"]},
+                         headers=admin_headers)
+
+    assert result.json() == {"dismissed": 0}
+    assert batch_ingest.read_manifest(manifest.batch_id).files
+
+
+def test_a_malformed_id_does_not_fail_the_whole_request(client, admin_headers):
+    """The UI polls every five seconds and prunes selections against what came
+    back, so one stale tick must not block the files that are still there."""
+    batch_id = _drop(client, "priya")
+
+    result = client.post(
+        DISMISS,
+        json={"file_ids": ["nonsense", f"{batch_id}:0", "also:bad:x"]},
+        headers=admin_headers,
+    )
+
+    assert result.status_code == 200, result.text
+    assert result.json() == {"dismissed": 1}
+
+
+# ---------------------------------------------------------------------------
+# Who may reach it
+# ---------------------------------------------------------------------------
+def test_the_inbox_is_admin_only(client, upload_headers):
+    """An uploader key sends files; it does not get to decide what runs."""
+    for method, path, kwargs in (
+        ("get", INBOX, {}),
+        ("post", FROM_INBOX, {"json": {"file_ids": []}}),
+        ("post", DISMISS, {"json": {"file_ids": []}}),
+    ):
+        response = getattr(client, method)(path, headers=upload_headers, **kwargs)
+        assert response.status_code == 403, f"{method.upper()} {path} -> {response.status_code}"
+
+
+def test_the_inbox_is_closed_to_anonymous(client):
+    assert client.get(INBOX).status_code == 401
+    assert client.post(FROM_INBOX, json={"file_ids": []}).status_code == 401
+    assert client.post(DISMISS, json={"file_ids": []}).status_code == 401

tests/test_uploads_api.py

@@ -153,11 +153,14 @@ def store(monkeypatch):
 # ---------------------------------------------------------------------------
 # The requirement: a file sent here is ingested
 # ---------------------------------------------------------------------------
-def test_an_uploaded_spreadsheet_is_queued_for_ingestion(client, upload_headers, submitted):
+def test_an_uploaded_spreadsheet_waits_for_review(client, upload_headers, submitted):
     """THE regression test for this endpoint.
 
-    An earlier version parked the file and started nothing, which is
-    indistinguishable from working if you only assert on the status code.
+    It must accept the file AND NOT RUN IT. Asserting only on the 202 cannot
+    tell the two apart, and both directions have been wrong here before: the
+    endpoint once parked files and started nothing while reporting success, and
+    it later ran everything the moment it arrived - which is unacceptable now
+    that anybody can post to it.
     """
     response = client.post(UPLOAD, files=_files(("catalog.xlsx", _sheet())),
                            headers=upload_headers)
@@ -165,10 +168,11 @@ def test_an_uploaded_spreadsheet_is_queued_for_ingestion(client, upload_headers,
     assert response.status_code == 202, response.text
     body = response.json()
     assert body["batch_id"]
-    assert body["status"] == "queued"
+    assert body["status"] == "pending"
     assert body["files_total"] == 1
-    # The batch reached the worker. This is the assertion that was missing.
-    assert submitted == [body["batch_id"]]
+    # Nothing reached the worker. This is the assertion the review gate rests on.
+    assert submitted == []
+    assert "review" in body["message"].lower()
 
 
 def test_an_excel_file_reaches_the_catalog(client, upload_headers, store):
@@ -211,24 +215,74 @@ def test_network_stages_are_off_unless_asked_for(client, upload_headers):
     assert response.json()["use_llm"] is False
 
 
-def test_network_stages_can_be_opted_into(client, upload_headers):
-    response = client.post(UPLOAD + "?fetch_images=true", files=_files(("a.csv", _csv())),
-                           headers=upload_headers)
-    assert response.json()["fetch_images"] is True
+def test_a_sender_cannot_opt_into_the_network_stages(client, upload_headers):
+    """The sender does not get to commit the host to Playwright image search.
+
+    `fetch_images` and `use_llm` moved to the admin who presses Start. Passing
+    them here must be inert rather than honoured - on an endpoint open to
+    anonymous callers, an accepted query parameter is an invitation.
+    """
+    response = client.post(UPLOAD + "?fetch_images=true&use_llm=true",
+                           files=_files(("a.csv", _csv())), headers=upload_headers)
+    assert response.status_code == 202, response.text
+    assert response.json()["fetch_images"] is False
+    assert response.json()["use_llm"] is False
 
 
 # ---------------------------------------------------------------------------
 # Who may call it
 # ---------------------------------------------------------------------------
-def test_upload_endpoint_rejects_anonymous(client):
-    assert client.post(UPLOAD, files=_files(("a.csv", _csv()))).status_code == 401
+def test_upload_endpoint_accepts_anonymous(client, submitted):
+    """The point of the endpoint: a colleague needs no credential to send a file.
+
+    Safe only because of the assertion below it - the drop is staged for review,
+    not run. If that ever changes, this test is the one that has to be argued
+    with first.
+    """
+    response = client.post(UPLOAD, files=_files(("a.csv", _csv())))
+    assert response.status_code == 202, response.text
+    body = response.json()
+    assert body["status"] == "pending"
+    assert body["submitted_by"] == "anonymous"
+    assert submitted == []
+
+
+def test_a_sender_label_is_recorded_when_given(client):
+    """The inbox groups by sender; three colleagues all reading "anonymous" is
+    an inbox nobody can triage."""
+    response = client.post(UPLOAD, files=_files(("a.csv", _csv())),
+                           data={"sender": "priya"})
+    assert response.status_code == 202, response.text
+    assert response.json()["submitted_by"] == "priya"
+
+
+def test_a_credential_outranks_the_sender_label(client, upload_headers):
+    """`sender` is free text. A real credential is not, so it wins."""
+    response = client.post(UPLOAD, files=_files(("a.csv", _csv())),
+                           data={"sender": "someone-else"}, headers=upload_headers)
+    assert response.status_code == 202, response.text
+    assert response.json()["submitted_by"] == "catalog-drop"
+
+
+def test_a_wrong_credential_is_still_refused(client):
+    """Anonymous is allowed; WRONG is not. A typo in a key must not silently
+    downgrade to an anonymous drop that the sender then cannot find."""
+    response = client.post(UPLOAD, files=_files(("a.csv", _csv())),
+                           headers={"X-API-Key": "n" * 43})
+    assert response.status_code == 401
 
 
 def test_admin_can_also_send_files(client, admin_headers, submitted):
-    """`admin` passes every permission check, so this path stays open to them."""
+    """`admin` reaches it too - and lands in the same review queue.
+
+    An admin who wants files to run immediately has
+    POST /api/admin/catalog-batch/ingest. This endpoint has one behaviour for
+    everyone, so the gate cannot be stepped around by whoever holds a key.
+    """
     response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=admin_headers)
     assert response.status_code == 202, response.text
-    assert submitted == [response.json()["batch_id"]]
+    assert response.json()["status"] == "pending"
+    assert submitted == []
 
 
 def test_uploader_key_is_refused_on_every_admin_route(client, upload_headers):
@@ -300,9 +354,12 @@ def test_a_bad_file_among_good_ones_is_reported_not_fatal(client, upload_headers
     )
     assert response.status_code == 202, response.text
     body = response.json()
-    assert submitted == [body["batch_id"]]
+    assert submitted == []
 
     by_name = {f["filename"]: f for f in body["files"]}
+    # The FILE is queued inside a batch that is pending: it is waiting its turn
+    # behind a decision, not behind the worker. `settle()` is what keeps the two
+    # apart - without its PENDING guard this batch would report itself queued.
     assert by_name["good.xlsx"]["status"] == "queued"
     assert by_name["bad.txt"]["status"] == "failed"
     assert by_name["bad.txt"]["detail"]
@@ -367,27 +424,48 @@ def test_uploaded_filenames_cannot_escape_the_batch_directory(client, upload_hea
     assert not (batch_root.parent / "evil.csv").exists()
 
 
-def test_a_full_queue_is_refused_but_the_batch_is_saved(client, upload_headers, monkeypatch):
-    """The caller has no Resume button, so they are told to retry - but the
-    files are on disk and an admin can start them."""
-    import queue
+def test_a_full_inbox_is_refused_and_stores_nothing(client, upload_headers, monkeypatch):
+    """The bound that replaced BATCH_QUEUE_MAX on this endpoint.
 
-    from app.core import batch_worker
+    Nothing here is queued any more, so the worker queue no longer limits what
+    an anonymous caller can leave on the volume. The inbox ceiling does, and a
+    refusal has to store NOTHING - a 429 that still wrote the file would be no
+    limit at all.
+    """
+    from app.api.routers import uploads
 
-    def full(_batch_id):
-        raise queue.Full()
+    monkeypatch.setattr(uploads, "INBOX_MAX_PENDING_FILES", 1)
 
-    monkeypatch.setattr(batch_worker, "submit", full)
-    response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers)
+    first = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers)
+    assert first.status_code == 202, first.text
 
-    assert response.status_code == 429
-    detail = response.json()["detail"]
-    assert "retry" in detail.lower()
-    # The id is in the message precisely so an admin can find it.
-    staged = batch_ingest.list_manifests()
-    assert len(staged) == 1
-    assert staged[0].batch_id in detail
-    assert staged[0].status == "queued"
+    second = client.post(UPLOAD, files=_files(("b.csv", _csv())), headers=upload_headers)
+    assert second.status_code == 429
+    assert "full" in second.json()["detail"].lower()
+
+    # Still exactly the one drop. The refused file was not written.
+    assert len(batch_ingest.list_manifests()) == 1
+
+
+def test_dismissing_a_drop_frees_inbox_capacity(client, upload_headers, admin_headers,
+                                                monkeypatch):
+    """The ceiling counts what is AWAITING review, so triage releases it."""
+    from app.api.routers import uploads
+
+    monkeypatch.setattr(uploads, "INBOX_MAX_PENDING_FILES", 1)
+
+    first = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers)
+    batch_id = first.json()["batch_id"]
+    assert client.post(UPLOAD, files=_files(("b.csv", _csv())),
+                       headers=upload_headers).status_code == 429
+
+    dismissed = client.post("/api/admin/catalog-batch/inbox/dismiss",
+                            json={"file_ids": [f"{batch_id}:0"]}, headers=admin_headers)
+    assert dismissed.status_code == 200, dismissed.text
+    assert dismissed.json()["dismissed"] == 1
+
+    assert client.post(UPLOAD, files=_files(("b.csv", _csv())),
+                       headers=upload_headers).status_code == 202
 
 
 # ---------------------------------------------------------------------------
@@ -465,6 +543,25 @@ def test_a_traversing_batch_id_is_not_found(client, upload_headers):
     assert client.get(f"{UPLOAD}/..%2F..%2Fetc", headers=upload_headers).status_code == 404
 
 
-def test_reads_require_a_credential(client):
+def test_the_batch_list_requires_a_credential(client):
+    """The LIST stays closed even though the drop is open.
+
+    Letting an anonymous caller enumerate every sender's submissions is a
+    different thing entirely from letting one check the id they were handed.
+    """
     assert client.get(UPLOAD).status_code == 401
-    assert client.get(f"{UPLOAD}/anything").status_code == 401
+
+
+def test_an_anonymous_sender_can_poll_the_id_they_were_given(client):
+    """The id IS the credential here - the sender needed none to post, so
+    requiring one to read the outcome would strand them."""
+    posted = client.post(UPLOAD, files=_files(("a.csv", _csv())), data={"sender": "priya"})
+    batch_id = posted.json()["batch_id"]
+
+    polled = client.get(f"{UPLOAD}/{batch_id}")
+    assert polled.status_code == 200, polled.text
+    assert polled.json()["status"] == "pending"
+
+    # An id nobody issued is a 404, not a 401: whether it exists is not the
+    # caller's business, so the two answers must be indistinguishable.
+    assert client.get(f"{UPLOAD}/anything").status_code == 404