Convert subscriber notification signal into regular function call from the view

[skolsuper]

The previous method of using the post_save signal to call notify_topic_subscribers meant that the function was called without the request available, creating an undocumented dependency on the django.contrib.sites app.

Moving the notify_topic_subscribers 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.

[DylannCordel]

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. Furthermore, if you move this in a view, we won't be able to use api to add / edit posts (in tests, in some specific commands etc.).

But you right : post_save is not the good signal. We should have a custom signal like "post_content_updated" which must be triggered only when a post has its content really updated. We could test the old body html version and the new one to know if updates are enough to notify again subscribers. For exemple, if I post with a typo, then edit to correct the typo : we should not send 2 notifications to users because the update is not meanful.

What do you think about it ?

[skolsuper]

Hmm, food for thought definitely. I have added a method call to notify_subscribers so that people can subclass the view and override it.

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.

[skolsuper]

@GeyseR I would appreciate your thoughts on this PR. A summary of the changes:

• Creates a custom signal topic_updated sent by AddPostView and EditPostView.
• It does nothing if notifications are disabled.
• It isn't sent by EditPostView unless user sets PYBB_NOTIFY_ON_EDIT = True
• It passes the request with the signal, removing an undocumented dependency of pybbm on the django.contrib.sites app
• I also added an ImproperlyConfigured exception if PYBB_USE_DJANGO_MAILER is True but django-mailer is not installed

@DylannCordel your thoughts would be appreciated too.

[DylannCordel]

Hi @skolsuper

Sorry for this tardive answer. I add some comments on your code. About the topic_updated signal wich requires the request, I continue to think this is not a good idea because you won't be able to use this signal "out of a view", for batch processing for exemple.

Else, GJ ;-)

[skolsuper]

Hi @DylannCordel, thanks for the feedback, I have incorporated most if not all of it :+1: