From 7154ca0a61c5fd022acc85cc0a02b253fdf5c59b Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 19:59:54 +0100 Subject: [PATCH] fix(core): race-safe entry upsert and lossless per-user moves - upsertEntry inserts with ON CONFLICT DO NOTHING and patches through the update path, so a concurrent creator never aborts the caller's transaction on Postgres. - Undoing a move deletes the forward alias and writes no reverse alias. - Merging into an existing entry keeps 'user' field_sources on combined fields, carries the moved entry's field_sources as-is when reviving a deleted target, concatenates differing review/private notes, records a dropped rating in history, and writes history for re-parented engagements (restorable). - Moves re-point this user's copies (with history) and list items to the target work. - rejectSuggestion requires a pending suggestion; engagements of a deleted entry are not editable; restoring an entry does not re-check custom fields against current definitions. Co-Authored-By: Claude Opus 5.5 --- packages/core/src/engagements.ts | 20 +- packages/core/src/entries.ts | 263 ++++++++++++++++++------- packages/core/test/engagements.test.ts | 10 +- packages/core/test/entries.test.ts | 72 +++++++ packages/core/test/history.test.ts | 23 ++- packages/core/test/suggestions.test.ts | 22 ++- 6 files changed, 327 insertions(+), 83 deletions(-) diff --git a/packages/core/src/engagements.ts b/packages/core/src/engagements.ts index 4c3c15d..948e6f0 100644 --- a/packages/core/src/engagements.ts +++ b/packages/core/src/engagements.ts @@ -67,7 +67,7 @@ function toEngagement(r: Row): Engagement { } function snapshot(e: Engagement): Record { - const s: Record = { deletedAt: e.deletedAt }; + const s: Record = { entryId: e.entryId, deletedAt: e.deletedAt }; for (const k of ENGAGEMENT_PATCH_KEYS) s[k] = e[k]; return s; } @@ -127,7 +127,7 @@ async function load( .select(["e.media_type_override", "w.media_type"]) .where("g.id", "=", engagementId) .where("e.user_id", "=", userId); - if (!opts.includeDeleted) q = q.where("g.deleted_at", "is", null); + if (!opts.includeDeleted) q = q.where("g.deleted_at", "is", null).where("e.deleted_at", "is", null); const r = await q.executeTakeFirst(); if (!r) throw new NotFoundError("engagement"); return { eg: toEngagement(r), mediaType: r.media_type_override ?? r.media_type }; @@ -140,7 +140,7 @@ async function write( mediaType: string, patch: Partial, actor: Actor, - extra: Partial> = {}, + extra: Partial> = {}, ): Promise { const cols = await toColumns(k, userId, mediaType, patch); const fieldSources = { ...current.fieldSources }; @@ -258,6 +258,18 @@ export async function restoreEngagementFromHistory( const { eg, mediaType } = await load(k, userId, engagementId, { includeDeleted: true }); const patch: Record = {}; for (const key of ENGAGEMENT_PATCH_KEYS) if (key in before) patch[key] = before[key]; - const extra = "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {}; + const extra: Partial> = + "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {}; + if (typeof before.entryId === "string" && before.entryId !== eg.entryId) { + // Undo a merge's re-parenting; the old entry must be this user's. + const owner = await k + .selectFrom("entries") + .select("id") + .where("id", "=", before.entryId) + .where("user_id", "=", userId) + .executeTakeFirst(); + if (!owner) throw new NotFoundError("entry"); + extra.entry_id = before.entryId; + } await write(k, userId, eg, mediaType, patch as Partial, "user", extra); } diff --git a/packages/core/src/entries.ts b/packages/core/src/entries.ts index 1533a86..35d8bba 100644 --- a/packages/core/src/entries.ts +++ b/packages/core/src/entries.ts @@ -232,6 +232,7 @@ async function normalisePatch( userId: string, mediaType: string, patch: Partial, + opts: { trustCustomFields?: boolean } = {}, ): Promise> { checkKeys("patch", patch, ENTRY_PATCH_KEYS); const out: Partial = {}; @@ -262,10 +263,14 @@ async function normalisePatch( } if ("overrides" in patch) out.overrides = checkOverrides(patch.overrides); if ("customFields" in patch) { - const effective = out.mediaTypeOverride ?? mediaType; - const defs = await customFieldsFor(k, userId, { mediaType: effective, level: "entry" }); - validateCustomValues(defs, patch.customFields as Record); - out.customFields = patch.customFields; + const cf = patch.customFields; + if (cf === null || typeof cf !== "object" || Array.isArray(cf)) throw new ValidationError("customFields", "not_object"); + if (!opts.trustCustomFields) { + const effective = out.mediaTypeOverride ?? mediaType; + const defs = await customFieldsFor(k, userId, { mediaType: effective, level: "entry" }); + validateCustomValues(defs, cf); + } + out.customFields = cf; } return out; } @@ -291,29 +296,43 @@ function checkOverrides(o: unknown): EntryOverrides { return out; } +interface WriteOpts { + /** Non-patch columns. */ + extra?: Partial>; + /** Per-key `field_sources` for patched keys instead of `actor`; `undefined` removes the key. */ + sources?: Record; + /** History `before` override: `null` records a creation. */ + historyBefore?: Record | null; + /** Extra keys merged into the history row's before/after snapshots. */ + historyExtra?: { before: Record; after: Record }; +} + /** - * Write `cols` to an entry, set `field_sources[key] = actor` for each patched key, and record history. - * `extra` carries non-patch columns (`work_id`, `deleted_at`). + * Write a patch to an entry, set `field_sources[key] = actor` for each patched key (unless + * `opts.sources` says otherwise), and record history. */ async function writeEntry( k: Executor, current: Entry, patch: Partial, actor: Actor, - extra: Partial> = {}, - sources?: FieldSources, + opts: WriteOpts = {}, ): Promise { - const fieldSources = { ...current.fieldSources }; - for (const key of Object.keys(patch)) fieldSources[key] = sources?.[key] ?? actor; - const cols = { ...patchToColumns(patch), ...extra, field_sources: toJson(fieldSources), updated_at: nowIso() }; + const fieldSources: Record = { ...current.fieldSources }; + for (const key of Object.keys(patch)) { + fieldSources[key] = opts.sources && key in opts.sources ? opts.sources[key] : actor; + if (fieldSources[key] === undefined) delete fieldSources[key]; + } + const cols = { ...patchToColumns(patch), ...opts.extra, field_sources: toJson(fieldSources), updated_at: nowIso() }; await k.updateTable("entries").set(cols).where("id", "=", current.id).execute(); const after = await loadEntry(k, current.userId, current.id, { includeDeleted: true }); + const before = opts.historyBefore === undefined ? snapshot(current) : opts.historyBefore; await recordHistory(k, { userId: current.userId, entity: "entry", entityId: current.id, - before: snapshot(current), - after: snapshot(after), + before: before === null ? null : { ...before, ...opts.historyExtra?.before }, + after: { ...snapshot(after), ...opts.historyExtra?.after }, actor, }); return after; @@ -372,39 +391,45 @@ export async function upsertEntry( ): Promise { return inTx(db, tx, async (k) => { const work = await resolveVisibleWork(k, userId, workId); - const existing = await findEntry(k, userId, work.id); + let existing = await findEntry(k, userId, work.id); const p = await normalisePatch(k, userId, existing?.mediaTypeOverride ?? work.media_type, patch); - if (existing) { - if (Object.keys(p).length === 0 && existing.deletedAt === null) return existing; - return writeEntry(k, existing, p, actor, existing.deletedAt === null ? {} : { deleted_at: null }); + let created = false; + if (!existing) { + // Insert-or-ignore, then patch through the update path: a concurrent creator never aborts our + // transaction (Postgres poisons a transaction on a unique violation). + const now = nowIso(); + const id = newId(); + await k + .insertInto("entries") + .values({ + id, + user_id: userId, + work_id: work.id, + status: "none", + rating: null, + review: null, + private_notes: null, + favorite: 0, + tags: toJson([]), + custom_fields: toJson({}), + visibility: await defaultVisibility(k, userId), + overrides: toJson({}), + media_type_override: null, + field_sources: toJson({}), + created_at: now, + updated_at: now, + deleted_at: null, + }) + .onConflict((oc) => oc.columns(["user_id", "work_id"]).doNothing()) + .execute(); + existing = (await findEntry(k, userId, work.id))!; + created = existing.id === id; } - const now = nowIso(); - const fieldSources: FieldSources = {}; - for (const key of Object.keys(p)) fieldSources[key] = actor; - const row: EntryRow = { - id: newId(), - user_id: userId, - work_id: work.id, - status: "none", - rating: null, - review: null, - private_notes: null, - favorite: 0, - tags: toJson([]), - custom_fields: toJson({}), - visibility: await defaultVisibility(k, userId), - overrides: toJson({}), - media_type_override: null, - ...patchToColumns(p), - field_sources: toJson(fieldSources), - created_at: now, - updated_at: now, - deleted_at: null, - }; - await k.insertInto("entries").values(row).execute(); - const entry = toEntry(row); - await recordHistory(k, { userId, entity: "entry", entityId: entry.id, before: null, after: snapshot(entry), actor }); - return entry; + if (!created && Object.keys(p).length === 0 && existing.deletedAt === null) return existing; + return writeEntry(k, existing, p, actor, { + extra: existing.deletedAt === null ? {} : { deleted_at: null }, + historyBefore: created ? null : undefined, + }); }); } @@ -418,7 +443,7 @@ export async function softDeleteEntry( ): Promise { await inTx(db, tx, async (k) => { const e = await loadEntry(k, userId, entryId); - await writeEntry(k, e, {}, actor, { deleted_at: nowIso() }); + await writeEntry(k, e, {}, actor, { extra: { deleted_at: nowIso() } }); }); } @@ -433,11 +458,13 @@ export async function restoreEntryFromHistory( const patch: Record = {}; for (const key of ENTRY_PATCH_KEYS) if (key in before) patch[key] = before[key]; if (typeof before.workId === "string" && before.workId !== current.workId) { - // Undo a move: drop the user's alias and un-tombstone their own private work, then move back. + // Undo a move: drop the forward alias and un-tombstone the user's own private work, then move back + // without writing a reverse alias (later syncs onto the target must not land on the old work). await k .deleteFrom("work_aliases") .where("user_id", "=", userId) .where("from_work_id", "=", before.workId) + .where("to_work_id", "=", current.workId) .execute(); await k .updateTable("works") @@ -445,25 +472,27 @@ export async function restoreEntryFromHistory( .where("id", "=", before.workId) .where("owner_user_id", "=", userId) .execute(); - await moveEntry(k, userId, entryId, before.workId, "user"); + await moveEntry(k, userId, entryId, before.workId, "user", { undo: true }); } const fresh = await loadEntry(k, userId, entryId, { includeDeleted: true }); const work = await k.selectFrom("works").select("media_type").where("id", "=", fresh.workId).executeTakeFirstOrThrow(); - const p = await normalisePatch(k, userId, fresh.mediaTypeOverride ?? work.media_type, patch as Partial); + // The user's own past data: custom values are not re-checked against today's definitions. + const p = await normalisePatch(k, userId, fresh.mediaTypeOverride ?? work.media_type, patch as Partial, { + trustCustomFields: true, + }); const extra = "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {}; - await writeEntry(k, fresh, p, "user", extra); + await writeEntry(k, fresh, p, "user", { extra }); } const isEmpty = (v: unknown) => - v === null || v === false || v === "none" || (Array.isArray(v) && v.length === 0) || + v === null || v === false || v === "none" || v === "" || (Array.isArray(v) && v.length === 0) || (typeof v === "object" && v !== null && Object.keys(v).length === 0); +/** Separator used when two entries' review or private notes are combined by a merge. */ +export const MERGE_TEXT_SEPARATOR = "\n\n---\n\n"; + /** - * Move the user's entry to another work (per-user merge, spec §6.1). Leaves a `work_aliases` row so - * old references resolve, and tombstones the user's own private draft work into the target. - * If the user already has an entry on the target, the two merge: the target's values win, gaps are - * filled from the moved entry, tags/custom fields/overrides are combined, engagements are re-parented, - * and the moved entry is soft-deleted. (A soft-deleted target entry is revived with the moved values.) + * Move the user's entry to another work (per-user merge, spec §6.1). See `moveEntry`. * Returns the surviving entry. */ export async function moveEntryToWork( @@ -477,7 +506,30 @@ export async function moveEntryToWork( return inTx(db, tx, (k) => moveEntry(k, userId, entryId, targetWorkId, actor)); } -async function moveEntry(k: Executor, userId: string, entryId: string, targetWorkId: string, actor: Actor): Promise { +/** The field_sources value for a combined field: "user" if either side was the user's, else the destination's. */ +function combinedSource(src: Entry, dst: Entry, key: string): Actor | undefined { + return src.fieldSources[key] === "user" || dst.fieldSources[key] === "user" ? "user" : dst.fieldSources[key]; +} + +/** + * Move an entry to `targetWorkId`. + * - No entry on the target: the entry's `work_id` changes. + * - A live entry on the target: merge into it. Its values win; empty fields are filled from the moved + * entry; tags/custom fields/overrides are combined; differing review/private notes are concatenated; + * a conflicting rating is kept but the moved rating is recorded in the history row's `before`. + * Engagements are re-parented (with history) and the moved entry is soft-deleted. + * - A soft-deleted entry on the target: revived with the moved entry's values and field_sources. + * Then (unless `undo`): this user's copies and list items follow to the target, a `work_aliases` row + * maps the old work to the target, and the user's own private draft is tombstoned into the target. + */ +async function moveEntry( + k: Executor, + userId: string, + entryId: string, + targetWorkId: string, + actor: Actor, + opts: { undo?: boolean } = {}, +): Promise { const src = await loadEntry(k, userId, entryId); const target = await resolveVisibleWork(k, userId, targetWorkId); if (target.id === src.workId) throw new ValidationError("targetWorkId", "same_work"); @@ -485,31 +537,51 @@ async function moveEntry(k: Executor, userId: string, entryId: string, targetWor let result: Entry; if (!dst) { - result = await writeEntry(k, src, {}, actor, { work_id: target.id }); + result = await writeEntry(k, src, {}, actor, { extra: { work_id: target.id } }); + } else if (dst.deletedAt !== null) { + const all: Partial = {}; + for (const key of ENTRY_PATCH_KEYS) (all as Record)[key] = src[key]; + const sources: Record = {}; + for (const key of ENTRY_PATCH_KEYS) sources[key] = src.fieldSources[key]; + result = await writeEntry(k, dst, all, actor, { extra: { deleted_at: null }, sources }); + await moveEngagements(k, userId, src.id, dst.id, actor); + await writeEntry(k, src, {}, actor, { extra: { deleted_at: nowIso() } }); } else { - const merged: Partial = {}; - const sources: FieldSources = {}; - const take = (key: K, v: EntryPatch[K]) => { - merged[key] = v; - if (src.fieldSources[key]) sources[key] = src.fieldSources[key]; - }; + const merged: Record = {}; + const sources: Record = {}; + let historyExtra: WriteOpts["historyExtra"]; for (const key of ENTRY_PATCH_KEYS) { - if (dst.deletedAt !== null) { - take(key, src[key]); - } else if (key === "tags") { + const s = src[key]; + const d = dst[key]; + if (key === "tags") { const tags = [...new Set([...dst.tags, ...src.tags])]; - if (tags.length !== dst.tags.length) take(key, tags); + if (tags.length !== dst.tags.length) { + merged[key] = tags; + sources[key] = combinedSource(src, dst, key); + } } else if (key === "customFields" || key === "overrides") { - const combined = { ...src[key], ...dst[key] }; - if (Object.keys(combined).length !== Object.keys(dst[key]).length) take(key, combined as never); - } else if (isEmpty(dst[key]) && !isEmpty(src[key])) { - take(key, src[key]); + const combined = { ...(s as object), ...(d as object) }; + if (Object.keys(combined).length !== Object.keys(d as object).length) { + merged[key] = combined; + sources[key] = combinedSource(src, dst, key); + } + } else if (isEmpty(d) && !isEmpty(s)) { + merged[key] = s; + sources[key] = src.fieldSources[key]; + } else if ((key === "review" || key === "privateNotes") && !isEmpty(s) && s !== d) { + merged[key] = `${d as string}${MERGE_TEXT_SEPARATOR}${s as string}`; + sources[key] = combinedSource(src, dst, key); + } else if (key === "rating" && s !== null && s !== d) { + historyExtra = { before: { rating: s }, after: { rating: d } }; } } - result = await writeEntry(k, dst, merged, actor, dst.deletedAt === null ? {} : { deleted_at: null }, sources); - await k.updateTable("engagements").set({ entry_id: dst.id }).where("entry_id", "=", src.id).execute(); - await writeEntry(k, src, {}, actor, { deleted_at: nowIso() }); + result = await writeEntry(k, dst, merged as Partial, actor, { sources, historyExtra }); + await moveEngagements(k, userId, src.id, dst.id, actor); + await writeEntry(k, src, {}, actor, { extra: { deleted_at: nowIso() } }); } + if (opts.undo) return result; + + await repointCopiesAndListItems(k, userId, src.workId, target.id, actor); // Aliases: re-point any that led to the old work, drop one leaving the target (no cycles), add ours. await k @@ -541,7 +613,49 @@ async function moveEntry(k: Executor, userId: string, entryId: string, targetWor .where("owner_user_id", "=", userId) .execute(); return result; +} +/** Re-parent an entry's live and deleted engagements, writing an `entryId` history row for each. */ +async function moveEngagements(k: Executor, userId: string, fromEntryId: string, toEntryId: string, actor: Actor) { + const rows = await k.selectFrom("engagements").select("id").where("entry_id", "=", fromEntryId).execute(); + for (const { id } of rows) { + await k.updateTable("engagements").set({ entry_id: toEntryId, updated_at: nowIso() }).where("id", "=", id).execute(); + await recordHistory(k, { + userId, + entity: "engagement", + entityId: id, + before: { entryId: fromEntryId }, + after: { entryId: toEntryId }, + actor, + }); + } +} + +/** Point this user's copies (with history) and the items of this user's lists at the new work. */ +async function repointCopiesAndListItems(k: Executor, userId: string, fromWorkId: string, toWorkId: string, actor: Actor) { + const copies = await k + .selectFrom("copies") + .select("id") + .where("user_id", "=", userId) + .where("work_id", "=", fromWorkId) + .execute(); + for (const { id } of copies) { + await k.updateTable("copies").set({ work_id: toWorkId, updated_at: nowIso() }).where("id", "=", id).execute(); + await recordHistory(k, { + userId, + entity: "copy", + entityId: id, + before: { workId: fromWorkId }, + after: { workId: toWorkId }, + actor, + }); + } + await k + .updateTable("list_items") + .set({ work_id: toWorkId }) + .where("work_id", "=", fromWorkId) + .where("list_id", "in", k.selectFrom("lists").select("id").where("user_id", "=", userId)) + .execute(); } /** Insert a private draft work owned by the user (no external IDs). Returns its id. */ @@ -832,5 +946,6 @@ export async function acceptSuggestion(db: Db, userId: string, suggestionId: str /** Reject a match suggestion. */ export async function rejectSuggestion(db: Db, userId: string, suggestionId: string): Promise { const s = await loadSuggestion(db.k, userId, suggestionId); + if (s.status !== "pending") throw new ValidationError("status", "not_pending"); await db.k.updateTable("match_suggestions").set({ status: "rejected" }).where("id", "=", s.id).execute(); } diff --git a/packages/core/test/engagements.test.ts b/packages/core/test/engagements.test.ts index 93d9f04..e635877 100644 --- a/packages/core/test/engagements.test.ts +++ b/packages/core/test/engagements.test.ts @@ -7,7 +7,7 @@ import { softDeleteEngagement, updateEngagement, } from "../src/engagements.ts"; -import { upsertEntry } from "../src/entries.ts"; +import { softDeleteEntry, upsertEntry } from "../src/entries.ts"; import { NotFoundError, ValidationError } from "../src/errors.ts"; import { getHistory, restoreVersion } from "../src/history.ts"; import { makeUser, makeWork } from "./user-layer-fixtures.ts"; @@ -114,6 +114,14 @@ forEachDialect("engagements", (getDb) => { expect((await listEngagements(db, userId, entry.id)).map((e) => e.id)).toEqual([early.id, later.id, undated.id]); }); + test("engagements of a soft-deleted entry cannot be changed", async () => { + const { db, userId, entry } = await setup(); + const eg = await addEngagement(db, userId, entry.id, {}, "user"); + await softDeleteEntry(db, userId, entry.id, "user"); + await expect(updateEngagement(db, userId, eg.id, { rating: 1 }, "user")).rejects.toThrow(NotFoundError); + await expect(softDeleteEngagement(db, userId, eg.id, "user")).rejects.toThrow(NotFoundError); + }); + test("another user's engagements are not found", async () => { const { db, userId, entry } = await setup(); const bob = await makeUser(db, "bob"); diff --git a/packages/core/test/entries.test.ts b/packages/core/test/entries.test.ts index 55e429e..e98eadd 100644 --- a/packages/core/test/entries.test.ts +++ b/packages/core/test/entries.test.ts @@ -2,6 +2,7 @@ import { expect, test } from "bun:test"; import { newId } from "@sttracker/db/ids"; import { forEachDialect } from "@sttracker/db/testing"; import { defineCustomField } from "../src/custom-fields.ts"; +import { addEngagement, listEngagements } from "../src/engagements.ts"; import { NotFoundError, ValidationError } from "../src/errors.ts"; import { getEntry, @@ -179,6 +180,23 @@ forEachDialect("entries", (getDb) => { expect([...seen].sort()).toEqual([...titles].sort()); }); + test("concurrent upserts of a new entry yield one entry with both patches", async () => { + const db = getDb(); + const userId = await makeUser(db); + const workId = await makeWork(db); + const [a, b] = await Promise.all([ + upsertEntry(db, userId, workId, { rating: 70 }, "plugin:trakt:c1"), + upsertEntry(db, userId, workId, { tags: ["cosy"] }, "user"), + ]); + expect(a.id).toBe(b.id); + const rows = await db.k.selectFrom("entries").selectAll().where("user_id", "=", userId).execute(); + expect(rows).toHaveLength(1); + const view = await getEntry(db, userId, a.id); + expect(view.rating).toBe(70); + expect(view.tags).toEqual(["cosy"]); + expect(view.fieldSources).toEqual({ rating: "plugin:trakt:c1", tags: "user" }); + }); + test("listEntries filters, sorts and paginates", async () => { const db = getDb(); const userId = await makeUser(db); @@ -302,4 +320,58 @@ forEachDialect("entries", (getDb) => { expect(merged.tags.sort()).toEqual(["x", "y"]); await expect(getEntry(db, userId, src.id)).rejects.toThrow(NotFoundError); }); + + test("merge keeps 'user' field_sources on combined fields", async () => { + const db = getDb(); + const userId = await makeUser(db); + const a = await makeWork(db, { title: "A" }); + const b = await makeWork(db, { title: "B" }); + const src = await upsertEntry(db, userId, a, { tags: ["x"], overrides: { year: 1 } }, "plugin:trakt:c1"); + await upsertEntry(db, userId, b, { tags: ["y"], status: "completed" }, "user"); + await upsertEntry(db, userId, b, { overrides: { title: "B!" } }, "plugin:hardcover:c2"); + const merged = await moveEntryToWork(db, userId, src.id, b, "plugin:steam:c3"); + expect(merged.tags.sort()).toEqual(["x", "y"]); + expect(merged.overrides).toEqual({ year: 1, title: "B!" }); + expect(merged.fieldSources).toEqual({ tags: "user", status: "user", overrides: "plugin:hardcover:c2" }); + }); + + test("merge into a soft-deleted entry carries the moved field_sources as-is", async () => { + const db = getDb(); + const userId = await makeUser(db); + const a = await makeWork(db, { title: "A" }); + const b = await makeWork(db, { title: "B" }); + const src = await upsertEntry(db, userId, a, { rating: 60, review: "mine" }, "user"); + const dst = await upsertEntry(db, userId, b, { status: "dropped", tags: ["old"] }, "plugin:trakt:c1"); + await softDeleteEntry(db, userId, dst.id, "user"); + const merged = await moveEntryToWork(db, userId, src.id, b, "plugin:trakt:c2"); + expect(merged.id).toBe(dst.id); + expect(merged.deletedAt).toBeNull(); + expect(merged.rating).toBe(60); + expect(merged.status).toBe("none"); + expect(merged.tags).toEqual([]); + expect(merged.fieldSources).toEqual({ rating: "user", review: "user" }); + }); + + test("merge concatenates differing text, records the dropped rating and moves engagements with history", async () => { + const db = getDb(); + const userId = await makeUser(db); + const a = await makeWork(db, { title: "A" }); + const b = await makeWork(db, { title: "B" }); + const src = await upsertEntry(db, userId, a, { review: "from A", privateNotes: "same", rating: 40 }, "user"); + const dst = await upsertEntry(db, userId, b, { review: "from B", privateNotes: "same", rating: 90 }, "user"); + const eg = await addEngagement(db, userId, src.id, { platform: "Kindle" }, "user"); + const merged = await moveEntryToWork(db, userId, src.id, b, "user"); + expect(merged.review).toBe("from B\n\n---\n\nfrom A"); + expect(merged.privateNotes).toBe("same"); + expect(merged.rating).toBe(90); + const [h] = await getHistory(db, userId, "entry", dst.id); + expect(h!.before.rating).toBe(40); + expect(h!.after.rating).toBe(90); + expect(h!.before.review).toBe("from B"); + + expect((await listEngagements(db, userId, dst.id)).map((e) => e.id)).toEqual([eg.id]); + const [egh] = await getHistory(db, userId, "engagement", eg.id); + expect(egh!.before).toEqual({ entryId: src.id }); + expect(egh!.after).toEqual({ entryId: dst.id }); + }); }); diff --git a/packages/core/test/history.test.ts b/packages/core/test/history.test.ts index 78e9930..de30484 100644 --- a/packages/core/test/history.test.ts +++ b/packages/core/test/history.test.ts @@ -1,5 +1,6 @@ import { expect, test } from "bun:test"; import { forEachDialect } from "@sttracker/db/testing"; +import { defineCustomField } from "../src/custom-fields.ts"; import { NotFoundError } from "../src/errors.ts"; import { getEntry, moveEntryToWork, quickAdd, softDeleteEntry, upsertEntry } from "../src/entries.ts"; import { getHistory, recordHistory, restoreVersion } from "../src/history.ts"; @@ -80,9 +81,25 @@ forEachDialect("history", (getDb) => { expect(back.draft).toBe(true); const work = await db.k.selectFrom("works").selectAll().where("id", "=", draft.workId).executeTakeFirstOrThrow(); expect(work.merged_into_id).toBeNull(); - // the shared work now aliases back to the draft for this user; no alias cycle remains - const aliases = await db.k.selectFrom("work_aliases").selectAll().where("user_id", "=", userId).execute(); - expect(aliases.map((a) => [a.from_work_id, a.to_work_id])).toEqual([[shared, draft.workId]]); + // undoing removes the forward alias and writes no reverse alias + expect(await db.k.selectFrom("work_aliases").selectAll().where("user_id", "=", userId).execute()).toEqual([]); + // a later sync onto the shared work lands on the shared work, not on the draft + const synced = await upsertEntry(db, userId, shared, { rating: 80 }, "plugin:hardcover:c1"); + expect(synced.workId).toBe(shared); + expect(synced.id).not.toBe(draft.id); + expect((await getEntry(db, userId, draft.id)).rating).toBeNull(); + }); + + test("restoreVersion does not re-check custom fields against today's definitions", async () => { + const db = getDb(); + const userId = await makeUser(db); + const def = await defineCustomField(db, userId, { name: "Mood", type: "text" }); + const e = await upsertEntry(db, userId, await makeWork(db), { customFields: { [def.id]: "cosy" } }, "user"); + await upsertEntry(db, userId, e.workId, { customFields: {} }, "user"); + await db.k.deleteFrom("custom_field_definitions").where("id", "=", def.id).execute(); + const [h] = await getHistory(db, userId, "entry", e.id); + await restoreVersion(db, userId, h!.id); + expect((await getEntry(db, userId, e.id)).customFields).toEqual({ [def.id]: "cosy" }); }); test("another user's history is invisible", async () => { diff --git a/packages/core/test/suggestions.test.ts b/packages/core/test/suggestions.test.ts index 991020c..aadb31a 100644 --- a/packages/core/test/suggestions.test.ts +++ b/packages/core/test/suggestions.test.ts @@ -3,7 +3,10 @@ import type { Db } from "@sttracker/db/db"; import { newId, nowIso } from "@sttracker/db/ids"; import { forEachDialect } from "@sttracker/db/testing"; import { acceptSuggestion, getEntry, quickAdd, rejectSuggestion } from "../src/entries.ts"; -import { NotFoundError } from "../src/errors.ts"; +import { createCopy, listCopies } from "../src/copies.ts"; +import { NotFoundError, ValidationError } from "../src/errors.ts"; +import { getHistory } from "../src/history.ts"; +import { addListItem, createList, getList } from "../src/lists.ts"; import { makeUser, makeWork } from "./user-layer-fixtures.ts"; async function suggest(db: Db, userId: string, workId: string, candidateWorkId: string): Promise { @@ -46,5 +49,22 @@ forEachDialect("suggestions", (getDb) => { expect(await statusOf(db, s)).toBe("pending"); await rejectSuggestion(db, ada, s); expect(await statusOf(db, s)).toBe("rejected"); + await expect(rejectSuggestion(db, ada, s)).rejects.toThrow(ValidationError); + await expect(acceptSuggestion(db, ada, s)).rejects.toThrow(ValidationError); + }); + + test("accepting moves this user's copies and list items to the target work", async () => { + const db = getDb(); + const userId = await makeUser(db); + const draft = await quickAdd(db, userId, { title: "Dune??", mediaType: "book" }); + const target = await makeWork(db, { title: "Dune", externalIds: { isbn13: "1" } }); + const copy = await createCopy(db, userId, { workId: draft.workId, format: "paperback" }); + const list = await createList(db, userId, { name: "Desert" }); + await addListItem(db, userId, list.id, { workId: draft.workId }); + await acceptSuggestion(db, userId, await suggest(db, userId, draft.workId, target)); + expect((await listCopies(db, userId, { workId: target })).map((c) => c.id)).toEqual([copy.id]); + const [ch] = await getHistory(db, userId, "copy", copy.id); + expect(ch!.after).toEqual({ workId: target }); + expect((await getList(db, userId, list.id)).items.map((i) => i.work.title)).toEqual(["Dune"]); }); });