feat: Give AD the action in ad-f-up doc state (ietf-tools#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 <rjsparks@nostrum.com>

Author: jennifer-richards

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"""

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(