From 0add3bbe2058c0b788c77e41561481c623b941a7 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sun, 3 Sep 2023 02:07:38 -0300 Subject: [PATCH 1/8] refactor: Add helper class to compare tag changes --- ietf/doc/utils.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 5b0e5aa8b3c..160e9592aa8 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -12,6 +12,7 @@ import textwrap from collections import defaultdict, namedtuple, Counter +from dataclasses import dataclass from typing import Union from zoneinfo import ZoneInfo @@ -460,6 +461,21 @@ def add_action_holder_change_event(doc, by, prev_set, reason=None): ) +@dataclass +class TagSetComparer: + before: set[str] + after: set[str] + + def changed(self): + return self.before != self.after + + def added(self, tag): + return tag in self.after and tag not in self.before + + def removed(self, tag): + return tag in self.before and tag not in self.after + + def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, new_tags=None): """Update the action holders for doc based on state transition @@ -473,12 +489,14 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, if prev_state and new_state: assert prev_state.type_id == new_state.type_id - # Convert tags to sets of slugs - prev_tag_slugs = {t.slug for t in (prev_tags or [])} - new_tag_slugs = {t.slug for t in (new_tags or [])} + # Convert tags to sets of slugs + tags = TagSetComparer( + before={t.slug for t in (prev_tags or [])}, + after={t.slug for t in (new_tags or [])}, + ) # Do nothing if state / tag have not changed - if (prev_state == new_state) and (prev_tag_slugs == new_tag_slugs): + if (prev_state == new_state) and not tags.changed(): return None # Remember original list of action holders to later check if it changed @@ -491,12 +509,12 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, # Default to responsible AD for states other than these doc.action_holders.add(doc.ad) - if prev_tag_slugs != new_tag_slugs: + if tags.changed(): # If we have added or removed the need-rev tag, add or remove authors as action holders - if ('need-rev' in prev_tag_slugs) and ('need-rev' not in new_tag_slugs): + 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 ('need-rev' not in prev_tag_slugs) and ('need-rev' in new_tag_slugs): + elif tags.added("need-rev"): # Added the 'need-rev' tag - add authors to the action holders list for auth in doc.authors(): if not doc.action_holders.filter(pk=auth.pk).exists(): From f1a5fa29ff130e6f289f44b2e38a77c9fa459428 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sun, 3 Sep 2023 02:09:07 -0300 Subject: [PATCH 2/8] feat: Give AD the action in ad-f-up state --- ietf/doc/utils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 160e9592aa8..39b4f43261d 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -519,6 +519,10 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, for auth in doc.authors(): if not doc.action_holders.filter(pk=auth.pk).exists(): doc.action_holders.add(auth) + + # If AD follow-up is needed, make sure they are an action holder + if tags.added("ad-f-up") and doc.ad: + doc.action_holders.add(doc.ad) # Now create an event if we changed the set return add_action_holder_change_event( From 1ee3f73498a34ff749522cb734e4f43014c09a8d Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sun, 3 Sep 2023 02:09:36 -0300 Subject: [PATCH 3/8] refactor: Remove unnecessary check --- ietf/doc/utils.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 39b4f43261d..98875ad2468 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -509,20 +509,19 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, # Default to responsible AD for states other than these doc.action_holders.add(doc.ad) - if tags.changed(): - # If we have added or removed the need-rev tag, add or remove authors as action holders - 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"): - # Added the 'need-rev' tag - add authors to the action holders list - for auth in doc.authors(): - if not doc.action_holders.filter(pk=auth.pk).exists(): - doc.action_holders.add(auth) - - # If AD follow-up is needed, make sure they are an action holder - if tags.added("ad-f-up") and doc.ad: - doc.action_holders.add(doc.ad) + # If we have added or removed the need-rev tag, add or remove authors as action holders + 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"): + # Added the 'need-rev' tag - add authors to the action holders list + for auth in doc.authors(): + if not doc.action_holders.filter(pk=auth.pk).exists(): + doc.action_holders.add(auth) + + # If AD follow-up is needed, make sure they are an action holder + if tags.added("ad-f-up") and doc.ad: + doc.action_holders.add(doc.ad) # Now create an event if we changed the set return add_action_holder_change_event( From bed9c3fa1ad4b4e8c1975397e3913dd2cafeea04 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 5 Sep 2023 11:07:37 -0300 Subject: [PATCH 4/8] refactor: Reorganize update_action_holders() --- ietf/doc/utils.py | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 98875ad2468..d5826edccdf 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -501,27 +501,30 @@ 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()) - # Only draft-iesg states are of interest (for now) - if (prev_state != new_state) and (getattr(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 doc.ad and new_state.slug not in DocumentActionHolder.CLEAR_ACTION_HOLDERS_STATES: - # Default to responsible AD for states other than these - doc.action_holders.add(doc.ad) - - # If we have added or removed the need-rev tag, add or remove authors as action holders 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"): - # Added the 'need-rev' tag - add authors to the action holders list + + # 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(): - if not doc.action_holders.filter(pk=auth.pk).exists(): - doc.action_holders.add(auth) - - # If AD follow-up is needed, make sure they are an action holder - if tags.added("ad-f-up") and doc.ad: - doc.action_holders.add(doc.ad) + doc.action_holders.add(auth) # Now create an event if we changed the set return add_action_holder_change_event( From 1013740887be46da140e7508ece04afa6e77cf21 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 5 Sep 2023 11:11:46 -0300 Subject: [PATCH 5/8] refactor: Remove unnecessary guard --- ietf/doc/utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index d5826edccdf..992659df3da 100644 --- a/ietf/doc/utils.py +++ b/ietf/doc/utils.py @@ -512,6 +512,9 @@ def update_action_holders(doc, prev_state=None, new_state=None, prev_tags=None, 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: From 60078cbb396b95019c6271f0c61c2b8f3db56957 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 5 Sep 2023 11:55:37 -0300 Subject: [PATCH 6/8] test: Update test for new AD handling --- ietf/doc/tests_utils.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/ietf/doc/tests_utils.py b/ietf/doc/tests_utils.py index be1f4a924d2..d56f868bf77 100644 --- a/ietf/doc/tests_utils.py +++ b/ietf/doc/tests_utils.py @@ -154,10 +154,24 @@ def test_update_action_holders_resets_age(self): self.assertGreaterEqual(doc.documentactionholder_set.get(person=self.ad).time_added, right_now) def test_update_action_holders_add_tag_need_rev(self): - """Adding need-rev tag adds authors as action holders""" + """Adding need-rev tag drops AD and adds authors as action holders""" doc = self.doc_in_iesg_state('pub-req') first_author = self.authors[0] doc.action_holders.add(first_author) + doc.action_holders.add(doc.ad) + self.assertCountEqual(doc.action_holders.all(), [first_author, doc.ad]) + self.update_doc_state(doc, + doc.get_state('draft-iesg'), + add_tags=['need-rev'], + remove_tags=None) + self.assertCountEqual(doc.action_holders.all(), self.authors) + self.assertNotIn(self.ad, doc.action_holders.all()) + + # Check case where an author is ad + doc = self.doc_in_iesg_state('pub-req') + doc.ad = first_author + doc.save() + doc.action_holders.add(first_author) self.assertCountEqual(doc.action_holders.all(), [first_author]) self.update_doc_state(doc, doc.get_state('draft-iesg'), From 61e1adc19557925a70c313347cdee3f8529bdcfd Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 5 Sep 2023 11:59:42 -0300 Subject: [PATCH 7/8] test: Update another test --- ietf/doc/tests_utils.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ietf/doc/tests_utils.py b/ietf/doc/tests_utils.py index d56f868bf77..1f3ae26d5ed 100644 --- a/ietf/doc/tests_utils.py +++ b/ietf/doc/tests_utils.py @@ -203,13 +203,14 @@ def test_update_action_holders_remove_tag_need_rev(self): def test_update_action_holders_add_tag_need_rev_ignores_non_authors(self): """Adding need-rev tag does not affect existing action holders""" doc = self.doc_in_iesg_state('pub-req') - doc.action_holders.add(self.ad) - self.assertCountEqual(doc.action_holders.all(),[self.ad]) + other_person = PersonFactory() + doc.action_holders.add(other_person) + self.assertCountEqual(doc.action_holders.all(),[other_person]) self.update_doc_state(doc, doc.get_state('draft-iesg'), add_tags=['need-rev'], remove_tags=None) - self.assertCountEqual(doc.action_holders.all(), [self.ad] + self.authors) + self.assertCountEqual(doc.action_holders.all(), [other_person] + self.authors) def test_update_action_holders_remove_tag_need_rev_ignores_non_authors(self): """Removing need-rev tag does not affect non-author action holders""" From c64e2b89d1c3064bd9df797f241514f5d94735c5 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sat, 9 Sep 2023 01:45:26 -0300 Subject: [PATCH 8/8] test: Test ad-f-up effect on action holders --- ietf/doc/tests_utils.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ietf/doc/tests_utils.py b/ietf/doc/tests_utils.py index 1f3ae26d5ed..e104b9ee516 100644 --- a/ietf/doc/tests_utils.py +++ b/ietf/doc/tests_utils.py @@ -189,6 +189,12 @@ def test_update_action_holders_add_tag_need_rev_no_dups(self): remove_tags=None) self.assertCountEqual(doc.action_holders.all(), self.authors) + def test_update_action_holders_add_tag_ad_f_up(self): + doc = self.doc_in_iesg_state('pub-req') + self.assertEqual(doc.action_holders.count(), 0) + self.update_doc_state(doc, doc.get_state('draft-iesg'), add_tags=['ad-f-up']) + self.assertCountEqual(doc.action_holders.all(), [self.ad]) + def test_update_action_holders_remove_tag_need_rev(self): """Removing need-rev tag drops authors as action holders""" doc = self.doc_in_iesg_state('pub-req')