Quote-path hardening: ultrareview + user review findings

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }

apps/accounting/services/quote_pdf.py

@@ -12,13 +12,18 @@
 
 @dataclass(frozen=True)
 class QuotePdfInspection:
- """Structured evidence from a provider-rendered quote PDF."""
+ """Structured evidence from a provider-rendered quote PDF.
+
+ ``retained_pdf_path`` names the kept file when the marker was absent —
+ an unlocatable diagnostic artifact is as good as a deleted one.
+ """
 
  quote_id: str
  remote_branding_theme_id: str | None
  configured_branding_theme_id: str | None
  page_count: int
  contains_expected_text: bool
+ retained_pdf_path: str | None
 
 
 def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
@@ -43,7 +48,10 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  page_count = len(reader.pages)
 
  if not page_text:
- raise ValueError(f"Quote {quote_id} PDF contains no extractable text")
+ raise ValueError(
+ f"Quote {quote_id} PDF contains no extractable text "
+ f"(file retained at {document.temporary_file_path})"
+ )
 
  # Xero's text layer sometimes wraps mid-phrase and sometimes drops word
  # spaces entirely — match both the space-normalised and compact forms so
@@ -52,22 +60,22 @@ def inspect_quote_pdf(quote_id: UUID, expected_text: str) -> QuotePdfInspection:
  compact_expected_text = "".join(normalized_expected_text.split())
  compact_document_text = "".join(normalized_document_text.split())
  configured_theme_id = CompanyDefaults.get_solo().xero_sales_branding_theme_id
- inspection = QuotePdfInspection(
+ contains_expected_text = (
+ normalized_expected_text in normalized_document_text
+ or compact_expected_text in compact_document_text
+ )
+ # Only when the marker was FOUND: an absent marker is exactly the case an
+ # operator needs the rendered PDF for, so it keeps the file just like the
+ # exception paths above do — and reports where it is.
+ if contains_expected_text:
+ Path(document.temporary_file_path).unlink(missing_ok=True)
+ return QuotePdfInspection(
  quote_id=document.external_id,
  remote_branding_theme_id=document.document_theme_external_id,
  configured_branding_theme_id=(
  str(configured_theme_id) if configured_theme_id is not None else None
  ),
  page_count=page_count,
- contains_expected_text=(
- normalized_expected_text in normalized_document_text
- or compact_expected_text in compact_document_text
- ),
+ contains_expected_text=contains_expected_text,
+ retained_pdf_path=None if contains_expected_text else document.temporary_file_path,
  )
- # Only when the marker was FOUND: an absent marker is exactly the case an
- # operator needs the rendered PDF for, so it keeps the file just like the
- # exception paths above do. (Improves on the ported behaviour, which
- # deleted the file in its own diagnostic case.)
- if inspection.contains_expected_text:
- Path(document.temporary_file_path).unlink(missing_ok=True)
- return inspection

apps/accounting/tests/test_quote_pdf.py

@@ -76,6 +76,7 @@ def test_terms_marker_survives_pdf_line_wrapping(self, mock_get_provider: Mock)
  assert result.page_count == 1
  assert result.remote_branding_theme_id == REMOTE_THEME_ID
  assert result.configured_branding_theme_id == REMOTE_THEME_ID
+ assert result.retained_pdf_path is None
  assert not pdf_path.exists()
 
  @patch("apps.accounting.services.quote_pdf.get_provider")
@@ -90,7 +91,9 @@ def test_missing_or_differently_cased_terms_marker_is_red(
 
  assert not result.contains_expected_text
  # The absent marker is the diagnostic case: the rendered PDF survives
- # for the operator, unlike a found marker's cleanup.
+ # for the operator, and the report says WHERE — an unlocatable
+ # artifact is as good as a deleted one.
+ assert result.retained_pdf_path == str(pdf_path)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -131,9 +134,10 @@ def test_blank_pages_raise_rather_than_reporting_terms_absent(
  pdf_path = _write_pdf([])
  mock_get_provider.return_value = _provider_for_pdf(quote_id, pdf_path)
 
- with pytest.raises(ValueError, match="no extractable text"):
+ with pytest.raises(ValueError, match="no extractable text") as caught:
  inspect_quote_pdf(quote_id, EXPECTED_TERMS)
 
+ assert str(pdf_path) in str(caught.value)
  assert pdf_path.exists()
  pdf_path.unlink()
 
@@ -154,6 +158,7 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  configured_branding_theme_id=REMOTE_THEME_ID,
  page_count=2,
  contains_expected_text=False,
+ retained_pdf_path="/var/artifacts/retained.pdf",
  )
  output = StringIO()
 
@@ -170,4 +175,5 @@ def test_command_emits_one_json_result(self, mock_inspect: Mock) -> None:
  "page_count": 2,
  "quote_id": str(quote_id),
  "remote_branding_theme_id": REMOTE_THEME_ID,
+ "retained_pdf_path": "/var/artifacts/retained.pdf",
  }