diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index ce98fe58..c455341f 100755 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -105,6 +105,11 @@ echo "3.1 Running 'python manage.py fix_sortedm2m_columns' to fix any missing so echo "******************************************" python manage.py fix_sortedm2m_columns +echo "****************** STEP 4.6/5: docker-entrypoint.sh ************************" +echo "4.6 Running 'python manage.py backfill_num_pages' to fill missing publication page counts" +echo "******************************************" +python manage.py backfill_num_pages + # echo "****************** STEP 4.3/5: docker-entrypoint.sh ************************" # echo "4.3 Running 'python manage.py rename_person_images' to rename person images" # echo "******************************************" diff --git a/requirements.txt b/requirements.txt index e7e3e8a5..927f65e4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -68,6 +68,11 @@ djangorestframework==3.15.2 # See: https://pypi.org/project/Wand/ wand==0.6.13 +# pypdf - pure-Python PDF library, used to auto-count a publication's pages +# (issue #1298) by reading the PDF's page tree directly (no rendering). +# See: https://pypi.org/project/pypdf/ +pypdf==6.13.2 + # Pillow - Python Imaging Library fork, for image manipulation # Note: Pillow 10.0+ dropped Python 3.7 support; 11.0+ dropped Python 3.8 # See: https://pypi.org/project/Pillow/ diff --git a/website/management/commands/backfill_num_pages.py b/website/management/commands/backfill_num_pages.py new file mode 100644 index 00000000..a64418b1 --- /dev/null +++ b/website/management/commands/backfill_num_pages.py @@ -0,0 +1,72 @@ +import logging +from django.core.management.base import BaseCommand +from website.models import Publication +import website.utils.fileutils as ml_fileutils + +# This retrieves a Python logging instance (or creates it) +_logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = ( + "Backfills Publication.num_pages for existing publications that have a " + "PDF but no page count, by reading the page count directly from the PDF " + "(issue #1298). Only fills empty values, so manually entered counts are " + "never overwritten. Idempotent: once a publication has a count it is " + "skipped, so this is safe to run on every container start." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help="Report what would change without writing to the database.", + ) + + def handle(self, *args, **options): + dry_run = options["dry_run"] + _logger.debug( + f"Running backfill_num_pages.py (dry_run={dry_run}) to populate " + f"num_pages for publications missing it." + ) + + # Only publications that have a PDF but no page count yet. + candidates = Publication.objects.filter( + num_pages__isnull=True + ).exclude(pdf_file="").exclude(pdf_file__isnull=True) + + num_updated = 0 + num_skipped = 0 + for pub in candidates: + page_count = ml_fileutils.get_pdf_page_count(pub.pdf_file) + if not page_count: + # Couldn't read the PDF (missing on disk, corrupt, etc.). + # Leave num_pages empty and move on. + _logger.debug( + f"Skipping pub id={pub.pk} '{pub.title}': could not determine " + f"page count from {pub.pdf_file.name}" + ) + num_skipped += 1 + continue + + if dry_run: + _logger.debug( + f"[dry-run] Would set num_pages={page_count} for pub " + f"id={pub.pk} '{pub.title}'" + ) + else: + # Write directly via the queryset so this stays a pure data + # backfill — no thumbnail regeneration or file-rename side + # effects from the model's save(). + Publication.objects.filter(pk=pub.pk).update(num_pages=page_count) + _logger.debug( + f"Set num_pages={page_count} for pub id={pub.pk} '{pub.title}'" + ) + num_updated += 1 + + verb = "Would update" if dry_run else "Updated" + _logger.info( + f"backfill_num_pages: {verb} {num_updated} publication(s); " + f"skipped {num_skipped} (PDF unreadable/missing)." + ) + _logger.debug("Completed backfill_num_pages.py") diff --git a/website/models/publication.py b/website/models/publication.py index e4030bec..a177798d 100644 --- a/website/models/publication.py +++ b/website/models/publication.py @@ -5,6 +5,7 @@ from datetime import date # for date comparisons import re # for regular expressions import website.utils.timeutils as timeutils +import website.utils.fileutils as ml_fileutils # for auto-counting PDF pages class PubAwardType(models.TextChoices): BEST_ARTIFACT_AWARD = "Best Artifact Award" @@ -77,8 +78,9 @@ class Publication(Artifact): acmid = models.CharField(max_length=255, blank=True, null=True) # Page numbers - num_pages = models.IntegerField(null=True) - num_pages.help_text = "The total number of pages in this publication (including references)" + num_pages = models.IntegerField(blank=True, null=True) + num_pages.help_text = "The total number of pages in this publication (including references). " \ + "Leave blank to auto-calculate from the uploaded PDF." # TODO, see if there is an IntegerRangeField or something like that for page_num_start and end # There is an IntegerRangeField but it's only for Postgres... hmm @@ -97,6 +99,27 @@ class Publication(Artifact): award = models.CharField(max_length=50, choices=PubAwardType.choices, blank=True, null=True) + def save(self, *args, **kwargs): + """ + Extends Artifact.save() to auto-populate num_pages from the uploaded PDF + when it hasn't been set manually (issue #1298). This removes a required + field from the publication form so students can add papers with less + friction; the page count is still recorded, just derived automatically. + + We only fill an empty value, so a manually entered num_pages is never + overwritten. The count is computed after super().save() because that's + when the PDF is guaranteed to be on disk (Artifact.save() also defers + thumbnail generation for the same reason). If we fill it, we persist with + a second, narrowly-scoped save(update_fields=['num_pages']). + """ + super().save(*args, **kwargs) + + if self.pdf_file and not self.num_pages: + page_count = ml_fileutils.get_pdf_page_count(self.pdf_file) + if page_count: + self.num_pages = page_count + super().save(update_fields=['num_pages']) + def get_upload_dir(self, filename): return os.path.join(self.UPLOAD_DIR, filename) diff --git a/website/tests/test_publication.py b/website/tests/test_publication.py index 4c473668..4e9c808e 100644 --- a/website/tests/test_publication.py +++ b/website/tests/test_publication.py @@ -1,12 +1,31 @@ """Tests for Publication model methods (BibTeX, forum name, author lookup).""" +import io +import os +import tempfile from unittest.mock import MagicMock -from django.test import SimpleTestCase +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import SimpleTestCase, override_settings from website.tests.base import DatabaseTestCase +def _make_pdf_bytes(num_pages): + """ + Build a minimal, valid in-memory PDF with `num_pages` blank pages, using + pypdf's own writer. Guarantees a real PDF (so pypdf can re-read its page + tree) with a known page count for assertions. + """ + from pypdf import PdfWriter + writer = PdfWriter() + for _ in range(num_pages): + writer.add_blank_page(width=612, height=792) # US Letter + buf = io.BytesIO() + writer.write(buf) + return buf.getvalue() + + # --- BibTeX citation regression ------------------------------------------- @@ -209,3 +228,148 @@ def test_returns_first_author_in_sorted_order(self): second = self.make_person(first_name="Second", last_name="Author") pub.authors.add(first, second) # SortedManyToManyField preserves order self.assertEqual(pub.get_person(), first) + + +# --- get_pdf_page_count helper (#1298) ------------------------------------ + + +class GetPdfPageCountTests(SimpleTestCase): + """ + Unit tests for fileutils.get_pdf_page_count, which backs the auto-population + of Publication.num_pages (#1298). The helper must return the page count for a + valid PDF and None (never raise) for anything it can't read, so a bad upload + never blocks saving a publication. + """ + + def _mock_field(self, name, path, exists=True): + """Build a minimal FileField stand-in with the attrs the helper reads.""" + field = MagicMock() + field.name = name + field.path = path + field.storage.exists.return_value = exists + return field + + def test_returns_page_count_for_valid_pdf(self): + from website.utils import fileutils + with tempfile.NamedTemporaryFile(suffix=".pdf", delete=False) as tmp: + tmp.write(_make_pdf_bytes(7)) + tmp_path = tmp.name + try: + field = self._mock_field("paper.pdf", tmp_path) + self.assertEqual(fileutils.get_pdf_page_count(field), 7) + finally: + os.unlink(tmp_path) + + def test_returns_none_for_non_pdf_extension(self): + from website.utils import fileutils + field = self._mock_field("notes.txt", "/tmp/notes.txt") + self.assertIsNone(fileutils.get_pdf_page_count(field)) + + def test_returns_none_when_file_missing_from_storage(self): + from website.utils import fileutils + field = self._mock_field("paper.pdf", "/tmp/does-not-exist.pdf", exists=False) + self.assertIsNone(fileutils.get_pdf_page_count(field)) + + def test_returns_none_for_corrupt_pdf(self): + from website.utils import fileutils + with tempfile.NamedTemporaryFile(suffix=".pdf", delete=False) as tmp: + tmp.write(b"%PDF-1.4 not actually a real pdf") + tmp_path = tmp.name + try: + field = self._mock_field("paper.pdf", tmp_path) + self.assertIsNone(fileutils.get_pdf_page_count(field)) + finally: + os.unlink(tmp_path) + + def test_returns_none_for_empty_field(self): + from website.utils import fileutils + empty = MagicMock() + empty.name = "" + self.assertIsNone(fileutils.get_pdf_page_count(empty)) + + +# --- num_pages auto-population on save (#1298) ---------------------------- + + +@override_settings(MEDIA_ROOT=tempfile.mkdtemp()) +class NumPagesAutoFillTests(DatabaseTestCase): + """ + Integration tests for Publication.save() auto-filling num_pages from the + uploaded PDF (#1298). Uses a temp MEDIA_ROOT so the real PDFs/thumbnails the + save path writes don't pollute the project's media/ directory. + """ + + def _pdf_upload(self, name, num_pages): + return SimpleUploadedFile( + name, _make_pdf_bytes(num_pages), content_type="application/pdf" + ) + + def test_num_pages_autofilled_from_pdf(self): + pub = self.make_publication( + title="Auto Pages", pdf_file=self._pdf_upload("auto.pdf", 5) + ) + pub.refresh_from_db() + self.assertEqual(pub.num_pages, 5) + + def test_manual_num_pages_is_preserved(self): + """A page count the editor typed in must never be overwritten.""" + pub = self.make_publication( + title="Manual Pages", + num_pages=99, + pdf_file=self._pdf_upload("manual.pdf", 5), + ) + pub.refresh_from_db() + self.assertEqual(pub.num_pages, 99) + + +# --- backfill_num_pages management command (#1298) ------------------------ + + +@override_settings(MEDIA_ROOT=tempfile.mkdtemp()) +class BackfillNumPagesCommandTests(DatabaseTestCase): + """ + Tests for the backfill_num_pages management command, which populates + num_pages for legacy publications that have a PDF but no page count (#1298). + """ + + def _pdf_upload(self, name, num_pages): + return SimpleUploadedFile( + name, _make_pdf_bytes(num_pages), content_type="application/pdf" + ) + + def _make_legacy_pub(self, title, num_pages_in_pdf): + """ + Create a publication and then null out num_pages directly in the DB to + simulate a legacy row (save() auto-fills it, so we can't create one with + an empty count through the normal path). + """ + from website.models import Publication + pub = self.make_publication( + title=title, pdf_file=self._pdf_upload(f"{title}.pdf", num_pages_in_pdf) + ) + Publication.objects.filter(pk=pub.pk).update(num_pages=None) + return pub + + def test_backfills_missing_page_count(self): + from django.core.management import call_command + pub = self._make_legacy_pub("Legacy", 8) + call_command("backfill_num_pages") + pub.refresh_from_db() + self.assertEqual(pub.num_pages, 8) + + def test_dry_run_makes_no_changes(self): + from django.core.management import call_command + pub = self._make_legacy_pub("DryRun", 8) + call_command("backfill_num_pages", "--dry-run") + pub.refresh_from_db() + self.assertIsNone(pub.num_pages) + + def test_existing_count_is_not_overwritten(self): + """Publications that already have a count are left untouched.""" + from django.core.management import call_command + pub = self.make_publication( + title="HasCount", num_pages=42, pdf_file=self._pdf_upload("hc.pdf", 8) + ) + call_command("backfill_num_pages") + pub.refresh_from_db() + self.assertEqual(pub.num_pages, 42) diff --git a/website/utils/fileutils.py b/website/utils/fileutils.py index e2c96d5b..44fed475 100644 --- a/website/utils/fileutils.py +++ b/website/utils/fileutils.py @@ -8,6 +8,8 @@ import time # for generating unique filenames from wand.image import Image, Color # for creating thumbnails from wand.exceptions import WandException# for creating thumbnails +from pypdf import PdfReader # for counting PDF pages +from pypdf.errors import PdfReadError # for counting PDF pages # This retrieves a Python logging instance (or creates it) _logger = logging.getLogger(__name__) @@ -262,5 +264,45 @@ def generate_thumbnail_for_pdf(pdf_file_field, thumbnail_image_field, thumbnail_ _logger.error(f"Thumbnail generation failed at all resolutions for: {pdf_file_field.name}") return None - - \ No newline at end of file + + +def get_pdf_page_count(pdf_file_field): + """ + Returns the number of pages in the given PDF, or None if the count can't be + determined (no file, not a PDF, file missing on disk, or unreadable/corrupt). + + Uses pypdf, which reads the PDF's internal page tree directly rather than + rendering pages, so it stays fast and light even for large documents. This + backs the auto-population of Publication.num_pages (issue #1298) so students + no longer have to count pages by hand. + + Returning None rather than raising is intentional: a malformed PDF should + never block saving a publication. The caller simply leaves num_pages unset. + + Args: + pdf_file_field (models.FileField): The PDF file field to inspect. + + Returns: + int | None: The page count, or None if it can't be determined. + + Example: + >>> get_pdf_page_count(publication.pdf_file) + 12 + """ + if not pdf_file_field or not pdf_file_field.name: + return None + + if not pdf_file_field.name.lower().endswith('.pdf'): + _logger.debug(f"Not counting pages for non-PDF file: {pdf_file_field.name}") + return None + + if not pdf_file_field.storage.exists(pdf_file_field.name): + _logger.debug(f"Cannot count pages; file not found in storage: {pdf_file_field.name}") + return None + + try: + reader = PdfReader(pdf_file_field.path) + return len(reader.pages) + except (PdfReadError, OSError, ValueError) as e: + _logger.warning(f"Could not determine page count for {pdf_file_field.name}: {e}") + return None