From dd1b59924fbe20be988ab026fb6875746838ff0e Mon Sep 17 00:00:00 2001 From: iabaako Date: Wed, 5 Aug 2026 14:26:32 +0000 Subject: [PATCH 01/11] fix: standardize error handling for bulk prep/correction reapply Prep reapply-all previously let a failing step raise uncaught, crashing the page (e.g. a re-import that drops a column an earlier step used). Correction reapply-all silently swallowed failures with a bare except. Both now collect per-item failures, skip just the failing item, keep applying the rest, and surface one shared st.warning at the UI boundary, matching the existing pattern for interactive single-item actions. Closes #253 Co-Authored-By: Claude Sonnet 5 --- src/datasure/processing/corrections.py | 100 ++++++++++++++------ src/datasure/processing/prep.py | 59 ++++++++---- src/datasure/utils/reapply_utils.py | 41 ++++++++ src/datasure/views/correction_view.py | 6 +- src/datasure/views/import_view.py | 24 +++-- src/datasure/views/prep_view.py | 6 +- tests/processing/test_corrections.py | 52 +++++++++-- tests/processing/test_prep.py | 124 +++++++++++++++++++++++-- 8 files changed, 348 insertions(+), 64 deletions(-) create mode 100644 src/datasure/utils/reapply_utils.py diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index d778e993..4afed679 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -5,6 +5,34 @@ import streamlit as st from datasure.utils.duckdb_utils import duckdb_get_table, duckdb_save_table +from datasure.utils.reapply_utils import ReapplyFailure + + +def _describe_correction_row(row: dict[str, Any]) -> str: + """Build a human-readable description of a correction-log row. + + Parameters + ---------- + row : dict[str, Any] + A single row from the correction log + + Returns + ------- + str + Description such as "Modify {column} for key {key_value} to '{new_value}'" + """ + action = row["action"] + key_value = row["KEY"] + column = row["column"] + new_value = row["new_value"] + + if action == "modify value": + return f"Modify {column} for key {key_value} to '{new_value}'" + if action == "remove value": + return f"Remove {column} value for key {key_value}" + if action == "remove row": + return f"Remove entire row for key {key_value}" + return f"{action} for key {key_value}" class CorrectionProcessor: @@ -441,7 +469,7 @@ def remove_correction_entry( self, alias: str, correction_index: int, - ) -> None: + ) -> list[ReapplyFailure]: """Remove a correction entry from the log and reapply corrections. Parameters @@ -450,6 +478,11 @@ def remove_correction_entry( The data alias/table name correction_index : int The index of the correction entry to remove + + Returns + ------- + list[ReapplyFailure] + Remaining corrections that failed to reapply after the removal. """ correction_log = self.get_correction_log(alias) @@ -511,9 +544,9 @@ def remove_correction_entry( self.get_correction_summary.clear() # Reapply all remaining corrections - self._reapply_all_corrections(alias) + return self._reapply_all_corrections(alias) - def refresh_corrected_data(self, alias: str) -> None: + def refresh_corrected_data(self, alias: str) -> list[ReapplyFailure]: """Rebuild corrected data from the current prep data. Used after an upstream refresh (e.g. re-importing raw data) @@ -524,16 +557,26 @@ def refresh_corrected_data(self, alias: str) -> None: ---------- alias : str The data alias/table name + + Returns + ------- + list[ReapplyFailure] + Corrections that failed to reapply against the refreshed data. """ - self._reapply_all_corrections(alias) + return self._reapply_all_corrections(alias) - def _reapply_all_corrections(self, alias: str) -> None: + def _reapply_all_corrections(self, alias: str) -> list[ReapplyFailure]: """Reapply all corrections from the log to fresh data. Parameters ---------- alias : str The data alias/table name + + Returns + ------- + list[ReapplyFailure] + Corrections that failed to reapply and were skipped, in log order. """ fresh_data = duckdb_get_table( project_id=self.project_id, @@ -543,19 +586,25 @@ def _reapply_all_corrections(self, alias: str) -> None: if fresh_data.width == 0: # Prep table doesn't exist yet, nothing to correct - return + return [] correction_log = self.get_correction_log(alias) corrected_data = fresh_data + failures: list[ReapplyFailure] = [] for row in correction_log.iter_rows(named=True): - corrected_data = self._apply_correction_row(corrected_data, row) + corrected_data, error = self._apply_correction_row(corrected_data, row) + if error: + failures.append( + ReapplyFailure(step=_describe_correction_row(row), reason=error) + ) self.save_corrected_data(alias, corrected_data) + return failures def _apply_correction_row( self, data: pl.DataFrame, row: dict[str, Any] - ) -> pl.DataFrame: + ) -> tuple[pl.DataFrame, str | None]: """Apply one correction-log row to data. Returns `data` unchanged if the row's key can't be located, or if @@ -571,13 +620,15 @@ def _apply_correction_row( Returns ------- - pl.DataFrame - The data with the correction applied, or unchanged on failure + tuple[pl.DataFrame, str | None] + The data (updated on success, unchanged on failure) and an error + message describing why the correction was skipped, or None on + success. """ key_value = row["KEY"] key_col = self._find_key_column(data, key_value) if not key_col: - return data + return data, f"Key '{key_value}' not found in current data" action = row["action"] column = row["column"] @@ -585,18 +636,21 @@ def _apply_correction_row( try: if action == "modify value" and column and new_value is not None: - return self._apply_modify_value( - data, key_col, key_value, column, new_value + return ( + self._apply_modify_value( + data, key_col, key_value, column, new_value + ), + None, ) if action == "remove value" and column: - return self._apply_remove_value(data, key_col, key_value, column) + return self._apply_remove_value(data, key_col, key_value, column), None if action == "remove row": - return self._apply_remove_row(data, key_col, key_value) - except Exception: + return self._apply_remove_row(data, key_col, key_value), None + except Exception as e: # Skip corrections that fail (data may have changed) - pass + return data, str(e) - return data + return data, None @staticmethod def _find_key_column(data: pl.DataFrame, key_value: str) -> str | None: @@ -650,15 +704,7 @@ def get_correction_summary(_self, alias: str) -> list[dict[str, Any]]: reason = row["reason"] date = row["date"] - # Create description based on action type - if action == "modify value": - description = f"Modify {column} for key {key_value} to '{new_value}'" - elif action == "remove value": - description = f"Remove {column} value for key {key_value}" - elif action == "remove row": - description = f"Remove entire row for key {key_value}" - else: - description = f"{action} for key {key_value}" + description = _describe_correction_row(row) summaries.append( { diff --git a/src/datasure/processing/prep.py b/src/datasure/processing/prep.py index 6f159c91..027a8f36 100644 --- a/src/datasure/processing/prep.py +++ b/src/datasure/processing/prep.py @@ -28,6 +28,7 @@ PrepActionResult, PrepConfirmationMessages, ) +from datasure.utils.reapply_utils import ReapplyFailure # === EXCEPTIONS === # @@ -835,19 +836,32 @@ def execute_single_action( def execute_all_actions( self, data: pl.DataFrame, actions: list[PrepAction] - ) -> pl.DataFrame: - """Execute a sequence of preparation actions.""" + ) -> tuple[pl.DataFrame, list[ReapplyFailure]]: + """Execute a sequence of preparation actions. + + An action that fails is skipped - the data is left as it was before + that action - so a single step made incompatible by upstream changes + (e.g. a re-import that renames/drops a column an earlier step relied + on) does not abort the rest of the sequence. + + Returns + ------- + Tuple of the resulting data and the list of actions that were + skipped because they failed. + """ result_data = data + failures: list[ReapplyFailure] = [] for action in actions: + step_description = _generate_action_description(action.prep_args) try: result_data, _ = self.execute_single_action(result_data, action) - except (ValidationError, OperationError): - raise + except (ValidationError, OperationError) as e: + failures.append(ReapplyFailure(step=step_description, reason=str(e))) except Exception as e: - raise OperationError(f"Failed to execute action '{action}': {e}") from e + failures.append(ReapplyFailure(step=step_description, reason=str(e))) - return result_data + return result_data, failures # === LOG MANAGEMENT (PRIVATE) === # @@ -869,7 +883,7 @@ def _parse_prep_log_to_actions(prep_log_df: pl.DataFrame) -> list[PrepAction]: if isinstance(args, str): try: args = json.loads(args) - except (json.JSONDecodeError, ValueError): + except ValueError: args = ast.literal_eval(args) prep_action = PrepActionResult(**args) actions.append(PrepAction.from_args(prep_action)) @@ -967,7 +981,7 @@ def _reapply_all_actions( alias: str, prep_log_df: pl.DataFrame, processor: PrepProcessor, -) -> None: +) -> list[ReapplyFailure]: """Re-apply all actions from the log to the raw data. Args: @@ -975,16 +989,21 @@ def _reapply_all_actions( alias: Dataset alias prep_log_df: DataFrame containing the preparation log processor: PrepProcessor instance for executing actions + + Returns + ------- + List of actions that were skipped because they failed to reapply. """ raw_data = duckdb_get_table(project_id, alias, db_name="raw") if prep_log_df.is_empty(): duckdb_save_table(project_id, raw_data, alias, db_name="prep") - return + return [] existing_actions = _parse_prep_log_to_actions(prep_log_df) - result_data = processor.execute_all_actions(raw_data, existing_actions) + result_data, failures = processor.execute_all_actions(raw_data, existing_actions) duckdb_save_table(project_id, result_data, alias, db_name="prep") + return failures def _apply_single_action( @@ -1029,7 +1048,7 @@ def prep_apply_action( project_id: str, alias: str, prep_args: PrepActionResult | None = None, -) -> None: +) -> list[ReapplyFailure]: """Apply data preparation action to dataset. When prep_args is provided, applies the single new action to the current @@ -1041,15 +1060,23 @@ def prep_apply_action( alias: Dataset alias prep_args: Optional action to apply. If None, re-applies all logged actions. + Returns + ------- + List of actions skipped while re-applying the full log. Always empty + when applying a single new action (prep_args is not None). + Raises ------ - ValidationError: If action/description validation fails - OperationError: If data operation fails + ValidationError: If action/description validation fails (only when + applying a single new action) + OperationError: If data operation fails (only when applying a single + new action) """ processor = PrepProcessor() prep_log_df = duckdb_get_table(project_id, f"prep_log_{alias}", db_name="logs") if prep_args is None: - _reapply_all_actions(project_id, alias, prep_log_df, processor) - else: - _apply_single_action(project_id, alias, prep_args, prep_log_df, processor) + return _reapply_all_actions(project_id, alias, prep_log_df, processor) + + _apply_single_action(project_id, alias, prep_args, prep_log_df, processor) + return [] diff --git a/src/datasure/utils/reapply_utils.py b/src/datasure/utils/reapply_utils.py new file mode 100644 index 00000000..3becc7ef --- /dev/null +++ b/src/datasure/utils/reapply_utils.py @@ -0,0 +1,41 @@ +"""Shared helpers for reporting partial failures during bulk reapply. + +Both the prep log (processing/prep.py) and correction log +(processing/corrections.py) can be reapplied in bulk against refreshed +upstream data - e.g. after a re-import changes the underlying columns. A +single incompatible step should not abort the whole sequence, but it also +should not be silently dropped. This module gives both paths a common type +for reporting what was skipped and why, and a single Streamlit helper for +surfacing it at the UI boundary. +""" + +from dataclasses import dataclass + + +@dataclass +class ReapplyFailure: + """A single step or correction skipped during a bulk reapply.""" + + step: str + reason: str + + +def warn_reapply_failures(failures: list[ReapplyFailure], context: str) -> None: + """Render one warning summarizing steps skipped during a bulk reapply. + + Parameters + ---------- + failures : list[ReapplyFailure] + Steps/corrections skipped during the reapply, in the order they were + encountered. No-op when empty. + context : str + Short lead-in describing what was being reapplied, e.g. "Some + preparation steps could not be reapplied". + """ + if not failures: + return + + import streamlit as st + + lines = "\n".join(f"- {f.step}: {f.reason}" for f in failures) + st.warning(f"{context} ({len(failures)} skipped):\n{lines}") diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 70d4d293..e69ef14c 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -21,6 +21,7 @@ page_navigation, ) from datasure.utils.onboarding_utils import ImportDemoInfo, demo_expander +from datasure.utils.reapply_utils import warn_reapply_failures from datasure.utils.settings_utils import get_check_config_settings from datasure.utils.ui_utils import ( confirm_dialog, @@ -876,9 +877,12 @@ def _handle_remove_correction( ) # Remove the correction - correction_processor.remove_correction_entry(alias, correction_index) + failures = correction_processor.remove_correction_entry(alias, correction_index) st.success(f"Correction '{selected_action}' removed successfully!") + warn_reapply_failures( + failures, "Some remaining corrections could not be reapplied" + ) st.rerun() except Exception as e: diff --git a/src/datasure/views/import_view.py b/src/datasure/views/import_view.py index 24bf0f65..27d7dc2d 100644 --- a/src/datasure/views/import_view.py +++ b/src/datasure/views/import_view.py @@ -30,6 +30,7 @@ ImportDemoInfo, is_demo_project, ) +from datasure.utils.reapply_utils import ReapplyFailure, warn_reapply_failures from datasure.utils.secure_credentials import ( delete_stored_credentials, list_stored_credentials, @@ -111,35 +112,46 @@ def _get_filtered_import_log(project_id: str) -> pl.DataFrame: def _process_import_log(project_id: str, import_log: pl.DataFrame) -> None: """Process all rows in import log with status updates.""" + all_failures: list[ReapplyFailure] = [] with st.status("Loading datasets ...", expanded=True) as status: for row in import_log.iter_rows(named=True): - _process_single_import(project_id, row) + all_failures.extend(_process_single_import(project_id, row)) status.update( label="Data loaded successfully!", state="complete", expanded=True ) + warn_reapply_failures( + all_failures, + "Some downstream data could not be fully reapplied after import", + ) -def _process_single_import(project_id: str, row: dict) -> None: +def _process_single_import(project_id: str, row: dict) -> list[ReapplyFailure]: """Process a single import configuration row.""" + failures: list[ReapplyFailure] = [] if row["refresh"]: _load_dataset_by_source(project_id, row) - _refresh_downstream_data(project_id, row["alias"]) + failures = _refresh_downstream_data(project_id, row["alias"]) _add_to_session_state(row["alias"]) + return failures -def _refresh_downstream_data(project_id: str, alias: str) -> None: +def _refresh_downstream_data(project_id: str, alias: str) -> list[ReapplyFailure]: """Rebuild prep and corrected data after a raw dataset refresh. Without this, the Prep and Correction pages keep showing data derived from the previous import, since both stages are cached copies that are otherwise only rebuilt when a prep step or correction is removed. """ + failures: list[ReapplyFailure] = [] + if duckdb_table_exists(project_id, alias=alias, db_name="prep"): - prep_apply_action(project_id, alias) + failures.extend(prep_apply_action(project_id, alias)) if duckdb_table_exists(project_id, alias=alias, db_name="corrected"): - CorrectionProcessor(project_id).refresh_corrected_data(alias) + failures.extend(CorrectionProcessor(project_id).refresh_corrected_data(alias)) + + return failures def _load_dataset_by_source(project_id: str, row: dict) -> None: diff --git a/src/datasure/views/prep_view.py b/src/datasure/views/prep_view.py index 64b441ae..3756c8d2 100644 --- a/src/datasure/views/prep_view.py +++ b/src/datasure/views/prep_view.py @@ -38,6 +38,7 @@ PrepActionResult, PrepDescriptions, ) +from datasure.utils.reapply_utils import warn_reapply_failures from datasure.utils.ui_utils import ( confirm_dialog, metric_row, @@ -967,8 +968,11 @@ def _remove_prep_step(action_index: str, log=prep_log, alias=label): alias=f"prep_log_{alias}", db_name="logs", ) - prep_apply_action(project_id, alias) + failures = prep_apply_action(project_id, alias) st.success(f"Action '{action_desc}' removed successfully!") + warn_reapply_failures( + failures, "Some preparation steps could not be reapplied" + ) if st.button( label="Remove", diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 863c63d6..266c5ad1 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -609,10 +609,11 @@ def test_remove_correction_entry( pl.DataFrame(), # empty log after removal ] - processor.remove_correction_entry("test_alias", 1) + failures = processor.remove_correction_entry("test_alias", 1) # Should save the updated log and reapplied data assert mock_save.call_count == 2 + assert failures == [] def test_remove_correction_entry_invalid_index( self, correction_processor, sample_corrections_log @@ -647,10 +648,11 @@ def test_reapply_all_corrections_empty_prep_data(self, correction_processor): processor, mock_get, mock_save = correction_processor mock_get.return_value = pl.DataFrame() # Empty prep data - processor._reapply_all_corrections("test_alias") + failures = processor._reapply_all_corrections("test_alias") # Should not save anything when prep data is empty mock_save.assert_not_called() + assert failures == [] def test_reapply_all_corrections_empty_log(self, correction_processor, sample_data): """Test reapplying corrections when correction log is empty.""" @@ -658,10 +660,11 @@ def test_reapply_all_corrections_empty_log(self, correction_processor, sample_da # Mock sequence: get prep data, get empty log mock_get.side_effect = [sample_data, pl.DataFrame()] - processor._reapply_all_corrections("test_alias") + failures = processor._reapply_all_corrections("test_alias") # Should save the fresh prep data as corrected data mock_save.assert_called_once() + assert failures == [] def test_reapply_all_corrections_with_data( self, correction_processor, sample_data, sample_corrections_log @@ -671,13 +674,14 @@ def test_reapply_all_corrections_with_data( # Mock sequence: get prep data, get correction log mock_get.side_effect = [sample_data, sample_corrections_log] - processor._reapply_all_corrections("test_alias") + failures = processor._reapply_all_corrections("test_alias") # Should save corrected data after applying all corrections mock_save.assert_called_once() call_args = mock_save.call_args assert call_args[1]["alias"] == "test_alias" assert call_args[1]["db_name"] == "corrected" + assert failures == [] def test_reapply_corrections_key_not_found(self, correction_processor, sample_data): """Test reapplying corrections when key value not found in data.""" @@ -699,10 +703,42 @@ def test_reapply_corrections_key_not_found(self, correction_processor, sample_da mock_get.side_effect = [sample_data, invalid_log] - processor._reapply_all_corrections("test_alias") + failures = processor._reapply_all_corrections("test_alias") # Should still save data even if some corrections fail mock_save.assert_called_once() + # ... and the skipped correction should be reported, not swallowed + assert len(failures) == 1 + assert "nonexistent_key" in failures[0].reason + + def test_reapply_corrections_partial_failure_continues( + self, correction_processor, sample_data + ): + """One bad correction is skipped and reported; the rest still apply.""" + processor, mock_get, mock_save = correction_processor + + mixed_log = pl.DataFrame( + { + "date": [datetime.now()] * 2, + "KEY": ["nonexistent_key", "key1"], + "ID": [None, None], + "action": ["modify value", "modify value"], + "column": ["name", "name"], + "current_value": ["test", "John"], + "new_value": ["changed", "Johnny"], + "reason": ["bad correction", "good correction"], + } + ) + + mock_get.side_effect = [sample_data, mixed_log] + + failures = processor._reapply_all_corrections("test_alias") + + mock_save.assert_called_once() + saved_data = mock_save.call_args[1]["table_data"] + assert saved_data.filter(pl.col("survey_key") == "key1")["name"][0] == "Johnny" + assert len(failures) == 1 + assert "nonexistent_key" in failures[0].reason def test_reapply_corrections_exception_handling( self, correction_processor, sample_data @@ -727,10 +763,14 @@ def test_reapply_corrections_exception_handling( mock_get.side_effect = [sample_data, problematic_log] # Should not raise exception, just skip problematic corrections - processor._reapply_all_corrections("test_alias") + failures = processor._reapply_all_corrections("test_alias") # Should still save data mock_save.assert_called_once() + # ... and report the skipped correction instead of swallowing it + assert len(failures) == 1 + assert failures[0].reason + assert "key1" in failures[0].step def test_private_apply_modify_value_string(self, correction_processor, sample_data): """Test private method _apply_modify_value with string column.""" diff --git a/tests/processing/test_prep.py b/tests/processing/test_prep.py index 8b828af1..3f6eb4d8 100644 --- a/tests/processing/test_prep.py +++ b/tests/processing/test_prep.py @@ -1372,16 +1372,18 @@ def test_execute_all_actions_success(self): ) ), ] - result = processor.execute_all_actions(data, actions) + result, failures = processor.execute_all_actions(data, actions) assert "c" not in result.columns assert "new" in result.columns + assert failures == [] def test_execute_all_actions_empty(self): """Test Execute all actions empty.""" processor = PrepProcessor() data = pl.DataFrame({"a": [1, 2]}) - result = processor.execute_all_actions(data, []) + result, failures = processor.execute_all_actions(data, []) assert result.equals(data) + assert failures == [] def test_execute_all_actions_failure(self): """Test Execute all actions failure.""" @@ -1394,9 +1396,31 @@ def test_execute_all_actions_failure(self): ) ) ] - # Domain errors raised by individual actions propagate unwrapped - with pytest.raises(OperationError, match="Columns not found"): - processor.execute_all_actions(data, actions) + # A failing action is skipped (not raised) and reported as a failure + result, failures = processor.execute_all_actions(data, actions) + assert result.equals(data) + assert len(failures) == 1 + assert "Columns not found" in failures[0].reason + + def test_execute_all_actions_partial_failure_continues(self): + """A failing action is skipped but later actions still apply.""" + processor = PrepProcessor() + data = pl.DataFrame({"a": [1, 2], "b": [3, 4]}) + actions = [ + PrepAction.from_args( + PrepActionResult( + action="remove column(s)", source_columns=["nonexistent"] + ) + ), + PrepAction.from_args( + PrepActionResult(action="remove column(s)", source_columns=["b"]) + ), + ] + result, failures = processor.execute_all_actions(data, actions) + assert "b" not in result.columns + assert "a" in result.columns + assert len(failures) == 1 + assert "Columns not found" in failures[0].reason # === LOG MANAGEMENT TESTS === # @@ -1612,10 +1636,64 @@ def test_reapply_all_actions_with_log(self, mock_save, mock_get): } ) processor = PrepProcessor() - _reapply_all_actions("proj", "alias", log, processor) + failures = _reapply_all_actions("proj", "alias", log, processor) mock_save.assert_called_once() saved_data = mock_save.call_args[0][1] assert "b" not in saved_data.columns + assert failures == [] + + @patch("datasure.processing.prep.duckdb_get_table") + @patch("datasure.processing.prep.duckdb_save_table") + def test_reapply_all_actions_partial_failure(self, mock_save, mock_get): + """A step referencing a column dropped upstream is skipped, not raised. + + The rest of the log (a step on a still-present column) still applies, + and the save reflects that partial result plus the reported failure. + """ + raw_data = pl.DataFrame({"a": [1, 2, 3], "b": [4, 5, 6]}) + mock_get.return_value = raw_data + log = pl.DataFrame( + { + "prep_args": [ + str( + { + "action": "remove column(s)", + "source_columns": ["missing_column"], + "column_names": None, + "affected_count": None, + "remaining_count": None, + "value": None, + "method": None, + "condition": None, + "failed_count": None, + "additional_info": None, + } + ), + str( + { + "action": "remove column(s)", + "source_columns": ["b"], + "column_names": None, + "affected_count": None, + "remaining_count": None, + "value": None, + "method": None, + "condition": None, + "failed_count": None, + "additional_info": None, + } + ), + ] + } + ) + processor = PrepProcessor() + failures = _reapply_all_actions("proj", "alias", log, processor) + + saved_data = mock_save.call_args[0][1] + assert "b" not in saved_data.columns + assert "a" in saved_data.columns + assert len(failures) == 1 + assert "Columns not found" in failures[0].reason @patch("datasure.processing.prep.duckdb_get_table") @patch("datasure.processing.prep.duckdb_save_table") @@ -1688,9 +1766,41 @@ def test_reapply_all(self, mock_save, mock_get): ) raw_data = pl.DataFrame({"a": [1, 2], "b": [3, 4]}) mock_get.side_effect = [log, raw_data] - prep_apply_action("proj", "alias", prep_args=None) + failures = prep_apply_action("proj", "alias", prep_args=None) assert mock_get.call_count == 2 mock_save.assert_called() + assert failures == [] + + @patch("datasure.processing.prep.duckdb_get_table") + @patch("datasure.processing.prep.duckdb_save_table") + def test_reapply_all_reports_partial_failure(self, mock_save, mock_get): + """A failing step during reapply-all is reported, not raised.""" + log = pl.DataFrame( + { + "prep_args": [ + str( + { + "action": "remove column(s)", + "source_columns": ["missing_column"], + "column_names": None, + "affected_count": None, + "remaining_count": None, + "value": None, + "method": None, + "condition": None, + "failed_count": None, + "additional_info": None, + } + ) + ] + } + ) + raw_data = pl.DataFrame({"a": [1, 2], "b": [3, 4]}) + mock_get.side_effect = [log, raw_data] + failures = prep_apply_action("proj", "alias", prep_args=None) + assert mock_save.called # data still saved, unmodified + assert len(failures) == 1 + assert "Columns not found" in failures[0].reason @patch("datasure.processing.prep.duckdb_get_table") @patch("datasure.processing.prep.duckdb_save_table") From ef305964a76a81632d8e2118d13a505d808c1316 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 6 Aug 2026 16:54:30 +0000 Subject: [PATCH 02/11] feat/Show status of prep steps --- src/datasure/processing/prep.py | 144 +++++++++++++++++++++++++------ src/datasure/utils/prep_utils.py | 19 +++- src/datasure/views/prep_view.py | 21 ++++- tests/processing/test_prep.py | 68 ++++++++++++--- tests/utils/test_prep_utils.py | 17 ++++ tests/views/test_prep_view.py | 99 +++++++++++++++++++++ 6 files changed, 325 insertions(+), 43 deletions(-) diff --git a/src/datasure/processing/prep.py b/src/datasure/processing/prep.py index 027a8f36..e97cf30d 100644 --- a/src/datasure/processing/prep.py +++ b/src/datasure/processing/prep.py @@ -54,6 +54,15 @@ class OperationError(PrepError): # === DATA MODELS === # +@dataclass +class PrepReapplyOutcome: + """Result of (re)applying one logged prep action during a bulk reapply.""" + + prep_args: PrepActionResult + status: str # "Successful" or "Failed" + error: str | None = None + + @dataclass class PrepAction: """Represents a data preparation action.""" @@ -162,29 +171,41 @@ class RemoveColumnsOperation(PrepOperation): def execute( self, data: pl.DataFrame, prep_args: PrepActionResult ) -> tuple[pl.DataFrame, PrepActionResult]: - """Remove columns specified in description.""" + """Remove columns specified in description. + + Columns that no longer exist (e.g. dropped by a later re-import) are + skipped rather than failing the whole step, as long as at least one + requested column is still present - so a step that removed 4 columns + still removes the 2 that remain instead of removing none. + """ try: - # Extract column names from description - columns = prep_args.source_columns - self._validate_columns_exist(data, columns) + columns = prep_args.source_columns or [] + existing_columns = [c for c in columns if c in data.columns] + missing_columns = [c for c in columns if c not in data.columns] - # drop columns - results = data.drop(columns) + if not existing_columns: + raise OperationError(f"Columns not found: {missing_columns}") # noqa: TRY301 + + # Remove columns using Polars + results = data.drop(existing_columns) updated_prep_args = { "action": PrepActions.remove_column.value, "column_names": None, - "affected_count": len(columns), - "remaining_count": data.width, + "affected_count": len(existing_columns), + "remaining_count": results.width, "value": None, "method": None, - "source_columns": columns, + "source_columns": existing_columns, "condition": prep_args.condition, - "failed_count": 0, - "additional_info": None, + "failed_count": len(missing_columns), + "additional_info": ( + f"Columns not found and skipped: {missing_columns}" + if missing_columns + else None + ), } - # Remove columns using Polars return results, PrepActionResult(**updated_prep_args) except (ValidationError, OperationError): @@ -836,7 +857,7 @@ def execute_single_action( def execute_all_actions( self, data: pl.DataFrame, actions: list[PrepAction] - ) -> tuple[pl.DataFrame, list[ReapplyFailure]]: + ) -> tuple[pl.DataFrame, list[PrepReapplyOutcome]]: """Execute a sequence of preparation actions. An action that fails is skipped - the data is left as it was before @@ -846,22 +867,36 @@ def execute_all_actions( Returns ------- - Tuple of the resulting data and the list of actions that were - skipped because they failed. + Tuple of the resulting data and one outcome per action, in order, + reflecting what actually happened this time (a step that used to + remove 4 columns but now only finds 2 is reported as removing 2, + not as a failure). """ result_data = data - failures: list[ReapplyFailure] = [] + outcomes: list[PrepReapplyOutcome] = [] for action in actions: - step_description = _generate_action_description(action.prep_args) try: - result_data, _ = self.execute_single_action(result_data, action) + result_data, updated_args = self.execute_single_action( + result_data, action + ) + outcomes.append( + PrepReapplyOutcome(prep_args=updated_args, status="Successful") + ) except (ValidationError, OperationError) as e: - failures.append(ReapplyFailure(step=step_description, reason=str(e))) + outcomes.append( + PrepReapplyOutcome( + prep_args=action.prep_args, status="Failed", error=str(e) + ) + ) except Exception as e: - failures.append(ReapplyFailure(step=step_description, reason=str(e))) + outcomes.append( + PrepReapplyOutcome( + prep_args=action.prep_args, status="Failed", error=str(e) + ) + ) - return result_data, failures + return result_data, outcomes # === LOG MANAGEMENT (PRIVATE) === # @@ -932,6 +967,7 @@ def _create_log_entry( description: str, prep_args: PrepActionResult, log_index: int, + status: str = "Successful", ) -> pl.DataFrame: """Create a new log entry DataFrame for a prep action. @@ -940,6 +976,8 @@ def _create_log_entry( description: Human-readable description prep_args: The preparation action result log_index: Current log size (for action_index) + status: "Successful" or "Failed" - the outcome of the most recent + (re)application of this step Returns ------- @@ -952,10 +990,58 @@ def _create_log_entry( "description": [description], "prep_args": [prep_args], "action_index": [action_index_val], + "status": [status], } ) +def _ensure_status_column(df: pl.DataFrame) -> pl.DataFrame: + """Backfill a "status" column for logs persisted before it was added.""" + if df.is_empty() or "status" in df.columns: + return df + return df.with_columns(pl.lit("Successful").alias("status")) + + +def _build_log_from_outcomes( + existing_actions: list[PrepAction], outcomes: list[PrepReapplyOutcome] +) -> pl.DataFrame: + """Rebuild the persisted prep log after a reapply. + + Each step's original request (`prep_args`) is kept as-is, so a column + that reappears in a later re-import is still requested for removal. + Only the display description and status are refreshed to reflect what + actually happened this time. + + Args: + existing_actions: The logged actions, as originally requested + outcomes: The result of (re)applying each action, in the same order + + Returns + ------- + A fresh prep log DataFrame reflecting the latest reapply + """ + entries = [] + for index, (action, outcome) in enumerate( + zip(existing_actions, outcomes, strict=True) + ): + if outcome.status == "Failed": + description = f"✗ Failed to reapply: {outcome.error}" + else: + description = _generate_action_description(outcome.prep_args) + + entries.append( + _create_log_entry( + action.prep_args.action, + description, + action.prep_args, + index, + outcome.status, + ) + ) + + return pl.concat([_convert_prep_args_to_string(e) for e in entries]) + + def _append_to_prep_log( existing_log: pl.DataFrame, new_entry: pl.DataFrame ) -> pl.DataFrame: @@ -971,7 +1057,7 @@ def _append_to_prep_log( """ if existing_log.is_empty(): return new_entry - existing_log_str = _convert_prep_args_to_string(existing_log) + existing_log_str = _ensure_status_column(_convert_prep_args_to_string(existing_log)) new_entry_str = _convert_prep_args_to_string(new_entry) return pl.concat([existing_log_str, new_entry_str]) @@ -1001,9 +1087,19 @@ def _reapply_all_actions( return [] existing_actions = _parse_prep_log_to_actions(prep_log_df) - result_data, failures = processor.execute_all_actions(raw_data, existing_actions) + result_data, outcomes = processor.execute_all_actions(raw_data, existing_actions) duckdb_save_table(project_id, result_data, alias, db_name="prep") - return failures + + refreshed_log = _build_log_from_outcomes(existing_actions, outcomes) + duckdb_save_table(project_id, refreshed_log, f"prep_log_{alias}", db_name="logs") + + return [ + ReapplyFailure( + step=_generate_action_description(outcome.prep_args), reason=outcome.error + ) + for outcome in outcomes + if outcome.status == "Failed" + ] def _apply_single_action( diff --git a/src/datasure/utils/prep_utils.py b/src/datasure/utils/prep_utils.py index 640cbde8..b5fc3160 100644 --- a/src/datasure/utils/prep_utils.py +++ b/src/datasure/utils/prep_utils.py @@ -245,17 +245,28 @@ def add_new_column(cls, result: PrepActionResult) -> str: @classmethod def remove_columns(cls, result: PrepActionResult) -> str: - """Generate message for removing columns.""" + """Generate message for removing columns. + + `result.source_columns` and `result.affected_count` reflect only the + columns actually removed - e.g. if 2 of 4 requested columns no longer + exist, this reports 2 removed, not 4. + """ + removed_columns = result.source_columns column_count = ( - len(result.source_columns) if isinstance(result.source_columns, list) else 1 + result.affected_count + if result.affected_count is not None + else (len(removed_columns) if isinstance(removed_columns, list) else 1) ) column_text = cls._pluralize(column_count, "column") remaining_text = cls._pluralize(result.remaining_count, "column") - column_display = cls._format_column_names(result.source_columns) - return ( + column_display = cls._format_column_names(removed_columns) + message = ( f"✓ {column_count} {column_text} removed. {column_display} deleted from " f"your dataset. {result.remaining_count} {remaining_text} remaining." ) + if result.failed_count: + message = f"{message} {result.additional_info}" + return message @classmethod def remove_rows(cls, result: PrepActionResult) -> str: diff --git a/src/datasure/views/prep_view.py b/src/datasure/views/prep_view.py index 3756c8d2..782ca97e 100644 --- a/src/datasure/views/prep_view.py +++ b/src/datasure/views/prep_view.py @@ -991,6 +991,13 @@ def _remove_prep_step(action_index: str, log=prep_log, alias=label): ) +def _highlight_failed_status(value: str) -> str: + """Style a Change Log status cell; highlights failed steps in red.""" + if value == "Failed": + return "background-color: #dc3545; color: white; font-weight: 600" + return "" + + # === PAGE LAYOUT === # # -- DATA PREP PAGE --# @@ -1085,8 +1092,18 @@ def _remove_prep_step(action_index: str, log=prep_log, alias=label): "No changes added yet. Click on the **Add**(:material/add:) button above to add a new data preparation step." ) else: - prep_logs_mod = st.dataframe( - prep_log[["action", "description"]], + if "status" not in prep_log.columns: + prep_log = prep_log.with_columns( + pl.lit("Successful").alias("status") + ) + + change_log = prep_log[ + ["action", "status", "description"] + ].to_pandas() + st.dataframe( + change_log.style.map( + _highlight_failed_status, subset=["status"] + ), width="stretch", key=label, hide_index=False, diff --git a/tests/processing/test_prep.py b/tests/processing/test_prep.py index 3f6eb4d8..2aa4cad0 100644 --- a/tests/processing/test_prep.py +++ b/tests/processing/test_prep.py @@ -196,6 +196,22 @@ def test_remove_nonexistent_column(self): with pytest.raises(OperationError, match="Columns not found"): op.execute(data, prep_args) + def test_remove_columns_partial_missing(self): + """Removing 4 columns where 2 no longer exist removes the other 2.""" + op = RemoveColumnsOperation() + data = pl.DataFrame({"a": [1], "b": [2], "c": [3], "d": [4]}) + prep_args = PrepActionResult( + action="remove column(s)", + source_columns=["a", "c", "missing1", "missing2"], + ) + result, args = op.execute(data, prep_args) + assert set(result.columns) == {"b", "d"} + assert args.affected_count == 2 + assert args.failed_count == 2 + assert args.source_columns == ["a", "c"] + assert "missing1" in args.additional_info + assert "missing2" in args.additional_info + def test_validate_columns_exist_valid(self): """Test Validate columns exist valid.""" op = RemoveColumnsOperation() @@ -1372,18 +1388,18 @@ def test_execute_all_actions_success(self): ) ), ] - result, failures = processor.execute_all_actions(data, actions) + result, outcomes = processor.execute_all_actions(data, actions) assert "c" not in result.columns assert "new" in result.columns - assert failures == [] + assert [o.status for o in outcomes] == ["Successful", "Successful"] def test_execute_all_actions_empty(self): """Test Execute all actions empty.""" processor = PrepProcessor() data = pl.DataFrame({"a": [1, 2]}) - result, failures = processor.execute_all_actions(data, []) + result, outcomes = processor.execute_all_actions(data, []) assert result.equals(data) - assert failures == [] + assert outcomes == [] def test_execute_all_actions_failure(self): """Test Execute all actions failure.""" @@ -1397,10 +1413,11 @@ def test_execute_all_actions_failure(self): ) ] # A failing action is skipped (not raised) and reported as a failure - result, failures = processor.execute_all_actions(data, actions) + result, outcomes = processor.execute_all_actions(data, actions) assert result.equals(data) - assert len(failures) == 1 - assert "Columns not found" in failures[0].reason + assert len(outcomes) == 1 + assert outcomes[0].status == "Failed" + assert "Columns not found" in outcomes[0].error def test_execute_all_actions_partial_failure_continues(self): """A failing action is skipped but later actions still apply.""" @@ -1416,11 +1433,30 @@ def test_execute_all_actions_partial_failure_continues(self): PrepActionResult(action="remove column(s)", source_columns=["b"]) ), ] - result, failures = processor.execute_all_actions(data, actions) + result, outcomes = processor.execute_all_actions(data, actions) assert "b" not in result.columns assert "a" in result.columns - assert len(failures) == 1 - assert "Columns not found" in failures[0].reason + assert [o.status for o in outcomes] == ["Failed", "Successful"] + assert "Columns not found" in outcomes[0].error + + def test_execute_all_actions_partial_column_removal(self): + """Removing 4 columns where 2 no longer exist removes the other 2.""" + processor = PrepProcessor() + data = pl.DataFrame({"a": [1], "b": [2], "c": [3], "d": [4]}) + actions = [ + PrepAction.from_args( + PrepActionResult( + action="remove column(s)", + source_columns=["a", "b", "missing1", "missing2"], + ) + ) + ] + result, outcomes = processor.execute_all_actions(data, actions) + assert set(result.columns) == {"c", "d"} + assert outcomes[0].status == "Successful" + assert outcomes[0].prep_args.affected_count == 2 + assert outcomes[0].prep_args.failed_count == 2 + assert outcomes[0].prep_args.source_columns == ["a", "b"] # === LOG MANAGEMENT TESTS === # @@ -1637,8 +1673,9 @@ def test_reapply_all_actions_with_log(self, mock_save, mock_get): ) processor = PrepProcessor() failures = _reapply_all_actions("proj", "alias", log, processor) - mock_save.assert_called_once() - saved_data = mock_save.call_args[0][1] + # Saves both the reapplied data and the refreshed log + assert mock_save.call_count == 2 + saved_data = mock_save.call_args_list[0][0][1] assert "b" not in saved_data.columns assert failures == [] @@ -1689,12 +1726,17 @@ def test_reapply_all_actions_partial_failure(self, mock_save, mock_get): processor = PrepProcessor() failures = _reapply_all_actions("proj", "alias", log, processor) - saved_data = mock_save.call_args[0][1] + saved_data = mock_save.call_args_list[0][0][1] assert "b" not in saved_data.columns assert "a" in saved_data.columns assert len(failures) == 1 assert "Columns not found" in failures[0].reason + # The refreshed log records the failed step's status and description + saved_log = mock_save.call_args_list[1][0][1] + assert saved_log["status"].to_list() == ["Failed", "Successful"] + assert "Failed to reapply" in saved_log["description"][0] + @patch("datasure.processing.prep.duckdb_get_table") @patch("datasure.processing.prep.duckdb_save_table") def test_apply_single_action(self, mock_save, mock_get): diff --git a/tests/utils/test_prep_utils.py b/tests/utils/test_prep_utils.py index 85a9b868..d9c17da3 100644 --- a/tests/utils/test_prep_utils.py +++ b/tests/utils/test_prep_utils.py @@ -483,6 +483,23 @@ def test_remove_columns_message_single_column(self): assert "✓ 1 column removed" in message assert "5 columns remaining" in message + def test_remove_columns_message_partial_failure(self): + """Test remove_columns message notes columns skipped as missing.""" + result = PrepActionResult( + action="remove column(s)", + source_columns=["col1", "col2"], + affected_count=2, + remaining_count=8, + failed_count=2, + additional_info="Columns not found and skipped: ['col3', 'col4']", + ) + + message = PrepConfirmationMessages.remove_columns(result) + + assert "✓ 2 columns removed" in message + assert '"col1", "col2"' in message + assert "Columns not found and skipped: ['col3', 'col4']" in message + def test_remove_rows_message(self): """Test remove_rows message generation.""" result = PrepActionResult( diff --git a/tests/views/test_prep_view.py b/tests/views/test_prep_view.py index 82cbb4bd..2d69b321 100644 --- a/tests/views/test_prep_view.py +++ b/tests/views/test_prep_view.py @@ -55,6 +55,7 @@ _get_column_options_for_condition, _get_unique_values_from_columns, _has_none_values, + _highlight_failed_status, _is_add_column_incomplete, _is_prep_form_incomplete, _is_remove_row_incomplete, @@ -1583,6 +1584,18 @@ def test_add_button_clicked(self, mock_prep_apply, sample_polars_df): _st.rerun.assert_called_once() +class TestHighlightFailedStatus: + """Test the Change Log status-cell styling helper.""" + + def test_failed_is_highlighted(self): + style = _highlight_failed_status("Failed") + assert "background-color" in style + assert "red" in style or "#dc3545" in style + + def test_successful_is_not_highlighted(self): + assert _highlight_failed_status("Successful") == "" + + class TestModuleLevelPageLayout: """Test the module-level page layout code by reloading the module.""" @@ -1672,6 +1685,92 @@ def test_page_layout_with_aliases(self): _st.session_state["st_project_id"] = None _st.stop = _orig_stop + def test_page_layout_with_failed_status_in_log(self): + """Change Log renders a status column and styles a Failed row.""" + import importlib + + import datasure.views.prep_view as pv_mod + + _st.session_state["st_project_id"] = "test_project" + _st.session_state["st_import_data_page"] = "import_page" + _st.session_state["st_config_checks_page"] = "config_page" + _st.stop = MagicMock() + + sample_df = pl.DataFrame({"name": ["Alice", "Bob"], "age": [25, 30]}) + # A log with one failed and one successful step (mixed status column) + prep_log_df = pl.DataFrame( + { + "action": ["remove column(s)", "add column"], + "description": [ + "✗ Failed to reapply: Columns not found: ['missing']", + "✓ 1 column added.", + ], + "status": ["Failed", "Successful"], + } + ) + + mock_tab = MagicMock() + mock_tab.__enter__ = MagicMock(return_value=mock_tab) + mock_tab.__exit__ = MagicMock(return_value=False) + _st.tabs = MagicMock(return_value=[mock_tab]) + + mock_col = MagicMock() + mock_col.__enter__ = MagicMock(return_value=mock_col) + mock_col.__exit__ = MagicMock(return_value=False) + _st.columns = MagicMock(return_value=[mock_col, mock_col, mock_col]) + + mock_container = MagicMock() + mock_container.__enter__ = MagicMock(return_value=mock_container) + mock_container.__exit__ = MagicMock(return_value=False) + _st.container = MagicMock(return_value=mock_container) + + mock_popover = MagicMock() + mock_popover.__enter__ = MagicMock(return_value=mock_popover) + mock_popover.__exit__ = MagicMock(return_value=False) + _st.popover = MagicMock(return_value=mock_popover) + + _st.button = MagicMock(return_value=False) + _st.selectbox = MagicMock(return_value=None) + _st.multiselect = MagicMock(return_value=[]) + _st.dataframe = MagicMock() + + with ( + patch( + "datasure.utils.duckdb_utils.duckdb_get_aliases", + return_value=["test_data"], + ), + patch( + "datasure.utils.duckdb_utils.duckdb_get_table", + side_effect=[ + prep_log_df, + sample_df, + prep_log_df, + prep_log_df, + ], + ), + patch("datasure.utils.duckdb_utils.duckdb_save_table"), + patch("datasure.utils.navigations_utils.page_navigation"), + patch("datasure.utils.navigations_utils.add_demo_navigation"), + patch("datasure.utils.navigations_utils.demo_sidebar_help"), + patch("datasure.utils.navigations_utils.demo_callout"), + patch("datasure.utils.navigations_utils.show_demo_next_action"), + patch( + "datasure.utils.onboarding_utils.is_demo_project", + return_value=False, + ), + patch("datasure.utils.onboarding_utils.demo_expander"), + patch("datasure.processing.prep.prep_apply_action"), + ): + importlib.reload(pv_mod) + + # The Change Log table (first st.dataframe call) is a styled pandas + # DataFrame with the status column positioned right after action + rendered = _st.dataframe.call_args_list[0][0][0] + assert list(rendered.data.columns) == ["action", "status", "description"] + + _st.session_state["st_project_id"] = None + _st.stop = _orig_stop + def test_page_layout_with_empty_prep_data(self): """Test when prep_data and prep_log are both empty (falls back to raw).""" import importlib From 7015e3622f3248bb0bef2c7a62e66e5ec0dd56bc Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 6 Aug 2026 18:03:55 +0000 Subject: [PATCH 03/11] feat/add text font color to status --- src/datasure/views/prep_view.py | 12 ++++++------ tests/views/test_prep_view.py | 21 +++++++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/datasure/views/prep_view.py b/src/datasure/views/prep_view.py index 782ca97e..dfacd916 100644 --- a/src/datasure/views/prep_view.py +++ b/src/datasure/views/prep_view.py @@ -991,10 +991,12 @@ def _remove_prep_step(action_index: str, log=prep_log, alias=label): ) -def _highlight_failed_status(value: str) -> str: - """Style a Change Log status cell; highlights failed steps in red.""" +def _highlight_status(value: str) -> str: + """Style a Change Log status cell: green text for Successful, red for Failed.""" if value == "Failed": - return "background-color: #dc3545; color: white; font-weight: 600" + return "color: #dc3545; font-weight: 600" + if value == "Successful": + return "color: #198754; font-weight: 600" return "" @@ -1101,9 +1103,7 @@ def _highlight_failed_status(value: str) -> str: ["action", "status", "description"] ].to_pandas() st.dataframe( - change_log.style.map( - _highlight_failed_status, subset=["status"] - ), + change_log.style.map(_highlight_status, subset=["status"]), width="stretch", key=label, hide_index=False, diff --git a/tests/views/test_prep_view.py b/tests/views/test_prep_view.py index 2d69b321..5689c9c9 100644 --- a/tests/views/test_prep_view.py +++ b/tests/views/test_prep_view.py @@ -55,7 +55,7 @@ _get_column_options_for_condition, _get_unique_values_from_columns, _has_none_values, - _highlight_failed_status, + _highlight_status, _is_add_column_incomplete, _is_prep_form_incomplete, _is_remove_row_incomplete, @@ -1584,16 +1584,21 @@ def test_add_button_clicked(self, mock_prep_apply, sample_polars_df): _st.rerun.assert_called_once() -class TestHighlightFailedStatus: +class TestHighlightStatus: """Test the Change Log status-cell styling helper.""" - def test_failed_is_highlighted(self): - style = _highlight_failed_status("Failed") - assert "background-color" in style - assert "red" in style or "#dc3545" in style + def test_failed_is_highlighted_red(self): + style = _highlight_status("Failed") + assert "background-color" not in style + assert "color: #dc3545" in style - def test_successful_is_not_highlighted(self): - assert _highlight_failed_status("Successful") == "" + def test_successful_is_highlighted_green(self): + style = _highlight_status("Successful") + assert "background-color" not in style + assert "color: #198754" in style + + def test_unknown_status_is_not_highlighted(self): + assert _highlight_status("") == "" class TestModuleLevelPageLayout: From 00006fab4a2fb6cd294f72bae2eacfc18d4ec825 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 6 Aug 2026 18:18:37 +0000 Subject: [PATCH 04/11] fix/correction log delays in rendering new entries --- src/datasure/processing/corrections.py | 3 ++ tests/processing/test_corrections.py | 45 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index 4afed679..bfa2aef9 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -198,6 +198,9 @@ def add_correction_entry( alias=f"corr_log_{alias}", db_name="logs", ) + # Clear correction log cache so the new entry shows immediately + self.get_correction_log.clear() + self.get_correction_summary.clear() def apply_correction( self, diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 266c5ad1..f7442d25 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -198,6 +198,51 @@ def test_add_correction_entry(self, correction_processor): assert saved_df["new_value"][0] == "Johnny" assert saved_df["KEY"][0] == "key1" + def test_add_correction_entry_clears_log_cache(self, correction_processor): + """Adding an entry must invalidate the cached log/summary. + + Without clearing the cache, the correction log and summary keep + serving the pre-add snapshot for up to the cache TTL, so a freshly + added correction doesn't show up right away. + """ + processor, mock_get, _ = correction_processor + + empty_log = pl.DataFrame( + { + "date": [], + "KEY": [], + "ID": [], + "action": [], + "column": [], + "current_value": [], + "new_value": [], + "reason": [], + } + ) + mock_get.return_value = empty_log + + # Prime the cache (simulates the log already having been displayed) + processor.get_correction_log("test_alias") + processor.get_correction_summary("test_alias") + calls_before = mock_get.call_count + + processor.add_correction_entry( + alias="test_alias", + key_value="key1", + current_id=None, + action="modify value", + column="name", + current_value="John", + new_value="Johnny", + reason="Name correction", + ) + + # A fresh read after adding must hit the database again, not the + # stale cached snapshot from before the correction was added. + processor.get_correction_log("test_alias") + processor.get_correction_summary("test_alias") + assert mock_get.call_count > calls_before + def test_add_correction_entry_with_existing_log( self, correction_processor, sample_corrections_log ): From 9379d9808604b14c4bc37d0b65b77639eede8c90 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 6 Aug 2026 19:13:32 +0000 Subject: [PATCH 05/11] feat/add correction status, and status reason to corrections log --- src/datasure/processing/corrections.py | 100 +++++++++++++++++- src/datasure/utils/reapply_utils.py | 13 +++ src/datasure/views/correction_view.py | 49 ++++++++- src/datasure/views/prep_view.py | 13 +-- tests/processing/test_corrections.py | 140 ++++++++++++++++++++++--- tests/utils/test_reapply_utils.py | 50 +++++++++ tests/views/test_correction_view.py | 57 ++++++++++ tests/views/test_prep_view.py | 18 ---- 8 files changed, 394 insertions(+), 46 deletions(-) create mode 100644 tests/utils/test_reapply_utils.py diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index bfa2aef9..7320895a 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -35,6 +35,17 @@ def _describe_correction_row(row: dict[str, Any]) -> str: return f"{action} for key {key_value}" +def _ensure_status_columns(df: pl.DataFrame) -> pl.DataFrame: + """Backfill status/status_reason columns for logs persisted before they existed.""" + if df.is_empty(): + return df + if "status" not in df.columns: + df = df.with_columns(pl.lit("Successful").alias("status")) + if "status_reason" not in df.columns: + df = df.with_columns(pl.lit(None, dtype=pl.String).alias("status_reason")) + return df + + class CorrectionProcessor: """Handles all data correction operations and persistence.""" @@ -157,7 +168,9 @@ def add_correction_entry( """ current_log = self.get_correction_log(alias) - # Create new entry DataFrame with proper schema + # Create new entry DataFrame with proper schema. A freshly added + # correction has just been applied successfully (apply_correction + # would have raised before reaching this point otherwise). new_entry_data = { "date": [datetime.now()], "KEY": [str(key_value)], @@ -169,8 +182,12 @@ def add_correction_entry( ], "new_value": [str(new_value) if new_value is not None else None], "reason": [str(reason)], + "status": ["Successful"], + "status_reason": [None], } - new_entry_df = pl.DataFrame(new_entry_data) + new_entry_df = pl.DataFrame(new_entry_data).with_columns( + pl.col("status_reason").cast(pl.String) + ) if current_log.is_empty(): # If no existing log, use the new entry schema @@ -178,7 +195,7 @@ def add_correction_entry( else: # Ensure schema compatibility before concatenating # Cast columns to match the new entry schema - aligned_current_log = current_log.with_columns( + aligned_current_log = _ensure_status_columns(current_log).with_columns( [ pl.col("date").cast(pl.Datetime("us")), pl.col("KEY").cast(pl.String), @@ -188,6 +205,8 @@ def add_correction_entry( pl.col("current_value").cast(pl.String), pl.col("new_value").cast(pl.String), pl.col("reason").cast(pl.String), + pl.col("status").cast(pl.String), + pl.col("status_reason").cast(pl.String), ] ) updated_log = pl.concat([aligned_current_log, new_entry_df]) @@ -595,12 +614,32 @@ def _reapply_all_corrections(self, alias: str) -> list[ReapplyFailure]: corrected_data = fresh_data failures: list[ReapplyFailure] = [] + statuses: list[str] = [] + status_reasons: list[str | None] = [] for row in correction_log.iter_rows(named=True): corrected_data, error = self._apply_correction_row(corrected_data, row) if error: + statuses.append("Failed") + status_reasons.append(error) failures.append( ReapplyFailure(step=_describe_correction_row(row), reason=error) ) + else: + statuses.append("Successful") + status_reasons.append(None) + + refreshed_log = correction_log.with_columns( + pl.Series("status", statuses, dtype=pl.String), + pl.Series("status_reason", status_reasons, dtype=pl.String), + ) + duckdb_save_table( + project_id=self.project_id, + table_data=refreshed_log, + alias=f"corr_log_{alias}", + db_name="logs", + ) + self.get_correction_log.clear() + self.get_correction_summary.clear() self.save_corrected_data(alias, corrected_data) return failures @@ -635,8 +674,19 @@ def _apply_correction_row( action = row["action"] column = row["column"] + recorded_value = row["current_value"] new_value = row["new_value"] + if action in ("modify value", "remove value") and column: + if column not in data.columns: + return data, f"Column '{column}' no longer available in the data" + + mismatch = self._current_value_mismatch( + data, key_col, key_value, column, recorded_value + ) + if mismatch: + return data, mismatch + try: if action == "modify value" and column and new_value is not None: return ( @@ -655,6 +705,50 @@ def _apply_correction_row( return data, None + @staticmethod + def _current_value_mismatch( + data: pl.DataFrame, + key_col: str, + key_value: str, + column: str, + recorded_value: str | None, + ) -> str | None: + """Check whether `column`'s value at `key_value` still matches what + was recorded when the correction was logged. + + Parameters + ---------- + data : pl.DataFrame + The data to check against + key_col : str + The key column name + key_value : str + The key value to match + column : str + The column the correction targets + recorded_value : str | None + The value recorded in the log at the time the correction was + made, or None if no value was recorded + + Returns + ------- + str | None + A description of the mismatch, or None if the value still + matches (or there is nothing recorded to compare against) + """ + if recorded_value is None: + return None + + actual_value = data.filter(pl.col(key_col) == key_value)[0, column] + actual_str = None if actual_value is None else str(actual_value) + + if actual_str != recorded_value: + return ( + f"Current value for '{column}' has changed since this correction " + f"was recorded (expected '{recorded_value}', found '{actual_str}')" + ) + return None + @staticmethod def _find_key_column(data: pl.DataFrame, key_value: str) -> str | None: """Find the first column containing the given key value. diff --git a/src/datasure/utils/reapply_utils.py b/src/datasure/utils/reapply_utils.py index 3becc7ef..549b32e1 100644 --- a/src/datasure/utils/reapply_utils.py +++ b/src/datasure/utils/reapply_utils.py @@ -20,6 +20,19 @@ class ReapplyFailure: reason: str +def highlight_status(value: str) -> str: + """Style a log status cell: green text for Successful, red for Failed. + + Used with a pandas ``Styler`` (``df.style.map(highlight_status, + subset=["status"])``) on the prep and correction Change Log tables. + """ + if value == "Failed": + return "color: #dc3545; font-weight: 600" + if value == "Successful": + return "color: #198754; font-weight: 600" + return "" + + def warn_reapply_failures(failures: list[ReapplyFailure], context: str) -> None: """Render one warning summarizing steps skipped during a bulk reapply. diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index e69ef14c..3468e33c 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -21,7 +21,7 @@ page_navigation, ) from datasure.utils.onboarding_utils import ImportDemoInfo, demo_expander -from datasure.utils.reapply_utils import warn_reapply_failures +from datasure.utils.reapply_utils import highlight_status, warn_reapply_failures from datasure.utils.settings_utils import get_check_config_settings from datasure.utils.ui_utils import ( confirm_dialog, @@ -889,6 +889,46 @@ def _handle_remove_correction( st.error(f"Error removing correction: {e!s}") +def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: + """Prepare a correction log for display in the Correction Log table. + + Backfills the status columns for logs saved before they existed, and + orders columns so status/status_reason sit right after action. + + Parameters + ---------- + correction_log : pl.DataFrame + The raw correction log, as persisted. + + Returns + ------- + pl.DataFrame + The log with status columns present, in display column order. + """ + if "status" not in correction_log.columns: + correction_log = correction_log.with_columns( + pl.lit("Successful").alias("status") + ) + if "status_reason" not in correction_log.columns: + correction_log = correction_log.with_columns( + pl.lit(None, dtype=pl.String).alias("status_reason") + ) + + display_columns = [ + "date", + "KEY", + "ID", + "action", + "status", + "status_reason", + "column", + "current_value", + "new_value", + "reason", + ] + return correction_log.select(display_columns) + + @st.fragment def render_correction_log( correction_processor: CorrectionProcessor, alias: str, tab_index: int @@ -915,7 +955,12 @@ def render_correction_log( ) else: section_header("Correction Log") - st.dataframe(data=correction_log, width="stretch") + + log_display = _build_correction_log_display(correction_log).to_pandas() + st.dataframe( + log_display.style.map(highlight_status, subset=["status"]), + width="stretch", + ) @st.fragment diff --git a/src/datasure/views/prep_view.py b/src/datasure/views/prep_view.py index dfacd916..a79ce3cc 100644 --- a/src/datasure/views/prep_view.py +++ b/src/datasure/views/prep_view.py @@ -38,7 +38,7 @@ PrepActionResult, PrepDescriptions, ) -from datasure.utils.reapply_utils import warn_reapply_failures +from datasure.utils.reapply_utils import highlight_status, warn_reapply_failures from datasure.utils.ui_utils import ( confirm_dialog, metric_row, @@ -991,15 +991,6 @@ def _remove_prep_step(action_index: str, log=prep_log, alias=label): ) -def _highlight_status(value: str) -> str: - """Style a Change Log status cell: green text for Successful, red for Failed.""" - if value == "Failed": - return "color: #dc3545; font-weight: 600" - if value == "Successful": - return "color: #198754; font-weight: 600" - return "" - - # === PAGE LAYOUT === # # -- DATA PREP PAGE --# @@ -1103,7 +1094,7 @@ def _highlight_status(value: str) -> str: ["action", "status", "description"] ].to_pandas() st.dataframe( - change_log.style.map(_highlight_status, subset=["status"]), + change_log.style.map(highlight_status, subset=["status"]), width="stretch", key=label, hide_index=False, diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index f7442d25..9d61d937 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -197,6 +197,9 @@ def test_add_correction_entry(self, correction_processor): assert saved_df["action"][0] == "modify value" assert saved_df["new_value"][0] == "Johnny" assert saved_df["KEY"][0] == "key1" + # A freshly applied correction is always logged as successful + assert saved_df["status"][0] == "Successful" + assert saved_df["status_reason"][0] is None def test_add_correction_entry_clears_log_cache(self, correction_processor): """Adding an entry must invalidate the cached log/summary. @@ -246,7 +249,11 @@ def test_add_correction_entry_clears_log_cache(self, correction_processor): def test_add_correction_entry_with_existing_log( self, correction_processor, sample_corrections_log ): - """Test adding correction entry to existing log.""" + """Test adding correction entry to existing log. + + `sample_corrections_log` predates the status columns, so this also + exercises backfilling a legacy log before concatenating the new row. + """ processor, mock_get, mock_save = correction_processor mock_get.return_value = sample_corrections_log @@ -267,6 +274,9 @@ def test_add_correction_entry_with_existing_log( call_args = mock_save.call_args saved_df = call_args[1]["table_data"] assert len(saved_df) == 4 # 3 original + 1 new + # Legacy rows backfilled, new row logged as successful + assert saved_df["status"].to_list() == ["Successful"] * 4 + assert saved_df["status_reason"].to_list() == [None] * 4 def test_apply_correction_modify_value_string( self, correction_processor, sample_data @@ -656,8 +666,9 @@ def test_remove_correction_entry( failures = processor.remove_correction_entry("test_alias", 1) - # Should save the updated log and reapplied data - assert mock_save.call_count == 2 + # Should save: the trimmed log, the reapply's refreshed log (with + # status), and the reapplied corrected data + assert mock_save.call_count == 3 assert failures == [] def test_remove_correction_entry_invalid_index( @@ -707,8 +718,8 @@ def test_reapply_all_corrections_empty_log(self, correction_processor, sample_da failures = processor._reapply_all_corrections("test_alias") - # Should save the fresh prep data as corrected data - mock_save.assert_called_once() + # Should save the refreshed (empty) log and the fresh prep data + assert mock_save.call_count == 2 assert failures == [] def test_reapply_all_corrections_with_data( @@ -721,13 +732,18 @@ def test_reapply_all_corrections_with_data( failures = processor._reapply_all_corrections("test_alias") - # Should save corrected data after applying all corrections - mock_save.assert_called_once() - call_args = mock_save.call_args + # Should save the refreshed log, then corrected data after applying + # all corrections + assert mock_save.call_count == 2 + call_args = mock_save.call_args_list[-1] assert call_args[1]["alias"] == "test_alias" assert call_args[1]["db_name"] == "corrected" assert failures == [] + # The refreshed log records every step as successful + saved_log = mock_save.call_args_list[0][1]["table_data"] + assert saved_log["status"].to_list() == ["Successful"] * 3 + def test_reapply_corrections_key_not_found(self, correction_processor, sample_data): """Test reapplying corrections when key value not found in data.""" processor, mock_get, mock_save = correction_processor @@ -751,11 +767,107 @@ def test_reapply_corrections_key_not_found(self, correction_processor, sample_da failures = processor._reapply_all_corrections("test_alias") # Should still save data even if some corrections fail - mock_save.assert_called_once() + assert mock_save.call_count == 2 # ... and the skipped correction should be reported, not swallowed assert len(failures) == 1 assert "nonexistent_key" in failures[0].reason + # ... and the log itself records the failure + saved_log = mock_save.call_args_list[0][1]["table_data"] + assert saved_log["status"][0] == "Failed" + assert "nonexistent_key" in saved_log["status_reason"][0] + + def test_reapply_corrections_column_no_longer_available( + self, correction_processor, sample_data + ): + """Failure reason 1: the targeted column was dropped upstream.""" + processor, mock_get, mock_save = correction_processor + + log = pl.DataFrame( + { + "date": [datetime.now()], + "KEY": ["key1"], + "ID": [None], + "action": ["modify value"], + "column": ["retired_column"], + "current_value": ["old"], + "new_value": ["new"], + "reason": ["test correction"], + } + ) + mock_get.side_effect = [sample_data, log] + + failures = processor._reapply_all_corrections("test_alias") + + assert len(failures) == 1 + assert "retired_column" in failures[0].reason + assert "no longer available" in failures[0].reason + + saved_log = mock_save.call_args_list[0][1]["table_data"] + assert saved_log["status"][0] == "Failed" + assert "no longer available" in saved_log["status_reason"][0] + + def test_reapply_corrections_current_value_changed( + self, correction_processor, sample_data + ): + """Failure reason 2: the value has changed since the correction was + logged, so blindly reapplying could clobber a legitimate update. + """ + processor, mock_get, mock_save = correction_processor + + # Recorded current_value ("Something Else") no longer matches + # sample_data's actual value for key1's name column ("John"). + log = pl.DataFrame( + { + "date": [datetime.now()], + "KEY": ["key1"], + "ID": [None], + "action": ["modify value"], + "column": ["name"], + "current_value": ["Something Else"], + "new_value": ["Johnny"], + "reason": ["test correction"], + } + ) + mock_get.side_effect = [sample_data, log] + + failures = processor._reapply_all_corrections("test_alias") + + assert len(failures) == 1 + assert "changed since this correction was recorded" in failures[0].reason + assert "Something Else" in failures[0].reason + assert "John" in failures[0].reason + + # The name column must be untouched since the correction was skipped + saved_data = mock_save.call_args_list[-1][1]["table_data"] + assert saved_data.filter(pl.col("survey_key") == "key1")["name"][0] == "John" + + def test_reapply_corrections_current_value_unchanged_still_applies( + self, correction_processor, sample_data + ): + """No recorded current_value, or a matching one, applies normally.""" + processor, mock_get, mock_save = correction_processor + + log = pl.DataFrame( + { + "date": [datetime.now()], + "KEY": ["key1"], + "ID": [None], + "action": ["modify value"], + "column": ["name"], + "current_value": ["John"], + "new_value": ["Johnny"], + "reason": ["test correction"], + } + ) + mock_get.side_effect = [sample_data, log] + + failures = processor._reapply_all_corrections("test_alias") + + assert failures == [] + saved_data = mock_save.call_args_list[-1][1]["table_data"] + assert saved_data.filter(pl.col("survey_key") == "key1")["name"][0] == "Johnny" + def test_reapply_corrections_partial_failure_continues( self, correction_processor, sample_data ): @@ -779,12 +891,15 @@ def test_reapply_corrections_partial_failure_continues( failures = processor._reapply_all_corrections("test_alias") - mock_save.assert_called_once() - saved_data = mock_save.call_args[1]["table_data"] + assert mock_save.call_count == 2 + saved_data = mock_save.call_args_list[-1][1]["table_data"] assert saved_data.filter(pl.col("survey_key") == "key1")["name"][0] == "Johnny" assert len(failures) == 1 assert "nonexistent_key" in failures[0].reason + saved_log = mock_save.call_args_list[0][1]["table_data"] + assert saved_log["status"].to_list() == ["Failed", "Successful"] + def test_reapply_corrections_exception_handling( self, correction_processor, sample_data ): @@ -811,10 +926,11 @@ def test_reapply_corrections_exception_handling( failures = processor._reapply_all_corrections("test_alias") # Should still save data - mock_save.assert_called_once() + assert mock_save.call_count == 2 # ... and report the skipped correction instead of swallowing it assert len(failures) == 1 assert failures[0].reason + assert "nonexistent_column" in failures[0].reason assert "key1" in failures[0].step def test_private_apply_modify_value_string(self, correction_processor, sample_data): diff --git a/tests/utils/test_reapply_utils.py b/tests/utils/test_reapply_utils.py new file mode 100644 index 00000000..136b53c9 --- /dev/null +++ b/tests/utils/test_reapply_utils.py @@ -0,0 +1,50 @@ +"""Test the reapply_utils module.""" + +from unittest.mock import patch + +from datasure.utils.reapply_utils import ( + ReapplyFailure, + highlight_status, + warn_reapply_failures, +) + + +class TestHighlightStatus: + """Test the shared Change Log status-cell styling helper.""" + + def test_failed_is_highlighted_red(self): + style = highlight_status("Failed") + assert "background-color" not in style + assert "color: #dc3545" in style + + def test_successful_is_highlighted_green(self): + style = highlight_status("Successful") + assert "background-color" not in style + assert "color: #198754" in style + + def test_unknown_status_is_not_highlighted(self): + assert highlight_status("") == "" + + +class TestWarnReapplyFailures: + """Test the shared bulk-reapply warning banner helper.""" + + def test_no_op_when_no_failures(self): + with patch("streamlit.warning") as mock_warning: + warn_reapply_failures([], "Some steps could not be reapplied") + mock_warning.assert_not_called() + + def test_renders_one_warning_summarizing_all_failures(self): + failures = [ + ReapplyFailure(step="Remove columns [a, b]", reason="Columns not found"), + ReapplyFailure(step="Modify value for key1", reason="Key not found"), + ] + with patch("streamlit.warning") as mock_warning: + warn_reapply_failures(failures, "Some steps could not be reapplied") + + mock_warning.assert_called_once() + message = mock_warning.call_args[0][0] + assert "Some steps could not be reapplied" in message + assert "2 skipped" in message + assert "Remove columns [a, b]: Columns not found" in message + assert "Modify value for key1: Key not found" in message diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index d717b37b..184edc86 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -2,6 +2,8 @@ import polars as pl +from datasure.views.correction_view import _build_correction_log_display + class TestCorrectionInputFormLogic: """Test the correction_input_form function logic patterns.""" @@ -350,3 +352,58 @@ def test_correction_form_database_interaction_logic(self): assert expected_project_id == "test_project_123" assert expected_alias == "survey_data" assert expected_db_name == "corrected" + + +class TestBuildCorrectionLogDisplay: + """Test _build_correction_log_display: status columns and ordering.""" + + def _base_log(self, **overrides) -> pl.DataFrame: + data = { + "date": ["2026-01-01"], + "KEY": ["key1"], + "ID": [None], + "action": ["modify value"], + "column": ["name"], + "current_value": ["John"], + "new_value": ["Johnny"], + "reason": ["typo"], + } + data.update(overrides) + return pl.DataFrame(data) + + def test_backfills_missing_status_columns(self): + """A legacy log without status columns gets defaults applied.""" + log = self._base_log() + + result = _build_correction_log_display(log) + + assert result["status"].to_list() == ["Successful"] + assert result["status_reason"].to_list() == [None] + + def test_preserves_existing_status_columns(self): + """An already-refreshed log keeps its real status/reason values.""" + log = self._base_log(status=["Failed"], status_reason=["Key not found"]) + + result = _build_correction_log_display(log) + + assert result["status"].to_list() == ["Failed"] + assert result["status_reason"].to_list() == ["Key not found"] + + def test_status_columns_ordered_right_after_action(self): + """status/status_reason are positioned right after action.""" + log = self._base_log() + + result = _build_correction_log_display(log) + + assert result.columns == [ + "date", + "KEY", + "ID", + "action", + "status", + "status_reason", + "column", + "current_value", + "new_value", + "reason", + ] diff --git a/tests/views/test_prep_view.py b/tests/views/test_prep_view.py index 5689c9c9..7fc052c6 100644 --- a/tests/views/test_prep_view.py +++ b/tests/views/test_prep_view.py @@ -55,7 +55,6 @@ _get_column_options_for_condition, _get_unique_values_from_columns, _has_none_values, - _highlight_status, _is_add_column_incomplete, _is_prep_form_incomplete, _is_remove_row_incomplete, @@ -1584,23 +1583,6 @@ def test_add_button_clicked(self, mock_prep_apply, sample_polars_df): _st.rerun.assert_called_once() -class TestHighlightStatus: - """Test the Change Log status-cell styling helper.""" - - def test_failed_is_highlighted_red(self): - style = _highlight_status("Failed") - assert "background-color" not in style - assert "color: #dc3545" in style - - def test_successful_is_highlighted_green(self): - style = _highlight_status("Successful") - assert "background-color" not in style - assert "color: #198754" in style - - def test_unknown_status_is_not_highlighted(self): - assert _highlight_status("") == "" - - class TestModuleLevelPageLayout: """Test the module-level page layout code by reloading the module.""" From 76f4af4c2834647dd3356f1c37223b785c54c33a Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 7 Aug 2026 09:08:45 +0000 Subject: [PATCH 06/11] Potential fix for pull request finding 'Module is imported with 'import' and 'import from'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- tests/views/test_prep_view.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/views/test_prep_view.py b/tests/views/test_prep_view.py index 7fc052c6..bb0ed20a 100644 --- a/tests/views/test_prep_view.py +++ b/tests/views/test_prep_view.py @@ -1676,8 +1676,6 @@ def test_page_layout_with_failed_status_in_log(self): """Change Log renders a status column and styles a Failed row.""" import importlib - import datasure.views.prep_view as pv_mod - _st.session_state["st_project_id"] = "test_project" _st.session_state["st_import_data_page"] = "import_page" _st.session_state["st_config_checks_page"] = "config_page" From 11b69e237038fce06b90d810b3ea275476ce15e6 Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 7 Aug 2026 09:25:32 +0000 Subject: [PATCH 07/11] fix: module import bug in test --- tests/views/test_prep_view.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/views/test_prep_view.py b/tests/views/test_prep_view.py index bb0ed20a..7fc052c6 100644 --- a/tests/views/test_prep_view.py +++ b/tests/views/test_prep_view.py @@ -1676,6 +1676,8 @@ def test_page_layout_with_failed_status_in_log(self): """Change Log renders a status column and styles a Failed row.""" import importlib + import datasure.views.prep_view as pv_mod + _st.session_state["st_project_id"] = "test_project" _st.session_state["st_import_data_page"] = "import_page" _st.session_state["st_config_checks_page"] = "config_page" From 51bd19d24a78ea9170a12af31af90c23874dd8ca Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 7 Aug 2026 14:55:49 +0000 Subject: [PATCH 08/11] feat: show SurveyID when key is selected on correction page --- src/datasure/views/correction_view.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 3468e33c..02185228 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -40,6 +40,9 @@ class TabConfig(BaseModel): page_name: str = Field(..., description="Name of the page/check") survey_data_name: str = Field(..., description="Name of the survey data alias") survey_key: str = Field(..., description="Name of the survey KEY column") + survey_id: str | None = Field( + None, description="Name of the survey ID column, if configured" + ) class CorrectionFormState(BaseModel): @@ -231,6 +234,7 @@ def load_tab_config(project_id: str, tab_index: int) -> TabConfig | None: page_name=page_config.get("page_name"), survey_data_name=page_config.get("survey_data_name"), survey_key=page_config.get("survey_key"), + survey_id=page_config.get("survey_id"), ) @@ -481,6 +485,7 @@ def render_add_correction_form( key_col: str, alias: str, tab_index: int, + survey_id_col: str | None = None, ) -> None: """ Render the add correction step form. @@ -498,6 +503,10 @@ def render_add_correction_form( The data alias/table name. tab_index : int The tab index for unique widget keys. + survey_id_col : str | None + The name of the configured Survey ID column, if any. When set (and + present in the data), the corresponding Survey ID is shown once a + KEY is selected. """ corrected_data = correction_processor.get_corrected_data(alias) @@ -519,6 +528,12 @@ def render_add_correction_form( if not corr_key_val: return + if survey_id_col and survey_id_col in corrected_data.columns: + survey_id_value = get_current_value( + corrected_data, key_col, corr_key_val, survey_id_col + ) + st.write(f"**Survey ID:** {survey_id_value}") + # Step 2: Select action corr_action = st.selectbox( label="Select Action", @@ -725,6 +740,7 @@ def render_correction_input_form( key_col: str, alias: str, tab_index: int, + survey_id_col: str | None = None, ) -> None: """ Render input form for corrections with add and remove functionality. @@ -739,6 +755,8 @@ def render_correction_input_form( The data alias/table name. tab_index : int The tab index for unique widget keys. + survey_id_col : str | None + The name of the configured Survey ID column, if any. """ corrected_data = correction_processor.get_corrected_data(alias) @@ -754,6 +772,7 @@ def render_correction_input_form( key_col=key_col, alias=alias, tab_index=tab_index, + survey_id_col=survey_id_col, ) with fc2: @@ -1032,6 +1051,7 @@ def render_correction_tab( key_col=config.survey_key, alias=config.survey_data_name, tab_index=tab_index, + survey_id_col=config.survey_id, ) render_correction_log( From dd8a794d4c4dc5be328ab27acaa8da3f30fe413e Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 7 Aug 2026 15:10:27 +0000 Subject: [PATCH 09/11] fix/show Survey ID in correction log --- src/datasure/processing/corrections.py | 9 +- src/datasure/views/correction_view.py | 24 +++- tests/processing/test_corrections.py | 67 +++++++++++ tests/views/test_correction_view.py | 160 ++++++++++++++++++++++++- 4 files changed, 252 insertions(+), 8 deletions(-) diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index 7320895a..a3b38a46 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -154,7 +154,8 @@ def add_correction_entry( key_value : str The key value being corrected current_id : str | None - The current ID value + The Survey ID value for this KEY, if a Survey ID column is + configured for the dataset action : str The correction action column : str | None @@ -231,6 +232,7 @@ def apply_correction( current_value: Any | None = None, new_value: Any | None = None, reason: str | None = None, + survey_id_value: Any | None = None, ) -> pl.DataFrame: """Apply a single correction to the data. @@ -252,6 +254,9 @@ def apply_correction( The new value reason : str | None The reason for correction + survey_id_value : Any | None + The Survey ID value for this KEY, if a Survey ID column is + configured, recorded in the log's ID column Returns ------- @@ -278,7 +283,7 @@ def apply_correction( self.add_correction_entry( alias=alias, key_value=key_value, - current_id=None, # Legacy field, not used in new implementation + current_id=survey_id_value, action=action, column=column, current_value=current_value, diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 02185228..16f958b0 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -528,6 +528,7 @@ def render_add_correction_form( if not corr_key_val: return + survey_id_value = None if survey_id_col and survey_id_col in corrected_data.columns: survey_id_value = get_current_value( corrected_data, key_col, corr_key_val, survey_id_col @@ -562,6 +563,7 @@ def render_add_correction_form( form_state=form_state, reason=reason, tab_index=tab_index, + survey_id_value=survey_id_value, ) @@ -615,6 +617,7 @@ def _render_apply_button( form_state: CorrectionFormState, reason: str, tab_index: int, + survey_id_value: Any = None, ) -> None: """ Render apply button and handle correction application. @@ -635,6 +638,9 @@ def _render_apply_button( Reason for correction. tab_index : int The tab index for unique widget keys. + survey_id_value : Any + The Survey ID value for the selected KEY, if a Survey ID column is + configured, to record alongside the correction log entry. """ apply_enabled = should_enable_apply_button( form_state.action, reason, form_state.new_value @@ -660,6 +666,7 @@ def _render_apply_button( current_value=form_state.current_value, new_value=form_state.new_value, reason=reason, + survey_id_value=survey_id_value, ) @@ -674,6 +681,7 @@ def _handle_apply_correction( current_value: Any, new_value: Any, reason: str, + survey_id_value: Any = None, ) -> None: """ Handle the application of a correction with validation. @@ -700,6 +708,9 @@ def _handle_apply_correction( The new value (if applicable). reason : str The reason for correction. + survey_id_value : Any + The Survey ID value for this KEY, if a Survey ID column is + configured, to record alongside the correction log entry. """ try: # Validate input @@ -726,6 +737,7 @@ def _handle_apply_correction( current_value=current_value, new_value=new_value, reason=reason, + survey_id_value=survey_id_value, ) st.success("Correction applied successfully!") @@ -911,8 +923,9 @@ def _handle_remove_correction( def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: """Prepare a correction log for display in the Correction Log table. - Backfills the status columns for logs saved before they existed, and - orders columns so status/status_reason sit right after action. + Backfills the status columns for logs saved before they existed, orders + columns so status/status_reason sit right after action, and relabels the + "ID" column as "Survey ID" for display. Parameters ---------- @@ -922,7 +935,8 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: Returns ------- pl.DataFrame - The log with status columns present, in display column order. + The log with status columns present, in display column order, ready + for display. """ if "status" not in correction_log.columns: correction_log = correction_log.with_columns( @@ -945,7 +959,9 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: "new_value", "reason", ] - return correction_log.select(display_columns) + # "ID" holds the Survey ID value recorded for the KEY, if one was + # configured - rename it for display so the column reads clearly. + return correction_log.select(display_columns).rename({"ID": "Survey ID"}) @st.fragment diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 9d61d937..10ad2e37 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -469,6 +469,73 @@ def test_apply_correction_without_reason(self, correction_processor, sample_data # Should only save data, not log (since no reason) assert mock_save.call_count == 1 + def test_apply_correction_records_survey_id( + self, correction_processor, sample_data + ): + """The Survey ID for the corrected KEY is recorded in the log's ID column.""" + processor, mock_get, mock_save = correction_processor + empty_log = pl.DataFrame( + { + "date": [], + "KEY": [], + "ID": [], + "action": [], + "column": [], + "current_value": [], + "new_value": [], + "reason": [], + } + ) + mock_get.side_effect = [sample_data, empty_log] + + processor.apply_correction( + alias="test_alias", + key_col="survey_key", + key_value="key2", + action="modify value", + column="name", + current_value="Jane", + new_value="Janet", + reason="Name correction", + survey_id_value="HH002", + ) + + saved_log = mock_save.call_args[1]["table_data"] + assert saved_log["ID"][0] == "HH002" + + def test_apply_correction_without_survey_id_leaves_id_blank( + self, correction_processor, sample_data + ): + """No Survey ID configured/available means the ID column stays blank.""" + processor, mock_get, mock_save = correction_processor + empty_log = pl.DataFrame( + { + "date": [], + "KEY": [], + "ID": [], + "action": [], + "column": [], + "current_value": [], + "new_value": [], + "reason": [], + } + ) + mock_get.side_effect = [sample_data, empty_log] + + processor.apply_correction( + alias="test_alias", + key_col="survey_key", + key_value="key2", + action="modify value", + column="name", + current_value="Jane", + new_value="Janet", + reason="Name correction", + ) + + saved_log = mock_save.call_args[1]["table_data"] + assert saved_log["ID"][0] is None + def test_get_data_summary(self, correction_processor, sample_data): """Test getting data summary.""" processor, _, _ = correction_processor diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index 184edc86..cd4511d7 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -1,8 +1,18 @@ """Tests for correction_view.py logic patterns.""" +import sys +from contextlib import contextmanager +from unittest.mock import MagicMock, patch + import polars as pl -from datasure.views.correction_view import _build_correction_log_display +from datasure.views.correction_view import ( + _build_correction_log_display, + load_tab_config, + render_add_correction_form, +) + +_st = sys.modules["streamlit"] class TestCorrectionInputFormLogic: @@ -398,7 +408,7 @@ def test_status_columns_ordered_right_after_action(self): assert result.columns == [ "date", "KEY", - "ID", + "Survey ID", "action", "status", "status_reason", @@ -407,3 +417,149 @@ def test_status_columns_ordered_right_after_action(self): "new_value", "reason", ] + + +class TestLoadTabConfig: + """Test that load_tab_config threads the configured Survey ID column.""" + + @patch("datasure.views.correction_view.get_check_config_settings") + def test_includes_survey_id_when_configured(self, mock_get_settings): + mock_get_settings.return_value = { + "page_name": "Household Survey", + "survey_data_name": "household_survey", + "survey_key": "KEY", + "survey_id": "hhid", + } + + config = load_tab_config("proj1", 0) + + assert config.survey_id == "hhid" + + @patch("datasure.views.correction_view.get_check_config_settings") + def test_survey_id_none_when_not_configured(self, mock_get_settings): + mock_get_settings.return_value = { + "page_name": "Household Survey", + "survey_data_name": "household_survey", + "survey_key": "KEY", + } + + config = load_tab_config("proj1", 0) + + assert config.survey_id is None + + +class TestRenderAddCorrectionFormSurveyId: + """Test the Survey ID display shown after a KEY is selected.""" + + def _mock_processor(self, data: pl.DataFrame) -> MagicMock: + processor = MagicMock() + processor.get_corrected_data.return_value = data + return processor + + @contextmanager + def _mocked_widgets( + self, selected_key: str, click_apply: bool = False, reason: str = "" + ): + """Mock the widgets this form uses, restoring originals afterward. + + `_st` is a single mock shared across every view test file, so + leaving these bound after the test (e.g. an exhausted list + `side_effect` on `selectbox`) can crash unrelated tests in + test_import_view.py that inherit the same mock later in the run. + """ + originals = { + "popover": _st.popover, + "markdown": _st.markdown, + "warning": _st.warning, + "write": _st.write, + "text_input": _st.text_input, + "button": _st.button, + "selectbox": _st.selectbox, + } + try: + mock_popover = MagicMock() + mock_popover.__enter__ = MagicMock(return_value=None) + mock_popover.__exit__ = MagicMock(return_value=False) + _st.popover = MagicMock(return_value=mock_popover) + _st.markdown = MagicMock() + _st.warning = MagicMock() + _st.write = MagicMock() + _st.text_input = MagicMock(return_value=reason) + _st.button = MagicMock(return_value=click_apply) + # First selectbox call selects the KEY, second selects the action. + _st.selectbox = MagicMock(side_effect=[selected_key, "remove row"]) + yield + finally: + for name, value in originals.items(): + setattr(_st, name, value) + + def test_shows_survey_id_when_configured(self): + """The configured Survey ID column's value is displayed for the KEY.""" + data = pl.DataFrame({"KEY": ["uuid:1", "uuid:2"], "hhid": ["HH001", "HH002"]}) + processor = self._mock_processor(data) + + with self._mocked_widgets(selected_key="uuid:1"): + render_add_correction_form( + correction_processor=processor, + key_col="KEY", + alias="survey", + tab_index=0, + survey_id_col="hhid", + ) + + written = [str(c.args[0]) for c in _st.write.call_args_list] + assert any("Survey ID" in text and "HH001" in text for text in written) + + def test_no_survey_id_display_when_not_configured(self): + """No Survey ID row is shown when no Survey ID column is configured.""" + data = pl.DataFrame({"KEY": ["uuid:1", "uuid:2"], "hhid": ["HH001", "HH002"]}) + processor = self._mock_processor(data) + + with self._mocked_widgets(selected_key="uuid:1"): + render_add_correction_form( + correction_processor=processor, + key_col="KEY", + alias="survey", + tab_index=0, + survey_id_col=None, + ) + + written = [str(c.args[0]) for c in _st.write.call_args_list] + assert not any("Survey ID" in text for text in written) + + def test_no_survey_id_display_when_column_missing_from_data(self): + """A configured but nonexistent Survey ID column is skipped, not an error.""" + data = pl.DataFrame({"KEY": ["uuid:1", "uuid:2"]}) + processor = self._mock_processor(data) + + with self._mocked_widgets(selected_key="uuid:1"): + render_add_correction_form( + correction_processor=processor, + key_col="KEY", + alias="survey", + tab_index=0, + survey_id_col="hhid", + ) + + written = [str(c.args[0]) for c in _st.write.call_args_list] + assert not any("Survey ID" in text for text in written) + + def test_apply_passes_survey_id_to_processor(self): + """Clicking Apply forwards the looked-up Survey ID to apply_correction.""" + data = pl.DataFrame({"KEY": ["uuid:1", "uuid:2"], "hhid": ["HH001", "HH002"]}) + processor = self._mock_processor(data) + processor.validate_correction_input.return_value = (True, "") + + with self._mocked_widgets( + selected_key="uuid:1", click_apply=True, reason="Test reason" + ): + render_add_correction_form( + correction_processor=processor, + key_col="KEY", + alias="survey", + tab_index=0, + survey_id_col="hhid", + ) + + processor.apply_correction.assert_called_once() + assert processor.apply_correction.call_args[1]["survey_id_value"] == "HH001" From b844d3d72a147158cb59308d73961d4212c4d237 Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 7 Aug 2026 15:30:27 +0000 Subject: [PATCH 10/11] chore: bump test coverage to 84% --- tests/views/test_correction_view.py | 642 ++++++++++++++++++++++++++++ 1 file changed, 642 insertions(+) diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index cd4511d7..811d8cf7 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -1,20 +1,70 @@ """Tests for correction_view.py logic patterns.""" +import datetime as dt import sys from contextlib import contextmanager from unittest.mock import MagicMock, patch import polars as pl +import pytest from datasure.views.correction_view import ( + CorrectionFormState, _build_correction_log_display, + _display_correction_details, + _handle_apply_correction, + _handle_remove_correction, + _render_action_ui, + _render_column_selector, + _render_modify_value_action, + _render_remove_row_action, + _render_remove_value_action, + get_current_value, + get_key_options, + load_hfc_config, load_tab_config, + main, + parse_date_value, render_add_correction_form, + render_correction_input_form, + render_page_header, + render_page_navigation, + render_value_input_widget, + should_enable_apply_button, + validate_numeric_input, + validate_prerequisites, ) _st = sys.modules["streamlit"] +@contextmanager +def _patched_st(**overrides): + """Temporarily set attributes on the shared streamlit mock. + + `_st` is a single mock shared across every view test file, so leaving + an override bound after a test (e.g. an exhausted list `side_effect`) + can crash unrelated tests elsewhere in the suite. This always restores + whatever was there before, even on failure. + """ + originals = {name: getattr(_st, name) for name in overrides} + try: + for name, value in overrides.items(): + setattr(_st, name, value) + yield + finally: + for name, value in originals.items(): + setattr(_st, name, value) + + +def _mock_context_widget() -> MagicMock: + """A MagicMock usable as a `with ...:` context manager target.""" + widget = MagicMock() + widget.__enter__ = MagicMock(return_value=widget) + widget.__exit__ = MagicMock(return_value=False) + return widget + + class TestCorrectionInputFormLogic: """Test the correction_input_form function logic patterns.""" @@ -456,6 +506,18 @@ def _mock_processor(self, data: pl.DataFrame) -> MagicMock: processor.get_corrected_data.return_value = data return processor + def test_empty_corrected_data_shows_warning(self): + processor = self._mock_processor(pl.DataFrame()) + + with _patched_st(warning=MagicMock()): + render_add_correction_form( + correction_processor=processor, + key_col="KEY", + alias="survey", + tab_index=0, + ) + assert _st.warning.called + @contextmanager def _mocked_widgets( self, selected_key: str, click_apply: bool = False, reason: str = "" @@ -563,3 +625,583 @@ def test_apply_passes_survey_id_to_processor(self): processor.apply_correction.assert_called_once() assert processor.apply_correction.call_args[1]["survey_id_value"] == "HH001" + + +class TestGetKeyOptions: + """Test get_key_options: unique key values, in first-seen order.""" + + def test_returns_unique_values_in_order(self): + data = pl.DataFrame({"KEY": ["b", "a", "b", "c"]}) + assert get_key_options(data, "KEY") == ["b", "a", "c"] + + def test_single_row(self): + data = pl.DataFrame({"KEY": ["only"]}) + assert get_key_options(data, "KEY") == ["only"] + + +class TestGetCurrentValue: + """Test get_current_value: lookup and graceful failure.""" + + def test_returns_value_when_found(self): + data = pl.DataFrame({"KEY": ["k1", "k2"], "name": ["Alice", "Bob"]}) + assert get_current_value(data, "KEY", "k2", "name") == "Bob" + + def test_key_not_found_does_not_raise(self): + """A key that doesn't exist yields an empty result, not a crash. + + In practice this path isn't reachable from the UI (the key selector + only ever offers values already present in the data), so this just + guards the try/except boundary rather than asserting a specific + "not found" sentinel. + """ + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + result = get_current_value(data, "KEY", "missing", "name") + assert result is None or len(result) == 0 + + def test_returns_none_for_nonexistent_column(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + assert get_current_value(data, "KEY", "k1", "nonexistent") is None + + +class TestParseDateValue: + """Test parse_date_value: string/datetime parsing and failure handling.""" + + def test_none_returns_none(self): + assert parse_date_value(None) is None + + def test_empty_string_returns_none(self): + assert parse_date_value("") is None + + def test_parses_iso_string(self): + assert parse_date_value("2024-01-15") == dt.date(2024, 1, 15) + + def test_parses_datetime_object(self): + assert parse_date_value(dt.datetime(2024, 1, 15, 10, 30)) == dt.date( + 2024, 1, 15 + ) + + def test_invalid_string_returns_none(self): + assert parse_date_value("not-a-date") is None + + def test_value_without_date_method_returns_none(self): + assert parse_date_value(12345) is None + + +class TestValidateNumericInput: + """Test validate_numeric_input across numeric and non-numeric dtypes.""" + + @pytest.mark.parametrize("dtype", [pl.Int64, pl.Int32, pl.Float64, pl.Float32]) + def test_valid_numeric_for_numeric_dtype(self, dtype): + is_valid, err = validate_numeric_input("42", dtype) + assert is_valid is True + assert err is None + + def test_invalid_numeric_for_numeric_dtype(self): + is_valid, err = validate_numeric_input("abc", pl.Float64) + assert is_valid is False + assert err == "New value must be a number." + + def test_non_numeric_dtype_always_valid(self): + is_valid, err = validate_numeric_input("anything at all", pl.Utf8) + assert is_valid is True + assert err is None + + +class TestShouldEnableApplyButton: + """Test should_enable_apply_button across every action/reason combination.""" + + def test_no_reason_disables_regardless_of_action(self): + assert should_enable_apply_button("remove row", "", None) is False + assert should_enable_apply_button("modify value", "", "new") is False + + def test_modify_value_requires_new_value(self): + assert should_enable_apply_button("modify value", "reason", None) is False + assert should_enable_apply_button("modify value", "reason", "") is False + assert should_enable_apply_button("modify value", "reason", "new") is True + + def test_remove_value_enabled_with_reason(self): + assert should_enable_apply_button("remove value", "reason") is True + + def test_remove_row_enabled_with_reason(self): + assert should_enable_apply_button("remove row", "reason") is True + + def test_unknown_action_disabled(self): + assert should_enable_apply_button("unknown action", "reason") is False + + +class TestLoadHfcConfig: + """Test load_hfc_config: empty vs populated check configuration.""" + + @patch("datasure.views.correction_view.duckdb_get_table") + def test_empty_config_returns_empty_and_no_pages(self, mock_get): + mock_get.return_value = pl.DataFrame() + + logs, pages = load_hfc_config("proj1") + + assert logs.is_empty() + assert pages == [] + + @patch("datasure.views.correction_view.duckdb_get_table") + def test_returns_page_names(self, mock_get): + mock_get.return_value = pl.DataFrame({"page_name": ["Page A", "Page B"]}) + + _logs, pages = load_hfc_config("proj1") + + assert pages == ["Page A", "Page B"] + mock_get.assert_called_once_with( + project_id="proj1", alias="check_config", db_name="logs" + ) + + +class TestValidatePrerequisites: + """Test validate_prerequisites: each st.stop() branch and the happy path.""" + + def test_no_project_id_stops(self): + with _patched_st(stop=MagicMock(side_effect=StopIteration), info=MagicMock()): + with pytest.raises(StopIteration): + validate_prerequisites(None) + assert _st.info.called + + @patch("datasure.views.correction_view.load_hfc_config") + def test_empty_hfc_config_stops(self, mock_load): + mock_load.return_value = (pl.DataFrame(), []) + + with ( + _patched_st(stop=MagicMock(side_effect=StopIteration), info=MagicMock()), + pytest.raises(StopIteration), + ): + validate_prerequisites("proj1") + + @patch("datasure.views.correction_view.load_hfc_config") + def test_empty_pages_stops(self, mock_load): + mock_load.return_value = (pl.DataFrame({"page_name": ["x"]}), []) + + with ( + _patched_st(stop=MagicMock(side_effect=StopIteration), info=MagicMock()), + pytest.raises(StopIteration), + ): + validate_prerequisites("proj1") + + @patch("datasure.views.correction_view.load_hfc_config") + def test_all_prerequisites_met_returns_data(self, mock_load): + mock_load.return_value = (pl.DataFrame({"page_name": ["x"]}), ["Page A"]) + + _logs, pages = validate_prerequisites("proj1") + + assert pages == ["Page A"] + + +class TestRenderValueInputWidget: + """Test render_value_input_widget: datetime vs text input, numeric validation.""" + + def test_datetime_dtype_uses_date_input(self): + with _patched_st(date_input=MagicMock(return_value=dt.date(2024, 1, 1))): + new_value, error = render_value_input_widget( + "col", pl.Datetime, "2023-06-01", tab_index=0 + ) + assert new_value == dt.date(2024, 1, 1) + assert error is None + + def test_non_datetime_valid_numeric(self): + with _patched_st(text_input=MagicMock(return_value="42")): + new_value, error = render_value_input_widget("col", pl.Int64, 10, 0) + assert new_value == "42" + assert error is None + + def test_non_datetime_invalid_numeric(self): + with _patched_st(text_input=MagicMock(return_value="abc")): + new_value, error = render_value_input_widget("col", pl.Int64, 10, 0) + assert new_value is None + assert error == "New value must be a number." + + def test_empty_new_value_skips_validation(self): + with _patched_st(text_input=MagicMock(return_value="")): + new_value, error = render_value_input_widget("col", pl.Int64, 10, 0) + assert new_value == "" + assert error is None + + def test_non_numeric_dtype_accepts_any_text(self): + with _patched_st(text_input=MagicMock(return_value="hello")): + new_value, error = render_value_input_widget("col", pl.Utf8, "old", 0) + assert new_value == "hello" + assert error is None + + +class TestRenderColumnSelector: + """Test _render_column_selector: column chosen vs left blank.""" + + def test_no_column_selected_returns_none_none(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value=None)): + column, value = _render_column_selector(data, "KEY", "k1", 0) + assert column is None + assert value is None + + def test_column_selected_returns_current_value(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value="name"), write=MagicMock()): + column, value = _render_column_selector(data, "KEY", "k1", 0) + assert column == "name" + assert value == "Alice" + + +class TestRenderModifyValueAction: + """Test _render_modify_value_action: no column vs a column selected.""" + + def test_no_column_selected(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value=None)): + state = _render_modify_value_action(data, "KEY", "k1", 0) + + assert isinstance(state, CorrectionFormState) + assert state.action == "modify value" + assert state.column is None + + def test_column_selected_builds_full_state(self): + data = pl.DataFrame({"KEY": ["k1"], "age": [25]}) + with _patched_st( + selectbox=MagicMock(return_value="age"), + write=MagicMock(), + text_input=MagicMock(return_value="30"), + ): + state = _render_modify_value_action(data, "KEY", "k1", 0) + + assert state.column == "age" + assert state.current_value == 25 + assert state.new_value == "30" + assert state.validation_error is None + + def test_column_selected_with_validation_error(self): + data = pl.DataFrame({"KEY": ["k1"], "age": [25]}) + with _patched_st( + selectbox=MagicMock(return_value="age"), + write=MagicMock(), + text_input=MagicMock(return_value="not-a-number"), + error=MagicMock(), + ): + state = _render_modify_value_action(data, "KEY", "k1", 0) + assert _st.error.called + + assert state.validation_error == "New value must be a number." + + +class TestRenderRemoveValueAction: + """Test _render_remove_value_action.""" + + def test_builds_remove_value_state(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value="name"), write=MagicMock()): + state = _render_remove_value_action(data, "KEY", "k1", 0) + + assert state.action == "remove value" + assert state.column == "name" + assert state.current_value == "Alice" + + +class TestRenderRemoveRowAction: + """Test _render_remove_row_action.""" + + def test_builds_remove_row_state_and_warns(self): + with _patched_st(warning=MagicMock()): + state = _render_remove_row_action("k1") + assert _st.warning.called + + assert state.action == "remove row" + assert state.key_value == "k1" + + +class TestRenderActionUi: + """Test _render_action_ui dispatches to the right per-action handler.""" + + def test_dispatches_modify_value(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value=None)): + state = _render_action_ui("modify value", data, "KEY", "k1", 0) + assert state.action == "modify value" + + def test_dispatches_remove_value(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(selectbox=MagicMock(return_value=None), write=MagicMock()): + state = _render_action_ui("remove value", data, "KEY", "k1", 0) + assert state.action == "remove value" + + def test_dispatches_remove_row(self): + data = pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}) + with _patched_st(warning=MagicMock()): + state = _render_action_ui("remove row", data, "KEY", "k1", 0) + assert state.action == "remove row" + + +class TestHandleApplyCorrection: + """Test _handle_apply_correction: validation failure, success, exception.""" + + def _base_kwargs(self, processor): + return dict( + correction_processor=processor, + corrected_data=pl.DataFrame({"KEY": ["k1"], "name": ["Alice"]}), + alias="survey", + key_col="KEY", + key_value="k1", + action="modify value", + column="name", + current_value="Alice", + new_value="Alicia", + reason="typo fix", + ) + + def test_validation_error_shows_error_and_does_not_apply(self): + processor = MagicMock() + processor.validate_correction_input.return_value = (False, "bad input") + + with _patched_st(error=MagicMock(), success=MagicMock(), rerun=MagicMock()): + _handle_apply_correction(**self._base_kwargs(processor)) + + assert _st.error.called + assert "bad input" in _st.error.call_args[0][0] + processor.apply_correction.assert_not_called() + assert not _st.success.called + + def test_success_applies_and_reruns(self): + processor = MagicMock() + processor.validate_correction_input.return_value = (True, "") + + with _patched_st(error=MagicMock(), success=MagicMock(), rerun=MagicMock()): + _handle_apply_correction( + survey_id_value="HH001", **self._base_kwargs(processor) + ) + + processor.apply_correction.assert_called_once() + assert processor.apply_correction.call_args[1]["survey_id_value"] == ( + "HH001" + ) + assert _st.success.called + assert _st.rerun.called + + def test_exception_during_apply_shows_error(self): + processor = MagicMock() + processor.validate_correction_input.return_value = (True, "") + processor.apply_correction.side_effect = RuntimeError("boom") + + with _patched_st(error=MagicMock(), success=MagicMock(), rerun=MagicMock()): + _handle_apply_correction(**self._base_kwargs(processor)) + + assert _st.error.called + assert "boom" in _st.error.call_args[0][0] + assert not _st.success.called + + +class TestRenderCorrectionInputForm: + """Test render_correction_input_form: empty vs populated corrected data.""" + + def test_empty_data_shows_warning(self): + processor = MagicMock() + processor.get_corrected_data.return_value = pl.DataFrame() + + with _patched_st(warning=MagicMock()): + render_correction_input_form(processor, "KEY", "survey", 0) + assert _st.warning.called + + def test_populated_data_renders_add_form_and_calls_remove_form(self): + """render_remove_correction_form is `@st.fragment`-wrapped, so under + the test harness's mock it becomes an opaque callable - this only + confirms it's invoked with the right args, not its internal + behavior (covered separately if extracted, per the existing + `@st.fragment` testing limitation noted elsewhere in this suite). + """ + processor = MagicMock() + processor.get_corrected_data.return_value = pl.DataFrame( + {"KEY": ["k1"], "name": ["Alice"]} + ) + + with ( + _patched_st( + columns=MagicMock( + return_value=[ + _mock_context_widget(), + _mock_context_widget(), + _mock_context_widget(), + ] + ), + popover=MagicMock(return_value=_mock_context_widget()), + selectbox=MagicMock(return_value=None), + markdown=MagicMock(), + info=MagicMock(), + ), + patch( + "datasure.views.correction_view.render_remove_correction_form" + ) as mock_remove_form, + ): + render_correction_input_form(processor, "KEY", "survey", 0) + + processor.get_corrected_data.assert_called() + mock_remove_form.assert_called_once_with( + correction_processor=processor, alias="survey", tab_index=0 + ) + + +class TestDisplayCorrectionDetails: + """Test _display_correction_details renders the selected summary's fields.""" + + def test_shows_all_optional_fields_when_present(self): + summaries = [ + { + "action_index": "0 - modify value - x", + "action": "modify value", + "key_value": "k1", + "column": "name", + "new_value": "Alicia", + "reason": "typo fix", + } + ] + + with _patched_st(write=MagicMock()): + _display_correction_details(summaries, "0 - modify value - x") + written = [str(c.args[0]) for c in _st.write.call_args_list] + + assert any("modify value" in t for t in written) + assert any("k1" in t for t in written) + assert any("name" in t for t in written) + assert any("Alicia" in t for t in written) + assert any("typo fix" in t for t in written) + + def test_skips_absent_optional_fields(self): + summaries = [ + { + "action_index": "0 - remove row - x", + "action": "remove row", + "key_value": "k1", + "column": None, + "new_value": None, + "reason": "duplicate", + } + ] + + with _patched_st(write=MagicMock()): + _display_correction_details(summaries, "0 - remove row - x") + written = [str(c.args[0]) for c in _st.write.call_args_list] + + assert not any("Column" in t for t in written) + assert not any("New Value" in t for t in written) + + +class TestHandleRemoveCorrection: + """Test _handle_remove_correction: success and exception paths.""" + + def test_success_removes_and_reruns(self): + processor = MagicMock() + processor.remove_correction_entry.return_value = [] + summaries = [{"action_index": "0 - remove row - x", "index": 0}] + + with _patched_st(success=MagicMock(), rerun=MagicMock(), warning=MagicMock()): + _handle_remove_correction( + processor, summaries, "survey", "0 - remove row - x" + ) + + processor.remove_correction_entry.assert_called_once_with("survey", 0) + assert _st.success.called + assert _st.rerun.called + + def test_reapply_failures_trigger_warning(self): + from datasure.utils.reapply_utils import ReapplyFailure + + processor = MagicMock() + processor.remove_correction_entry.return_value = [ + ReapplyFailure(step="Modify name for key2", reason="Key not found") + ] + summaries = [{"action_index": "0 - remove row - x", "index": 0}] + + with _patched_st(success=MagicMock(), rerun=MagicMock(), warning=MagicMock()): + _handle_remove_correction( + processor, summaries, "survey", "0 - remove row - x" + ) + + assert _st.warning.called + + def test_exception_shows_error(self): + processor = MagicMock() + processor.remove_correction_entry.side_effect = RuntimeError("db error") + summaries = [{"action_index": "0 - remove row - x", "index": 0}] + + with _patched_st(success=MagicMock(), rerun=MagicMock(), error=MagicMock()): + _handle_remove_correction( + processor, summaries, "survey", "0 - remove row - x" + ) + + assert _st.error.called + assert "db error" in _st.error.call_args[0][0] + + +class TestRenderPageHeaderAndNavigation: + """Test the small page-chrome rendering functions.""" + + @patch("datasure.views.correction_view.demo_expander") + @patch("datasure.views.correction_view.page_header") + def test_render_page_header(self, mock_page_header, mock_demo_expander): + render_page_header() + + mock_page_header.assert_called_once() + mock_demo_expander.assert_called_once() + + @patch("datasure.views.correction_view.page_navigation") + def test_render_page_navigation_without_replication_page(self, mock_nav): + with _patched_st(session_state={"st_output_page1": "output_view_1"}): + render_page_navigation() + + mock_nav.assert_called_once() + assert mock_nav.call_args[1]["next"] is None + + @patch("datasure.views.correction_view.page_navigation") + def test_render_page_navigation_with_replication_page(self, mock_nav): + with _patched_st( + session_state={ + "st_output_page1": "output_view_1", + "st_replication_page": "replication_view", + } + ): + render_page_navigation() + + mock_nav.assert_called_once() + assert mock_nav.call_args[1]["next"] is not None + + +class TestMain: + """Test the main() entry point orchestration.""" + + @patch("datasure.views.correction_view.render_page_navigation") + @patch("datasure.views.correction_view.render_correction_tab") + @patch("datasure.views.correction_view.CorrectionProcessor") + @patch("datasure.views.correction_view.validate_prerequisites") + @patch("datasure.views.correction_view.render_page_header") + @patch("datasure.views.correction_view.add_demo_navigation") + @patch("datasure.views.correction_view.demo_sidebar_help") + def test_renders_a_tab_per_hfc_page( + self, + mock_demo_sidebar, + mock_add_demo_nav, + mock_page_header, + mock_validate, + mock_processor_cls, + mock_render_tab, + mock_page_nav, + ): + mock_validate.return_value = (pl.DataFrame({"x": [1]}), ["Page A", "Page B"]) + mock_processor_instance = MagicMock() + mock_processor_cls.return_value = mock_processor_instance + + with _patched_st( + session_state={"st_project_id": "proj1"}, + tabs=MagicMock( + return_value=[_mock_context_widget(), _mock_context_widget()] + ), + ): + main() + + mock_demo_sidebar.assert_called_once() + mock_add_demo_nav.assert_called_once_with("correction_view", step=6) + mock_page_header.assert_called_once() + mock_validate.assert_called_once_with("proj1") + mock_processor_cls.assert_called_once_with("proj1") + assert mock_render_tab.call_count == 2 + mock_render_tab.assert_any_call(mock_processor_instance, "proj1", 0) + mock_render_tab.assert_any_call(mock_processor_instance, "proj1", 1) + mock_page_nav.assert_called_once() From 56016746b8a38180427fc663aa8a1fb8e80740be Mon Sep 17 00:00:00 2001 From: iabaako Date: Tue, 18 Aug 2026 19:01:38 +0000 Subject: [PATCH 11/11] chore: fix linting --- tests/views/test_correction_view.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index df28b446..811d8cf7 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -64,8 +64,6 @@ def _mock_context_widget() -> MagicMock: widget.__exit__ = MagicMock(return_value=False) return widget -from datasure.views.correction_view import _build_correction_log_display - class TestCorrectionInputFormLogic: """Test the correction_input_form function logic patterns."""