From 968fb1bd7ecbc0b0200d88062418beadb0e3fd9c Mon Sep 17 00:00:00 2001 From: Ayobami Adegoke Date: Wed, 16 Sep 2026 05:47:45 +0100 Subject: [PATCH] fix(salary): pair a bare Count/Index column pair instead of splitting it into two categories (#470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 its 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. Any suffix ("Antal alle") made pairing work, which is why the shipped tests, all suffixed, never saw it. Pair on equal derived names, empty included, and give the nameless pair the README's top-level category name (all_employees). A bare "Antal" with no bare index column still stays a standalone count; named pairs alongside are untouched. Four new cases in test_convert_salary_excel.py, one of them rendering the converter's output through salary_lookup.format_entry; all four fail against the old pairing rule. --- CHANGELOG.md | 18 +++++++ tests/test_convert_salary_excel.py | 79 ++++++++++++++++++++++++++++++ tools/convert_salary_excel.py | 16 ++++-- 3 files changed, 110 insertions(+), 3 deletions(-) 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,