respond to review comments

2 files changed, 16 insertions, 24 deletions

diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index 0df2119258..9ca7d9b5d3 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -86,12 +86,10 @@ class MonthlyActiveUsersStore(SQLBaseStore):
             if len(self.reserved_users) > 0:
                 # questionmarks is a hack to overcome sqlite not supporting
                 # tuples in 'WHERE IN %s'
-                questionmarks = "?" * len(self.reserved_users)
+                question_marks = ",".join("?" * len(self.reserved_users))
 
                 query_args.extend(self.reserved_users)
-                sql = base_sql + """ AND user_id NOT IN ({})""".format(
-                    ",".join(questionmarks)
-                )
+                sql = base_sql + """ AND user_id NOT IN ({})""".format(question_marks)
             else:
                 sql = base_sql
 
@@ -120,9 +118,9 @@ class MonthlyActiveUsersStore(SQLBaseStore):
                 # Need if/else since 'AND user_id NOT IN ({})' fails on Postgres
                 # when len(reserved_users) == 0. Works fine on sqlite.
                 else:
-                    safe_guard = max_mau_value - len(self.reserved_users)
-                    # Must be greater than zero for postgres
-                    safe_guard = safe_guard if safe_guard > 0 else 0
+                    # Must be >= 0 for postgres
+                    num_of_non_reserved_users_to_remove = max(max_mau_value - len(self.reserved_users), 0)
+                    
                     # It is important to filter reserved users twice to guard
                     # against the case where the reserved user is present in the
                     # SELECT, meaning that a legitmate mau is deleted.
@@ -130,19 +128,13 @@ class MonthlyActiveUsersStore(SQLBaseStore):
                         DELETE FROM monthly_active_users
                         WHERE user_id NOT IN (
                             SELECT user_id FROM monthly_active_users
-                            WHERE user_id NOT IN ({})""".format(
-                        ",".join(questionmarks)
-                    ) + """
+                            WHERE user_id NOT IN ({})
                             ORDER BY timestamp DESC
                             LIMIT ?
                         )
-                        AND user_id NOT IN ({})""".format(
-                        ",".join(questionmarks)
-                    )
-                    query_args = []
-                    query_args.extend(self.reserved_users)
-                    query_args.append(safe_guard)
-                    query_args.extend(self.reserved_users)
+                        AND user_id NOT IN ({})""".format(question_marks, question_marks)
+
+                    query_args = [*self.reserved_users, num_of_non_reserved_users_to_remove, *self.reserved_users]
 
                     txn.execute(sql, query_args)
 
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index b7a90cbbf6..93a5be594e 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -167,29 +167,29 @@ class MonthlyActiveUsersTestCase(unittest.HomeserverTestCase):
         for i in range(initial_users):
             user = "@user%d:server" % i
             email = "user%d@example.com" % i
-            self.store.upsert_monthly_active_user(user)
+            self.get_success(self.store.upsert_monthly_active_user(user))
             threepids.append({"medium": "email", "address": email})
             # Need to ensure that the most recent entries in the
             # monthly_active_users table are reserved
             now = int(self.hs.get_clock().time_msec())
             if i != 0:
-                self.store.register_user(user_id=user, password_hash=None)
-                self.pump()
-                self.store.user_add_threepid(user, "email", email, now, now)
+                self.get_success(self.store.register_user(user_id=user, password_hash=None))
+                #self.pump()
+                self.get_success(self.store.user_add_threepid(user, "email", email, now, now))
 
         self.hs.config.mau_limits_reserved_threepids = threepids
         self.store.runInteraction(
             "initialise", self.store._initialise_reserved_users, threepids
         )
-        self.pump()
+        #self.pump()
         count = self.store.get_monthly_active_count()
         self.assertTrue(self.get_success(count), initial_users)
 
         count = self.store.get_registered_reserved_users_count()
         self.assertEquals(self.get_success(count), reserved_user_number)
 
-        self.store.reap_monthly_active_users()
-        self.pump()
+        self.get_success(self.store.reap_monthly_active_users())
+        #self.pump()
         count = self.store.get_monthly_active_count()
         self.assertEquals(self.get_success(count), self.hs.config.max_mau_value)