From 54014772cc408b8185220ae509e9efd9ea0666d5 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 27 Jul 2026 15:51:23 -0700 Subject: [PATCH 1/2] Add an artifact-type suffix to standardized filenames (#1404) Standardized artifact filenames gain a trailing type segment for the two types whose file is ambiguous once downloaded: a talk's exported slides and a poster generate the same Author_TitleInTitleCase_VenueYear name as the paper itself. Talks now end in "_Talk", posters in "_Poster"; publications and grants stay unsuffixed (a bare paper PDF is the default expectation, and that name is also the .bib download name). Trailing rather than the existing mid-name suffix slot, so everything before it stays byte-identical to the old scheme and the "-" uniquifier still appends cleanly. The label comes from a class attribute (FILENAME_TYPE_SUFFIX), not from data -- Poster has no type field and Talk.talk_type is nullable and editor-editable, so deriving it would rename files on a metadata-only edit. Applied retroactively via restandardize_artifact_filenames, which is already an idempotent every-container-start step. It ships here with --dry-run so the scope is reviewable in the logs before any file moves. The load-bearing subtlety is ordering: backfill_original_filenames runs BEFORE that step, and its "already standardized?" guard is what stops it recording a renamed file's current name as its "Originally uploaded as" provenance. The moment the scheme changed, every already-renamed talk and poster would have failed that guard and had its old standardized name written in as a fake original -- so the guard now accepts both schemes. Also extracts the uniquifier-tolerant name comparison that three commands had each copy-pasted into one tested helper (matches_standardized_basename), and teaches repair_diverged_artifact_filenames to look for orphans under the pre-#1404 base too (that bug predates the scheme change). Known and accepted: /media/talks/ and /media/posters/ 404s are answered by Apache and never reach Django, so unlike publications (which serve_pdf can redirect via the recorded original filename) external links to renamed talk/poster PDFs cannot be rescued from this repo. Tracked separately. Tests: 661 OK. New coverage for the trailing segment, per-class labels, the shared matcher, the retroactive rename of a pre-#1404 talk (pdf + raw + thumbnail) and its idempotency, publications not churning, and the backfill guard for both a legacy name and its uniquified variant. Co-Authored-By: Claude Opus 5 (1M context) --- docker-entrypoint.sh | 12 +- makeabilitylab/settings.py | 4 +- .../commands/backfill_original_filenames.py | 34 ++-- .../repair_diverged_artifact_filenames.py | 9 +- .../restandardize_artifact_filenames.py | 42 ++-- website/models/artifact.py | 38 +++- website/models/poster.py | 4 + website/models/talk.py | 4 + website/tests/test_artifact.py | 181 +++++++++++++++++- website/tests/test_fileutils.py | 60 ++++++ website/utils/fileutils.py | 56 +++++- 11 files changed, 390 insertions(+), 54 deletions(-) diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index 16e25be7..1dff43d0 100755 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -164,9 +164,15 @@ echo "******************************************" python manage.py setup_admin_groups echo "****************** STEP 4.10b/5: docker-entrypoint.sh ************************" -echo "4.10b Running 'python manage.py restandardize_artifact_filenames' to rename legacy talk/poster/pub files to the standardized scheme (#1401)" -echo "******************************************" -python manage.py restandardize_artifact_filenames +echo "4.10b Running 'python manage.py restandardize_artifact_filenames' to rename legacy talk/poster/pub files to the standardized scheme (#1401/#1404)" +echo "******************************************" +# TEMPORARY (#1404): the scheme just gained a trailing artifact-type segment +# ("..._CHI2024_Talk"), so this step would re-rename EVERY already-standardized +# talk and poster in one unattended pass. --dry-run logs what WOULD be renamed, +# touching nothing on disk or in the DB, so the scope can be reviewed on the +# test server (and then prod) first. REMOVE --dry-run and redeploy to perform +# the rename; after that this step is idempotent again and stays in place. +python manage.py restandardize_artifact_filenames --dry-run echo "****************** STEP 4.10c/5: docker-entrypoint.sh ************************" echo "4.10c Running 'python manage.py repair_diverged_artifact_filenames' to recover artifacts whose files were renamed on disk but not in the DB (#1390 dotted-name bug)" diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index eb3187f7..8fe3c2ca 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -86,8 +86,8 @@ SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https') # Makeability Lab Global Variables, including Makeability Lab version -ML_WEBSITE_VERSION = "2.28.1" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "Adds Project People and Activity Log quick links to the admin header, so two internal pages that nothing in the site nav points at are one click away. The Activity Log link is superuser-only, matching that page's own gate." +ML_WEBSITE_VERSION = "2.29.0" # Keep this updated with each release and also change the short description below +ML_WEBSITE_VERSION_DESCRIPTION = "Standardized filenames for talks and posters now end in '_Talk' and '_Poster' (#1404), so a downloaded slide deck or poster is no longer indistinguishable from the paper PDF of the same work. Publication filenames are unchanged. Existing talk/poster files are renamed once, on deploy." DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed MAX_BANNERS = 7 # Maximum number of banners on a page diff --git a/website/management/commands/backfill_original_filenames.py b/website/management/commands/backfill_original_filenames.py index b6e6a816..d10e2ab1 100644 --- a/website/management/commands/backfill_original_filenames.py +++ b/website/management/commands/backfill_original_filenames.py @@ -4,6 +4,7 @@ from django.core.management.base import BaseCommand from website.models import Artifact, Talk, Poster, Publication +from website.utils import fileutils as ml_fileutils # This retrieves a Python logging instance (or creates it) _logger = logging.getLogger(__name__) @@ -122,19 +123,26 @@ def _backfill_row(self, model, artifact, file_attr, original_attr, dry_run): current_basename = os.path.basename(file_field.name) current_no_ext = os.path.splitext(current_basename)[0] - standardized_no_ext = Artifact.generate_filename(artifact) - - # Treat the file as already-standardized when its name equals the - # standardized scheme OR is a uniquified variant of it. When a - # standardized name collides on disk, ensure_filename_is_unique() - # (fileutils.py) appends "-" — e.g. - # "Lee_Talk_CHI2021-1782399772.42.pdf" — so the on-disk name still - # STARTS WITH the standardized base. Matching only on exact equality - # would misread those as never-renamed and record the standardized+ - # suffix name as the "original" — a false positive. - already_standardized = ( - current_no_ext == standardized_no_ext - or current_no_ext.startswith(standardized_no_ext + "-") + + # Treat the file as already-standardized when its name matches the + # standardized scheme. matches_standardized_basename also accepts the + # "-" that ensure_filename_is_unique appends on a disk + # collision (e.g. "Lee_Talk_CHI2021-1782399772.42.pdf"), so a renamed- + # then-uniquified file isn't misread as an original upload. + # + # BOTH schemes count (#1404). This command runs at container start + # BEFORE restandardize_artifact_filenames, so on the deploy that + # introduces the artifact-type segment ("..._Talk") every already-renamed + # talk and poster still carries its pre-#1404 name. Checking only the + # current scheme would read the whole corpus as never-renamed and record + # those old standardized names as originals — false provenance, shown to + # editors as "Originally uploaded as". + already_standardized = any( + ml_fileutils.matches_standardized_basename( + current_no_ext, + Artifact.generate_filename( + artifact, include_type_suffix=include_type_suffix)) + for include_type_suffix in (True, False) ) if already_standardized: # Already renamed — the original upload name is gone. diff --git a/website/management/commands/repair_diverged_artifact_filenames.py b/website/management/commands/repair_diverged_artifact_filenames.py index 7cb59fb6..e3d8c461 100644 --- a/website/management/commands/repair_diverged_artifact_filenames.py +++ b/website/management/commands/repair_diverged_artifact_filenames.py @@ -4,6 +4,7 @@ from django.core.management.base import BaseCommand from website.models import Artifact, Talk, Poster, Publication +from website.utils import fileutils as ml_fileutils # This retrieves a Python logging instance (or creates it) _logger = logging.getLogger(__name__) @@ -150,9 +151,15 @@ def _repair_field(self, artifact, field_name, dry_run): # base, possibly with a "-" uniqueness suffix from colliding # with the sibling files. Match those, then confirm by content type so we # never mis-pair a pdf with a pptx/thumbnail (they share the base name). + # The pre-#1404 base (no trailing "_Talk"/"_Poster") is searched too: the + # bug predates that scheme change, so an orphan it left behind on disk + # carries the old base even though the repair target uses the new one. + legacy_base = get_valid_filename( + Artifact.generate_filename(artifact, include_type_suffix=False)) candidates = [] for entry in os.listdir(directory): - if entry == valid_base or entry.startswith(valid_base + "-"): + if any(ml_fileutils.matches_standardized_basename(entry, base) + for base in {valid_base, legacy_base}): full = os.path.join(directory, entry) if os.path.isfile(full): candidates.append(entry) diff --git a/website/management/commands/restandardize_artifact_filenames.py b/website/management/commands/restandardize_artifact_filenames.py index 884b70de..861d0b0a 100644 --- a/website/management/commands/restandardize_artifact_filenames.py +++ b/website/management/commands/restandardize_artifact_filenames.py @@ -4,6 +4,7 @@ from django.core.management.base import BaseCommand from website.models import Artifact, Talk, Poster, Publication +from website.utils import fileutils as ml_fileutils # This retrieves a Python logging instance (or creates it) _logger = logging.getLogger(__name__) @@ -11,17 +12,20 @@ class Command(BaseCommand): help = ( - "Re-standardizes legacy talk/poster/publication filenames that were " - "never renamed to the Author_TitleInTitleCase_VenueYear scheme (issue " - "#1401). Production has many such rows (bulk-imported, so they never " - "went through an authored Artifact.save()). This reuses the existing, " - "now-correct rename path: when Artifact.do_filenames_need_updating() is " - "True it calls artifact.save(), which renames the pdf_file, raw_file, " - "and thumbnail on disk AND in the DB together. The original upload name " - "is preserved (it was captured into original_*_filename by " - "backfill_original_filenames / #1391 before this runs). Idempotent: " - "once a row is standardized the check returns False, so re-runs do " - "nothing. Safe to run on every container start." + "Re-standardizes talk/poster/publication filenames that don't match the " + "current scheme. Two populations: files that were never renamed at all " + "(issue #1401 — bulk-imported rows that never went through an authored " + "Artifact.save()), and files standardized under a superseded scheme " + "(issue #1404 added the trailing artifact-type segment, e.g. " + "'..._CHI2024_Talk'), which this migrates in one pass. It reuses the " + "existing, now-correct rename path: when Artifact.do_filenames_need_" + "updating() is True it calls artifact.save(), which renames the " + "pdf_file, raw_file, and thumbnail on disk AND in the DB together. The " + "original upload name is preserved (it was captured into " + "original_*_filename by backfill_original_filenames / #1391 before this " + "runs). Idempotent: once a row matches the current scheme the check " + "returns False, so re-runs do nothing. Safe to run on every container " + "start." ) # The concrete artifact models this covers. Posters are already @@ -156,9 +160,12 @@ def _needs_restandardizing(artifact): standardized name collided on disk and got a ``-`` suffix (``ensure_filename_is_unique``) reads as "needs updating" forever and would be re-renamed on every run — churning duplicate-name artifacts' - filenames on every deploy. Here a name that equals the standardized - base OR is a ``-`` variant of it counts as already standardized, - which keeps the command idempotent. + filenames on every deploy. ``matches_standardized_basename`` accepts + those variants, which keeps the command idempotent. + + Only the CURRENT scheme counts as standardized: a file named under the + pre-#1404 scheme (no trailing "_Talk"/"_Poster") is deliberately flagged, + which is what migrates the existing corpus in a single pass. """ standardized = Artifact.generate_filename(artifact) for file_attr in ("pdf_file", "raw_file"): @@ -167,10 +174,7 @@ def _needs_restandardizing(artifact): continue current_no_ext = os.path.splitext( os.path.basename(file_field.name))[0] - is_standardized = ( - current_no_ext == standardized - or current_no_ext.startswith(standardized + "-") - ) - if not is_standardized: + if not ml_fileutils.matches_standardized_basename( + current_no_ext, standardized): return True return False diff --git a/website/models/artifact.py b/website/models/artifact.py index a72fb10a..cdb2dbea 100644 --- a/website/models/artifact.py +++ b/website/models/artifact.py @@ -23,6 +23,14 @@ class Artifact(models.Model): 2. also hook up an authors_changed signal in signals.py. For example, add the line "@receiver(m2m_changed, sender=Grant.authors.through)" to signals.py's def authors_changed(sender, instance, action, reverse, **kwargs): """ + # The artifact-type segment appended to standardized filenames (#1404), so a + # downloaded file says what kind of artifact it is. Set on the subclasses + # whose files are otherwise ambiguous — a talk's exported slides and a poster + # generate the same Author_Title_VenueYear name as the paper itself. Left + # None here (and on Publication/Grant): a bare paper PDF is the default + # expectation, and that same name is also the .bib download name. + FILENAME_TYPE_SUFFIX = None + title = models.CharField(max_length=255, blank=True, null=True) authors = SortedManyToManyField('Person', blank=True) date = models.DateField(null=True) @@ -473,32 +481,44 @@ def do_filenames_need_updating(artifact): return False @staticmethod - def generate_filename(artifact, file_extension=None, max_pub_title_length = -1): + def generate_filename(artifact, file_extension=None, max_pub_title_length = -1, + include_type_suffix=True): """ Generates a filename for the given artifact. - This method generates a filename based on the artifact's first author's last name, title, forum name, and date. + This method generates a filename based on the artifact's first author's last name, title, forum name, and date, + plus the artifact-type segment of its class (``FILENAME_TYPE_SUFFIX``, e.g. "_Talk" — see #1404). If a file extension is provided, it is appended to the filename. Otherwise, a filename without extension is returned. Parameters: artifact (Artifact): The artifact for which the filename is to be generated. file_extension (str, optional): The file extension to be appended to the filename. Defaults to None. + include_type_suffix (bool, optional): Pass False for the pre-#1404 name (no type segment). Only the + filename management commands need this, to recognize a file that was standardized under the old + scheme; everything else wants the current scheme. Defaults to True. Returns: str: The generated filename. Example: - >>> artifact = Artifact(first_author_last_name="Froehlich", title="Research Artifact Title", forum_name="CHI", date="2023-12-16") - >>> generate_filename(artifact, file_extension=".pdf") + >>> talk = Talk(title="Research Artifact Title", forum_name="CHI", date="2023-12-16") # first author: Froehlich + >>> generate_filename(talk, file_extension=".pdf") + 'Froehlich_ResearchArtifactTitle_CHI2023_Talk.pdf' + >>> generate_filename(talk, file_extension=".pdf", include_type_suffix=False) 'Froehlich_ResearchArtifactTitle_CHI2023.pdf' """ - + + # Read off the class, not the instance, so the type segment is a property of the model + # (Poster has no type field, and Talk.talk_type is nullable and editor-editable — deriving + # the segment from data would rename files on a metadata-only edit). + type_suffix = type(artifact).FILENAME_TYPE_SUFFIX if include_type_suffix else None + # An empty string or a string with only whitespace characters is considered False in a boolean context. if not file_extension or not file_extension.strip(): return ml_fileutils.get_filename_without_ext_for_artifact( - artifact.get_first_author_last_name(), artifact.title, - artifact.forum_name, artifact.date) + artifact.get_first_author_last_name(), artifact.title, + artifact.forum_name, artifact.date, type_suffix=type_suffix) else: return ml_fileutils.get_filename_for_artifact( - artifact.get_first_author_last_name(), artifact.title, - artifact.forum_name, artifact.date, file_extension) \ No newline at end of file + artifact.get_first_author_last_name(), artifact.title, + artifact.forum_name, artifact.date, file_extension, type_suffix=type_suffix) \ No newline at end of file diff --git a/website/models/poster.py b/website/models/poster.py index d7a105d9..da5ebb0e 100644 --- a/website/models/poster.py +++ b/website/models/poster.py @@ -6,6 +6,10 @@ class Poster(Artifact): UPLOAD_DIR = 'posters/' THUMBNAIL_DIR = os.path.join(UPLOAD_DIR, 'images/') + # Standardized filenames end in "_Poster" (#1404) so a downloaded poster + # isn't indistinguishable from the paper's PDF of the same work. + FILENAME_TYPE_SUFFIX = 'Poster' + external_slides_url = models.URLField(blank=True, null=True) external_slides_url.help_text = ( "Optional link to the source design (e.g., Figma, Canva, Illustrator " diff --git a/website/models/talk.py b/website/models/talk.py index 605c86de..16c8bd33 100644 --- a/website/models/talk.py +++ b/website/models/talk.py @@ -23,6 +23,10 @@ class Talk(Artifact): UPLOAD_DIR = 'talks/' THUMBNAIL_DIR = os.path.join(UPLOAD_DIR, 'images/') + # Standardized filenames end in "_Talk" (#1404) so a downloaded slide deck + # isn't indistinguishable from the paper's PDF of the same work. + FILENAME_TYPE_SUFFIX = 'Talk' + external_slides_url = models.URLField(blank=True, null=True) external_slides_url.help_text = ( "Optional link to the source slide deck (e.g., Figma, Google Slides, " diff --git a/website/tests/test_artifact.py b/website/tests/test_artifact.py index 4269910d..e9a5a93d 100644 --- a/website/tests/test_artifact.py +++ b/website/tests/test_artifact.py @@ -1,6 +1,8 @@ """Tests for Artifact model methods (filename-drift check, raw-file label).""" import os +import shutil +import tempfile from datetime import date from unittest.mock import MagicMock, patch @@ -8,9 +10,9 @@ from django.core.files.storage import default_storage from django.core.files.uploadedfile import SimpleUploadedFile from django.core.management import call_command -from django.test import SimpleTestCase +from django.test import SimpleTestCase, override_settings -from website.models import Artifact, Publication, Talk +from website.models import Artifact, Grant, Poster, Publication, Talk from website.tests.base import DatabaseTestCase from website.tests.factories import TalkFactory @@ -455,3 +457,178 @@ def test_malformed_row_is_skipped_and_batch_continues(self): self.assertNotIn("Good_Original", good.pdf_file.name) # The malformed row is left exactly as it was. self.assertEqual(bad.pdf_file.name, bad_name) + + +# --- #1404: artifact-type filename suffix --------------------------------- + + +class ArtifactTypeSuffixTests(SimpleTestCase): + """ + generate_filename appends an artifact-type segment for the types whose + downloaded file is otherwise ambiguous (#1404): a talk's exported slides and + a poster produce the same Author_Title_VenueYear name as the paper itself. + Publications (and grants) stay unsuffixed — a bare paper PDF is the default + expectation, and that same name is the .bib download name (get_pub_filename). + + No DB: get_first_author_last_name() short-circuits to "Unknown" on an + unsaved instance, so plain model construction is enough. + """ + + KWARGS = dict(title="My Cool Talk", forum_name="CHI", date=date(2024, 1, 1)) + LEGACY = "Unknown_MyCoolTalk_CHI2024" + + def test_talk_gets_a_trailing_talk_segment(self): + self.assertEqual(Artifact.generate_filename(Talk(**self.KWARGS)), + self.LEGACY + "_Talk") + + def test_poster_gets_a_trailing_poster_segment(self): + self.assertEqual(Artifact.generate_filename(Poster(**self.KWARGS)), + self.LEGACY + "_Poster") + + def test_publication_is_unsuffixed(self): + self.assertEqual(Artifact.generate_filename(Publication(**self.KWARGS)), + self.LEGACY) + + def test_grant_is_unsuffixed(self): + self.assertEqual(Artifact.generate_filename(Grant(**self.KWARGS)), + self.LEGACY) + + def test_extension_follows_the_type_segment(self): + self.assertEqual( + Artifact.generate_filename(Talk(**self.KWARGS), ".pdf"), + self.LEGACY + "_Talk.pdf", + ) + + def test_include_type_suffix_false_returns_the_pre_1404_name(self): + """The legacy form is what the backfill guard compares against so a + pre-#1404 standardized file isn't mistaken for an original upload.""" + self.assertEqual( + Artifact.generate_filename(Talk(**self.KWARGS), + include_type_suffix=False), + self.LEGACY, + ) + self.assertEqual( + Artifact.generate_filename(Talk(**self.KWARGS), ".pdf", + include_type_suffix=False), + self.LEGACY + ".pdf", + ) + + +class TypeSuffixRestandardizationTests(DatabaseTestCase): + """ + The #1404 scheme change is applied retroactively: files already carrying the + pre-#1404 standardized name are re-renamed once by + restandardize_artifact_filenames (the entrypoint step that already runs on + every container start), and publications — which keep the old scheme — must + not churn. + """ + + def setUp(self): + super().setUp() + # Disposable MEDIA_ROOT: these tests write real files, and the renames + # must never touch the developer's media/ tree. + self.media_root = tempfile.mkdtemp(prefix="ml_media_test_") + self.addCleanup(shutil.rmtree, self.media_root, ignore_errors=True) + override = override_settings(MEDIA_ROOT=self.media_root) + override.enable() + self.addCleanup(override.disable) + + def _talk_with_legacy_scheme_files(self, last_name="Kim", title="My Talk", + year=2019, name_suffix=""): + """ + A talk whose pdf/raw on disk carry the *pre-#1404* standardized name — + i.e. what every already-renamed talk on prod looks like today. + + Built with ``pdf_file=None`` so the factory's own (already new-scheme) + upload doesn't occupy the rename target, then real files are dropped at + the legacy path and the row repointed at them. + Returns ``(talk, legacy_base)``. + """ + person = self.make_person(last_name=last_name) + talk = TalkFactory(title=title, forum_name="CHI", date=date(year, 1, 1), + pdf_file=None, authors=[person]) + legacy_base = Artifact.generate_filename(talk, include_type_suffix=False) + pdf_name = default_storage.save( + f"talks/{legacy_base}{name_suffix}.pdf", ContentFile(b"%PDF-1.4 x")) + raw_name = default_storage.save( + f"talks/{legacy_base}{name_suffix}.pptx", ContentFile(b"PKx")) + thumb_name = default_storage.save( + f"talks/images/{legacy_base}{name_suffix}.jpg", ContentFile(b"\xff\xd8jpg")) + Talk.objects.filter(pk=talk.pk).update( + pdf_file=pdf_name, raw_file=raw_name, thumbnail=thumb_name, + original_pdf_filename=None, original_raw_filename=None, + ) + talk.refresh_from_db() + return talk, legacy_base + + def test_legacy_scheme_talk_gains_the_type_segment(self): + talk, legacy_base = self._talk_with_legacy_scheme_files() + + call_command("restandardize_artifact_filenames") + + talk.refresh_from_db() + self.assertEqual(os.path.basename(talk.pdf_file.name), + f"{legacy_base}_Talk.pdf") + self.assertEqual(os.path.basename(talk.raw_file.name), + f"{legacy_base}_Talk.pptx") + # The thumbnail tracks the same base — save()'s three rename branches + # and its thumbnail-existence probe all derive from generate_filename, + # so a disagreement here would mean a thumbnail regenerated (or renamed) + # on every save. + self.assertEqual(os.path.basename(talk.thumbnail.name), + f"{legacy_base}_Talk.jpg") + self.assertTrue(default_storage.exists(talk.pdf_file.name)) + self.assertTrue(default_storage.exists(talk.raw_file.name)) + self.assertTrue(default_storage.exists(talk.thumbnail.name)) + + # Idempotent: the corpus-wide rename happens exactly once, not on every + # deploy (this command runs at every container start). + pdf_after, raw_after = talk.pdf_file.name, talk.raw_file.name + call_command("restandardize_artifact_filenames") + talk.refresh_from_db() + self.assertEqual(talk.pdf_file.name, pdf_after) + self.assertEqual(talk.raw_file.name, raw_after) + + def test_publication_keeps_the_unsuffixed_name(self): + """Publications are excluded from the suffix, so the scheme change must + not re-rename them — their PDFs are the indexed, externally linked ones.""" + from website.tests.factories import PublicationFactory + + person = self.make_person(last_name="Lee") + pub = PublicationFactory(title="A Paper", forum_name="CHI", + date=date(2021, 1, 1), authors=[person]) + before = pub.pdf_file.name + self.assertNotIn("_Publication", before) + + call_command("restandardize_artifact_filenames") + + pub.refresh_from_db() + self.assertEqual(pub.pdf_file.name, before) + + def test_backfill_does_not_record_a_legacy_scheme_name_as_the_original(self): + """ + backfill_original_filenames runs BEFORE the re-standardization on every + container start. Without a legacy-aware guard it would read every + already-renamed talk as "never renamed" the moment the scheme changed, + and write the old standardized name into "Originally uploaded as" — + false provenance, on the very next deploy. + """ + talk, _ = self._talk_with_legacy_scheme_files(last_name="Park") + + call_command("backfill_original_filenames") + + talk.refresh_from_db() + self.assertIsNone(talk.original_pdf_filename) + self.assertIsNone(talk.original_raw_filename) + + def test_backfill_ignores_a_uniquified_legacy_scheme_name(self): + """Same guard, for a legacy name that collided on disk and picked up the + "-" uniqueness suffix.""" + talk, _ = self._talk_with_legacy_scheme_files( + last_name="Chen", name_suffix="-1782399772.42") + + call_command("backfill_original_filenames") + + talk.refresh_from_db() + self.assertIsNone(talk.original_pdf_filename) + self.assertIsNone(talk.original_raw_filename) diff --git a/website/tests/test_fileutils.py b/website/tests/test_fileutils.py index a0f5ab20..44acdbf3 100644 --- a/website/tests/test_fileutils.py +++ b/website/tests/test_fileutils.py @@ -20,6 +20,7 @@ get_filename_no_ext, get_filename_without_ext_for_artifact, is_image, + matches_standardized_basename, rename_artifact_on_filesystem, ) @@ -134,6 +135,65 @@ def test_title_truncation(self): title_part = result.split("_")[1] self.assertEqual(len(title_part), 5) + def test_type_suffix_is_appended_after_the_year(self): + """The artifact-type segment (#1404) goes at the END, so the rest of the + name stays byte-identical to the pre-#1404 scheme (a paper and its talk + still sort next to each other).""" + self.assertEqual( + get_filename_without_ext_for_artifact( + "Doe", "Paper", "CHI", date(2022, 1, 1), type_suffix="Talk" + ), + "Doe_Paper_CHI2022_Talk", + ) + + def test_type_suffix_composes_with_the_mid_name_suffix(self): + """The trailing type segment is independent of the (unused, legacy) + mid-name suffix slot; adding one must not displace the other.""" + self.assertEqual( + get_filename_without_ext_for_artifact( + "Doe", "Paper", "CHI", date(2022, 1, 1), + suffix="poster", type_suffix="Talk", + ), + "Doe_Paper_poster_CHI2022_Talk", + ) + + def test_type_suffix_precedes_the_extension(self): + self.assertEqual( + get_filename_for_artifact( + "Doe", "Paper", "CHI", date(2022, 1, 1), "pdf", + type_suffix="Poster", + ), + "Doe_Paper_CHI2022_Poster.pdf", + ) + + +class MatchesStandardizedBasenameTests(SimpleTestCase): + """The shared uniquifier-tolerant name comparison used by the filename + management commands (#1404 extracted it from three copies).""" + + def test_exact_match(self): + self.assertTrue( + matches_standardized_basename("Doe_Paper_CHI2022", "Doe_Paper_CHI2022")) + + def test_uniquified_variant_matches(self): + """ensure_filename_is_unique appends "-" on a disk collision; + such a file is still standardized (else the commands re-rename it on + every deploy).""" + self.assertTrue(matches_standardized_basename( + "Doe_Paper_CHI2022-1782399772.42", "Doe_Paper_CHI2022")) + + def test_unrelated_name_does_not_match(self): + self.assertFalse( + matches_standardized_basename("MyTalk_v3_final", "Doe_Paper_CHI2022")) + + def test_type_suffixed_name_does_not_match_the_unsuffixed_base(self): + """The two schemes must stay distinguishable: this is what makes a + pre-#1404 talk file get re-standardized rather than read as current.""" + self.assertFalse(matches_standardized_basename( + "Doe_Paper_CHI2022_Talk", "Doe_Paper_CHI2022")) + self.assertFalse(matches_standardized_basename( + "Doe_Paper_CHI2022", "Doe_Paper_CHI2022_Talk")) + class EnsureFilenameIsUniqueTests(SimpleTestCase): def test_nonexistent_path_returned_unchanged(self): diff --git a/website/utils/fileutils.py b/website/utils/fileutils.py index d55fbf14..7f3f7e9b 100644 --- a/website/utils/fileutils.py +++ b/website/utils/fileutils.py @@ -239,9 +239,11 @@ def get_filename_no_ext(filename): """Returns the *just* filename without the extension (no other path information)""" return os.path.splitext(os.path.basename(filename))[0] -def get_filename_for_artifact(last_name, title, forum_name, date, ext, suffix=None, max_pub_title_length=-1): +def get_filename_for_artifact(last_name, title, forum_name, date, ext, suffix=None, max_pub_title_length=-1, + type_suffix=None): """Generates a filename from the provided content.""" - filename_without_ext = get_filename_without_ext_for_artifact(last_name, title, forum_name, date, suffix, max_pub_title_length) + filename_without_ext = get_filename_without_ext_for_artifact(last_name, title, forum_name, date, suffix, + max_pub_title_length, type_suffix) # Check if ext starts with a dot. If not, add it if not ext.startswith('.'): @@ -250,8 +252,26 @@ def get_filename_for_artifact(last_name, title, forum_name, date, ext, suffix=No # Combine filename with extension return filename_without_ext + ext -def get_filename_without_ext_for_artifact(last_name, title, forum_name, date, suffix=None, max_pub_title_length=-1): - """Generates a filename from the provided content""" +def get_filename_without_ext_for_artifact(last_name, title, forum_name, date, suffix=None, max_pub_title_length=-1, + type_suffix=None): + """ + Generates a filename from the provided content. + + The scheme is ``LastName_TitleInTitleCase_[suffix_]ForumYear[_TypeSuffix]``. + + :param suffix: an optional segment inserted *between* the title and the + forum. Legacy slot; nothing in the codebase passes it today. + :param type_suffix: an optional artifact-type segment appended at the *end* + (e.g. "Talk", "Poster") so a downloaded file says what kind of artifact + it is (#1404). Trailing rather than mid-name so the rest of the name is + byte-identical to the pre-#1404 scheme — a paper and its talk still sort + next to each other, and the "-" uniqueness suffix + (:func:`ensure_filename_is_unique`) still appends cleanly after it. + + >>> get_filename_without_ext_for_artifact("Froehlich", "Making In The HCIL", + ... "CHI", date(2024, 1, 1), type_suffix="Talk") + 'Froehlich_MakingInTheHcil_CHI2024_Talk' + """ if not last_name: last_name = "None" @@ -280,8 +300,34 @@ def get_filename_without_ext_for_artifact(last_name, title, forum_name, date, su # Add the rest of the metadata new_filename_no_ext += f"{forum_name}{year}" + # Add the artifact-type segment last (#1404), e.g. "..._CHI2024_Talk" + if type_suffix: + new_filename_no_ext += f"_{type_suffix}" + return get_valid_filename(new_filename_no_ext) - + + +def matches_standardized_basename(basename_no_ext, standardized_base): + """ + Whether an on-disk (extension-less) basename is the given standardized name, + tolerating the uniqueness suffix. + + When a standardized name collides on disk, :func:`ensure_filename_is_unique` + appends ``-`` (e.g. ``Lee_Talk_CHI2021-1782399772.42``), so such a + file IS standardized even though it isn't equal to the generated name. + Comparing with plain equality instead would mark those rows dirty forever and + churn their filenames on every deploy. + + Note the two schemes stay distinguishable: an unsuffixed base does not match + a ``_Talk``-suffixed name (or vice versa), which is what makes a pre-#1404 + file get re-standardized exactly once. + + >>> matches_standardized_basename("Lee_Talk_CHI2021-1782399772.42", "Lee_Talk_CHI2021") + True + """ + return (basename_no_ext == standardized_base + or basename_no_ext.startswith(standardized_base + "-")) + def ensure_filename_is_unique(filename_with_full_path): """Will return a filename with full path that is guaranteed to be unique""" From e290502bcc0c228deebc64da3bf2fc860501e534 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 27 Jul 2026 16:18:29 -0700 Subject: [PATCH 2/2] Review follow-ups: pin legacy-base orphan repair, honest dry-run version note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items from the PR review: 1. Test the legacy-base branch of repair_diverged_artifact_filenames — the one changed path with no test. An orphan left on disk under the pre-#1404 base (plain and "-"-uniquified) is found and repaired forward to the new-scheme name; without these, dropping the legacy_base half of the scan would silently strand old orphans. 2. The 2.29.0 version description now says this is the dry-run stage (renames inventoried in debug.log, not performed), so the admin dashboard doesn't claim a rename that hasn't happened. The stage-2 commit that removes --dry-run should restore the final wording. Co-Authored-By: Claude Fable 5 --- makeabilitylab/settings.py | 2 +- ...test_repair_diverged_artifact_filenames.py | 59 ++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index 8fe3c2ca..9cc2ad47 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -87,7 +87,7 @@ # Makeability Lab Global Variables, including Makeability Lab version ML_WEBSITE_VERSION = "2.29.0" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "Standardized filenames for talks and posters now end in '_Talk' and '_Poster' (#1404), so a downloaded slide deck or poster is no longer indistinguishable from the paper PDF of the same work. Publication filenames are unchanged. Existing talk/poster files are renamed once, on deploy." +ML_WEBSITE_VERSION_DESCRIPTION = "Standardized filenames for talks and posters now end in '_Talk' and '_Poster' (#1404), so a downloaded slide deck or poster is no longer indistinguishable from the paper PDF of the same work. Publication filenames are unchanged. Dry-run stage: existing files are NOT renamed yet — the pending renames are inventoried in debug.log for review, and the one-time rename ships in the follow-up release." DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed MAX_BANNERS = 7 # Maximum number of banners on a page diff --git a/website/tests/test_repair_diverged_artifact_filenames.py b/website/tests/test_repair_diverged_artifact_filenames.py index 54f00a44..7fa273ff 100644 --- a/website/tests/test_repair_diverged_artifact_filenames.py +++ b/website/tests/test_repair_diverged_artifact_filenames.py @@ -32,12 +32,20 @@ def setUp(self): override.enable() self.addCleanup(override.disable) - def _simulate_divergence(self, talk): + def _simulate_divergence(self, talk, include_type_suffix=True, + name_suffix=""): """Reproduce the bug's end state for the pdf_file: move the real file to the extension-less standardized base (the orphan) and point the DB at a - now-missing old name.""" + now-missing old name. + + ``include_type_suffix=False`` names the orphan under the pre-#1404 + scheme (no trailing "_Talk") — what an orphan left behind before that + scheme change looks like. ``name_suffix`` appends a + "-"-style uniqueness suffix to the orphan. + """ directory = os.path.dirname(talk.pdf_file.path) - valid_base = get_valid_filename(Artifact.generate_filename(talk)) + valid_base = get_valid_filename(Artifact.generate_filename( + talk, include_type_suffix=include_type_suffix)) + name_suffix orphan_path = os.path.join(directory, valid_base) # no extension os.rename(talk.pdf_file.path, orphan_path) # DB now references a file that isn't there. @@ -69,6 +77,51 @@ def test_repairs_pdf_pointing_at_missing_file(self): self.assertTrue(talk.pdf_file.storage.exists(talk.pdf_file.name)) self.assertFalse(os.path.exists(orphan_path)) + def test_repairs_orphan_left_under_the_pre_1404_base(self): + """The #1390 bug predates the #1404 scheme change, so an orphan it left + behind carries the OLD standardized base (no trailing "_Talk"). The + command must still find it — and repairs it forward to the new-scheme + name.""" + alice = self.make_person(first_name="Alice", last_name="Smith") + talk = self.make_talk( + title="A Recoverable Talk", forum_name="CHI", year=2024, + authors=[alice], + ) + _, orphan_path = self._simulate_divergence( + talk, include_type_suffix=False) + self.assertTrue(os.path.exists(orphan_path)) + + call_command("repair_diverged_artifact_filenames") + + talk.refresh_from_db() + new_base = get_valid_filename(Artifact.generate_filename(talk)) + self.assertEqual( + os.path.basename(talk.pdf_file.name), new_base + ".pdf" + ) + self.assertTrue(talk.pdf_file.storage.exists(talk.pdf_file.name)) + self.assertFalse(os.path.exists(orphan_path)) + + def test_repairs_uniquified_orphan_under_the_pre_1404_base(self): + """Same, for a pre-#1404 orphan whose name collided on disk and picked + up the "-" uniqueness suffix (ensure_filename_is_unique).""" + alice = self.make_person(first_name="Alice", last_name="Smith") + talk = self.make_talk( + title="A Recoverable Talk", forum_name="CHI", year=2024, + authors=[alice], + ) + _, orphan_path = self._simulate_divergence( + talk, include_type_suffix=False, name_suffix="-1782399772.42") + + call_command("repair_diverged_artifact_filenames") + + talk.refresh_from_db() + new_base = get_valid_filename(Artifact.generate_filename(talk)) + self.assertEqual( + os.path.basename(talk.pdf_file.name), new_base + ".pdf" + ) + self.assertTrue(talk.pdf_file.storage.exists(talk.pdf_file.name)) + self.assertFalse(os.path.exists(orphan_path)) + def test_dry_run_changes_nothing(self): alice = self.make_person(first_name="Alice", last_name="Smith") talk = self.make_talk(