Log in

Email notification integration

boulderdaveDave Brown
  • 5 Dec '15

Unless I'm missing something... it seems like in it's current state, it would be difficult to implement email notifications when someone has a new TopicNotification (i.e. someone has commented on a topic, etc.).

I thought I'd be able to easily hook into a signal - however - there are several locations where .updates() are called, so that isn't an option.

Am I missing something? This seems like a very basic feature that I would expect to be easily supported.

thank you for the help.
Dave

boulderdaveDave Brown
  • 5 Dec '15

Looks like I came across the start of integrating it: https://github.com/nitely/Spirit/blob/318b84c8e52b45ae36f0c6f1e4b4a875eb6a744b/spirit/user/utils/email.py#L65

Any idea when this will be integrated?

nitelyEsteban Castro Borsani
  • 5 Dec '15

The notification model should be the right place for sending email notifications.

nitelyEsteban Castro Borsani
  • 5 Dec '15

Any idea when this will be integrated?

I dunno. It's not as simple as setting up Celery, I have to add the unsubscribe thing and the user settings to receive digest emails and/or individual emails.

boulderdaveDave Brown
  • 5 Dec '15

Could be done in steps if need be... implement individual emails so at least a continued conversation can happen (who is going to continually check a forum to see if a reply has posted?)

Then perhaps the second iteration could include the ability to receive a digest.

If you would like some help, I'd be more than happy to work on a pull request for revision 1 immediately.

boulderdaveDave Brown
  • 5 Dec '15

Seems though notify_new_comment() and notify_new_mentions() are going to "clash" with each other in the case that someone replies to your Topic, and also mentions you... You wouldn't want to receive two emails in that scenario..

boulderdaveDave Brown
  • 5 Dec '15

I deployed it to my production site with the assumption that it had email support - not a problem - but I need to get it added one way or another quickly. Would be more than happy to help if you haven't started on it - feel free to share any thoughts if you want it done in a particular way (i.e. the comment about regarding mentions & comments sending out 2 emails, etc.)

boulderdaveDave Brown
  • 5 Dec '15

With the digest setting....
Would it be that you would get the latest comment for each topic you have contributed to, for that period (i.e. a week)
or is it that you would get every comment that for each topic you have contributed to, for that period?

nitelyEsteban Castro Borsani
  • 5 Dec '15

Would be more than happy to help if you haven't started on it

Would be nice, go ahead.

Seems though notify_new_comment() and notify_new_mentions() are going to "clash" with each other in the case that someone replies to your Topic, and also mentions you... You wouldn't want to receive two emails in that scenario..

Users are not notified of mentions on topics they are subscribed, so that's not a problem.

I guess I should explain how notifications currently work: the user gets notified of the first new comment to a topic she is subscribed, so if there are more new replies over time, when she look at the notifications, she will be taken to the first new comment since the last visit to that topic.

When someone mentions you on a topic you have not visited, then you will get the mention notification. When someone mentions you on a topic you have visited, then you get the new comment notification.

I would refactor the notify_new_comment() method into something like this:

# Untested, may not even work as expected

@classmethod
    def notify_new_comment(cls, comment):
        notifications = cls.objects\
            .filter(topic=comment.topic, is_active=True, is_read=True)\
            .exclude(user=comment.user)

        send_notifications(notifications, comment=comment, action=COMMENT)

        notifications.update(comment=comment, is_read=False, action=COMMENT, date=timezone.now())

Would it be that you would get the latest comment for each topic you have contributed to, for that period (i.e. a week)
or is it that you would get every comment that for each topic you have contributed to, for that period?

The unread notifications (same as what gets displayed in the header) for that period. The notification model contains all the required info.

boulderdaveDave Brown
  • 5 Dec '15

Thanks for the info @nitely - working on it now.

boulderdaveDave Brown
  • 7 Dec '15

Hi @nitely.
I've made a quick first pass to at least get something working for my site.

A few things I thought I'd mention, see what your thoughts are..
(here is the compare to reference: https://github.com/nitely/Spirit/compare/master...boulderdave:master )

I've moved your constants (i.e. ACTION_CHOICES, etc.) onto the TopicNotification model..

Ive frequently ran into issues when I'm wanting to filter this data through a reverse look up... and can't do so unless I import the constants - which sometimes may not be possible without creating a circular import.
Where as, if its on the model, you can access the_instance.ACTION_CHOICES.

I've removed "notify_new_comment" off of the TopicNotification model. Having it be a class method in this scenario I feel defeats the purpose of having a util for it... and to carry into this a bit more - I've made the new notify_new_comment() call a signal - so that if someone (like myself) would prefer to hook into this rather than having to rely on your sender() code - I can easily do so by disconnecting the existing one from spirit and plugging in my own...

Interesting in hearing your thoughts - this was quick and dirty - but I feel it is the direction I would take it.

nitelyEsteban Castro Borsani
  • 7 Dec '15

I've moved your constants (i.e. ACTION_CHOICES, etc.) onto the TopicNotification model..
Ive frequently ran into issues when I'm wanting to filter this data through a reverse look up... and can't do so unless I import the constants - which sometimes may not be possible without creating a circular import.

uhm, They are still in the same module, I don't see how doing that would avoid a circular import.

I've removed "notify_new_comment" off of the TopicNotification model. Having it be a class method in this scenario I feel defeats the purpose of having a util for it...

You should take a look at fat models, thin views, dumb templates practices.

and to carry into this a bit more - I've made the new notify_new_comment() call a signal

I've answered this before, let me quote myself:

Can you explain, please, the reason, why you removed most signals from project and moved it to utils and etc?
Signals was overhead or something else bad or what?

Hi!

Signals are hard to mantain. There is something called signal hell (kinda like the javascript callback hell), when you emit a signal you don't know who is listening, what they are doing, etc. When coding, in general, you want to be explicit, so you can re-visit the code later and know exactly what is going on.

Now I can read a view (controller in other frameworks) and I can tell what the comment_posted function is doing, I can follow all the function calls. Before, I would have to do a search over all the files to find what is connected to the comment_post signal.

Signals do have a time and a place, when you are releasing an app as open source, you want other people to be a able to create plugins and that sort of stuff, that's where signals are a good fit. They are not a good choice when you are doing things within your own app, there's no point in making everything decoupled when it'll be extremelly hard to mantain.

nitelyEsteban Castro Borsani
  • 1
  • 8 Dec '15

so that if someone (like myself) would prefer to hook into this rather than having to rely on your sender() code - I can easily do so by disconnecting the existing one from spirit and plugging in my own...

What about adding a setting for this?

CALLBACK_EMAIL_SEND = 'my_custom_spirit.email.send'

If the setting is None, then the default send() is called, otherwise the my_custom_spirit.email module is imported and the custom send() function is called.

BTW, don't iterate the notifications, think what would happend if there were 300k of them. Ultimately a Celery task should be called, I have not putted much thought on this, but probably you should pass the comment's PK to the task, the task would fetch all the unread notifications related to that comment from the DB and send the emails (so don't take my previous example as the way to go )

  • 22 Mar '16

hi! I'm also interested in this feature, have there been any progress on this?

nitelyEsteban Castro Borsani
  • 22 Mar '16

Nop. Since there has not been any progress, we might wait for Djang channels to be production ready, either that or make the task not to deepend on Celery or any other task manager (which it should not anyway).

  • 22 Mar '16

Channels looks promising. I wish I was that good in Python and the different protocols present to understand everything I read through, but based on its concept, it should make notifications fairly easy to implement.

Reply