From 4d918940bd045f669d8111b5424b34f796ba489e Mon Sep 17 00:00:00 2001 From: iabaako Date: Tue, 6 Oct 2026 09:54:29 +0000 Subject: [PATCH 1/3] feat(corrections): record who made each correction log entry Add a `user` column to `corr_log_{alias}`. Every entry, from the Corrections page or a check page, records the reviewer: the "Reviewer name" set in the sidebar, else the OS login. The name is saved in cache/user_settings.json and remembered across sessions. Logs saved before this change load with an empty user, and the Correction Log table and the replication package's correction_log.csv include it. Closes #321 Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 3 + docs/USER_GUIDE.md | 3 + src/datasure/app.py | 2 + src/datasure/processing/correction_log.py | 5 +- src/datasure/processing/corrections.py | 5 +- src/datasure/utils/reviewer.py | 89 +++++++++++++++++++ src/datasure/views/correction_view.py | 5 +- tests/processing/test_corrections.py | 101 ++++++++++++++++++++++ tests/replication/test_package_builder.py | 10 ++- tests/utils/test_reviewer.py | 77 +++++++++++++++++ tests/views/test_correction_view.py | 11 +++ 11 files changed, 304 insertions(+), 7 deletions(-) create mode 100644 src/datasure/utils/reviewer.py create mode 100644 tests/utils/test_reviewer.py diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index bc29b1f6..e66ce4e8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -99,6 +99,9 @@ Per project (UUID-keyed): - `cache/{project_id}/settings/` — `logs.duckdb` (import/prep logs), JSON settings, and credential metadata - `cache/projects.json` — the project registry +- `cache/user_settings.json` — per-user preferences, currently the + "Reviewer name" recorded as `user` in correction logs + (`utils/reviewer.py`). Display names only, never credentials. The `cache/` directory is gitignored. diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 72343026..3c4395d1 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -462,6 +462,9 @@ All corrections are tracked with: - Timestamp - Status of the last reapply, with the reason if it failed - Source: the page that made the entry (`corrections_page` for this page) +- User: who made the entry. This is your computer login unless you set a + **Reviewer name** in the sidebar, which DataSure remembers across sessions. + Entries logged before DataSure recorded the user show an empty User. The log can also contain **accept** entries. An accept entry records that a flagged value was reviewed and is correct. It never changes the data, and the diff --git a/src/datasure/app.py b/src/datasure/app.py index 75b9fca6..71a818a2 100644 --- a/src/datasure/app.py +++ b/src/datasure/app.py @@ -6,6 +6,7 @@ import streamlit as st from datasure.utils.config_utils import ConfigurationService +from datasure.utils.reviewer import render_reviewer_setting @st.cache_data @@ -182,6 +183,7 @@ def _image_data_uri(path: str) -> str: with st.sidebar: st.divider() + render_reviewer_setting() _horizontal_logo = _assets_dir / "datasure-horizontal.svg" if _horizontal_logo.exists(): st.image(str(_horizontal_logo), width="stretch") diff --git a/src/datasure/processing/correction_log.py b/src/datasure/processing/correction_log.py index dd42f56c..0700fc7c 100644 --- a/src/datasure/processing/correction_log.py +++ b/src/datasure/processing/correction_log.py @@ -55,17 +55,20 @@ class Action(StrEnum): # For "accept", how serious the accepted flag is: "hard" for a hard # constraint violation, null otherwise. "severity": pl.String, + # Who made the entry: the reviewer name set in the app, else the OS login. + "user": pl.String, } # Values given to columns that were added to the log after some logs were # already persisted. Every legacy entry came from the Corrections page and -# was applied successfully when it was logged. +# was applied successfully when it was logged. Who made it was not recorded. _LOG_BACKFILL_DEFAULTS: dict[str, str | None] = { "status": "Successful", "status_reason": None, "source": CORRECTIONS_PAGE_SOURCE, "check_type": None, "severity": None, + "user": None, } diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index 27447d48..37f693bb 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -24,6 +24,7 @@ duckdb_table_exists, ) from datasure.utils.reapply_utils import ReapplyFailure +from datasure.utils.reviewer import get_reviewer_name def _describe_correction_row(row: dict[str, Any]) -> str: @@ -231,7 +232,8 @@ def _build_log_row( """Build one correction-log row. A freshly logged entry has just been applied successfully (the apply - step raises before logging otherwise). + step raises before logging otherwise). The entry records the current + reviewer as its user. """ return { "date": datetime.now(), @@ -247,6 +249,7 @@ def _build_log_row( "source": str(source), "check_type": check_type, "severity": severity, + "user": get_reviewer_name(), } diff --git a/src/datasure/utils/reviewer.py b/src/datasure/utils/reviewer.py new file mode 100644 index 00000000..b8bcaf31 --- /dev/null +++ b/src/datasure/utils/reviewer.py @@ -0,0 +1,89 @@ +"""The reviewer identity recorded on every correction-log entry. + +The reviewer defaults to the OS login. A "Reviewer name" set in the app +overrides it and is remembered across sessions in the user settings file. +The setting holds a display name only, never credentials. +""" + +import getpass +import json +import logging +from pathlib import Path + +import streamlit as st + +from datasure.utils.cache_utils import get_cache_path + +logger = logging.getLogger(__name__) + +_REVIEWER_NAME_KEY = "reviewer_name" + + +def _user_settings_path() -> Path: + return get_cache_path("user_settings.json") + + +def _load_user_settings() -> dict: + path = _user_settings_path() + if not path.exists(): + return {} + try: + settings = json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError): + logger.warning("Could not read user settings from %s", path) + return {} + return settings if isinstance(settings, dict) else {} + + +def _os_login() -> str: + try: + return getpass.getuser() + except Exception: # getpass raises OSError, or KeyError on some platforms + return "" + + +def get_reviewer_override() -> str: + """Return the saved "Reviewer name", or "" if none is set.""" + name = _load_user_settings().get(_REVIEWER_NAME_KEY) + return name.strip() if isinstance(name, str) else "" + + +def get_reviewer_name() -> str: + """Return who is making corrections: the saved name, else the OS login.""" + return get_reviewer_override() or _os_login() + + +def set_reviewer_name(name: str) -> None: + """Save the "Reviewer name" override. A blank name clears it.""" + settings = _load_user_settings() + name = name.strip() + if name: + settings[_REVIEWER_NAME_KEY] = name + else: + settings.pop(_REVIEWER_NAME_KEY, None) + path = _user_settings_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(settings, indent=2), encoding="utf-8") + + +def _save_reviewer_name_input() -> None: + set_reviewer_name(st.session_state.get("reviewer_name_input", "")) + + +def render_reviewer_setting() -> None: + """Render the "Reviewer name" setting, saved as soon as it changes.""" + with st.popover( + f":material/person: Reviewer: {get_reviewer_name() or 'unknown'}", + width="stretch", + ): + st.text_input( + "Reviewer name", + value=get_reviewer_override(), + key="reviewer_name_input", + placeholder=_os_login(), + help=( + "Recorded as the user on every correction and acceptance you " + "make. Leave blank to use your computer login." + ), + on_change=_save_reviewer_name_input, + ) diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 12434e90..952aeb03 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -540,7 +540,9 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: columns so status/status_reason sit right after action, and relabels the "ID" column as "Survey ID" for display. "accept" rows carry the check whose flag was accepted in check_type and, for a hard constraint - violation, severity "hard"; source names the page that made each entry. + violation, severity "hard"; source names the page that made each entry + and user names who made it (empty for entries logged before it was + recorded). Parameters ---------- @@ -557,6 +559,7 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: display_columns = [ "date", + "user", "KEY", "ID", "action", diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 285b480a..368841e7 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -1170,9 +1170,110 @@ def test_removing_the_only_entry_leaves_an_empty_log_with_full_schema( "source", "check_type", "severity", + "user", ] +class TestCorrectionLogUser: + """Every log entry records who made it.""" + + @pytest.fixture(autouse=True) + def reviewer(self, monkeypatch): + monkeypatch.setattr( + "datasure.processing.corrections.get_reviewer_name", lambda: "ama" + ) + + def test_apply_correction_records_the_reviewer(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + processor.apply_correction( + alias="survey", + key_col="survey_key", + key_value="key1", + action="modify value", + column="name", + current_value="John", + new_value="Johnny", + reason="typo", + ) + + assert processor.get_correction_log("survey")["user"].to_list() == ["ama"] + + def test_accept_value_records_the_reviewer(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + processor.accept_value( + alias="survey", + key_col="survey_key", + key_value="key1", + check_type="outliers", + column="age", + current_value=25, + reason="verified", + ) + + assert processor.get_correction_log("survey")["user"].to_list() == ["ama"] + + def test_apply_corrections_records_the_reviewer_on_every_entry( + self, store, sample_data + ): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[ + CorrectionEntry( + key_value="key1", + action="modify value", + column="name", + current_value="John", + new_value="Jon", + reason="typo", + ), + CorrectionEntry( + key_value="key2", action="remove row", reason="duplicate" + ), + ], + source="duplicates", + ) + + log = processor.get_correction_log("survey") + assert log["user"].to_list() == ["ama", "ama"] + + def test_legacy_log_loads_with_an_empty_user_and_no_data_loss( + self, store, sample_data, sample_corrections_log + ): + _seed_prep(store, sample_data) + legacy_log = sample_corrections_log.with_columns( + pl.lit("corrections_page").alias("source") + ) + store[("p1", "logs", "corr_log_survey")] = legacy_log + processor = CorrectionProcessor("p1") + + log = processor.get_correction_log("survey") + assert log["user"].to_list() == [None] * 3 + assert log.select(legacy_log.columns).equals(legacy_log) + + processor.add_correction_entry( + alias="survey", + key_value="key1", + current_id=None, + action="remove row", + column=None, + current_value=None, + new_value=None, + reason="duplicate", + ) + + log = processor.get_correction_log("survey") + assert log["user"].to_list() == [None, None, None, "ama"] + assert log.head(3).select(legacy_log.columns).equals(legacy_log) + + class TestAcceptAction: """Accepting a flagged value records a decision without changing data.""" diff --git a/tests/replication/test_package_builder.py b/tests/replication/test_package_builder.py index a68d6e49..1ec7456c 100644 --- a/tests/replication/test_package_builder.py +++ b/tests/replication/test_package_builder.py @@ -282,6 +282,7 @@ def test_correction_log_csv_keeps_accept_rows(self): "reason": ["verified", "typo"], "source": ["outliers", "corrections_page"], "check_type": ["outliers", None], + "user": ["ama", "kofi"], } ) @@ -309,9 +310,9 @@ def _duckdb_get(project_id, table, db_name): corrections_do = zf.read("replication_p_s/2_scripts/4_corrections.do") audit = pl.read_csv(BytesIO(log_csv)) - assert audit.select("KEY", "action", "check_type").rows() == [ - ("k1", "accept", "outliers"), - ("k2", "modify value", None), + assert audit.select("KEY", "action", "check_type", "user").rows() == [ + ("k1", "accept", "outliers", "ama"), + ("k2", "modify value", None, "kofi"), ] assert b'"k1"' not in corrections_do assert b'"k2"' in corrections_do @@ -342,7 +343,7 @@ def _duckdb_get(project_id, table, db_name): ) assert header == ( "date,KEY,ID,action,column,current_value,new_value,reason," - "status,status_reason,source,check_type,severity" + "status,status_reason,source,check_type,severity,user" ) @@ -410,3 +411,4 @@ def test_legacy_log_exports_with_backfilled_columns(self): assert audit["source"].to_list() == ["corrections_page"] assert audit["status"].to_list() == ["Successful"] assert "check_type" in audit.columns + assert audit["user"].to_list() == [None] diff --git a/tests/utils/test_reviewer.py b/tests/utils/test_reviewer.py new file mode 100644 index 00000000..922db39a --- /dev/null +++ b/tests/utils/test_reviewer.py @@ -0,0 +1,77 @@ +"""Tests for the reviewer identity recorded in the correction log.""" + +import json + +import pytest + +from datasure.utils import reviewer +from datasure.utils.reviewer import get_reviewer_name, set_reviewer_name + + +@pytest.fixture(autouse=True) +def user_settings_file(tmp_path, monkeypatch): + """Point the user settings file at a temporary path.""" + path = tmp_path / "user_settings.json" + monkeypatch.setattr(reviewer, "_user_settings_path", lambda: path) + monkeypatch.setattr(reviewer.getpass, "getuser", lambda: "os_login") + return path + + +class TestGetReviewerName: + def test_defaults_to_the_os_login(self): + assert get_reviewer_name() == "os_login" + + def test_override_takes_precedence_over_the_os_login(self): + set_reviewer_name("Ama Mensah") + assert get_reviewer_name() == "Ama Mensah" + + def test_override_persists_in_the_user_settings_file(self, user_settings_file): + set_reviewer_name("Ama Mensah") + assert json.loads(user_settings_file.read_text())["reviewer_name"] == ( + "Ama Mensah" + ) + + def test_override_is_stripped(self): + set_reviewer_name(" Ama Mensah ") + assert get_reviewer_name() == "Ama Mensah" + + def test_clearing_the_override_falls_back_to_the_os_login(self): + set_reviewer_name("Ama Mensah") + set_reviewer_name(" ") + assert get_reviewer_name() == "os_login" + + def test_keeps_other_user_settings(self, user_settings_file): + user_settings_file.write_text(json.dumps({"other": 1})) + set_reviewer_name("Ama Mensah") + assert json.loads(user_settings_file.read_text())["other"] == 1 + + def test_unreadable_settings_file_falls_back_to_the_os_login( + self, user_settings_file + ): + user_settings_file.write_text("not json") + assert get_reviewer_name() == "os_login" + + def test_os_login_lookup_failure_gives_an_empty_name(self, monkeypatch): + def fail(): + raise OSError("no login") + + monkeypatch.setattr(reviewer.getpass, "getuser", fail) + assert get_reviewer_name() == "" + + +def _reviewer_setting_app(): + from datasure.utils.reviewer import render_reviewer_setting + + render_reviewer_setting() + + +class TestRenderReviewerSetting: + def test_entering_a_name_saves_the_override(self): + from streamlit.testing.v1 import AppTest + + at = AppTest.from_function(_reviewer_setting_app).run() + at.text_input(key="reviewer_name_input").input("Ama Mensah").run() + + assert not at.exception + assert get_reviewer_name() == "Ama Mensah" + assert at.text_input(key="reviewer_name_input").value == "Ama Mensah" diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index f51f6ef7..1a915a13 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -461,6 +461,7 @@ def test_status_columns_ordered_right_after_action(self): assert result.columns == [ "date", + "user", "KEY", "Survey ID", "action", @@ -508,6 +509,16 @@ def test_hard_violation_acceptances_show_their_severity(self): assert result["severity"].to_list() == ["hard"] + def test_shows_who_made_each_entry(self): + result = _build_correction_log_display(self._base_log(user=["ama"])) + + assert result["user"].to_list() == ["ama"] + + def test_legacy_log_shows_an_empty_user(self): + result = _build_correction_log_display(self._base_log()) + + assert result["user"].to_list() == [None] + class TestHighlightHardAcceptance: """Hard-violation acceptances stand out in the Correction Log.""" From a6a629a5cfbb481775d4e370ca7c63ff63110c76 Mon Sep 17 00:00:00 2001 From: iabaako Date: Tue, 6 Oct 2026 09:56:47 +0000 Subject: [PATCH 2/3] fix(corrections): address review of correction log user Resolve the reviewer once per apply instead of once per log row, catch only the OS-login lookup errors getpass raises and log them, show an error instead of crashing when the reviewer name can't be saved, and add the changelog entry. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++++ src/datasure/processing/corrections.py | 10 +++++++--- src/datasure/utils/reviewer.py | 12 +++++++++--- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e9b4392..babd7d8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `eligibility_values`). `settings_from_page_config` builds `BackcheckSettings` from the page config, where a target of 0 means not set — #318 +- **Correction log user**: New `user` column records who made each entry, + from any source. `get_reviewer_name()` in the new + `src/datasure/utils/reviewer.py` returns the "Reviewer name" set in the + sidebar, saved in `cache/user_settings.json`, else the OS login + (`getpass.getuser()`). Existing logs load with a null `user`. The + Correction Log table and `correction_log.csv` include it — #321 ### Changed diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index 37f693bb..cfcc8230 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -227,13 +227,14 @@ def _build_log_row( reason: str, source: str, check_type: str | None, + user: str, severity: str | None = None, ) -> dict[str, Any]: """Build one correction-log row. A freshly logged entry has just been applied successfully (the apply - step raises before logging otherwise). The entry records the current - reviewer as its user. + step raises before logging otherwise). `user` is the reviewer making + the entry (see `get_reviewer_name`). """ return { "date": datetime.now(), @@ -249,7 +250,7 @@ def _build_log_row( "source": str(source), "check_type": check_type, "severity": severity, - "user": get_reviewer_name(), + "user": user, } @@ -444,6 +445,7 @@ def add_correction_entry( reason=reason, source=source, check_type=check_type, + user=get_reviewer_name(), ) ], ) @@ -751,6 +753,7 @@ def apply_corrections( for entry in entries: corrected_data = self._apply_entry(corrected_data, key_col, entry) + user = get_reviewer_name() log_rows = [ _build_log_row( key_value=entry.key_value, @@ -762,6 +765,7 @@ def apply_corrections( reason=entry.reason, source=source, check_type=entry.check_type, + user=user, severity=entry.severity, ) for entry in entries diff --git a/src/datasure/utils/reviewer.py b/src/datasure/utils/reviewer.py index b8bcaf31..ec6e9cff 100644 --- a/src/datasure/utils/reviewer.py +++ b/src/datasure/utils/reviewer.py @@ -17,6 +17,7 @@ logger = logging.getLogger(__name__) _REVIEWER_NAME_KEY = "reviewer_name" +_REVIEWER_NAME_WIDGET_KEY = "reviewer_name_input" def _user_settings_path() -> Path: @@ -38,7 +39,8 @@ def _load_user_settings() -> dict: def _os_login() -> str: try: return getpass.getuser() - except Exception: # getpass raises OSError, or KeyError on some platforms + except (OSError, KeyError): # KeyError when the uid has no passwd entry + logger.warning("Could not look up the OS login", exc_info=True) return "" @@ -67,7 +69,11 @@ def set_reviewer_name(name: str) -> None: def _save_reviewer_name_input() -> None: - set_reviewer_name(st.session_state.get("reviewer_name_input", "")) + try: + set_reviewer_name(st.session_state.get(_REVIEWER_NAME_WIDGET_KEY, "")) + except OSError: + logger.exception("Could not save the reviewer name") + st.error("Could not save the reviewer name. Check the cache folder.") def render_reviewer_setting() -> None: @@ -79,7 +85,7 @@ def render_reviewer_setting() -> None: st.text_input( "Reviewer name", value=get_reviewer_override(), - key="reviewer_name_input", + key=_REVIEWER_NAME_WIDGET_KEY, placeholder=_os_login(), help=( "Recorded as the user on every correction and acceptance you " From c84cfaca30107c2ac8548be2e48cdbd38880d122 Mon Sep 17 00:00:00 2001 From: iabaako Date: Tue, 6 Oct 2026 10:27:58 +0000 Subject: [PATCH 3/3] fix(corrections): handle ImportError when looking up the OS login On Windows with Python 3.11/3.12 and no login environment variable, getpass.getuser() falls back to importing the Unix-only pwd module and raises ImportError, which broke the sidebar on every page. Catch it with the other lookup errors so the reviewer falls back to an empty name. Co-Authored-By: Claude Opus 5.5 --- src/datasure/utils/reviewer.py | 5 ++++- tests/utils/test_reviewer.py | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/datasure/utils/reviewer.py b/src/datasure/utils/reviewer.py index ec6e9cff..68145e2b 100644 --- a/src/datasure/utils/reviewer.py +++ b/src/datasure/utils/reviewer.py @@ -39,7 +39,10 @@ def _load_user_settings() -> dict: def _os_login() -> str: try: return getpass.getuser() - except (OSError, KeyError): # KeyError when the uid has no passwd entry + # With no login environment variable set, getpass falls back to the + # password database: KeyError when the uid has no entry, ImportError on + # Windows before Python 3.13 (no pwd module), OSError from 3.13. + except (OSError, KeyError, ImportError): logger.warning("Could not look up the OS login", exc_info=True) return "" diff --git a/tests/utils/test_reviewer.py b/tests/utils/test_reviewer.py index 922db39a..a9d4e56f 100644 --- a/tests/utils/test_reviewer.py +++ b/tests/utils/test_reviewer.py @@ -58,6 +58,15 @@ def fail(): monkeypatch.setattr(reviewer.getpass, "getuser", fail) assert get_reviewer_name() == "" + def test_missing_pwd_module_gives_an_empty_name(self, monkeypatch): + # Python 3.11/3.12 on Windows with no login environment variables + # falls back to importing the Unix-only pwd module. + def fail(): + raise ModuleNotFoundError("No module named 'pwd'") + + monkeypatch.setattr(reviewer.getpass, "getuser", fail) + assert get_reviewer_name() == "" + def _reviewer_setting_app(): from datasure.utils.reviewer import render_reviewer_setting