-
Notifications
You must be signed in to change notification settings - Fork 152
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
[Discussion] Use a custom signal instead of post_save for notifications #54
Comments
Sounds good, this functionality can be useful |
I disagree. Post can be updated for example by API request or other way, i think the best idea it's by checking that post.body is updated during pre_save/post_save or it's new post has been added. |
Well, if there was an API we could always fire the custom signal there, too.
It's possible to determine whether a post is new by checking "created" argument of post_save signal. But it's not possible to determine why post was changed. For example, if you have a post going into moderation, you don't want to notify users about it (yet), but you want to notify when moderator has approved the post. |
👍 to have specific signal. from django.conf import settings
from pybb import subscription, defaults
#De-activate the autosubscribe conf during importation
settings.PYBB_DEFAULT_AUTOSUBSCRIBE = False
defaults.PYBB_DEFAULT_AUTOSUBSCRIBE = False
#deactivate notification during importation
def do_nothing(*args, **kwargs):
pass
subscription.notify_topic_subscribers = do_nothing
#import process here |
post_delete has also problematic behaviour when a Post is deleted via user.delete cascade. See #162 for more informations. |
post_save signal may not be the best choice for notification, as it fires every time when the object is saved, for example when you apply data migration or import data via loaddata management command.
I think it would be better if we had our own "post_created" and "post_updated" signals, which would be fired at the end of form_valid in AddPostView/EditPostView. What do you think?
I would be willing to implement that (it's not really difficult after all), just wanted to ask for opinions first.
The text was updated successfully, but these errors were encountered: