From 8bc4507defaec744f6196a242055685bda575113 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 12 Sep 2023 11:44:33 -0300 Subject: [PATCH] feat: Give AD the action in ad-f-up doc state (#6272) * refactor: Add helper class to compare tag changes * feat: Give AD the action in ad-f-up state * refactor: Remove unnecessary check * refactor: Reorganize update_action_holders() * refactor: Remove unnecessary guard * test: Update test for new AD handling * test: Update another test * test: Test ad-f-up effect on action holders --------- Co-authored-by: Robert Sparks --- ietf/doc/tests_utils.py | 29 +++++++++++++++--- ietf/doc/utils.py | 65 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/ietf/doc/tests_utils.py b/ietf/doc/tests_utils.py index be1f4a924d..e104b9ee51 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'), @@ -175,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') @@ -189,13 +209,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""" diff --git a/ietf/doc/utils.py b/ietf/doc/utils.py index 5b0e5aa8b3..992659df3d 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,34 +489,45 @@ 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 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 + 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 prev_tag_slugs != new_tag_slugs: - # 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): - # 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): - # 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"): + 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(