US03-04: Review and Approve Proposals in the Browser #64
@@ -154,3 +154,18 @@ a.link:hover { text-decoration: underline; }
|
||||
overflow: auto;
|
||||
white-space: pre-wrap;
|
||||
}
|
||||
|
||||
/* Albums: two-pane proposal review — folder list left, evidence/edit right. */
|
||||
.two-pane { display: grid; grid-template-columns: minmax(200px, 320px) 1fr; gap: 16px; align-items: start; }
|
||||
.album-list { list-style: none; margin: 0; padding: 0; display: grid; gap: 4px; }
|
||||
.album-list li { margin: 0; }
|
||||
.album-list a { display: block; padding: 6px 8px; border-radius: 8px; border: 1px solid transparent; }
|
||||
.album-list a[aria-current="true"] { border-color: var(--border); background: var(--surface-2); }
|
||||
.badge.stale { color: var(--warn); border-color: var(--warn); }
|
||||
.badge.approved { color: var(--ok); border-color: var(--ok); }
|
||||
.badge.error { color: var(--danger); border-color: var(--danger); }
|
||||
|
||||
/* Narrow screens: stack the panes so blockers and actions stay reachable. */
|
||||
@media (max-width: 720px) {
|
||||
.two-pane { grid-template-columns: 1fr; }
|
||||
}
|
||||
|
||||
@@ -16,6 +16,7 @@
|
||||
<a href="#/safety" data-nav="safety">Safety</a>
|
||||
<a href="#/library" data-nav="library">Library</a>
|
||||
<a href="#/analyze" data-nav="analyze">Analyze</a>
|
||||
<a href="#/albums" data-nav="albums">Albums</a>
|
||||
<a href="#/stats" data-nav="stats">Stats</a>
|
||||
</nav>
|
||||
</header>
|
||||
|
||||
@@ -79,4 +79,24 @@ export const api = {
|
||||
libraryAssets: (params = {}, opts = {}) =>
|
||||
request("/library/assets?" + new URLSearchParams(params).toString(), opts),
|
||||
libraryStats: (opts = {}) => request("/library/stats", opts),
|
||||
|
||||
// ── Albums: evidence and naming proposals ───────────────────────────────
|
||||
albumEvidence: (params = {}, opts = {}) =>
|
||||
request("/albums/evidence?" + new URLSearchParams(params).toString(), opts),
|
||||
albumProposals: (params = {}, opts = {}) =>
|
||||
request("/albums/proposals?" + new URLSearchParams(params).toString(), opts),
|
||||
generateProposals: (payload = {}, opts = {}) =>
|
||||
request("/albums/proposals", { method: "POST", body: JSON.stringify(payload), ...opts }),
|
||||
editProposal: (album, payload, opts = {}) =>
|
||||
request(`/albums/proposals/${encodeURIComponent(album)}/edit`, {
|
||||
method: "POST",
|
||||
body: JSON.stringify(payload),
|
||||
...opts,
|
||||
}),
|
||||
approveProposal: (album, payload, opts = {}) =>
|
||||
request(`/albums/proposals/${encodeURIComponent(album)}/approve`, {
|
||||
method: "POST",
|
||||
body: JSON.stringify(payload),
|
||||
...opts,
|
||||
}),
|
||||
};
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { api } from "./api.js";
|
||||
import { navigate, onRouteChange, parseHash } from "./router.js";
|
||||
import {
|
||||
renderAlbums,
|
||||
renderAnalyze,
|
||||
renderLibrary,
|
||||
renderSafety,
|
||||
@@ -360,6 +361,7 @@ function render() {
|
||||
else if (path === "/safety") renderSafety(root, params);
|
||||
else if (path === "/library") renderLibrary(root, params);
|
||||
else if (path === "/analyze") renderAnalyze(root, params);
|
||||
else if (path === "/albums") renderAlbums(root, params);
|
||||
else if (path === "/stats") renderStats(root, params);
|
||||
else show(errorBanner("Unknown view"));
|
||||
}
|
||||
|
||||
@@ -321,6 +321,227 @@ export async function renderStats(root) {
|
||||
);
|
||||
}
|
||||
|
||||
// ── Albums: evidence + naming proposals ──────────────────────────────────────
|
||||
// Two panes (concept §7): albums on the left with proposal state, the selected
|
||||
// album's evidence, rationale, confidence, and editable final name on the right.
|
||||
// Approving never renames anything — it records the approved name only.
|
||||
export async function renderAlbums(root, params = {}) {
|
||||
setActiveNav("albums");
|
||||
let evidence, proposals;
|
||||
try {
|
||||
[evidence, proposals] = await Promise.all([
|
||||
api.albumEvidence({ limit: 500 }),
|
||||
api.albumProposals(),
|
||||
]);
|
||||
} catch (error) {
|
||||
root.replaceChildren(errorBanner(`Failed to load albums: ${error.message}`));
|
||||
return;
|
||||
}
|
||||
|
||||
const byAlbum = new Map(proposals.items.map((item) => [item.album, item]));
|
||||
const folders = evidence.folders;
|
||||
const selected = params.album || (folders[0] && folders[0].album) || null;
|
||||
|
||||
const list = el(
|
||||
"ul",
|
||||
{ class: "album-list", "data-testid": "album-list", "aria-label": "Albums" },
|
||||
...folders.map((folder) => {
|
||||
const proposal = byAlbum.get(folder.album);
|
||||
const state = proposal ? (proposal.stale ? "stale" : proposal.status) : "none";
|
||||
return el(
|
||||
"li",
|
||||
{},
|
||||
el(
|
||||
"a",
|
||||
{
|
||||
class: "link",
|
||||
href: `#/albums?album=${encodeURIComponent(folder.album)}`,
|
||||
"data-testid": "album-row",
|
||||
"data-album": folder.album,
|
||||
"aria-current": folder.album === selected ? "true" : false,
|
||||
},
|
||||
folder.album,
|
||||
" ",
|
||||
el("span", { class: `badge ${state}`, "data-testid": "album-state" }, state),
|
||||
" ",
|
||||
el("span", { class: "muted" }, `${folder.asset_count} photos`)
|
||||
)
|
||||
);
|
||||
})
|
||||
);
|
||||
|
||||
const detail = selected
|
||||
? albumDetail(
|
||||
root,
|
||||
folders.find((folder) => folder.album === selected),
|
||||
byAlbum.get(selected),
|
||||
params
|
||||
)
|
||||
: el("p", { class: "muted" }, "No albums with evidence yet.");
|
||||
|
||||
root.replaceChildren(
|
||||
el("h1", {}, "Albums"),
|
||||
el(
|
||||
"div",
|
||||
{ class: "toolbar" },
|
||||
el(
|
||||
"button",
|
||||
{
|
||||
class: "primary",
|
||||
"data-testid": "generate-proposals",
|
||||
onclick: async () => {
|
||||
try {
|
||||
await api.generateProposals({});
|
||||
render();
|
||||
} catch (error) {
|
||||
alert(`Could not generate proposals: ${error.message}`);
|
||||
}
|
||||
},
|
||||
},
|
||||
"Generate proposals"
|
||||
)
|
||||
),
|
||||
el("div", { class: "two-pane" }, el("div", { class: "pane-left" }, list), el("div", { class: "pane-right" }, detail))
|
||||
);
|
||||
}
|
||||
|
||||
function albumDetail(root, folder, proposal, params) {
|
||||
if (!folder) return el("p", { class: "muted" }, "Album not found.");
|
||||
|
||||
const evidenceBlock = el(
|
||||
"div",
|
||||
{ "data-testid": "album-evidence" },
|
||||
el("h2", {}, "Evidence"),
|
||||
el(
|
||||
"dl",
|
||||
{},
|
||||
el("dt", {}, "Source album"),
|
||||
el("dd", { "data-testid": "source-album" }, folder.album),
|
||||
el("dt", {}, "Photos affected"),
|
||||
el("dd", { "data-testid": "affected-count" }, String(folder.asset_count)),
|
||||
el("dt", {}, "Analyzed"),
|
||||
el("dd", {}, `${folder.analyzed_count} (${folder.nsfw_count} NSFW, ${folder.pending_count} pending)`),
|
||||
el("dt", {}, "Year"),
|
||||
el(
|
||||
"dd",
|
||||
{},
|
||||
folder.dominant_year != null
|
||||
? String(folder.dominant_year)
|
||||
: folder.year_conflict
|
||||
? "conflicting"
|
||||
: "unknown"
|
||||
),
|
||||
el("dt", {}, "Locations"),
|
||||
el("dd", {}, folder.locations.map((l) => l.value).join(", ") || "—")
|
||||
),
|
||||
facetBlock("Top tags", folder.tags.slice(0, 12))
|
||||
);
|
||||
|
||||
if (!proposal) {
|
||||
return el(
|
||||
"div",
|
||||
{},
|
||||
el("h2", {}, folder.album),
|
||||
el("p", { class: "muted", "data-testid": "no-proposal" }, "No proposal yet — generate one."),
|
||||
evidenceBlock
|
||||
);
|
||||
}
|
||||
|
||||
const issues = el("div", { class: "alert", role: "alert", "data-testid": "name-issues", hidden: "hidden" });
|
||||
const input = el("input", {
|
||||
type: "text",
|
||||
id: "final-name",
|
||||
value: proposal.name || "",
|
||||
"aria-label": "Final album name",
|
||||
"data-testid": "final-name",
|
||||
// Prompt validation: mirror the server's naming policy so a bad name is
|
||||
// flagged as it is typed, before any request is sent.
|
||||
oninput: () => showIssues(issues, input.value),
|
||||
});
|
||||
|
||||
const save = el(
|
||||
"button",
|
||||
{
|
||||
"data-testid": "save-name",
|
||||
onclick: () => mutate(root, () => api.editProposal(proposal.album, { name: input.value, expected_version: proposal.version }), params),
|
||||
},
|
||||
"Save name"
|
||||
);
|
||||
|
||||
const approve = el(
|
||||
"button",
|
||||
{
|
||||
class: "primary",
|
||||
"data-testid": "approve",
|
||||
disabled: proposal.stale || proposal.status === "error" ? "disabled" : false,
|
||||
title: proposal.stale ? "evidence changed — regenerate first" : false,
|
||||
onclick: () => mutate(root, () => api.approveProposal(proposal.album, { expected_version: proposal.version }), params),
|
||||
},
|
||||
proposal.status === "approved" ? "Approved" : "Approve name"
|
||||
);
|
||||
|
||||
const nodes = [
|
||||
el("h2", {}, folder.album),
|
||||
el(
|
||||
"div",
|
||||
{ class: "decision-bar" },
|
||||
el("span", { class: `badge ${proposal.status}`, "data-testid": "proposal-status" }, proposal.status),
|
||||
proposal.confidence != null
|
||||
? el("span", { class: "badge", "data-testid": "confidence" }, `confidence ${proposal.confidence}`)
|
||||
: null,
|
||||
proposal.stale ? el("span", { class: "badge attention", "data-testid": "stale" }, "stale — regenerate") : null
|
||||
),
|
||||
];
|
||||
if (params.conflict) {
|
||||
nodes.push(
|
||||
el(
|
||||
"div",
|
||||
{ class: "alert", role: "alert", "data-testid": "conflict" },
|
||||
"This proposal changed since you loaded it. Nothing was modified; showing the latest state."
|
||||
)
|
||||
);
|
||||
}
|
||||
if (proposal.status === "error") {
|
||||
nodes.push(
|
||||
el("div", { class: "alert", role: "alert", "data-testid": "proposal-error" }, `Generation failed: ${proposal.error_code}`)
|
||||
);
|
||||
}
|
||||
nodes.push(
|
||||
el("p", { class: "muted", "data-testid": "suggested-name" }, `Suggested: ${proposal.proposed_name || "—"}`),
|
||||
el("p", { "data-testid": "rationale" }, proposal.rationale || "No rationale given."),
|
||||
el("div", { class: "toolbar" }, el("label", { for: "final-name" }, "Final name "), input, save, approve),
|
||||
issues,
|
||||
evidenceBlock
|
||||
);
|
||||
return el("div", {}, ...nodes);
|
||||
}
|
||||
|
||||
// Mirrors photo_pipeline/services/naming.py so typing feedback matches the server.
|
||||
const FORBIDDEN_RE = /[/\\:*?"<>|]/;
|
||||
|
||||
function showIssues(node, value) {
|
||||
const problems = [];
|
||||
if (FORBIDDEN_RE.test(value)) problems.push('a name cannot contain / \\ : * ? " < > |');
|
||||
if (!value.replace(/[\s.]/g, "")) problems.push("a name cannot be empty");
|
||||
node.textContent = problems.join("; ");
|
||||
node.hidden = problems.length === 0;
|
||||
}
|
||||
|
||||
async function mutate(root, action, params) {
|
||||
try {
|
||||
await action();
|
||||
render();
|
||||
} catch (error) {
|
||||
if (error.status === 409) {
|
||||
// Stale browser state: show the conflict and the server's latest truth.
|
||||
navigate("/albums", { ...params, conflict: "1" });
|
||||
render();
|
||||
} else {
|
||||
alert(`Failed: ${error.message}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── shared bits ───────────────────────────────────────────────────────────────
|
||||
function stat(label, value) {
|
||||
return el("div", { class: "stat card" }, el("div", { class: "stat-value" }, String(value ?? 0)), el("div", { class: "muted" }, label));
|
||||
|
||||
222
tests/e2e/test_albums_ui.py
Normal file
222
tests/e2e/test_albums_ui.py
Normal file
@@ -0,0 +1,222 @@
|
||||
"""Browser journeys for the Albums proposal view (US03-04).
|
||||
|
||||
Covers evidence display, editing with prompt validation, collision/invalid-name
|
||||
guidance, explicit approval, visible stale-conflict handling, keyboard operation,
|
||||
and the safety property that approving a name renames nothing on disk.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import uuid
|
||||
from datetime import datetime, timezone
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
from playwright.sync_api import expect
|
||||
|
||||
from tests.e2e._pipeline_harness import Server, image, seed_library
|
||||
|
||||
NOW = datetime(2026, 1, 1, tzinfo=timezone.utc)
|
||||
TIMEOUT = 10
|
||||
|
||||
|
||||
def _seed(tmp_path):
|
||||
"""A library with one album ("rome") whose two photos are SFW and analysed."""
|
||||
seeded = seed_library(tmp_path, {}, {}) # empty scan; add the album below
|
||||
album = seeded.lib / "rome"
|
||||
album.mkdir()
|
||||
image(album / "a.jpg", 1)
|
||||
image(album / "b.jpg", 2)
|
||||
|
||||
from sqlalchemy import select
|
||||
|
||||
from photo_pipeline.config import Config
|
||||
from photo_pipeline.db import create_db_engine, create_session_factory, run_migrations
|
||||
from photo_pipeline.models import AnalysisResult, Asset, SafetyReview
|
||||
from photo_pipeline.services.inventory import InventoryService
|
||||
|
||||
config = Config.from_env(
|
||||
{
|
||||
"PHOTO_PIPELINE_DATA_DIR": str(seeded.data),
|
||||
"PHOTO_PIPELINE_LIBRARY_ROOTS": str(seeded.lib),
|
||||
}
|
||||
)
|
||||
run_migrations(config.database_url)
|
||||
engine = create_db_engine(config.database_url)
|
||||
sf = create_session_factory(engine)
|
||||
InventoryService(sf).scan(seeded.lib)
|
||||
with sf() as session:
|
||||
rows = list(session.execute(select(Asset.id, Asset.current_path)).all())
|
||||
for asset_id, path in rows:
|
||||
session.add(
|
||||
SafetyReview(
|
||||
id=str(uuid.uuid4()), asset_id=asset_id, decision="sfw", created_at=NOW
|
||||
)
|
||||
)
|
||||
session.add(
|
||||
AnalysisResult(
|
||||
asset_id=asset_id,
|
||||
status="analyzed",
|
||||
description=f"a view of {path}",
|
||||
tags='["ruins", "city"]',
|
||||
approx_year=2019,
|
||||
location_hint="Rome",
|
||||
)
|
||||
)
|
||||
session.commit()
|
||||
engine.dispose()
|
||||
seeded.asset_ids.update({p.rsplit("/", 1)[-1]: a for a, p in rows})
|
||||
return seeded
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def server(tmp_path):
|
||||
seeded = _seed(tmp_path)
|
||||
running = Server(seeded).start()
|
||||
running.seeded = seeded
|
||||
try:
|
||||
yield running
|
||||
finally:
|
||||
running.stop()
|
||||
|
||||
|
||||
def _generate(base):
|
||||
httpx.post(f"{base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT).raise_for_status()
|
||||
|
||||
|
||||
def test_view_shows_album_evidence_rationale_and_affected_count(page, server):
|
||||
errors = []
|
||||
page.on("console", lambda m: errors.append(m.text) if m.type == "error" else None)
|
||||
_generate(server.base)
|
||||
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
page.get_by_test_id("album-row").first.wait_for()
|
||||
|
||||
expect(page.get_by_test_id("source-album")).to_have_text("rome")
|
||||
expect(page.get_by_test_id("affected-count")).to_have_text("2")
|
||||
# Suggested name, rationale and confidence are all visible before approving.
|
||||
assert "2019" in page.get_by_test_id("suggested-name").inner_text()
|
||||
assert page.get_by_test_id("rationale").inner_text().strip() != ""
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("proposed")
|
||||
assert errors == [], f"console errors: {errors}"
|
||||
|
||||
|
||||
def test_album_without_a_proposal_is_explained(page, server):
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
expect(page.get_by_test_id("no-proposal")).to_be_visible()
|
||||
|
||||
|
||||
def test_edit_persists_across_reload(page, server):
|
||||
_generate(server.base)
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
|
||||
field = page.get_by_test_id("final-name")
|
||||
field.wait_for()
|
||||
field.fill("2019 Rome Holiday")
|
||||
page.get_by_test_id("save-name").click()
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("edited")
|
||||
|
||||
page.reload()
|
||||
expect(page.get_by_test_id("final-name")).to_have_value("2019 Rome Holiday")
|
||||
|
||||
|
||||
def test_invalid_name_is_flagged_promptly_while_typing(page, server):
|
||||
_generate(server.base)
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
|
||||
field = page.get_by_test_id("final-name")
|
||||
field.wait_for()
|
||||
field.fill("2019/Rome")
|
||||
# Validation is immediate — no request needed to learn the name is unusable.
|
||||
issues = page.get_by_test_id("name-issues")
|
||||
expect(issues).to_be_visible()
|
||||
assert "cannot contain" in issues.inner_text()
|
||||
|
||||
field.fill("2019 Rome")
|
||||
expect(issues).to_be_hidden()
|
||||
|
||||
|
||||
def test_approval_is_explicit_and_changes_no_file_paths(page, server):
|
||||
_generate(server.base)
|
||||
before = {
|
||||
row["current_path"]
|
||||
for row in httpx.get(f"{server.base}/api/v1/inventory/assets", timeout=TIMEOUT).json()[
|
||||
"items"
|
||||
]
|
||||
}
|
||||
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
approve = page.get_by_test_id("approve")
|
||||
approve.wait_for()
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("proposed")
|
||||
|
||||
approve.click() # approval only happens on this explicit action
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("approved")
|
||||
|
||||
after = {
|
||||
row["current_path"]
|
||||
for row in httpx.get(f"{server.base}/api/v1/inventory/assets", timeout=TIMEOUT).json()[
|
||||
"items"
|
||||
]
|
||||
}
|
||||
assert after == before, "approving a proposal must not rename anything"
|
||||
|
||||
page.reload()
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("approved")
|
||||
|
||||
|
||||
def test_stale_evidence_blocks_approval_visibly(page, server):
|
||||
_generate(server.base)
|
||||
# Change the album's evidence behind the UI's back: a new analysed photo.
|
||||
image(server.seeded.lib / "rome" / "c.jpg", 3)
|
||||
httpx.post(f"{server.base}/api/v1/inventory/scan", timeout=TIMEOUT).raise_for_status()
|
||||
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
page.get_by_test_id("album-row").first.wait_for()
|
||||
expect(page.get_by_test_id("stale")).to_be_visible()
|
||||
expect(page.get_by_test_id("approve")).to_be_disabled()
|
||||
|
||||
|
||||
def test_version_conflict_is_reported_without_mutating(page, server):
|
||||
_generate(server.base)
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
field = page.get_by_test_id("final-name")
|
||||
field.wait_for()
|
||||
|
||||
# Another client edits first, so the page's expected_version goes stale.
|
||||
current = httpx.get(f"{server.base}/api/v1/albums/proposals/rome", timeout=TIMEOUT).json()
|
||||
httpx.post(
|
||||
f"{server.base}/api/v1/albums/proposals/rome/edit",
|
||||
json={"name": "Someone Else", "expected_version": current["version"]},
|
||||
timeout=TIMEOUT,
|
||||
).raise_for_status()
|
||||
|
||||
field.fill("My Name")
|
||||
page.get_by_test_id("save-name").click()
|
||||
|
||||
expect(page.get_by_test_id("conflict")).to_be_visible()
|
||||
# The other client's value survived; this page's edit was not applied.
|
||||
expect(page.get_by_test_id("final-name")).to_have_value("Someone Else")
|
||||
|
||||
|
||||
def test_album_list_and_editing_work_from_the_keyboard(page, server):
|
||||
_generate(server.base)
|
||||
page.goto(f"{server.base}/app/#/albums")
|
||||
page.get_by_test_id("album-row").first.wait_for()
|
||||
|
||||
# Follow the album link with the keyboard — no mouse involved. Selecting it
|
||||
# re-renders, so wait for the selection to be reflected before typing;
|
||||
# otherwise focus lands on a node the re-render is about to replace.
|
||||
page.get_by_test_id("album-row").first.focus()
|
||||
page.keyboard.press("Enter")
|
||||
expect(page.get_by_test_id("album-row").first).to_have_attribute("aria-current", "true")
|
||||
expect(page.get_by_test_id("final-name")).to_be_visible()
|
||||
|
||||
field = page.get_by_test_id("final-name")
|
||||
field.focus()
|
||||
page.keyboard.press("ControlOrMeta+a")
|
||||
page.keyboard.type("Keyboard Named Album")
|
||||
page.keyboard.press("Tab")
|
||||
page.keyboard.press("Enter") # focus is now the Save button
|
||||
expect(page.get_by_test_id("proposal-status")).to_have_text("edited")
|
||||
expect(page.get_by_test_id("final-name")).to_have_value("Keyboard Named Album")
|
||||
@@ -78,6 +78,9 @@
|
||||
],
|
||||
"US03-03": [
|
||||
"tests/integration/test_album_proposals.py"
|
||||
],
|
||||
"US03-04": [
|
||||
"tests/e2e/test_albums_ui.py"
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user