-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: keep draft-iesg state on expiration. Update action holders. #8321
Conversation
Keeping at draft as #8212 fix isn't complete and to investigate if we can remediate docs recently moved to dead just because of expiry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change suggested (getting the system Person by name instead of pk, though if you want to start a change in our practice that's fine).
More significantly, it looks to me like you might be trying to check that notification emails aren't being sent in a way that I don't think tests that. This might be my misunderstanding. If you are trying to check that, I think you need to test that the skip_community_list_notification
flag is set on the event you save. We already have a test that proves the flag works as intended. However, getting the event instance might be a bit involved.
ietf/doc/tests_draft.py
Outdated
d.latest_event(StateDocEvent).desc, | ||
"IESG state changed to <b>I-D Exists</b> from Dead", | ||
) | ||
self.assertEqual(len(outbox), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is meant to test that community notifications aren't sent, additional work will be needed because the signal handler doesn't actually send messages when running tests. (I'm not that's what you're checking)
ietf/doc/expire.py
Outdated
|
||
|
||
def repair_dead_on_expire(): | ||
by = Person.objects.get(pk=1) # a.k.a. "(System)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We canonically use Person.objects.get(user__username="(System)")
. Better to stick to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it's mostly (name="(System)")
rather than user__username
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er, yes, name
. Sorry, I don't know why I thought I saw that.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8321 +/- ##
=======================================
Coverage 88.72% 88.73%
=======================================
Files 310 312 +2
Lines 40882 40910 +28
=======================================
+ Hits 36273 36301 +28
Misses 4609 4609 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
An alternative to counting events and manipulating indexes would be to call mock_notify.reset_mock()
before calling your method under test. That'll empty call_args
and reset called
to False. Purely optional though.
empty_outbox() | ||
notified_during_factory_work = mock_notify.call_count | ||
for call_args in mock_notify.call_args_list: | ||
e = call_args.args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! I had missed that call_args
now has args
and kwargs
properties! That is so much better than call_args[0][0]
.
Fixes #5560 and #8212