From f5fac45ff9bede1217b2283c68a51d0904b6d31d Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 29 Nov 2012 17:00:29 +0100 Subject: [PATCH] [#1635] Don't send old emails when user enables notifications After a user enables her email notifications preference, don't send her emails about old activities that happened before she enabled it (even if those activities are still marked as new on her dashboard). This prevents the user from getting a flood of email notifications as soon as she enables the preference (currently she would only get one email anyway as all activities are always packed into one digest email, but if that changes in future this safeguard might come in handy) --- ckan/lib/email_notifications.py | 48 +++++++++++----------- ckan/tests/lib/test_email_notifications.py | 34 ++++++++++++++- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/ckan/lib/email_notifications.py b/ckan/lib/email_notifications.py index dae232b1980..d6256c5b5cd 100644 --- a/ckan/lib/email_notifications.py +++ b/ckan/lib/email_notifications.py @@ -105,35 +105,37 @@ def send_notification(user, email_dict): try: ckan.lib.mailer.mail_recipient(user['display_name'], user['email'], email_dict['subject'], email_dict['body']) - # FIXME: We are accessing model from lib here but I'm not sure what - # else to do unless we add a update_email_last_sent() - # logic function which would only be needed by this lib. - model.Dashboard.get(user['id']).email_last_sent = ( - datetime.datetime.now()) - # TODO: Do something with response? except ckan.lib.mailer.MailerException: raise def get_and_send_notifications_for_user(user): - # Don't send email if the user has her email notifications preference - # turned off. - if not user['email_notifications']: - return - - # FIXME: We are accessing model from lib here but I'm not sure what else - # to do unless we add a get_email_last_sent() logic function - # which would only be needed by this lib. - email_last_sent = model.Dashboard.get(user['id']).email_last_sent - activity_stream_last_viewed = ( - model.Dashboard.get(user['id']).activity_stream_last_viewed) - since = max(email_last_sent, activity_stream_last_viewed) - - notifications = get_notifications(user['id'], since) - # TODO: Handle failures from send_email_notification. - for notification in notifications: - send_notification(user, notification) + if user['email_notifications']: + + # FIXME: We are accessing model from lib here but I'm not sure what else + # to do unless we add a get_email_last_sent() logic function + # which would only be needed by this lib. + email_last_sent = model.Dashboard.get(user['id']).email_last_sent + activity_stream_last_viewed = ( + model.Dashboard.get(user['id']).activity_stream_last_viewed) + since = max(email_last_sent, activity_stream_last_viewed) + + notifications = get_notifications(user['id'], since) + + # TODO: Handle failures from send_email_notification. + for notification in notifications: + send_notification(user, notification) + + # Whether the user had har 'email_notifications' preference turned on or + # not, we still update her email_last_sent time. This prevents users from + # getting me emails about old activities when they turn on email + # notifications. + '''Update the given user's email_last_sent time to the current time.''' + # FIXME: We are accessing model from lib here but I'm not sure what + # else to do unless we add a update_email_last_sent() + # logic function which would only be needed by this lib. + model.Dashboard.get(user['id']).email_last_sent = datetime.datetime.now() def get_and_send_notifications_for_all_users(): diff --git a/ckan/tests/lib/test_email_notifications.py b/ckan/tests/lib/test_email_notifications.py index 9b216c7ecb6..46e788f820d 100644 --- a/ckan/tests/lib/test_email_notifications.py +++ b/ckan/tests/lib/test_email_notifications.py @@ -234,15 +234,45 @@ def test_01_no_email_notifications_when_disabled(self): def test_02_enable_email_notifications(self): '''Users should be able to turn email notifications on.''' + # Mark all Sara's new activities as old, just to get a fresh start. + post(self.app, 'dashboard_mark_activities_old', + apikey=self.sara['apikey']) + assert post(self.app, 'dashboard_new_activities_count', + apikey=self.sara['apikey']) == 0 + + # Update the followed dataset a few times so Sara gets a few new + # activities. + for i in range(1, 4): + post(self.app, 'package_update', apikey=self.joeadmin['apikey'], + id='warandpeace', notes='updated {0} times'.format(i)) + + # Now Sara should have new activities. + assert post(self.app, 'dashboard_new_activities_count', + apikey=self.sara['apikey']) == 3 + + # Run the email notifier job. + email_notifications.get_and_send_notifications_for_all_users() + assert len(self.get_smtp_messages()) == 0 + + # Enable email notifications for Sara. self.sara['email_notifications'] = True post(self.app, 'user_update', **self.sara) + email_notifications.get_and_send_notifications_for_all_users() + assert len(self.get_smtp_messages()) == 0, ("After a user enables " + "email notifications she should _not_ get emails about activities " + "that happened before she enabled them, even if those activities " + "are still marked as 'new' on her dashboard.") + + # Update the package to generate another new activity. post(self.app, 'package_update', apikey=self.joeadmin['apikey'], - id='warandpeace', notes='updated again') + id='warandpeace', notes='updated yet again') + # Check that Sara has a new activity. assert post(self.app, 'dashboard_new_activities_count', - apikey=self.sara['apikey']) > 0 + apikey=self.sara['apikey']) == 4 + # Run the email notifier job, this time Sara should get one email. email_notifications.get_and_send_notifications_for_all_users() assert len(self.get_smtp_messages()) == 1 self.clear_smtp_messages()