feat: Give AD the action in ad-f-up doc state - #6272
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6272 +/- ##
==========================================
- Coverage 88.69% 88.69% -0.01%
==========================================
Files 290 290
Lines 40390 40404 +14
==========================================
+ Hits 35825 35836 +11
- Misses 4565 4568 +3
|
| 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"): |
There was a problem hiding this comment.
Only a few seconds thought given to it, but I'm not seeing a case where this tag being added shouldn't relieve the AD of being an action holder. (In case that's too many nested nots, I think the ad should be removed as an action holder when this tag is added.
@larseggert - am I missing any use cases that say the AD should remain as an action holder when this tag is added?
There was a problem hiding this comment.
Sounds reasonable to me - I'll add
There was a problem hiding this comment.
When which tag is added?
There was a problem hiding this comment.
the "need-rev" (identified as the "Revised I-D needed" in the UI) tag
There was a problem hiding this comment.
When that is added, IMO the AD should never remain an action holder.
No objection. |
| # 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: |
There was a problem hiding this comment.
I think this needs a test around it proving that #6266 has been addressed?
There was a problem hiding this comment.
You're right, thanks for catching that. I've added a test.
3 participants
Submitting as a draft PR because I have not updated any tests, but would like feedback on the basic premise here.
Python-wise, I'm using a dataclass, a Python feature we have not used much if at all yet. It has been around since 3.7 and cuts down on boilerplate for a use like this.
In terms of the functional change, this adds the AD to the action holders when the
ad-f-upstate is added. It does not remove them when that tag is removed. It could if this is going to be needed, but will take some care because it will interact more strongly with action holder updates on other state changes. I.e., it may not be as easy to consider the substate tags separately from the states when deciding who the next action holder are.Fixes #6266