diff --git a/src/app/services/accounting/functions/__init__.py b/src/app/services/accounting/functions/__init__.py index 1fb6840..0650293 100644 --- a/src/app/services/accounting/functions/__init__.py +++ b/src/app/services/accounting/functions/__init__.py @@ -1,3 +1,15 @@ +"""Accounting helper functions.""" + from .accounting_operations import AccountingOperations +from .document_filters import ( + FULL_TEXT_FILTER, + PAPERLESS_FILTERS, + build_document_filters, +) -__all__ = ["AccountingOperations"] +__all__ = [ + "FULL_TEXT_FILTER", + "PAPERLESS_FILTERS", + "AccountingOperations", + "build_document_filters", +] diff --git a/src/app/services/accounting/functions/document_filters.py b/src/app/services/accounting/functions/document_filters.py new file mode 100644 index 0000000..5d5ed29 --- /dev/null +++ b/src/app/services/accounting/functions/document_filters.py @@ -0,0 +1,62 @@ +""" +Translate this API's document query parameters into Paperless filter names. + +Extracted from the router so it can be tested without a running Paperless. That +matters more than it sounds: Paperless **ignores** filter names it does not +recognise rather than rejecting them, so a wrong name does not fail — it returns +the entire collection and looks like a search that matched a lot. + +That is exactly what happened with `text`, which Paperless has never had. +""" + +from __future__ import annotations + +from typing import Any + +# Filters this service sends, and what each means upstream. Anything not in +# here is a name Paperless would silently discard. +FULL_TEXT_FILTER = "query" + +PAPERLESS_FILTERS: frozenset[str] = frozenset( + { + "page", + "page_size", + "ordering", + FULL_TEXT_FILTER, + "correspondent__id", + "document_type__id", + "storage_path__id", + "tags__id__all", + } +) + + +def build_document_filters( + *, + page: int, + page_size: int, + query: str | None = None, + correspondent: int | None = None, + document_type: int | None = None, + storage_path: int | None = None, + tags: list[int] | None = None, + ordering: str | None = None, +) -> dict[str, Any]: + """ + Build the upstream query string, dropping anything unset. + + `query` becomes Paperless' `query`, which searches the OCR'd content and the + title. It is deliberately not `text`: that name is accepted by the HTTP + layer, matched by nothing, and therefore returns every document. + """ + candidates: dict[str, Any] = { + "page": page, + "page_size": page_size, + FULL_TEXT_FILTER: query, + "correspondent__id": correspondent, + "document_type__id": document_type, + "storage_path__id": storage_path, + "tags__id__all": ",".join(map(str, tags)) if tags else None, + "ordering": ordering, + } + return {key: value for key, value in candidates.items() if value is not None} diff --git a/src/app/services/accounting/routers/accounting_router.py b/src/app/services/accounting/routers/accounting_router.py index 315532d..ad5e7eb 100644 --- a/src/app/services/accounting/routers/accounting_router.py +++ b/src/app/services/accounting/routers/accounting_router.py @@ -8,6 +8,7 @@ from app.services.admin.dependencies import require_admin from app.shared.responses import DataResponse +from ..functions import build_document_filters from ..dependencies import AccountingOperationsDependency from ..models import ( AccountingStatus, @@ -47,18 +48,17 @@ async def list_documents( tags: list[int] | None = Query(None), ordering: str | None = Query(None, max_length=100), ) -> DataResponse[PaperlessPage]: - params = { - "page": page, - "page_size": page_size, - "text": query, - "correspondent__id": correspondent, - "document_type__id": document_type, - "storage_path__id": storage_path, - "tags__id__all": ",".join(map(str, tags)) if tags else None, - "ordering": ordering, - } data = await operations.list_documents( - {k: v for k, v in params.items() if v is not None} + build_document_filters( + page=page, + page_size=page_size, + query=query, + correspondent=correspondent, + document_type=document_type, + storage_path=storage_path, + tags=tags, + ordering=ordering, + ) ) return DataResponse( data=PaperlessPage.model_validate(data), message="Documents retrieved" diff --git a/src/app/services/analytics/routers/analytics_router.py b/src/app/services/analytics/routers/analytics_router.py index f1c92aa..8a4c706 100644 --- a/src/app/services/analytics/routers/analytics_router.py +++ b/src/app/services/analytics/routers/analytics_router.py @@ -403,6 +403,12 @@ async def get_storefront_funnel( None, alias="from", description="First day, inclusive" ), date_to: date | None = Query(None, alias="to", description="Last day, inclusive"), + limit: int = Query( + 10, + ge=1, + le=_MAX_PRODUCT_ROWS, + description="Rows returned in top_paths and product_interest", + ), session: AsyncSession = Depends(get_session_dependency), ) -> AnalyticsStorefrontResponse: period = await _resolve_period(date_from, date_to) @@ -443,7 +449,7 @@ async def get_storefront_funnel( ) previous = value - interest = await events.product_interest(period.start, period.end) + interest = await events.product_interest(period.start, period.end, limit=limit) names = await AnalyticsRepository(session).item_names( [row["sku"] for row in interest] ) @@ -456,7 +462,8 @@ async def get_storefront_funnel( page_views=page_views, steps=steps, top_paths=[ - PathViews(**row) for row in await events.top_paths(period.start, period.end) + PathViews(**row) + for row in await events.top_paths(period.start, period.end, limit=limit) ], product_interest=[ ProductInterest(**row, name=names.get(row["sku"])) for row in interest diff --git a/tests/test_accounting_filters_unit.py b/tests/test_accounting_filters_unit.py new file mode 100644 index 0000000..9b6f03e --- /dev/null +++ b/tests/test_accounting_filters_unit.py @@ -0,0 +1,95 @@ +""" +Unit tests for the Paperless document filter mapping. + +Paperless ignores filter names it does not recognise instead of rejecting them, +so a wrong name does not produce an error — the request succeeds and returns the +entire collection. A search that quietly matches everything looks like a search +that worked. + +That is not hypothetical: `query` was mapped onto `text`, which Paperless has +never had, and `?query=Barolo` returned all 183 seeded documents instead of 25. +""" + +import pytest + +from app.services.accounting.functions import ( + FULL_TEXT_FILTER, + PAPERLESS_FILTERS, + build_document_filters, +) + + +def test_full_text_search_uses_a_filter_paperless_honours(): + """ + The regression itself. `text` is accepted by the HTTP layer, matched by + nothing, and therefore returns everything. + """ + filters = build_document_filters(page=1, page_size=50, query="Barolo") + + assert filters[FULL_TEXT_FILTER] == "Barolo" + assert "text" not in filters, ( + "`text` is not a Paperless filter; it matches everything" + ) + + +def test_the_full_text_filter_is_the_documented_one(): + assert FULL_TEXT_FILTER == "query" + + +def test_every_filter_sent_is_one_paperless_recognises(): + """ + Guards the whole mapping, not just search. Any name added here that + Paperless does not know widens the result set silently. + """ + filters = build_document_filters( + page=2, + page_size=25, + query="Champagne", + correspondent=3, + document_type=1, + storage_path=2, + tags=[4, 5], + ordering="-created", + ) + + unknown = set(filters) - PAPERLESS_FILTERS + assert not unknown, f"filters Paperless would ignore: {sorted(unknown)}" + + +def test_unset_parameters_are_omitted_entirely(): + """ + Sending `correspondent__id=None` would filter on the literal string "None" + and return nothing — the opposite failure, equally silent. + """ + filters = build_document_filters(page=1, page_size=50) + + assert filters == {"page": 1, "page_size": 50} + assert "correspondent__id" not in filters + assert FULL_TEXT_FILTER not in filters + + +def test_tags_are_sent_as_a_comma_separated_all_match(): + filters = build_document_filters(page=1, page_size=50, tags=[7, 9, 11]) + assert filters["tags__id__all"] == "7,9,11" + + +def test_an_empty_tag_list_is_not_sent(): + """An empty `tags__id__all=` would match nothing rather than everything.""" + filters = build_document_filters(page=1, page_size=50, tags=[]) + assert "tags__id__all" not in filters + + +@pytest.mark.parametrize( + "field,value,expected_key", + [ + ("correspondent", 3, "correspondent__id"), + ("document_type", 1, "document_type__id"), + ("storage_path", 2, "storage_path__id"), + ], +) +def test_id_filters_carry_the_double_underscore_suffix(field, value, expected_key): + """Paperless filters relations by `__id`; the bare name is ignored.""" + filters = build_document_filters(page=1, page_size=50, **{field: value}) + + assert filters[expected_key] == value + assert field not in filters diff --git a/tests/test_accounting_unit.py b/tests/test_accounting_unit.py index e17cdee..fd32ff4 100644 --- a/tests/test_accounting_unit.py +++ b/tests/test_accounting_unit.py @@ -37,8 +37,10 @@ def test_document_list_forwards_admin_filters() -> None: ) assert response.status_code == 200 + # "query", not "text": Paperless has no `text` filter and ignores unknown + # names, so the old value made every search return the whole collection. operations.list_documents.assert_awaited_once_with( - {"page": 1, "page_size": 25, "text": "invoice", "tags__id__all": "2,3"} + {"page": 1, "page_size": 25, "query": "invoice", "tags__id__all": "2,3"} ) diff --git a/tests/test_storefront_analytics_integration.py b/tests/test_storefront_analytics_integration.py index 8dc670f..b0882a6 100644 --- a/tests/test_storefront_analytics_integration.py +++ b/tests/test_storefront_analytics_integration.py @@ -299,7 +299,10 @@ def test_add_to_cart_rate_is_reported_per_sku(): ] ) - payload = requests.get(FUNNEL_URL, headers=_HEADERS).json() + # A wide limit rather than the default ten: on a shop with real traffic a + # two-view fixture SKU will never reach a top-ten list, and the assertion + # is about the rate, not about ranking. + payload = requests.get(FUNNEL_URL, headers=_HEADERS, params={"limit": 200}).json() row = next(p for p in payload["product_interest"] if p["sku"] == sku) assert row["sessions_viewed"] == 2