diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 28bdf17..4c69ff3 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -34,7 +34,9 @@ Several behaviours are not obvious from the spec and would otherwise have to be - **Hierarchy visibility.** `childSummary` counts the user's live entries up to three levels below the entry's work (child, grandchild, great-grandchild). The default library view (`parentId: null`) lists entries with no ancestor, up to three levels, on which the user has a live entry. An episode whose show - is untracked therefore stays visible, and is rolled up once the show gets an entry. + is untracked therefore stays visible, and is rolled up once the show gets an entry. Drilling in + (`parentId: X`) applies the same rule relative to X: descendants up to three levels with no live-entry + work strictly between them and X. `childSummary` still counts every descendant entry. - **Concurrent creates.** `upsertEntry` inserts with `ON CONFLICT (user_id, work_id) DO NOTHING`, then patches through the update path, so a concurrent creator never aborts the caller's transaction. diff --git a/packages/core/src/engagements.ts b/packages/core/src/engagements.ts index 948e6f0..e758308 100644 --- a/packages/core/src/engagements.ts +++ b/packages/core/src/engagements.ts @@ -264,11 +264,12 @@ export async function restoreEngagementFromHistory( // Undo a merge's re-parenting; the old entry must be this user's. const owner = await k .selectFrom("entries") - .select("id") + .select(["id", "deleted_at"]) .where("id", "=", before.entryId) .where("user_id", "=", userId) .executeTakeFirst(); if (!owner) throw new NotFoundError("entry"); + if (owner.deleted_at !== null) throw new ValidationError("entryId", "target entry is deleted"); 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 35d8bba..28d5110 100644 --- a/packages/core/src/entries.ts +++ b/packages/core/src/entries.ts @@ -730,6 +730,21 @@ const HAS_ENTRY_ANCESTOR = sql`exists (select 1 from works a1 and (pe.work_id = a1.id or pe.work_id = a2.id or pe.work_id = a3.id) where a1.id = w.parent_id)`; +/** + * True when the entry's work is a descendant of `parentId` (up to three levels) with no work strictly + * between them carrying a live entry of this user: the top-level rule, relative to `parentId`. + */ +function underParent(parentId: string) { + const liveEntryOn = (ids: ReturnType) => + sql`exists (select 1 from entries pe where pe.user_id = e.user_id and pe.deleted_at is null and pe.work_id in (${ids}))`; + return sql`(w.parent_id = ${parentId} + or exists (select 1 from works p1 where p1.id = w.parent_id and p1.parent_id = ${parentId} + and not ${liveEntryOn(sql`p1.id`)}) + or exists (select 1 from works p1 inner join works p2 on p2.id = p1.parent_id + where p1.id = w.parent_id and p2.parent_id = ${parentId} + and not ${liveEntryOn(sql`p1.id, p2.id`)}))`; +} + function viewQuery(k: Executor, userId: string) { return k .selectFrom("entries as e") @@ -801,7 +816,8 @@ export interface EntryFilter { q?: string; /** * Default `null`: the top of the user's library, i.e. entries with no ancestor work (up to three - * levels) carrying a live entry of this user. A work id lists that work's direct children. + * levels) carrying a live entry of this user. A work id applies the same rule relative to that work: + * its descendants (up to three levels) with no live-entry work strictly between them and it. */ parentId?: string | null; sort?: "updated" | "title" | "rating"; @@ -829,7 +845,7 @@ export async function listEntries( let q = viewQuery(db.k, userId); const parentId = filter.parentId === undefined ? null : filter.parentId; - q = parentId === null ? q.where(sql`not ${HAS_ENTRY_ANCESTOR}`) : q.where("w.parent_id", "=", parentId); + q = q.where(parentId === null ? sql`not ${HAS_ENTRY_ANCESTOR}` : underParent(parentId)); if (filter.mediaType) q = q.where(sql`coalesce(e.media_type_override, w.media_type)`, "=", filter.mediaType); if (filter.status) q = q.where("e.status", "=", filter.status); if (filter.tag) { diff --git a/packages/core/test/engagements.test.ts b/packages/core/test/engagements.test.ts index e635877..6b20545 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 { softDeleteEntry, upsertEntry } from "../src/entries.ts"; +import { moveEntryToWork, 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"; @@ -122,6 +122,19 @@ forEachDialect("engagements", (getDb) => { await expect(softDeleteEngagement(db, userId, eg.id, "user")).rejects.toThrow(NotFoundError); }); + test("restoring a re-parenting onto a soft-deleted entry is rejected", async () => { + const { db, userId, entry } = await setup(); + const other = await upsertEntry(db, userId, await makeWork(db, { title: "Other" }), {}, "user"); + const eg = await addEngagement(db, userId, entry.id, {}, "user"); + await moveEntryToWork(db, userId, entry.id, other.workId, "user"); + const [h] = await getHistory(db, userId, "engagement", eg.id); + expect(h!.before).toEqual({ entryId: entry.id }); + // the merged-away entry is soft-deleted, so the engagement cannot go back to it + const err = await restoreVersion(db, userId, h!.id).catch((e: unknown) => e); + expect(err).toBeInstanceOf(ValidationError); + expect(err).toMatchObject({ field: "entryId", reason: "target entry is deleted" }); + }); + 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 e98eadd..08dbafb 100644 --- a/packages/core/test/entries.test.ts +++ b/packages/core/test/entries.test.ts @@ -140,6 +140,18 @@ forEachDialect("entries", (getDb) => { const top2 = await listEntries(db, userId, {}); expect(top2.items.map((i) => i.id).sort()).toEqual([showEntry.id, lone.id, show2Entry.id].sort()); expect(top2.items.find((i) => i.id === show2Entry.id)!.childSummary).toEqual({ total: 2, completed: 1 }); + + // drilling into the show lists the episodes, since the season between them has no entry + const drill = await listEntries(db, userId, { parentId: show2 }); + expect(drill.items.map((i) => i.work.mediaType)).toEqual(["episode", "episode"]); + expect(await listEntries(db, userId, { parentId: show2, q: "nothing matches" })).toEqual({ items: [], nextCursor: null }); + + // once the season has an entry, the show lists the season and the season lists the episodes + const seasonEntry = await upsertEntry(db, userId, season, { status: "in_progress" }, "user"); + expect((await listEntries(db, userId, { parentId: show2 })).items.map((i) => i.id)).toEqual([seasonEntry.id]); + const seasonDrill = await listEntries(db, userId, { parentId: season }); + expect(seasonDrill.items.map((i) => i.work.mediaType)).toEqual(["episode", "episode"]); + expect(seasonDrill.items.find((i) => i.id === seasonEntry.id)).toBeUndefined(); }); test("orphaned child entries are top level until an ancestor has an entry", async () => {