fix(versioning): match version-history search on the change author name - #43835
fix(versioning): match version-history search on the change author name#43835mikebridge wants to merge 1 commit into
Conversation
The version-history panel's 'Search actions' box runs a server-side substring filter (_record_matches). Its haystack covered summary, entity_name, kind, the path segments, and the from/to values -- but never the change author, so typing an author's name returned 'No actions found' even when every timeline entry was that author's. Author-scoped search, a primary use of the box, was silently broken and the count undercounted. Add the author's display name to the haystack, sourced from the projected changed_by DTO. Decoration redacts changed_by to None for a tombstoned related entity whose editor identity must not be disclosed, so a redacted record contributes no author text and stays unsearchable by author -- preserving that security contract. Partial names (only first or last) still match on the present part. Regression tests for author substring / full-name / case-insensitive matching, partial names, and the redacted (None changed_by) case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQeAcprGvkFS8F3nghtvwv
b0af413 to
39d568d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43835 +/- ##
==========================================
- Coverage 79.42% 79.41% -0.01%
==========================================
Files 2895 2895
Lines 167947 167949 +2
Branches 38896 38896
==========================================
- Hits 133388 133385 -3
- Misses 32059 32064 +5
Partials 2500 2500
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #ee2e2dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at 39d568d3559978b47219d52c167e67e1fce7174e. Ran tests/unit_tests/versioning/ — 139 passed.
The security argument in the new docstring is the load-bearing part of this change, so I verified it against the decoration code rather than taking the docstring's word for it. It holds. One test-coverage gap, described at the end.
The redaction contract checks out
render.py:191 is the only path that redacts an author, and it does not stand alone — the same block blanks every other haystack this function reads:
record["entity_name"] = "" # render.py:189
record["summary"] = f"(deleted) {label}" # render.py:190
record["changed_by"] = None # render.py:191
record["from_value"] = None # render.py:192
record["to_value"] = None # render.py:193
record["path"] = None # render.py:194So for a redacted record every one of the seven haystacks at orchestrator.py:214-220 is either empty or a fixed "(deleted) <kind>" literal. _build_summary (render.py:251-271) is built from label/verb/entity_name and never interpolates the author, and the raw first_name/last_name/changed_by_id columns are popped at render.py:197-205, so nothing survives out of band. The contract is only as strong as the weakest haystack, and here there isn't a weak one — widening the search to the author doesn't open a gap.
It exposes nothing new for records the caller can already see
author_name at orchestrator.py:206-211 is constructed identically to what the panel already prints:
" ".join(str(p) for p in (first_name, last_name) if p) # orchestrator.py:207-211[changedBy.first_name, changedBy.last_name].filter(Boolean).join(' ') // display.ts:55-57formatAuthor renders on every row (ActionRow.tsx:154, RelatedUpdateRow.tsx:137,177), and the search runs post-visibility-filter, so the needle is matched against a string the requester is already being handed in the payload. No probing oracle — purely additive. Matching the rendered string byte-for-byte is the right call, and it's also why "Doe, John" not matching "John Doe" is defensible: the search matches what's on screen. If you ever want username/email searchable that'd be a separate, deliberate widening, not a gap here.
Two other things I checked and found fine: an empty q can't reach _record_matches (orchestrator.py:106 strips-and-truthiness-gates before the param is set, and orchestrator.py:315 re-guards), so the "" in "" matches-everything case is unreachable; and the scan stays bounded by the pre-existing _MAX_FETCHED_RECORDS = 5000 per kind (queries.py:543) — the filter adds a substring pass over an already-materialized list, no new query and no new memory ceiling.
The one gap: the invariant this PR now depends on is untested where it's enforced
The docstring makes changed_by is None at the decoration layer an explicit, load-bearing security invariant. The two new tests assert it against hand-built dicts, which is correct for _record_matches but doesn't exercise decoration. And the test that does cover the redaction path — test_decoration_redacts_record_from_reused_entity_id (test_activity.py:182) — asserts five redacted fields at test_activity.py:222-227 but not changed_by, even though its fixture seeds exactly the identity that must be suppressed ("first_name": "Ada", "last_name": "Lovelace", test_activity.py:199-201).
I confirmed this empirically: deleting record["changed_by"] = None from render.py:191 leaves the entire tests/unit_tests/versioning/ suite green — including both tests added here. The enforcement point can regress silently.
One line at test_activity.py:227 closes it:
assert record["changed_by"] is NoneI verified it passes as-is and fails (assert {'id': 1, 'first_name': 'Ada', 'last_name': 'Lovelace'} is None) with render.py:191 removed.
Verification
Reverted only the 17 production lines with the tests kept: test_record_matches_searches_author_name and test_record_matches_author_partial_and_missing_name both fail, so the tests genuinely gate the behavior. Nice touch that the nameless case asserts not _record_matches(nameless, "none") — that specifically guards the if part filter, since dropping it would stringify None into the haystack.
Small, well-scoped, and the security reasoning is sound. Only the one test line above from me. Not approving as I'm not a committer on this repo — flagging for a committer.
2 participants
SUMMARY
The version-history panel's "Search actions" box runs a server-side substring filter (
_record_matchesinsuperset/versioning/activity/orchestrator.py). Its search haystack coveredsummary,entity_name,kind, the joinedpathsegments, and the JSON form offrom_value/to_value— but never the change author. So typing an author's name returned "No actions found" even when every entry in the timeline was authored by that user, and the result count undercounted. Filtering history by who made a change is a primary expected use of the box, and it was silently broken.The fix adds the change author's display name to the haystack, sourced from the already-projected
changed_byDTO ({id, first_name, last_name}). Two properties are preserved deliberately:changed_by = Nonefor a tombstoned related entity whose editor identity must not be disclosed. Reading the author fromchanged_bymeans a redacted record contributes no author text and stays unsearchable by author — the redaction contract is kept intact.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Backend-only change. Before:
GET /api/v1/chart/<uuid>/activity/?include=all&q=<author name>→count 0on a timeline entirely authored by that user. After: the same query matches that author's entries.TESTING INSTRUCTIONS
pytest tests/unit_tests/versioning/test_activity.py -k record_matches— adds coverage for author substring / full-name / case-insensitive matching, single-part names, and the redacted (changed_by is None) case.Manual: open a chart/dashboard version-history panel whose saves are by one author, type that author's name in "Search actions" — the timeline filters to their entries instead of showing "No actions found".
ADDITIONAL INFORMATION