From 42009f1d7306603b92de8abe7d98cd4d1930c92b Mon Sep 17 00:00:00 2001 From: Joypal Navaneeth Date: Tue, 1 Sep 2026 12:51:22 +0530 Subject: [PATCH] fix: escape webUrl in Rich output, reconcile now reports what it actually compared --- README.md | 12 +++++--- docs/ARCHITECTURE.md | 6 +++- src/tendertracker/cli.py | 15 ++++++++-- src/tendertracker/pipeline/reconcile.py | 38 +++++++++++++++++++------ tests/test_reconcile.py | 16 +++++++++++ 5 files changed, 71 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index ea32478..efc2751 100644 --- a/README.md +++ b/README.md @@ -137,10 +137,14 @@ relevance filtering, the core pipeline (dedup, field-level authority, per-record error isolation — using a real temp SQLite DB, not mocked), and every external integration (Pipedrive, OneDrive/SharePoint, Outlook Calendar) via mocked HTTP request-shape assertions, since no live -credentials for those services are available in CI. Pipedrive/calendar/ -reconciliation orchestration logic is tested against a real DB with the -external client mocked — covering dry-run-vs-apply, idempotency, and the -create-then-update transition. +credentials for those services are available in CI. Pipedrive and calendar +sync orchestration are each tested against a real DB with the external +client mocked, covering dry-run-vs-apply, idempotency, and the +create-then-update transition. Reconciliation orchestration is tested +separately against a real DB and a real generated Excel file (plus a mocked +Pipedrive client) — read-only behavior is asserted directly (confirms zero +writes), along with correctly distinguishing "verified in sync" from +"nothing was available to compare against." ## Adding a source or a sync target diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 66943ae..8e25f5b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -158,7 +158,11 @@ makes zero API calls. DB, the exported Excel file, and (if configured) live Pipedrive state, and reports differences on a small, explicit set of fields — it does not decide which system is "right" or write a fix. That decision is left to a human, -by design. +by design. It also reports what it actually had available to compare +against (`excel_available`, `pipedrive_configured`) — "no discrepancies +found" only means something if at least one comparison actually ran; a repo +where Excel was never exported and Pipedrive isn't configured would +otherwise report a misleadingly clean result. **Naive UTC everywhere, not timezone-aware datetimes.** SQLite doesn't preserve timezone info across a save/reload — a timezone-aware value read diff --git a/src/tendertracker/cli.py b/src/tendertracker/cli.py index 816b8e6..4afb30d 100644 --- a/src/tendertracker/cli.py +++ b/src/tendertracker/cli.py @@ -174,7 +174,7 @@ def _export_excel_and_cloud_sync() -> None: try: result = sync_excel_if_configured(settings) if result is not None: - console.print(f"[green]Cloud sync:[/green] {result.get('webUrl', settings.ms_graph_upload_path)}") + console.print(f"[green]Cloud sync:[/green] {escape(result.get('webUrl', settings.ms_graph_upload_path))}") except Exception as exc: console.print(f"[red]Cloud sync failed: {escape(str(exc))}[/red]") @@ -226,7 +226,7 @@ def sync_cloud() -> None: raise typer.Exit(code=1) result = sync_excel_if_configured(settings) - console.print(f"[green]Cloud sync:[/green] {result.get('webUrl', settings.ms_graph_upload_path)}") + console.print(f"[green]Cloud sync:[/green] {escape(result.get('webUrl', settings.ms_graph_upload_path))}") @app.command(name="sync-pipedrive") @@ -288,6 +288,17 @@ def reconcile() -> None: result = run_reconcile(settings) console.print(f"Checked {result.checked} tenders.") + console.print( + f"Excel comparison: {'available' if result.excel_available else '[yellow]skipped — no exported .xlsx found, run tendertracker export first[/yellow]'}" + ) + console.print( + f"Pipedrive comparison: {'available' if result.pipedrive_configured else '[yellow]skipped — not configured[/yellow]'}" + ) + + if not result.excel_available and not result.pipedrive_configured: + console.print( + "[yellow]Nothing was actually compared — 'no discrepancies' below doesn't mean verified in sync.[/yellow]" + ) if result.discrepancies: table = Table(title="Discrepancies") diff --git a/src/tendertracker/pipeline/reconcile.py b/src/tendertracker/pipeline/reconcile.py index 88f40c6..c928d11 100644 --- a/src/tendertracker/pipeline/reconcile.py +++ b/src/tendertracker/pipeline/reconcile.py @@ -28,30 +28,40 @@ class ReconcileResult: checked: int = 0 discrepancies: list[Discrepancy] = field(default_factory=list) errors: list[str] = field(default_factory=list) - - -def _read_excel_statuses(path: str) -> dict[str, str]: - """Returns {external_id: status} read directly from the exported .xlsx — - the one field the spreadsheet's dropdown lets a human edit. Everything - else in Excel is a display copy of the DB, not an independent source.""" + # What was actually available to compare against — "no discrepancies" + # only means something was verified if these are true. Both false is a + # real, distinct outcome: nothing was checked, not "everything's fine." + excel_available: bool = False + pipedrive_configured: bool = False + + +def _read_excel_statuses(path: str) -> tuple[dict[str, str], bool]: + """Returns ({external_id: status}, available) read directly from the + exported .xlsx — the one field the spreadsheet's dropdown lets a human + edit. Everything else in Excel is a display copy of the DB, not an + independent source. `available` is False if the file doesn't exist yet + (never exported) or has no sheet with the expected columns — distinct + from "available but genuinely empty," which is a real, comparable state.""" statuses: dict[str, str] = {} try: wb = load_workbook(path, data_only=True) except FileNotFoundError: - return statuses + return statuses, False + found_expected_columns = False for sheet_name in wb.sheetnames: ws = wb[sheet_name] headers = [cell.value for cell in ws[1]] if "External ID" not in headers or "Status" not in headers: continue + found_expected_columns = True id_idx = headers.index("External ID") status_idx = headers.index("Status") for row in ws.iter_rows(min_row=2, values_only=True): external_id = row[id_idx] if external_id: statuses[external_id] = row[status_idx] - return statuses + return statuses, found_expected_columns def reconcile(settings: "Settings") -> ReconcileResult: @@ -63,11 +73,12 @@ def reconcile(settings: "Settings") -> ReconcileResult: human to resolve, don't auto-resolve it. """ result = ReconcileResult() - excel_statuses = _read_excel_statuses(settings.excel_export_path) + excel_statuses, result.excel_available = _read_excel_statuses(settings.excel_export_path) pipedrive_client = None if settings.pipedrive_api_token and settings.pipedrive_domain: pipedrive_client = PipedriveClient(settings.pipedrive_api_token, settings.pipedrive_domain) + result.pipedrive_configured = True engine = get_engine(settings.db_path) session_factory = get_session_factory(engine) @@ -96,6 +107,15 @@ def reconcile(settings: "Settings") -> ReconcileResult: db_value = tender.estimated_value deal_value = deal.get("value") + # deal_value == 0 is treated as "not set," same as None — a + # deliberate but unverified tradeoff: Pipedrive's API isn't + # confirmed live in this project (no live account available, + # see integrations/pipedrive.py), so it's not known whether + # value=0 means "genuinely zero" or "field left blank." A + # false negative (missing a real $0 deal) was judged safer + # than a false positive on every newly-created, not-yet-priced + # deal. Revisit against a real Pipedrive account if this + # matters for your use case. if db_value is not None and deal_value not in (None, 0) and float(db_value) != float(deal_value): result.discrepancies.append( Discrepancy(tender.external_id, "estimated_value", str(db_value), str(deal_value), "pipedrive") diff --git a/tests/test_reconcile.py b/tests/test_reconcile.py index 88ff346..27e91bf 100644 --- a/tests/test_reconcile.py +++ b/tests/test_reconcile.py @@ -24,6 +24,21 @@ def test_no_drift_on_fresh_export(populated): result = reconcile(settings) assert result.checked == 5 assert result.discrepancies == [] + assert result.excel_available is True + assert result.pipedrive_configured is False + + +def test_excel_unavailable_when_never_exported(tmp_settings, db_session): + """Distinct from 'no discrepancies' — nothing to compare against yet, + since export was never run. Regression test for a real gap: this used + to be indistinguishable from a genuinely-verified clean result.""" + run_source(db_session, SANDBOX_PORTAL) + db_session.commit() + + result = reconcile(tmp_settings) + assert result.excel_available is False + assert result.pipedrive_configured is False + assert result.discrepancies == [] # would misleadingly look "clean" without the availability flags def test_catches_status_edited_directly_in_excel(populated): @@ -79,6 +94,7 @@ def test_pipedrive_value_drift_detected(populated): value_discrepancies = [d for d in result.discrepancies if d.field == "estimated_value"] assert len(value_discrepancies) == 1 assert value_discrepancies[0].other_system == "pipedrive" + assert result.pipedrive_configured is True def test_pipedrive_fetch_failure_recorded_as_error_not_crash(populated):