Repository navigation
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
An unhandled login-lookup exception can interrupt app rendering on supported Windows configurations.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds reviewer attribution to correction logs for #321 and provides shared identity handling for future backcheck attribution.
Changes:
- Adds a persistent reviewer-name override with an OS-login fallback.
- Records and displays
user, preserving legacy logs and CSV exports. - Updates tests and documentation.
| File | Description |
|---|---|
| tests/views/test_correction_view.py | Tests reviewer display and legacy entries. |
| tests/utils/test_reviewer.py | Tests identity settings and fallbacks. |
| tests/replication/test_package_builder.py | Verifies reviewer CSV exports. |
| tests/processing/test_corrections.py | Tests attribution across correction paths. |
| src/datasure/views/correction_view.py | Displays the reviewer column. |
| src/datasure/utils/reviewer.py | Adds reviewer identity and settings. |
| src/datasure/processing/corrections.py | Records reviewers on new entries. |
| src/datasure/processing/correction_log.py | Extends schema and legacy backfill. |
| src/datasure/app.py | Adds the sidebar reviewer setting. |
| docs/USER_GUIDE.md | Explains reviewer attribution. |
| docs/ARCHITECTURE.md | Documents settings storage. |
| CHANGELOG.md | Records the attribution feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _os_login() -> str: | ||
| try: | ||
| return getpass.getuser() | ||
| except (OSError, KeyError): # KeyError when the uid has no passwd entry |
There was a problem hiding this comment.
Confirmed and fixed in c84cfac. Reproduced on Windows with Python 3.11: with LOGNAME, USER, LNAME and USERNAME unset, getpass.getuser() raises ModuleNotFoundError. _os_login() now catches ImportError along with OSError and KeyError, logs a warning and returns an empty name. A regression test (test_missing_pwd_module_gives_an_empty_name) covers it.
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 <noreply@anthropic.com>
|
Pull Request Summary 🚀
What does this PR do? 📝
This PR records who made each correction log entry. The correction log
(
corr_log_{alias}) gets a newusercolumn. Every new entry fills it,whether it comes from the Corrections page or a check page. Entries logged
before this change show an empty user.
Closes #321. This PR is stacked on #318, so its base is
feat/318-backcheck-target-metrics. Retarget it tomainonce #318 merges.Why is this change needed? 🤔
The log already records when, why and from where a change was made, but not
who made it. Reviewers need that for the audit trail. The backcheck
attribution log in #301 will use the same identity.
How was this implemented? 🛠️
get_reviewer_name(). It returns the "Reviewer name" set in the sidebar,otherwise the computer (OS) login. The name is saved in
cache/user_settings.jsonand remembered across sessions. It holds adisplay name only, never credentials. Clearing it falls back to the OS
login. If the login can't be looked up, the user is left empty and a warning
is logged.
per row. All entries in one batch therefore record the same user.
Older logs load with an empty user and nothing else changes. Removing,
refreshing, or importing corrections from a project config keeps the column
as it is.
correction_log.csvincludes theusercolumn.USER_GUIDE.md(audit trail),ARCHITECTURE.md(cache files) andCHANGELOG.md.Merge danger: this is a two-way door, but a narrow one. The column is
added to saved logs, and older code simply ignores it. The blast radius is the
correction log: the Correction Log table and the CSV header each gain a
column.
📝 The Reviewer name is saved once per install, not per browser session. This
is intentional: DataSure runs as a single-user desktop install today. When
shared installs are supported, a login will be added and the reviewer will
come from it, behind the same
get_reviewer_name()helper.How to test or reproduce ? 🧪
Automated:
just testgives 3552 passed and 6 skipped, andjust pre-commit-runpasses all hooks. The new and updated tests cover:in the settings file, and a blank value clears it
crash anything
entries
userManual:
Correct Data. The Correction Log shows your login under
user.the Outliers page. The new entry shows that name.
4_output/3_logs/correction_log.csvhas ausercolumn.Screenshots (if applicable) 📷
N/A. The only UI changes are a sidebar popover and one extra column in the
Correction Log table. Manual step 2 covers both.
Checklist ✅
getpassis in the standard library)Reviewer Emoji Legend
:code::smiley::+1::100:...and I want the author to know it! This is a way to highlight positive parts of a code review.
:star: :star: :star:And I am providing reasons why it needs to be addressed as well as suggested improvements.
:star: :star:And I am providing suggestions where it could be improved either in this PR or later.
:star:...and consider this a suggestion, not a requirement.
:question:This should be a fully formed question with sufficient information and context that requires a response.
:memo::pick:This does not require any changes and is often better left unsaid. This may include stylistic, formatting, or organization suggestions and should likely be prevented/enforced by linting if they really matter
:recycle:Should include enough context to be actionable and not be considered a nitpick.
🤖 Generated with Claude Code