fix(comicvine): map Publisher and Editors from API responses (bookshelf-ic7s) #975

Merged

zombor merged 1 commit from bd-bookshelf-ic7s into main 2026-07-06 13:16:00 +00:00

Conversation 2 Commits 1 Files changed 2 +368 -32

zombor commented 2026-07-06 12:43:48 +00:00

Owner

Copy link

Summary

Two ComicVine mapping gaps fixed in internal/metadata/comicvine/mapping.go:

G1 — Publisher always empty in manual fetch modal
applyVolumeInfo set m.Comic.VolumeYear + m.Comic.VolumeID but not m.Publisher, even though the volume search already requests and decodes publisher. Now sets m.Publisher = vol.Publisher.Name when non-empty and not already set upstream. Fixes the empty Publisher row in the manual fetch modal for both the free-text search path and the NewFetchIssueDetail path.

G3 — Editor credits silently dropped
Both buildComicMetadataFromList and buildComicMetadata had role-dispatch switches with no editor case, so ComicVine Editor/editor-in-chief credits were dropped entirely (ComicMetadata.Editors always nil). Added case strings.Contains(role, "editor") to both.

Refactor: extracted applyPersonCredit helper shared by both functions — the new editor case pushed both functions past the CC-15 gocyclo gate. Extracting the switch brings them back under the gate and eliminates the duplication.

Test plan

• make test — all tests pass including new mapping_test.go specs
• make lint — gocyclo, staticcheck clean
• make coverage — 100% on changed functions (applyVolumeInfo, applyPersonCredit, buildComicMetadataFromList, buildComicMetadata)
• New black-box tests via httptest.Server: publisher via free-text and NewFetchIssueDetail paths; editors via both list and detail paths
• No new .golangci.yml exclusions added

Closes bead bookshelf-ic7s on merge.

## Summary Two ComicVine mapping gaps fixed in `internal/metadata/comicvine/mapping.go`: **G1 — Publisher always empty in manual fetch modal** `applyVolumeInfo` set `m.Comic.VolumeYear` + `m.Comic.VolumeID` but not `m.Publisher`, even though the volume search already requests and decodes `publisher`. Now sets `m.Publisher = vol.Publisher.Name` when non-empty and not already set upstream. Fixes the empty Publisher row in the manual fetch modal for both the free-text search path and the `NewFetchIssueDetail` path. **G3 — Editor credits silently dropped** Both `buildComicMetadataFromList` and `buildComicMetadata` had role-dispatch switches with no `editor` case, so ComicVine Editor/editor-in-chief credits were dropped entirely (`ComicMetadata.Editors` always nil). Added `case strings.Contains(role, "editor")` to both. **Refactor**: extracted `applyPersonCredit` helper shared by both functions — the new `editor` case pushed both functions past the CC-15 gocyclo gate. Extracting the switch brings them back under the gate and eliminates the duplication. ## Test plan - [x] `make test` — all tests pass including new `mapping_test.go` specs - [x] `make lint` — gocyclo, staticcheck clean - [x] `make coverage` — 100% on changed functions (`applyVolumeInfo`, `applyPersonCredit`, `buildComicMetadataFromList`, `buildComicMetadata`) - [x] New black-box tests via `httptest.Server`: publisher via free-text and `NewFetchIssueDetail` paths; editors via both list and detail paths - [x] No new `.golangci.yml` exclusions added Closes bead bookshelf-ic7s on merge.

zombor added 1 commit 2026-07-06 12:43:48 +00:00

fix(comicvine): map Publisher and Editors from ComicVine API responses (bookshelf-ic7s) ... 7b74f0454a

G1 Publisher: applyVolumeInfo now sets m.Publisher from vol.Publisher.Name
when the volume carries a non-empty publisher and m.Publisher is not already
set. Both the free-text search path (via applyFreeTextVolumeYears) and the
NewFetchIssueDetail path (which calls applyVolumeInfo after the volume lookup)
now populate Metadata.Publisher, fixing the empty Publisher row in the manual
fetch modal.

G3 Editors: extract applyPersonCredit helper from buildComicMetadataFromList
and buildComicMetadata (both had duplicate role-dispatch switches). The new
helper adds an 'editor' case (strings.Contains(role, "editor")) so ComicVine
Editor credits land in ComicMetadata.Editors instead of being silently dropped.
Extracting the helper also brings both callers back under the gocyclo CC-15 gate
that adding the new case pushed them over.

Tests: black-box coverage in mapping_test.go via httptest.Server — publisher via
free-text and NewFetchIssueDetail paths; editors via both list and detail paths.

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

zombor commented 2026-07-06 12:52:16 +00:00

Author

Owner

Copy link

Security Review — bookshelf-ic7s (PR #975)

ComicVine mapping fix: applyVolumeInfo publisher propagation + applyPersonCredit editor-role dispatch.

---

[MINOR] internal/metadata/comicvine/mapping_test.go:209,271 — misleading NotTo(BeNil()) assertion on a struct value
The two It("fetches without error") blocks in the NewFetchIssueDetail describe groups assert Expect(result, err).NotTo(BeNil()) where result is of type metadata.Metadata — a struct value, never nil. The NotTo(BeNil()) half always passes unconditionally; only the multi-actual err-must-be-nil gate does real work. This makes the intent misleading and masks the fact that the value assertion is absent from these specific It blocks (the field-specific It blocks carry the real check). The fix is to either remove the standalone no-error It and fold the nil-error constraint into the field-assertion It via Expect(result.Publisher, err).To(Equal("DC Comics")), or use Expect(err).NotTo(HaveOccurred()) explicitly. No security or correctness impact — the field assertions in the sibling It blocks do verify the actual values.

---

All other checklist items: clean.

• Injection: vol.Publisher.Name and p.Name/p.Role are provider strings flowing into metadata.Metadata struct fields, ultimately rendered by html/template which auto-escapes. No SQL path. No injection concern.
• Nil/panic safety: volumePublisher is embedded by value (not a pointer) in volumeSearchResult, so vol.Publisher.Name is always a valid string access with zero value "". The vol.Publisher.Name != "" guard is correct. applyPersonCredit receives a non-nil *metadata.ComicMetadata (callers pass &c of a locally declared value). No panic path introduced.
• Secret/PII logging: No logging anywhere in the changed code.
• Architecture boundary: mapping.go imports only strconv, strings, and internal/metadata. No workflow-engine import. Boundary clean.
• Multi-user scoping: Pure provider-response-to-domain-struct mapping; no DB access, no user data. N/A.
• Test package: mapping_test.go declares package comicvine_test. Black-box convention satisfied.
• .golangci.yml: No new exclusions introduced.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — bookshelf-ic7s (PR #975) ComicVine mapping fix: `applyVolumeInfo` publisher propagation + `applyPersonCredit` editor-role dispatch. --- [MINOR] internal/metadata/comicvine/mapping_test.go:209,271 — misleading `NotTo(BeNil())` assertion on a struct value The two `It("fetches without error")` blocks in the `NewFetchIssueDetail` describe groups assert `Expect(result, err).NotTo(BeNil())` where `result` is of type `metadata.Metadata` — a struct value, never nil. The `NotTo(BeNil())` half always passes unconditionally; only the multi-actual `err`-must-be-nil gate does real work. This makes the intent misleading and masks the fact that the value assertion is absent from these specific `It` blocks (the field-specific `It` blocks carry the real check). The fix is to either remove the standalone no-error `It` and fold the nil-error constraint into the field-assertion `It` via `Expect(result.Publisher, err).To(Equal("DC Comics"))`, or use `Expect(err).NotTo(HaveOccurred())` explicitly. No security or correctness impact — the field assertions in the sibling `It` blocks do verify the actual values. --- **All other checklist items: clean.** - Injection: `vol.Publisher.Name` and `p.Name`/`p.Role` are provider strings flowing into `metadata.Metadata` struct fields, ultimately rendered by `html/template` which auto-escapes. No SQL path. No injection concern. - Nil/panic safety: `volumePublisher` is embedded by value (not a pointer) in `volumeSearchResult`, so `vol.Publisher.Name` is always a valid string access with zero value `""`. The `vol.Publisher.Name != ""` guard is correct. `applyPersonCredit` receives a non-nil `*metadata.ComicMetadata` (callers pass `&c` of a locally declared value). No panic path introduced. - Secret/PII logging: No logging anywhere in the changed code. - Architecture boundary: `mapping.go` imports only `strconv`, `strings`, and `internal/metadata`. No workflow-engine import. Boundary clean. - Multi-user scoping: Pure provider-response-to-domain-struct mapping; no DB access, no user data. N/A. - Test package: `mapping_test.go` declares `package comicvine_test`. Black-box convention satisfied. - `.golangci.yml`: No new exclusions introduced. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-07-06 12:55:42 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0 — DEMO Verification

No terminal-command DEMO block exists in the bead or comments. The acceptance note ("Justice League #6 → DC Comics in the modal") requires a live system with a real ComicVine API key. CI is green and the orchestrator indicated CI is the behavioral truth source for this review.

Phase 1 — Spec Compliance

All requirements from the bead description are met:

• G1 Publisher: applyVolumeInfo now sets m.Publisher = vol.Publisher.Name guarded by vol.Publisher.Name != "" && m.Publisher == ""
• Publisher is book-level (m.Publisher, not m.Comic.Publisher) ✓
• Both search path (search.go:121, 186, 625) and detail path (provider.go:256) call applyVolumeInfo, so both now pick up Publisher ✓
• G3 Editors: extracted applyPersonCredit helper (mapping.go:84) with case strings.Contains(role, "editor") dispatching to c.Editors ✓
• Refactor is behavior-preserving for all pre-existing roles (writer/penciller/penciler/inker/colorist/colourist/letterer/cover) — none dropped or mis-routed ✓
• No new .golangci.yml exclusions (zero net diff to both files) ✓

Phase 2 — Code Quality

[MINOR] internal/metadata/comicvine/mapping_test.go — missing publisher-no-clobber assertion
The review spec explicitly required a test that verifies a pre-set m.Publisher is NOT overwritten when applyVolumeInfo is called with a non-empty vol.Publisher.Name. The current tests exercise the true branch (publisher populated) and the vol.Publisher.Name == "" false branch (publisher stays empty), but no test exercises the case where m.Publisher is already non-empty. Go statement coverage still reaches 100% because the if line itself is hit, but the no-clobber guard’s false-path is untested. Suggested fix: add a Context("publisher already set — not clobbered") case in the G1 free-text Describe where m.Publisher is pre-populated (e.g. by having the issue detail already carry a publisher) and assert it remains unchanged after applyVolumeInfo merges the volume.

All other checks pass:

• applyPersonCredit takes *metadata.ComicMetadata by pointer; callers correctly pass &c ✓
• Switch case order: "cover" precedes "editor" — a role string like "cover, editor" would match "cover" only, but this is identical to the pre-existing behavior for other role combinations and is not a new issue introduced by this PR ✓
• package comicvine_test throughout — no white-box package declaration in the new file ✓
• One-Expect-per-It throughout; multi-actual form Expect(results, err).To(HaveLen(1)) correctly folds the nil-error check into the value assertion per convention ✓
• No new scripts/check-coverage.sh exclusions ✓

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## CODE REVIEW: APPROVED ### Phase 0 — DEMO Verification No terminal-command DEMO block exists in the bead or comments. The acceptance note ("Justice League #6 → DC Comics in the modal") requires a live system with a real ComicVine API key. CI is green and the orchestrator indicated CI is the behavioral truth source for this review. ### Phase 1 — Spec Compliance All requirements from the bead description are met: - G1 Publisher: `applyVolumeInfo` now sets `m.Publisher = vol.Publisher.Name` guarded by `vol.Publisher.Name != "" && m.Publisher == ""` - Publisher is book-level (`m.Publisher`, not `m.Comic.Publisher`) ✓ - Both search path (`search.go:121, 186, 625`) and detail path (`provider.go:256`) call `applyVolumeInfo`, so both now pick up Publisher ✓ - G3 Editors: extracted `applyPersonCredit` helper (`mapping.go:84`) with `case strings.Contains(role, "editor")` dispatching to `c.Editors` ✓ - Refactor is behavior-preserving for all pre-existing roles (writer/penciller/penciler/inker/colorist/colourist/letterer/cover) — none dropped or mis-routed ✓ - No new `.golangci.yml` exclusions (zero net diff to both files) ✓ ### Phase 2 — Code Quality [MINOR] `internal/metadata/comicvine/mapping_test.go` — missing publisher-no-clobber assertion The review spec explicitly required a test that verifies a pre-set `m.Publisher` is NOT overwritten when `applyVolumeInfo` is called with a non-empty `vol.Publisher.Name`. The current tests exercise the `true` branch (publisher populated) and the `vol.Publisher.Name == ""` false branch (publisher stays empty), but no test exercises the case where `m.Publisher` is already non-empty. Go statement coverage still reaches 100% because the `if` line itself is hit, but the no-clobber guard’s false-path is untested. Suggested fix: add a `Context("publisher already set — not clobbered")` case in the G1 free-text Describe where `m.Publisher` is pre-populated (e.g. by having the issue detail already carry a publisher) and assert it remains unchanged after `applyVolumeInfo` merges the volume. All other checks pass: - `applyPersonCredit` takes `*metadata.ComicMetadata` by pointer; callers correctly pass `&c` ✓ - Switch case order: "cover" precedes "editor" — a role string like "cover, editor" would match "cover" only, but this is identical to the pre-existing behavior for other role combinations and is not a new issue introduced by this PR ✓ - `package comicvine_test` throughout — no white-box package declaration in the new file ✓ - One-Expect-per-It throughout; multi-actual form `Expect(results, err).To(HaveLen(1))` correctly folds the nil-error check into the value assertion per convention ✓ - No new `scripts/check-coverage.sh` exclusions ✓ --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-ic7s from 7b74f0454a to ae26c4ce7e 2026-07-06 13:03:08 +00:00 Compare

zombor merged commit 381892fd33 into main 2026-07-06 13:16:00 +00:00

zombor referenced this pull request from a commit 2026-07-06 13:16:03 +00:00

Merge pull request 'fix(comicvine): map Publisher and Editors from API responses (bookshelf-ic7s)' (#975) from bd-bookshelf-ic7s into main

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

1 participant

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!975

No description provided.