From e8259e89b8c7c4f99a366fa2e09aed73d3449b29 Mon Sep 17 00:00:00 2001 From: iabaako Date: Sat, 3 Oct 2026 19:37:04 +0100 Subject: [PATCH 1/3] fix(backchecks): pass duplicate option and target to BackcheckSettings backchecks_report_settings passed `drop_duplicates` and `backcheck_goal`, which BackcheckSettings silently ignored, so duplicates were always dropped and the target was always 10. Use the model field names, and save the target under `backcheck_target_percent` so it reloads. Closes #299 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++ src/datasure/checks/backchecks/settings_ui.py | 6 +- tests/checks/backchecks/test_settings_ui.py | 65 +++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1263af8a..cca2328c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 logs. The processor is now hashed by `project_id` — #296 - **Apply button**: A new value of `0` no longer disables Apply. An empty string still does; use "remove value" to blank a cell — #296 +- **Backcheck settings**: `backchecks_report_settings` passed the duplicate + option and target as `drop_duplicates` and `backcheck_goal`, which + `BackcheckSettings` silently dropped, so duplicates were always dropped and + the target was always 10. They are now passed as `drop_duplicates_option` + and `backcheck_target_percent`, and the target is saved under + `backcheck_target_percent` so it reloads in later sessions — #299 - **Correction log schema**: Removing the last correction entry now leaves an empty log with the full schema, including status columns — #296 - **Constraint violations**: A value past a hard bound was reported as a soft diff --git a/src/datasure/checks/backchecks/settings_ui.py b/src/datasure/checks/backchecks/settings_ui.py index 4618c809..05552685 100644 --- a/src/datasure/checks/backchecks/settings_ui.py +++ b/src/datasure/checks/backchecks/settings_ui.py @@ -277,7 +277,7 @@ def _render_tracking_options( kwargs={"state_name": TAB_NAME + "_backcheck_goal"}, ) save_check_settings( - settings_file, TAB_NAME, {"backcheck_goal": backcheck_goal} + settings_file, TAB_NAME, {"backcheck_target_percent": backcheck_goal} ) return backcheck_goal @@ -516,8 +516,8 @@ def backchecks_report_settings( backcheck_date=backcheck_date, enumerator=enumerator, backchecker=backchecker, - backcheck_goal=backcheck_goal, - drop_duplicates=drop_duplicates_option, + backcheck_target_percent=backcheck_goal, + drop_duplicates_option=drop_duplicates_option, no_differences_list=no_diff_values, exclude_values_list=exclude_values, case_option=string_comp_options.case_option, diff --git a/tests/checks/backchecks/test_settings_ui.py b/tests/checks/backchecks/test_settings_ui.py index 78b05258..b7c62d49 100644 --- a/tests/checks/backchecks/test_settings_ui.py +++ b/tests/checks/backchecks/test_settings_ui.py @@ -4,6 +4,7 @@ import sys from unittest.mock import MagicMock, patch +import pandas as pd import pytest from datasure.checks.backchecks.models import BackcheckSettings, StrCompareOptions @@ -17,6 +18,7 @@ _render_survey_identifiers, _render_tracking_options, _render_value_list_display, + backchecks_report_settings, ) from tests.checks.backchecks.conftest import make_mock_st @@ -227,3 +229,66 @@ def test_render_exclude_values_settings_fragment(bc): """_render_exclude_values_settings runs without error and returns a list.""" result = bc._render_exclude_values_settings("settings.json") assert isinstance(result, list) + + +# ============================================ +# backchecks_report_settings: user choices reach the returned settings +# ============================================ + + +@pytest.fixture +def report_settings_with_choices(patched_bc): + """Run backchecks_report_settings with non-default UI selections.""" + module = "datasure.checks.backchecks.settings_ui" + with ( + patch( + f"{module}.load_default_backchecks_settings", + return_value=BackcheckSettings(survey_key=None), + ), + patch(f"{module}._render_survey_identifiers", return_value=("key", "sid")), + patch(f"{module}._render_date_columns", return_value=(None, None)), + patch(f"{module}._render_staff_identifiers", return_value=(None, None)), + patch(f"{module}._render_tracking_options", return_value=35), + patch( + f"{module}._render_additional_options", + return_value=("last", [], [], StrCompareOptions()), + ), + ): + yield backchecks_report_settings( + "project", + "settings.json", + pd.DataFrame(), + pd.DataFrame(), + BackcheckSettings(survey_key=None), + [], + [], + [], + [], + ) + + +def test_report_settings_keeps_selected_duplicate_option( + report_settings_with_choices, +): + """A non-default duplicate-handling choice is not replaced by the default.""" + assert report_settings_with_choices.drop_duplicates_option == "last" + + +def test_report_settings_keeps_selected_target_percent( + report_settings_with_choices, +): + """A non-default backcheck target is not replaced by the default.""" + assert report_settings_with_choices.backcheck_target_percent == 35 + + +def test_render_tracking_options_saves_under_model_field_name(patched_bc): + """The target is saved under the key the settings model loads it from.""" + patched_bc.number_input.return_value = 35 + with patch( + "datasure.checks.backchecks.settings_ui.save_check_settings" + ) as mock_save: + _render_tracking_options("settings.json", BackcheckSettings(survey_key=None)) + + mock_save.assert_called_once_with( + "settings.json", "backchecks", {"backcheck_target_percent": 35} + ) From 69cbf8013d82c7c7980d047b5cc9b69ac432386b Mon Sep 17 00:00:00 2001 From: iabaako Date: Sat, 3 Oct 2026 19:38:35 +0100 Subject: [PATCH 2/3] refactor(backchecks): name the target local after its model field Rename `backcheck_goal` to `backcheck_target_percent` so the local matches the BackcheckSettings field it feeds, and note in the changelog that targets saved under the old key are ignored. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++- src/datasure/checks/backchecks/settings_ui.py | 16 ++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cca2328c..cce6456a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,7 +95,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `BackcheckSettings` silently dropped, so duplicates were always dropped and the target was always 10. They are now passed as `drop_duplicates_option` and `backcheck_target_percent`, and the target is saved under - `backcheck_target_percent` so it reloads in later sessions — #299 + `backcheck_target_percent` so it reloads in later sessions. Targets saved + under the old `backcheck_goal` key were never applied and are ignored — #299 - **Correction log schema**: Removing the last correction entry now leaves an empty log with the full schema, including status columns — #296 - **Constraint violations**: A value past a hard bound was reported as a soft diff --git a/src/datasure/checks/backchecks/settings_ui.py b/src/datasure/checks/backchecks/settings_ui.py index 05552685..a7fee2f3 100644 --- a/src/datasure/checks/backchecks/settings_ui.py +++ b/src/datasure/checks/backchecks/settings_ui.py @@ -260,14 +260,14 @@ def _render_tracking_options( Returns ------- int - Backcheck goal. + Backcheck target percent. """ with st.container(border=True): st.subheader("Tracking Options") to1, _, _ = st.columns(3) with to1: - backcheck_goal = st.number_input( + backcheck_target_percent = st.number_input( "Target number of backchecks", min_value=0, help="Total number of backchecks expected", @@ -277,10 +277,12 @@ def _render_tracking_options( kwargs={"state_name": TAB_NAME + "_backcheck_goal"}, ) save_check_settings( - settings_file, TAB_NAME, {"backcheck_target_percent": backcheck_goal} + settings_file, + TAB_NAME, + {"backcheck_target_percent": backcheck_target_percent}, ) - return backcheck_goal + return backcheck_target_percent def _render_duplicate_handling( @@ -500,7 +502,9 @@ def backchecks_report_settings( backcheck_categorical_columns, ) - backcheck_goal = _render_tracking_options(settings_file, default_settings) + backcheck_target_percent = _render_tracking_options( + settings_file, default_settings + ) ( drop_duplicates_option, @@ -516,7 +520,7 @@ def backchecks_report_settings( backcheck_date=backcheck_date, enumerator=enumerator, backchecker=backchecker, - backcheck_target_percent=backcheck_goal, + backcheck_target_percent=backcheck_target_percent, drop_duplicates_option=drop_duplicates_option, no_differences_list=no_diff_values, exclude_values_list=exclude_values, From 6ca1226609513ba2c8e1bad9045437684e65abb3 Mon Sep 17 00:00:00 2001 From: iabaako Date: Sun, 4 Oct 2026 08:55:10 +0100 Subject: [PATCH 3/3] fix(backchecks): set the save flag the target's save guard checks save_check_settings only writes when `backchecks_` is set in session state. The key is now `backcheck_target_percent`, but the widget callback still set `backchecks_backcheck_goal`, so the target was never saved. Test the round trip through the real save guard instead of mocking the save. Co-Authored-By: Claude Opus 5.5 --- src/datasure/checks/backchecks/settings_ui.py | 2 +- tests/checks/backchecks/test_settings_ui.py | 31 +++++++++++++------ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/datasure/checks/backchecks/settings_ui.py b/src/datasure/checks/backchecks/settings_ui.py index a7fee2f3..92901eee 100644 --- a/src/datasure/checks/backchecks/settings_ui.py +++ b/src/datasure/checks/backchecks/settings_ui.py @@ -274,7 +274,7 @@ def _render_tracking_options( key="backcheck_goal_backchecks", value=default_settings.backcheck_target_percent, on_change=trigger_save, - kwargs={"state_name": TAB_NAME + "_backcheck_goal"}, + kwargs={"state_name": TAB_NAME + "_backcheck_target_percent"}, ) save_check_settings( settings_file, diff --git a/tests/checks/backchecks/test_settings_ui.py b/tests/checks/backchecks/test_settings_ui.py index b7c62d49..97f53262 100644 --- a/tests/checks/backchecks/test_settings_ui.py +++ b/tests/checks/backchecks/test_settings_ui.py @@ -20,6 +20,7 @@ _render_value_list_display, backchecks_report_settings, ) +from datasure.utils.settings_utils import load_check_settings from tests.checks.backchecks.conftest import make_mock_st # ============================================ @@ -281,14 +282,24 @@ def test_report_settings_keeps_selected_target_percent( assert report_settings_with_choices.backcheck_target_percent == 35 -def test_render_tracking_options_saves_under_model_field_name(patched_bc): - """The target is saved under the key the settings model loads it from.""" - patched_bc.number_input.return_value = 35 - with patch( - "datasure.checks.backchecks.settings_ui.save_check_settings" - ) as mock_save: - _render_tracking_options("settings.json", BackcheckSettings(survey_key=None)) +def test_render_tracking_options_persists_changed_target(tmp_path): + """A changed target passes the real save guard and reloads from disk.""" + settings_file = str(tmp_path / "settings.json") + session_state: dict = {} + mock_st = make_mock_st() + mock_st.session_state = session_state - mock_save.assert_called_once_with( - "settings.json", "backchecks", {"backcheck_target_percent": 35} - ) + def change_target(*_args, on_change, kwargs, **_widget_kwargs): + on_change(**kwargs) + return 35 + + mock_st.number_input.side_effect = change_target + with ( + patch("datasure.checks.backchecks.settings_ui.st", mock_st), + patch("datasure.utils.settings_utils.st", mock_st), + ): + _render_tracking_options(settings_file, BackcheckSettings(survey_key=None)) + + assert load_check_settings(settings_file, "backchecks") == { + "backcheck_target_percent": 35 + }