diff --git a/CHANGELOG.md b/CHANGELOG.md index 40909fa..bfd2ece 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,24 @@ per-file diff commands. ### Fixed +- **`convert_salary_excel.py` pairs a bare `Count`/`Index` column pair instead of + splitting it, so `salary_lookup.py` no longer labels a published headcount as + privacy-suppressed** - the pairing loop required a non-empty derived category name on + both sides, but a header with no category word (`Count` + `Index`, Danish `Antal` + + `Lønindeks`) strips to an empty name, so the simplest layout the README advertises + ("auto-pairs count/index columns") came out as two unrelated standalone categories: + `{"count": {"count": 500}, "index": {"index": 108.5}}`. `salary_lookup` then rendered + a `Count 500 N/A*` row above an `Index - 108.5` row, and the footnote read the + `N/A*` as "too few employees to publish (privacy)" - a false statement about a company + whose headcount is in the file, shown during `/apply`'s salary step. Demonstrated + through the documented Excel -> JSON -> lookup path with `openpyxl`; adding any suffix + (`Antal alle`) made pairing work, which is why the shipped tests, all suffixed, never + saw it. Bare pairs now pair under the README's top-level category name + (`all_employees`); a bare `Antal` with no bare index column still stays a standalone + count, and named pairs alongside are untouched. Four new cases in + `test_convert_salary_excel.py`, including one that renders the converter's output + through `salary_lookup.format_entry`; all four fail on the old pairing rule. + - **`tools/verify_layout.py`'s `skipped:` message named only one cause of a broken extractor when there are two** (#451) - it blamed the xpdf-based `pdftotext` Git for Windows puts ahead of Poppler in PATH (no `-bbox` flag, exits 99), but a real Poppler diff --git a/tests/test_convert_salary_excel.py b/tests/test_convert_salary_excel.py index 1571114..0812257 100644 --- a/tests/test_convert_salary_excel.py +++ b/tests/test_convert_salary_excel.py @@ -3,6 +3,7 @@ import unittest from contextlib import redirect_stderr from types import SimpleNamespace +from salary_lookup import format_entry from tools.convert_salary_excel import ( INDEX_PATTERNS, detect_column_type, @@ -391,6 +392,84 @@ class ParseNumericCellLocaleTests(unittest.TestCase): self.assertEqual(parse_numeric_cell("1.234.567,89"), 1234567.89) +class BareCountIndexPairingTests(unittest.TestCase): + """A count/index pair whose headers carry no category word is still a pair. + + "Count" + "Index" (Danish "Antal" + "Lønindeks") both strip to an empty + category name, and the pairing loop used to require a non-empty name on + both sides, so the single-category layout the README describes as + "auto-pairs count/index columns" came out as two unrelated standalone + columns. salary_lookup then rendered the count row with "N/A*" for the + index - "too few employees to publish (privacy)" - about a company whose + headcount was right there in the file. The literal name is asserted (not + the module constant) so the cases run, and fail, against the old converter. + """ + + DEFAULT_CATEGORY = "all_employees" + + def test_bare_english_pair_is_paired_under_the_default_category(self): + ws = FakeWorksheet([ + ("Company", "City", "Count", "Index"), + ("Acme Corp", "Copenhagen", 500, 108.5), + ]) + + companies = parse_sheet(ws) + + self.assertEqual( + companies[0]["categories"], + {self.DEFAULT_CATEGORY: {"count": 500, "index": 108.5}}, + ) + + def test_bare_danish_pair_is_paired_under_the_default_category(self): + ws = FakeWorksheet([ + ("Firma", "By", "Antal", "Lønindeks"), + ("Acme Corp", "Aarhus", 500, 108.5), + ]) + + companies = parse_sheet(ws) + + self.assertEqual( + companies[0]["categories"], + {self.DEFAULT_CATEGORY: {"count": 500, "index": 108.5}}, + ) + + def test_bare_pair_does_not_cross_pair_with_a_named_category(self): + # The bare pair and the named pair coexist; neither steals the other's + # column, and a lone "Antal" with no bare index column stays standalone + # (pinned separately by test_standalone_count_column_is_stored_as_count_not_index). + ws = FakeWorksheet([ + ("Company", "Antal", "IT Count", "IT Index", "Lønindeks"), + ("Acme Corp", 500, 30, 112.0, 108.5), + ]) + + companies = parse_sheet(ws) + + self.assertEqual( + companies[0]["categories"], + { + self.DEFAULT_CATEGORY: {"count": 500, "index": 108.5}, + "it": {"count": 30, "index": 112.0}, + }, + ) + + def test_lookup_renders_the_bare_pair_as_one_row_without_the_privacy_footnote_firing(self): + # End to end through the documented path: converter output is what + # salary_lookup.format_entry displays during /apply. Before the fix the + # same sheet produced a "Count 500 N/A*" row plus an "Index - 108.5" + # row - the N/A* asserting a privacy suppression that never happened. + ws = FakeWorksheet([ + ("Company", "City", "Count", "Index"), + ("Acme Corp", "Copenhagen", 500, 108.5), + ]) + entry = parse_sheet(ws)[0] + + rendered = format_entry(entry, {"index_baseline": 100, "index_label": "Index"}) + + self.assertNotIn("N/A*", rendered.split("* N/A =")[0]) + self.assertRegex(rendered, r"All Employees\s+500\s+108\.5\s+\+8\.5%") + self.assertNotRegex(rendered, r"^\s*Count\s+500", ) + + class CompoundCategoryPairingTests(unittest.TestCase): def test_parse_sheet_pairs_danish_compound_index_with_count(self): # "Lønindeks alle" is *detected* as an index column via diff --git a/tools/convert_salary_excel.py b/tools/convert_salary_excel.py index 3771058..24c7186 100644 --- a/tools/convert_salary_excel.py +++ b/tools/convert_salary_excel.py @@ -52,6 +52,9 @@ COMPOUND_PATTERNS = {"antal", "indeks", "løn", "gennemsnit", "medarbejdere"} # data. They are dropped at classification so they are not mistaken for a salary # category. Matched as whole tokens only, like other pattern sets. ID_PATTERNS = {"id", "personnummer"} +# Category name for a count/index pair whose headers carry no category word at +# all ("Count" + "Index"). Matches the top-level category in README_SALARY_TOOL.md. +DEFAULT_CATEGORY = "all_employees" def parse_numeric_cell(value): @@ -216,7 +219,14 @@ def parse_sheet(ws, sheet_label=None): else: untyped_cols.append((col_idx, col_header)) - # Pair count/index columns by matching category name + # Pair count/index columns by matching category name. A bare "Count" / + # "Index" pair (Danish "Antal" / "Lønindeks") strips to an empty name on + # both sides - the single-category layout the README's "auto-pairs + # count/index columns" line describes. It is still one pair, so it gets + # the README's default category name instead of being emitted as two + # unrelated standalone columns: salary_lookup renders that split as a + # count row whose index reads "N/A*", i.e. "too few employees to publish + # (privacy)", about a company with a published headcount. categories = [] used_counts = set() used_indexes = set() @@ -225,8 +235,8 @@ def parse_sheet(ws, sheet_label=None): for ii, (i_idx, i_header, i_cat) in enumerate(index_cols): if ii in used_indexes: continue - if c_cat and i_cat and c_cat == i_cat: - cat_name = c_cat.replace(" ", "_").replace("-", "_") + if c_cat == i_cat: + cat_name = (c_cat or DEFAULT_CATEGORY).replace(" ", "_").replace("-", "_") categories.append({ "name": cat_name, "count_col": c_idx,