Carry secondary practice locations on the provider (address + phones)

[brianmacy]

What

The CMS practice-location file (pl_pfile) lists additional locations per NPI. The mapper wrote each as a separate, nameless NPI-LOCATIONS record. A nameless record cannot get a name key, a name+place key or a category key, so the providers were invisible at those 1.2M addresses (810,348 NPIs).

Now each location's address is carried on the provider as an extra ADDR_TYPE: SECONDARY address (deduplicated against the provider's own, placed after them) and its telephone and fax numbers are carried as PHONE_NUMBER features (a fax keeps PHONE_TYPE: FAX). The nameless NPI-LOCATIONS records are written only with --locationRecords.

Behaviour change

Default output changes: no NPI-LOCATIONS records (the output file is created and empty). Anyone relying on those records, or on their Secondary Location relationship, needs --locationRecords.

Verified on the 2026-09 dissemination

Output compared record by record with the previous run: 9,443,429 providers in both; 735,071 changed, all by additions only (1,111,747 secondary addresses, 852,163 phones, 308,029 faxes); officials (1,972,058) and affiliations (49,670) identical; the 4,594 deactivated NPIs that had locations now carry them on their NPI_DEACTIVE record.

Prototype on a VA + NV subset (325K providers): category search by primary taxonomy code + a secondary-location ZIP finds the provider 245 of 246 times (0 before); a secondary-location phone number alone finds the provider 113 of 150 times (the misses are numbers shared by 13 to 3,348 records).

Tests

tests/test_secondary_locations.py (3 tests): address and phones carried, deduplicated, location records only with the flag. pylint 10/10.

[github-actions]

🤖 Claude Code Review

PR Code Review: secondary practice locations carried on the provider

I read the diff and checked add_phone, the imports, and the __main__ guard in src/npi_mapper.py. I did not run the tests.

Code Quality

• ❌ Style conventions
  • src/npi_mapper.py:62 puts import re ahead of import csv. The imports are not alphabetical, but they weren't before either, so this is minor.
  • src/npi_mapper.py:79 adds a stray blank line (two blank lines before a comment block).
  • The map_locations return line is about 130 characters. Reformat with black or prettier to match the rest of the file.
• ✅ No commented-out code. The if False: dead block was removed.
• ❌ Meaningful names
  • map_locations(inNPI, inName, inType) no longer uses inName or inType, and neither does the call site. Drop them.
  • The "phone": rsltRecord["PH1"] entries are fine.
• ✅ DRY. Address and phone handling is split into helpers. One small repeat remains: write_location_record and map_locations both build the address dict. They differ only in ADDR_TYPE, so this is tolerable.
• ❌ Defects
  1. Dedupe is on street + ZIP5 only. address_identity drops ADDR_LINE2, so two suites in the same building (Ste 2 and Ste 300) collapse into one. That may be intended, but it should be documented. It also hides distinct locations such as separate departments.
  2. No dedupe among location phones. add_phone already dedupes by (type, number), so this is fine. However, updateStat is called only when add_phone returns a truthy value, which is correct.
  3. Missing-ZIP behavior. If the ZIP is empty, the identity becomes "LINE1|". It then matches only another address with a blank ZIP. A provider address with a ZIP and a location without one (same street) will not dedupe. This is an edge case.
  4. Different ADDR_LINE1 spellings. Normalization strips punctuation but not suffix variants (STREET vs ST). The test only covers punctuation and case. Acceptable, but it undercuts the claim "not when it repeats an address the provider already has".
  5. EMIT_LOCATION_RECORDS is set inside if __name__ == "__main__":. That works because it is a module global. If the mapper is ever driven from another entry point, the flag stays at the default. This is fine, but it is coupled to global state.
  6. map_locations now returns data and also writes records conditionally. One function does two jobs (a side-effecting write plus a return value). Consider having map_npi call write_location_record itself.
  7. ADDR_TYPE: SECONDARY is a new value. Confirm it is registered or tolerated in the Senzing config. npi_config_updates.g2c may need an entry.
  8. Behavior change. NPI-LOCATIONS records, previously written by default, are now off. Anyone with downstream REL_POINTER consumers of that data source loses them silently. The CHANGELOG notes this under "Changed", which is good. It might merit a "Breaking" label.
• ✅ Project .claude/CLAUDE.md. Not touched by this PR.

Testing

• ✅ Unit tests for new functions. address_identity, secondary_addresses, and the map_npi integration path are covered.
• ✅ Integration. map_npi is tested end to end against an in-memory SQLite database and a real NPPES fixture row.
• ❌ Edge cases. The following are missing:
  • A location with no street line (address is None).
  • A blank ZIP.
  • ADDR2 == "NONE".
  • No pl_pfile rows at all.
  • Two locations that differ only in ADDR_LINE2.
• ⚠️ Coverage > 80%. Not measurable from the diff. The new code paths look well covered, apart from the edge cases above.
• ⚠️ test_location_records_only_with_the_flag mutates a module global. The try/finally restores it, which is good.
• ⚠️ The 1-row CSV fixture is one very wide header line (about 300 columns). It is valid test data, but hard to review.

Documentation

• ✅ README. The new "Secondary practice locations" section is added.
• ⚠️ API docs. There are none. Docstrings are present and clear.
• ✅ Inline comments. The comments explain why the flag defaults to off and why the SECONDARY addresses come after the provider's own.
• ✅ CHANGELOG. The "Added" and "Changed" entries are present. The --locationRecords flag is mentioned in the "Changed" entry.
• ⚠️ Markdown formatting. The README and CHANGELOG lines wrap at about 120 characters, but nearby entries wrap at about 100. Run prettier to confirm. The CHANGELOG indentation of the continuation lines is inconsistent with the neighboring bullets (some indent two spaces, the new ones don't).

Security

• ✅ No hardcoded credentials. There are no .lic files and no AQAAAD strings.
• ✅ Input validation. The SQL uses a parameterized query ((str(inNPI),)). Address strings are normalized with regex before comparison.
• ⚠️ Error handling. There is none for malformed rows, such as None values in the rsltRecord dict. address_identity coerces with str(), so it won't crash.
• ✅ No sensitive data in logs. updateStat records only address lines, phone numbers, and similar fields. The fixture contains public NPPES data.

Summary

The change is sound and well tested for the main path. Before merging, I'd address these:

1. Remove the unused inName and inType parameters.
2. Add edge-case tests (no street, blank ZIP, differing ADDR_LINE2).
3. Document that dedupe ignores ADDR_LINE2.
4. Confirm that SECONDARY is acceptable in the Senzing config.
5. Fix the formatting nits (blank line, long line, markdown wrapping).

---

Automated code review analyzing defects and coding standards