fe63f3985b
CI & Build / Python lint (push) Successful in 3s
CI & Build / integration (push) Successful in 24s
CI & Build / TypeScript typecheck (push) Successful in 36s
CI & Build / Python tests (push) Successful in 54s
CI & Build / Build & push image (push) Successful in 40s
Closes the last of milestone #232. The task said to settle the design before coding; here is what was settled and why. THE HAZARD. Restoring a merged-in source from the trash brought the record back but never stripped its locations off the survivor, so both claimed the same call sites and the reverse lookup read the duplicate claims as real. Subtracting blindly is not a fix: a location can arrive from a source AND genuinely be the survivor's own, and _normalize_locations dedups them into one, so blind subtraction would strip a call site the survivor owns. Same problem defeated partial un-merge — `merged_from` recorded ids, not which locations came from which source. THE ANSWER. Record per-source attribution AT MERGE TIME, where it is known exactly: each entry keeps only what that source ADDED, computed incrementally as sources fold in. Anything the survivor already had, or an earlier source already brought, is attributed to nobody. Both open questions fall out of that one change — partial un-merge is exact, and a survivor-owned location can never be stripped, because it was never attributed in the first place. The shape moved from [id] to [{id, locations, tags}]. Free to do: the corpus holds one snippet and zero merges, so there is no legacy data (rule #22). A bare int still normalizes to {"id": n} — not legacy tolerance, but because snippet_fields falls back to PARSING THE BODY when a row has no `data`, and the body's provenance line can only carry ids. Such an entry shows history and refuses un-merge with a reason rather than guessing. WHICH SURFACE. Neither option in the task, quite. Making trash-restore notice the merge would teach the generic trash path snippet semantics for one record type. Instead un-merge OWNS the restore: one operation, one authorization check, trash stays ignorant. Restoring by hand is still allowed and still leaves both records claiming the same places — so un-merge treats an already-alive source as the normal case and goes straight to the subtraction that repairs it. That is the state that motivated the feature, not an error. Adds trash.restore_entity(user_id, type, id) — the missing inverse of delete(), which returns a batch id callers don't keep. Restores the whole batch, since the batch is the entity plus its cascade. Refs #2165 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs
114 lines
4.9 KiB
Python
114 lines
4.9 KiB
Python
"""Merge records what it absorbed (#2087).
|
|
|
|
Merging keeps the target's scalar fields, unions locations + tags, and trashes
|
|
the sources. Without provenance the fact that a variant ever existed survives
|
|
only in the trash — and only for someone who already knew to go looking. So the
|
|
survivor carries `merged_from`, written to the body and the indexed mirror from
|
|
one value, and ordinary edits must carry it forward rather than erase it.
|
|
"""
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from scribe.services import snippets as s
|
|
|
|
|
|
def _snippet(id=1, owner=9, body=None, data=None, tags=None):
|
|
n = MagicMock()
|
|
n.id = id
|
|
n.user_id = owner
|
|
n.title = "formatDuration — humanize a millisecond count"
|
|
n.body = body if body is not None else s.compose_body(code="x = 1", language="ts")
|
|
n.tags = tags if tags is not None else ["ts", "snippet"]
|
|
n.note_type = "snippet"
|
|
n.deleted_at = None
|
|
# Explicitly None — snippet_fields prefers `data` when truthy, and an
|
|
# auto-MagicMock attribute is truthy (note 2109).
|
|
n.data = data
|
|
return n
|
|
|
|
|
|
async def _run_merge(target, sources, source_ids):
|
|
"""Drive merge_snippets with the DB stubbed out; return update_note's kwargs."""
|
|
by_id = {target.id: target, **{x.id: x for x in sources}}
|
|
|
|
async def fake_get(_uid, sid):
|
|
return by_id.get(sid)
|
|
|
|
with patch.object(s, "get_snippet", AsyncMock(side_effect=fake_get)), \
|
|
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
|
|
patch.object(s.notes_svc, "update_note",
|
|
AsyncMock(return_value=target)) as mock_update, \
|
|
patch("scribe.services.trash.delete", AsyncMock(return_value=object())), \
|
|
patch.object(s, "_embed_snippet", MagicMock()):
|
|
await s.merge_snippets(7, target.id, source_ids)
|
|
return mock_update.await_args.kwargs
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_merge_records_what_it_absorbed_in_body_and_mirror():
|
|
kwargs = await _run_merge(
|
|
_snippet(id=1), [_snippet(id=2), _snippet(id=3)], [2, 3],
|
|
)
|
|
assert "**Merged from:** #2, #3" in kwargs["body"]
|
|
assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_merge_accumulates_across_merges():
|
|
"""A target merged twice keeps both histories — the second merge must not
|
|
replace the record of the first."""
|
|
target = _snippet(id=1, data=s.compose_data(name="f", merged_from=[2]))
|
|
kwargs = await _run_merge(target, [_snippet(id=3)], [3])
|
|
assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
|
|
assert "**Merged from:** #2, #3" in kwargs["body"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_merge_does_not_record_a_skipped_cross_owner_source():
|
|
"""A source owned by someone else is skipped, not folded in (#231) — so it
|
|
must not appear in the provenance either, or the record would claim to
|
|
contain something it never absorbed."""
|
|
kwargs = await _run_merge(
|
|
_snippet(id=1, owner=9),
|
|
[_snippet(id=2, owner=9), _snippet(id=3, owner=42)],
|
|
[2, 3],
|
|
)
|
|
assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2]
|
|
assert "#3" not in kwargs["body"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_an_ordinary_edit_carries_provenance_forward():
|
|
"""Only a merge may add to it, but nothing else may drop it — update
|
|
recomposes body and mirror from scratch, so an omission here would silently
|
|
erase the history on the next unrelated edit."""
|
|
note = _snippet(id=1, data=s.compose_data(name="f", language="ts",
|
|
merged_from=[2, 3]))
|
|
with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \
|
|
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
|
|
patch.object(s.notes_svc, "update_note",
|
|
AsyncMock(return_value=note)) as mock_update, \
|
|
patch.object(s, "_embed_snippet", MagicMock()):
|
|
await s.update_snippet(7, 1, signature="f(ms) -> string")
|
|
|
|
kwargs = mock_update.await_args.kwargs
|
|
assert s.merged_from_ids(kwargs["data"]["merged_from"]) == [2, 3]
|
|
assert "**Merged from:** #2, #3" in kwargs["body"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_pre_0070_row_keeps_provenance_through_the_body():
|
|
"""Rows written before the `data` column are never backfilled, so the body
|
|
line is their only copy — and it has to survive an edit too."""
|
|
body = s.compose_body(code="x = 1", language="ts", merged_from=[5])
|
|
note = _snippet(id=1, body=body, data=None)
|
|
with patch.object(s, "get_snippet", AsyncMock(return_value=note)), \
|
|
patch("scribe.services.access.can_write_note", AsyncMock(return_value=True)), \
|
|
patch.object(s.notes_svc, "update_note",
|
|
AsyncMock(return_value=note)) as mock_update, \
|
|
patch.object(s, "_embed_snippet", MagicMock()):
|
|
await s.update_snippet(7, 1, when_to_use="humanize a ms count")
|
|
|
|
assert "**Merged from:** #5" in mock_update.await_args.kwargs["body"]
|