From 1fe734e68b7e8cab4be848c0761e6a69fc41c19f Mon Sep 17 00:00:00 2001 From: Sergio Vera Date: Sat, 7 Nov 2026 19:35:52 +0100 Subject: [PATCH 1/2] Some fixes to bilioreads implementation --- internal/datasource/biblioreads/adapter.go | 12 ++ .../datasource/biblioreads/adapter_test.go | 159 +++++++++++++++++- internal/datasource/biblioreads/matching.go | 30 +++- .../webserver/background_progress_test.go | 63 +++++++ .../webserver/document_biblioreads_test.go | 4 +- .../views/partials/background-progress.html | 2 +- 6 files changed, 262 insertions(+), 8 deletions(-) diff --git a/internal/datasource/biblioreads/adapter.go b/internal/datasource/biblioreads/adapter.go index 7315e3aa..f8068c9b 100644 --- a/internal/datasource/biblioreads/adapter.go +++ b/internal/datasource/biblioreads/adapter.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "time" @@ -151,6 +152,17 @@ func (p *Enricher) enrich(ctx context.Context, idx *index.BleveIndexer, document if title := strings.TrimSpace(document.Title); title != q { queries = append(queries, title) } + if title := strings.TrimSpace(withoutParenthetical(document.Title)); title != "" && title != strings.TrimSpace(document.Title) { + if len(document.Authors) > 0 { + query := strings.TrimSpace(title + " " + document.Authors[0]) + if !slices.Contains(queries, query) { + queries = append(queries, query) + } + } + if !slices.Contains(queries, title) { + queries = append(queries, title) + } + } for _, query := range queries { response, err := p.source.SearchBooks(ctx, query) if err != nil { diff --git a/internal/datasource/biblioreads/adapter_test.go b/internal/datasource/biblioreads/adapter_test.go index ef7fb4a9..4d739757 100644 --- a/internal/datasource/biblioreads/adapter_test.go +++ b/internal/datasource/biblioreads/adapter_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "image" "reflect" "slices" @@ -514,13 +515,22 @@ func TestTitleMatchScore(t *testing.T) { {name: "subtitle", title: "La promesa", candidate: "La promesa: Una historia", want: 1}, {name: "document subtitle", title: "La promesa: Una historia", candidate: "La promesa", want: 1}, {name: "parenthetical", title: "La promesa", candidate: "La promesa (The Promise)", want: 1}, + {name: "document parenthetical", title: "La promesa (The Promise)", candidate: "La promesa", want: 1}, + {name: "document volume parenthetical retained", title: "La promesa (Volume 2)", candidate: "La promesa"}, + {name: "candidate volume parenthetical retained", title: "La promesa", candidate: "La promesa (Volume 2)"}, + {name: "document edition", title: "Crímenes Célebres (2ª ed.)", candidate: "Crímenes célebres", want: 1}, + {name: "candidate edition", title: "Crímenes Célebres", candidate: "Crímenes célebres (2ª ed.)", want: 1}, + {name: "different editions", title: "La promesa (2ª ed.)", candidate: "La promesa (3ª ed.)"}, + {name: "different parentheticals", title: "La promesa (First)", candidate: "La promesa (Second)"}, {name: "different subtitles", title: "La promesa: Una historia", candidate: "La promesa: Otra historia"}, {name: "standalone volume", title: "La promesa", candidate: "La promesa (#1)"}, + {name: "document standalone volume", title: "La promesa (#1)", candidate: "La promesa"}, {name: "unrelated", title: "La promesa", candidate: "Otro libro"}, {name: "empty candidate", title: "La promesa"}, } { t.Run(tt.name, func(t *testing.T) { - got := titleMatchScore(tt.candidate, normalizedTitle(tt.title), mainTitle(tt.title)) + withoutSuffix := normalizedTitle(withoutParenthetical(tt.title)) + got := titleMatchScore(tt.candidate, normalizedTitle(tt.title), mainTitle(tt.title), withoutSuffix) if got != tt.want { t.Fatalf("title match score = %d, want %d", got, tt.want) } @@ -528,6 +538,35 @@ func TestTitleMatchScore(t *testing.T) { } } +func TestWithoutParenthetical(t *testing.T) { + for _, tt := range []struct { + title, want string + }{ + {"Crímenes Célebres (2ª ed.)", "Crímenes Célebres"}, + {"Book (2nd edition)", "Book"}, + {"Libro (2.ª edición)", "Libro"}, + {"Book (Revised edition)", "Book"}, + {"Book (Illustrated)", "Book"}, + {"Book (2024 revised)", "Book"}, + {"Book (#2)", "Book (#2)"}, + {"Book (2)", "Book (2)"}, + {"Book (II)", "Book (II)"}, + {"Book (Volume 2)", "Book (Volume 2)"}, + {"Book (Vol. II)", "Book (Vol. II)"}, + {"Book (Tomo 2)", "Book (Tomo 2)"}, + {"Book (2nd part)", "Book (2nd part)"}, + {"Book (Series, #2)", "Book (Series, #2)"}, + {"Book (nested (annotation))", "Book (nested (annotation))"}, + {"Book ()", "Book ()"}, + } { + t.Run(tt.title, func(t *testing.T) { + if got := withoutParenthetical(tt.title); got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + func TestPreferBook(t *testing.T) { for _, tt := range []struct { name string @@ -875,6 +914,124 @@ func TestTitleFallback(t *testing.T) { } } +func TestParentheticalFallback(t *testing.T) { + const title = "Crímenes Célebres" + const author = "Alexandre Dumas" + const bookURL = "/book/show/20371277-cr-menes-c-lebres" + for _, suffix := range []string{"(2ª ed.)", "(Revised edition)", "(Illustrated)"} { + for _, withAuthor := range []bool{true, false} { + t.Run(fmt.Sprint(suffix, " with author ", withAuthor), func(t *testing.T) { + editionTitle := title + " " + suffix + result := SearchResult{Title: title, Author: author, BookURL: bookURL, Rating: "4.00"} + query := title + if withAuthor { + query += " " + author + } + source := &fakeSource{byQuery: map[string][]SearchResult{ + editionTitle + " " + author: {{Title: "Otro libro", Author: author, BookURL: "/book/show/10"}}, + query: {result}, + }} + idx := newTestIndexer(t, source) + doc := index.Document{ID: testID, Metadata: metadata.Metadata{Title: editionTitle, Authors: []string{author}}} + if err := index.IndexDocument(idx.documentsIdx, doc.ID, doc); err != nil { + t.Fatal(err) + } + got, err := idx.Book(context.Background(), doc) + if err != nil || got.BookURL != bookURL { + t.Fatalf("got %+v, %v; want %q", got, err, bookURL) + } + stored, err := idx.DocumentByID(doc.ID) + if err != nil || !mustData(t, stored).Enriched || mustData(t, stored).URL != bookURL { + t.Fatalf("expected persisted match, got %+v, %v", stored, err) + } + want := []string{editionTitle + " " + author, editionTitle, title + " " + author} + if !withAuthor { + want = append(want, title) + } + if !slices.Equal(source.Queries(), want) { + t.Fatalf("queries = %q, want %q", source.Queries(), want) + } + }) + } + } +} + +func TestParentheticalFallbackRejectsWrongAuthor(t *testing.T) { + const title = "Crímenes Célebres" + const author = "Alexandre Dumas" + source := &fakeSource{byQuery: map[string][]SearchResult{ + title + " " + author: {{Title: title, Author: "Another Writer", BookURL: "/book/show/1"}}, + title: {{Title: title, Author: "Another Writer", BookURL: "/book/show/1"}}, + }} + idx := newTestIndexer(t, source) + doc := index.Document{ID: testID, Metadata: metadata.Metadata{Title: title + " (Revised edition)", Authors: []string{author}}} + if err := index.IndexDocument(idx.documentsIdx, doc.ID, doc); err != nil { + t.Fatal(err) + } + got, err := idx.Book(context.Background(), doc) + if err != nil || got.BookURL != "" { + t.Fatalf("wrong author must not match, got %+v, %v", got, err) + } +} + +func TestVolumeDoesNotBroadenSearch(t *testing.T) { + const title = "A book (Volume 2)" + const author = "A Writer" + source := &fakeSource{} + idx := newTestIndexer(t, source) + doc := index.Document{ID: testID, Metadata: metadata.Metadata{Title: title, Authors: []string{author}}} + if err := index.IndexDocument(idx.documentsIdx, doc.ID, doc); err != nil { + t.Fatal(err) + } + if _, err := idx.Book(context.Background(), doc); err != nil { + t.Fatal(err) + } + if want := []string{title + " " + author, title}; !slices.Equal(source.Queries(), want) { + t.Fatalf("queries = %q, want %q", source.Queries(), want) + } +} + +func TestParentheticalFallbackDoesNotDuplicateQueries(t *testing.T) { + source := &fakeSource{} + idx := newTestIndexer(t, source) + doc := index.Document{ID: testID, Metadata: metadata.Metadata{Title: "Crímenes Célebres (2ª ed.)", Authors: []string{""}}} + if err := index.IndexDocument(idx.documentsIdx, doc.ID, doc); err != nil { + t.Fatal(err) + } + if _, err := idx.Book(context.Background(), doc); err != nil { + t.Fatal(err) + } + want := []string{"Crímenes Célebres (2ª ed.)", "Crímenes Célebres"} + if !slices.Equal(source.Queries(), want) { + t.Fatalf("queries = %q, want %q", source.Queries(), want) + } +} + +func TestParentheticalFallbackErrorLeavesDocumentPending(t *testing.T) { + const title = "Crímenes Célebres" + const author = "Alexandre Dumas" + source := &fakeSource{ + byQuery: map[string][]SearchResult{}, + queryErrors: map[string]error{title + " " + author: ErrUnavailable}, + } + idx := newTestIndexer(t, source) + doc := index.Document{ID: testID, Metadata: metadata.Metadata{Title: title + " (2ª ed.)", Authors: []string{author}}} + if err := index.IndexDocument(idx.documentsIdx, doc.ID, doc); err != nil { + t.Fatal(err) + } + if _, err := idx.Book(context.Background(), doc); !errors.Is(err, ErrUnavailable) { + t.Fatalf("expected unavailable error, got %v", err) + } + stored, err := idx.DocumentByID(doc.ID) + if err != nil || mustData(t, stored).Enriched { + t.Fatalf("expected document left pending, got %+v, %v", stored, err) + } + want := []string{doc.Title + " " + author, doc.Title, title + " " + author} + if !slices.Equal(source.Queries(), want) { + t.Fatalf("queries = %q, want %q", source.Queries(), want) + } +} + func TestUnavailableSearchDoesNotBroadenQuery(t *testing.T) { const title = "El economista camuflado" const author = "Tim Harford" diff --git a/internal/datasource/biblioreads/matching.go b/internal/datasource/biblioreads/matching.go index 149c6a56..0a088ff9 100644 --- a/internal/datasource/biblioreads/matching.go +++ b/internal/datasource/biblioreads/matching.go @@ -17,7 +17,8 @@ var seriesSuffix = regexp.MustCompile(`\s*\((?:[^()]+,\s*#[0-9]+(?:\.[0-9]+)?|#[ var subtitleSeparator = regexp.MustCompile(`:\s*|\.\s+|\s+[-–—]\s+`) -var parentheticalTitle = regexp.MustCompile(`\s+\([\p{L}][^()]*\)\s*$`) +var parentheticalTitle = regexp.MustCompile(`\s+\(([^()]*)\)\s*$`) +var standaloneVolume = regexp.MustCompile(`(?i)^(?:[0-9]+|[ivxlcdm]+)$`) var goodreadsBookPath = regexp.MustCompile(`^/book/show/([1-9][0-9]*)(?:[.-][A-Za-z0-9._~-]+)?/?$`) @@ -88,6 +89,25 @@ func normalizedTitle(value string) string { return normalizeName(seriesSuffix.ReplaceAllString(value, "")) } +func withoutParenthetical(value string) string { + match := parentheticalTitle.FindStringSubmatchIndex(value) + if match == nil { + return value + } + annotation := value[match[2]:match[3]] + normalized := normalizeName(annotation) + if normalized == "" || strings.Contains(annotation, "#") || standaloneVolume.MatchString(normalized) { + return value + } + for _, word := range strings.Fields(normalized) { + switch word { + case "vol", "volume", "volumen", "tomo", "tome", "part", "parte", "book", "libro", "livre": + return value + } + } + return strings.TrimSpace(value[:match[0]]) +} + func mainTitle(value string) string { value = seriesSuffix.ReplaceAllString(value, "") if separator := subtitleSeparator.FindStringIndex(value); separator != nil { @@ -96,7 +116,7 @@ func mainTitle(value string) string { return normalizeName(value) } -func titleMatchScore(candidate, title, documentMainTitle string) int { +func titleMatchScore(candidate, title, documentMainTitle, documentWithoutParenthetical string) int { candidateTitle := normalizedTitle(candidate) if candidateTitle == "" { return 0 @@ -105,7 +125,8 @@ func titleMatchScore(candidate, title, documentMainTitle string) int { return 3 } if mainTitle(candidate) == title || documentMainTitle == candidateTitle || - normalizedTitle(parentheticalTitle.ReplaceAllString(candidate, "")) == title { + normalizedTitle(withoutParenthetical(candidate)) == title || + (documentWithoutParenthetical != "" && documentWithoutParenthetical == candidateTitle) { return 1 } return 0 @@ -163,13 +184,14 @@ func matchingBook(document index.Document, results []SearchResult) (SearchResult return SearchResult{}, false } mainTitle := mainTitle(document.Title) + documentWithoutParenthetical := normalizedTitle(withoutParenthetical(document.Title)) var best SearchResult bestScore := 0 for _, result := range results { if result.BookURL == "" { continue } - score := titleMatchScore(result.Title, title, mainTitle) + score := titleMatchScore(result.Title, title, mainTitle, documentWithoutParenthetical) if score == 0 { continue } diff --git a/internal/webserver/background_progress_test.go b/internal/webserver/background_progress_test.go index 00190ae4..c7221f65 100644 --- a/internal/webserver/background_progress_test.go +++ b/internal/webserver/background_progress_test.go @@ -1,6 +1,7 @@ package webserver_test import ( + "errors" "net/http" "sync/atomic" "testing" @@ -22,6 +23,16 @@ type progressCountingInfo struct { calls atomic.Int64 } +type progressStateInfo struct { + webserver.IndexInfo + tasks []index.Progress + err error +} + +func (i *progressStateInfo) BackgroundProgress() ([]index.Progress, error) { + return i.tasks, i.err +} + func (i *progressCountingInfo) BackgroundProgress() ([]index.Progress, error) { i.calls.Add(1) return i.IndexInfo.BackgroundProgress() @@ -199,6 +210,58 @@ func TestBackgroundProgressPollingAccess(t *testing.T) { } } +func TestBackgroundProgressPollingIntervalTransitions(t *testing.T) { + db := infrastructure.Connect(":memory:", 250) + sqlDB, err := db.DB() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = sqlDB.Close() }) + cfg := defaultTestConfig() + sender := &infrastructure.NoEmail{} + appFS := afero.NewMemMapFs() + unusedApp, idx := bootstrapAppWithIndexer(db, sender, appFS, cfg) + t.Cleanup(func() { _ = unusedApp.Shutdown() }) + t.Cleanup(func() { _ = idx.Close() }) + source := wikidata.NewWikidataSource(wikidata.Gowikidata{}) + provider := biblioreads.NewReviewProvider(nil, nil) + controllers := webserver.SetupControllers(cfg, db, idx, sender, appFS, source, nil, source, provider) + state := &progressStateInfo{IndexInfo: idx} + app := webserver.New(cfg, controllers, sender, state, &model.UserRepository{DB: db}) + t.Cleanup(func() { _ = app.Shutdown() }) + + for _, step := range []struct { + name string + tasks []index.Progress + err error + trigger string + }{ + {name: "initially idle", trigger: "every 60s"}, + {name: "work starts", tasks: []index.Progress{{Kind: index.ProgressDocuments, InProgress: true}}, trigger: "every 5s"}, + {name: "work completes", trigger: "every 60s"}, + {name: "progress read fails", err: errors.New("progress unavailable"), trigger: "every 5s"}, + {name: "progress read recovers", trigger: "every 60s"}, + } { + t.Run(step.name, func(t *testing.T) { + state.tasks, state.err = step.tasks, step.err + for _, path := range []string{"/?l=en", "/background-progress?l=en"} { + page := getPage(t, app, path) + banner := page.Find("#background-progress") + if got := banner.AttrOr("hx-trigger", ""); got != step.trigger { + t.Errorf("%s: trigger = %q, want %q", path, got, step.trigger) + } + if banner.AttrOr("hx-get", "") != "/background-progress?l=en" || + banner.AttrOr("hx-swap", "") != "outerHTML" { + t.Errorf("%s: polling endpoint or swap changed", path) + } + if (banner.Find(`[role="alert"]`).Length() != 0) != (step.err != nil) { + t.Errorf("%s: incorrect error alert", path) + } + } + }) + } +} + func TestBackgroundProgressErrorHandlerUsesFeedbackScript(t *testing.T) { db := infrastructure.Connect(":memory:", 250) app := bootstrapApp(db, &infrastructure.NoEmail{}, afero.NewMemMapFs(), webserver.Config{}) diff --git a/internal/webserver/document_biblioreads_test.go b/internal/webserver/document_biblioreads_test.go index 1adb77af..71d581f8 100644 --- a/internal/webserver/document_biblioreads_test.go +++ b/internal/webserver/document_biblioreads_test.go @@ -175,8 +175,8 @@ func TestFailedReviewEnrichmentLeavesNoAlert(t *testing.T) { if banner.Find(".alert").Length() != 0 || strings.TrimSpace(banner.Text()) != "" { t.Fatal("failed ratings retrieval must not leave an empty alert or end-user message") } - if banner.AttrOr("hx-trigger", "") != "every 5s" { - t.Fatal("progress polling must remain available for future tasks") + if banner.AttrOr("hx-trigger", "") != "every 60s" { + t.Fatal("idle progress polling must remain available for future tasks") } } diff --git a/internal/webserver/embedded/views/partials/background-progress.html b/internal/webserver/embedded/views/partials/background-progress.html index 6de5c90d..fcb87d24 100644 --- a/internal/webserver/embedded/views/partials/background-progress.html +++ b/internal/webserver/embedded/views/partials/background-progress.html @@ -1,6 +1,6 @@ {{if or (not .RequireAuth) (and .Session .Session.Uuid)}} {{$progress := backgroundProgress}} -
+
{{if $progress.Error}} {{end}} From 97cb7a01903775cd49c993a780f6aee08d99b27f Mon Sep 17 00:00:00 2001 From: Sergio Vera Date: Mon, 9 Nov 2026 13:33:32 +0100 Subject: [PATCH 2/2] Avoid score skeleton when score is available --- .../webserver/controller/document/detail.go | 10 +++ .../webserver/controller/search/search.go | 6 ++ .../webserver/document_biblioreads_test.go | 73 +++++++++++++++++-- internal/webserver/document_reviews_test.go | 45 ++++++++++++ .../embedded/views/document/detail.html | 2 +- .../views/partials/document-metadata.html | 5 ++ .../views/partials/document-score.html | 4 +- .../search-filters-sidebar-content.html | 2 +- .../webserver/embedded/views/search/list.html | 2 +- .../infrastructure/template_engine.go | 16 ++++ internal/webserver/search_test.go | 65 +++++++++++++++++ 11 files changed, 217 insertions(+), 13 deletions(-) diff --git a/internal/webserver/controller/document/detail.go b/internal/webserver/controller/document/detail.go index 86f4998f..f1d4fae5 100644 --- a/internal/webserver/controller/document/detail.go +++ b/internal/webserver/controller/document/detail.go @@ -44,6 +44,15 @@ func (d *Controller) Detail(c fiber.Ctx) error { title, _ := documentPageTitle(document) + var storedScore datasourcemodel.Score + if d.reviewProvider != nil { + storedScore, err = d.StoredScore(document) + if err != nil { + log.Printf("Error reading stored score for %s: %s", document.ID, err) + storedScore = datasourcemodel.Score{} + } + } + lang, _ := c.Locals("Lang").(string) authorSummaries, illustratorSummaries := d.authorAndIllustratorSummaries(document, lang) @@ -75,6 +84,7 @@ func (d *Controller) Detail(c fiber.Ctx) error { "IllustratorSummaries": illustratorSummaries, "Debug": c.RequestCtx().QueryArgs().Has("debug"), "ReviewsEnabled": d.reviewProvider != nil, + "StoredScore": storedScore, "DocumentLink": d.dataSourceDocumentLink(document), }, "layout") } diff --git a/internal/webserver/controller/search/search.go b/internal/webserver/controller/search/search.go index f618edc1..9d5253a8 100644 --- a/internal/webserver/controller/search/search.go +++ b/internal/webserver/controller/search/search.go @@ -150,9 +150,15 @@ func (s *Controller) renderAuthorSearch(c fiber.Ctx, session model.Session, page } func (s *Controller) baseTemplateVars(c fiber.Ctx, searchType string, similarToDocument index.Document) fiber.Map { + searchPath := "/search" + if similarToDocument.Slug != "" { + searchPath = c.Path() + } + return fiber.Map{ "SearchType": searchType, "SearchPage": true, + "SearchPath": searchPath, "ReviewSourceConfigured": s.config.ReviewSourceConfigured, "EmailFrom": s.sender.From(), "URL": view.URL(c), diff --git a/internal/webserver/document_biblioreads_test.go b/internal/webserver/document_biblioreads_test.go index 71d581f8..6f5758a0 100644 --- a/internal/webserver/document_biblioreads_test.go +++ b/internal/webserver/document_biblioreads_test.go @@ -251,15 +251,23 @@ func TestDocumentDetailBiblioReadsRating(t *testing.T) { }} app, idx := newBiblioReadsTestApp(t, source) - placeholder := documentScorePlaceholders(t, getBody(t, app, "/documents/linked")) - if placeholder.Length() != 1 { - t.Fatalf("expected one BiblioReads placeholder, got %d", placeholder.Length()) + detail := getPage(t, app, "/documents/linked") + scoreRow := detail.Find(`li[hx-get="/documents/linked/score"]`) + if scoreRow.Length() != 1 || strings.TrimSpace(scoreRow.Find("a").Text()) != "3.82" { + t.Fatalf("expected one cached BiblioReads score row, got %q", scoreRow.Text()) } - if url, _ := placeholder.Attr("hx-get"); url != "/documents/linked/score" { - t.Errorf("unexpected placeholder URL %q", url) + if scoreRow.Find(".placeholder").Length() != 0 || scoreRow.HasClass("placeholder-glow") { + t.Error("a cached score must not show a loading skeleton") } - if placeholder.ParentsFiltered(`[id="document-metadata-linked"]`).Length() != 1 { - t.Error("expected the placeholder inside the document metadata list") + if scoreRow.AttrOr("hx-trigger", "") != "load" || scoreRow.AttrOr("hx-target", "") != "this" || + scoreRow.AttrOr("hx-swap", "") != "outerHTML" { + t.Error("expected the cached score to refresh asynchronously in place") + } + if scoreRow.ParentsFiltered(`[id="document-metadata-linked"]`).Length() != 1 { + t.Error("expected the score inside the document metadata list") + } + if len(source.ids) != 0 || len(source.queries) != 0 { + t.Error("rendering the cached score must not call the review service") } page := getPage(t, app, "/documents/linked/score") @@ -288,11 +296,50 @@ func TestDocumentDetailBiblioReadsRating(t *testing.T) { } } +func TestBiblioReadsRatingCountFormatting(t *testing.T) { + for _, tt := range []struct { + count, lang, want string + }{ + {count: "999", lang: "en", want: "999"}, + {count: "2285", lang: "en", want: "2,285"}, + {count: "2,285", lang: "en", want: "2,285"}, + {count: "90366", lang: "es", want: "90.366"}, + {count: "123456", lang: "en", want: "123,456"}, + {count: "123,456", lang: "es", want: "123.456"}, + {count: "1234567", lang: "de", want: "1.234.567"}, + {count: "0", lang: "en", want: "0"}, + } { + t.Run(tt.count+"/"+tt.lang, func(t *testing.T) { + source := &fakeBookSource{book: biblioreads.BookResponse{Rating: "3.82", RatingCount: tt.count}} + app, idx := newBiblioReadsTestApp(t, source) + labels := map[string]string{"en": "ratings", "es": "valoraciones", "de": "Bewertungen"} + want := "(" + tt.want + " " + labels[tt.lang] + ")" + for _, path := range []string{ + "/documents/linked/score?l=" + tt.lang, + "/search?type=documents&search=La&l=" + tt.lang, + } { + page := getPage(t, app, path) + row := page.Find("i.bi-star-fill").Closest("li") + if got := row.Find("small").Text(); row.Length() != 1 || got != want { + t.Errorf("%s: expected %q, got %q", path, want, got) + } + } + stored, err := idx.Document("linked") + if err != nil { + t.Fatal(err) + } + if got := mustBiblioReadsData(t, stored).RatingCount; got != tt.count { + t.Fatalf("formatting must not change stored count: got %q", got) + } + }) + } +} + func TestBiblioReadsRatingCountEscaping(t *testing.T) { for _, count := range []string{ - "2,285", ``, `<img src=x> & "quoted" 'count'`, + "12,34", } { t.Run(count, func(t *testing.T) { source := &fakeBookSource{book: biblioreads.BookResponse{ @@ -422,6 +469,12 @@ func TestDocumentDetailUpdatesStoredBiblioReadsRating(t *testing.T) { }} app, idx := newBiblioReadsTestApp(t, source) + detail := getPage(t, app, "/documents/linked") + cached := detail.Find(`li[hx-get="/documents/linked/score"]`) + if got := strings.TrimSpace(cached.Find("a").Text()); got != "3.82" { + t.Fatalf("expected cached rating before refreshing, got %q", got) + } + page := getPage(t, app, "/documents/linked/score") link := page.Find(`li a[href="https://www.goodreads.com/book/show/12207444-la-reina-dragon"]`) if got := strings.TrimSpace(link.Text()); got != "4.12" { @@ -434,6 +487,10 @@ func TestDocumentDetailUpdatesStoredBiblioReadsRating(t *testing.T) { if stored.DataSourceScoreValue != 4.12 { t.Fatalf("expected latest rating and searchable value to be stored, got %+v", stored) } + reloaded := getPage(t, app, "/documents/linked") + if got := strings.TrimSpace(reloaded.Find(`li[hx-get="/documents/linked/score"] a`).Text()); got != "4.12" { + t.Fatalf("expected refreshed rating on subsequent detail loads, got %q", got) + } results, err := idx.Search(index.SearchFields{DataSourceScoreFrom: 4.1, DataSourceScoreTo: new(4.2)}, 1, 10) if err != nil || results.TotalHits() != 1 { t.Fatalf("expected document to match updated rating range, got %d results and error %v", results.TotalHits(), err) diff --git a/internal/webserver/document_reviews_test.go b/internal/webserver/document_reviews_test.go index f26f840c..7c6a3854 100644 --- a/internal/webserver/document_reviews_test.go +++ b/internal/webserver/document_reviews_test.go @@ -496,6 +496,51 @@ func TestDocumentReviewsHeader(t *testing.T) { } } +func TestDocumentDetailCachedScore(t *testing.T) { + for _, tc := range []struct { + name string + score datasourcemodel.Score + err error + want string + }{ + {name: "rated", score: datasourcemodel.Score{Value: "4.20", Count: "2285"}, want: "4.20"}, + {name: "zero with ratings", score: datasourcemodel.Score{Value: "0.00", Count: "12"}, want: "0.00"}, + {name: "unrated", score: datasourcemodel.Score{Value: "0.00"}}, + {name: "unavailable"}, + {name: "stored score error", err: errors.New("stored score unavailable")}, + } { + t.Run(tc.name, func(t *testing.T) { + tc.score.Source = "Another service" + tc.score.URL = "https://example.org/books/book-1" + provider := &fakeReviewProvider{score: tc.score, scoreErr: tc.err, documentURL: tc.score.URL} + app, _ := newReviewProviderTestApp(t, provider) + page := getPage(t, app, "/documents/linked") + row := page.Find(`li[hx-get="/documents/linked/score"]`) + if row.Length() != 1 || row.AttrOr("hx-trigger", "") != "load" || + row.AttrOr("hx-target", "") != "this" || row.AttrOr("hx-swap", "") != "outerHTML" { + t.Fatal("expected one asynchronously refreshed score row") + } + if tc.want == "" { + if row.Find(".placeholder").Length() != 1 { + t.Fatal("expected a skeleton when no cached score is available") + } + } else { + if row.Find(".placeholder").Length() != 0 || row.HasClass("placeholder-glow") || + strings.TrimSpace(row.Find("a").Text()) != tc.want { + t.Fatalf("expected cached score %q without a skeleton, got %q", tc.want, row.Text()) + } + if row.Find("small").Length() != 1 { + t.Fatal("expected the cached rating count") + } + } + if page.Find("#document-reviews-linked").Length() != 1 || len(provider.scoreSlugs) != 0 || + len(provider.reviewSlugs) != 0 { + t.Fatal("cached rendering must preserve the reviews target without API calls") + } + }) + } +} + func TestDocumentReviewsDisabled(t *testing.T) { app, _ := newReviewProviderTestApp(t, nil) if getPage(t, app, "/background-progress").Find(".alert").Length() != 0 { diff --git a/internal/webserver/embedded/views/document/detail.html b/internal/webserver/embedded/views/document/detail.html index d8eac799..35a552bc 100644 --- a/internal/webserver/embedded/views/document/detail.html +++ b/internal/webserver/embedded/views/document/detail.html @@ -31,7 +31,7 @@

{{.Document.Title}}

{{template "partials/actions" dict "Lang" .Lang "Document" .Document "Session" .Session "FQDN" .fqdn "EmailSendingConfigured" .EmailSendingConfigured "DefaultAction" .DefaultAction "CanShare" .CanShare "PreferredEpub" .PreferredEpub "EmailFrom" .EmailFrom "ShareMaxRecipients" .ShareMaxRecipients "ShareCommentMaxSize" .ShareCommentMaxSize "ButtonSize" "btn-lg" "ButtonStyle" "btn-primary"}}
- {{template "partials/document-metadata" dict "Lang" .Lang "Document" .Document "Session" .Session "WordsPerMinute" .WordsPerMinute "IllustratedMinAmount" .IllustratedMinAmount "ReviewsEnabled" .ReviewsEnabled}} + {{template "partials/document-metadata" dict "Lang" .Lang "Document" .Document "Session" .Session "WordsPerMinute" .WordsPerMinute "IllustratedMinAmount" .IllustratedMinAmount "ReviewsEnabled" .ReviewsEnabled "StoredScore" .StoredScore}}
diff --git a/internal/webserver/embedded/views/partials/document-metadata.html b/internal/webserver/embedded/views/partials/document-metadata.html index 42d31b6b..62a45a5b 100644 --- a/internal/webserver/embedded/views/partials/document-metadata.html +++ b/internal/webserver/embedded/views/partials/document-metadata.html @@ -13,7 +13,12 @@
  • {{.Document.Pages}}
    • {{ end }} {{ if .ReviewsEnabled }} + {{$score := .StoredScore}} + {{if and $score.URL $score.HasScore}} + {{template "partials/document-score" dict "Lang" .Lang "Source" $score.Source "Rating" $score.Value "RatingCount" $score.Count "Scored" true "URL" $score.URL "RefreshScoreSlug" .Document.Slug}} + {{else}}
    • + {{end}} {{ end }} {{if .ShowScore}} {{$score := storedDocumentScore .Document.Document}} diff --git a/internal/webserver/embedded/views/partials/document-score.html b/internal/webserver/embedded/views/partials/document-score.html index 24267558..361f4132 100644 --- a/internal/webserver/embedded/views/partials/document-score.html +++ b/internal/webserver/embedded/views/partials/document-score.html @@ -1,5 +1,5 @@ -
  • - {{if .ScoreUnavailable}}{{t .Lang "Unable to load %s score" .Source}}{{else if .Scored}}{{.Rating}}{{if .RatingCount}} ({{t .Lang "%s ratings" (html .RatingCount)}}){{end}}{{else}}{{t .Lang "Not scored"}}{{end}} + + {{if .ScoreUnavailable}}{{t .Lang "Unable to load %s score" .Source}}{{else if .Scored}}{{.Rating}}{{if .RatingCount}} ({{t .Lang "%s ratings" (html (ratingCount .Lang .RatingCount))}}){{end}}{{else}}{{t .Lang "Not scored"}}{{end}} {{if .CanEditDocumentLink}}