fix(salary): split combined name-and-city workbook columns (#660)

* fix(salary): split combined name-and-city workbook columns

The IDA demonstration workbook heads its company column "Firmanavn og by"
and stores "Ørsted A/S, Fredericia" in that one cell. After the header is
detected, convert_salary_excel copied the whole cell into company with
city "". salary_lookup --city only reads entry["city"], so the documented
lookup for Ørsted in Fredericia missed.

When the company header also names a city and there is no city column,
split the cell on the last ", ". A Company-only header still keeps commas
in the name.

* fix(salary): split a combined name-and-city cell on the last comma

Real IDA cells have no space after the separating comma
("Ørsted A/S,Fredericia"), and some company names contain ", "
themselves ("Rambøll Danmark A/S, Odense,Odense C"). Splitting on the
last ", " missed almost every real row, so --city still dropped out.

split_combined_company_city now splits on the last "," and strips both
parts. Test fixtures use the real unspaced cell shape, and a new unit
test covers a company name that contains a comma. The docstrings,
CHANGELOG entry, tools/README_SALARY_TOOL.md and the README salary
section describe the new split.

* docs(changelog): move the combined-city entry under Unreleased

The merge of master brought in the 1.7.3 release section, which left
this PR's Fixed entry under an already released version. Move it to
the Unreleased Fixed list.

* chore: drop the CHANGELOG hunk, the file is frozen since #800

Release notes are generated from PR titles now.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Co-authored-by: Mads Lorentzen <madslorentzen17@gmail.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

README.md

@@ -318,6 +318,8 @@ Market-specific *data sources* (a national salary database, local award-rate tab
 
 The salary tool works with any salary data you provide (union statistics, Glassdoor exports, personal research, etc.). See `tools/README_SALARY_TOOL.md` for the expected format and setup. If you don't have salary data, the salary step is simply skipped.
 
+`tools/convert_salary_excel.py` turns an Excel export into that format. When the company column also holds the city, as in the IDA workbook's "Firmanavn og by" column (`Ørsted A/S,Fredericia`), the converter splits each cell on its last comma and strips both parts, so `salary_lookup.py --city` can filter. A company name that itself contains ", " stays intact.
+
 ### Starting over
 
 To wipe your profile data and start fresh:

tests/test_convert_salary_excel.py

@@ -173,13 +173,60 @@ class DetectColumnTypeTests(unittest.TestCase):
         self.assertTrue(header_matches("Firmanavn og by", COMPANY_PATTERNS))
         ws = FakeWorksheet([
             ("Firmanavn og by", "Antal", "Indeks"),
-            ("Example A/S, Aarhus", 12, 105.0),
+            ("Example A/S,Aarhus", 12, 105.0),
         ])
         companies = parse_sheet(ws)
         self.assertEqual(len(companies), 1)
-        self.assertEqual(companies[0]["company"], "Example A/S, Aarhus")
+        self.assertEqual(companies[0]["company"], "Example A/S")
+        self.assertEqual(companies[0]["city"], "Aarhus")
         self.assertEqual(companies[0]["categories"]["all_employees"], {"count": 12, "index": 105.0})
 
+    def test_parse_sheet_does_not_split_a_comma_in_a_company_only_column(self):
+        # A "Company" header is not a combined name-and-city column. "Acme, Inc."
+        # stays the company; splitting it would invent a city named "Inc.".
+        ws = FakeWorksheet([
+            ("Company", "Antal", "Indeks"),
+            ("Acme, Inc.", 12, 105.0),
+        ])
+        companies = parse_sheet(ws)
+        self.assertEqual(len(companies), 1)
+        self.assertEqual(companies[0]["company"], "Acme, Inc.")
+        self.assertEqual(companies[0]["city"], "")
+
+    def test_parse_sheet_splits_english_combined_company_and_city_header(self):
+        ws = FakeWorksheet([
+            ("Company and City", "Count", "Index"),
+            ("Ørsted A/S,Kongens Lyngby", 200, 105.2),
+        ])
+        companies = parse_sheet(ws)
+        self.assertEqual(len(companies), 1)
+        self.assertEqual(companies[0]["company"], "Ørsted A/S")
+        self.assertEqual(companies[0]["city"], "Kongens Lyngby")
+
+    def test_parse_sheet_splits_on_the_last_comma_when_the_company_contains_one(self):
+        # IDA cells have no space after the separating comma. A company can
+        # itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C").
+        ws = FakeWorksheet([
+            ("Firmanavn og by", "Antal", "Indeks"),
+            ("Rambøll Danmark A/S, Odense,Odense C", 12, 105.0),
+            ("Ørsted A/S,Fredericia", 200, 105.2),
+        ])
+        companies = parse_sheet(ws)
+        self.assertEqual(
+            [(c["company"], c["city"]) for c in companies],
+            [("Rambøll Danmark A/S, Odense", "Odense C"), ("Ørsted A/S", "Fredericia")],
+        )
+
+    def test_parse_sheet_keeps_a_combined_cell_with_no_comma_as_company_only(self):
+        ws = FakeWorksheet([
+            ("Firmanavn og by", "Antal", "Indeks"),
+            ("Example A/S", 12, 105.0),
+        ])
+        companies = parse_sheet(ws)
+        self.assertEqual(len(companies), 1)
+        self.assertEqual(companies[0]["company"], "Example A/S")
+        self.assertEqual(companies[0]["city"], "")
+
     def test_parse_sheet_detects_city_column_with_token_header(self):
         # City headers are matched with the same token-based header_matches()
         # used for the company column, not exact string equality. Real-world

tests/test_salary_combined_city_cli.py

@@ -0,0 +1,80 @@
+"""A combined name-and-city workbook column must feed salary_lookup --city.
+
+The IDA demonstration workbook heads its company column "Firmanavn og by"
+and stores "Ørsted A/S,Fredericia" in that one cell (no space after the
+comma). convert_salary_excel used to copy the whole cell into company with
+city "". The documented
+lookup `python3 salary_lookup.py "Ørsted" --city "Fredericia"` then missed,
+even though the city was in the converted file - just not in the field
+--city reads.
+"""
+import json
+import shutil
+import subprocess
+import sys
+import unittest
+from pathlib import Path
+from tempfile import TemporaryDirectory
+
+try:
+    import openpyxl
+except ImportError:
+    openpyxl = None
+
+REPO = Path(__file__).resolve().parent.parent
+
+
+@unittest.skipUnless(openpyxl, "requires optional openpyxl (installed in CI)")
+class CombinedCityWorkbookCLI(unittest.TestCase):
+    def convert_and_lookup(self, headers, row, *lookup_args):
+        with TemporaryDirectory() as directory:
+            root = Path(directory)
+            workbook = root / "salary.xlsx"
+            output = root / "salary_data.json"
+            book = openpyxl.Workbook()
+            try:
+                book.active.append(headers)
+                book.active.append(row)
+                book.save(workbook)
+            finally:
+                book.close()
+            conversion = subprocess.run(
+                [sys.executable, str(REPO / "tools/convert_salary_excel.py"),
+                 str(workbook), "--output", str(output)],
+                capture_output=True, text=True, encoding="utf-8",
+            )
+            self.assertEqual(conversion.returncode, 0, conversion.stderr)
+            companies = json.loads(output.read_text(encoding="utf-8"))["companies"]
+            shutil.copyfile(REPO / "salary_lookup.py", root / "salary_lookup.py")
+            lookup = subprocess.run(
+                [sys.executable, str(root / "salary_lookup.py"), *lookup_args],
+                capture_output=True, text=True, encoding="utf-8",
+            )
+            return companies, lookup
+
+    def test_ida_firmanavn_og_by_row_is_findable_with_city_filter(self):
+        companies, lookup = self.convert_and_lookup(
+            ["Firmanavn og by", "Antal", "Indeks"],
+            ["Ørsted A/S,Fredericia", 200, 105.2],
+            "Ørsted", "--city", "Fredericia",
+        )
+        self.assertEqual(companies, [{
+            "company": "Ørsted A/S",
+            "city": "Fredericia",
+            "categories": {"all_employees": {"count": 200, "index": 105.2}},
+        }])
+        self.assertEqual(lookup.returncode, 0, lookup.stdout + lookup.stderr)
+        self.assertIn("Ørsted A/S", lookup.stdout)
+        self.assertIn("Fredericia", lookup.stdout)
+        self.assertNotIn("No results found", lookup.stdout)
+
+    def test_separate_city_column_is_unchanged(self):
+        companies, lookup = self.convert_and_lookup(
+            ["Company", "City", "Count", "Index"],
+            ["Novo Nordisk A/S", "Bagsværd", 500, 108.5],
+            "Novo Nordisk", "--city", "Bagsværd",
+        )
+        self.assertEqual(companies[0]["company"], "Novo Nordisk A/S")
+        self.assertEqual(companies[0]["city"], "Bagsværd")
+        self.assertEqual(lookup.returncode, 0, lookup.stdout + lookup.stderr)
+        self.assertIn("Novo Nordisk A/S", lookup.stdout)

tools/README_SALARY_TOOL.md

@@ -79,6 +79,7 @@ On Windows, use `py` if that is how Python is exposed on your PATH. If your syst
 
 The converter auto-detects the Excel layout:
 - Looks for a "Company"/"Firma" column and an optional "City"/"By" column
+- A combined header such as "Firmanavn og by" or "Company and City" (no separate city column) is split on the last comma, with both parts stripped, so `salary_lookup.py --city` can filter. Cells look like "Ørsted A/S,Fredericia"; a company name may itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C")
 - Treats remaining columns as salary data (auto-pairs count/index columns)
 
 ### Option C: Build from research

tools/convert_salary_excel.py

@@ -18,7 +18,9 @@ The output file (salary_data.json) will be written to the repository root.
 Expected Excel format:
     - A header row with column names
     - A "Company" or "Firma" column (required)
-    - An optional "City" or "By" column
+    - An optional "City" or "By" column. A combined header such as
+      "Firmanavn og by" / "Company and City" (no separate city column)
+      is split on the last comma, and both parts are stripped, into company and city.
     - Any number of numeric data columns (salary index, count, etc.)
 
 The script auto-detects the header row and column layout. For Excel files
@@ -97,6 +99,28 @@ def parse_numeric_cell(value):
     return _finite_float(text)
 
 
+def split_combined_company_city(value):
+    """Split a combined name-and-city cell on its last comma.
+
+    The IDA demonstration workbook heads its company column "Firmanavn og by"
+    and stores "Ørsted A/S,Fredericia" (no space after the comma). salary_lookup's
+    --city filter only reads entry["city"], so leaving the city in the company
+    string makes the documented
+    `python3 salary_lookup.py "Ørsted" --city "Fredericia"` miss. Split on the
+    last comma and strip both parts: a city may contain spaces ("Kongens Lyngby")
+    and a company may contain ", " ("Rambøll Danmark A/S, Odense,Odense C"
+    keeps "Rambøll Danmark A/S, Odense"). A cell with no comma stays the company
+    with an empty city.
+    """
+    if not isinstance(value, str) or "," not in value:
+        return value, ""
+    company, city = value.rsplit(",", 1)
+    company, city = company.strip(), city.strip()
+    if company and city:
+        return company, city
+    return value, ""
+
+
 def header_matches(header, patterns):
     """Return True when a header contains a meaningful pattern match.
 
@@ -224,6 +248,12 @@ def parse_sheet(ws, sheet_label=None):
         print(f"Warning: Could not find company column in sheet '{ws.title}'.", file=sys.stderr)
         return []
 
+    # "Firmanavn og by" / "Company and City": one column holds both, and there
+    # is no separate city column. Split the cell so --city can see the city.
+    combined_name_and_city = city_col is None and header_matches(
+        headers[company_col], CITY_PATTERNS
+    )
+
     # Identify data columns (everything that's not company/city or an identifier)
     data_cols = []
     for i, h in enumerate(headers):
@@ -327,6 +357,8 @@ def parse_sheet(ws, sheet_label=None):
             city_name = str(row[city_col]).strip()
         else:
             city_name = ""
+        if combined_name_and_city and not city_name:
+            company_name, city_name = split_combined_company_city(company_name)
 
         entry = {
             "company": company_name,