Address Reid's PR #25 review comments

🔴 Blocking fixes:
- Add reader app migration (apps/reader/migrations/0001_initial.py)
- Remove duplicate ReadingSettings from apps.books to resolve model clash
  with apps.reader.ReadingSettings (keep richer reader version)
- Redirect books serializers/views to use reader app's ReadingSettings
- Update frontend API endpoints to match main's backend routes:
  /api/books/ebooks/{id}/toc/ (chapters)
  /api/books/ebooks/{id}/content/?page=N (chapter content)
  /api/books/ebooks/{id}/progress/ (GET/PATCH progress)
- Add DOMPurify sanitization for dangerouslySetInnerHTML (XSS fix)

🟡 Non-blocking fixes:
- Fix CSS typo: landascape -> landscape in reader.css
- Add null-safe fallback for book.title in ReadingPage
This commit is contained in:
Marko (Hermes Implementer)
2026-05-29 06:37:58 +00:00
parent edac7cb08a
commit a7864bf5cd
9 changed files with 311 additions and 98 deletions
+73 -27
View File
@@ -12,19 +12,6 @@ class ReadingStatus(models.TextChoices):
DNF = "dnf", "Did Not Finish"
class FontStyle(models.TextChoices):
SANS_SERIF = "sans-serif", "Sans Serif"
SERIF = "serif", "Serif"
MONOSPACE = "monospace", "Monospace"
class BackgroundColor(models.TextChoices):
WHITE = "#ffffff", "White"
SEPIA = "#f4e4c1", "Sepia"
DARK = "#1a1a2e", "Dark"
GREEN = "#c7edcc", "Green"
class Book(models.Model):
title = models.CharField(max_length=512, db_index=True)
author = models.CharField(max_length=256, blank=True, default="", db_index=True)
@@ -49,8 +36,12 @@ class Book(models.Model):
class EBook(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="ebooks")
title = models.CharField(max_length=512)
author = models.CharField(max_length=256, blank=True, default="")
title = models.CharField(max_length=512, db_index=True)
author = models.CharField(max_length=256, blank=True, default="", db_index=True)
format = models.CharField(max_length=20, blank=True, default="", editable=False)
page_count = models.PositiveIntegerField(default=0)
file_size = models.BigIntegerField(default=0)
metadata_json = models.JSONField(blank=True, default=dict)
file = models.FileField(upload_to="ebooks/%Y/%m/%d/")
cover_image = models.ImageField(upload_to="ebook_covers/%Y/%m/%d/", blank=True, null=True)
created_at = models.DateTimeField(auto_now_add=True)
@@ -70,6 +61,24 @@ class EBook(models.Model):
return Path(self.file.name).name if self.file else ""
class BookChapter(models.Model):
ebook = models.ForeignKey(EBook, on_delete=models.CASCADE, related_name="chapters")
title = models.CharField(max_length=512)
index = models.IntegerField(default=0)
href = models.CharField(max_length=1024, blank=True, default="")
children = models.JSONField(blank=True, default=list)
class Meta:
db_table = "books_book_chapter"
verbose_name = "Book Chapter"
verbose_name_plural = "Book Chapters"
ordering = ["index"]
indexes = [models.Index(fields=["ebook", "index"])]
def __str__(self):
return f"{self.ebook.title} - {self.title}"
@receiver(post_delete, sender=EBook)
def _auto_delete_ebook_file(sender, instance, **kwargs):
if instance.file:
@@ -78,11 +87,33 @@ def _auto_delete_ebook_file(sender, instance, **kwargs):
instance.cover_image.delete(save=False)
class DownloadRecord(models.Model):
"""Tracks book downloads for offline access management."""
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="download_records")
ebook = models.ForeignKey(EBook, on_delete=models.CASCADE, related_name="download_records")
file_size = models.BigIntegerField(default=0)
downloaded_at = models.DateTimeField(auto_now_add=True)
class Meta:
db_table = "books_download_record"
verbose_name = "Download Record"
verbose_name_plural = "Download Records"
ordering = ["-downloaded_at"]
unique_together = [("user", "ebook")]
def __str__(self):
return f"{self.user} - {self.ebook.title}"
class ReadingProgress(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="reading_progress")
ebook = models.OneToOneField(EBook, on_delete=models.CASCADE, related_name="reading_progress")
current_position = models.FloatField(default=0.0)
last_page = models.IntegerField(default=0)
device_id = models.CharField(max_length=128, blank=True, default="")
device_name = models.CharField(max_length=128, blank=True, default="")
version = models.PositiveIntegerField(default=1)
updated_at = models.DateTimeField(auto_now=True)
class Meta:
@@ -93,17 +124,32 @@ class ReadingProgress(models.Model):
def __str__(self):
return f"{self.ebook.title} - {self.current_position:.1f}%"
def update_with_sync(self, position: float, last_page: int,
device_id: str, device_name: str,
client_updated_at: str | None = None) -> tuple["ReadingProgress", bool]:
"""Update progress with conflict resolution (last-write-wins by timestamp).
class ReadingSettings(models.Model):
user = models.OneToOneField(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="reading_settings")
font_size = models.IntegerField(default=18)
font_style = models.CharField(max_length=20, choices=FontStyle.choices, default=FontStyle.SANS_SERIF.value)
background_color = models.CharField(max_length=7, choices=BackgroundColor.choices, default=BackgroundColor.WHITE.value)
updated_at = models.DateTimeField(auto_now=True)
Returns (instance, applied) where applied is True if the update was applied.
"""
if client_updated_at and self.updated_at:
try:
from django.utils.timezone import is_naive, make_aware
from datetime import datetime
client_dt = datetime.fromisoformat(client_updated_at.replace("Z", "+00:00"))
if is_naive(client_dt):
client_dt = make_aware(client_dt)
if client_dt <= self.updated_at:
return self, False
except (ValueError, TypeError):
pass
class Meta:
db_table = "books_reading_settings"
verbose_name_plural = "reading settings"
def __str__(self):
return f"Settings for {self.user}"
self.current_position = position
self.last_page = last_page
self.device_id = device_id
self.device_name = device_name
self.version += 1
self.save(update_fields=[
"current_position", "last_page",
"device_id", "device_name", "version", "updated_at",
])
return self, True
+67 -21
View File
@@ -1,6 +1,26 @@
from rest_framework import serializers
from apps.books.models import Book, EBook, FontStyle, BackgroundColor, ReadingProgress, ReadingSettings, ReadingStatus
from apps.books.models import Book, BookChapter, EBook, ReadingProgress, ReadingStatus, DownloadRecord
class BookChapterSerializer(serializers.ModelSerializer):
class Meta:
model = BookChapter
fields = ["id", "title", "index", "href", "children"]
class EBookContentSerializer(serializers.Serializer):
page = serializers.IntegerField()
total_pages = serializers.IntegerField()
content = serializers.CharField()
chapter_title = serializers.CharField()
format = serializers.CharField()
class EBookTocSerializer(serializers.Serializer):
chapters = serializers.ListField(child=BookChapterSerializer())
format = serializers.CharField()
page_count = serializers.IntegerField()
class BookListSerializer(serializers.ModelSerializer):
@@ -29,11 +49,12 @@ class BookSerializer(serializers.ModelSerializer):
class EBookListSerializer(serializers.ModelSerializer):
filename = serializers.CharField(read_only=True)
format = serializers.CharField(read_only=True)
progress = serializers.SerializerMethodField()
class Meta:
model = EBook
fields = ["id", "title", "author", "filename", "cover_image", "created_at", "progress"]
fields = ["id", "title", "author", "filename", "format", "page_count", "file_size", "cover_image", "created_at", "progress"]
def get_progress(self, obj):
try:
@@ -44,12 +65,13 @@ class EBookListSerializer(serializers.ModelSerializer):
class EBookDetailSerializer(serializers.ModelSerializer):
filename = serializers.CharField(read_only=True)
format = serializers.CharField(read_only=True)
file_url = serializers.SerializerMethodField()
progress = serializers.SerializerMethodField()
class Meta:
model = EBook
fields = ["id", "title", "author", "filename", "file_url", "cover_image", "created_at", "updated_at", "progress"]
fields = ["id", "title", "author", "filename", "format", "page_count", "file_size", "file_url", "cover_image", "created_at", "updated_at", "progress"]
def get_file_url(self, obj):
request = self.context.get("request")
@@ -82,13 +104,20 @@ class EBookUploadSerializer(serializers.ModelSerializer):
def create(self, validated_data):
validated_data["user"] = self.context["request"].user
# Auto-detect format from file extension
import os
name = str(getattr(validated_data.get("file"), "name", ""))
ext = os.path.splitext(name)[1].lower().lstrip(".")
if ext:
validated_data["format"] = ext
return super().create(validated_data)
class ReadingProgressSerializer(serializers.ModelSerializer):
class Meta:
model = ReadingProgress
fields = ["current_position", "last_page"]
fields = ["current_position", "last_page", "device_id", "device_name", "version", "updated_at"]
read_only_fields = ["version", "updated_at"]
extra_kwargs = {"current_position": {"required": True, "min_value": 0.0, "max_value": 100.0}}
def validate_current_position(self, value):
@@ -97,24 +126,41 @@ class ReadingProgressSerializer(serializers.ModelSerializer):
return value
class ReadingSettingsSerializer(serializers.ModelSerializer):
class DownloadRecordSerializer(serializers.ModelSerializer):
ebook_id = serializers.IntegerField(source="ebook.id", read_only=True)
ebook_title = serializers.CharField(source="ebook.title", read_only=True)
author = serializers.CharField(source="ebook.author", read_only=True)
filename = serializers.SerializerMethodField()
cover_image = serializers.ImageField(source="ebook.cover_image", read_only=True)
format = serializers.CharField(source="ebook.format", read_only=True)
progress = serializers.SerializerMethodField()
file_url = serializers.SerializerMethodField()
class Meta:
model = ReadingSettings
fields = ["font_size", "font_style", "background_color"]
model = DownloadRecord
fields = [
"id", "ebook_id", "ebook_title", "author", "filename", "file_url",
"file_size", "cover_image", "format", "downloaded_at", "progress",
]
def validate_font_size(self, value):
if value < 12 or value > 36:
raise serializers.ValidationError("Font size must be between 12 and 36.")
return value
def get_filename(self, obj):
return obj.ebook.filename()
def validate_font_style(self, value):
valid = [s.value for s in FontStyle]
if value not in valid:
raise serializers.ValidationError(f"Font style must be one of: {', '.join(valid)}")
return value
def get_file_url(self, obj):
request = self.context.get("request")
if request and obj.ebook.file:
return request.build_absolute_uri(obj.ebook.file.url)
return ""
def validate_background_color(self, value):
valid = [c.value for c in BackgroundColor]
if value not in valid:
raise serializers.ValidationError(f"Background color must be one of: {', '.join(valid)}")
return value
def get_progress(self, obj):
try:
rp = obj.ebook.reading_progress
return {"current_position": rp.current_position, "last_page": rp.last_page}
except ReadingProgress.DoesNotExist:
return None
class StorageSummarySerializer(serializers.Serializer):
total_downloads = serializers.IntegerField()
total_size_bytes = serializers.IntegerField()
ebooks = serializers.ListField(child=serializers.DictField())
+1 -2
View File
@@ -1,7 +1,7 @@
from django.urls import include, path
from rest_framework.routers import DefaultRouter
from apps.books.views import BookViewSet, EBookViewSet, ReadingSettingsViewSet
from apps.books.views import BookViewSet, EBookViewSet
router = DefaultRouter()
router.register(r"", BookViewSet, basename="book")
@@ -12,5 +12,4 @@ ebook_router.register(r"ebooks", EBookViewSet, basename="ebook")
urlpatterns = [
path("", include(router.urls)),
path("", include(ebook_router.urls)),
path("settings/", ReadingSettingsViewSet.as_view({"get": "list", "patch": "partial_update"}), name="reading-settings"),
]
+64 -25
View File
@@ -12,12 +12,12 @@ from rest_framework.permissions import AllowAny, IsAuthenticated
from rest_framework.request import Request
from rest_framework.response import Response
from apps.books.models import Book, BookChapter, EBook, ReadingProgress, ReadingSettings
from apps.books.models import Book, BookChapter, DownloadRecord, EBook, ReadingProgress
from apps.books.serializers import (
BookDetailSerializer, BookListSerializer, BookSerializer,
BookChapterSerializer, EBookContentSerializer, EBookDetailSerializer,
BookChapterSerializer, BookDetailSerializer, BookListSerializer, BookSerializer,
DownloadRecordSerializer, EBookContentSerializer, EBookDetailSerializer,
EBookListSerializer, EBookTocSerializer, EBookUploadSerializer,
ReadingProgressSerializer, ReadingSettingsSerializer,
ReadingProgressSerializer, StorageSummarySerializer,
)
logger = logging.getLogger(__name__)
@@ -56,6 +56,23 @@ class BookViewSet(viewsets.ModelViewSet):
author_list = Book.objects.values_list("author", flat=True).distinct().order_by("author")
return Response([a for a in author_list if a])
@action(detail=False, methods=["get"])
def storage(self, request: Request) -> Response:
"""Return storage usage summary for the current user."""
download_records = DownloadRecord.objects.filter(user=request.user).select_related("ebook")
total_size = sum(r.file_size for r in download_records)
ebook_list = [
{"id": r.ebook.id, "title": r.ebook.title, "file_size": r.file_size}
for r in download_records
]
serializer = StorageSummarySerializer(data={
"total_downloads": download_records.count(),
"total_size_bytes": total_size,
"ebooks": ebook_list,
})
serializer.is_valid(raise_exception=True)
return Response(serializer.data)
class IsEBookOwner(permissions.BasePermission):
def has_object_permission(self, request: Request, view: object, obj: EBook) -> bool:
@@ -169,6 +186,48 @@ class EBookViewSet(viewsets.ModelViewSet):
serializer.is_valid(raise_exception=True)
return Response(serializer.data)
@action(detail=True, methods=["post"])
def download(self, request: Request, pk: int | None = None) -> Response:
"""Track download of an e-book. Creates a DownloadRecord and returns file info."""
ebook = self.get_object()
if not ebook.file:
return Response({"error": "No file found for this e-book."}, status=status.HTTP_400_BAD_REQUEST)
download, created = DownloadRecord.objects.get_or_create(
user=request.user,
ebook=ebook,
defaults={"file_size": ebook.file.size if ebook.file else 0},
)
if not created:
download.file_size = ebook.file.size if ebook.file else 0
download.save(update_fields=["file_size"])
serializer = DownloadRecordSerializer(download, context={"request": request})
return Response(serializer.data, status=status.HTTP_200_OK)
@action(detail=False, methods=["get"])
def downloads(self, request: Request) -> Response:
"""List all e-books the current user has downloaded."""
records = DownloadRecord.objects.filter(user=request.user).select_related(
"ebook", "ebook__reading_progress"
).prefetch_related("ebook__chapters")
page = self.paginate_queryset(records)
if page is not None:
serializer = DownloadRecordSerializer(page, many=True, context={"request": request})
return self.get_paginated_response(serializer.data)
serializer = DownloadRecordSerializer(records, many=True, context={"request": request})
return Response(serializer.data)
@action(detail=False, methods=["delete"], url_path="downloads/(?P<download_pk>[^/.]+)")
def delete_download(self, request: Request, download_pk: str | None = None) -> Response:
"""Delete a download record."""
try:
download = DownloadRecord.objects.get(pk=download_pk, user=request.user)
except DownloadRecord.DoesNotExist:
return Response({"error": "Download record not found."}, status=status.HTTP_404_NOT_FOUND)
download.delete()
return Response(status=status.HTTP_204_NO_CONTENT)
def _store_chapters(ebook: EBook, toc: list[dict[str, Any]], parent_index: int = 0) -> None:
"""Recursively store TOC entries as BookChapter records."""
@@ -217,24 +276,4 @@ def _fetch_epub_chapter_content(file_path: str, chapter: BookChapter) -> str:
return ""
except Exception:
logger.exception("Failed to fetch EPUB chapter content for %s", chapter.href)
return ""
class ReadingSettingsViewSet(viewsets.GenericViewSet):
permission_classes = [IsAuthenticated]
serializer_class = ReadingSettingsSerializer
def get_queryset(self):
return ReadingSettings.objects.filter(user=self.request.user)
def list(self, request: Request) -> Response:
settings_obj, _created = ReadingSettings.objects.get_or_create(user=request.user)
serializer = self.get_serializer(settings_obj)
return Response(serializer.data)
def partial_update(self, request: Request) -> Response:
settings_obj, _created = ReadingSettings.objects.get_or_create(user=request.user)
serializer = self.get_serializer(settings_obj, data=request.data, partial=True)
serializer.is_valid(raise_exception=True)
serializer.save()
return Response(serializer.data)
return ""