From 90fe2914e034211a9797c97a1165427622443250 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Wed, 8 Jul 2026 11:42:17 +0000 Subject: [PATCH] Select documents by HubSpot project_code(s), unioned with hand-picked property_ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'all properties' selection resolved against property.portfolio_id, but a portfolio spans multiple HubSpot projects — so 'all' pulled the whole portfolio, not the project the user was looking at. The project↔property grain lives on hubspot_deal_data, not property. task.inputs is now {project_codes?: str[], property_ids?: int[], portfolio_id?}: the route resolves the distinct landlord_property_id set as the union of every property in the named project_codes (from hubspot_deal_data) and the hand-picked property_ids; portfolio_id is optional and only names the package. Drops the portfolio-scoped select_all. Cap now applies to the resolved set size. ADR-0060, CONTEXT.md and the request schema updated to match. New router tests cover project-code resolution, the union+dedup, null landlord_property_id drop, and the empty-project rejection. Co-Authored-By: Claude Opus 4.8 (1M context) --- CONTEXT.md | 2 +- backend/app/documents/download_tasks.py | 53 ++++++------ backend/app/documents/router.py | 64 ++++++++------- backend/app/documents/schemas.py | 10 ++- ...60-bulk-document-download-package-model.md | 22 +++-- .../documents/test_bulk_download_router.py | 81 +++++++++++++++++++ 6 files changed, 167 insertions(+), 65 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 33d79f62e..c736d712d 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 (a hand-picked list or a whole portfolio) and emails the requester a link. Runs on the app-owned-task + attach-mode lane (ADR-0055): the FastAPI route pins the resolved `property_id` set and the recipient email into `tasks.inputs`, 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 `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. _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 272da5e47..7109224a6 100644 --- a/backend/app/documents/download_tasks.py +++ b/backend/app/documents/download_tasks.py @@ -8,7 +8,7 @@ Modelling Run distributor works around — see ``modelling/run_tasks.py``). import json from datetime import datetime, timezone -from typing import Any, Optional, cast +from typing import Any, cast from uuid import UUID from sqlalchemy import text @@ -51,39 +51,44 @@ class DocumentDownloadTasks: def resolve_selection( self, - portfolio_id: int, - property_ids: Optional[list[int]], - select_all: bool, - ) -> tuple[int, list[str]]: - """Resolve the selection to (selected property count, the distinct - ``landlord_property_id`` values that carry a downloadable identifier). + project_codes: list[str], + property_ids: list[int], + ) -> list[str]: + """Resolve the selection to the distinct ``landlord_property_id`` set the + Download Package is built from (ADR-0060). Two independent sources, + unioned: - The count is the cap basis (property count, ADR-0060); files are matched - by ``landlord_property_id`` (the missing ``property_id`` on - ``uploaded_files`` is a known future gap), so a selected property with a - null ``landlord_property_id`` contributes no files. + - **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. + + 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). """ - if select_all: + landlord_property_ids: set[str] = set() + if project_codes: rows = self._session.connection().execute( text( - "SELECT id, landlord_property_id FROM property" - " WHERE portfolio_id = :portfolio_id" + "SELECT DISTINCT landlord_property_id FROM hubspot_deal_data" + " WHERE project_code = ANY(:codes)" + " AND landlord_property_id IS NOT NULL" ), - {"portfolio_id": portfolio_id}, + {"codes": project_codes}, ) - else: + landlord_property_ids.update(row[0] for row in rows.all()) + if property_ids: rows = self._session.connection().execute( text( - "SELECT id, landlord_property_id FROM property" - " WHERE portfolio_id = :portfolio_id AND id = ANY(:ids)" + "SELECT landlord_property_id FROM property" + " WHERE id = ANY(:ids) AND landlord_property_id IS NOT NULL" ), - {"portfolio_id": portfolio_id, "ids": property_ids or []}, + {"ids": property_ids}, ) - resolved = rows.all() - landlord_property_ids = sorted( - {row[1] for row in resolved if row[1] is not None} - ) - return len(resolved), landlord_property_ids + landlord_property_ids.update(row[0] for row in rows.all()) + return sorted(landlord_property_ids) 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 52d57d06a..50b7a957b 100644 --- a/backend/app/documents/router.py +++ b/backend/app/documents/router.py @@ -90,53 +90,59 @@ async def bulk_download( ) config = tasks.read_selection_config(body.task_id) - portfolio_id = config.get("portfolio_id") - property_ids = config.get("property_ids") - select_all = bool(config.get("select_all", False)) - if not isinstance(portfolio_id, int): + 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 [] + if not isinstance(raw_project_codes, list) or not all( + isinstance(code, str) for code in cast(list[Any], raw_project_codes) + ): raise HTTPException( status_code=400, - detail="task.inputs is missing an integer portfolio_id.", + detail="task.inputs project_codes must be a list of strings.", ) - if not select_all and not property_ids: + if not isinstance(raw_property_ids, list) or not all( + isinstance(pid, int) for pid in cast(list[Any], raw_property_ids) + ): raise HTTPException( status_code=400, - detail="task.inputs must provide property_ids or set select_all.", + detail="task.inputs property_ids must be a list of integers.", + ) + 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: + raise HTTPException( + status_code=400, + detail="task.inputs must provide project_codes or property_ids.", ) - property_count, landlord_property_ids = tasks.resolve_selection( - portfolio_id, property_ids, select_all - ) - if property_count == 0: - raise HTTPException( - status_code=400, - detail=( - f"The selection resolves to no properties in portfolio " - f"{portfolio_id}." - ), - ) - if property_count > MAX_PROPERTIES: - raise HTTPException( - status_code=400, - detail=( - f"The selection has {property_count} properties, over the " - f"{MAX_PROPERTIES} limit — narrow your selection." - ), - ) + landlord_property_ids = tasks.resolve_selection(project_codes, property_ids) if not landlord_property_ids: raise HTTPException( status_code=400, detail=( - "None of the selected properties have a landlord_property_id, " - "so no documents can be matched." + "The selection resolves to no downloadable properties — none of " + "the selected projects or properties has a landlord_property_id." + ), + ) + if len(landlord_property_ids) > MAX_PROPERTIES: + raise HTTPException( + status_code=400, + detail=( + f"The selection has {len(landlord_property_ids)} properties, over " + f"the {MAX_PROPERTIES} limit — narrow your selection." ), ) subtask_id = uuid4() + package_name = ( + f"portfolio-{portfolio_id}-{body.task_id}" + if isinstance(portfolio_id, int) + else f"documents-{body.task_id}" + ) recipe = { "landlord_property_ids": landlord_property_ids, "recipient_email": recipient_email, - "package_name": f"portfolio-{portfolio_id}-{body.task_id}", + "package_name": package_name, } created = tasks.create_download_subtask(body.task_id, subtask_id, recipe) if not created: diff --git a/backend/app/documents/schemas.py b/backend/app/documents/schemas.py index 0d60006d8..c383ddfaf 100644 --- a/backend/app/documents/schemas.py +++ b/backend/app/documents/schemas.py @@ -8,9 +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 - ``{portfolio_id, property_ids?, select_all?}`` — so a large hand-picked set - never travels in an HTTP body), then passes only the ``task_id`` here. The - backend reads the selection from ``task.inputs``. + ``{project_codes?: str[], property_ids?: int[], 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; + ``portfolio_id`` is optional and used only to name the package. """ task_id: UUID diff --git a/docs/adr/0060-bulk-document-download-package-model.md b/docs/adr/0060-bulk-document-download-package-model.md index 103a01b47..10959d19a 100644 --- a/docs/adr/0060-bulk-document-download-package-model.md +++ b/docs/adr/0060-bulk-document-download-package-model.md @@ -7,8 +7,9 @@ status: accepted (builds on ADR-0055, ADR-0059) Users need to pull the documents held in `uploaded_files` for many properties at once — per property, the latest file of each **Document Type** — as a single archive, without clicking through the UI file by file. The request is initiated -in the front end, can span a hand-picked set of properties or a whole -portfolio, and finishes with a link emailed to the requester (ADR-0059). +in the front end, can span a hand-picked set of properties or one or more whole +HubSpot projects (by project code), and finishes with a link emailed to the +requester (ADR-0059). `uploaded_files` links to a property by `uprn` **or** `landlord_property_id` (both nullable) and has **no `property_id`** column; `file_type` (the Document @@ -24,12 +25,17 @@ lane (ADR-0055).** - **Trigger & lifecycle (ADR-0055).** The **front end** creates the app-owned `tasks` row and writes the **selection config** — - `{portfolio_id, property_ids?, select_all?}` — into `tasks.inputs` (TEXT/JSON, - FE-owned), then calls the FastAPI route with **only the `task_id`**. A large - hand-picked selection therefore never travels in an HTTP body. The route - **reads the selection from `tasks.inputs`**, resolves it to the distinct - `landlord_property_id` set, caps it, resolves the recipient email from the - authenticated user (ADR-0059), **pins the resolved recipe** + `{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** (`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 ae5f38725..c21da8ca1 100644 --- a/tests/backend/app/documents/test_bulk_download_router.py +++ b/tests/backend/app/documents/test_bulk_download_router.py @@ -16,6 +16,7 @@ from fastapi.testclient import TestClient from sqlalchemy import Engine from sqlmodel import Session +from backend.app.db.models.hubspot_deal_data import HubspotDealData from backend.app.dependencies import validate_token from backend.app.documents import router as documents_router from backend.app.documents.router import ( @@ -61,6 +62,19 @@ class Api: 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).""" + with Session(self.engine) as session: + session.add( + HubspotDealData( + deal_id=f"deal-{project_code}-{landlord_property_id}", + project_code=project_code, + landlord_property_id=landlord_property_id, + ) + ) + session.commit() + def subtask_inputs(self, task_id: UUID) -> list[dict[str, Any]]: with Session(self.engine) as session: subtasks = SubTaskPostgresRepository(session).list_by_task(task_id) @@ -113,6 +127,73 @@ def test_pins_the_recipe_and_enqueues_one_message(api: Api) -> None: assert "subtask_id" in message +def test_resolves_all_properties_for_the_given_project_codes(api: Api) -> None: + # arrange — two projects the user selects, plus one they don't; the + # project↔property grain lives on hubspot_deal_data, not `property`. + api.seed_deal("PROJ-A", "LP1") + api.seed_deal("PROJ-A", "LP2") + api.seed_deal("PROJ-B", "LP3") + api.seed_deal("PROJ-C", "LP9") # a different project — must not appear + task_id = api.seed_task({"project_codes": ["PROJ-A", "PROJ-B"]}) + + # act + response = _trigger(api, task_id) + + # assert — the union of the selected projects' landlord_property_ids is pinned. + assert response.status_code == 202 + inputs = api.subtask_inputs(task_id) + 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. + 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]} + ) + + # act + response = _trigger(api, task_id) + + # assert — both sources contribute; the set is de-duplicated and sorted. + assert response.status_code == 202 + inputs = api.subtask_inputs(task_id) + assert inputs[0]["landlord_property_ids"] == ["LP1", "LP2", "LP5"] + + +def test_deal_rows_with_no_landlord_property_id_are_dropped(api: Api) -> None: + # arrange — a project whose deal rows carry no landlord_property_id can match + # no files (files are keyed on landlord_property_id), so the selection is empty. + api.seed_deal("PROJ-A", "LP1") + with Session(api.engine) as session: + session.add(HubspotDealData(deal_id="deal-null", project_code="PROJ-A")) + session.commit() + task_id = api.seed_task({"project_codes": ["PROJ-A"]}) + + # act + response = _trigger(api, task_id) + + # assert — only the row that carries an id contributes. + assert response.status_code == 202 + inputs = api.subtask_inputs(task_id) + assert inputs[0]["landlord_property_ids"] == ["LP1"] + + +def test_rejects_a_project_code_that_resolves_to_no_properties(api: Api) -> None: + # arrange — a project with no matching deal rows at all. + task_id = api.seed_task({"project_codes": ["PROJ-EMPTY"]}) + + # act + response = _trigger(api, task_id) + + # assert — refused; nothing created or sent. + assert response.status_code == 400 + assert api.subtask_inputs(task_id) == [] + assert api.sent_bodies == [] + + 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)