From 8bc4507defaec744f6196a242055685bda575113 Mon Sep 17 00:00:00 2001
From: Jennifer Richards <jennifer@staff.ietf.org>
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 <rjsparks@nostrum.com>
---
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 be1f4a924..e104b9ee5 100644
--- a/ietf/doc/tests_utils.py
+++ b/ietf/doc/tests_utils.py
@@ -154,10 +154,24 @@ class ActionHoldersTests(TestCase):
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 @@ class ActionHoldersTests(TestCase):
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 @@ class ActionHoldersTests(TestCase):
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 5b0e5aa8b..992659df3 100644
--- a/ietf/doc/utils.py
+++ b/ietf/doc/utils.py
@@ -12,6 +12,7 @@ import re
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(