fix(metadata): seed age/content rating into Fetch Metadata modal (bookshelf-ry66) #883

Merged

zombor merged 3 commits from bd-bookshelf-ry66 into main 2026-07-02 17:17:15 +00:00

Conversation 10 Commits 3 Files changed 5 +450 -4

zombor commented 2026-07-02 16:27:47 +00:00

Owner

Copy link

Summary

• Adds AgeRating *int32 and ContentRating string to CandidateResult DTO and populates them from the provider Metadata struct in metadataToCandidateResult.
• Extends metadata_fetch_controller.js to render compare rows for both fields (with numeric-safe displayValue handling for age_rating: 0 = All Ages, and age_rating sent as a JSON number in the save POST body).
• Metron maps rating.name → canonical AgeRating + ContentRating (unchanged); ComicVine provides neither (unchanged). The persist layer already wired these to book_metadata.age_rating/content_rating.

Root cause fixed

The candidate-to-modal seed step (metadataToCandidateResult at metadata_service.go:259) copied Rating but omitted m.AgeRating / m.ContentRating. All downstream layers (form parser, service, store, SQL) already handled them correctly.

Test plan

• Go: new black-box specs in metadata_candidates_test.go covering age rating (13, 0=All Ages), content rating, and nil/missing cases. 100% coverage maintained (make coverage passes).
• JS: 11 new Vitest specs in metadata_fetch_controller.test.js covering row rendering, All Ages edge case, copy button, and save POST body shape. All 3304 JS tests pass.
• No DB migrations required (existing columns).

Closes bead bookshelf-ry66 on merge.

## Summary - Adds `AgeRating *int32` and `ContentRating string` to `CandidateResult` DTO and populates them from the provider `Metadata` struct in `metadataToCandidateResult`. - Extends `metadata_fetch_controller.js` to render compare rows for both fields (with numeric-safe `displayValue` handling for `age_rating: 0` = All Ages, and `age_rating` sent as a JSON number in the save POST body). - Metron maps `rating.name` → canonical `AgeRating + ContentRating` (unchanged); ComicVine provides neither (unchanged). The persist layer already wired these to `book_metadata.age_rating/content_rating`. ## Root cause fixed The candidate-to-modal seed step (`metadataToCandidateResult` at metadata_service.go:259) copied `Rating` but omitted `m.AgeRating` / `m.ContentRating`. All downstream layers (form parser, service, store, SQL) already handled them correctly. ## Test plan - Go: new black-box specs in `metadata_candidates_test.go` covering age rating (13, 0=All Ages), content rating, and nil/missing cases. 100% coverage maintained (`make coverage` passes). - JS: 11 new Vitest specs in `metadata_fetch_controller.test.js` covering row rendering, All Ages edge case, copy button, and save POST body shape. All 3304 JS tests pass. - No DB migrations required (existing columns). Closes bead bookshelf-ry66 on merge.

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

zombor added 1 commit 2026-07-02 16:27:48 +00:00

fix(metadata): seed age/content rating from provider into Fetch Metadata modal (bookshelf-ry66) ... 87e9ea8eb5

CandidateResult now carries AgeRating (*int32) and ContentRating (string),
populated in metadataToCandidateResult from the provider Metadata struct.
Metron maps rating.name → canonical AgeRating + ContentRating via mapRating;
ComicVine provides neither (unchanged).

The fetch-metadata compare modal (metadata_fetch_controller.js) now renders
age_rating and content_rating compare rows alongside the other standard
fields. age_rating 0 ("All Ages") is treated as a valid non-empty value in
displayValue. age_rating is sent as a JSON number (added to the numeric field
list in _saveFromModal) so the server decodes it into SaveMetadataRequest.AgeRating
(*int32) correctly. content_rating is sent as a string.

Both fields map to existing persist paths (book_metadata.age_rating /
book_metadata.content_rating) which were already wired end-to-end; only the
candidate→modal seed step was missing.

Tests: Go black-box (FetchCandidates age/content rating mapping), JS Vitest
(compare row rendering, copy, save POST body). 100% coverage maintained.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-02 16:29:06 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-07-02 16:29:23 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

zombor commented 2026-07-02 16:36:09 +00:00

Author

Owner

Copy link

Security Review — PR #883 (bookshelf-ry66)

Scope: age/content rating carried into CandidateResult → compare modal → existing save path.

Checks performed:

• Input validation at boundary (age_rating / content_rating canonical sets)
• Book-ownership gate on the apply endpoint (user_id from session)
• XSS: rating values rendered via textContent vs innerHTML
• Per-user data path / multi-user scoping
• Architecture boundary (books package must not import workflow engine)
• Secrets / PII in logs
• CSP: no new inline style=

---

Findings

No security issues found in this diff.

Input validation — PASS. Both age_rating and content_rating are validated against their canonical sets in internal/books/metadata_service.go:374–387 (IsCanonicalAgeRating / IsCanonicalContentRating) before any write, for both the JSON path and the form path. The form path additionally range-checks the integer at metadata_handler.go:311–321 before the canonical gate fires. A client POSTing an arbitrary integer (e.g. 99) or an arbitrary string content rating is rejected with ErrValidation. The "stored-value pass-through" allowance (lines 375–376, 383–384) is loaded server-side and cannot be spoofed from the request body.

Ownership — PASS. SaveMetadataHandler calls checkBookAccess(r.Context(), userIDFromRequest(r), id) at line 197 before decoding the body. userIDFromRequest is session-derived. The existing gate is unchanged and covers this new field path.

XSS — PASS. Provider-supplied rating values flow through displayValue() → makeEl({text: …}) → node.textContent. The controller's line 9 states this policy explicitly. No new innerHTML sink is introduced in the diff. innerHTML = "" clears (line 126, 671, etc.) are pre-existing and carry eslint-disable no-unsanitized/property where needed.

Multi-user scoping — PASS. age_rating and content_rating are columns on book_metadata (book-scoped, not per-user). This PR introduces no per-user data path requiring user_id scoping.

Architecture boundary — PASS. internal/books/metadata_service.go imports only metadata, middleware, standard library. No workflow engine import.

Secrets / PII — PASS. Log lines in this diff emit only book_id and trace_id.

CSP / inline style= — PASS. No style= attributes added.

JS numeric coercion edge case — noted, not a finding. The JS save path uses Number(v) which accepts floats (e.g. "13.5"). If a user manually edits the field to a float, Go's json.Unmarshal into *int32 will return a decode error (fractional JSON numbers cannot be decoded into integer types), surfacing as a 400. Canonical validation at the service layer is the authoritative gate regardless.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #883 (bookshelf-ry66) **Scope:** age/content rating carried into `CandidateResult` → compare modal → existing save path. **Checks performed:** - Input validation at boundary (age_rating / content_rating canonical sets) - Book-ownership gate on the apply endpoint (user_id from session) - XSS: rating values rendered via textContent vs innerHTML - Per-user data path / multi-user scoping - Architecture boundary (books package must not import workflow engine) - Secrets / PII in logs - CSP: no new inline `style=` --- ### Findings No security issues found in this diff. **Input validation — PASS.** Both `age_rating` and `content_rating` are validated against their canonical sets in `internal/books/metadata_service.go:374–387` (`IsCanonicalAgeRating` / `IsCanonicalContentRating`) before any write, for both the JSON path and the form path. The form path additionally range-checks the integer at `metadata_handler.go:311–321` before the canonical gate fires. A client POSTing an arbitrary integer (e.g. 99) or an arbitrary string content rating is rejected with `ErrValidation`. The "stored-value pass-through" allowance (lines 375–376, 383–384) is loaded server-side and cannot be spoofed from the request body. **Ownership — PASS.** `SaveMetadataHandler` calls `checkBookAccess(r.Context(), userIDFromRequest(r), id)` at line 197 before decoding the body. `userIDFromRequest` is session-derived. The existing gate is unchanged and covers this new field path. **XSS — PASS.** Provider-supplied rating values flow through `displayValue()` → `makeEl({text: …})` → `node.textContent`. The controller's line 9 states this policy explicitly. No new `innerHTML` sink is introduced in the diff. `innerHTML = ""` clears (line 126, 671, etc.) are pre-existing and carry `eslint-disable no-unsanitized/property` where needed. **Multi-user scoping — PASS.** `age_rating` and `content_rating` are columns on `book_metadata` (book-scoped, not per-user). This PR introduces no per-user data path requiring `user_id` scoping. **Architecture boundary — PASS.** `internal/books/metadata_service.go` imports only `metadata`, `middleware`, standard library. No workflow engine import. **Secrets / PII — PASS.** Log lines in this diff emit only `book_id` and `trace_id`. **CSP / inline style= — PASS.** No `style=` attributes added. **JS numeric coercion edge case — noted, not a finding.** The JS save path uses `Number(v)` which accepts floats (e.g. `"13.5"`). If a user manually edits the field to a float, Go's `json.Unmarshal` into `*int32` will return a decode error (fractional JSON numbers cannot be decoded into integer types), surfacing as a 400. Canonical validation at the service layer is the authoritative gate regardless. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-07-02 16:41:54 +00:00

Author

Owner

Copy link

Code Review — PR #883 (bookshelf-ry66)

Diff reviewed: origin/main...origin/bd-bookshelf-ry66 (head 87e9ea8eb5)
Files: internal/books/dto.go, internal/books/metadata_service.go, internal/books/metadata_candidates_test.go, static/js/controllers/metadata_fetch_controller.js, static/js/test/metadata_fetch_controller.test.js

---

Phase 0: DEMO Verification

No DEMO block in the bead completion comment. CI is confirmed green via Forgejo commit-status API (state: success) and PR is mergeable. Per the review task the CI green state serves as functional verification; findings below are diff-based.

---

Phase 1: Spec Compliance

age_rating 0 (All Ages) on the Go side - PASS.

• CandidateResult.AgeRating *int32 with json tag age_rating,omitempty: Go JSON encoder omits pointer fields only when the pointer is nil, NOT when the pointed-to value is zero. A Metron-provided All Ages rating (AgeRating = &int32(0)) serialises to "age_rating": 0 in the candidate JSON. Correct.
• metadataToCandidateResult at metadata_service.go:275 copies m.AgeRating directly. The all-ages case passes through. Correct.
• Service validation at metadata_service.go:372-380: if req.AgeRating != nil is non-nil for &0 so the branch is entered. IsCanonicalAgeRating(0) returns true (ratings_test.go:26). Not rejected. Correct.
• metadataToUpsertParams at metadata_store.go:1197: both conditions true for &0. Sets sql.NullInt32{Int32: 0, Valid: true}. Persisted as 0, not NULL. Correct.

age_rating 0 on the JS side - PASS.

• displayValue("age_rating", {age_rating: 0}): new two-step guard (lines 60-62) returns "0" not "". !("0") is false in JS, so buildCompareRow does not skip the row. Compare row renders for All Ages. Correct.
• Copy action: _applyCopyToField(curEl, "0", btn) sets the modal text input to "0".
• Save path (line 851-856): v = "0", v !== "" is true, k === "age_rating" branch executes, Number("0") = 0, !isNaN(0) true, body.age_rating = 0. Sent as JSON number 0. Server decodes to *int32(0). Correct.

Locks - PASS. SQL upsert in metadata.sql.go:120 uses IF(COALESCE(age_rating_locked, 0) = 0 AND COALESCE(all_fields_locked, 0) = 0, VALUES(age_rating), age_rating). Lock enforcement at DB level, unchanged.

Canonical validation - PASS. Service rejects non-canonical age_rating values (metadata_service.go:374-379). Go json.Unmarshal into *int32 rejects floats at decode time. IsCanonicalContentRating gate at line 382 unmodified.

Serialisation field names - PASS. JS sends age_rating (number) and content_rating (string), matching SaveMetadataRequest fields.

Scope vs PR 882 (7mzg) - PASS. This PR adds to fieldLabel/_editorFieldID/displayValue/_saveFromModal. It does NOT touch buildComicCompareRow, _comicEditorFieldID, comicFieldLabel, or comicFieldOrder. Non-conflicting sections; clean rebase expected.

Bulk enrich path - PASS. No changes to persistInTxWithComic or wfengine bulk enrichment activities. Bulk path writes AgeRating/ContentRating via metadataToUpsertParams unchanged.

Black-box tests - PASS. metadata_candidates_test.go declares package books_test. No unexported symbols referenced.

CSP / inline style= - PASS. No new style= attributes in the diff.

---

Phase 2: Code Quality Findings

[MINOR] static/js/test/metadata_fetch_controller.test.js — missing save-path test for age_rating=0
The JS test suite tests copy-and-save with age_rating 13 but never exercises the All Ages path (age_rating 0) through to the POST body. The code is correct (v !== "" passes for "0", Number("0") = 0), and coverage is maintained because the same branch handles both values. But age_rating 0 is the critical case motivating this fix; a future regression that special-cases 0 in the save path would go undetected. Suggested addition: open modal with METRON_CANDIDATE_ALL_AGES, copy age_rating, save, assert capturedBody.age_rating === 0 (number, not string, not absent).

[MINOR] static/js/controllers/metadata_fetch_controller.js:308-315 — compare row shows raw "0" not "All Ages" in fetched column
displayValue("age_rating", {age_rating: 0}) returns "0". The modal fetched column shows 0 while the main form select shows the label "All Ages". Not a correctness bug — the value copies and saves correctly — but potentially confusing to users. Consider mapping numeric buckets to labels inside displayValue when key === "age_rating".

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — PR #883 (bookshelf-ry66) **Diff reviewed:** origin/main...origin/bd-bookshelf-ry66 (head 87e9ea8eb5b3) **Files:** internal/books/dto.go, internal/books/metadata_service.go, internal/books/metadata_candidates_test.go, static/js/controllers/metadata_fetch_controller.js, static/js/test/metadata_fetch_controller.test.js --- ### Phase 0: DEMO Verification No DEMO block in the bead completion comment. CI is confirmed green via Forgejo commit-status API (state: success) and PR is mergeable. Per the review task the CI green state serves as functional verification; findings below are diff-based. --- ### Phase 1: Spec Compliance **age_rating 0 (All Ages) on the Go side - PASS.** - CandidateResult.AgeRating *int32 with json tag age_rating,omitempty: Go JSON encoder omits pointer fields only when the pointer is nil, NOT when the pointed-to value is zero. A Metron-provided All Ages rating (AgeRating = &int32(0)) serialises to "age_rating": 0 in the candidate JSON. Correct. - metadataToCandidateResult at metadata_service.go:275 copies m.AgeRating directly. The all-ages case passes through. Correct. - Service validation at metadata_service.go:372-380: if req.AgeRating != nil is non-nil for &0 so the branch is entered. IsCanonicalAgeRating(0) returns true (ratings_test.go:26). Not rejected. Correct. - metadataToUpsertParams at metadata_store.go:1197: both conditions true for &0. Sets sql.NullInt32{Int32: 0, Valid: true}. Persisted as 0, not NULL. Correct. **age_rating 0 on the JS side - PASS.** - displayValue("age_rating", {age_rating: 0}): new two-step guard (lines 60-62) returns "0" not "". !("0") is false in JS, so buildCompareRow does not skip the row. Compare row renders for All Ages. Correct. - Copy action: _applyCopyToField(curEl, "0", btn) sets the modal text input to "0". - Save path (line 851-856): v = "0", v !== "" is true, k === "age_rating" branch executes, Number("0") = 0, !isNaN(0) true, body.age_rating = 0. Sent as JSON number 0. Server decodes to *int32(0). Correct. **Locks - PASS.** SQL upsert in metadata.sql.go:120 uses IF(COALESCE(age_rating_locked, 0) = 0 AND COALESCE(all_fields_locked, 0) = 0, VALUES(age_rating), age_rating). Lock enforcement at DB level, unchanged. **Canonical validation - PASS.** Service rejects non-canonical age_rating values (metadata_service.go:374-379). Go json.Unmarshal into *int32 rejects floats at decode time. IsCanonicalContentRating gate at line 382 unmodified. **Serialisation field names - PASS.** JS sends age_rating (number) and content_rating (string), matching SaveMetadataRequest fields. **Scope vs PR 882 (7mzg) - PASS.** This PR adds to fieldLabel/_editorFieldID/displayValue/_saveFromModal. It does NOT touch buildComicCompareRow, _comicEditorFieldID, comicFieldLabel, or comicFieldOrder. Non-conflicting sections; clean rebase expected. **Bulk enrich path - PASS.** No changes to persistInTxWithComic or wfengine bulk enrichment activities. Bulk path writes AgeRating/ContentRating via metadataToUpsertParams unchanged. **Black-box tests - PASS.** metadata_candidates_test.go declares package books_test. No unexported symbols referenced. **CSP / inline style= - PASS.** No new style= attributes in the diff. --- ### Phase 2: Code Quality Findings [MINOR] static/js/test/metadata_fetch_controller.test.js — missing save-path test for age_rating=0 The JS test suite tests copy-and-save with age_rating 13 but never exercises the All Ages path (age_rating 0) through to the POST body. The code is correct (v !== "" passes for "0", Number("0") = 0), and coverage is maintained because the same branch handles both values. But age_rating 0 is the critical case motivating this fix; a future regression that special-cases 0 in the save path would go undetected. Suggested addition: open modal with METRON_CANDIDATE_ALL_AGES, copy age_rating, save, assert capturedBody.age_rating === 0 (number, not string, not absent). [MINOR] static/js/controllers/metadata_fetch_controller.js:308-315 — compare row shows raw "0" not "All Ages" in fetched column displayValue("age_rating", {age_rating: 0}) returns "0". The modal fetched column shows 0 while the main form select shows the label "All Ages". Not a correctness bug — the value copies and saves correctly — but potentially confusing to users. Consider mapping numeric buckets to labels inside displayValue when key === "age_rating". --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor referenced this pull request from a commit 2026-07-02 16:53:11 +00:00

fix(metadata-fetch): map age_rating bucket to label in modal; add All Ages save-path test

zombor added 1 commit 2026-07-02 16:53:11 +00:00

fix(metadata-fetch): map age_rating bucket to label in modal; add All Ages save-path test ... 8aa4cbea34

Minor fixes from code review of PR #883 (bookshelf-ry66):

1. displayValue() now returns canonical age_rating labels ("13+", "All Ages")
   instead of raw numeric strings. Mirrors books.AgeRatingOptions via inline
   ageRatingLabels map with a comment pointing to the Go source.

2. _saveFromModal() handles the label-string case for age_rating: tries
   Number(v) first (handles pre-filled "0"/"13"), then reverse-looks up via
   ageRatingLabels to resolve "All Ages"→0, "13+"→13, etc.

3. Updated three existing tests to assert on label strings ("13+", "All Ages")
   instead of raw numbers.

4. Added new test: saving after copying age_rating=0 (All Ages) sends
   age_rating as 0 (JSON number) in the POST body — guards the critical
   0-is-a-real-value path all the way through to the POST.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-02 16:55:50 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-02 16:56:02 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor added 1 commit 2026-07-02 17:01:58 +00:00

test(metadata-fetch): add numeric-string save-path test for age_rating coverage ... e4e565804a

The previous commit's _saveFromModal age_rating branch has two sub-paths:
1. Numeric string (Number(v) succeeds) — e.g. pre-filled "13"
2. Label reverse-lookup (Number(v) = NaN) — e.g. copied "13+" / "All Ages"

Both copy-then-save tests now go through the label path (since displayValue
returns "13+" / "All Ages"), leaving the numeric-string branch uncovered.

Add a test that sets the age_rating input directly to "13" (simulating
pre-fill or manual entry) and saves, asserting body.age_rating === 13.
This restores 100% JS branch coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-02 17:04:35 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-07-02 17:04:55 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

zombor referenced this pull request from a commit 2026-07-02 17:10:41 +00:00

fix(metadata-fetch): map age_rating bucket to label in modal; add All Ages save-path test

zombor force-pushed bd-bookshelf-ry66 from e4e565804a to da3ad9fb42 2026-07-02 17:10:41 +00:00 Compare

forgejo-actions commented 2026-07-02 17:13:23 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-02 17:13:31 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor merged commit 49fa2ee86d into main 2026-07-02 17:17:15 +00:00

zombor referenced this pull request from a commit 2026-07-02 17:17:17 +00:00

Merge pull request 'fix(metadata): seed age/content rating into Fetch Metadata modal (bookshelf-ry66)' (#883) from bd-bookshelf-ry66 into main

zombor referenced this pull request 2026-07-03 02:02:35 +00:00

fix(e2e-browser): per-op deadline reset kills shared-budget flake (bookshelf-rpr4) #892

zombor referenced this pull request 2026-07-03 18:15:31 +00:00

fix(comics): lazy detail-fetch for bulk enrich (bookshelf-bkzy) #906

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!883

No description provided.