[#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

@@ -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():

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()