fix: keep draft-iesg state on expiration. Update action holders. (ietf-tools#8321)

* fix: keep draft-iesg state on expiration. Update action holders

* feat: task to repair docs in dead because expiry

* fix: restore all to-date flows through update_action_holders

* fix: Fetch the System user following more regular conventions

* fix: better signal test

Author: rjsparks

ietf/doc/expire.py

@@ -3,6 +3,8 @@
 # expiry of Internet-Drafts
 
 
+import debug    # pyflakes:ignore
+
 from django.conf import settings
 from django.utils import timezone
 
@@ -11,12 +13,12 @@
 
 from typing import List, Optional      # pyflakes:ignore
 
+from ietf.doc.utils import new_state_change_event, update_action_holders
 from ietf.utils import log
 from ietf.utils.mail import send_mail
-from ietf.doc.models import Document, DocEvent, State, IESG_SUBSTATE_TAGS
+from ietf.doc.models import Document, DocEvent, State, StateDocEvent
 from ietf.person.models import Person 
 from ietf.meeting.models import Meeting
-from ietf.doc.utils import add_state_change_event, update_action_holders
 from ietf.mailtrigger.utils import gather_address_lists
 from ietf.utils.timezone import date_today, datetime_today, DEADLINE_TZINFO
 
@@ -161,24 +163,11 @@ def expire_draft(doc):
 
     events = []
 
-    # change the state
-    if doc.latest_event(type='started_iesg_process'):
-        new_state = State.objects.get(used=True, type="draft-iesg", slug="dead")
-        prev_state = doc.get_state(new_state.type_id)
-        prev_tags = doc.tags.filter(slug__in=IESG_SUBSTATE_TAGS)
-        if new_state != prev_state:
-            doc.set_state(new_state)
-            doc.tags.remove(*prev_tags)
-            e = add_state_change_event(doc, system, prev_state, new_state, prev_tags=prev_tags, new_tags=[])
-            if e:
-                events.append(e)
-            e = update_action_holders(doc, prev_state, new_state, prev_tags=prev_tags, new_tags=[])
-            if e:
-                events.append(e)
-
     events.append(DocEvent.objects.create(doc=doc, rev=doc.rev, by=system, type="expired_document", desc="Document has expired"))
 
+    prev_draft_state=doc.get_state("draft")
     doc.set_state(State.objects.get(used=True, type="draft", slug="expired"))
+    events.append(update_action_holders(doc, prev_draft_state, doc.get_state("draft"),[],[]))
     doc.save_with_history(events)
 
 def clean_up_draft_files():
@@ -238,3 +227,42 @@ def move_file_to(subdir):
         except Document.DoesNotExist:
             # All uses of this past 2014 seem related to major system failures.
             move_file_to("unknown_ids")
+
+
+def repair_dead_on_expire():
+    by = Person.objects.get(name="(System)")
+    id_exists = State.objects.get(type="draft-iesg", slug="idexists")
+    dead = State.objects.get(type="draft-iesg", slug="dead")
+    dead_drafts = Document.objects.filter(
+        states__type="draft-iesg", states__slug="dead", type_id="draft"
+    )
+    for d in dead_drafts:
+        dead_event = d.latest_event(
+            StateDocEvent, state_type="draft-iesg", state__slug="dead"
+        )
+        if dead_event is not None:
+            if d.docevent_set.filter(type="expired_document").exists():
+                closest_expiry = min(
+                    [
+                        abs(e.time - dead_event.time)
+                        for e in d.docevent_set.filter(type="expired_document")
+                    ]
+                )
+                if closest_expiry.total_seconds() < 60:
+                    d.set_state(id_exists)
+                    events = []
+                    e = DocEvent(
+                        doc=d,
+                        rev=d.rev,
+                        type="added_comment",
+                        by=by,
+                        desc="IESG Dead state was set due only to document expiry - changing IESG state to ID-Exists",
+                    )
+                    e.skip_community_list_notification = True
+                    e.save()
+                    events.append(e)
+                    e = new_state_change_event(d, by, dead, id_exists)
+                    e.skip_community_list_notification = True
+                    e.save()
+                    events.append(e)
+                    d.save_with_history(events)

ietf/doc/tasks.py

@@ -18,6 +18,7 @@
     in_draft_expire_freeze,
     get_expired_drafts,
     expirable_drafts,
+    repair_dead_on_expire,
     send_expire_notice_for_draft,
     expire_draft,
     clean_up_draft_files,
@@ -61,6 +62,11 @@ def expire_ids_task():
         raise
 
 
+@shared_task
+def repair_dead_on_expire_task():
+    repair_dead_on_expire()
+
+
 @shared_task
 def notify_expirations_task(notify_days=14):
     for doc in get_soon_to_expire_drafts(notify_days):

ietf/doc/tests_draft.py

@@ -19,10 +19,10 @@
 
 import debug                            # pyflakes:ignore
 
-from ietf.doc.expire import get_expired_drafts, send_expire_notice_for_draft, expire_draft
-from ietf.doc.factories import EditorialDraftFactory, IndividualDraftFactory, WgDraftFactory, RgDraftFactory, DocEventFactory
+from ietf.doc.expire import expirable_drafts, get_expired_drafts, repair_dead_on_expire, send_expire_notice_for_draft, expire_draft
+from ietf.doc.factories import EditorialDraftFactory, IndividualDraftFactory, StateDocEventFactory, WgDraftFactory, RgDraftFactory, DocEventFactory
 from ietf.doc.models import ( Document, DocReminder, DocEvent,
-    ConsensusDocEvent, LastCallDocEvent, RelatedDocument, State, TelechatDocEvent, 
+    ConsensusDocEvent, LastCallDocEvent, RelatedDocument, State, StateDocEvent, TelechatDocEvent, 
     WriteupDocEvent, DocRelationshipName, IanaExpertDocEvent )
 from ietf.doc.utils import get_tags_for_stream_id, create_ballot_if_not_open
 from ietf.doc.views_draft import AdoptDraftForm
@@ -36,7 +36,7 @@
 from ietf.utils.test_utils import login_testing_unauthorized
 from ietf.utils.mail import outbox, empty_outbox, get_payload_text
 from ietf.utils.test_utils import TestCase
-from ietf.utils.timezone import date_today, datetime_from_date, DEADLINE_TZINFO
+from ietf.utils.timezone import date_today, datetime_today, datetime_from_date, DEADLINE_TZINFO
 
 
 class ChangeStateTests(TestCase):
@@ -763,13 +763,16 @@ def test_expire_drafts(self):
         txt = "%s-%s.txt" % (draft.name, draft.rev)
         self.write_draft_file(txt, 5000)
 
+        self.assertFalse(expirable_drafts(Document.objects.filter(pk=draft.pk)).exists())
+        draft.set_state(State.objects.get(used=True, type="draft-iesg", slug="idexists"))
+        self.assertTrue(expirable_drafts(Document.objects.filter(pk=draft.pk)).exists())
         expire_draft(draft)
 
         draft = Document.objects.get(name=draft.name)
         self.assertEqual(draft.get_state_slug(), "expired")
-        self.assertEqual(draft.get_state_slug("draft-iesg"), "dead")
+        self.assertEqual(draft.get_state_slug("draft-iesg"), "idexists")
         self.assertTrue(draft.latest_event(type="expired_document"))
-        self.assertCountEqual(draft.action_holders.all(), [])
+        self.assertEqual(draft.action_holders.count(), 0)
         self.assertIn('Removed all action holders', draft.latest_event(type='changed_action_holders').desc)
         self.assertTrue(not os.path.exists(os.path.join(settings.INTERNET_DRAFT_PATH, txt)))
         self.assertTrue(os.path.exists(os.path.join(settings.INTERNET_DRAFT_ARCHIVE_DIR, txt)))
@@ -842,6 +845,77 @@ def test_clean_up_draft_files(self):
         self.assertTrue(not os.path.exists(os.path.join(settings.INTERNET_DRAFT_PATH, txt)))
         self.assertTrue(os.path.exists(os.path.join(settings.INTERNET_DRAFT_ARCHIVE_DIR, txt)))
 
+    @mock.patch("ietf.community.signals.notify_of_event")
+    def test_repair_dead_on_expire(self, mock_notify):
+
+        # Create a draft in iesg idexists - ensure it doesn't get new docevents.
+        # Create a draft in iesg dead with no expires within the window - ensure it doesn't get new docevents and its state doesn't change.
+        # Create a draft in iesg dead with an expiry in the window - ensure it gets the right doc events, iesg state changes, draft state doesn't change.
+        last_year = datetime_today() - datetime.timedelta(days=365)
+
+        not_dead = WgDraftFactory(name="draft-not-dead")
+        not_dead_event_count = not_dead.docevent_set.count()
+
+        dead_not_from_expires = WgDraftFactory(name="draft-dead-not-from-expiring")
+        dead_not_from_expires.set_state(
+            State.objects.get(type="draft-iesg", slug="dead")
+        )
+        StateDocEventFactory(
+            doc=dead_not_from_expires, state=("draft-iesg", "dead"), time=last_year
+        )
+        DocEventFactory(
+            doc=dead_not_from_expires,
+            type="expired_document",
+            time=last_year + datetime.timedelta(days=1),
+        )
+        dead_not_from_expires_event_count = dead_not_from_expires.docevent_set.count()
+
+        dead_from_expires = []
+        dead_from_expires_event_count = dict()
+        for delta in [-5, 5]:
+            d = WgDraftFactory(
+                name=f"draft-dead-from-expiring-just-{'before' if delta<0 else 'after'}"
+            )
+            d.set_state(State.objects.get(type="draft-iesg", slug="dead"))
+            StateDocEventFactory(doc=d, state=("draft-iesg", "dead"), time=last_year)
+            DocEventFactory(
+                doc=d,
+                type="expired_document",
+                time=last_year + datetime.timedelta(seconds=delta),
+            )
+            dead_from_expires.append(d)
+            dead_from_expires_event_count[d] = d.docevent_set.count()
+
+        notified_during_factory_work = mock_notify.call_count
+        for call_args in mock_notify.call_args_list:
+            e = call_args.args[0]
+            self.assertTrue(isinstance(e,DocEvent))
+            self.assertFalse(hasattr(e,"skip_community_list_notification"))
+
+        repair_dead_on_expire()
+
+        self.assertEqual(not_dead.docevent_set.count(), not_dead_event_count)
+        self.assertEqual(
+            dead_not_from_expires.docevent_set.count(),
+            dead_not_from_expires_event_count,
+        )
+        for d in dead_from_expires:
+            self.assertEqual(
+                d.docevent_set.count(), dead_from_expires_event_count[d] + 2
+            )
+            self.assertIn(
+                "due only to document expiry", d.latest_event(type="added_comment").desc
+            )
+            self.assertEqual(
+                d.latest_event(StateDocEvent).desc,
+                "IESG state changed to <b>I-D Exists</b> from Dead",
+            )
+        self.assertEqual(mock_notify.call_count, 4+notified_during_factory_work)
+        for call_args in mock_notify.call_args_list[-4:]:
+            e = call_args.args[0]
+            self.assertTrue(isinstance(e,DocEvent))
+            self.assertTrue(hasattr(e,"skip_community_list_notification"))
+            self.assertTrue(e.skip_community_list_notification)
 
 class ExpireLastCallTests(TestCase):
     def test_expire_last_call(self):

ietf/doc/tests_tasks.py

@@ -1,4 +1,6 @@
 # Copyright The IETF Trust 2024, All Rights Reserved
+
+import debug    # pyflakes:ignore
 import datetime
 import mock
 
@@ -19,6 +21,7 @@
     generate_idnits2_rfcs_obsoleted_task,
     generate_idnits2_rfc_status_task,
     notify_expirations_task,
+    repair_dead_on_expire_task,
 )
 
 class TaskTests(TestCase):
@@ -96,6 +99,10 @@ def test_expire_last_calls_task(self, mock_get_expired, mock_expire):
         self.assertEqual(mock_expire.call_args_list[1], mock.call(docs[1]))
         self.assertEqual(mock_expire.call_args_list[2], mock.call(docs[2]))
 
+    @mock.patch("ietf.doc.tasks.repair_dead_on_expire")
+    def test_repair_dead_on_expire_task(self, mock_repair):
+        repair_dead_on_expire_task()
+        self.assertEqual(mock_repair.call_count, 1)
 
 class Idnits2SupportTests(TestCase):
     settings_temp_path_overrides = TestCase.settings_temp_path_overrides + ['DERIVED_DIR']

ietf/doc/utils.py

@@ -491,8 +491,9 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None,
     
     Returns an event describing the change which should be passed to doc.save_with_history()
     
-    Only cares about draft-iesg state changes. Places where other state types are updated
-    may not call this method. If you add rules for updating action holders on other state
+    Only cares about draft-iesg state changes and draft expiration. 
+    Places where other state types are updated may not call this method. 
+    If you add rules for updating action holders on other state
     types, be sure this is called in the places that change that state.
     """
     # Should not call this with different state types
@@ -511,41 +512,50 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None,
 
     # Remember original list of action holders to later check if it changed
     prev_set = list(doc.action_holders.all())
-    
-    # Update the action holders. To get this right for people with more
-    # than one relationship to the document, do removals first, then adds.
-    # Remove outdated action holders
-    iesg_state_changed = (prev_state != new_state) and (getattr(new_state, "type_id", None) == "draft-iesg") 
-    if iesg_state_changed:
-        # Clear the action_holders list on a state change. This will reset the age of any that get added back.
+
+    if new_state and new_state.type_id=="draft" and new_state.slug=="expired":
         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()
-    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()
-
-    # Add new action holders
-    if doc.ad:
-        # AD is an action holder unless specified otherwise for the new state
-        if iesg_state_changed and new_state.slug not in DocumentActionHolder.CLEAR_ACTION_HOLDERS_STATES:
-            doc.action_holders.add(doc.ad)
-        # If AD follow-up is needed, make sure they are an action holder 
-        if tags.added("ad-f-up"):
-            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():
-            doc.action_holders.add(auth)
-
-    # Now create an event if we changed the set
-    return add_action_holder_change_event(
-        doc, 
-        Person.objects.get(name='(System)'), 
-        prev_set,
-        reason='IESG state changed',
-    )
+        return add_action_holder_change_event(
+            doc, 
+            Person.objects.get(name='(System)'), 
+            prev_set,
+            reason='draft expired',
+        )
+    else:
+        # Update the action holders. To get this right for people with more
+        # than one relationship to the document, do removals first, then adds.
+        # Remove outdated action holders
+        iesg_state_changed = (prev_state != new_state) and (getattr(new_state, "type_id", None) == "draft-iesg") 
+        if iesg_state_changed:
+            # Clear the action_holders list on a state change. This will reset the age of any that get added back.
+            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()
+        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()
+
+        # Add new action holders
+        if doc.ad:
+            # AD is an action holder unless specified otherwise for the new state
+            if iesg_state_changed and new_state.slug not in DocumentActionHolder.CLEAR_ACTION_HOLDERS_STATES:
+                doc.action_holders.add(doc.ad)
+            # If AD follow-up is needed, make sure they are an action holder 
+            if tags.added("ad-f-up"):
+                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():
+                doc.action_holders.add(auth)
+
+        # Now create an event if we changed the set
+        return add_action_holder_change_event(
+            doc, 
+            Person.objects.get(name='(System)'), 
+            prev_set,
+            reason='IESG state changed',
+        )
 
 
 def update_documentauthors(doc, new_docauthors, by=None, basis=None):

ietf/doc/views_draft.py

@@ -95,7 +95,8 @@ def change_state(request, name):
     and logging the change as a comment."""
     doc = get_object_or_404(Document, name=name)
 
-    if (not doc.latest_event(type="started_iesg_process")) or doc.get_state_slug() == "expired":
+    # Steer ADs towards "Begin IESG Processing"
+    if doc.get_state_slug("draft-iesg")=="idexists" and not has_role(request.user,"Secretariat"):
         raise Http404
 
     login = request.user.person