Merged in [15864] from rjsparks@nostrum.com:

    Refactored DocumentInfo to address overloading the external_url field with strings that are not URLs.
 - Legacy-Id: 15876
Note: SVN reference [15864] has been migrated to Git commit 25cc00f

Author: levkowetz

ietf/doc/migrations/0008_add_uploaded_filename.py

@@ -0,0 +1,25 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.17 on 2018-12-28 13:11
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('doc', '0007_idexists'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='dochistory',
+            name='uploaded_filename',
+            field=models.TextField(blank=True),
+        ),
+        migrations.AddField(
+            model_name='document',
+            name='uploaded_filename',
+            field=models.TextField(blank=True),
+        ),
+    ]

ietf/doc/migrations/0009_move_non_url_externalurls_to_uploaded_filename.py

@@ -0,0 +1,46 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.17 on 2018-12-28 13:33
+from __future__ import unicode_literals
+
+from django.db import migrations
+from django.db.models import F
+
+def forward(apps, schema_editor):
+    Document = apps.get_model('doc','Document')
+    Document.objects.exclude(type_id__in=('review','recording')).update(uploaded_filename = F('external_url'))
+    Document.objects.exclude(type_id__in=('review','recording')).update(external_url="")
+
+    Document.objects.filter(name='slides-100-edu-sessf-patents-at-ietf-an-overview-of-bcp79bis').update(uploaded_filename='slides-100-edu-sessf-patents-at-ietf-an-overview-of-bcp79bis-00.pdf')
+
+    DocHistory = apps.get_model('doc','DocHistory')
+    DocHistory.objects.exclude(type_id__in=('review','recording')).update(uploaded_filename = F('external_url'))
+    DocHistory.objects.exclude(type_id__in=('review','recording')).update(external_url="")
+    
+    DocHistory.objects.filter(uploaded_filename='https://www.ietf.org/proceedings/97/slides/slides-97-edu-sessb-local-version-of-newcomers-training-in-korean-00.pdf').update(uploaded_filename='slides-97-edu-sessb-local-version-of-newcomers-training-in-korean-00.pdf')
+    DocHistory.objects.filter(uploaded_filename='http://materials-98-codec-opus-newvectors-00.tar.gz').update(uploaded_filename='materials-98-codec-opus-newvectors-00.tar.gz')
+    DocHistory.objects.filter(uploaded_filename='http://materials-98-codec-opus-update-00.patch').update(uploaded_filename='materials-98-codec-opus-update-00.patch')
+    DocHistory.objects.filter(uploaded_filename='http://slides-100-edu-sessf-patents-at-ietf-an-overview-of-bcp79bis-00.pdf').update(uploaded_filename='slides-100-edu-sessf-patents-at-ietf-an-overview-of-bcp79bis-00.pdf')
+    DocHistory.objects.filter(uploaded_filename='http://bluesheets-97-6man-201611150930-00.pdf/').update(uploaded_filename='bluesheets-97-6man-201611150930-00.pdf')
+    DocHistory.objects.filter(uploaded_filename='http://agenda-interim-2017-stir-01-stir-01-01.txt').update(uploaded_filename='agenda-interim-2017-stir-01-stir-01-01.txt')
+    DocHistory.objects.filter(uploaded_filename='http://agenda-interim-2017-icnrg-02-icnrg-01-05.html').update(uploaded_filename='agenda-interim-2017-icnrg-02-icnrg-01-05.html')
+
+
+def reverse(apps, schema_editor):
+    Document = apps.get_model('doc','Document')
+    Document.objects.exclude(type_id__in=('review','recording')).update(external_url = F('uploaded_filename'))
+    Document.objects.exclude(type_id__in=('review','recording')).update(uploaded_filename="")
+
+    DocHistory = apps.get_model('doc','DocHistory')
+    DocHistory.objects.exclude(type_id__in=('review','recording')).update(external_url = F('uploaded_filename'))
+    DocHistory.objects.exclude(type_id__in=('review','recording')).update(uploaded_filename="")
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('doc', '0008_add_uploaded_filename'),
+    ]
+
+    operations = [
+        migrations.RunPython(forward,reverse),
+    ]

ietf/doc/models.py

@@ -97,13 +97,19 @@ class DocumentInfo(models.Model):
     shepherd = ForeignKey(Email, related_name='shepherd_%(class)s_set', blank=True, null=True)
     expires = models.DateTimeField(blank=True, null=True)
     notify = models.CharField(max_length=255, blank=True)
-    external_url = models.URLField(blank=True) # Should be set for documents with type 'External'.
+    external_url = models.URLField(blank=True)
+    uploaded_filename = models.TextField(blank=True)
     note = models.TextField(blank=True)
     internal_comments = models.TextField(blank=True)
 
     def file_extension(self):
-        _,ext = os.path.splitext(self.external_url)
-        return ext.lstrip(".").lower()
+        if not hasattr(self, '_cached_extension'):
+            if self.uploaded_filename:
+                _, ext= os.path.splitext(self.uploaded_filename)
+                self._cached_extension = ext.lstrip(".").lower()
+            else:
+                self._cached_extension = "txt"
+        return self._cached_extension
 
     def get_file_path(self):
         if not hasattr(self, '_cached_file_path'):
@@ -138,7 +144,9 @@ def get_file_path(self):
 
     def get_base_name(self):
         if not hasattr(self, '_cached_base_name'):
-            if self.type_id == 'draft':
+            if self.uploaded_filename:
+                self._cached_base_name = self.uploaded_filename
+            elif self.type_id == 'draft':
                 if self.is_dochistory():
                     self._cached_base_name = "%s-%s.txt" % (self.doc.name, self.rev)
                 else:
@@ -147,12 +155,9 @@ def get_base_name(self):
                     else:
                         self._cached_base_name = "%s-%s.txt" % (self.name, self.rev)
             elif self.type_id in ["slides", "agenda", "minutes", "bluesheets", ] and self.meeting_related():
-                if self.external_url:
-                    # we need to remove the extension for the globbing below to work
-                    self._cached_base_name = self.external_url
-                else:
-                    self._cached_base_name = "%s.txt" % self.canonical_name() # meeting materials are unversioned at the moment
+                    self._cached_base_name = "%s-%s.txt" % self.canonical_name() 
             elif self.type_id == 'review':
+                # TODO: This will be wrong if a review is updated on the same day it was created (or updated more than once on the same day)
                 self._cached_base_name = "%s.txt" % self.name
             else:
                 if self.rev:
@@ -195,15 +200,14 @@ def _get_ref(self, meeting=None, meeting_doc_refs=settings.MEETING_DOC_HREFS):
         # the earlier resulution order, but there's at the moment one single
         # instance which matches this (with correct results), so we won't
         # break things all over the place.
-        # XXX TODO: move all non-URL 'external_url' values into a field which is
-        # better named, or regularize the filename based on self.name
         if not hasattr(self, '_cached_href'):
             validator = URLValidator()
             if self.external_url and self.external_url.split(':')[0] in validator.schemes:
                 try:
                     validator(self.external_url)
                     return self.external_url
                 except ValidationError:
+                    log.unreachable('2018-12-28')
                     pass
 
 
@@ -223,10 +227,7 @@ def _get_ref(self, meeting=None, meeting_doc_refs=settings.MEETING_DOC_HREFS):
             elif self.type_id in meeting_doc_refs:
                 self.is_meeting_related = True
             else:
-                if len(self.external_url):
-                    return self.external_url
-                else:
-                    return None
+                return None
 
             if self.is_meeting_related:
                 if not meeting:
@@ -625,7 +626,7 @@ def get_absolute_url(self):
                     if self.type_id == 'recording':
                         url = self.external_url
                     else:
-                        filename = self.external_url
+                        filename = self.uploaded_filename
                         url = '%sproceedings/%s/%s/%s' % (settings.IETF_HOST_URL,meeting.number,self.type_id,filename)
             else:
                 url = urlreverse('ietf.doc.views_doc.document_main', kwargs={ 'name': name }, urlconf="ietf.urls")

ietf/doc/resources.py

@@ -116,6 +116,7 @@ class Meta:
             "expires": ALL,
             "notify": ALL,
             "external_url": ALL,
+            "uploaded_filename": ALL,
             "note": ALL,
             "internal_comments": ALL,
             "name": ALL,
@@ -228,6 +229,7 @@ class Meta:
             "expires": ALL,
             "notify": ALL,
             "external_url": ALL,
+            "uploaded_filename": ALL,
             "note": ALL,
             "internal_comments": ALL,
             "name": ALL,

ietf/doc/views_doc.py

@@ -549,11 +549,9 @@ def document_main(request, name, rev=None):
     if doc.type_id in ("slides", "agenda", "minutes", "bluesheets",):
         can_manage_material = can_manage_materials(request.user, doc.group)
         presentations = doc.future_presentations()
-        if doc.meeting_related():
-            basename = doc.canonical_name() # meeting materials are unversioned at the moment
-            if doc.external_url:
-                # we need to remove the extension for the globbing below to work
-                basename = os.path.splitext(doc.external_url)[0]
+        if doc.uploaded_filename:
+            # we need to remove the extension for the globbing below to work
+            basename = os.path.splitext(doc.uploaded_filename)[0]
         else:
             basename = "%s-%s" % (doc.canonical_name(), doc.rev)
 

ietf/liaisons/forms.py

@@ -366,7 +366,7 @@ def save_attachments(self):
                 defaults=dict(
                     title = attachment_title,
                     type_id = "liai-att",
-                    external_url = name + extension, # strictly speaking not necessary, but just for the time being ...
+                    uploaded_filename = name + extension, 
                     )
                 )
             if created:

ietf/liaisons/tests.py

@@ -432,7 +432,7 @@ def test_edit_liaison(self):
         self.assertEqual(new_liaison.attachments.count(), attachments_before + 1)
         attachment = new_liaison.attachments.order_by("-name")[0]
         self.assertEqual(attachment.title, "attachment")
-        with open(os.path.join(self.liaison_dir, attachment.external_url)) as f:
+        with open(os.path.join(self.liaison_dir, attachment.uploaded_filename)) as f:
             written_content = f.read()
 
         test_file.seek(0)
@@ -736,7 +736,7 @@ def test_add_incoming_liaison(self):
         self.assertEqual(l.attachments.count(), 1)
         attachment = l.attachments.all()[0]
         self.assertEqual(attachment.title, "attachment")
-        with open(os.path.join(self.liaison_dir, attachment.external_url)) as f:
+        with open(os.path.join(self.liaison_dir, attachment.uploaded_filename)) as f:
             written_content = f.read()
 
         test_file.seek(0)
@@ -815,7 +815,7 @@ def test_add_outgoing_liaison(self):
         self.assertEqual(l.attachments.count(), 1)
         attachment = l.attachments.all()[0]
         self.assertEqual(attachment.title, "attachment")
-        with open(os.path.join(self.liaison_dir, attachment.external_url)) as f:
+        with open(os.path.join(self.liaison_dir, attachment.uploaded_filename)) as f:
             written_content = f.read()
 
         test_file.seek(0)

ietf/meeting/feeds.py

@@ -15,12 +15,14 @@ class LatestMeetingMaterialFeed(Feed):
 
     def items(self):
         objs = []
+        # FIXME: why aren't other materials types in here?
         for doc in Document.objects.filter(type__in=("agenda", "minutes", "slides")).order_by('-time')[:60]:
             obj = dict(
                 title=doc.type_id,
                 group_acronym=doc.name.split("-")[2],
                 date=doc.time,
-                link=self.base_url + os.path.join(doc.get_file_path(), doc.external_url)[len(settings.AGENDA_PATH):],
+                # FIXME: why isn't this using gref or href?
+                link=self.base_url + os.path.join(doc.get_file_path(), doc.uploaded_filename)[len(settings.AGENDA_PATH):],
                 author=""
                 )
             objs.append(obj)

ietf/meeting/forms.py

@@ -267,7 +267,9 @@ def save_agenda(self):
                 group=self.group,
                 name=filename,
                 rev='00',
-                external_url='{}-00.txt'.format(filename))
+                # FIXME: if these are always computed, they shouldn't be in uploaded_filename - just compute them when needed
+                # FIXME: What about agendas in html or markdown format?
+                uploaded_filename='{}-00.txt'.format(filename))
             doc.set_state(State.objects.get(type__slug=doc.type.slug, slug='active'))
             DocAlias.objects.create(name=doc.name, document=doc)
             self.instance.sessionpresentation_set.create(document=doc, rev=doc.rev)

ietf/meeting/helpers.py

@@ -208,7 +208,8 @@ def read_session_file(type, num, doc):
     # style python format, something like this:
     #  DOC_PATH_FORMAT = { "agenda": "/foo/bar/agenda-{meeting.number}/agenda-{meeting-number}-{doc.group}*", }
     #
-    path = os.path.join(settings.AGENDA_PATH, "%s/%s/%s" % (num, type, doc.external_url))
+    # FIXME: uploaded_filename should be replaced with a function call that computes names that are fixed
+    path = os.path.join(settings.AGENDA_PATH, "%s/%s/%s" % (num, type, doc.uploaded_filename))
     if os.path.exists(path):
         with open(path) as f:
             return f.read(), path