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>
This commit is contained in:
Jennifer Richards
GitHub
parent
18a1af22f8
commit
8bc4507def
|
|
@ -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"""
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Reference in a new issue