From 8607ac745d6bcfb3887e747d6854cea72f0f4640 Mon Sep 17 00:00:00 2001 From: Kevin Costner <120246174+kevincostner17@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:19:44 +0530 Subject: [PATCH] fix(reports): unique insight ids, working HTML filters, honest stakeholder change and completeness wording insight_report (#329): issue ids were built as issue.., so columns whose names slug the same ("a b", "a_b", "A-B") shared one id. Issue ids and the fallback recommended_action_id now go through the same seen-counter used for action ids (base, base.2, base.3, ...) in profile order. Ids for non-colliding columns are unchanged, and issues still point at the real action id when a clean report is supplied. HTML filters (#336): filterable_table derived a global function name from the table id (function fdFilter_fd-ledger), which is a JavaScript syntax error, so no filter box in any HTML report worked. The inline oninput handlers and the global function are gone; a static IIFE emitted right after the table finds its own table and filter inputs through document.currentScript and DOM siblings. Nothing in the script depends on the table id, output stays deterministic, and several reports can share a page. The committed docs/examples pages get the same markup swap. Change counting (#337): stakeholder_summary counted every action with a column as a change, and Peel's plain_step reworded informational "preserved N missing value(s)" notes (count=0) as "filled missing values". A shared changed_values() predicate (count > 0 and status not skipped or suggested) now drives the "column(s) changed meaningfully" count, the technical "Steps applied" list, and plain_step, which keeps the action's own description when nothing changed. Completeness (#339): _pct returned 100.0 for a zero-cell frame, so a run that dropped every column claimed 100% completeness. _pct now returns None for zero cells; the completeness sentence is skipped when either side is unknown, metrics["completeness"] is "n/a", and the headline says "Cleaning removed every column, ..." (or "every record" when only rows are gone). Un-materialized engine results also stop claiming a completeness figure. Closes #329 Closes #336 Closes #337 Closes #339 --- docs/examples/action_timeline.html | 2 +- docs/examples/baseline_drift.html | 2 +- docs/examples/compare_plans_grid.html | 2 +- docs/examples/profile_cockpit.html | 2 +- docs/examples/quality_debt.html | 2 +- src/freshdata/insight.py | 23 ++- src/freshdata/render/_vocabulary.py | 22 ++- src/freshdata/render/html.py | 40 +++-- src/freshdata/stakeholder.py | 39 +++-- tests/test_report_rendering_fixes.py | 205 ++++++++++++++++++++++++++ 10 files changed, 300 insertions(+), 39 deletions(-) create mode 100644 tests/test_report_rendering_fixes.py diff --git a/docs/examples/action_timeline.html b/docs/examples/action_timeline.html index fde968d6..fb787a7d 100644 --- a/docs/examples/action_timeline.html +++ b/docs/examples/action_timeline.html @@ -31,5 +31,5 @@

freshdata clean report

backend=pandas · 0.015s
205 → 200
rows
5 → 5
columns
76 → 23
missing cells
5
duplicates removed
0
outliers handled
5
actions

Action timeline

fix_dtypes signup_date — converted to datetime64[ns] (205 affected) low automatic
confidence
100%
drop_duplicates — dropped 5 duplicate row(s) (2.4% of rows, keep='first') (5 affected) low automatic
confidence
100%
missing email — preserved 23 missing value(s) (0 affected) medium automatic
why: identifier-like column — imputing keys would fabricate identities
confidence
100%
missing tier — filled 51 missing value(s) with sentinel "Missing" ('Missing') (51 affected) medium automatic
why: medium missingness without a dominant category; an explicit sentinel keeps the gap visible
confidence
70%
outliers revenue — preserved 5 outlier(s), 2.5% of values (method=iqr, factor=1.5) (0 affected) low automatic
why: protected column role or domain
confidence
90%
-

Audit ledger

⬇ JSON⬇ CSV
columnactionriskconfidencecountdescription
signup_datefix_dtypeslow100%205converted to datetime64[ns]
drop_duplicateslow100%5dropped 5 duplicate row(s) (2.4% of rows, keep='first')
emailmissingmedium100%0preserved 23 missing value(s)
tiermissingmedium70%51filled 51 missing value(s) with sentinel "Missing" ('Missing')
revenueoutlierslow90%0preserved 5 outlier(s), 2.5% of values (method=iqr, factor=1.5)
+

Audit ledger

⬇ JSON⬇ CSV
columnactionriskconfidencecountdescription
signup_datefix_dtypeslow100%205converted to datetime64[ns]
drop_duplicateslow100%5dropped 5 duplicate row(s) (2.4% of rows, keep='first')
emailmissingmedium100%0preserved 23 missing value(s)
tiermissingmedium70%51filled 51 missing value(s) with sentinel "Missing" ('Missing')
revenueoutlierslow90%0preserved 5 outlier(s), 2.5% of values (method=iqr, factor=1.5)

Needs review

  • id column 'email' has 23 missing value(s) (11.5%); check whether those rows are joinable at all
  • column 'revenue' has 5 extreme value(s) that were deliberately preserved; review them in their domain context
\ No newline at end of file diff --git a/docs/examples/baseline_drift.html b/docs/examples/baseline_drift.html index 417b8023..312a3f7c 100644 --- a/docs/examples/baseline_drift.html +++ b/docs/examples/baseline_drift.html @@ -31,5 +31,5 @@

freshdata drift report — baseline

FAIL · 1 error(s), 1 warning(s) · baseline v1.0.0
FAIL
verdict
1
errors
1
warnings

What likely matters

  • 50 new record(s) since the baseline.
-

Findings

checkcolumnstatusmessage
drift.kscustomer_idmediumKS 0.244 >= warn threshold 0.1
drift.psicustomer_idhighPSI 0.877 >= fail threshold 0.25
+

Findings

checkcolumnstatusmessage
drift.kscustomer_idmediumKS 0.244 >= warn threshold 0.1
drift.psicustomer_idhighPSI 0.877 >= fail threshold 0.25
⬇ JSON
\ No newline at end of file diff --git a/docs/examples/compare_plans_grid.html b/docs/examples/compare_plans_grid.html index 9f2d7c27..c41e10b2 100644 --- a/docs/examples/compare_plans_grid.html +++ b/docs/examples/compare_plans_grid.html @@ -29,4 +29,4 @@ .fd-del-pos{color:#1a7f37}.fd-del-neg{color:#cf222e} .fd-mono{font-family:ui-monospace,SFMono-Regular,Menlo,monospace;font-size:.8rem} -

freshdata strategy comparison

strategy diff grid
columnstrategymissing_modeloutlier_actionn_outliers
emailbalancedpreserve0
revenuebalanced5
tierbalancedsentinel0
emailaggressivepreserve0
revenueaggressive5
tieraggressivesentinel0
\ No newline at end of file +

freshdata strategy comparison

strategy diff grid
columnstrategymissing_modeloutlier_actionn_outliers
emailbalancedpreserve0
revenuebalanced5
tierbalancedsentinel0
emailaggressivepreserve0
revenueaggressive5
tieraggressivesentinel0
\ No newline at end of file diff --git a/docs/examples/profile_cockpit.html b/docs/examples/profile_cockpit.html index dccbcd27..9d8e8660 100644 --- a/docs/examples/profile_cockpit.html +++ b/docs/examples/profile_cockpit.html @@ -31,7 +31,7 @@

freshdata profile

inline quality cockpit
82/100
quality score
205
rows
5
columns
7.4%
missing
5
duplicate rows
4
issues

Type inference

  • signup_date: object → datetime64[ns]
-

Columns (issue-ranked)

columndtypemissinguniqueissues
tierobject
25%		325.4% missing
emailobject
12%		17711.7% missing
revenuefloat64
0%		1997 potential outlier(s) (iqr)
signup_dateobject
0%		200would convert to datetime64[ns]
customer_idint64
0%		200—
+

Columns (issue-ranked)

columndtypemissinguniqueissues
tierobject
25%		325.4% missing
emailobject
12%		17711.7% missing
revenuefloat64
0%		1997 potential outlier(s) (iqr)
signup_dateobject
0%		200would convert to datetime64[ns]
customer_idint64
0%		200—

Outlier warnings

revenue

Correlations

Numeric correlations are computed on demand to keep this view fast. Call profile.to_frame() or compute df.corr() when you need them.
⬇ JSON
\ No newline at end of file diff --git a/docs/examples/quality_debt.html b/docs/examples/quality_debt.html index f7bc1f7f..400abde9 100644 --- a/docs/examples/quality_debt.html +++ b/docs/examples/quality_debt.html @@ -31,5 +31,5 @@

freshdata quality-debt gate

WARN · policy warn_then_fail · total debt 1.25
WARN
WARN
status
1.25
total debt
2
over threshold
97.6
trust
-
dimensionscorethresholdseveritytrenddetail
pii_risk1.000.00medium177 potential PII column(s)
human_review_backlog0.200.00medium2 item(s) awaiting review
duplicates0.020.10low5 duplicate row(s) removed
missingness0.020.10low23 missing cell(s) remain
schema_drift0.000.00lowno baseline supplied
type_instability0.000.10low0 column(s) with unstable types
outlier_spikes0.000.10low0 outlier(s) flagged
category_churn0.000.10lowno baseline supplied
failed_repairs0.000.00low0 high-risk action(s)
+
dimensionscorethresholdseveritytrenddetail
pii_risk1.000.00medium177 potential PII column(s)
human_review_backlog0.200.00medium2 item(s) awaiting review
duplicates0.020.10low5 duplicate row(s) removed
missingness0.020.10low23 missing cell(s) remain
schema_drift0.000.00lowno baseline supplied
type_instability0.000.10low0 column(s) with unstable types
outlier_spikes0.000.10low0 outlier(s) flagged
category_churn0.000.10lowno baseline supplied
failed_repairs0.000.00low0 high-risk action(s)
⬇ JSON
\ No newline at end of file diff --git a/src/freshdata/insight.py b/src/freshdata/insight.py index db0c170e..349da20f 100644 --- a/src/freshdata/insight.py +++ b/src/freshdata/insight.py @@ -263,6 +263,11 @@ def _issues_from_profile( action_lookup: dict[tuple[str, str], str] | None = None, ) -> list[dict[str, Any]]: issues: list[dict[str, Any]] = [] + # Distinct column names can slug identically ("a b", "a_b", "A-B"); suffix + # repeats in profile order so every issue id (and fallback action id) is + # unique while non-colliding ids stay unchanged. + seen_issue_ids: dict[str, int] = {} + seen_action_ids: dict[str, int] = {} for column in profile.columns: if not column.issues: continue @@ -270,13 +275,12 @@ def _issues_from_profile( role = getattr(ctx, "role", "unknown") severity = _severity(column.issues, column.missing_pct) hint = _action_hint(column.issues, role) - action_id = (action_lookup or {}).get( - (column.name, hint), - f"action.{_slug(column.name)}.{hint}", - ) + action_id = (action_lookup or {}).get((column.name, hint)) + if action_id is None: + action_id = _unique_id(f"action.{_slug(column.name)}.{hint}", seen_action_ids) issues.append( { - "id": f"issue.{_slug(column.name)}.{hint}", + "id": _unique_id(f"issue.{_slug(column.name)}.{hint}", seen_issue_ids), "column": column.name, "severity": severity, "finding": "; ".join(column.issues), @@ -317,8 +321,7 @@ def _actions_from_clean_report( for action in report.actions: entry = CleanReport._action_dict(action) base_id = f"action.{_slug(action.column or 'table')}.{_slug(action.step)}" - seen[base_id] = seen.get(base_id, 0) + 1 - entry["id"] = base_id if seen[base_id] == 1 else f"{base_id}.{seen[base_id]}" + entry["id"] = _unique_id(base_id, seen) entry["impact"] = _action_impact(action.column, before=before, after=after) actions.append(entry) return actions @@ -665,6 +668,12 @@ def _recommended_next_step(dataset_name: str, config: CleanConfig) -> str: ) +def _unique_id(base_id: str, seen: dict[str, int]) -> str: + """Return *base_id* the first time, then ``base_id.2``, ``base_id.3``, …""" + seen[base_id] = seen.get(base_id, 0) + 1 + return base_id if seen[base_id] == 1 else f"{base_id}.{seen[base_id]}" + + def _slug(value: str) -> str: out = [] for char in str(value).lower(): diff --git a/src/freshdata/render/_vocabulary.py b/src/freshdata/render/_vocabulary.py index 7b0f09ff..72fd666b 100644 --- a/src/freshdata/render/_vocabulary.py +++ b/src/freshdata/render/_vocabulary.py @@ -34,15 +34,29 @@ } +#: Action statuses that record a decision *not* to change data. +_NOT_APPLIED = frozenset({"skipped", "suggested"}) + + +def changed_values(action: Action) -> bool: + """``True`` when *action* actually changed cells or rows. + + Informational notes (``count == 0``, e.g. "preserved 3 missing value(s)") + and actions that were only suggested or deliberately skipped are not + changes. + """ + return action.count > 0 and action.status not in _NOT_APPLIED + + def plain_step(action: Action) -> str: """A short plain-language phrase for *action*, for per-column summaries. Falls back to the action's own description — descriptions are already human sentences; the map only replaces the jargon-heavy step families. + Actions that changed nothing keep their description, so a preserved gap + is never reworded as a fill. """ phrase = _STEP_PHRASES.get(action.step) - if phrase is None: + if phrase is None or not changed_values(action): return action.description - if action.count: - return f"{phrase} ({action.count:,})" - return phrase + return f"{phrase} ({action.count:,})" diff --git a/src/freshdata/render/html.py b/src/freshdata/render/html.py index 3ead2879..8da43f04 100644 --- a/src/freshdata/render/html.py +++ b/src/freshdata/render/html.py @@ -153,7 +153,10 @@ def filterable_table( """A table with client-side text/select filters — no JS libraries needed. *filters* maps a label to the 0-based column index it filters (a free-text - box). The whole thing is self-contained vanilla JS scoped by *table_id*. + box). The script is a self-contained vanilla-JS IIFE emitted right after + the table: it finds its own table and filter boxes through + ``document.currentScript`` and DOM siblings, so no global names are + derived from *table_id* and several reports can share one page. """ tbl = table(headers, rows, raw_columns=raw_columns).replace( '', f'
', 1 @@ -162,24 +165,35 @@ def filterable_table( js = "" if filters: boxes = "".join( - f'' + f'' for label, idx in filters.items() ) controls = f'
{boxes}
' - js = ( - f"" - ) + js = f"" return f"{controls}{tbl}{js}" +#: Filter behaviour for :func:`filterable_table`. Static (no interpolation), so +#: the output stays deterministic and the script is always valid JavaScript. +_FILTER_JS = ( + "(function(){" + "var s=document.currentScript;if(!s)return;" + "var t=s.previousElementSibling;" + "if(!t||t.tagName!=='TABLE'||!t.tBodies.length)return;" + "var c=t.previousElementSibling;" + "if(!c||(' '+c.className+' ').indexOf(' fd-controls ')<0)return;" + "var inp=c.querySelectorAll('input[data-col]');" + "function run(){var rows=t.tBodies[0].rows;" + "for(var i=0;i str: """A download link that embeds *content* as a data URI (no server).""" import base64 diff --git a/src/freshdata/stakeholder.py b/src/freshdata/stakeholder.py index 9ea41263..23365b5c 100644 --- a/src/freshdata/stakeholder.py +++ b/src/freshdata/stakeholder.py @@ -14,6 +14,7 @@ from typing import TYPE_CHECKING, Any from .render import html as H +from .render._vocabulary import changed_values from .render.mixins import SimpleHtmlReport if TYPE_CHECKING: # pragma: no cover - typing only @@ -23,8 +24,9 @@ _FORMATS = ("markdown", "html") -def _pct(part: int, whole: int) -> float: - return 100.0 * (1 - part / whole) if whole else 100.0 +def _pct(part: int, whole: int) -> float | None: + """Percent of *whole* that is not *part*; ``None`` when there are no cells.""" + return 100.0 * (1 - part / whole) if whole else None @dataclass @@ -129,13 +131,16 @@ def stakeholder_summary( if format not in _FORMATS: raise ValueError(f"format must be one of {_FORMATS}, got {format!r}") + materialized = getattr(report, "materialized", True) cells_before = report.rows_before * report.cols_before cells_after = report.rows_after * report.cols_after comp_before = _pct(report.missing_before, cells_before) - comp_after = _pct(report.missing_after, cells_after) + # An un-materialized result has no computed "after" counts to measure. + comp_after = _pct(report.missing_after, cells_after) if materialized else None changed: list[str] = [] - if report.missing_before != report.missing_after: + if (report.missing_before != report.missing_after + and comp_before is not None and comp_after is not None): direction = "rose" if comp_after > comp_before else "fell" changed.append( f"Overall data completeness {direction} from {comp_before:.1f}% to " @@ -150,11 +155,12 @@ def stakeholder_summary( changed.append( f"{len(report.columns_dropped)} unusable column(s) were removed: " f"{', '.join(report.columns_dropped[:6])}.") - n_changed_cols = len({a.column for a in report.actions if a.column}) + n_changed_cols = len( + {a.column for a in report.actions if a.column and changed_values(a)}) if n_changed_cols: changed.append(f"{n_changed_cols} column(s) changed meaningfully.") if audience == "technical": - steps = sorted({a.step for a in report.actions if a.count}) + steps = sorted({a.step for a in report.actions if changed_values(a)}) if steps: changed.append("Steps applied: " + ", ".join(steps) + ".") @@ -172,13 +178,26 @@ def stakeholder_summary( review: list[str] = list(report.warnings) + list(report.recommendations) - headline = ( - f"Cleaning kept {comp_after:.1f}% of fields complete across " - f"{report.rows_after:,} record(s); {len(review)} item(s) need review.") + n_review = f"{len(review)} item(s) need review." + if not materialized: + headline = ( + "Cleaning kept the result in the engine, so completeness was not " + f"computed; {n_review}") + elif comp_after is not None: + headline = ( + f"Cleaning kept {comp_after:.1f}% of fields complete across " + f"{report.rows_after:,} record(s); {n_review}") + elif report.cols_after == 0: + headline = ( + "Cleaning removed every column, leaving no fields to measure across " + f"{report.rows_after:,} record(s); {n_review}") + else: + headline = ( + f"Cleaning removed every record, leaving no fields to measure; {n_review}") metrics = { "records": f"{report.rows_after:,}", - "completeness": f"{comp_after:.1f}%", + "completeness": "n/a" if comp_after is None else f"{comp_after:.1f}%", "duplicates removed": f"{report.duplicates_removed:,}", "needs review": len(review), } diff --git a/tests/test_report_rendering_fixes.py b/tests/test_report_rendering_fixes.py new file mode 100644 index 00000000..793456d8 --- /dev/null +++ b/tests/test_report_rendering_fixes.py @@ -0,0 +1,205 @@ +"""Regression tests for report-rendering fixes (#329, #336, #337, #339).""" + +from __future__ import annotations + +import re +import shutil +import subprocess + +import pandas as pd +import pytest + +import freshdata as fd +from freshdata.render import html as H +from freshdata.render._vocabulary import changed_values, plain_step +from freshdata.render.normalize import normalize_clean_report +from freshdata.report import Action, CleanReport + +_SCRIPT_RE = re.compile(r"", re.S) + + +# -- #329: insight issue ids are unique --------------------------------------- + + +def test_insight_issue_ids_unique_when_column_slugs_collide() -> None: + df = pd.DataFrame({"a b": [1, None] * 20, "a_b": [1, None] * 20, "A-B": ["x", None] * 20}) + rep = fd.insight_report(df) + pairs = [(i["id"], i["column"]) for i in rep.issues] + assert pairs == [ + ("issue.a_b.missing", "a b"), + ("issue.a_b.missing.2", "a_b"), + ("issue.a_b.missing.3", "A-B"), + ] + action_ids = [i["recommended_action_id"] for i in rep.issues] + assert action_ids == [ + "action.a_b.missing", + "action.a_b.missing.2", + "action.a_b.missing.3", + ] + + +def test_insight_ids_unchanged_for_non_colliding_columns() -> None: + df = pd.DataFrame({"age": [1, None] * 20, "City Name": ["x", None] * 20}) + ids = sorted(i["id"] for i in fd.insight_report(df).issues) + assert ids == ["issue.age.missing", "issue.city_name.missing"] + + +def test_insight_recommended_action_ids_point_at_real_actions() -> None: + df = pd.DataFrame({"a b": [1.0, None] * 20, "a_b": [2.0, None] * 20, "keep": range(40)}) + cleaned, report = fd.clean(df, return_report=True, verbose=False) + rep = fd.insight_report(df, clean_report=report, cleaned_df=cleaned) + issue_ids = [i["id"] for i in rep.issues] + assert len(issue_ids) == len(set(issue_ids)) + action_ids = {a["id"] for a in rep.actions} + by_column = {a["column"]: a["id"] for a in rep.actions if a["step"] == "missing"} + for issue in rep.issues: + if issue["column"] in by_column: + assert issue["recommended_action_id"] == by_column[issue["column"]] + assert issue["recommended_action_id"] in action_ids + + +# -- #336: HTML filter script is valid and id-free ----------------------------- + + +def _clean_html() -> str: + df = pd.DataFrame({"name": [" a", "b ", None, "b "], "v": [1.0, None, 3.0, 3.0]}) + _, rep = fd.clean(df, return_report=True, verbose=False) + return rep.to_html() + + +def test_filterable_table_has_no_inline_handlers_or_global_functions() -> None: + out = H.filterable_table("fd-ledger", ["a", "b"], [["1", "2"]], filters={"a": 0, "b": 1}) + assert "oninput=" not in out + assert "fdFilter_" not in out + assert "document.currentScript" in out + scripts = _SCRIPT_RE.findall(out) + assert len(scripts) == 1 + assert "fd-ledger" not in scripts[0] + # controls, then table, then the script: the DOM-sibling lookup relies on it. + assert out.index('class="fd-controls"') < out.index("") + assert out.endswith("
" + f"") + + +def test_filterable_table_output_is_deterministic() -> None: + args = ("fd-x", ["h"], [["v"]]) + assert H.filterable_table(*args, filters={"h": 0}) == H.filterable_table( + *args, filters={"h": 0} + ) + assert "