From 2efa50fcaf135ce00240e3074d354e926fa0de00 Mon Sep 17 00:00:00 2001 From: Guy Corbaz Date: Fri, 11 Sep 2026 00:17:07 +0200 Subject: [PATCH 1/2] fix(#478): register the restore route the Trash panel has been calling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Trash panel has rendered a Restore button since story 8-6, pointing at `/admin/trash/{table}/{id}/restore`. That route was never registered, so every click returned 404 — and HTMX does not swap a 4xx, so the click did nothing at all: no restore, no error, no feedback entry. The suite stayed green because `TrashService::restore` and `restore_with_conflicts_cleared` were only ever called from `#[cfg(test)]`. Everything under the button already existed; this is the missing wiring. The handler registers as **POST**, not the GET the button used to emit: a state-changing GET sits outside the story 8-2 CSRF layer, which guards POST/PUT/PATCH/DELETE only. The button now posts, with the panel filters threaded through so the post-restore re-render lands the admin back on the view they were on — the same contract as its permanent-delete sibling. When relationships changed while the item sat in the trash (a series whose title was reassigned, a contributor role removed), the first pass returns the UX-DR8 conflict modal — retargeted into #modal-slot — listing what the Confirm would clear; the Confirm re-posts with `clear_conflicts=1`. The `admin.trash.restore_modal_*` locale keys for exactly this flow have been sitting unused in both locale files since story 8-6. A conflict-free restore stays a single click. A restore also writes an `admin_audit` row, best-effort: restoring is the inverse of purging and belongs in the same trail, but the restore has already committed by then, so a failed audit INSERT must not turn a successful action into an error page. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D5cV9QRqQAFpiNKZwZfoNX --- locales/en.yml | 1 + locales/fr.yml | 1 + src/routes/admin.rs | 245 +++++++++ src/routes/mod.rs | 5 + templates/fragments/admin_trash_panel.html | 8 +- .../fragments/admin_trash_restore_modal.html | 26 + tests/admin_trash_restore.rs | 475 ++++++++++++++++++ .../journeys/admin-permanent-delete.spec.ts | 11 +- .../journeys/admin-trash-restore.spec.ts | 116 +++++ 9 files changed, 885 insertions(+), 3 deletions(-) create mode 100644 templates/fragments/admin_trash_restore_modal.html create mode 100644 tests/admin_trash_restore.rs create mode 100644 tests/e2e/specs/journeys/admin-trash-restore.spec.ts diff --git a/locales/en.yml b/locales/en.yml index c7a284a4..58d8b919 100644 --- a/locales/en.yml +++ b/locales/en.yml @@ -838,6 +838,7 @@ admin: days_remaining_warning: "%{days} day" days_remaining_critical: "<7 days" restore_success: "Restored: %{name}" + restore_error_not_found: "That item is no longer in the trash. It may have been restored or purged already." delete_permanent_success: "Deleted permanently: %{name}" delete_permanent_modal_title: "Delete permanently?" delete_permanent_modal_warning: "This cannot be undone." diff --git a/locales/fr.yml b/locales/fr.yml index 9dcd2f7d..0898cca9 100644 --- a/locales/fr.yml +++ b/locales/fr.yml @@ -835,6 +835,7 @@ admin: days_remaining_warning: "%{days} jour" days_remaining_critical: "<7 jours" restore_success: "Restauré : %{name}" + restore_error_not_found: "Cet élément n'est plus dans la corbeille. Il a peut-être déjà été restauré ou purgé." delete_permanent_success: "Supprimé définitivement : %{name}" delete_permanent_modal_title: "Supprimer définitivement ?" delete_permanent_modal_warning: "Ceci ne peut pas être annulé." diff --git a/src/routes/admin.rs b/src/routes/admin.rs index 514d52bd..1696871e 100644 --- a/src/routes/admin.rs +++ b/src/routes/admin.rs @@ -225,6 +225,21 @@ struct AdminTrashPermanentDeleteModal { inner_form_html: String, } +#[derive(Template)] +#[template(path = "fragments/admin_trash_restore_modal.html")] +struct AdminTrashRestoreModal { + /// Issue #478 — thin wrapper around the same UX-DR8 macro as the + /// permanent-delete modal. Shown only on the conflict path; a + /// conflict-free restore never opens a dialog. + title: String, + body_html: String, + confirm_label: String, + cancel_label: String, + action_url: String, + csrf_token: String, + version: i32, +} + #[derive(Template)] #[template(path = "fragments/admin_trash_panel.html")] struct AdminTrashPanel { @@ -554,6 +569,24 @@ pub struct PermanentDeleteConfirmQuery { pub page: Option, } +/// Query for `POST /admin/trash/{table}/{id}/restore` (issue #478). +/// `version` drives the optimistic lock; `clear_conflicts` marks the +/// second pass, after the admin confirmed the conflict modal; the rest +/// are the panel filters threaded through so the post-restore re-render +/// lands on the same view (same contract as `PermanentDeleteConfirmQuery`). +#[derive(Debug, Deserialize)] +pub struct RestoreQuery { + pub version: Option, + /// Typed as a string, not a `bool`: `serde_urlencoded` accepts only + /// `true` / `false` for a bool, so a hand-edited `?clear_conflicts=1` + /// would 400 instead of doing the obvious thing. Parsed through the + /// strict accept-set the project uses for its flags elsewhere. + pub clear_conflicts: Option, + pub entity_type: Option, + pub search: Option, + pub page: Option, +} + pub async fn admin_trash_permanent_delete_confirm( State(state): State, session: Session, @@ -808,6 +841,218 @@ pub(crate) async fn render_admin_for_reference_data( render_admin(state, session, loc, uri, is_htmx, AdminTab::ReferenceData, None).await } +/// Restore a soft-deleted item from the Trash (issue #478). +/// +/// The Restore button had been rendering a URL to a route that was never +/// registered, so a click produced a 404 that HTMX does not swap — no +/// restore, no error, nothing. Everything below it already existed: +/// `TrashService::restore`, `detect_restore_conflicts`, +/// `restore_with_conflicts_cleared`, and the `admin.trash.restore_*` +/// locale keys. This handler is the missing wiring. +/// +/// **POST, not GET.** The button used to emit `hx-get`. A state-changing +/// GET sits outside the CSRF middleware (story 8-2 guards +/// POST/PUT/PATCH/DELETE only) and is fair game for any prefetcher, so +/// the route is registered as POST and the button posts. +/// +/// Two passes when relationships changed while the item sat in the trash: +/// the first returns the conflict modal (retargeted into `#modal-slot`), +/// its Confirm re-posts with `clear_conflicts=1`. A conflict-free restore +/// takes the single-click path. +pub async fn admin_trash_restore( + State(state): State, + session: Session, + Extension(locale): Extension, + axum::extract::Path((table, id)): axum::extract::Path<(String, u64)>, + Query(params): Query, +) -> Result { + session.require_role_with_return(Role::Admin, "/admin?tab=trash", locale.0)?; + let loc = locale.0; + + let version = params + .version + .ok_or(AppError::BadRequest("Missing or invalid version".to_string()))?; + + // `get_trash_entry` validates `table` against ALLOWED_TABLES and + // filters on `deleted_at IS NOT NULL`, so a purged (or already + // restored) row lands here rather than in the service layer. + let entry = crate::models::trash::TrashModel::get_trash_entry(&state.pool, &table, id) + .await? + .ok_or_else(|| { + AppError::NotFound( + rust_i18n::t!("admin.trash.restore_error_not_found", locale = loc).to_string(), + ) + })?; + + let clear_conflicts = matches!( + params.clear_conflicts.as_deref(), + Some("1" | "true" | "TRUE") + ); + let conflicts = + crate::services::trash::TrashService::detect_restore_conflicts(&state.pool, &table, id) + .await?; + + if !conflicts.is_empty() && !clear_conflicts { + // The version in the URL is the one the admin's page carried, not + // a fresh read: a stale panel must still lose the optimistic lock + // when the modal's Confirm comes back. + let action_url = format!( + "/admin/trash/{}/{}/restore?version={}&clear_conflicts=1&entity_type={}&search={}&page={}", + crate::utils::html_escape(&table), + id, + version, + crate::utils::url_encode(params.entity_type.as_deref().unwrap_or("")), + crate::utils::url_encode(params.search.as_deref().unwrap_or("")), + params.page.unwrap_or(1).max(1), + ); + let modal_html = render_restore_conflict_modal( + &session, + loc, + &action_url, + version, + &entry.item_name, + &conflicts, + )?; + + // The click came from the panel's Restore button, whose + // `hx-target` is `#admin-trash-panel`. Retarget so the dialog + // lands in the stable `#modal-slot` (polish-1: outside any + // HTMX-swappable region) instead of replacing the panel with a + // modal. Not stripped by `ModalConfirmRetargetGuard` — that + // layer only touches responses to requests carrying + // `X-Modal-Confirm`, which a panel button never sends. + let mut response = Html(modal_html).into_response(); + response.headers_mut().insert( + axum::http::HeaderName::from_static("hx-retarget"), + axum::http::HeaderValue::from_static("#modal-slot"), + ); + response.headers_mut().insert( + axum::http::HeaderName::from_static("hx-reswap"), + axum::http::HeaderValue::from_static("innerHTML"), + ); + return Ok(response); + } + + let restored = if clear_conflicts { + crate::services::trash::TrashService::restore_with_conflicts_cleared( + &state.pool, + &table, + id, + version, + ) + .await? + } else { + crate::services::trash::TrashService::restore(&state.pool, &table, id, version).await? + }; + + // Forensics, mirroring `permanent_delete_from_trash` — restoring a + // row is the inverse of purging it and belongs in the same trail. + // Best-effort on purpose: the restore already committed, so a failed + // audit INSERT must not turn a successful action into an error page. + if let Some(user_id) = session.user_id { + let actor_username: String = sqlx::query_scalar("SELECT username FROM users WHERE id = ?") + .bind(user_id) + .fetch_optional(&state.pool) + .await + .ok() + .flatten() + .unwrap_or_else(|| format!("user-{}", user_id)); + + if let Err(e) = crate::models::admin_audit::AdminAuditModel::create( + &state.pool, + user_id, + "restore_from_trash", + Some(&table), + Some(id), + Some(serde_json::json!({ + "user_username": actor_username, + "user_role": session.role.to_string(), + "item_name": restored.item_name, + "conflicts_cleared": clear_conflicts, + })), + ) + .await + { + tracing::warn!(error = %e, table = %table, id = id, "restore audit entry failed"); + } + } + + let success_msg = + rust_i18n::t!("admin.trash.restore_success", locale = loc, name = &restored.item_name) + .to_string(); + let feedback = feedback_html("success", &success_msg, ""); + + // Re-render with the filters the admin was looking at, exactly as the + // permanent-delete handler does (patch P12). + let filters = TrashQuery { + entity_type: params.entity_type, + search: params.search, + page: params.page, + }; + let panel_html = render_trash_panel(&state, loc, &filters).await?; + + // `HX-Trigger: modal-close` closes the conflict modal on the second + // pass. Harmless on the single-click path: modal.js only closes when + // the finished Confirm came from its own slot. + Ok(HtmxResponse { + main: panel_html, + oob: vec![OobUpdate { + swap_mode: Default::default(), + target: "feedback-list".to_string(), + content: feedback, + }], + } + .into_response_with_hx_trigger("modal-close")) +} + +/// Build the conflict modal for [`admin_trash_restore`]. Kept separate so +/// the handler reads as one flow. Every interpolated value carrying user +/// data goes through `html_escape` — the macro consumes `body_html` with +/// `|safe` (CSP-clean, no inline script/style). +fn render_restore_conflict_modal( + session: &Session, + loc: &'static str, + action_url: &str, + version: i32, + item_name: &str, + conflicts: &[crate::services::trash::ConflictInfo], +) -> Result { + let explanation = rust_i18n::t!("admin.trash.restore_modal_explanation", locale = loc).to_string(); + let conflicts_label = rust_i18n::t!("admin.trash.restore_modal_conflicts", locale = loc).to_string(); + + let items = conflicts + .iter() + .map(|c| format!("
  • {}
    • ", crate::utils::html_escape(&c.description))) + .collect::>() + .join(""); + + let body_html = format!( + r##"

    {explanation}

    +

    {item_name}

    +

    {conflicts_label}

    +
      {items}
    "##, + explanation = crate::utils::html_escape(&explanation), + item_name = crate::utils::html_escape(item_name), + conflicts_label = crate::utils::html_escape(&conflicts_label), + items = items, + ); + + let modal = AdminTrashRestoreModal { + title: rust_i18n::t!("admin.trash.restore_modal_title", locale = loc).to_string(), + body_html, + confirm_label: rust_i18n::t!("admin.trash.restore_modal_clear_conflicts", locale = loc) + .to_string(), + cancel_label: rust_i18n::t!("admin.trash.restore_modal_cancel", locale = loc).to_string(), + action_url: action_url.to_string(), + csrf_token: session.csrf_token.clone(), + version, + }; + + modal + .render() + .map_err(|_| AppError::Internal("Modal render failed".to_string())) +} + pub async fn admin_trash_panel( State(state): State, session: Session, diff --git a/src/routes/mod.rs b/src/routes/mod.rs index 9221e998..585991a1 100644 --- a/src/routes/mod.rs +++ b/src/routes/mod.rs @@ -589,6 +589,11 @@ pub fn build_router(state: AppState) -> Router { ) .route("/admin/trash", axum::routing::get(admin::admin_trash_panel)) .route("/admin/trash/{table}/{id}/permanent-delete", axum::routing::get(admin::admin_trash_permanent_delete_confirm).post(admin::admin_trash_permanent_delete)) + // Issue #478: the panel's Restore button rendered this URL from + // story 8-6 on, but the route was never registered — every click + // 404'd, and HTMX does not swap a 4xx, so nothing happened at all. + // POST rather than GET so the CSRF layer (story 8-2) covers it. + .route("/admin/trash/{table}/{id}/restore", axum::routing::post(admin::admin_trash_restore)) // Admin → System settings (story 8-5). 4 routes — 1 GET panel + 3 // POST per-form saves. All Admin-gated, all CSRF-protected via the // 8-2 middleware (none added to CSRF_EXEMPT_ROUTES). diff --git a/templates/fragments/admin_trash_panel.html b/templates/fragments/admin_trash_panel.html index 1d609df0..0990d4bb 100644 --- a/templates/fragments/admin_trash_panel.html +++ b/templates/fragments/admin_trash_panel.html @@ -70,7 +70,13 @@

    {{ heading }}

    {{ entry.deleted_at.format("%Y-%m-%d %H:%M") }} {{ entry.days_remaining }} - + {# Issue #478: POST (the route mutates, so it rides the CSRF layer), + and the current filters / page are threaded through so the + post-restore re-render lands the admin back on the same view — + same contract as the permanent-delete button below. + `data-modal-trigger` is a focus anchor for the conflict modal + this may open; a conflict-free restore opens no dialog. #} + {# R3-N1: thread current filters / page through the confirm-modal URL so the post-delete re-render lands the admin back on the same view. #} diff --git a/templates/fragments/admin_trash_restore_modal.html b/templates/fragments/admin_trash_restore_modal.html new file mode 100644 index 00000000..341efeeb --- /dev/null +++ b/templates/fragments/admin_trash_restore_modal.html @@ -0,0 +1,26 @@ +{# Issue #478 — restore-conflict modal. Shown only when + `TrashService::detect_restore_conflicts` reports that relationships + changed while the item sat in the trash (series reassigned, contributor + role removed). The Confirm button re-posts the same restore URL with + `clear_conflicts=1`, which routes to + `TrashService::restore_with_conflicts_cleared`. + + Variant "warning" (indigo), not "delete" — restoring is not destructive; + what the Confirm clears is the stale relationship rows, and the body + lists them so the admin knows what they are agreeing to. Cancel keeps + the item deleted, per the `restore_modal_cancel` copy. #} +{% import "components/modal.html" as modal %} +{% call modal::modal( + "warning", + title, + body_html, + confirm_label, + cancel_label, + action_url, + "POST", + csrf_token, + "#admin-trash-panel", + "outerHTML", + version, + "", +) %}{% endcall %} diff --git a/tests/admin_trash_restore.rs b/tests/admin_trash_restore.rs new file mode 100644 index 00000000..b3cc88cd --- /dev/null +++ b/tests/admin_trash_restore.rs @@ -0,0 +1,475 @@ +//! Issue #478 — HTTP-level coverage for `POST /admin/trash/{table}/{id}/restore`. +//! +//! The Trash panel had rendered a Restore button since story 8-6, but the +//! route behind it was never registered: every click returned 404, and HTMX +//! does not swap a 4xx, so the button did nothing at all — no restore, no +//! error, no feedback. `TrashService::restore` was green the whole time +//! because its only callers were `#[cfg(test)]`. These tests exercise the +//! route over HTTP so the wiring itself is covered, not just the service. +//! +//! Run locally: +//! docker compose -f tests/docker-compose.rust-test.yml up -d +//! SQLX_OFFLINE=true DATABASE_URL='mysql://root:root_test@localhost:3307/mybibli_rust_test' \ +//! cargo test --test admin_trash_restore + +use std::path::PathBuf; +use std::sync::{Arc, RwLock}; + +use axum::body::Body; +use axum::http::{Method, Request, StatusCode, header}; +use sqlx::MySqlPool; +use tower::ServiceExt; + +use mybibli::AppState; +use mybibli::config::AppSettings; +use mybibli::metadata::registry::ProviderRegistry; +use mybibli::routes::build_router; +use mybibli::services::admin_health::new_mariadb_version_cache; +use mybibli::tasks::provider_health::new_provider_health_map; + +fn build_state(pool: MySqlPool) -> AppState { + AppState { + pool, + settings: Arc::new(RwLock::new(AppSettings::default())), + http_client: reqwest::Client::new(), + registry: Arc::new(ProviderRegistry::new()), + covers_dir: PathBuf::from("/tmp/mybibli-test-covers"), + provider_health: new_provider_health_map(), + mariadb_version_cache: new_mariadb_version_cache(), + setup_gate: Arc::new(RwLock::new( + mybibli::middleware::setup_gate::SetupGateState::default(), + )), + bulk_cover_fetch: Arc::new(RwLock::new( + mybibli::services::bulk_cover_fetch::BulkCoverFetchStatus::default(), + )), + log_level_reloader: mybibli::noop_log_level_reloader(), + } +} + +const TEST_CSRF_TOKEN: &str = "trash_restore_test_csrf_token_abcdef1234567890"; + +fn rand_suffix() -> String { + use base64::Engine; + let bytes: [u8; 8] = rand::random(); + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) +} + +/// Seed a session for a user of the given role. Returns (user_id, token). +async fn seed_session(pool: &MySqlPool, role: &str) -> (u64, String) { + let username = format!("{}-{}", &role[..1], rand_suffix()); + let user_id: u64 = sqlx::query_scalar( + "INSERT INTO users (username, password_hash, role) VALUES (?, '$argon2id$v=19$m=65536,t=3,p=4$placeholder$placeholder', ?) RETURNING id", + ) + .bind(&username) + .bind(role) + .fetch_one(pool) + .await + .expect("insert user"); + + // sessions.token is VARCHAR(44) so keep the prefix short. + let token = format!("tr-{}", rand_suffix()); + sqlx::query("INSERT INTO sessions (token, user_id, csrf_token, data) VALUES (?, ?, ?, '{}')") + .bind(&token) + .bind(user_id) + .bind(TEST_CSRF_TOKEN) + .execute(pool) + .await + .expect("insert session"); + + (user_id, token) +} + +/// Insert a soft-deleted title as the restore target. Returns (id, name, version). +async fn seed_soft_deleted_title(pool: &MySqlPool) -> (u64, String, i32) { + let name = format!("Trashed Title {}", rand_suffix()); + let id = sqlx::query( + "INSERT INTO titles (title, media_type, genre_id, version, deleted_at) \ + VALUES (?, 'book', 1, 1, NOW())", + ) + .bind(&name) + .execute(pool) + .await + .expect("insert soft-deleted title") + .last_insert_id(); + + (id, name, 1) +} + +async fn deleted_at_of(pool: &MySqlPool, table: &str, id: u64) -> Option { + sqlx::query_scalar(&format!( + "SELECT CAST(deleted_at AS DATETIME) FROM {table} WHERE id = ?" + )) + .bind(id) + .fetch_one(pool) + .await + .expect("read deleted_at") +} + +async fn body_text(resp: axum::response::Response) -> String { + let bytes = axum::body::to_bytes(resp.into_body(), 1024 * 1024) + .await + .expect("body bytes"); + String::from_utf8(bytes.to_vec()).expect("utf-8 body") +} + +/// A restore request as the browser sends it: HTMX `hx-post` with the CSRF +/// token in the header (`static/js/csrf.js`), no body. +fn restore_request(uri: String, token: &str, csrf: Option<&str>) -> Request { + let mut builder = Request::builder() + .method(Method::POST) + .uri(uri) + .header(header::COOKIE, format!("session={token}")) + .header("hx-request", "true"); + if let Some(csrf) = csrf { + builder = builder.header("x-csrf-token", csrf); + } + builder.body(Body::empty()).unwrap() +} + +// ─── The route exists and restores ───────────────────────────────── + +#[sqlx::test(migrations = "./migrations")] +async fn restore_clears_deleted_at_and_reports_success(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + let (title_id, title_name, version) = seed_soft_deleted_title(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/titles/{title_id}/restore?version={version}"), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::OK, + "the restore route must be registered — a 404 here is the #478 bug itself" + ); + // The conflict modal closes on success; harmless on the single-click path. + assert_eq!( + resp.headers() + .get("hx-trigger") + .and_then(|v| v.to_str().ok()), + Some("modal-close"), + ); + + let html = body_text(resp).await; + assert!( + html.contains("admin-trash-panel"), + "response should re-render the trash panel" + ); + assert!( + html.contains(&title_name), + "the success FeedbackEntry should name the restored item" + ); + + assert!( + deleted_at_of(&pool, "titles", title_id).await.is_none(), + "deleted_at should be NULL after a restore" + ); + let new_version: i32 = sqlx::query_scalar("SELECT version FROM titles WHERE id = ?") + .bind(title_id) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!(new_version, version + 1, "optimistic-lock version should bump"); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_writes_an_audit_row(pool: MySqlPool) { + let (admin_id, admin_token) = seed_session(&pool, "admin").await; + let (title_id, _title_name, version) = seed_soft_deleted_title(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/titles/{title_id}/restore?version={version}"), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let (actor, entity_id): (Option, Option) = sqlx::query_as( + "SELECT CAST(user_id AS SIGNED), CAST(entity_id AS SIGNED) FROM admin_audit \ + WHERE action = 'restore_from_trash' AND entity_type = 'titles'", + ) + .fetch_one(&pool) + .await + .expect("a restore should leave an audit row"); + + assert_eq!(actor, Some(admin_id as i64)); + assert_eq!(entity_id, Some(title_id as i64)); +} + +// ─── Guards ──────────────────────────────────────────────────────── + +#[sqlx::test(migrations = "./migrations")] +async fn restore_without_a_csrf_token_is_rejected(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + let (title_id, _name, version) = seed_soft_deleted_title(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/titles/{title_id}/restore?version={version}"), + &admin_token, + None, + )) + .await + .unwrap(); + + // The reason the route is POST rather than the GET the button used to + // emit: state-changing verbs ride the story 8-2 CSRF layer. + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + assert_eq!( + resp.headers() + .get("hx-trigger") + .and_then(|v| v.to_str().ok()), + Some("csrf-rejected"), + ); + assert!( + deleted_at_of(&pool, "titles", title_id).await.is_some(), + "a rejected request must not restore anything" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_is_refused_to_a_librarian(pool: MySqlPool) { + let (_lib_id, lib_token) = seed_session(&pool, "librarian").await; + let (title_id, _name, version) = seed_soft_deleted_title(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/titles/{title_id}/restore?version={version}"), + &lib_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + assert!( + deleted_at_of(&pool, "titles", title_id).await.is_some(), + "a librarian must not be able to restore" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_with_a_stale_version_returns_409(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + let (title_id, _name, version) = seed_soft_deleted_title(&pool).await; + + // Someone else touched the row after the panel was rendered. + sqlx::query("UPDATE titles SET version = version + 1 WHERE id = ?") + .bind(title_id) + .execute(&pool) + .await + .unwrap(); + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/titles/{title_id}/restore?version={version}"), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::CONFLICT); + assert!( + deleted_at_of(&pool, "titles", title_id).await.is_some(), + "a lost optimistic lock must leave the row in the trash" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_of_a_purged_row_returns_404(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + + let app = build_router(build_state(pool)); + let resp = app + .oneshot(restore_request( + "/admin/trash/titles/999999/restore?version=1".to_string(), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let html = body_text(resp).await; + // i18n-aware: the test stack's default language is FR, and the copy + // must be the operator-facing sentence in either language — never a + // bare 404 page. + assert!( + html.contains("no longer in the trash") || html.contains("plus dans la corbeille"), + "the operator should get the friendly copy, not a bare 404: {html}" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_rejects_a_table_outside_the_whitelist(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + + let app = build_router(build_state(pool)); + let resp = app + .oneshot(restore_request( + "/admin/trash/sessions/1/restore?version=1".to_string(), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "`table` is interpolated into SQL — only ALLOWED_TABLES may pass" + ); +} + +// ─── Conflict path ───────────────────────────────────────────────── + +/// Seed the issue-#66 shape: a soft-deleted series whose title has since +/// been reassigned to a live series. Returns the soft-deleted series id. +async fn seed_series_conflict(pool: &MySqlPool) -> u64 { + let old_series_id = sqlx::query("INSERT INTO series (name, version, deleted_at) VALUES (?, 1, NOW())") + .bind(format!("Old Series {}", rand_suffix())) + .execute(pool) + .await + .unwrap() + .last_insert_id(); + + let new_series_id = sqlx::query("INSERT INTO series (name, version) VALUES (?, 1)") + .bind(format!("New Series {}", rand_suffix())) + .execute(pool) + .await + .unwrap() + .last_insert_id(); + + let title_id = sqlx::query( + "INSERT INTO titles (title, media_type, genre_id, version) VALUES (?, 'book', 1, 1)", + ) + .bind("Reassigned Title") + .execute(pool) + .await + .unwrap() + .last_insert_id(); + + sqlx::query( + "INSERT INTO title_series (title_id, series_id, position_number, deleted_at) \ + VALUES (?, ?, 1, NOW())", + ) + .bind(title_id) + .bind(old_series_id) + .execute(pool) + .await + .unwrap(); + + sqlx::query("INSERT INTO title_series (title_id, series_id, position_number) VALUES (?, ?, 1)") + .bind(title_id) + .bind(new_series_id) + .execute(pool) + .await + .unwrap(); + + old_series_id +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_with_conflicts_returns_the_modal_and_changes_nothing(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + let series_id = seed_series_conflict(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/series/{series_id}/restore?version=1"), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + // The panel button targets #admin-trash-panel; the dialog belongs in + // the stable #modal-slot, so the response retargets. + assert_eq!( + resp.headers() + .get("hx-retarget") + .and_then(|v| v.to_str().ok()), + Some("#modal-slot"), + ); + assert_eq!( + resp.headers().get("hx-reswap").and_then(|v| v.to_str().ok()), + Some("innerHTML"), + ); + + let html = body_text(resp).await; + assert!(html.contains("data-modal-confirm"), "UX-DR8 macro shape expected"); + assert!(html.contains("data-modal-cancel"), "Cancel must be present"); + assert!( + html.contains("clear_conflicts=1"), + "Confirm should re-post the same route with the conflicts flag" + ); + assert!( + html.contains("Reassigned Title"), + "the modal should name the conflicting title: {html}" + ); + assert!( + html.contains(TEST_CSRF_TOKEN), + "story 8-2 invariant: the modal form carries the CSRF token" + ); + + assert!( + deleted_at_of(&pool, "series", series_id).await.is_some(), + "showing the conflict modal must not restore anything yet" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn restore_with_clear_conflicts_restores_and_drops_the_stale_link(pool: MySqlPool) { + let (_admin_id, admin_token) = seed_session(&pool, "admin").await; + let series_id = seed_series_conflict(&pool).await; + + let app = build_router(build_state(pool.clone())); + let resp = app + .oneshot(restore_request( + format!("/admin/trash/series/{series_id}/restore?version=1&clear_conflicts=1"), + &admin_token, + Some(TEST_CSRF_TOKEN), + )) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!( + resp.headers() + .get("hx-trigger") + .and_then(|v| v.to_str().ok()), + Some("modal-close"), + "the confirm pass must close the modal" + ); + + assert!( + deleted_at_of(&pool, "series", series_id).await.is_none(), + "the series should be restored" + ); + let stale_links: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM title_series WHERE series_id = ?") + .bind(series_id) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!( + stale_links, 0, + "the shadowed assignment to the restored series should be cleared" + ); +} diff --git a/tests/e2e/specs/journeys/admin-permanent-delete.spec.ts b/tests/e2e/specs/journeys/admin-permanent-delete.spec.ts index cf887185..f68a656f 100644 --- a/tests/e2e/specs/journeys/admin-permanent-delete.spec.ts +++ b/tests/e2e/specs/journeys/admin-permanent-delete.spec.ts @@ -61,7 +61,12 @@ test.describe("Story 8-7: Permanent Delete & Auto-Purge", () => { const row = await openTrashAndFindRow(page, name); // Click "Delete permanently" on our row → modal opens - const deleteBtn = row.locator("button[data-modal-trigger]"); + // Issue #478 gave the Restore button `data-modal-trigger` too (it can + // open the restore-conflict modal), so the delete button is addressed + // by its accessible name rather than by the marker attribute. + const deleteBtn = row.getByRole("button", { + name: /delete permanently|supprimer définitivement/i, + }); await expect(deleteBtn).toBeVisible(); await deleteBtn.click(); @@ -97,7 +102,9 @@ test.describe("Story 8-7: Permanent Delete & Auto-Purge", () => { const name = await seedSoftDeletedSeries(page, "SC2"); const row = await openTrashAndFindRow(page, name); - await row.locator("button[data-modal-trigger]").click(); + await row + .getByRole("button", { name: /delete permanently|supprimer définitivement/i }) + .click(); const modal = page.locator("#modal-slot dialog[open]"); await expect(modal).toBeVisible({ timeout: 5000 }); diff --git a/tests/e2e/specs/journeys/admin-trash-restore.spec.ts b/tests/e2e/specs/journeys/admin-trash-restore.spec.ts new file mode 100644 index 00000000..c043d1ba --- /dev/null +++ b/tests/e2e/specs/journeys/admin-trash-restore.spec.ts @@ -0,0 +1,116 @@ +import { test, expect, Page } from "@playwright/test"; +import { loginAs } from "../../helpers/auth"; +import { createBorrower } from "../../helpers/loans"; + +/** + * Issue #478 — the Trash panel's Restore journey. + * + * The button existed since story 8-6 but pointed at a route that was never + * registered: HTMX does not swap a 4xx, so a click produced nothing at all + * — no restore, no error, no feedback entry. Nothing in the suite noticed, + * because `TrashService::restore` was only ever called from `#[cfg(test)]`. + * + * The journey uses a **borrower**, not a series, on purpose: restoring + * leaves the entity live, and `empty-states.spec.ts` asserts that the + * /series list is empty. A live borrower collides with nothing. + */ + +/** Create a borrower, soft-delete it through its modal, return the name. */ +async function seedDeletedBorrower(page: Page, slug: string): Promise { + const name = `TR-${slug}-${Date.now() % 1000000}`; + await createBorrower(page, name); + + const link = page + .locator('tbody a[href^="/borrower/"]') + .filter({ hasText: new RegExp(`^\\s*${name}\\s*$`) }); + await link.click(); + await expect(page.locator("h1")).toContainText(name, { timeout: 5000 }); + + await page.locator("button[data-modal-trigger]").click(); + await expect(page.locator("#modal-slot dialog[open]")).toBeVisible({ + timeout: 5000, + }); + await page.locator("[data-modal-confirm]").click(); + await page.waitForURL("**/borrowers", { timeout: 10000 }); + return name; +} + +/** Open the Trash tab filtered to borrowers and return the row for `name`. */ +async function openTrashRow(page: Page, name: string) { + await page.goto("/admin?tab=trash", { waitUntil: "domcontentloaded" }); + await expect( + page.locator('section[aria-labelledby="admin-trash-heading"]'), + ).toBeVisible({ timeout: 10000 }); + await page.locator("#filter-entity-type").selectOption("borrowers"); + const row = page.locator("tbody tr").filter({ hasText: name }); + await expect(row).toBeVisible({ timeout: 10000 }); + return row; +} + +const restoreButton = (row: ReturnType) => + row.getByRole("button", { name: /^(restore|restaurer)$/i }); + +test.describe("Issue #478: restore from the Trash", () => { + test.beforeEach(async ({ page }) => { + await loginAs(page, "admin"); + }); + + test("delete a borrower, restore it from the Trash, find it back in the list", async ({ + page, + }) => { + const name = await seedDeletedBorrower(page, "SC1"); + const row = await openTrashRow(page, name); + + await restoreButton(row).click(); + + // The panel re-renders and the OOB feedback entry names the item. + await expect(page.locator(".feedback-entry").first()).toContainText( + new RegExp(`(Restored|Restauré).*${name}`, "i"), + { timeout: 10000 }, + ); + + // The row is gone from the Trash — the restore actually committed. + await expect(page.locator("tbody tr").filter({ hasText: name })).toHaveCount( + 0, + { timeout: 10000 }, + ); + + // And the borrower is back in its own list. + await page.goto("/borrowers"); + await expect( + page.locator('tbody a[href^="/borrower/"]').filter({ hasText: name }), + ).toBeVisible({ timeout: 10000 }); + }); + + test("replaying a restore from a stale panel says so instead of failing silently", async ({ + page, + }) => { + const name = await seedDeletedBorrower(page, "SC2"); + const row = await openTrashRow(page, name); + + // Capture the exact request the panel would re-send if the admin left + // the tab open and clicked twice. + const url = await restoreButton(row).getAttribute("hx-post"); + expect(url).toBeTruthy(); + + await restoreButton(row).click(); + await expect(page.locator(".feedback-entry").first()).toContainText( + new RegExp(`(Restored|Restauré).*${name}`, "i"), + { timeout: 10000 }, + ); + + // The row is no longer in the trash, so the replay gets the friendly + // "already gone" copy — never a silent no-op, which is the #478 + // symptom this spec exists to prevent. + const csrf = await page + .locator('meta[name="csrf-token"]') + .getAttribute("content"); + const replay = await page.request.post(url as string, { + headers: { "X-CSRF-Token": csrf as string, "HX-Request": "true" }, + }); + expect(replay.status()).toBe(404); + expect(await replay.text()).toMatch( + /no longer in the trash|plus dans la corbeille/i, + ); + }); +}); From bd7f64c0bf0148f8a69c39e70abe7af8bccae98c Mon Sep 17 00:00:00 2001 From: Guy Corbaz Date: Fri, 11 Sep 2026 00:38:45 +0200 Subject: [PATCH 2/2] fix(#478): translate the new trash key into de and it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tests/locale_parity.rs` requires every key to exist in all four locale files; `restore_error_not_found` had landed in en and fr only, which turned the DB-integration job red. CLAUDE.md said the locales were "en.yml + fr.yml" — stale since German and Italian were added, and the reason the omission looked correct while writing it. Fixed there too, with the parity test named so the next person runs it. Refs #478 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D5cV9QRqQAFpiNKZwZfoNX --- CLAUDE.md | 2 +- locales/de.yml | 1 + locales/it.yml | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 645fb803..7d4b59fd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,7 +119,7 @@ If/when multi-tenancy is on the roadmap, both #54 and #16 should be re-opened to - **Error handling:** `AppError` enum with `IntoResponse`. Conflict = 409, Unauthorized = 303 redirect with HX-Redirect. - **Logging:** `tracing` macros only — no `println!`. -- **i18n:** `rust_i18n::t!("key")` for ALL user-facing text. Keys in `locales/en.yml` + `locales/fr.yml`. JS strings: read `` and use embedded string map. **CRITICAL YAML FORMAT: locale files must NOT have `en:` or `fr:` as top-level wrapper — the filename determines the locale. Keys start at root level (e.g., `nav:` not `en: nav:`). After adding/changing keys, run `touch src/lib.rs` before `cargo build` to force proc macro recompilation.** +- **i18n:** `rust_i18n::t!("key")` for ALL user-facing text. Keys in **all four** locale files — `locales/{en,fr,de,it}.yml`. `tests/locale_parity.rs` fails the build on any key present in one file and missing from another, so a key added to en/fr alone turns the DB-integration job red; run `cargo test --test locale_parity` after touching a locale. JS strings: read `` and use embedded string map. **CRITICAL YAML FORMAT: locale files must NOT have `en:` or `fr:` as top-level wrapper — the filename determines the locale. Keys start at root level (e.g., `nav:` not `en: nav:`). After adding/changing keys, run `touch src/lib.rs` before `cargo build` to force proc macro recompilation.** - **DB queries:** MUST include `deleted_at IS NULL` in every SELECT/JOIN on entity tables. Every entity table has `deleted_at`, `version`, `created_at`, `updated_at` columns. **MariaDB type gotchas:** (1) `JSON` columns are stored as `BLOB` — use `CAST(col AS CHAR)` to read as String. (2) `BIGINT UNSIGNED NULL` columns — use `CAST(col AS SIGNED)` and read as `Option`, then convert to `u64`. (3) Never use `CAST(... AS UNSIGNED)` in SELECT — SQLx can't decode `BIGINT UNSIGNED` into Rust integers reliably. (4) `TIMESTAMP` columns in dynamic queries (`sqlx::query()`) — use `CAST(col AS DATETIME) AS col` to read as `NaiveDateTime`. Without CAST, SQLx returns a type mismatch error. This does NOT affect typed macros (`sqlx::query!`) which handle conversion automatically. - **Optimistic locking:** UPDATE with `WHERE id = ? AND version = ?`, then `check_update_result()` from `services/locking.rs`. - **Soft delete:** `services/soft_delete.rs` with table whitelist. Never hard-delete. diff --git a/locales/de.yml b/locales/de.yml index da21e793..c74a64ff 100644 --- a/locales/de.yml +++ b/locales/de.yml @@ -823,6 +823,7 @@ admin: days_remaining_warning: "%{days} Tage" days_remaining_critical: "<7 Tage" restore_success: "Wiederhergestellt: %{name}" + restore_error_not_found: "Dieser Eintrag liegt nicht mehr im Papierkorb. Er wurde vermutlich schon wiederhergestellt oder endgültig gelöscht." delete_permanent_success: "Endgültig gelöscht: %{name}" delete_permanent_modal_title: "Endgültig löschen?" delete_permanent_modal_warning: "Dies kann nicht rückgängig gemacht werden." diff --git a/locales/it.yml b/locales/it.yml index 572fab9f..4df0ce75 100644 --- a/locales/it.yml +++ b/locales/it.yml @@ -811,6 +811,7 @@ admin: days_remaining_warning: "%{days} giorno" days_remaining_critical: "<7 giorni" restore_success: "Ripristinato: %{name}" + restore_error_not_found: "Questo elemento non è più nel cestino. Probabilmente è già stato ripristinato o eliminato definitivamente." delete_permanent_success: "Eliminato definitivamente: %{name}" delete_permanent_modal_title: "Eliminare definitivamente?" delete_permanent_modal_warning: "Questa azione non può essere annullata."