refactor: Do not use canonical_name() for charters (ietf-tools#5818)

* fix: Enforce naming of charter docs in submit()

* style: Reformat submit() with Black

* refactor: Remove redundant check of charter name

* style: Reformat charter_with_milestones_txt with Black

* refactor: Drop canonical_name, use Path in charter_with_milestones_txt

* style: Reformat review_announcement_text() with Black

* style: Reformat action_announcement_text() with Black

* refactor: Change uses of charter.canonical_name() to charter.name

* refactor: Skip docialias when retrieving charter

* refactor: Change canonical_name() to name in utils_charter.py

* refactor: Use Path in read_charter_text()

* refactor: Drop canonical_name, minor refactor of tests_charter.py

* refactor: charter.name instead of canonical_name in milestones.py

* refactor: charter.name instead of canonical_name in tests_info.py

* refactor: Remove unused functions in ietf/secr/utils/groups.py

* refactor: charter.canonical_name -> charter.name in templates

* refactor: Remove charter handling from canonical_name

Temporarily raise an exception for testing

* refactor: Refactor get_charter_text() without canonical_name

* refactor: Remove raise when canonical_name called on a charter

* fix: Add back missing ".txt" extension

* test: Test rejection of invalid charter names

Author: jennifer-richards

ietf/doc/models.py

@@ -856,12 +856,6 @@ def canonical_name(self):
                 a = self.docalias.filter(name__startswith="rfc").order_by('-name').first()
                 if a:
                     name = a.name
-            elif self.type_id == "charter":
-                from ietf.doc.utils_charter import charter_name_for_group # Imported locally to avoid circular imports
-                try:
-                    name = charter_name_for_group(self.chartered_group)
-                except Group.DoesNotExist:
-                    pass
             self._canonical_name = name
         return self._canonical_name
 

ietf/doc/tests_charter.py

@@ -88,10 +88,7 @@ class EditCharterTests(TestCase):
     settings_temp_path_overrides = TestCase.settings_temp_path_overrides + ['CHARTER_PATH']
 
     def write_charter_file(self, charter):
-        with (Path(settings.CHARTER_PATH) /
-              ("%s-%s.txt" % (charter.canonical_name(), charter.rev))
-        ).open("w") as f:
-            f.write("This is a charter.")
+        (Path(settings.CHARTER_PATH) / f"{charter.name}-{charter.rev}.txt").write_text("This is a charter.")
 
     def test_startstop_process(self):
         CharterFactory(group__acronym='mars')
@@ -509,8 +506,13 @@ def test_submit_charter(self):
         self.assertEqual(charter.rev, next_revision(prev_rev))
         self.assertTrue("new_revision" in charter.latest_event().type)
 
-        with (Path(settings.CHARTER_PATH) / (charter.canonical_name() + "-" + charter.rev + ".txt")).open(encoding='utf-8') as f:
-            self.assertEqual(f.read(), "Windows line\nMac line\nUnix line\n" + utf_8_snippet.decode('utf-8'))
+        file_contents = (
+            Path(settings.CHARTER_PATH) / (charter.name + "-" + charter.rev + ".txt")
+        ).read_text("utf-8")
+        self.assertEqual(
+            file_contents,
+            "Windows line\nMac line\nUnix line\n" + utf_8_snippet.decode("utf-8"),
+        )
 
     def test_submit_initial_charter(self):
         group = GroupFactory(type_id='wg',acronym='mars',list_email='mars-wg@ietf.org')
@@ -538,6 +540,24 @@ def test_submit_initial_charter(self):
         group = Group.objects.get(pk=group.pk)
         self.assertEqual(group.charter, charter)
 
+    def test_submit_charter_with_invalid_name(self):
+        self.client.login(username="secretary", password="secretary+password")
+        ietf_group = GroupFactory(type_id="wg")
+        for bad_name in ("charter-irtf-{}", "charter-randomjunk-{}", "charter-ietf-thisisnotagroup"):
+            url = urlreverse("ietf.doc.views_charter.submit", kwargs={"name": bad_name.format(ietf_group.acronym)})
+            r = self.client.get(url)
+            self.assertEqual(r.status_code, 404, f"GET of charter named {bad_name} should 404")
+            r = self.client.post(url, {})
+            self.assertEqual(r.status_code, 404, f"POST of charter named {bad_name} should 404")
+
+        irtf_group = GroupFactory(type_id="rg")
+        for bad_name in ("charter-ietf-{}", "charter-whatisthis-{}", "charter-irtf-thisisnotagroup"):
+            url = urlreverse("ietf.doc.views_charter.submit", kwargs={"name": bad_name.format(irtf_group.acronym)})
+            r = self.client.get(url)
+            self.assertEqual(r.status_code, 404, f"GET of charter named {bad_name} should 404")
+            r = self.client.post(url, {})
+            self.assertEqual(r.status_code, 404, f"POST of charter named {bad_name} should 404")
+
     def test_edit_review_announcement_text(self):
         area = GroupFactory(type_id='area')
         RoleFactory(name_id='ad',group=area,person=Person.objects.get(user__username='ad'))

ietf/doc/utils_charter.py

@@ -3,11 +3,12 @@
 
 
 import datetime
-import io
 import os
 import re
 import shutil
 
+from pathlib import Path
+
 from django.conf import settings
 from django.urls import reverse as urlreverse
 from django.template.loader import render_to_string
@@ -62,10 +63,9 @@ def next_approved_revision(rev):
     return "%#02d" % (int(m.group('major')) + 1)
 
 def read_charter_text(doc):
-    filename = os.path.join(settings.CHARTER_PATH, '%s-%s.txt' % (doc.canonical_name(), doc.rev))
+    filename = Path(settings.CHARTER_PATH) / f"{doc.name}-{doc.rev}.txt"
     try:
-        with io.open(filename, 'r') as f:
-            return f.read()
+        return filename.read_text()
     except IOError:
         return "Error: couldn't read charter text"
 
@@ -92,16 +92,16 @@ def change_group_state_after_charter_approval(group, by):
 def fix_charter_revision_after_approval(charter, by):
     # according to spec, 00-02 becomes 01, so copy file and record new revision
     try:
-        old = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.canonical_name(), charter.rev))
-        new = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.canonical_name(), next_approved_revision(charter.rev)))
+        old = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.name, charter.rev))
+        new = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.name, next_approved_revision(charter.rev)))
         shutil.copy(old, new)
     except IOError:
         log("There was an error copying %s to %s" % (old, new))
 
     events = []
     e = NewRevisionDocEvent(doc=charter, by=by, type="new_revision")
     e.rev = next_approved_revision(charter.rev)
-    e.desc = "New version available: <b>%s-%s.txt</b>" % (charter.canonical_name(), e.rev)
+    e.desc = "New version available: <b>%s-%s.txt</b>" % (charter.name, e.rev)
     e.save()
     events.append(e)