From b4451194288ec8a8d1359331a1d98d7d684d1530 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Wed, 3 Jan 2024 17:30:06 -0500 Subject: [PATCH] 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 | 36 ++++++++++++++++++++++++++++++++++++ ietf/doc/views_draft.py | 34 ++++++++++------------------------ 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/ietf/doc/tests_draft.py b/ietf/doc/tests_draft.py index e168f685f9..045fdc7f46 100644 --- a/ietf/doc/tests_draft.py +++ b/ietf/doc/tests_draft.py @@ -1483,6 +1483,42 @@ def test_confirm_submission(self): 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): diff --git a/ietf/doc/views_draft.py b/ietf/doc/views_draft.py index 4f6659af9f..ea30e7bd2d 100644 --- a/ietf/doc/views_draft.py +++ b/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 %s" % 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 {target_state['iesg'].name}")) + + # 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)