fix(slice5): validate raw metadata input + prune stale bulk selections
Code-review follow-ups: - Validate the raw submitted value before apply_values coerces it, in both the single-edit and bulk endpoints — a scalar set onto a tags field now rejects (422 / rejected) instead of silently char-splitting into a list. - Catalog prunes bulk selections to the currently-visible (filtered) entries after each list fetch, so the bulk bar can't act on rows the user has filtered out of view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -89,11 +89,13 @@ def make_router(config: Config, gitea: Gitea, bot: Bot) -> APIRouter:
|
||||
if st is None:
|
||||
raise HTTPException(404, f"{md_path} not found")
|
||||
|
||||
new_entry = metadata_mod.apply_values(st.entry, body.values)
|
||||
problems = metadata_schema.validate(
|
||||
metadata_mod.metadata_dict(new_entry), fields)
|
||||
# Validate the *raw* submitted values (INV-4): catch a type mismatch
|
||||
# before `apply_values` coerces it — e.g. a scalar handed to a `tags`
|
||||
# field would otherwise char-split into a valid-looking list.
|
||||
problems = metadata_schema.validate(body.values, fields)
|
||||
if problems:
|
||||
raise HTTPException(422, {"problems": [p.as_dict() for p in problems]})
|
||||
new_entry = metadata_mod.apply_values(st.entry, body.values)
|
||||
|
||||
files = metadata_mod.write_entry_files(md_path, new_entry, st)
|
||||
try:
|
||||
@@ -153,14 +155,15 @@ def make_router(config: Config, gitea: Gitea, bot: Bot) -> APIRouter:
|
||||
rejected.append({"slug": slug, "reason": "not found"})
|
||||
continue
|
||||
new_value = _apply_op(st.entry, body.op, body.field, body.value)
|
||||
new_entry = metadata_mod.apply_values(st.entry, {body.field: new_value})
|
||||
problems = metadata_schema.validate(
|
||||
metadata_mod.metadata_dict(new_entry), fields)
|
||||
# Validate the *raw* new value before coercion (see edit_meta) so a
|
||||
# scalar `set` onto a tags field is rejected, not char-split.
|
||||
problems = metadata_schema.validate({body.field: new_value}, fields)
|
||||
if problems:
|
||||
rejected.append({"slug": slug,
|
||||
"reason": "; ".join(p.message for p in problems)})
|
||||
continue
|
||||
applied.append(slug)
|
||||
new_entry = metadata_mod.apply_values(st.entry, {body.field: new_value})
|
||||
if metadata_mod.metadata_dict(new_entry) != metadata_mod.metadata_dict(st.entry):
|
||||
all_ops.extend(metadata_mod.write_entry_files(md_path, new_entry, st))
|
||||
|
||||
|
||||
@@ -119,6 +119,39 @@ def test_bulk_remove_tag(app_with_fake_gitea):
|
||||
assert _sidecar(fake, "a")["tags"] == ["y"]
|
||||
|
||||
|
||||
def test_bulk_set_scalar_on_tags_rejected(app_with_fake_gitea):
|
||||
# A scalar `set` onto a tags field must reject, not char-split into a list.
|
||||
app, fake = app_with_fake_gitea
|
||||
with TestClient(app) as client:
|
||||
_set_fields({"tags": {"type": "tags"}})
|
||||
_seed_legacy(fake, "a", tags=["x"])
|
||||
_refresh()
|
||||
_login_owner(client)
|
||||
r = client.post(f"{BASE}/meta/bulk",
|
||||
json={"slugs": ["a"], "op": "set",
|
||||
"field": "tags", "value": "checkout"})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["applied"] == []
|
||||
assert len(r.json()["rejected"]) == 1
|
||||
assert r.json()["committed"] is False
|
||||
assert not _has_sidecar(fake, "a")
|
||||
|
||||
|
||||
def test_bulk_set_list_on_tags_ok(app_with_fake_gitea):
|
||||
app, fake = app_with_fake_gitea
|
||||
with TestClient(app) as client:
|
||||
_set_fields({"tags": {"type": "tags"}})
|
||||
_seed_legacy(fake, "a", tags=["x"])
|
||||
_refresh()
|
||||
_login_owner(client)
|
||||
r = client.post(f"{BASE}/meta/bulk",
|
||||
json={"slugs": ["a"], "op": "set",
|
||||
"field": "tags", "value": ["x", "y"]})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["applied"] == ["a"]
|
||||
assert _sidecar(fake, "a")["tags"] == ["x", "y"]
|
||||
|
||||
|
||||
def test_bulk_add_remove_requires_tags_field(app_with_fake_gitea):
|
||||
app, fake = app_with_fake_gitea
|
||||
with TestClient(app) as client:
|
||||
|
||||
@@ -93,7 +93,16 @@ export default function Catalog({ viewer, onProposeRFC, version }) {
|
||||
Object.entries(selections).map(([f, set]) => [f, [...set]])
|
||||
)
|
||||
listRFCs(pid, cid, { selections: selObj, malformed: malformedOnly })
|
||||
.then(d => { setRfcs(d.items); setFacets(d.facets || {}) })
|
||||
.then(d => {
|
||||
setRfcs(d.items); setFacets(d.facets || {})
|
||||
// §22.4a SLICE-5: drop any bulk selections that the new (filtered)
|
||||
// list no longer contains, so the bar can't act on hidden rows.
|
||||
const visible = new Set(d.items.map(it => it.slug))
|
||||
setSelected(prev => {
|
||||
const kept = [...prev].filter(s => visible.has(s))
|
||||
return kept.length === prev.size ? prev : new Set(kept)
|
||||
})
|
||||
})
|
||||
.catch(() => { setRfcs([]); setFacets({}) })
|
||||
.finally(() => setLoading(false))
|
||||
}, [version, pid, cid, selections, malformedOnly])
|
||||
|
||||
@@ -90,6 +90,30 @@ describe('Catalog faceted pane', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('drops a selection when a filter change hides the row', async () => {
|
||||
getCollection.mockResolvedValue({
|
||||
fields: { priority: { type: 'enum', values: ['P0', 'P1'] } },
|
||||
viewer: { can_contribute: true }, entry_noun: 'RFC',
|
||||
})
|
||||
// Content-driven: unfiltered → row "a"; once the P0 facet is selected the
|
||||
// server response omits "a" (drives the prune). Robust to extra mount-time
|
||||
// fetches the collection effect triggers.
|
||||
listRFCs.mockImplementation((p, c, opts) => Promise.resolve({
|
||||
items: opts?.selections?.priority?.includes('P0')
|
||||
? [{ slug: 'b', title: 'B', state: 'active', tags: [], starred_by_me: false }]
|
||||
: [{ slug: 'a', title: 'A', state: 'active', tags: [], starred_by_me: false }],
|
||||
facets: { priority: { P0: 1 }, state: { active: 1 } },
|
||||
}))
|
||||
const { container } = renderCatalog()
|
||||
fireEvent.click(await screen.findByLabelText(/select A/i))
|
||||
expect(await screen.findByText(/1 selected/i)).toBeInTheDocument()
|
||||
// Toggle the P0 facet checkbox → re-fetch returns a list without "a".
|
||||
fireEvent.click(container.querySelector('.facet-group input[type="checkbox"]'))
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByText(/1 selected/i)).not.toBeInTheDocument()
|
||||
})
|
||||
})
|
||||
|
||||
it('does not offer row selection for a non-contributor', async () => {
|
||||
getCollection.mockResolvedValue({
|
||||
fields: { priority: { type: 'enum', values: ['P0'] } },
|
||||
|
||||
Reference in New Issue
Block a user