Conversation
The lock refresh in 4c2d59d floated flake8 7.1 -> 7.3 (pyflakes 3.2 -> 3.4), which added F824. These declarations were for names only read, never assigned, in the inner scope, so removing them does not change behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…extension, typed lookups, single Excel open - Scan, rewrite, and append only within the reader's logical rows (before the first empty row). - Extend an existing ProtectedDonor sheet's headers for copied fields, or reject unsafe shapes before mutating. - Resolve non-token references (UUID/accession) as ProtectedDonors via workbook identifiers or typed portal lookup. - Report permission/outage lookup failures distinctly from definitive absence. - Open the Excel workbook once in StructuredDataSet so the progress counting pass cannot pre-create the staged output.
Coverage Report for CI Build 37038186504Coverage increased (+0.9%) to 75.501%Details
Uncovered Changes
Coverage Regressions3 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
Member
Author
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
2 participants
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.
Intent
Implement the approved fixes for the six review concerns on #339: preserve copied fields when an existing ProtectedDonor sheet lacks destination headers; keep generated rows visible by matching the reader's logical empty-row termination; accept supported UUID/accession identifiers by resolving identity and type rather than using submitted-ID spelling; scan references only within the same logical rows used for ingestion; distinguish permission, outage, and other lookup failures from definitive ProtectedDonor absence; and prevent default CustomExcel counting-pass saves from creating an output that breaks the parsing pass. Add regression tests for each behavior and validate the changes.
What Changed
dcicutils/submitr/donor_transformer.pywithanalyze_protected_donorsandProtectedDonorWorkbookTransformer. They convert plainDonorreferences in the Demographic, DeathCircumstances, FamilyHistory, MedicalHistory and TissueCollection sheets toProtectedDonor, and add the matching ProtectedDonor rows. Only the logical rows the reader ingests (those before the first empty row) are scanned and appended to. If an existing ProtectedDonor sheet lacks the destination headers for copied fields, the transformer adds those headers so the fields are kept. UUID and accession identifiers are resolved through the portal by identity and type. A typedProtectedDonorLookupErrorand per-reference kinds keep permission-denied (401/403) and other lookup failures, such as outages, separate from a definitive absence (404/410).CustomExcelhas new opt-in options:transform_protected_donor,transformed_workbook_pathandallow_existing_staging_path, andCustomExcel.with_portal(portal, **options)now passes them through. Transformed workbooks are saved atomically through a temporary file. A save never overwrites the input workbook, and it only overwrites an existing path when the caller marks it as its own staging path.StructuredDataSet._load_excel_filenow opens the workbook once and uses it for both the progress-counting pass and the parsing pass, so aCustomExcelsave on construction cannot break parsing. Multi-schema JSON files now follow the specified schema order. The branch also removes unusednonlocal/globaldeclarations flagged by pyflakes F824, bumps the version to 8.19.1, adds a changelog entry and docs, and adds regression tests intest/test_protected_donor_transform.pyandtest/test_structured_data.py.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The fix round is small and bounded. The Donor-sheet safety check now runs in the validation phase before any mutation, and it uses the same logical rows and header width as the later column insertion. The protected_donor column is now added only to sheets with transformed rows. Numeric status patterns are checked before name patterns. The regression tests assert observable workbook and classification behavior, and they would fail against the pre-fix code.
Testing
I ran the focused ProtectedDonor and progress-bar tests (all pass). I confirmed the new regression tests fail on the pre-fix source (22 failures across all six concerns). I also ran an end-to-end StructuredDataSet/CustomExcel parse of a real workbook with progress turned on. In that run, the existing ProtectedDonor sheet gained the external_id/status headers. The generated rows stayed visible past the formatted empty rows. The reference after the blank-row terminator was ignored. The counting pass did not break the parse, and the staged output was written. UUID references resolved to ProtectedDonor through their type. 403, 503 and connection errors were reported as ProtectedDonorLookupError ("not known to be absent"), separately from a real 404 absence.
Evidence: End-to-end demo output (StructuredDataSet/CustomExcel parse + lookup failure classification)
ProtectedDonor rows parsed: [{'submitted_id': 'A_PROTECTED-DONOR_OLD'}, {'submitted_id': 'A_PROTECTED-DONOR_1', 'external_id': 'ext-1', 'status': 'in review'}] Demographic rows parsed: [{'donor': 'A_PROTECTED-DONOR_1'}] progress events seen (counting pass ran): True ; staged output exists: True Staged ProtectedDonor header: ['submitted_id', 'external_id', 'status'] UUID ref transform ok: True ; Demographic donors: ['11111111-...-555555555555', 'A_PROTECTED-DONOR_1'] [403] PERMISSION_DENIED -> ProtectedDonorLookupError (not known to be absent) [503] LOOKUP_FAILED -> ProtectedDonorLookupError [conn] LOOKUP_FAILED -> ProtectedDonorLookupError [404] MISSING -> ProtectedDonorTransformError: absent from the workbook and portalEvidence: End-to-end demo script
/var/folders/t8/8mk98wrj1j34nc5hhqxqdqq80000gr/T/no-mistakes-evidence/01M3YQEBAG4Z3V8EZ202WH4VA2/e2e_staged.xlsx)Evidence: Regression tests against pre-fix code (22 fail)
Evidence: Focused pytest run on the change (42 passed)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
dcicutils/submitr/donor_transformer.py:366- The append-target safety check added for the ProtectedDonor sheet is not applied to the Donor sheet._add_protected_donor_columnscalls_ensure_column(sheet, headers, 'protected_donor'), which writes the new header atlen(headers)+1without first checking_has_content(sheet, min_col=len(headers)+1). Example: a Donor header rowsubmitted_id, age, <blank>, notes, or data rows with values past the last header. The reader ignores those cells today because it stops at the first blank header. Puttingprotected_donorinto the blank column C makes the reader ingestnotesas a Donor property, and any stray column-C values in non-transformed rows becomeprotected_donorvalues. It also happens after the ProtectedDonor rows were already appended, so a failure here would not leave the workbook unchanged. Fix: run the same 'no content beyond the header columns' check on every Donor sheet that needs the new column, during the validation phase before any mutation.dcicutils/submitr/donor_transformer.py:81-_http_status_ofchecks the name patterns (HTTPNotFound, ...) before the explicit numeric status in the ff_utils message (: NNN. Reason:). The ff_utils error text embeds the URL and the response JSON. So a 5xx response whose body or URL mentionsHTTPNotFound(for example, a server error caused by a failed sub-lookup) is classified ABSENT. It is then reported as definitive ProtectedDonor absence rather than a lookup failure, which is the confusion the intent says to prevent. Fix: check the numeric-status patterns first and use the name patterns only as a fallback.dcicutils/submitr/donor_transformer.py:547- The transformer always skips hidden and bracketed sheets via its own_is_hidden_sheet. If CustomExcel is built withinclude_hidden_sheets=True(possible throughwith_portal(**options)), the reader ingests sheets the transformer never scanned, so the 'same rows as ingestion' guarantee no longer holds. No current caller in the repo passes that option; it would only matter if one did.🔧 Fix: Guard Donor column insertion; prefer numeric HTTP status
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
poetry run pytest --noconftest test/test_protected_donor_transform.py test/test_progress_bar.py -v(42 passed;--noconftestbecause test/conftest.py needs AWS identity/network that isn't available locally, and these tests use no conftest fixtures)Pre-fix regression check: the current test file run againstgit archive 4c2d59d dcicutilsin a temp dir, with missing new symbols (ProtectedDonorLookupError, DonorReferenceKind.PERMISSION_DENIED/LOOKUP_FAILED) shimmed so it could collect -> 22 failed / 18 passed, covering header extension, empty-row termination, UUID/accession typing, logical-row reference scanning, permission/outage vs absence, and the counting-pass saveEnd-to-end scripte2e_demo.py: StructuredDataSet withCustomExcel.with_portal(None, transform_protected_donor=True, transformed_workbook_path=...)and a progress callback, on an xlsx where the existing ProtectedDonor sheet has only a submitted_id header, has formatted empty rows, and has a Demographic reference after a blank-row terminatorEnd-to-end ProtectedDonorWorkbookTransformer with a stub portal: a UUID reference typed as ProtectedDonor, and 403 / 503 / ConnectionError / 404 lookups🔧 **Document** - 1 issue found → auto-fixed ✅
CHANGELOG.rst:10- pyproject.toml is at beta version 8.19.0.1b2, but CHANGELOG.rst has no entry for the ProtectedDonor transformer, the new CustomExcel options (transform_protected_donor, transformed_workbook_path, allow_existing_staging_path), or the StructuredDataSet change that opens the workbook once and orders sheets/JSON keys. The repo usually adds the changelog entry together with the final version bump (e.g. commit d390ccf), so I left it alone. Please confirm the release version heading and add the entry before releasing.🔧 Fix: Bump version to 8.19.1 and add changelog entry
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.