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/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..cfcc8230 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: @@ -226,12 +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). + step raises before logging otherwise). `user` is the reviewer making + the entry (see `get_reviewer_name`). """ return { "date": datetime.now(), @@ -247,6 +250,7 @@ def _build_log_row( "source": str(source), "check_type": check_type, "severity": severity, + "user": user, } @@ -441,6 +445,7 @@ def add_correction_entry( reason=reason, source=source, check_type=check_type, + user=get_reviewer_name(), ) ], ) @@ -748,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, @@ -759,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 new file mode 100644 index 00000000..68145e2b --- /dev/null +++ b/src/datasure/utils/reviewer.py @@ -0,0 +1,98 @@ +"""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" +_REVIEWER_NAME_WIDGET_KEY = "reviewer_name_input" + + +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() + # 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 "" + + +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: + 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: + """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_WIDGET_KEY, + 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..a9d4e56f --- /dev/null +++ b/tests/utils/test_reviewer.py @@ -0,0 +1,86 @@ +"""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 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 + + 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."""