Merge lp:~robru/friends/limit-notifications into lp:friends

[Robert Bruce Park]

Limit the rate and age of friends notifications.

I've taken a two-pronged approach at limiting notifications here.

The first is by preventing messages more than 5 days old from being notified. Lots of people were complaining about ancient messages being re-notified, so this should curb that. We might even want to consider making the limit less than 5 days, but I think this is a good choice for now and we can easily finesse it in future based on user feedback (if any).

Secondly, I also implement a global limit of 5 notifications per instance of friends-dispatcher. In practise this means that friends will never notify more than 5 times per refresh interval (which is 15 minutes by default). This value of 5 may also need finessing as well, but the point is that the infrastructure is now in place and it is easy to tweak as we see fit later on.

As usual, massive test coverage is included so we know for sure that these limitations are actually taking effect as expected.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:190
http://jenkins.qa.ubuntu.com/job/friends-ci/27/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/friends-raring-amd64-ci/27

Click here to trigger a rebuild:
http://s-jenkins:8080/job/friends-ci/27/rebuild

review: Approve (continuous-integration)

[Ken VanDine (ken-vandine)]

I like the idea of not notifying for older events, and 5 days is probably good. But I don't think we should limit the number shown per refresh, at least not across all operations. For example, it is more important to see a notification for a direct message or mention than it is to see one for some random status update. So perhaps the limit should be per operation, with different thresholds, so we can notify for all direct messages but only 5 status updates (assuming notifications are enabled for all).

review: Needs Information

[Robert Bruce Park (robru)]

Well, the default is to not even show notifications for regular status updates, only direct & mentions. I disagree that it's important to notify for all the mentions, because once you see 5 notifications you're going to either check your Friends-app, or maybe even sign in in your browser, and there you will discover all of the messages that are waiting for you anyway.

Also, it's not currently possible to know whether a notify was for a mention or a regular status update from within notify(), so it would be difficult to implement the way you request.

[lp:~robru/friends/limit-notifications]

191. By Robert Bruce Park on 2013-04-16

Revert r188, so allow more than 5 notifications

(but keep the logic about not allowing notifications older than 5 days)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:191
http://jenkins.qa.ubuntu.com/job/friends-ci/36/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/friends-raring-amd64-ci/36/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/friends-ci/36/rebuild

review: Needs Fixing (continuous-integration)

[lp:~robru/friends/limit-notifications]

192. By Robert Bruce Park on 2013-04-16

Fix tests.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:192
http://jenkins.qa.ubuntu.com/job/friends-ci/37/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/friends-raring-amd64-ci/37

Click here to trigger a rebuild:
http://s-jenkins:8080/job/friends-ci/37/rebuild

review: Approve (continuous-integration)

[Ken VanDine (ken-vandine)]

Good solution for now, thanks

review: Approve

=== modified file 'friends/tests/test_notify.py'
--- friends/tests/test_notify.py	2013-04-08 19:33:45 +0000
+++ friends/tests/test_notify.py	2013-04-16 17:56:28 +0000
@@ -26,6 +26,11 @@
 from friends.utils.base import Base
 from friends.utils.notify import notify
 
+from datetime import datetime, timedelta
+
+RIGHT_NOW = datetime.now().isoformat()
+YESTERDAY = (datetime.now() - timedelta(1)).isoformat()
+LAST_WEEK = (datetime.now() - timedelta(7)).isoformat()
 
 class TestNotifications(unittest.TestCase):
     """Test notification details."""
@@ -43,12 +48,27 @@
             message='http://example.com!',
             message_id='1234',
             sender='Benjamin',
+            timestamp=RIGHT_NOW,
             )
         notify.assert_called_once_with('Benjamin', 'http://example.com!', '')
 
     @mock.patch('friends.utils.base.Model', TestModel)
     @mock.patch('friends.utils.base._seen_ids', {})
     @mock.patch('friends.utils.base.notify')
+    def test_publish_no_stale(self, notify):
+        Base._do_notify = lambda protocol, stream: True
+        base = Base(FakeAccount())
+        base._publish(
+            message='http://example.com!',
+            message_id='1234',
+            sender='Benjamin',
+            timestamp=LAST_WEEK,
+            )
+        self.assertEqual(notify.call_count, 0)
+
+    @mock.patch('friends.utils.base.Model', TestModel)
+    @mock.patch('friends.utils.base._seen_ids', {})
+    @mock.patch('friends.utils.base.notify')
     def test_publish_all(self, notify):
         Base._do_notify = lambda protocol, stream: True
         base = Base(FakeAccount())
@@ -56,6 +76,7 @@
             message='notify!',
             message_id='1234',
             sender='Benjamin',
+            timestamp=YESTERDAY,
             )
         notify.assert_called_once_with('Benjamin', 'notify!', '')
 
@@ -71,6 +92,7 @@
             message_id='1234',
             sender='Benjamin',
             stream='private',
+            timestamp=RIGHT_NOW,
             )
         notify.assert_called_once_with('Benjamin', 'This message is private!', '')
 
@@ -86,6 +108,7 @@
             message_id='1234',
             sender='Benjamin',
             stream='messages',
+            timestamp=RIGHT_NOW,
             )
         self.assertEqual(notify.call_count, 0)
 
@@ -100,6 +123,7 @@
             message_id='1234',
             sender='Benjamin',
             stream='messages',
+            timestamp=RIGHT_NOW,
             )
         self.assertEqual(notify.call_count, 0)
 

=== modified file 'friends/utils/base.py'
--- friends/utils/base.py	2013-04-08 19:33:45 +0000
+++ friends/utils/base.py	2013-04-16 17:56:28 +0000
@@ -28,7 +28,7 @@
 import logging
 import threading
 
-from datetime import datetime
+from datetime import datetime, timedelta
 from oauthlib.oauth1 import Client
 
 from gi.repository import GLib, GObject, EDataServer, EBook
@@ -40,6 +40,7 @@
 from friends.utils.time import ISO8601_FORMAT
 
 
+FIVE_DAYS_AGO = (datetime.now() - timedelta(5)).isoformat()
 STUB = lambda *ignore, **kwignore: None
 COMMA_SPACE = ', '
 SCHEMA = Schema()
@@ -366,9 +367,13 @@
             # Don't let duplicate messages into the model
             if message_id not in _seen_ids:
                 _seen_ids[message_id] = Model.get_position(Model.append(*args))
-                # I think it's safe not to notify the user about
-                # messages that they sent themselves...
-                if not args[FROM_ME_IDX] and self._do_notify(args[STREAM_IDX]):
+
+                # Don't notify messages from me, or older than five days.
+                if args[FROM_ME_IDX] or args[TIME_IDX] < FIVE_DAYS_AGO:
+                    return True
+
+                # Check if notifications are enabled before notifying.
+                if self._do_notify(args[STREAM_IDX]):
                     notify(
                         args[SENDER_IDX],
                         orig_message,