Commit Graph
21 Commits
Author SHA1 Message Date
John O'Keefe 8fc74c908a fix(conflicts): resolution writes funnel through SaveProgress — audio position survives
Batch H1 step 3b — THE data fix. Conflict resolution was the one live
erasure writer: applyProgressResolution and its verbatim twin
applyResolution both wrote the winner via a raw UpdateUniversalProgress
whose param list omits audio_offset_seconds/context_text/viewports, and
the SQL's DO UPDATE SET ... = EXCLUDED... NULLed exactly those. The web
UI resolves through POST /api/conflicts/:id/resolve and
/api/conflicts/bulk-resolve (main.js resolveConflict/bulkResolve), and
the reader's mark-as-read fires a resolution first when the reader SSR
stamps data-conflict-id/-winner — so every resolution today erased the
audiobook position and reader context.

Now: ProgressService is a required ConflictHandler dependency; BOTH
call sites (single resolve + bulk resolve) share one
applyProgressResolution that funnels through SaveProgress (the verbatim
twin is deleted). Deliberate choices, per the plan:
- SkipConflictDetection on the resolution write: both flows apply
  BEFORE the conflict row is marked resolved, so the funnel's LWW
  detector would otherwise file a duplicate pending conflict
  (HasRecentConflictResolution is false at apply time).
- Source "conflict_resolution" — the row's last-write attribution; the
  winner stays durably recorded in resolution_data.winner.
- Broadcast: true — live devices jump to the winning position.
- The winner snapshot maps percentage/epubcfi/context_text/chapter/
  character/page/total_pages; a winner without a CFI clears the locator
  pair (ClearLocator) instead of re-pairing with the loser's stale
  anchor. Everything the snapshot doesn't carry SURVIVES via the LWW
  seed: audio_offset_seconds above all. chapter_progress keeps its
  stored value (was overwritten with the book percentage), an absent
  percentage survives (was written as 0%).
- Accepted: the resolution captures one reading_history session
  (attributed to conflict_resolution; the 2h idle ceiling applies).

Verification: TestH1_ConflictResolutionPreservesAudio drives the real
flow — web push with audio 300s, diverging push from a second source
detects a conflict, resolve with the second source as winner, GET shows
percentage 0.3 AND audio_offset_seconds 300 (previously NULLed), and no
duplicate pending conflict remains. Full contract pin suite green.
The host-run TestConflict*/TestProgress* failures are environmental
(container-only /app/uploads path + db hostname; fail identically at
baseline b4ae423) — the same suites pass in the test-runner container.
2026-10-17 10:26:22 -04:00
John O'Keefe e83c00f869 refactor(progress): ProgressService required at construction; delete the 9 unreachable erase-fallbacks
Batch H1 step 3a. The ceiling census (exhaustive UpdateUniversalProgress
grep) found 9 nil-guarded `if progressSvc != nil { funnel } else { raw
erase-write }` fallbacks — the survey's "four" under-counted kobo.go,
which has FOUR sites (Markup ReadingSync, Markup last-read-place,
AnalyticsGettests, SyncFromServer), plus one each in media.go, queue.go
and koreader.go. All unreachable: the service is created unconditionally
at main.go:127 and wired into every handler and the queue processor; the
test harness wires them too.

The erasure pattern is now unrepresentable:
- NewMediaHandler / NewKOReaderHandler / NewKoboHandler / both
  NewSyncQueueProcessor flavors take the ProgressService as a REQUIRED
  constructor arg; the SetProgressService setters are gone.
- Every fallback body (raw UpdateUniversalProgress with explicit-NULL
  pgtypes for viewport/scroll/panel/reading_mode/zoom and no
  audio_offset_seconds/context_text) is deleted; each write funnels
  through SaveProgress with the LWW seed, so fields the writer doesn't
  know survive.
- Removed the now-dead koreader pg-type helpers (textPtrToPgText,
  intPtrToPgInt4, int64PtrToPgInt8) and their self-tests.

Behavior deltas (stored-row content, no wire shape changes —
KOReader/Kobo response pins stayed green):
- KOReader fallback used to write explicit NULL viewports and drop the
  context_text the koplugin sends; the funneled mainline already maps
  book.ContextText.
- Queue flushes no longer overwrite chapter_progress with the book
  percentage (bogus data; preserved via the seed).
- A web PUT without percentage no longer writes 0% via the deleted
  fallback; percentage seeds from the stored row.

Contract suite: KOReader×4 + Kobo×3 pins green.
2026-10-16 17:28:06 -04:00
John O'Keefe 6457fbaf72 feat(stats): reading statistics — session capture wired + /stats page + widget + per-book card
The design parked in the handoff (multi-tier stats off reading_history;
bookorbit-style in data, not looks) is in. The real prerequisite work
was data: reading_history existed but NOTHING wrote to it — the whole
stats feature was sitting on an empty table.

- CAPTURE (no client changes, no repo-boundary crossings): every
  successful progress write through SaveProgress (KOReader + app + web
  + Kobo + offline queue — the one chokepoint) now records one
  reading_history session row: span = the interval since the PREVIOUS
  write instant, pages = positive page delta when both sides know the
  page index, marker rows for first contact. Spans over 2h are kept as
  zero-second markers (overnight suspend→resume, offline flushes must
  not inflate totals) — the honest 'sync cadence, not a stopwatch'
  caveat stays. device_id only when a REAL device row matches (the
  web/app progress path historically stuffs the USER id into DeviceID
  — probe instead of trusting it; FK violation would silently drop
  every capture). Source preserved in device_metadata.

- API: /api/stats/summary?range=7|30|90|year (totals, zero-filled
  daily arrays, streaks, most-active day/weekday, top books, library
  breakdown — one call), /api/stats/this-week (widget: rolling 7d vs
  previous + streak), /api/stats/books/:id (lifetime + recent
  sessions). All bucketed in the user's timezone; finished ≡ any push
  ≥0.995; visible-library lens enforced at source. Legacy
  /api/analytics/* kept untouched. Range windows anchor end-of-day to
  the START of today (blasting the clock time into tomorrow resolved
  'today' a calendar day late — found by the streak test).

- /stats page (sidebar Stats between Progress and Devices, previously
  /analytics was an orphan): range selector, five stat cards, two
  Chart.js v4 charts (daily minutes bar, pages/day line — chunked via
  dynamic import so main.js stays untouched), per-library breakdown,
  most-read list, empty-state explainer. /analytics now 301s to /stats.
  The orphaned analytics.templ + analytics.ts removed (their containers
  and the page JSON never matched; the page was astub).

- Dashboard 'This Week' widget (server-rendered, suppressed on service
  failure) and the book detail 'Your Reading Stats' card (lifetime
  totals of the viewer's own sessions) complete the tiers: per-user
  default, per-library breakdown inline, per-book on the page,
  per-system/admin aggregate parked as before.

Tests: statistics_test.go (capture semantics at the chokepoint,
tz/day buckets, zero-fill, streak math, finished detection, range
validation, auth gate, widget, per-book lifetime). Full suite PASS.
2026-10-14 12:46:38 -04:00
John O'Keefe 740213f0a4 fix(auth): new users inherit the system-wide default timezone
default_timezone existed as a system setting (admin-editable, exposed
via SettingsRegistry.DefaultTimezone) but nothing read it at user
creation — the CreateUser query didn't even carry the column, so every
new user got the DB default UTC regardless. Both creation paths
(public register + admin create) now seed the user's timezone from the
setting, validated with time.LoadLocation and falling back to UTC when
unwired/empty/invalid.

Test-infra fixes surfaced by the run: TestUpdateProfileSelfEdit was
self-editing the SHARED admin (renaming it past the harness cleanup's
preserve-list, leaking a second admin row that broke the last-admin
tests) — it now edits a dedicated user with a unique name; the
last-admin tests get an ensureSingleAdmin precondition helper; the
websocket collection name is run-unique (non-test names persist in the
shared DB and UNIQUE(user_id,name) 409s on rerun).
2026-10-13 13:17:12 -04:00
John O'Keefe 9cd6356892 feat(server): ghost-session guard — JWTs for deleted users stop authenticating
The JWT middleware validated signature + expiry and fabricated the
request user purely from token claims, so a signature-valid token for a
DELETED user kept full API access until token expiry — observed live on
hardware (a phone session survived the test suite's user-table
recreation and kept serving its true empty sets; open handoff bug 1).

An ensureUserExistsMiddleware already existed but was only wired on the
frontend SSR group (frontend.go) — every /api group used bare
createJWTMiddleware. The existence check now lives INSIDE
createJWTMiddleware's SuccessHandler so every protected group (SSR and
API alike) gets it:

- After claim validation, GetUser by the token's user_id; a missing row
  clears the token cookie and answers 401 user_not_found for API calls
  (the app's TokenAuthenticator then re-logins with stored credentials)
  or redirects to /login?session=invalid for page requests — same
  shape-aware split the error handler already uses.
- Cost: one primary-key lookup per authenticated request (~0.2ms local);
  instant revocation, no TTL semantics. The redundant SSR-only
  middleware is deleted (its redirect behavior is preserved inside the
  guard).

The guard exposed two processing-issues tests that were unknowingly
RELYING on ghost semantics: they nested a second setupTestServer whose
bootstrap deleted the first setup's regular user, then asserted 403
from the dead token. De-nested: a createLibraryDirect helper inserts
the library under the live setup's user, no second bootstrap.

New TestDeletedUserTokenRejected pins the contract: token works while
the user exists -> 401 user_not_found after deletion. Full suite: 219
groups, zero failures.
2026-10-09 22:33:30 -04:00
John O'Keefe f24a2d6c22 test(server): fixture collisions fixed, stale contract asserts re-anchored, gofmt — suite now fully green
Closes the remaining integration-suite failures (handoff open bug 3),
line by line:

- Fixture family (the upsert equivalence): tags_filter_test
  ("Science Fiction" found 1 of 3; "Fantasy" 404'd on zero results),
  media_item_isbn_test (normalization read back the first case's ISBN
  everywhere; edge cases saw a nil ISBN; pagination collapsed 5
  creates onto 1 row), search_unified_test (its 3 books were actually
  1 row + two no-op creates) — all POSTed a constant file_path into
  one library while CreateMediaItem upserts ON CONFLICT
  (library_id, file_path). Every fixture now uses unique per-item
  paths, the suite's universal convention elsewhere. TestTagsFilter
  confirmed same-family: counts came right with no second bug.
- New TestMediaItemCreateDuplicatePath pins the honest-create
  contract from da13898: 201 for the first create; 200 + duplicate
  flag + the EXISTING row (never a 201 lie), second payload not
  overwritten.
- library_browse_test: the admin subtest browsed the container's
  /tmp and required it non-empty — env-coupled (empty /tmp → "was
  []"). Now browses a self-created t.TempDir with two subdirs and a
  decoy file, asserting ONLY directories are listed.
- series_integration_test: three stale assertions re-anchored to the
  shipped contracts, each with a ground-truth comment: /dashboard/
  sections key identity on the collection NAME (BuildSections ID =
  CollectionName; the web/app reorder + hidden prefs persist those
  strings) — the kebab query_type is the storage key and is not
  exposed; /api/series/books is name-ONLY (the app client and the
  web SSR page both send name only; library_id is ignored, never
  validated); /api/series is optional-scope BY DESIGN (GetDistinct-
  Series: narg IS NULL OR library_id = narg aggregates across
  libraries when omitted). The former 400-on-missing-library_id
  expectations asserted validation that never shipped.
- gofmt rider on three untouched-in-this-arc test files
  (dashboard_integration, search, test_helpers).

Full suite with -parallel=1: 217 groups pass, zero failures.
2026-10-09 17:11:12 -04:00
John O'Keefe 208fc2c4d6 fix(server): kobo markup panic — wire LibraryService into the test harness, guard typed-nil resolver
TestProgressWeb_KoboIntegration 500s'd on every markup with a
last-read-place bookmark: the panic-recovered stack was a nil pointer
dereference in LibraryService.ResolveMediaPath (s.db deref), reached
from KoboHandler.convertKoboCFIToStandard during kepub CFI conversion.

Two-layer bug:

1. Test harness (root cause): the router.Config built in
   test_helpers_test.go omitted LibraryService, so router/sync.go's
   SetLibraryService(cfg.LibraryService) stored a nil
   *services.LibraryService in the interface field. Production wiring
   (main.go:246) was always correct.
2. Latent trap (hardening): `h.libraryService != nil` at the
   convertKoboCFIToStandard call site can never catch a typed nil —
   an interface holding a nil *LibraryService is non-nil. ResolveMediaPath
   now degrades with a "library service not configured" error instead
   of panicking on a nil receiver; callers already handle the error
   by skipping CFI conversion.

Harness-only fix, zero production behavior change: the endpoint now
returns a clean 200 with the position saved wherever kepub conversion
is unavailable instead of a panic-recovered 500.

Verified: TestProgressWeb_KoboIntegration passes all three subtests
(401 unauthenticated, ReadingSync→last-read-place percentage
preservation, last-read-place without prior ReadingSync) and on
immediate rerun — previously it failed every run.
2026-10-09 14:53:48 -04:00
John O'Keefe 843f35bbfc test(server): runs leave no residue — auto-cleanup for every resource the suite creates
Symptom: repeated full-suite runs poisoned the dev database with
leaked rows (duplicate Prefs Scope libraries, colliding device names,
saved-filter name conflicts on rerun) because tests created rows but
never deleted them, and stale-binary runs left rows the new cleanup
could not match.

- test_helpers: library creation now registers t.Cleanup DELETE (the
  leak root cause — createTestLibraryWithFolder never cleaned up).
  Suite-wide cleanup passes for libraries/collections/devices/
  saved-filters run both at setup (pre-pass against legacy residue)
  and in t.Cleanup (no per-run residue). Preserve the admin's real
  libraries; delete only convention-named test rows.
- setup seeds canonical system collections every run keyed on
  query_type (idempotent — heals renames/stale tombstones), moved out
  of the preserved-admin branch so it runs for every suite start.
- dashboard_integration: scope-testing library renamed into the
  test-row convention; ordering assertions on the restored
  "Want to Read" collection preserved.
- filters_test: filter names moved into the convention (rerun-safe —
  name-uniqueness 409 no longer trips second runs); five  .(string)
  type assertions panic-proofed with require.True guards (a panic here
  used to kill the whole test process mid-run, which is why a stack of
  "never ran" failures surfaced at once later).
- device_test: device names moved into the convention (ILIKE %test%
  hit them; duplicate-device-name conflicts gone).
2026-10-09 11:04:34 -04:00
John O'Keefe e7ccd7b222 test(dashboard): align test seed naming with the canonical Title-case set + fix stale expectations
The test helper createDefaultCollectionsForUser carried its own
kebab-case copy of the default system collections — drifted from the
canonical Title-case list the registration seed and
RestoreSystemCollection use. Every suite run against the shared dev DB
re-created a kebab set beside the canonical one (the duplicate sections
on the customize screens); the dev DB is cleaned and the helper now
uses the canonical list.

Two stale expectations corrected while re-running the suite:
TestGetSections_EndToEndFlow asserted the kebab ids (now asserts the
Title-case names — section ids ARE collection names), and
TestGetSections_MissingLibraryID expected a 400 the handler never
implemented — absent library_id is the All Libraries view by design
(now asserts 200 + aggregated sections).
2026-10-07 20:32:36 -04:00
John O'Keefe 7b0b2793a6 test(infra): self-healing admin password; cross-library move/merge suite
setupTestServer reuses the shared dev admin (testuser@tests.bookhoard.internal)
instead of re-inserting it, but never reset its password — once any test
mutated the admin's password, every later test in the run failed to log in
with 401s until the database was manually wiped. The seeding step now
resets the password hash to the known test constant on reuse, so full
integration runs are repeatable against an existing database.

Adds cross_library_move_test.go, seven integration tests covering the
duplicate-content features end to end:

- TestCrossLibraryMovePreservesHistory: a book moved between two same-type
  libraries keeps its row ID — progress and annotations survive, no
  duplicate, no archived ghost, library_type_name stays truthful.
- TestCrossLibraryCopyStaysIndependent: deliberate copies in two libraries
  stay independent rows with isolated progress.
- TestCrossLibraryMoveRejectedForTypeMismatch: a reflowable EPUB is not
  repointed between manga libraries; a library_type_mismatch processing
  issue is recorded instead.
- TestListHiddenMediaItemsMatchesActiveTwin: archived rows expose their
  active same-SHA twin (and rows without one report no match).
- TestMergeArchivedItemIntoActiveTwin: merging moves progress and
  annotations onto the active copy and removes the archived row.
- TestMergeArchivedItemRejections: non-admin 403, active source, hash
  mismatch, missing target_id, self-merge, and archived target all fail
  closed.
- TestLibraryTypeNameTriggerOnLibraryChange: the UPDATE trigger refreshes
  library_type_name when a row changes libraries.

Tests create their own temp-dir library folders (host runs don't have
/app/uploads), use unique device identifiers (device_identifier is
UNIQUE and leftover rows broke reruns), and clean up via an explicit
defer that runs while the pool is still open — a t.Cleanup registered
for the same purpose silently no-ops because it executes after
setup.Close has closed the pool.
2026-09-21 18:10:37 -04:00
John O'Keefe 4faa217822 test: repair integration harness — wire SettingsRegistry, reuse preserved dev admin
Two pre-existing harness breaks found while running the device tests:

- RegisterRoutes dereferences cfg.Settings (auth rate limit) but
  setupTestServer never set it — every integration test nil-panicked at
  route registration. Wire database.NewSettingsRegistry(queries) the
  same way main.go does.
- The cleanup deliberately PRESERVES testuser@tests.bookhoard.internal
  (shared dev admin), but setup then blindly re-INSERTed that user, so
  every run after the first failed on users_email_key. Reuse the user
  when it exists; default collections are created only for a NEW user
  (the preserved admin already has its set).

Also documented the offline runner used on slow links: build the test
binary on the host (CGO_ENABLED=0 go test -c) and execute it inside the
bookhoard-tests image against the compose network — no in-container
module downloads, no image rebuild.
2026-09-18 16:06:14 -04:00
john-okeefe 75b33fdae6 feat(sync): wire annotation sync into all device and web handlers
Complete the annotation sync pipeline across all ingest and serve paths.
Previously, annotations sent inline with KOReader progress pushes were
silently discarded, and no annotations were ever served back to devices.

INGEST (device → server):

KOReader (koreader.go):
  - Add processBookAnnotations helper that processes inline highlights,
    notes, and bookmarks from every progress push (immediate + checkpoint)
  - Highlights get CRE→CFI position conversion before SaveHighlight
  - KOReader 'notes' (text + notes) stored as highlights with NoteText
    to ensure correct round-trip classification
  - Bookmarks routed through SaveBookmark with device sync data
  - Called from both updateProgressForBook and handleCheckpointSync

Kobo (kobo.go):
  - Markup handler: annotations and bookmarks route through
    AnnotationService (SaveHighlight/SaveBookmark)
  - Bookmark handler: same routing with device sync data
  - SyncFromServer handler: same routing
  - All handlers fall back to direct DB calls when annotationSvc == nil

Web reader (media.go):
  - CreateMediaHighlight → SaveHighlight (Source="web", ModifiedAt=now)
  - CreateMediaNote → SaveNote (Source="web")
  - DeleteMediaHighlight → TombstoneHighlightByID
  - DeleteMediaNote → TombstoneNoteByID (was hard delete, now tombstone)
  - All fall back to old behavior when annotationSvc == nil

SERVE (server → device):

KOReader GetMetadata (koreader.go):
  - Query and serve bookmarks from media_bookmarks table (was missing)
  - Serve deleted_highlights and deleted_bookmarks arrays containing
    device_sync_data + dedup_key for client-side deletion
  - Highlights/notes already served with reverse CFI conversion

Kobo Markup handler (kobo.go):
  - Track processed books during sync
  - Query tombstones per book, extract bookmark_id from device_sync_data
  - Return DeletedAnnotations array in KoboSyncStatus response

Conflict resolution (conflicts.go):
  - Enable annotation conflict types in ResolveConflict handler
  - Add applyAnnotationResolution dispatching to:
    applyHighlightResolution / applyBookmarkResolution / applyNoteResolution
  - Each looks up by dedup_key and applies winner's fields
  - Allow manual override of auto_resolved conflicts
    (changed check from != "unresolved" to == "user_resolved")

Infrastructure:
  - AnnotationService field + SetAnnotationService in router Config
  - Inject AnnotationService into KOReader, Kobo, Media handlers
  - Start tombstone purger goroutine in main.go (24h interval)
  - Test helpers: construct AnnotationService in test setup
2026-07-29 14:49:19 -04:00
john-okeefe 73fc609d7b fix(tests): protect dev admin from test cleanup, use isolated test names
Tests were deleting the development admin user, causing ON DELETE SET NULL
to cascade and set created_by_admin_id to NULL on all libraries.

- test_helpers: skip deletion of testuser@tests.bookhoard.internal
- sync_integration_test: use test-sync% prefix for isolated test data
2026-05-16 19:31:46 -04:00
john-okeefe cce7ad4907 test(series): add unit and integration tests for series feature
Unit tests:
- series_service_test.go: test continueSeriesRowToMediaItems conversion
  with valid fields, null fields, and comprehensive field mapping
- series_test.go: test handler initialization and textToString helper

Integration tests (series_integration_test.go):
- GET /api/series: requires library_id, rejects invalid UUID, returns
  empty array for empty library, pagination params, limit clamped to
  100, response structure validation, special characters in names
- GET /api/series/books: requires library_id and name, handles
  nonexistent series, unauthorized access
- Restore Continue Series system collection
- Dashboard sections include all 5 collections (including continue-series)

Update test helpers:
- Add SeriesHandler to setupTestServer router config
- Add 5th Continue Series collection to createDefaultCollectionsForUser
- Update dashboard integration test for 5 collections
2026-05-08 20:27:40 -04:00
john-okeefe 94102af4d7 test(progress): add comprehensive integration tests for ProgressService
Adds 30 integration tests across 7 test functions covering all progress
endpoints with real HTTP requests and database verification:

- AuthContexts (8 tests): unauthenticated PUT/GET return 401, regular
  user and admin both get 200, invalid UUID returns 400, nonexistent
  item returns 200 with empty data.

- MergePreservesFields (2 tests): second PUT with only percentage
  preserves epubcfi and chapter from first save via GET verification;
  web save preserves koreader character_offset via DB query.

- EnrichmentComputesFields (2 tests): character_offset computed from
  percentage when total_characters is set on media item; GET returns
  enriched format_group and total_characters.

- ConflictDetection (3 tests): different sources with >1% diff within
  5 minutes creates sync_conflicts record; same-source rapid saves
  create no conflict; <1% diff creates no conflict.

- KoboIntegration (3 tests): ReadingSync then last-read-place preserves
  percentage via DB; standalone last-read-place sets epubcfi/chapter;
  unauthenticated returns 401.

- KOReaderIntegration (2 tests): Bearer token auth with proper request
  body returns 202 Accepted; unauthenticated returns 401.

- DeleteProgress (2 tests): DELETE clears progress; unauthenticated
  returns 401.

- EdgeCases (4 tests): empty body succeeds, 0.0% and 1.0% boundaries,
  all fields with full DB verification of each column.

Updates test_helpers to create ProgressService in setupTestServer and
inject into all handlers. Fixes previous tests that used testing.Short()
(which caused all tests to be skipped in the container) and assertions
against wrong JSON format (pgtype serializes as plain values, not
wrapped objects).
2026-04-25 21:17:08 -04:00
john-okeefe 17f2dc3120 fix(tests): initialize ProcessingIssuesHandler in test server setup
The setupTestServer() helper in test_helpers_test.go was not creating
a ProcessingIssuesHandler and not passing one to the router config,
causing a nil pointer dereference when any processing issues route was
hit during tests. Add handler creation and wire it into routerConfig
to match how cmd/server/main.go does it.
2026-04-22 15:43:45 -04:00
john-okeefe a6700f73e0 fix(tests): handle all Close() and Decode() errors across integration tests
Replace all unhandled resp.Body.Close() calls throughout the test suite:

- Deferred calls: replace 'defer VAR.Body.Close()' with a closure that explicitly
  discards the error via 'defer func(Body io.ReadCloser) { _ = Body.Close() }(VAR.Body)'
- Immediate calls: replace 'VAR.Body.Close()' with '_ = VAR.Body.Close()'

Replace all unhandled json.NewDecoder(VAR.Body).Decode(&x) calls with error capture
and require.NoError assertion. Files using httptest.ResponseRecorder (collections_preview,
processing_issues) use 'err :=' declaration; suite-style tests (scanner_integration,
dashboard_integration) use s.T() instead of t.
2026-04-21 20:33:05 -04:00
john-okeefe 8a5e6963d1 fix: ensure test libraries are cleaned up after each test completes
Problem:
When running `make test-integration`, the last test to run would leave its
"test" libraries in the database. This happened because:

1. setupTestServer() cleaned up old "test" libraries at the START
2. Tests created their own libraries
3. When tests finished, t.Cleanup() called setup.Close() which only closed
   connections but did NOT delete libraries
4. The LAST test's libraries persisted because no subsequent test cleaned them

For example, "Job Status Test Library" from TestWorker_JobStatusTracking
would remain in the database after all tests completed, visible when logging
into the UI.

Root Cause:
The cleanup logic only ran at the START of each test (in setupTestServer),
not at the END. This worked for intermediate tests (each test cleaned up
the previous test's libraries), but the final test had no cleanup.

Solution:
Added library cleanup to the t.Cleanup() function in setupTestServer(). Now
each test deletes its own "test" libraries when it completes, ensuring:
- Clean state after `make test-integration` finishes
- No residual test data in the database
- Safe for tests with subtests (cleanup runs after all subtests finish)

Note on Test Structure:
Tests like TestOPDSEndpoints and TestCollectionSearchLibraryFilter create
libraries once and share them across all subtests. The t.Cleanup() function
runs AFTER all subtests complete, so this change is safe and doesn't
interfere with subtest resource sharing.

Files changed:
- cmd/server/tests/test_helpers_test.go: Added library cleanup to t.Cleanup()
2026-03-24 20:46:07 -04:00
john-okeefe ddfc832b68 feat(api): implement saved filters backend service and handlers
Add complete backend implementation for saved filters CRUD operations
with proper service layer architecture and RESTful API endpoints.

Service Layer (internal/services/filters.go):
- NewFiltersService() constructor following project patterns
- GetSavedFilters(): Retrieve all filters for user + resource type
- CreateSavedFilter(): Create filter with duplicate name validation
- UpdateSavedFilter(): Update filter with ownership verification
- DeleteSavedFilter(): Delete filter with user scoping

Business Logic:
- Filter name uniqueness enforced per user + resource type
- User ownership validation on all operations (JWT user_id)
- JSONB marshaling/unmarshaling for flexible filter storage
- Proper error wrapping with context messages

Handler Layer (internal/handlers/filters.go):
- NewFiltersHandler() constructor (receives db.Queries)
- GetSavedFilters: GET /api/saved-filters?resource_type=X
- CreateSavedFilter: POST /api/saved-filters
- UpdateSavedFilter: PUT /api/saved-filters/:id
- DeleteSavedFilter: DELETE /api/saved-filters/:id

Content Negotiation:
- Supports both JSON (API clients) and HTML (HTMX) responses
- wantsHTML() helper checks Accept header
- HX-Redirect header for HTMX form submissions
- Proper status codes (200, 201, 204, 400, 401, 404, 409)

Router Configuration:
- registerFiltersRoutes() function in internal/router/filters.go
- JWT middleware protection on all endpoints
- RESTful route structure: /api/saved-filters
- Registered in main router.go RegisterRoutes() function
- Added FiltersHandler to router.Config struct

Test Infrastructure:
- Added FiltersHandler to test server setup (test_helpers_test.go)
- FiltersHandler initialized in setupTestServer() function
- Router.Config includes FiltersHandler for integration tests

Code Quality:
- Follows PROJECT_GUIDELINES.md service layer patterns
- Uses database models (not custom domain models)
- JSONB returned as []byte (matches collections pattern)
- All errors wrapped with context using fmt.Errorf
- Handlers create services internally (not dependency injection)

Part of: Saved Filters Implementation (Phase 2: Backend)
Related: #saved-filters-feature
2026-03-21 00:16:10 -04:00
john-okeefe ec4b598728 test(server): update CORS configuration in test helpers with explicit settings
Replace default CORS middleware with explicit CORS configuration in test
server setup to align with production security settings. This ensures test
environment matches production behavior and prevents potential CORS-related
test failures.

Changes:
- Replace echomiddleware.CORS() with echomiddleware.CORSWithConfig()
- Configure allowed origins, methods, and headers explicitly
- Set AllowCredentials to false for test environment
- Add ExposeHeaders for Content-Length

This maintains consistency with the CORS configuration applied to the main
server in commit fb05c49.
2026-03-06 14:35:00 -05:00
john-okeefe 1e8d3c7107 test: rename test files to follow Go conventions
Rename test helper files from .go to _test.go suffix to comply with
Go testing conventions. This ensures proper test file recognition by
the Go toolchain and improves build organization.

- library_test_comprehensive.go → library_test_comprehensive_test.go
- test_helpers.go → test_helpers_test.go
- test_helpers_db.go → test_helpers_db_test.go
2026-03-06 14:15:04 -05:00