From 5dc524be8fadf02d47be96d33fdd60f51a071d1f Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:42:33 +0000 Subject: [PATCH 01/25] =?UTF-8?q?Report=20how=20many=20files=20a=20rename?= =?UTF-8?q?=20run=20renamed=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) --- .../sharepoint_renamer_orchestrator.py | 33 ++++++++++++++++++- tests/scripts/test_rename_sharepoint_files.py | 29 ++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index a73373188..fd4d64154 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -1,5 +1,6 @@ import csv import os +from dataclasses import dataclass, field from typing import Optional from domain.pashub_fetcher.sharepoint_subfolders import SharepointSubfolders @@ -16,6 +17,34 @@ BATCH_FOLDER_PREFIX = "_Sero Batch" logger = setup_logger() +@dataclass +class RenameFailure: + """One file SharePoint refused to rename, and why.""" + + uprn: str + original_name: str + new_name: str + error: str + + +@dataclass +class RenameSummary: + """What a run did, threaded back up through the traversal. + + Counts are per-**file**; ``missing_folders`` is per-**property** (a UPRN with + no assessment folder under any root, so no file was even looked at).""" + + renamed: int = 0 + would_rename: int = 0 + skipped_already_canonical: int = 0 + skipped_images: int = 0 + missing_folders: list[str] = field(default_factory=list) + failed: list[RenameFailure] = field(default_factory=list) + + def merge_in(self, other: "RenameSummary") -> None: + raise NotImplementedError + + def build_canonical_filename( uprn: str, address: str, postcode: str, original_name: str ) -> Optional[str]: @@ -76,7 +105,8 @@ class SharepointRenamerOrchestrator: ) return [BASE_PATH, *batch_roots] - def run(self) -> None: + def run(self) -> RenameSummary: + summary = RenameSummary() roots = self._discover_roots() with open(self._csv_path, newline="", encoding="utf-8-sig") as f: reader = csv.DictReader(f) @@ -93,6 +123,7 @@ class SharepointRenamerOrchestrator: address=row["Address"].strip(), postcode=row["Postcode"].strip(), ) + return summary def _process_row( self, roots: list[str], uprn: str, address: str, postcode: str diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index cd27d8a29..8605a5e9d 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -8,6 +8,7 @@ from domain.pashub_fetcher.sharepoint_subfolders import SharepointSubfolders from orchestration.sharepoint_renamer_orchestrator import ( ASSESSMENT_SUBFOLDER, BASE_PATH, + RenameSummary, SharepointRenamerOrchestrator, build_canonical_filename, ) @@ -124,6 +125,34 @@ def test_run_renames_files_for_property_under_batch_root(tmp_path: Path) -> None sp.rename_file.assert_called_once_with("id-1", "100_1 High St AB1 2CD_Survey.pdf") +def test_run_summary_counts_each_renamed_file(tmp_path: Path) -> None: + # Arrange + csv_path = _write_csv(tmp_path, [("100", "1 High St", "AB1 2CD")]) + assessment = _assessment_path(BASE_PATH, "1 High St", "AB1 2CD") + + sp = MagicMock() + + def fake_get(path: str) -> dict[str, Any]: + if path == BASE_PATH: + return {"value": []} + if path == assessment: + return { + "value": [ + _make_file("Survey.pdf", "id-1"), + _make_file("Report.docx", "id-2"), + ] + } + raise ValueError("not found") + + sp.get_folders_in_path.side_effect = fake_get + + # Act + summary = SharepointRenamerOrchestrator(sp, csv_path).run() + + # Assert + assert summary.renamed == 2 + + def test_run_uprn_prefixed_address_yields_single_uprn_filename( tmp_path: Path, ) -> None: From 391e7bee37bf3501ca2394cc24310726ec1afef0 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:43:31 +0000 Subject: [PATCH 02/25] =?UTF-8?q?Report=20how=20many=20files=20a=20rename?= =?UTF-8?q?=20run=20renamed=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer_orchestrator.py | 41 +++++++++++++------ tests/scripts/test_rename_sharepoint_files.py | 4 +- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index fd4d64154..afa878d60 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -42,7 +42,12 @@ class RenameSummary: failed: list[RenameFailure] = field(default_factory=list) def merge_in(self, other: "RenameSummary") -> None: - raise NotImplementedError + self.renamed += other.renamed + self.would_rename += other.would_rename + self.skipped_already_canonical += other.skipped_already_canonical + self.skipped_images += other.skipped_images + self.missing_folders.extend(other.missing_folders) + self.failed.extend(other.failed) def build_canonical_filename( @@ -117,17 +122,19 @@ class SharepointRenamerOrchestrator: ) for row in reader: - self._process_row( - roots, - uprn=row["UPRN"].strip(), - address=row["Address"].strip(), - postcode=row["Postcode"].strip(), + summary.merge_in( + self._process_row( + roots, + uprn=row["UPRN"].strip(), + address=row["Address"].strip(), + postcode=row["Postcode"].strip(), + ) ) return summary def _process_row( self, roots: list[str], uprn: str, address: str, postcode: str - ) -> None: + ) -> RenameSummary: # Batch folders name properties "{uprn}_{address}"; filenames must not # repeat the UPRN, so the folder lookup and the canonical name diverge. display_address = address.removeprefix(f"{uprn}_") @@ -136,25 +143,32 @@ class SharepointRenamerOrchestrator: f"{root}/{address}, {postcode}" f"/{SharepointSubfolders.ASSESSMENT.value}/{ASSESSMENT_SUBFOLDER}" ) - if self._process_folder(folder_path, uprn, display_address, postcode): - return + found = self._process_folder(folder_path, uprn, display_address, postcode) + if found is not None: + return found logger.warning( f"Missing folder for UPRN {uprn} in any root: {address}, {postcode}" ) + return RenameSummary() def _process_folder( self, folder_path: str, uprn: str, address: str, postcode: str - ) -> bool: + ) -> Optional[RenameSummary]: + """What renaming this folder (and its subfolders) did, or None when the + folder does not exist under this root.""" try: contents = self._sp_client.get_folders_in_path(folder_path) except ValueError: - return False + return None + summary = RenameSummary() for item in contents.get("value", []): if "folder" in item: - self._process_folder( + child = self._process_folder( f"{folder_path}/{item['name']}", uprn, address, postcode ) + if child is not None: + summary.merge_in(child) elif "file" in item: original_name: str = item["name"] if os.path.splitext(original_name)[1].lower() in {".jpg", ".heic"}: @@ -173,6 +187,7 @@ class SharepointRenamerOrchestrator: else: try: self._sp_client.rename_file(item["id"], new_name) + summary.renamed += 1 logger.info( f'Renamed: "{original_name}" → "{new_name}" (UPRN: {uprn})' ) @@ -180,4 +195,4 @@ class SharepointRenamerOrchestrator: logger.error( f'Failed to rename "{original_name}" → "{new_name}" (UPRN: {uprn}): {e}' ) - return True + return summary diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index 8605a5e9d..e3b039d91 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -254,7 +254,7 @@ def test_ignores_package_items() -> None: # --------------------------------------------------------------------------- -def test_missing_folder_returns_false_without_warning( +def test_missing_folder_returns_nothing_without_warning( caplog: pytest.LogCaptureFixture, ) -> None: # Arrange @@ -267,7 +267,7 @@ def test_missing_folder_returns_false_without_warning( ) # Assert - assert found is False + assert found is None sp.rename_file.assert_not_called() assert not any("Missing folder" in r.message for r in caplog.records) From 0135e715908f48c28f0e97a9294dd0ed9e6008b5 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:44:20 +0000 Subject: [PATCH 03/25] =?UTF-8?q?Count=20dry-run=20rename=20intents=20sepa?= =?UTF-8?q?rately=20from=20real=20renames=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) --- docs/abri-hubspot-ops-guide.md | 173 +++++++++++++----- tests/scripts/test_rename_sharepoint_files.py | 16 ++ 2 files changed, 141 insertions(+), 48 deletions(-) diff --git a/docs/abri-hubspot-ops-guide.md b/docs/abri-hubspot-ops-guide.md index ae5db8922..97c6cfed0 100644 --- a/docs/abri-hubspot-ops-guide.md +++ b/docs/abri-hubspot-ops-guide.md @@ -9,6 +9,24 @@ field changes are picked up automatically and sent to Abri's scheduling system --- +## At a glance: which fields trigger what + +| You change… | Abri's system… | +|---|---| +| **Expected commencement date** — set for the first time | Sends us the tenancy details, so tenant contacts appear on the deal | +| **Confirmed survey date** + **Third-party surveyor identifier** — the moment *both* are filled in | Creates the survey booking (a "job") | +| **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor identifier** — changed on a deal that already has a booking | Updates the existing appointment | +| **Number of attempts** reaches 3 *and* **Outcome** is one of the four unsuccessful values | Cancels the booking as abandoned | + +Three fields are inputs to those triggers rather than triggers themselves: + +- **Deal owner** — a HubSpot workflow maps it to the surveyor's Abri resource ID and + stores that in **Third-party surveyor identifier**, so setting the owner is how you set + the surveyor. +- **Client booking reference** — written back **by the system** with Abri's job number + once the booking is created. Never edit it (see [watch-outs](#things-to-watch-out-for)). +- **Deal name** — used as the job description Abri's staff see. + ## Which deals does this apply to? Only deals associated with the **Abri Stock Condition — Privately Funded** project. @@ -16,22 +34,86 @@ Changes to any other deal are ignored by this integration. ## How fast does it happen? -Changes are picked up automatically shortly after you save them in HubSpot — usually -within a few minutes. Nothing fires while you are mid-edit; it's the saved change that +Changes are picked up automatically shortly after you save them in HubSpot — the workflow runs every 20 minutes. Nothing fires while you are mid-edit; it's the saved change that counts. --- -## What your changes do in Abri's system +## The detail: what each trigger does -| When you… | Abri's system… | How you know it worked | -|---|---|---| -| Set **Expected commencement date** for the first time | Sends us the tenancy details for the property | Tenant contact records appear in HubSpot, linked to the deal — unless Abri reports the property as empty (see below) | -| Set **Confirmed survey date** for the first time | Creates the survey booking (a "job") against the property, assigned to the surveyor on the deal | Abri's job number appears in **Client booking reference** on the deal | -| Change **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor identifier** on a deal that already has a booking | Updates the existing appointment — new date/time and/or reassigns it to the new surveyor | The deal keeps the same Client booking reference | -| Record a **3rd attempt** (Number of attempts reaches 3) **and** set **Outcome** to an unsuccessful value (see below) | Cancels the booking as abandoned | — (this only fires once per deal) | +### Fetching tenant details -**Unsuccessful outcomes** that count towards abandonment — the wording must match exactly: +Setting **Expected commencement date** for the first time asks Abri for the property's +tenancy details. Tenant contact records then appear in HubSpot, linked to the deal — +unless Abri reports the property as empty (see below). Only the *first* time the date is +set counts; changing it later does not re-fetch. + +**Empty properties.** If Abri has no live tenancy for the property, no tenant contacts +appear on the deal — there is nobody to add — and **Extra booking information** is +prefixed with `Void.`. Anything already in that field is +kept, after the prefix. This is expected, and nothing needs re-triggering. + +Delete the `Void.` prefix if you establish the property is not empty after all — the +system only adds it once, so it will not come back on its own. + +### The surveyor comes from the Deal owner + +You don't fill in **Third-party surveyor identifier** yourself. HubSpot workflows do it: +each recognised surveyor has a workflow that takes the **Deal owner**, maps it to their +Abri resource ID, and stores that as the identifier. So the field to get right is **Deal +owner** — set it to the surveyor doing the visit and the identifier follows. + +Because the owner is picked from a dropdown it can't be misspelt, so this mostly just +works. The two things that can still go wrong: + +- The deal owner is someone who isn't one of the recognised surveyors — no workflow + fires, the identifier stays blank, and no booking is sent. +- Someone edits the identifier field itself by hand — free-typed or misspelt codes are + rejected by Abri's system and the booking won't be made. Leave it to the workflow. + +The tech team maintains the workflows and the list of recognised surveyors — if a +surveyor is missing, that's where to raise it. + +### Creating the booking + +The booking is created the moment the deal has **both** a **Confirmed survey date** and a +**Third-party surveyor identifier**. The order doesn't matter — whichever of the two +lands second is the change that sends the booking. Until both are in place, nothing is +sent. + +Since the identifier comes from the Deal owner (above), in practice booking a survey +means making sure the deal owner is the surveyor and setting the confirmed date. + +The booking is assigned within Abri's system to the surveyor on the deal on the confirmed date, in the slot +implied by **Confirmed survey time**: + +| Confirmed survey time | Slot sent to Abri | +|---|---| +| `morning` | AM | +| `afternoon` | PM | +| A clock time like `09:30` | AM or PM by whether it's before midday | +| Left blank | All day | + +**How you know it worked:** Abri's job number appears in **Client booking reference** on +the deal. A deal only ever gets one booking — once it has a job number, later changes +update that booking rather than creating another. + +### Changing the booking + +Changing **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor +identifier** on a deal that already has a booking updates the existing appointment — +new date/time and/or reassigns it to the new surveyor. (Reassigning works the same way as +booking: change the **Deal owner** and the workflow updates the identifier.) The deal +keeps the same Client booking reference. The date and surveyor must both still be filled in for the change to +be sent (see [watch-outs](#things-to-watch-out-for) on clearing fields). + +### Abandoning the booking + +When **Number of attempts** reaches 3 (or more) *and* **Outcome** is set to an +unsuccessful value, the booking is cancelled as abandoned. This fires once per deal. + +**Unsuccessful outcomes** that count towards abandonment — the wording must match +exactly: - `no answer` - `cancelled / no show` @@ -41,61 +123,56 @@ counts. Any other outcome (or fewer than 3 attempts) does **not** cancel anything in Abri's system. -**Empty properties.** If Abri has no live tenancy for the property, no tenant contacts -appear on the deal — there is nobody to add — and **Extra booking information** is -prefixed with `Void.` so the surveyor sees it first. Anything already in that field is -kept, after the prefix. This is expected, not a fault, and nothing needs re-triggering. - -Note that Abri sends the same signal for a property reference it does not recognise, so -if you were expecting tenants, check the property is right before assuming it's empty. -Delete the `Void.` prefix if you establish the property is not empty after all — the -system only adds it once, so it will not come back on its own. - -## The fields, and why they matter - -| HubSpot deal field | What it feeds | -|---|---| -| **Expected commencement date** | Setting it the first time triggers the tenant-details fetch | -| **Confirmed survey date** | The appointment date sent to Abri | -| **Confirmed survey time** | The appointment slot: `morning` → AM, `afternoon` → PM, a clock time like `09:30` → AM/PM by whether it's before midday, left blank → all day | -| **Third-party surveyor identifier** | Which surveyor the booking is assigned to in Abri's system. **Required** — a booking or appointment change cannot be sent without it, and it must be one of Abri's valid surveyor codes. Changing it on a deal that already has a booking reassigns that booking to the new surveyor | -| **Number of attempts** + **Outcome** | Together they trigger abandonment (3+ attempts and an unsuccessful outcome) | -| **Client booking reference** | Abri's job number, written back **by the system** after the booking is created | -| **Deal name** | Used as the job description Abri's staff see | - --- -## Things that will catch you out +## Things to watch out for + +### No booking is made until both date and surveyor are filled in + +Entering a survey date on a deal whose **Third-party surveyor identifier** is blank sends +nothing — the system is waiting for the surveyor. It fires as soon as the second of the +two fields is filled in, so a deal can sit half-complete indefinitely without an error. +If a booking you expected hasn't appeared, check both fields are set — and since the +identifier comes from the +[Deal owner](#the-surveyor-comes-from-the-deal-owner), a blank identifier usually means +the owner is missing or isn't a recognised surveyor. ### Clearing the survey date does NOT cancel the booking -If you delete or blank the confirmed survey date, the booking **still exists in Abri's -system** and the surveyor is still expected. There is currently no way to cancel a -booking from HubSpot other than the 3-attempts abandonment route. If a booking needs -cancelling for any other reason, contact the tech team. +If you delete or blank the confirmed survey date (or the surveyor), the booking **still +exists in Abri's system** and the surveyor is still expected — clearing a field sends +nothing. There is currently no way to cancel a booking from HubSpot other than the +3-attempts abandonment route. If a booking needs cancelling for any other reason, +contact the tech team. (Re-entering a date later updates the existing booking; it will +not create a duplicate.) ### Don't edit Client booking reference -It is written automatically with Abri's job number and is how the system finds the -booking when you later change or abandon it. If you overwrite or clear it, appointment -changes and abandonments will stop working for that deal. +It is written automatically with Abri's job number and is how the system tells a new +booking from a change to an existing one: -### The surveyor identifier (Third-Party Surveyor Identifier) must be a valid Abri code - -Free-typed or misspelt values will be rejected by Abri's system and the booking won't be -made. The tech team holds the current list of valid surveyor codes from Abri — use a -value from that list. Soon this field will be set automatically from the deal -owner. +- On a deal that is **not yet booked**, typing anything into this field stops the + booking from ever being created — the system reads a value there as "already booked". +- On a **booked** deal, overwriting or clearing it breaks appointment changes and + abandonment for that deal — those changes fail over to the tech team rather than + reaching Abri. ### Outcome wording is exact Abandonment only recognises the four unsuccessful outcomes listed above, spelled exactly that way. A variation like "No Answer - left card" won't trigger it. +### Abandonment only cancels an actual booking + +Recording 3 unsuccessful attempts on a deal that never got as far as a booking (no +Client booking reference) has nothing to cancel — the attempt lands with the tech team +as an error rather than doing anything in Abri's system. That's harmless, but worth +knowing if you're tidying up deals that never went ahead. + ### If the job number never appears The booking didn't go through — most often because the surveyor identifier was missing or invalid, or the deal isn't on the Abri project. Failures land with the tech team, not in HubSpot, so you won't see an error message. If **Client booking reference** is still -empty well after you set the survey date, flag it to the tech team rather than re-editing -fields. +empty well after both the survey date and surveyor are set, flag it to the tech team +rather than re-editing fields. diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index e3b039d91..e83782b2a 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -331,3 +331,19 @@ def test_dry_run_logs_would_rename_without_calling_api( sp.rename_file.assert_not_called() assert any("Would rename" in r.message for r in caplog.records) + + +def test_dry_run_counts_intents_separately_from_renames() -> None: + # Arrange + sp = MagicMock() + sp.get_folders_in_path.return_value = { + "value": [_make_file("Survey.pdf", "id-1"), _make_file("Report.docx", "id-2")] + } + + # Act + summary = _make_orchestrator(sp, dry_run=True)._process_folder( + "some/path", "100", "1 High St", "AB1 2CD" + ) + + # Assert + assert summary == RenameSummary(renamed=0, would_rename=2) From 371df4d54f1fd6bccebe8fabe190ecf7282a07a4 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:44:37 +0000 Subject: [PATCH 04/25] =?UTF-8?q?Count=20dry-run=20rename=20intents=20sepa?= =?UTF-8?q?rately=20from=20real=20renames=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- orchestration/sharepoint_renamer_orchestrator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index afa878d60..5a722f5af 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -181,6 +181,7 @@ class SharepointRenamerOrchestrator: continue if self._dry_run: + summary.would_rename += 1 logger.info( f'Would rename: "{original_name}" → "{new_name}" (UPRN: {uprn})' ) From 9e48f39fa5e0247525255f45f9b4a60f57836678 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:45:20 +0000 Subject: [PATCH 05/25] =?UTF-8?q?List=20the=20UPRNs=20with=20no=20SharePoi?= =?UTF-8?q?nt=20folder=20under=20any=20root=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) --- tests/scripts/test_rename_sharepoint_files.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index e83782b2a..7b1ee0680 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -296,6 +296,33 @@ def test_run_warns_once_when_property_missing_from_all_roots( assert "400" in warnings[0].message +def test_run_summary_lists_uprns_with_no_folder_under_any_root( + tmp_path: Path, +) -> None: + # Arrange + csv_path = _write_csv( + tmp_path, [("400", "4 Elm St", "AA2 2BB"), ("500", "5 Pine Ln", "BB3 3CC")] + ) + found_assessment = _assessment_path(BASE_PATH, "5 Pine Ln", "BB3 3CC") + + sp = MagicMock() + + def fake_get(path: str) -> dict[str, Any]: + if path == BASE_PATH: + return {"value": []} + if path == found_assessment: + return {"value": []} + raise ValueError("not found") + + sp.get_folders_in_path.side_effect = fake_get + + # Act + summary = SharepointRenamerOrchestrator(sp, csv_path).run() + + # Assert + assert summary.missing_folders == ["400"] + + # --------------------------------------------------------------------------- # _process_folder — already-canonical files are skipped # --------------------------------------------------------------------------- From 9248aee139c37a4b616f4964a6280d985108acd6 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:45:46 +0000 Subject: [PATCH 06/25] =?UTF-8?q?List=20the=20UPRNs=20with=20no=20SharePoi?= =?UTF-8?q?nt=20folder=20under=20any=20root=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- orchestration/sharepoint_renamer_orchestrator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index 5a722f5af..68857a60b 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -149,7 +149,7 @@ class SharepointRenamerOrchestrator: logger.warning( f"Missing folder for UPRN {uprn} in any root: {address}, {postcode}" ) - return RenameSummary() + return RenameSummary(missing_folders=[uprn]) def _process_folder( self, folder_path: str, uprn: str, address: str, postcode: str From a69d603cded43469ade12bd597b53ba551c667bd Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:49:17 +0000 Subject: [PATCH 07/25] =?UTF-8?q?Record=20each=20file=20SharePoint=20refus?= =?UTF-8?q?ed=20to=20rename,=20with=20its=20error=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) --- tests/scripts/test_rename_sharepoint_files.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index 7b1ee0680..aa54c3af0 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -8,6 +8,7 @@ from domain.pashub_fetcher.sharepoint_subfolders import SharepointSubfolders from orchestration.sharepoint_renamer_orchestrator import ( ASSESSMENT_SUBFOLDER, BASE_PATH, + RenameFailure, RenameSummary, SharepointRenamerOrchestrator, build_canonical_filename, @@ -36,6 +37,10 @@ def _make_folder(name: str) -> dict[str, Any]: return {"name": name, "folder": {}} +def _raise(error: Exception) -> None: + raise error + + def _make_package(name: str) -> dict[str, Any]: return {"name": name, "package": {}} @@ -328,6 +333,35 @@ def test_run_summary_lists_uprns_with_no_folder_under_any_root( # --------------------------------------------------------------------------- +def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename() -> None: + # Arrange + sp = MagicMock() + sp.get_folders_in_path.return_value = { + "value": [_make_file("Locked.pdf", "id-1"), _make_file("Survey.pdf", "id-2")] + } + sp.rename_file.side_effect = lambda item_id, _new_name: ( + _raise(PermissionError("locked by another user")) if item_id == "id-1" else None + ) + + # Act + summary = _make_orchestrator(sp)._process_folder( + "some/path", "100", "1 High St", "AB1 2CD" + ) + + # Assert + assert summary == RenameSummary( + renamed=1, + failed=[ + RenameFailure( + uprn="100", + original_name="Locked.pdf", + new_name="100_1 High St AB1 2CD_Locked.pdf", + error="locked by another user", + ) + ], + ) + + def test_skips_already_canonical_files() -> None: sp = MagicMock() sp.get_folders_in_path.return_value = { From ae7c9cdd46cf41e8f173202d19c31cd63a03c61d Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:49:51 +0000 Subject: [PATCH 08/25] =?UTF-8?q?Record=20each=20file=20SharePoint=20refus?= =?UTF-8?q?ed=20to=20rename,=20with=20its=20error=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- orchestration/sharepoint_renamer_orchestrator.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index 68857a60b..88912eb45 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -193,6 +193,14 @@ class SharepointRenamerOrchestrator: f'Renamed: "{original_name}" → "{new_name}" (UPRN: {uprn})' ) except Exception as e: + summary.failed.append( + RenameFailure( + uprn=uprn, + original_name=original_name, + new_name=new_name, + error=str(e), + ) + ) logger.error( f'Failed to rename "{original_name}" → "{new_name}" (UPRN: {uprn}): {e}' ) From 2aa698da1ef8bd6340db19012f2e350e4d7acf29 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:50:45 +0000 Subject: [PATCH 09/25] =?UTF-8?q?Count=20files=20skipped=20as=20already=20?= =?UTF-8?q?canonical=20apart=20from=20skipped=20images=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) --- tests/scripts/test_rename_sharepoint_files.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index aa54c3af0..b26b204a5 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -373,6 +373,28 @@ def test_skips_already_canonical_files() -> None: sp.rename_file.assert_not_called() +def test_summary_counts_already_canonical_and_image_skips_apart() -> None: + # Arrange + sp = MagicMock() + sp.get_folders_in_path.return_value = { + "value": [ + _make_file("500_Pine Ln BB3 3CC_Doc.pdf", "id-1"), + _make_file("Front elevation.JPG", "id-2"), + _make_file("Rear elevation.heic", "id-3"), + ] + } + + # Act + summary = _make_orchestrator(sp)._process_folder( + "some/path", "500", "5 Pine Ln", "BB3 3CC" + ) + + # Assert + assert summary == RenameSummary( + skipped_already_canonical=1, skipped_images=2 + ) + + # --------------------------------------------------------------------------- # _process_folder — dry_run=True logs intent but never calls rename_file # --------------------------------------------------------------------------- From b738840c84d08ffa31df03fcdbd6538ff500f9f6 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:51:02 +0000 Subject: [PATCH 10/25] =?UTF-8?q?Count=20files=20skipped=20as=20already=20?= =?UTF-8?q?canonical=20apart=20from=20skipped=20images=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- orchestration/sharepoint_renamer_orchestrator.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index 88912eb45..f35b14912 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -172,12 +172,14 @@ class SharepointRenamerOrchestrator: elif "file" in item: original_name: str = item["name"] if os.path.splitext(original_name)[1].lower() in {".jpg", ".heic"}: + summary.skipped_images += 1 continue new_name = build_canonical_filename( uprn, address, postcode, original_name ) if new_name is None: + summary.skipped_already_canonical += 1 continue if self._dry_run: From 5309c1f097e7468c7c4c70b45ed76b9ff7a38c5a Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:51:36 +0000 Subject: [PATCH 11/25] =?UTF-8?q?Merge=20subfolder=20rename=20results=20in?= =?UTF-8?q?to=20their=20parent=20folder's=20summary=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passed on arrival: threading the summary back up through the traversal already carried subfolder results. Pinned so the merge cannot regress. Co-Authored-By: Claude Opus 5 (1M context) --- tests/scripts/test_rename_sharepoint_files.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index b26b204a5..c3dc9cd03 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -239,6 +239,31 @@ def test_recurses_into_subfolders_and_renames_all_files() -> None: sp.get_folders_in_path.assert_any_call("base/path/SubA") +def test_subfolder_results_merge_into_the_parent_summary() -> None: + # Arrange + sp = MagicMock() + contents: dict[str, dict[str, Any]] = { + "base/path": { + "value": [_make_file("Root.pdf", "root-file"), _make_folder("SubA")] + }, + "base/path/SubA": { + "value": [ + _make_file("Sub.pdf", "sub-file"), + _make_file("200_2 Main Rd XY9 8ZW_Done.pdf", "canonical-file"), + ] + }, + } + sp.get_folders_in_path.side_effect = lambda path: contents[path] + + # Act + summary = _make_orchestrator(sp)._process_folder( + "base/path", "200", "2 Main Rd", "XY9 8ZW" + ) + + # Assert + assert summary == RenameSummary(renamed=2, skipped_already_canonical=1) + + # --------------------------------------------------------------------------- # _process_folder — non-file, non-folder items are skipped # --------------------------------------------------------------------------- From e7785f6b760653d1e726cea95e3eadac9a36d539 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:58:18 +0000 Subject: [PATCH 12/25] =?UTF-8?q?Record=20a=20rename=20run's=20result=20su?= =?UTF-8?q?mmary=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] From 694881c110a1f8922ab15f67e9417093ddec8ca1 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:58:47 +0000 Subject: [PATCH 13/25] =?UTF-8?q?Record=20a=20rename=20run's=20result=20su?= =?UTF-8?q?mmary=20on=20its=20sub=5Ftask=20=F0=9F=9F=A9?= 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 | 29 +++++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/applications/sharepoint_renamer/handler.py b/applications/sharepoint_renamer/handler.py index b2d94f8c4..d204c8f46 100644 --- a/applications/sharepoint_renamer/handler.py +++ b/applications/sharepoint_renamer/handler.py @@ -34,17 +34,38 @@ CSV_PATH = os.path.join( def _summary_json(summary: RenameSummary) -> dict[str, Any]: - raise NotImplementedError + """The portal's view of a run, mapped field by field rather than derived + from ``RenameSummary``. The explicit mapping is the point: it keeps the + portal's contract stable when the internal type is renamed or reshaped, and + ``outputs`` is serialised to text so it must be plain JSON either way. + + Counts are per-**file**; ``missing_folders`` is per-**property**.""" + return { + "renamed": summary.renamed, + "would_rename": summary.would_rename, + "skipped_already_canonical": summary.skipped_already_canonical, + "skipped_images": summary.skipped_images, + "missing_folders": summary.missing_folders, + "failed": [ + { + "uprn": failure.uprn, + "original_name": failure.original_name, + "new_name": failure.new_name, + "error": failure.error, + } + for failure in summary.failed + ], + } @task_handler(task_source="sharepoint_renamer", source=Source.SHAREPOINT_SITE) -def handler(body: dict[str, Any], context: Any) -> Any: +def handler(body: dict[str, Any], context: Any) -> dict[str, Any]: request = SharepointRenamerRequest.model_validate(body) sp_client = DomnaSharepointClient(resolve_site(request.sharepoint_site)) - SharepointRenamerOrchestrator( + summary = SharepointRenamerOrchestrator( sp_client, CSV_PATH, dry_run=request.dry_run ).run() - return None + return _summary_json(summary) if __name__ == "__main__": From 1213b80e3f53d244e5f706074c3df34d3b75d184 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 08:59:45 +0000 Subject: [PATCH 14/25] =?UTF-8?q?Honour=20dry=5Frun=20on=20a=20queue-deliv?= =?UTF-8?q?ered=20rename=20request=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Green on arrival: reading the request from the SQS record body is what taking the task lane gave us. Verified the guard bites by restoring the raw-event read, under which the "dry" run renames a live file. Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer/test_handler.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index 06ea241e6..cd0ce98ca 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -197,3 +197,38 @@ def test_completed_subtask_carries_the_run_summary( "failed": [], } } + + +# --------------------------------------------------------------------------- +# A queue-delivered dry run is actually a dry run +# --------------------------------------------------------------------------- + + +def test_dry_run_in_an_sqs_record_body_renames_nothing( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The request must come from the SQS *record body*, not the raw Lambda + event. Validating the raw event leaves every unknown key ignored, so + ``dry_run`` fell back to False and every queue-triggered run renamed live + files — the flag only ever worked on a direct invoke.""" + # Arrange + sharepoint = FakeSharepoint(_one_property_site(_file("Survey.pdf", "id-1"))) + _install(monkeypatch, tmp_path, sharepoint) + + # Act + result = _handler(harness)( + _sqs_event({"sharepoint_site": SITE, "dry_run": True}), None + ) + + # Assert + assert sharepoint.renamed == [] + assert harness.subtasks.get(_subtask_id(result)).outputs == { + "result": { + "renamed": 0, + "would_rename": 1, + "skipped_already_canonical": 0, + "skipped_images": 0, + "missing_folders": [], + "failed": [], + } + } From 0836b18d5be9284cf4d8e4472140addeae21a335 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:01:51 +0000 Subject: [PATCH 15/25] =?UTF-8?q?Attribute=20a=20rename=20run=20to=20its?= =?UTF-8?q?=20request=20and=20target=20site=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Green on arrival: @task_handler records inputs and reads source_id from the body key matching the source's value. Pinned because it is the only place the new sharepoint_site member is proven against a real Postgres enum rather than a Python one. Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer/test_handler.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index cd0ce98ca..f88182005 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -199,6 +199,34 @@ def test_completed_subtask_carries_the_run_summary( } +# --------------------------------------------------------------------------- +# A run is attributable: its request, and the site it targeted +# --------------------------------------------------------------------------- + + +def test_run_records_its_request_and_target_site( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """``sharepoint_site`` round-trips through the real ``source`` pgEnum — the + member the FE's Drizzle migration must add before this deploys.""" + # Arrange + sharepoint = FakeSharepoint(_one_property_site()) + _install(monkeypatch, tmp_path, sharepoint) + body = {"sharepoint_site": SITE, "dry_run": True} + + # Act + result = _handler(harness)(_sqs_event(body), None) + + # Assert + task = harness.tasks.get(_task_id(result)) + assert (task.task_source, task.source, task.source_id) == ( + "sharepoint_renamer", + Source.SHAREPOINT_SITE, + SITE, + ) + assert harness.subtasks.get(_subtask_id(result)).inputs == body + + # --------------------------------------------------------------------------- # A queue-delivered dry run is actually a dry run # --------------------------------------------------------------------------- From be1d6faea03ec3901e75faaf1728f61ffad286fd Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:03:43 +0000 Subject: [PATCH 16/25] =?UTF-8?q?Complete=20a=20rename=20run=20that=20hit?= =?UTF-8?q?=20locked=20files=20or=20missing=20folders,=20and=20reject=20an?= =?UTF-8?q?=20unusable=20site=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Green on arrival: both fall out of the summary the run now returns and of the guarded site lookup. Pinned after verifying they bite — an unguarded DomnaSites[name] completes an "ECO" run that renames files under a different site's alias. Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer/test_handler.py | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index f88182005..d30a388d5 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -260,3 +260,96 @@ def test_dry_run_in_an_sqs_record_body_renames_nothing( "failed": [], } } + + +# --------------------------------------------------------------------------- +# Partial failures complete, they do not fail the run +# --------------------------------------------------------------------------- + + +def test_run_completes_with_rejected_files_and_missing_folders_listed( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """One locked file, or a property with no folder, must not turn a + productive run red — the failures are the record (ADR-0060).""" + # Arrange + sharepoint = FakeSharepoint( + _one_property_site( + _file("Locked.pdf", "id-1"), _file("Survey.pdf", "id-2") + ), + reject={"id-1": PermissionError("locked by another user")}, + ) + _install( + monkeypatch, + tmp_path, + sharepoint, + rows=[(UPRN, ADDRESS, POSTCODE), ("999", "9 Nowhere Rd", "ZZ9 9ZZ")], + ) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": SITE}), None) + + # Assert + assert harness.tasks.get(_task_id(result)).status is TaskStatus.COMPLETE + 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": 0, + "skipped_images": 0, + "missing_folders": ["999"], + "failed": [ + { + "uprn": UPRN, + "original_name": "Locked.pdf", + "new_name": f"{UPRN}_{ADDRESS} {POSTCODE}_Locked.pdf", + "error": "locked by another user", + } + ], + } + } + + +# --------------------------------------------------------------------------- +# An unusable site is rejected before anything is touched +# --------------------------------------------------------------------------- + + +def test_unconfigured_site_fails_the_run_without_touching_sharepoint( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Sites take their id from the environment at import, so every site this + Lambda does not configure collapses into an alias of the first one. An + unguarded lookup would return a valid member for the *wrong* site.""" + # Arrange + sharepoint = FakeSharepoint(_one_property_site(_file("Survey.pdf", "id-1"))) + _install(monkeypatch, tmp_path, sharepoint) + monkeypatch.delenv("ECO_SHAREPOINT_ID", raising=False) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": "ECO"}), None) + + # Assert + subtask = harness.subtasks.get(_subtask_id(result)) + assert subtask.status is SubTaskStatus.FAILED + assert "ECO_SHAREPOINT_ID is unset" in (subtask.outputs or {})["error"] + assert sharepoint.renamed == [] + + +def test_unrecognised_site_fails_the_run( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Arrange + sharepoint = FakeSharepoint(_one_property_site(_file("Survey.pdf", "id-1"))) + _install(monkeypatch, tmp_path, sharepoint) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": "NOT_A_SITE"}), None) + + # Assert + subtask = harness.subtasks.get(_subtask_id(result)) + assert subtask.status is SubTaskStatus.FAILED + assert "Unrecognised SharePoint site 'NOT_A_SITE'" in (subtask.outputs or {})["error"] + assert sharepoint.renamed == [] From 9870005230bbc43e1ecd747d424e9b20dbd52736 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:11:41 +0000 Subject: [PATCH 17/25] =?UTF-8?q?Ship=20the=20Renamer=20image=20and=20infr?= =?UTF-8?q?astructure=20with=20database=20access=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The task lane pulls the task domain types, both task repositories and four Postgres modules into the image, plus SQLAlchemy, SQLModel and a driver. Terraform gains the DB credentials block and five Postgres env vars; the deploy job gains the three DB secrets the shared workflow already declares. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/deploy_terraform.yml | 3 +++ .../sharepoint_renamer/handler/Dockerfile | 13 ++++++++++++ .../handler/requirements.txt | 7 +++++++ .../lambda/sharepoint_renamer/main.tf | 16 ++++++++++++++ .../lambda/sharepoint_renamer/variables.tf | 21 ++++++++++++++++++- 5 files changed, 59 insertions(+), 1 deletion(-) diff --git a/.github/workflows/deploy_terraform.yml b/.github/workflows/deploy_terraform.yml index 48158e4b3..91fb49c18 100644 --- a/.github/workflows/deploy_terraform.yml +++ b/.github/workflows/deploy_terraform.yml @@ -532,6 +532,9 @@ jobs: TF_VAR_sharepoint_client_secret: ${{ secrets.SHAREPOINT_CLIENT_SECRET }} TF_VAR_sharepoint_tenant_id: ${{ secrets.SHAREPOINT_TENANT_ID }} TF_VAR_social_housing_wave_3_sharepoint_id: ${{ secrets.SOCIAL_HOUSING_WAVE_3_SHAREPOINT_ID }} + TF_VAR_db_host: ${{ secrets.DEV_DB_HOST }} + TF_VAR_db_name: ${{ secrets.DEV_DB_NAME }} + TF_VAR_db_port: ${{ secrets.DEV_DB_PORT }} # ============================================================ diff --git a/applications/sharepoint_renamer/handler/Dockerfile b/applications/sharepoint_renamer/handler/Dockerfile index 40373e7b0..7a11d7ee6 100644 --- a/applications/sharepoint_renamer/handler/Dockerfile +++ b/applications/sharepoint_renamer/handler/Dockerfile @@ -10,6 +10,19 @@ COPY utilities/ utilities/ COPY backend/__init__.py backend/__init__.py # SharepointSubfolders only — the rest of the PasHub service is not needed here. COPY domain/pashub_fetcher/ domain/pashub_fetcher/ +# The app-owned-task lane (@task_handler -> TaskOrchestrator -> Postgres): the +# task domain types, both task repositories, and the Postgres modules they +# reach. Copied file by file rather than whole-package to keep the image +# minimal — tests/test_lambda_packaging.py is what says whether it is complete. +COPY domain/tasks/ domain/tasks/ +COPY repositories/__init__.py repositories/__init__.py +COPY repositories/tasks/ repositories/tasks/ +COPY infrastructure/__init__.py infrastructure/__init__.py +COPY infrastructure/postgres/__init__.py infrastructure/postgres/__init__.py +COPY infrastructure/postgres/config.py infrastructure/postgres/config.py +COPY infrastructure/postgres/engine.py infrastructure/postgres/engine.py +COPY infrastructure/postgres/task_table.py infrastructure/postgres/task_table.py +COPY infrastructure/postgres/subtask_table.py infrastructure/postgres/subtask_table.py COPY orchestration/ orchestration/ COPY applications/sharepoint_renamer/ applications/sharepoint_renamer/ CMD ["applications.sharepoint_renamer.handler.handler"] diff --git a/applications/sharepoint_renamer/handler/requirements.txt b/applications/sharepoint_renamer/handler/requirements.txt index 6b7cf3baa..bca62b224 100644 --- a/applications/sharepoint_renamer/handler/requirements.txt +++ b/applications/sharepoint_renamer/handler/requirements.txt @@ -1,3 +1,10 @@ msal requests pydantic-settings==2.6.0 + +# The app-owned-task lane: the handler creates its own task + sub_task through +# TaskOrchestrator's Postgres repositories. psycopg2 is the driver +# PostgresConfig defaults to (tests use a different one). +sqlalchemy==2.0.36 +sqlmodel +psycopg2-binary==2.9.10 diff --git a/deployment/terraform/lambda/sharepoint_renamer/main.tf b/deployment/terraform/lambda/sharepoint_renamer/main.tf index 0c2450611..bbfe7c82a 100644 --- a/deployment/terraform/lambda/sharepoint_renamer/main.tf +++ b/deployment/terraform/lambda/sharepoint_renamer/main.tf @@ -1,3 +1,11 @@ +data "aws_secretsmanager_secret_version" "db_credentials" { + secret_id = "${var.stage}/assessment_model/db_credentials" +} + +locals { + db_credentials = jsondecode(data.aws_secretsmanager_secret_version.db_credentials.secret_string) +} + module "lambda" { source = "../../modules/lambda_with_sqs" @@ -18,5 +26,13 @@ module "lambda" { SHAREPOINT_CLIENT_SECRET = var.sharepoint_client_secret SHAREPOINT_TENANT_ID = var.sharepoint_tenant_id SOCIAL_HOUSING_WAVE_3_SHAREPOINT_ID = var.social_housing_wave_3_sharepoint_id + + # The run creates its own task + sub_task so the admin portal can see it. + # No VPC needed — the same public path every other task-lane Lambda uses. + POSTGRES_USERNAME = local.db_credentials.db_assessment_model_username + POSTGRES_PASSWORD = local.db_credentials.db_assessment_model_password + POSTGRES_HOST = var.db_host + POSTGRES_DATABASE = var.db_name + POSTGRES_PORT = var.db_port } } diff --git a/deployment/terraform/lambda/sharepoint_renamer/variables.tf b/deployment/terraform/lambda/sharepoint_renamer/variables.tf index 79b1a8d4b..3e17d2a36 100644 --- a/deployment/terraform/lambda/sharepoint_renamer/variables.tf +++ b/deployment/terraform/lambda/sharepoint_renamer/variables.tf @@ -27,7 +27,11 @@ variable "timeout" { variable "reserved_concurrent_executions" { type = number default = 1 - description = "Prevent parallel renames causing race conditions on SharePoint." + description = <<-EOT + Prevent parallel renames causing race conditions on SharePoint. Since the + Renamer joined the app-owned-task lane this also caps its DB connections at + one — a second lever hanging off the same number. + EOT } variable "batch_size" { @@ -55,6 +59,21 @@ variable "social_housing_wave_3_sharepoint_id" { sensitive = true } +variable "db_host" { + type = string + sensitive = true +} + +variable "db_name" { + type = string + sensitive = true +} + +variable "db_port" { + type = string + sensitive = true +} + locals { image_uri = "${var.ecr_repo_url}@${var.image_digest}" } From bbb3c9e0571b91b802b72cacec5d4b0fc6e55a6b Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:19:51 +0000 Subject: [PATCH 18/25] =?UTF-8?q?Verify=20a=20local=20rename=20run=20the?= =?UTF-8?q?=20way=20the=20queue=20delivers=20it=20=F0=9F=9F=AA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local invoke now sends an SQS-shaped payload — a direct invoke is the one shape that cannot reproduce the dry_run bug class. Test modules route protected-method calls through typed helpers so both pass pyright strict. Co-Authored-By: Claude Opus 5 (1M context) --- .../local_handler/invoke_local_lambda.py | 26 ++++- .../sharepoint_renamer_orchestrator.py | 4 +- .../sharepoint_renamer/test_handler.py | 18 +-- tests/scripts/test_rename_sharepoint_files.py | 107 +++++++++++------- 4 files changed, 105 insertions(+), 50 deletions(-) diff --git a/applications/sharepoint_renamer/local_handler/invoke_local_lambda.py b/applications/sharepoint_renamer/local_handler/invoke_local_lambda.py index 073808eb4..90eb91c55 100644 --- a/applications/sharepoint_renamer/local_handler/invoke_local_lambda.py +++ b/applications/sharepoint_renamer/local_handler/invoke_local_lambda.py @@ -1,4 +1,19 @@ #!/usr/bin/env python3 +"""Invoke the locally-running Renamer image with a realistic SQS payload. + +The event is hand-built SQS-shaped on purpose. A direct invoke (a bare body, +no ``Records``) is the one path where the request is read the same way whether +or not the record body is unwrapped — so it is the one path that would NOT have +caught ``dry_run`` being ignored on queue-triggered runs. Exercise the shape +production actually delivers. + +This writes a real task + sub_task row to whatever database ``.env`` points at +(dev), and is the only place the production Postgres driver is exercised — the +tests use a different one. ``dry_run`` is True so nothing is renamed for real. +""" + +import json + import requests HOST = "localhost" @@ -6,7 +21,16 @@ PORT = "9003" LAMBDA_URL = f"http://{HOST}:{PORT}/2015-03-31/functions/function/invocations" -payload = {"dry_run": True} +payload = { + "Records": [ + { + "messageId": "local-test-1", + "body": json.dumps( + {"sharepoint_site": "SOCIAL_HOUSING_WAVE_3", "dry_run": True} + ), + } + ] +} response = requests.post(LAMBDA_URL, json=payload) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index f35b14912..05ee06952 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -38,8 +38,8 @@ class RenameSummary: would_rename: int = 0 skipped_already_canonical: int = 0 skipped_images: int = 0 - missing_folders: list[str] = field(default_factory=list) - failed: list[RenameFailure] = field(default_factory=list) + missing_folders: list[str] = field(default_factory=list[str]) + failed: list[RenameFailure] = field(default_factory=list[RenameFailure]) def merge_in(self, other: "RenameSummary") -> None: self.renamed += other.renamed diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index d30a388d5..afe07a3e9 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -14,7 +14,7 @@ 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 typing import Any, Callable, cast from uuid import UUID import pytest @@ -32,6 +32,7 @@ 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 +from utils.sharepoint.domna_sites import DomnaSites SITE = "SOCIAL_HOUSING_WAVE_3" UPRN = "100" @@ -84,8 +85,8 @@ class FakeSharepoint: 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) + reject: dict[str, Exception] = field(default_factory=dict[str, Exception]) + renamed: list[tuple[str, str]] = field(default_factory=list[tuple[str, str]]) def get_folders_in_path(self, path: str) -> dict[str, Any]: if path not in self.contents: @@ -128,9 +129,11 @@ def _install( import applications.sharepoint_renamer.handler as module monkeypatch.setenv(f"{SITE}_SHAREPOINT_ID", "test-site-id") - monkeypatch.setattr( - module, "DomnaSharepointClient", lambda _site: sharepoint - ) + + def client_for(_site: DomnaSites) -> FakeSharepoint: + return sharepoint + + monkeypatch.setattr(module, "DomnaSharepointClient", client_for) csv_file = tmp_path / "addresses.csv" csv_file.write_text( @@ -144,11 +147,12 @@ 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 + undecorated = cast(Callable[..., Any], getattr(module.handler, "__wrapped__")) return task_handler( task_source="sharepoint_renamer", source=Source.SHAREPOINT_SITE, orchestrator_cm=harness.factory, - )(module.handler.__wrapped__) + )(undecorated) def _sqs_event(body: dict[str, Any]) -> dict[str, Any]: diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index c3dc9cd03..c5a831029 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -1,5 +1,5 @@ from pathlib import Path -from typing import Any +from typing import Any, Optional from unittest.mock import MagicMock import pytest @@ -37,32 +37,57 @@ def _make_folder(name: str) -> dict[str, Any]: return {"name": name, "folder": {}} -def _raise(error: Exception) -> None: - raise error - - def _make_package(name: str) -> dict[str, Any]: return {"name": name, "package": {}} -def _make_orchestrator(sp: MagicMock, dry_run: bool = False) -> SharepointRenamerOrchestrator: +def _make_orchestrator( + sp: MagicMock, dry_run: bool = False +) -> SharepointRenamerOrchestrator: orchestrator = SharepointRenamerOrchestrator.__new__(SharepointRenamerOrchestrator) - orchestrator._sp_client = sp - orchestrator._dry_run = dry_run + orchestrator._sp_client = sp # pyright: ignore[reportPrivateUsage] + orchestrator._dry_run = dry_run # pyright: ignore[reportPrivateUsage] return orchestrator +# The folder walk and root discovery are protected, but they are the unit these +# tests pin: one file can be walked without driving a whole CSV through run(). +# Routed through these two helpers so the access is declared once here rather +# than at every call site. +def _process_folder( + orchestrator: SharepointRenamerOrchestrator, + folder_path: str, + uprn: str, + address: str, + postcode: str, +) -> Optional[RenameSummary]: + return orchestrator._process_folder( # pyright: ignore[reportPrivateUsage] + folder_path, uprn, address, postcode + ) + + +def _discover_roots(orchestrator: SharepointRenamerOrchestrator) -> list[str]: + return orchestrator._discover_roots() # pyright: ignore[reportPrivateUsage] + + # --------------------------------------------------------------------------- # build_canonical_filename # --------------------------------------------------------------------------- def test_already_canonical_returns_none() -> None: - assert build_canonical_filename("100", "1 High St", "AB1 2CD", "100_High St AB1 2CD_Report.pdf") is None + assert ( + build_canonical_filename( + "100", "1 High St", "AB1 2CD", "100_High St AB1 2CD_Report.pdf" + ) + is None + ) def test_strips_address_prefix_and_adds_uprn() -> None: - result = build_canonical_filename("100", "1 High St", "AB1 2CD", "1 High St AB1 2CD - Survey.pdf") + result = build_canonical_filename( + "100", "1 High St", "AB1 2CD", "1 High St AB1 2CD - Survey.pdf" + ) assert result == "100_1 High St AB1 2CD_Survey.pdf" @@ -89,7 +114,7 @@ def test_discover_roots_includes_sero_batch_folders() -> None: } # Act - roots = _make_orchestrator(sp)._discover_roots() + roots = _discover_roots(_make_orchestrator(sp)) # Assert assert roots == [ @@ -199,7 +224,7 @@ def test_renames_top_level_files(caplog: pytest.LogCaptureFixture) -> None: ] } - _make_orchestrator(sp)._process_folder("some/path", "100", "1 High St", "AB1 2CD") + _process_folder(_make_orchestrator(sp), "some/path", "100", "1 High St", "AB1 2CD") assert sp.rename_file.call_count == 2 sp.rename_file.assert_any_call("id-1", "100_1 High St AB1 2CD_Survey.pdf") @@ -226,11 +251,12 @@ def test_recurses_into_subfolders_and_renames_all_files() -> None: ] } - sp.get_folders_in_path.side_effect = lambda path: ( - root_contents if path == "base/path" else suba_contents - ) + def fake_get(path: str) -> dict[str, Any]: + return root_contents if path == "base/path" else suba_contents - _make_orchestrator(sp)._process_folder("base/path", "200", "2 Main Rd", "XY9 8ZW") + sp.get_folders_in_path.side_effect = fake_get + + _process_folder(_make_orchestrator(sp), "base/path", "200", "2 Main Rd", "XY9 8ZW") assert sp.rename_file.call_count == 2 sp.rename_file.assert_any_call("root-file", "200_2 Main Rd XY9 8ZW_Root.pdf") @@ -253,11 +279,14 @@ def test_subfolder_results_merge_into_the_parent_summary() -> None: ] }, } - sp.get_folders_in_path.side_effect = lambda path: contents[path] + def fake_get(path: str) -> dict[str, Any]: + return contents[path] + + sp.get_folders_in_path.side_effect = fake_get # Act - summary = _make_orchestrator(sp)._process_folder( - "base/path", "200", "2 Main Rd", "XY9 8ZW" + summary = _process_folder( + _make_orchestrator(sp), "base/path", "200", "2 Main Rd", "XY9 8ZW" ) # Assert @@ -273,7 +302,7 @@ def test_ignores_package_items() -> None: sp = MagicMock() sp.get_folders_in_path.return_value = {"value": [_make_package("Notebook")]} - _make_orchestrator(sp)._process_folder("some/path", "300", "3 Oak Ave", "ZZ1 1ZZ") + _process_folder(_make_orchestrator(sp), "some/path", "300", "3 Oak Ave", "ZZ1 1ZZ") sp.rename_file.assert_not_called() assert sp.get_folders_in_path.call_count == 1 @@ -292,8 +321,8 @@ def test_missing_folder_returns_nothing_without_warning( sp.get_folders_in_path.side_effect = ValueError("not found") # Act - found = _make_orchestrator(sp)._process_folder( - "missing/path", "400", "4 Elm St", "AA2 2BB" + found = _process_folder( + _make_orchestrator(sp), "missing/path", "400", "4 Elm St", "AA2 2BB" ) # Assert @@ -364,13 +393,15 @@ def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename() -> sp.get_folders_in_path.return_value = { "value": [_make_file("Locked.pdf", "id-1"), _make_file("Survey.pdf", "id-2")] } - sp.rename_file.side_effect = lambda item_id, _new_name: ( - _raise(PermissionError("locked by another user")) if item_id == "id-1" else None - ) + def fake_rename(item_id: str, _new_name: str) -> None: + if item_id == "id-1": + raise PermissionError("locked by another user") + + sp.rename_file.side_effect = fake_rename # Act - summary = _make_orchestrator(sp)._process_folder( - "some/path", "100", "1 High St", "AB1 2CD" + summary = _process_folder( + _make_orchestrator(sp), "some/path", "100", "1 High St", "AB1 2CD" ) # Assert @@ -393,7 +424,7 @@ def test_skips_already_canonical_files() -> None: "value": [_make_file("500_Pine Ln BB3 3CC_Doc.pdf", "id-y")] } - _make_orchestrator(sp)._process_folder("some/path", "500", "5 Pine Ln", "BB3 3CC") + _process_folder(_make_orchestrator(sp), "some/path", "500", "5 Pine Ln", "BB3 3CC") sp.rename_file.assert_not_called() @@ -410,14 +441,12 @@ def test_summary_counts_already_canonical_and_image_skips_apart() -> None: } # Act - summary = _make_orchestrator(sp)._process_folder( - "some/path", "500", "5 Pine Ln", "BB3 3CC" + summary = _process_folder( + _make_orchestrator(sp), "some/path", "500", "5 Pine Ln", "BB3 3CC" ) # Assert - assert summary == RenameSummary( - skipped_already_canonical=1, skipped_images=2 - ) + assert summary == RenameSummary(skipped_already_canonical=1, skipped_images=2) # --------------------------------------------------------------------------- @@ -429,12 +458,10 @@ def test_dry_run_logs_would_rename_without_calling_api( caplog: pytest.LogCaptureFixture, ) -> None: sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [_make_file("Survey.pdf", "id-1")] - } + sp.get_folders_in_path.return_value = {"value": [_make_file("Survey.pdf", "id-1")]} - _make_orchestrator(sp, dry_run=True)._process_folder( - "some/path", "100", "1 High St", "AB1 2CD" + _process_folder( + _make_orchestrator(sp, dry_run=True), "some/path", "100", "1 High St", "AB1 2CD" ) sp.rename_file.assert_not_called() @@ -449,8 +476,8 @@ def test_dry_run_counts_intents_separately_from_renames() -> None: } # Act - summary = _make_orchestrator(sp, dry_run=True)._process_folder( - "some/path", "100", "1 High St", "AB1 2CD" + summary = _process_folder( + _make_orchestrator(sp, dry_run=True), "some/path", "100", "1 High St", "AB1 2CD" ) # Assert From bf1f5693be7bbdaa8c0233628ea81712ef66d733 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:21:11 +0000 Subject: [PATCH 19/25] =?UTF-8?q?Build=20the=20handler=20test's=20address?= =?UTF-8?q?=20list=20without=20a=20shared=20mutable=20default=20?= =?UTF-8?q?=F0=9F=9F=AA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 (1M context) --- tests/applications/sharepoint_renamer/test_handler.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index afe07a3e9..e385302de 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -14,7 +14,7 @@ from collections.abc import Generator, Iterator from contextlib import contextmanager from dataclasses import dataclass, field from pathlib import Path -from typing import Any, Callable, cast +from typing import Any, Callable, Optional, cast from uuid import UUID import pytest @@ -122,12 +122,14 @@ def _install( tmp_path: Path, sharepoint: FakeSharepoint, *, - rows: list[tuple[str, str, str]] = [(UPRN, ADDRESS, POSTCODE)], + rows: Optional[list[tuple[str, str, str]]] = None, ) -> 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 + rows = rows if rows is not None else [(UPRN, ADDRESS, POSTCODE)] + monkeypatch.setenv(f"{SITE}_SHAREPOINT_ID", "test-site-id") def client_for(_site: DomnaSites) -> FakeSharepoint: From 737d07a951805b29b72008c99efb31fd897d7a25 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:47:20 +0000 Subject: [PATCH 20/25] =?UTF-8?q?Plan=20each=20property's=20renames=20befo?= =?UTF-8?q?re=20applying=20any=20of=20them=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) --- .../sharepoint_renamer_orchestrator.py | 53 +++++++++++++++++++ tests/scripts/test_rename_sharepoint_files.py | 51 ++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index 05ee06952..d1507c890 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -1,5 +1,6 @@ import csv import os +from collections.abc import Iterator from dataclasses import dataclass, field from typing import Optional @@ -27,6 +28,41 @@ class RenameFailure: error: str +@dataclass +class RenameIntent: + """One file the walk found needing a rename, and what to rename it to.""" + + uprn: str + item_id: str + original_name: str + new_name: str + + +@dataclass +class PropertyPlan: + """What one address-list row needs, found by walking its folder tree. + + Yielded for *every* row, including rows with nothing to do — a plan with no + intents still carries the skips it observed, so run-level totals stay + complete even though only rows with intents are worth a sub_task. + + The plan is what makes "when is this property finished?" answerable: its + size is known before the first rename is attempted.""" + + uprn: str + address: str + postcode: str + intents: list[RenameIntent] = field(default_factory=list[RenameIntent]) + skipped_already_canonical: int = 0 + skipped_images: int = 0 + missing_folder: bool = False + + def merge_in(self, other: "PropertyPlan") -> None: + self.intents.extend(other.intents) + self.skipped_already_canonical += other.skipped_already_canonical + self.skipped_images += other.skipped_images + + @dataclass class RenameSummary: """What a run did, threaded back up through the traversal. @@ -110,6 +146,23 @@ class SharepointRenamerOrchestrator: ) return [BASE_PATH, *batch_roots] + def plan(self) -> Iterator[PropertyPlan]: + """Walk the address list, yielding what each property needs. + + Lazy on purpose: the caller renames one property before the next is + walked, so a run that is killed part-way still leaves everything it + reached already renamed — and, on the task lane, a completed sub_task + recording it.""" + raise NotImplementedError + + def rename(self, plan: PropertyPlan) -> RenameSummary: + """Apply one property's plan, returning what actually happened. + + Never raises for a file SharePoint rejects: the rejection is recorded + in the summary so one locked file cannot fail the property, let alone + the run.""" + raise NotImplementedError + def run(self) -> RenameSummary: summary = RenameSummary() roots = self._discover_roots() diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index c5a831029..98129bd88 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -8,7 +8,9 @@ from domain.pashub_fetcher.sharepoint_subfolders import SharepointSubfolders from orchestration.sharepoint_renamer_orchestrator import ( ASSESSMENT_SUBFOLDER, BASE_PATH, + PropertyPlan, RenameFailure, + RenameIntent, RenameSummary, SharepointRenamerOrchestrator, build_canonical_filename, @@ -155,6 +157,55 @@ def test_run_renames_files_for_property_under_batch_root(tmp_path: Path) -> None sp.rename_file.assert_called_once_with("id-1", "100_1 High St AB1 2CD_Survey.pdf") +def test_plan_lists_each_file_a_property_needs_renamed(tmp_path: Path) -> None: + # Arrange + csv_path = _write_csv(tmp_path, [("100", "1 High St", "AB1 2CD")]) + assessment = _assessment_path(BASE_PATH, "1 High St", "AB1 2CD") + + sp = MagicMock() + + def fake_get(path: str) -> dict[str, Any]: + if path == BASE_PATH: + return {"value": []} + if path == assessment: + return { + "value": [ + _make_file("Survey.pdf", "id-1"), + _make_file("Report.docx", "id-2"), + ] + } + raise ValueError("not found") + + sp.get_folders_in_path.side_effect = fake_get + + # Act + plans = list(SharepointRenamerOrchestrator(sp, csv_path).plan()) + + # Assert + assert plans == [ + PropertyPlan( + uprn="100", + address="1 High St", + postcode="AB1 2CD", + intents=[ + RenameIntent( + uprn="100", + item_id="id-1", + original_name="Survey.pdf", + new_name="100_1 High St AB1 2CD_Survey.pdf", + ), + RenameIntent( + uprn="100", + item_id="id-2", + original_name="Report.docx", + new_name="100_1 High St AB1 2CD_Report.docx", + ), + ], + ) + ] + sp.rename_file.assert_not_called() + + def test_run_summary_counts_each_renamed_file(tmp_path: Path) -> None: # Arrange csv_path = _write_csv(tmp_path, [("100", "1 High St", "AB1 2CD")]) From cc07cc0114fc5e4bdd3c38fe3713002d86dc7479 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:50:28 +0000 Subject: [PATCH 21/25] =?UTF-8?q?Plan=20each=20property's=20renames=20befo?= =?UTF-8?q?re=20applying=20any=20of=20them=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk now yields a PropertyPlan per address-list row, and rename() applies one. run() composes the two, so single-pass callers are unchanged. Knowing a property's size before the first rename is what makes "when is this property finished?" answerable. Tests that reached through the old private _process_folder are rewritten against the public run(), so they survive this kind of change. Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer_orchestrator.py | 135 ++++---- tests/scripts/test_rename_sharepoint_files.py | 300 ++++++++---------- 2 files changed, 212 insertions(+), 223 deletions(-) diff --git a/orchestration/sharepoint_renamer_orchestrator.py b/orchestration/sharepoint_renamer_orchestrator.py index d1507c890..c1bf76d7e 100644 --- a/orchestration/sharepoint_renamer_orchestrator.py +++ b/orchestration/sharepoint_renamer_orchestrator.py @@ -153,18 +153,6 @@ class SharepointRenamerOrchestrator: walked, so a run that is killed part-way still leaves everything it reached already renamed — and, on the task lane, a completed sub_task recording it.""" - raise NotImplementedError - - def rename(self, plan: PropertyPlan) -> RenameSummary: - """Apply one property's plan, returning what actually happened. - - Never raises for a file SharePoint rejects: the rejection is recorded - in the summary so one locked file cannot fail the property, let alone - the run.""" - raise NotImplementedError - - def run(self) -> RenameSummary: - summary = RenameSummary() roots = self._discover_roots() with open(self._csv_path, newline="", encoding="utf-8-sig") as f: reader = csv.DictReader(f) @@ -175,19 +163,65 @@ class SharepointRenamerOrchestrator: ) for row in reader: - summary.merge_in( - self._process_row( - roots, - uprn=row["UPRN"].strip(), - address=row["Address"].strip(), - postcode=row["Postcode"].strip(), + yield self._plan_row( + roots, + uprn=row["UPRN"].strip(), + address=row["Address"].strip(), + postcode=row["Postcode"].strip(), + ) + + def rename(self, plan: PropertyPlan) -> RenameSummary: + """Apply one property's plan, returning what actually happened. + + Never raises for a file SharePoint rejects: the rejection is recorded + in the summary so one locked file cannot fail the property, let alone + the run.""" + summary = RenameSummary( + skipped_already_canonical=plan.skipped_already_canonical, + skipped_images=plan.skipped_images, + missing_folders=[plan.uprn] if plan.missing_folder else [], + ) + for intent in plan.intents: + if self._dry_run: + summary.would_rename += 1 + logger.info( + f'Would rename: "{intent.original_name}" → ' + f'"{intent.new_name}" (UPRN: {intent.uprn})' + ) + continue + try: + self._sp_client.rename_file(intent.item_id, intent.new_name) + summary.renamed += 1 + logger.info( + f'Renamed: "{intent.original_name}" → ' + f'"{intent.new_name}" (UPRN: {intent.uprn})' + ) + except Exception as e: + summary.failed.append( + RenameFailure( + uprn=intent.uprn, + original_name=intent.original_name, + new_name=intent.new_name, + error=str(e), ) ) + logger.error( + f'Failed to rename "{intent.original_name}" → ' + f'"{intent.new_name}" (UPRN: {intent.uprn}): {e}' + ) return summary - def _process_row( + def run(self) -> RenameSummary: + """Plan and rename every property in one pass — the entry point for + callers that want no per-property bookkeeping.""" + summary = RenameSummary() + for plan in self.plan(): + summary.merge_in(self.rename(plan)) + return summary + + def _plan_row( self, roots: list[str], uprn: str, address: str, postcode: str - ) -> RenameSummary: + ) -> PropertyPlan: # Batch folders name properties "{uprn}_{address}"; filenames must not # repeat the UPRN, so the folder lookup and the canonical name diverge. display_address = address.removeprefix(f"{uprn}_") @@ -196,67 +230,54 @@ class SharepointRenamerOrchestrator: f"{root}/{address}, {postcode}" f"/{SharepointSubfolders.ASSESSMENT.value}/{ASSESSMENT_SUBFOLDER}" ) - found = self._process_folder(folder_path, uprn, display_address, postcode) + found = self._plan_folder(folder_path, uprn, display_address, postcode) if found is not None: return found logger.warning( f"Missing folder for UPRN {uprn} in any root: {address}, {postcode}" ) - return RenameSummary(missing_folders=[uprn]) + return PropertyPlan( + uprn=uprn, + address=display_address, + postcode=postcode, + missing_folder=True, + ) - def _process_folder( + def _plan_folder( self, folder_path: str, uprn: str, address: str, postcode: str - ) -> Optional[RenameSummary]: - """What renaming this folder (and its subfolders) did, or None when the - folder does not exist under this root.""" + ) -> Optional[PropertyPlan]: + """What this folder (and its subfolders) needs renaming, or None when + the folder does not exist under this root.""" try: contents = self._sp_client.get_folders_in_path(folder_path) except ValueError: return None - summary = RenameSummary() + plan = PropertyPlan(uprn=uprn, address=address, postcode=postcode) for item in contents.get("value", []): if "folder" in item: - child = self._process_folder( + child = self._plan_folder( f"{folder_path}/{item['name']}", uprn, address, postcode ) if child is not None: - summary.merge_in(child) + plan.merge_in(child) elif "file" in item: original_name: str = item["name"] if os.path.splitext(original_name)[1].lower() in {".jpg", ".heic"}: - summary.skipped_images += 1 + plan.skipped_images += 1 continue new_name = build_canonical_filename( uprn, address, postcode, original_name ) - if new_name is None: - summary.skipped_already_canonical += 1 + plan.skipped_already_canonical += 1 continue - - if self._dry_run: - summary.would_rename += 1 - logger.info( - f'Would rename: "{original_name}" → "{new_name}" (UPRN: {uprn})' + plan.intents.append( + RenameIntent( + uprn=uprn, + item_id=item["id"], + original_name=original_name, + new_name=new_name, ) - else: - try: - self._sp_client.rename_file(item["id"], new_name) - summary.renamed += 1 - logger.info( - f'Renamed: "{original_name}" → "{new_name}" (UPRN: {uprn})' - ) - except Exception as e: - summary.failed.append( - RenameFailure( - uprn=uprn, - original_name=original_name, - new_name=new_name, - error=str(e), - ) - ) - logger.error( - f'Failed to rename "{original_name}" → "{new_name}" (UPRN: {uprn}): {e}' - ) - return summary + ) + return plan diff --git a/tests/scripts/test_rename_sharepoint_files.py b/tests/scripts/test_rename_sharepoint_files.py index 98129bd88..6fd5f35d7 100644 --- a/tests/scripts/test_rename_sharepoint_files.py +++ b/tests/scripts/test_rename_sharepoint_files.py @@ -1,5 +1,5 @@ from pathlib import Path -from typing import Any, Optional +from typing import Any from unittest.mock import MagicMock import pytest @@ -52,26 +52,42 @@ def _make_orchestrator( return orchestrator -# The folder walk and root discovery are protected, but they are the unit these -# tests pin: one file can be walked without driving a whole CSV through run(). -# Routed through these two helpers so the access is declared once here rather -# than at every call site. -def _process_folder( - orchestrator: SharepointRenamerOrchestrator, - folder_path: str, - uprn: str, - address: str, - postcode: str, -) -> Optional[RenameSummary]: - return orchestrator._process_folder( # pyright: ignore[reportPrivateUsage] - folder_path, uprn, address, postcode - ) - - def _discover_roots(orchestrator: SharepointRenamerOrchestrator) -> list[str]: + # Root discovery is protected but is its own unit; the access is declared + # once here rather than at the call site. return orchestrator._discover_roots() # pyright: ignore[reportPrivateUsage] +def _one_property_site( + tmp_path: Path, + folders: dict[str, list[dict[str, Any]]], + uprn: str = "100", + address: str = "1 High St", + postcode: str = "AB1 2CD", +) -> tuple[MagicMock, str]: + """A SharePoint site holding exactly one property, plus its address-list CSV. + + ``folders`` maps a path relative to the property's assessment folder ("" for + the folder itself, "SubA" for a subfolder) to its Graph listing. Anything + else raises ValueError, as the real client does for a folder that is not + there.""" + assessment = _assessment_path(BASE_PATH, address, postcode) + listings: dict[str, dict[str, Any]] = {BASE_PATH: {"value": []}} + for relative, items in folders.items(): + path = assessment if relative == "" else f"{assessment}/{relative}" + listings[path] = {"value": items} + + sp = MagicMock() + + def fake_get(path: str) -> dict[str, Any]: + if path not in listings: + raise ValueError(f"not found: {path}") + return listings[path] + + sp.get_folders_in_path.side_effect = fake_get + return sp, _write_csv(tmp_path, [(uprn, address, postcode)]) + + # --------------------------------------------------------------------------- # build_canonical_filename # --------------------------------------------------------------------------- @@ -262,123 +278,101 @@ def test_run_uprn_prefixed_address_yields_single_uprn_filename( # --------------------------------------------------------------------------- -# _process_folder — files only at root level +# Walking one property's folder tree — driven through the public run() # --------------------------------------------------------------------------- -def test_renames_top_level_files(caplog: pytest.LogCaptureFixture) -> None: - sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [ - _make_file("Survey.pdf", "id-1"), - _make_file("Report.docx", "id-2"), - ] - } +def test_renames_every_file_in_a_property_folder(tmp_path: Path) -> None: + # Arrange + sp, csv_path = _one_property_site( + tmp_path, + {"": [_make_file("Survey.pdf", "id-1"), _make_file("Report.docx", "id-2")]}, + ) - _process_folder(_make_orchestrator(sp), "some/path", "100", "1 High St", "AB1 2CD") + # Act + SharepointRenamerOrchestrator(sp, csv_path).run() + # Assert assert sp.rename_file.call_count == 2 sp.rename_file.assert_any_call("id-1", "100_1 High St AB1 2CD_Survey.pdf") sp.rename_file.assert_any_call("id-2", "100_1 High St AB1 2CD_Report.docx") -# --------------------------------------------------------------------------- -# _process_folder — recursive two-level hierarchy -# --------------------------------------------------------------------------- - - -def test_recurses_into_subfolders_and_renames_all_files() -> None: - sp = MagicMock() - - root_contents: dict[str, Any] = { - "value": [ - _make_file("Root.pdf", "root-file"), - _make_folder("SubA"), - ] - } - suba_contents: dict[str, Any] = { - "value": [ - _make_file("Sub.pdf", "sub-file"), - ] - } - - def fake_get(path: str) -> dict[str, Any]: - return root_contents if path == "base/path" else suba_contents - - sp.get_folders_in_path.side_effect = fake_get - - _process_folder(_make_orchestrator(sp), "base/path", "200", "2 Main Rd", "XY9 8ZW") - - assert sp.rename_file.call_count == 2 - sp.rename_file.assert_any_call("root-file", "200_2 Main Rd XY9 8ZW_Root.pdf") - sp.rename_file.assert_any_call("sub-file", "200_2 Main Rd XY9 8ZW_Sub.pdf") - - sp.get_folders_in_path.assert_any_call("base/path/SubA") - - -def test_subfolder_results_merge_into_the_parent_summary() -> None: +def test_recurses_into_subfolders_and_renames_all_files(tmp_path: Path) -> None: # Arrange - sp = MagicMock() - contents: dict[str, dict[str, Any]] = { - "base/path": { - "value": [_make_file("Root.pdf", "root-file"), _make_folder("SubA")] + sp, csv_path = _one_property_site( + tmp_path, + { + "": [_make_file("Root.pdf", "root-file"), _make_folder("SubA")], + "SubA": [_make_file("Sub.pdf", "sub-file")], }, - "base/path/SubA": { - "value": [ - _make_file("Sub.pdf", "sub-file"), - _make_file("200_2 Main Rd XY9 8ZW_Done.pdf", "canonical-file"), - ] - }, - } - def fake_get(path: str) -> dict[str, Any]: - return contents[path] - - sp.get_folders_in_path.side_effect = fake_get + ) # Act - summary = _process_folder( - _make_orchestrator(sp), "base/path", "200", "2 Main Rd", "XY9 8ZW" + SharepointRenamerOrchestrator(sp, csv_path).run() + + # Assert + assert sp.rename_file.call_count == 2 + sp.rename_file.assert_any_call("root-file", "100_1 High St AB1 2CD_Root.pdf") + sp.rename_file.assert_any_call("sub-file", "100_1 High St AB1 2CD_Sub.pdf") + + +def test_subfolder_results_merge_into_the_parent_summary(tmp_path: Path) -> None: + # Arrange + sp, csv_path = _one_property_site( + tmp_path, + { + "": [_make_file("Root.pdf", "root-file"), _make_folder("SubA")], + "SubA": [ + _make_file("Sub.pdf", "sub-file"), + _make_file("100_1 High St AB1 2CD_Done.pdf", "canonical-file"), + ], + }, ) + # Act + summary = SharepointRenamerOrchestrator(sp, csv_path).run() + # Assert assert summary == RenameSummary(renamed=2, skipped_already_canonical=1) -# --------------------------------------------------------------------------- -# _process_folder — non-file, non-folder items are skipped -# --------------------------------------------------------------------------- - - -def test_ignores_package_items() -> None: - sp = MagicMock() - sp.get_folders_in_path.return_value = {"value": [_make_package("Notebook")]} - - _process_folder(_make_orchestrator(sp), "some/path", "300", "3 Oak Ave", "ZZ1 1ZZ") - - sp.rename_file.assert_not_called() - assert sp.get_folders_in_path.call_count == 1 - - -# --------------------------------------------------------------------------- -# _process_folder — missing folder -# --------------------------------------------------------------------------- - - -def test_missing_folder_returns_nothing_without_warning( - caplog: pytest.LogCaptureFixture, -) -> None: +def test_ignores_package_items(tmp_path: Path) -> None: # Arrange - sp = MagicMock() - sp.get_folders_in_path.side_effect = ValueError("not found") + sp, csv_path = _one_property_site(tmp_path, {"": [_make_package("Notebook")]}) # Act - found = _process_folder( - _make_orchestrator(sp), "missing/path", "400", "4 Elm St", "AA2 2BB" - ) + SharepointRenamerOrchestrator(sp, csv_path).run() # Assert - assert found is None sp.rename_file.assert_not_called() + + +def test_property_found_under_a_later_root_is_not_reported_missing( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A miss on an earlier root is silent — only exhausting every root warns.""" + # Arrange + csv_path = _write_csv(tmp_path, [("100", "1 High St", "AB1 2CD")]) + batch_assessment = _assessment_path( + f"{BASE_PATH}/_Sero Batch 2", "1 High St", "AB1 2CD" + ) + sp = MagicMock() + + def fake_get(path: str) -> dict[str, Any]: + if path == BASE_PATH: + return {"value": [_make_folder("_Sero Batch 2")]} + if path == batch_assessment: + return {"value": [_make_file("Survey.pdf", "id-1")]} + raise ValueError("not found") + + sp.get_folders_in_path.side_effect = fake_get + + # Act + summary = SharepointRenamerOrchestrator(sp, csv_path).run() + + # Assert + assert summary.missing_folders == [] assert not any("Missing folder" in r.message for r in caplog.records) @@ -434,16 +428,19 @@ def test_run_summary_lists_uprns_with_no_folder_under_any_root( # --------------------------------------------------------------------------- -# _process_folder — already-canonical files are skipped +# Skips, failures and dry runs — driven through the public run() # --------------------------------------------------------------------------- -def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename() -> None: +def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename( + tmp_path: Path, +) -> None: # Arrange - sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [_make_file("Locked.pdf", "id-1"), _make_file("Survey.pdf", "id-2")] - } + sp, csv_path = _one_property_site( + tmp_path, + {"": [_make_file("Locked.pdf", "id-1"), _make_file("Survey.pdf", "id-2")]}, + ) + def fake_rename(item_id: str, _new_name: str) -> None: if item_id == "id-1": raise PermissionError("locked by another user") @@ -451,9 +448,7 @@ def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename() -> sp.rename_file.side_effect = fake_rename # Act - summary = _process_folder( - _make_orchestrator(sp), "some/path", "100", "1 High St", "AB1 2CD" - ) + summary = SharepointRenamerOrchestrator(sp, csv_path).run() # Assert assert summary == RenameSummary( @@ -469,67 +464,40 @@ def test_rejected_file_is_recorded_with_its_error_and_siblings_still_rename() -> ) -def test_skips_already_canonical_files() -> None: - sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [_make_file("500_Pine Ln BB3 3CC_Doc.pdf", "id-y")] - } - - _process_folder(_make_orchestrator(sp), "some/path", "500", "5 Pine Ln", "BB3 3CC") - - sp.rename_file.assert_not_called() - - -def test_summary_counts_already_canonical_and_image_skips_apart() -> None: +def test_summary_counts_already_canonical_and_image_skips_apart(tmp_path: Path) -> None: # Arrange - sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [ - _make_file("500_Pine Ln BB3 3CC_Doc.pdf", "id-1"), - _make_file("Front elevation.JPG", "id-2"), - _make_file("Rear elevation.heic", "id-3"), - ] - } + sp, csv_path = _one_property_site( + tmp_path, + { + "": [ + _make_file("100_1 High St AB1 2CD_Doc.pdf", "id-1"), + _make_file("Front elevation.JPG", "id-2"), + _make_file("Rear elevation.heic", "id-3"), + ] + }, + ) # Act - summary = _process_folder( - _make_orchestrator(sp), "some/path", "500", "5 Pine Ln", "BB3 3CC" - ) + summary = SharepointRenamerOrchestrator(sp, csv_path).run() # Assert assert summary == RenameSummary(skipped_already_canonical=1, skipped_images=2) - - -# --------------------------------------------------------------------------- -# _process_folder — dry_run=True logs intent but never calls rename_file -# --------------------------------------------------------------------------- - - -def test_dry_run_logs_would_rename_without_calling_api( - caplog: pytest.LogCaptureFixture, -) -> None: - sp = MagicMock() - sp.get_folders_in_path.return_value = {"value": [_make_file("Survey.pdf", "id-1")]} - - _process_folder( - _make_orchestrator(sp, dry_run=True), "some/path", "100", "1 High St", "AB1 2CD" - ) - sp.rename_file.assert_not_called() - assert any("Would rename" in r.message for r in caplog.records) -def test_dry_run_counts_intents_separately_from_renames() -> None: +def test_dry_run_logs_intent_and_counts_it_without_calling_the_api( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: # Arrange - sp = MagicMock() - sp.get_folders_in_path.return_value = { - "value": [_make_file("Survey.pdf", "id-1"), _make_file("Report.docx", "id-2")] - } + sp, csv_path = _one_property_site( + tmp_path, + {"": [_make_file("Survey.pdf", "id-1"), _make_file("Report.docx", "id-2")]}, + ) # Act - summary = _process_folder( - _make_orchestrator(sp, dry_run=True), "some/path", "100", "1 High St", "AB1 2CD" - ) + summary = SharepointRenamerOrchestrator(sp, csv_path, dry_run=True).run() # Assert assert summary == RenameSummary(renamed=0, would_rename=2) + sp.rename_file.assert_not_called() + assert any("Would rename" in r.message for r in caplog.records) From 5b1b319c2dc8ba4e9a201dc2270963b544d091f5 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:52:30 +0000 Subject: [PATCH 22/25] =?UTF-8?q?Give=20each=20property=20that=20needs=20r?= =?UTF-8?q?enaming=20its=20own=20sub=5Ftask=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Properties are planned and renamed one at a time, so only those with work get a row and a run killed by the timeout still leaves every property it reached both renamed and recorded. Co-Authored-By: Claude Opus 5 (1M context) --- applications/sharepoint_renamer/handler.py | 64 +++++++++++++++-- .../sharepoint_renamer/test_handler.py | 71 ++++++++++++++++++- 2 files changed, 130 insertions(+), 5 deletions(-) diff --git a/applications/sharepoint_renamer/handler.py b/applications/sharepoint_renamer/handler.py index d204c8f46..55507be8b 100644 --- a/applications/sharepoint_renamer/handler.py +++ b/applications/sharepoint_renamer/handler.py @@ -15,15 +15,18 @@ turn a productive run red. Missing folders likewise do not fail a run. import os from typing import Any +from uuid import UUID from applications.sharepoint_renamer.sharepoint_renamer_request import ( SharepointRenamerRequest, ) from domain.tasks.tasks import Source from orchestration.sharepoint_renamer_orchestrator import ( + PropertyPlan, RenameSummary, SharepointRenamerOrchestrator, ) +from orchestration.task_orchestrator import TaskOrchestrator from utilities.aws_lambda.task_handler import task_handler from utils.sharepoint.domna_sharepoint_client import DomnaSharepointClient from utils.sharepoint.domna_sites import resolve_site @@ -58,16 +61,69 @@ def _summary_json(summary: RenameSummary) -> dict[str, Any]: } -@task_handler(task_source="sharepoint_renamer", source=Source.SHAREPOINT_SITE) -def handler(body: dict[str, Any], context: Any) -> dict[str, Any]: +def _plan_json(plan: PropertyPlan) -> dict[str, Any]: + """A property sub_task's inputs: enough to see what was intended, and to + re-run just this property by re-sending them.""" + return { + "uprn": plan.uprn, + "address": plan.address, + "postcode": plan.postcode, + "renames": [ + { + "item_id": intent.item_id, + "original_name": intent.original_name, + "new_name": intent.new_name, + } + for intent in plan.intents + ], + } + + +@task_handler( + task_source="sharepoint_renamer", + source=Source.SHAREPOINT_SITE, + pass_task_orchestrator=True, +) +def handler( + body: dict[str, Any], context: Any, orchestrator: TaskOrchestrator, task_id: UUID +) -> dict[str, Any]: request = SharepointRenamerRequest.model_validate(body) sp_client = DomnaSharepointClient(resolve_site(request.sharepoint_site)) - summary = SharepointRenamerOrchestrator( + renamer = SharepointRenamerOrchestrator( sp_client, CSV_PATH, dry_run=request.dry_run - ).run() + ) + + summary = RenameSummary() + # Plan and rename one property at a time (the plan is a lazy generator), so + # a run killed by the timeout still leaves every property it reached both + # renamed and recorded. Only properties with something to do are worth a + # sub_task — a steady-state run over already-canonical folders creates none. + for plan in renamer.plan(): + if not plan.intents: + summary.merge_in(renamer.rename(plan)) + continue + subtask = orchestrator.create_child_subtask(task_id, inputs=_plan_json(plan)) + orchestrator.run_subtask( + subtask.id, work=lambda p=plan: _record(renamer, p, summary) + ) return _summary_json(summary) +def _record( + renamer: SharepointRenamerOrchestrator, plan: PropertyPlan, summary: RenameSummary +) -> dict[str, Any]: + """Rename one property, adding its result to the run total and returning it + for that property's own sub_task outputs. + + ``rename`` records a rejected file rather than raising, so the sub_task + completes with the failure in its outputs. Failing it instead would turn + the whole Task red — any failed child does — and one locked file must not + do that (ADR-0060).""" + result = renamer.rename(plan) + summary.merge_in(result) + return _summary_json(result) + + if __name__ == "__main__": # 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. diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index e385302de..f5136c31e 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -22,7 +22,7 @@ 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.subtasks import SubTask, SubTaskStatus from domain.tasks.tasks import Source, TaskStatus from orchestration.sharepoint_renamer_orchestrator import ( ASSESSMENT_SUBFOLDER, @@ -154,9 +154,21 @@ def _handler(harness: Harness) -> Callable[..., Any]: task_source="sharepoint_renamer", source=Source.SHAREPOINT_SITE, orchestrator_cm=harness.factory, + pass_task_orchestrator=True, )(undecorated) +def _property_subtasks(harness: Harness, result: Any) -> list[SubTask]: + """The per-property children, i.e. every sub_task except the wrapping one + the decorator creates for the run itself.""" + wrapper = _subtask_id(result) + return [ + s + for s in harness.subtasks.list_by_task(_task_id(result)) + if s.id != wrapper + ] + + def _sqs_event(body: dict[str, Any]) -> dict[str, Any]: return {"Records": [{"messageId": "msg-1", "body": json.dumps(body)}]} @@ -205,6 +217,63 @@ def test_completed_subtask_carries_the_run_summary( } +# --------------------------------------------------------------------------- +# One sub_task per property that actually needs work +# --------------------------------------------------------------------------- + + +def test_each_property_with_work_gets_its_own_subtask( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Arrange: one property with two renameable files, one with nothing to do + assessment_2 = ( + f"{BASE_PATH}/2 Low St, AB1 2CE" + f"/{SharepointSubfolders.ASSESSMENT.value}/{ASSESSMENT_SUBFOLDER}" + ) + sharepoint = FakeSharepoint( + { + BASE_PATH: {"value": []}, + ASSESSMENT_PATH: { + "value": [_file("Survey.pdf", "id-1"), _file("Report.docx", "id-2")] + }, + assessment_2: {"value": [_file("200_2 Low St AB1 2CE_Done.pdf", "id-3")]}, + } + ) + _install( + monkeypatch, + tmp_path, + sharepoint, + rows=[(UPRN, ADDRESS, POSTCODE), ("200", "2 Low St", "AB1 2CE")], + ) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": SITE}), None) + + # Assert + children = _property_subtasks(harness, result) + assert [c.inputs for c in children] == [ + { + "uprn": UPRN, + "address": ADDRESS, + "postcode": POSTCODE, + "renames": [ + { + "item_id": "id-1", + "original_name": "Survey.pdf", + "new_name": f"{UPRN}_{ADDRESS} {POSTCODE}_Survey.pdf", + }, + { + "item_id": "id-2", + "original_name": "Report.docx", + "new_name": f"{UPRN}_{ADDRESS} {POSTCODE}_Report.docx", + }, + ], + } + ] + assert children[0].status is SubTaskStatus.COMPLETE + assert (children[0].outputs or {})["result"]["renamed"] == 2 + + # --------------------------------------------------------------------------- # A run is attributable: its request, and the site it targeted # --------------------------------------------------------------------------- From 35a3ebe0e34d48928770282a43b1e1189e1afd0c Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 09:53:58 +0000 Subject: [PATCH 23/25] =?UTF-8?q?Complete=20a=20property=20whose=20file=20?= =?UTF-8?q?SharePoint=20rejected=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Green on arrival — rename() records rejections rather than raising. Pinned after verifying it bites: letting the rejection escape fails the child sub_task, which turns the whole Task red. Co-Authored-By: Claude Opus 5 (1M context) --- .../sharepoint_renamer/test_handler.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/tests/applications/sharepoint_renamer/test_handler.py b/tests/applications/sharepoint_renamer/test_handler.py index f5136c31e..08a5d4144 100644 --- a/tests/applications/sharepoint_renamer/test_handler.py +++ b/tests/applications/sharepoint_renamer/test_handler.py @@ -163,9 +163,7 @@ def _property_subtasks(harness: Harness, result: Any) -> list[SubTask]: the decorator creates for the run itself.""" wrapper = _subtask_id(result) return [ - s - for s in harness.subtasks.list_by_task(_task_id(result)) - if s.id != wrapper + s for s in harness.subtasks.list_by_task(_task_id(result)) if s.id != wrapper ] @@ -349,9 +347,7 @@ def test_run_completes_with_rejected_files_and_missing_folders_listed( productive run red — the failures are the record (ADR-0060).""" # Arrange sharepoint = FakeSharepoint( - _one_property_site( - _file("Locked.pdf", "id-1"), _file("Survey.pdf", "id-2") - ), + _one_property_site(_file("Locked.pdf", "id-1"), _file("Survey.pdf", "id-2")), reject={"id-1": PermissionError("locked by another user")}, ) _install( @@ -387,6 +383,35 @@ def test_run_completes_with_rejected_files_and_missing_folders_listed( } +def test_property_whose_file_was_rejected_completes_rather_than_failing( + harness: Harness, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed child would turn the whole Task red — any failed sub_task does. + A locked file must be recorded on a *completed* property instead.""" + # Arrange + sharepoint = FakeSharepoint( + _one_property_site(_file("Locked.pdf", "id-1"), _file("Survey.pdf", "id-2")), + reject={"id-1": PermissionError("locked by another user")}, + ) + _install(monkeypatch, tmp_path, sharepoint) + + # Act + result = _handler(harness)(_sqs_event({"sharepoint_site": SITE}), None) + + # Assert + child = _property_subtasks(harness, result)[0] + assert child.status is SubTaskStatus.COMPLETE + assert (child.outputs or {})["result"]["failed"] == [ + { + "uprn": UPRN, + "original_name": "Locked.pdf", + "new_name": f"{UPRN}_{ADDRESS} {POSTCODE}_Locked.pdf", + "error": "locked by another user", + } + ] + assert harness.tasks.get(_task_id(result)).status is TaskStatus.COMPLETE + + # --------------------------------------------------------------------------- # An unusable site is rejected before anything is touched # --------------------------------------------------------------------------- @@ -426,5 +451,7 @@ def test_unrecognised_site_fails_the_run( # Assert subtask = harness.subtasks.get(_subtask_id(result)) assert subtask.status is SubTaskStatus.FAILED - assert "Unrecognised SharePoint site 'NOT_A_SITE'" in (subtask.outputs or {})["error"] + assert ( + "Unrecognised SharePoint site 'NOT_A_SITE'" in (subtask.outputs or {})["error"] + ) assert sharepoint.renamed == [] From 3e6464fb159b05b4852dbbe310a11c1f1a541da0 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 10:05:29 +0000 Subject: [PATCH 24/25] =?UTF-8?q?Bind=20a=20property's=20rename=20callback?= =?UTF-8?q?=20without=20a=20default-argument=20trick=20=F0=9F=9F=AA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit partial binds plan eagerly and without introducing a parameter the caller never passes, so there is no late-binding question to reason about. Co-Authored-By: Claude Opus 5 (1M context) --- applications/sharepoint_renamer/handler.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/applications/sharepoint_renamer/handler.py b/applications/sharepoint_renamer/handler.py index 55507be8b..02f58859f 100644 --- a/applications/sharepoint_renamer/handler.py +++ b/applications/sharepoint_renamer/handler.py @@ -14,6 +14,7 @@ turn a productive run red. Missing folders likewise do not fail a run. """ import os +from functools import partial from typing import Any from uuid import UUID @@ -104,7 +105,7 @@ def handler( continue subtask = orchestrator.create_child_subtask(task_id, inputs=_plan_json(plan)) orchestrator.run_subtask( - subtask.id, work=lambda p=plan: _record(renamer, p, summary) + subtask.id, work=partial(_record, renamer, plan, summary) ) return _summary_json(summary) From 292bd7e17dde54181f7fb94e3248a1a68a8d34f6 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 29 Jul 2026 10:09:39 +0000 Subject: [PATCH 25/25] Restore the Abri ops guide to main An unrelated in-progress edit that was already in the working tree when this branch's work started, swept in by a bare `git add -A`. It belongs to its author, not to this PR. Co-Authored-By: Claude Opus 5 (1M context) --- docs/abri-hubspot-ops-guide.md | 173 +++++++++------------------------ 1 file changed, 48 insertions(+), 125 deletions(-) diff --git a/docs/abri-hubspot-ops-guide.md b/docs/abri-hubspot-ops-guide.md index 97c6cfed0..ae5db8922 100644 --- a/docs/abri-hubspot-ops-guide.md +++ b/docs/abri-hubspot-ops-guide.md @@ -9,24 +9,6 @@ field changes are picked up automatically and sent to Abri's scheduling system --- -## At a glance: which fields trigger what - -| You change… | Abri's system… | -|---|---| -| **Expected commencement date** — set for the first time | Sends us the tenancy details, so tenant contacts appear on the deal | -| **Confirmed survey date** + **Third-party surveyor identifier** — the moment *both* are filled in | Creates the survey booking (a "job") | -| **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor identifier** — changed on a deal that already has a booking | Updates the existing appointment | -| **Number of attempts** reaches 3 *and* **Outcome** is one of the four unsuccessful values | Cancels the booking as abandoned | - -Three fields are inputs to those triggers rather than triggers themselves: - -- **Deal owner** — a HubSpot workflow maps it to the surveyor's Abri resource ID and - stores that in **Third-party surveyor identifier**, so setting the owner is how you set - the surveyor. -- **Client booking reference** — written back **by the system** with Abri's job number - once the booking is created. Never edit it (see [watch-outs](#things-to-watch-out-for)). -- **Deal name** — used as the job description Abri's staff see. - ## Which deals does this apply to? Only deals associated with the **Abri Stock Condition — Privately Funded** project. @@ -34,86 +16,22 @@ Changes to any other deal are ignored by this integration. ## How fast does it happen? -Changes are picked up automatically shortly after you save them in HubSpot — the workflow runs every 20 minutes. Nothing fires while you are mid-edit; it's the saved change that +Changes are picked up automatically shortly after you save them in HubSpot — usually +within a few minutes. Nothing fires while you are mid-edit; it's the saved change that counts. --- -## The detail: what each trigger does +## What your changes do in Abri's system -### Fetching tenant details +| When you… | Abri's system… | How you know it worked | +|---|---|---| +| Set **Expected commencement date** for the first time | Sends us the tenancy details for the property | Tenant contact records appear in HubSpot, linked to the deal — unless Abri reports the property as empty (see below) | +| Set **Confirmed survey date** for the first time | Creates the survey booking (a "job") against the property, assigned to the surveyor on the deal | Abri's job number appears in **Client booking reference** on the deal | +| Change **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor identifier** on a deal that already has a booking | Updates the existing appointment — new date/time and/or reassigns it to the new surveyor | The deal keeps the same Client booking reference | +| Record a **3rd attempt** (Number of attempts reaches 3) **and** set **Outcome** to an unsuccessful value (see below) | Cancels the booking as abandoned | — (this only fires once per deal) | -Setting **Expected commencement date** for the first time asks Abri for the property's -tenancy details. Tenant contact records then appear in HubSpot, linked to the deal — -unless Abri reports the property as empty (see below). Only the *first* time the date is -set counts; changing it later does not re-fetch. - -**Empty properties.** If Abri has no live tenancy for the property, no tenant contacts -appear on the deal — there is nobody to add — and **Extra booking information** is -prefixed with `Void.`. Anything already in that field is -kept, after the prefix. This is expected, and nothing needs re-triggering. - -Delete the `Void.` prefix if you establish the property is not empty after all — the -system only adds it once, so it will not come back on its own. - -### The surveyor comes from the Deal owner - -You don't fill in **Third-party surveyor identifier** yourself. HubSpot workflows do it: -each recognised surveyor has a workflow that takes the **Deal owner**, maps it to their -Abri resource ID, and stores that as the identifier. So the field to get right is **Deal -owner** — set it to the surveyor doing the visit and the identifier follows. - -Because the owner is picked from a dropdown it can't be misspelt, so this mostly just -works. The two things that can still go wrong: - -- The deal owner is someone who isn't one of the recognised surveyors — no workflow - fires, the identifier stays blank, and no booking is sent. -- Someone edits the identifier field itself by hand — free-typed or misspelt codes are - rejected by Abri's system and the booking won't be made. Leave it to the workflow. - -The tech team maintains the workflows and the list of recognised surveyors — if a -surveyor is missing, that's where to raise it. - -### Creating the booking - -The booking is created the moment the deal has **both** a **Confirmed survey date** and a -**Third-party surveyor identifier**. The order doesn't matter — whichever of the two -lands second is the change that sends the booking. Until both are in place, nothing is -sent. - -Since the identifier comes from the Deal owner (above), in practice booking a survey -means making sure the deal owner is the surveyor and setting the confirmed date. - -The booking is assigned within Abri's system to the surveyor on the deal on the confirmed date, in the slot -implied by **Confirmed survey time**: - -| Confirmed survey time | Slot sent to Abri | -|---|---| -| `morning` | AM | -| `afternoon` | PM | -| A clock time like `09:30` | AM or PM by whether it's before midday | -| Left blank | All day | - -**How you know it worked:** Abri's job number appears in **Client booking reference** on -the deal. A deal only ever gets one booking — once it has a job number, later changes -update that booking rather than creating another. - -### Changing the booking - -Changing **Confirmed survey date**, **Confirmed survey time** or **Third-party surveyor -identifier** on a deal that already has a booking updates the existing appointment — -new date/time and/or reassigns it to the new surveyor. (Reassigning works the same way as -booking: change the **Deal owner** and the workflow updates the identifier.) The deal -keeps the same Client booking reference. The date and surveyor must both still be filled in for the change to -be sent (see [watch-outs](#things-to-watch-out-for) on clearing fields). - -### Abandoning the booking - -When **Number of attempts** reaches 3 (or more) *and* **Outcome** is set to an -unsuccessful value, the booking is cancelled as abandoned. This fires once per deal. - -**Unsuccessful outcomes** that count towards abandonment — the wording must match -exactly: +**Unsuccessful outcomes** that count towards abandonment — the wording must match exactly: - `no answer` - `cancelled / no show` @@ -123,56 +41,61 @@ exactly: Any other outcome (or fewer than 3 attempts) does **not** cancel anything in Abri's system. +**Empty properties.** If Abri has no live tenancy for the property, no tenant contacts +appear on the deal — there is nobody to add — and **Extra booking information** is +prefixed with `Void.` so the surveyor sees it first. Anything already in that field is +kept, after the prefix. This is expected, not a fault, and nothing needs re-triggering. + +Note that Abri sends the same signal for a property reference it does not recognise, so +if you were expecting tenants, check the property is right before assuming it's empty. +Delete the `Void.` prefix if you establish the property is not empty after all — the +system only adds it once, so it will not come back on its own. + +## The fields, and why they matter + +| HubSpot deal field | What it feeds | +|---|---| +| **Expected commencement date** | Setting it the first time triggers the tenant-details fetch | +| **Confirmed survey date** | The appointment date sent to Abri | +| **Confirmed survey time** | The appointment slot: `morning` → AM, `afternoon` → PM, a clock time like `09:30` → AM/PM by whether it's before midday, left blank → all day | +| **Third-party surveyor identifier** | Which surveyor the booking is assigned to in Abri's system. **Required** — a booking or appointment change cannot be sent without it, and it must be one of Abri's valid surveyor codes. Changing it on a deal that already has a booking reassigns that booking to the new surveyor | +| **Number of attempts** + **Outcome** | Together they trigger abandonment (3+ attempts and an unsuccessful outcome) | +| **Client booking reference** | Abri's job number, written back **by the system** after the booking is created | +| **Deal name** | Used as the job description Abri's staff see | + --- -## Things to watch out for - -### No booking is made until both date and surveyor are filled in - -Entering a survey date on a deal whose **Third-party surveyor identifier** is blank sends -nothing — the system is waiting for the surveyor. It fires as soon as the second of the -two fields is filled in, so a deal can sit half-complete indefinitely without an error. -If a booking you expected hasn't appeared, check both fields are set — and since the -identifier comes from the -[Deal owner](#the-surveyor-comes-from-the-deal-owner), a blank identifier usually means -the owner is missing or isn't a recognised surveyor. +## Things that will catch you out ### Clearing the survey date does NOT cancel the booking -If you delete or blank the confirmed survey date (or the surveyor), the booking **still -exists in Abri's system** and the surveyor is still expected — clearing a field sends -nothing. There is currently no way to cancel a booking from HubSpot other than the -3-attempts abandonment route. If a booking needs cancelling for any other reason, -contact the tech team. (Re-entering a date later updates the existing booking; it will -not create a duplicate.) +If you delete or blank the confirmed survey date, the booking **still exists in Abri's +system** and the surveyor is still expected. There is currently no way to cancel a +booking from HubSpot other than the 3-attempts abandonment route. If a booking needs +cancelling for any other reason, contact the tech team. ### Don't edit Client booking reference -It is written automatically with Abri's job number and is how the system tells a new -booking from a change to an existing one: +It is written automatically with Abri's job number and is how the system finds the +booking when you later change or abandon it. If you overwrite or clear it, appointment +changes and abandonments will stop working for that deal. -- On a deal that is **not yet booked**, typing anything into this field stops the - booking from ever being created — the system reads a value there as "already booked". -- On a **booked** deal, overwriting or clearing it breaks appointment changes and - abandonment for that deal — those changes fail over to the tech team rather than - reaching Abri. +### The surveyor identifier (Third-Party Surveyor Identifier) must be a valid Abri code + +Free-typed or misspelt values will be rejected by Abri's system and the booking won't be +made. The tech team holds the current list of valid surveyor codes from Abri — use a +value from that list. Soon this field will be set automatically from the deal +owner. ### Outcome wording is exact Abandonment only recognises the four unsuccessful outcomes listed above, spelled exactly that way. A variation like "No Answer - left card" won't trigger it. -### Abandonment only cancels an actual booking - -Recording 3 unsuccessful attempts on a deal that never got as far as a booking (no -Client booking reference) has nothing to cancel — the attempt lands with the tech team -as an error rather than doing anything in Abri's system. That's harmless, but worth -knowing if you're tidying up deals that never went ahead. - ### If the job number never appears The booking didn't go through — most often because the surveyor identifier was missing or invalid, or the deal isn't on the Abri project. Failures land with the tech team, not in HubSpot, so you won't see an error message. If **Client booking reference** is still -empty well after both the survey date and surveyor are set, flag it to the tech team -rather than re-editing fields. +empty well after you set the survey date, flag it to the tech team rather than re-editing +fields.