Merge pull request #799 from matrix-org/matthew/quieter-email-notifs

Tune email notifs to make them quieter:

3 files changed, 72 insertions, 23 deletions

diff --git a/synapse/push/emailpusher.py b/synapse/push/emailpusher.py
index b4b728adc5..a72cba8306 100644
--- a/synapse/push/emailpusher.py
+++ b/synapse/push/emailpusher.py
@@ -32,12 +32,19 @@ DELAY_BEFORE_MAIL_MS = 10 * 60 * 1000
 # Each room maintains its own throttle counter, but each new mail notification
 # sends the pending notifications for all rooms.
 THROTTLE_START_MS = 10 * 60 * 1000
-THROTTLE_MAX_MS = 24 * 60 * 60 * 1000  # (2 * 60 * 1000) * (2 ** 11)  # ~3 days
-THROTTLE_MULTIPLIER = 6                # 10 mins, 1 hour, 6 hours, 24 hours
+THROTTLE_MAX_MS = 24 * 60 * 60 * 1000  # 24h
+# THROTTLE_MULTIPLIER = 6              # 10 mins, 1 hour, 6 hours, 24 hours
+THROTTLE_MULTIPLIER = 144              # 10 mins, 24 hours - i.e. jump straight to 1 day
 
 # If no event triggers a notification for this long after the previous,
 # the throttle is released.
-THROTTLE_RESET_AFTER_MS = (2 * 60 * 1000) * (2 ** 11)  # ~3 days
+# 12 hours - a gap of 12 hours in conversation is surely enough to merit a new
+# notification when things get going again...
+THROTTLE_RESET_AFTER_MS = (12 * 60 * 60 * 1000)
+
+# does each email include all unread notifs, or just the ones which have happened
+# since the last mail?
+INCLUDE_ALL_UNREAD_NOTIFS = True
 
 
 class EmailPusher(object):
@@ -126,8 +133,9 @@ class EmailPusher(object):
         up logging, measures and guards against multiple instances of it
         being run.
         """
+        start = 0 if INCLUDE_ALL_UNREAD_NOTIFS else self.last_stream_ordering
         unprocessed = yield self.store.get_unread_push_actions_for_user_in_range(
-            self.user_id, self.last_stream_ordering, self.max_stream_ordering
+            self.user_id, start, self.max_stream_ordering
         )
 
         soonest_due_at = None
@@ -150,7 +158,6 @@ class EmailPusher(object):
                 # we then consider all previously outstanding notifications
                 # to be delivered.
 
-                # debugging:
                 reason = {
                     'room_id': push_action['room_id'],
                     'now': self.clock.time_msec(),
@@ -165,9 +172,12 @@ class EmailPusher(object):
                 yield self.save_last_stream_ordering_and_success(max([
                     ea['stream_ordering'] for ea in unprocessed
                 ]))
-                yield self.sent_notif_update_throttle(
-                    push_action['room_id'], push_action
-                )
+
+                # we update the throttle on all the possible unprocessed push actions
+                for ea in unprocessed:
+                    yield self.sent_notif_update_throttle(
+                        ea['room_id'], ea
+                    )
                 break
             else:
                 if soonest_due_at is None or should_notify_at < soonest_due_at:
diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py
index c2c2ca3fa7..d8a0c35c79 100644
--- a/synapse/push/mailer.py
+++ b/synapse/push/mailer.py
@@ -44,8 +44,11 @@ MESSAGE_FROM_PERSON_IN_ROOM = "You have a message on %(app)s from %(person)s " \
                               "in the %s room..."
 MESSAGE_FROM_PERSON = "You have a message on %(app)s from %(person)s..."
 MESSAGES_FROM_PERSON = "You have messages on %(app)s from %(person)s..."
-MESSAGES_IN_ROOM = "There are some messages on %(app)s for you in the %(room)s room..."
-MESSAGES_IN_ROOMS = "Here are some messages on %(app)s you may have missed..."
+MESSAGES_IN_ROOM = "You have messages on %(app)s in the %(room)s room..."
+MESSAGES_IN_ROOM_AND_OTHERS = \
+    "You have messages on %(app)s in the %(room)s room and others..."
+MESSAGES_FROM_PERSON_AND_OTHERS = \
+    "You have messages on %(app)s from %(person)s and others..."
 INVITE_FROM_PERSON_TO_ROOM = "%(person)s has invited you to join the " \
                              "%(room)s room on %(app)s..."
 INVITE_FROM_PERSON = "%(person)s has invited you to chat on %(app)s..."
@@ -128,9 +131,14 @@ class Mailer(object):
             state_by_room[room_id] = room_state
 
         # Run at most 3 of these at once: sync does 10 at a time but email
-        # notifs are much realtime than sync so we can afford to wait a bit.
+        # notifs are much less realtime than sync so we can afford to wait a bit.
         yield concurrently_execute(_fetch_room_state, rooms_in_order, 3)
 
+        # actually sort our so-called rooms_in_order list, most recent room first
+        rooms_in_order.sort(
+            key=lambda r: -(notifs_by_room[r][-1]['received_ts'] or 0)
+        )
+
         rooms = []
 
         for r in rooms_in_order:
@@ -139,12 +147,12 @@ class Mailer(object):
             )
             rooms.append(roomvars)
 
-        summary_text = self.make_summary_text(
-            notifs_by_room, state_by_room, notif_events, user_id
+        reason['room_name'] = calculate_room_name(
+            state_by_room[reason['room_id']], user_id, fallback_to_members=True
         )
 
-        reason['room_name'] = calculate_room_name(
-            state_by_room[reason['room_id']], user_id, fallback_to_members=False
+        summary_text = self.make_summary_text(
+            notifs_by_room, state_by_room, notif_events, user_id, reason
         )
 
         template_vars = {
@@ -296,7 +304,8 @@ class Mailer(object):
 
         return messagevars
 
-    def make_summary_text(self, notifs_by_room, state_by_room, notif_events, user_id):
+    def make_summary_text(self, notifs_by_room, state_by_room,
+                          notif_events, user_id, reason):
         if len(notifs_by_room) == 1:
             # Only one room has new stuff
             room_id = notifs_by_room.keys()[0]
@@ -371,9 +380,28 @@ class Mailer(object):
                     }
         else:
             # Stuff's happened in multiple different rooms
-            return MESSAGES_IN_ROOMS % {
-                "app": self.app_name,
-            }
+
+            # ...but we still refer to the 'reason' room which triggered the mail
+            if reason['room_name'] is not None:
+                return MESSAGES_IN_ROOM_AND_OTHERS % {
+                    "room": reason['room_name'],
+                    "app": self.app_name,
+                }
+            else:
+                # If the reason room doesn't have a name, say who the messages
+                # are from explicitly to avoid, "messages in the Bob room"
+                sender_ids = list(set([
+                    notif_events[n['event_id']].sender
+                    for n in notifs_by_room[reason['room_id']]
+                ]))
+
+                return MESSAGES_FROM_PERSON_AND_OTHERS % {
+                    "person": descriptor_from_member_events([
+                        state_by_room[reason['room_id']][("m.room.member", s)]
+                        for s in sender_ids
+                    ]),
+                    "app": self.app_name,
+                }
 
     def make_room_link(self, room_id):
         # need /beta for Universal Links to work on iOS
diff --git a/synapse/util/presentable_names.py b/synapse/util/presentable_names.py
index 3efa8a8206..a6866f6117 100644
--- a/synapse/util/presentable_names.py
+++ b/synapse/util/presentable_names.py
@@ -14,6 +14,9 @@
 # limitations under the License.
 
 import re
+import logging
+
+logger = logging.getLogger(__name__)
 
 # intentionally looser than what aliases we allow to be registered since
 # other HSes may allow aliases that we would not
@@ -105,13 +108,21 @@ def calculate_room_name(room_state, user_id, fallback_to_members=True):
             # or inbound invite, or outbound 3PID invite.
             if all_members[0].sender == user_id:
                 if "m.room.third_party_invite" in room_state_bytype:
-                    third_party_invites = room_state_bytype["m.room.third_party_invite"]
+                    third_party_invites = (
+                        room_state_bytype["m.room.third_party_invite"].values()
+                    )
+
                     if len(third_party_invites) > 0:
                         # technically third party invite events are not member
                         # events, but they are close enough
-                        return "Inviting %s" (
-                            descriptor_from_member_events(third_party_invites)
-                        )
+
+                        # FIXME: no they're not - they look nothing like a member;
+                        # they have a great big encrypted thing as their name to
+                        # prevent leaking the 3PID name...
+                        # return "Inviting %s" % (
+                        #     descriptor_from_member_events(third_party_invites)
+                        # )
+                        return "Inviting email address"
                     else:
                         return ALL_ALONE
             else: