From 7f709eda57184dff22981a95192286976b8623c0 Mon Sep 17 00:00:00 2001 From: OluwaJomiloju Date: Thu, 3 Sep 2026 07:19:08 +0100 Subject: [PATCH] fix(salary): require corroboration before accepting a header row (#415) * fix(salary): require corroboration before accepting a header row Header-row detection accepted the first row (of the first 10) where any cell merely contained a company-pattern word - no check that the row actually looked like a header. A source-citation row above the real header table (standard in real Danish union/statistics exports, e.g. "Kilde: ... opdelt efter arbejdsgiver ...") tripped it purely because "arbejdsgiver" appeared in prose. The real header row then parsed as data (its "Firma" cell became a bogus company), and every genuine company silently lost all its salary data - exit 0, no warning. A candidate row is now only accepted when a second cell also matches a city/count/index pattern, and a sheet that ends up with zero detected salary columns prints a warning instead of reporting success silently. Fixes #414. * fix(salary): require cross-cell corroboration, fall back for untyped columns Two edge cases found in review of the corroboration fix: - Same-cell corroboration wasn't enough: a citation sentence can pack a count-pattern word into the same sentence as the company-pattern one ("...opdelt efter arbejdsgiver, antal svar 1234"), which still passed the gate. Corroboration must now come from a different cell. - The corroboration requirement itself broke sheets whose only real header has purely untyped salary columns (e.g. "Base pay 2025" / "Bonus 2025" - neither matches a known city/count/index pattern), so header detection found nothing at all. Falls back to the original any-cell-mentions-company rule when the strict pass finds no row in the first 10. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- CHANGELOG.md | 21 ++++++++ tests/test_convert_salary_excel.py | 81 ++++++++++++++++++++++++++++++ tools/convert_salary_excel.py | 52 ++++++++++++++++--- 3 files changed, 148 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff6acd..f291de8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,27 @@ per-file diff commands. ### Fixed +- **`convert_salary_excel.py` no longer mistakes a title/citation row for the header row** + (#414) - header-row detection accepted the first row in the first 10 where *any* cell merely + contained a company-pattern word, with no check that the row actually looked like a header. A + source-citation line above the real header table - standard in real Danish union/statistics + exports, e.g. "Kilde: ... opdelt efter arbejdsgiver ..." - tripped it purely because + "arbejdsgiver" (employer) appeared in prose. The real header row then got parsed as a data row + (its "Firma" cell became a bogus company entry), and every genuine company lost all its salary + data, silently: exit 0, "Done! Wrote N company entries," with `categories: {}` on every one. A + candidate row is now accepted only when a *different* cell in the same row also matches a + city/count/index pattern - same-cell corroboration doesn't count, since a citation sentence can + pack a count-pattern word into the same sentence as the company-pattern one (e.g. "...opdelt + efter arbejdsgiver, antal svar 1234"). Sheets whose only real header has purely untyped salary + columns (e.g. "Base pay 2025" / "Bonus 2025", neither of which matches a known city/count/index + pattern) have nothing to corroborate against in any row, so detection falls back to the original + any-cell-mentions-company rule when the strict pass finds nothing in the first 10 rows. As a + backstop independent of either pass, a sheet that ends up with zero detected salary columns now + prints a warning instead of reporting success silently. Pinned by four cases in + `tests/test_convert_salary_excel.py`: the original citation-row and zero-columns cases fail + against the pre-fix script; the same-cell-corroboration and untyped-column-fallback cases each + fail against the single-pass version of this fix that came before the fallback was added. + - **`jobbank-search` no longer dies over one malformed feed date** (#416) - `new Date()` on a present-but-unparseable `pubDate` yields an Invalid Date whose `toISOString()` throws `RangeError`, and `normalizeSearchItem` runs inside an unguarded `items.map()`, diff --git a/tests/test_convert_salary_excel.py b/tests/test_convert_salary_excel.py index 665825e..1571114 100644 --- a/tests/test_convert_salary_excel.py +++ b/tests/test_convert_salary_excel.py @@ -1,4 +1,6 @@ +import io import unittest +from contextlib import redirect_stderr from types import SimpleNamespace from tools.convert_salary_excel import ( @@ -286,6 +288,85 @@ class DetectColumnTypeTests(unittest.TestCase): self.assertEqual(categories["a"], {"count": 10, "index": 100.0}) self.assertEqual(categories["b"], {"count": 20, "index": 200.0}) + def test_parse_sheet_ignores_citation_row_mentioning_company_pattern_word(self): + # A title/source-citation row above the real header - standard in + # real Danish union/statistics exports - can contain a stray + # company-pattern word ("arbejdsgiver" = employer) in running prose. + # It must not be mistaken for the header: that misreads the real + # header row as data (producing a bogus "Firma" company) and drops + # every real company's salary data (issue #414). + ws = FakeWorksheet([ + ("Lønstatistik 2025",), + ("Kilde: Medlemsundersøgelse opdelt efter arbejdsgiver og branche",), + (), + ("Firma", "By", "Antal alle", "Lønindeks alle"), + ("Novo Nordisk A/S", "Bagsværd", 500, 108.5), + ("Ørsted A/S", "Fredericia", 200, 105.2), + ]) + + companies = parse_sheet(ws) + + self.assertEqual(len(companies), 2) + self.assertEqual(companies[0]["company"], "Novo Nordisk A/S") + self.assertEqual(companies[0]["city"], "Bagsværd") + self.assertEqual(companies[0]["categories"]["alle"], {"count": 500, "index": 108.5}) + self.assertEqual(companies[1]["company"], "Ørsted A/S") + + def test_parse_sheet_rejects_citation_row_with_count_word_in_same_cell(self): + # Corroboration must come from a DIFFERENT cell than the company + # match. A single free-text sentence can pack both a company-pattern + # word and a count-pattern word together (e.g. "... opdelt efter + # arbejdsgiver, antal svar 1234") - same-cell corroboration must not + # be enough, or this citation row reintroduces the bogus-header bug. + ws = FakeWorksheet([ + ("Lønstatistik 2025",), + ("Kilde: undersøgelse opdelt efter arbejdsgiver, antal svar 1234",), + (), + ("Firma", "By", "Antal alle", "Lønindeks alle"), + ("Novo Nordisk A/S", "Bagsværd", 500, 108.5), + ]) + + companies = parse_sheet(ws) + + self.assertEqual(len(companies), 1) + self.assertEqual(companies[0]["company"], "Novo Nordisk A/S") + self.assertEqual(companies[0]["categories"]["alle"], {"count": 500, "index": 108.5}) + + def test_parse_sheet_falls_back_when_no_row_has_cross_cell_corroboration(self): + # A header with only untyped salary columns (no header matches a + # known city/count/index pattern - "Base pay"/"Bonus" don't) has + # nothing to corroborate against in any row. The strict cross-cell + # check must fall back to the original any-cell-mentions-company + # rule rather than failing to find a header at all. + ws = FakeWorksheet([ + ("Company", "Base pay 2025", "Bonus 2025"), + ("Example Corp", 55000, 5000), + ]) + + companies = parse_sheet(ws) + + self.assertEqual(len(companies), 1) + self.assertEqual(companies[0]["company"], "Example Corp") + self.assertEqual(companies[0]["categories"]["base_pay_2025"], {"index": 55000.0}) + self.assertEqual(companies[0]["categories"]["bonus_2025"], {"index": 5000.0}) + + def test_parse_sheet_warns_when_no_salary_columns_detected(self): + # A header row with only company/city columns and no salary data + # is a strong signal something is wrong (a misdetected header row, + # or a sheet with no salary data at all) - it should be flagged, + # not silently reported as a successful conversion. + ws = FakeWorksheet([ + ("Company", "City"), + ("Example Corp", "Aarhus"), + ]) + + stderr = io.StringIO() + with redirect_stderr(stderr): + companies = parse_sheet(ws) + + self.assertEqual(companies[0]["categories"], {}) + self.assertIn("No salary data columns detected", stderr.getvalue()) + if __name__ == "__main__": unittest.main() diff --git a/tools/convert_salary_excel.py b/tools/convert_salary_excel.py index 2e8b6bb..3771058 100644 --- a/tools/convert_salary_excel.py +++ b/tools/convert_salary_excel.py @@ -127,15 +127,47 @@ def detect_column_type(header): def parse_sheet(ws, sheet_label=None): """Parse a single worksheet into a list of company entries and detected categories.""" - # Find header row + # Find header row. Two passes: + # + # Strict pass: a candidate row needs a company-pattern cell AND a + # DIFFERENT cell matching a city/count/index pattern. Corroboration must + # come from a separate cell - a single free-text sentence can pack both + # a company-pattern word and a count-pattern word together (e.g. "... + # opdelt efter arbejdsgiver, antal svar 1234"), and that must not read + # as a header any more than a citation mentioning just one of them does. + # A real header row always has these as separate columns. + # + # Fallback pass: some real headers have no recognizable city/count/index + # column at all (e.g. "Company | Base pay 2025 | Bonus 2025" - neither + # data header matches a known pattern, so they're picked up later as + # untyped/standalone categories). Nothing can corroborate a company match + # there, so if the strict pass finds no row in the first 10, fall back to + # the original any-cell-mentions-company rule. + rows = list(ws.iter_rows(min_row=1, max_row=10, values_only=False)) + + def _cell_texts(row): + return [str(cell.value).strip() for cell in row if cell.value] + header_row = None - for row_idx, row in enumerate(ws.iter_rows(min_row=1, max_row=10, values_only=False), start=1): - for cell in row: - if cell.value and header_matches(str(cell.value), COMPANY_PATTERNS): + for row_idx, row in enumerate(rows, start=1): + cell_texts = _cell_texts(row) + company_idxs = {i for i, t in enumerate(cell_texts) if header_matches(t, COMPANY_PATTERNS)} + if not company_idxs: + continue + other_idxs = { + i + for i, t in enumerate(cell_texts) + if header_matches(t, CITY_PATTERNS) or header_matches(t, COUNT_PATTERNS) or header_matches(t, INDEX_PATTERNS) + } + if other_idxs - company_idxs: + header_row = row_idx + break + + if header_row is None: + for row_idx, row in enumerate(rows, start=1): + if any(header_matches(t, COMPANY_PATTERNS) for t in _cell_texts(row)): header_row = row_idx break - if header_row: - break if header_row is None: print(f"Warning: Could not find header row in sheet '{ws.title}'. Skipping.", file=sys.stderr) @@ -222,6 +254,14 @@ def parse_sheet(ws, sheet_label=None): for col_idx, col_header in untyped_cols: categories.append({"name": col_header.lower().replace(" ", "_"), "value_col": col_idx}) + if not categories: + print( + f"Warning: No salary data columns detected in sheet '{ws.title}' " + "(only a company/city column was found) - the header row may be " + "wrong, or this sheet has no salary data.", + file=sys.stderr, + ) + # Parse data rows companies = [] for row in ws.iter_rows(min_row=header_row + 1, values_only=True):