refactor(channels): shared tag-mutation hook + a channel_detail tag_ids test (R8 review)
Address the two remaining review findings on the tag work:
- extract useChannelTagActions — the optimistic attach/detach + create-and-attach flow that
the channel manager and the channel detail page had copy-pasted; the cache-shape-specific
optimistic update is injected (manager maps a ManagedChannel[]; detail patches one
ChannelDetail), so the two stay in sync;
- add a DB-lane test asserting GET /api/channels/{id} returns exactly this user's personal
tag links — not another user's, not the auto/system (user_id NULL) links.
This commit is contained in:
@@ -0,0 +1,54 @@
|
|||||||
|
"""DB-backed regression (R8 Part 2): GET /api/channels/{id} returns exactly THIS user's personal
|
||||||
|
tag links for the channel — not another user's, and not the auto/system links (user_id NULL) —
|
||||||
|
so the channel detail page can reuse the manager's tag editor. Would ship broken if the tag_ids
|
||||||
|
query ever dropped its user_id filter.
|
||||||
|
|
||||||
|
Uses the DB lane (`db` fixture) — skipped when no Postgres is reachable.
|
||||||
|
"""
|
||||||
|
from datetime import datetime, timezone
|
||||||
|
|
||||||
|
from fastapi.testclient import TestClient
|
||||||
|
|
||||||
|
import app.state as state
|
||||||
|
from app.auth import current_user
|
||||||
|
from app.db import get_db
|
||||||
|
from app.main import app
|
||||||
|
from app.models import Channel, ChannelTag, Tag, User
|
||||||
|
|
||||||
|
|
||||||
|
def test_channel_detail_returns_only_this_users_tag_ids(db, monkeypatch):
|
||||||
|
# published_at set so channel_detail skips the YouTube-enrich path (no quota / network in tests).
|
||||||
|
ch = Channel(
|
||||||
|
id="UC_tags_test",
|
||||||
|
title="Tag Test Channel",
|
||||||
|
published_at=datetime(2020, 1, 1, tzinfo=timezone.utc),
|
||||||
|
)
|
||||||
|
db.add(ch)
|
||||||
|
me = User(email="me@example.com")
|
||||||
|
other = User(email="other@example.com")
|
||||||
|
db.add_all([me, other])
|
||||||
|
db.flush()
|
||||||
|
mine = Tag(user_id=me.id, name="Mine", category="other")
|
||||||
|
theirs = Tag(user_id=other.id, name="Theirs", category="other")
|
||||||
|
auto = Tag(user_id=None, name="English", category="language") # a system/auto tag
|
||||||
|
db.add_all([mine, theirs, auto])
|
||||||
|
db.flush()
|
||||||
|
# My personal link, another user's link, and a system/auto link on the same channel — only MINE
|
||||||
|
# must come back for `me`.
|
||||||
|
db.add(ChannelTag(channel_id=ch.id, tag_id=mine.id, user_id=me.id, source="user"))
|
||||||
|
db.add(ChannelTag(channel_id=ch.id, tag_id=theirs.id, user_id=other.id, source="user"))
|
||||||
|
db.add(ChannelTag(channel_id=ch.id, tag_id=auto.id, user_id=None, source="auto"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
# The request gate 503s an unconfigured instance; flip the process-level flag (auto-restored).
|
||||||
|
monkeypatch.setattr(state, "_configured_cache", True)
|
||||||
|
app.dependency_overrides[get_db] = lambda: db
|
||||||
|
app.dependency_overrides[current_user] = lambda: me
|
||||||
|
try:
|
||||||
|
client = TestClient(app) # no context-manager: skip lifespan (scheduler/HLS sweep)
|
||||||
|
resp = client.get(f"/api/channels/{ch.id}")
|
||||||
|
finally:
|
||||||
|
app.dependency_overrides.clear()
|
||||||
|
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
assert resp.json()["tag_ids"] == [mine.id]
|
||||||
@@ -26,6 +26,7 @@ import { api, type FeedFilters, type Me, type ChannelDetail } from "../lib/api";
|
|||||||
import type { FeedView } from "../lib/feedView";
|
import type { FeedView } from "../lib/feedView";
|
||||||
import { channelYouTubeUrl, formatViews } from "../lib/format";
|
import { channelYouTubeUrl, formatViews } from "../lib/format";
|
||||||
import { ChannelTagsControl } from "./ChannelTags";
|
import { ChannelTagsControl } from "./ChannelTags";
|
||||||
|
import { useChannelTagActions } from "../lib/useChannelTagActions";
|
||||||
|
|
||||||
// A dedicated channel page: an "About"-style header (banner, avatar, stats, subscribe) over the
|
// A dedicated channel page: an "About"-style header (banner, avatar, stats, subscribe) over the
|
||||||
// channel's videos (the catalog filtered to this channel). For an un-subscribed channel it
|
// channel's videos (the catalog filtered to this channel). For an un-subscribed channel it
|
||||||
@@ -167,38 +168,24 @@ export default function ChannelPage({
|
|||||||
onError: (err) => notifyYouTubeActionError(err, t("channel.fullHistoryFailed")),
|
onError: (err) => notifyYouTubeActionError(err, t("channel.fullHistoryFailed")),
|
||||||
});
|
});
|
||||||
|
|
||||||
// Personal tags, editable right here (the same control the channel manager uses). Optimistic on
|
// Personal tags, editable right here (the same control + flow the channel manager uses). Optimistic
|
||||||
// this channel's detail cache; a failed attach/detach reverts by refetching. Invalidate the manager
|
// on this channel's detail cache; a failed attach/detach reverts by refetching, and a success
|
||||||
// list too so a tag added here shows there. createTag mirrors the manager's create-and-attach.
|
// invalidates the manager list so a tag added here shows there too.
|
||||||
const tagsQuery = useQuery({ queryKey: qk.tags(), queryFn: api.tags });
|
const tagsQuery = useQuery({ queryKey: qk.tags(), queryFn: api.tags });
|
||||||
const userTags = (tagsQuery.data ?? []).filter((tg) => !tg.system);
|
const userTags = (tagsQuery.data ?? []).filter((tg) => !tg.system);
|
||||||
const toggleTag = (tagId: number) => {
|
const { toggleTag, createAndAttachTag } = useChannelTagActions({
|
||||||
if (!ch) return;
|
optimisticToggle: (id, tagId, attach) =>
|
||||||
const hasTag = ch.tag_ids.includes(tagId);
|
qc.setQueryData<ChannelDetail>(qk.channel(id), (old) =>
|
||||||
qc.setQueryData<ChannelDetail>(qk.channel(channelId), (old) =>
|
old
|
||||||
old
|
? {
|
||||||
? {
|
...old,
|
||||||
...old,
|
tag_ids: attach ? [...old.tag_ids, tagId] : old.tag_ids.filter((x) => x !== tagId),
|
||||||
tag_ids: hasTag ? old.tag_ids.filter((x) => x !== tagId) : [...old.tag_ids, tagId],
|
}
|
||||||
}
|
: old,
|
||||||
: old,
|
),
|
||||||
);
|
onSuccess: () => qc.invalidateQueries({ queryKey: qk.channels() }),
|
||||||
const call = hasTag
|
onError: (id) => qc.invalidateQueries({ queryKey: qk.channel(id) }),
|
||||||
? api.detachChannelTag(channelId, tagId)
|
});
|
||||||
: api.attachChannelTag(channelId, tagId);
|
|
||||||
call
|
|
||||||
.then(() => qc.invalidateQueries({ queryKey: qk.channels() }))
|
|
||||||
.catch(() => qc.invalidateQueries({ queryKey: qk.channel(channelId) }));
|
|
||||||
};
|
|
||||||
const createAndAttachTag = (name: string) => {
|
|
||||||
api
|
|
||||||
.createTag({ name })
|
|
||||||
.then((tag) => {
|
|
||||||
qc.invalidateQueries({ queryKey: qk.tags() });
|
|
||||||
toggleTag(tag.id);
|
|
||||||
})
|
|
||||||
.catch(() => notify({ level: "error", message: t("channels.tags.createFailed") }));
|
|
||||||
};
|
|
||||||
|
|
||||||
const onUnsubscribe = async () => {
|
const onUnsubscribe = async () => {
|
||||||
const ok = await confirm({
|
const ok = await confirm({
|
||||||
@@ -452,8 +439,8 @@ export default function ChannelPage({
|
|||||||
<ChannelTagsControl
|
<ChannelTagsControl
|
||||||
userTags={userTags}
|
userTags={userTags}
|
||||||
attachedIds={ch.tag_ids}
|
attachedIds={ch.tag_ids}
|
||||||
onToggle={toggleTag}
|
onToggle={(tagId) => toggleTag(channelId, tagId, ch.tag_ids.includes(tagId))}
|
||||||
onCreate={createAndAttachTag}
|
onCreate={(name) => createAndAttachTag(channelId, name)}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -28,6 +28,7 @@ import DataTable, { type Column } from "./DataTable";
|
|||||||
import ChannelLayoutGrid from "./ChannelLayoutGrid";
|
import ChannelLayoutGrid from "./ChannelLayoutGrid";
|
||||||
import { LoadingState, StateMessage } from "./QueryState";
|
import { LoadingState, StateMessage } from "./QueryState";
|
||||||
import { ChannelTagsControl } from "./ChannelTags";
|
import { ChannelTagsControl } from "./ChannelTags";
|
||||||
|
import { useChannelTagActions } from "../lib/useChannelTagActions";
|
||||||
import ViewSwitcher, { type ViewOption } from "./ViewSwitcher";
|
import ViewSwitcher, { type ViewOption } from "./ViewSwitcher";
|
||||||
import ColumnSortControl from "./ColumnSortControl";
|
import ColumnSortControl from "./ColumnSortControl";
|
||||||
import Pager from "./Pager";
|
import Pager from "./Pager";
|
||||||
@@ -195,34 +196,22 @@ export default function Channels() {
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
// Tagging is updated optimistically in place (no refetch of the whole channel list, which
|
// Tagging is updated optimistically in place (no refetch of the whole channel list, which
|
||||||
// made the toggle feel ~2s slow); only revert by refetching if the server call fails.
|
// made the toggle feel ~2s slow); only revert by refetching if the server call fails. The
|
||||||
const toggleTag = (id: string, tagId: number, hasTag: boolean) => {
|
// attach/detach + create-and-attach flow is shared with the channel detail page.
|
||||||
qc.setQueryData<ManagedChannel[]>(qk.channels(), (old) =>
|
const { toggleTag, createAndAttachTag } = useChannelTagActions({
|
||||||
(old ?? []).map((ch) =>
|
optimisticToggle: (id, tagId, attach) =>
|
||||||
ch.id === id
|
qc.setQueryData<ManagedChannel[]>(qk.channels(), (old) =>
|
||||||
? {
|
(old ?? []).map((ch) =>
|
||||||
...ch,
|
ch.id === id
|
||||||
tag_ids: hasTag ? ch.tag_ids.filter((x) => x !== tagId) : [...ch.tag_ids, tagId],
|
? {
|
||||||
}
|
...ch,
|
||||||
: ch,
|
tag_ids: attach ? [...ch.tag_ids, tagId] : ch.tag_ids.filter((x) => x !== tagId),
|
||||||
|
}
|
||||||
|
: ch,
|
||||||
|
),
|
||||||
),
|
),
|
||||||
);
|
onError: () => qc.invalidateQueries({ queryKey: qk.channels() }),
|
||||||
const call = hasTag ? api.detachChannelTag(id, tagId) : api.attachChannelTag(id, tagId);
|
});
|
||||||
call.catch(() => qc.invalidateQueries({ queryKey: qk.channels() }));
|
|
||||||
};
|
|
||||||
|
|
||||||
// Create a brand-new personal tag from the row/card editor and attach it to this channel in one
|
|
||||||
// step (so tagging no longer requires the top-of-page "Manage tags" dialog first). Refresh the tag
|
|
||||||
// catalog so the new tag appears everywhere; attach optimistically via toggleTag.
|
|
||||||
const createAndAttachTag = (channelId: string, name: string) => {
|
|
||||||
api
|
|
||||||
.createTag({ name })
|
|
||||||
.then((tag) => {
|
|
||||||
qc.invalidateQueries({ queryKey: qk.tags() });
|
|
||||||
toggleTag(channelId, tag.id, false);
|
|
||||||
})
|
|
||||||
.catch(() => notify({ level: "error", message: t("channels.tags.createFailed") }));
|
|
||||||
};
|
|
||||||
const syncSubs = useMutation({
|
const syncSubs = useMutation({
|
||||||
mutationFn: () => api.syncSubscriptions(),
|
mutationFn: () => api.syncSubscriptions(),
|
||||||
onSuccess: (r: { subscriptions?: number }) => {
|
onSuccess: (r: { subscriptions?: number }) => {
|
||||||
|
|||||||
@@ -0,0 +1,43 @@
|
|||||||
|
import { useQueryClient } from "@tanstack/react-query";
|
||||||
|
import { useTranslation } from "react-i18next";
|
||||||
|
import { api } from "./api";
|
||||||
|
import { qk } from "./queryKeys";
|
||||||
|
import { notify } from "./notifications";
|
||||||
|
|
||||||
|
/** The shared channel-tag mutation flow used by both the channel manager and the channel detail
|
||||||
|
* page: optimistic attach/detach + a create-then-attach shortcut, with the cache-shape-specific
|
||||||
|
* optimistic update injected (the manager maps over a ManagedChannel[]; the detail patches one
|
||||||
|
* ChannelDetail). Keeps the two call sites from drifting. */
|
||||||
|
export function useChannelTagActions(opts: {
|
||||||
|
/** Apply the optimistic tag change to whichever cache the caller owns. `attach` true = add. */
|
||||||
|
optimisticToggle: (channelId: string, tagId: number, attach: boolean) => void;
|
||||||
|
/** Called after the attach/detach lands (e.g. sync a sibling cache). Optional. */
|
||||||
|
onSuccess?: (channelId: string) => void;
|
||||||
|
/** Called if the attach/detach fails — revert by refetching the owning cache. */
|
||||||
|
onError: (channelId: string) => void;
|
||||||
|
}) {
|
||||||
|
const qc = useQueryClient();
|
||||||
|
const { t } = useTranslation();
|
||||||
|
|
||||||
|
const toggleTag = (channelId: string, tagId: number, hasTag: boolean) => {
|
||||||
|
opts.optimisticToggle(channelId, tagId, !hasTag);
|
||||||
|
const call = hasTag
|
||||||
|
? api.detachChannelTag(channelId, tagId)
|
||||||
|
: api.attachChannelTag(channelId, tagId);
|
||||||
|
call.then(() => opts.onSuccess?.(channelId)).catch(() => opts.onError(channelId));
|
||||||
|
};
|
||||||
|
|
||||||
|
// Create a brand-new personal tag and attach it in one step (so tagging doesn't require the
|
||||||
|
// top-of-page "Manage tags" dialog first). Refresh the tag catalog so it appears everywhere.
|
||||||
|
const createAndAttachTag = (channelId: string, name: string) => {
|
||||||
|
api
|
||||||
|
.createTag({ name })
|
||||||
|
.then((tag) => {
|
||||||
|
qc.invalidateQueries({ queryKey: qk.tags() });
|
||||||
|
toggleTag(channelId, tag.id, false);
|
||||||
|
})
|
||||||
|
.catch(() => notify({ level: "error", message: t("channels.tags.createFailed") }));
|
||||||
|
};
|
||||||
|
|
||||||
|
return { toggleTag, createAndAttachTag };
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user