[ckan-dev] RFC: Activity streams on-site and email notifications

[Sean Hammond]

I've updated the spec to reflect our latest discussions:

https://github.com/okfn/ckan/wiki/Spec:-Activity-Stream-Notifications

The on-site notifications part is almost implemented. Just starting on
the email notifications implementation. The spec has the user stories,
open questions, and implementation analysis.

One thing I've been thinking about is how to handle failures when trying
to send an email? On the one hand, if sending an email is failing e.g.
because the user's email address is invalid, when she fixes her email
address we don't want her to then receive a barrage of old unsent
emails. On the other hand, if sending an email fails for some temporary
reason we don't want to give up right away, we should try a number of
times to send an email before giving up and discarding it.

If we're going to try to resend emails up to a maximum number of times,
then the question is when do we do it? Do we try again immediately? Or
do we store the failed emails and try to send them again the next time
the email notifier job runs (which implies an unsent_email_notifications
table in the db even if we are using the pull model)? Or both?

The simplest implementation (but probably not the most effective at
making sure users get emails) is probably to try again immediately up to
a maximum number of times and then give up.

Another thing I've been thinking about is how quickly emails get sent.
With the pull model, emails are sent only as quickly as the email
notifier job runs, e.g. once an hour, or once a day. You don't get an
email about an activity straight away as soon as that activity happens.
That's fine for activity notifications as I don't think they need to be
sent urgently and we want to group multiple activities together into one
email to avoid sending too many emails anyway. But might there be some
other cases where we would want to send a notification email to a user
promptly? Maybe there also needs to be direct access to the
send_notification_email() function for that reason, even though activity
streams notifications themselves would not make use of that.


On Thu, Nov 01, 2012 at 03:16:43PM +0100, Sean Hammond wrote:
> > When we go for the pull model, I would suggest that we use the
> > following (package) structure:
> > 
> > notifier/
> >  - notifier.py
> > 
> >  - notification.py
> > 
> >  - notifiers/
> >      - activity_stream.py
> >      - general_announcements.py
> >      - to_admins.py
> >      - etc.
> > 
> >  - methods/
> >      - Email.py
> >      - Twitter.py
> >      - etc.
> > 
> >  - presentations/
> >     - email_activities.py
> >     - email_general.py
> >     - email_to_admins.py
> >     - twitter_general.py
> 
> Thanks for this Sven. My main concern about this is that it's too much
> code. I wonder if we can get away with something simpler. The minimal
> core of a pull-based implementation seems to be:
> 
> 1. Periodically loop over every user in CKAN.
> 
> 2. For each user find any new activities. Requires storing the date of
>    the last old activity per-user, new activities are defined as those
>    newer than this stored date.
> 
> 3. If there are any new activities then format an email subject and body
>    for them (rolling all the new activities into a single email).
> 
> 4. Send the email.
> 
> 5. If the email was sent successfully, update the date of the last old
>    activity for that user.
> 
> It might turn out differently when we try to actually implement it, but
> I think there are two distinct modules here:
> 
> get_notifications(user_id, since)
> 
>     Returns a list of notifications, probably just dicts with keys 'to',
>     'subject' and 'body', that need to be sent to the given user.
> 
>     `since` should be datetime of the last email notification that was
>     successfully sent to this user.
> 
>     This would call dashboard_activity_list() for the user, filter the
>     list to just those newer than since, then format an email subject
>     and body for them.
> 
> send_notification(notification_dict)
> 
>     Try to send the given email notification and raise an exception if
>     sending fails.
> 
> get_and_send_notifications()
> 
>     The periodic job that loops over all users in CKAN.
> 
>     Calls get_notifications(user_id, since) for each user.
>     This means that it needs to store `since` for each user somewhere.
> 
>     If get_notifications() returns any notifications for the user, loops
>     over the notifications and calls send_notification(notification_dict)
>     for each.
> 
>     If send_notification() returns successfully, updates `since` for
>     that user.
> 
> The user's `since` time is slightly more complicated than shown, it
> needs to be the time of the last successfully sent email notification
> _or_ the last time the user viewed her activity stream on the site,
> whichever is newer.
> 
> We can make this extensible. Note the get_notifications() just accepts a
> `user_id` and `since` date and returns email dictionaries, there's
> nothing in this interface that's specific to activity streams.
> So we could one day extend get_notifications() to call multiple helper
> functions _get_activity_streams_notifications(),
> _get_general_notifications(), etc. and then just combine all the
> notifications into one list and return them. But I don't think we should
> do this now.
> 
> As for adding different methods of sending notifications, I think user
> experience questions would need to be asked that would inform how we
> might implement something like that. I definitely think we should leave
> it out of the picture for now in terms of the code we write. But I think
> this design can potentially be extended to handle that as well. We would
> have to split get_notifications() and send_notification() into multiple
> methods get_email_notifications(), get_twitter_notifications(),
> send_email_notification(), send_twitter_notification(), etc. And we'd
> have to consider whether we want to store separate `since` dates for
> each method of notification, and think about how these interact with
> each other.