Merge pull request #1504 from Hestia-Homes/fix/bulk-download-select-by-landlord-property-id

Bulk download: hand-pick selection by landlord_property_id, not property.id

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

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]

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:

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.
     """
 

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

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