users: Rewrite get_cross_realm_dicts to call format_user_row.

This modifies get_cross_realm_dicts in zerver.lib.users to call
format_user_row.  This is done to remove current and prevent future
inconsistencies between in the dictionary formats for get_raw_user_data
and get_cross_realm_dicts.

Implementation substantially rewritten by tabbott.

Fixes #13638.
This commit is contained in:
akashaviator
2020-01-14 22:49:35 +05:30
committed by Tim Abbott
parent 7d06293ac0
commit 20b8b29d11
2 changed files with 69 additions and 31 deletions

View File

@@ -5,10 +5,11 @@ from collections import defaultdict
from django.conf import settings from django.conf import settings
from django.db.models.query import QuerySet from django.db.models.query import QuerySet
from django.forms.models import model_to_dict
from django.utils.translation import ugettext as _ from django.utils.translation import ugettext as _
from zerver.lib.cache import generic_bulk_cached_fetch, user_profile_cache_key_id, \ from zerver.lib.cache import generic_bulk_cached_fetch, user_profile_cache_key_id, \
user_profile_by_id_cache_key user_profile_by_id_cache_key, realm_user_dict_fields
from zerver.lib.request import JsonableError from zerver.lib.request import JsonableError
from zerver.lib.avatar import avatar_url, get_avatar_field from zerver.lib.avatar import avatar_url, get_avatar_field
from zerver.lib.exceptions import OrganizationAdministratorRequired from zerver.lib.exceptions import OrganizationAdministratorRequired
@@ -326,23 +327,45 @@ def format_user_row(realm: Realm, acting_user: UserProfile, row: Dict[str, Any],
return result return result
def get_cross_realm_dicts() -> List[Dict[str, Any]]: def get_cross_realm_dicts() -> List[Dict[str, Any]]:
users = bulk_get_users(list(settings.CROSS_REALM_BOT_EMAILS), None, users = bulk_get_users(list(settings.CROSS_REALM_BOT_EMAILS), None,
base_query=UserProfile.objects.filter( base_query=UserProfile.objects.filter(
realm__string_id=settings.SYSTEM_BOT_REALM)).values() realm__string_id=settings.SYSTEM_BOT_REALM)).values()
return [{'email': user.email, result = []
'user_id': user.id, for user in users:
'is_admin': user.is_realm_admin, # Important: We filter here, is addition to in
'is_bot': user.is_bot, # `base_query`, because of how bulk_get_users shares its
'avatar_url': avatar_url(user), # cache with other UserProfile caches.
'timezone': user.timezone, if user.realm.string_id != settings.SYSTEM_BOT_REALM:
'date_joined': user.date_joined.isoformat(), continue
'bot_owner_id': None,
'full_name': user.full_name} # What we're trying to do is simulate bulk_get_users returning
for user in users # `.values(*realm_user_dict_fields)` even though we fetched
# Important: We filter here, is addition to in # UserProfile objects. This is messier than it seems.
# `base_query`, because of how bulk_get_users shares its #
# cache with other UserProfile caches. # What we'd like to do is just call model_to_dict(user,
if user.realm.string_id == settings.SYSTEM_BOT_REALM] # fields=realm_user_dict_fields). The problem with this is
# that model_to_dict has a different convention than
# `.values()` in its handling of foreign keys, naming them as
# e.g. `bot_owner`, not `bot_owner_id`; we work around that
# here. And then, because we want to avoid clients dealing
# with the implementation detail that these bots are
# self-owned, we just set bot_owner_id=None.
#
# This could be potentially simplified in the future by
# changing realm_user_dict_fields to name the bot owner with
# the less readable `bot_owner` (instead of `bot_owner_id`).
user_row = model_to_dict(user,
fields=realm_user_dict_fields)
user_row['bot_owner_id'] = None
result.append(format_user_row(user.realm,
acting_user=user,
row=user_row,
client_gravatar=False,
custom_profile_field_data=None))
return result
def get_custom_profile_field_values(realm_id: int) -> Dict[int, Dict[str, Any]]: def get_custom_profile_field_values(realm_id: int) -> Dict[int, Dict[str, Any]]:
# TODO: Consider optimizing this query away with caching. # TODO: Consider optimizing this query away with caching.

View File

@@ -2,6 +2,7 @@ import datetime
import lxml.html import lxml.html
import ujson import ujson
from django.conf import settings
from django.http import HttpResponse from django.http import HttpResponse
from django.utils.timezone import now as timezone_now from django.utils.timezone import now as timezone_now
from mock import MagicMock, patch from mock import MagicMock, patch
@@ -576,33 +577,47 @@ class HomeTest(ZulipTestCase):
del cross_bot['date_joined'] del cross_bot['date_joined']
notification_bot = self.notification_bot() notification_bot = self.notification_bot()
email_gateway_bot = get_system_bot(settings.EMAIL_GATEWAY_BOT)
welcome_bot = get_system_bot(settings.WELCOME_BOT)
by_email = lambda d: d['email'] by_email = lambda d: d['email']
self.assertEqual(sorted(cross_bots, key=by_email), sorted([ self.assertEqual(sorted(cross_bots, key=by_email), sorted([
dict( dict(
user_id=get_system_bot('emailgateway@zulip.com').id,
is_admin=False,
email='emailgateway@zulip.com',
full_name='Email Gateway',
bot_owner_id=None, bot_owner_id=None,
is_bot=True bot_type=1,
email=email_gateway_bot.email,
user_id=email_gateway_bot.id,
full_name=email_gateway_bot.full_name,
is_active=True,
is_bot=True,
is_admin=False,
is_cross_realm_bot=True,
is_guest=False
), ),
dict( dict(
user_id=notification_bot.id, bot_owner_id=None,
is_admin=False, bot_type=1,
email=notification_bot.email, email=notification_bot.email,
full_name='Notification Bot', user_id=notification_bot.id,
bot_owner_id=None, full_name=notification_bot.full_name,
is_bot=True is_active=True,
is_bot=True,
is_admin=False,
is_cross_realm_bot=True,
is_guest=False
), ),
dict( dict(
user_id=get_system_bot('welcome-bot@zulip.com').id,
is_admin=False,
email='welcome-bot@zulip.com',
full_name='Welcome Bot',
bot_owner_id=None, bot_owner_id=None,
is_bot=True bot_type=1,
email=welcome_bot.email,
user_id=welcome_bot.id,
full_name=welcome_bot.full_name,
is_active=True,
is_bot=True,
is_admin=False,
is_cross_realm_bot=True,
is_guest=False
), ),
], key=by_email)) ], key=by_email))