fix: Properly set AD as action holder when submitting to IESG for publication (#6854)

* fix: Properly set AD as action holder when submitting to IESG for publication (#5227)

The clear intent of `to_iesg` is that
a) the document AD should be the group AD, if not already set, and
b) the document Action Holder should be the document AD;
but there was an order-of-operation error,
such that the Action Holder remained empty.

* refactor: Don't take values out of the parent scope, don't insert values into the parent scope

* refactor: Streamline DocEvent creation

ietf/doc/tests_draft.py

@@ -1483,6 +1483,42 @@ class SubmitToIesgTests(TestCase):
         self.assertTrue("aread@" in outbox[-1]['To'])
         self.assertTrue("iesg-secretary@" in outbox[-1]['Cc'])
 
+    def test_confirm_submission_no_doc_ad(self):
+        url = urlreverse('ietf.doc.views_draft.to_iesg', kwargs=dict(name=self.docname))
+        self.client.login(username="marschairman", password="marschairman+password")
+
+        doc = Document.objects.get(name=self.docname)
+        RoleFactory(name_id='ad', group=doc.group, person=doc.ad)
+        e = DocEvent(type="changed_document", by=doc.ad, doc=doc, rev=doc.rev, desc="Remove doc AD")
+        e.save()
+        doc.ad = None
+        doc.save_with_history([e])
+
+        docevents_pre = set(doc.docevent_set.all())
+        mailbox_before = len(outbox)
+
+        r = self.client.post(url, dict(confirm="1"))
+        self.assertEqual(r.status_code, 302)
+
+        doc = Document.objects.get(name=self.docname)
+        self.assertTrue(doc.get_state('draft-iesg').slug=='pub-req')
+        self.assertTrue(doc.get_state('draft-stream-ietf').slug=='sub-pub')
+
+        self.assertCountEqual(doc.action_holders.all(), [doc.ad])
+
+        new_docevents = set(doc.docevent_set.all()) - docevents_pre
+        self.assertEqual(len(new_docevents), 5)
+        new_docevent_type_count = Counter([e.type for e in new_docevents])
+        self.assertEqual(new_docevent_type_count['changed_state'],2)
+        self.assertEqual(new_docevent_type_count['started_iesg_process'],1)
+        self.assertEqual(new_docevent_type_count['changed_action_holders'], 1)
+        self.assertEqual(new_docevent_type_count['changed_document'], 1)
+
+        self.assertEqual(len(outbox), mailbox_before + 1)
+        self.assertTrue("Publication has been requested" in outbox[-1]['Subject'])
+        self.assertTrue("aread@" in outbox[-1]['To'])
+        self.assertTrue("iesg-secretary@" in outbox[-1]['Cc'])
+
 
 
 class RequestPublicationTests(TestCase):

ietf/doc/views_draft.py

@@ -560,22 +560,19 @@ def to_iesg(request,name):
     if request.method == 'POST':
 
         if request.POST.get("confirm", ""): 
-
             by = request.user.person
 
             events = []
-
-            changes = []
+            def doc_event(type, by, doc, desc):
+                return DocEvent.objects.create(type=type, by=by, doc=doc, rev=doc.rev, desc=desc)
 
             if doc.get_state_slug("draft-iesg") == "idexists":
-                e = DocEvent()
-                e.type = "started_iesg_process"
-                e.by = by
-                e.doc = doc
-                e.rev = doc.rev
-                e.desc = "Document is now in IESG state <b>%s</b>" % target_state['iesg'].name
-                e.save()
-                events.append(e)
+                events.append(doc_event("started_iesg_process", by, doc, f"Document is now in IESG state <b>{target_state['iesg'].name}</b>"))
+
+            # do this first, so AD becomes action holder
+            if not doc.ad == ad :
+                doc.ad = ad
+                events.append(doc_event("changed_document", by, doc, f"Responsible AD changed to {doc.ad}"))
 
             for state_type in ['draft-iesg','draft-stream-ietf']:
                 prev_state=doc.get_state(state_type)
@@ -587,25 +584,14 @@ def to_iesg(request,name):
                         events.append(e)
                     events.append(add_state_change_event(doc=doc,by=by,prev_state=prev_state,new_state=new_state))
 
-            if not doc.ad == ad :
-                doc.ad = ad
-                changes.append("Responsible AD changed to %s" % doc.ad)
-
             if not doc.notify == notify :
                 doc.notify = notify
-                changes.append("State Change Notice email list changed to %s" % doc.notify)
+                events.append(doc_event("changed_document", by, doc, f"State Change Notice email list changed to {doc.notify}"))
 
             # Get the last available writeup
             previous_writeup = doc.latest_event(WriteupDocEvent,type="changed_protocol_writeup")
             if previous_writeup != None:
-                changes.append(previous_writeup.text)
-
-            for c in changes:
-                e = DocEvent(doc=doc, rev=doc.rev, by=by)
-                e.desc = c
-                e.type = "changed_document"
-                e.save()
-                events.append(e)
+                events.append(doc_event("changed_document", by, doc, previous_writeup.text))
 
             doc.save_with_history(events)