From 7997d883759dd025a5583567b86332ff24d53057 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 9 Jul 2026 16:27:50 +0000 Subject: [PATCH 1/3] minor updates following Abri feedback --- domain/abri/models.py | 3 ++- domain/abri/phone_selection.py | 6 +++--- infrastructure/abri/abri_client.py | 11 +++++------ tests/infrastructure/abri/test_abri_client.py | 16 ++++++++++++++++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/domain/abri/models.py b/domain/abri/models.py index e0cd20c98..df726608b 100644 --- a/domain/abri/models.py +++ b/domain/abri/models.py @@ -49,7 +49,8 @@ class LogJobRequest: @dataclass(frozen=True) class JobLogged: job_no: str - logged_info: str + # Informational only (never read downstream); absent on some successes. + logged_info: Optional[str] @dataclass(frozen=True) diff --git a/domain/abri/phone_selection.py b/domain/abri/phone_selection.py index fe3588d5f..7e2ae98f0 100644 --- a/domain/abri/phone_selection.py +++ b/domain/abri/phone_selection.py @@ -22,9 +22,9 @@ class SelectedPhoneNumbers: secondary: Optional[str] -# TODO(#1474): confirm the priority ordering with Abri — this assumes the -# lowest numeric priority marks the preferred number, and that entries with -# a missing or non-numeric priority rank last (ties keep document order). +# Abri confirmed the lowest numeric priority marks the preferred number; +# entries with a missing or non-numeric priority rank last (ties keep +# document order). def _priority_sort_key(entry: PhoneEntry) -> Tuple[int, int]: """Sort key: lowest numeric priority first; unusable priorities last.""" try: diff --git a/infrastructure/abri/abri_client.py b/infrastructure/abri/abri_client.py index ebcd65f64..19054d200 100644 --- a/infrastructure/abri/abri_client.py +++ b/infrastructure/abri/abri_client.py @@ -174,14 +174,13 @@ class AbriClient: ) job_no = job_logged.get("job_no") - logged_info = job_logged.get("logged_info") - if job_no is None or logged_info is None: - raise AbriResponseParseError( - "Job_logged element missing job_no or logged_info" - ) + if job_no is None: + raise AbriResponseParseError("Job_logged element missing job_no") - return JobLogged(job_no=job_no, logged_info=logged_info) + # logged_info is purely informational and absent on some successes, so + # its presence is not required to accept the job as logged. + return JobLogged(job_no=job_no, logged_info=job_logged.get("logged_info")) @staticmethod def _parse_appointment_amended(root: ET.Element) -> AppointmentAmended: diff --git a/tests/infrastructure/abri/test_abri_client.py b/tests/infrastructure/abri/test_abri_client.py index 13bac577b..e891e4283 100644 --- a/tests/infrastructure/abri/test_abri_client.py +++ b/tests/infrastructure/abri/test_abri_client.py @@ -106,6 +106,22 @@ def test_log_job_returns_job_logged_with_openhousing_job_number( ) +def test_log_job_accepts_a_success_without_logged_info( + client: AbriClient, mock_session: MagicMock +) -> None: + # Arrange: logged_info is informational, so a success carrying only a + # job_no must still be accepted rather than fail a job that was logged. + mock_session.post.return_value.content = ( + b'' + ) + + # Act + result = client.log_job(_spec_example_request()) + + # Assert + assert result == JobLogged(job_no="AD1", logged_info=None) + + def test_log_job_sends_the_recorded_logjob_envelope_to_the_relay_endpoint( client: AbriClient, mock_session: MagicMock ) -> None: From f8b9d82200f4f85b2dbe068f180d695f73a6366f Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 10 Jul 2026 10:37:35 +0000 Subject: [PATCH 2/3] Always send a bookable resource on Abri amend_job OpenHousing logs a job but silently drops the appointment when no bookable resource is sent, so the survey date never lands. log_job already sends the configured default_resource; amend_job omitted the parameter unless the request carried an explicit surveyor. Default amend's resource to the configured surveyor too, and fail loudly at config load on a blank ABRI_RELAY_DEFAULT_RESOURCE so the misconfiguration can't ship an empty resource again. Co-Authored-By: Claude Opus 4.8 --- infrastructure/abri/abri_client.py | 11 +++++++++-- infrastructure/abri/config.py | 9 ++++++++- tests/infrastructure/abri/test_abri_client.py | 13 ++++++++----- tests/infrastructure/abri/test_abri_config.py | 17 +++++++++++++++++ .../test_abri_orchestrator_amend_job.py | 3 +++ 5 files changed, 45 insertions(+), 8 deletions(-) diff --git a/infrastructure/abri/abri_client.py b/infrastructure/abri/abri_client.py index 19054d200..66e094eab 100644 --- a/infrastructure/abri/abri_client.py +++ b/infrastructure/abri/abri_client.py @@ -78,14 +78,21 @@ class AbriClient: return self._parse_job_logged(outcome) def amend_job(self, request: AmendJobRequest) -> AmendJobResult: + # OpenHousing silently drops the appointment unless a bookable resource + # is sent, so fall back to the configured surveyor when the request + # carries no explicit override (mirrors log_job). + resource = ( + request.resource + if request.resource is not None + else self._config.default_resource + ) parameters = [ ("job_no", request.job_no), ("action", "amend"), ("appointment_date", _format_appointment_date(request.appointment_date)), ("appointment_time", request.appointment_time), + ("resource", resource), ] - if request.resource is not None: - parameters.append(("resource", request.resource)) outcome = self._exchange( request_type="amendoptiappt", diff --git a/infrastructure/abri/config.py b/infrastructure/abri/config.py index f2da18a77..f29e3f10c 100644 --- a/infrastructure/abri/config.py +++ b/infrastructure/abri/config.py @@ -11,9 +11,16 @@ class AbriConfig: @classmethod def from_env(cls, env: Mapping[str, str]) -> "AbriConfig": + # A blank default_resource does not error at the relay — OpenHousing + # logs the job but silently drops the appointment — so reject it here + # rather than let the misconfiguration ship an empty resource. + default_resource = env["ABRI_RELAY_DEFAULT_RESOURCE"] + if not default_resource.strip(): + raise ValueError("ABRI_RELAY_DEFAULT_RESOURCE must not be blank") + return cls( endpoint_url=env["ABRI_RELAY_URL"], username=env["ABRI_RELAY_USERNAME"], password=env["ABRI_RELAY_PASSWORD"], - default_resource=env["ABRI_RELAY_DEFAULT_RESOURCE"], + default_resource=default_resource, ) diff --git a/tests/infrastructure/abri/test_abri_client.py b/tests/infrastructure/abri/test_abri_client.py index e891e4283..42ede432a 100644 --- a/tests/infrastructure/abri/test_abri_client.py +++ b/tests/infrastructure/abri/test_abri_client.py @@ -410,10 +410,12 @@ def test_amend_job_sends_the_recorded_amendoptiappt_envelope_to_the_relay_endpoi ) -def test_amend_job_omits_the_resource_parameter_when_no_surveyor_is_provided( +def test_amend_job_defaults_the_resource_to_the_configured_surveyor( client: AbriClient, mock_session: MagicMock ) -> None: - # Arrange + # Arrange: OpenHousing silently drops the appointment unless a bookable + # resource is sent, so an amend without an explicit surveyor falls back to + # the configured default rather than omitting the parameter. mock_session.post.return_value.content = _load_fixture( "abri_relay_amendoptiappt_success_response.xml" ) @@ -423,11 +425,12 @@ def test_amend_job_omits_the_resource_parameter_when_no_surveyor_is_provided( # Assert sent_body: bytes = mock_session.post.call_args.kwargs["data"] - sent_attributes = [ - parameter.get("attribute") + resources = [ + parameter.get("attribute_value") for parameter in ET.fromstring(sent_body).findall(".//Parameters") + if parameter.get("attribute") == "resource" ] - assert "resource" not in sent_attributes + assert resources == [CONFIG.default_resource] # --- amend_job: rejection --- diff --git a/tests/infrastructure/abri/test_abri_config.py b/tests/infrastructure/abri/test_abri_config.py index 4e1d8c87d..0884a14a3 100644 --- a/tests/infrastructure/abri/test_abri_config.py +++ b/tests/infrastructure/abri/test_abri_config.py @@ -1,3 +1,5 @@ +import pytest + from infrastructure.abri.config import AbriConfig @@ -20,3 +22,18 @@ def test_config_hydrates_relay_credentials_and_defaults_from_environment() -> No password="secret", default_resource="CBRYAN", ) + + +@pytest.mark.parametrize("blank", ["", " "]) +def test_config_rejects_a_blank_default_resource(blank: str) -> None: + # A blank resource does not error at the relay: OpenHousing logs the job + # but silently drops the appointment. Fail loudly at config load instead. + env = { + "ABRI_RELAY_URL": "https://relay.example.test/api/DomnaRelay?code=key", + "ABRI_RELAY_USERNAME": "DomnaWeb", + "ABRI_RELAY_PASSWORD": "secret", + "ABRI_RELAY_DEFAULT_RESOURCE": blank, + } + + with pytest.raises(ValueError, match="ABRI_RELAY_DEFAULT_RESOURCE"): + AbriConfig.from_env(env) diff --git a/tests/orchestration/test_abri_orchestrator_amend_job.py b/tests/orchestration/test_abri_orchestrator_amend_job.py index f5ba20e57..1a3386a5b 100644 --- a/tests/orchestration/test_abri_orchestrator_amend_job.py +++ b/tests/orchestration/test_abri_orchestrator_amend_job.py @@ -114,6 +114,9 @@ def test_amend_job_sends_an_amendoptiappt_envelope_for_the_deals_recorded_job( "action": "amend", "appointment_date": "24/06/2026", "appointment_time": "PM", + # OpenHousing drops the appointment without a bookable resource, so the + # amend defaults to the configured surveyor when none is overridden. + "resource": CONFIG.default_resource, } From 208113ae6d5fea0d4ea6aa01580e6f4096addf89 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 10 Jul 2026 10:45:57 +0000 Subject: [PATCH 3/3] send AM or PM instead of trying to parse morning or afternoon --- domain/abri/slots.py | 9 ++++++++- tests/domain/abri/test_slots.py | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/domain/abri/slots.py b/domain/abri/slots.py index 992426ee9..d6b7a2026 100644 --- a/domain/abri/slots.py +++ b/domain/abri/slots.py @@ -5,9 +5,16 @@ from domain.abri.models import SlotCode _MIDDAY = time(12, 0) +# HubSpot's confirmed-time field carries these half-day labels, not clock times. +_LABEL_SLOTS: dict[str, SlotCode] = {"morning": "AM", "afternoon": "PM"} + def slot_for_confirmed_time(confirmed_time: Optional[str]) -> SlotCode: if confirmed_time is None or confirmed_time.strip() == "": return "AD" - parsed = datetime.strptime(confirmed_time.strip(), "%H:%M").time() + normalised = confirmed_time.strip() + label_slot = _LABEL_SLOTS.get(normalised.lower()) + if label_slot is not None: + return label_slot + parsed = datetime.strptime(normalised, "%H:%M").time() return "AM" if parsed < _MIDDAY else "PM" diff --git a/tests/domain/abri/test_slots.py b/tests/domain/abri/test_slots.py index fa5283691..7d72a00c4 100644 --- a/tests/domain/abri/test_slots.py +++ b/tests/domain/abri/test_slots.py @@ -38,6 +38,26 @@ def test_a_confirmed_time_at_or_after_midday_books_the_afternoon_slot( assert slot == "PM" +@pytest.mark.parametrize("confirmed_time", ["morning", "Morning", " MORNING "]) +def test_hubspots_morning_label_books_the_morning_slot(confirmed_time: str) -> None: + # Act + slot = slot_for_confirmed_time(confirmed_time) + + # Assert + assert slot == "AM" + + +@pytest.mark.parametrize("confirmed_time", ["afternoon", "Afternoon", " AFTERNOON "]) +def test_hubspots_afternoon_label_books_the_afternoon_slot( + confirmed_time: str, +) -> None: + # Act + slot = slot_for_confirmed_time(confirmed_time) + + # Assert + assert slot == "PM" + + @pytest.mark.parametrize("confirmed_time", ["half nine", "25:00", "9.30am"]) def test_an_unparseable_confirmed_time_fails_loudly_rather_than_booking_all_day( confirmed_time: str,