-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#2055] Add option to use different email tamplate for status notifications that require action #1001
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1001 +/- ##
===========================================
+ Coverage 94.75% 94.76% +0.01%
===========================================
Files 869 872 +3
Lines 30288 30559 +271
===========================================
+ Hits 28698 28958 +260
- Misses 1590 1601 +11 ☔ View full report in Codecov by Sentry. |
67abe20
to
2fd1225
Compare
@stevenbal I'm waiting for Jiro's PR for the action_required template, but the rest should work as is. |
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, just a few remarks/questions. Good that you split up handle_status_notification
into several functions, because it was getting quite complicated logic-wise
"This email is used to notify people about a new status being set on their case " | ||
"that requires action on their part." | ||
), | ||
"subject_default": "Action required", |
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.
I would probably make this Dutch, something like this maybe:
"subject_default": "Action required", | |
"subject_default": "Uw zaak is bijgewerkt op {{ site_name }} (actie vereist)", |
In case there is a specific subject in the design that we're going to use, you can ignore this
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.
Design is still WIP, so I'm going to change this as suggested.
) | ||
return | ||
if ( | ||
not oz_config.skip_notification_statustype_informeren |
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.
Does oz_config.skip_notification_statustype_informeren
have to be checked here? Since it is being checked in check_zaaktype_config
as well
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.
I think so, because oz_config.skip_notification_statustype_informeren
is not checked on its own in either function. In check_status_type
, it is part of a check that involves status_type.informeren
, and in check_zaaktype_config
it's part of a check that also involves ztc
. So I don't think it is redundant in either case.
2fd1225
to
32ba6a1
Compare
Taiga #2055
openzaak/notifications.py
TODO:
case_status_notification_action_required
template needs to be implemented (cf. this PR)