refactor: authors() -> author_persons()

ietf/doc/models.py

@@ -419,9 +419,10 @@ def friendly_state(self):
             return state.name
 
     def author_names(self):
+        """Author names as a list of strings"""
         names = []
         if self.type_id == "rfc" and self.rfcauthor_set.exists():
-            for author in self.rfcauthor_set.all():
+            for author in self.rfcauthor_set.select_related("person"):
                 if author.person:
                     names.append(author.person.name)
                 else:
@@ -435,16 +436,27 @@ def author_names(self):
         return names
 
     def author_persons_or_names(self):
+        """Authors as a list of named tuples with person and/or titlepage_name"""
         Author = namedtuple("Author", "person titlepage_name")
         persons_or_names = []
         if self.type_id=="rfc" and self.rfcauthor_set.exists():
-            for author in self.rfcauthor_set.all():
+            for author in self.rfcauthor_set.select_related("person"):
                 persons_or_names.append(Author(person=author.person, titlepage_name=author.titlepage_name))
         else:
-            for author in self.documentauthor_set.all():
+            for author in self.documentauthor_set.select_related("person"):
                 persons_or_names.append(Author(person=author.person, titlepage_name=""))
         return persons_or_names
 
+    def author_persons(self):
+        """Authors as a list of Persons
+
+        Omits any RfcAuthors with a null person field.
+        """
+        if self.type_id == "rfc" and self.rfcauthor_set.exists():
+            authors_qs = self.rfcauthor_set.filter(person__isnull=False)
+        else:
+            authors_qs = self.documentauthor_set.all()
+        return [a.person for a in authors_qs.select_related("person")]
 
     def author_list(self):
         """List of author emails"""
@@ -462,18 +474,6 @@ def author_list(self):
                     best_addresses.append(author.email.person.email_address())
         return ", ".join(best_addresses)
 
-    def authors(self):
-        if self.type_id == "rfc" and self.rfcauthor_set.exists():
-            # todo deal with non-Person RfcAuthors if they exist
-            rfc_authors = [a.person for a in self.rfcauthor_set.all()]
-            if None in rfc_authors:
-                log.log(
-                    f"FIXME: authors() cannot handle non-Person authors in {self}"
-                )
-                rfc_authors = [author for author in rfc_authors if author is not None]
-            return rfc_authors
-        return [ a.person for a in self.documentauthor_set.all() ]
-
     # This, and several other ballot related functions here, assume that there is only one active ballot for a document at any point in time.
     # If that assumption is violated, they will only expose the most recently created ballot
     def ballot_open(self, ballot_type_slug):
@@ -994,7 +994,7 @@ class Meta:
     def role_for_doc(self):
         """Brief string description of this person's relationship to the doc"""
         roles = []
-        if self.person in self.document.authors():
+        if self.person in self.document.author_persons():
             roles.append('Author')
         if self.person == self.document.ad:
             roles.append('Responsible AD')

ietf/doc/tests.py

@@ -983,7 +983,7 @@ def test_edit_authors_permissions(self):
         # Relevant users not authorized to edit authors
         unauthorized_usernames = [
             'plain',
-            *[author.user.username for author in draft.authors()],
+            *[author.user.username for author in draft.author_persons()],
             draft.group.get_chair().person.user.username,
             'ad'
         ]
@@ -998,7 +998,7 @@ def test_edit_authors_permissions(self):
         self.client.logout()
 
         # Try to add an author via POST - still only the secretary should be able to do this.
-        orig_authors = draft.authors()
+        orig_authors = draft.author_persons()
         post_data = self.make_edit_authors_post_data(
             basis='permission test',
             authors=draft.documentauthor_set.all(),
@@ -1016,12 +1016,12 @@ def test_edit_authors_permissions(self):
         for username in unauthorized_usernames:
             login_testing_unauthorized(self, username, url, method='post', request_kwargs=dict(data=post_data))
             draft = Document.objects.get(pk=draft.pk)
-            self.assertEqual(draft.authors(), orig_authors)  # ensure draft author list was not modified
+            self.assertEqual(draft.author_persons(), orig_authors)  # ensure draft author list was not modified
         login_testing_unauthorized(self, 'secretary', url, method='post', request_kwargs=dict(data=post_data))
         r = self.client.post(url, post_data)
         self.assertEqual(r.status_code, 302)
         draft = Document.objects.get(pk=draft.pk)
-        self.assertEqual(draft.authors(), orig_authors + [new_auth_person])
+        self.assertEqual(draft.author_persons(), orig_authors + [new_auth_person])
 
     def make_edit_authors_post_data(self, basis, authors):
         """Helper to generate edit_authors POST data for a set of authors"""
@@ -1369,8 +1369,8 @@ def test_edit_authors_edit_fields(self):
             basis=change_reason
         )
 
-        old_address = draft.authors()[0].email()
-        new_email = EmailFactory(person=draft.authors()[0], address=f'changed-{old_address}')
+        old_address = draft.author_persons()[0].email()
+        new_email = EmailFactory(person=draft.author_persons()[0], address=f'changed-{old_address}')
         post_data['author-0-email'] = new_email.address
         post_data['author-1-affiliation'] = 'University of Nowhere'
         post_data['author-2-country'] = 'Chile'
@@ -1403,17 +1403,17 @@ def test_edit_authors_edit_fields(self):
         country_event = change_events.filter(desc__icontains='changed country').first()
 
         self.assertIsNotNone(email_event)
-        self.assertIn(draft.authors()[0].name, email_event.desc)
+        self.assertIn(draft.author_persons()[0].name, email_event.desc)
         self.assertIn(before[0]['email'], email_event.desc)
         self.assertIn(after[0]['email'], email_event.desc)
 
         self.assertIsNotNone(affiliation_event)
-        self.assertIn(draft.authors()[1].name, affiliation_event.desc)
+        self.assertIn(draft.author_persons()[1].name, affiliation_event.desc)
         self.assertIn(before[1]['affiliation'], affiliation_event.desc)
         self.assertIn(after[1]['affiliation'], affiliation_event.desc)
 
         self.assertIsNotNone(country_event)
-        self.assertIn(draft.authors()[2].name, country_event.desc)
+        self.assertIn(draft.author_persons()[2].name, country_event.desc)
         self.assertIn(before[2]['country'], country_event.desc)
         self.assertIn(after[2]['country'], country_event.desc)
 

ietf/doc/tests_draft.py

@@ -140,7 +140,7 @@ def test_change_state(self):
         self.assertEqual(draft.get_state_slug("draft-iesg"), "review-e")
         self.assertTrue(not draft.tags.filter(slug="ad-f-up"))
         self.assertTrue(draft.tags.filter(slug="need-rev"))
-        self.assertCountEqual(draft.action_holders.all(), [ad] + draft.authors())
+        self.assertCountEqual(draft.action_holders.all(), [ad] + draft.author_persons())
         self.assertEqual(draft.docevent_set.count(), events_before + 3)
         self.assertTrue("Test comment" in draft.docevent_set.all()[0].desc)
         self.assertTrue("Changed action holders" in draft.docevent_set.all()[1].desc)
@@ -179,7 +179,7 @@ def test_pull_from_rfc_queue(self):
             states=[('draft-iesg','rfcqueue')],
         )
         DocEventFactory(type='started_iesg_process',by=ad,doc=draft,rev=draft.rev,desc="Started IESG Process")
-        draft.action_holders.add(*(draft.authors()))
+        draft.action_holders.add(*(draft.author_persons()))
 
         url = urlreverse('ietf.doc.views_draft.change_state', kwargs=dict(name=draft.name))
         login_testing_unauthorized(self, "secretary", url)
@@ -279,7 +279,7 @@ def test_request_last_call(self):
             states=[('draft-iesg','ad-eval')],
         )
         DocEventFactory(type='started_iesg_process',by=ad,doc=draft,rev=draft.rev,desc="Started IESG Process")
-        draft.action_holders.add(*(draft.authors()))
+        draft.action_holders.add(*(draft.author_persons()))
 
         self.client.login(username="secretary", password="secretary+password")
         url = urlreverse('ietf.doc.views_draft.change_state', kwargs=dict(name=draft.name))
@@ -1369,7 +1369,7 @@ def _test_changing_ah(action_holders, reason):
 
         _test_changing_ah([doc.ad, doc.shepherd.person], 'this is a first test')
         _test_changing_ah([doc.ad], 'this is a second test')
-        _test_changing_ah(doc.authors(), 'authors can do it, too')
+        _test_changing_ah(doc.author_persons(), 'authors can do it, too')
         _test_changing_ah([], 'clear it back out')
 
     def test_doc_change_action_holders_as_doc_manager(self):

ietf/doc/tests_review.py

@@ -822,7 +822,7 @@ def test_complete_review_upload_content(self):
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, assignment.review_request.team.list_email)
-        for author in assignment.review_request.doc.authors():
+        for author in assignment.review_request.doc.author_persons():
             self.assertContains(r, author.formatted_email())
 
         # faulty post

ietf/doc/utils.py

@@ -541,7 +541,7 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None,
             doc.action_holders.clear()
         if tags.removed("need-rev"):
             # Removed the 'need-rev' tag - drop authors from the action holders list
-            DocumentActionHolder.objects.filter(document=doc, person__in=doc.authors()).delete()
+            DocumentActionHolder.objects.filter(document=doc, person__in=doc.author_persons()).delete()
         elif tags.added("need-rev"):
             # Remove the AD if we're asking for a new revision
             DocumentActionHolder.objects.filter(document=doc, person=doc.ad).delete()
@@ -556,7 +556,7 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None,
                 doc.action_holders.add(doc.ad)
         # Authors get the action if a revision is needed
         if tags.added("need-rev"):
-            for auth in doc.authors():
+            for auth in doc.author_persons():
                 doc.action_holders.add(auth)
 
         # Now create an event if we changed the set

ietf/doc/views_doc.py

@@ -1948,9 +1948,9 @@ def edit_action_holders(request, name):
     role_ids = dict()  # maps role slug to list of Person IDs (assumed numeric in the JavaScript)
     extra_prefetch = []  # list of Person objects to prefetch for select2 field
 
-    if len(doc.authors()) > 0:
+    authors = doc.author_persons()
+    if len(authors) > 0:
         doc_role_labels.append(dict(slug='authors', label='Authors'))
-        authors = doc.authors()
         role_ids['authors'] = [p.pk for p in authors]
         extra_prefetch += authors
 

ietf/ietfauth/utils.py

@@ -287,7 +287,7 @@ def is_individual_draft_author(user, doc):
     if not hasattr(user, 'person'):
         return False
 
-    if user.person in doc.authors():
+    if user.person in doc.author_persons():
         return True
 
     return False

ietf/secr/telechat/tests.py

@@ -256,7 +256,7 @@ def test_doc_detail_post_update_state_action_holder_automation(self):
         self.assertEqual(response.status_code,302)
         draft = Document.objects.get(name=draft.name)
         self.assertEqual(draft.get_state('draft-iesg').slug,'defer')
-        self.assertCountEqual(draft.action_holders.all(), [draft.ad] + draft.authors())
+        self.assertCountEqual(draft.action_holders.all(), [draft.ad] + draft.author_persons())
         self.assertEqual(draft.docevent_set.filter(type='changed_action_holders').count(), 1)
 
         # Removing need-rev should remove authors
@@ -273,7 +273,7 @@ def test_doc_detail_post_update_state_action_holder_automation(self):
 
         # Setting to approved should remove all action holders
         # noinspection DjangoOrm
-        draft.action_holders.add(*(draft.authors()))  # add() with through model ok in Django 2.2+
+        draft.action_holders.add(*(draft.author_persons()))  # add() with through model ok in Django 2.2+
         response = self.client.post(url,{
             'submit': 'update_state',
             'state': State.objects.get(type_id='draft-iesg', slug='approved').pk,

ietf/submit/tests.py

@@ -595,7 +595,7 @@ def submit_existing(self, formats, change_authors=True, group_type='wg', stream_
         TestBlobstoreManager().emptyTestBlobstores()
 
         def _assert_authors_are_action_holders(draft, expect=True):
-            for author in draft.authors():
+            for author in draft.author_persons():
                 if expect:
                     self.assertIn(author, draft.action_holders.all())
                 else:

ietf/templates/doc/index_active_drafts.html

@@ -29,7 +29,7 @@ <h1>Active Internet-Drafts</h1>
     </p>
     {% for group in groups %}
         <h2 class="mt-3" id="id-{{ group.acronym }}">{{ group.name }} ({{ group.acronym }})</h2>
-        {% for d in group.active_drafts %}
+        {% for d in group.active_drafts %}{# n.b., d is a dict, not a Document #}
             <div class="card mb-3">
                 <div class="card-body">
                     <b>{{ d.title }}.</b>

ietf/templates/doc/review/request_info.html

@@ -74,13 +74,13 @@
                 <td>{% person_link review_req.requested_by %}</td>
             </tr>
         {% endif %}
-        {% if review_req.doc.authors %}
+        {% if review_req.doc.author_persons_or_names %}
             <tr>
                 <td></td>
                 <th scope="row">Authors</th>
                 <td>
-                    {% for author in review_req.doc.authors %}
-                        {% person_link author %}{% if not forloop.last %},{% endif %}
+                    {% for person, tp_name in review_req.doc.author_persons_or_names %}
+                        {% if person %}{% person_link person %}{% else %}<span>{{ tp_name }}</span>{% endif %}{% if not forloop.last %},{% endif %}
                     {% endfor %}
                 </td>
             </tr>

ietf/templates/group/manage_review_requests.html

@@ -66,10 +66,10 @@ <h1>
                                     <span class="badge rounded-pill text-bg-info">Auto-suggested</span>
                                     <br>
                                 {% endif %}
-                                {% if r.doc.authors %}
+                                {% if r.doc.author_persons_or_names %}
                                     <span class="fw-bold">Authors:</span>
-                                    {% for person in r.doc.authors %}
-                                        {% person_link person %}{% if not forloop.last %},{% endif %}
+                                    {% for person, tp_name in r.doc.author_persons_or_names %}
+                                        {% if person %}{% person_link person %}{% else %}<span>{{ tp_name }}</span>{% endif %}{% if not forloop.last %},{% endif %}
                                     {% endfor %}
                                     <br>
                                 {% endif %}