Fm/protected donor dcicutils

.github/workflows/main.yml

@@ -37,9 +37,14 @@ jobs:
         with:
           python-version: ${{ matrix.python_version }}
 
+
       - name: Install Deps
         run: |
-          sudo apt install redis-server
+          for i in 1 2 3; do
+            sudo apt-get update && sudo apt-get install -y redis-server && break
+            echo "apt failed, retrying..."
+            sleep 5
+          done
           sudo ln -s /usr/bin/redis-server /usr/local/bin/redis-server
           sudo ln -s /usr/bin/redis-cli /usr/local/bin/redis-cli
           make build

CHANGELOG.rst

@@ -7,6 +7,27 @@ Change Log
 ----------
 
 
+8.19.1
+======
+* aschroed / 2026-10-02 / branch: fm/utils-protected-donor-review-fixes / PR-339
+  - Added dcicutils/submitr/donor_transformer.py (ProtectedDonorWorkbookTransformer,
+    analyze_protected_donors) to selectively convert Donor references in the
+    Demographic, DeathCircumstances, FamilyHistory, MedicalHistory, and TissueCollection
+    sheets to ProtectedDonor, adding the corresponding ProtectedDonor rows. Only the
+    logical rows the workbook reader ingests (before the first empty row) are scanned and
+    appended; UUID/accession identifiers are resolved via the portal; permission and
+    lookup failures (ProtectedDonorLookupError) are distinguished from a definitive absence;
+    copied fields are preserved when an existing ProtectedDonor sheet lacks their headers.
+  - Added opt-in CustomExcel options transform_protected_donor, transformed_workbook_path,
+    and allow_existing_staging_path; CustomExcel.with_portal now accepts these options.
+    Transformed workbooks are saved atomically and never overwrite the input workbook;
+    an existing output path is replaced only when allow_existing_staging_path indicates
+    the caller owns it as a staging path.
+  - StructuredDataSet now opens an Excel workbook once for both the progress-counting and
+    parsing passes, and applies the specified schema ordering to multi-schema JSON files.
+  - Removed unused nonlocal/global declarations flagged by pyflakes F824.
+
+
 8.19.0
 ======
 * ajs/wrr/sn 2026-07-29 / branch: sn_refactor_custom_excel

dcicutils/command_utils.py

@@ -423,7 +423,6 @@ def __init__(self, question: Optional[str] = None,
     def ask(self, question: Optional[str] = None) -> bool:
 
         def question_automatic(value: str) -> bool:
-            nonlocal self
             RARROW = "▶"
             LARROW = "◀"
             if yes_or_no(f"{RARROW}{RARROW}{RARROW}"

dcicutils/progress_bar.py

@@ -229,7 +229,6 @@ def _format_description(self, value: str) -> str:
 
     def _define_interrupt_handler(self) -> None:
         def handle_interrupt(signum: int, frame: frame) -> None:  # noqa
-            nonlocal self
             def handle_secondary_interrupt(signum: int, frame: frame) -> None:  # noqa
                 print("\nEnter 'yes' or 'no' or CTRL-\\ to completely abort ...")
             self.disable()
@@ -262,10 +261,8 @@ def handle_secondary_interrupt(signum: int, frame: frame) -> None:  # noqa
             self._interrupt_continue(self) if self._interrupt_continue else None
             self.enable()
         def restore_interrupt_handler() -> None:  # noqa
-            nonlocal self, previous_interrupt_handler
             set_interrupt_handler(previous_interrupt_handler)
         def set_interrupt_handler(interrupt_handler: Callable) -> Optional[Callable]:  # noqa
-            nonlocal self
             if callable(interrupt_handler) and (threading.current_thread() == threading.main_thread()):
                 return signal(SIGINT, interrupt_handler)
             return None
@@ -281,7 +278,7 @@ def _define_tidy_output_hack(self) -> None:
         sys_stdout_write = sys.stdout.write
         last_text = None ; last_captured_output_text = None ; last_spin_change_time = None  # noqa
         def tidy_stdout_write(text: str) -> None:  # noqa
-            nonlocal self, sys_stdout_write, sentinel_internal, spina, spini, spinn
+            nonlocal spini
             nonlocal last_text, last_captured_output_text, last_spin_change_time
             def replace_first(value: str, match: str, replacement: str) -> str:  # noqa
                 return value[:i] + replacement + value[i + len(match):] if (i := value.find(match)) >= 0 else value
@@ -338,7 +335,6 @@ def replace_time_dependent_values_with_static(text: str) -> str:
                         self._captured_output_for_testing.append(captured_output_text)
                         last_captured_output_text = captured_output_text
         def restore_stdout_write() -> None:  # noqa
-            nonlocal sys_stdout_write
             if sys_stdout_write is not None:
                 sys.stdout.write = sys_stdout_write
         def ascii_spinners() -> list:  # noqa

dcicutils/scripts/update_portal_object.py

@@ -150,7 +150,6 @@ def main():
     args = parser.parse_args()
 
     def usage(message: Optional[str] = None) -> None:
-        nonlocal parser
         _print(message) if isinstance(message, str) else None
         parser.print_help()
         sys.exit(1)
@@ -230,7 +229,6 @@ def post_or_patch_or_upsert(portal: Portal, file: str, schema_name: Optional[str
                                 confirm: bool = False, verbose: bool = False,
                                 quiet: bool = False, debug: bool = False) -> None:
 
-        nonlocal update_function, update_action_name
         if not quiet:
             _print(f"Processing {update_action_name} file: {file}")
         if data := _read_json_from_file(file):
@@ -416,7 +414,6 @@ def _load_data(portal: Portal, load: str, ini_file: str, explicit_schema_name: O
 
     def loadxl(portal: Portal, inserts_directory: str, schema_names_to_load: dict):
 
-        nonlocal LoadGenWrapper, load_all_gen, loadxl_summary, verbose, debug
         nonlocal loadxl_total_item_count, loadxl_total_error_count
         progress_total = sum(schema_names_to_load.values()) * 2  # loadxl does two passes
         progress_bar = ProgressBar(progress_total, interrupt_exit=True) if not noprogress else None

dcicutils/scripts/view_portal_object.py

@@ -268,15 +268,13 @@ def _get_portal_object(portal: Portal, uuid: str,
                        verbose: bool = False, debug: bool = False) -> dict:
 
     def prune_data(data: dict) -> dict:
-        nonlocal ignore
         if not isinstance(ignore, list) or not ignore:
             return data
         return {key: value for key, value in data.items() if key not in ignore}
 
     def get_metadata_for_individual_result_type(uuid: str) -> Optional[dict]:  # noqa
         # There can be a lot of individual results for which we may need to get the actual type,
         # so do this in a function we were can give verbose output feedback.
-        nonlocal portal, results_index, results_total, verbose
         if verbose:
             _print(f"Getting actual type for {results_type} result:"
                    f" {uuid} [{results_index} of {results_total}]", end="")
@@ -291,7 +289,6 @@ def get_metadata_for_individual_result_type(uuid: str) -> Optional[dict]:  # noq
         return None
 
     def get_metadata_types(path: str) -> Optional[dict]:
-        nonlocal portal, debug
         metadata_types = {}
         try:
             if verbose:
@@ -308,7 +305,6 @@ def get_metadata_types(path: str) -> Optional[dict]:
         return metadata_types
 
     def write_insert_files(response: dict) -> None:
-        nonlocal insert_files, force
         output_directory = insert_files if isinstance(insert_files, str) else os.getcwd()
         for schema_name in response:
             schema_data = response[schema_name]
@@ -756,7 +752,6 @@ def _get_parent_schema_name(schema: dict) -> Optional[str]:
 
 def _print_schemas_tree(schemas: dict) -> None:
     def children_of(name: str) -> List[str]:
-        nonlocal schemas
         children = []
         if not (name is None or isinstance(name, str)):
             return children
@@ -767,7 +762,6 @@ def children_of(name: str) -> List[str]:
                 children.append(schema_name)
         return sorted(children)
     def name_of(name: str) -> str:  # noqa
-        nonlocal schemas
         if not (name is None or isinstance(name, str)):
             return name
         if (schema := schemas.get(name)) and schema.get("isAbstract") is True:
@@ -837,7 +831,6 @@ def _print(*args, **kwargs):
 
 
 def _print_output(value: str):
-    global _output_file
     if _output_file:
         _output_file.write(value)
         _output_file.write("\n")
@@ -848,7 +841,6 @@ def _print_output(value: str):
 
 
 def _exit(message: Optional[Union[str, int]] = None, status: Optional[int] = None) -> None:
-    global _output_file
     if isinstance(message, str):
         _print(f"ERROR: {message}")
     elif isinstance(message, int) and not isinstance(status, int):

dcicutils/structured_data.py

@@ -116,7 +116,6 @@ def load(file: str, portal: Optional[Union[VirtualApp, TestApp, Portal]] = None,
 
     def validate(self, force: bool = False) -> None:
         def data_without_deleted_properties(data: dict) -> dict:
-            nonlocal self
             def isempty(value: Any) -> bool:  # noqa
                 if value == RowReader.CELL_DELETION_SENTINEL:
                     return True
@@ -305,23 +304,28 @@ def _load_packed_file(self, file: str) -> None:
     def _load_csv_file(self, file: str) -> None:
         self._load_reader(CsvReader(file), type_name=Schema.type_name(file))
 
+    def _ordered_schema_names(self, schema_names: List[str]) -> List[str]:
+        if not self._order:
+            return schema_names
+        order = {Schema.type_name(key): index for index, key in enumerate(self._order)}
+        return sorted(schema_names, key=lambda key: order.get(Schema.type_name(key), sys.maxsize))
+
     def _load_excel_file(self, file: str) -> None:
-        def get_counts() -> Tuple[int, int]:
-            nonlocal file
-            excel = self._excel_class(file)
+        def get_counts(excel: Excel) -> Tuple[int, int]:
             nrows = 0
             for sheet_name in excel.sheet_names:
                 for row in excel.sheet_reader(sheet_name):
                     nrows += 1
             return nrows, len(excel.sheet_names)
+        # Open the workbook once so that the progress counting pass and the parsing pass read the same
+        # instance; excel classes may have side effects on construction (e.g. CustomExcel workbook staging).
+        excel = self._excel_class(file)
         if self._progress:  # TODO: Move to _load_reader
-            nrows, nsheets = get_counts()
+            nrows, nsheets = get_counts(excel)
             self._progress({PROGRESS.LOAD_START: PROGRESS.NOW(),
                             PROGRESS.LOAD_COUNT_SHEETS: nsheets, PROGRESS.LOAD_COUNT_ROWS: nrows})
-        excel = self._excel_class(file)
         # Order the sheet names by any specified ordering (e.g. ala snovault.loadxl).
-        order = {Schema.type_name(key): index for index, key in enumerate(self._order)} if self._order else {}
-        for sheet_name in sorted(excel.sheet_names, key=lambda key: order.get(Schema.type_name(key), sys.maxsize)):
+        for sheet_name in self._ordered_schema_names(excel.sheet_names):
             # This effective_sheet_name function added 2025-01-21 to allow sheets whose sheet names are
             # other than simply the name of the type, but which do contain that type somehow; i.e. e.g.
             # specifically where the sheet name is like "DSA_ExternalQualityMetric" where the "DSA"
@@ -378,7 +382,7 @@ def _load_json_file(self, file: str) -> None:
                 # Otherwise if the JSON file name does not look like a schema name then
                 # assume it a dictionary where each property is the name of a schema, and
                 # which (each property) contains a list of object of that schema type.
-                for schema_name in data:
+                for schema_name in self._ordered_schema_names(list(data)):
                     item = data[schema_name]
                     if self._merge:  # New merge functionality (2024-05-25)
                         item = self._merge_with_existing_portal_object(item, schema_name)
@@ -609,7 +613,7 @@ def set_value_internal(data: Union[dict, list], value: Optional[Any], src: Optio
                                path: List[Union[str, int]], typeinfo: Optional[dict], mapv: Optional[Callable]) -> None:
 
             def set_value_backtrack_object(path_index: int, path_element: str) -> None:
-                nonlocal data, path, original_data
+                nonlocal data
                 backtrack_data = original_data
                 for j in range(path_index - 1):
                     if not isinstance(path[j], str):
@@ -744,7 +748,6 @@ def _map_function_integer(self, typeinfo: dict) -> Callable:
         allow_commas = typeinfo.get("allow_commas") is True
         allow_multiplier_suffix = typeinfo.get("allow_multiplier_suffix") is True
         def map_integer(value: str, src: Optional[str]) -> Any:  # noqa
-            nonlocal allow_commas, allow_multiplier_suffix
             return to_integer(value, fallback=value,
                               allow_commas=allow_commas,
                               allow_multiplier_suffix=allow_multiplier_suffix)
@@ -754,7 +757,6 @@ def _map_function_number(self, typeinfo: dict) -> Callable:
         allow_commas = typeinfo.get("allow_commas") is True
         allow_multiplier_suffix = typeinfo.get("allow_multiplier_suffix") is True
         def map_number(value: str, src: Optional[str]) -> Any:  # noqa
-            nonlocal allow_commas, allow_multiplier_suffix
             return to_float(value, fallback=value,
                             allow_commas=allow_commas,
                             allow_multiplier_suffix=allow_multiplier_suffix)
@@ -781,7 +783,6 @@ def map_datetime(value: str, src: Optional[str]) -> str:
 
     def _map_function_ref(self, typeinfo: dict) -> Callable:
         def map_ref(value: str, link_to: str, portal: Optional[Portal], src: Optional[str]) -> Any:
-            nonlocal self, typeinfo
             if self._norefs:
                 # Here the caller has specified the (StructuredDataSet) norefs option
                 # which means we do not check for the existence of references at all.
@@ -875,7 +876,7 @@ def _create_typeinfo(self, schema_json: dict, parent_key: Optional[str] = None)
                     property_value = array_property_items
                     property_value_type = property_value.get("type")
                 typeinfo = self._create_typeinfo(array_property_items, parent_key=key)
-                if unique:
+                if unique and key in typeinfo:
                     typeinfo[key]["unique"] = True
                 result.update(typeinfo)
                 continue
@@ -1150,7 +1151,6 @@ def _is_valid_ref(self, type_name: str, value: str, ref_validator: Optional[Call
         otherwise we could handle it generically here.
         """
         def is_possibly_valid(schema: dict, property_name: str, property_value: str) -> Optional[Callable]:  # noqa
-            nonlocal ref_validator
             if callable(ref_validator):
                 if (ref_validator_result := ref_validator(schema, property_name, property_value)) is False:
                     return False

dcicutils/submitr/custom_excel.py

@@ -2,9 +2,12 @@
 import io
 import json
 import os
+import tempfile
 from typing import Any, List, Optional
 from dcicutils.data_readers import Excel, ExcelSheetReader
 from dcicutils.misc_utils import to_boolean, to_float, to_integer
+from dcicutils.portal_utils import Portal as PortalBase
+from dcicutils.submitr.donor_transformer import ProtectedDonorWorkbookTransformer
 
 # This module implements a custom Excel spreadsheet class which supports "custom column mappings",
 # meaning that, at a very low/early level in processing, the columns/values in the spreadsheet
@@ -100,27 +103,75 @@ def _array_name_of(synthetic_column_name: str) -> Optional[str]:
 
 class CustomExcel(Excel):
 
-    def __init__(self, *args, portal=None, **kwargs):
+    def __init__(self, *args, portal=None, transform_protected_donor: bool = False,
+                 transformed_workbook_path: Optional[str] = None,
+                 allow_existing_staging_path: bool = False, **kwargs):
+        self._workbook = None
+        self._transform_protected_donor = bool(transform_protected_donor)
+        self._transformed_workbook_path = transformed_workbook_path
+        self._allow_existing_staging_path = bool(allow_existing_staging_path)
         super().__init__(*args, **kwargs)
+        if self._transform_protected_donor:
+            transformed = ProtectedDonorWorkbookTransformer(
+                effective_sheet_name=lambda name: PortalBase.schema_name(self.effective_sheet_name(name))
+            ).transform(self._workbook, portal=portal)
+            self.sheet_names = [sheet_name for sheet_name in self._workbook.sheetnames
+                                if not self.is_hidden_sheet(self._workbook[sheet_name])]
+            if transformed and self._transformed_workbook_path:
+                self._save_transformed_workbook(
+                    self._transformed_workbook_path,
+                    allow_existing_staging_path=self._allow_existing_staging_path,
+                )
         self._custom_column_mappings = CustomExcel._get_custom_column_mappings(portal=portal)
 
     @classmethod
-    def with_portal(cls, portal):
-        """Return a subclass of CustomExcel with portal baked in.
+    def with_portal(cls, portal, **options):
+        """Return CustomExcel with the portal and worker options baked in.
 
-        Use this when passing excel_class to StructuredDataSet, which requires
-        a real class (it calls issubclass() on the argument internally):
-
-            excel_class=CustomExcel.with_portal(portal)
+        The submitr worker should pass ``transform_protected_donor=True`` when
+        ProtectedDonor conversion is part of its upload flow.  This keeps the
+        conversion opt-in for existing StructuredDataSet callers.
         """
         class _CustomExcelWithPortal(cls):
             def __init__(self, *args, **kwargs):
                 kwargs.setdefault("portal", portal)
+                for key, value in options.items():
+                    kwargs.setdefault(key, value)
                 super().__init__(*args, **kwargs)
         _CustomExcelWithPortal.__name__ = "CustomExcel"
         _CustomExcelWithPortal.__qualname__ = "CustomExcel"
         return _CustomExcelWithPortal
 
+    def _save_transformed_workbook(self, path: str, *, allow_existing_staging_path: bool = False) -> None:
+        path = os.path.abspath(os.path.expanduser(path))
+        input_path = os.path.abspath(os.path.expanduser(self._file)) if self._file else None
+        if not path.lower().endswith(".xlsx"):
+            raise ValueError(f"Transformed workbook output path must end with .xlsx: {path}")
+        if input_path and path == input_path:
+            raise ValueError(f"Transformed workbook output path must differ from input workbook path: {path}")
+        directory = os.path.dirname(path)
+        if not os.path.isdir(directory):
+            raise ValueError(f"Directory for transformed workbook output does not exist: {directory}")
+        if os.path.exists(path) and not allow_existing_staging_path:
+            raise ValueError(f"Transformed workbook output path already exists: {path}")
+        temporary_path = None
+        try:
+            temporary_fd, temporary_path = tempfile.mkstemp(
+                dir=directory, prefix=f".{os.path.basename(path)}.", suffix=".xlsx"
+            )
+            os.close(temporary_fd)
+            self._workbook.save(temporary_path)
+            os.replace(temporary_path, path)
+            temporary_path = None
+        except Exception as error:
+            raise ValueError(f"Cannot save transformed workbook to {path}: {error}") from error
+        finally:
+            if temporary_path and os.path.exists(temporary_path):
+                try:
+                    os.unlink(temporary_path)
+                except OSError:
+                    pass
+
     def sheet_reader(self, sheet_name: str) -> ExcelSheetReader:
         return CustomExcelSheetReader(self, sheet_name=sheet_name, workbook=self._workbook,
                                       custom_column_mappings=self._custom_column_mappings)