diff --git a/.gitignore b/.gitignore index b9aa3c4..751e823 100644 --- a/.gitignore +++ b/.gitignore @@ -28,13 +28,29 @@ htmlcov/ # OS .DS_Store +.AppleDouble +.LSOverride +._* Thumbs.db +ehthumbs.db +Desktop.ini -# Editor +# Editor / IDE .idea/ .vscode/ +.cursor/ *.swp *~ +*.orig +*.bak +*.tmp + +# Local launchers (macOS double-click scripts) +start.command +*.command + +# Logs +*.log # Excel lock / temp files ~$*.xlsx diff --git a/README.md b/README.md index 3d5a9e5..b496f9e 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,14 @@ makes it available to a team. ### The dashboard +Double-click **start.command** in this folder. That is the whole thing — it +checks Python, installs `openpyxl` if it is missing, picks a free port, starts +the server and opens your browser. Keep the Terminal window it opens; closing it +(or Ctrl+C) stops the dashboard. Double-clicking again while it is already +running just reopens the tab instead of starting a second copy. + +From a terminal instead: + ```bash python3 serve.py ``` @@ -146,6 +154,31 @@ which is precisely backwards. Only the delivery half of that change type The window is always the span the data actually covers, never what you asked Amazon for. One day of export can only ever say "no action in 1 day". +## When an export is unusable + +The tool refuses a file rather than guessing, and says exactly why. The common +causes, all seen in the wild: + +**No timestamps.** If `Date and time` and `Date and time (ISO)` are both empty, +there is no way to know *when* a campaign went in or out of budget, so hours +cannot be measured at all. Nothing can be salvaged from that file — re-run the +extraction. (If only the ISO column is empty, the human-readable one is used +instead and the file works fine.) + +**Exporter warnings.** The export may carry an "Errors and Warnings" sheet. +`PARTIAL_PAGE_HARVEST` means the extension collected fewer rows than the page +reported, so changes are missing and every duration becomes a lower bound. That +is surfaced in Data Quality rather than swallowed. + +**Mixed marketplaces.** One file can end up holding rows from two consoles — +`advertising.amazon.de` and `advertising.amazon.es`, say — if two extraction runs +were merged. Account spend and ROAS come from a single metadata block, so money +would be attributed to the wrong marketplace. Export each marketplace separately. + +Loading several files at once? Any file that cannot be used is named in a red +banner at the top of the dashboard with the reason, and the rest are still +analysed. A dropped file is never silent. + ## The diagnoses | Label | Means | @@ -262,11 +295,11 @@ python3 tests/test_golden.py # the analysis python3 tests/test_auth_smoke.py # the hosted app ``` -`test_golden.py` is 34 checks: frozen totals from the reference export, +`test_golden.py` is 38 checks: frozen totals from the reference export, structural invariants, overlapping-export handling, action classification, and edge cases. The important ones are `test_chain_breaks_canary` and `test_amazon_pacing_rows_are_not_actions`. It needs the reference export in -`data/`; the 15 synthetic-fixture checks run without it. +`data/`; the synthetic-fixture checks run without it. `test_auth_smoke.py` is 53 checks covering the whole account lifecycle plus upload, analyse and export. It runs against in-memory SQLite with mail captured diff --git a/app/services/analysis.py b/app/services/analysis.py index c6e0f14..62aa33a 100644 --- a/app/services/analysis.py +++ b/app/services/analysis.py @@ -41,15 +41,21 @@ def run_analysis(history: Sequence[Path], perf: Path | None, try: evs, meta, qa = load_history(path) except (ValueError, KeyError, OSError) as exc: - skipped.append(f"{path.name}: {exc}") + message = str(exc) + skipped.append(message if path.name in message + else f"{path.name}: {message}") continue events.extend(evs) metas.append(meta) qas.append(qa) if not events: - detail = " ".join(skipped) or "no readable rows" - raise ValueError(f"None of the files could be read as a change-history export. {detail}") + if skipped: + # The per-file reason is the useful part; do not bury it behind a + # generic "could not be read". + raise ValueError(" ".join(skipped) if len(skipped) == 1 + else "No file could be used. " + " ".join(skipped)) + raise ValueError("The file has no readable change rows.") events, overlap_rows = dedupe_events(events) days = score_all(events, merge_gap_min=int(settings_in.get("merge_gap", 5))) diff --git a/ppcbudget/excelout.py b/ppcbudget/excelout.py index a09b4ae..a40e2d8 100644 --- a/ppcbudget/excelout.py +++ b/ppcbudget/excelout.py @@ -556,6 +556,20 @@ def _sheet_quality(wb: Workbook, qas: list[QaReport], days: list[CampaignDay], if m.status and m.status != "completed": check(f"Extraction status: {qa.path.name[:28]}", False, m.status, "completed", "A partial extraction can be missing whole campaigns, not just rows.") + if qa.warning_categories: + check(f"Exporter warnings: {qa.path.name[:28]}", False, + ", ".join(f"{n}x {c}" for c, n in qa.warning_categories.items()), "none", + "The export recorded its own failures. PARTIAL_PAGE_HARVEST means it " + "collected fewer rows than the page reported, so changes are missing and " + "every duration here is a lower bound. Re-run the extraction.") + if qa.mixed_sources: + check(f"Mixed sources: {qa.path.name[:28]}", False, + f"{len(qa.source_urls)} consoles / {len(qa.row_marketplaces)} marketplaces", + "1 / 1", + "This file holds rows from more than one console or marketplace (" + + ", ".join(sorted(qa.row_marketplaces)[:4]) + + "). Account spend and ROAS come from one metadata block, so money would be " + "attributed to the wrong marketplace. Export each marketplace separately.") if len(qas) > 1: check("Overlapping exports", True, diff --git a/ppcbudget/ingest.py b/ppcbudget/ingest.py index 2e10f70..dd880d7 100644 --- a/ppcbudget/ingest.py +++ b/ppcbudget/ingest.py @@ -57,6 +57,8 @@ class WorkbookMeta: rows_exported: int | None = None duplicates_skipped: int | None = None pages_processed: int | None = None + started_at: str = "" + completed_at: str = "" spend: float | None = None sales: float | None = None roas: float | None = None @@ -73,10 +75,22 @@ class QaReport: rows_unparsable_time: int = 0 rows_no_campaign: int = 0 # account- or portfolio-level rows rows_blank: int = 0 + # Warnings the exporter itself recorded, and provenance of the rows. A file + # holding two marketplaces or two extraction runs was stitched together. + extraction_warnings: list[str] = field(default_factory=list) + warning_categories: dict = field(default_factory=dict) + source_urls: list[str] = field(default_factory=list) + row_marketplaces: list[str] = field(default_factory=list) + row_run_ids: list[str] = field(default_factory=list) distinct_campaigns: int = 0 campaigns_with_budget_events: int = 0 date_keys: list[str] = field(default_factory=list) + @property + def mixed_sources(self) -> bool: + """True when one file holds rows from more than one console or run.""" + return len(self.source_urls) > 1 or len(self.row_marketplaces) > 1 + @property def rows_seen(self) -> int: """Every data row in the sheet, including ones we deliberately drop.""" @@ -118,6 +132,39 @@ class QaReport: return " = ".join([parts[0], " + ".join(parts[1:])]) +_WHEN_FORMATS = ( + "%d %b %Y %H:%M:%S.%f", "%d %b %Y %H:%M:%S", "%d %b %Y %H:%M", + "%m/%d/%Y %H:%M:%S.%f", "%m/%d/%Y %H:%M:%S", "%m/%d/%Y %H:%M", + "%Y-%m-%d %H:%M:%S", "%Y-%m-%d %H:%M", +) + + +def parse_when(*candidates) -> datetime | None: + """First readable timestamp among the given cells. + + The ISO column is preferred, but some exports fill only the human-readable + one, so both are tried before a row is written off. + """ + for raw in candidates: + if raw is None or raw == "": + continue + if isinstance(raw, datetime): + return raw + text = str(raw).strip() + if not text or text == "None": + continue + try: + return datetime.fromisoformat(text.replace("Z", "+00:00")) + except ValueError: + pass + for fmt in _WHEN_FORMATS: + try: + return datetime.strptime(text, fmt) + except ValueError: + continue + return None + + def _num(value) -> float | None: if value is None or value == "": return None @@ -168,6 +215,8 @@ def _read_meta(wb, path: Path) -> WorkbookMeta: meta.date_range = str(pairs.get("Date range", "") or "") meta.run_id = str(pairs.get("Extraction run ID", "") or "") meta.status = str(pairs.get("Status", "") or "") + meta.started_at = str(pairs.get("Started at", "") or "") + meta.completed_at = str(pairs.get("Completed at", "") or "") for attr, key in ( ("rows_expected", "Rows expected"), ("rows_exported", "Rows exported"), @@ -189,6 +238,37 @@ def _read_meta(wb, path: Path) -> WorkbookMeta: return meta +def _read_warnings(wb, qa: QaReport) -> None: + """The exporter records its own failures on an 'Errors and Warnings' sheet. + + Ignoring it means silently inheriting whatever it could not collect, so the + categories and a sample of messages are carried through to Data Quality. + """ + sheet = next((n for n in wb.sheetnames if "error" in n.lower() + or "warning" in n.lower()), None) + if sheet is None: + return + rows = list(wb[sheet].iter_rows(values_only=True)) + if len(rows) < 2: + return + header = [str(h).strip().lower() if h else "" for h in rows[0]] + + def idx(name): + return header.index(name) if name in header else None + + i_cat, i_msg, i_pg = idx("category"), idx("message"), idx("page number") + for row in rows[1:]: + if not row or all(v is None for v in row): + continue + cat = str(row[i_cat]).strip() if i_cat is not None and row[i_cat] else "WARNING" + msg = str(row[i_msg]).strip() if i_msg is not None and row[i_msg] else "" + page = row[i_pg] if i_pg is not None else None + qa.warning_categories[cat] = qa.warning_categories.get(cat, 0) + 1 + if len(qa.extraction_warnings) < 8: + qa.extraction_warnings.append( + f"{cat}" + (f" (page {page})" if page else "") + (f": {msg}" if msg else "")) + + def load_history(path: str | Path) -> tuple[list[Event], WorkbookMeta, QaReport]: """Parse one change-history workbook.""" path = Path(path) @@ -198,6 +278,7 @@ def load_history(path: str | Path) -> tuple[list[Event], WorkbookMeta, QaReport] meta = _read_meta(wb, path) qa = QaReport(path=path, meta=meta) + _read_warnings(wb, qa) rows = wb["History"].iter_rows(values_only=True) header = next(rows, None) @@ -231,10 +312,8 @@ def load_history(path: str | Path) -> tuple[list[Event], WorkbookMeta, QaReport] campaign = str(campaign).strip() campaigns.add(campaign) - iso = cell(row, "Date and time (ISO)") - try: - when = datetime.fromisoformat(str(iso)) - except (TypeError, ValueError): + when = parse_when(cell(row, "Date and time (ISO)"), cell(row, "Date and time")) + if when is None: qa.rows_unparsable_time += 1 continue @@ -248,6 +327,13 @@ def load_history(path: str | Path) -> tuple[list[Event], WorkbookMeta, QaReport] if in_b[0] != in_b[1]: qa.crossover_violations += 1 + for name, bucket in (("Source URL", qa.source_urls), + ("Marketplace", qa.row_marketplaces), + ("Run ID", qa.row_run_ids)): + v = cell(row, name) + if v and str(v).strip() not in bucket: + bucket.append(str(v).strip()) + date_key = when.date().isoformat() dates.add(date_key) events.append( @@ -270,6 +356,29 @@ def load_history(path: str | Path) -> tuple[list[Event], WorkbookMeta, QaReport] wb.close() + # No timestamps means no timeline, and no timeline means out-of-budget hours + # cannot be measured at all. Say exactly that, plus whatever the exporter + # already admitted, instead of a generic "could not be read". + if not events and qa.rows_unparsable_time: + detail = [ + f"{path.name}: none of the {qa.rows_unparsable_time:,} rows have a usable " + "timestamp — 'Date and time' and 'Date and time (ISO)' are both empty. " + "Without a time on each change there is no way to reconstruct when a " + "campaign was in or out of budget, so hours cannot be measured." + ] + if qa.warning_categories: + worst = ", ".join(f"{n}x {c}" for c, n in qa.warning_categories.items()) + detail.append(f"The export also recorded its own problems ({worst}), " + "so rows are missing as well.") + if not qa.row_accounting_ok and meta.rows_expected: + detail.append(f"Its row counts do not reconcile either: " + f"{meta.rows_expected:,} expected - " + f"{meta.duplicates_skipped:,} duplicates != " + f"{meta.rows_exported:,} exported.") + detail.append("Re-run the extraction in the Amazon Ads console and let it " + "finish before downloading.") + raise ValueError(" ".join(detail)) + qa.rows_parsed = len(events) + qa.rows_unparsable_time qa.distinct_campaigns = len(campaigns) qa.campaigns_with_budget_events = len( diff --git a/ppcbudget/payload.py b/ppcbudget/payload.py index e754f23..2db4fc6 100644 --- a/ppcbudget/payload.py +++ b/ppcbudget/payload.py @@ -106,6 +106,24 @@ def _quality(qas: list[QaReport], days: list[CampaignDay], totals: Totals, if m.status and m.status != "completed": add(f"Extraction status - {qa.path.name}", False, m.status, "A partial extraction can be missing whole campaigns, not just rows.") + if qa.warning_categories: + worst = ", ".join(f"{n}x {c}" for c, n in qa.warning_categories.items()) + add(f"Exporter warnings - {qa.path.name}", False, worst, + "The export recorded its own failures on an 'Errors and Warnings' sheet. " + "PARTIAL_PAGE_HARVEST means it collected fewer rows than the page reported, so " + "changes are missing and every duration here is a lower bound. Re-run the " + "extraction and let it finish. Sample: " + + " | ".join(qa.extraction_warnings[:3])) + + if qa.mixed_sources: + add(f"Mixed sources - {qa.path.name}", False, + f"{len(qa.source_urls)} consoles, {len(qa.row_marketplaces)} marketplaces", + "This one file holds rows from more than one Amazon console or marketplace: " + + ", ".join(sorted(qa.row_marketplaces)[:4]) + + ". Account-level spend and ROAS come from a single metadata block, so the " + "money figures would be attributed to the wrong marketplace. Export each " + "marketplace separately.") + if qa.crossover_violations: add("State machines crossed", False, qa.crossover_violations, "A 'Campaign status' row mixed budget and delivery vocabularies, so splitting " diff --git a/serve.py b/serve.py index 6670211..4346d83 100644 --- a/serve.py +++ b/serve.py @@ -49,8 +49,12 @@ class Session: self.lock = threading.Lock() def add(self, name: str, data: bytes, kind: str) -> Path: + # One subfolder per upload keeps path.name the user's real filename, so + # error messages and the Data Quality sheet never show a "0_" prefix. safe = Path(name).name.replace("/", "_") or "upload.xlsx" - target = self.dir / f"{len(self.history)}_{safe}" + slot = self.dir / f"{len(self.history)}{'p' if kind == 'perf' else ''}" + slot.mkdir(parents=True, exist_ok=True) + target = slot / safe target.write_bytes(data) if kind == "perf": self.perf = target @@ -119,8 +123,8 @@ class Handler(BaseHTTPRequestHandler): if path == "/api/state": self._json({ - "history": [p.name.split("_", 1)[-1] for p in SESSION.history], - "perf": SESSION.perf.name.split("_", 1)[-1] if SESSION.perf else None, + "history": [p.name for p in SESSION.history], + "perf": SESSION.perf.name if SESSION.perf else None, }) return diff --git a/tests/test_golden.py b/tests/test_golden.py index 7cac267..43f2f21 100644 --- a/tests/test_golden.py +++ b/tests/test_golden.py @@ -137,6 +137,39 @@ def test_row_accounting_explains_unscoreable_rows(): assert "UNACCOUNTED" in broken.accounting_detail +# ------------------------------------------------------ broken exports + +def test_human_readable_timestamp_is_a_valid_fallback(): + """Some extractions fill only 'Date and time', not the ISO column.""" + from ppcbudget.ingest import parse_when + from datetime import datetime + assert parse_when(None, "5 Aug 2026 23:59") == datetime(2026, 8, 5, 23, 59) + assert parse_when("", "5 Aug 2026 14:17:18.440").hour == 14 + assert parse_when("2026-08-05T14:17:18", "ignored").minute == 17 + assert parse_when("2026-08-05T14:17:18Z") is not None + # The ISO column wins when both are present and disagree. + assert parse_when("2026-08-05T01:00:00", "5 Aug 2026 23:59").hour == 1 + + +def test_no_timestamp_anywhere_is_unusable_and_says_why(): + """A timeline needs a time on every row. Failing quietly would be worse.""" + from ppcbudget.ingest import parse_when + assert parse_when(None, None) is None + assert parse_when("", "") is None + assert parse_when("None", "not a date") is None + + +def test_exporter_warnings_are_never_ignored(): + """The reference file is clean, but the reader must expose the fields the + Data Quality panel relies on.""" + qa = load()["qa"] + assert qa.warning_categories == {} + assert qa.extraction_warnings == [] + assert not qa.mixed_sources + assert len(qa.source_urls) == 1, "one export should come from one console" + assert len(qa.row_marketplaces) == 1 + + # ------------------------------------------------------- last meaningful action def test_amazon_pacing_rows_are_not_actions(): diff --git a/web/app.js b/web/app.js index 9a9de7a..301eccf 100644 --- a/web/app.js +++ b/web/app.js @@ -133,6 +133,16 @@ function render() { row.unt = row.act.unt; } + // A file that could not be read must never disappear quietly. + const skipped = d.skipped || []; + $('skipped').hidden = skipped.length === 0; + $('skipped-list').innerHTML = skipped.map((line) => { + const cut = line.indexOf(': '); + const name = cut > 0 ? line.slice(0, cut) : 'A file'; + const why = cut > 0 ? line.slice(cut + 2) : line; + return `
${escapeHtml(name)}${escapeHtml(why)}
`; + }).join(''); + $('grain').hidden = t.days < 2; renderAnswer(t, m); renderKpis(t, m); diff --git a/web/index.html b/web/index.html index cf661ce..528bd24 100644 --- a/web/index.html +++ b/web/index.html @@ -84,6 +84,11 @@