Add read_gifts as one entry point over the gift bridge presets - #243
Merged
Merged
Conversation
CiviCRM, Raiser's Edge and NPSP each ship their own read_<source>_...
loader and <source>_..._to_features aggregator pair. That's fine when a
caller only ever works with one CRM's export, but anyone supporting more
than one CRM (a consultant with clients on different systems, a tool that
takes "whichever export you have") ends up importing three differently
named function pairs to do the same read-then-aggregate operation.
read_gifts(path_or_df, source=...) looks that pair up in a small registry
(GIFT_SOURCES) instead, and runs it: read_gifts("gifts.csv",
source="npsp") is exactly equivalent to
npsp_opportunities_to_features(read_npsp_opportunities("gifts.csv")).
Extra keyword arguments pass straight through to the matched aggregator,
so exclude_stages, exclude_gift_types and statuses all still work.
Kept the aggregator itself where it already lives in _civicrm.py rather
than moving it to a new neutral module: it's the lower-risk option, and
"extract" is satisfied by exposing all three presets uniformly through
one function instead of relocating logic that raisers_edge and npsp
already import cleanly. cli.py's own three-way --source dispatch is left
untouched since it does its own per-step error handling that read_gifts
would have to either duplicate or change.
Tested with make ci (2148 passed, 8 skipped, 98.58% coverage) and make
riskcov (98% over the risk tier, floor 93%). No existing test file
changed.
# Conflicts: # docs/reference/index.md # philanthropy/ingest/__init__.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CiviCRM, Raiser's Edge and NPSP each ship their own
read_<source>_...loader and<source>_..._to_featuresaggregator pair. That's fine for a caller who only ever works with one CRM's export, but a caller working with more than one CRM export format (a consultant with clients on different systems, a tool that takes "whichever export you have") ends up importing three differently named function pairs to do the same read-then-aggregate operation.read_gifts(path_or_df, *, source=...)looks that pair up in a small registry (GIFT_SOURCES = ("civicrm", "raisers_edge", "npsp")) instead, and runs it:It also accepts an already-in-memory DataFrame or iterable of mappings (skips the read step), and passes any extra keyword arguments straight through to the matched aggregator, so
exclude_stages,exclude_gift_typesandstatusesall still work per-source.Design choice: kept
civicrm_contributions_to_featureswhere it already lives in_civicrm.py, rather than physically relocating it into a new neutral module. Raiser's Edge and NPSP already import it cleanly as their shared aggregator; the "extraction" this PR delivers is uniform access through one function name rather than three, which is the lower-risk, smaller change and needed no edits to_civicrm.py,_raisers_edge.py, or_npsp.py.cli.py's existing three-way--sourcedispatch in_cmd_featuresis left untouched: it does its own per-step error handling (a separatetryaround the read and the aggregate step, each raising a different CLI-friendly message) that routing throughread_giftswould have to either duplicate or change, and this PR's job is the library function, not a CLI refactor.Test plan
python -c "from philanthropy.ingest import read_gifts, GIFT_SOURCES; print('OK')"before writing teststests/test_read_gifts.py: one test per preset showingread_gifts(path, source=X)matches the two-step reader+aggregator call, a DataFrame-input test, an iterable-of-mappings test, an unknown-sourceValueErrortest, and a kwarg-passthrough test (exclude_stages=Nonereaching the NPSP aggregator)tests/test_civicrm.py,tests/test_raisers_edge.py,tests/test_npsp.py,tests/test_cli.pyall pass unmodifiedmake ci: 2148 passed, 8 skipped, 98.58% coverage (floor 92%)make riskcov: 98% coverage over the risk tier (floor 93%)CHANGELOG.mdentry under[Unreleased] -> Addeddocs/reference/index.mdTier 2 (Beta) row forread_gifts/GIFT_SOURCES