From 3e4a25c8dff064b606a31ac4e83e037b6a87347b Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sun, 27 Sep 2026 11:41:18 -0700 Subject: [PATCH 1/4] fix(db): keep reading history when its book is hard-deleted reading_sessions and read_completions cascaded on books.id, so purging deleted books or removing a library erased every statistic derived from that reading, with nothing left to rebuild it from. book_id is now nullable on both tables with ON DELETE SET NULL: the row survives and loses only its attribution. The user cascade is unchanged. PostgreSQL alters the constraint in place. SQLite rebuilds each table in one transaction, checking the copied row count before the original is dropped and recreating every index. Nothing references either table, so the rebuild never has to switch foreign key enforcement off. Rolling back deletes the orphaned rows first, since they cannot satisfy NOT NULL. --- .../codex-db/src/entities/read_completions.rs | 13 +- .../codex-db/src/entities/reading_sessions.rs | 9 +- .../src/repositories/read_completions.rs | 11 +- .../src/repositories/read_progress.rs | 2 +- .../src/repositories/reading_sessions/fold.rs | 2 +- .../src/repositories/reading_sessions/mod.rs | 2 +- .../src/repositories/reading_stats.rs | 4 +- migration/src/lib.rs | 4 + ...14_reading_history_survives_book_delete.rs | 480 ++++++++++++++++++ tests/db/mod.rs | 1 + tests/db/reading_history_retention.rs | 441 ++++++++++++++++ tests/db/reading_stats.rs | 2 +- 12 files changed, 955 insertions(+), 16 deletions(-) create mode 100644 migration/src/m20260927_000114_reading_history_survives_book_delete.rs create mode 100644 tests/db/reading_history_retention.rs diff --git a/crates/codex-db/src/entities/read_completions.rs b/crates/codex-db/src/entities/read_completions.rs index 596c13ff8..ed3cc371e 100644 --- a/crates/codex-db/src/entities/read_completions.rs +++ b/crates/codex-db/src/entities/read_completions.rs @@ -2,8 +2,9 @@ //! //! An append-only log of finished read-throughs: one row per completed pass of //! one book by one user. Rows are inserted when a book is completed and only -//! ever removed by an explicit history reset (or by a cascade when the user or -//! book is deleted). Nothing updates them. +//! ever removed by an explicit history reset or by a cascade when the user is +//! deleted. Deleting the book keeps the row and clears `book_id`. Nothing else +//! updates them. //! //! This is deliberately separate from `read_progress`, which tracks the //! *current* pass and is deleted when a book is marked unread. Keeping the @@ -21,7 +22,11 @@ pub struct Model { #[sea_orm(primary_key, auto_increment = false)] pub id: Uuid, pub user_id: Uuid, - pub book_id: Uuid, + /// The finished book. `None` once that book has been hard-deleted: the fact + /// that it was finished survives the file, only the attribution is lost. + /// Every write path supplies a book; `None` is only ever reached by the + /// foreign key's `ON DELETE SET NULL`. + pub book_id: Option, /// When this pass started, copied from the `read_progress` row that was /// current when the book completed. pub started_at: DateTime, @@ -36,7 +41,7 @@ pub enum Relation { from = "Column::BookId", to = "super::books::Column::Id", on_update = "NoAction", - on_delete = "Cascade" + on_delete = "SetNull" )] Books, #[sea_orm( diff --git a/crates/codex-db/src/entities/reading_sessions.rs b/crates/codex-db/src/entities/reading_sessions.rs index bc16f85cf..831efe044 100644 --- a/crates/codex-db/src/entities/reading_sessions.rs +++ b/crates/codex-db/src/entities/reading_sessions.rs @@ -28,7 +28,12 @@ pub struct Model { #[sea_orm(primary_key, auto_increment = false)] pub id: Uuid, pub user_id: Uuid, - pub book_id: Uuid, + /// The book this reading happened in. `None` once that book has been + /// hard-deleted: the session outlives the file so the time still counts + /// towards the reader's statistics, but nothing can say which book it was. + /// Every write path supplies a book; `None` is only ever reached by the + /// foreign key's `ON DELETE SET NULL`. + pub book_id: Option, /// Stable per install. Producers with no device concept of their own /// (Komga, OPDS) derive one from the API key or user agent. pub device_id: String, @@ -146,7 +151,7 @@ pub enum Relation { from = "Column::BookId", to = "super::books::Column::Id", on_update = "NoAction", - on_delete = "Cascade" + on_delete = "SetNull" )] Books, #[sea_orm( diff --git a/crates/codex-db/src/repositories/read_completions.rs b/crates/codex-db/src/repositories/read_completions.rs index 00841cd9c..1e598315a 100644 --- a/crates/codex-db/src/repositories/read_completions.rs +++ b/crates/codex-db/src/repositories/read_completions.rs @@ -53,7 +53,7 @@ impl ReadCompletionRepository { let row = read_completions::ActiveModel { id: Set(Uuid::new_v4()), user_id: Set(user_id), - book_id: Set(book_id), + book_id: Set(Some(book_id)), started_at: Set(started_at), completed_at: Set(completed_at), }; @@ -240,8 +240,11 @@ impl ReadCompletionRepository { .all(db) .await?; for row in rows { + let Some(book_id) = row.book_id else { + continue; + }; by_book - .entry(row.book_id) + .entry(book_id) .or_default() .push((row.started_at, row.completed_at)); } @@ -611,8 +614,8 @@ mod tests { .unwrap(); assert_eq!(entries.len(), 2, "the unrelated series must not leak in"); // Newest first. - assert_eq!(entries[0].book_id, second.id); - assert_eq!(entries[1].book_id, first.id); + assert_eq!(entries[0].book_id, Some(second.id)); + assert_eq!(entries[1].book_id, Some(first.id)); } #[tokio::test] diff --git a/crates/codex-db/src/repositories/read_progress.rs b/crates/codex-db/src/repositories/read_progress.rs index 6a51350ec..5816c73eb 100644 --- a/crates/codex-db/src/repositories/read_progress.rs +++ b/crates/codex-db/src/repositories/read_progress.rs @@ -1771,7 +1771,7 @@ mod tests { rows.push(reading_sessions::ActiveModel { id: Set(Uuid::new_v4()), user_id: Set(user.id), - book_id: Set(book.id), + book_id: Set(Some(book.id)), device_id: Set(format!("bench-device-{}", i % 3)), device_name: Set(None), pass: Set(pass), diff --git a/crates/codex-db/src/repositories/reading_sessions/fold.rs b/crates/codex-db/src/repositories/reading_sessions/fold.rs index ec2851b8b..0dfdf9f3f 100644 --- a/crates/codex-db/src/repositories/reading_sessions/fold.rs +++ b/crates/codex-db/src/repositories/reading_sessions/fold.rs @@ -274,7 +274,7 @@ mod tests { Session { id: Uuid::new_v4(), user_id: Uuid::nil(), - book_id: Uuid::nil(), + book_id: Some(Uuid::nil()), device_id: self.device.to_string(), device_name: None, pass: self.pass, diff --git a/crates/codex-db/src/repositories/reading_sessions/mod.rs b/crates/codex-db/src/repositories/reading_sessions/mod.rs index ae699791e..8e21e865d 100644 --- a/crates/codex-db/src/repositories/reading_sessions/mod.rs +++ b/crates/codex-db/src/repositories/reading_sessions/mod.rs @@ -455,7 +455,7 @@ impl ReadingSessionRepository { let model = reading_sessions::ActiveModel { id: Set(session.id), user_id: Set(session.user_id), - book_id: Set(session.book_id), + book_id: Set(Some(session.book_id)), device_id: Set(session.device_id), device_name: Set(session.device_name), pass: Set(pass), diff --git a/crates/codex-db/src/repositories/reading_stats.rs b/crates/codex-db/src/repositories/reading_stats.rs index 6dd0f57ef..99bce9cb2 100644 --- a/crates/codex-db/src/repositories/reading_stats.rs +++ b/crates/codex-db/src/repositories/reading_stats.rs @@ -849,7 +849,7 @@ mod tests { reading_sessions::ActiveModel { id: Set(Uuid::new_v4()), user_id: Set(user_id), - book_id: Set(book_id), + book_id: Set(Some(book_id)), device_id: Set(spec.device.to_string()), device_name: Set(spec.device_name.map(str::to_string)), pass: Set(spec.pass), @@ -2177,7 +2177,7 @@ mod tests_support { reading_sessions::ActiveModel { id: Set(Uuid::new_v4()), user_id: Set(user_id), - book_id: Set(book_id), + book_id: Set(Some(book_id)), device_id: Set(device.to_string()), device_name: Set(Some(device.to_string())), pass: Set(1), diff --git a/migration/src/lib.rs b/migration/src/lib.rs index c9c25b388..74eaff100 100644 --- a/migration/src/lib.rs +++ b/migration/src/lib.rs @@ -215,6 +215,7 @@ mod m20260825_000110_add_task_progress; mod m20260826_000111_normalize_library_reading_direction; mod m20260826_000112_create_user_series_reader_settings; mod m20260828_000113_relax_renumber_series_task_dedup; +mod m20260927_000114_reading_history_survives_book_delete; pub struct Migrator; @@ -414,6 +415,9 @@ impl MigratorTrait for Migrator { // already running: that pass may have read the series before the // book existed, so the request would be lost. Box::new(m20260828_000113_relax_renumber_series_task_dedup::Migration), + // Reading history keeps its rows when the book is hard-deleted; + // only the attribution is lost. + Box::new(m20260927_000114_reading_history_survives_book_delete::Migration), ] } } diff --git a/migration/src/m20260927_000114_reading_history_survives_book_delete.rs b/migration/src/m20260927_000114_reading_history_survives_book_delete.rs new file mode 100644 index 000000000..fed47150c --- /dev/null +++ b/migration/src/m20260927_000114_reading_history_survives_book_delete.rs @@ -0,0 +1,480 @@ +//! Let reading history outlive the books it was recorded against. +//! +//! `reading_sessions` is the only source of every reading statistic and +//! `read_completions` the only record that a book was ever finished. Both +//! cascaded on `books.id`, so purging deleted books or removing a library +//! erased hours the user really spent reading, with nothing left to rebuild +//! them from. A session records something that happened; it does not stop +//! having happened because the file was later removed. +//! +//! After this migration `book_id` is nullable on both tables and its foreign +//! key is `ON DELETE SET NULL`: the row survives and loses only its +//! attribution. `user_id` keeps `ON DELETE CASCADE`, because deleting a user +//! must still remove their history. +//! +//! # How each backend gets there +//! +//! PostgreSQL alters the column and swaps the constraint in place. +//! +//! SQLite cannot alter a column's nullability or a foreign key's action, so +//! each table is rebuilt: create a replacement, copy every row, drop the +//! original, rename the replacement, recreate the indexes. SQLite's documented +//! procedure also switches `foreign_keys` off around the swap, but that step +//! exists to stop dropping a *parent* table from cascading into its children. +//! Nothing references a session or a completion, so these tables have no +//! children and the rebuild is safe with enforcement left on. That matters: the +//! pragma is a no-op inside a transaction and is per connection in a pool, +//! which is what made the swap unreliable in +//! `m20260508_000081_add_release_sources_plugin_uuid_fk`. Here each rebuild +//! runs in one transaction and never touches the pragma. +//! +//! The copy is checked by row count before the original is dropped, and every +//! index from the create migrations is recreated with its original columns and +//! ordering; a rebuild that silently lost an index would slow the statistics +//! queries without failing anything. +//! +//! # Rollback is lossy +//! +//! `down` restores `NOT NULL` and `CASCADE`, which orphaned rows cannot +//! satisfy, so it deletes every row whose `book_id` is null first. Those are +//! exactly the rows this migration exists to keep. + +use sea_orm::{ + ConnectionTrait, DatabaseTransaction, DbBackend, DbErr, Statement, TransactionTrait, +}; +use sea_orm_migration::prelude::*; + +#[derive(DeriveMigrationName)] +pub struct Migration; + +#[async_trait::async_trait] +impl MigrationTrait for Migration { + async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { + migrate(manager, Direction::Up).await + } + + async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { + migrate(manager, Direction::Down).await + } +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum Direction { + /// `book_id` nullable, `ON DELETE SET NULL`. + Up, + /// `book_id` required, `ON DELETE CASCADE`, as originally created. + Down, +} + +impl Direction { + fn on_delete(self) -> ForeignKeyAction { + match self { + Self::Up => ForeignKeyAction::SetNull, + Self::Down => ForeignKeyAction::Cascade, + } + } +} + +async fn migrate(manager: &SchemaManager<'_>, direction: Direction) -> Result<(), DbErr> { + let db = manager.get_connection(); + + if direction == Direction::Down { + for table in ["reading_sessions", "read_completions"] { + db.execute_unprepared(&format!("DELETE FROM {table} WHERE book_id IS NULL")) + .await?; + } + } + + match db.get_database_backend() { + DbBackend::Postgres => { + alter_postgres( + db, + "reading_sessions", + "fk_reading_sessions_book_id", + direction, + ) + .await?; + alter_postgres( + db, + "read_completions", + "fk_read_completions_book_id", + direction, + ) + .await?; + } + DbBackend::Sqlite => { + let txn = db.begin().await?; + rebuild_sqlite(&txn, &ReadingSessionsTable, direction).await?; + rebuild_sqlite(&txn, &ReadCompletionsTable, direction).await?; + txn.commit().await?; + } + DbBackend::MySql => { + return Err(DbErr::Migration( + "MySQL is not a supported backend".to_string(), + )); + } + } + + Ok(()) +} + +async fn alter_postgres( + db: &C, + table: &str, + fk_name: &str, + direction: Direction, +) -> Result<(), DbErr> { + let (nullability, action) = match direction { + Direction::Up => ("DROP NOT NULL", "SET NULL"), + Direction::Down => ("SET NOT NULL", "CASCADE"), + }; + db.execute_unprepared(&format!( + "ALTER TABLE {table} ALTER COLUMN book_id {nullability}" + )) + .await?; + db.execute_unprepared(&format!("ALTER TABLE {table} DROP CONSTRAINT {fk_name}")) + .await?; + db.execute_unprepared(&format!( + "ALTER TABLE {table} ADD CONSTRAINT {fk_name} FOREIGN KEY (book_id) \ + REFERENCES books (id) ON DELETE {action} ON UPDATE NO ACTION" + )) + .await?; + Ok(()) +} + +/// One table's shape, as SQLite needs it to rebuild the table from scratch. +trait RebuildableTable: Sync { + fn name(&self) -> &'static str; + /// Every column, in the order the copy lists them. + fn columns(&self) -> &'static [&'static str]; + /// The full table definition under `table_name`, with `book_id` shaped by + /// `direction`. + fn create(&self, table_name: &str, direction: Direction) -> TableCreateStatement; + /// Every index the create migration made, against the final table name. + fn indexes(&self) -> Vec; +} + +async fn rebuild_sqlite( + txn: &DatabaseTransaction, + table: &dyn RebuildableTable, + direction: Direction, +) -> Result<(), DbErr> { + let backend = DbBackend::Sqlite; + let name = table.name(); + let staging = format!("{name}_rebuild"); + let columns = table.columns().join(", "); + + let before = count_rows(txn, name).await?; + + txn.execute(backend.build(&table.create(&staging, direction))) + .await?; + txn.execute_unprepared(&format!( + "INSERT INTO {staging} ({columns}) SELECT {columns} FROM {name}" + )) + .await?; + + let copied = count_rows(txn, &staging).await?; + if copied != before { + return Err(DbErr::Migration(format!( + "rebuilding {name} copied {copied} of {before} rows; aborting before the original is dropped" + ))); + } + + // Dropping the original drops its indexes with it, which frees their + // names for the recreation below. + txn.execute_unprepared(&format!("DROP TABLE {name}")) + .await?; + txn.execute_unprepared(&format!("ALTER TABLE {staging} RENAME TO {name}")) + .await?; + for index in table.indexes() { + txn.execute(backend.build(&index)).await?; + } + + // A row the copy let through that references a missing user or book would + // otherwise surface only on some later write. + let violations = txn + .query_all(Statement::from_string( + backend, + format!("PRAGMA foreign_key_check({name})"), + )) + .await?; + if !violations.is_empty() { + return Err(DbErr::Migration(format!( + "rebuilding {name} left {} foreign key violations", + violations.len() + ))); + } + + Ok(()) +} + +async fn count_rows(db: &C, table: &str) -> Result { + let row = db + .query_one(Statement::from_string( + db.get_database_backend(), + format!("SELECT COUNT(*) AS n FROM {table}"), + )) + .await? + .ok_or_else(|| DbErr::Migration(format!("COUNT(*) on {table} returned no row")))?; + row.try_get("", "n") +} + +fn book_id_column(direction: Direction) -> ColumnDef { + let mut column = ColumnDef::new(Alias::new("book_id")); + column.uuid(); + match direction { + Direction::Up => column.null(), + Direction::Down => column.not_null(), + }; + column +} + +struct ReadingSessionsTable; + +impl RebuildableTable for ReadingSessionsTable { + fn name(&self) -> &'static str { + "reading_sessions" + } + + fn columns(&self) -> &'static [&'static str] { + &[ + "id", + "user_id", + "book_id", + "device_id", + "device_name", + "pass", + "kind", + "to_page", + "to_percentage", + "r2_progression", + "active_duration_ms", + "duration_source", + "pages_read", + "client_started_at", + "client_ended_at", + "server_recorded_at", + ] + } + + // Mirrors `m20260814_000105_create_reading_sessions` column for column. + fn create(&self, table_name: &str, direction: Direction) -> TableCreateStatement { + let table = Alias::new(table_name); + Table::create() + .table(table.clone()) + .col( + ColumnDef::new(ReadingSessions::Id) + .uuid() + .not_null() + .primary_key(), + ) + .col(ColumnDef::new(ReadingSessions::UserId).uuid().not_null()) + .col(book_id_column(direction)) + .col( + ColumnDef::new(ReadingSessions::DeviceId) + .string() + .not_null(), + ) + .col(ColumnDef::new(ReadingSessions::DeviceName).string().null()) + .col( + ColumnDef::new(ReadingSessions::Pass) + .integer() + .not_null() + .default(1), + ) + .col(ColumnDef::new(ReadingSessions::Kind).string().not_null()) + .col(ColumnDef::new(ReadingSessions::ToPage).integer().null()) + .col( + ColumnDef::new(ReadingSessions::ToPercentage) + .double() + .null(), + ) + .col(ColumnDef::new(ReadingSessions::R2Progression).text().null()) + .col( + ColumnDef::new(ReadingSessions::ActiveDurationMs) + .big_integer() + .null(), + ) + .col( + ColumnDef::new(ReadingSessions::DurationSource) + .string() + .not_null() + .default("unknown"), + ) + .col(ColumnDef::new(ReadingSessions::PagesRead).integer().null()) + .col( + ColumnDef::new(ReadingSessions::ClientStartedAt) + .timestamp_with_time_zone() + .not_null(), + ) + .col( + ColumnDef::new(ReadingSessions::ClientEndedAt) + .timestamp_with_time_zone() + .not_null(), + ) + .col( + ColumnDef::new(ReadingSessions::ServerRecordedAt) + .timestamp_with_time_zone() + .not_null(), + ) + .foreign_key( + ForeignKey::create() + .name("fk_reading_sessions_user_id") + .from(table.clone(), ReadingSessions::UserId) + .to(Users::Table, Users::Id) + .on_delete(ForeignKeyAction::Cascade) + .on_update(ForeignKeyAction::NoAction), + ) + .foreign_key( + ForeignKey::create() + .name("fk_reading_sessions_book_id") + .from(table, ReadingSessions::BookId) + .to(Books::Table, Books::Id) + .on_delete(direction.on_delete()) + .on_update(ForeignKeyAction::NoAction), + ) + .to_owned() + } + + fn indexes(&self) -> Vec { + vec![ + Index::create() + .name("idx_reading_sessions_fold") + .table(ReadingSessions::Table) + .col(ReadingSessions::UserId) + .col(ReadingSessions::BookId) + .col(ReadingSessions::Pass) + .col(ReadingSessions::ClientEndedAt) + .to_owned(), + Index::create() + .name("idx_reading_sessions_stats") + .table(ReadingSessions::Table) + .col(ReadingSessions::UserId) + .col(ReadingSessions::ClientStartedAt) + .to_owned(), + Index::create() + .name("idx_reading_sessions_coalesce") + .table(ReadingSessions::Table) + .col(ReadingSessions::UserId) + .col(ReadingSessions::BookId) + .col(ReadingSessions::DeviceId) + .col(ReadingSessions::Pass) + .col((ReadingSessions::ClientEndedAt, IndexOrder::Desc)) + .to_owned(), + ] + } +} + +struct ReadCompletionsTable; + +impl RebuildableTable for ReadCompletionsTable { + fn name(&self) -> &'static str { + "read_completions" + } + + fn columns(&self) -> &'static [&'static str] { + &["id", "user_id", "book_id", "started_at", "completed_at"] + } + + // Mirrors `m20260729_000103_create_read_completions` column for column. + fn create(&self, table_name: &str, direction: Direction) -> TableCreateStatement { + let table = Alias::new(table_name); + Table::create() + .table(table.clone()) + .col( + ColumnDef::new(ReadCompletions::Id) + .uuid() + .not_null() + .primary_key(), + ) + .col(ColumnDef::new(ReadCompletions::UserId).uuid().not_null()) + .col(book_id_column(direction)) + .col( + ColumnDef::new(ReadCompletions::StartedAt) + .timestamp_with_time_zone() + .not_null(), + ) + .col( + ColumnDef::new(ReadCompletions::CompletedAt) + .timestamp_with_time_zone() + .not_null(), + ) + .foreign_key( + ForeignKey::create() + .name("fk_read_completions_user_id") + .from(table.clone(), ReadCompletions::UserId) + .to(Users::Table, Users::Id) + .on_delete(ForeignKeyAction::Cascade) + .on_update(ForeignKeyAction::NoAction), + ) + .foreign_key( + ForeignKey::create() + .name("fk_read_completions_book_id") + .from(table, ReadCompletions::BookId) + .to(Books::Table, Books::Id) + .on_delete(direction.on_delete()) + .on_update(ForeignKeyAction::NoAction), + ) + .to_owned() + } + + fn indexes(&self) -> Vec { + vec![ + Index::create() + .name("idx_read_completions_user_book") + .table(ReadCompletions::Table) + .col(ReadCompletions::UserId) + .col(ReadCompletions::BookId) + .to_owned(), + Index::create() + .name("idx_read_completions_user_date") + .table(ReadCompletions::Table) + .col(ReadCompletions::UserId) + .col((ReadCompletions::CompletedAt, IndexOrder::Desc)) + .to_owned(), + ] + } +} + +#[derive(DeriveIden)] +enum ReadingSessions { + Table, + Id, + UserId, + BookId, + DeviceId, + DeviceName, + Pass, + Kind, + ToPage, + ToPercentage, + R2Progression, + ActiveDurationMs, + DurationSource, + PagesRead, + ClientStartedAt, + ClientEndedAt, + ServerRecordedAt, +} + +#[derive(DeriveIden)] +enum ReadCompletions { + Table, + Id, + UserId, + BookId, + StartedAt, + CompletedAt, +} + +#[derive(DeriveIden)] +enum Users { + Table, + Id, +} + +#[derive(DeriveIden)] +enum Books { + Table, + Id, +} diff --git a/tests/db/mod.rs b/tests/db/mod.rs index 450d841a5..0cfd457f3 100644 --- a/tests/db/mod.rs +++ b/tests/db/mod.rs @@ -9,6 +9,7 @@ mod entity_event_bridge; mod migrations; mod oidc_pending_state; mod postgres; +mod reading_history_retention; mod reading_sessions; mod reading_stats; mod refresh_token_repository; diff --git a/tests/db/reading_history_retention.rs b/tests/db/reading_history_retention.rs new file mode 100644 index 000000000..066171e27 --- /dev/null +++ b/tests/db/reading_history_retention.rs @@ -0,0 +1,441 @@ +//! Reading history outlives the books it was recorded against. +//! +//! `reading_sessions` is the only source of every reading statistic, and +//! `read_completions` is the only record that a book was ever finished. Both +//! used to cascade on `books.id`, so purging deleted books or removing a +//! library destroyed hours the user really did spend reading, with nothing left +//! to reconstruct them from. +//! +//! A session is a record of something that happened. It does not stop having +//! happened because the file was later removed, so the row survives the delete +//! and loses only its attribution. +//! +//! The user cascade is deliberately unchanged and tested here too: deleting a +//! user must still take their history with them. + +#[path = "../common/mod.rs"] +mod common; + +use chrono::{Duration, Utc}; +use codex::db::ScanningStrategy; +use codex::db::entities::reading_sessions::SessionKind; +use codex::db::entities::{books, read_completions, reading_sessions, users}; +use codex::db::repositories::{ + BookRepository, LibraryRepository, NewSession, ReadCompletionRepository, + ReadProgressRepository, SeriesRepository, UserRepository, +}; +use common::*; +use migration::{Migrator, MigratorTrait}; +use sea_orm::{ + ColumnTrait, ConnectionTrait, DatabaseBackend, DatabaseConnection, EntityTrait, QueryFilter, + Statement, +}; +use tempfile::TempDir; +use uuid::Uuid; + +fn unique() -> String { + Uuid::new_v4().to_string() +} + +async fn persist_user(db: &DatabaseConnection) -> Uuid { + let handle = format!("reader-{}", unique()); + let model = create_test_user(&handle, &format!("{handle}@test.test"), "hash", true); + UserRepository::create(db, &model).await.unwrap().id +} + +async fn persist_book(db: &DatabaseConnection) -> Uuid { + let library = LibraryRepository::create( + db, + "Lib", + &format!("/lib/{}", unique()), + ScanningStrategy::Default, + ) + .await + .unwrap(); + let series = SeriesRepository::create(db, library.id, "Series", None) + .await + .unwrap(); + let book = create_test_book( + series.id, + library.id, + &format!("/lib/{}.cbz", unique()), + "book", + &format!("hash_{}", unique()), + "cbz", + 200, + ); + BookRepository::create(db, &book, None).await.unwrap().id +} + +/// One measured sitting plus one banked completion, which is the pair a +/// finished read leaves behind. +async fn record_history(db: &DatabaseConnection, user: Uuid, book: Uuid) { + let now = Utc::now(); + let session = NewSession::from_client( + Uuid::new_v4(), + user, + book, + "device-1", + Some("Codex Web".to_string()), + SessionKind::Progress, + Some(900_000), + Some(24), + now - Duration::minutes(20), + now - Duration::minutes(5), + ) + .with_page(24); + + ReadProgressRepository::record_session(db, session) + .await + .unwrap(); + + ReadCompletionRepository::record(db, user, book, now - Duration::minutes(20), now) + .await + .unwrap(); +} + +async fn sessions_for_user(db: &DatabaseConnection, user: Uuid) -> Vec { + reading_sessions::Entity::find() + .filter(reading_sessions::Column::UserId.eq(user)) + .all(db) + .await + .unwrap() +} + +async fn completions_for_user(db: &DatabaseConnection, user: Uuid) -> Vec { + read_completions::Entity::find() + .filter(read_completions::Column::UserId.eq(user)) + .all(db) + .await + .unwrap() +} + +/// Hard-deleting a book keeps the history and nulls only the attribution. +/// +/// This is the whole point: `purge_deleted_in_library` and library deletion both +/// reach a real `DELETE`, and either one used to erase the statistics silently. +async fn exercise_history_survives_book_delete(db: &DatabaseConnection) { + let user = persist_user(db).await; + let book = persist_book(db).await; + record_history(db, user, book).await; + + assert_eq!( + sessions_for_user(db, user).await.len(), + 1, + "fixture should have recorded one session" + ); + assert_eq!( + completions_for_user(db, user).await.len(), + 1, + "fixture should have recorded one completion" + ); + + books::Entity::delete_by_id(book).exec(db).await.unwrap(); + + let sessions = sessions_for_user(db, user).await; + assert_eq!( + sessions.len(), + 1, + "the session must outlive the book it was recorded against" + ); + assert!( + sessions[0].book_id.is_none(), + "the surviving session must lose its attribution, not keep a dangling book id" + ); + + let completions = completions_for_user(db, user).await; + assert_eq!( + completions.len(), + 1, + "the completion must outlive the book it was recorded against" + ); + assert!( + completions[0].book_id.is_none(), + "the surviving completion must lose its attribution, not keep a dangling book id" + ); +} + +/// Deleting the user still takes their history. That cascade is unchanged, and +/// a migration that relaxed it by accident would be a privacy defect. +async fn exercise_user_delete_still_removes_history(db: &DatabaseConnection) { + let user = persist_user(db).await; + let book = persist_book(db).await; + record_history(db, user, book).await; + + users::Entity::delete_by_id(user).exec(db).await.unwrap(); + + assert!( + sessions_for_user(db, user).await.is_empty(), + "deleting a user must still remove their sessions" + ); + assert!( + completions_for_user(db, user).await.is_empty(), + "deleting a user must still remove their completions" + ); +} + +#[tokio::test] +async fn history_survives_book_delete_sqlite() { + let (db, _temp_dir) = setup_test_db().await; + exercise_history_survives_book_delete(&db).await; +} + +#[tokio::test] +async fn user_delete_still_removes_history_sqlite() { + let (db, _temp_dir) = setup_test_db().await; + exercise_user_delete_still_removes_history(&db).await; +} + +/// Both of the above against PostgreSQL, sequenced in one test on purpose: +/// `setup_test_db_postgres` truncates a database shared by the whole run, so two +/// PostgreSQL tests running at once delete each other's fixtures. +/// +/// This one matters more than most: the SQLite path rebuilds the table to change +/// the constraint while PostgreSQL alters it in place, so the two engines reach +/// the same schema by different routes and only this test says they agree. +#[tokio::test] +#[ignore] // Requires PostgreSQL test database +async fn reading_history_retention_postgres() { + let Some(db) = setup_test_db_postgres().await else { + eprintln!("PostgreSQL test database not available, skipping"); + return; + }; + + exercise_history_survives_book_delete(&db).await; + exercise_user_delete_still_removes_history(&db).await; +} + +// ============================================================================ +// The SQLite table rebuild +// ============================================================================ + +const HISTORY_TABLES: [&str; 2] = ["reading_sessions", "read_completions"]; + +/// A SQLite database migrated up to, but not including, the migration that +/// relaxes the cascade: the schema exactly as the original create migrations +/// left it. +async fn sqlite_before_retention_migration() -> (Database, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir.path().join("test.db"); + let config = DatabaseConfig { + db_type: DatabaseType::SQLite, + postgres: None, + sqlite: Some(SQLiteConfig { + path: db_path.to_str().unwrap().to_string(), + pragmas: None, + ..SQLiteConfig::default() + }), + ..DatabaseConfig::default() + }; + let db = Database::new(&config).await.unwrap(); + + let migrations = Migrator::migrations(); + let target = migrations + .iter() + .position(|m| m.name().contains("reading_history_survives_book_delete")) + .expect("the retention migration should be registered"); + Migrator::up(db.sea_orm_connection(), Some(target as u32)) + .await + .unwrap(); + (db, temp_dir) +} + +async fn apply_retention_migration(db: &DatabaseConnection) { + Migrator::up(db, Some(1)).await.unwrap(); +} + +async fn strings(db: &DatabaseConnection, sql: &str) -> Vec { + db.query_all(Statement::from_string( + DatabaseBackend::Sqlite, + sql.to_string(), + )) + .await + .unwrap() + .into_iter() + .map(|row| row.try_get_by_index::(0).unwrap()) + .collect() +} + +/// Every index on the table, as the SQL that created it. Comparing the SQL +/// catches a dropped index, a lost column and a lost `DESC` alike. +async fn index_definitions(db: &DatabaseConnection, table: &str) -> Vec { + strings( + db, + &format!( + "SELECT sql FROM sqlite_master WHERE type = 'index' AND tbl_name = '{table}' \ + AND sql IS NOT NULL ORDER BY name" + ), + ) + .await +} + +/// `name type notnull default pk` per column, in column order. +async fn column_shapes(db: &DatabaseConnection, table: &str) -> Vec { + strings( + db, + &format!( + "SELECT name || ' ' || type || ' ' || \"notnull\" || ' ' || \ + COALESCE(dflt_value, '-') || ' ' || pk FROM pragma_table_info('{table}') ORDER BY cid" + ), + ) + .await +} + +/// `column -> table on_delete` per foreign key. +async fn foreign_keys(db: &DatabaseConnection, table: &str) -> Vec { + strings( + db, + &format!( + "SELECT \"from\" || ' -> ' || \"table\" || ' ' || on_delete \ + FROM pragma_foreign_key_list('{table}') ORDER BY \"from\"" + ), + ) + .await +} + +/// The rebuild keeps every row and attribution, reproduces the original +/// indexes and columns exactly, and changes only `book_id`'s nullability and +/// delete action. +#[tokio::test] +async fn sqlite_rebuild_keeps_rows_and_indexes() { + let (db, _temp_dir) = sqlite_before_retention_migration().await; + let conn = db.sea_orm_connection(); + + let user = persist_user(conn).await; + let mut books = Vec::new(); + for _ in 0..3 { + let book = persist_book(conn).await; + record_history(conn, user, book).await; + books.push(book); + } + + let mut indexes_before = Vec::new(); + let mut columns_before = Vec::new(); + for table in HISTORY_TABLES { + indexes_before.push(index_definitions(conn, table).await); + columns_before.push(column_shapes(conn, table).await); + } + assert_eq!( + indexes_before[0].len(), + 3, + "reading_sessions starts with 3 indexes" + ); + assert_eq!( + indexes_before[1].len(), + 2, + "read_completions starts with 2 indexes" + ); + + apply_retention_migration(conn).await; + + for (i, table) in HISTORY_TABLES.into_iter().enumerate() { + assert_eq!( + index_definitions(conn, table).await, + indexes_before[i], + "{table}: the rebuild must recreate every index exactly" + ); + + let expected_columns: Vec = columns_before[i] + .iter() + .map(|shape| { + if shape.starts_with("book_id ") { + shape.replacen(" 1 ", " 0 ", 1) + } else { + shape.clone() + } + }) + .collect(); + assert_eq!( + column_shapes(conn, table).await, + expected_columns, + "{table}: only book_id's nullability may change" + ); + + assert_eq!( + foreign_keys(conn, table).await, + vec![ + "book_id -> books SET NULL".to_string(), + "user_id -> users CASCADE".to_string(), + ], + "{table}: book deletes null the column, user deletes still cascade" + ); + } + + let sessions = sessions_for_user(conn, user).await; + let completions = completions_for_user(conn, user).await; + assert_eq!(sessions.len(), 3, "every session survives the rebuild"); + assert_eq!( + completions.len(), + 3, + "every completion survives the rebuild" + ); + for book in &books { + assert!(sessions.iter().any(|s| s.book_id == Some(*book))); + assert!(completions.iter().any(|c| c.book_id == Some(*book))); + } + + // Enforcement is on for application connections, so this proves the new + // constraint is live and not merely declared. + books::Entity::delete_by_id(books[0]) + .exec(conn) + .await + .unwrap(); + let sessions = sessions_for_user(conn, user).await; + assert_eq!(sessions.len(), 3); + assert_eq!(sessions.iter().filter(|s| s.book_id.is_none()).count(), 1); + + db.close().await; +} + +/// Rolling back restores the original constraint, which orphaned rows cannot +/// satisfy, so it discards exactly those and keeps everything attributed. +#[tokio::test] +async fn sqlite_rollback_drops_only_orphans() { + let (db, _temp_dir) = setup_test_db_wrapper().await; + let conn = db.sea_orm_connection(); + + let user = persist_user(conn).await; + let kept = persist_book(conn).await; + let removed = persist_book(conn).await; + record_history(conn, user, kept).await; + record_history(conn, user, removed).await; + books::Entity::delete_by_id(removed) + .exec(conn) + .await + .unwrap(); + + Migrator::down(conn, Some(1)).await.unwrap(); + + for table in HISTORY_TABLES { + assert!( + column_shapes(conn, table) + .await + .iter() + .any(|shape| shape.starts_with("book_id ") && shape.contains(" 1 ")), + "{table}: book_id is required again" + ); + assert!( + foreign_keys(conn, table) + .await + .contains(&"book_id -> books CASCADE".to_string()), + "{table}: book deletes cascade again" + ); + } + + let sessions = sessions_for_user(conn, user).await; + let completions = completions_for_user(conn, user).await; + assert_eq!(sessions.len(), 1, "only the orphaned session is discarded"); + assert_eq!(sessions[0].book_id, Some(kept)); + assert_eq!( + completions.len(), + 1, + "only the orphaned completion is discarded" + ); + assert_eq!(completions[0].book_id, Some(kept)); + + // And forward again, so the migration is re-runnable. + Migrator::up(conn, None).await.unwrap(); + assert_eq!(sessions_for_user(conn, user).await.len(), 1); + + db.close().await; +} diff --git a/tests/db/reading_stats.rs b/tests/db/reading_stats.rs index 4ed96b296..02f90c7bc 100644 --- a/tests/db/reading_stats.rs +++ b/tests/db/reading_stats.rs @@ -92,7 +92,7 @@ async fn seed( reading_sessions::ActiveModel { id: Set(Uuid::new_v4()), user_id: Set(user_id), - book_id: Set(book_id), + book_id: Set(Some(book_id)), device_id: Set(device.to_string()), device_name: Set(Some(device.to_string())), pass: Set(1), From 8c93411231b94eb50c1444ad7aead7899819ac1c Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sun, 27 Sep 2026 11:54:38 -0700 Subject: [PATCH 2/4] feat(api): count reading of deleted books, and let a reader purge it Reading history now survives its book being hard-deleted, with a null book_id. Totals, the per-period chart, the per-device split and coverage never joined books, so they keep counting that time unchanged. The series and format breakdowns did join books, so they now LEFT JOIN and gather those sessions into one row flagged removedFromLibrary, with a null series or format. The orphan row sorts after named rows on both engines, which disagree about where a null goes. Books read and books finished count distinct books, and deleted books cannot be told apart, so those two figures drop a deleted book's contribution. Time, pages and sittings are per-row sums and stay exact. DELETE /api/v1/reading-stats/orphaned removes the caller's detached sessions and completions in one transaction and reports the counts. Nothing else is touched, and no other user's history is reachable. --- crates/codex-api/src/docs.rs | 2 + .../src/routes/v1/dto/reading_stats.rs | 43 ++++- .../src/routes/v1/handlers/reading_stats.rs | 47 +++++- .../codex-api/src/routes/v1/routes/books.rs | 6 + crates/codex-db/src/repositories/mod.rs | 6 +- .../src/repositories/reading_stats.rs | 107 +++++++++--- docs/api/openapi.json | 91 ++++++++++- tests/api/reading_stats.rs | 145 ++++++++++++++++- tests/db/reading_history_retention.rs | 152 +++++++++++++++++- tests/db/reading_stats.rs | 6 +- web/openapi.json | 91 ++++++++++- web/src/types/api.generated.ts | 109 ++++++++++++- 12 files changed, 745 insertions(+), 60 deletions(-) diff --git a/crates/codex-api/src/docs.rs b/crates/codex-api/src/docs.rs index f32e0fdb4..c36870572 100644 --- a/crates/codex-api/src/docs.rs +++ b/crates/codex-api/src/docs.rs @@ -369,6 +369,7 @@ The following paths are exempt from rate limiting: v1::handlers::record_reading_sessions, v1::handlers::get_reading_stats, v1::handlers::get_reading_coverage, + v1::handlers::purge_orphaned_reading_history, // Reading progress endpoints v1::handlers::update_reading_progress, @@ -1046,6 +1047,7 @@ The following paths are exempt from rate limiting: v1::dto::ReadingStatsGranularity, v1::dto::ReadingStatsSort, v1::dto::ReadingCoverageDto, + v1::dto::PurgedOrphanedHistoryDto, // Reading session DTOs v1::dto::RecordReadingSessionsRequest, diff --git a/crates/codex-api/src/routes/v1/dto/reading_stats.rs b/crates/codex-api/src/routes/v1/dto/reading_stats.rs index cf6143c73..152d1d7e7 100644 --- a/crates/codex-api/src/routes/v1/dto/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/dto/reading_stats.rs @@ -6,8 +6,8 @@ use chrono::{DateTime, Utc}; use codex_db::repositories::{ - DurationBreakdown, ReadingByDevice, ReadingByFormat, ReadingBySeries, ReadingCoverage, - ReadingPeriod, ReadingSummary, StatsGranularity, StatsSort, + DurationBreakdown, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, ReadingBySeries, + ReadingCoverage, ReadingPeriod, ReadingSummary, StatsGranularity, StatsSort, }; use serde::{Deserialize, Serialize}; use utoipa::{IntoParams, ToSchema}; @@ -219,9 +219,17 @@ impl From for ReadingByDeviceDto { #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "camelCase")] pub struct ReadingBySeriesDto { - pub series_id: Uuid, + /// Null on the removed-from-library row. + pub series_id: Option, + /// Null on the removed-from-library row. #[schema(example = "Berserk")] - pub series_name: String, + pub series_name: Option, + /// True on the single row that gathers reading whose book has since been + /// deleted from the server. That time still counts towards every total, + /// but which series it belonged to is no longer known. Its `books` and + /// `booksFinished` are always 0: both count distinct books, and deleted + /// books cannot be told apart. + pub removed_from_library: bool, pub duration: DurationBreakdownDto, pub pages_read: i64, pub sessions: i64, @@ -235,6 +243,7 @@ pub struct ReadingBySeriesDto { impl From for ReadingBySeriesDto { fn from(value: ReadingBySeries) -> Self { Self { + removed_from_library: value.series_id.is_none(), series_id: value.series_id, series_name: value.series_name, duration: value.duration.into(), @@ -250,8 +259,12 @@ impl From for ReadingBySeriesDto { #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "camelCase")] pub struct ReadingByFormatDto { + /// Null on the removed-from-library row. #[schema(example = "cbz")] - pub format: String, + pub format: Option, + /// True on the single row that gathers reading whose book has since been + /// deleted from the server, and whose format is therefore unknown. + pub removed_from_library: bool, pub duration: DurationBreakdownDto, pub pages_read: i64, pub sessions: i64, @@ -261,6 +274,7 @@ pub struct ReadingByFormatDto { impl From for ReadingByFormatDto { fn from(value: ReadingByFormat) -> Self { Self { + removed_from_library: value.format.is_none(), format: value.format, duration: value.duration.into(), pages_read: value.pages_read, @@ -308,3 +322,22 @@ pub struct ReadingStatsResponse { pub series: Vec, pub formats: Vec, } + +/// What purging the removed-from-library history deleted. +#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] +#[serde(rename_all = "camelCase")] +pub struct PurgedOrphanedHistoryDto { + /// Reading sessions deleted. Their time no longer counts anywhere. + pub sessions_removed: u64, + /// Finished read-throughs deleted. + pub completions_removed: u64, +} + +impl From for PurgedOrphanedHistoryDto { + fn from(value: PurgedOrphanedHistory) -> Self { + Self { + sessions_removed: value.sessions, + completions_removed: value.completions, + } + } +} diff --git a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs index 4ac3c4330..25e0e4827 100644 --- a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs @@ -1,9 +1,9 @@ //! Reading statistics, aggregated from the session log. use super::super::dto::{ - DurationBreakdownDto, ReadingByDeviceDto, ReadingByFormatDto, ReadingBySeriesDto, - ReadingCoverageDto, ReadingPeriodDto, ReadingStatsGranularity, ReadingStatsQuery, - ReadingStatsResponse, ReadingStatsSort, ReadingSummaryDto, + DurationBreakdownDto, PurgedOrphanedHistoryDto, ReadingByDeviceDto, ReadingByFormatDto, + ReadingBySeriesDto, ReadingCoverageDto, ReadingPeriodDto, ReadingStatsGranularity, + ReadingStatsQuery, ReadingStatsResponse, ReadingStatsSort, ReadingSummaryDto, }; use crate::{AppState, error::ApiError, extractors::AuthContext, permissions::Permission}; use axum::{ @@ -34,7 +34,7 @@ const MAX_TZ_OFFSET_MINUTES: i32 = 14 * 60; #[derive(OpenApi)] #[openapi( - paths(get_reading_stats), + paths(get_reading_stats, purge_orphaned_reading_history), components(schemas( ReadingStatsResponse, ReadingSummaryDto, @@ -44,6 +44,7 @@ const MAX_TZ_OFFSET_MINUTES: i32 = 14 * 60; ReadingByFormatDto, DurationBreakdownDto, ReadingStatsGranularity, + PurgedOrphanedHistoryDto, )), tags( (name = "Reading Statistics", description = "Aggregated reading time and pages") @@ -186,3 +187,41 @@ pub async fn get_reading_coverage( Ok(Json(coverage.into())) } + +/// Delete the caller's reading history for books that no longer exist +/// +/// When a book is deleted from the server its reading sessions and finished +/// read-throughs are kept, so the time still counts towards every statistic; +/// the series and format breakdowns show it as one "removed from library" row. +/// This discards those rows for the caller, and only for the caller. +/// +/// Irreversible, and it forecloses the other way out: importing a reading +/// progress export taken before the delete puts those sessions back on their +/// books. Only history already detached from any book is touched; attributed +/// reading is never affected. +#[utoipa::path( + delete, + path = "/api/v1/reading-stats/orphaned", + responses( + (status = 200, description = "What was deleted", body = PurgedOrphanedHistoryDto), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Forbidden"), + ), + security( + ("jwt_bearer" = []), + ("api_key" = []) + ), + tag = "Reading Statistics" +)] +pub async fn purge_orphaned_reading_history( + State(state): State>, + auth: AuthContext, +) -> Result, ApiError> { + auth.require_permission(&Permission::ProgressWrite)?; + + let purged = ReadingStatsRepository::purge_orphaned(&state.db, auth.user_id) + .await + .map_err(|e| ApiError::Internal(format!("Failed to purge orphaned history: {}", e)))?; + + Ok(Json(purged.into())) +} diff --git a/crates/codex-api/src/routes/v1/routes/books.rs b/crates/codex-api/src/routes/v1/routes/books.rs index 4f1478969..469d74f21 100644 --- a/crates/codex-api/src/routes/v1/routes/books.rs +++ b/crates/codex-api/src/routes/v1/routes/books.rs @@ -186,6 +186,12 @@ pub fn routes(_state: Arc) -> Router> { "/reading-stats/coverage", get(handlers::get_reading_coverage), ) + // Reading whose book has been deleted is kept until the reader says + // otherwise. + .route( + "/reading-stats/orphaned", + delete(handlers::purge_orphaned_reading_history), + ) // Mark as read/unread routes .route("/books/{book_id}/read", post(handlers::mark_book_as_read)) .route( diff --git a/crates/codex-db/src/repositories/mod.rs b/crates/codex-db/src/repositories/mod.rs index 05ff2a2f6..366be3850 100644 --- a/crates/codex-db/src/repositories/mod.rs +++ b/crates/codex-db/src/repositories/mod.rs @@ -97,9 +97,9 @@ pub use read_progress::ReadProgressRepository; pub use reading_sessions::{AppendOutcome, DeviceContext, NewSession, ReadingSessionRepository}; #[allow(unused_imports)] pub use reading_stats::{ - DurationBreakdown, ReadingByDevice, ReadingByFormat, ReadingBySeries, ReadingCoverage, - ReadingPeriod, ReadingStatsRepository, ReadingSummary, StatsGranularity, StatsSort, - StatsWindow, + DurationBreakdown, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, ReadingBySeries, + ReadingCoverage, ReadingPeriod, ReadingStatsRepository, ReadingSummary, StatsGranularity, + StatsSort, StatsWindow, }; #[allow(unused_imports)] pub use refresh_token::{NewRefreshToken, RefreshTokenRepository}; diff --git a/crates/codex-db/src/repositories/reading_stats.rs b/crates/codex-db/src/repositories/reading_stats.rs index 99bce9cb2..7a0d1a998 100644 --- a/crates/codex-db/src/repositories/reading_stats.rs +++ b/crates/codex-db/src/repositories/reading_stats.rs @@ -17,9 +17,13 @@ #![allow(dead_code)] +use crate::entities::{read_completions, reading_sessions}; use anyhow::Result; use chrono::{DateTime, Utc}; -use sea_orm::{ConnectionTrait, DatabaseBackend, FromQueryResult, Statement, Value}; +use sea_orm::{ + ColumnTrait, ConnectionTrait, DatabaseBackend, EntityTrait, FromQueryResult, QueryFilter, + Statement, TransactionTrait, Value, +}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -182,10 +186,14 @@ pub struct ReadingByDevice { } /// Totals for one series. +/// +/// `series_id` and `series_name` are both `None` on the one row that gathers +/// reading whose book has since been hard-deleted. That time is real and +/// counts towards every total, but nothing says which series it came from. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct ReadingBySeries { - pub series_id: Uuid, - pub series_name: String, + pub series_id: Option, + pub series_name: Option, pub duration: DurationBreakdown, pub pages_read: i64, pub sessions: i64, @@ -194,9 +202,12 @@ pub struct ReadingBySeries { } /// Totals for one file format. +/// +/// `format` is `None` on the one row that gathers reading whose book has since +/// been hard-deleted, for the same reason as [`ReadingBySeries`]. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct ReadingByFormat { - pub format: String, + pub format: Option, pub duration: DurationBreakdown, pub pages_read: i64, pub sessions: i64, @@ -211,6 +222,13 @@ pub struct ReadingCoverage { pub last_read_at: Option>, } +/// What [`ReadingStatsRepository::purge_orphaned`] removed. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct PurgedOrphanedHistory { + pub sessions: u64, + pub completions: u64, +} + /// The window a query covers. #[derive(Copy, Clone, Debug)] pub struct StatsWindow { @@ -281,7 +299,18 @@ const READING_KINDS: &str = "rs.kind IN ('progress', 'completed')"; /// /// Left-joined and coalesced rather than inner-joined: a series whose metadata /// row is missing has to keep appearing in the reader's own history. -const SERIES_TITLE_SORT: &str = "COALESCE(sm.title_sort, sm.title, s.name)"; +/// +/// Sessions outlive a hard-deleted book with a null `book_id`, so the series +/// and format breakdowns `LEFT JOIN` too, and those orphans group into a single +/// row whose name is null. The engines disagree about where a null sorts (first +/// on SQLite, last on PostgreSQL), so the orphan row is placed after every +/// named one explicitly rather than by whichever backend is running. +const SERIES_TIEBREAK: &str = + "CASE WHEN s.id IS NULL THEN 1 ELSE 0 END ASC, COALESCE(sm.title_sort, sm.title, s.name)"; + +/// The format breakdown's tiebreak, with the orphan row last. `books.format` is +/// never null, so a null format means the book is gone. +const FORMAT_TIEBREAK: &str = "CASE WHEN b.format IS NULL THEN 1 ELSE 0 END ASC, b.format"; #[derive(Debug, FromQueryResult)] struct SummaryRow { @@ -319,8 +348,8 @@ struct DeviceRow { #[derive(Debug, FromQueryResult)] struct SeriesRow { - series_id: Uuid, - series_name: String, + series_id: Option, + series_name: Option, measured_ms: i64, inferred_ms: i64, pages_read: i64, @@ -331,7 +360,7 @@ struct SeriesRow { #[derive(Debug, FromQueryResult)] struct FormatRow { - format: String, + format: Option, measured_ms: i64, inferred_ms: i64, pages_read: i64, @@ -501,7 +530,7 @@ impl ReadingStatsRepository { limit: u64, ) -> Result> { let backend = db.get_database_backend(); - let order_by = sort.order_by(SERIES_TITLE_SORT); + let order_by = sort.order_by(SERIES_TIEBREAK); let sql = format!( "SELECT s.id AS series_id, \ COALESCE(sm.title, s.name) AS series_name, \ @@ -512,8 +541,8 @@ impl ReadingStatsRepository { {COMPLETIONS_SUM} AS books_finished, \ COUNT(DISTINCT rs.book_id) AS books \ FROM reading_sessions rs \ - JOIN books b ON b.id = rs.book_id \ - JOIN series s ON s.id = b.series_id \ + LEFT JOIN books b ON b.id = rs.book_id \ + LEFT JOIN series s ON s.id = b.series_id \ LEFT JOIN series_metadata sm ON sm.series_id = s.id \ WHERE rs.user_id = $1 AND {READING_KINDS} \ AND rs.client_started_at >= $2 AND rs.client_started_at < $3 \ @@ -556,7 +585,7 @@ impl ReadingStatsRepository { sort: StatsSort, ) -> Result> { let backend = db.get_database_backend(); - let order_by = sort.order_by("format"); + let order_by = sort.order_by(FORMAT_TIEBREAK); let sql = format!( "SELECT b.format AS format, \ {MEASURED_SUM} AS measured_ms, \ @@ -565,7 +594,7 @@ impl ReadingStatsRepository { COUNT(*) AS sessions, \ {COMPLETIONS_SUM} AS books_finished \ FROM reading_sessions rs \ - JOIN books b ON b.id = rs.book_id \ + LEFT JOIN books b ON b.id = rs.book_id \ WHERE rs.user_id = $1 AND {READING_KINDS} \ AND rs.client_started_at >= $2 AND rs.client_started_at < $3 \ GROUP BY b.format \ @@ -635,6 +664,41 @@ impl ReadingStatsRepository { ) } + /// Delete the caller's reading history whose book has been hard-deleted, + /// and nothing else. + /// + /// Those rows keep counting towards every total until the reader decides + /// otherwise; this is that decision. It is never run on a schedule: the + /// same rows can be reattached to their books by importing an export taken + /// before the delete, and a purge makes that impossible. + /// + /// Sessions and completions go together in one transaction so the two + /// logs cannot disagree about whether the history exists. + pub async fn purge_orphaned( + db: &C, + user_id: Uuid, + ) -> Result { + let txn = db.begin().await?; + let sessions = reading_sessions::Entity::delete_many() + .filter(reading_sessions::Column::UserId.eq(user_id)) + .filter(reading_sessions::Column::BookId.is_null()) + .exec(&txn) + .await? + .rows_affected; + let completions = read_completions::Entity::delete_many() + .filter(read_completions::Column::UserId.eq(user_id)) + .filter(read_completions::Column::BookId.is_null()) + .exec(&txn) + .await? + .rows_affected; + txn.commit().await?; + + Ok(PurgedOrphanedHistory { + sessions, + completions, + }) + } + /// How many session rows a user has, for judging whether retention or /// compaction is worth building. pub async fn row_count(db: &C, user_id: Uuid) -> Result { @@ -1553,10 +1617,10 @@ mod tests { .unwrap(); assert_eq!(series.len(), 2); - assert_eq!(series[0].series_name, "Berserk"); + assert_eq!(series[0].series_name.as_deref(), Some("Berserk")); assert_eq!(series[0].duration.measured_ms, 90 * MINUTE_MS); assert_eq!(series[0].books, 1); - assert_eq!(series[1].series_name, "Vinland Saga"); + assert_eq!(series[1].series_name.as_deref(), Some("Vinland Saga")); } /// Coverage answers "which years can this reader ask for", so it must not @@ -1826,6 +1890,7 @@ mod tests { .unwrap()[0] .series_name .clone() + .unwrap_or_default() }; assert_eq!(top(StatsSort::Time).await, "Long Sitting"); @@ -1856,7 +1921,7 @@ mod tests { .await .unwrap(); - assert_eq!(series[0].series_name, "Prison School"); + assert_eq!(series[0].series_name.as_deref(), Some("Prison School")); } /// Metadata is created alongside every series today, but a missing row must @@ -1886,7 +1951,7 @@ mod tests { .unwrap(); assert_eq!(series.len(), 1); - assert_eq!(series[0].series_name, "Berserk"); + assert_eq!(series[0].series_name.as_deref(), Some("Berserk")); } #[tokio::test] @@ -1910,8 +1975,8 @@ mod tests { .unwrap(); assert_eq!(series.len(), 2); - assert_eq!(series[0].series_name, "A"); - assert_eq!(series[1].series_name, "B"); + assert_eq!(series[0].series_name.as_deref(), Some("A")); + assert_eq!(series[1].series_name.as_deref(), Some("B")); } #[tokio::test] @@ -1942,9 +2007,9 @@ mod tests { .unwrap(); assert_eq!(formats.len(), 2); - assert_eq!(formats[0].format, "cbz"); + assert_eq!(formats[0].format.as_deref(), Some("cbz")); assert_eq!(formats[0].duration.measured_ms, 90 * MINUTE_MS); - assert_eq!(formats[1].format, "epub"); + assert_eq!(formats[1].format.as_deref(), Some("epub")); } /// Pages are summed independently of time, so a client that reports one and diff --git a/docs/api/openapi.json b/docs/api/openapi.json index c616015f6..cc919876d 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -10434,6 +10434,42 @@ ] } }, + "/api/v1/reading-stats/orphaned": { + "delete": { + "tags": [ + "Reading Statistics" + ], + "summary": "Delete the caller's reading history for books that no longer exist", + "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible, and it forecloses the other way out: importing a reading\nprogress export taken before the delete puts those sessions back on their\nbooks. Only history already detached from any book is touched; attributed\nreading is never affected.", + "operationId": "purge_orphaned_reading_history", + "responses": { + "200": { + "description": "What was deleted", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PurgedOrphanedHistoryDto" + } + } + } + }, + "401": { + "description": "Unauthorized" + }, + "403": { + "description": "Forbidden" + } + }, + "security": [ + { + "jwt_bearer": [] + }, + { + "api_key": [] + } + ] + } + }, "/api/v1/readlists": { "get": { "tags": [ @@ -38682,6 +38718,28 @@ } } }, + "PurgedOrphanedHistoryDto": { + "type": "object", + "description": "What purging the removed-from-library history deleted.", + "required": [ + "sessionsRemoved", + "completionsRemoved" + ], + "properties": { + "completionsRemoved": { + "type": "integer", + "format": "int64", + "description": "Finished read-throughs deleted.", + "minimum": 0 + }, + "sessionsRemoved": { + "type": "integer", + "format": "int64", + "description": "Reading sessions deleted. Their time no longer counts anywhere.", + "minimum": 0 + } + } + }, "QueueHealthMetricsDto": { "type": "object", "description": "Queue health metrics", @@ -39002,7 +39060,7 @@ "type": "object", "description": "Totals for one file format.", "required": [ - "format", + "removedFromLibrary", "duration", "pagesRead", "sessions", @@ -39017,13 +39075,21 @@ "$ref": "#/components/schemas/DurationBreakdownDto" }, "format": { - "type": "string", + "type": [ + "string", + "null" + ], + "description": "Null on the removed-from-library row.", "example": "cbz" }, "pagesRead": { "type": "integer", "format": "int64" }, + "removedFromLibrary": { + "type": "boolean", + "description": "True on the single row that gathers reading whose book has since been\ndeleted from the server, and whose format is therefore unknown." + }, "sessions": { "type": "integer", "format": "int64" @@ -39034,8 +39100,7 @@ "type": "object", "description": "Totals for one series, most-read first.", "required": [ - "seriesId", - "seriesName", + "removedFromLibrary", "duration", "pagesRead", "sessions", @@ -39060,12 +39125,24 @@ "type": "integer", "format": "int64" }, + "removedFromLibrary": { + "type": "boolean", + "description": "True on the single row that gathers reading whose book has since been\ndeleted from the server. That time still counts towards every total,\nbut which series it belonged to is no longer known. Its `books` and\n`booksFinished` are always 0: both count distinct books, and deleted\nbooks cannot be told apart." + }, "seriesId": { - "type": "string", - "format": "uuid" + "type": [ + "string", + "null" + ], + "format": "uuid", + "description": "Null on the removed-from-library row." }, "seriesName": { - "type": "string", + "type": [ + "string", + "null" + ], + "description": "Null on the removed-from-library row.", "example": "Berserk" }, "sessions": { diff --git a/tests/api/reading_stats.rs b/tests/api/reading_stats.rs index 9b5957845..44494c1fd 100644 --- a/tests/api/reading_stats.rs +++ b/tests/api/reading_stats.rs @@ -4,7 +4,7 @@ mod common; use chrono::{DateTime, Duration, TimeZone, Utc}; -use codex::api::routes::v1::dto::ReadingStatsResponse; +use codex::api::routes::v1::dto::{PurgedOrphanedHistoryDto, ReadingStatsResponse}; use codex::db::ScanningStrategy; use codex::db::repositories::{ BookRepository, LibraryRepository, SeriesRepository, UserRepository, @@ -191,8 +191,8 @@ async fn one_response_carries_every_breakdown() { assert_eq!(stats.formats.len(), 2); assert_eq!(stats.devices[0].device_id, "phone", "ranked by time read"); - assert_eq!(stats.series[0].series_name, "Berserk"); - assert_eq!(stats.formats[0].format, "cbz"); + assert_eq!(stats.series[0].series_name.as_deref(), Some("Berserk")); + assert_eq!(stats.formats[0].format.as_deref(), Some("cbz")); } /// A reader with no history gets an empty dashboard rather than an error. @@ -364,6 +364,7 @@ async fn the_sort_key_decides_which_series_survive_the_limit() { body.expect("expected a JSON body").series[0] .series_name .clone() + .unwrap_or_default() } }; @@ -638,3 +639,141 @@ async fn the_response_reports_the_window_it_used() { "the default window covers roughly the last ninety days" ); } + +// ============================================================================ +// Reading whose book was deleted +// ============================================================================ + +async fn hard_delete_book(db: &sea_orm::DatabaseConnection, book_id: Uuid) { + use sea_orm::EntityTrait; + codex::db::entities::books::Entity::delete_by_id(book_id) + .exec(db) + .await + .unwrap(); +} + +async fn orphaned_completions(db: &sea_orm::DatabaseConnection, user_id: Uuid) -> u64 { + use codex::db::entities::read_completions; + use sea_orm::{ColumnTrait, EntityTrait, PaginatorTrait, QueryFilter}; + read_completions::Entity::find() + .filter(read_completions::Column::UserId.eq(user_id)) + .filter(read_completions::Column::BookId.is_null()) + .count(db) + .await + .unwrap() +} + +async fn purge_orphans( + state: std::sync::Arc, + token: &str, +) -> (StatusCode, Option) { + let app = create_test_router(state).await; + let request = delete_request_with_auth("/api/v1/reading-stats/orphaned", token); + make_json_request(app, request).await +} + +/// Reading a deleted book keeps counting, and the series and format panels +/// show it as one flagged row rather than dropping it or naming a null series. +#[tokio::test] +async fn deleted_books_reading_shows_as_one_flagged_row() { + let (db, _temp_dir) = setup_test_db().await; + let kept = book_in_series(&db, "Berserk", "cbz").await; + let removed = book_in_series(&db, "Dune", "epub").await; + let state = create_test_auth_state(db.clone()).await; + let (_user_id, token) = admin_and_token(&db, &state, "reader").await; + + record_session(state.clone(), &token, kept.id, "phone", 60, at(3)).await; + record_session(state.clone(), &token, removed.id, "phone", 20, at(4)).await; + hard_delete_book(&db, removed.id).await; + + let window = format!("?from={}&to={}", q(at(1)), q(at(30))); + let (status, response) = fetch_stats(state, &token, &window).await; + let stats = response.expect("expected a JSON body"); + assert_eq!(status, StatusCode::OK); + + assert_eq!( + stats.summary.duration.total_ms, + 80 * MINUTE_MS, + "the deleted book's time still counts" + ); + + assert_eq!(stats.series.len(), 2); + assert!(!stats.series[0].removed_from_library); + assert_eq!(stats.series[0].series_name.as_deref(), Some("Berserk")); + let bucket = &stats.series[1]; + assert!(bucket.removed_from_library); + assert!(bucket.series_id.is_none()); + assert!(bucket.series_name.is_none()); + assert_eq!(bucket.duration.total_ms, 20 * MINUTE_MS); + + let formats: Vec<_> = stats + .formats + .iter() + .map(|f| (f.format.clone(), f.removed_from_library)) + .collect(); + assert_eq!( + formats, + vec![(Some("cbz".to_string()), false), (None, true)], + "the epub is gone, so its time is in the removed row, ranked like any other" + ); +} + +/// Purging removes exactly the caller's orphaned history: their attributed +/// reading stays, and another reader's orphans of the same book are untouched. +#[tokio::test] +async fn purging_orphaned_history_is_scoped_to_the_caller() { + let (db, _temp_dir) = setup_test_db().await; + let kept = book_in_series(&db, "Berserk", "cbz").await; + let removed = book_in_series(&db, "Dune", "epub").await; + let state = create_test_auth_state(db.clone()).await; + let (purger, purger_token) = admin_and_token(&db, &state, "purger").await; + let (_bystander, bystander_token) = admin_and_token(&db, &state, "bystander").await; + + record_session(state.clone(), &purger_token, kept.id, "phone", 60, at(3)).await; + record_reading( + state.clone(), + &purger_token, + removed.id, + "phone", + 20, + 10, + "completed", + at(4), + ) + .await; + record_session( + state.clone(), + &bystander_token, + removed.id, + "tablet", + 15, + at(5), + ) + .await; + hard_delete_book(&db, removed.id).await; + + let completions = orphaned_completions(&db, purger).await; + let (status, purged) = purge_orphans(state.clone(), &purger_token).await; + assert_eq!(status, StatusCode::OK); + let purged = purged.expect("expected a JSON body"); + assert_eq!(purged.sessions_removed, 1); + assert_eq!(purged.completions_removed, completions); + assert_eq!(orphaned_completions(&db, purger).await, 0); + + let window = format!("?from={}&to={}", q(at(1)), q(at(30))); + let (_, mine) = fetch_stats(state.clone(), &purger_token, &window).await; + let mine = mine.expect("expected a JSON body"); + assert_eq!(mine.summary.duration.total_ms, 60 * MINUTE_MS); + assert!(mine.series.iter().all(|s| !s.removed_from_library)); + + let (_, theirs) = fetch_stats(state.clone(), &bystander_token, &window).await; + let theirs = theirs.expect("expected a JSON body"); + assert_eq!(theirs.summary.duration.total_ms, 15 * MINUTE_MS); + assert!(theirs.series[0].removed_from_library); + + // Nothing left to purge is not an error. + let (status, again) = purge_orphans(state, &purger_token).await; + assert_eq!(status, StatusCode::OK); + let again = again.expect("expected a JSON body"); + assert_eq!((again.sessions_removed, again.completions_removed), (0, 0)); +} diff --git a/tests/db/reading_history_retention.rs b/tests/db/reading_history_retention.rs index 066171e27..6d3328fa3 100644 --- a/tests/db/reading_history_retention.rs +++ b/tests/db/reading_history_retention.rs @@ -22,7 +22,8 @@ use codex::db::entities::reading_sessions::SessionKind; use codex::db::entities::{books, read_completions, reading_sessions, users}; use codex::db::repositories::{ BookRepository, LibraryRepository, NewSession, ReadCompletionRepository, - ReadProgressRepository, SeriesRepository, UserRepository, + ReadProgressRepository, ReadingStatsRepository, SeriesRepository, StatsGranularity, StatsSort, + StatsWindow, UserRepository, }; use common::*; use migration::{Migrator, MigratorTrait}; @@ -174,12 +175,160 @@ async fn exercise_user_delete_still_removes_history(db: &DatabaseConnection) { ); } +/// Everything the statistics page shows that is a plain sum over sessions. +/// +/// `books` and `books_finished` are deliberately absent: both count distinct +/// books, and a book that no longer exists cannot be told apart from another +/// deleted one, so those two figures lose the deleted book's contribution by +/// design. Time, pages and sittings are additive per row and must not move. +#[derive(Debug, PartialEq)] +struct AdditiveTotals { + summary: (i64, i64, i64, i64), + periods: Vec<(String, i64, i64, i64)>, + devices: Vec<(String, i64, i64, i64)>, + first_read_at: Option>, + last_read_at: Option>, +} + +fn whole_history() -> StatsWindow { + StatsWindow { + from: Utc::now() - Duration::days(30), + to: Utc::now() + Duration::days(1), + } +} + +async fn additive_totals(db: &DatabaseConnection, user: Uuid) -> AdditiveTotals { + let window = whole_history(); + let summary = ReadingStatsRepository::summary(db, user, window) + .await + .unwrap(); + let periods = ReadingStatsRepository::by_period(db, user, window, StatsGranularity::Day, 0) + .await + .unwrap(); + let devices = ReadingStatsRepository::by_device(db, user, window, StatsSort::Time) + .await + .unwrap(); + let coverage = ReadingStatsRepository::coverage(db, user).await.unwrap(); + + AdditiveTotals { + summary: ( + summary.duration.measured_ms, + summary.duration.inferred_ms, + summary.pages_read, + summary.sessions, + ), + periods: periods + .into_iter() + .map(|p| (p.bucket, p.duration.total_ms(), p.pages_read, p.sessions)) + .collect(), + devices: devices + .into_iter() + .map(|d| (d.device_id, d.duration.total_ms(), d.pages_read, d.sessions)) + .collect(), + first_read_at: coverage.first_read_at, + last_read_at: coverage.last_read_at, + } +} + +/// A hard delete changes no additive total, and the two breakdowns that name a +/// book's series or format carry its time in one "removed from library" row +/// instead of dropping it. +async fn exercise_statistics_keep_deleted_reading(db: &DatabaseConnection) { + let user = persist_user(db).await; + let kept = persist_book(db).await; + let removed = persist_book(db).await; + record_history(db, user, kept).await; + record_history(db, user, removed).await; + + let before = additive_totals(db, user).await; + books::Entity::delete_by_id(removed).exec(db).await.unwrap(); + let after = additive_totals(db, user).await; + + assert_eq!( + after, before, + "deleting a book must not change any time, page or sitting total" + ); + + let window = whole_history(); + let series = ReadingStatsRepository::by_series(db, user, window, StatsSort::Time, 50) + .await + .unwrap(); + assert_eq!(series.len(), 2, "one real series plus the removed bucket"); + let bucket: Vec<_> = series.iter().filter(|s| s.series_id.is_none()).collect(); + assert_eq!(bucket.len(), 1, "orphans collapse into exactly one bucket"); + assert_eq!(bucket[0].duration.measured_ms, 900_000); + assert_eq!(bucket[0].pages_read, 24); + assert_eq!(bucket[0].sessions, 1); + assert!(bucket[0].series_name.is_none()); + let series_time: i64 = series.iter().map(|s| s.duration.total_ms()).sum(); + assert_eq!( + series_time, + before.summary.0 + before.summary.1, + "the series breakdown, bucket included, must add up to the summary" + ); + + let formats = ReadingStatsRepository::by_format(db, user, window, StatsSort::Time) + .await + .unwrap(); + let cbz: Vec<_> = formats + .iter() + .filter(|f| f.format.as_deref() == Some("cbz")) + .collect(); + let orphaned: Vec<_> = formats.iter().filter(|f| f.format.is_none()).collect(); + assert_eq!(cbz.len(), 1); + assert_eq!( + cbz[0].duration.measured_ms, 900_000, + "only the kept book remains cbz" + ); + assert_eq!(orphaned.len(), 1, "the deleted book's format is unknowable"); + assert_eq!(orphaned[0].duration.measured_ms, 900_000); +} + +/// Purging removes only the caller's orphans: their attributed history stays, +/// and another reader's orphans of the same book are untouched. +async fn exercise_purge_is_scoped_to_orphans_of_one_user(db: &DatabaseConnection) { + let purger = persist_user(db).await; + let bystander = persist_user(db).await; + let kept = persist_book(db).await; + let removed = persist_book(db).await; + record_history(db, purger, kept).await; + record_history(db, purger, removed).await; + record_history(db, bystander, removed).await; + books::Entity::delete_by_id(removed).exec(db).await.unwrap(); + + let purged = ReadingStatsRepository::purge_orphaned(db, purger) + .await + .unwrap(); + assert_eq!((purged.sessions, purged.completions), (1, 1)); + + let sessions = sessions_for_user(db, purger).await; + assert_eq!(sessions.len(), 1); + assert_eq!(sessions[0].book_id, Some(kept), "attributed history stays"); + assert_eq!(completions_for_user(db, purger).await.len(), 1); + + assert_eq!(sessions_for_user(db, bystander).await.len(), 1); + assert_eq!(completions_for_user(db, bystander).await.len(), 1); +} + #[tokio::test] async fn history_survives_book_delete_sqlite() { let (db, _temp_dir) = setup_test_db().await; exercise_history_survives_book_delete(&db).await; } +#[tokio::test] +async fn statistics_keep_deleted_reading_sqlite() { + let (db, _temp_dir) = setup_test_db().await; + exercise_statistics_keep_deleted_reading(&db).await; + exercise_purge_is_scoped_to_orphans_of_one_user(&db).await; +} + +#[tokio::test] +async fn purge_is_scoped_to_orphans_of_one_user_sqlite() { + let (db, _temp_dir) = setup_test_db().await; + exercise_purge_is_scoped_to_orphans_of_one_user(&db).await; +} + #[tokio::test] async fn user_delete_still_removes_history_sqlite() { let (db, _temp_dir) = setup_test_db().await; @@ -203,6 +352,7 @@ async fn reading_history_retention_postgres() { exercise_history_survives_book_delete(&db).await; exercise_user_delete_still_removes_history(&db).await; + exercise_statistics_keep_deleted_reading(&db).await; } // ============================================================================ diff --git a/tests/db/reading_stats.rs b/tests/db/reading_stats.rs index 02f90c7bc..37ad83365 100644 --- a/tests/db/reading_stats.rs +++ b/tests/db/reading_stats.rs @@ -272,9 +272,9 @@ async fn exercise_reading_stats(db: &DatabaseConnection) { .await .expect("series breakdown must decode on this engine"); assert_eq!(series.len(), 2); - assert_eq!(series[0].series_name, "Berserk"); + assert_eq!(series[0].series_name.as_deref(), Some("Berserk")); assert_eq!(series[0].duration.total_ms(), 50 * MINUTE_MS); - assert_eq!(series[1].series_name, "Dune"); + assert_eq!(series[1].series_name.as_deref(), Some("Dune")); assert_eq!( series[1].books_finished, 1, "the duplicated finish collapses in the series breakdown too" @@ -284,7 +284,7 @@ async fn exercise_reading_stats(db: &DatabaseConnection) { .await .expect("format breakdown must decode on this engine"); assert_eq!(formats.len(), 2); - assert_eq!(formats[0].format, "cbz"); + assert_eq!(formats[0].format.as_deref(), Some("cbz")); // Each ranking key is a different `ORDER BY` over aggregate expressions, // and PostgreSQL is the strict one about what may appear there. Ranking by diff --git a/web/openapi.json b/web/openapi.json index c616015f6..cc919876d 100644 --- a/web/openapi.json +++ b/web/openapi.json @@ -10434,6 +10434,42 @@ ] } }, + "/api/v1/reading-stats/orphaned": { + "delete": { + "tags": [ + "Reading Statistics" + ], + "summary": "Delete the caller's reading history for books that no longer exist", + "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible, and it forecloses the other way out: importing a reading\nprogress export taken before the delete puts those sessions back on their\nbooks. Only history already detached from any book is touched; attributed\nreading is never affected.", + "operationId": "purge_orphaned_reading_history", + "responses": { + "200": { + "description": "What was deleted", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PurgedOrphanedHistoryDto" + } + } + } + }, + "401": { + "description": "Unauthorized" + }, + "403": { + "description": "Forbidden" + } + }, + "security": [ + { + "jwt_bearer": [] + }, + { + "api_key": [] + } + ] + } + }, "/api/v1/readlists": { "get": { "tags": [ @@ -38682,6 +38718,28 @@ } } }, + "PurgedOrphanedHistoryDto": { + "type": "object", + "description": "What purging the removed-from-library history deleted.", + "required": [ + "sessionsRemoved", + "completionsRemoved" + ], + "properties": { + "completionsRemoved": { + "type": "integer", + "format": "int64", + "description": "Finished read-throughs deleted.", + "minimum": 0 + }, + "sessionsRemoved": { + "type": "integer", + "format": "int64", + "description": "Reading sessions deleted. Their time no longer counts anywhere.", + "minimum": 0 + } + } + }, "QueueHealthMetricsDto": { "type": "object", "description": "Queue health metrics", @@ -39002,7 +39060,7 @@ "type": "object", "description": "Totals for one file format.", "required": [ - "format", + "removedFromLibrary", "duration", "pagesRead", "sessions", @@ -39017,13 +39075,21 @@ "$ref": "#/components/schemas/DurationBreakdownDto" }, "format": { - "type": "string", + "type": [ + "string", + "null" + ], + "description": "Null on the removed-from-library row.", "example": "cbz" }, "pagesRead": { "type": "integer", "format": "int64" }, + "removedFromLibrary": { + "type": "boolean", + "description": "True on the single row that gathers reading whose book has since been\ndeleted from the server, and whose format is therefore unknown." + }, "sessions": { "type": "integer", "format": "int64" @@ -39034,8 +39100,7 @@ "type": "object", "description": "Totals for one series, most-read first.", "required": [ - "seriesId", - "seriesName", + "removedFromLibrary", "duration", "pagesRead", "sessions", @@ -39060,12 +39125,24 @@ "type": "integer", "format": "int64" }, + "removedFromLibrary": { + "type": "boolean", + "description": "True on the single row that gathers reading whose book has since been\ndeleted from the server. That time still counts towards every total,\nbut which series it belonged to is no longer known. Its `books` and\n`booksFinished` are always 0: both count distinct books, and deleted\nbooks cannot be told apart." + }, "seriesId": { - "type": "string", - "format": "uuid" + "type": [ + "string", + "null" + ], + "format": "uuid", + "description": "Null on the removed-from-library row." }, "seriesName": { - "type": "string", + "type": [ + "string", + "null" + ], + "description": "Null on the removed-from-library row.", "example": "Berserk" }, "sessions": { diff --git a/web/src/types/api.generated.ts b/web/src/types/api.generated.ts index 620a328c0..8aae9c3ca 100644 --- a/web/src/types/api.generated.ts +++ b/web/src/types/api.generated.ts @@ -3360,6 +3360,34 @@ export interface paths { patch?: never; trace?: never; }; + "/api/v1/reading-stats/orphaned": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + /** + * Delete the caller's reading history for books that no longer exist + * @description When a book is deleted from the server its reading sessions and finished + * read-throughs are kept, so the time still counts towards every statistic; + * the series and format breakdowns show it as one "removed from library" row. + * This discards those rows for the caller, and only for the caller. + * + * Irreversible, and it forecloses the other way out: importing a reading + * progress export taken before the delete puts those sessions back on their + * books. Only history already detached from any book is touched; attributed + * reading is never affected. + */ + delete: operations["purge_orphaned_reading_history"]; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/v1/readlists": { parameters: { query?: never; @@ -16691,6 +16719,19 @@ export interface components { */ deleted: number; }; + /** @description What purging the removed-from-library history deleted. */ + PurgedOrphanedHistoryDto: { + /** + * Format: int64 + * @description Finished read-throughs deleted. + */ + completionsRemoved: number; + /** + * Format: int64 + * @description Reading sessions deleted. Their time no longer counts anywhere. + */ + sessionsRemoved: number; + }; /** @description Queue health metrics */ QueueHealthMetricsDto: { /** @@ -16895,10 +16936,18 @@ export interface components { /** Format: int64 */ booksFinished: number; duration: components["schemas"]["DurationBreakdownDto"]; - /** @example cbz */ - format: string; + /** + * @description Null on the removed-from-library row. + * @example cbz + */ + format?: string | null; /** Format: int64 */ pagesRead: number; + /** + * @description True on the single row that gathers reading whose book has since been + * deleted from the server, and whose format is therefore unknown. + */ + removedFromLibrary: boolean; /** Format: int64 */ sessions: number; }; @@ -16918,10 +16967,24 @@ export interface components { duration: components["schemas"]["DurationBreakdownDto"]; /** Format: int64 */ pagesRead: number; - /** Format: uuid */ - seriesId: string; - /** @example Berserk */ - seriesName: string; + /** + * @description True on the single row that gathers reading whose book has since been + * deleted from the server. That time still counts towards every total, + * but which series it belonged to is no longer known. Its `books` and + * `booksFinished` are always 0: both count distinct books, and deleted + * books cannot be told apart. + */ + removedFromLibrary: boolean; + /** + * Format: uuid + * @description Null on the removed-from-library row. + */ + seriesId?: string | null; + /** + * @description Null on the removed-from-library row. + * @example Berserk + */ + seriesName?: string | null; /** Format: int64 */ sessions: number; }; @@ -29443,6 +29506,40 @@ export interface operations { }; }; }; + purge_orphaned_reading_history: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description What was deleted */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["PurgedOrphanedHistoryDto"]; + }; + }; + /** @description Unauthorized */ + 401: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Forbidden */ + 403: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; list_readlists: { parameters: { query?: never; From b5c3e0292ae779702f6897659244e593e75cacb4 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sun, 27 Sep 2026 12:05:21 -0700 Subject: [PATCH 3/4] feat(web): show reading of deleted books, and a confirmed way to delete it The series and format panels render the removedFromLibrary row as an italic "Removed from library" line with no link, and the series row carries a Delete action. The confirmation states what will actually go. The dashboard row covers only the dates on screen while a purge is not windowed, so the dialog reads its figures from a new GET /api/v1/reading-stats/orphaned, which totals every detached session and completion for the caller. The user guide explains what survives a permanent delete, why books read and books finished drop the deleted book, and how to clear it. --- crates/codex-api/src/docs.rs | 2 + .../src/routes/v1/dto/reading_stats.rs | 28 +++- .../src/routes/v1/handlers/reading_stats.rs | 45 +++++- .../codex-api/src/routes/v1/routes/books.rs | 3 +- crates/codex-db/src/repositories/mod.rs | 6 +- .../src/repositories/reading_stats.rs | 73 ++++++++- docs/api/openapi.json | 64 ++++++++ docs/docs/libraries.md | 6 + docs/docs/reading-progress.md | 35 +++++ tests/api/reading_stats.rs | 45 +++++- tests/db/reading_history_retention.rs | 8 + web/openapi.json | 64 ++++++++ web/src/api/readingStats.ts | 21 +++ .../reading/ReadingStatsPanels.test.tsx | 60 ++++++++ .../components/reading/ReadingStatsPanels.tsx | 98 +++++++++---- .../reading/RemovedHistoryPurge.test.tsx | 90 ++++++++++++ .../reading/RemovedHistoryPurge.tsx | 138 ++++++++++++++++++ web/src/mocks/handlers/readingStats.ts | 18 +++ web/src/types/api.generated.ts | 60 +++++++- 19 files changed, 824 insertions(+), 40 deletions(-) create mode 100644 web/src/components/reading/RemovedHistoryPurge.test.tsx create mode 100644 web/src/components/reading/RemovedHistoryPurge.tsx diff --git a/crates/codex-api/src/docs.rs b/crates/codex-api/src/docs.rs index c36870572..a868a5ac6 100644 --- a/crates/codex-api/src/docs.rs +++ b/crates/codex-api/src/docs.rs @@ -369,6 +369,7 @@ The following paths are exempt from rate limiting: v1::handlers::record_reading_sessions, v1::handlers::get_reading_stats, v1::handlers::get_reading_coverage, + v1::handlers::get_orphaned_reading_history, v1::handlers::purge_orphaned_reading_history, // Reading progress endpoints @@ -1047,6 +1048,7 @@ The following paths are exempt from rate limiting: v1::dto::ReadingStatsGranularity, v1::dto::ReadingStatsSort, v1::dto::ReadingCoverageDto, + v1::dto::OrphanedHistoryDto, v1::dto::PurgedOrphanedHistoryDto, // Reading session DTOs diff --git a/crates/codex-api/src/routes/v1/dto/reading_stats.rs b/crates/codex-api/src/routes/v1/dto/reading_stats.rs index 152d1d7e7..d0bb9a567 100644 --- a/crates/codex-api/src/routes/v1/dto/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/dto/reading_stats.rs @@ -6,8 +6,8 @@ use chrono::{DateTime, Utc}; use codex_db::repositories::{ - DurationBreakdown, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, ReadingBySeries, - ReadingCoverage, ReadingPeriod, ReadingSummary, StatsGranularity, StatsSort, + DurationBreakdown, OrphanedHistory, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, + ReadingBySeries, ReadingCoverage, ReadingPeriod, ReadingSummary, StatsGranularity, StatsSort, }; use serde::{Deserialize, Serialize}; use utoipa::{IntoParams, ToSchema}; @@ -341,3 +341,27 @@ impl From for PurgedOrphanedHistoryDto { } } } + +/// The caller's reading history whose book has since been deleted, across +/// every date. What a purge would remove. +#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] +#[serde(rename_all = "camelCase")] +pub struct OrphanedHistoryDto { + pub duration: DurationBreakdownDto, + pub pages_read: i64, + /// Session rows, including bookkeeping rows such as a mark-unread. + pub sessions: i64, + /// Finished read-throughs. + pub completions: u64, +} + +impl From for OrphanedHistoryDto { + fn from(value: OrphanedHistory) -> Self { + Self { + duration: value.duration.into(), + pages_read: value.pages_read, + sessions: value.sessions, + completions: value.completions, + } + } +} diff --git a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs index 25e0e4827..37caf8208 100644 --- a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs @@ -1,9 +1,10 @@ //! Reading statistics, aggregated from the session log. use super::super::dto::{ - DurationBreakdownDto, PurgedOrphanedHistoryDto, ReadingByDeviceDto, ReadingByFormatDto, - ReadingBySeriesDto, ReadingCoverageDto, ReadingPeriodDto, ReadingStatsGranularity, - ReadingStatsQuery, ReadingStatsResponse, ReadingStatsSort, ReadingSummaryDto, + DurationBreakdownDto, OrphanedHistoryDto, PurgedOrphanedHistoryDto, ReadingByDeviceDto, + ReadingByFormatDto, ReadingBySeriesDto, ReadingCoverageDto, ReadingPeriodDto, + ReadingStatsGranularity, ReadingStatsQuery, ReadingStatsResponse, ReadingStatsSort, + ReadingSummaryDto, }; use crate::{AppState, error::ApiError, extractors::AuthContext, permissions::Permission}; use axum::{ @@ -34,7 +35,11 @@ const MAX_TZ_OFFSET_MINUTES: i32 = 14 * 60; #[derive(OpenApi)] #[openapi( - paths(get_reading_stats, purge_orphaned_reading_history), + paths( + get_reading_stats, + get_orphaned_reading_history, + purge_orphaned_reading_history + ), components(schemas( ReadingStatsResponse, ReadingSummaryDto, @@ -44,6 +49,7 @@ const MAX_TZ_OFFSET_MINUTES: i32 = 14 * 60; ReadingByFormatDto, DurationBreakdownDto, ReadingStatsGranularity, + OrphanedHistoryDto, PurgedOrphanedHistoryDto, )), tags( @@ -188,6 +194,37 @@ pub async fn get_reading_coverage( Ok(Json(coverage.into())) } +/// The caller's reading history for books that no longer exist, in total +/// +/// Unwindowed: exactly what `DELETE /api/v1/reading-stats/orphaned` would +/// remove, so a client can say so before asking the reader to confirm. +#[utoipa::path( + get, + path = "/api/v1/reading-stats/orphaned", + responses( + (status = 200, description = "Totals of the detached history", body = OrphanedHistoryDto), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Forbidden"), + ), + security( + ("jwt_bearer" = []), + ("api_key" = []) + ), + tag = "Reading Statistics" +)] +pub async fn get_orphaned_reading_history( + State(state): State>, + auth: AuthContext, +) -> Result, ApiError> { + auth.require_permission(&Permission::ProgressRead)?; + + let totals = ReadingStatsRepository::orphaned_totals(&state.db, auth.user_id) + .await + .map_err(|e| ApiError::Internal(format!("Failed to total orphaned history: {}", e)))?; + + Ok(Json(totals.into())) +} + /// Delete the caller's reading history for books that no longer exist /// /// When a book is deleted from the server its reading sessions and finished diff --git a/crates/codex-api/src/routes/v1/routes/books.rs b/crates/codex-api/src/routes/v1/routes/books.rs index 469d74f21..b83210204 100644 --- a/crates/codex-api/src/routes/v1/routes/books.rs +++ b/crates/codex-api/src/routes/v1/routes/books.rs @@ -190,7 +190,8 @@ pub fn routes(_state: Arc) -> Router> { // otherwise. .route( "/reading-stats/orphaned", - delete(handlers::purge_orphaned_reading_history), + get(handlers::get_orphaned_reading_history) + .delete(handlers::purge_orphaned_reading_history), ) // Mark as read/unread routes .route("/books/{book_id}/read", post(handlers::mark_book_as_read)) diff --git a/crates/codex-db/src/repositories/mod.rs b/crates/codex-db/src/repositories/mod.rs index 366be3850..637169419 100644 --- a/crates/codex-db/src/repositories/mod.rs +++ b/crates/codex-db/src/repositories/mod.rs @@ -97,9 +97,9 @@ pub use read_progress::ReadProgressRepository; pub use reading_sessions::{AppendOutcome, DeviceContext, NewSession, ReadingSessionRepository}; #[allow(unused_imports)] pub use reading_stats::{ - DurationBreakdown, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, ReadingBySeries, - ReadingCoverage, ReadingPeriod, ReadingStatsRepository, ReadingSummary, StatsGranularity, - StatsSort, StatsWindow, + DurationBreakdown, OrphanedHistory, PurgedOrphanedHistory, ReadingByDevice, ReadingByFormat, + ReadingBySeries, ReadingCoverage, ReadingPeriod, ReadingStatsRepository, ReadingSummary, + StatsGranularity, StatsSort, StatsWindow, }; #[allow(unused_imports)] pub use refresh_token::{NewRefreshToken, RefreshTokenRepository}; diff --git a/crates/codex-db/src/repositories/reading_stats.rs b/crates/codex-db/src/repositories/reading_stats.rs index 7a0d1a998..e598111cc 100644 --- a/crates/codex-db/src/repositories/reading_stats.rs +++ b/crates/codex-db/src/repositories/reading_stats.rs @@ -21,8 +21,8 @@ use crate::entities::{read_completions, reading_sessions}; use anyhow::Result; use chrono::{DateTime, Utc}; use sea_orm::{ - ColumnTrait, ConnectionTrait, DatabaseBackend, EntityTrait, FromQueryResult, QueryFilter, - Statement, TransactionTrait, Value, + ColumnTrait, ConnectionTrait, DatabaseBackend, EntityTrait, FromQueryResult, PaginatorTrait, + QueryFilter, Statement, TransactionTrait, Value, }; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -222,6 +222,15 @@ pub struct ReadingCoverage { pub last_read_at: Option>, } +/// The caller's reading history whose book has been hard-deleted, in total. +#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct OrphanedHistory { + pub duration: DurationBreakdown, + pub pages_read: i64, + pub sessions: i64, + pub completions: u64, +} + /// What [`ReadingStatsRepository::purge_orphaned`] removed. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct PurgedOrphanedHistory { @@ -664,6 +673,66 @@ impl ReadingStatsRepository { ) } + /// Everything [`Self::purge_orphaned`] would delete for this user, across + /// their whole history rather than any window. + /// + /// A purge is not windowed, so the confirmation in front of it cannot be + /// either: the removed-from-library row on the dashboard covers only the + /// dates on screen and would understate what is about to go. + pub async fn orphaned_totals( + db: &C, + user_id: Uuid, + ) -> Result { + #[derive(Debug, FromQueryResult)] + struct OrphanRow { + measured_ms: i64, + inferred_ms: i64, + pages_read: i64, + sessions: i64, + } + + let backend = db.get_database_backend(); + // Counts every orphaned row, reading or not: a purge removes `reset` + // rows too, and the count has to match what it reports afterwards. + let sql = format!( + "SELECT {MEASURED_SUM} AS measured_ms, \ + {INFERRED_SUM} AS inferred_ms, \ + {PAGES_SUM} AS pages_read, \ + COUNT(*) AS sessions \ + FROM reading_sessions rs \ + WHERE rs.user_id = $1 AND rs.book_id IS NULL" + ); + let row = OrphanRow::find_by_statement(Statement::from_sql_and_values( + backend, + &sql, + [Value::Uuid(Some(Box::new(user_id)))], + )) + .one(db) + .await?; + + let completions = read_completions::Entity::find() + .filter(read_completions::Column::UserId.eq(user_id)) + .filter(read_completions::Column::BookId.is_null()) + .count(db) + .await?; + + Ok(row.map_or_else( + || OrphanedHistory { + completions, + ..OrphanedHistory::default() + }, + |r| OrphanedHistory { + duration: DurationBreakdown { + measured_ms: r.measured_ms, + inferred_ms: r.inferred_ms, + }, + pages_read: r.pages_read, + sessions: r.sessions, + completions, + }, + )) + } + /// Delete the caller's reading history whose book has been hard-deleted, /// and nothing else. /// diff --git a/docs/api/openapi.json b/docs/api/openapi.json index cc919876d..29cd9c5a9 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -10435,6 +10435,40 @@ } }, "/api/v1/reading-stats/orphaned": { + "get": { + "tags": [ + "Reading Statistics" + ], + "summary": "The caller's reading history for books that no longer exist, in total", + "description": "Unwindowed: exactly what `DELETE /api/v1/reading-stats/orphaned` would\nremove, so a client can say so before asking the reader to confirm.", + "operationId": "get_orphaned_reading_history", + "responses": { + "200": { + "description": "Totals of the detached history", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/OrphanedHistoryDto" + } + } + } + }, + "401": { + "description": "Unauthorized" + }, + "403": { + "description": "Forbidden" + } + }, + "security": [ + { + "jwt_bearer": [] + }, + { + "api_key": [] + } + ] + }, "delete": { "tags": [ "Reading Statistics" @@ -35626,6 +35660,36 @@ } } }, + "OrphanedHistoryDto": { + "type": "object", + "description": "The caller's reading history whose book has since been deleted, across\nevery date. What a purge would remove.", + "required": [ + "duration", + "pagesRead", + "sessions", + "completions" + ], + "properties": { + "completions": { + "type": "integer", + "format": "int64", + "description": "Finished read-throughs.", + "minimum": 0 + }, + "duration": { + "$ref": "#/components/schemas/DurationBreakdownDto" + }, + "pagesRead": { + "type": "integer", + "format": "int64" + }, + "sessions": { + "type": "integer", + "format": "int64", + "description": "Session rows, including bookkeeping rows such as a mark-unread." + } + } + }, "PageDto": { "type": "object", "description": "Page data transfer object", diff --git a/docs/docs/libraries.md b/docs/docs/libraries.md index ff3e2f101..f0b088d6d 100644 --- a/docs/docs/libraries.md +++ b/docs/docs/libraries.md @@ -300,6 +300,12 @@ Deleted files are soft-deleted in the database: - Can be restored if file returns - Permanent deletion available via API +Permanent deletion (purging deleted books, or deleting the library) removes the +book for good, including your current progress in it. Your reading time and +finished read-throughs are kept and still count in your statistics, under a +**Removed from library** line; see +[Reading Progress](./reading-progress.md#when-a-book-is-deleted-from-the-server). + ### Moving Files If you move files: diff --git a/docs/docs/reading-progress.md b/docs/docs/reading-progress.md index ab7572b09..8ee4f5008 100644 --- a/docs/docs/reading-progress.md +++ b/docs/docs/reading-progress.md @@ -114,6 +114,41 @@ whichever book is holding the count up updates the series straight away. Since the series number is the *lowest* count across its books, it only moves if that book was the one setting it. +## When a book is deleted from the server + +A scan that finds a file missing only hides the book (a soft delete), and +everything about it, progress and history included, comes back if the file +does. A **permanent** deletion is different: purging deleted books, or deleting +a whole library, removes the book's row for good. + +What happens to your reading then: + +- **Your reading time, pages and sittings are kept.** They still count towards + every total on the **Reading Statistics** page, the calendar and the + per-device breakdown. The time happened; removing the file does not undo it. +- **Which book it was is lost.** The series and format panels show that time on + one row labelled **Removed from library**, since there is no longer a series + or format to put it under. +- **Books read and books finished drop the deleted book.** Both count distinct + books, and two deleted books can no longer be told apart, so they are left + out rather than guessed at. Time, pages and sittings are unaffected. +- **Current progress is removed.** Where you were in a book that no longer + exists has nothing to resume, so your position in it goes with the book. + Your finished read-throughs are kept, detached from the book, like your + sittings. + +### Deleting the removed-from-library history + +If you would rather that reading stopped counting, use **Delete** on the +**Removed from library** row of the series panel. The confirmation states how +many sittings, how much reading time and how many finished read-throughs will +go, across **all** dates rather than only the ones on screen, because that is +what is deleted. It is permanent, affects only your own history, and never +touches reading attributed to a book that still exists. + +The same is available through the API: `GET /api/v1/reading-stats/orphaned` +returns the totals, and `DELETE /api/v1/reading-stats/orphaned` removes them. + ## What is not recorded - **Abandoned reads leave no trace.** Marking a book unread at page 50 without diff --git a/tests/api/reading_stats.rs b/tests/api/reading_stats.rs index 44494c1fd..5e7597abe 100644 --- a/tests/api/reading_stats.rs +++ b/tests/api/reading_stats.rs @@ -4,7 +4,9 @@ mod common; use chrono::{DateTime, Duration, TimeZone, Utc}; -use codex::api::routes::v1::dto::{PurgedOrphanedHistoryDto, ReadingStatsResponse}; +use codex::api::routes::v1::dto::{ + OrphanedHistoryDto, PurgedOrphanedHistoryDto, ReadingStatsResponse, +}; use codex::db::ScanningStrategy; use codex::db::repositories::{ BookRepository, LibraryRepository, SeriesRepository, UserRepository, @@ -777,3 +779,44 @@ async fn purging_orphaned_history_is_scoped_to_the_caller() { let again = again.expect("expected a JSON body"); assert_eq!((again.sessions_removed, again.completions_removed), (0, 0)); } + +/// The confirmation before a purge has to state what will actually go, which +/// is every orphaned row regardless of the window the dashboard is showing. +#[tokio::test] +async fn orphaned_totals_ignore_the_window_and_the_other_reader() { + let (db, _temp_dir) = setup_test_db().await; + let removed = book_in_series(&db, "Dune", "epub").await; + let state = create_test_auth_state(db.clone()).await; + let (_reader, token) = admin_and_token(&db, &state, "reader").await; + let (_other, other_token) = admin_and_token(&db, &state, "other").await; + + // Two months apart, so no single dashboard window shows both. + record_session(state.clone(), &token, removed.id, "phone", 20, at(3)).await; + record_session( + state.clone(), + &token, + removed.id, + "phone", + 40, + at(3) - Duration::days(60), + ) + .await; + record_session(state.clone(), &other_token, removed.id, "tablet", 5, at(4)).await; + hard_delete_book(&db, removed.id).await; + + let app = create_test_router(state.clone()).await; + let request = get_request_with_auth("/api/v1/reading-stats/orphaned", &token); + let (status, body): (_, Option) = make_json_request(app, request).await; + assert_eq!(status, StatusCode::OK); + let body = body.expect("expected a JSON body"); + assert_eq!(body.sessions, 2); + assert_eq!(body.duration.total_ms, 60 * MINUTE_MS); + assert_eq!(body.completions, 0); + + purge_orphans(state.clone(), &token).await; + let app = create_test_router(state).await; + let request = get_request_with_auth("/api/v1/reading-stats/orphaned", &token); + let (_, body): (_, Option) = make_json_request(app, request).await; + let body = body.expect("expected a JSON body"); + assert_eq!((body.sessions, body.duration.total_ms), (0, 0)); +} diff --git a/tests/db/reading_history_retention.rs b/tests/db/reading_history_retention.rs index 6d3328fa3..4449486bc 100644 --- a/tests/db/reading_history_retention.rs +++ b/tests/db/reading_history_retention.rs @@ -282,6 +282,14 @@ async fn exercise_statistics_keep_deleted_reading(db: &DatabaseConnection) { ); assert_eq!(orphaned.len(), 1, "the deleted book's format is unknowable"); assert_eq!(orphaned[0].duration.measured_ms, 900_000); + + let totals = ReadingStatsRepository::orphaned_totals(db, user) + .await + .unwrap(); + assert_eq!(totals.duration.measured_ms, 900_000); + assert_eq!(totals.pages_read, 24); + assert_eq!(totals.sessions, 1); + assert_eq!(totals.completions, 1); } /// Purging removes only the caller's orphans: their attributed history stays, diff --git a/web/openapi.json b/web/openapi.json index cc919876d..29cd9c5a9 100644 --- a/web/openapi.json +++ b/web/openapi.json @@ -10435,6 +10435,40 @@ } }, "/api/v1/reading-stats/orphaned": { + "get": { + "tags": [ + "Reading Statistics" + ], + "summary": "The caller's reading history for books that no longer exist, in total", + "description": "Unwindowed: exactly what `DELETE /api/v1/reading-stats/orphaned` would\nremove, so a client can say so before asking the reader to confirm.", + "operationId": "get_orphaned_reading_history", + "responses": { + "200": { + "description": "Totals of the detached history", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/OrphanedHistoryDto" + } + } + } + }, + "401": { + "description": "Unauthorized" + }, + "403": { + "description": "Forbidden" + } + }, + "security": [ + { + "jwt_bearer": [] + }, + { + "api_key": [] + } + ] + }, "delete": { "tags": [ "Reading Statistics" @@ -35626,6 +35660,36 @@ } } }, + "OrphanedHistoryDto": { + "type": "object", + "description": "The caller's reading history whose book has since been deleted, across\nevery date. What a purge would remove.", + "required": [ + "duration", + "pagesRead", + "sessions", + "completions" + ], + "properties": { + "completions": { + "type": "integer", + "format": "int64", + "description": "Finished read-throughs.", + "minimum": 0 + }, + "duration": { + "$ref": "#/components/schemas/DurationBreakdownDto" + }, + "pagesRead": { + "type": "integer", + "format": "int64" + }, + "sessions": { + "type": "integer", + "format": "int64", + "description": "Session rows, including bookkeeping rows such as a mark-unread." + } + } + }, "PageDto": { "type": "object", "description": "Page data transfer object", diff --git a/web/src/api/readingStats.ts b/web/src/api/readingStats.ts index 228fa8df2..5e8420a69 100644 --- a/web/src/api/readingStats.ts +++ b/web/src/api/readingStats.ts @@ -14,6 +14,9 @@ export type ReadingStatsGranularity = components["schemas"]["ReadingStatsGranularity"]; export type ReadingStatsSort = components["schemas"]["ReadingStatsSort"]; export type ReadingCoverage = components["schemas"]["ReadingCoverageDto"]; +export type OrphanedHistory = components["schemas"]["OrphanedHistoryDto"]; +export type PurgedOrphanedHistory = + components["schemas"]["PurgedOrphanedHistoryDto"]; export interface ReadingStatsParams { from?: Date; @@ -69,4 +72,22 @@ export const readingStatsApi = { const response = await api.get("/reading-stats/coverage"); return response.data; }, + + /** + * Totals of the reader's history whose book has since been deleted, across + * every date. Exactly what `purgeOrphaned` would remove, which the windowed + * "removed from library" row cannot say. + */ + orphaned: async (): Promise => { + const response = await api.get("/reading-stats/orphaned"); + return response.data; + }, + + /** Permanently delete the reader's history whose book has been deleted. */ + purgeOrphaned: async (): Promise => { + const response = await api.delete( + "/reading-stats/orphaned", + ); + return response.data; + }, }; diff --git a/web/src/components/reading/ReadingStatsPanels.test.tsx b/web/src/components/reading/ReadingStatsPanels.test.tsx index c980daa7e..e85e2beb6 100644 --- a/web/src/components/reading/ReadingStatsPanels.test.tsx +++ b/web/src/components/reading/ReadingStatsPanels.test.tsx @@ -124,6 +124,7 @@ describe("TopSeries", () => { { seriesId: "11111111-1111-1111-1111-111111111111", seriesName: "Berserk", + removedFromLibrary: false, duration: duration(2 * HOUR), pagesRead: 120, sessions: 4, @@ -133,6 +134,7 @@ describe("TopSeries", () => { { seriesId: "22222222-2222-2222-2222-222222222222", seriesName: "Vinland Saga", + removedFromLibrary: false, duration: duration(30 * MINUTE), pagesRead: 40, sessions: 1, @@ -156,6 +158,37 @@ describe("TopSeries", () => { expect(screen.getByText("40 pages across 1 book")).toBeInTheDocument(); }); + /// Reading of deleted books still counts, but it is not a series: it must + /// not link anywhere or claim a book count it cannot know. + it("shows reading of deleted books as a labelled row, not a series", () => { + renderWithProviders( + , + ); + + const label = screen.getByText("Removed from library"); + expect(label.closest("a")).toBeNull(); + expect(screen.getByText("45m")).toBeInTheDocument(); + expect( + screen.getByText("3 sittings of books no longer on the server"), + ).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Delete" })).toBeInTheDocument(); + expect(screen.getAllByRole("link")).toHaveLength(2); + }); + it("says so plainly when nothing was read", () => { renderWithProviders(); @@ -224,6 +257,7 @@ describe("empty rows", () => { { seriesId: "11111111-1111-1111-1111-111111111111", seriesName: "Berserk", + removedFromLibrary: false, duration: duration(2 * HOUR), pagesRead: 120, sessions: 4, @@ -232,6 +266,7 @@ describe("empty rows", () => { { seriesId: "22222222-2222-2222-2222-222222222222", seriesName: "Imported Series", + removedFromLibrary: false, duration: duration(0), pagesRead: 0, sessions: 6, @@ -252,6 +287,7 @@ describe("empty rows", () => { { seriesId: "22222222-2222-2222-2222-222222222222", seriesName: "Imported Series", + removedFromLibrary: false, duration: duration(0), pagesRead: 0, sessions: 6, @@ -300,6 +336,7 @@ describe("empty rows", () => { formats={[ { format: "cbz", + removedFromLibrary: false, duration: duration(90 * MINUTE), pagesRead: 60, sessions: 3, @@ -307,6 +344,7 @@ describe("empty rows", () => { }, { format: "pdf", + removedFromLibrary: false, duration: duration(0), pagesRead: 0, sessions: 0, @@ -321,12 +359,33 @@ describe("empty rows", () => { expect(screen.getByText("1h 30m")).toBeInTheDocument(); }); + it("names the deleted books' format row instead of printing nothing", () => { + renderWithProviders( + , + ); + + expect(screen.getByText("Removed from library")).toBeInTheDocument(); + expect(screen.getByText("20m")).toBeInTheDocument(); + }); + it("renders no format panel at all when every format is empty", () => { renderWithProviders( { { seriesId: "11111111-1111-1111-1111-111111111111", seriesName: "Berserk", + removedFromLibrary: false, duration: duration(2 * HOUR), pagesRead: 420, sessions: 4, diff --git a/web/src/components/reading/ReadingStatsPanels.tsx b/web/src/components/reading/ReadingStatsPanels.tsx index 3e0b31c4b..42a59bac8 100644 --- a/web/src/components/reading/ReadingStatsPanels.tsx +++ b/web/src/components/reading/ReadingStatsPanels.tsx @@ -11,7 +11,7 @@ */ import { Anchor, Box, Group, Paper, Stack, Text, Tooltip } from "@mantine/core"; -import { useLayoutEffect, useRef } from "react"; +import { type ReactNode, useLayoutEffect, useRef } from "react"; import { Link } from "react-router-dom"; import type { ReadingByDeviceDto, @@ -20,6 +20,7 @@ import type { } from "@/api/readingStats"; import type { ReadingMetric } from "@/store/readingStatsPreferencesStore"; import classes from "./ReadingStatsCharts.module.css"; +import { RemovedHistoryPurge } from "./RemovedHistoryPurge"; import { type CalendarDay, formatDayLabel, @@ -402,11 +403,16 @@ function deviceLabel(device: ReadingByDeviceDto): string { return device.deviceName ?? device.deviceId; } +/** How the series and format panels name reading of since-deleted books. */ +const REMOVED_LABEL = "Removed from library"; + /** A labelled row with a proportional bar. Used for series, devices, formats. */ function RankedRow({ label, href, sublabel, + muted = false, + action, measuredMs, inferredMs, value, @@ -416,6 +422,9 @@ function RankedRow({ label: string; href?: string; sublabel?: string; + /** Set for a row that is not a real item, so it cannot pass for one. */ + muted?: boolean; + action?: ReactNode; measuredMs: number; inferredMs: number; value: number; @@ -444,17 +453,26 @@ function RankedRow({ {label} ) : ( - + {label} )} - - {formatMetric(value, metric)} - + + {action} + + {formatMetric(value, metric)} + +
- {read.map((s) => ( - - ))} + {read.map((s) => + s.removedFromLibrary ? ( + // Reading of books since deleted from the server. It still counts, + // but there is no series to link to or name, and its book counts + // are always zero because deleted books cannot be told apart. + } + sublabel={`${s.sessions} ${s.sessions === 1 ? "sitting" : "sittings"} of books no longer on the server`} + measuredMs={s.duration.measuredMs} + inferredMs={s.duration.inferredMs} + value={rowValue(s, metric)} + metric={metric} + max={max} + /> + ) : ( + + ), + )} ); } @@ -563,10 +599,20 @@ export function FormatBreakdown({ return ( {read.map((f) => ( - - - {f.format} - + + {f.removedFromLibrary ? ( + + {REMOVED_LABEL} + + ) : ( + + {f.format} + + )} {formatMetric(rowValue(f, metric), metric)} diff --git a/web/src/components/reading/RemovedHistoryPurge.test.tsx b/web/src/components/reading/RemovedHistoryPurge.test.tsx new file mode 100644 index 000000000..5e6c1673a --- /dev/null +++ b/web/src/components/reading/RemovedHistoryPurge.test.tsx @@ -0,0 +1,90 @@ +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { readingStatsApi } from "@/api/readingStats"; +import { renderWithProviders, screen, waitFor } from "@/test/utils"; +import { RemovedHistoryPurge } from "./RemovedHistoryPurge"; + +vi.mock("@/api/readingStats", () => ({ + readingStatsApi: { + orphaned: vi.fn(), + purgeOrphaned: vi.fn(), + }, +})); + +const HOUR = 60 * 60_000; + +describe("RemovedHistoryPurge", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + /// The dashboard row is windowed and a purge is not, so the dialog has to + /// state the unwindowed totals it fetched, not the row's. + it("states what will be deleted across all dates before asking", async () => { + const user = userEvent.setup(); + vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ + duration: { measuredMs: 2 * HOUR, inferredMs: 0, totalMs: 2 * HOUR }, + pagesRead: 80, + sessions: 5, + completions: 2, + }); + renderWithProviders(); + + await user.click(screen.getByRole("button", { name: "Delete" })); + + expect( + await screen.findByText( + /permanently deletes 5 sittings \(2h of reading\)/, + ), + ).toBeInTheDocument(); + expect(screen.getByText(/2 finished reads/)).toBeInTheDocument(); + expect(screen.getByText(/across all dates/)).toBeInTheDocument(); + expect(readingStatsApi.purgeOrphaned).not.toHaveBeenCalled(); + }); + + it("deletes only after the second confirmation", async () => { + const user = userEvent.setup(); + vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ + duration: { measuredMs: HOUR, inferredMs: 0, totalMs: HOUR }, + pagesRead: 10, + sessions: 1, + completions: 0, + }); + vi.mocked(readingStatsApi.purgeOrphaned).mockResolvedValue({ + sessionsRemoved: 1, + completionsRemoved: 0, + }); + renderWithProviders(); + + await user.click(screen.getByRole("button", { name: "Delete" })); + const confirm = await screen.findByRole("button", { + name: "Delete permanently", + }); + await waitFor(() => expect(confirm).toBeEnabled()); + await user.click(confirm); + + await waitFor(() => + expect(readingStatsApi.purgeOrphaned).toHaveBeenCalledTimes(1), + ); + }); + + it("cannot delete when there is nothing left to delete", async () => { + const user = userEvent.setup(); + vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ + duration: { measuredMs: 0, inferredMs: 0, totalMs: 0 }, + pagesRead: 0, + sessions: 0, + completions: 0, + }); + renderWithProviders(); + + await user.click(screen.getByRole("button", { name: "Delete" })); + + expect( + await screen.findByText("There is no removed history left to delete."), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Delete permanently" }), + ).toBeDisabled(); + }); +}); diff --git a/web/src/components/reading/RemovedHistoryPurge.tsx b/web/src/components/reading/RemovedHistoryPurge.tsx new file mode 100644 index 000000000..14a0db684 --- /dev/null +++ b/web/src/components/reading/RemovedHistoryPurge.tsx @@ -0,0 +1,138 @@ +/** + * The way out of the "removed from library" row. + * + * Reading of a book that has since been deleted from the server keeps counting + * towards every statistic. This lets the reader decide it should not, and says + * exactly what that costs before doing it. + * + * The figures in the dialog come from their own request rather than from the + * row: the row covers only the dates on screen, while a purge removes every + * such session regardless of date, so the row would understate what is lost. + */ + +import { + Alert, + Button, + Group, + Loader, + Modal, + Stack, + Text, +} from "@mantine/core"; +import { notifications } from "@mantine/notifications"; +import { IconAlertTriangle } from "@tabler/icons-react"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { readingStatsApi } from "@/api/readingStats"; +import { formatDuration } from "./readingStatsFormat"; + +function plural(count: number, one: string, many: string): string { + return `${count} ${count === 1 ? one : many}`; +} + +export function RemovedHistoryPurge() { + const [opened, setOpened] = useState(false); + const queryClient = useQueryClient(); + + const totals = useQuery({ + queryKey: ["readingStats", "orphaned"], + queryFn: () => readingStatsApi.orphaned(), + enabled: opened, + // Always re-read on open: the dialog's whole job is to be accurate. + staleTime: 0, + }); + + const purge = useMutation({ + mutationFn: () => readingStatsApi.purgeOrphaned(), + onSuccess: (result) => { + setOpened(false); + // Every total on the page included this history, and coverage may have + // moved too, so everything under the prefix is stale. + queryClient.invalidateQueries({ queryKey: ["readingStats"] }); + notifications.show({ + title: "Removed history deleted", + message: `${plural(result.sessionsRemoved, "sitting", "sittings")} and ${plural( + result.completionsRemoved, + "finished read", + "finished reads", + )} no longer count.`, + color: "green", + }); + }, + onError: () => { + notifications.show({ + title: "Could not delete removed history", + message: "Nothing was deleted. Try again in a moment.", + color: "red", + }); + }, + }); + + const data = totals.data; + const nothingToDelete = data !== undefined && data.sessions === 0; + + return ( + <> + + setOpened(false)} + title="Delete reading of removed books?" + centered + > + + + These books have been deleted from the server. Their reading still + counts in your statistics, but which book or series it came from is + no longer known. + + {totals.isLoading && ( + + + + )} + {totals.error && ( + }> + Could not total the removed history, so nothing can be deleted + right now. + + )} + {data && !nothingToDelete && ( + }> + This permanently deletes{" "} + {plural(data.sessions, "sitting", "sittings")} ( + {formatDuration(data.duration.totalMs)} of reading) and{" "} + {plural(data.completions, "finished read", "finished reads")}, + across all dates, not only the ones shown. It cannot be undone. + + )} + {nothingToDelete && ( + + There is no removed history left to delete. + + )} + + + + + + + + ); +} diff --git a/web/src/mocks/handlers/readingStats.ts b/web/src/mocks/handlers/readingStats.ts index ab24ec360..9fc9e757a 100644 --- a/web/src/mocks/handlers/readingStats.ts +++ b/web/src/mocks/handlers/readingStats.ts @@ -209,6 +209,22 @@ export const readingStatsHandlers = [ }); }), + // Nothing in the mock library has been deleted, so there is nothing to purge. + http.get("*/api/v1/reading-stats/orphaned", async () => { + await delay(80); + return HttpResponse.json({ + duration: { measuredMs: 0, inferredMs: 0, totalMs: 0 }, + pagesRead: 0, + sessions: 0, + completions: 0, + }); + }), + + http.delete("*/api/v1/reading-stats/orphaned", async () => { + await delay(80); + return HttpResponse.json({ sessionsRemoved: 0, completionsRemoved: 0 }); + }), + http.get("*/api/v1/reading-stats", async ({ request }) => { await delay(150); @@ -245,6 +261,7 @@ export const readingStatsHandlers = [ return { seriesId: source.id, seriesName: source.title, + removedFromLibrary: false, duration: durationOf(days), pagesRead: days.reduce((sum, d) => sum + d.pagesRead, 0), sessions: days.reduce((sum, d) => sum + d.sessions, 0), @@ -274,6 +291,7 @@ export const readingStatsHandlers = [ ] .map(([format, days]) => ({ format, + removedFromLibrary: false, duration: durationOf(days), pagesRead: days.reduce((sum, d) => sum + d.pagesRead, 0), sessions: days.reduce((sum, d) => sum + d.sessions, 0), diff --git a/web/src/types/api.generated.ts b/web/src/types/api.generated.ts index 8aae9c3ca..5e14bbb4e 100644 --- a/web/src/types/api.generated.ts +++ b/web/src/types/api.generated.ts @@ -3367,7 +3367,12 @@ export interface paths { path?: never; cookie?: never; }; - get?: never; + /** + * The caller's reading history for books that no longer exist, in total + * @description Unwindowed: exactly what `DELETE /api/v1/reading-stats/orphaned` would + * remove, so a client can say so before asking the reader to confirm. + */ + get: operations["get_orphaned_reading_history"]; put?: never; post?: never; /** @@ -14905,6 +14910,25 @@ export interface components { */ sizeBytes: number; }; + /** + * @description The caller's reading history whose book has since been deleted, across + * every date. What a purge would remove. + */ + OrphanedHistoryDto: { + /** + * Format: int64 + * @description Finished read-throughs. + */ + completions: number; + duration: components["schemas"]["DurationBreakdownDto"]; + /** Format: int64 */ + pagesRead: number; + /** + * Format: int64 + * @description Session rows, including bookkeeping rows such as a mark-unread. + */ + sessions: number; + }; /** @description Page data transfer object */ PageDto: { /** @@ -29506,6 +29530,40 @@ export interface operations { }; }; }; + get_orphaned_reading_history: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Totals of the detached history */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["OrphanedHistoryDto"]; + }; + }; + /** @description Unauthorized */ + 401: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Forbidden */ + 403: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; purge_orphaned_reading_history: { parameters: { query?: never; From 0ed41c7e54d7c5d378b127629c64078aca1240b7 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sun, 27 Sep 2026 12:39:42 -0700 Subject: [PATCH 4/4] fix: make removed-book history reachable and its purge honest The purge control lived on the "Removed from library" series row, which only exists when that reading falls inside the dates on screen, ranks in the top eight, and is non-zero for the chosen metric. Under "books finished" it never is, and orphaned completions with no sittings never produce a row at all. The entry point is now a page-level notice driven by the unwindowed totals, shown whenever any removed history exists. The confirmation re-reads those totals on open and stays disabled until they arrive, so it cannot confirm against figures from before it opened or alongside a failed refresh. "Nothing left" requires no sittings and no completions, and sittings are counted as the dashboard counts them. Also drop doc comments promising a reading-progress import that does not exist yet, give the trigger an accessible name, and in the migration: run a rollback's orphan delete inside the SQLite rebuild's transaction, name the table when pre-existing rows would fail the copy, and add a book_id index so each deleted book stops scanning both tables in full for the foreign key action. --- .../src/routes/v1/dto/reading_stats.rs | 2 +- .../src/routes/v1/handlers/reading_stats.rs | 6 +- .../src/repositories/reading_stats.rs | 11 +- docs/api/openapi.json | 4 +- docs/docs/reading-progress.md | 11 +- ...14_reading_history_survives_book_delete.rs | 96 ++++++++-- tests/db/reading_history_retention.rs | 18 +- web/openapi.json | 4 +- .../reading/ReadingStatsPanels.test.tsx | 1 - .../components/reading/ReadingStatsPanels.tsx | 27 ++- .../reading/RemovedHistoryNotice.test.tsx | 133 +++++++++++++ .../reading/RemovedHistoryNotice.tsx | 180 ++++++++++++++++++ .../reading/RemovedHistoryPurge.test.tsx | 90 --------- .../reading/RemovedHistoryPurge.tsx | 138 -------------- web/src/pages/ReadingStats.tsx | 3 + web/src/types/api.generated.ts | 8 +- 16 files changed, 442 insertions(+), 290 deletions(-) create mode 100644 web/src/components/reading/RemovedHistoryNotice.test.tsx create mode 100644 web/src/components/reading/RemovedHistoryNotice.tsx delete mode 100644 web/src/components/reading/RemovedHistoryPurge.test.tsx delete mode 100644 web/src/components/reading/RemovedHistoryPurge.tsx diff --git a/crates/codex-api/src/routes/v1/dto/reading_stats.rs b/crates/codex-api/src/routes/v1/dto/reading_stats.rs index d0bb9a567..1d039f7c0 100644 --- a/crates/codex-api/src/routes/v1/dto/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/dto/reading_stats.rs @@ -349,7 +349,7 @@ impl From for PurgedOrphanedHistoryDto { pub struct OrphanedHistoryDto { pub duration: DurationBreakdownDto, pub pages_read: i64, - /// Session rows, including bookkeeping rows such as a mark-unread. + /// Sittings, counted as the dashboard counts them. pub sessions: i64, /// Finished read-throughs. pub completions: u64, diff --git a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs index 37caf8208..f79ff0c29 100644 --- a/crates/codex-api/src/routes/v1/handlers/reading_stats.rs +++ b/crates/codex-api/src/routes/v1/handlers/reading_stats.rs @@ -232,10 +232,8 @@ pub async fn get_orphaned_reading_history( /// the series and format breakdowns show it as one "removed from library" row. /// This discards those rows for the caller, and only for the caller. /// -/// Irreversible, and it forecloses the other way out: importing a reading -/// progress export taken before the delete puts those sessions back on their -/// books. Only history already detached from any book is touched; attributed -/// reading is never affected. +/// Irreversible. Only history already detached from any book is touched; +/// attributed reading is never affected. #[utoipa::path( delete, path = "/api/v1/reading-stats/orphaned", diff --git a/crates/codex-db/src/repositories/reading_stats.rs b/crates/codex-db/src/repositories/reading_stats.rs index e598111cc..ef392d478 100644 --- a/crates/codex-db/src/repositories/reading_stats.rs +++ b/crates/codex-db/src/repositories/reading_stats.rs @@ -692,15 +692,16 @@ impl ReadingStatsRepository { } let backend = db.get_database_backend(); - // Counts every orphaned row, reading or not: a purge removes `reset` - // rows too, and the count has to match what it reports afterwards. + // Sittings are counted the way the dashboard counts them, so the + // confirmation agrees with the row the reader clicked. A purge also + // removes orphaned `reset` rows, but those count towards nothing. let sql = format!( "SELECT {MEASURED_SUM} AS measured_ms, \ {INFERRED_SUM} AS inferred_ms, \ {PAGES_SUM} AS pages_read, \ COUNT(*) AS sessions \ FROM reading_sessions rs \ - WHERE rs.user_id = $1 AND rs.book_id IS NULL" + WHERE rs.user_id = $1 AND rs.book_id IS NULL AND {READING_KINDS}" ); let row = OrphanRow::find_by_statement(Statement::from_sql_and_values( backend, @@ -737,9 +738,7 @@ impl ReadingStatsRepository { /// and nothing else. /// /// Those rows keep counting towards every total until the reader decides - /// otherwise; this is that decision. It is never run on a schedule: the - /// same rows can be reattached to their books by importing an export taken - /// before the delete, and a purge makes that impossible. + /// otherwise; this is that decision, so it is never run on a schedule. /// /// Sessions and completions go together in one transaction so the two /// logs cannot disagree about whether the history exists. diff --git a/docs/api/openapi.json b/docs/api/openapi.json index 29cd9c5a9..157908ff7 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -10474,7 +10474,7 @@ "Reading Statistics" ], "summary": "Delete the caller's reading history for books that no longer exist", - "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible, and it forecloses the other way out: importing a reading\nprogress export taken before the delete puts those sessions back on their\nbooks. Only history already detached from any book is touched; attributed\nreading is never affected.", + "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible. Only history already detached from any book is touched;\nattributed reading is never affected.", "operationId": "purge_orphaned_reading_history", "responses": { "200": { @@ -35686,7 +35686,7 @@ "sessions": { "type": "integer", "format": "int64", - "description": "Session rows, including bookkeeping rows such as a mark-unread." + "description": "Sittings, counted as the dashboard counts them." } } }, diff --git a/docs/docs/reading-progress.md b/docs/docs/reading-progress.md index 8ee4f5008..d0d7235e2 100644 --- a/docs/docs/reading-progress.md +++ b/docs/docs/reading-progress.md @@ -139,11 +139,12 @@ What happens to your reading then: ### Deleting the removed-from-library history -If you would rather that reading stopped counting, use **Delete** on the -**Removed from library** row of the series panel. The confirmation states how -many sittings, how much reading time and how many finished read-throughs will -go, across **all** dates rather than only the ones on screen, because that is -what is deleted. It is permanent, affects only your own history, and never +Whenever you have any, the **Reading Statistics** page shows a **Reading of +removed books** notice above the series panel, whatever dates you are viewing. +If you would rather that reading stopped counting, use **Delete** on that +notice. The confirmation states how many sittings, how much reading time and +how many finished read-throughs will go, across **all** dates rather than only +the ones on screen, because that is what is deleted. It is permanent, affects only your own history, and never touches reading attributed to a book that still exists. The same is available through the API: `GET /api/v1/reading-stats/orphaned` diff --git a/migration/src/m20260927_000114_reading_history_survives_book_delete.rs b/migration/src/m20260927_000114_reading_history_survives_book_delete.rs index fed47150c..b1263fe68 100644 --- a/migration/src/m20260927_000114_reading_history_survives_book_delete.rs +++ b/migration/src/m20260927_000114_reading_history_survives_book_delete.rs @@ -33,6 +33,14 @@ //! ordering; a rebuild that silently lost an index would slow the statistics //! queries without failing anything. //! +//! # A `book_id` index +//! +//! Neither table had an index leading with `book_id`, so every deleted book +//! made the foreign key action scan both tables in full. Deleting a library +//! deletes every book in it, one scan each. That was already true under +//! `CASCADE`; `SET NULL` additionally rewrites the rows it finds, and the +//! rebuild is the cheapest moment to add the index. +//! //! # Rollback is lossy //! //! `down` restores `NOT NULL` and `CASCADE`, which orphaned rows cannot @@ -75,18 +83,37 @@ impl Direction { } } -async fn migrate(manager: &SchemaManager<'_>, direction: Direction) -> Result<(), DbErr> { - let db = manager.get_connection(); +const HISTORY_TABLES: [&str; 2] = ["reading_sessions", "read_completions"]; - if direction == Direction::Down { - for table in ["reading_sessions", "read_completions"] { - db.execute_unprepared(&format!("DELETE FROM {table} WHERE book_id IS NULL")) - .await?; - } +fn book_index_name(table: &str) -> String { + format!("idx_{table}_book_id") +} + +/// `down` restores `NOT NULL`, which orphaned rows cannot satisfy. +async fn delete_orphans(db: &C) -> Result<(), DbErr> { + for table in HISTORY_TABLES { + db.execute_unprepared(&format!("DELETE FROM {table} WHERE book_id IS NULL")) + .await?; } + Ok(()) +} + +async fn migrate(manager: &SchemaManager<'_>, direction: Direction) -> Result<(), DbErr> { + let db = manager.get_connection(); match db.get_database_backend() { + // Already inside the migrator's transaction. DbBackend::Postgres => { + if direction == Direction::Down { + delete_orphans(db).await?; + for table in HISTORY_TABLES { + db.execute_unprepared(&format!( + "DROP INDEX IF EXISTS {}", + book_index_name(table) + )) + .await?; + } + } alter_postgres( db, "reading_sessions", @@ -101,9 +128,24 @@ async fn migrate(manager: &SchemaManager<'_>, direction: Direction) -> Result<() direction, ) .await?; + if direction == Direction::Up { + for table in HISTORY_TABLES { + db.execute_unprepared(&format!( + "CREATE INDEX {} ON {table} (book_id)", + book_index_name(table) + )) + .await?; + } + } } + // The migrator does not wrap SQLite migrations in a transaction, so + // this one opens its own: a rollback that deleted the orphans and then + // failed the rebuild would otherwise lose them for nothing. DbBackend::Sqlite => { let txn = db.begin().await?; + if direction == Direction::Down { + delete_orphans(&txn).await?; + } rebuild_sqlite(&txn, &ReadingSessionsTable, direction).await?; rebuild_sqlite(&txn, &ReadCompletionsTable, direction).await?; txn.commit().await?; @@ -164,6 +206,17 @@ async fn rebuild_sqlite( let staging = format!("{name}_rebuild"); let columns = table.columns().join(", "); + // A row that already references a missing user or book would fail the + // copy below with SQLite's bare "FOREIGN KEY constraint failed", and since + // migrations run at startup the server would not start. Say which table. + let existing = foreign_key_violations(txn, name).await?; + if existing > 0 { + return Err(DbErr::Migration(format!( + "{name} has {existing} rows referencing a missing user or book; \ + `PRAGMA foreign_key_check({name})` lists them. Delete them and restart." + ))); + } + let before = count_rows(txn, name).await?; txn.execute(backend.build(&table.create(&staging, direction))) @@ -189,25 +242,34 @@ async fn rebuild_sqlite( for index in table.indexes() { txn.execute(backend.build(&index)).await?; } - - // A row the copy let through that references a missing user or book would - // otherwise surface only on some later write. - let violations = txn - .query_all(Statement::from_string( - backend, - format!("PRAGMA foreign_key_check({name})"), + if direction == Direction::Up { + txn.execute_unprepared(&format!( + "CREATE INDEX {} ON {name} (book_id)", + book_index_name(name) )) .await?; - if !violations.is_empty() { + } + + let violations = foreign_key_violations(txn, name).await?; + if violations > 0 { return Err(DbErr::Migration(format!( - "rebuilding {name} left {} foreign key violations", - violations.len() + "rebuilding {name} left {violations} foreign key violations" ))); } Ok(()) } +async fn foreign_key_violations(txn: &DatabaseTransaction, table: &str) -> Result { + Ok(txn + .query_all(Statement::from_string( + DbBackend::Sqlite, + format!("PRAGMA foreign_key_check({table})"), + )) + .await? + .len()) +} + async fn count_rows(db: &C, table: &str) -> Result { let row = db .query_one(Statement::from_string( diff --git a/tests/db/reading_history_retention.rs b/tests/db/reading_history_retention.rs index 4449486bc..4cb89bb8b 100644 --- a/tests/db/reading_history_retention.rs +++ b/tests/db/reading_history_retention.rs @@ -487,10 +487,22 @@ async fn sqlite_rebuild_keeps_rows_and_indexes() { apply_retention_migration(conn).await; for (i, table) in HISTORY_TABLES.into_iter().enumerate() { + let book_index = format!("idx_{table}_book_id"); + let (added, kept): (Vec, Vec) = index_definitions(conn, table) + .await + .into_iter() + .partition(|sql| { + sql.contains(&format!("\"{book_index}\"")) + || sql.contains(&format!(" {book_index} ")) + }); + assert_eq!( + kept, indexes_before[i], + "{table}: the rebuild must recreate every original index exactly" + ); assert_eq!( - index_definitions(conn, table).await, - indexes_before[i], - "{table}: the rebuild must recreate every index exactly" + added.len(), + 1, + "{table}: the rebuild adds one index on book_id for the foreign key action" ); let expected_columns: Vec = columns_before[i] diff --git a/web/openapi.json b/web/openapi.json index 29cd9c5a9..157908ff7 100644 --- a/web/openapi.json +++ b/web/openapi.json @@ -10474,7 +10474,7 @@ "Reading Statistics" ], "summary": "Delete the caller's reading history for books that no longer exist", - "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible, and it forecloses the other way out: importing a reading\nprogress export taken before the delete puts those sessions back on their\nbooks. Only history already detached from any book is touched; attributed\nreading is never affected.", + "description": "When a book is deleted from the server its reading sessions and finished\nread-throughs are kept, so the time still counts towards every statistic;\nthe series and format breakdowns show it as one \"removed from library\" row.\nThis discards those rows for the caller, and only for the caller.\n\nIrreversible. Only history already detached from any book is touched;\nattributed reading is never affected.", "operationId": "purge_orphaned_reading_history", "responses": { "200": { @@ -35686,7 +35686,7 @@ "sessions": { "type": "integer", "format": "int64", - "description": "Session rows, including bookkeeping rows such as a mark-unread." + "description": "Sittings, counted as the dashboard counts them." } } }, diff --git a/web/src/components/reading/ReadingStatsPanels.test.tsx b/web/src/components/reading/ReadingStatsPanels.test.tsx index e85e2beb6..c71b85869 100644 --- a/web/src/components/reading/ReadingStatsPanels.test.tsx +++ b/web/src/components/reading/ReadingStatsPanels.test.tsx @@ -185,7 +185,6 @@ describe("TopSeries", () => { expect( screen.getByText("3 sittings of books no longer on the server"), ).toBeInTheDocument(); - expect(screen.getByRole("button", { name: "Delete" })).toBeInTheDocument(); expect(screen.getAllByRole("link")).toHaveLength(2); }); diff --git a/web/src/components/reading/ReadingStatsPanels.tsx b/web/src/components/reading/ReadingStatsPanels.tsx index 42a59bac8..25f7204b4 100644 --- a/web/src/components/reading/ReadingStatsPanels.tsx +++ b/web/src/components/reading/ReadingStatsPanels.tsx @@ -11,7 +11,7 @@ */ import { Anchor, Box, Group, Paper, Stack, Text, Tooltip } from "@mantine/core"; -import { type ReactNode, useLayoutEffect, useRef } from "react"; +import { useLayoutEffect, useRef } from "react"; import { Link } from "react-router-dom"; import type { ReadingByDeviceDto, @@ -20,7 +20,6 @@ import type { } from "@/api/readingStats"; import type { ReadingMetric } from "@/store/readingStatsPreferencesStore"; import classes from "./ReadingStatsCharts.module.css"; -import { RemovedHistoryPurge } from "./RemovedHistoryPurge"; import { type CalendarDay, formatDayLabel, @@ -412,7 +411,6 @@ function RankedRow({ href, sublabel, muted = false, - action, measuredMs, inferredMs, value, @@ -424,7 +422,6 @@ function RankedRow({ sublabel?: string; /** Set for a row that is not a real item, so it cannot pass for one. */ muted?: boolean; - action?: ReactNode; measuredMs: number; inferredMs: number; value: number; @@ -463,16 +460,13 @@ function RankedRow({ {label} )} - - {action} - - {formatMetric(value, metric)} - - + + {formatMetric(value, metric)} +
} sublabel={`${s.sessions} ${s.sessions === 1 ? "sitting" : "sittings"} of books no longer on the server`} measuredMs={s.duration.measuredMs} inferredMs={s.duration.inferredMs} diff --git a/web/src/components/reading/RemovedHistoryNotice.test.tsx b/web/src/components/reading/RemovedHistoryNotice.test.tsx new file mode 100644 index 000000000..856bfefd7 --- /dev/null +++ b/web/src/components/reading/RemovedHistoryNotice.test.tsx @@ -0,0 +1,133 @@ +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { type OrphanedHistory, readingStatsApi } from "@/api/readingStats"; +import { renderWithProviders, screen, waitFor } from "@/test/utils"; +import { RemovedHistoryNotice } from "./RemovedHistoryNotice"; + +vi.mock("@/api/readingStats", () => ({ + readingStatsApi: { + orphaned: vi.fn(), + purgeOrphaned: vi.fn(), + }, +})); + +const HOUR = 60 * 60_000; + +function totals( + sessions: number, + totalMs: number, + completions: number, +): OrphanedHistory { + return { + duration: { measuredMs: totalMs, inferredMs: 0, totalMs }, + pagesRead: 0, + sessions, + completions, + }; +} + +async function openDialog() { + const user = userEvent.setup(); + await user.click( + await screen.findByRole("button", { + name: "Delete reading of removed books", + }), + ); + return user; +} + +describe("RemovedHistoryNotice", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("renders nothing when no removed history exists", async () => { + vi.mocked(readingStatsApi.orphaned).mockResolvedValue(totals(0, 0, 0)); + renderWithProviders(); + + await waitFor(() => expect(readingStatsApi.orphaned).toHaveBeenCalled()); + expect( + screen.queryByText("Reading of removed books"), + ).not.toBeInTheDocument(); + }); + + /// Not bound to the dashboard window or the series ranking: finished reads + /// with no sittings at all would never produce a series row. + it("offers the purge for completions that have no sittings", async () => { + vi.mocked(readingStatsApi.orphaned).mockResolvedValue(totals(0, 0, 3)); + renderWithProviders(); + + expect( + await screen.findByText(/3 finished reads from books since deleted/), + ).toBeInTheDocument(); + }); + + it("states what will be deleted across all dates before asking", async () => { + vi.mocked(readingStatsApi.orphaned).mockResolvedValue( + totals(5, 2 * HOUR, 2), + ); + renderWithProviders(); + await openDialog(); + + expect( + await screen.findByText( + /permanently deletes 5 sittings \(2h of reading\) and 2 finished reads, across all dates/, + ), + ).toBeInTheDocument(); + expect(readingStatsApi.purgeOrphaned).not.toHaveBeenCalled(); + }); + + it("re-reads the totals on open and deletes only after confirming", async () => { + vi.mocked(readingStatsApi.orphaned).mockResolvedValue(totals(1, HOUR, 0)); + vi.mocked(readingStatsApi.purgeOrphaned).mockResolvedValue({ + sessionsRemoved: 1, + completionsRemoved: 0, + }); + renderWithProviders(); + const user = await openDialog(); + + await waitFor(() => + expect(readingStatsApi.orphaned).toHaveBeenCalledTimes(2), + ); + const confirm = await screen.findByRole("button", { + name: "Delete permanently", + }); + await waitFor(() => expect(confirm).toBeEnabled()); + await user.click(confirm); + + await waitFor(() => + expect(readingStatsApi.purgeOrphaned).toHaveBeenCalledTimes(1), + ); + }); + + it("will not confirm against totals it failed to refresh", async () => { + vi.mocked(readingStatsApi.orphaned) + .mockResolvedValueOnce(totals(4, HOUR, 0)) + .mockRejectedValue(new Error("offline")); + renderWithProviders(); + await openDialog(); + + expect( + await screen.findByText(/Could not total the removed history/), + ).toBeInTheDocument(); + expect(screen.queryByText(/permanently deletes/)).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Delete permanently" }), + ).toBeDisabled(); + }); + + it("says so when the history is already gone by the time it opens", async () => { + vi.mocked(readingStatsApi.orphaned) + .mockResolvedValueOnce(totals(2, HOUR, 0)) + .mockResolvedValue(totals(0, 0, 0)); + renderWithProviders(); + await openDialog(); + + expect( + await screen.findByText("There is no removed history left to delete."), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Delete permanently" }), + ).toBeDisabled(); + }); +}); diff --git a/web/src/components/reading/RemovedHistoryNotice.tsx b/web/src/components/reading/RemovedHistoryNotice.tsx new file mode 100644 index 000000000..b058e91b4 --- /dev/null +++ b/web/src/components/reading/RemovedHistoryNotice.tsx @@ -0,0 +1,180 @@ +/** + * Reading of books since deleted from the server, and the way to discard it. + * + * That reading keeps counting towards every statistic. This notice says so and + * lets the reader decide it should not, stating exactly what that costs first. + * + * It is driven by its own unwindowed request rather than by the "Removed from + * library" row in the series panel. That row only exists when the reading falls + * inside the dates on screen, ranks in the top few, and is non-zero for the + * chosen metric, so a reader could hold removed history with no row, and no + * control, anywhere on the page. A purge is not windowed either, so the row's + * figures would understate what is lost. + */ + +import { + Alert, + Button, + Group, + Loader, + Modal, + Stack, + Text, +} from "@mantine/core"; +import { notifications } from "@mantine/notifications"; +import { IconAlertTriangle, IconInfoCircle } from "@tabler/icons-react"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { type OrphanedHistory, readingStatsApi } from "@/api/readingStats"; +import { formatDuration } from "./readingStatsFormat"; + +const ORPHANED_KEY = ["readingStats", "orphaned"] as const; + +function plural(count: number, one: string, many: string): string { + return `${count} ${count === 1 ? one : many}`; +} + +function isEmpty(totals: OrphanedHistory): boolean { + return totals.sessions === 0 && totals.completions === 0; +} + +/** What the totals amount to, in one clause. */ +function describe(totals: OrphanedHistory): string { + const parts = []; + if (totals.sessions > 0) { + parts.push( + `${plural(totals.sessions, "sitting", "sittings")} (${formatDuration(totals.duration.totalMs)} of reading)`, + ); + } + if (totals.completions > 0) { + parts.push(plural(totals.completions, "finished read", "finished reads")); + } + return parts.join(" and "); +} + +export function RemovedHistoryNotice() { + const [opened, setOpened] = useState(false); + const queryClient = useQueryClient(); + + const totals = useQuery({ + queryKey: ORPHANED_KEY, + queryFn: () => readingStatsApi.orphaned(), + staleTime: 60_000, + }); + + const purge = useMutation({ + mutationFn: () => readingStatsApi.purgeOrphaned(), + onSuccess: () => { + setOpened(false); + // Every total on the page included this history, and coverage may have + // moved too, so everything under the prefix is stale. + queryClient.invalidateQueries({ queryKey: ["readingStats"] }); + notifications.show({ + title: "Removed history deleted", + message: "Reading of books no longer on the server no longer counts.", + color: "green", + }); + }, + onError: () => { + notifications.show({ + title: "Could not delete removed history", + message: "Nothing was deleted. Try again in a moment.", + color: "red", + }); + }, + }); + + const data = totals.data; + // Stays mounted while the dialog is open, so a refetch that finds nothing + // left can say so instead of the dialog vanishing under the reader. + if (!data || (isEmpty(data) && !opened)) return null; + + const open = () => { + setOpened(true); + // The dialog's whole job is to be accurate, so it never confirms against + // figures fetched before it opened. + totals.refetch(); + }; + + // Settled means the figures on screen are the ones just fetched. + const settled = !totals.isFetching && !totals.isError; + const nothingLeft = settled && isEmpty(data); + + return ( + <> + {!isEmpty(data) && ( + } + title="Reading of removed books" + > + + + {describe(data)} from books since deleted from the server still + count in your statistics, shown as "Removed from library". + + + + + )} + setOpened(false)} + title="Delete reading of removed books?" + centered + > + + + These books have been deleted from the server. Their reading still + counts in your statistics, but which book or series it came from is + no longer known. + + {totals.isFetching && ( + + + + )} + {totals.isError && !totals.isFetching && ( + }> + Could not total the removed history, so nothing can be deleted + right now. + + )} + {settled && !nothingLeft && ( + }> + This permanently deletes {describe(data)}, across all dates, not + only the ones shown. It cannot be undone. + + )} + {nothingLeft && ( + + There is no removed history left to delete. + + )} + + + + + + + + ); +} diff --git a/web/src/components/reading/RemovedHistoryPurge.test.tsx b/web/src/components/reading/RemovedHistoryPurge.test.tsx deleted file mode 100644 index 5e6c1673a..000000000 --- a/web/src/components/reading/RemovedHistoryPurge.test.tsx +++ /dev/null @@ -1,90 +0,0 @@ -import userEvent from "@testing-library/user-event"; -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { readingStatsApi } from "@/api/readingStats"; -import { renderWithProviders, screen, waitFor } from "@/test/utils"; -import { RemovedHistoryPurge } from "./RemovedHistoryPurge"; - -vi.mock("@/api/readingStats", () => ({ - readingStatsApi: { - orphaned: vi.fn(), - purgeOrphaned: vi.fn(), - }, -})); - -const HOUR = 60 * 60_000; - -describe("RemovedHistoryPurge", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - /// The dashboard row is windowed and a purge is not, so the dialog has to - /// state the unwindowed totals it fetched, not the row's. - it("states what will be deleted across all dates before asking", async () => { - const user = userEvent.setup(); - vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ - duration: { measuredMs: 2 * HOUR, inferredMs: 0, totalMs: 2 * HOUR }, - pagesRead: 80, - sessions: 5, - completions: 2, - }); - renderWithProviders(); - - await user.click(screen.getByRole("button", { name: "Delete" })); - - expect( - await screen.findByText( - /permanently deletes 5 sittings \(2h of reading\)/, - ), - ).toBeInTheDocument(); - expect(screen.getByText(/2 finished reads/)).toBeInTheDocument(); - expect(screen.getByText(/across all dates/)).toBeInTheDocument(); - expect(readingStatsApi.purgeOrphaned).not.toHaveBeenCalled(); - }); - - it("deletes only after the second confirmation", async () => { - const user = userEvent.setup(); - vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ - duration: { measuredMs: HOUR, inferredMs: 0, totalMs: HOUR }, - pagesRead: 10, - sessions: 1, - completions: 0, - }); - vi.mocked(readingStatsApi.purgeOrphaned).mockResolvedValue({ - sessionsRemoved: 1, - completionsRemoved: 0, - }); - renderWithProviders(); - - await user.click(screen.getByRole("button", { name: "Delete" })); - const confirm = await screen.findByRole("button", { - name: "Delete permanently", - }); - await waitFor(() => expect(confirm).toBeEnabled()); - await user.click(confirm); - - await waitFor(() => - expect(readingStatsApi.purgeOrphaned).toHaveBeenCalledTimes(1), - ); - }); - - it("cannot delete when there is nothing left to delete", async () => { - const user = userEvent.setup(); - vi.mocked(readingStatsApi.orphaned).mockResolvedValue({ - duration: { measuredMs: 0, inferredMs: 0, totalMs: 0 }, - pagesRead: 0, - sessions: 0, - completions: 0, - }); - renderWithProviders(); - - await user.click(screen.getByRole("button", { name: "Delete" })); - - expect( - await screen.findByText("There is no removed history left to delete."), - ).toBeInTheDocument(); - expect( - screen.getByRole("button", { name: "Delete permanently" }), - ).toBeDisabled(); - }); -}); diff --git a/web/src/components/reading/RemovedHistoryPurge.tsx b/web/src/components/reading/RemovedHistoryPurge.tsx deleted file mode 100644 index 14a0db684..000000000 --- a/web/src/components/reading/RemovedHistoryPurge.tsx +++ /dev/null @@ -1,138 +0,0 @@ -/** - * The way out of the "removed from library" row. - * - * Reading of a book that has since been deleted from the server keeps counting - * towards every statistic. This lets the reader decide it should not, and says - * exactly what that costs before doing it. - * - * The figures in the dialog come from their own request rather than from the - * row: the row covers only the dates on screen, while a purge removes every - * such session regardless of date, so the row would understate what is lost. - */ - -import { - Alert, - Button, - Group, - Loader, - Modal, - Stack, - Text, -} from "@mantine/core"; -import { notifications } from "@mantine/notifications"; -import { IconAlertTriangle } from "@tabler/icons-react"; -import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { useState } from "react"; -import { readingStatsApi } from "@/api/readingStats"; -import { formatDuration } from "./readingStatsFormat"; - -function plural(count: number, one: string, many: string): string { - return `${count} ${count === 1 ? one : many}`; -} - -export function RemovedHistoryPurge() { - const [opened, setOpened] = useState(false); - const queryClient = useQueryClient(); - - const totals = useQuery({ - queryKey: ["readingStats", "orphaned"], - queryFn: () => readingStatsApi.orphaned(), - enabled: opened, - // Always re-read on open: the dialog's whole job is to be accurate. - staleTime: 0, - }); - - const purge = useMutation({ - mutationFn: () => readingStatsApi.purgeOrphaned(), - onSuccess: (result) => { - setOpened(false); - // Every total on the page included this history, and coverage may have - // moved too, so everything under the prefix is stale. - queryClient.invalidateQueries({ queryKey: ["readingStats"] }); - notifications.show({ - title: "Removed history deleted", - message: `${plural(result.sessionsRemoved, "sitting", "sittings")} and ${plural( - result.completionsRemoved, - "finished read", - "finished reads", - )} no longer count.`, - color: "green", - }); - }, - onError: () => { - notifications.show({ - title: "Could not delete removed history", - message: "Nothing was deleted. Try again in a moment.", - color: "red", - }); - }, - }); - - const data = totals.data; - const nothingToDelete = data !== undefined && data.sessions === 0; - - return ( - <> - - setOpened(false)} - title="Delete reading of removed books?" - centered - > - - - These books have been deleted from the server. Their reading still - counts in your statistics, but which book or series it came from is - no longer known. - - {totals.isLoading && ( - - - - )} - {totals.error && ( - }> - Could not total the removed history, so nothing can be deleted - right now. - - )} - {data && !nothingToDelete && ( - }> - This permanently deletes{" "} - {plural(data.sessions, "sitting", "sittings")} ( - {formatDuration(data.duration.totalMs)} of reading) and{" "} - {plural(data.completions, "finished read", "finished reads")}, - across all dates, not only the ones shown. It cannot be undone. - - )} - {nothingToDelete && ( - - There is no removed history left to delete. - - )} - - - - - - - - ); -} diff --git a/web/src/pages/ReadingStats.tsx b/web/src/pages/ReadingStats.tsx index c81bdd378..e0556a744 100644 --- a/web/src/pages/ReadingStats.tsx +++ b/web/src/pages/ReadingStats.tsx @@ -34,6 +34,7 @@ import { StatTile, TopSeries, } from "@/components/reading/ReadingStatsPanels"; +import { RemovedHistoryNotice } from "@/components/reading/RemovedHistoryNotice"; import { buildCalendar, groupIntoYears, @@ -356,6 +357,8 @@ export function ReadingStats() { + + diff --git a/web/src/types/api.generated.ts b/web/src/types/api.generated.ts index 5e14bbb4e..00df781f0 100644 --- a/web/src/types/api.generated.ts +++ b/web/src/types/api.generated.ts @@ -3382,10 +3382,8 @@ export interface paths { * the series and format breakdowns show it as one "removed from library" row. * This discards those rows for the caller, and only for the caller. * - * Irreversible, and it forecloses the other way out: importing a reading - * progress export taken before the delete puts those sessions back on their - * books. Only history already detached from any book is touched; attributed - * reading is never affected. + * Irreversible. Only history already detached from any book is touched; + * attributed reading is never affected. */ delete: operations["purge_orphaned_reading_history"]; options?: never; @@ -14925,7 +14923,7 @@ export interface components { pagesRead: number; /** * Format: int64 - * @description Session rows, including bookkeeping rows such as a mark-unread. + * @description Sittings, counted as the dashboard counts them. */ sessions: number; };