diff --git a/CONTEXT.md b/CONTEXT.md index 964319486..1204aa9d9 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -108,7 +108,7 @@ The single ZIP archive a **Bulk Document Download** produces: one folder per pro _Avoid_: export (that is the plan/scenario XLSX export), bundle, archive (ambiguous), zip (the format, not the concept) **Bulk Document Download**: -The feature/job that assembles a **Download Package** for a chosen set of properties (the union of one or more whole HubSpot projects, selected by `project_code`, and a hand-picked `property_id` list) and emails the requester a link. Runs on the app-owned-task + attach-mode lane (ADR-0055): the FastAPI route resolves the selection to a distinct `landlord_property_id` set — project codes are expanded via `hubspot_deal_data` (which holds the project↔property grain) — pins that set and the recipient email onto the `sub_task`, and the `applications/bulk_document_download` Lambda builds the package, writes the URL to `sub_task.outputs`, and emails it (ADR-0059/0060). Files are matched by `landlord_property_id` (the missing `property_id` on `uploaded_files` is a known future gap). Selection is capped by property count at the trigger. +The feature/job that assembles a **Download Package** for a chosen set of properties (the union of one or more whole HubSpot projects, selected by `project_code`, and a hand-picked `landlord_property_id` list) and emails the requester a link. Runs on the app-owned-task + attach-mode lane (ADR-0055): the FastAPI route resolves the selection to a distinct `landlord_property_id` set — project codes are expanded via `hubspot_deal_data` (which holds the project↔property grain), hand-picked ids are taken as given — pins that set and the recipient email onto the `sub_task`, and the `applications/bulk_document_download` Lambda builds the package, writes the URL to `sub_task.outputs`, and emails it (ADR-0059/0060). `landlord_property_id` is the selection key throughout because `uploaded_files` is matched on it (it has no `property_id`). Files are matched by `landlord_property_id` (the missing `property_id` on `uploaded_files` is a known future gap). Selection is capped by property count at the trigger. _Avoid_: document export, bulk export, file dump ### Source data diff --git a/backend/app/documents/download_tasks.py b/backend/app/documents/download_tasks.py index 7109224a6..ef4afeba3 100644 --- a/backend/app/documents/download_tasks.py +++ b/backend/app/documents/download_tasks.py @@ -52,23 +52,20 @@ class DocumentDownloadTasks: def resolve_selection( self, project_codes: list[str], - property_ids: list[int], + landlord_property_ids: list[str], ) -> list[str]: """Resolve the selection to the distinct ``landlord_property_id`` set the - Download Package is built from (ADR-0060). Two independent sources, - unioned: + Download Package is built from (ADR-0060) — files are matched on + ``landlord_property_id`` (``uploaded_files`` has no ``property_id``), so + that is the selection key throughout. Two independent sources, unioned: - **project codes** — every property in the named HubSpot projects, read - from ``hubspot_deal_data`` (the project↔property grain lives there, not - on ``property``). - - **hand-picked property ids** — the selected ``property`` rows. + from ``hubspot_deal_data`` (the project↔property grain lives there). + - **hand-picked landlord_property_ids** — used as given. - Files are matched by ``landlord_property_id`` (the missing ``property_id`` - on ``uploaded_files`` is a known future gap), so a source row with a null - ``landlord_property_id`` contributes nothing and is dropped here. The - returned set size is the cap basis (property count, ADR-0060). + The returned set size is the cap basis (property count, ADR-0060). """ - landlord_property_ids: set[str] = set() + resolved: set[str] = {lpid for lpid in landlord_property_ids if lpid} if project_codes: rows = self._session.connection().execute( text( @@ -78,17 +75,8 @@ class DocumentDownloadTasks: ), {"codes": project_codes}, ) - landlord_property_ids.update(row[0] for row in rows.all()) - if property_ids: - rows = self._session.connection().execute( - text( - "SELECT landlord_property_id FROM property" - " WHERE id = ANY(:ids) AND landlord_property_id IS NOT NULL" - ), - {"ids": property_ids}, - ) - landlord_property_ids.update(row[0] for row in rows.all()) - return sorted(landlord_property_ids) + resolved.update(row[0] for row in rows.all()) + return sorted(resolved) def create_download_subtask( self, task_id: UUID, subtask_id: UUID, recipe: dict[str, Any] diff --git a/backend/app/documents/router.py b/backend/app/documents/router.py index 50b7a957b..a4994bb82 100644 --- a/backend/app/documents/router.py +++ b/backend/app/documents/router.py @@ -92,7 +92,7 @@ async def bulk_download( config = tasks.read_selection_config(body.task_id) portfolio_id: Any = config.get("portfolio_id") raw_project_codes: Any = config.get("project_codes") or [] - raw_property_ids: Any = config.get("property_ids") or [] + raw_landlord_property_ids: Any = config.get("landlord_property_ids") or [] if not isinstance(raw_project_codes, list) or not all( isinstance(code, str) for code in cast(list[Any], raw_project_codes) ): @@ -100,28 +100,32 @@ async def bulk_download( status_code=400, detail="task.inputs project_codes must be a list of strings.", ) - if not isinstance(raw_property_ids, list) or not all( - isinstance(pid, int) for pid in cast(list[Any], raw_property_ids) + if not isinstance(raw_landlord_property_ids, list) or not all( + isinstance(lpid, str) for lpid in cast(list[Any], raw_landlord_property_ids) ): raise HTTPException( status_code=400, - detail="task.inputs property_ids must be a list of integers.", + detail="task.inputs landlord_property_ids must be a list of strings.", ) project_codes: list[str] = cast(list[str], raw_project_codes) - property_ids: list[int] = cast(list[int], raw_property_ids) - if not project_codes and not property_ids: + selected_landlord_property_ids: list[str] = cast( + list[str], raw_landlord_property_ids + ) + if not project_codes and not selected_landlord_property_ids: raise HTTPException( status_code=400, - detail="task.inputs must provide project_codes or property_ids.", + detail="task.inputs must provide project_codes or landlord_property_ids.", ) - landlord_property_ids = tasks.resolve_selection(project_codes, property_ids) + landlord_property_ids = tasks.resolve_selection( + project_codes, selected_landlord_property_ids + ) if not landlord_property_ids: raise HTTPException( status_code=400, detail=( - "The selection resolves to no downloadable properties — none of " - "the selected projects or properties has a landlord_property_id." + "The selection resolves to no properties — the project codes " + "matched nothing and no landlord_property_ids were given." ), ) if len(landlord_property_ids) > MAX_PROPERTIES: diff --git a/backend/app/documents/schemas.py b/backend/app/documents/schemas.py index c383ddfaf..fbea98294 100644 --- a/backend/app/documents/schemas.py +++ b/backend/app/documents/schemas.py @@ -8,12 +8,13 @@ class BulkDownloadRequest(BaseModel): The front end creates the app-owned Task first and writes the **selection config** into ``task.inputs`` (a JSON object of - ``{project_codes?: str[], property_ids?: int[], portfolio_id?: int}`` — so a - large selection never travels in an HTTP body), then passes only the + ``{project_codes?: str[], landlord_property_ids?: str[], portfolio_id?: int}`` + — so a large selection never travels in an HTTP body), then passes only the ``task_id`` here. The backend reads the selection from ``task.inputs`` and - resolves it to the distinct ``landlord_property_id`` set: the union of every - property in the named HubSpot ``project_codes`` (from ``hubspot_deal_data``) - and the hand-picked ``property_ids``. At least one of the two must be given; + resolves it to the distinct ``landlord_property_id`` set (the key + ``uploaded_files`` is matched on): the union of every property in the named + HubSpot ``project_codes`` (from ``hubspot_deal_data``) and the hand-picked + ``landlord_property_ids``. At least one of the two must be given; ``portfolio_id`` is optional and used only to name the package. """ diff --git a/docs/adr/0060-bulk-document-download-package-model.md b/docs/adr/0060-bulk-document-download-package-model.md index 10959d19a..2eedb6d9f 100644 --- a/docs/adr/0060-bulk-document-download-package-model.md +++ b/docs/adr/0060-bulk-document-download-package-model.md @@ -25,17 +25,18 @@ lane (ADR-0055).** - **Trigger & lifecycle (ADR-0055).** The **front end** creates the app-owned `tasks` row and writes the **selection config** — - `{project_codes?: str[], property_ids?: int[], portfolio_id?: int}` — into - `tasks.inputs` (TEXT/JSON, FE-owned), then calls the FastAPI route with **only - the `task_id`**. A large selection therefore never travels in an HTTP body. - The route **reads the selection from `tasks.inputs`** and resolves it to the - distinct `landlord_property_id` set — the **union** of every property in the - named HubSpot `project_codes` (read from `hubspot_deal_data`, where the - project↔property grain lives — `property` carries only `portfolio_id`) and the - hand-picked `property_ids`. At least one of the two must be provided; - `portfolio_id` is optional and used only to name the package. The route then - caps the set, resolves the recipient email from the authenticated user - (ADR-0059), **pins the resolved recipe** + `{project_codes?: str[], landlord_property_ids?: str[], portfolio_id?: int}` — + into `tasks.inputs` (TEXT/JSON, FE-owned), then calls the FastAPI route with + **only the `task_id`**. A large selection therefore never travels in an HTTP + body. The route **reads the selection from `tasks.inputs`** and resolves it to + the distinct `landlord_property_id` set — the **union** of every property in + the named HubSpot `project_codes` (read from `hubspot_deal_data`, where the + project↔property grain lives) and the hand-picked `landlord_property_ids`. + `landlord_property_id` is the selection key throughout because + `uploaded_files` is matched on it (it has no `property_id`). At least one of + the two must be provided; `portfolio_id` is optional and used only to name the + package. The route then caps the set, resolves the recipient email from the + authenticated user (ADR-0059), **pins the resolved recipe** (`landlord_property_ids`, `recipient_email`, `package_name`) onto **one pre-created `sub_task`'s `inputs`**, and drops one SQS message (`task_id`, `sub_task_id`). The new `applications/bulk_document_download` Lambda runs in diff --git a/tests/backend/app/documents/test_bulk_download_router.py b/tests/backend/app/documents/test_bulk_download_router.py index c21da8ca1..25ce84ec2 100644 --- a/tests/backend/app/documents/test_bulk_download_router.py +++ b/tests/backend/app/documents/test_bulk_download_router.py @@ -24,7 +24,6 @@ from backend.app.documents.router import ( get_requesting_user_email, get_session, ) -from infrastructure.postgres.property_table import PropertyRow from infrastructure.postgres.task_table import TaskRow from repositories.tasks.subtask_postgres_repository import SubTaskPostgresRepository @@ -51,17 +50,6 @@ class Api: session.refresh(row) return row.id - def seed_property(self, landlord_property_id: str) -> int: - with Session(self.engine) as session: - row = PropertyRow( - portfolio_id=PORTFOLIO_ID, landlord_property_id=landlord_property_id - ) - session.add(row) - session.commit() - session.refresh(row) - assert row.id is not None - return row.id - def seed_deal(self, project_code: str, landlord_property_id: str) -> None: """A hubspot_deal_data row carrying the project↔property link the project-code selection resolves against (ADR-0060).""" @@ -105,11 +93,10 @@ def _trigger(api: Api, task_id: UUID) -> Any: def test_pins_the_recipe_and_enqueues_one_message(api: Api) -> None: - # arrange — two selected properties, pinned as the FE's task.inputs config. - p1 = api.seed_property("LP1") - p2 = api.seed_property("LP2") + # arrange — two hand-picked properties, by landlord_property_id (the key + # uploaded_files is matched on), pinned as the FE's task.inputs config. task_id = api.seed_task( - {"portfolio_id": PORTFOLIO_ID, "property_ids": [p1, p2]} + {"portfolio_id": PORTFOLIO_ID, "landlord_property_ids": ["LP1", "LP2"]} ) # act @@ -145,13 +132,12 @@ def test_resolves_all_properties_for_the_given_project_codes(api: Api) -> None: assert inputs[0]["landlord_property_ids"] == ["LP1", "LP2", "LP3"] -def test_unions_project_codes_with_hand_picked_property_ids(api: Api) -> None: - # arrange — one project plus a hand-picked property from outside it. +def test_unions_project_codes_with_hand_picked_landlord_property_ids(api: Api) -> None: + # arrange — one project, plus hand-picks that overlap it (LP2) and extend it (LP5). api.seed_deal("PROJ-A", "LP1") api.seed_deal("PROJ-A", "LP2") - hand_picked = api.seed_property("LP5") task_id = api.seed_task( - {"project_codes": ["PROJ-A"], "property_ids": [hand_picked]} + {"project_codes": ["PROJ-A"], "landlord_property_ids": ["LP2", "LP5"]} ) # act @@ -197,10 +183,8 @@ def test_rejects_a_project_code_that_resolves_to_no_properties(api: Api) -> None def test_rejects_a_selection_over_the_cap(api: Api, monkeypatch: Any) -> None: # arrange — cap of 1, two selected properties. monkeypatch.setattr(documents_router, "MAX_PROPERTIES", 1) - p1 = api.seed_property("LP1") - p2 = api.seed_property("LP2") task_id = api.seed_task( - {"portfolio_id": PORTFOLIO_ID, "property_ids": [p1, p2]} + {"portfolio_id": PORTFOLIO_ID, "landlord_property_ids": ["LP1", "LP2"]} ) # act @@ -214,8 +198,9 @@ def test_rejects_a_selection_over_the_cap(api: Api, monkeypatch: Any) -> None: def test_rejects_a_task_that_already_has_a_subtask(api: Api) -> None: # arrange — a task already triggered once. - p1 = api.seed_property("LP1") - task_id = api.seed_task({"portfolio_id": PORTFOLIO_ID, "property_ids": [p1]}) + task_id = api.seed_task( + {"portfolio_id": PORTFOLIO_ID, "landlord_property_ids": ["LP1"]} + ) assert _trigger(api, task_id).status_code == 202 # act @@ -228,8 +213,7 @@ def test_rejects_a_task_that_already_has_a_subtask(api: Api) -> None: def test_rejects_a_task_with_no_selection_config(api: Api) -> None: - # arrange — a task whose inputs carry no portfolio_id/selection. - api.seed_property("LP1") + # arrange — a task whose inputs carry no selection. task_id = api.seed_task({}) # act