From 1946692f9a1995b15717ca3d451d7f28f0ab7d3e Mon Sep 17 00:00:00 2001
From: "Hemanth V. Alluri" <hdrive1999@gmail.com>
Date: Sun, 20 Oct 2019 22:45:44 +0530
Subject: [PATCH] users: Refactor get_members_backend endpoint to use
 get_raw_user_data.

Modify the get_raw_user_data method for use by the /users API endpoint
and then modify the /users endpoint to use it.
---
 zerver/lib/cache.py                      |  3 +-
 zerver/lib/events.py                     | 13 ++--
 zerver/openapi/zulip.yaml                |  8 +++
 zerver/tests/test_bots.py                |  4 +-
 zerver/tests/test_custom_profile_data.py | 47 ++++++++++++++
 zerver/views/users.py                    | 78 ++++--------------------
 6 files changed, 80 insertions(+), 73 deletions(-)

diff --git a/zerver/lib/cache.py b/zerver/lib/cache.py
index f3c35e8b40356..f6bcc2609c755 100644
--- a/zerver/lib/cache.py
+++ b/zerver/lib/cache.py
@@ -352,7 +352,8 @@ def user_profile_by_api_key_cache_key(api_key: str) -> str:
     'id', 'full_name', 'short_name', 'email',
     'avatar_source', 'avatar_version', 'is_active',
     'role', 'is_bot', 'realm_id', 'timezone',
-    'date_joined', 'bot_owner_id', 'delivery_email'
+    'date_joined', 'bot_owner_id', 'delivery_email',
+    'bot_type'
 ]  # type: List[str]
 
 def realm_user_dicts_cache_key(realm_id: int) -> str:
diff --git a/zerver/lib/events.py b/zerver/lib/events.py
index 2e0f09cf89c2b..e5c14b47d3c26 100644
--- a/zerver/lib/events.py
+++ b/zerver/lib/events.py
@@ -75,7 +75,8 @@ def get_custom_profile_field_values(realm_id: int) -> Dict[int, Dict[str, Any]]:
     return profiles_by_user_id
 
 
-def get_raw_user_data(realm: Realm, user_profile: UserProfile, client_gravatar: bool,
+def get_raw_user_data(realm: Realm, user_profile: UserProfile,
+                      client_gravatar: bool, for_api: bool=False,
                       include_custom_profile_fields: bool=True) -> Dict[int, Dict[str, str]]:
     user_dicts = get_realm_user_dicts(realm.id)
 
@@ -114,14 +115,18 @@ def user_data(row: Dict[str, Any]) -> Dict[str, Any]:
                 user_profile.is_realm_admin):
             result['delivery_email'] = row['delivery_email']
 
+        if for_api:
+            # The API currently has a quirk that it expects to include
+            # a bot_type field even for human users; this field is
+            # invalid so we plan to eventually remove this.
+            result['bot_type'] = row['bot_type']
         if is_bot:
             if row['email'] in settings.CROSS_REALM_BOT_EMAILS:
                 result['is_cross_realm_bot'] = True
             elif row['bot_owner_id'] is not None:
                 result['bot_owner_id'] = row['bot_owner_id']
-        else:
-            if include_custom_profile_fields:
-                result['profile_data'] = profiles_by_user_id.get(row['id'], {})
+        elif include_custom_profile_fields:
+            result['profile_data'] = profiles_by_user_id.get(row['id'], {})
         return result
 
     return {
diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index 901653840d3ea..d04306f7a60cd 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -1013,6 +1013,14 @@ paths:
           type: boolean
           default: false
         example: true
+      - name: include_custom_profile_fields
+        in: query
+        description: Set to `true` if the client wants custom profile field
+          data to be included in the response.
+        schema:
+          type: boolean
+          default: false
+        example: true
       security:
       - basicAuth: []
       responses:
diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py
index 24b17726f5cda..6e1e4cf710419 100644
--- a/zerver/tests/test_bots.py
+++ b/zerver/tests/test_bots.py
@@ -185,7 +185,7 @@ def test_add_bot(self) -> None:
         bots = [m for m in members if m['email'] == 'hambot-bot@zulip.testserver']
         self.assertEqual(len(bots), 1)
         bot = bots[0]
-        self.assertEqual(bot['bot_owner'], self.example_email('hamlet'))
+        self.assertEqual(bot['bot_owner_id'], self.example_user('hamlet').id)
         self.assertEqual(bot['user_id'], self.get_bot_user(email).id)
 
     def test_add_bot_with_username_in_use(self) -> None:
@@ -317,7 +317,7 @@ def test_add_bot_email_address_visibility(self) -> None:
         bots = [m for m in members if m['email'] == 'hambot-bot@zulip.testserver']
         self.assertEqual(len(bots), 1)
         bot = bots[0]
-        self.assertEqual(bot['bot_owner'], user.email)
+        self.assertEqual(bot['bot_owner_id'], user.id)
         self.assertEqual(bot['user_id'], self.get_bot_user(email).id)
 
     def test_bot_add_subscription(self) -> None:
diff --git a/zerver/tests/test_custom_profile_data.py b/zerver/tests/test_custom_profile_data.py
index d2015d48b07a4..e74e3f23961a4 100644
--- a/zerver/tests/test_custom_profile_data.py
+++ b/zerver/tests/test_custom_profile_data.py
@@ -638,6 +638,53 @@ def test_list_order(self) -> None:
         self.assertListEqual(content["custom_fields"],
                              sorted(content["custom_fields"], key=lambda x: -x["id"]))
 
+    def test_get_custom_profile_fields_from_api(self) -> None:
+        iago = self.example_user("iago")
+        test_bot = self.create_test_bot("foo-bot", iago)
+        assert(test_bot)
+
+        url = "/json/users?client_gravatar=false&include_custom_profile_fields=true"
+        response = self.client_get(url)
+        self.assertEqual(response.status_code, 200)
+        raw_users_data = response.json()["members"]
+
+        iago_raw_data = None
+        test_bot_raw_data = None
+
+        for user_dict in raw_users_data:
+            if user_dict["user_id"] == iago.id:
+                iago_raw_data = user_dict
+                continue
+            if user_dict["user_id"] == test_bot.id:
+                test_bot_raw_data = user_dict
+                continue
+
+        if (not iago_raw_data) or (not test_bot_raw_data):
+            raise AssertionError("Could not find required data from the response.")
+
+        expected_keys_for_iago = {
+            "email", "user_id", "avatar_url", "is_admin", "is_guest", "is_bot",
+            "full_name", "timezone", "is_active", "date_joined", "bot_type", "profile_data"}
+        self.assertEqual(set(iago_raw_data.keys()), expected_keys_for_iago)
+        self.assertIsNone(iago_raw_data["bot_type"])  # the key should exist though
+        self.assertNotEqual(iago_raw_data["profile_data"], {})
+
+        expected_keys_for_test_bot = {
+            "email", "user_id", "avatar_url", "is_admin", "is_guest", "is_bot", "full_name",
+            "timezone", "is_active", "date_joined", "bot_type", "bot_owner_id"}
+        self.assertEqual(set(test_bot_raw_data.keys()), expected_keys_for_test_bot)
+        self.assertEqual(test_bot_raw_data["bot_type"], 1)
+        self.assertEqual(test_bot_raw_data["bot_owner_id"], iago_raw_data["user_id"])
+
+        url = "/json/users?client_gravatar=false"
+        response = self.client_get(url)
+        self.assertEqual(response.status_code, 200)
+        raw_users_data = response.json()["members"]
+        for user_dict in raw_users_data:
+            with self.assertRaises(KeyError):
+                user_dict["profile_data"]
+
+
 class ReorderCustomProfileFieldTest(CustomProfileFieldTestCase):
     def test_reorder(self) -> None:
         self.login(self.example_email("iago"))
diff --git a/zerver/views/users.py b/zerver/views/users.py
index 9dba87e468e1b..1f208ecccfd95 100644
--- a/zerver/views/users.py
+++ b/zerver/views/users.py
@@ -9,6 +9,7 @@
 
 from zerver.decorator import require_realm_admin, require_member_or_admin
 from zerver.forms import CreateUserForm
+from zerver.lib.events import get_raw_user_data
 from zerver.lib.actions import do_change_avatar_fields, do_change_bot_owner, \
     do_change_is_admin, do_change_default_all_public_streams, \
     do_change_default_events_register_stream, do_change_default_sending_stream, \
@@ -16,7 +17,7 @@
     check_change_full_name, notify_created_bot, do_update_outgoing_webhook_service, \
     do_update_bot_config_data, check_change_bot_full_name, do_change_is_guest, \
     do_update_user_custom_profile_data_if_changed, check_remove_custom_profile_field_value
-from zerver.lib.avatar import avatar_url, get_gravatar_url, get_avatar_field
+from zerver.lib.avatar import avatar_url, get_gravatar_url
 from zerver.lib.bot_config import set_bot_config
 from zerver.lib.exceptions import CannotDeactivateLastUserError
 from zerver.lib.integrations import EMBEDDED_BOTS
@@ -398,82 +399,27 @@ def bot_info(bot_profile: UserProfile) -> Dict[str, Any]:
 
 @has_request_variables
 def get_members_backend(request: HttpRequest, user_profile: UserProfile,
-                        client_gravatar: bool=REQ(validator=check_bool, default=False)) -> HttpResponse:
+                        include_custom_profile_fields: bool=REQ(validator=check_bool,
+                                                                default=False),
+                        client_gravatar: bool=REQ(validator=check_bool, default=False)
+                        ) -> HttpResponse:
     '''
     The client_gravatar field here is set to True if clients can compute
     their own gravatars, which saves us bandwidth.  We want to eventually
     make this the default behavior, but we have old clients that expect
     the server to compute this for us.
     '''
-
     realm = user_profile.realm
-
-    fields = [
-        'id',
-        'email',
-        'realm_id',
-        'full_name',
-        'is_bot',
-        'is_active',
-        'bot_type',
-        'role',
-        'avatar_source',
-        'avatar_version',
-        'bot_owner__email',
-        'timezone',
-    ]
     if realm.email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS:
         # If email addresses are only available to administrators,
         # clients cannot compute gravatars, so we force-set it to false.
         client_gravatar = False
-    include_delivery_email = user_profile.is_realm_admin
-    if include_delivery_email:
-        fields.append("delivery_email")
-        fields.append("bot_owner__delivery_email")
-
-    query = UserProfile.objects.filter(
-        realm_id=realm.id
-    ).values(*fields)
-
-    def get_member(row: Dict[str, Any]) -> Dict[str, Any]:
-        email = row['email']
-        user_id = row['id']
-
-        result = dict(
-            user_id=user_id,
-            email=email,
-            full_name=row['full_name'],
-            is_bot=row['is_bot'],
-            is_active=row['is_active'],
-            bot_type=row['bot_type'],
-            is_admin=row['role'] == UserProfile.ROLE_REALM_ADMINISTRATOR,
-            is_guest=row['role'] == UserProfile.ROLE_GUEST,
-            timezone=row['timezone'],
-        )
-
-        result['avatar_url'] = get_avatar_field(
-            user_id=user_id,
-            email=email,
-            avatar_source=row['avatar_source'],
-            avatar_version=row['avatar_version'],
-            realm_id=row['realm_id'],
-            medium=False,
-            client_gravatar=client_gravatar,
-        )
-
-        if row['bot_owner__email']:
-            result['bot_owner'] = row['bot_owner__email']
-
-        if include_delivery_email:
-            result['delivery_email'] = row['delivery_email']
-            if row['bot_owner__delivery_email']:
-                result['bot_owner_delivery_email'] = row['bot_owner__delivery_email']
-
-        return result
-
-    members = [get_member(row) for row in query]
-
-    return json_success({'members': members})
+    members = get_raw_user_data(realm,
+                                user_profile=user_profile,
+                                client_gravatar=client_gravatar,
+                                for_api=True,
+                                include_custom_profile_fields=include_custom_profile_fields)
+    return json_success({'members': members.values()})
 
 @require_realm_admin
 @has_request_variables