From 58a4ec046801fa3ec35e6ee68856c6c540ae8040 Mon Sep 17 00:00:00 2001 From: Ayobami Adegoke Date: Wed, 21 Oct 2026 06:12:41 +0100 Subject: [PATCH] fix(salary): never take a qualified salary header as the company column (#700) * fix(salary): never take a qualified salary header as the company column The detection loop kept the LAST header matching a company pattern, so "Company Name | City | Company Count | Company Index" made the index column the company column and every converted row carried "company": "108.5"; IDA's "Firmanavn og by | Antal | Firmaets indeks" (#615) did the same. #571's exact-match preference only helped when a header was exactly "Company" or "Firma". Detection is now three tiers, each taking the first match: an exact pattern header, then a pattern header that is not a count/index column, then any pattern header; the city column can never alias the company column. Four new cases; the two qualified-header ones fail on master. * changelog: place the #700 entry under [Unreleased] Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Mads Lorentzen Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 13 ++++++ tests/test_convert_salary_excel.py | 75 ++++++++++++++++++++++++++++++ tools/convert_salary_excel.py | 34 ++++++++------ 3 files changed, 109 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd75315b..3458f6c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -354,6 +354,19 @@ per-file diff commands. present on the machine (bash, zsh, sh, dash), with a missing-folder case and a guard that no bashism returns. The zsh subtests fail on master. +- **`convert_salary_excel.py` no longer takes a qualified salary header as the company + column** (#700, `tools/convert_salary_excel.py`, `tests/test_convert_salary_excel.py`) - the + detection loop kept the *last* header matching a company pattern, so + `Company Name | City | Company Count | Company Index` made the index column the company + column and every converted row carried `"company": "108.5"`; a Danish header in the same + shape (`Firmanavn og by | Antal | Firmaets indeks`, constructed - the IDA workbook heads its + pairs `Antal` and `Indeks` and already converts) did the same. #571's exact-match preference + only helped when a header was exactly `Company` or `Firma`. Detection is now three tiers, + each taking the first match: an exact pattern header, then a pattern header that is not a + count/index column, then any pattern header; the city lookup skips the company column, so a + combined name-and-city header before a real city column no longer drops the city. Five new + cases; the two qualified-header ones fail on master. + ### Security - **CI no longer leaves the job token in `.git/config` or expands a PR-controlled name into a diff --git a/tests/test_convert_salary_excel.py b/tests/test_convert_salary_excel.py index 8096a4f3..7fb653fb 100644 --- a/tests/test_convert_salary_excel.py +++ b/tests/test_convert_salary_excel.py @@ -542,6 +542,81 @@ class BareCountIndexPairingTests(unittest.TestCase): self.assertNotRegex(rendered, r"^\s*Count\s+500", ) +class QualifiedCompanyHeaderNextToQualifiedSalaryHeader(unittest.TestCase): + """A name header must win over a salary header that also carries the word. + + The detection loop kept the LAST header matching a company pattern, so + "Company Name | City | Company Count | Company Index" made the index + column the company column and every row's company read "108.5". #571 + added an exact-match preference, which only helps when a header is + exactly "Company"; the qualified name header stayed exposed. The Danish + case below is a constructed header in the same shape, not the IDA + workbook, whose pairs are headed "Antal" and "Indeks" and already convert. + """ + + def test_company_name_wins_over_company_index(self): + ws = FakeWorksheet([ + ("Company Name", "City", "Company Count", "Company Index"), + ("Acme Corp", "Berlin", 25, 108.5), + ]) + + entry = parse_sheet(ws)[0] + + self.assertEqual(entry["company"], "Acme Corp") + self.assertEqual(entry["city"], "Berlin") + self.assertEqual(entry["categories"], {"company": {"count": 25, "index": 108.5}}) + + def test_danish_name_header_wins_over_qualified_index_header(self): + # The cell carries no comma so the case tests which column wins and + # does not depend on how a combined name-and-city cell is split (#660). + ws = FakeWorksheet([ + ("Firmanavn og by", "Antal", "Firmaets indeks"), + ("Acme A/S", 25, 108.5), + ]) + + entry = parse_sheet(ws)[0] + + self.assertEqual(entry["company"], "Acme A/S") + self.assertEqual(sum(1 for c in entry["categories"].values() if c.get("index") == 108.5), 1) + self.assertEqual(sum(1 for c in entry["categories"].values() if c.get("count") == 25), 1) + + def test_exact_header_still_wins_over_a_qualified_name_header(self): + # Tier 1 (#571) is unchanged: "Company" beats "Company Name" wherever it sits. + ws = FakeWorksheet([ + ("Company Name", "Company", "Salary Index"), + ("Acme Corp (legal)", "Acme", 108.5), + ]) + + self.assertEqual(parse_sheet(ws)[0]["company"], "Acme") + + def test_city_column_never_aliases_the_company_column(self): + # "Firmanavn og by" carries both a company and a city pattern word. + ws = FakeWorksheet([ + ("Firmanavn og by", "Antal", "Indeks"), + ("Acme A/S", 25, 108.5), + ]) + + entry = parse_sheet(ws)[0] + + self.assertEqual(entry["company"], "Acme A/S") + self.assertEqual(entry["city"], "") + self.assertEqual(entry["categories"], {"all_employees": {"count": 25, "index": 108.5}}) + + def test_combined_header_before_a_real_city_column_keeps_the_city(self): + # The city lookup must skip the company column: with the combined + # header first, the first city-pattern match used to land on it and + # was then cleared, dropping a real city column that followed. + for headers, row, city in ( + (("Company and city", "City name", "Count", "Index"), ("Acme Corp", "Berlin", 25, 108.5), "Berlin"), + (("Firmanavn og by", "City name", "Antal", "Indeks"), ("Acme A/S", "Aarhus", 25, 108.5), "Aarhus"), + ): + with self.subTest(headers=headers): + entry = parse_sheet(FakeWorksheet([headers, row]))[0] + self.assertEqual(entry["company"], row[0]) + self.assertEqual(entry["city"], city) + self.assertEqual(entry["categories"], {"all_employees": {"count": 25, "index": 108.5}}) + + 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 728f427c..85d1c7c8 100644 --- a/tools/convert_salary_excel.py +++ b/tools/convert_salary_excel.py @@ -197,20 +197,28 @@ def parse_sheet(ws, sheet_label=None): for cell in ws[header_row]: headers.append(" ".join(str(cell.value).split()) if cell.value else "") - # Find company and city columns - company_col = None - city_col = None - for i, h in enumerate(headers): - if header_matches(h, COMPANY_PATTERNS): - company_col = i - elif header_matches(h, CITY_PATTERNS): - city_col = i + # Find company and city columns. Three tiers, each taking the FIRST match: + # 1. an exact pattern header ("Company", "Firma", "By"); + # 2. a header that carries the pattern but is not a count/index column + # ("Company Name", "Firmanavn og by"); + # 3. any header carrying the pattern. + # The old loop kept the LAST pattern match, so "Company Name | ... | + # Company Index" made the salary index the company column: every row's + # company read "108.5". #571 added tier 1; tiers 2-3 cover a qualified + # name header that sits next to a qualified salary header. The city + # lookup skips the company column: a combined "Company and city" header + # carries both words, and must not shadow a real city column after it. + def metadata_column(patterns, skip=None): + usable = [(i, h) for i, h in enumerate(headers) if i != skip] + exact = next((i for i, h in usable if h.lower().strip() in patterns), None) + if exact is not None: + return exact + candidates = [i for i, h in usable if header_matches(h, patterns)] + plain = [i for i in candidates if detect_column_type(headers[i]) is None] + return (plain or candidates or [None])[0] - # A qualified salary header such as "Company Index" may also contain a - # metadata token. Prefer an unqualified metadata column when one exists, - # leaving the qualified column available as salary data. - company_col = next((i for i, h in enumerate(headers) if h.lower() in COMPANY_PATTERNS), company_col) - city_col = next((i for i, h in enumerate(headers) if h.lower() in CITY_PATTERNS), city_col) + company_col = metadata_column(COMPANY_PATTERNS) + city_col = metadata_column(CITY_PATTERNS, skip=company_col) if company_col is None: print(f"Warning: Could not find company column in sheet '{ws.title}'.", file=sys.stderr)