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
Convert subscriber notification signal into regular function call from the view #195
Conversation
…views. The previous method of using the post_save signal to notify subscribers meant that the function was called without the request available, creating an undocumented dependency on the django.contrib.sites app. Moving the method call into the view enables the use of the get_current_site shortcut which removes the dependency. It has the added benefit of ensuring that subscribers are only notified when a post is successfully added or edited in a thread, and not in any of the other possible scenarios where the post_save signal is sent.
Hi @skolsuper I think signals are better because third party apps can remove existing listeners and/or add new listeners to have custom notification process for exemple. But you right : What do you think about it ? |
Hmm, food for thought definitely. I have added a method call to This is firmly in the realm of opinion, but I'm not a big fan of using signals and slots in python, I don't like the possibility of action occurring at a distance, without appearing in the code that triggers it. However, using a custom signal means it will always be explicitly thrown, and we can include the request in the kwargs, so it solves the dependency issue too. That would also mean it could be an override in the settings, which is easier for non-technical users to change. On the subject of multiple notifications, in my experience as a user of other forums, once I get a notification that a thread has been updated, I won't get another until I have checked the thread. I think for a busy forum it will be essential to have this functionality in pybb. Maybe it is better to make another pull request for that though. |
…isable notifications for edits. Imho, notifications for post edits will be confusing to users the majority of the time, when they visit the thread and see no new posts. The fraction of the time where they understand what has happened, the edit will be of no material value (typo fix, spelling/grammar) a lot of the time too.
@GeyseR I would appreciate your thoughts on this PR. A summary of the changes:
@DylannCordel your thoughts would be appreciated too. |
@@ -113,6 +113,7 @@ def getsetting_with_deprecation_check(all_settings, setting_name): | |||
|
|||
PYBB_DISABLE_SUBSCRIPTIONS = getattr(settings, 'PYBB_DISABLE_SUBSCRIPTIONS', False) | |||
PYBB_DISABLE_NOTIFICATIONS = getattr(settings, 'PYBB_DISABLE_NOTIFICATIONS', False) | |||
PYBB_NOTIFY_ON_EDIT = getattr(settings, 'PYBB_NOTIFY_ON_EDIT', False) |
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.
You should have True
as default value to stay fully backward compatible as in current version we are notified each time a post is updated.
Hi @skolsuper Sorry for this tardive answer. I add some comments on your code. About the Else, GJ ;-) |
…. Move notifications signal dispatch into form_valid method to allow for overriding post method in view. Change default value for PYBB_NOTIFY_ON_EDIT to True
Hi @DylannCordel, thanks for the feedback, I have incorporated most if not all of it 👍 |
Security patch re: issue hovel#200
…views. The previous method of using the post_save signal to notify subscribers meant that the function was called without the request available, creating an undocumented dependency on the django.contrib.sites app. Moving the method call into the view enables the use of the get_current_site shortcut which removes the dependency. It has the added benefit of ensuring that subscribers are only notified when a post is successfully added or edited in a thread, and not in any of the other possible scenarios where the post_save signal is sent.
…isable notifications for edits. Imho, notifications for post edits will be confusing to users the majority of the time, when they visit the thread and see no new posts. The fraction of the time where they understand what has happened, the edit will be of no material value (typo fix, spelling/grammar) a lot of the time too.
…. Move notifications signal dispatch into form_valid method to allow for overriding post method in view. Change default value for PYBB_NOTIFY_ON_EDIT to True
…to subscriptions-cleanup
The previous method of using the
post_save
signal to callnotify_topic_subscribers
meant that the function was called without the request available, creating an undocumented dependency on thedjango.contrib.sites
app.Moving the
notify_topic_subscribers
call into the view enables the use of theget_current_site
shortcut which removes the dependency. It has the added benefit of ensuring that subscribers are only notified when a post is successfully added or edited in a thread, and not in any of the other possible scenarios where thepost_save
signal is sent.