refactor: address /code-review on R11 feedback symmetry

- admin._decide: add an idempotency guard (409 unless status was 'pending') so a
  repeat/stale approve OR deny can't re-run its side effects — now that deny emails,
  this stops duplicate denial mails and a rejection mail to an already-approved user.
- SavedViewsWidget: extract runViewMutation(fn, failMsg) (returns success) — dedups
  the five try/catch+refresh blocks AND lets create/rename close their input only on
  success, so a failed create keeps the user's typed name.
- worker._notify_download_done: take user_id/title captured BEFORE the caller's commit
  (no post-commit refresh SELECT); add a  so the inbox labels an edit job
  'Edit ready' instead of 'Download ready' (new inbox.editDone i18n, HU+EN).

Gate green (ruff/tsc/eslint-0/prettier/knip/vitest 79/pytest 193); smoke-tested the
saved-views + notifications surfaces render clean on a fresh dev server.
This commit is contained in:
2026-08-11 20:25:18 +02:00
parent e2fe6fa286
commit 359bac149b
6 changed files with 72 additions and 49 deletions
+6
View File
@@ -74,6 +74,12 @@ def _decide(db: Session, invite_id: int, admin: User, approved: bool) -> Invite:
inv = db.get(Invite, invite_id)
if inv is None:
raise HTTPException(status_code=404, detail="No such invite")
# Idempotency guard: only a still-pending invite can be decided. Without this a repeat/stale
# approve or deny re-runs its side effects — re-activating a user, or (now that deny emails)
# sending a duplicate decision mail, even flipping an already-approved invite to "denied" and
# mailing a rejection to an already-active account.
if inv.status != "pending":
raise HTTPException(status_code=409, detail="Invite already decided")
inv.status = "approved" if approved else "denied"
inv.decided_at = datetime.now(timezone.utc)
inv.decided_by = admin.email
+21 -12
View File
@@ -40,24 +40,28 @@ from app.titles import normalize_title
log = logging.getLogger("siftlode.worker")
def _notify_download_done(db, job: DownloadJob | None, title: str | None) -> None:
"""Post a best-effort 'download ready' notice to the owner's inbox — downloads used to slide to
Done silently while scheduler jobs and playlist-push already notified (R11 feedback symmetry).
A notification failure must never mask the completed download, so it is isolated + rolled back
(mirrors scheduler._notify_done). English title is a fallback; the inbox localizes from `data`."""
if not job:
def _notify_download_done(
db, user_id: int | None, title: str | None, job_id, *, kind: str
) -> None:
"""Post a best-effort 'ready' notice to the owner's inbox — downloads/edits used to finish
silently while scheduler jobs and playlist-push already notified (R11 feedback symmetry). A
notification failure must never mask the completed job, so it is isolated + rolled back (mirrors
scheduler._notify_done). `kind` ("download"/"edit") lets the inbox pick accurate copy; the
English title here is only a fallback for an unknown type. `user_id`/`title` are captured by the
caller BEFORE its commit, so reading them here needs no post-commit refresh of an expired row."""
if user_id is None:
return
try:
create_notification(
db,
user_id=job.user_id,
user_id=user_id,
type="download_done",
title=f"Download ready: {title}" if title else "Download ready",
data={"title": title},
title=f"Ready to watch: {title}" if title else "Ready to watch",
data={"title": title, "kind": kind},
)
except Exception:
db.rollback()
log.exception("failed to write download-done notification for job %s", job.id)
log.exception("failed to write download-done notification for job %s", job_id)
_stop = threading.Event()
@@ -657,8 +661,10 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe
if not job.display_name:
# display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early).
job.display_name = meta.title[:255] if meta.title else None
# Capture the owner before commit expires the row (avoids a refresh SELECT just to notify).
owner_id = job.user_id if job else None
db.commit()
_notify_download_done(db, job, meta.title)
_notify_download_done(db, owner_id, meta.title, job_id, kind="download")
log.info("job %s done: %s", job_id, rel)
except (_Aborted, editmod.EditAborted):
@@ -790,8 +796,11 @@ def _process_edit(job_id: int, asset_id: int) -> None:
job.status = "done"
job.progress = 100
job.phase = None
# Capture owner + title before commit expires the rows (avoids a refresh SELECT).
owner_id = job.user_id if job else None
dl_title = asset.title if asset else None
db.commit()
_notify_download_done(db, job, asset.title if asset else None)
_notify_download_done(db, owner_id, dl_title, job_id, kind="edit")
log.info("edit job %s done: %s", job_id, rel)
except editmod.EditAborted:
@@ -259,9 +259,10 @@ function NotificationRow({
});
body = d.summary || n.body;
} else if (isDownloadDone) {
// "Download ready", with the video's title as the body (the producer ships it in data.title).
const d = n.data as { title?: string | null };
title = t("inbox.downloadDone.title");
// "Download ready" / "Edit ready" per the producer's kind, with the video's title as the body
// (shipped in data.title). Same notification type; the copy differs so an edit isn't mislabelled.
const d = n.data as { title?: string | null; kind?: string };
title = t(d.kind === "edit" ? "inbox.editDone.title" : "inbox.downloadDone.title");
body = d.title || n.body;
}
return (
+35 -34
View File
@@ -72,6 +72,22 @@ export default function SavedViewsWidget({
}, [viewsQuery.data]);
const refresh = () => qc.invalidateQueries({ queryKey: qk.savedViews() });
// Run a saved-view mutation with uniform feedback: on failure surface the server's reason instead
// of a silent swallow, and always refetch afterwards (which also reverts an optimistic change to
// the server's truth). Returns whether it succeeded, so callers can gate success-only side effects
// (e.g. closing an editor) and keep the input open — with the user's text — on failure.
async function runViewMutation(fn: () => Promise<unknown>, failMsg: string): Promise<boolean> {
try {
await fn();
return true;
} catch (e) {
notifyActionError(e, failMsg);
return false;
} finally {
refresh();
}
}
const currentKey = keyOf(filters);
async function save() {
@@ -79,35 +95,30 @@ export default function SavedViewsWidget({
if (!name) return;
// Strip the transient shuffle seed so a saved "shuffle" view re-rolls fresh on apply.
const { seed: _seed, ...clean } = filters;
setNaming(false);
setDraftName("");
try {
await api.createView(name, clean);
} catch (e) {
notifyActionError(e, t("views.actionFailed"));
// Close the naming input only on success, so a failed create keeps the user's typed name.
if (await runViewMutation(() => api.createView(name, clean), t("views.actionFailed"))) {
setNaming(false);
setDraftName("");
}
refresh();
}
async function rename(id: number, name: string) {
const next = name.trim();
setRenameId(null);
if (!next) return;
try {
await api.updateView(id, { name: next });
} catch (e) {
notifyActionError(e, t("views.actionFailed"));
if (!next) {
setRenameId(null);
return;
}
// Close the rename editor only on success, so a failed rename stays open for a retry.
if (await runViewMutation(() => api.updateView(id, { name: next }), t("views.actionFailed"))) {
setRenameId(null);
}
refresh();
}
async function toggleDefault(v: SavedView) {
try {
await api.updateView(v.id, { is_default: !v.is_default });
} catch (e) {
notifyActionError(e, t("views.actionFailed"));
}
refresh();
await runViewMutation(
() => api.updateView(v.id, { is_default: !v.is_default }),
t("views.actionFailed"),
);
}
async function remove(v: SavedView) {
@@ -117,12 +128,7 @@ export default function SavedViewsWidget({
danger: true,
});
if (!ok) return;
try {
await api.deleteView(v.id);
} catch (e) {
notifyActionError(e, t("views.actionFailed"));
}
refresh();
await runViewMutation(() => api.deleteView(v.id), t("views.actionFailed"));
}
async function share(v: SavedView) {
@@ -146,14 +152,9 @@ export default function SavedViewsWidget({
qk.savedViews(),
next.map((id) => views.find((v) => v.id === id)!),
);
// On failure the refetch below reverts the optimistic order to the server's truth; without the
// toast the drop would silently snap back with no explanation.
try {
await api.reorderViews(next);
} catch (e) {
notifyActionError(e, t("views.reorderFailed"));
}
refresh();
// On failure runViewMutation's refetch reverts the optimistic order to the server's truth;
// without the toast the drop would silently snap back with no explanation.
await runViewMutation(() => api.reorderViews(next), t("views.reorderFailed"));
}
return (
+3
View File
@@ -17,6 +17,9 @@
"downloadDone": {
"title": "Download ready"
},
"editDone": {
"title": "Edit ready"
},
"maintenance": {
"title": "Videos removed",
"body_one": "{{count}} saved or playlisted video was removed because it's no longer available on YouTube.",
+3
View File
@@ -17,6 +17,9 @@
"downloadDone": {
"title": "Letöltés kész"
},
"editDone": {
"title": "Vágás kész"
},
"maintenance": {
"title": "Videók eltávolítva",
"body_one": "{{count}} mentett vagy lejátszási listás videó eltávolítva, mert már nem elérhető a YouTube-on.",