feat: remove obsolete dead-provider metadata UI (bookshelf-o0m9) #779
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-o0m9"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
MatchWeightsstruct 24→19 fields;DefaultMatchWeightstotal 87→79;MatchScoreInputdrops 10 dead fieldsTest plan
make test— all unit tests passnpm run coverage— JS gate 100% maintainedCloses bead bookshelf-o0m9 on merge.
04_series_detail.png
01_book_detail.png
02_book_metadata_editor.png
03_settings_match_weights.png
Security Review — bookshelf-o0m9 (PR #779)
Scope: Removal of obsolete Goodreads/Amazon/Audible provider UI, match-weight fields, and score contribution fields.
Findings
[MINOR] internal/settings/match_weights_handler.go:34 — stale "24 weight fields" comment
The handler docstring still says "all 24 weight fields" after the removal of the 5 Goodreads/Amazon/Audible fields. The live
ErrIncompleteWeightsmessage inmatch_weights.gowas correctly updated to "all 19 weight fields", but this comment was missed. No security impact (the enforcement is indecodeMatchWeightsInput, not the comment), but a reviewer reading the handler may be confused about the actual acceptance criteria.Security-specific checks (all clean)
GET/PUT/DELETE /settings/match-weights,POST /settings/match-weights/recalculate) remain registered ininternal/settings/routes.goand are all wrapped withadminRequired. The removal did not touch routes.adminRequiredguard. TheSaveMatchWeightsHandler/ResetMatchWeightsHandler/RecalcMatchScoresHandlerwiring is unchanged..sqlfiles changed. Thegoodreads_rating,amazon_rating,audible_rating,goodreads_review_count,amazon_review_count,goodreads_rating_locked, etc. columns remain untouched onbook_metadata— the removal is read/display-layer only. Existing data is safe.internal/books/dto.gostill carriesGoodreadsID,AudibleID,AmazonASIN,GoodreadsRating,AmazonRating,AudibleRatingfields (these are the book-metadata edit form fields which were intentionally NOT removed in this PR — the template still renders them for the metadata editor). This is consistent; only the rating display and match-score contribution of those providers was removed.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
UI Review — PR #779 (bookshelf-o0m9)
Screenshot observations
01_book_detail.png — Book detail (Details tab): Provider ratings row shows HARDCOVER 4.70 / GOOGLE 4.50 / OPEN LIBRARY 4.20. No Goodreads, Amazon, or Audible badge present. Spacing and alignment of the ratings row look clean. No orphaned container. Looks correct.
02_book_metadata_editor.png — Edit Metadata tab: The "PROVIDER IDS & RATINGS" accordion section is rendered collapsed (not expanded). The main fields above it (title, authors, ISBNs, series, description, age/content rating) are all well-aligned using canonical metadata-field rows. However, because the accordion is collapsed the screenshot does NOT show whether the Goodreads Rating / Amazon Rating / Audible Rating input fields inside it were actually removed. The source confirms they were NOT removed (see finding below).
03_settings_match_weights.png — Named to verify "Settings -> Metadata" match-weights removal, but the screenshot shows the Email Settings tab, not the Metadata tab. The Metadata/match-weights panel is never rendered in this screenshot. This is a wrong-tab capture — the required rendered proof of the match-weights removal is absent from the PR screenshots.
04_series_show.png — Series detail: badges show "3 BOOKS", "0/3 READ", "4.7★ AVG. HARDCOVER" only. AvgGoodreads badge is gone. No orphaned container. Clean.
Findings
[MAJOR] templates/pages/books_show.html:1330-1387 — Goodreads/Amazon/Audible rating inputs not removed from the metadata editor (Edit Metadata -> Provider IDs & Ratings accordion)
The bead scope says to remove the Goodreads/Amazon/Audible rating displays. The provider-ratings display row on the book detail page was correctly removed (lines 228-246 deleted). The Goodreads ID field was correctly removed (lines 1213-1231 deleted). But the three matching rating-input fields inside the Edit Metadata accordion — "Goodreads Rating", "Amazon Rating", "Audible Rating" (with their lock buttons) — remain at lines 1330-1387 of the branch. These are editable inputs for dead-provider data that can never be refreshed. The comment at line 1330 still reads: Row: Goodreads rating · Amazon rating · Audible rating. Fix: remove that entire metadata-row div block (lines 1330-1387).
[MAJOR] templates/pages/books_show.html — Screenshot 02 does not show the expanded "Provider IDs & Ratings" section, so the above issue was not caught by the screenshot. The accordion must be captured in its open state to verify the provider-rating inputs are actually gone.
[MINOR] templates/pages/settings_shell.html — Screenshot 03 (named 03_settings_match_weights.png) shows the Email tab, not the Metadata tab. The Settings -> Metadata panel with the match-weights grid is never visible. A re-capture at /settings/metadata is needed to visually confirm the Goodreads/Amazon/Audible weight cards are absent.
REVIEW VERDICT: 0 blocker, 2 major, 1 minor
Code Review — bookshelf-o0m9 (PR #779)
Phase 0: DEMO Verification
No runnable DEMO block was present in this PR. This is a UI removal/cleanup task. CI is green (SHA
c07bdff9), which is the behavioral source of truth. Screenshots are posted via PR comments (01_book_detail.png, 02_book_metadata_editor.png, 03_settings_match_weights.png, 04_series_detail.png). Proceeding on CI-green + screenshot evidence.Phase 1: Spec Compliance
Approach taken: option (b) — trim dead-provider cards from the match-weights feature (feature retained, Goodreads/Amazon/Audible cards removed). Match-weights feature (controller, routes, handler, recalculate) all retained — confirmed load-bearing for real providers.
Spec items confirmed done:
templates/pages/settings_shell.htmlbooks_show.html: Goodreads/Amazon/Audible provider rating display blocks removed; Goodreads ID input removedseries_show.html: AvgGoodreads badge removed; AvgHardcover retainedProviderRatingsstruct: Goodreads/Amazon/Audible fields removedMatchWeightsstruct: 24→19 fields;DefaultMatchWeightstotal 87→79 (arithmetic verified)MatchScoreInput: 10 dead fields removed; score calculation updatedinternal/db/in diff)Phase 2: Code Quality
[MINOR]
templates/pages/books_show.html:1180— stale section commentThe comment
{{/* Row: Google ID · Hardcover ID · Goodreads ID */}}still references "Goodreads ID" even though the Goodreads ID input was removed by this PR. The row now only contains Google Books ID and Hardcover ID. No correctness impact.No other issues found:
TotalWeight()denominator update (87→79): arithmetic correct (removed 2+1+2+1+2=8 points)ErrIncompleteWeightsmessage updated to "all 19 weight fields" and verified against 19 pointer fields inmatchWeightsInputmatch_score_test.goall updated from 87 to 79 — correctmatch_score_store_test.go: removed assertions for Goodreads/Amazon/Audible mapping (correct — the domain struct fields were removed); sqlc row fields still populated in the test fixture to confirm they are silently ignoredseries/store.go:goodreads_ratingcolumn removed frombooksInSeriesBaseQueryand scan target — correct; no other scan-target misalignmentstyle=attributes introducedapp.jsstill registersmatch-weights-settings(controller file retained), templates reference no removed Go struct fieldsREVIEW VERDICT: 0 blocker, 0 major, 1 minor
Corrected Screenshots — fix commit
d05f5a54Fix applied: Goodreads Rating, Amazon Rating, and Audible Rating edit inputs removed from Edit Metadata accordion.
(a) Edit Metadata accordion — Provider IDs & Ratings section (open)
Only real providers remain: Hardcover Rating, Google Books Rating, Open Library Rating. Dead-provider rating inputs gone.
(b) Settings → Metadata tab — match-weights grid
Match-weights grid shows no Goodreads/Amazon/Audible weight cards (never had them — those were match-weight fields already removed in the prior commit). Grid is intact.
Security Review — PR #779 (bookshelf-o0m9) — round 2
Reviewing
git diff origin/main...origin/bd-bookshelf-o0m9(HEADd05f5a54). Scope: Goodreads/Amazon/Audible rating edit inputs, DTO/handler/store write path removal; seriesAvgGoodreadsremoval; settingsMatchWeightsfield removal.Checks performed
No destructive migration —
git diff origin/main...origin/bd-bookshelf-o0m9 -- internal/db/produced no output. The Grimmory DB columns (goodreads_rating,amazon_rating,audible_rating,goodreads_rating_locked, etc.) are untouched; the PR only removes the Go code that read/wrote them from the form. Thegoodreads_id,amazon_rating,audible_ratingcolumns remain in the schema.No orphaned handler path —
formToProviderIDsRequestterminates cleanly afterOpenLibraryRating; no deadif p.GoodreadsRating ...branches remain inmetadata_handler.go. TheUpdateProviderIDsstore function still writesGoodreadsID,AudibleID,AmazonASIN(IDs are retained — only the rating fields are gone from the write path).Metadata-update endpoint still works for retained fields —
ProviderIDsRequestretainsHardcoverRating,GoogleBooksRating,OpenLibraryRating;UpdateProviderIDsmaps all three correctly tosql.NullFloat64. No auth or validation check was accidentally dropped.Lock-toggle endpoint fully intact —
lockableFieldsstill contains"goodreads_rating","amazon_rating","audible_rating"entries, and the switch inToggleFieldLockdispatches them correctly. Users who previously locked these fields can still unlock them via the lock-toggle endpoint even though the edit inputs are no longer rendered.MatchWeights consistency —
MatchWeightsstruct,matchWeightsInput,decodeMatchWeightsInputptrs slice,Validate()fields slice, andTotalWeight()all have the same 19 fields. TheErrIncompleteWeightserror string, the handler comment, and the test fixture JSON are all updated to 19. No partial-update inconsistency.No secrets — no credentials, tokens, or API keys in the diff.
No new attack surface — surface is reduced (fewer form fields accepted). JSON tags for the removed fields are simply absent; unknown form keys are silently ignored by
setFloat/setStr(returnsnil, nil), so no 500 risk from stale clients submitting the old fields.Findings
None.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
UI Re-Review — PR #779 (bookshelf-o0m9) — fix commit
d05f5a54Verifying the two prior MAJORs against the corrected screenshots in comment 9959.
Screenshot observations
Edit Metadata accordion (open) — attachment 2e0fbf6a: The accordion is rendered OPEN. The "PROVIDER IDS & RATINGS" section shows the legend title and all fields are visible. The rating row at the bottom contains exactly three inputs: Hardcover Rating, Google Books Rating, Open Library Rating. No Goodreads Rating, Amazon Rating, or Audible Rating input is present. Audible ID and Amazon ASIN identifier fields correctly remain (these are provider IDs, not rating inputs — correct). The layout is clean and uses canonical
.metadata-field+.metadata-field-inputrows throughout. No orphaned rows or containers.Settings → Metadata tab — attachment 4bbf508a: The Metadata tab is open (not Email). The match-weights grid is visible. Scanning the grid: no Goodreads, Amazon, or Audible weight cards present. Real-provider cards (Google Books, Open Library, Hardcover, etc.) are present. Grid layout looks intact.
Source cross-check
grep -i "goodreads.*rating\|amazon.*rating\|audible.*rating"ontemplates/pages/books_show.htmlat the branch HEAD returns empty — no rating inputs for dead providers remain.grep -i "goodreads\|amazon\|audible"ontemplates/pages/settings_shell.htmlreturns empty — no dead-provider references.style=inline attributes found in either changed template.Prior MAJOR resolution
Both prior MAJORs are resolved:
d05f5a54removed these inputs. The open accordion screenshot confirms their absence. The template source confirms no such inputs exist in the branch.Findings
No new findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
CODE REVIEW — bookshelf-o0m9 (PR #779)
Diff reviewed:
git diff origin/main...origin/bd-bookshelf-o0m9HEADd05f5a54.Phase 0: DEMO — N/A (UI/cleanup bead, no DEMO block required per bead type)
Phase 1: Spec Compliance
All five spec requirements verified:
Dead-provider rating edit inputs removed from
books_show.html— Goodreads Rating, Amazon Rating, Audible Rating<input>rows removed from the Edit Metadata accordion. Confirmed: noprovider_goodreads_rating,provider_amazon_rating,provider_audible_ratingin template. ✓Real providers retained — Google Books, Open Library, Hardcover, ISBN inputs remain. ✓
DTO / handler / store write path —
GoodreadsRating,AmazonRating,AudibleRatingremoved fromProviderIDsRequest,formToProviderIDsRequest, andUpdateProviderIDs. ✓ (verified no dangling rating write references)DB columns untouched — no migration files changed. DB read paths (
GetProviderRatings,buildMetaFromRow, ratings JSON map) preserve Goodreads/Amazon/Audible columns. ✓Match weights — 5 dead-provider weight fields removed from
MatchWeights,matchWeightsInput,TotalWeight,decodeMatchWeightsInput,Validate,DefaultMatchWeights; error message updated 24→19; settings UI cards removed fromsettings_shell.html. ✓Phase 2: Findings
[BLOCKER]
internal/books/metadata_store.go:592 +internal/books/metadata_handler.go:406 — Goodreads ID silently destroyed on every provider-form savebooks_show.htmlhad the Goodreads ID edit input (name="provider_goodreads_id") removed in this PR (it was present onmainat line 1235). However,formToProviderIDsRequeststill callssetStr("provider_goodreads_id")(handler.go:406), which returnsnilwhen the key is absent from the submitted form.UpdateProviderIDsthen maps thatniltonullStr(nil)→sql.NullString{Valid: false}and writes it tobook_metadata.goodreads_id(store.go:592). Any user who saves the Provider IDs accordion will silently NULL out their stored Goodreads ID with no warning or way to recover it.The Goodreads ID write path (
GoodreadsIDinProviderIDsRequest, thesetStrcall, and thenullStr(req.GoodreadsID)in the store) must either:GoodreadsIDfrom the write path entirely (DTO field, handlersetStrcall, storep.GoodreadsID = nullStr(...)call, and thelockableFieldsallow-list entry"goodreads_id") so the field is read-only from this form. The DB column stays; only the edit surface changes.[MINOR]
internal/settings/match_weights_handler_test.go:127 — stale commentComment reads
"the other 23 fields are absent"but the payload now has 19 total fields, so it should say"the other 18 fields".REVIEW VERDICT: 1 blocker, 0 major, 1 minor
The goodreads_id input was removed from the UI form but the write path still read it, causing UpdateBookProviderIDs to always pass NULL for goodreads_id — silently destroying the user's stored Goodreads ID on every Provider IDs accordion save. Fix (option b — make goodreads_id read-only via this form): - Remove GoodreadsID from ProviderIDsRequest DTO - Remove provider_goodreads_id from providerIDMaxLen map - Remove setStr("provider_goodreads_id") call in formToProviderIDsRequest - Remove p.GoodreadsID assignment in UpdateProviderIDs store func - Remove bm.goodreads_id SET clause from UpdateBookProviderIDs SQL query - Remove "goodreads_id" from lockableFields (no longer UI-editable) - Remove case "goodreads_id" from ToggleFieldLock switch (unreachable) - Update all tests to remove goodreads_id from write/lock assertions - Fix stale "other 23 fields" comment → "other 18 fields" in match_weights_handler_test.go The DB column, read path (GetBookProviderIDsRow→MetadataRow), and enrichment/lock-setter wiring are all preserved unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>CODE REVIEW: BLOCKER FIX VERIFICATION — PR #779 (bookshelf-o0m9)
Focused re-review of the BLOCKER fix commit. CI is green. Verified on
origin/bd-bookshelf-o0m9HEAD.Phase 0: DEMO
This is a focused code-correctness re-review of a BLOCKER fix (not a full feature demo). No standalone DEMO block applies — correctness verified by static analysis of the diff below.
Findings
BLOCKER resolved — CONFIRMED.
ProviderIDsRequeststruct (internal/books/dto.go:105) —GoodreadsID *stringfield is absent. Comment at line 103 documents the intent explicitly.formToProviderIDsRequest(internal/books/metadata_handler.go:370) — nosetStr("provider_goodreads_id")call. Lines 400-412 map only Google, Hardcover, ComicVine, Audible, ASIN.UpdateBookProviderIDsParams(internal/db/sqlc/metadata_extra.go:109) —GoodreadsIDfield absent. The UPDATE SQL SET clause (lines 136-146) has nobm.goodreads_id = ?entry and no corresponding arg inExecContext. The DB column cannot be NULLed by the form-save path.DB column preserved —
GetBookProviderIDsSELECT (metadata_extra.go:48) still readsbm.goodreads_idandbm.goodreads_id_locked. No migration touching the column. Read path intact.Enrichment write preserved —
UpsertBookMetadata/metadataToUpsertParamspath in metadata_store.go is unchanged for goodreads_id; enrichment can still populate the column.lockableFields (
internal/books/metadata_store.go) —"goodreads_id"removed from the allow-list map.IsValidLockField("goodreads_id")returns false. Correct.ToggleFieldLock switch —
case "goodreads_id":removed.goodreads_ratingcase retained at line 746. No other field case disturbed.[MINOR] internal/books/metadata_store.go:669 —
SetGoodreadsIDLockedremains inToggleFieldLockDepsstruct but is now unreachable via the switch/lockableFields.The field is never dispatched to (no case in the switch, not in lockableFields). It compiles cleanly, tests still populate it for completeness, and wire.go:336 still wires it. Dead code on the deps struct. No correctness or security impact. Could be removed in a follow-up cleanup, does not block merge.
Scope sanity
The diff correctly removes only Goodreads/Amazon/Audible dead-provider fields:
MatchWeightsstruct: 24 to 19 fields (removed GoodreadsRating, GoodreadsReviewCount, AmazonRating, AmazonReviewCount, AudibleRating)MatchScoreInput: removed GoodreadsRating, AmazonRating, AudibleRating and related lock/review-count fieldsProviderRatingsread path in store.go: removed Goodreads/Amazon/AudibleREVIEW VERDICT: 0 blocker, 0 major, 1 minor
APPROVED — original BLOCKER is fully resolved. The form-write path cannot NULL
bm.goodreads_id. DB column, read path, and enrichment writers are all preserved. Weights feature retained. The sole minor finding (dead SetGoodreadsIDLocked struct field) does not block merge.233ca4c9ec55d593a363