From e7785f6b760653d1e726cea95e3eadac9a36d539 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:58:18 +0000 Subject: [PATCH] =?UTF-8?q?Record=20a=20rename=20run's=20result=20summary?= =?UTF-8?q?=20on=20its=20sub=5Ftask=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- applications/sharepoint_renamer/handler.py | 45 +++- .../sharepoint_renamer_request.py | 6 + backend/app/db/models/tasks.py | 5 + domain/tasks/tasks.py | 7 + .../sharepoint_renamer/__init__.py | 0 .../sharepoint_renamer/test_handler.py | 199 ++++++++++++++++++ utils/sharepoint/domna_sites.py | 29 +++ 7 files changed, 282 insertions(+), 9 deletions(-) create mode 100644 tests/applications/sharepoint_renamer/__init__.py create mode 100644 tests/applications/sharepoint_renamer/test_handler.py diff --git a/applications/sharepoint_renamer/handler.py b/applications/sharepoint_renamer/handler.py index f55f54741..b2d94f8c4 100644 --- a/applications/sharepoint_renamer/handler.py +++ b/applications/sharepoint_renamer/handler.py @@ -1,26 +1,53 @@ +"""SQS-triggered Lambda: rename Sero SharePoint files to their canonical names. + +Runs on the app-owned-task lane. Each invocation creates its own ``task`` plus +an initial ``sub_task`` and runs inside the ``TaskOrchestrator`` lifecycle, so a +button press in the admin portal leaves a row with a status, a CloudWatch link, +and — on completion — a summary of what the run did. The portal reads +``tasks`` / ``sub_task`` directly, so the trigger stays fire-and-forget SQS and +nothing is handed back to the caller. + +Partial failures complete rather than fail: a run where SharePoint rejected some +files ends **complete** with those failures listed in ``outputs``, matching the +best-effort Download Package convention (ADR-0060). One locked file should not +turn a productive run red. Missing folders likewise do not fail a run. +""" + import os from typing import Any from applications.sharepoint_renamer.sharepoint_renamer_request import ( SharepointRenamerRequest, ) -from orchestration.sharepoint_renamer_orchestrator import SharepointRenamerOrchestrator +from domain.tasks.tasks import Source +from orchestration.sharepoint_renamer_orchestrator import ( + RenameSummary, + SharepointRenamerOrchestrator, +) +from utilities.aws_lambda.task_handler import task_handler from utils.sharepoint.domna_sharepoint_client import DomnaSharepointClient -from utils.sharepoint.domna_sites import DomnaSites +from utils.sharepoint.domna_sites import resolve_site CSV_PATH = os.path.join( os.path.dirname(os.path.abspath(__file__)), "sero_address_list.csv" ) -def handler(event: dict[str, Any], context: Any) -> None: - request = SharepointRenamerRequest.model_validate(event) - sp_client = DomnaSharepointClient(DomnaSites.SOCIAL_HOUSING_WAVE_3) - orchestrator = SharepointRenamerOrchestrator( +def _summary_json(summary: RenameSummary) -> dict[str, Any]: + raise NotImplementedError + + +@task_handler(task_source="sharepoint_renamer", source=Source.SHAREPOINT_SITE) +def handler(body: dict[str, Any], context: Any) -> Any: + request = SharepointRenamerRequest.model_validate(body) + sp_client = DomnaSharepointClient(resolve_site(request.sharepoint_site)) + SharepointRenamerOrchestrator( sp_client, CSV_PATH, dry_run=request.dry_run - ) - orchestrator.run() + ).run() + return None if __name__ == "__main__": - handler({"dry_run": False}, None) + # Defaults to a dry run: this module is now decorated, so invoking it by + # hand writes a real task row *and*, without the flag, renames live files. + handler({"sharepoint_site": "SOCIAL_HOUSING_WAVE_3", "dry_run": True}, None) diff --git a/applications/sharepoint_renamer/sharepoint_renamer_request.py b/applications/sharepoint_renamer/sharepoint_renamer_request.py index 6a10447bd..5d17f30a9 100644 --- a/applications/sharepoint_renamer/sharepoint_renamer_request.py +++ b/applications/sharepoint_renamer/sharepoint_renamer_request.py @@ -2,4 +2,10 @@ from pydantic import BaseModel class SharepointRenamerRequest(BaseModel): + # The DomnaSites member name the run targets, e.g. "SOCIAL_HOUSING_WAVE_3". + # Also the @task_handler `source_id` (the body key matches + # Source.SHAREPOINT_SITE's value), so a run is attributable after the fact. + # The base path is still hardcoded and Sero-specific — this records what + # ran; it is not yet a multi-site lever. + sharepoint_site: str = "SOCIAL_HOUSING_WAVE_3" dry_run: bool = False diff --git a/backend/app/db/models/tasks.py b/backend/app/db/models/tasks.py index 373a16b33..5a22dcc14 100644 --- a/backend/app/db/models/tasks.py +++ b/backend/app/db/models/tasks.py @@ -8,8 +8,13 @@ from sqlmodel import SQLModel, Field, Relationship class SourceEnum(enum.Enum): # TODO: move to domain? + # Legacy mirror of the same `source` pgEnum as domain.tasks.tasks.Source. + # Already drifted — PROPERTY is missing here — so this is not a reliable + # lockstep copy. SHAREPOINT_SITE is added because the backend maps rows + # through this model and an unknown value raises LookupError on read. PORTFOLIO = "portfolio_id" HUBSPOT_DEAL = "hubspot_deal_id" + SHAREPOINT_SITE = "sharepoint_site" class Task(SQLModel, table=True): diff --git a/domain/tasks/tasks.py b/domain/tasks/tasks.py index 992ae8d71..09f234563 100644 --- a/domain/tasks/tasks.py +++ b/domain/tasks/tasks.py @@ -18,6 +18,13 @@ class Source(str, Enum): PORTFOLIO = "portfolio_id" HUBSPOT_DEAL = "hubspot_deal_id" PROPERTY = "property_id" + # A run anchored on a SharePoint site rather than anything in our own + # schema — the SharePoint Renamer. The pgEnum is FE-owned via Drizzle + # (ADR-0003), so the Drizzle migration adding 'sharepoint_site' MUST apply + # before this deploys or the first run fails on an invalid enum value. + # Tests won't catch that: ephemeral DBs build from the SQLModel mirrors + # below and emit the member automatically. + SHAREPOINT_SITE = "sharepoint_site" @dataclass diff --git a/tests/applications/sharepoint_renamer/__init__.py b/tests/applications/sharepoint_renamer/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py new file mode 100644 index 000000000..06ea241e6 --- /dev/null +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -0,0 +1,199 @@ +"""Tests for the SharePoint Renamer Lambda handler on the app-owned-task lane. + +A real Lambda event goes through the real ``@task_handler`` decorator against a +real ephemeral PostgreSQL database; only SharePoint is faked, and it is faked at +the **client** boundary with canned Graph responses so the real rename +orchestrator runs. What the tests assert is therefore what is visible outside +the module: the rows that land in Postgres, and the calls made to SharePoint. +""" + +from __future__ import annotations + +import json +from collections.abc import Generator, Iterator +from contextlib import contextmanager +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Callable +from uuid import UUID + +import pytest +from sqlalchemy import Engine +from sqlmodel import Session + +from domain.pashub_fetcher.sharepoint_subfolders import SharepointSubfolders +from domain.tasks.subtasks import SubTaskStatus +from domain.tasks.tasks import Source, TaskStatus +from orchestration.sharepoint_renamer_orchestrator import ( + ASSESSMENT_SUBFOLDER, + BASE_PATH, +) +from orchestration.task_orchestrator import TaskOrchestrator +from repositories.tasks.subtask_postgres_repository import SubTaskPostgresRepository +from repositories.tasks.task_postgres_repository import TaskPostgresRepository +from utilities.aws_lambda.task_handler import task_handler + +SITE = "SOCIAL_HOUSING_WAVE_3" +UPRN = "100" +ADDRESS = "1 High St" +POSTCODE = "AB1 2CD" + +ASSESSMENT_PATH = ( + f"{BASE_PATH}/{ADDRESS}, {POSTCODE}" + f"/{SharepointSubfolders.ASSESSMENT.value}/{ASSESSMENT_SUBFOLDER}" +) + + +# --------------------------------------------------------------------------- +# Harness: a TaskOrchestrator on a real ephemeral PostgreSQL database +# --------------------------------------------------------------------------- + + +@dataclass +class Harness: + orchestrator: TaskOrchestrator + tasks: TaskPostgresRepository + subtasks: SubTaskPostgresRepository + + @contextmanager + def factory(self) -> Generator[TaskOrchestrator, None, None]: + yield self.orchestrator + + +@pytest.fixture +def harness(db_engine: Engine) -> Iterator[Harness]: + with Session(db_engine) as session: + tasks = TaskPostgresRepository(session=session) + subtasks = SubTaskPostgresRepository(session=session) + yield Harness( + orchestrator=TaskOrchestrator(task_repo=tasks, subtask_repo=subtasks), + tasks=tasks, + subtasks=subtasks, + ) + + +# --------------------------------------------------------------------------- +# Fake SharePoint: canned Graph responses keyed by path +# --------------------------------------------------------------------------- + + +@dataclass +class FakeSharepoint: + """Stands in for DomnaSharepointClient with canned Graph listings. Paths not + in ``contents`` raise ValueError, exactly as the real client does for a + folder that does not exist.""" + + contents: dict[str, dict[str, Any]] + reject: dict[str, Exception] = field(default_factory=dict) + renamed: list[tuple[str, str]] = field(default_factory=list) + + def get_folders_in_path(self, path: str) -> dict[str, Any]: + if path not in self.contents: + raise ValueError(f"not found: {path}") + return self.contents[path] + + def rename_file(self, item_id: str, new_name: str) -> None: + if item_id in self.reject: + raise self.reject[item_id] + self.renamed.append((item_id, new_name)) + + +def _file(name: str, item_id: str) -> dict[str, Any]: + return {"name": name, "id": item_id, "file": {}} + + +def _one_property_site(*files: dict[str, Any]) -> dict[str, dict[str, Any]]: + """A site holding exactly one property folder, with `files` in its + assessment folder.""" + return { + BASE_PATH: {"value": []}, + ASSESSMENT_PATH: {"value": list(files)}, + } + + +# --------------------------------------------------------------------------- +# Wiring: the real handler body, re-decorated onto the test orchestrator +# --------------------------------------------------------------------------- + + +def _install( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + sharepoint: FakeSharepoint, + *, + rows: list[tuple[str, str, str]] = [(UPRN, ADDRESS, POSTCODE)], +) -> None: + """Point the handler at the fake SharePoint and a small address list, and + configure the site the tests target.""" + import applications.sharepoint_renamer.handler as module + + monkeypatch.setenv(f"{SITE}_SHAREPOINT_ID", "test-site-id") + monkeypatch.setattr( + module, "DomnaSharepointClient", lambda _site: sharepoint + ) + + csv_file = tmp_path / "addresses.csv" + csv_file.write_text( + "\n".join(["UPRN,Address,Postcode"] + [",".join(r) for r in rows]) + "\n", + encoding="utf-8", + ) + monkeypatch.setattr(module, "CSV_PATH", str(csv_file)) + + +def _handler(harness: Harness) -> Callable[..., Any]: + """The real handler body under the real decorator, wired to the test DB.""" + import applications.sharepoint_renamer.handler as module + + return task_handler( + task_source="sharepoint_renamer", + source=Source.SHAREPOINT_SITE, + orchestrator_cm=harness.factory, + )(module.handler.__wrapped__) + + +def _sqs_event(body: dict[str, Any]) -> dict[str, Any]: + return {"Records": [{"messageId": "msg-1", "body": json.dumps(body)}]} + + +def _subtask_id(result: Any) -> UUID: + return UUID(result["tasks"][0]["subtask_id"]) + + +def _task_id(result: Any) -> UUID: + return UUID(result["tasks"][0]["task_id"]) + + +# --------------------------------------------------------------------------- +# A completed run records what it did +# --------------------------------------------------------------------------- + + +def test_completed_subtask_carries_the_run_summary( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Arrange + sharepoint = FakeSharepoint( + _one_property_site( + _file("Survey.pdf", "id-1"), + _file(f"{UPRN}_High St {POSTCODE}_Done.pdf", "id-2"), + _file("Front.jpg", "id-3"), + ) + ) + _install(monkeypatch, tmp_path, sharepoint) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": SITE}), None) + + # Assert + subtask = harness.subtasks.get(_subtask_id(result)) + assert subtask.status is SubTaskStatus.COMPLETE + assert subtask.outputs == { + "result": { + "renamed": 1, + "would_rename": 0, + "skipped_already_canonical": 1, + "skipped_images": 1, + "missing_folders": [], + "failed": [], + } + } diff --git a/utils/sharepoint/domna_sites.py b/utils/sharepoint/domna_sites.py index 32decaf67..ee92bf787 100644 --- a/utils/sharepoint/domna_sites.py +++ b/utils/sharepoint/domna_sites.py @@ -11,3 +11,32 @@ class DomnaSites(Enum): SOCIAL_HOUSING_WAVE_3 = os.getenv("SOCIAL_HOUSING_WAVE_3_SHAREPOINT_ID") SOCIAL_HOUSING = os.getenv("SOCIAL_HOUSING_SHAREPOINT_ID") ECO = os.getenv("ECO_SHAREPOINT_ID") + + +def resolve_site(name: str) -> DomnaSites: + """The site `name` refers to, or ValueError if it is not a real, configured + site. + + A bare ``DomnaSites[name]`` is not safe here. Members take their value from + the environment at import, so every site an app does not configure is None — + and equal values make Enum collapse them into *aliases of the first member*. + ``DomnaSites["ECO"]`` on a Lambda that only sets + ``SOCIAL_HOUSING_WAVE_3_SHAREPOINT_ID`` therefore returns a perfectly valid + member for a different site, and the run silently touches the wrong + SharePoint. Both failures are rejected here: an unrecognised name, and a + recognised name with no configured id. + + Configuration is checked against the environment at call time rather than + against ``member.value``, which the alias collapse has already destroyed. + """ + if name not in DomnaSites.__members__: + raise ValueError( + f"Unrecognised SharePoint site {name!r}. " + f"Expected one of: {sorted(DomnaSites.__members__)}" + ) + if not os.environ.get(f"{name}_SHAREPOINT_ID"): + raise ValueError( + f"SharePoint site {name!r} has no configured site id " + f"({name}_SHAREPOINT_ID is unset), so it cannot be resolved." + ) + return DomnaSites[name]