diff --git a/.github/workflows/backend-Tests.yml b/.github/workflows/backend-Tests.yml index 64b1e25..a29867f 100644 --- a/.github/workflows/backend-Tests.yml +++ b/.github/workflows/backend-Tests.yml @@ -34,6 +34,10 @@ jobs: env: DATABASE_URL: postgresql+psycopg://cms:cmspass@localhost:5432/cmstest ALEMBIC_UPGRADE_HEAD_ON_START: false + STAGING_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + STAGING_BASE_PATH: staging + QUARANTINE_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + QUARANTINE_BASE_PATH: quarantine steps: - name: Retrieve source code @@ -78,6 +82,8 @@ jobs: --network host \ -e DATABASE_URL=$DATABASE_URL \ -e JWT_SECRET=DH8kSxcflUVfNRdkEiJJCn2dOOKI3qfw \ + -e STAGING_WAREHOUSE_ID=11111111-1111-1111-1111-111111111111 \ + -e QUARANTINE_WAREHOUSE_ID=11111111-1111-1111-1111-111111111111 \ -p 8000:80 \ cms-backend-api:test # wait for container to be ready @@ -110,6 +116,8 @@ jobs: docker run -d --name cms-shuttle-test \ --network host \ -e DATABASE_URL=$DATABASE_URL \ + -e QUARANTINE_WAREHOUSE_ID=11111111-1111-1111-1111-111111111111 \ + -e STAGING_WAREHOUSE_ID=11111111-1111-1111-1111-111111111111 \ cms-backend-shuttle:test cms-shuttle --help - name: Run tests diff --git a/backend/src/cms_backend/api/routes/books.py b/backend/src/cms_backend/api/routes/books.py index 60b4cad..a55d0bc 100644 --- a/backend/src/cms_backend/api/routes/books.py +++ b/backend/src/cms_backend/api/routes/books.py @@ -1,13 +1,18 @@ -from typing import Annotated +from typing import Annotated, Literal from uuid import UUID from fastapi import APIRouter, Depends, Path, Query from sqlalchemy.orm import Session as OrmSession +from cms_backend.api.routes.dependencies import require_permission from cms_backend.api.routes.fields import LimitFieldMax200, NotEmptyString, SkipField from cms_backend.api.routes.models import ListResponse, calculate_pagination_metadata from cms_backend.db import gen_dbsession -from cms_backend.db.books import get_book as db_get_book +from cms_backend.db.book import create_book_full_schema +from cms_backend.db.book import delete_book as db_delete_book +from cms_backend.db.book import get_book as db_get_book +from cms_backend.db.book import move_book as db_move_book +from cms_backend.db.book import recover_book as db_recover_book from cms_backend.db.books import get_books as db_get_books from cms_backend.db.books import get_zim_urls as db_get_zim_urls from cms_backend.schemas import BaseModel @@ -15,12 +20,15 @@ from cms_backend.schemas.orms import ( BookFullSchema, BookLightSchema, - BookLocationSchema, ) router = APIRouter(prefix="/books", tags=["books"]) +class BookMoveSchema(BaseModel): + destination: Literal["prod", "staging"] + + class BooksGetSchema(BaseModel): skip: SkipField = 0 limit: LimitFieldMax200 = 20 @@ -76,49 +84,44 @@ def get_book( session: Annotated[OrmSession, Depends(gen_dbsession)], ) -> BookFullSchema: """Get a book by ID""" + return create_book_full_schema(db_get_book(session=session, book_id=book_id)) + + +@router.delete( + "/{book_id}", + dependencies=[Depends(require_permission(namespace="book", name="delete"))], +) +def delete_book( + book_id: Annotated[UUID, Path()], + session: Annotated[OrmSession, Depends(gen_dbsession)], + *, + force_delete: Annotated[bool, Query()] = False, +) -> BookFullSchema: + return create_book_full_schema( + db_delete_book(session, book_id=book_id, force_delete=force_delete) + ) + - db_book = db_get_book(session=session, book_id=book_id) - - # Separate current and target locations - current_locations = [ - BookLocationSchema( - warehouse_name=location.warehouse.name, - path=str(location.path), - filename=location.filename, - status=location.status, - ) - for location in db_book.locations - if location.status == "current" - ] - - target_locations = [ - BookLocationSchema( - warehouse_name=location.warehouse.name, - path=str(location.path), - filename=location.filename, - status=location.status, - ) - for location in db_book.locations - if location.status == "target" - ] - - return BookFullSchema( - id=db_book.id, - title_id=db_book.title_id, - needs_processing=db_book.needs_processing, - has_error=db_book.has_error, - needs_file_operation=db_book.needs_file_operation, - location_kind=db_book.location_kind, - created_at=db_book.created_at, - name=db_book.name, - date=db_book.date, - flavour=db_book.flavour, - article_count=db_book.article_count, - media_count=db_book.media_count, - size=db_book.size, - zimcheck_result_url=db_book.zimcheck_result_url, - zim_metadata=db_book.zim_metadata, - events=db_book.events, - current_locations=current_locations, - target_locations=target_locations, +@router.post( + "/{book_id}/recover", + dependencies=[Depends(require_permission(namespace="book", name="update"))], +) +def recover_book( + book_id: Annotated[UUID, Path()], + session: Annotated[OrmSession, Depends(gen_dbsession)], +) -> BookFullSchema: + return create_book_full_schema(db_recover_book(session, book_id=book_id)) + + +@router.post( + "/{book_id}/move", + dependencies=[Depends(require_permission(namespace="book", name="update"))], +) +def move_book( + book_id: Annotated[UUID, Path()], + session: Annotated[OrmSession, Depends(gen_dbsession)], + request: BookMoveSchema, +) -> BookFullSchema: + return create_book_full_schema( + db_move_book(session, book_id=book_id, destination=request.destination) ) diff --git a/backend/src/cms_backend/context.py b/backend/src/cms_backend/context.py index 0135b70..e3b0dbb 100644 --- a/backend/src/cms_backend/context.py +++ b/backend/src/cms_backend/context.py @@ -1,7 +1,12 @@ import dataclasses import os +from dataclasses import field +from datetime import timedelta from pathlib import Path from typing import Any, TypeVar +from uuid import UUID + +from humanfriendly import parse_timespan T = TypeVar("T") @@ -37,3 +42,18 @@ class Context: alembic_upgrade_head_on_start: bool = parse_bool( get_mandatory_env("ALEMBIC_UPGRADE_HEAD_ON_START") ) + + # delay before old books are deleted + old_book_deletion_delay: timedelta = timedelta( + seconds=parse_timespan(os.getenv("OLD_BOOK_DELETION_DELAY", default="1d")) + ) + staging_warehouse_id: UUID = field( + default=UUID(get_mandatory_env("STAGING_WAREHOUSE_ID")) + ) + staging_base_path: Path = field(default=Path(os.getenv("STAGING_BASE_PATH", ""))) + quarantine_warehouse_id: UUID = field( + default=UUID(get_mandatory_env("QUARANTINE_WAREHOUSE_ID")) + ) + quarantine_base_path: Path = field( + default=Path(os.getenv("QUARANTINE_BASE_PATH", "")) + ) diff --git a/backend/src/cms_backend/db/book.py b/backend/src/cms_backend/db/book.py index d9c6d25..e39ef1d 100644 --- a/backend/src/cms_backend/db/book.py +++ b/backend/src/cms_backend/db/book.py @@ -1,12 +1,16 @@ from dataclasses import dataclass from pathlib import Path -from typing import Any +from typing import Any, Literal from uuid import UUID from sqlalchemy import select from sqlalchemy.orm import Session as OrmSession +from sqlalchemy.orm import selectinload +from cms_backend.context import Context +from cms_backend.db.exceptions import RecordDoesNotExistError from cms_backend.db.models import Book, BookLocation, Warehouse, ZimfarmNotification +from cms_backend.schemas.orms import BookFullSchema, BookLocationSchema from cms_backend.utils.datetime import getnow @@ -17,6 +21,91 @@ class FileLocation: filename: str +def get_book_or_none( + session: OrmSession, + book_id: UUID, + *, + needs_file_operation: bool | None = None, + needs_processing: bool | None = None, + locations: list[str] | None = None, + has_error: bool | None = None, +) -> Book | None: + """Get a book by ID if possible else None""" + return session.scalars( + select(Book) + .where( + # If a client provides an argument i.e it is not None, + # we compare the corresponding model field against the argument, + # otherwise, we compare the argument to its default which translates + # to a SQL true i.e we don't filter based on this argument (a no-op). + (Book.id == book_id), + (Book.needs_file_operation.is_(needs_file_operation)) + | (needs_file_operation is None), + (Book.needs_processing.is_(needs_processing)) | (needs_processing is None), + (Book.has_error.is_(has_error)) | (has_error is None), + (Book.location_kind.in_(locations or [])) | (locations is None), + ) + .options( + selectinload(Book.title), + selectinload(Book.zimfarm_notification), + selectinload(Book.locations), + ) + ).one_or_none() + + +def get_book(session: OrmSession, book_id: UUID) -> Book: + """Get a book by ID if possible else raise an exception""" + if (book := get_book_or_none(session, book_id=book_id)) is None: + raise RecordDoesNotExistError(f"Book with ID {book_id} does not exist") + return book + + +def create_book_full_schema(book: Book) -> BookFullSchema: + # Separate current and target locations + current_locations = [ + BookLocationSchema( + warehouse_name=location.warehouse.name, + path=str(location.path), + filename=location.filename, + status=location.status, + ) + for location in book.locations + if location.status == "current" + ] + + target_locations = [ + BookLocationSchema( + warehouse_name=location.warehouse.name, + path=str(location.path), + filename=location.filename, + status=location.status, + ) + for location in book.locations + if location.status == "target" + ] + + return BookFullSchema( + id=book.id, + title_id=book.title_id, + needs_processing=book.needs_processing, + has_error=book.has_error, + needs_file_operation=book.needs_file_operation, + location_kind=book.location_kind, + created_at=book.created_at, + name=book.name, + date=book.date, + flavour=book.flavour, + article_count=book.article_count, + media_count=book.media_count, + size=book.size, + zimcheck_result_url=book.zimcheck_result_url, + zim_metadata=book.zim_metadata, + events=book.events, + current_locations=current_locations, + target_locations=target_locations, + ) + + def create_book( session: OrmSession, *, @@ -213,3 +302,167 @@ def get_next_book_to_process_or_none( .order_by(Book.created_at) .limit(1) ).one_or_none() + + +def delete_book( + session: OrmSession, *, book_id: UUID, force_delete: bool = False +) -> Book: + """Mark a book as deleted. + + A book is marked as deleted depending on it's location kind and/or status. + - Books in quarantine can be immediately deleted if they have an error. + - Books in staging/prod are marked as deleted without consideration. + """ + now = getnow() + book = get_book_or_none( + session, + book_id=book_id, + needs_processing=False, + needs_file_operation=False, + locations=["staging", "prod", "quarantine"], + ) + if book is None: + raise RecordDoesNotExistError( + f"Book {book_id} does not meet criteria to be deleted." + ) + + deletion_date = now if force_delete else now + Context.old_book_deletion_delay + + if book.location_kind == "quarantine": + if book.has_error: + book.deletion_date = now + else: + book.deletion_date = deletion_date + elif book.location_kind in ("staging", "prod"): + book.deletion_date = deletion_date + else: # should never get here because of filtering by locations + raise ValueError( + "Cannot determine how to delete book with location kind " + f"'{book.location_kind}'" + ) + book.location_kind = "to_delete" + book.needs_file_operation = True + book.events.append( + f"{now}: marked for deletion, will be deleted after {deletion_date}" + ) + session.add(book) + session.flush() + return book + + +def move_book( + session: OrmSession, *, book_id: UUID, destination: Literal["staging", "prod"] +) -> Book: + """Move a book in staging/prod to prod/staging. + + Destination location must be different from current location. + """ + book = get_book_or_none( + session, + book_id=book_id, + needs_file_operation=False, + needs_processing=False, + locations=["staging", "prod"], + has_error=False, + ) + + if book is None: + raise RecordDoesNotExistError( + f"Book {book_id} does not meet criteria to be moved." + ) + + if book.location_kind == destination: + raise ValueError("Book destination must be different from current location.") + + current_location = next( + (loc for loc in book.locations if loc.status == "current"), None + ) + if not current_location: + raise ValueError(f"Book {book_id} has no current location") + + if not book.title: + raise ValueError(f"Book {book_id} has no associated title.") + + existing_filename = current_location.filename + + goes_to_staging = destination == "staging" + target_locations = ( + [ + FileLocation( + Context.staging_warehouse_id, + Context.staging_base_path, + existing_filename, + ) + ] + if goes_to_staging + else [ + FileLocation(tc.collection.warehouse_id, tc.path, existing_filename) + for tc in book.title.collections + ] + ) + + create_book_target_locations( + session=session, + book=book, + target_locations=target_locations, + ) + book.events.append( + f"{getnow()}: Book scheduled to be moved from '{book.location_kind}' to " + f"'{destination}'" + ) + book.location_kind = "staging" if goes_to_staging else "prod" + session.add(book) + session.flush() + return book + + +def determine_current_location_kind( + book: Book, +) -> Literal["prod", "staging", "quarantine"]: + """Determine the location kind of a book based on its current locations.""" + current_locations = [loc for loc in book.locations if loc.status == "current"] + if not current_locations: + raise ValueError(f"Book {book.id} has no current location.") + + for loc in current_locations: + if loc.warehouse_id == Context.quarantine_warehouse_id: + try: + loc.path.relative_to(Context.quarantine_base_path) + return "quarantine" + except ValueError: + pass + + if loc.warehouse_id == Context.staging_warehouse_id: + try: + loc.path.relative_to(Context.staging_base_path) + return "staging" + except ValueError: + pass + + return "prod" + + +def recover_book(session: OrmSession, book_id: UUID) -> Book: + """Recover a book marked for deletion.""" + now = getnow() + book = get_book_or_none( + session, + book_id, + needs_file_operation=True, + needs_processing=False, + locations=["to_delete"], + ) + if book is None or (book.deletion_date and book.deletion_date <= now): + raise RecordDoesNotExistError(f"Book {book_id} is not eligible for deletion.") + + location_kind = determine_current_location_kind(book) + book.needs_processing = False + book.events.append( + f"{now}: Book restored from {book.location_kind} to {location_kind}" + ) + book.location_kind = location_kind + book.deletion_date = None + book.needs_file_operation = False + session.add(book) + session.flush() + return book diff --git a/backend/src/cms_backend/db/books.py b/backend/src/cms_backend/db/books.py index 64bebb8..99e92dc 100644 --- a/backend/src/cms_backend/db/books.py +++ b/backend/src/cms_backend/db/books.py @@ -3,34 +3,14 @@ from pydantic import AnyUrl from sqlalchemy import String, and_, select from sqlalchemy.orm import Session as OrmSession -from sqlalchemy.orm import selectinload from cms_backend.db import count_from_stmt -from cms_backend.db.exceptions import ( - RecordDoesNotExistError, -) from cms_backend.db.models import Book, BookLocation, Collection, CollectionTitle, Title from cms_backend.schemas.models import ZimUrlSchema, ZimUrlsSchema from cms_backend.schemas.orms import BookLightSchema, ListResult from cms_backend.utils.filename import construct_download_url -def get_book_or_none(session: OrmSession, book_id: UUID) -> Book | None: - """Get a book by ID if possible else None""" - return session.scalars( - select(Book) - .where(Book.id == book_id) - .options(selectinload(Book.title), selectinload(Book.zimfarm_notification)) - ).one_or_none() - - -def get_book(session: OrmSession, book_id: UUID) -> Book: - """Get a book by ID if possible else raise an exception""" - if (book := get_book_or_none(session, book_id=book_id)) is None: - raise RecordDoesNotExistError(f"Book with ID {book_id} does not exist") - return book - - def get_books( session: OrmSession, *, diff --git a/backend/src/cms_backend/db/collection.py b/backend/src/cms_backend/db/collection.py index 1f75722..1de679e 100644 --- a/backend/src/cms_backend/db/collection.py +++ b/backend/src/cms_backend/db/collection.py @@ -92,6 +92,7 @@ def get_latest_books_for_collection( .join(Collection) .where( and_( + Book.location_kind.in_(["prod", "staging", "quarantine"]), BookLocation.status == "current", BookLocation.warehouse_id == Collection.warehouse_id, BookLocation.path == CollectionTitle.path, diff --git a/backend/src/cms_backend/mill/context.py b/backend/src/cms_backend/mill/context.py index c6f7e2a..0bd25fa 100644 --- a/backend/src/cms_backend/mill/context.py +++ b/backend/src/cms_backend/mill/context.py @@ -1,14 +1,10 @@ import os -from dataclasses import dataclass, field +from dataclasses import dataclass from datetime import timedelta -from pathlib import Path from typing import TypeVar -from uuid import UUID from humanfriendly import parse_timespan -from cms_backend.context import get_mandatory_env - T = TypeVar("T") @@ -35,19 +31,3 @@ class Context: os.getenv("PROCESS_TILTE_MODIFICATIONS_INTERVAL", default="1m") ) ) - - quarantine_warehouse_id: UUID = field( - default=UUID(get_mandatory_env("QUARANTINE_WAREHOUSE_ID")) - ) - quarantine_base_path: Path = field( - default=Path(os.getenv("QUARANTINE_BASE_PATH", "")) - ) - - staging_warehouse_id: UUID = field( - default=UUID(get_mandatory_env("STAGING_WAREHOUSE_ID")) - ) - staging_base_path: Path = field(default=Path(os.getenv("STAGING_BASE_PATH", ""))) - - old_book_deletion_delay: timedelta = timedelta( - seconds=parse_timespan(os.getenv("OLD_BOOK_DELETION_DELAY", default="1d")) - ) diff --git a/backend/src/cms_backend/mill/processors/title.py b/backend/src/cms_backend/mill/processors/title.py index 8d76700..a2ad314 100644 --- a/backend/src/cms_backend/mill/processors/title.py +++ b/backend/src/cms_backend/mill/processors/title.py @@ -4,9 +4,9 @@ from sqlalchemy.orm import Session as OrmSession from cms_backend import logger +from cms_backend.context import Context from cms_backend.db.book import FileLocation, create_book_target_locations from cms_backend.db.models import Book, Title -from cms_backend.mill.context import Context as MillContext from cms_backend.utils.datetime import getnow from cms_backend.utils.filename import ( PERIOD_LENGTH, @@ -60,7 +60,7 @@ def apply_retention_rules(session: OrmSession, title: Title): for period in sorted_periods[2:]: books_to_delete.extend(books_by_period[period]) - deletion_date = now + MillContext.old_book_deletion_delay + deletion_date = now + Context.old_book_deletion_delay for book in books_to_delete: logger.info( @@ -117,8 +117,8 @@ def add_book_to_title(session: OrmSession, book: Book, title: Title): target_locations = ( [ FileLocation( - MillContext.staging_warehouse_id, - MillContext.staging_base_path, + Context.staging_warehouse_id, + Context.staging_base_path, target_filename, ) ] diff --git a/backend/src/cms_backend/mill/processors/zimfarm_notification.py b/backend/src/cms_backend/mill/processors/zimfarm_notification.py index 9cf90cd..ab468e4 100644 --- a/backend/src/cms_backend/mill/processors/zimfarm_notification.py +++ b/backend/src/cms_backend/mill/processors/zimfarm_notification.py @@ -1,9 +1,9 @@ from sqlalchemy.orm import Session as ORMSession from cms_backend import logger +from cms_backend.context import Context from cms_backend.db.book import create_book, create_book_location from cms_backend.db.models import ZimfarmNotification -from cms_backend.mill.context import Context as MillContext from cms_backend.mill.processors.book import process_book from cms_backend.utils.datetime import getnow @@ -85,8 +85,8 @@ def process_notification(session: ORMSession, notification: ZimfarmNotification) create_book_location( session=session, book=book, - warehouse_id=MillContext.quarantine_warehouse_id, - path=MillContext.quarantine_base_path / folder_name, + warehouse_id=Context.quarantine_warehouse_id, + path=Context.quarantine_base_path / folder_name, filename=filename, status="current", ) diff --git a/backend/src/cms_backend/shuttle/delete_files.py b/backend/src/cms_backend/shuttle/delete_files.py index 6eafaea..e74536f 100644 --- a/backend/src/cms_backend/shuttle/delete_files.py +++ b/backend/src/cms_backend/shuttle/delete_files.py @@ -33,7 +33,7 @@ def delete_files(session: OrmSession): f"{getnow()}: error encountered while deleting files\n{exc}" ) logger.exception(f"Failed to delete files for book {book.id}") - book.has_error = True + book.needs_file_operation = False logger.info(f"Done deleting {nb_zim_files_deleted} ZIM files") @@ -45,7 +45,6 @@ def get_next_book_to_delete(session: OrmSession, now: datetime.datetime) -> Book .where( Book.location_kind == "to_delete", Book.deletion_date <= now, - Book.has_error.is_(False), Book.needs_file_operation.is_(True), ) .order_by(Book.deletion_date) diff --git a/backend/src/cms_backend/shuttle/move_files.py b/backend/src/cms_backend/shuttle/move_files.py index 9ff038c..e31ced9 100644 --- a/backend/src/cms_backend/shuttle/move_files.py +++ b/backend/src/cms_backend/shuttle/move_files.py @@ -10,7 +10,6 @@ def move_files(session: OrmSession): - logger.info("Moving ZIM files") nb_zim_files_moved = 0 while True: with session.begin_nested(): @@ -18,6 +17,8 @@ def move_files(session: OrmSession): if not book: break + logger.info("Moving ZIM files") + try: logger.debug(f"Processing ZIM file of book {book.id}") move_book_files(session, book) diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index f62eaf2..ce16fa3 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -11,6 +11,7 @@ from werkzeug.security import generate_password_hash from cms_backend.api.token import generate_access_token +from cms_backend.context import Context from cms_backend.db import Session from cms_backend.db.models import ( Base, @@ -193,7 +194,7 @@ def _create_warehouse( def warehouse( create_warehouse: Callable[..., Warehouse], ) -> Warehouse: - return create_warehouse() + return create_warehouse(warehouse_id=Context.staging_warehouse_id) @pytest.fixture diff --git a/backend/tests/db/test_books.py b/backend/tests/db/test_books.py index 30ec104..25f8b9c 100644 --- a/backend/tests/db/test_books.py +++ b/backend/tests/db/test_books.py @@ -6,7 +6,15 @@ import pytest from sqlalchemy.orm import Session as OrmSession -from cms_backend.db.books import get_book, get_book_or_none, get_books, get_zim_urls +from cms_backend.context import Context +from cms_backend.db.book import ( + delete_book, + get_book, + get_book_or_none, + move_book, + recover_book, +) +from cms_backend.db.books import get_books, get_zim_urls from cms_backend.db.exceptions import RecordDoesNotExistError from cms_backend.db.models import ( Book, @@ -361,6 +369,199 @@ def test_get_zim_urls_book_with_subpath( assert view_url.collection == collection.name +@pytest.mark.parametrize( + "location_kind,force_delete", + [ + pytest.param("staging", False, id="staging"), + pytest.param("prod", True, id="prod"), + ], +) +def test_delete_book( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], + monkeypatch: pytest.MonkeyPatch, + *, + location_kind: str, + force_delete: bool, +): + """Test deleting a book""" + warehouse = create_warehouse() + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = location_kind + create_book_location( + book=book, + warehouse_id=warehouse.id, + path=Path(""), + filename="test_en_all_2024-01.zim", + status="current", + ) + dbsession.flush() + + now = getnow() + monkeypatch.setattr("cms_backend.db.book.getnow", lambda: now) + delete_book(dbsession, book_id=book.id, force_delete=force_delete) + + book = get_book(dbsession, book_id=book.id) + + assert book.location_kind == "to_delete" + assert book.needs_file_operation is True + assert book.deletion_date is not None + if force_delete: + assert book.deletion_date == now + else: + assert book.deletion_date > now + + +@pytest.mark.parametrize( + "has_error", + [pytest.param(True, id="has-error"), pytest.param(False, id="has-no-error")], +) +def test_delete_book_in_quarantine( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], + monkeypatch: pytest.MonkeyPatch, + *, + has_error: bool, +): + """Test deleting a book in quarantine with error""" + warehouse = create_warehouse() + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "quarantine" + book.has_error = has_error + create_book_location( + book=book, + warehouse_id=warehouse.id, + path=Path(""), + filename="test_en_all_2024-01.zim", + status="current", + ) + dbsession.flush() + + now = getnow() + monkeypatch.setattr("cms_backend.db.book.getnow", lambda: now) + delete_book(dbsession, book_id=book.id) + + book = get_book(dbsession, book_id=book.id) + + assert book.location_kind == "to_delete" + assert book.needs_file_operation is True + assert book.deletion_date is not None + if has_error: + assert book.deletion_date == now + else: + assert book.deletion_date > now + + +def test_move_book_staging_to_prod( + dbsession: OrmSession, + warehouse: Warehouse, + create_book: Callable[..., Book], + create_title: Callable[..., Title], + create_collection: Callable[..., Collection], + create_collection_title: Callable[..., CollectionTitle], + create_book_location: Callable[..., BookLocation], + monkeypatch: pytest.MonkeyPatch, +): + """Test moving a book from staging to prod""" + title = create_title(name="test_en_all") + collection = create_collection(warehouse=warehouse) + create_collection_title(title=title, collection=collection, path=Path("zim")) + + book = create_book(name="test_en_all", date="2024-01") + book.title = title + book.location_kind = "staging" + create_book_location( + book=book, + warehouse_id=Context.staging_warehouse_id, + path=Context.staging_base_path, + filename="test_en_all_2024-01.zim", + status="current", + ) + dbsession.flush() + + now = getnow() + monkeypatch.setattr("cms_backend.db.book.getnow", lambda: now) + move_book(dbsession, book_id=book.id, destination="prod") + + book = get_book(dbsession, book_id=book.id) + + assert book.location_kind == "prod" + assert book.needs_file_operation is True + + # Check that target locations were created + target_locations = [loc for loc in book.locations if loc.status == "target"] + assert len(target_locations) == 1 + assert target_locations[0].warehouse_id == warehouse.id + assert target_locations[0].path == Path("zim") + assert target_locations[0].filename == "test_en_all_2024-01.zim" + + +def test_move_book_same_destination_raises_error( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], +): + """Test that moving a book to its current location raises an error""" + warehouse = create_warehouse() + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "staging" + create_book_location( + book=book, + warehouse_id=warehouse.id, + path=Path(""), + filename="test_en_all_2024-01.zim", + status="current", + ) + dbsession.flush() + + with pytest.raises( + ValueError, match="Book destination must be different from current location" + ): + move_book(dbsession, book_id=book.id, destination="staging") + + +def test_move_book_no_current_location_raises_error( + dbsession: OrmSession, + create_book: Callable[..., Book], +): + """Test that moving a book without a current location raises an error""" + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "staging" + dbsession.flush() + + with pytest.raises(ValueError, match="has no current location"): + move_book(dbsession, book_id=book.id, destination="prod") + + +def test_move_book_no_title_raises_error( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], +): + """Test that moving a book without an associated title raises an error""" + warehouse = create_warehouse() + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "staging" + book.title = None + create_book_location( + book=book, + warehouse_id=warehouse.id, + path=Path(""), + filename="test_en_all_2024-01.zim", + status="current", + ) + dbsession.flush() + + with pytest.raises(ValueError, match="has no associated title"): + move_book(dbsession, book_id=book.id, destination="prod") + + def test_get_zim_urls_single_view_link_for_multiple_books_with_same_title_flavour( dbsession: OrmSession, create_book: Callable[..., Book], @@ -426,3 +627,104 @@ def test_get_zim_urls_single_view_link_for_multiple_books_with_same_title_flavou book2_view_url = next((u for u in result.urls[book2.id] if u.kind == "view"), None) assert book2_view_url is None + + +@pytest.mark.parametrize( + "location_kind,warehouse_id,path", + [ + pytest.param( + "quarantine", + Context.quarantine_warehouse_id, + Context.quarantine_base_path, + id="quarantine", + ), + pytest.param( + "staging", + Context.staging_warehouse_id, + Context.staging_base_path, + id="staging", + ), + pytest.param( + "prod", + uuid4(), + Path("zim"), + id="prod", + ), + ], +) +def test_recover_book( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], + *, + location_kind: str, + warehouse_id: str, + path: Path, +): + """Test recovering a book marked for deletion""" + warehouse = create_warehouse(warehouse_id=warehouse_id) + book = create_book(name="test_en_all", date="2024-01") + create_book_location( + book=book, + warehouse_id=warehouse.id, + path=path, + filename="test_en_all_2024-01.zim", + status="current", + ) + + now = getnow() + book.location_kind = "to_delete" + book.needs_file_operation = True + book.deletion_date = now + datetime.timedelta(days=1) + dbsession.flush() + + book = recover_book(dbsession, book_id=book.id) + + assert book.location_kind == location_kind + assert book.needs_file_operation is False + assert book.deletion_date is None + assert f"Book restored from to_delete to {location_kind}" in book.events[-1] + + +def test_recover_book_with_past_deletion_date( + dbsession: OrmSession, + create_book: Callable[..., Book], +): + """Test that recovering a book with past deletion_date raises error""" + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "to_delete" + book.deletion_date = getnow() + dbsession.flush() + + with pytest.raises(RecordDoesNotExistError, match="not eligible for deletion"): + recover_book(dbsession, book_id=book.id) + + +def test_recover_book_not_marked_for_deletion( + dbsession: OrmSession, + create_book: Callable[..., Book], +): + """Test that recovering a book not marked for deletion raises an error""" + book = create_book(name="test_en_all", date="2024-01") + book.location_kind = "staging" + dbsession.flush() + + with pytest.raises(RecordDoesNotExistError, match="not eligible for deletion"): + recover_book(dbsession, book_id=book.id) + + +def test_recover_book_no_current_location( + dbsession: OrmSession, + create_book: Callable[..., Book], +): + """Test that recovering a book without current location raises an error""" + book = create_book(name="test_en_all", date="2024-01") + + book.location_kind = "to_delete" + book.needs_file_operation = True + book.deletion_date = getnow() + datetime.timedelta(days=1) + dbsession.flush() + + with pytest.raises(ValueError, match="has no current location"): + recover_book(dbsession, book_id=book.id) diff --git a/backend/tests/mill/processors/conftest.py b/backend/tests/mill/processors/conftest.py index 279603e..e69de29 100644 --- a/backend/tests/mill/processors/conftest.py +++ b/backend/tests/mill/processors/conftest.py @@ -1,53 +0,0 @@ -"""Fixtures for mill processors tests.""" - -import os -from collections.abc import Callable -from uuid import UUID - -import pytest - -from cms_backend.db.models import Warehouse - - -def pytest_configure(config): # pyright: ignore # noqa: ARG001 - """Configure pytest and set up environment variables before imports. - - This is called before any tests are collected, so MillContext will read - these environment variables when it's imported. - """ - # Use fixed UUIDs for test warehouses - os.environ.setdefault( - "QUARANTINE_WAREHOUSE_ID", "00000000-0000-0000-0000-000000000001" - ) - os.environ.setdefault( - "STAGING_WAREHOUSE_ID", "00000000-0000-0000-0000-000000000002" - ) - os.environ.setdefault("QUARANTINE_BASE_PATH", "quarantine") - os.environ.setdefault("STAGING_BASE_PATH", "staging") - - -@pytest.fixture -def quarantine_warehouse(create_warehouse: Callable[..., Warehouse]) -> Warehouse: - """Create the quarantine warehouse with the expected test ID.""" - return create_warehouse( - name="quarantine", warehouse_id=UUID("00000000-0000-0000-0000-000000000001") - ) - - -@pytest.fixture -def staging_warehouse(create_warehouse: Callable[..., Warehouse]) -> Warehouse: - """Create the staging warehouse with the expected test ID.""" - return create_warehouse( - name="staging", warehouse_id=UUID("00000000-0000-0000-0000-000000000002") - ) - - -@pytest.fixture(autouse=True) -def _ensure_warehouses( # pyright: ignore[reportUnusedFunction] - quarantine_warehouse: Warehouse, - staging_warehouse: Warehouse, -) -> None: - """Ensure test warehouses exist in the database. - - This fixture is autouse so that every test has the warehouses it needs. - """ diff --git a/backend/tests/mill/processors/test_zimfarm_notification.py b/backend/tests/mill/processors/test_zimfarm_notification.py index f9086b1..538ef7b 100644 --- a/backend/tests/mill/processors/test_zimfarm_notification.py +++ b/backend/tests/mill/processors/test_zimfarm_notification.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import Session as OrmSession +from cms_backend.context import Context from cms_backend.db.models import ( Book, Collection, @@ -19,7 +20,6 @@ Warehouse, ZimfarmNotification, ) -from cms_backend.mill.context import Context as MillContext from cms_backend.mill.processors.zimfarm_notification import process_notification VALID_NOTIFICATION_CONTENT = { @@ -127,6 +127,7 @@ class TestValidNotificationNoMatchingTitle: def test_creates_book_in_quarantine( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], ): """Valid notification, no matching title → book created in quarantine.""" @@ -160,6 +161,7 @@ def test_creates_book_in_quarantine( def test_creates_book_with_empty_folder_name( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], ): """ @@ -190,7 +192,7 @@ def test_creates_book_with_empty_folder_name( location = book.locations[0] assert location.filename == VALID_NOTIFICATION_CONTENT["filename"] assert location.status == "current" - assert location.path == MillContext.quarantine_base_path + assert location.path == Context.quarantine_base_path # Verify book is not in error state assert book.has_error is False @@ -205,6 +207,7 @@ class TestValidNotificationMissingZimMetadata: def test_missing_metadata_sets_error_flag( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], ): """Valid notification but missing ZIM metadata → book marked with error.""" @@ -242,6 +245,7 @@ class TestValidNotificationWithMatchingTitleDevMaturity: def test_moves_book_to_staging( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], create_title: Callable[..., Title], ): @@ -264,15 +268,15 @@ def test_moves_book_to_staging( current_locations = [loc for loc in book.locations if loc.status == "current"] assert len(current_locations) == 1 - assert current_locations[0].warehouse_id == MillContext.quarantine_warehouse_id - assert current_locations[0].path == MillContext.quarantine_base_path / str( + assert current_locations[0].warehouse_id == Context.quarantine_warehouse_id + assert current_locations[0].path == Context.quarantine_base_path / str( VALID_NOTIFICATION_CONTENT["folder_name"] ) target_locations = [loc for loc in book.locations if loc.status == "target"] assert len(target_locations) == 1 - assert target_locations[0].warehouse_id == MillContext.staging_warehouse_id - assert target_locations[0].path == MillContext.staging_base_path + assert target_locations[0].warehouse_id == Context.staging_warehouse_id + assert target_locations[0].path == Context.staging_base_path assert book.location_kind == "staging" assert book.has_error is False @@ -282,6 +286,7 @@ def test_moves_book_to_staging( def test_moves_book_to_staging_with_empty_folder_name( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], create_title: Callable[..., Title], ): @@ -310,13 +315,13 @@ def test_moves_book_to_staging_with_empty_folder_name( current_locations = [loc for loc in book.locations if loc.status == "current"] assert len(current_locations) == 1 - assert current_locations[0].warehouse_id == MillContext.quarantine_warehouse_id - assert current_locations[0].path == MillContext.quarantine_base_path + assert current_locations[0].warehouse_id == Context.quarantine_warehouse_id + assert current_locations[0].path == Context.quarantine_base_path target_locations = [loc for loc in book.locations if loc.status == "target"] assert len(target_locations) == 1 - assert target_locations[0].warehouse_id == MillContext.staging_warehouse_id - assert target_locations[0].path == MillContext.staging_base_path + assert target_locations[0].warehouse_id == Context.staging_warehouse_id + assert target_locations[0].path == Context.staging_base_path assert book.location_kind == "staging" assert book.has_error is False @@ -333,6 +338,7 @@ class TestValidNotificationWithMatchingTitleRobustMaturity: def test_moves_book_to_collection_warehouses( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], create_title: Callable[..., Title], create_collection: Callable[..., Collection], @@ -367,8 +373,8 @@ def test_moves_book_to_collection_warehouses( current_locations = [loc for loc in book.locations if loc.status == "current"] assert len(current_locations) == 1 - assert current_locations[0].warehouse_id == MillContext.quarantine_warehouse_id - assert current_locations[0].path == MillContext.quarantine_base_path / str( + assert current_locations[0].warehouse_id == Context.quarantine_warehouse_id + assert current_locations[0].path == Context.quarantine_base_path / str( VALID_NOTIFICATION_CONTENT["folder_name"] ) @@ -385,6 +391,7 @@ def test_moves_book_to_collection_warehouses( def test_moves_book_to_collection_warehouses_with_empty_folder_name( self, dbsession: OrmSession, + warehouse: Warehouse, # noqa: ARG002 create_zimfarm_notification: Callable[..., ZimfarmNotification], create_title: Callable[..., Title], create_collection: Callable[..., Collection], @@ -425,8 +432,8 @@ def test_moves_book_to_collection_warehouses_with_empty_folder_name( current_locations = [loc for loc in book.locations if loc.status == "current"] assert len(current_locations) == 1 - assert current_locations[0].warehouse_id == MillContext.quarantine_warehouse_id - assert current_locations[0].path == MillContext.quarantine_base_path + assert current_locations[0].warehouse_id == Context.quarantine_warehouse_id + assert current_locations[0].path == Context.quarantine_base_path target_locations = [loc for loc in book.locations if loc.status == "target"] assert len(target_locations) == 1 diff --git a/backend/tests/shuttle/test_delete_files.py b/backend/tests/shuttle/test_delete_files.py index c49208a..17f5e03 100644 --- a/backend/tests/shuttle/test_delete_files.py +++ b/backend/tests/shuttle/test_delete_files.py @@ -24,7 +24,6 @@ def test_delete_files_processes_eligible_book( book = create_book() book.location_kind = "to_delete" book.deletion_date = now - timedelta(days=1) - book.has_error = False book.needs_file_operation = True dbsession.flush() @@ -58,7 +57,6 @@ def test_delete_files_inaccessible_warehouse( book = create_book() book.location_kind = "to_delete" book.deletion_date = now - timedelta(days=1) - book.has_error = False book.needs_file_operation = True dbsession.flush() @@ -87,7 +85,6 @@ def test_delete_files_handles_file_deletion_error( book = create_book() book.location_kind = "to_delete" book.deletion_date = now - timedelta(days=1) - book.has_error = False book.needs_file_operation = True dbsession.flush() @@ -108,7 +105,6 @@ def test_delete_files_handles_file_deletion_error( delete_files(dbsession) # Book should be marked with error - assert book.has_error is True assert book.location_kind == "to_delete" assert any( "error encountered while deleting files" in event for event in book.events @@ -129,13 +125,11 @@ def test_delete_files_handles_errors_and_continues_processing( book1 = create_book() book1.location_kind = "to_delete" book1.deletion_date = now - timedelta(days=1) - book1.has_error = False book1.needs_file_operation = True book2 = create_book() book2.location_kind = "to_delete" book2.deletion_date = now - timedelta(days=2) - book2.has_error = False book2.needs_file_operation = True dbsession.flush() @@ -158,11 +152,9 @@ def test_delete_files_handles_errors_and_continues_processing( delete_files(dbsession) # book2 is processed first (older deletion_date) and should have error - assert book2.has_error is True assert any( "error encountered while deleting files" in event for event in book2.events ) # book1 is processed second and should succeed assert book1.location_kind == "deleted" - assert book1.has_error is False diff --git a/dev/docker-compose.yml b/dev/docker-compose.yml index 23c08a9..bf825ea 100644 --- a/dev/docker-compose.yml +++ b/dev/docker-compose.yml @@ -47,6 +47,10 @@ services: JWT_SECRET: DH8kSxcflUVfNRdkEiJJCn2dOOKI3qfw INIT_USERNAME: admin INIT_PASSWORD: admin_pass + STAGING_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + STAGING_BASE_PATH: staging + QUARANTINE_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + QUARANTINE_BASE_PATH: quarantine command: - uvicorn - cms_backend.api.main:app @@ -96,6 +100,10 @@ services: DEBUG: 1 DATABASE_URL: postgresql+psycopg://cms:cmspass@postgresdb:5432/cms LOCAL_WAREHOUSE_PATHS: "11111111-1111-1111-1111-111111111111:/warehouses/hidden,22222222-2222-2222-2222-222222222222:/warehouses/prod,33333333-3333-3333-3333-333333333333:/warehouses/client1" + STAGING_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + STAGING_BASE_PATH: staging + QUARANTINE_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + QUARANTINE_BASE_PATH: quarantine depends_on: postgresdb: condition: service_healthy @@ -113,6 +121,10 @@ services: ALEMBIC_UPGRADE_HEAD_ON_START: false AUTH_MODES: local JWT_SECRET: DH8kSxcflUVfNRdkEiJJCn2dOOKI3qfw + STAGING_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + STAGING_BASE_PATH: staging + QUARANTINE_WAREHOUSE_ID: 11111111-1111-1111-1111-111111111111 + QUARANTINE_BASE_PATH: quarantine depends_on: - postgresdb frontend: diff --git a/frontend/src/components/DeleteBookDialog.vue b/frontend/src/components/DeleteBookDialog.vue new file mode 100644 index 0000000..70fcc35 --- /dev/null +++ b/frontend/src/components/DeleteBookDialog.vue @@ -0,0 +1,198 @@ + + + diff --git a/frontend/src/components/MoveBookDialog.vue b/frontend/src/components/MoveBookDialog.vue new file mode 100644 index 0000000..ebd1191 --- /dev/null +++ b/frontend/src/components/MoveBookDialog.vue @@ -0,0 +1,139 @@ + + + diff --git a/frontend/src/components/RecoverBookDialog.vue b/frontend/src/components/RecoverBookDialog.vue new file mode 100644 index 0000000..7373f26 --- /dev/null +++ b/frontend/src/components/RecoverBookDialog.vue @@ -0,0 +1,91 @@ + + + diff --git a/frontend/src/stores/book.ts b/frontend/src/stores/book.ts index 1d6e8b5..6fb9d92 100644 --- a/frontend/src/stores/book.ts +++ b/frontend/src/stores/book.ts @@ -94,6 +94,52 @@ export const useBookStore = defineStore('book', () => { } } + const deleteBook = async (bookId: string, force_delete: boolean = false) => { + const service = await authStore.getApiService('books') + try { + const response = await service.delete<{ force_delete: boolean }, Book>(`/${bookId}`, { + params: { force_delete }, + }) + errors.value = [] + book.value = response + return response + } catch (_error) { + console.error('Failed to delete book', _error) + errors.value = translateErrors(_error as ErrorResponse) + return null + } + } + + const recoverBook = async (bookId: string) => { + const service = await authStore.getApiService('books') + try { + const response = await service.post(`/${bookId}/recover`) + errors.value = [] + book.value = response + return response + } catch (_error) { + console.error('Failed to recover book', _error) + errors.value = translateErrors(_error as ErrorResponse) + return null + } + } + + const moveBook = async (bookId: string, destination: 'staging' | 'prod') => { + const service = await authStore.getApiService('books') + try { + const response = await service.post<{ destination: string }, Book>(`/${bookId}/move`, { + destination, + }) + errors.value = [] + book.value = response + return response + } catch (_error) { + console.error('Failed to move book', _error) + errors.value = translateErrors(_error as ErrorResponse) + return null + } + } + return { // State defaultLimit, @@ -106,5 +152,8 @@ export const useBookStore = defineStore('book', () => { fetchBooks, savePaginatorLimit, fetchZimUrls, + deleteBook, + recoverBook, + moveBook, } }) diff --git a/frontend/src/types/book.ts b/frontend/src/types/book.ts index 955b114..1c12db0 100644 --- a/frontend/src/types/book.ts +++ b/frontend/src/types/book.ts @@ -1,3 +1,5 @@ +export type LocationKind = 'quarantine' | 'staging' | 'prod' | 'to_delete' | 'deleted' + export interface Producer { display_name: string display_url: string @@ -17,7 +19,7 @@ export interface Book { needs_processing: boolean has_error: boolean needs_file_operation: boolean - location_kind: 'quarantine' | 'staging' | 'prod' + location_kind: LocationKind created_at: string name?: string date?: string @@ -38,7 +40,7 @@ export interface BookLight { needs_processing: boolean has_error: boolean needs_file_operation: boolean - location_kind: 'quarantine' | 'staging' | 'prod' + location_kind: LocationKind created_at: string name?: string date?: string diff --git a/frontend/src/views/BookView.vue b/frontend/src/views/BookView.vue index dc2e22b..2d6e986 100644 --- a/frontend/src/views/BookView.vue +++ b/frontend/src/views/BookView.vue @@ -9,6 +9,35 @@
+
+ + Move Book + + + Recover Book + + + Delete Book + +
+
@@ -238,19 +267,27 @@
+ + + +