fix: Use more robust filename/rev matching for meeting materials (#5192)

* fix: Use more robust filename/rev matching for meeting materials

* chore: Remove debug statement

* test: Update test to catch bug fixed by this PR

* fix: Be more discerning when parsing revision

* test: Update test to exercise revision parsing/validation

* style: Apply Black styling to replaced code

ietf/meeting/tests_views.py

@@ -35,7 +35,7 @@ from django.utils.text import slugify
 
 import debug           # pyflakes:ignore
 
-from ietf.doc.models import Document
+from ietf.doc.models import Document, NewRevisionDocEvent
 from ietf.group.models import Group, Role, GroupFeatures
 from ietf.group.utils import can_manage_group
 from ietf.person.models import Person
@@ -607,13 +607,73 @@ class MeetingTests(BaseMeetingTestCase):
 
     @override_settings(MEETING_MATERIALS_SERVE_LOCALLY=True)
     def test_materials_name_endswith_hyphen_number_number(self):
-        sp = SessionPresentationFactory(document__name='slides-junk-15',document__type_id='slides',document__states=[('reuse_policy','single')])
-        sp.document.uploaded_filename = '%s-%s.pdf'%(sp.document.name,sp.document.rev)
+        # be sure a shadowed filename without the hyphen does not interfere
+        shadow = SessionPresentationFactory(
+            document__name="slides-115-junk",
+            document__type_id="slides",
+            document__states=[("reuse_policy", "single")],
+        )
+        shadow.document.uploaded_filename = (
+            f"{shadow.document.name}-{shadow.document.rev}.pdf"
+        )
+        shadow.document.save()
+        # create the material we want to find for the test
+        sp = SessionPresentationFactory(
+            document__name="slides-115-junk-15",
+            document__type_id="slides",
+            document__states=[("reuse_policy", "single")],
+        )
+        sp.document.uploaded_filename = f"{sp.document.name}-{sp.document.rev}.pdf"
         sp.document.save()
-        self.write_materials_file(sp.session.meeting, sp.document, 'Fake slide contents')
-        url = urlreverse("ietf.meeting.views.materials_document", kwargs=dict(document=sp.document.name,num=sp.session.meeting.number))
+        self.write_materials_file(
+            sp.session.meeting, sp.document, "Fake slide contents rev 00"
+        )
+
+        # create rev 01
+        sp.document.rev = "01"
+        sp.document.uploaded_filename = f"{sp.document.name}-{sp.document.rev}.pdf"
+        sp.document.save_with_history(
+            [
+                NewRevisionDocEvent.objects.create(
+                    type="new_revision",
+                    doc=sp.document,
+                    rev=sp.document.rev,
+                    by=Person.objects.get(name="(System)"),
+                    desc=f"New version available: <b>{sp.document.name}-{sp.document.rev}.txt</b>",
+                )
+            ]
+        )
+        self.write_materials_file(
+            sp.session.meeting, sp.document, "Fake slide contents rev 01"
+        )
+        url = urlreverse(
+            "ietf.meeting.views.materials_document",
+            kwargs=dict(document=sp.document.name, num=sp.session.meeting.number),
+        )
         r = self.client.get(url)
-        self.assertEqual(r.status_code, 200)
+        self.assertContains(
+            r,
+            "Fake slide contents rev 01",
+            status_code=200,
+            msg_prefix="Should return latest rev by default",
+        )
+        url = urlreverse(
+            "ietf.meeting.views.materials_document",
+            kwargs=dict(document=sp.document.name + "-00", num=sp.session.meeting.number),
+        )
+        r = self.client.get(url)
+        self.assertContains(
+            r,
+            "Fake slide contents rev 00",
+            status_code=200,
+            msg_prefix="Should return existing version on request",
+        )
+        url = urlreverse(
+            "ietf.meeting.views.materials_document",
+            kwargs=dict(document=sp.document.name + "-02", num=sp.session.meeting.number),
+        )
+        r = self.client.get(url)
+        self.assertEqual(r.status_code, 404, "Should not find nonexistent version")
 
     def test_important_dates(self):
         meeting=MeetingFactory(type_id='ietf')

ietf/meeting/views.py

@@ -202,32 +202,37 @@ def current_materials(request):
     else:
         raise Http404('No such meeting')
 
+
+def _get_materials_doc(meeting, name):
+    """Get meeting materials document named by name
+
+    Raises Document.DoesNotExist if a match cannot be found.
+    """
+    # try an exact match first
+    doc = Document.objects.filter(name=name).first()
+    if doc is not None and doc.get_related_meeting() == meeting:
+        return doc, None
+    # try parsing a rev number
+    if "-" in name:
+        docname, rev = name.rsplit("-", 1)
+        if len(rev) == 2 and rev.isdigit():
+            doc = Document.objects.get(name=docname)  # may raise Document.DoesNotExist
+            if doc.get_related_meeting() == meeting and rev in doc.revisions():
+                return doc, rev
+    # give up
+    raise Document.DoesNotExist
+
+
 @cache_page(1 * 60)
 def materials_document(request, document, num=None, ext=None):
     meeting=get_meeting(num,type_in=['ietf','interim'])
     num = meeting.number
-    if (re.search(r'^\w+-\d+-.+-\d\d$', document) or
-        re.search(r'^\w+-interim-\d+-.+-\d\d-\d\d$', document) or
-        re.search(r'^\w+-interim-\d+-.+-sess[a-z]-\d\d$', document) or
-        re.search(r'^(minutes|slides|chatlog|polls)-interim-\d+-.+-\d\d$', document)):
-        name, rev = document.rsplit('-', 1)
-    else:
-        name, rev = document, None
     # This view does not allow the use of DocAliases. Right now we are probably only creating one (identity) alias, but that may not hold in the future.
-    doc = Document.objects.filter(name=name).first()
-    # Handle edge case where the above name, rev splitter misidentifies the end of a document name as a revision number
-    if not doc:
-        if rev:
-            name = name + '-' + rev
-            rev = None
-            doc = get_object_or_404(Document, name=name)
-        else:
-            raise Http404("No such document")
-
-    if not doc.meeting_related():
-        raise Http404("Not a meeting related document")
-    if doc.get_related_meeting() != meeting:
+    try:
+        doc, rev = _get_materials_doc(meeting=meeting, name=document)
+    except Document.DoesNotExist:
         raise Http404("No such document for meeting %s" % num)
+
     if not rev:
         filename = doc.get_file_name()
     else: