-
Notifications
You must be signed in to change notification settings - Fork 67
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
Editors have more control over publication notifications #4094
Conversation
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.
Thanks Joe, this I think also provides a nice alternative for #3685
I believe the current implementation won't cascade the form data properly onto the events framework, so that needs to be added.
A similar refactor was done in f8edb28 and 8fc2ea1 . You can follow the "email_data"
argument to see how it is implemented.
7882b42
to
9820e96
Compare
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.
These are all very welcome changes!
I added one small comment inline.
More generally, I'm a bit scared by the fact this relatively large refactor hasn't required updating tests.
Would it be possible to add a TestCase that confirms emails are being sent to the relevant parties according to configured settings?
src/events/logic.py
Outdated
# raised when an Editor notifies an author that publication is set | ||
ON_AUTHOR_PUBLICATION = 'on_author_publication' |
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.
Even if we de-register the send_author_publication_notification
, we should properly deprecate an event like this one before deletion as others might depend on it.
I'd recommend adding such warning on events.logic.Event.register_for_event
perhaps with a set of deprecated events:
class Event():
DEPRECATED_EVENTS = {
ON_AUTHOR_PUBLICATION,
}
def register_for_event(event, *functions):
if event in self.DEPRECATED_EVENTS:
# raise DeprecationWarning
[...]
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.
Good call, but if we are trying to add a deprecation warning for this event because we think people might be using it in plugins, we need to make it work with all the supporting functions.
I've added back the full set of functions to make this old event work the way it used to, and I've added the deprecation warning in raise_event
.
That would be because I didn't run the tests 😮
Yes, for sure. |
0112df6
to
58fb5fa
Compare
This now has 7 failing tests, but I believe they are unrelated and some may be fixed in #4206. |
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.
good changes!
@@ -36,3 +38,31 @@ def update_translated_settings(apps, setting_name, group_name, values_to_replace | |||
setting_value.value = value | |||
setting_value.save() | |||
|
|||
|
|||
def update_default_setting_values(apps, setting_name, group_name, values_to_replace, replacement_value): |
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.
WWMD.
Closes #4089. Also closes #1186.
This pull request improves the usability of the prepub notification modal, as well as upgrading a few underlying utilities and forms.
initial
, a list of dicts. Without this, we'd have to modify each form in the formset based on its index value before callingsend_email
, which would be prone to bugs and not declarative enough.migration_utils
and tests it.