From 14d2d4e50666832ea4ba2b0fc1cdf4d7e1d767e3 Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulipchat.com>
Date: Sat, 21 Oct 2017 18:14:44 -0700
Subject: [PATCH] Fix bug in flush_user_profile().

Every time we updated a UserProfile object, we were calling
delete_display_recipient_cache(), which churns the cache and
does an extra database hop to find subscriptions.  This was
due to saying `updated_fields` instead of `update_fields`.

This made us prone to cache churn for fields like UserProfile.pointer
that are fairly volatile.

Now we use the helper function changed().  To prevent the
opposite problem, we use all the fields that could invalidate
the cache.
---
 zerver/lib/cache.py           |  3 +--
 zerver/tests/test_home.py     |  2 +-
 zerver/tests/test_messages.py |  2 +-
 zerver/tests/test_signup.py   |  2 +-
 zerver/tests/test_users.py    | 16 ++++++++++++++++
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/zerver/lib/cache.py b/zerver/lib/cache.py
index e0b70b3e23..44e86d8e74 100644
--- a/zerver/lib/cache.py
+++ b/zerver/lib/cache.py
@@ -401,8 +401,7 @@ def flush_user_profile(sender, **kwargs):
     if changed(['is_active']):
         cache_delete(active_user_ids_cache_key(user_profile.realm_id))
 
-    if kwargs.get('updated_fields') is None or \
-            'email' in kwargs['update_fields']:
+    if changed(['email', 'full_name', 'short_name', 'id', 'is_mirror_dummy']):
         delete_display_recipient_cache(user_profile)
 
     # Invalidate our bots_in_realm info dict if any bot has
diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py
index 5090f0e1a5..6b5b7b6998 100644
--- a/zerver/tests/test_home.py
+++ b/zerver/tests/test_home.py
@@ -178,7 +178,7 @@ class HomeTest(ZulipTestCase):
             with patch('zerver.lib.cache.cache_set') as cache_mock:
                 result = self._get_home_page(stream='Denmark')
 
-        self.assert_length(queries, 41)
+        self.assert_length(queries, 40)
         self.assert_length(cache_mock.call_args_list, 10)
 
         html = result.content.decode('utf-8')
diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py
index e7d99cb10f..64121a4315 100644
--- a/zerver/tests/test_messages.py
+++ b/zerver/tests/test_messages.py
@@ -2412,7 +2412,7 @@ class SoftDeactivationMessageTest(ZulipTestCase):
         self.assertNotEqual(idle_user_msg_list[-1].content, message)
         with queries_captured() as queries:
             maybe_catch_up_soft_deactivated_user(long_term_idle_user)
-        self.assert_length(queries, 8)
+        self.assert_length(queries, 7)
         self.assertFalse(long_term_idle_user.long_term_idle)
         self.assertEqual(last_realm_audit_log_entry(
             'user_soft_activated').modified_user, long_term_idle_user)
diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py
index d2f40cdf7d..943c3cfedb 100644
--- a/zerver/tests/test_signup.py
+++ b/zerver/tests/test_signup.py
@@ -324,7 +324,7 @@ class LoginTest(ZulipTestCase):
         with queries_captured() as queries:
             self.register(self.nonreg_email('test'), "test")
         # Ensure the number of queries we make is not O(streams)
-        self.assert_length(queries, 68)
+        self.assert_length(queries, 67)
         user_profile = self.nonreg_user('test')
         self.assertEqual(get_session_dict_user(self.client.session), user_profile.id)
         self.assertFalse(user_profile.enable_stream_desktop_notifications)
diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py
index b90f9c8ceb..d68d0e84ca 100644
--- a/zerver/tests/test_users.py
+++ b/zerver/tests/test_users.py
@@ -38,6 +38,7 @@ from zerver.lib.stream_topic import StreamTopicTarget
 from django.conf import settings
 
 import datetime
+import mock
 import os
 import sys
 import time
@@ -260,6 +261,21 @@ class UserProfileTest(ZulipTestCase):
         self.assertEqual(dct[hamlet.id], self.example_email("hamlet"))
         self.assertEqual(dct[othello.id], self.example_email("othello"))
 
+    def test_cache_invalidation(self):
+        # type: () -> None
+        hamlet = self.example_user('hamlet')
+        with mock.patch('zerver.lib.cache.delete_display_recipient_cache') as m:
+            hamlet.full_name = 'Hamlet Junior'
+            hamlet.save(update_fields=["full_name"])
+
+        self.assertTrue(m.called)
+
+        with mock.patch('zerver.lib.cache.delete_display_recipient_cache') as m:
+            hamlet.long_term_idle = True
+            hamlet.save(update_fields=["long_term_idle"])
+
+        self.assertFalse(m.called)
+
 class ActivateTest(ZulipTestCase):
     def test_basics(self):
         # type: () -> None