From 141f86e5f061478dfcb9b3ea6c6391b5f2252b0c Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Tue, 10 Dec 2024 12:34:42 -0600 Subject: [PATCH 1/5] fix: keep draft-iesg state on expiration. Update action holders --- ietf/doc/expire.py | 21 ++--------- ietf/doc/tests_draft.py | 9 +++-- ietf/doc/utils.py | 84 +++++++++++++++++++++++------------------ 3 files changed, 58 insertions(+), 56 deletions(-) diff --git a/ietf/doc/expire.py b/ietf/doc/expire.py index 681ca963a3..ce550dd244 100644 --- a/ietf/doc/expire.py +++ b/ietf/doc/expire.py @@ -11,12 +11,12 @@ from typing import List, Optional # pyflakes:ignore +from ietf.doc.utils import 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 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 +161,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(): diff --git a/ietf/doc/tests_draft.py b/ietf/doc/tests_draft.py index 09a7b38999..2405806682 100644 --- a/ietf/doc/tests_draft.py +++ b/ietf/doc/tests_draft.py @@ -19,7 +19,7 @@ import debug # pyflakes:ignore -from ietf.doc.expire import get_expired_drafts, send_expire_notice_for_draft, expire_draft +from ietf.doc.expire import expirable_drafts, get_expired_drafts, send_expire_notice_for_draft, expire_draft from ietf.doc.factories import EditorialDraftFactory, IndividualDraftFactory, WgDraftFactory, RgDraftFactory, DocEventFactory from ietf.doc.models import ( Document, DocReminder, DocEvent, ConsensusDocEvent, LastCallDocEvent, RelatedDocument, State, TelechatDocEvent, @@ -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))) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index b2bc0997b1..ac9fd87849 100644 --- a/ietf/doc/utils.py +++ b/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,52 @@ 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.type_id=="draft-iesg": + # 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', + ) + elif 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: + return None def update_documentauthors(doc, new_docauthors, by=None, basis=None): From 9e8132b960eacfe6bfa0c624c9999cf59e9114a2 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Wed, 11 Dec 2024 16:45:19 -0600 Subject: [PATCH 2/5] feat: task to repair docs in dead because expiry --- ietf/doc/expire.py | 45 ++++++++++++++++++++++++-- ietf/doc/tasks.py | 6 ++++ ietf/doc/tests_draft.py | 71 ++++++++++++++++++++++++++++++++++++++--- ietf/doc/tests_tasks.py | 7 ++++ ietf/doc/views_draft.py | 3 +- 5 files changed, 125 insertions(+), 7 deletions(-) diff --git a/ietf/doc/expire.py b/ietf/doc/expire.py index ce550dd244..a6c93e0cbe 100644 --- a/ietf/doc/expire.py +++ b/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,10 +13,10 @@ from typing import List, Optional # pyflakes:ignore -from ietf.doc.utils import update_action_holders +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 +from ietf.doc.models import Document, DocEvent, State, StateDocEvent from ietf.person.models import Person from ietf.meeting.models import Meeting from ietf.mailtrigger.utils import gather_address_lists @@ -225,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(pk=1) # a.k.a. "(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) diff --git a/ietf/doc/tasks.py b/ietf/doc/tasks.py index f1de459dd8..b7f89e1f9b 100644 --- a/ietf/doc/tasks.py +++ b/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): diff --git a/ietf/doc/tests_draft.py b/ietf/doc/tests_draft.py index 2405806682..ab035316af 100644 --- a/ietf/doc/tests_draft.py +++ b/ietf/doc/tests_draft.py @@ -19,10 +19,10 @@ import debug # pyflakes:ignore -from ietf.doc.expire import expirable_drafts, 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): @@ -846,6 +846,69 @@ def test_clean_up_draft_files(self): self.assertTrue(os.path.exists(os.path.join(settings.INTERNET_DRAFT_ARCHIVE_DIR, txt))) + def test_repair_dead_on_expire(self): + + # 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() + + empty_outbox() + + 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 I-D Exists from Dead", + ) + self.assertEqual(len(outbox), 0) + + class ExpireLastCallTests(TestCase): def test_expire_last_call(self): from ietf.doc.lastcall import get_expired_last_calls, expire_last_call diff --git a/ietf/doc/tests_tasks.py b/ietf/doc/tests_tasks.py index b75f58656b..135b52f604 100644 --- a/ietf/doc/tests_tasks.py +++ b/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'] diff --git a/ietf/doc/views_draft.py b/ietf/doc/views_draft.py index d709aedd42..34104b2005 100644 --- a/ietf/doc/views_draft.py +++ b/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 From 0a8c7993a23e9ad57ec727e3b2353fe414d55ac3 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Wed, 11 Dec 2024 17:39:28 -0600 Subject: [PATCH 3/5] fix: restore all to-date flows through update_action_holders --- ietf/doc/utils.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index ac9fd87849..ff19dfbde6 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -513,7 +513,15 @@ 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()) - if new_state.type_id=="draft-iesg": + if new_state and new_state.type_id=="draft" and new_state.slug=="expired": + doc.action_holders.clear() + 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 @@ -548,16 +556,6 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, prev_set, reason='IESG state changed', ) - elif new_state.type_id=="draft" and new_state.slug=="expired": - doc.action_holders.clear() - return add_action_holder_change_event( - doc, - Person.objects.get(name='(System)'), - prev_set, - reason='draft expired', - ) - else: - return None def update_documentauthors(doc, new_docauthors, by=None, basis=None): From afe9a022ebcfdca81a93d08889c3140c72f48261 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 12 Dec 2024 12:20:59 -0600 Subject: [PATCH 4/5] fix: Fetch the System user following more regular conventions --- ietf/doc/expire.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/doc/expire.py b/ietf/doc/expire.py index a6c93e0cbe..0581c9a03f 100644 --- a/ietf/doc/expire.py +++ b/ietf/doc/expire.py @@ -230,7 +230,7 @@ def move_file_to(subdir): def repair_dead_on_expire(): - by = Person.objects.get(pk=1) # a.k.a. "(System)" + 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( From cb435b69efd686199fa3b3133a74aa07c0882704 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 12 Dec 2024 17:35:14 -0600 Subject: [PATCH 5/5] fix: better signal test --- ietf/doc/tests_draft.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/ietf/doc/tests_draft.py b/ietf/doc/tests_draft.py index ab035316af..84959625c9 100644 --- a/ietf/doc/tests_draft.py +++ b/ietf/doc/tests_draft.py @@ -845,8 +845,8 @@ 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))) - - def test_repair_dead_on_expire(self): + @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. @@ -886,7 +886,11 @@ def test_repair_dead_on_expire(self): dead_from_expires.append(d) dead_from_expires_event_count[d] = d.docevent_set.count() - empty_outbox() + 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() @@ -906,8 +910,12 @@ def test_repair_dead_on_expire(self): d.latest_event(StateDocEvent).desc, "IESG state changed to I-D Exists from Dead", ) - self.assertEqual(len(outbox), 0) - + 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):