[aschroed]

Intent

Validate the compatibility fix on the existing ProtectedDonor implementation in #339: submitr passes allow_existing_staging_path when it supplies an owned staging workbook, so dcicutils CustomExcel must accept that option, replace the staged output safely, and initialize its workbook state early enough that construction failures do not trigger an AttributeError in Excel.del. Preserve the existing PR339 behavior and ensure the focused ProtectedDonor tests pass.

What Changed

• Added dcicutils/submitr/donor_transformer.py (ProtectedDonorWorkbookTransformer, analyze_protected_donors). It converts Donor references to ProtectedDonor in the Demographic, DeathCircumstances, FamilyHistory, MedicalHistory, and TissueCollection sheets, and adds the matching ProtectedDonor rows. It only scans the logical rows the reader ingests, resolves UUID/accession identifiers through the portal, and raises ProtectedDonorLookupError for permission or lookup failures. Tests are in test/test_protected_donor_transform.py.
• CustomExcel now accepts three opt-in options: transform_protected_donor, transformed_workbook_path, and allow_existing_staging_path. CustomExcel.with_portal(portal, **options) passes them through. Workbook state is set before Excel.__init__ runs, so a failed construction no longer raises AttributeError in Excel.__del__. The transformed workbook is saved atomically (temp file plus os.replace) and never overwrites the input workbook. An existing output path is replaced only when allow_existing_staging_path=True.
• StructuredDataSet now opens an Excel workbook once for both the progress-count pass and the parse pass. It also applies the specified schema order to multi-schema JSON files and guards a unique typeinfo lookup. Other changes:
  • Removed unused nonlocal/global declarations flagged by pyflakes F824.
  • Updated pytest warning filters and moved the last marker from pytest.ini into pyproject.toml.
  • CI now retries the Redis apt install.
  • Made the jh_utils and deployment_utils tests less sensitive to the environment.
  • Bumped the version to 8.19.1 and updated the CHANGELOG, docs, and poetry.lock.

Risk Assessment

✅ Low: The fix is small and does what the intent asks: CustomExcel accepts and forwards allow_existing_staging_path, sets _workbook before super().init so Excel.del is safe, and saves atomically with temp-file cleanup; the main gap is that the regression tests covering this were removed earlier and not restored.

Testing

I ran the focused ProtectedDonor and CustomExcel tests, adding three regression tests that fail before the fix and pass after it. They cover replacing an owned staging path, keeping the original file and cleaning up when a save fails, and construction failures no longer triggering an AttributeError in __del__. I also ran a script that mimics submitr: it parses a workbook through StructuredDataSet with a pre-created staging file and confirms the transformed output, that the flag-less path still rejects the existing file, and that no exceptions are raised in __del__. Everything passed. There is no UI surface, so the evidence is a CLI transcript.

Evidence: submitr-style staging replacement transcript

staging file pre-exists: True size 0 parsed Demographic.donor: A_PROTECTED-DONOR_1 staged sheets: ['Donor', 'Demographic', 'ProtectedDonor'] staged ProtectedDonor ids: ['A_PROTECTED-DONOR_1'] dir contents: no temp leftovers without allow flag -> ValueError: True bad kwarg -> TypeError: Excel.init() got an unexpected keyword argument 'bogus_option' unraisable exceptions in del: []

  import pkg_resources
staging file pre-exists: True size 0
parsed Demographic.donor: A_PROTECTED-DONOR_1
staged sheets: ['Donor', 'Demographic', 'ProtectedDonor']
staged ProtectedDonor ids: ['A_PROTECTED-DONOR_1']
dir contents: no temp leftovers
without allow flag -> ValueError: True
bad kwarg -> TypeError: Excel.__init__() got an unexpected keyword argument 'bogus_option'
unraisable exceptions in __del__: []

Evidence: Demo script

import os, sys, gc, tempfile, openpyxl
from dcicutils.structured_data import StructuredDataSet
from dcicutils.submitr.custom_excel import CustomExcel

d = tempfile.mkdtemp()
inp = os.path.join(d, "submission.xlsx")
wb = openpyxl.Workbook(); wb.remove(wb.active)
s = wb.create_sheet("Donor"); s.append(["submitted_id"]); s.append(["A_DONOR_1"])
s = wb.create_sheet("Demographic"); s.append(["donor"]); s.append(["A_DONOR_1"])
wb.save(inp)
# submitr pre-creates an owned staging file (e.g. via mkstemp), then passes it in
fd, staged = tempfile.mkstemp(dir=d, suffix=".xlsx"); os.close(fd)
print("staging file pre-exists:", os.path.exists(staged), "size", os.path.getsize(staged))

class SubmitrExcel(CustomExcel):
    def __init__(self, *a, **k):
        super().__init__(*a, transform_protected_donor=True, transformed_workbook_path=staged,
                         allow_existing_staging_path=True, **k)

data = StructuredDataSet(file=inp, portal=None, norefs=True, excel_class=SubmitrExcel).data
print("parsed Demographic.donor:", data["Demographic"][0]["donor"])
out = openpyxl.load_workbook(staged)
print("staged sheets:", out.sheetnames)
print("staged ProtectedDonor ids:", [r[0] for r in out["ProtectedDonor"].iter_rows(min_row=2, values_only=True)])
print("dir contents:", sorted(os.listdir(d)) == sorted(["submission.xlsx", os.path.basename(staged)]) and "no temp leftovers")

try:
    CustomExcel(file=inp, transform_protected_donor=True, transformed_workbook_path=staged)
except ValueError as e:
    print("without allow flag -> ValueError:", "already exists" in str(e))

seen = []; sys.unraisablehook = seen.append
try:
    CustomExcel(file=inp, bogus_option=1)
except TypeError as e:
    print("bad kwarg -> TypeError:", e)
gc.collect()
print("unraisable exceptions in __del__:", [repr(x.exc_value) for x in seen])

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 3 issues (1 warning, 2 infos)

• ⚠️ dcicutils/submitr/custom_excel.py:107 - The compatibility fix has no regression tests. Commit e883d1c removed test_save_transformed_workbook_allows_owned_staging_path and test_failed_save_does_not_damage_existing_target, and 93d1927 brings back allow_existing_staging_path and the temp-file/os.replace save without bringing those tests back. Nothing in test/test_protected_donor_transform.py passes allow_existing_staging_path=True. Nothing checks that a failed save leaves the existing target alone and deletes the temp file. Nothing checks that a construction failure (for example, a TypeError from a bad argument to Excel.init, or a missing file) no longer raises AttributeError in Excel.del. The intent names all three behaviors. Add behavioral tests: (1) a placeholder pre-created by submitr is replaced with the transformed workbook when allow_existing_staging_path=True, and the call still raises 'already exists' without the flag; (2) when workbook.save is monkeypatched to raise, the target bytes are unchanged and no .<name>.*.xlsx temp file is left behind; (3) CustomExcel(transform_protected_donor=True) with a bad file argument raises the original error, and garbage collection raises no AttributeError (check with pytest's unraisable-exception warning or sys.unraisablehook).
• ℹ️ dcicutils/submitr/custom_excel.py:120 - With allow_existing_staging_path=True, the staging path is only written when the transform actually changes the workbook (if transformed and ...). If submitr pre-creates an empty or placeholder staging file and the workbook has no Donor references to convert, the placeholder is left as is. CustomExcel also exposes no attribute that tells the caller whether a transform happened. If submitr uploads or reads the staging path without checking, it could use the placeholder instead of the original workbook. Confirm that submitr handles the 'nothing transformed' case, for example by falling back to the input file when the staging file is unchanged, or decide whether CustomExcel should expose the transformed flag.
• ℹ️ dcicutils/submitr/custom_excel.py:159 - tempfile.mkstemp creates the file with mode 0600, and os.replace keeps that mode. The staged workbook therefore ends up owner-only instead of following the umask, as a direct workbook.save would. This is fine for a submitr-owned temp staging file, but it differs from the earlier direct-save behavior if another process or user reads the output.

✅ **Test** - passed

✅ No issues found.

• NO_SERVER_FIXTURES=TRUE USE_SAMPLE_ENVUTILS=TRUE poetry run pytest test/test_protected_donor_transform.py test/test_custom_excel.py -q (49 passed, including the new tests)
• New test test_owned_staging_path_is_replaced_when_existing_path_is_allowed: a stale existing staged workbook is replaced with the transformed ProtectedDonor workbook and no temp files remain
• New test test_failed_save_to_owned_staging_path_keeps_existing_file_and_leaves_no_temporary: a save that fails partway leaves the original staged bytes intact and removes the temp file
• New test test_construction_failure_does_not_raise_attribute_error_in_excel_del: an unsupported kwarg raises TypeError with no unraisable AttributeError from Excel.__del__
• Regression check: replaced custom_excel.py with the pre-fix version (HEAD~1) and reran; all 3 new tests failed, including the reproduced 'CustomExcel' object has no attribute '_workbook' error, then restored the file
• Existing PR339 test test_repeated_construction_still_cannot_clobber_the_staged_path still passes, so overwriting is still refused without the flag
• End-to-end script submitr_staging_demo.py: mimics submitr by pre-creating a staging file with mkstemp and passing allow_existing_staging_path=True through a StructuredDataSet excel_class

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

[coveralls]

Coverage Report for CI Build 38962637325

Coverage increased (+0.9%) to 75.532%

Details

• Coverage increased (+0.9%) from the base build.
• Patch coverage: 29 uncovered changes across 2 files (391 of 420 lines covered, 93.1%).
• 3 coverage regressions across 3 files.

Uncovered Changes

File	Changed	Covered	%
dcicutils/submitr/donor_transformer.py	367	343	93.46%
dcicutils/submitr/custom_excel.py	42	37	88.1%
Total (3 files)	420	391	93.1%

Coverage Regressions

3 previously-covered lines in 3 files lost coverage.

File	Lines Losing Coverage	Coverage
dcicutils/command_utils.py	1	58.57%
dcicutils/progress_bar.py	1	74.61%
dcicutils/structured_data.py	1	71.96%

---

Coverage Stats

Relevant Lines:	16025
Covered Lines:	12104
Line Coverage:	75.53%
Coverage Strength:	0.76 hits per line

---

💛 - Coveralls