From eea08ecc15839f5a4883a6718442e874102f456f Mon Sep 17 00:00:00 2001
From: Shivam Lalakiya <50960482+shivamlalakiya@users.noreply.github.com>
Date: Fri, 25 Sep 2026 17:28:04 -0500
Subject: [PATCH] Fix silent column collisions and accounting-style negatives
 in ingest

map_columns let two source columns map to the same target name and
silently produced duplicate-named output columns that could still pass
a required= check. It now raises a ValueError naming the colliding
sources.

_to_amount, shared by the CiviCRM, Raiser's Edge and NPSP readers, was
parsing accounting-style negatives like "($50.00)" as positive because
it stripped parentheses before the digits without checking for the
minus sign they imply. It now converts a parenthesised amount to a
leading minus sign first.
---
 CHANGELOG.md                        |  9 +++++++++
 philanthropy/ingest/_civicrm.py     |  7 ++++++-
 philanthropy/ingest/_map_columns.py | 16 +++++++++++++++-
 tests/test_civicrm.py               | 20 ++++++++++++++++++++
 tests/test_map_columns.py           |  7 +++++++
 5 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ea9fe81..82bb455 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,15 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
   `MajorGiftClassifier`, validated with a fiscal-year walk-forward split and
   compared against a naive "gave $500+ last FY" rule on top-N upgrade rate.
 
+### Fixed
+- `philanthropy.ingest.map_columns` now raises a `ValueError` naming the
+  colliding source columns when a mapping sends two different source columns
+  to the same target name, instead of silently producing a duplicate-named
+  output column that could still pass a `required=` check.
+- `philanthropy.ingest._civicrm._to_amount` (shared by the CiviCRM, Raiser's
+  Edge and NPSP readers) now treats an accounting-style parenthesised amount
+  like `"($50.00)"` as negative instead of dropping the sign.
+
 ## [0.8.0] - 2026-09-24
 
 The first release with a Raiser's Edge on-ramp and `as_of` scoring cutoffs on the
diff --git a/philanthropy/ingest/_civicrm.py b/philanthropy/ingest/_civicrm.py
index c1b72a6..832904e 100644
--- a/philanthropy/ingest/_civicrm.py
+++ b/philanthropy/ingest/_civicrm.py
@@ -359,7 +359,12 @@ def _to_amount(series: pd.Series) -> pd.Series:
     """
     if pd.api.types.is_numeric_dtype(series):
         return pd.to_numeric(series, errors="coerce")
-    cleaned = series.astype("string").str.replace(r"[^\d.\-]", "", regex=True)
+    # Accounting notation wraps a negative in parens, e.g. "($50.00)"; turn it
+    # into a leading minus sign before stripping everything else, or it reads
+    # as a plain positive.
+    text = series.astype("string").str.strip()
+    text = text.str.replace(r"^\((.*)\)$", r"-\1", regex=True)
+    cleaned = text.str.replace(r"[^\d.\-]", "", regex=True)
     return pd.to_numeric(cleaned, errors="coerce")
 
 
diff --git a/philanthropy/ingest/_map_columns.py b/philanthropy/ingest/_map_columns.py
index eb0cccc..01496ab 100644
--- a/philanthropy/ingest/_map_columns.py
+++ b/philanthropy/ingest/_map_columns.py
@@ -70,7 +70,21 @@ def map_columns(
         ...
     ValueError: missing required column(s) after mapping: amount
     """
-    renamed = df.rename(columns=dict(mapping))
+    mapping = dict(mapping)
+    targets: "dict[str, list[str]]" = {}
+    for source, target in mapping.items():
+        if source in df.columns:
+            targets.setdefault(target, []).append(source)
+    collisions = {target: sources for target, sources in targets.items() if len(sources) > 1}
+    if collisions:
+        detail = "; ".join(
+            f"{target!r} <- {sources}" for target, sources in collisions.items()
+        )
+        raise ValueError(
+            "mapping assigns multiple source columns to the same target: " + detail
+        )
+
+    renamed = df.rename(columns=mapping)
     missing = [col for col in required if col not in renamed.columns]
     if missing:
         raise ValueError(
diff --git a/tests/test_civicrm.py b/tests/test_civicrm.py
index ec26ce2..47106a4 100644
--- a/tests/test_civicrm.py
+++ b/tests/test_civicrm.py
@@ -561,3 +561,23 @@ def test_features_feed_donor_propensity_model():
     scores = model.predict_affinity_score(feats[cols].to_numpy())
     assert scores.shape == (len(feats),)
     assert np.isfinite(scores).all()
+
+
+# --------------------------------------------------------------------------- #
+# _to_amount (shared by CiviCRM, Raiser's Edge and NPSP readers)
+# --------------------------------------------------------------------------- #
+@pytest.mark.parametrize(
+    "raw, expected",
+    [
+        ("($50.00)", -50.0),
+        ("(50)", -50.0),
+        ("-25.50", -25.50),
+        ("$1,000.00", 1000.0),
+    ],
+)
+def test_to_amount_parses_signed_and_formatted_values(raw, expected):
+    from philanthropy.ingest._civicrm import _to_amount
+
+    out = _to_amount(pd.Series([raw]))
+
+    assert out.iloc[0] == pytest.approx(expected)
diff --git a/tests/test_map_columns.py b/tests/test_map_columns.py
index 737d936..db3db65 100644
--- a/tests/test_map_columns.py
+++ b/tests/test_map_columns.py
@@ -62,3 +62,10 @@ def test_does_not_mutate_input_dataframe():
     map_columns(df, {"CnID": "contact_id"})
 
     assert list(df.columns) == ["CnID"]
+
+
+def test_raises_on_collision_naming_the_colliding_sources():
+    df = pd.DataFrame({"A": [1], "B": [2]})
+
+    with pytest.raises(ValueError, match="A.*B|B.*A"):
+        map_columns(df, {"A": "x", "B": "x"}, required=["x"])