From 1417e3cbdf34a8b203a662a857f94d41971776ed Mon Sep 17 00:00:00 2001
From: Alaa-Taieb <93670187+Alaa-Taieb@users.noreply.github.com>
Date: Tue, 14 Jul 2026 19:11:11 +0100
Subject: [PATCH] fix(salary): skip non-numeric and identifier columns in Excel
 conversion (#152)

parse_sheet treated every column that was not company/city as a salary category, with no check that the column actually held numeric salary data. This turned free-text columns (e.g. Notes) into bogus string categories and numeric identifier columns (e.g. Id) into mistaken salary indexes.

- Drop identifier headers (ID_PATTERNS = {id, personnummer}) at classification time.

- Skip non-numeric standalone values and fully-null count/index pairs at row-processing time.

- Adds regression tests (skips_free_text_column, skips_numeric_identifier_column, keeps_numeric_salary_column) that fail on master and pass after the fix.
---
 tests/test_convert_salary_excel.py | 36 ++++++++++++++++++++++++++++++
 tools/convert_salary_excel.py      | 16 +++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/tests/test_convert_salary_excel.py b/tests/test_convert_salary_excel.py
index 790221b..7a2394f 100644
--- a/tests/test_convert_salary_excel.py
+++ b/tests/test_convert_salary_excel.py
@@ -104,6 +104,42 @@ class DetectColumnTypeTests(unittest.TestCase):
                     companies[0]["categories"]["salary"], {"index": 105.5}
                 )
 
+    def test_skips_free_text_column(self):
+        # A free-text "Notes" column must not become a bogus salary category.
+        ws = FakeWorksheet([
+            ("Company", "Salary Index", "Notes"),
+            ("Example Corp", 105.5, "good"),
+        ])
+
+        companies = parse_sheet(ws)
+
+        self.assertIn("salary_index", companies[0]["categories"])
+        self.assertNotIn("notes", companies[0]["categories"])
+
+    def test_skips_numeric_identifier_column(self):
+        # A numeric "Id" column (employee id) must not be treated as a salary index.
+        ws = FakeWorksheet([
+            ("Company", "Salary Index", "Id"),
+            ("Example Corp", 105.5, 7),
+        ])
+
+        companies = parse_sheet(ws)
+
+        self.assertIn("salary_index", companies[0]["categories"])
+        self.assertNotIn("id", companies[0]["categories"])
+
+    def test_keeps_numeric_salary_column(self):
+        # A genuine numeric salary column still produces a salary category.
+        ws = FakeWorksheet([
+            ("Company", "Salary Index"),
+            ("Example Corp", 105.5),
+        ])
+
+        companies = parse_sheet(ws)
+
+        self.assertIn("salary_index", companies[0]["categories"])
+        self.assertEqual(companies[0]["categories"]["salary_index"], {"index": 105.5})
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/convert_salary_excel.py b/tools/convert_salary_excel.py
index b921e92..88e8fbf 100644
--- a/tools/convert_salary_excel.py
+++ b/tools/convert_salary_excel.py
@@ -48,6 +48,10 @@ INDEX_PATTERNS = {"indeks", "index", "idx", "salary", "løn", "median", "average
 # Ships populated for this repo's Danish demonstration data; a fork targeting
 # another locale edits this constant.
 COMPOUND_PATTERNS = {"antal", "indeks", "løn", "gennemsnit", "medarbejdere"}
+# Identifier columns (employee id, Danish "personnummer", etc.) are never salary
+# 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"}
 
 
 def header_matches(header, patterns):
@@ -120,11 +124,13 @@ def parse_sheet(ws, sheet_label=None):
         print(f"Warning: Could not find company column in sheet '{ws.title}'.", file=sys.stderr)
         return []
 
-    # Identify data columns (everything that's not company/city)
+    # Identify data columns (everything that's not company/city or an identifier)
     data_cols = []
     for i, h in enumerate(headers):
         if i == company_col or i == city_col or not h:
             continue
+        if header_matches(h, ID_PATTERNS):
+            continue
         data_cols.append((i, h))
 
     # Try to detect paired count/index columns per category
@@ -205,6 +211,10 @@ def parse_sheet(ws, sheet_label=None):
                         index_val = float(row[cat["index_col"]])
                     except (ValueError, TypeError):
                         pass
+                # A count/index pair that is entirely empty for this row carries
+                # no salary information, so skip it rather than emit nulls.
+                if count_val is None and index_val is None:
+                    continue
                 entry["categories"][cat_name] = {"count": count_val, "index": index_val}
             elif "value_col" in cat:
                 if cat["value_col"] < len(row) and row[cat["value_col"]] is not None:
@@ -212,7 +222,9 @@ def parse_sheet(ws, sheet_label=None):
                     try:
                         val = float(val)
                     except (ValueError, TypeError):
-                        val = str(val)
+                        # Non-numeric standalone value (e.g. a free-text "Notes"
+                        # column) is not salary data; skip it for this row.
+                        continue
                     entry["categories"][cat_name] = {"index": val}
 
         companies.append(entry)